fix(controller): reject query string or fragment in upstreamDefinitions URLs

[mehara-rothila]

Purpose

upstreamDefinitions[].upstreams[].url is intended to be host[:port] only — an upstream definition's base path is configured exclusively via upstreamDefinitions[].basePath (per #2064 and #369 (comment)).
A path in such a URL is already rejected with a 400 (#2065), but a query string (?foo=bar) or fragment (#section) is silently accepted: the upstream cluster is built from host:port only, so the query string and fragment are dropped and never reach the upstream — with no signal to the user that the value was ignored.
Fixes #2068.
Related #2064.

Goals

Reject a query string or fragment in an upstreamDefinitions URL with a 400 at deploy time, consistent with how an embedded path is already handled — giving the user a clear, unambiguous contract that these URLs are host[:port] only.

Approach

• pkg/config/api_validator.go: in validateUpstreamDefinitions, immediately after the existing feat(controller)!: take upstreamDefinitions base path from basePath #2065 path check, reject a URL whose RawQuery is non-empty or that carries a bare ? (ForceQuery) with "URL must not include a query string", and a URL with a non-empty Fragment with "URL must not include a fragment".
• The validator is the single deploy-time gate for upstreamDefinitions URLs; the downstream consumers (pkg/transform/restapi.go, pkg/xds/translator.go) run after validation and read only host/scheme, so no change is needed there.
• Direct upstream.main/upstream.sandbox URLs are unchanged — they legitimately carry a base path in the URL path and are validated separately (validateUpstreamUrl); query/fragment handling for those is outside the scope of this issue.

User stories

As an API developer, when I mistakenly put a query string or fragment in an upstreamDefinitions URL, I get a clear 400 at deploy time instead of having the value silently dropped at runtime.

Documentation

N/A — no new configuration is introduced; this tightens validation of an existing field to match the documented host[:port]-only contract.

Automation tests

• Unit tests

  Added TestValidateUpstreamDefinitions_URLWithQueryRejected and TestValidateUpstreamDefinitions_URLWithFragmentRejected, mirroring the existing TestValidateUpstreamDefinitions_URLWithPathRejected. go test ./pkg/config/ passes with no regression.

• Integration tests

  N/A — this is deploy-time input validation with no runtime behavior change.

Security checks

• Followed secure coding standards in http://wso2.com/technical-reports/wso2-secure-engineering-guidelines? yes
• Ran FindSecurityBugs plugin and verified report? N/A (Go code)
• Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets? yes

Samples

N/A

Related PRs

#2065 (take upstreamDefinitions base path from basePath) introduced the path rejection this builds on; #2059 (dynamic-endpoint sandbox routing) is adjacent work on the same area.

Test environment

Go (multi-module workspace); go test ./pkg/config/.

[CLAassistant]

All committers have signed the CLA.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bb6e9325-3068-4bea-9492-cd8a8064be37

📥 Commits

Reviewing files that changed from the base of the PR and between 227e19b and d5bc0b5.

📒 Files selected for processing (1)

• gateway/gateway-controller/pkg/config/api_validator.go

✅ Files skipped from review due to trivial changes (1)

• gateway/gateway-controller/pkg/config/api_validator.go

---

📝 Walkthrough

Changes

URL Validation Enhancement for Upstream Definitions

File: gateway/gateway-controller/pkg/config/api_validator.go

• Extend validateUpstreamDefinitions to reject upstream URLs (spec.upstreamDefinitions[].upstreams[].url) that include a query string (including a bare ?) or a fragment (#...).
• Errors produced: "URL must not include a query string" and "URL must not include a fragment".
• This complements existing rejection of URLs containing a path and ensures basePath remains the sole place for path components.

File: gateway/gateway-controller/pkg/config/validator_test.go

• Added unit tests:
  • TestValidateUpstreamDefinitions_URLWithQueryRejected
  • TestValidateUpstreamDefinitions_URLWithBareQueryRejected
  • TestValidateUpstreamDefinitions_URLWithFragmentRejected
• Each test asserts a single validation error targeted at spec.upstreamDefinitions[0].upstreams[0].url with the appropriate message.

Impact

Deploy-time validation now returns a clear 400 when upstreamDefinitions URLs include query strings or fragments, avoiding silent dropping of those components at runtime. Downstream consumers and direct upstream.main/upstream.sandbox URL validation are unchanged.

Walkthrough

This pull request updates validateUpstreamDefinitions to reject upstream target URLs that include query strings (including bare '?'/ForceQuery) or fragments. The validator parses each upstream URL and returns field-specific ValidationError entries when a query string or fragment is detected. Three unit tests were added to assert rejection for URLs with a query string, a bare query marker, and a fragment, each targeting spec.upstreamDefinitions[0].upstreams[0].url and checking the error message content.

Suggested reviewers

• RakhithaRR
• Tharsanan1
• VirajSalaka

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: rejecting query strings or fragments in upstreamDefinitions URLs, matching the primary objective of the pull request.
Description check	✅ Passed	The description is comprehensive, covering purpose, goals, approach, user stories, documentation impact, unit tests added, and security checks. All major sections of the template are addressed.
Linked Issues check	✅ Passed	The PR directly addresses #2068 by adding validation to reject query strings and fragments in upstreamDefinitions URLs at deploy time, consistent with existing path rejection.
Out of Scope Changes check	✅ Passed	All changes are scoped to the validation logic for upstreamDefinitions URLs. No unrelated modifications to downstream consumers or direct upstream URLs are included.
Docstring Coverage	✅ Passed	Docstring coverage is 80.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies"

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[mehara-rothila]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

gateway/gateway-controller/pkg/config/validator_test.go (1)

623-643: ⚡ Quick win

Add coverage for the trailing ? case.

Line 624 covers a non-empty query, but the validator also rejects parsedURL.ForceQuery for URLs like http://backend-1:8080?. Please add that case so the full new branch stays protected.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@gateway/gateway-controller/pkg/config/validator_test.go` around lines 623 -
643, TestValidateUpstreamDefinitions_URLWithQueryRejected only covers URLs with
a non-empty query; add a second assertion case that passes an upstream URL
ending with a trailing '?' (e.g., "http://backend-1:8080?") to exercise the
parsedURL.ForceQuery branch. Update the test (within
TestValidateUpstreamDefinitions_URLWithQueryRejected or a small companion test)
to call validator.validateUpstreamDefinitions with that URL and assert the
returned error references "spec.upstreamDefinitions[0].upstreams[0].url" and
contains "query string" (or the same message used for ForceQuery), ensuring the
validator.validateUpstreamDefinitions/parsedURL.ForceQuery behavior is covered.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@gateway/gateway-controller/pkg/config/validator_test.go`:
- Around line 623-643: TestValidateUpstreamDefinitions_URLWithQueryRejected only
covers URLs with a non-empty query; add a second assertion case that passes an
upstream URL ending with a trailing '?' (e.g., "http://backend-1:8080?") to
exercise the parsedURL.ForceQuery branch. Update the test (within
TestValidateUpstreamDefinitions_URLWithQueryRejected or a small companion test)
to call validator.validateUpstreamDefinitions with that URL and assert the
returned error references "spec.upstreamDefinitions[0].upstreams[0].url" and
contains "query string" (or the same message used for ForceQuery), ensuring the
validator.validateUpstreamDefinitions/parsedURL.ForceQuery behavior is covered.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f311aa0a-4685-4e02-aafa-3e039c750a17

📥 Commits

Reviewing files that changed from the base of the PR and between 08d3f58 and 5d4499b.

📒 Files selected for processing (2)

• gateway/gateway-controller/pkg/config/api_validator.go
• gateway/gateway-controller/pkg/config/validator_test.go

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds stricter validation for upstreamDefinitions[].upstreams[].url to reject query strings and fragments (which would otherwise be silently ignored), along with new unit tests covering these cases.

Changes:

• Reject upstream URLs that include a query string (including a bare ?).
• Reject upstream URLs that include a fragment (#...).
• Add unit tests for query and fragment rejection.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
gateway/gateway-controller/pkg/config/validator_test.go	Adds tests ensuring upstream URLs with query/fragment are rejected.
gateway/gateway-controller/pkg/config/api_validator.go	Adds validation that flags query strings and fragments as invalid in upstream URLs.