feat: let an OAuth2 refresh narrow the granted scope - #28751

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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.

DeleteOAuth2ProviderAppCodeByID and its ReturningRow twin differed only by
RETURNING *, and the pair propagated through seven layers: two SQL queries, two
querier entries, two dbauthz wrappers, two dbmetrics wrappers, two dbmock
methods and two MethodTestSuite subtests.

The :exec variant's one caller, revokeOAuth2CodeOnPKCEFailure, already treats
sql.ErrNoRows as success, and both dbauthz wrappers fetched the code and
authorized ActionDelete, so the swap preserves behavior. Removing the variant
also frees the plain name: no other Delete... :one query in the repo carries a
suffix naming its RETURNING clause.

scripts/dbgen reuses existing method bodies and does not drop removed methods,
so the dbmetrics wrapper needed the orphan deleted and the survivor's body
widened by hand.

…reak

The comment at the delete explained the race mechanics, which db.InTx
already implies. Replace it with the reason the row is deleted at all:
RFC 6749 §10.5 single use, spent inside the transaction so a later
failure leaves the code redeemable.

The premise worth recording is that arbitration depends on READ
COMMITTED. That belongs on the query, next to the ErrNoRows contract it
qualifies, where sqlc carries it into the generated Go for callers.

The client-side barrier only aligned the request sends. Nothing stopped one
handler from committing before the other read the code, in which case the
pre-transaction read refuses the second redemption and the delete never
arbitrates. The test then passes against an implementation that does not
enforce single use.

barrierStore holds both redemptions at that read until each has taken it.
Against the parent implementation the test now fails 10/10 on minted=2.

…h-scope

…rror descriptions

A refresh that names an unknown scope has that name quoted back in
error_description, and the name comes straight from the request form. RFC 6749
section 5.2 restricts the parameter to %x20-21 / %x23-5B / %x5D-7E, and the rule
is on the decoded value, so JSON escaping does not satisfy it. Nothing capped
the length either.

The authorize path had already solved both halves, in sanitizeErrorDescription
and in redirectAuthorizeError's cap, and the token endpoint inherited neither.
Apply them at the write rather than at each call site, so the guarantee belongs
to the endpoint instead of to whoever writes the next message.

The code_verifier message spells "section" out, since the sanitizer drops the
sign it used to carry.

A refresh that named a scope wrote the narrowed value into
oauth2_provider_app_tokens.scope, which is also the ceiling the next refresh
reads. One narrowing therefore lowered the ceiling permanently, and a client
that gave up authority for one call could not ask for a sibling permission of
the same grant afterwards, nor take the grant back, without a human at the
consent page.

The refresh row now keeps the consented grant and only the minted API key
narrows. OAuth 2.1 section 4.3.3 requires a rotated refresh token to carry the
scope of the one presented, and section 4.3 gives "previously obtained an access
token with a scope more narrow than approved by the respective grant and later
requires an access token with a different scope under the same grant" as a
reason to refresh at all. RFC 6749 section 6 bounds the request by what the
resource owner granted, which is the value this column now holds for the life of
the grant.

That column was also the only surviving record of the consent: the code row
carrying it is deleted at redemption, the API key holds current scopes only, and
oauth2_provider_app_tokens is not audited. Overwriting it left the deployment
unable to answer what the user had approved.

narrowGrantedScope becomes narrowAccessScope, and grantedScope becomes
accessScope in refreshTokenGrant, since neither is the grant any more.

errScopeNotGranted stated the ceiling but not the remedy, unlike errStaleScope
one line up. No refresh can widen a grant no matter what the client sends, so a
client reading only the ceiling has nothing to distinguish "ask differently"
from "cannot be asked for at all", and retries scope combinations that cannot
succeed. The message now ends with the only step that works.

The wording "originally granted" is left as it is. It became true again when the
refresh row stopped carrying the narrowed scope, and it matches the phrase RFC
6749 section 6 and OAuth 2.1 section 4.3.1 both use for the same ceiling.

narrowAccessScope's doc comment loses a paragraph that argued for a decision the
function does not make: the refresh row keeps the grant at the insert, and that
call site already carries the reasoning.

CRF-3: firstScopeBeyondCeiling's two conversion loops are slice.StringEnums,
already used six times in registration.go. The local allowedNames becomes
ceilingNames, which is what the parameter has been since the rename.

CRF-5: a stored grant outside the catalog answered 500 when the client named a
scope and 400 when it did not, because RBAC cannot expand such a name and the
coverage check reached it first. narrowAccessScope now validates the ceiling
before comparing, as authorizationCodeGrant validates the code's scope before
its own, so both exits answer 400 naming the scope at fault.

CRF-29: phase gains phaseAuthorize, phaseRedeem and phaseRefresh. It is a log
field operators filter on, and a typo in a literal compiles. The doc paragraph
listed two of the three values and now names none, since the constants are the
list.

CRF-30: the duplicated const block in tokens_internal_test.go is hoisted to the
file.

CRF-31: "these two" counts the conditions under it and goes stale; the sibling
comment says "these".

CRF-35: the errCoverageUndecidable wrap now says the description carries stored
grant values, since only the handlers keep it from being rendered.

CRF-36: the invalid_grant comment claimed all three sentinels report stored
values. errUnmintableScope can report a client-named scope, and is unreachable
that way only because the catalog check runs first.

CRF-37: LegacyAliasRefreshesTheSameEitherWay seeds two apps rather than two
grants on one, since nothing enforces one holder of a refreshed key's name.

Also one spelling for errUnknownScope's two call sites: negotiateScope used %q,
which emits the double quote RFC 6749 section 5.2 excludes and leaves the
sanitizer to rewrite what it just wrote.

…t returns

CRF-11: the refresh-narrowing prose sat under the invalid_grant heading and
named no error code, so a client reading invalid_scope from the token endpoint
searched the page, found the authorize-side section that opens "it redirects to
your registered callback", and left. It now has its own heading opening with the
code and status, matching its two neighbours, and the authorize-side section
points here.

CRF-17: "anything the original grant confers" is false for six of the composite
scopes. organization_member:read, provisioner_jobs:read and
template:view_insights all expand under RBAC and none is requestable, so a
client narrowing coder:workspaces.create to the fullest set it can name loses
the org member read a workspace build needs. Says so, and says to refresh
without a scope to get the composite back.

CRF-18: "a scope this deployment does not define" is the wrong cause for an
internal-only name like debug_info:read, which this deployment does define and
will never offer. Fixed in the new prose and in the authorize-side bullet that
predates this PR.

CRF-28: the Limitations list records that a scope on a refresh used to be
discarded and is now enforced, since deleting a bullet was the only trace that
a shipped endpoint tightened.

CRF-33: an application_connect row beside the all row, since the catalog check
runs before canonicalization and scopeAliases holds exactly those two.

CRF-21: RefreshCannotWidenTheScope claimed the refused refresh left the token
redeemable and proved it by reading a database row. It now redeems it. The row
read would still pass if a rejection rotated the refresh hash or moved
ExpiresAt, neither of which the old assertion could see.

CRF-22: RefreshUnknownScopeRejected asserted the reason but never the name, so
deleting the wrap that carries it passed. The catalog check runs before the
coverage check precisely to hand a client that typo'd a scope the name to fix,
and nothing tested that.

CRF-2 and CRF-23: the refresh half of ResponseStatesTheScopeGranted sent
workspace:ssh and asserted workspace:ssh, which is the one case RFC 6749
section 5.1 does not require the parameter for, so it restated its siblings
instead of testing its own comment. It now grants coder:all and refreshes with
the alias all, where the granted spelling differs from the requested one.
Returning the requested spelling instead of the canonical one now fails an
end-to-end test rather than only two unit rows.

CRF-24: requireTokenGrantError and requireTokenScopeError were the same twelve
lines with one constant changed. One requireTokenError over codersdk.OAuth2Error,
which is the type the server marshals, with both names kept as wrappers. The
sanitizer subtest declared the same struct a third time and now uses it too.

…pe refusals

CRF-15: the code grant parsed scope and discarded it, which is the defect this
PR fixes on the refresh branch. A client asking for less was handed the broader
token it tried to give up. It now narrows the same way, against the code's
scope. RFC 6749 section 4.1.3 defines no scope parameter there, so ignoring it
was defensible; accepting and discarding it is not.

CRF-13: a scope refusal on this path logged nothing, while every sibling refusal
in the package logs at Warn. A leaked token being probed for what it can be
traded up to looked the same as an ordinary client error. Both refusals now log
with the phase the PR already threads through for that purpose.

CRF-16: narrowAccessScope took the whole app but must never read app.Scope. It
takes appID, so the rule that a narrowed registration applies at the next
authorization is enforced by the signature rather than by a comment on another
function.

CRF-25: the catalog loop was a second copy of negotiateScope's, and the copy is
where the quoting diverged. One firstUnknownScope, called from both.

CRF-26 and CRF-27: the canonicalization comment stated its reason instead of
pointing at another function's, and the doc comment now carries why coverage
rather than membership, which was only in a test case.

Also the unlabelled ordering comment, already fixed in f54bbbc.

Comments throughout are cut back: RFC sections are cited rather than explained,
and test headers say what is verified rather than why the rule exists. One of
them had been orphaned onto the wrong function by an earlier commit and is back
above TestOAuth2TokenExchangeSingleUse.

Drop the notes that restate what the code already shows, and shorten the
narrowAccessScope and invalid_grant comments to the reasoning a reader
cannot get from the code.