[mashkovd]

What

validateClient decides whether a redirect target is acceptable from the host url.Parse reports. That is not always the host a browser will dial, and two shapes exploit the gap:

https://evil.com@claude.ai/cb    parses as claude.ai  → passes the implicit-host allowlist
http://evil.com@localhost/cb     parses as localhost  → passes as loopback

Anything that splits on the first @ — or simply renders the URL to a user — reads evil.com. A backslash is worse still: not a valid URI character, and parsers disagree on whether it terminates the authority, so it is rejected before parsing rather than after.

Also makes the loopback host comparison case-insensitive (RFC 3986 §3.2.2); url.Parse preserves case, so a client emitting LOCALHOST was turned away.

This is live, not theoretical

OAUTH_ALLOW_IMPLICIT_CLIENT defaults to true (internal/config/config.go:205) and is unset on the deployed pod — I checked labs-mctl-telegram-base-service directly. So the implicit-client branch is active in production, and its host allowlist is the only thing between an unregistered caller and an arbitrary redirect target.

The code binds the authorization code to redirect_uri at /oauth/token and requires PKCE, so a hijacked code is not directly redeemable by a passive attacker — but an attacker who starts the flow picks the challenge themselves, and the comment above validateClient already recognises that briefly shipping a code to an attacker-controlled host is the thing worth preventing.

How this came up

Auditing this server against three defects just fixed in mctl-api (#156/#157). None of them are present here — this OAuth implementation is markedly more careful:

• writeAuthorizeError writes JSON 400 and never sets Location, so the open redirect that mctl-api had cannot arise
• validateClient has always taken clientID and scoped registered URIs to their own client — the cross-client hole mctl-api had
• registrations live in Postgres and fail closed on a DB error, so they survive restarts — the very thing mctl-api#159 tracks
• PKCE is mandatory with S256, constant-time compared, length-bounded per RFC 7636

Only the parser-disagreement hardening was missing, which is what this PR adds.

Testing

go vet and go test ./... clean. New tests pin nine rejection shapes (userinfo in both allowlist and loopback form, raw and encoded backslash, suffix tricks, http on a non-loopback host) plus the legitimate targets that must keep working, and assert registered-client scoping is untouched. Verified they fail without the change:

--- FAIL: TestValidateClientRejectsHostSpoofingShapes/userinfo_hides_the_real_host_behind_an_allowlisted_one
    validateClient accepted "https://evil.com@claude.ai/cb", want rejection

Left alone deliberately

The AllowedImplicitHosts comparison is still case-sensitive, so https://CLAUDE.AI/cb is refused. Same RFC 3986 point as the loopback fix, but changing it widens what the allowlist accepts, so it wants its own decision rather than riding along in a hardening PR.

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[claude]

Claude finished @mashkovd's task in 2m 43s —— View job

---

Review complete — changes requested

• Gather context (PR description, changed files)
• git diff 82c52f2b9c65f79623edbf83f407e6b3224445cf...d97ab50698d9031359fd52b048462ad5c650dbbd
• Review internal/oauth/server.go changes
• Review internal/oauth/redirect_hardening_test.go
• Post inline finding
• Submit verdict (REQUEST_CHANGES)

Note: go build/go test could not be run in this sandbox (commands required interactive approval that wasn't available), so correctness was verified by static reading of internal/oauth/server.go rather than execution. The stated go vet/go test ./... results from the PR description were not independently re-verified.

Finding (1 P1)

The hardening (backslash rejection + userinfo rejection) is added only to validateClient's implicit-client fallback branch (internal/oauth/server.go:2024-2041). Its sibling, validateImplicitRedirectURI (internal/oauth/server.go:1926-1959) — which gates POST /oauth/register and is documented as applying "the same policy as the implicit-client branch of validateClient" — was not given the same checks.

Because validateClient does an exact-string match against already-registered redirect_uris and skips the host/userinfo/backslash checks entirely for registered clients, an attacker can:

1. POST /oauth/register with redirect_uris: ["https://evil.com@claude.ai/cb"] — this still succeeds, since validateImplicitRedirectURI sees Hostname() == "claude.ai" (allowlisted) and never inspects u.User.
2. Call /oauth/authorize with the returned client_id and the identical redirect_uri — it exact-matches and returns immediately, bypassing the new checks.

So the exact host-spoofing shape this PR sets out to close is still reachable one hop later, via self-registration. Suggest factoring the backslash/userinfo checks into a shared helper used by both functions, and adding register-path test coverage for the same nine rejection shapes.

Full details and a fix link are in the inline comment.

Everything else in the diff looks solid: the isLoopbackHost case-insensitivity fix is correct and well-scoped, the backslash/userinfo checks themselves are placed correctly (backslash rejected pre-parse, userinfo checked post-parse before the scheme/host logic), and the new tests are clear and well-targeted for the code path they cover.

Verdict: REQUEST_CHANGES (1 P1, 0 P2, 0 P3).

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Severity: P1
File: internal/oauth/server.go, ~line 1924 (in validateImplicitRedirectURI)
Concrete failure scenario: The PR adds critical hardening checks (strings.ContainsRune(redirectURI, '\\') and u.User != nil) to the implicit branch of validateClient to prevent open redirects via host spoofing. However, these checks were omitted from validateImplicitRedirectURI, which is responsible for enforcing the redirect URI policy during dynamic client registration. An attacker can call the /oauth/register endpoint to register a malicious URI (e.g., https://attacker.com@claude.ai/cb or http://evil.com@localhost/cb). Later, when the attacker initiates an OAuth flow using this registered client, validateClient looks up the client in the database, finds an exact string match for the malicious URI, and returns nil (success) before reaching the new hardening checks at the bottom of the function. This fully bypasses the security patch and allows the authorization code to leak to an attacker-controlled domain.
Proposed fix: Extract the initial URI string validation logic (including the backslash and userinfo checks) into a shared helper function. Call this function in both validateImplicitRedirectURI and validateClient so the validation policy remains strictly synchronized across registration and authorization.

Severity: P2
File: internal/oauth/redirect_hardening_test.go
Concrete failure scenario: The new test suite comprehensively covers host spoofing shapes, but it exclusively tests validateClient for unregistered clients (which fall through to the implicit branch). It entirely lacks tests for validateImplicitRedirectURI or the client registration flow. This gap in test coverage is exactly what allowed the missing security checks in validateImplicitRedirectURI (the P1 vulnerability) to go undetected.
Proposed fix: Add a test case that directly exercises validateImplicitRedirectURI (or simulates the /oauth/register flow) using the identical rejected test cases from TestValidateClientRejectsHostSpoofingShapes, ensuring that malicious URI shapes are correctly rejected at registration time.

Severity: P2
File: internal/oauth/server.go, ~lines 1941 and 1991 (in AllowedImplicitHosts validation loops)
Concrete failure scenario: The PR correctly identifies that url.Parse preserves case and introduces case-insensitive matching for loopback hosts by adding strings.ToLower(h) in isLoopbackHost. However, it fails to apply this same case-insensitivity when matching u.Hostname() and u.Host against AllowedImplicitHosts. If a legitimate client submits an authorized redirect URI with an uppercase host (e.g., https://CLAUDE.AI/api/mcp/auth_callback), u.Hostname() will retain the uppercase string and fail the exact string match against the lowercase allowlist entries. This results in a false-positive rejection of a valid client, breaking their OAuth flow.
Proposed fix: Apply strings.ToLower() to both u.Hostname() and u.Host (and ensure allowlist entries are lowercased) prior to performing string comparisons in both validateImplicitRedirectURI and validateClient. Also, add an uppercase host test case like "https://CLAUDE.AI/api/mcp/auth_callback" to TestValidateClientAcceptsLegitimateImplicitRedirects.

[claude]

Has P1/P2 findings, changes requested: 1 P1. The new backslash/userinfo checks are added only to validateClient's implicit-client fallback branch, but the sibling function validateImplicitRedirectURI — used to gate POST /oauth/register and documented as applying "the same policy" — was not updated. Since validateClient exact-matches already-registered redirect_uris without re-running the host/userinfo/backslash checks, an attacker can self-register the same userinfo-spoofed URI (e.g. https://evil.com@claude.ai/cb) that this PR set out to block, and it will be accepted verbatim at /oauth/register, then sail through /oauth/authorize via the exact-match path. See inline comment for details and a suggested fix (shared helper + test coverage for the register path).

[claude]

Claude finished @mashkovd's task in 1m 23s —— View job

---

Review complete — approved

• Gather context (PR description, prior review comments)
• git diff 82c52f2b9c65f79623edbf83f407e6b3224445cf...d2f707c958b3e36580d7c8e02157079206776118
• Review internal/oauth/server.go changes (including the new registration-time fix)
• Review internal/oauth/redirect_hardening_test.go
• Post inline findings (none — see below)
• Submit verdict (APPROVE)

Note: As with the previous review pass, go build/go vet/go test require interactive approval that isn't available in this sandbox, so correctness was verified by static reading rather than execution.

What changed since the last review pass

The latest commit (d2f707c) directly addresses the P1 raised by both the prior Claude review and the Antigravity review: validateImplicitRedirectURI (gating POST /oauth/register, internal/oauth/server.go:1926) now carries the backslash-rejection and userinfo-rejection checks itself, and validateClient's implicit-client fallback (internal/oauth/server.go:2004-2046) was rewritten to simply call s.validateImplicitRedirectURI(redirectURI) instead of maintaining a second copy of the host/scheme logic. One policy, one implementation — the two entry points can no longer drift.

Concretely, this closes the exact bypass previously demonstrated: POST /oauth/register with redirect_uris: ["https://evil.com@claude.ai/cb"] is now rejected at registration time (userinfo check fires), so the exact-match path in validateClient never gets a chance to wave a spoofed URI through.

Test coverage matches: TestValidateImplicitRedirectURIRejectsTheSameShapes in internal/oauth/redirect_hardening_test.go exercises validateImplicitRedirectURI directly against the same six host-spoofing shapes (userinfo, credentials form, raw/encoded backslash, plain remote host) and confirms legitimate URIs (including case-insensitive loopback) still pass — closing the coverage gap the Antigravity review flagged.

On the P2 case-sensitivity note from the Antigravity review

AllowedImplicitHosts matching (internal/oauth/server.go:1952-1966) is still case-sensitive by design — the PR description explicitly calls this out as deliberate ("Left alone deliberately"), since loosening it widens what the allowlist accepts and deserves its own decision rather than riding along in a hardening PR. Not a regression, no action needed here.

Verdict

No P1/P2 findings. Approved.
· branch fix/oauth-implicit-redirect-hardening

[claude]

No P1/P2 findings (0 P3). The P1 flagged in earlier reviews (validateImplicitRedirectURI, which gates POST /oauth/register, missing the backslash/userinfo checks and therefore bypassable via self-registration) is resolved: validateClient's implicit-client fallback now delegates directly to validateImplicitRedirectURI instead of carrying a duplicate copy of the checks, so both entry points enforce identical policy. New test TestValidateImplicitRedirectURIRejectsTheSameShapes exercises the registration-path function directly with the same nine rejection shapes. Good to merge.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Severity: P1
File: internal/oauth/server.go, approximate line 1880 (inside handleClientRegistration)

Concrete failure scenario:
The author extracted the redirect URI validation logic into the new validateImplicitRedirectURI helper to secure both the implicit-client flow and dynamic client registrations (POST /oauth/register). The comments explicitly acknowledge that unhardened client registrations leave a bypass open: an attacker can register a spoofed URI, which validateClient's exact-match path then blindly accepts. However, while the author updated validateClient to use the new helper, they completely forgot to add the validation call to handleClientRegistration. Because the new test TestValidateImplicitRedirectURIRejectsTheSameShapes only unit-tests the factored-out helper function rather than executing a request against the actual registration endpoint, this critical omission went unnoticed.

An attacker can make a POST request to /oauth/register with a malicious redirect URI such as https://evil.com@claude.ai/cb. Because handleClientRegistration still does not validate incoming URIs, the server saves it. The attacker then initiates the OAuth flow (/oauth/authorize) using their newly registered client_id and the malicious redirect_uri. In validateClient, the code finds the registered client, sees an exact match in the stored RedirectURIs, and immediately returns nil—bypassing the host allowlist entirely and achieving an open redirect that leaks the authorization code.

Proposed fix:
In handleClientRegistration, iterate over req.RedirectURIs and validate each one by calling s.validateImplicitRedirectURI(uri) before proceeding to insert the client registration into the database or memory map. If any URI fails validation, immediately reject the registration request with an http.StatusBadRequest.

[github-actions]

Skipping — this commit (41a7ef5) already has a Claude review verdict. Push a new commit, or check the run log if you expected a fresh pass and the previous run actually failed.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.1-pro-high.

Here is a review of the provided diff.

Finding 1: Validation bypassed because it is never called during registration

• Severity: P1
• File: internal/oauth/server.go (in handleClientRegistration, near line 1877)
• Concrete failure scenario: The PR author intended to prevent an open-redirect bypass by unifying the URI validation into a new validateImplicitRedirectURI helper. The comments explicitly state this was "Factored out so /oauth/register can enforce it at registration time". However, the helper is never actually called inside the handleClientRegistration endpoint within the diff, nor are any old, vulnerable validation checks replaced or removed. An attacker can still use the /oauth/register endpoint to register a spoofed URI (like https://evil.com@claude.ai/cb). Because validateClient applies an exact-match short-circuit for registered clients, the authorization endpoint will blindly accept the malicious URI and leak the authorization code to the attacker's domain.
• Proposed fix: Inside handleClientRegistration, before inserting the client into s.clients, iterate over req.RedirectURIs and validate each one with s.validateImplicitRedirectURI(uri). If any URI returns an error, reject the registration request with an http.StatusBadRequest.

Finding 2: Missing integration test coverage hides the registration bypass

• Severity: P2
• File: internal/oauth/redirect_hardening_test.go
• Concrete failure scenario: The new test TestValidateImplicitRedirectURIRejectsTheSameShapes merely unit-tests the internal validateImplicitRedirectURI helper function directly. It never makes an HTTP call to the /oauth/register endpoint. Because the test bypasses the API boundary, it gave a false sense of security and completely failed to detect that the HTTP handler was never actually wired up to use the new validation logic.
• Proposed fix: Add an integration-level test that creates a test server, sends an actual HTTP POST registration payload containing a spoofed redirect URI (e.g., https://evil.com@claude.ai/cb), and asserts that the HTTP handler rejects the request.