fix: make OAuth2 code redemption single-use under concurrency - #28744

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.

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.

…e-code

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.