{{ message }}
feat: re-check the app allowlist at OAuth2 code redemption - #28740
Merged
Conversation
Add ScopesCover, which reports whether every permission a requested scope grants is also granted by at least one of a set of allowed scopes. It expands both sides and compares the resulting permissions, so coder:workspaces.access covers workspace:read even though it never names it, and coder:all covers everything. The comparison is deliberately asymmetric. Positive permissions on the allowed side that it does not model are dropped, which can only make the answer stricter. Anything unmodelled on the requested side is an error instead, because ignoring it would answer "covered" about authority that was never compared. Negative permissions are the exception and fail closed on both sides, since dropping an anti-grant from the ceiling would widen it rather than narrow it. Add CanonicalScopeName, which maps the backward-compatibility aliases IsExternalScope accepts onto the names the api_key_scope enum stores. IsExternalScope answers whether a name may be requested, not how that name is spelled once persisted, so a caller that stores what it validated has to canonicalize in between. Both functions are added without production callers. The OAuth2 authorize endpoint uses them to negotiate a requested scope against an app's configured allowlist, which follows in a separate change.
State the rule the guards enforce, site-level grants only, instead of describing the asymmetry abstractly. The allow-list case is now covered alongside negative permissions, which the previous wording omitted even though the code treats them identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ScopesCover checked the requested scope for org and user grants but not the allowed scopes, whose User and ByOrgID permissions were discarded unread. A scope granting workspace:* at site level while negating workspace:delete for the user would have covered a request for workspace:delete, because the negative that carves the action back out lives in the half coverage never examined. No catalog scope populates those fields today, so nothing was miscompared in practice. The gap mattered because these guards exist to keep the comparison fail-closed, and this one failed open. Both sides now run the same checkCoverable helper, which refuses a scope carrying org or user grants, a negative permission, or a resource allow list. The helper names the side, so an error reports which half of the comparison was undecidable. The doc comment claimed an unmodeled grant on the allowed side is dropped; nothing is dropped now, so it is gone. ScopesCover builds every Scope it reads from ExpandScope, which cannot produce these shapes, so the guards are unreachable through the public API. scopes_internal_test.go drives synthetic Scope values through checkCoverable instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
permissionCovered skipped negative permissions, but checkCoverable now refuses a scope carrying one on either side, so the branch was dead. It was never defense in depth. Had a negative reached it, skipping the anti-grant would leave any wildcard beside it free to match, and a scope granting workspace:* while negating workspace:delete would report workspace:delete as covered. The skip widened the ceiling while looking like it narrowed it. The precondition moves to the doc comment, which names checkCoverable as what enforces it and says why subsumption cannot answer the question an anti-grant poses. No behavior change: the branch was unreachable. permissionCovered goes from 88.9% to 100% statement coverage.
Five review findings on the coverage tests, all in scopes_test.go. CanonicalScopeName had both alias arms at zero coverage. Its only caller in the tests loops over ExternalScopeNames, which yields canonical names only, so the canonicalizing call returned its input unchanged on every iteration and read as coverage without being any. Swapping the arms, so that `all` persisted application_connect and the reverse, kept the suite green. TestCanonicalScopeName now pins the mapping and the loop appends the aliases, taking the function from 50% to 100%. The appended aliases raise branch coverage and assert a requestable name is comparable once canonicalized, but they cannot detect a swapped mapping, since both aliases resolve to scopes that cover themselves. The comment says so rather than implying the loop guards more than it does. CompositeDoesNotCoverNonMember and CompositeDoesNotCoverWiderActionOnCoveredResource both asked for an ungranted action on a resource coder:workspaces.access does grant, so they tested one branch twice and left "resource not granted at all" untested. They are now split along that line, with names that describe which failure each one is. The three wantErr rows shared a bare require.Error, so any error passed any row and a bug failing every input on the requested side would have left the allowed-side row green. wantErrContains replaces the bool and names the side. Rewording the allowed-side message as the requested-side one now fails three rows that previously all passed. Alias rejection was tested for one alias on one side. Both aliases are now tested on both sides. The allowed-side rows are the ones that earn their place: they are what would catch someone canonicalizing inside the allowed loop and widening the contract without a caller asking.
ScopesCover expanded and compared in a single pass, so the invariant guards only ever ran on scopes ExpandScope had produced. Every such scope satisfies them, which left the guards unverified in the position that matters: the existing test called checkCoverable directly and could not tell whether ScopesCover consulted it on both sides, or at all. Split the comparison into scopesCoverExpanded, which takes already expanded scopes paired with the names they came from. Tests drive synthetic Scope values through it, so dropping the guard from either side now fails, as does an allowed scope that grants every workspace action except delete answering a request for delete. Expanding every allowed scope before any guard runs reorders two error paths against each other: a requested scope that fails a guard alongside an unknown allowed name now reports the expansion failure rather than the guard failure. Both return (false, error), and no ScopeName reaches that combination today.
…ontract The knowledge of which spellings are backward-compatibility aliases lived in two switches, one in IsExternalScope and one in CanonicalScopeName, kept in step by discipline. Drift between them is asymmetric: a name the first accepts and the second does not rewrite is declared public and then fails to expand on every request naming it. Both now read one table, so they agree by construction, and an internal test walks that table asserting each alias is public, resolves to a public name, and resolves to one ExpandScope accepts. A third alias is covered the day it is added. ScopesCover stated "names must be canonical" in prose only, which is wrong for exactly the two inputs IsExternalScope accepts and ExpandScope does not. The parameters are now canonicalAllowed and canonicalRequested, so the requirement shows up in editor hints at every call site rather than only in a doc comment the caller may not have opened. Naming the parameters was chosen over canonicalizing inside ScopesCover. The single downstream caller already canonicalizes both sides in bulk before comparing, so absorbing the step would remove nothing from it while dissolving the distinction between a public spelling and a stored one at the layer that should hold it.
…roken The site-only, wildcard-allow-list, no-negatives invariant was described on ScopesCover and enforced by its guards, but ExpandScope, which is what produces those values, had no doc comment at all. Someone adding a scope reads ExpandScope and its neighbors; nothing there warned that populating User or adding a negative makes the scope uncomparable. State it there, along with the canonicalization requirement, and name the consequence rather than just the rule. Also note on ScopesCover that a wildcard request needs a wildcard grant. Enumerating today's concrete actions genuinely is narrower than `workspace:*`, so the rejection is intended. The OneActionDoesNotCoverResourceWildcard row already pins the behavior; the note stops the next reader of an authorize endpoint from taking it for a bug and closing the gap. Comments only. Checked that the documented invariant actually holds for all three builtin scopes and all seven composites.
TestScopesCoverAllowedNegativeDoesNotWiden drove the same scope shape as the NegativeUserPermission row of TestScopesCoverGuards, but asserted only that some error came back. The row asserts the message, the side it names, and that the comparison reports no coverage, and it runs the shape on both sides rather than one. The weaker copy could pass on a regression that returned the wrong error or stopped naming the side. Fold the scenario it documented into the row's comment and drop the copy. Rename the shared permission fixtures after the value they hold. The site prefix read as "belongs in Role.Site", while two of the three are placed in Role.User to build the shapes the guards refuse, and the No suffix gave no hint that it means Negate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The allowed-side wrap printed the scope name and then wrapped an error that prints it again, so the two sides of one comparison read differently: expand allowed scope "foo": no scope named "foo" expand requested scope: no scope named "foo" Drop the redundant verb and let the inner error carry the name on both sides. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The docstring said the list includes the `all` and `application_connect` special scopes. It appends ScopeAll and ScopeApplicationConnect, which are the `coder:` spellings, so the bare aliases are absent. Two callers already compensate by appending them by hand, one of them with a comment stating the mismatch. Describe what the function returns and name the helper that bridges the gap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The invariant that expansion populates Site only was stated in full on ExpandScope, checkCoverable and ScopesCover, and the "everything except delete" example appeared on checkCoverable and again on permissionCovered twenty lines below. State it once on ScopesCover, which is the function whose behaviour depends on it, and cross-reference from the other two. Drop framing that ranked implementation choices nobody proposed, and cut the two test comments down to the facts the assertions do not already carry. Kept in full: what each guard in checkCoverable defends, since no other comment says it, and the wildcard rule on ScopesCover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
checkCoverable said a negative site permission would be skipped, naming a branch permissionCovered no longer has. A negative reaching it matches on resource type and action like any other grant, so the anti-grant would read as a grant. Name that instead, so the cross-reference lands on a doc that matches the code.
The docstring listed the aliases and the low-level scopes, omitting the curated composites the function also accepts. A caller consulting it to decide whether coder:workspaces.access is public read no from the doc and yes from the code.
ExternalScopeNames promises it offers each scope under one canonical spelling, and no test held it to that. TestScopesCoverEveryExternalScope appended the two aliases, but canonicalized them back into names the list already carries, so it re-ran assertions the list iteration had made and left the promise itself unpinned. Assert on the alias table instead: the list omits the alias and offers its canonical target. Every offered name is already proven coverable, so the aliases inherit coverage, and a third alias inherits both invariants the day it is added rather than needing a third hardcoded pair here.
The authorize endpoint parsed the scope parameter and discarded it, so an app's configured allowlist never restricted anything and a client asking for more than it should get was never told no. Phase 1 added the columns that carry a negotiated scope from a code to the token it becomes, but nothing wrote one, so every code was stamped unrestricted. Negotiate the scope at authorization time and persist the result: - Requested names must be in the external scope catalog, and the app's stored allowlist is filtered through that same catalog. Filtering only ever narrows what can be granted. - The allowlist bounds authority, not spelling. A request is granted when every permission it grants is also granted by the allowlist, whether or not the allowlist names it, so an app allowed coder:workspaces.access can approve a client asking only for workspace:ssh. - Omitting scope grants the filtered allowlist, per RFC 6749 section 3.3. - Both handlers negotiate, so a request that cannot succeed fails before the consent page renders rather than after the user clicks Allow. Each reports the failure the way it already reports its own errors: a static error page on the GET side, an OAuth2 error body on the POST side. - Two paths produce an empty result and are deliberately distinct. No allowlist and no request keeps the previous unrestricted grant, written as an explicit sentinel because the column is NOT NULL with a non-empty CHECK. An allowlist that filters to nothing is rejected, since falling back would grant strictly more than the allowlist ever permitted. Dynamic client registration performs no catalog validation, so apps registered with scopes such as openid or admin hold allowlists this server cannot grant from. They now fail authorization in both directions. Grandfathering unknown names would seed the enforcement path with values it cannot evaluate, trading a visible negotiation-time error for a silent enforcement-time hole. The failure names the registered scopes and the remedy. Issued tokens are still unrestricted: the exchange copies the negotiated scope onto the token record, but the API key it mints carries no scope. This changes which authorization requests succeed, not what a token can do.
The consent page told every user the app was getting full access to their account, which stopped being true once the authorize endpoint began negotiating a narrower scope. A user approving a request has no other place to learn what they are handing over, so the page has to follow the grant rather than a fixed sentence. List the negotiated permissions when the grant is bounded, and keep the original full-access wording when it is not. An unrestricted grant is reported as full access rather than as "coder:all", since the scope name tells a user less than the sentence does. The list collapses to the full-access wording whenever the unrestricted scope is present, not only when it stands alone: an allowlist registered as `coder:all coder:workspaces.access` grants everything, and naming the narrower entry beside it would describe the grant as bounded. role="list" and role="listitem" are explicit because WebKit drops the implicit list semantics from a list styled with list-style: none, which would otherwise leave VoiceOver announcing the permissions as loose text. Also narrow the fragment the tests match for one rejection branch. The GET side renders its description into HTML, which escapes the apostrophe in "this app's allowed scope list", so the fragment stops before it.
…back A rejected authorization request answered on Coder, which reaches only the user's screen. The client's error handling never ran, and the state it sent was dropped, so it could not correlate the failure with the request that caused it. RFC 6749 section 4.1.2.1 requires the error be delivered to the client's redirect URI once the client is known. Redirect to the app's registered callback with error, error_description, and the state exactly as it arrived. Both handlers use this, replacing the static error page on the GET side and the OAuth2 error body on the POST side. This is safe here specifically because of ordering: extractAuthorizeParams exact-matches the redirect URI against the app's registered callback, and it runs before the scope check, so the destination is the app's own no matter what the request carried. Only errors raised after that point may use this helper, which its precondition states. Errors from extractAuthorizeParams itself must not, since the URI is unvalidated there. MismatchedRedirectURINotRedirected pins the ordering: an unregistered redirect_uri fails on Coder with no Location header on either verb, even when the same request also carries a scope the app cannot be granted. The other error paths in this file are unchanged, since several of them are where redirect URI validation fails.
permissionCovered could drop its action comparison and the suite stayed
green: no ScopeName expands to {*, <specific action>}, since the wildcard
entry in policy.RBACPermissions carries no actions and coder:all is the
only wildcard resource the catalog spells. Reach the shape through
scopesCoverExpanded instead, with a positive control so the case fails on
the action rather than on resource matching, and a mirror pinning that a
single-resource grant does not cover a request for every resource.
Every allowed-side error row named the bad scope as the only entry, so an
implementation that answers as soon as one entry covers the request never
reached it. Add a row where the bad name sits behind coder:all, the only
row that fails when ScopesCover expands inside the comparison loop rather
than up front.
…otiateScope The function does not check a requested scope and hand back a verdict. It decides what scope the code will carry, which for an omitted request is the app's allowlist and for an app with no allowlist is coder:all. Neither is a value the caller asked for, so the name promised the wrong thing.
… sentinels The black-box tests assert on the description that reaches the client, and they did so through hand-copied fragments of the sentinel messages. A reworded sentinel would leave every case asserting on text no branch produces, and each case would still pass through whichever branch happened to match next. The sentinels live in package oauth2provider and the tests live in oauth2provider_test, so they are bound through exported values declared in the package's internal test file, which compiles into the same binary.
…turning it rbac.ScopesCover reports an error when it cannot expand one of the names it was handed. That is a deployment-side condition: the app's stored allowlist holds something RBAC will not resolve, and no client can fix it by asking differently. Folding it into errScopeNotAllowed both told the client it had asked for too much, which is not what happened, and rendered RBAC internals into error_description. The failure now goes to the log with the app that provoked it, and the client receives a sentinel of its own. negotiateScope takes the whole app rather than its scope alone so the log line can name it.
… is grantable The rejection named the filter's input, rejoined from fields. For a whitespace-only allowlist that input is empty, so the app owner was shown "" as the value they had to change: the one configuration where the message is the only clue anything is set at all. It now names the stored value verbatim.
…asons Two reasons said things the code does not do. "scope is not in this app's allowed scope list" described membership, but the check is permission coverage: a scope the allowlist never names is granted when a listed composite already confers it. A client reading the old text would go looking for its scope in a list it was never matched against. "re-register the app with supported scopes" prescribed the one remedy a DCR client has. An admin-created app is edited, not re-registered, and a DCR client can update itself in place through RFC 7592. The new text carries an apostrophe on the path the GET handler renders through an HTML template, so the helper that reads those responses now unescapes before matching.
The swagger annotation said a requested scope must be within the app's configured allowlist, which is wrong twice over. The allowlist is checked by permission coverage, not name membership, and it is not the only gate: every requested name must also be in this deployment's scope catalog, including for an app that has no allowlist at all. The omitted-scope default was likewise stated only for apps that have one. Two code comments went stale the same way. The branch table called the omitted-scope default the whole allowlist when it is the catalog-filtered one, and the comment over the persisted scope said the token minted from the code will carry it, which is the next phase's work, not this one's.
…tiated-scope # Conflicts: # coderd/oauth2provider/authorize.go # coderd/oauth2provider/authorize_test.go # coderd/rbac/scopes_internal_test.go
…-negotiated-scope
…scope-recheck # Conflicts: # coderd/oauth2provider/authorize.go
Shorten the comments on the scope negotiation helpers to plain sentences and drop restatements of what the code shows. Also trim the PKCE revocation comments in tokens.go.
…-negotiated-scope # Conflicts: # coderd/oauth2provider/authorize.go
Shorten the comments introduced with the negotiated-scope exchange to plain sentences and drop restatements of what the code shows.
…scope-recheck # Conflicts: # coderd/oauth2provider/authorize.go # coderd/oauth2provider/tokens.go
Shorten the comments on the allowlist filter and the stale-scope recheck to plain sentences.
… coverage negotiateScope returns errCoverageUndecidable when the coverage comparison against the app's allowlist fails outright, which is this server failing to decide rather than a defect in the request. Both authorize call sites reported it as invalid_scope and echoed the sentinel text, which names RBAC internals. Map it to server_error (RFC 6749 §4.1.2.1) with a fixed description. The other rejections keep invalid_scope and their existing text.
…-negotiated-scope
Both conflicts are comment and prose wording that #28179 trimmed on main while this branch reworded for scope enforcement. Took this branch's wording in each: the negotiated scope now bounds the issued token, so "the pre-enforcement grant" and "does not yet restrict what the issued token can do" are no longer true.
RFC 6749 §5.2 scopes invalid_scope to what the client requested, but a stored scope outside the api_key_scope enum is server state the client cannot change by asking differently. The grant is what is unusable and re-authorizing is the only remedy, which is what invalid_grant says.
…scope-recheck The base branch changed an unmintable stored scope from invalid_scope to invalid_grant. That reasoning covers this branch's sibling rejections: a stale allowlist and an app with nothing grantable are stored state the client cannot change by asking differently, so both answer invalid_grant. errCoverageUndecidable is this server failing to compare rather than a defect in the grant, so it answers server_error as authorization does. Mintability is now decided before the allowlist recheck. RBAC cannot expand a stored name outside the enum, so the coverage comparison reported such a name as undecidable instead of naming the value an operator has to fix.
BobbyHo
added a commit
that referenced
this pull request
Sep 2, 2026
**TL;DR** First of PLAT-480, continuing the merged PLAT-479 stack. #28178 decided what the scope is and #28179 reported it; this PR is the one that makes it bind. Until now the scope negotiated at `/oauth2/authorize` was recorded on the grant but never reached the token: every OAuth2 access key was minted with `coder:all`, so a client that asked for `coder:workspaces.access` still got full API access. | PR | What it does | |---|---| | #28167 | `ScopesCover` compares an allowlist against a request by permission coverage rather than by name. Merged. | | #28178 | The authorize endpoint checks the request against the scope catalog, grants it only if the app's filtered allowlist covers it, and persists the result on the code. Merged. | | #28179 | The consent page lists the negotiated permissions, and `invalid_scope` reaches the client's own callback per RFC 6749 §4.1.2.1. Merged. | | **#28237 (this)** | The exchange mints the API key with the stored scope, and the token response states what was granted. | | #28740 | Re-checks the app allowlist at redemption, for a code whose app was narrowed after the code was issued. | | #28751 | Lets a refresh narrow the granted scope, which is the `scope` request parameter this PR still ignores. | - #28179 has merged, so this diff is against main and reviews on its own. - **This is the first PR in the stack where a token's authority actually changes.** Everything before it recorded and reported. That is the blast radius worth reviewing: a grant that negotiated a narrow scope now gets a key that is genuinely narrower, and anything that was quietly relying on `coder:all` will start seeing 403. - Grants that predate the scope columns carry what migration 000569 backfilled, `coder:all`, so they refresh unrestricted and reach what they reached before. Pinned by `BackfilledScopeRefreshesUnrestricted`, seeded the way the migration leaves a row rather than exchanged. - The actor that writes the key stays `rbac.ScopeAll`. Narrowing it to the granted scope would deny `api_key:create` and fail the exchange. The bound lives on `api_keys.scopes`, which dbauthz reads on every request, not on the actor doing the insert. **Where in the flow** <details> <summary>Diagram: where the stored scope turns into a bound on the token</summary> ```mermaid flowchart TD AZ["GET or POST /oauth2/authorize<br/>negotiates the scope and persists it on the code<br/>PR 28178, merged"] AZ --> TK["POST /oauth2/tokens"] TK --> GT{"grant_type"} GT -->|authorization_code| S1["scope read from codes.scope"] GT -->|refresh_token| S2["scope read from tokens.scope<br/>req.Scope is parsed and ignored, PR 28751"] S1 --> CV["scopeStringToAPIKeyScopes, this PR<br/>every name must be in the api_key_scope enum<br/>an empty list is an error, not coder:all"] S2 --> CV CV -->|"unknown name, or empty"| ERR["400 invalid_grant, this PR<br/>error_description names the offending value"] CV -->|mintable| GEN["apikey.Generate with Scopes, this PR<br/>was: no Scopes, so every key defaulted to coder:all"] GEN --> KEY["api_keys.scopes<br/>read by dbauthz on every later request:<br/>this is where the grant becomes a bound"] GEN --> ROW["the new tokens row carries the same scope,<br/>so the next refresh reads back the same grant"] KEY --> RESP["200 with scope in the token response, this PR<br/>RFC 6749 5.1, OAuth 2.1 1.4.1"] ROW --> RESP ``` </details> - Token endpoint only, both grants. Authorize is untouched apart from two comments that said the scope was not yet enforced. - `scopeStringToAPIKeyScopes` turns the stored scope string into `database.APIKeyScopes` for `apikey.Generate`. Names are checked there rather than inside `apikey.Generate`, whose error would surface as a 500 rather than an OAuth2 error. - An empty list is an error, not an unrestricted key. `apikey.Generate` defaults an empty scope list to `coder:all`, so an empty stored scope would silently widen the grant instead of failing it. The same reasoning is why the `expandRBACScope` comment in `modelmethods.go` was stale: that function already errored on an empty list while its comment still promised `rbac.ScopeAll`. - A stored scope outside the `api_key_scope` enum answers 400 `invalid_grant` rather than 500, with the offending name in `error_description`. Not `invalid_scope`: RFC 6749 §5.2 and OAuth 2.1 §3.2.4 both scope that code to what the client requested, and this value is server state the client cannot change by asking differently. The grant is what is unusable, and re-authorizing is the only way out. - Refresh copies the presented token's scope onto both the new API key and the rotated refresh row. `req.Scope` is parsed but ignored; #28751 is where it starts being honoured. - The swagger annotation moves from `oauth2.Token` to `codersdk.OAuth2TokenResponse`, because `oauth2.Token` has no `scope` field to document. - `EveryCatalogNameMintable` pins the catalog against the `api_key_scope` enum. The two are maintained separately, and a name negotiable at authorization but unmintable at exchange would leave a client holding a code it can never redeem. **What it satisfies** Checked against both RFC 6749 and the OAuth 2.1 draft (draft-ietf-oauth-v2-1-15). The 2.1 draft moves the scope-reporting rule from RFC 6749 §5.1 into §1.4.1 and grades it in §3.2.3, but the requirement is the same one. - RFC 6749 §5.1, OAuth 2.1 §1.4.1 — "If the issued access token scope is different from the one requested by the client, the authorization server MUST include the `scope` response parameter in the token response". OAuth 2.1 §3.2.3 grades `scope` as "RECOMMENDED, if identical to the scope requested by the client; otherwise, REQUIRED". Both grants set it unconditionally, which covers both grades. Without it, a client whose request was narrowed against the app's allowlist, or defaulted because it requested nothing, first learns its bounds from an unexplained 403. Pinned by `IssuedTokenBoundsTheAPI`, which requests no scope and asserts the response names what it got. - OAuth 2.1 §1.4.1 — "The authorization server MAY fully or partially ignore the scope requested by the client, based on the authorization server policy or the resource owner's instructions." This is what licenses ignoring `scope` on a refresh, and the licence is conditional on reporting the actual grant. Before this PR the exchange both ignored the request and said nothing, which is the half that was not defensible. - OAuth 2.1 §4.3.1, RFC 6749 §6 — the refresh `scope` parameter is OPTIONAL and "if omitted is treated as equal to the scope originally granted by the resource owner". Exactly what `refreshTokenGrant` does with `dbToken.Scope`. - OAuth 2.1 §4.3.3 — "If a new refresh token is issued, the refresh token scope MUST be identical to that of the refresh token included by the client in the request." Rotation copies `dbToken.Scope` onto the new refresh row. - OAuth 2.1 §3.2.3 — "If refresh tokens are issued, those refresh tokens MUST be bound to the scope and resource servers as consented by the resource owner. This is to prevent privilege escalation by the legitimate client". The refresh row has carried the negotiated scope since #28178; this PR is what makes the access token minted from it carry the same bound. Pinned by `RefreshDoesNotWidenTheScope`. - OAuth 2.1 §1.4.1 — a client that omits `scope` must be met with "a pre-defined default value or fail the request indicating an invalid scope", and the server "SHOULD document its scope requirements and default value". #28178 chose the default; `scopes_supported` and the integration guide document it. Unchanged here, but it is why `IssuedTokenBoundsTheAPI` can request nothing and still receive a bounded token. - Not yet: OAuth 2.1 §4.3.1 also says "The requested scope MUST NOT include any scope not originally granted by the resource owner." A refresh naming a narrower scope is ignored rather than honoured, and one naming a wider scope is ignored rather than rejected with `invalid_scope` (§3.2.4, "exceeds the scope granted by the resource owner"). Neither case widens a token, so there is no escalation, and §1.4.1's MAY plus the `scope` response parameter keep the client correctly informed in the meantime. #28751 is where narrowing lands. Documented under Limitations until then. --- First of three, [PLAT-480](https://linear.app/codercom/issue/PLAT-480). Depends on #28179, which has merged, so this reviews against main. #28178 decided the scope and #28179 reported it. This PR is the one that makes a token feel it. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
…-recheck # Conflicts: # coderd/oauth2provider/tokens.go # coderd/oauth2provider/tokens_internal_test.go # coderd/oauth2provider/tokens_test.go # docs/admin/integrations/oauth2-provider.md
Nineteen review findings. The substantive ones: rejections put a double quote into error_description, which RFC 6749 §5.2 excludes; the docs and a comment named an administrator as the actor that narrows a registration, when only the client can through RFC 7592; the refresh exemption cited §6, which caps what a client may request rather than obliging the server; the shared coverage warning named no endpoint; and the refusal the check exists to produce left no server-side trace. firstScopeOutsideAllowlist takes appID rather than the app, so the log cannot name an allowlist the comparison did not use, and defers to a new rbac.FirstScopeNotCovered that expands the allowed side once rather than once per granted scope.
Plainer wording for the scope checks and their error classification, and drop rationale that repeats what the code or the docs already say.
BobbyHo
marked this pull request as ready for review
September 3, 2026 14:44
dylanhuff-at-coder
approved these changes
Sep 3, 2026
dylanhuff-at-coder
left a comment
dylanhuff-at-coder
left a comment
Contributor
There was a problem hiding this comment.
Two smaller things worth addressing imo but otherwise looks good
Two review findings on the allowlist re-check. An app's registered scope is unvalidated RFC 7591 metadata bounded only by the 4 MiB request body limit, and both paths that read it treated it as small and well formed. grantableScopes sized its slice from strings.Count(appScope, " "), so a whitespace-only registration preallocated roughly four million string headers, around 64 MiB, on every authorization and every redemption. It now canonicalizes and drops duplicates as names are read, holding the slice to the size of the external scope catalog whatever the input and removing the second unbounded allocation canonicalScopes made. errNoGrantableScope no longer names the registered value. Rejoining the fields excluded the tabs and newlines RFC 6749 §5.2 forbids in error_description but left the quotes, backslashes, and non-ASCII a registration may also carry, and put up to 4 MiB of it in the body. The reason alone is actionable, since the client is the party that registered the value, so the refusals log app_id instead. The same echo in negotiateScope goes with it, or the authorize endpoint would keep returning what the token endpoint stopped returning. The integration guide said the description opens with the name that caused the rejection, which is now true only of the requested name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Second of the PLAT-480 stack. #28237 made the negotiated scope bound the issued token; this PR makes a narrowing of an app's registered scopes apply to codes that were already issued.
An authorization code is valid for ten minutes, and its scope is checked against the app's registered scopes exactly once, when the code is issued. A registration narrowed inside that window had the narrowing ignored: the code still redeemed for a token carrying the wider scope.
invalid_grant. RFC 6749 §5.2 reservesinvalid_scopefor the scope the client asked for, and this one comes from the stored grant.rbac.FirstScopeNotCovered, which expands the allowlist once rather than once per granted scope.scope, through RFC 7592; no administrator surface writes the column. The re-check reflects the registration at redemption rather than constraining the client.coder:all. For an app registered with a narrowerscopethose are refused at redemption until they expire, at most ten minutes.Stack: #28237 (merged), this PR, #28744.