fix: allow bare custom-scheme redirects for public clients - #28041
fix: allow bare custom-scheme redirects for public clients#28041BobbyHo wants to merge 2 commits into
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)
|
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
Automated review via Coder Agents |
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