feat: validate and persist OAuth2 authorization scope - #28045
Conversation
/oauth2/authorize ignored the scope parameter entirely and wrote a hardcoded coder:all onto every authorization code. It now negotiates: each requested scope must be in the external scope catalog (rbac.IsExternalScope), and the result must fall within the app's configured allowlist, which is itself filtered through the same catalog. An omitted scope defaults to the filtered allowlist per RFC 6749 section 3.3. The negotiated value is persisted on oauth2_provider_app_codes.scope, replacing the placeholder written when the column was added. Both GET and POST validate, so a request that cannot succeed is rejected before the consent page renders rather than after the user clicks Allow. This matches how the handler already treats PKCE's code_challenge requirement. Two cases produce an empty result and are handled deliberately differently. An app with no allowlist and no requested scope keeps today's unrestricted grant, spelled as the explicit coder:all sentinel because the column is NOT NULL with a non-empty CHECK. An app whose allowlist filters to nothing is rejected instead, since falling back there would grant strictly more than the allowlist ever permitted. NULL and the empty string are one "no allowlist configured" state, unified in a single predicate now that reading the column is an authorization decision. Accepted compatibility break: dynamic client registration performs no catalog validation, so apps registered with scopes such as openid or admin hold allowlists this server cannot grant from. Those apps now fail authorization in both directions with invalid_scope. Grandfathering unknown names through would seed api_keys.scopes with values dbauthz cannot evaluate, trading a visible negotiation-time error for a silent enforcement-time hole. Registration-time expectations are unchanged; the two tests asserting registration accepts these values carried comments promising the opposite of what authorization does, and those were corrected. Issued tokens are not yet restricted: authorizationCodeGrant still mints rbac.ScopeAll and does not read the persisted column. That lands with the grant path. Refs PLAT-479
…lat-470' into coder-oauth2-scope-enforcement-plat-470-phrase-2
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
deep-review v0.9.0 | Round 1 | Last posted: Round 1, 22 findings (5 P2, 6 P3, 5 Nit, 6 Note), REQUEST_CHANGES. Review Finding inventoryFindings
Round logRound 1Panel of 21 (Netero, Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Komugi, Kurapika, Razor, Ging-go, Ryosuke, Chopper, Kite, Gon, Leorio, Robin, Zoro, Luffy, Meruem, Knov, Melody, Killua). 5 P2, 6 P3, 5 Nit, 6 Note posted; 2 Netero context notes retained in inventory but not posted. Reviewed against About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Round 1. 5 P2, 6 P3, 5 Nit, 6 Note.
What lands well: validateRequestedScope's four branches are exhaustive and each one is pinned to at least one test row; the NOT NULL + CHECK (scope <> '') invariant is asserted literally (assert.NotEmpty(t, got)) rather than through indirection; and AllowlistFilteringToEmptyRejected is the load-bearing decision that keeps a filtered-to-empty allowlist from being punned as "no allowlist" through the unrestricted fallback. Netero's empirical revert-and-test showed six of the eight scope-negotiation sub-tests fail on base, so the suite genuinely validates the change. Kite: "the single load-bearing decision that keeps this from becoming a privilege-escalation vector; the tests pin it explicitly."
Blockers this round:
- Bare aliases
allandapplication_connectreachoauth2_provider_app_codes.scopeverbatim.rbac.IsExternalScopeaccepts the backward-compat forms; Phase 3's typedapi_keys.scopes(api_key_scope[]) will reject the enum parse, andExpandScope("all")already returns "no scope named". Normalize at the negotiation boundary. - Rejection tests assert only
require.Error. The three separately-worded errors (unknown-scope, no-grantable, not-in-allowlist) collapse to one column of "some error happened"; a refactor that routed one branch through another would still pass the whole table. - Plan-doc labels (
AC1..AC16,Edge Case 19/20/22,§4.2.2) in both new test files reference identifiers the repo does not carry. Each row already restates its content in plain English, so the labels contribute nothing and rot on the first plan renumbering. - The consent page never renders the negotiated scope.
ShowAuthorizePagecomputes it, discards it into_, and handsRenderOAuthAllowDataa struct with noScopefield. Pre-PR this was inert because every code wascoder:all; this PR is what changes the precondition.
Deferrable but worth naming:
- Every
WriteOAuth2ErrorandRenderStaticErrorPagesite in this file diverges from RFC 6749 §4.1.2.1, which requires a redirect toredirect_uriwitherror=invalid_scope&state=...onceclient_id/redirect_uriare resolved. This PR extends the class rather than creating it; if the intent is to keep both surfaces, name it, otherwise track the class fix so clientstatecorrelation stops being dropped. - The static "Invalid Scope" page description attributes the failure to the requester in the branch where the app's registered allowlist, not the request, is what cannot be satisfied.
- DCR registration writes any scope string verbatim; the read-side catalog filter and the empty-filter rejection both exist because of that. A registration-time catalog check collapses both.
- Swagger
@Param scopeon both authorize handlers still reads "Token scopes (currently ignored)"; that annotation regenerates into the shipped API docs.
Process notes: the preceding commit (fix(coderd): set scope on oauth2 test inserts) shows the sibling audit was already applied at the InsertOAuth2ProviderApp{Code,Token}Params call sites. Every claim in the PR description traces. Netero's revert-and-test also observed that NoAllowlistStaysUnrestricted and NullAndEmptyAllowlistBehaveIdentically PASS on base, i.e. they act as regression guards for the intended non-change rather than as validators of this PR's behavior; worth knowing when reading the suite as evidence of AC3/AC16.
Fun quote from Hisoka: "I came looking for a fight. I got a clean opponent instead. ♥"
coderd/oauth2provider/authorize.go:284
P2 [CRF-5] The consent page renders without displaying the negotiated scope; the user clicks Allow against a template that hardcodes "full access" regardless of what the app requested. (Knov P2, Mafuuu P3, Kurapika P4, Meruem Note, Pariston Note)
Knov:
ShowAuthorizePagecomputes_, err := validateRequestedScope(params.scope, app.Scope)for the pre-consent rejection, then throws the successful return value away. Ten lines later,RenderOAuthAllowPageis handedRenderOAuthAllowData(defined at+site/site.go:794), a struct that has no scope field, and the template at+site/static/oauth2allow.html:117renders a fixed description readingAllow {{ .AppName }} to have full access to your {{ .Username }} account?.
Mafuuu:
This PR persists the negotiated scope onto
oauth2_provider_app_codes.scope, and the same value flows tooauth2_provider_app_tokens.scopeattokens.go:378. Later PRs (per the description: "Applying the negotiated scope inauthorizationCodeGrant, refresh narrowing") make that persisted value the effective ceiling on the token. From the user's side, the Allow button starts meaning "grant this specific subset," but they still see nothing that names the subset.
Pre-PR this was inert: every consent produced database.OAuth2ScopeUnrestricted regardless, so the "full access" wording was accurate. This PR changes that precondition. Under "assume no follow-up" the row is dead data and the wording still cannot describe it; under the described follow-up the consent shown and the consent recorded diverge, and no round in the flow shows the user the delta. The fix is small and belongs with the persistence work: return grantedScope from the GET-side call and thread it into a new field on RenderOAuthAllowData. TestOAuthConsentFormIncludesCSRFToken gains a sibling that pins the scope's presence.
🤖
coderd/oauth2provider/registration.go:111
P3 [CRF-9] DCR registration stores any scope string verbatim, so an app registered with openid succeeds at registration and then fails every authorize with invalid_scope. Fix at the write boundary collapses both read-side patches this PR adds. (Ryosuke P3, Pariston P3)
Ryosuke:
The catalog check runs only on the read side.
req.ScopereachesInsertOAuth2ProviderAppParams.Scopewith no filter, so the DB accepts allowlists that authorization can never satisfy: request the registered name and hit the subset check, omitscopeand hitfiltered == 0. The affected client cannot self-heal; the remedy is re-registration.
Pariston reframed the same finding as a design origin question: the read-side noScopeAllowlist predicate exists because DCR writes an unconditional sql.NullString{String: req.Scope, Valid: true} (turning oauth2_provider_apps.scope into a de-facto tri-state), and the read-side catalog filter exists because DCR is catalog-unaware. A one-line canonicalization plus a catalog check at registration.go:111 (Valid: req.Scope != "" collapses the state; looping rbac.IsExternalScope over strings.Fields(req.Scope) rejects non-catalog names with RFC 7591 §3.2.1 invalid_client_metadata) makes noScopeAllowlist collapse to !appScope.Valid, turns the read-side filter into defense-in-depth the runtime never has to trigger, and bounds the DCR compat break to already-stored rows rather than every new bad registration going forward.
The PR description addresses one alternative (grandfathering unknown names) and correctly rejects it; that does not rebut the write-side rejection alternative. Registration-time invalid_client_metadata also keeps non-catalog names out of the enforcement path and additionally surfaces the failure at the earliest possible moment, before a client_id is issued and users start clicking Allow.
🤖
coderd/oauth2.go:123
P3 [CRF-10] Swagger @Param scope on both /oauth2/authorize handlers still reads "Token scopes (currently ignored)", which is now the opposite of what the endpoint does. (Ryosuke)
Line 123 (GET) and line 138 (POST) both carry the stale annotation. Callers reading coderd/apidoc/swagger.json or docs/reference/api/enterprise.md will construct requests assuming the parameter is ignored and be rejected with invalid_scope at runtime. This is a shipped contract detail, not just a comment, since the annotation is regenerated into the API docs.
🤖
coderd/oauth2provider/apps.go:102
Note [CRF-19] Admin-created OAuth2 apps hardcode Scope: sql.NullString{} on create and preserve it on update; the scope allowlist feature is reachable only through DCR. (Mafuuu Note, Melody Note, Ryosuke Note)
Producers of oauth2_provider_apps.scope: apps.go:102 (create) writes sql.NullString{} unconditionally, and apps.go:159 (update) writes app.Scope back unchanged. registration.go:111 and :329 (DCR create/update) are the only paths that write a non-null value. PostOAuth2ProviderAppRequest and PutOAuth2ProviderAppRequest (codersdk/oauth2.go:77, :98) have no Scope field, so there is no API to set one.
Every admin-created app hits noScopeAllowlist(sql.NullString{}) == true, which means validateRequestedScope's allowlist logic is dead code for admin apps: either they get OAuth2ScopeUnrestricted (client omitted scope) or they get exactly what the client requested from the catalog. May be exactly the intended design ("admin apps are trusted, only DCR needs a leash"), but the framing in the PR title/description reads as blanket enforcement. Worth stating explicitly, either in the PR description or in the code path, and worth deciding whether the admin API should grow a Scope field so "restrict this admin-created app to X" is expressible.
🤖
🤖 This review was automatically generated with Coder Agents.
| // would violate the column's CHECK. | ||
| return database.OAuth2ScopeUnrestricted, nil | ||
| } | ||
| return strings.Join(requested, " "), nil |
There was a problem hiding this comment.
P2 [CRF-1] Bare aliases all and application_connect reach the persisted oauth2_provider_app_codes.scope verbatim; they are not in the api_key_scope enum, so Phase 3's typed api_keys.scopes insert will fail on them. (Mafuuu P2, Razor P2, Kurapika P3, Melody P3)
Mafuuu, verified against a compiled binary:
IsExternalScope(all): true IsExternalScope(application_connect): true IsExternalScope(coder:all): true
Razor traced the pipeline: rbac.IsExternalScope accepts the pre-canonicalization names as backward-compat aliases (scopes_catalog.go:91-95), so the catalog check waves them through, and strings.Join(requested, " ") at both the no-allowlist branch (line 74) and the subset-accepted branch (line 113) writes "all" or "application_connect" into a column whose comment states values are "drawn from the api_key_scope vocabulary" (dump.sql:2616). tokens.go:378 then propagates the same string to oauth2_provider_app_tokens.scope, and Phase 3's api_keys.scopes::api_key_scope[] insert will trip the enum, with rbac.ExpandScope("all") returning "no scope named "all"".
No runtime break today because authorizationCodeGrant still mints rbac.ScopeAll, but this ships a data-vocabulary violation the next PR has to unwind. Normalize at the catalog check: map "all" -> string(rbac.ScopeAll) and "application_connect" -> string(rbac.ScopeApplicationConnect) on both the requested list and the filtered allowlist, or drop the two aliases from IsExternalScope. Add a TestValidateRequestedScope case with requested: []string{"all"} and noAllowlist asserting ExpandScope recognizes the persisted value.
🤖
|
|
||
| func TestNoScopeAllowlist(t *testing.T) { | ||
| t.Parallel() | ||
|
|
There was a problem hiding this comment.
P2 [CRF-2] Every wantErr: true case is asserted with require.Error(t, err) and nothing else, so the three distinct rejection paths in validateRequestedScope are indistinguishable to the test. (Chopper)
Chopper:
validateRequestedScopeemits three separately-worded errors:
"unknown or unsupported scope: %q"(catalog check,authorize.go:63)"this app's allowed scope list contains no grantable scope"(all-entries filtered,authorize.go:95)"scope %q is not in this app's allowed scope list"(subset check,authorize.go:110)The table has eight
wantErr: truecases spanning all three paths, but the loop asserts onlyrequire.Error(t, err)andassert.Empty(t, got). Cases that must land in different branches (InternalOnlyScopeRejected,PartiallyOutOfAllowlistRejected,AllowlistFilteringToEmptyRejected,WhitespaceOnlyAllowlistRejected) are structurally interchangeable at the assertion. A refactor that routedWhitespaceOnlyAllowlistthrough the "unknown scope" branch, orPartiallyOutOfAllowlistthrough the "no grantable scope" branch, would still pass every test.
The point of naming InternalOnlyScopeRejected vs UnknownRequestedScopeRejected in the table is that they hit the same code path today but are distinct guarantees; the assertions do not enforce that. Add a per-case wantErrContains string field (or typed sentinels) so each rejection pins the branch it claims to cover. The HTTP-level suite in authorize_test.go extends the same class through requireInvalidScope: OutOfAllowlistRejected, UnknownScopeRejected, AllowlistFilteringToEmptyRejected, and both DCR subtests all reach different rejection reasons but assert the same invalid_scope body without touching ErrorDescription. Adding a wantContains on the description closes both.
🤖
| }) | ||
| } | ||
|
|
||
| // AC1: a scope outside the app's allowlist is rejected and no code is |
There was a problem hiding this comment.
P2 [CRF-3] Plan-doc labels (AC1..AC16, Edge Case 19/20/22, §4.2.2) reference identifiers that do not resolve inside the repository. (Gon)
Rule from Gon: source comments must not reference plan documents, audit IDs, PR shorthand, or chat history; readers of the code cannot resolve AC1, AC15, AC16, Edge Case 20, or §4.2.2, and any renumber in the Linear ticket silently invalidates them. Each of these comments already carries a self-contained explanation; deleting the label prefix does not lose information.
Instances in this file:
:82// AC1: a scope outside the app's allowlist is rejected...:95// AC1's catalog half: a scope name the enforcement layer cannot evaluate...:108// AC2: omitting scope grants the app's full allowlist (RFC 6749 §3.3).(dropAC2:; the RFC ref is fine):132// AC3: apps with no configured allowlist keep today's unrestricted behavior...:146// AC16: NULL (admin-created apps) and '' (DCR apps that sent no scope)...:166// Edge Case 20: an allowlist entry no longer in the catalog is dropped, not granted...:180// AC15: an allowlist whose every entry is dropped rejects...:194// AC8: the GET handler rejects before the consent page renders...:219// §4.2.2 accepts: dynamic client registration performs no catalog validation...(RFC 6749 §4.2.2 is the implicit-grant token response, unrelated; this is a plan-doc section reference)
Fix: strip the AC*:, Edge Case *:, and §4.2.2 prefixes and keep the substantive text.
🤖
| wantErr: true, | ||
| }, | ||
| { | ||
| // AC3/AC16: the literal return value matters. "" is exactly what |
There was a problem hiding this comment.
P2 [CRF-4] Same plan-doc labels (AC*, Edge Case, §4.2.2) appear on table-case comments in this file. (Gon)
Same rule and same fix as the sibling in authorize_test.go. Instances:
:65// AC3/AC16: the literal return value matters...:74// AC16/Edge Case 22: '' is the DCR-registered encoding...:120// Edge Case 20: catalog drift. The stale entry is dropped by the filter...:136// AC15/Edge Case 19: the all-entries-dropped counterpart to the case above...:145// §4.2.2's compatibility break, in its most direct form: a DCR client requesting exactly what it registered.
Drop the prefixes; each remaining sentence stands on its own.
🤖
| // TODO: Ignoring scope for now, but should look into implementing. | ||
| grantedScope, err := validateRequestedScope(params.scope, app.Scope) | ||
| if err != nil { | ||
| httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidScope, err.Error()) |
There was a problem hiding this comment.
P3 [CRF-6] invalid_scope is returned as a JSON body with status 400, but RFC 6749 §4.1.2.1 puts invalid_scope on the "redirect back to redirect_uri" side of the line once client_id/redirect_uri have resolved. (Chopper P3, Mafuuu P3, Razor Note, Knov Note, Melody Note, Hisoka Note)
Chopper:
The receiving end is an OAuth client library on the user's browser callback. It expects
?error=invalid_scope&error_description=...&state=...on its redirect URI. It gets a JSON response body on the Coder host instead, so its error-handling code never runs; the user is stranded at Coder with no path back to the requesting app. Thestateround-trip that clients use to correlate the failed request is also lost.
Razor pointed out this is a file-wide pattern: authorize.go:184, 205, 222, 269, 307, 313, 320, 332, 344, 390 all use the same non-redirecting shape, and access_denied at line 268 is the only branch that follows §4.1.2.1's redirect. Mafuuu called out the class problem explicitly: this PR does not create the class, but it extends it with a new error code that RFC 6749 §4.1.2.1 names verbatim. The GET-side path at line 246 has the same shape.
If the direction of travel is to keep both surfaces, state it in a comment; otherwise this is the natural place to file the class fix (a helper that all authorize-endpoint error paths route through, covering both the JSON and static-page sites). The narrower fix for just this PR is to build the redirect (error, error_description, state query params), call http.Redirect, and stop calling WriteOAuth2Error for invalid_scope.
🤖
| // configured value that grants nothing, so it falls through to | ||
| // validateRequestedScope's filtered-to-empty rejection instead of the | ||
| // unrestricted fallback. | ||
| func noScopeAllowlist(appScope sql.NullString) bool { |
There was a problem hiding this comment.
Note [CRF-17] noScopeAllowlist is a named function for a two-clause predicate with one production caller, and the whole reason it exists lives in a comment next to it. (Luffy)
Luffy:
The body is
!appScope.Valid || appScope.String == "". It is called exactly once, at line 67 invalidateRequestedScope, and again from its own unit test. The docstring says "this is the only place the two are unified", which is a claim that a name enforces something. It doesn't. Nothing stops the next caller from writing!app.Scope.Valid || app.Scope.String == ""inline. What the function actually buys is that one branch label reads "no allowlist" instead of the two-clause predicate.
Either that reading is worth a function, or the same words go on the branch as a comment. Raising it as an observation, not a demand; the abstraction is single-use and the comment does the work.
🤖
|
|
||
| // The subset check runs against the filtered allowlist, not the raw one, | ||
| // so a dropped entry cannot be requested explicitly either. | ||
| allowedSet := make(map[string]bool, len(filtered)) |
There was a problem hiding this comment.
Note [CRF-18] The literal-name subset check rejects semantic narrowing across the catalog hierarchy; a client asking for less privilege gets no token and is pushed to ask for the broader composite instead. (Meruem)
Meruem:
coder:workspaces.accessexpands toworkspace:{read,ssh,application_connect}(coderd/rbac/scopes.go:158).workspace:readsits alone in the catalog (coderd/rbac/scopes_catalog.go:13). Both are external. An admin who configures an app withScope: "coder:workspaces.access", i.e. the natural composite an app registration UI would surface, cannot approve a client that requestsscope=workspace:read:allowedSetholds only the literal composite name, so the subset check at line 109 rejects. The client's only path to a token is to re-issue the request with the broadercoder:workspaces.access, which is the opposite direction from what they were asking for.
The invariant being hidden is what an allowlist entry means. Every admin who reads "allowlist" will read it as a ceiling. Class fix: normalize the allowlist to its low-level names before the subset check runs (using rbac.CompositeSitePermissions); or refuse to accept composite names in the allowlist so the ceiling is always spelled in the vocabulary the check runs in. Phase 3 hasn't wired enforcement to this column yet, so no live client is being narrowed today; the constraint should be settled before it does.
🤖
| // persisting it here would store unvalidated client input, so | ||
| // the code records an unrestricted grant. | ||
| Scope: database.OAuth2ScopeUnrestricted, | ||
| // The negotiated scope, not the requested one: it has been |
There was a problem hiding this comment.
Note [CRF-20] The persisted grantedScope has no reader yet; authorizationCodeGrant still mints tokens at rbac.ScopeAll. (Hisoka Note, Pariston Note, Ryosuke Note)
Disclosed in the PR description ("Issued tokens are still unrestricted"). Cross-linking as a Note because: (1) codes.scope and tokens.scope now carry a truthful negotiation, and api_keys.scopes (the row enforcement actually reads) does not; introspection on the token row will suggest a restriction the token does not have. (2) Under "assume no follow-up" the persisted column is metadata no enforcement path reads, and consent recorded here is not a promise the token upholds. (3) CRF-1's alias-normalization work will land in Phase 3 anyway; doing it here avoids the next PR needing to normalize on read and remember to also normalize refresh.
🤖
| // renders, rather than after the user clicks Allow. This mirrors how | ||
| // the PKCE code_challenge requirement is already handled: inside | ||
| // extractAuthorizeParams, which both GET and POST call. | ||
| if _, err := validateRequestedScope(params.scope, app.Scope); err != nil { |
There was a problem hiding this comment.
Note [CRF-21] validateRequestedScope is called at two sites over the same inputs, and the first call discards its result. (Meruem)
Meruem: GET (ShowAuthorizePage, line 241) calls it and throws the string away. POST (ProcessAuthorize, line 336) calls it and persists the string. Both calls are validateRequestedScope(params.scope, app.Scope), and both derive params.scope from the same query parser and app.Scope from the same middleware. No bug today because the inputs are identical; the class of bug the shape invites is any future side effect added to the negotiation (telemetry, consent-page display, a signed record of what was approved) has to be added at both sites and stay in sync.
The class fix is not to move the call inside extractAuthorizeParams (which does not have app); it is to make the negotiated scope a value on authorizeParams, computed once via a method like params.negotiateScope(app.Scope) that both handlers call, or to extend extractAuthorizeParams to take the app and return an authorizeParams that already carries grantedScope. Either way: negotiate once per request, use the value or reject. This is also where CRF-5 (consent page shows scope) becomes natural.
🤖
| } | ||
|
|
||
| // TestOAuth2ClientScopeValidation tests scope parameter validation | ||
| // TestOAuth2ClientScopeValidation tests scope parameter validation at |
There was a problem hiding this comment.
Note [CRF-22] TestOAuth2ClientScopeValidation exists in both coderd/oauth2provider/validation_test.go and coderd/oauth2_metadata_validation_test.go as near-duplicates; this PR now writes the same comment twice. (Razor Note, Robin Note, Zoro Note)
wc -l reports 785 vs. 782 lines, and diff shows the second file is the first with package oauth2provider_test swapped for package coderd_test and typed codersdk.OAuth2Provider* constants inlined as strings. Both call client.PostOAuth2ClientRegistration; neither covers a path the other does not.
Pre-existing shape debt, not this PR's to fix, but the PR made every future edit of this test cost two edits instead of one, and any future PR that lands a registration-time catalog check will need a third rewrite in both places. The clean shape is one file, imported once. Consider consolidating on the coderd/oauth2provider copy in a follow-up, since that is the package that owns the behavior.
🤖
/oauth2/authorizeparses thescopeparameter and then discards it (// TODO: Ignoring scope for now), so an app's configured allowlist has never restricted anything and a client asking for more than it should be granted is never told no. Phase 1 added the columns that carry a negotiated scope from a code to the token it becomes, but nothing negotiates one yet:authorize.gowrites a hardcodedcoder:allonto every code it issues.This PR makes the authorize step negotiate.
validateRequestedScopechecks every requested scope name against the external scope catalog (rbac.IsExternalScope), filters the app's stored allowlist through that same catalog, and requires the request to be a subset of what survives. An omittedscopedefaults to the filtered allowlist per RFC 6749 section 3.3. The result is persisted onoauth2_provider_app_codes.scope, replacing Phase 1's placeholder. Both handlers validate, so a request that cannot succeed is rejected before the consent page renders rather than after the user clicks Allow, matching how this file already treats PKCE'scode_challenge.Two paths can produce an empty result, and they are deliberately not the same path. An app with no allowlist and no requested scope keeps today's unrestricted grant, written as the explicit
coder:allsentinel because the column isNOT NULLwith a non-emptyCHECK. An app whose allowlist filters to nothing is rejected instead, since falling back there would grant strictly more than the allowlist ever permitted.NULLand''are one "no allowlist configured" state, unified in a single predicate now that reading that column is an authorization decision rather than a display detail.This carries an accepted compatibility break. Dynamic client registration performs no catalog validation, so apps registered with scopes such as
openid,read, oradminhold allowlists this server cannot grant from. Those apps now fail authorization in both directions: requesting the scope they registered is rejected, and omittingscopehits the filtered-to-empty rejection. Grandfathering unknown names through would seedapi_keys.scopeswith valuesdbauthzcannot evaluate, trading a visible negotiation-time error for a silent enforcement-time hole. The affected population is bounded to DCR self-registered apps that sent ascope, the failure is immediate and carries a standardinvalid_scope, and the remedy is re-registration with catalog scopes.Issued tokens are still unrestricted:
authorizationCodeGrantcontinues to mintrbac.ScopeAlland does not yet read the persisted column. This PR changes which authorization requests succeed, not what a token can do.Phase 2 of PLAT-470, tracked as PLAT-479. Stacked on #28007. Applying the negotiated scope in
authorizationCodeGrant, refresh narrowing, and the docs update follow as separate PRs.Where this sits in the scope pipeline (green marks what this PR touches)
flowchart TD subgraph authorize["/oauth2/authorize"] AZ1["ShowAuthorizePage (GET)<br/>validates scope, then renders consent"] AZ2["ProcessAuthorize (POST)<br/>validates scope, then issues code"] V["validateRequestedScope<br/>catalog check + allowlist subset"] AZ1 --> V AZ2 --> V end APP[("oauth2_provider_apps.scope<br/>the allowlist, read as input")] APP --> V V --> CODES[("oauth2_provider_app_codes.scope<br/>negotiated value, was a placeholder")] subgraph codegrant["POST /oauth2/tokens, authorization_code"] G1["authorizationCodeGrant<br/>still hardcoded to rbac.ScopeAll"] end CODES --> G1 G1 --> TOKENS[("oauth2_provider_app_tokens.scope")] subgraph enforce["Every authenticated API request"] E1["httpmw ExtractAPIKey"] --> E2["APIKey.ScopeSet()"] --> E3["UserRBACSubject"] --> E4["dbauthz authorize"] end TOKENS --> E1 classDef changed fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,color:#1b3c1e class AZ1,AZ2,V,CODES changedGreen is added or changed here. The grant path and the enforcement engine below it are untouched: they already read a key's scopes correctly and are waiting on real data, which the next PR feeds them.
How to review this PR
coderd/oauth2provider/authorize.go: the whole behavior change.noScopeAllowlistfirst, since it defines what "no allowlist" means, thenvalidateRequestedScope's four branches, then the two call sites. The catalog check runs on the request before any allowlist logic, so an internal-only name likedebug_info:readis rejected whether or not the app has an allowlist. That check is a curation, not a validity check: RBAC expands such names fine and theapi_key_scopeenum stores them, which is exactly why the 55-name catalog is narrower than both.coderd/oauth2provider/authorize_internal_test.go: the table covers the four branches plus the cases that pin the contract the signature cannot. A rejection returns an empty string and a success never does, because the value goes straight to aCHECK (scope <> '')column.coderd/oauth2provider/authorize_test.go: HTTP-level coverage against a real database, asserting the persisted column by parsing the issued code's prefix out of the redirect rather than inferring the grant.TestOAuth2AuthorizeDCRScopeCompatibilitypins the break above in executable form, registering through DCR because that is the only route that produces a non-catalog allowlist naturally.coderd/oauth2provider/validation_test.goandcoderd/oauth2_metadata_validation_test.go: comments only. Both files carried a near-duplicateTestOAuth2ClientScopeValidationasserting registration acceptsadmin, commented "should be allowed but validated during authorization." That promised the opposite of what authorization now does. Registration-time expectations are unchanged, since this PR adds no registration-time validation.Verified locally: the full
coderd/oauth2providerpackage,coderd'sTestOAuth2*suites, andcoderd/mcpall pass against Postgres, andgolangci-lintis clean on both changed packages.