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>

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.

…ated-scope

# Conflicts:
#	coderd/oauth2provider/authorize_test.go

Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>

…og check

…tiated-scope

# Conflicts:
#	coderd/oauth2provider/authorize.go
#	coderd/oauth2provider/authorize_test.go
#	coderd/rbac/scopes_internal_test.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.