fix: deliver the rest of the invalid_request class to the client - #28736

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.

…-invalid-request

# Conflicts:
#	coderd/oauth2provider/authorize.go

… into plat479-4-consolidate-redirects

…-invalid-request

blamesClient asked whether any parser error named client_id, so two shapes
that leave the identity settled stayed on Coder and the client never learned
its request failed: a client_id repeated in the query, which parseSingle
collapses to "" after logging the duplicate, and a POST carrying client_id in
the form body, which httpmw resolves and this parser never reads.

clientIDInDoubt reads the raw values and compares parsed UUIDs against the app
httpmw resolved. A repeated client_id still stays here, since the callback was
matched against one of several candidates. TestCarveOutDelivery pins both
directions; deleting or inverting the carve-out now fails.

… the callback

newAuthorizeResponse decided the redirect URI had failed by counting errors
across a span, and the span included the state read. A repeated state therefore
nilled the callback, so a request whose redirect_uri exact-matched the
registration still answered 400 here and the app waited on an authorization
that never arrived.

The condition now names the field it means.

RFC 6749 §3.1 and OAuth 2.1 §3.1 both say the authorization server MUST ignore
unrecognized request parameters. ErrorExcessParams rejected them instead, and
since this PR delivers rejections to the callback, an OIDC client sending nonce
or prompt received a spec-shaped invalid_request asserting its request was
malformed when it was not.

The names are logged at debug instead, so a misspelled parameter is still
visible to an operator. Repeats of the parameters this endpoint does read are
still rejected, by parseSingle.

…error code

response_type was parsed through the SDK enum, so a value with a Go constant
(token) answered unsupported_response_type while id_token or a typo answered
invalid_request. Read it as plain text instead: the client made one mistake and
now gets one code for it.

…ource

The token endpoint already answers invalid_target (RFC 8707 §2) for a resource
the same validator rejects; authorize flattened it into invalid_request, which a
client cannot retry. Carry an error code on the failure and use invalid_target
when resource is the only field that failed.

The description joined Go struct dumps with commas, and details contain commas,
so the client could not split it back into per-field diagnostics. Join
"field: detail" with "; " instead.

Its length was also the client's to choose, and it reaches both a Location
header and an Info log. Cap it at redirectAuthorizeError, which covers the
invalid_scope path too.

The reference listed a parameter set that fails: code_challenge is required for
every response_type=code request but was undocumented, and now that failures
redirect the client would see nothing on Coder. state was marked required and is
not (OAuth 2.1 §4.1.1 makes it OPTIONAL). Also document code_challenge_method,
whose S256 default inverts the spec default, and resource.

… endpoint

The invalid_request section listed two causes that no longer produce it: an
unparseable response_type now answers unsupported_response_type, and an
unrecognized parameter is ignored. A malformed resource gets its own
invalid_target section. The client_id bullet describes the identity check that
is actually run.

…t' into plat479-5-deliver-invalid-request

# Conflicts:
#	coderd/oauth2provider/tokens_internal_test.go

The three answers an authorization failure can get were spelled as
independent fields, so both handlers re-derived the precedence by hand and
either could be reordered without the suite noticing. kind() states it once
and both handlers switch on it.

Fold the registered callback's url.Parse into newAuthorizeResponse so a
callback that does not parse joins the class the type already claims to
represent. That removes the two pre-parse branches, which had no coverage:
GET rendered the raw Go parse error, carrying the stored URL, into the
browser, and POST answered "Failed to validate query parameters" for a
failure that read no query parameter at all. Both now say the callback is
not usable, which is true of either cause.

…it returns

String made authorizeResponse an implicit fmt.Stringer, and it dereferences
the callback unconditionally. The zero value is now routine on failure paths,
so a %v on one prints %!v(PANIC=...) instead of the state, at exactly the
moment someone is debugging why a request did not redirect.

message said it joins the parser's errors "for the response body", but both
delivery paths put it in the error_description query parameter. Rename it to
description, which also reads as a pair with the code field next to it.

The type doc called the value "a request that did not parse", which its own
corruptCallback field contradicts. Drop canRedirect's comment, which restated
its one-line body.

…tion says

Both callers passed invalid_request and asserted nothing else, so a missing
code_challenge and a malformed one were indistinguishable: collapsing the whole
parse stage into one blanket code would have left both green. Take the
description too.

Rename with it. "ExpectingError" described a status code; the helper requires a
redirect.

Both verbs answer 400 for the RFC 6749 4.1.2.1 carve-outs and 500 for an
unusable registered callback, and the reference listed neither. A redirect_uri
mismatch is the most common integration mistake there is, and the admin guide
already covers it, so the two artifacts disagreed about whether the status
exists.

POST declares Produce json so its error body renders as the JSON it is, which
also documents codersdk.OAuth2Error for the first time.

The consent page section named only the blocked-scheme cause. The same page,
and server_error on POST /oauth2/authorize, now also answer a stored callback
that does not parse.