[aterga]

Summary

Addresses security finding MCP05: "Redirect URL with query parameters can be injected" (rated Low). The redirect-URI allow-list validated scheme/host/path/port but accepted a query or fragment component. /oauth/authorize appends ?code=…&state=… to the redirect (build_redirect), so a redirect_uri that already carried a query produced code=123&code=456 parameter pollution that a lax downstream parser (the AI provider / client) could resolve to an attacker-seeded value.

Not exploitable today — PKCE plus the exact-registered-redirect match (an attacker can't overwrite an existing client's redirect_uri) block it — but, per the finding's recommendation, disallowing query and fragment removes the ambiguity as defense in depth against a future weakening (e.g. PKCE removed, or looser redirect matching) or a permissive client-side parser.

Related issues

Security review finding MCP05. Follows the hosted-redirect allow-list work (#69, #73). No GitHub issue open.

Changes

• redirect_uri_permitted: parse the URI first, then reject any URI carrying a query or fragment — on loopback and hosted alike — ahead of the existing checks. This function gates both /oauth/register (per-URI validation) and /oauth/authorize (via redirect_allowed), so the rule holds at registration and at code issuance.
• is_wellformed_hosted_redirect: also exclude query/fragment, so a query-bearing hosted redirect is classified invalid_request (a client-side request error) rather than routed to the "request access" page (which is for genuine approval gaps).
• Tests: hosted_redirect_allow_list now asserts ?code=123, ?x=1, and #frag are refused on both a hosted vendor callback and a loopback URI, and that the same URI without a query still passes.
• README.md: the hosted-redirect allow-list note documents the query/fragment rule.

Notes

• Nothing legitimate is rejected: every seeded vendor callback path and every registered loopback URI (per oauth-clients.json) is already query/fragment-free. The OAuth authorization server is the sole party that adds query params to the redirect.
• The finding also says "consider not allowing fragments" — done (fragments are rejected too).

Testing

• cargo build --locked --all-targets (via the test build)
• cargo test --locked --all-targets — 141 lib + 7 router + 1 doctest passing
• cargo clippy --all-targets — no new warnings in auth.rs
• npm test --prefix monitoring/mcp-status — not applicable (no dashboard change; the DCR probes register query-free redirects, unaffected)

Checklist

• I have read the Contributing guidelines.
• Docs (README / comments) updated for the change.
• No secrets, credentials, or internal-only information are included.

---

Generated by Claude Code

[Copilot]

Pull request overview

This PR hardens OAuth redirect-URI validation to address security finding MCP05 by rejecting redirect_uris that include a query string or fragment, preventing ambiguity/parameter pollution when the authorization endpoint appends ?code=…&state=….

Changes:

• Update redirect_uri_permitted to parse first and reject any query/fragment on both hosted and loopback redirects.
• Update is_wellformed_hosted_redirect to treat query/fragment redirects as malformed/ineligible (so they surface as invalid_request, not “request access”).
• Extend allow-list tests and document the new rule in README.md.

Reviewed changes

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

File	Description
src/auth.rs	Enforces query/fragment rejection in redirect validation, adjusts hosted-redirect wellformedness classification, and adds test coverage for the new restriction.
README.md	Documents the “no query/fragment” redirect rule as part of the hosted-redirect allow-list description.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/auth.rs:375

• is_wellformed_hosted_redirect is used to decide whether to show the “request access” allow-list page vs classify the request as invalid_request. Right now it treats redirects with an explicit port (e.g. https://host:444/...) or dot-segment traversal in the path as “well-formed hosted”, even though redirect_uri_permitted rejects both as ineligible. That misclassifies these cases as an approval gap (403 / invalid_client) instead of a client error (400 / invalid_request).

                // A query or fragment makes it ineligible (MCP05), not merely
                // unapproved, so it is classified `invalid_request` rather than
                // routed to the "request access" page.
                && url.query().is_none()
                && url.fragment().is_none()

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

src/auth.rs:375

• is_wellformed_hosted_redirect is used to decide whether to show the "request access" allow-list page vs returning invalid_request, but it currently treats some redirects that redirect_uri_permitted will always reject (e.g., explicit non-default ports like :444 and dot-segment paths) as “well-formed hosted”. That can misclassify an ineligible redirect as an approval gap and show a misleading page / wrong error taxonomy. Consider aligning the “well-formed hosted” predicate with the hosted-shape constraints by rejecting non-default ports and ./.. segments here too.

            url.scheme() == "https"
                && url.username().is_empty()
                && url.password().is_none()
                && url.host_str().is_some_and(|h| !h.is_empty())
                // A query or fragment makes it ineligible (MCP05), not merely

src/auth.rs:345

• redirect_uri_permitted currently reparses a newly formatted URL just to enforce the allowed hosted “shape” (https, no userinfo, no explicit non-default port). This adds an extra allocation + parse on the /oauth/register and /oauth/authorize path for all hosted redirects, even though the same constraints can be checked directly on the already-parsed url.

    // Allowlist the redirect's SHAPE rather than denylisting each disallowed part: a
    // permitted hosted redirect serialises to exactly `https://<host><path>` (https,
    // no userinfo, no port, no query, no fragment). Rebuild that canonical form from
    // the parts we allow and require an exact match, so any unexpected component is
    // refused in one comparison. url::Url drops the https-default `:443` (so it still

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (2)

src/auth.rs:1747

• is_loopback_redirect/is_loopback_url will currently return true for loopback URLs that include a query or fragment, even though redirect_uri_permitted now rejects query/fragment for all redirects. Tightening the loopback helper to also require query/fragment to be absent keeps these helpers consistent with the new invariant and reduces the chance of future misuse in other call sites/tests.

    url.scheme() == "http"
        && url.username().is_empty()
        && url.password().is_none()
        && matches!(url.host_str(), Some("localhost" | "127.0.0.1" | "[::1]"))
}

src/auth.rs:325

• This canonical-shape check reparses a freshly formatted URL (Url::parse(...)) for every hosted redirect, adding a second URL parse on the /oauth/register and /oauth/authorize path. You can enforce the same “exactly https://<host><path>” constraint more cheaply by comparing against url.as_str() instead of reparsing.

    let Ok(canonical) = url::Url::parse(&format!("https://{host}{}", url.path())) else {
        return false;
    };
    if url != canonical {
        return false;