fix: allow bare custom-scheme redirects for public clients - #28041
Conversation
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 3 findings (1 P2, 2 Note), COMMENT. Review Finding inventoryFinding inventory (PR #28041)Findings
Round logRound 1Netero-only first pass. 1 P2, 2 Notes. Reviewed against 912ce41..d95d4f9. Panel deferred: P2 gates panel selection per Netero decision gate. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
BobbyHo
changed the title
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review from Netero only; the full review panel has not yet looked at this PR and will run after the mechanical findings are addressed.
What this PR does well: the change is small, focused, and the removed helper had no other callers. The two new subtests (vscode://coder.authenticate, jetbrains://coder-callback) fail against the base and pass at head, so they exercise the behavior change rather than restating it.
Severity count: 1 P2, 2 Notes.
One blocker before the panel spends time on this: the title CI job is red because the PR title's scope codersdk/oauth2_validation does not cover coderd/oauth2_security_test.go. AGENTS.md requires the scope to be a real filesystem path that contains every changed file, or omitted for cross-cutting changes. See CRF-1 for the mechanical fix.
One behavior question worth answering before the panel picks this up: the removed isValidCustomScheme check also blocked mailto, tel, and sms for public clients; validateScheme does not. The new tests only cover vscode:// and jetbrains://. Intended scope of "allow bare custom schemes": strictly bare app-style schemes like vscode, jetbrains, cursor, or any non-dangerous scheme? See CRF-2.
No Netero verbatim quote worth carrying up: the first-pass output was mechanical.
codersdk/oauth2_validation.go:1
P2 [CRF-1] PR title scope does not contain every changed file, so the title CI job fails. (Netero)
The title is
fix(codersdk/oauth2_validation): allow bare custom-scheme redirects for public clients. The scopecodersdk/oauth2_validationresolves viaisStemagainstcodersdk/oauth2_validation.go, butcoderd/oauth2_security_test.gois undercoderd/, notcodersdk/oauth2_validation/, so.github/workflows/contrib.yaml:212-220reports it as an "outside file" and callscore.setFailed.
AGENTS.md states the scope must be a real filesystem path containing every changed file, and to use a broader path or omit the scope for cross-cutting changes. There is no shared prefix between codersdk/ and coderd/, so the mechanical fix is to drop the scope: fix: allow bare custom-scheme redirects for public clients. The commit title on the branch is currently the same as the PR title and should be rewritten in lockstep.
🤖
codersdk/oauth2_validation.go:85
Note [CRF-3] ValidateRedirectURIScheme's doc comment is duplicated verbatim on lines 78-84 and 85-91. (Netero)
Introduced in commit ed908ed019, not by this PR; godoc renders both copies as one long comment. Out of scope for the diff but sitting immediately next to the touched code, so worth cleaning up in a follow-up (or a one-line ride-along here). Drop lines 85-91.
🤖
🤖 This review was automatically generated with Coder Agents.
isValidCustomScheme required a literal "." in the scheme for a public client's redirect URI, so vscode://, jetbrains://, and cursor:// all 400'd while the identical schemes passed for a confidential client through the separate, more permissive validateScheme. Native and CLI apps, the population public clients exist for, register those exact schemes with their OS. Removed the extra restriction: validateScheme already blocks the schemes that are actually dangerous in a redirect context, and RFC 8252 section 7.1 only recommends reverse-domain notation rather than requiring it. PKCE, not the scheme's spelling, is what secures a public client's redirect. That removal also stopped rejecting mailto, tel, and sms for public clients specifically, since validateScheme's dangerous-scheme blocklist never covered them either. Those three hand off to a mail client, dialer, or SMS app rather than returning control to the client, so unlike vscode:// or jetbrains://, none of them can deliver an authorization code. A public client's redirect URI scheme is its only mechanism for regaining control, so they are rejected again here, scoped specifically to public clients rather than folded into validateScheme's blocklist, since they are harmless for a confidential client's redirect.
d95d4f9 to
9440708
Compare
…/tel/sms scope, not an invented one The previous comment claimed mailto, tel, and sms are harmless for a confidential client's redirect specifically. That is not true: the client_secret only matters at token exchange, not at redirect delivery, so nothing about being confidential changes what happens when the browser is sent to one of these schemes. The actual reason they are checked only in the isPublicClient branch is that custom-scheme validation was already scoped there before this PR; confidential clients were never subject to any scheme-shape check here, independent of any judgment about these three schemes.
Documentation CheckUpdates Needed
All documentation needs for this PR are addressed. Automated review via Coder Agents |
…lients The mailto, tel, and sms scheme rejection for public clients had no documentation, flagged by the doc-check bot on this PR. Note the restriction in the Callback URL schemes section and the Security Considerations list, and add a troubleshooting entry for the error.
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
jdomeracki-coder
left a comment
jdomeracki-coder
left a comment
There was a problem hiding this comment.
Security review of 7e616a3 — no regression found.
- PKCE is enforced end-to-end (
code_challengerequired at authorize; token endpoint rejects codes without a stored challenge, constant-time S256 verify), which is the actual mitigation for custom-scheme hijacking — reverse-domain notation never prevented it. - Exact redirect URI matching (OAuth 2.1) is unchanged, so no prefix/subdomain abuse opens up.
- Old blocklist coverage is fully preserved for public clients: http/https via the loopback branch, ftp via
validateScheme, mailto/tel/sms via the new switch. - No case-bypass:
url.Parselowercases schemes, so the exact-match switch is effectively case-insensitive;Validate()runs on both DCR create and update.
LGTM.
The token endpoint accepted any non-empty `code_verifier`, so a one-character verifier was enough to authenticate. RFC 7636 §4.1 requires 43 to 128 characters from the unreserved set. That fix plus the related gaps review surfaced in the same path: - Enforce the length and charset floor on the verifier before the S256 comparison runs. - Validate the challenge at the authorize endpoint too. It was only checked for non-emptiness, so a malformed challenge was stored and then failed late at token exchange, blaming the wrong parameter. - A malformed verifier now returns `invalid_request` (RFC 6749 §5.2); a well-formed but wrong one still returns `invalid_grant` (RFC 7636 §4.6). Both looked identical before, so a client had no way to tell a syntax error from a hash mismatch and would retry the same bad verifier forever. - Revoke the authorization code when a PKCE check fails. Without that, a leaked code could be replayed with unlimited verifier guesses for its remaining lifetime, and RFC 6749 §10.5 requires codes to be single use. - Fix verifier generation in `scripts/oauth2/*.sh` and the docs example. They deleted reserved base64 characters instead of translating them to the URL-safe alphabet, so most runs produced verifiers under the new floor. Also carries #28041, which merged into this branch: public clients may register bare custom schemes such as `vscode://` again, with `mailto`, `tel`, and `sms` rejected. Split out of #27873 (public OAuth2 client support). PKCE is already mandatory for every client, so this stands on its own. <details> <summary>Manual verification</summary> Ran against a local dev server on this branch, using a session token and a throwaway app from `scripts/oauth2/setup-test-app.sh`. 1. Happy path unchanged: HTTP 200, verifier length 43. 2. `code_verifier=short`, and a 43-character verifier ending in `!`: both HTTP 400 `invalid_request`, so charset is enforced and not just length. 3. `code_challenge=tooshort` at authorize: HTTP 400 `invalid_request`, no code issued. An empty challenge still hits the older "required and cannot be empty" message. 4. Well-formed but wrong verifier: HTTP 400 `invalid_grant`, distinct from the cases above. 5. Retrying that same code with the correct verifier: HTTP 400, code already revoked by the failed check. 6. `generate-pkce.sh` produces a 43-character verifier (20 out of 20 runs); the docs example produces 128. 7. `scripts/oauth2/test-mcp-oauth2.sh` passes end to end. The two bearer-token failures in its output are a pre-existing script bug (`09c50559f3`, July 2025) that reuses a resource-scoped token against the real API, not a regression here. </details>
2 participants
Split out of #27873 to make that PR smaller to review. First in the stack; the rest of the public-client work builds on this.
isValidCustomSchemerequired a literal.in the scheme for a public client's redirect URI, sovscode://,jetbrains://, andcursor://all 400'd while the identical schemes passed for a confidential client through the separate, more permissivevalidateScheme. Native and CLI apps, the population public clients exist for, register those exact schemes with their OS.Removed the extra restriction:
validateSchemealready blocks the schemes that are actually dangerous in a redirect context, and RFC 8252 section 7.1 only recommends reverse-domain notation rather than requiring it. PKCE, not the scheme's spelling, is what secures a public client's redirect.That removal also stopped rejecting
mailto,tel, andsmsfor public clients specifically, sincevalidateScheme's dangerous-scheme blocklist never covered them either. Those three hand off to a mail client, dialer, or SMS app rather than returning control to the application that started the flow, so a public client registered with one of them could never actually complete authorization. They are rejected again here, scoped to public clients only because that is how custom-scheme validation was already scoped before this change, not because they are known to be safe for a confidential client's redirect; confidential clients were never subject to any scheme-shape check beyondvalidateSchemeand remain so here.Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client