fix: allow bare custom-scheme redirects for public clients - #28041
fix: allow bare custom-scheme redirects for public clients#28041BobbyHo wants to merge 1 commit into
Conversation
…or public clients 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.
|
/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.
| } | ||
| // For confidential clients, custom schemes are less common but allowed | ||
| } | ||
| // Custom schemes need no further check here: validateScheme already |
There was a problem hiding this comment.
Note [CRF-2] The removed check also rejected mailto, tel, and sms for public clients; the new code does not. (Netero)
validateScheme (line 96) blocks only javascript, data, file, ftp. The deleted isValidCustomScheme required a literal . in the scheme (which excluded mailto, tel, sms) and additionally listed those three plus http, https, ftp in a well-known-scheme blocklist. With the check gone, a public client can now register mailto:foo@example.com, tel:..., or sms:... as a redirect URI. The new subtests cover vscode:// and jetbrains:// only; no test asserts what happens for mailto/tel/sms in either direction.
PKCE prevents another party from redeeming the code, so this is not a code-exfiltration vector, but it is a semantic shift from the previous behavior. If the intended scope of the change is "bare app-style custom schemes," consider either keeping mailto/tel/sms on a small blocklist or pinning the current behavior with a subtest that asserts what happens when a public client registers one. If the intent is "any non-dangerous scheme," the current diff matches the intent; add a subtest to lock it in.
🤖
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.Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client