iframe-proxy

BobbyHo · 2026-08-11T23:10:28Z

Phase 2 of PLAT-470, tracked as PLAT-479. Builds on #28007, merged.

What this is. The authorize endpoint parses scope and then discards it, so an app's configured allowlist has never restricted anything and a client asking for more than it should get is never told no. Phase 1 added the columns that carry a negotiated scope from a code to the token it becomes, but nothing writes one, so every code is stamped unrestricted. This PR makes the authorize step negotiate and persist the result.

The scenario it covers. A CI bot registered through dynamic client registration with scope: "coder:workspaces.access" that only needs to SSH into a workspace:

Asking for workspace:ssh issues a code carrying exactly that, instead of an unrestricted one. It no longer has to request the broader composite to get any token at all.
Asking for template:update, which its allowlist never permitted, is rejected before the consent page renders, instead of quietly issuing an unrestricted code.
Asking for nothing gets the app's filtered allowlist, per RFC 6749 section 3.3.

What changes

Requested names are checked against the external scope catalog, and the app's stored allowlist is filtered through that same catalog.
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.
The negotiated value is persisted on the authorization code, replacing phase 1's placeholder.
Both handlers negotiate, so a request that cannot succeed fails before the consent page renders rather than after the user clicks Allow.
The consent page lists what was negotiated. An unrestricted grant still reads as full access, since the scope name for it tells a user less than the sentence does.
Invalid scope now redirects to the client's callback with the error, its description, and state, per RFC 6749 section 4.1.2.1, instead of answering on Coder. That is safe here specifically: the redirect URI is exact-matched against the registered callback before the scope check runs. Other error paths are unchanged, since several of them are where that validation fails.
Two paths produce an empty result and are deliberately not the same path. No allowlist and no request keeps today's 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.

Accepted compatibility break. Dynamic client registration performs no catalog validation, so apps registered with scopes such as openid, read, or admin hold allowlists this server cannot grant from. They now fail authorization in both directions: requesting what they registered is rejected, and omitting scope hits the filtered-to-empty rejection. 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 population is bounded to DCR apps that sent a scope, and the failure is immediate, arriving at the app's own callback with a description naming the registered scopes and the remedy.

What this does not change

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 PR changes which authorization requests succeed, not what a token can do.
The allowlist is reachable only through dynamic client registration. The admin create and update APIs carry no scope field, so an admin-created app always takes the no-allowlist path. Giving the admin API one is its own change.

Applying the negotiated scope at token exchange, refresh narrowing, and the docs update follow as separate PRs.

Where this sits in the scope pipeline (green marks what this PR touches)

flowchart TD
    subgraph authorize["/oauth2/authorize"]
        AZ1["GET: negotiates, then lists<br/>the scope on the consent page"]
        AZ2["POST: negotiates, then issues the code"]
        V["scope negotiation<br/>catalog check + permission coverage"]
        AZ1 --> V
        AZ2 --> V
    end

    APP[("apps.scope<br/>the allowlist, read as input")]
    APP --> V
    V --> CODES[("codes.scope<br/>negotiated value, was a placeholder")]

    subgraph codegrant["POST /oauth2/tokens, authorization_code"]
        G1["token exchange<br/>still mints an unrestricted token"]
    end

    CODES --> G1
    G1 --> TOKENS[("tokens.scope")]

    subgraph enforce["Every authenticated API request"]
        E1["extract API key"] --> E2["scope set"] --> E3["RBAC subject"] --> E4["authorize"]
    end

    TOKENS --> E1

    classDef changed fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,color:#1b3c1e
    class AZ1,AZ2,V,CODES changed

The grant path and the enforcement engine below it are untouched. They already read a key's scopes correctly and are waiting on real data, which the next PR feeds them.

How to review this PR

coderd/oauth2provider/authorize.go is the whole behavior change. Read what "no allowlist" means first, since it defines the branches, then the negotiation, then its two call sites. The catalog check runs on the request before any allowlist logic, so an internal-only name is rejected whether or not the app has an allowlist. That check is a curation, not a validity check: RBAC expands such names fine, which is why the catalog is narrower than both RBAC and the enum.
coderd/rbac/scopes.go decides coverage by expanding both sides and comparing permissions. It is asymmetric on purpose. On the requested side, org or user permissions, a negative permission, and a resource allow list are all errors, since answering "covered" about authority that was never compared is the failure that matters. On the allowed side, negatives and allow lists are errors too, because ignoring an anti-grant would widen the ceiling rather than narrow it. What is dropped there is org and user permissions, which can only make the answer stricter.
The rejection path, and the test asserting an unregistered redirect URI still fails on Coder with no Location on both verbs. That ordering is what keeps the new redirect out of a request's reach.
site/site.go and site/static/oauth2allow.html carry the consent page list, with the unrestricted grant branching back to the original wording.
Tests. The internal table covers each branch plus the contract the signature cannot express: a rejection returns empty and a success never does, because the value goes to a non-empty CHECK column. The HTTP-level tests run against a real database and assert the persisted column by parsing the issued code out of the redirect rather than inferring the grant. DCR compatibility is pinned in executable form, registered through DCR because that is the only route producing a non-catalog allowlist naturally.

Verified locally against Postgres: the full oauth2provider package, rbac, mcp, and coderd's OAuth2 suites. make pre-commit passes, including the generated-file drift check.

Manual Tests

Verified by hand against a local dev deployment (v2.36.0-devel+ab90213137, dev Postgres), in addition to the automated suite. The negotiated scope is not exposed by any API, so each scenario asserts the persisted oauth2_provider_app_codes.scope directly.

Two apps stand in for the two allowlist states: plat470-admin-app, created through the admin API so its scope column is NULL, and plat470-ci-bot, registered through DCR with scope: "coder:workspaces.access". DCR is the only route that can set an allowlist, since the admin create and update APIs carry no scope field.

No correctness defects found. Details, commands, and captured output for each scenario below.

Open item, non-blocking. Scenario 8 renders correctly but the scope list has no visual affordance marking it as a list. #scope-list sets list-style: none and inherits the centered body text, so the permission names appear as two plain centered lines directly under the prompt, with no bullets, indentation, or label. Visually they read as a continuation of the sentence above rather than as the enumerated grant the user is approving. The screen-reader side is handled correctly, which is what the explicit role="list" and role="listitem" are for, so this affects sighted users only. Left-aligning the items, indenting them, or giving the group a short "Permissions" heading would each make the grant scannable. Raising it because the consent screen is the one place a user decides what to hand over, so the presentation seems worth a deliberate choice rather than an inherited default.

Shell helpers used throughout

export BASE_URL=http://localhost:3000
export AUTH_HEADER="Coder-Session-Token: $(cat ./.coderv2/session)"
export PGPASSWORD=$(cat ./.coderv2/postgres/password)
export PGPORT=$(cat ./.coderv2/postgres/port)

# The assertion target: no API returns this column.
code_scope() {
  psql -h localhost -p "$PGPORT" -U coder -d coder -tAc \
    "SELECT scope FROM oauth2_provider_app_codes
     WHERE app_id = '$1' ORDER BY created_at DESC LIMIT 1;"
}

urlenc() { jq -rn --arg v "$1" '$v|@uri'; }

new_pkce() {
  VERIFIER=$(openssl rand 32 | base64 | tr -d '\n=' | tr '+/' '-_')
  CHALLENGE=$(printf '%s' "$VERIFIER" | openssl dgst -sha256 -binary \
    | base64 | tr -d "=" | tr '+/' '-_')
  STATE=$(openssl rand -hex 16)
}

# $1=client_id, $2=scope (empty to omit), $3=redirect_uri
authz_url() {
  local url="$BASE_URL/oauth2/authorize?client_id=$1&response_type=code"
  url="$url&redirect_uri=$(urlenc "$3")&state=$STATE"
  url="$url&code_challenge=$CHALLENGE&code_challenge_method=S256"
  if [ -n "$2" ]; then url="$url&scope=$(urlenc "$2")"; fi
  printf '%s' "$url"
}

# Prints "<status> <redirect target>".
authz_post() {
  new_pkce
  curl -s -o /dev/null -X POST "$(authz_url "$1" "$2" "$3")" \
    -H "$AUTH_HEADER" -w '%{http_code} %{redirect_url}\n'
}
authz_get() {
  new_pkce
  curl -s -o /dev/null "$(authz_url "$1" "$2" "$3")" \
    -H "$AUTH_HEADER" -w '%{http_code} %{redirect_url}\n'
}

Note for anyone reusing these: generate the PKCE verifier by base64url-encoding the raw 32 bytes as above. The common openssl rand -base64 32 | tr -d "=+/" | cut -c -43 recipe usually yields fewer than 43 characters, which authorize accepts but the token endpoint rejects under RFC 7636 section 4.1.

1. No allowlist, no request, stays unrestricted

The compatibility floor: apps that predate scope enforcement must behave exactly as before. noScopeAllowlist is true and no scope was requested, so the code is stamped with the explicit coder:all sentinel rather than an empty string, which the column's CHECK (scope <> '') would reject.

curl -s -X POST "$BASE_URL/api/v2/oauth2-provider/apps" -H "$AUTH_HEADER" \
  -H "Content-Type: application/json" \
  -d '{"name":"plat470-admin-app","callback_url":"http://localhost:9876/callback"}'

psql ... -tAc "SELECT COALESCE(scope::text,'<NULL>') FROM oauth2_provider_apps WHERE id='$ADMIN_APP_ID';"
authz_post "$ADMIN_APP_ID" "" "http://localhost:9876/callback"
code_scope "$ADMIN_APP_ID"

<NULL>
302 http://localhost:9876/callback?code=coder_7fHdyq7yYr_...&state=e3f327dbd8b81c0aacdbd9b8bb82acb0
coder:all

The app's scope column is NULL, confirming the admin API cannot set an allowlist, and the pre-enforcement grant is preserved.

2. Request narrower than the allowlist is granted by coverage, not name matching

The core semantic claim. workspace:ssh never appears in the allowlist, but coder:workspaces.access expands to a permission set that already includes it, so ScopesCover approves it. Under name membership this client's only route to a token would be to request the broader composite.

authz_post "$CI_APP_ID" "workspace:ssh" "http://localhost:9876/callback"
code_scope "$CI_APP_ID"

302 http://localhost:9876/callback?code=coder_1Qxc34c6eU_...&state=44272ff110823c273155412e18acb3c1
workspace:ssh

The persisted value is what was requested and granted, not the ceiling it was checked against.

3. Request outside the allowlist is refused, including a prefix-shaped one

coder:workspaces.access grants workspace:{read,ssh,application_connect} and never delete, so a scope sharing the resource prefix with three covered scopes is still refused. Coverage is a real permission comparison, not a prefix match.

authz_post "$CI_APP_ID" "workspace:delete" "http://localhost:9876/callback"
code_scope "$CI_APP_ID"

302 http://localhost:9876/callback?error=invalid_scope&error_description=%22workspace%3Adelete%22%3A+scope+is+not+in+this+app%27s+allowed+scope+list&state=aa8bcb2dafe0b27654fb9ca03b59793b
workspace:ssh

Decoded: "workspace:delete": scope is not in this app's allowed scope list. The persisted value is unchanged from scenario 2, confirming no code was written.

4. Omitted scope defaults to the app's allowlist

authz_post "$CI_APP_ID" "" "http://localhost:9876/callback"
code_scope "$CI_APP_ID"

302 http://localhost:9876/callback?code=coder_OVQFIZ2MtE_...&state=24f8edb8f11d13a6716c2b3e09100a45
coder:workspaces.access

Contrast with scenario 1, where an absent request against an absent allowlist yielded coder:all. The two empty-input paths stay distinct, which is the point of not unifying them.

5. Rejection is delivered to the client's own callback, on both verbs

template:update is a valid catalog scope, refused only because this app's ceiling does not cover it. The refusal redirects to the registered callback with all three RFC 6749 section 4.1.2.1 parameters, rather than rendering on Coder where the client's error handling never runs.

authz_post "$CI_APP_ID" "template:update" "http://localhost:9876/callback"
echo "sent state: $STATE"
code_scope "$CI_APP_ID"

# GET rejects before the consent page renders, so the user is never shown
# a page for a request that cannot succeed.
authz_get "$CI_APP_ID" "template:update" "http://localhost:9876/callback"

302 http://localhost:9876/callback?error=invalid_scope&error_description=%22template%3Aupdate%22%3A+scope+is+not+in+this+app%27s+allowed+scope+list&state=adc691370029248670561f1e67aacb62
sent state: adc691370029248670561f1e67aacb62
coder:workspaces.access

302 http://localhost:9876/callback?error=invalid_scope&error_description=...&state=5e47695b50264e0b73c3966846ad0b59

The state echoed back is byte-identical to the one sent, the persisted scope is unchanged, and the GET side returns 302 rather than 200 with consent HTML.

Here is the same rejection as an app author sees it, landing at a callback that renders the parameters:

_{template:update refused. The browser ends at the app's own registered callback, carrying error, error_description, and the original state. The receiver on port 9876 is a throwaway script that renders whatever parameters arrive.}

6. Names outside the external catalog are rejected

Two failure shapes share this branch. openid is unrecognized entirely. debug_info:read is recognized by RBAC and storable by the enum, but deliberately excluded from the curated catalog, which is what makes the catalog a curation rather than a validity check.

authz_post "$CI_APP_ID" "openid" "http://localhost:9876/callback"
authz_post "$CI_APP_ID" "debug_info:read" "http://localhost:9876/callback"
# The check runs before allowlist logic, so it fires on a no-allowlist app too.
authz_post "$ADMIN_APP_ID" "openid" "http://localhost:9876/callback"

302 ...error_description=%22openid%22%3A+unknown+or+unsupported+scope&state=cd34c887f01640b1daf23692fc8f682d
302 ...error_description=%22debug_info%3Aread%22%3A+unknown+or+unsupported+scope&state=25d37f076c00dedd3d0998dc1b5dd3fc
302 ...error_description=%22openid%22%3A+unknown+or+unsupported+scope&state=449e84924dcc736fe8fbaa7ecc084be3

The third call is the ordering check: an app with no allowlist still rejects the name rather than accepting the request verbatim.

7. Canonicalization and deduplication before persisting

IsExternalScope accepts the aliases all and application_connect, which are not members of the api_key_scope enum, so persisting a validated name verbatim would write a value outside the column's vocabulary.

authz_post "$ADMIN_APP_ID" "all" "http://localhost:9876/callback"
code_scope "$ADMIN_APP_ID"

authz_post "$CI_APP_ID" "workspace:ssh workspace:ssh" "http://localhost:9876/callback"
code_scope "$CI_APP_ID"

coder:all
workspace:ssh

The alias was accepted at the door and stored as the enum spelling, and the duplicated request collapsed to a single name, keeping the stored value set-valued.

8. Consent page states the negotiated scope

new_pkce
curl -s "$(authz_url "$CI_APP_ID" "workspace:ssh workspace:read" "http://localhost:9876/callback")" \
  -H "$AUTH_HEADER" | grep -A 4 '<ul id="scope-list"'

      <ul id="scope-list" role="list">
        <li role="listitem">workspace:ssh</li>
        <li role="listitem">workspace:read</li>
      </ul>

_{Consent page for a DCR app whose allowlist is coder:workspaces.access, with a negotiated scope of workspace:ssh workspace:read.}

An app with no allowlist renders zero <ul id="scope-list"> elements and keeps the original wording:

_{Consent page for an admin-created app, which has no allowlist. Original full-access wording, no list.}

consentScopes checks for presence rather than sole occupancy, so an allowlist of coder:all coder:workspaces.access also renders as full access, which is accurate, while the persisted value still records both names:

authz_post "$BOTH_APP_ID" "" "http://localhost:9876/callback" > /dev/null
code_scope "$BOTH_APP_ID"

coder:all coder:workspaces.access

9. DCR compatibility break behaves as designed

An app registered with openid read now fails in both directions. Registration itself still succeeds, so the break surfaces at authorization time with a message naming the pre-filter registered list, which is what the owner has to change.

curl -s -X POST "$BASE_URL/oauth2/register" -H "Content-Type: application/json" \
  -d '{"client_name":"plat470-legacy-app","redirect_uris":["http://localhost:9876/callback"],"scope":"openid read"}'

authz_post "$LEGACY_APP_ID" "openid" "http://localhost:9876/callback"
authz_post "$LEGACY_APP_ID" "" "http://localhost:9876/callback"

201 Created, scope: "openid read"

302 ...error_description=%22openid%22%3A+unknown+or+unsupported+scope
302 ...error_description=%22openid+read%22%3A+none+of+the+scopes+registered+for+this+app+are+supported+by+this+deployment%3B+re-register+the+app+with+supported+scopes

Two adjacent cases were checked as well. A partially stale allowlist keeps its usable entries, since filtering only ever narrows:

# registered scope: "openid workspace:read"
authz_post "$MIXED_APP_ID" "" "http://localhost:9876/callback"
code_scope "$MIXED_APP_ID"

302 http://localhost:9876/callback?code=coder_1AcK45UMn1_...
workspace:read

And a whitespace-only allowlist is treated as configured-but-empty rather than absent, so it rejects instead of falling back to unrestricted. DCR stores the literal " ":

authz_post "$SPACE_APP_ID" "" "http://localhost:9876/callback"

302 ...error_description=%22%22%3A+none+of+the+scopes+registered+for+this+app+are+supported+by+this+deployment%3B+...

That message names "" rather than " ", because strings.Fields collapses the value before the error is built. The remedy sentence still reads correctly, so this is noted rather than raised.

10. An unregistered redirect_uri is never redirected to

The ordering that makes scenario 5's redirect safe. If it ever inverted, an attacker-supplied redirect_uri plus a deliberately invalid scope would turn the authorize endpoint into an open redirect.

new_pkce
curl -s -o /dev/null -D - -X POST \
  "$(authz_url "$CI_APP_ID" "template:update" "http://evil.example/steal")" \
  -H "$AUTH_HEADER" | grep -iE '^HTTP/|^location:'
# same again without -X POST for the GET side

HTTP/1.1 400 Bad Request
HTTP/1.1 400 Bad Request

The absence is the result: no Location header on either verb. The body confirms the request died at redirect-URI validation, before the scope check ran:

{"error":"invalid_request","error_description":"Invalid query params: field: redirect_uri detail: Query param \"redirect_uri\" must exactly match http://localhost:9876/callback"}

The error names redirect_uri, not the scope, even though the request carried a scope this app is not allowed.

11. Issued token is still unrestricted, confirming the phase boundary

Recorded deliberately as a before-state for the enforcement PR. The exchange copies the negotiated scope onto the token row, but the API key it mints still carries coder:all.

# exchange a code negotiated as workspace:ssh
curl -s -X POST "$BASE_URL/oauth2/tokens" -H "Content-Type: application/x-www-form-urlencoded" \
  -d "grant_type=authorization_code" -d "code=$CODE" -d "client_id=$CI_APP_ID" \
  -d "client_secret=$CI_CLIENT_SECRET" -d "redirect_uri=http://localhost:9876/callback" \
  -d "code_verifier=$VERIFIER"

psql ... -c "SELECT t.scope AS token_scope, k.scopes AS api_key_scopes
             FROM oauth2_provider_app_tokens t JOIN api_keys k ON k.id = t.api_key_id
             WHERE t.app_id = '$CI_APP_ID' ORDER BY t.created_at DESC LIMIT 1;"

{"token_type":"Bearer","expires_in":86399,"has_access_token":true}

  token_scope  | api_key_scopes
---------------+----------------
 workspace:ssh | {coder:all}

The exchange does not break on a non-coder:all code, which is the compatibility risk phase 1's columns introduced. Using that token against endpoints far outside workspace:ssh:

GET /api/v2/templates -> 200
GET /api/v2/deployment/config -> 200

A token negotiated as workspace:ssh reads the deployment config, because enforcement reads api_keys.scopes, which is still {coder:all}. Rerunning these two calls after the enforcement PR should turn both into 403.

Automated equivalents

Every scenario above has automated coverage. Run together:

make test RUN='TestValidateRequestedScope|TestNoScopeAllowlist|TestConsentScopes|TestOAuth2AuthorizeScopeNegotiation|TestOAuth2AuthorizeDCRScopeCompatibility|TestOAuthConsentFormStatesNegotiatedScope|TestScopesCover|TestOAuth2ClientScopeValidation'

ok  github.com/coder/coder/v2/coderd                 15.395s
ok  github.com/coder/coder/v2/coderd/oauth2provider  20.189s
ok  github.com/coder/coder/v2/coderd/rbac             1.529s

Exit 0, zero failures. What the manual run adds on top: the exact error_description text a client receives, the rendered consent page including its accessibility attributes, and the scenario 11 pairing that pins this phase's boundary in a directly re-runnable form.

Migration 000567 adds a nullable `scope text` to oauth2_provider_app_codes and oauth2_provider_app_tokens so the scope negotiated at /oauth2/authorize can travel from a code to the token it is exchanged for. No backfill, and every insert writes NULL for now, which reads as unrestricted access, so behavior is unchanged. DeleteOAuth2ProviderAppCodeByIDReturningID and DeleteAPIKeyByIDReturningID return sql.ErrNoRows when the row is already gone, letting the grant paths enforce single use without a read-then-write race. The existing blind deletes and their call sites are unchanged. Refs PLAT-478

Both scope columns were nullable with NULL meaning "unrestricted", which made the most privileged state the one a forgotten field produces: sql.NullString{} is NULL is full access, and exhaustruct is satisfied by exactly that literal. An audit of either table could not separate a deliberate legacy grant from a mint path that dropped the scope. Backfill both columns to coder:all, which records what existing rows already have in fact since apikey.Generate defaults minted OAuth2 keys to that scope, then apply NOT NULL and CHECK (scope <> ''). NOT NULL alone would not be enough: sqlc maps text NOT NULL to a Go string whose zero value inserts cleanly, so the fail-closed property needs both clauses. No DEFAULT survives, or an INSERT omitting the column would silently receive an unrestricted grant. Matches the encoding api_keys.scopes and workspace_agents.api_key_scope already use, and follows migration 000389's backfill-then-constrain shape. The two grant paths now carry the parent's scope forward (Scope: dbCode.Scope, Scope: dbToken.Scope) instead of hardcoding an empty value, which is RFC 6749 section 6's default and removes the phase-ordering hazard where a scoped token could refresh into an unrestricted one. ProcessAuthorize writes the sentinel, since persisting a requested scope before validation exists would store unvalidated client input. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…etes Both single-use deletes returned a bare id, which forced a hand-written dbauthz wrapper each. Returning the whole row lets them collapse into the existing fetchAndQuery generic, since that helper unifies its fetch and query on one rbac.Objecter and a bare id satisfies no such interface. Each 10-line wrapper becomes a single call, and a caller now reads the deleted row's state, including a code's negotiated scope, from the same atomic delete rather than trusting an earlier unauthorized read. Renamed to ...ByIDReturningRow, since ...ReturningID no longer describes them. Add TestSingleUseDeleteByIDReturningRow, which pins the contract both queries exist for: the first delete returns the row, a second returns sql.ErrNoRows. Neither query previously executed against a real database on its already-gone path, so converting one back to :exec or adding a soft delete would have broken single use with CI still green. The concurrent exactly-one-winner half is deliberately not covered here; it exercises Postgres row-lock semantics rather than this code. Rename migration 000567 to oauth2_scope_columns. It adds columns and constraints; enforcement lands in a later phase, and migration names freeze at merge. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…000569 origin/main merged 000567_chat_file_purge_indexes and 000568_service_account_notifications after this branch's point. CI validates the PR merge, where two files numbered 000567 coexisted and the migrate iofs driver panicked with "duplicate migration file", taking down gen, lint, sqlc-vet and every test-go-pg job. Git reports the merge as MERGEABLE because the two are different filenames; the collision is on the version number, which git cannot see. Renumbered with ./coderd/database/migrations/fix_migration_numbers.sh. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Two test sites built InsertOAuth2ProviderAppCodeParams and InsertOAuth2ProviderAppTokenParams without Scope, so after the columns became NOT NULL with CHECK (scope <> '') they inserted an empty string and tripped the constraint. Broke TestOAuth2ProviderTokenExchange/ExpiredCode and every TestOAuth2ProviderTokenRefresh subtest on the Linux postgres jobs. exhaustruct is disabled for _test.go (.golangci.yaml:222), so nothing forces the field in tests and the constraint is the only backstop. Audited every remaining InsertOAuth2ProviderApp{Code,Token}Params literal in the tree; these two were the only omissions, and no raw SQL inserts bypass sqlc. Refs PLAT-478 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

/oauth2/authorize ignored the scope parameter entirely and wrote a hardcoded coder:all onto every authorization code. It now negotiates: each requested scope must be in the external scope catalog (rbac.IsExternalScope), and the result must fall within the app's configured allowlist, which is itself filtered through the same catalog. An omitted scope defaults to the filtered allowlist per RFC 6749 section 3.3. The negotiated value is persisted on oauth2_provider_app_codes.scope, replacing the placeholder written when the column was added. Both GET and POST validate, so a request that cannot succeed is rejected before the consent page renders rather than after the user clicks Allow. This matches how the handler already treats PKCE's code_challenge requirement. Two cases produce an empty result and are handled deliberately differently. An app with no allowlist and no requested scope keeps today's unrestricted grant, spelled as the explicit coder:all sentinel because the column is NOT NULL with a non-empty CHECK. An app whose allowlist filters to nothing is rejected instead, since falling back there would grant strictly more than the allowlist ever permitted. NULL and the empty string are one "no allowlist configured" state, unified in a single predicate now that reading the column is an authorization decision. Accepted compatibility break: dynamic client registration performs no catalog validation, so apps registered with scopes such as openid or admin hold allowlists this server cannot grant from. Those apps now fail authorization in both directions with invalid_scope. Grandfathering unknown names through would seed api_keys.scopes with values dbauthz cannot evaluate, trading a visible negotiation-time error for a silent enforcement-time hole. Registration-time expectations are unchanged; the two tests asserting registration accepts these values carried comments promising the opposite of what authorization does, and those were corrected. Issued tokens are not yet restricted: authorizationCodeGrant still mints rbac.ScopeAll and does not read the persisted column. That lands with the grant path. Refs PLAT-479

…lat-470' into coder-oauth2-scope-enforcement-plat-470-phrase-2

linear-code · 2026-08-11T23:10:31Z

PLAT-470

BobbyHo · 2026-08-12T01:23:15Z

/coder-agents-review

coder-agents-review · 2026-08-12T01:23:19Z

Chat: Review posted | View chat
Requested: 2026-08-13 16:30 UTC by @BobbyHo

Review history

R1 (2026-08-12): 21 reviewers, 5 Nit, 6 Note, 5 P2, 6 P3, REQUEST_CHANGES. Review
R2 (2026-08-13), 5 Nit, 6 Note, 5 P2, 6 P3, COMMENT. Review
R3 (2026-08-13): 23 reviewers, 11 Nit, 10 Note, 5 P2, 8 P3, 1 P4, COMMENT. Review

deep-review v0.9.0 | Round 3 | 02076e1..bcd9e9f

Last posted: Round 3, 35 findings (5 P2, 8 P3, 1 P4, 11 Nit, 10 Note), COMMENT. Review

Finding inventory

Findings

#	Sev	Status	Location	Summary	Round	Reviewer	Posted
CRF-1	P2	Author fixed (`c15059f`)	authorize.go:74	Bare `all`/`application_connect` aliases persist verbatim; violate documented api_key_scope vocabulary and Phase 3's enum insert	R1	Mafuuu P2, Razor P2, Kurapika P3, Melody P3	Yes
CRF-2	P2	Author fixed (`c15059f`)	authorize_internal_test.go:184	Rejection tests assert only `require.Error`; three distinct error paths indistinguishable	R1	Chopper P2	Yes
CRF-3	P2	Author fixed (`ab78087`)	authorize_test.go:82	Plan-doc labels (AC*, Edge Case, §4.2.2) reference identifiers not in the repo	R1	Gon P2	Yes
CRF-4	P2	Author fixed (`ab78087`)	authorize_internal_test.go:65	Same plan-doc labels in the internal test file	R1	Gon P2	Yes
CRF-5	P2	Author fixed (`bcd9e9f`)	authorize.go:284	Consent page renders without displaying the negotiated scope	R1	Knov P2, Mafuuu P3, Kurapika P4, Meruem Note, Pariston Note	Yes
CRF-6	P3	Author fixed (`339714c`)	authorize.go:338	`invalid_scope` returned as JSON body, not `redirect_uri?error=invalid_scope&state=...` per RFC 6749 §4.1.2.1	R1	Chopper P3, Mafuuu P3, Razor Note, Knov Note, Melody Note, Hisoka Note	Yes
CRF-7	P3	Author fixed (`a290850`, `339714c`)	authorize.go:246	"Invalid Scope" page description misdiagnoses the empty-request/no-grantable-allowlist failure modes	R1	Mafuuu P3, Leorio P3	Yes
CRF-8	P3	Author fixed (`a290850`)	authorize.go:95	Filter-to-empty rejection message insufficient for the DCR compat break; does not name entries or the remedy	R1	Chopper P3, Leorio Nit	Yes
CRF-9	P3	Deferred (PLAT-503)	registration.go:111	DCR registration accepts non-catalog scopes without validation; fix at write boundary collapses two read-side patches	R1	Ryosuke P3, Pariston P3	Yes
CRF-10	P3	Author fixed (`ab78087`)	coderd/oauth2.go:123	Swagger `@Param scope` still reads "Token scopes (currently ignored)"; contradicts shipped behavior in API docs	R1	Ryosuke P3	Yes
CRF-11	P3	Author fixed (`a290850`, `339714c`)	authorize_test.go:317	`requireInvalidScope` duplicates `oauth2providertest.RequireOAuth2Error`	R1	Zoro P3, Robin Nit	Yes
CRF-12	Nit	Author fixed (`c15059f`)	authorize.go:74	Persisted scope not deduplicated; `strings.Join(requested, " ")` preserves duplicates verbatim	R1	Mafuuu Nit, Bisky Note	Yes
CRF-13	Nit	Author fixed (`ab78087`)	authorize.go:26	Doc comment overstates no-allowlist branch; per-branch return contract would be clearer	R1	Leorio Nit	Yes
CRF-14	Nit	Author fixed (`ab78087`)	authorize.go:237	Comment claims check is "inside extractAuthorizeParams"; call actually sits after it in both handlers	R1	Meruem Nit	Yes
CRF-15	Nit	Author accepted R2 (duplication contained to one test file, no third caller yet)	authorize_test.go:262	`authorizeRequest` retraces `oauth2providertest.doAuthorizeRequest`; extend the helper instead	R1	Robin Nit	Yes
CRF-16	Nit	Author contested; panel closed R3 (5/5 accept: loud drift failure is intentional, no new evidence)	authorize_internal_test.go:15	Duplicated catalog-membership scope constants across the two new test files	R1	Knov Nit	Yes
CRF-17	Note	Author contested; panel closed R3 (5/5 accept: name is load-bearing across CRF-9's design, no new evidence)	authorize.go:39	`noScopeAllowlist` is a single-use abstraction; docstring does the work a branch label could	R1	Luffy Note	Yes
CRF-18	Note	Author fixed (`32275ac`)	authorize.go:104	Literal-name subset check rejects semantic narrowing across catalog hierarchy (composite vs member)	R1	Meruem Note	Yes
CRF-19	Note	Author accepted R2 (admin API Scope field is its own change; PR description now states the design explicitly)	apps.go:102	Admin-created apps hardcode `sql.NullString{}`; scope allowlist reachable only via DCR	R1	Mafuuu Note, Melody Note, Ryosuke Note	Yes
CRF-20	Note	Author accepted R2 (PLAT-480 linked as the next phase where authorizationCodeGrant will read the column)	authorize.go:378	Persisted `grantedScope` has no reader yet; `authorizationCodeGrant` still mints `rbac.ScopeAll`	R1	Hisoka Note, Pariston Note, Ryosuke Note	Yes
CRF-21	Note	Author fixed (`bcd9e9f`)	authorize.go:241	`validateRequestedScope` called twice per request across GET and POST; first call discards its result	R1	Meruem Note	Yes
CRF-22	Note	Author accepted R2 (consolidation is a separate change that should own the whole file)	validation_test.go:544	`TestOAuth2ClientScopeValidation` lives in two near-duplicate files; PR now writes the same comment twice	R1	Razor Note, Robin Note, Zoro Note	Yes
CRF-23	Note	Dropped by orchestrator (context, not finding: regression guard for intended non-change)	authorize_test.go:135	`NoAllowlistStaysUnrestricted` passes on base	R1	Netero Note	No
CRF-24	Note	Dropped by orchestrator (context, not finding: regression guard for intended non-change)	authorize_test.go:148	`NullAndEmptyAllowlistBehaveIdentically` passes on base	R1	Netero Note	No
CRF-25	P3	Open	coderd/oauth2provider/SCOPES.md:160	"Token exchange" section claims negotiated scope is copied to the API key; the API key half is still `coder:all` in this PR (only the refresh-token row copies dbCode.Scope)	R3	Netero P3, Mafu-san Note	Yes
CRF-26	P3	Open	authorize.go:440	RFC 6749 §4.1.2.1 class not fully closed by CRF-6 fix: 5 sibling error sites (POST `unsupported_response_type` :440, POST `invalid_request` PKCE :452, POST `server_error` GenerateSecret :465, POST `server_error` InTx :511, GET `unsupported_response_type` :349) still respond on Coder rather than redirecting to the app's callback	R3	Hisoka P3, Chopper P4, Melody P4, Ryosuke Note	Yes
CRF-27	P4	Open	coderd/rbac/scopes.go:359	`ScopesCover` allowed-side ignores `Negate` permissions asymmetrically vs the requested-side guard; docstring's "can only make the answer stricter" is wrong for a hypothetical Negate perm; live consequence is zero today, class fix is 3 lines matching the requested-side pattern	R3	Meruem P4, Razor Note, Zoro Note	Yes
CRF-28	Nit	Open	authorize.go:59	`canonicalScopes` reimplements order-preserving dedup that already lives at `coderd/util/slice.Unique`	R3	Robin Nit	Yes
CRF-29	Nit	Open	authorize.go:284	RFC 6749 §4.1.2.1 error-URL construction lives twice (cancel path in `ShowAuthorizePage` and `redirectAuthorizeError`); a future §4.1.2.1 field would have to be added twice	R3	Robin Note	Yes
CRF-30	Nit	Open	authorize.go:161	`errNoGrantableScope` wraps with `%v` on a `[]string`, so `error_description` ships Go's bracket syntax `[openid profile email]:...` instead of the space-separated scope string	R3	Zoro Nit	Yes
CRF-31	Nit	Open	authorize.go:34	Stale comment on sentinel errors still claims the messages are rendered "onto the authorize error page", but that page no longer exists for scope errors after `339714cb30`; duplicated at authorize_internal_test.go:280	R3	Leorio Nit	Yes
CRF-32	Note	Open	site/static/oauth2allow.html:129	Consent page renders raw catalog IDs (`workspace:ssh`, `template:read`); a user cannot read them and know what they are granting. Same reasoning the author applied to the `coder:all` branch ("the name is not one a user would recognize") should apply to every entry	R3	Nami Note	Yes
CRF-33	Nit	Open	site/static/oauth2allow.html:127	Missing `role="list"` on `<ul>` with `list-style: none`; VoiceOver on Safari strips list semantics	R3	Nami Nit	Yes
CRF-34	Note	Open	authorize.go:214	`consentScopes` collapses to "full access" only when `coder:all` is the sole entry; a mixed allowlist like `coder:all coder:workspaces.access` with an omitted request would list both, reading to a user as narrower than what the code will carry	R3	Hisoka Note	Yes
CRF-35	Note	Open	authorize_internal_test.go:308	The invariant that every persisted scope name is an `api_key_scope` enum member is checked only on names `TestValidateRequestedScope` happens to exercise; a mechanical loop over `ExternalScopeNames()` would catch a future Go-side addition that lacks a matching migration	R3	Knuckle Note	Yes
CRF-36	Note	Open	authorize_test.go:192	Three HTTP-level subtests (`RequestedSubsetGranted`, `DuplicateRequestedScopePersistedOnce`, `StaleAllowlistEntryDropped`) each re-run branches already proven at the internal-table layer; every wiring under test is one line already covered by `ScopeCoveredByAllowlistGranted` or `OmittedScopeDefaultsToAllowlist`	R3	Bisky Note	Yes
CRF-37	Nit	Open	authorize_test.go:139	`OutOfAllowlistRejected` subtest lede restates the subtest name and helper contract; delete the comment or move the guarantee into the helper's docstring	R3	Gon Nit (downgraded from P2 by orchestrator: single-instance stylistic outlier, no functional impact)	Yes

Contested and acknowledged

CRF-15 (Nit, `authorize_test.go:262`) - `authorizeRequest` retraces `doAuthorizeRequest`

Finding: Extend oauth2providertest's AuthorizeParams/doAuthorizeRequest with a Method field and use codersdk.SessionTokenHeader, then collapse authorizeRequest to two lines rather than reimplementing the request builder locally.
Author defense (R2, PRRC_kwDOGkVX1s7gv_tN): The three deltas (GET support, session-token header constant, raw *http.Response return) are real and the extension is the right shape, but duplication is contained to one test file and is not load-bearing; defer the public wrapper until a third caller appears.
Author accepted: Recorded here. The local helper stays as sendAuthorizeRequest at authorize_test.go:450.

CRF-9 (P3, `registration.go:111`) - DCR registration accepts non-catalog scopes

Finding: Reject non-catalog scopes at DCR registration with invalid_client_metadata; fixes the compat break at the write boundary and collapses both read-side patches.
Author defense (R3, IC_kwDOGkVX1s8AAAABOutFng): Filed PLAT-503 with the reasoning inline. Read side already rejects allowlist-filters-to-empty (P3 severity is delayed failure, not authorization hole); the CRF-6 redirect + CRF-8 message moved the failure to the app's own callback naming registered scopes and remedy; and the naive filter-and-store rewrite is unsafe because noScopeAllowlist treats "" and NULL as "no allowlist configured", so filtering openid profile email down to "" at the write boundary would flip today's hard rejection into the most permissive grant. The safe registration-time options (reject vs add an empty-allowlist state) are public API contract changes needing their own PR.
Deferred (PLAT-503). Do not re-evaluate.

CRF-16 (Nit, `authorize_internal_test.go:15`) - Duplicated catalog-membership constants

Panel closure (R3, 5/5 accept): Ryosuke, Kurapika, Mafu-san, Netero, and Knov (the persona who raised it) all applied the re-raise gate and found no new evidence; the author's R2 defense stands. A rename in externalComposite produces a loud failure in exactly the file that lags, and the constants carry different meaning per file (unit-test catalog membership vs HTTP-level DCR fixtures).

CRF-17 (Note, `authorize.go:39`) - Single-use `noScopeAllowlist` abstraction

Panel closure (R3, 5/5 accept): Mafu-san added new reasoning that reinforces the closure: noScopeAllowlist is exactly the invariant that makes CRF-9's filter-and-store fix unsafe; the same primitive is load-bearing across two designs, which is what a good abstraction looks like even at one call site. Netero, Kurapika, Knov, and Ryosuke confirmed no new evidence. Author's R2 defense stands.

CRF-19 (Note, `apps.go:102`) - Admin-created apps hardcode `sql.NullString{}`

Finding: Every admin-created app takes the noScopeAllowlist branch; the allowlist feature is reachable only via DCR. The framing in the PR title read as blanket enforcement.
Author defense (R2, PR description update): The description now explicitly states the design and notes "Giving the admin API a Scope field is its own change." No inline reply, no linked ticket.
Author accepted: Recorded here. Admin-side allowlist is out of scope for Phase 2.

CRF-20 (Note, `authorize.go:378`) - Persisted `grantedScope` has no reader yet

Finding: authorizationCodeGrant still mints rbac.ScopeAll regardless of the persisted column; the PR persists a promise it does not yet keep.
Author defense (R2, PRRC_kwDOGkVX1s7gv_rG): Accepts the two remaining points, notes CRF-1's alias normalization is closed by c15059f930, and links PLAT-480 as the next phase where authorizationCodeGrant will read the column.
Author accepted: Recorded here. Tracked in PLAT-480.

CRF-22 (Note, `validation_test.go:544`) - `TestOAuth2ClientScopeValidation` duplicated across two files

Finding: The same test lives at coderd/oauth2provider/validation_test.go:544 and coderd/oauth2_metadata_validation_test.go:544; this PR wrote the same comment twice.
Author defense (R2, PRRC_kwDOGkVX1s7gv_u5): Agrees on the diagnosis, explains why both copies had to be edited in this PR (both carried the same stale claim), and states consolidation "is a separate change that should own the whole file rather than ride along here." No linked ticket.
Author accepted: Recorded here. No ticket, so a future reader will need to rediscover the duplication.

Law analysis

Effective LOC: +1556 / -16 (12 files). Head SHA: bcd9e9f302. Verdict: Don't split. Enforcement: N/A (advisory would be wrong here). One reviewable idea (negotiate and persist OAuth2 scope at authorize step) touching one security-critical decision boundary; the RBAC primitive, negotiation function, persisted column value, consent page list, and swagger/reference doc all serve the same feature at different layers. Below the 3000 LOC threshold with 63.9% test density. First analysis; recorded for the record.

Round log

Round 1

Panel of 21 (Netero + 20 panel + wildcards). 5 P2, 6 P3, 5 Nit, 6 Note posted; 2 Netero context notes retained but not posted. Reviewed against 08f2c9a2..e98cac87. Effective LOC 633. Event: REQUEST_CHANGES.

Round 2

BLOCKED. CRF-9 has no code change, no substantive author response, and no linked ticket, despite being grouped by the author with CRF-6 and CRF-18 as "need decisions before implementation"; the other two in that group were subsequently implemented. No panel spawned. Effective LOC 1556 (+923 since round 1). Reviewed against 02076e18..bcd9e9f3. Event: COMMENT. Author response needed on CRF-9 (fix, file a ticket, or state why re-registration-time catalog rejection should not happen).

Round 3

PROCEED. CRF-9 deferred to PLAT-503 with substantive reasoning: read side already rejects allowlist-filters-to-empty (P3 severity confirmed as delayed failure, not authorization hole), the naive registration-time filter is unsafe because noScopeAllowlist treats "" and NULL as one "no allowlist configured" state (filtering openid profile email to "" would flip a hard rejection into the most permissive grant). CRF-16 and CRF-17 remain contested from R2. Head SHA unchanged since R2 (bcd9e9f302), but panel has never reviewed this diff (R2 was blocked). Netero + Law run against 02076e18..bcd9e9f3; panel follows if Netero clears.

Panel of 24 (Netero + Law + 21 panel + 2 wildcards). Law verdict: Don't split. Netero P3 (CRF-25): SCOPES.md misdescribes token exchange. Panel closed CRF-16 and CRF-17 (5/5 accept). 3 P3+ (CRF-25, CRF-26, CRF-27), 5 Nit, 5 Note new. Event: COMMENT.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer	Focus
Bisky	tests
Chopper	ops/errors
Churn-guard	change verification
Ging	language modernization
Gon	naming
Hisoka	edge cases
Killua	perf
Kite	change integrity
Knov	contracts
Knuckle	SQL
Komugi	flake/determinism
Kurapika	security
Law	decomposition
Leorio	docs
Luffy	product
Mafu-san	process
Mafuuu	contracts
Melody	dispatch/pairing
Meruem	structural
Nami	frontend
Netero	mechanical checks
Pariston	premise testing
Pen-botter	product gaps
Razor	verification
Robin	duplication
Ryosuke	Go arch
Takumi	concurrency
Zoro	shape

🤖 Managed by Coder Agents.

coder-agents-review

Round 1. 5 P2, 6 P3, 5 Nit, 6 Note.

What lands well: validateRequestedScope's four branches are exhaustive and each one is pinned to at least one test row; the NOT NULL + CHECK (scope <> '') invariant is asserted literally (assert.NotEmpty(t, got)) rather than through indirection; and AllowlistFilteringToEmptyRejected is the load-bearing decision that keeps a filtered-to-empty allowlist from being punned as "no allowlist" through the unrestricted fallback. Netero's empirical revert-and-test showed six of the eight scope-negotiation sub-tests fail on base, so the suite genuinely validates the change. Kite: "the single load-bearing decision that keeps this from becoming a privilege-escalation vector; the tests pin it explicitly."

Blockers this round:

Bare aliases all and application_connect reach oauth2_provider_app_codes.scope verbatim. rbac.IsExternalScope accepts the backward-compat forms; Phase 3's typed api_keys.scopes (api_key_scope[]) will reject the enum parse, and ExpandScope("all") already returns "no scope named". Normalize at the negotiation boundary.
Rejection tests assert only require.Error. The three separately-worded errors (unknown-scope, no-grantable, not-in-allowlist) collapse to one column of "some error happened"; a refactor that routed one branch through another would still pass the whole table.
Plan-doc labels (AC1..AC16, Edge Case 19/20/22, §4.2.2) in both new test files reference identifiers the repo does not carry. Each row already restates its content in plain English, so the labels contribute nothing and rot on the first plan renumbering.
The consent page never renders the negotiated scope. ShowAuthorizePage computes it, discards it into _, and hands RenderOAuthAllowData a struct with no Scope field. Pre-PR this was inert because every code was coder:all; this PR is what changes the precondition.

Deferrable but worth naming:

Every WriteOAuth2Error and RenderStaticErrorPage site in this file diverges from RFC 6749 §4.1.2.1, which requires a redirect to redirect_uri with error=invalid_scope&state=... once client_id/redirect_uri are resolved. This PR extends the class rather than creating it; if the intent is to keep both surfaces, name it, otherwise track the class fix so client state correlation stops being dropped.
The static "Invalid Scope" page description attributes the failure to the requester in the branch where the app's registered allowlist, not the request, is what cannot be satisfied.
DCR registration writes any scope string verbatim; the read-side catalog filter and the empty-filter rejection both exist because of that. A registration-time catalog check collapses both.
Swagger @Param scope on both authorize handlers still reads "Token scopes (currently ignored)"; that annotation regenerates into the shipped API docs.

Process notes: the preceding commit (fix(coderd): set scope on oauth2 test inserts) shows the sibling audit was already applied at the InsertOAuth2ProviderApp{Code,Token}Params call sites. Every claim in the PR description traces. Netero's revert-and-test also observed that NoAllowlistStaysUnrestricted and NullAndEmptyAllowlistBehaveIdentically PASS on base, i.e. they act as regression guards for the intended non-change rather than as validators of this PR's behavior; worth knowing when reading the suite as evidence of AC3/AC16.

Fun quote from Hisoka: "I came looking for a fight. I got a clean opponent instead. ♥"

coderd/oauth2provider/authorize.go:284

P2 [CRF-5] The consent page renders without displaying the negotiated scope; the user clicks Allow against a template that hardcodes "full access" regardless of what the app requested. (Knov P2, Mafuuu P3, Kurapika P4, Meruem Note, Pariston Note)

Knov:

ShowAuthorizePage computes _, err := validateRequestedScope(params.scope, app.Scope) for the pre-consent rejection, then throws the successful return value away. Ten lines later, RenderOAuthAllowPage is handed RenderOAuthAllowData (defined at +site/site.go:794), a struct that has no scope field, and the template at +site/static/oauth2allow.html:117 renders a fixed description reading Allow {{ .AppName }} to have full access to your {{ .Username }} account?.

Mafuuu:

This PR persists the negotiated scope onto oauth2_provider_app_codes.scope, and the same value flows to oauth2_provider_app_tokens.scope at tokens.go:378. Later PRs (per the description: "Applying the negotiated scope in authorizationCodeGrant, refresh narrowing") make that persisted value the effective ceiling on the token. From the user's side, the Allow button starts meaning "grant this specific subset," but they still see nothing that names the subset.

Pre-PR this was inert: every consent produced database.OAuth2ScopeUnrestricted regardless, so the "full access" wording was accurate. This PR changes that precondition. Under "assume no follow-up" the row is dead data and the wording still cannot describe it; under the described follow-up the consent shown and the consent recorded diverge, and no round in the flow shows the user the delta. The fix is small and belongs with the persistence work: return grantedScope from the GET-side call and thread it into a new field on RenderOAuthAllowData. TestOAuthConsentFormIncludesCSRFToken gains a sibling that pins the scope's presence.

🤖

coderd/oauth2provider/registration.go:111

P3 [CRF-9] DCR registration stores any scope string verbatim, so an app registered with openid succeeds at registration and then fails every authorize with invalid_scope. Fix at the write boundary collapses both read-side patches this PR adds. (Ryosuke P3, Pariston P3)

Ryosuke:

The catalog check runs only on the read side. req.Scope reaches InsertOAuth2ProviderAppParams.Scope with no filter, so the DB accepts allowlists that authorization can never satisfy: request the registered name and hit the subset check, omit scope and hit filtered == 0. The affected client cannot self-heal; the remedy is re-registration.

Pariston reframed the same finding as a design origin question: the read-side noScopeAllowlist predicate exists because DCR writes an unconditional sql.NullString{String: req.Scope, Valid: true} (turning oauth2_provider_apps.scope into a de-facto tri-state), and the read-side catalog filter exists because DCR is catalog-unaware. A one-line canonicalization plus a catalog check at registration.go:111 (Valid: req.Scope != "" collapses the state; looping rbac.IsExternalScope over strings.Fields(req.Scope) rejects non-catalog names with RFC 7591 §3.2.1 invalid_client_metadata) makes noScopeAllowlist collapse to !appScope.Valid, turns the read-side filter into defense-in-depth the runtime never has to trigger, and bounds the DCR compat break to already-stored rows rather than every new bad registration going forward.

The PR description addresses one alternative (grandfathering unknown names) and correctly rejects it; that does not rebut the write-side rejection alternative. Registration-time invalid_client_metadata also keeps non-catalog names out of the enforcement path and additionally surfaces the failure at the earliest possible moment, before a client_id is issued and users start clicking Allow.

🤖

coderd/oauth2.go:123

P3 [CRF-10] Swagger @Param scope on both /oauth2/authorize handlers still reads "Token scopes (currently ignored)", which is now the opposite of what the endpoint does. (Ryosuke)

Line 123 (GET) and line 138 (POST) both carry the stale annotation. Callers reading coderd/apidoc/swagger.json or docs/reference/api/enterprise.md will construct requests assuming the parameter is ignored and be rejected with invalid_scope at runtime. This is a shipped contract detail, not just a comment, since the annotation is regenerated into the API docs.

🤖

coderd/oauth2provider/apps.go:102

Note [CRF-19] Admin-created OAuth2 apps hardcode Scope: sql.NullString{} on create and preserve it on update; the scope allowlist feature is reachable only through DCR. (Mafuuu Note, Melody Note, Ryosuke Note)

Producers of oauth2_provider_apps.scope: apps.go:102 (create) writes sql.NullString{} unconditionally, and apps.go:159 (update) writes app.Scope back unchanged. registration.go:111 and :329 (DCR create/update) are the only paths that write a non-null value. PostOAuth2ProviderAppRequest and PutOAuth2ProviderAppRequest (codersdk/oauth2.go:77, :98) have no Scope field, so there is no API to set one.

Every admin-created app hits noScopeAllowlist(sql.NullString{}) == true, which means validateRequestedScope's allowlist logic is dead code for admin apps: either they get OAuth2ScopeUnrestricted (client omitted scope) or they get exactly what the client requested from the catalog. May be exactly the intended design ("admin apps are trusted, only DCR needs a leash"), but the framing in the PR title/description reads as blanket enforcement. Worth stating explicitly, either in the PR description or in the code path, and worth deciding whether the admin API should grow a Scope field so "restrict this admin-created app to X" is expressible.

🤖

🤖 This review was automatically generated with Coder Agents.

rbac.IsExternalScope accepts `all` and `application_connect` as backward-compatible aliases, but neither is a member of the api_key_scope enum, and rbac.ExpandScope cannot expand either. A request naming one passed the catalog check and was persisted verbatim onto oauth2_provider_app_codes.scope, whose documented vocabulary is that enum. Add rbac.CanonicalScopeName, which maps the two aliases onto the names the enum stores, and apply it to the requested scope, the filtered allowlist, and the subset comparison between them. Canonicalizing both sides also makes an allowlist entry of `all` cover a request for `coder:all`, which the previous raw string comparison treated as two different scopes. Deduplicate the persisted value in the same pass. A space-separated scope denotes a set, so a repeated name is stored once. Replace the three inline rejection messages with sentinels wrapped around the offending name, so the tests can assert which check rejected a request instead of only that some error occurred. requirePersistableScope asserts on every passing table row that each negotiated name is an api_key_scope member and is expandable by RBAC.

BobbyHo · 2026-08-12T14:36:48Z

CRF-1, CRF-12, and CRF-2 are addressed in c15059f.

CRF-1. rbac.CanonicalScopeName (new, next to IsExternalScope) maps all and application_connect onto the names the api_key_scope enum stores. It is applied to the requested scope, to the filtered allowlist, and to the subset comparison between them. It lives in rbac rather than in oauth2provider because the alias set is only knowable from IsExternalScope, so a local copy would be free to drift out of sync with it, which is this finding's failure mode.

Canonicalizing the allowlist as well as the request also fixes a latent matching bug that was not in the report: an allowlist entry of all did not previously cover a request for coder:all, since allowedSet compared raw strings.

CRF-12. Deduplicated in the same pass, preserving order of first appearance.

Both are pinned by requirePersistableScope, which runs on every passing row of the table and asserts each negotiated name is an api_key_scope member and expandable by rbac.ExpandScope. That covers rows added later, not only the alias rows added here. Verified the seven new cases fail with the canonicalization removed.

CRF-2. wantErr is now an error rather than a bool, and rejections are typed sentinels (errUnknownScope, errNoGrantableScope, errScopeNotAllowed) wrapped around the offending name, asserted with errors.Is. Sentinels rather than wantErrContains because CRF-7 and CRF-8 reword two of these three messages, which would have re-broken substring assertions.

This sharpened one existing case: StaleAllowlistEntryNotRequestableExplicitly asserts errUnknownScope, not errScopeNotAllowed. The request-side catalog check rejects before the allowlist is consulted at all, which the case's comment had implied otherwise. Comment updated.

One caveat, since it affects a later round: the HTTP-level requireInvalidScope pins branches by substring on error_description, because the sentinels are not reachable from the external _test package. CRF-7 and CRF-8 will need those three substrings updated when they land.

Remaining findings are unaddressed and tracked separately. CRF-6 (redirect vs JSON), CRF-9 (registration-time validation), and CRF-18 (composite vs member subset semantics) need decisions before implementation. On CRF-18 specifically, normalizing the allowlist via CompositeSitePermissions does not work as suggested: it returns []Permission rather than scope names, and coder:workspaces.access expands to include organization_member:read, which is not in externalLowLevel and would be re-dropped by the catalog filter. A correct semantic subset check is permission-set containment.

@PARAM

The swagger @PARAM on both /oauth2/authorize handlers described scope as "Token scopes (currently ignored)". That annotation regenerates into coderd/apidoc/swagger.json and docs/reference/api/enterprise.md, so the published API reference told integrators a parameter was ignored when sending an unsupported value now returns invalid_scope. Describe what the parameter does and regenerate. Drop the AC*, Edge Case *, and section-number prefixes from the scope negotiation test comments. They refer to a planning document that is not in the repository, so a reader here cannot resolve them, and they rot on the first renumbering. Each comment already restates its content, so only the prefix is removed. The RFC 6749 citations are genuine and stay; the one that read as an RFC section reference was a plan-doc reference and is reworded. State validateRequestedScope's return contract per branch. The previous wording claimed the no-allowlist branch preserves unrestricted behavior, which holds only when the client also requested nothing: with a request, that branch returns the request, which is narrower. Correct the comment claiming the scope check sits inside extractAuthorizeParams. It runs after that function returns, in both handlers.

github-actions · 2026-08-12T15:42:55Z

Docs preview

Check off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here.

docs/reference/api/enterprise.md

BobbyHo · 2026-08-12T15:43:11Z

CRF-3, CRF-4, CRF-10, CRF-13, and CRF-14 are addressed in ab78087.

CRF-3 and CRF-4. All 14 label prefixes removed from both test files. Confirmed by grep across coderd/ rather than only the reported lines, so no other plan-doc reference is left in the tree. The substantive text of each comment is unchanged; only the unresolvable pointer is gone.

Two details. The genuine RFC 6749 §3.3 citations stay. The reference that read as an RFC section was, as noted, a plan-doc reference and not RFC 6749 §4.2.2, so TestOAuth2AuthorizeDCRScopeCompatibility now describes what it pins as "an accepted compatibility break" instead of citing a section number.

CRF-10. Both @Param scope annotations now describe the parameter's actual behavior, and coderd/apidoc/swagger.json, coderd/apidoc/docs.go, and docs/reference/api/enterprise.md are regenerated. The generated diff contains nothing but the description change and the markdown table reflow it causes.

This one has no inline thread to resolve, since it was raised in the review summary rather than as a file comment. Noting it here so it is not read as skipped.

CRF-13. The doc comment now states the return contract per branch as a table, which also settles the overstatement directly: with no allowlist and a non-empty request, the branch returns the request, and the table says so rather than calling it unrestricted.

CRF-14. Reworded to describe what the code does. Both handlers run the check so a request that cannot succeed is rejected before consent renders, and only the POST side needs the returned value. If CRF-21 lands, the negotiate-once shape will replace this wording rather than amend it.

Note for whoever picks up CRF-7 and CRF-8: the HTTP-level assertions added for CRF-2 pin rejection branches by substring on error_description, because the sentinels are not reachable from the external _test package. Rewording those two messages means updating three constants at the bottom of authorize_test.go.

OAuth2ScopeUnrestricted was an alias for ApiKeyScopeCoderAll, so the unrestricted grant had two spellings while every other call site (coderd/apikey.go, coderd/apikey/apikey.go, coderd/users.go) names ApiKeyScopeCoderAll directly. Use that name at the oauth2 code and token sites too, with an explicit string conversion marking where the api_key_scope enum crosses into the text columns. The alias carried no enforcement. The property that a grant's authority is always stated, and that a caller omitting the column fails rather than receiving full access, comes from NOT NULL plus CHECK (scope <> '') in migration 000569 and is unaffected.

…hub.com/coder/coder into coder-oauth2-scope-enforcement-plat-470

DeleteAPIKeyByIDReturningRow and DeleteOAuth2ProviderAppCodeByIDReturningRow had no production caller, here or on the Phase 2 branch. Both exist for the redemption path that makes the code delete the single-use arbiter and reads the negotiated scope off the returned row, but that call-site swap is in neither phase, so the queries and the test pinning their contract were dead weight across five generated files plus two dbauthz authorization decisions no caller could exercise. The plain :exec deletes they were added alongside are untouched and remain what authorizationCodeGrant and the revoke paths call. PLAT-480 covers reintroducing the query and its contract test in the PR that switches authorizationCodeGrant over to it. Refs PLAT-478

…lat-470' into coder-oauth2-scope-enforcement-plat-470-phrase-2 Conflict in coderd/oauth2provider/authorize.go at the authorization code insert. Phase 1 changed its placeholder to string(ApiKeyScopeCoderAll) when OAuth2ScopeUnrestricted was inlined; Phase 2 replaces that placeholder with the negotiated scope, so the Phase 2 side is kept. Inlining OAuth2ScopeUnrestricted also removed a constant this branch used in four places that merged without conflict. Those now name string(database.ApiKeyScopeCoderAll) directly, matching every other call site.

The filtered-to-empty rejection fires on an app whose registered allowlist has no supported entry, which a user reaches even when they requested no scope at all. Name the registered scopes and the remedy, and give the authorize error page a second description for that branch so it points at the application rather than the requester. Wrap all three rejections with the offending value ahead of the sentinel. xerrors repeats the wrapped text unless %w is the final verb, so each description carried its reason twice. A table assertion pins the count. Fold requireInvalidScope's duplicated decode into oauth2providertest, which grows RequireOAuth2ErrorWithDescription so a caller can pin which branch it hit.

RFC 6749 §4.1.2.1 delivers an authorization error by redirecting to the client's callback once the client is known. Both handlers returned it on Coder instead, as an HTML page on GET and a JSON body on POST, so the client's error handling never ran and the state it sent was dropped. Redirect both verbs with error, error_description, and state. The redirect URI is exact-matched against the app's registered callback well before this point, so the destination is the app's own whatever the request carried. A test pins that an unregistered URI still fails on Coder with no Location, since that ordering is what keeps this redirect out of a request's reach. This removes the Invalid Scope page and the two descriptions added in a290850. The distinction survives in error_description, which now reaches the app owner who can act on it rather than the user who cannot. requireInvalidScope no longer decodes a JSON error body, so the helper it delegated to is reverted with it.

The allowlist bounds what an app may be granted, but the check compared scope names, so a request was accepted only when the allowlist spelled it the same way. A client registered for coder:workspaces.access and needing only workspace:ssh had no route to that narrow token: to get any token it had to request the broader composite, which is the opposite of what an allowlist is for. Add rbac.ScopesCover, which expands both sides and asks whether every permission the request grants is also granted by the allowlist. The comparison is asymmetric about what it ignores. Anything on the allowed side it does not model is dropped, which can only make the answer stricter; anything on the requested side it does not model is an error, since answering "covered" about authority that was never compared is the failure that matters. Order the undecidable branch's wrap so %w is last. xerrors repeats the wrapped text otherwise, and this text is rendered into error_description, so the reason would have appeared twice. PartiallyOutOfAllowlistRejected asserted a rejection using template:read, which coder:workspaces.access genuinely grants, so it now names template:update instead. A property test pins what the allowlist check depends on: coder:all covers the whole external catalog, and every catalog name covers itself. SCOPES.md documents the negotiation from the client's side, including the gaps this phase leaves: the token response omits scope, and the consent page still claims full access.

The consent page asked the user to approve "full access" to their account whatever the client requested. That was accurate while every code carried coder:all, and this branch is what made it false: a request for workspace:ssh now records workspace:ssh and still asks the user to approve everything. ShowAuthorizePage already negotiated the scope and then discarded it, since only the POST side persisted the result. Keep it and pass it to the page, so the permissions a user approves are the ones the code will carry. Both handlers negotiate the same query string, because the consent form posts back to the URL that rendered it. An unrestricted grant keeps the full-access wording rather than being listed by name, since coder:all states less to a user than the sentence does. consentScopes returns nil for that case and the template branches on it. The feedback path hides the list along with the buttons, so a submitted page does not leave a stale set of permissions on screen. Two tests, at the levels that fail differently. The template one asserts both directions: a narrow grant is not described as full access, and a full grant is not labeled by a scope name. The end-to-end one asserts the served page names the negotiated scope and not the app's allowlist, which is broader and would satisfy every other assertion while overstating what is being approved. SCOPES.md listed this as a known gap. Drop the entry and describe the behaviour alongside the rest of the negotiation.

…se-2 Two conflicts, both from #28007 landing on main while this branch carried a newer copy of the same work. authorize.go: main stamps the phase-1 placeholder onto every code, with a comment saying negotiation lands in a later phase. This branch is that phase, so the negotiated scope wins. querier_test.go: an append-at-end collision rather than a real disagreement. Main added TestGetAIModelPrices and this branch added nothing there, so main's test is kept as-is.

coder-agents-review

Round 3. 3 P3, 5 Nit, 5 Note.

Every R1 blocker resolved. The R1 -> R3 arc shows genuine work rather than performed compliance: 15 findings landed with code across five commits (c15059f9, ab780870c3, a2908505a3, 339714cb30, 32275ac6, bcd9e9f3), each addressing a different root cause rather than blanket-touching flagged locations. The consent-page fix (CRF-5) also collapsed CRF-21's double validateRequestedScope call as a byproduct, which is the shape of a fix that understood both findings as one issue. rbac.ScopesCover (CRF-18) pivots from name matching to permission coverage: a client allowed coder:workspaces.access can now approve a request for only workspace:ssh, matching what an allowlist actually represents. redirectAuthorizeError (CRF-6) is documented with the precondition that makes it safe, and MismatchedRedirectURINotRedirected pins the ordering on both verbs.

CRF-9 accepted as deferred to PLAT-503 with substantive reasoning: the naive registration-time filter is unsafe because noScopeAllowlist treats "" and NULL as one "no allowlist configured" state, so filtering openid profile email down to "" at the write boundary would flip today's hard rejection into the most permissive grant. Reject-at-registration and add-an-empty-allowlist-state are both public API contract changes needing their own PR. The R2 process worked: the block forced disclosure and produced disclosure, rather than being circumvented by a hedge.

CRF-16 and CRF-17 closed by panel consensus (5/5 accept). Mafu-san added new reasoning on CRF-17: noScopeAllowlist is exactly the invariant that makes the CRF-9 fix unsafe if written naively, i.e., the same primitive is load-bearing across two designs, which is what a good abstraction looks like even at one call site.

Three open findings in this round.

CRF-25 (P3, SCOPES.md:160). The new "Token exchange" section claims the negotiated scope is copied to the API key and refresh token. The refresh-token half is true (tokens.go:378), but the API-key half is not: apikey.Generate is called with no Scope/Scopes at tokens.go:318 and defaults to [coder:all]. The Known gaps section (SCOPES.md:184) lists two smaller gaps and omits this one. The PR description resolves the same tension honestly ("Issued tokens are still unrestricted"); SCOPES.md should too. Related to CRF-20 (acknowledged, tracked in PLAT-480), but new because it is a doc file whose top-line description of token exchange contradicts what ships.
CRF-26 (P3, authorize.go:440). RFC 6749 §4.1.2.1 covers a class of errors, not just invalid_scope. Five siblings (POST unsupported_response_type :440, POST invalid_request PKCE :452, POST server_error GenerateSecret :465, POST server_error InTx :511, GET unsupported_response_type :349) still respond on Coder even though they sit past the extractAuthorizeParams boundary that redirectAuthorizeError's docstring names as the sole precondition. The CRF-6 reply argued each remaining site needs individual judgment; that judgment has now been made by four reviewers and each site is structurally identical to the branch that was fixed. Either route them through the existing helper or file a follow-up ticket so this class stays visible.
CRF-27 (P4, rbac/scopes.go:359). ScopesCover guards the requested side against Negate permissions but silently drops them on the allowed side. The docstring's "can only make the answer stricter" holds for User/ByOrgID (dropping a positive narrows) but fails for Negate (dropping an anti-grant widens). Zero live consequence today (no scope in the catalog carries Negate), because expandLowLevel never sets it. The property test TestScopesCoverEveryExternalScope guards the invariant on the requested side; the allowed side would fail open silently the moment a future "everything except X" scope is added. Three-line class fix matches the existing pattern.

Deferrables named separately: consent-page raw catalog IDs (CRF-32), consent-page a11y (CRF-33), catalog-membership mechanical check (CRF-35), test overlap between HTTP and internal tables (CRF-36), stale error-page comment (CRF-31), duplicated §4.1.2.1 URL construction (CRF-29), and a handful of Nits.

Process note: Law verdict Don't split. Effective LOC 1556 with 63.9% test density; every layer touched serves one reviewable idea (negotiate and persist OAuth2 scope at authorize step) and no cleanup, no unrelated refactor, no second risk domain rides along. Netero: unchanged-diff since R2, but panel had never seen this diff since R2 was blocked; treated as first panel look.

Fun quote from Mafu-san: "the process worked: the block forced disclosure and produced disclosure, rather than the block being circumvented by a hedge."

coderd/oauth2provider/authorize.go:440

P3 [CRF-26] unsupported_response_type still responds on Coder rather than redirecting to the app's callback per RFC 6749 §4.1.2.1. Same class as CRF-6 (fixed for invalid_scope), four siblings over. (Hisoka P3, Chopper P4, Melody P4, Ryosuke Note)

Hisoka reproduced against HEAD (bcd9e9f302) with a well-formed request whose redirect_uri exactly matched the app's callback:

[POST response_type=token]      status=400 Location=""  Content-Type="application/json; charset=utf-8"
[POST invalid PKCE method]      status=400 Location=""  Content-Type="application/json; charset=utf-8"
[GET  response_type=token]      status=400 Location=""  Content-Type="text/html; charset=utf-8"

Sites (all post-extractAuthorizeParams, i.e., past the exact-match on redirect_uri that redirectAuthorizeError's docstring names as the precondition for use):

POST authorize.go:440 - unsupported_response_type when params.responseType != Code
POST authorize.go:452 - invalid_request when ValidatePKCECodeChallengeMethod fails
POST authorize.go:465 - server_error when GenerateSecret fails
POST authorize.go:511 - server_error when InTx fails
GET authorize.go:349 - unsupported_response_type still calls site.RenderStaticErrorPage

§4.1.2.1 covers all of these codes (unsupported_response_type, invalid_request, server_error) alongside invalid_scope; the RFC is section-scoped, not error-code-scoped. The consequence is exactly what CRF-6 was closed against: the client's OAuth2 error handler never runs, the state it sent is dropped, and the user is stranded at Coder.

The CRF-6 reply argued the class fix needs individual judgment per site because "several of the ten sites are exactly where that validation fails." That judgment has now been made by four reviewers: the five sites above all run after the redirect URI has been exact-matched (via p.RedirectURL in extractAuthorizeParams), so redirectAuthorizeError's precondition holds unchanged at each. Either apply the helper to all five now, or file a follow-up ticket so the class stays visible. MismatchedRedirectURINotRedirected already pins the pre-validation direction; adding sibling cases for the four now-inline error codes would pin the extension the same way.

🤖

🤖 This review was automatically generated with Coder Agents.

Round 3 review findings, none of which change what the negotiation grants. SCOPES.md said the negotiated scope is copied to the API key. Only the refresh token record carries it: apikey.Generate is called without a scope, so enforcement still sees an unrestricted key. The section now says that, and Known gaps lists it alongside the other two, since a doc whose headline description of token exchange contradicts what ships is worse than one that admits the boundary. The filtered-to-empty rejection wrapped a []string with %v, so error_description shipped Go's bracket syntax to the app owner reading it. Joined and quoted, matching the other three wraps in the same function. ScopesCover dropped negative permissions on the allowed side while rejecting them on the requested side. The docstring's claim that dropping from the ceiling "can only make the answer stricter" holds for positive permissions and inverts for anti-grants: an "everything except delete" scope would have covered a request for delete. No catalog scope expands to one today, because scope expansion never sets Negate, so this closes a fail-open path rather than a live bug. Left untested for the same reason the requested-side guard is: reaching it means mutating the package-level scope map that parallel tests read. consentScopes collapsed to "full access" only when coder:all was the sole entry. An allowlist registered as `coder:all coder:workspaces.access` defaults to both names, so the page listed the one string the collapse exists to hide while describing an unrestricted grant as if the other name bounded it. Now keyed on presence. The consent list carries role="list" and role="listitem". WebKit drops the implicit semantics when list-style is none, which left VoiceOver announcing the permissions as loose text. Two comments corrected: the scope sentinels no longer claim their messages reach an authorize error page, which 339714c removed, and a test comment that restated its own subtest name is gone.

…ed test canonicalScopes hand-rolled the order-preserving dedup that coderd/util/slice.Unique already provides and eleven other files use. The canonicalization pass and the dedup are now separate, which costs a second pass over a list that holds a handful of scope names. Verified load-bearing: removing the dedup fails three internal table cases and, at HTTP level, DuplicateRequestedScopePersistedOnce, which is the only test proving the persisted column is set-valued. RequestedSubsetGranted is removed. Now that coverage rather than name matching decides the allowlist question, a literally-listed name takes the same path as a covered one, so ScopeCoveredByAllowlistGranted subsumes it and is the stronger case: the name it grants is not in the allowlist at all. Addresses CRF-28 and the narrow half of CRF-36 from the round-3 review of #28045. The other two subtests CRF-36 names are kept, with reasoning on the thread: StaleAllowlistEntryDropped is the only test proving a non-catalog allowlist entry never reaches the enum-constrained column, which the internal table cannot check because it never writes.

BobbyHo · 2026-08-13T21:18:40Z

Round 3 close-out

All thirteen findings verified against the tree; none were inaccurate. Nine taken, four deferred with tickets or a named home.

Taken in 1710f2cde0: CRF-25, CRF-30, CRF-31, CRF-33, CRF-34, CRF-37, and the Negate guard in CRF-27.

Taken in 85565b1039: CRF-28 (slice.Unique), and the narrow half of CRF-36 (RequestedSubsetGranted removed as subsumed by ScopeCoveredByAllowlistGranted).

Deferred: CRF-26 and CRF-29 to a ticket (below), CRF-32 to consent-UX work, CRF-35 to PLAT-480. Reasoning is on each thread.

CRF-26 has no thread, so recording it here

CRF-26 appeared only in the review body, so it will never show in the resolved count. Decision: ticket it, do not absorb it here.

The finding is correct and its site list is precise: five sites after extractAuthorizeParams still answer on Coder while invalid_scope redirects, and the review correctly excluded the three that must not redirect (the two where extractAuthorizeParams itself failed, and "Invalid Callback URL", where the registered callback is the thing found invalid).

It is deferred because it is a client-visible contract change on five more error paths, each needing its own test, and because the two server_error sites are a different risk shape: redirecting an internal fault to a third party is correct per §4.1.2.1 but deserves its own review rather than arriving inside a scope PR.

I will name the discomfort rather than leave it implied: this is the second consecutive deferral, after CRF-9 to PLAT-503, and two in a row can read as avoidance. The difference is that CRF-9 was deferred because the remedy was unsafe as proposed, while CRF-26's remedy is correct and merely out of scope, which is the weaker reason of the two. What I have not done is take three of the five and leave two.

The ticket carries the full site table, the three exclusions with the reason each must stay, the server_error risk note, and CRF-29's shared URL builder folded in, since that extraction is better motivated at seven call sites than at two.

Not adopted

DuplicateRequestedScopePersistedOnce and StaleAllowlistEntryDropped from CRF-36 are kept. Reasoning on the thread, which is the one thread left open deliberately: the first is the only HTTP-level proof that the persisted value is set-valued, and it fails when CRF-28's refactor drops the dedup; the second is the only proof that a non-catalog allowlist entry never reaches an enum-constrained column, which the internal table cannot check because it never writes.

The negotiation doc is integrator-facing reference material, not a design note explaining the code beside it, and nothing in the tree linked to it, so a reader of authorize.go would never have found it. Its Known gaps section is also phase-boundary state that goes stale the moment enforcement starts reading the column. Held outside the repo while its destination under docs/ is decided. The PR description no longer lists it.

…se-2

BobbyHo · 2026-08-13T23:18:28Z

coderagents · 2026-08-13T23:27:03Z

…se-2

Split out of #28045 ([PLAT-479](https://linear.app/codercom/issue/PLAT-479)) so the coverage rules can be reviewed on their own. First of three. No behavior change: both functions are added without production callers, and the OAuth2 authorize endpoint consumes them in the next PR. **ScopesCover(allowed, requested)** reports whether every permission the request grants is also granted by the allowlist. It expands both sides and compares permissions rather than names, so coder:workspaces.access covers workspace:read without ever naming it. The fail-closed rules are the part worth reviewing: - Coverage models site-level grants only. A scope carrying org or user permissions, a negative permission, or a narrowed allow list is refused on either side rather than compared on the part that is modeled. Answering "covered" from the fraction that was read would report authority that was never examined. - An unknown name on either side is an error, not a false, since a caller cannot tell "not covered" apart from "could not decide". - A wildcard request is covered only by a wildcard grant. Enumerating every workspace action that exists today does not cover workspace:*, because the wildcard also authorizes the actions added tomorrow. **CanonicalScopeName** maps the aliases IsExternalScope accepts, all and application_connect, onto the api_key_scope enum spellings. IsExternalScope answers whether a name may be requested, not how it is spelled once persisted, so anything storing a validated name has to canonicalize in between. Both sides of ScopesCover must already be canonical, which its parameter names restate at every call site. Covered by 17 coverage cases, guard cases that drive each unmodeled shape through both sides of the comparison, and an exhaustive sweep over the external catalog: coder:all covers all of it, and every catalog name covers itself. Stack: this PR, then negotiate and persist the scope, then the consent page and invalid_scope redirect. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

**TL;DR** Middle of the stack, split out of #28045, that makes an OAuth2 app's scope allowlist actually restrict what a client can be granted. The authorize endpoint has always parsed `scope` and then thrown it away. | PR | What it does | |---|---| | #28007 | Schema: `codes.scope` and `tokens.scope`, so a negotiated scope has somewhere to live. Every code is still stamped unrestricted. | | #28167 | `ScopesCover` compares an allowlist against a request by permission coverage rather than by name, and fails closed on any shape it does not model. Added with no production callers. | | **#28178 (this)** | The authorize endpoint checks the request against the scope catalog, grants it only if the app's filtered allowlist covers it, and persists the result on the code. Both GET and POST negotiate, so a doomed request fails before the consent page renders. | | #28179 | The consent page states what was negotiated, and `invalid_scope` is delivered to the client's own callback per RFC 6749 §4.1.2.1 instead of a Coder error page. | - Each earlier PR is inert alone: the columns carry a placeholder, the coverage rules have no caller. - Visible on upgrade, unlike the rest of the stack. A scope name outside the catalog, OIDC vocabulary included, now gets `invalid_scope`, on admin-created apps as well as DCR ones. - Tokens are unaffected. This changes which authorization requests succeed, not what a token can do. **Where in the flow** - Authorize endpoint only, both legs: GET before the consent page renders, POST before the code is minted. The token exchange, refresh, and revocation are untouched. - The result is persisted on the authorization code. The exchange copies it onto the token row, but the API key it mints still carries no scope, so nothing is enforced at request time yet. - Registration and discovery are unchanged. The catalog checked against is the `scopes_supported` both discovery documents already advertise. **What it satisfies** - OAuth 2.1 §1.4.1 — the server "MAY fully or partially ignore the scope requested by the client, based on the authorization server policy". The app's allowlist is that policy; until now the server ignored scope in every direction. - OAuth 2.1 §1.4.1 — on an omitted scope it "MUST either process the request using a pre-defined default value or fail the request". The filtered allowlist is that default; an allowlist filtering to nothing takes the second branch. Same rule as RFC 6749 §3.3. - OAuth 2.1 §1.4.1 — scope values "are defined by the authorization server", and OIDC is named there as an extension that defines its own. Hence `invalid_scope` for openid/profile/email rather than silent acceptance. - OAuth 2.1 §4.1.2.1 — `invalid_scope` is "invalid, unknown, or malformed", which now covers an uncatalogued name and an over-broad one alike. Delivery is interim here (static page on GET, error body on POST); §4.1.2.1 wants it redirected to the client's callback, which is #28179. - OAuth 2.1 §4.1.1 — `scope` stays OPTIONAL. Omitting it selects a default, never an error. - RFC 7591 §2 — a registered `scope` is the list "the client can use when requesting access tokens", a ceiling rather than a menu. Enforced here for the first time; a client that registered an unsupportable one repairs it via RFC 7592. - RFC 8414 §2 — `scopes_supported` is that catalog, advertised since before this stack, so a conformant client had no grounds to send a name outside it. - Not yet: §1.4.1 also requires `scope` in the token response when the grant differs from the request. Tokens are untouched here, so that lands with enforcement. --- Split out of #28045 ([PLAT-479](https://linear.app/codercom/issue/PLAT-479)). #28167 has merged, so this diff is against main and reviews on its own. The authorize endpoint parsed scope and threw it away, so an app's allowlist never restricted anything and a client asking for too much was never told no. Phase 1 added the columns that carry a negotiated scope from a code to the token, but nothing wrote one. **What changes** - Requested names must be in the external scope catalog. The app's stored allowlist is filtered through that same catalog, which only ever narrows. - The allowlist bounds authority, not spelling. A request is granted when every permission it grants is also granted by the allowlist, named or not, so an app allowed coder:workspaces.access can approve a client asking only for workspace:ssh. - Omitting scope grants the filtered allowlist (RFC 6749 §3.3). - Both handlers negotiate, so a request that cannot succeed fails before the consent page renders rather than after the user clicks Allow. - Two paths produce an empty result and stay 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 filtering to nothing is rejected, since falling back would grant more than it ever permitted. **Accepted compatibility break, and it is wider than DCR.** The catalog check runs before the allowlist is consulted, so it covers admin-created apps too. - Admin-created apps store no allowlist and had scope discarded outright. A client sending OIDC vocabulary (openid, profile, email) now gets invalid_scope on upgrade. - DCR apps registered with openid, read or admin also hold allowlists this server cannot grant from, and fail in both directions. - scopes_supported has always been advertised on both discovery documents, predating this stack, so a conformant client had no grounds to send an uncatalogued name. RFC 6749 §4.1.2.1 makes invalid_scope the answer for one that does. - Grandfathering would seed the enforcement path with values it cannot evaluate: a visible error now, a silent hole later. - The rejection names the registered scopes and the remedy. Pinned in TestOAuth2AuthorizeDCRScopeCompatibility. **Interim error delivery, replaced in PR 3.** Rejections go out however each handler already reports errors: a static error page on GET, an OAuth2 error body on POST. #28179 delivers them to the client's own callback (RFC 6749 §4.1.2.1) and rewrites the requireInvalidScope helper. About 25 lines here are interim on purpose and flagged as such in that helper. **Not changed.** 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. Stack: #28167 (merged), this PR, then #28179 for the consent page and the invalid_scope redirect. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>

…8097) **TL;DR** On top of #28047, closing the last two gaps in the stack split out of #27873. A public client can now register (#28046) and exchange a code (#28047), but the admin secrets API will still issue it a secret that authenticates nothing, and the registration responses still report an auth method the token endpoint will not accept. | PR | What it does | |---|---| | #27712 | Schema: `app_secret_id` becomes nullable and tokens gain an always-populated `app_id`, so ownership checks work without a secret row. | | #28041 | Accepts bare custom-scheme redirect URIs (`vscode://`, `cursor://`) that native apps actually register. | | #28043 | Derives and stores `client_type` (public vs confidential) from the requested `token_endpoint_auth_method`, and pins it across updates. | | #28046 | Registration issues no secret for a public client and returns no `client_secret`. | | #28047 | The token endpoint accepts the exchange without a `client_secret`. Discovery advertises `none`. | | **#28097 (this)** | The admin secrets API refuses to create a secret for a public client, and registration, read, and update report the auth method the token endpoint actually enforces. | A public client can only come into existence through dynamic client registration, which is off by default, so neither guard is reachable until DCR is enabled. **Where in the flow** - `POST /oauth2-provider/apps/{app}/secrets`, the admin secrets API. The rest of the secret lifecycle is untouched: `GET` and `DELETE` are unchanged, and a confidential app behaves exactly as before. - The three RFC 7591 and 7592 responses that carry `token_endpoint_auth_method`: registration (`POST`), read (`GET`), and update (`PUT`). Only what is reported changes; the stored column is left as the client sent it. - Authorize, token exchange, refresh, and revocation are untouched. **What it satisfies** - RFC 7591 §2: `none` means the client is public and "does not have a client secret", so the secrets API should not create one for it. - RFC 7591 §3.2.1: the server MUST return all registered metadata and MAY replace a requested value with a suitable one. Reporting the enforced method in place of a stored one that contradicts `client_type` is that substitution. - RFC 7592 §2.2: an update MUST include all metadata as returned by a previous registration, read, or update. Because clients echo back what Coder reports, reporting the enforced method is what lets a legacy row heal on the client's next `PUT`. - RFC 7591 §2 default: an app with no stored method reports `client_secret_basic`, which is both the RFC default and what `ApplyDefaults` substitutes on update. - OAuth 2.1 §3.2.1: the token endpoint enforces on `client_type`, so a reported method that disagrees with it tells a client to authenticate in a way the server will reject. --- - `CreateAppSecret` returns 400 for a public client before generating anything. The secret would authenticate nothing, since the token endpoint no longer checks one for a public client. Deleting it would be worse: `oauth2_provider_app_tokens.app_secret_id` is `ON DELETE CASCADE`, so removing a confidential app's secret takes its tokens with it, but a public client's tokens carry a NULL `app_secret_id` (#27712) and survive. An admin who deleted the secret would think they had cut off the client when they had not. - `reportedAuthMethod()` replaces the raw stored value in all three registration responses. It returns the stored method when it agrees with `client_type`, `none` for a public app, and `client_secret_basic` otherwise. - The rows that can disagree are clients registered before #28043 derived the type from the method: the method was stored as sent while the type was always `confidential`. Such a client holds a secret its exchange still requires, while `GET` reported `none` and told it to drop that secret. - The stored column is deliberately not rewritten, so the response can still differ from the row. That divergence is what a read-modify-write client resolves on its next update, and the test asserts the row keeps what the client sent. - Coverage: the secrets guard asserts the 400 and that no partial secret row was left behind. A table test walks `GET` then `PUT` over six stored-method and client-type pairs: the legacy mismatch in both resend shapes, the reverse mismatch that registration cannot produce but the function still handles, an empty stored method, and the single pair that already agrees and must be reported as stored. - Docs: the client type is fixed at registration and a type-changing update is rejected with `invalid_client_metadata` (behavior from #28043 that was never written down), plus what a legacy `none` client now sees. Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client ## Manual Tests Verified by hand against a local dev deployment (`v2.36.3-devel+c85e4621ef`, dev Postgres), in addition to the automated suite. Both guards are reachable only through dynamic client registration, which was confirmed off by default on this deployment and enabled for the run. Two of the scenarios need a row where `client_type` and `token_endpoint_auth_method` disagree, which registration cannot produce since #28043 derives one from the other, so those rows were seeded by updating the method column directly on a purpose-built client. | # | Scenario | Result | |---|----------|--------| | 1 | Secrets API refuses a public client, with the guard's own message | Pass | | 2 | No partial secret row is left behind by the rejection | Pass | | 3 | A confidential app's secret create, list, and delete are unchanged | Pass | | 4 | Admin-created apps are always confidential, so the guard is unreachable there | Pass | | 5 | When stored method and client type agree, the stored value is reported unchanged | Pass | | 6 | A legacy confidential row storing `none` reports `client_secret_basic` | Pass | | 7 | The reported method is the one the token endpoint enforces, in both directions | Pass | | 8 | An update resending the stored value is accepted and leaves the row alone | Pass | | 9 | An update resending the reported value heals the row | Pass | | 10 | The reverse mismatch, a public row storing a secret method, reports `none` | Pass | | 11 | An empty or unrecognized stored method falls back to `client_secret_basic` | Pass | | 12 | Type-changing updates are rejected both ways, `client_secret_basic` to `client_secret_post` is allowed | Pass | | 13 | Regression sweep of the merged stack, discovery through revocation | Pass | | 14 | Every claim in the new docs paragraphs matches observed behavior | Pass | No correctness defects found. Commands and captured output for each scenario below. **Two notes for anyone re-running this.** The previous runbook for this stack asserts the opposite of scenario 12: it flips a confidential client to public with a `PUT` and records `200`. That was correct before #28043 pinned the client type. The `400` here is the fix, not a regression. Generate the PKCE verifier with enough entropy that stripping `=+/` still leaves 43 characters. The `openssl rand -base64 32 | tr -d "=+/" | cut -c -43` recipe yields 38 to 43, so most runs are rejected by the token endpoint under RFC 7636 section 4.1 with an error that looks unrelated. Same trap noted in #28045. <details> <summary>Shell helpers used throughout</summary> ```bash export BASE_URL=http://localhost:3000 export AUTH_HEADER="Coder-Session-Token: $(cat ./.coderv2/session)" export PGURL="postgres://coder@localhost:$(cat ./.coderv2/postgres/port)/coder?sslmode=disable&password=$(cat ./.coderv2/postgres/password)" # Enable DCR; both guards are unreachable without it. curl -s -X PUT "$BASE_URL/api/v2/oauth2-provider/settings" -H "$AUTH_HEADER" \ -H "Content-Type: application/json" \ -d '{"dynamic_client_registration_enabled": true}' # Print "HTTP <code>" then the body. show() { local out; out=$(curl -s -w '\n%{http_code}' "$@") echo "HTTP $(printf '%s\n' "$out" | tail -n1)" printf '%s\n' "$out" | sed '$d' | jq . 2>/dev/null || printf '%s\n' "$out" | sed '$d' } # $1=name, $2=auth method ("" to omit the field). register() { local body if [ -z "$2" ]; then body=$(jq -nc --arg n "$1" '{client_name:$n,redirect_uris:["http://localhost:9876/callback"]}') else body=$(jq -nc --arg n "$1" --arg m "$2" '{client_name:$n,redirect_uris:["http://localhost:9876/callback"],token_endpoint_auth_method:$m}') fi curl -s -X POST "$BASE_URL/oauth2/register" -H "Content-Type: application/json" -d "$body" } # The stored row, as opposed to what the API reports. stored() { psql "$PGURL" -At -c "select client_type || ' | ' || coalesce(token_endpoint_auth_method,'<NULL>') from oauth2_provider_apps where id = '$1';" } # 43 characters from the unreserved set, always. See the note above. gen_verifier() { openssl rand -base64 96 | tr -d "=+/\n" | cut -c -43; } challenge_for() { printf '%s' "$1" | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_'; } # Full PKCE authorize plus exchange. $1=client_id, $2=client_secret ("" for none). exchange() { local verifier challenge redirect code args verifier=$(gen_verifier); challenge=$(challenge_for "$verifier") redirect=$(curl -s -X POST \ "$BASE_URL/oauth2/authorize?client_id=$1&response_type=code&redirect_uri=http://localhost:9876/callback&state=$(openssl rand -hex 16)&code_challenge=$challenge&code_challenge_method=S256" \ -H "$AUTH_HEADER" -w '\n%{redirect_url}' -o /dev/null) code=$(printf '%s' "$redirect" | grep -oE 'code=[^&]+' | sed 's/code=//') args=(-d "grant_type=authorization_code" -d "code=$code" -d "client_id=$1" -d "redirect_uri=http://localhost:9876/callback" -d "code_verifier=$verifier") [ -n "$2" ] && args+=(-d "client_secret=$2") curl -s -X POST "$BASE_URL/oauth2/tokens" -H "Content-Type: application/x-www-form-urlencoded" "${args[@]}" } ``` </details> <details> <summary>1. Secrets API refuses a public client</summary> A bare non-`201` would also be produced by a request that failed on authentication, routing, or a malformed UUID before reaching the guard, so the status and the message are both asserted. ```bash PUB=$(register manual-28097-public none) PUB_ID=$(echo "$PUB" | jq -r '.client_id') stored "$PUB_ID" show -X POST "$BASE_URL/api/v2/oauth2-provider/apps/$PUB_ID/secrets" -H "$AUTH_HEADER" ``` ```text public | none HTTP 400 { "message": "Cannot create a client secret for a public OAuth2 app.", "detail": "Public clients authenticate with PKCE and have no client secret. The client type is fixed at registration, so register a new confidential client instead." } ``` The message is the guard's own, so execution reached `CreateAppSecret` and returned at the `IsPublic()` check. The detail points at registering a new confidential client, which scenario 12 confirms is the only remedy. </details> <details> <summary>2. No partial secret row is left behind</summary> The guard returns before `GenerateSecret()`, so nothing should exist at either layer. The count was already `0` at registration, and a confidential app is queried the same way as a control, so `0` is not merely what this query always returns. ```bash curl -s "$BASE_URL/api/v2/oauth2-provider/apps/$PUB_ID/secrets" -H "$AUTH_HEADER" | jq -c . psql "$PGURL" -At -c "select count(*) from oauth2_provider_app_secrets where app_id = '$PUB_ID';" psql "$PGURL" -At -c "select count(*) from oauth2_provider_app_secrets where app_id = '$CONF_ID';" ``` ```text [] 0 1 ``` </details> <details> <summary>3. A confidential app's secret lifecycle is unchanged</summary> ```bash CONF=$(register manual-28097-confidential "") CONF_ID=$(echo "$CONF" | jq -r '.client_id') show -X POST "$BASE_URL/api/v2/oauth2-provider/apps/$CONF_ID/secrets" -H "$AUTH_HEADER" curl -s "$BASE_URL/api/v2/oauth2-provider/apps/$CONF_ID/secrets" -H "$AUTH_HEADER" | jq -c '[.[].id]' curl -s -X DELETE "$BASE_URL/api/v2/oauth2-provider/apps/$CONF_ID/secrets/$SECRET_ID" \ -H "$AUTH_HEADER" -o /dev/null -w "delete: HTTP %{http_code}\n" ``` ```text HTTP 201 { "id": "da7a1029-47c4-435e-988a-557ffa4aeacf", "client_secret_full": "coder_POUtoElODM_<redacted>" } ["b2fc55b6-...","da7a1029-...","24f8f8c2-..."] delete: HTTP 204 ["b2fc55b6-...","da7a1029-..."] ``` Create, list, and delete all behave as before. The guard keys on `client_type` and nothing else on this path changed. Unrelated observation, not a finding against this PR: the listing returns two different `client_secret_truncated` formats. Secrets issued at registration are asterisk-padded (`***...TzqEOt`, from `createDisplaySecret`), while secrets issued through the admin API show the bare last six characters (`Rixs60`). Both write the same column and surface through the same field. Cosmetic and pre-existing, but a UI listing both kinds together would render them inconsistently. </details> <details> <summary>4. Admin-created apps are always confidential</summary> `postOAuth2ProviderApp` hardcodes the client type, so the guard is unreachable through the admin create path. ```bash ADMIN_APP=$(curl -s -X POST "$BASE_URL/api/v2/oauth2-provider/apps" -H "$AUTH_HEADER" \ -H "Content-Type: application/json" \ -d '{"name":"manual-28097-admin","callback_url":"http://localhost:9876/callback"}') ADMIN_ID=$(echo "$ADMIN_APP" | jq -r '.id') stored "$ADMIN_ID" show -X POST "$BASE_URL/api/v2/oauth2-provider/apps/$ADMIN_ID/secrets" -H "$AUTH_HEADER" ``` ```text confidential | client_secret_post HTTP 201 { "id": "a77ab276-ea74-4711-ba8b-955a8cb97951", "client_secret_full": "coder_eGlt04LhYg_<redacted>" } ``` Together with scenario 1 this brackets the guard: it fires for a dynamically registered public client and for nothing else. Note the admin API stores `client_secret_post`, not the RFC 7591 section 2 default, so every admin-created app is an agreement case for scenario 5. Such apps carry no registration access token, so they cannot reach the RFC 7592 endpoints where the reporting change applies at all. </details> <details> <summary>5. Agreement cases report the stored value unchanged</summary> The `client_secret_post` case is the one that matters. An implementation that substituted the type default on every call would still pass the public case, since `none` is the public default, but would rewrite this client to `client_secret_basic` and tell it to send its secret in the wrong place. ```bash curl -s "$BASE_URL/oauth2/clients/$PUB_ID" -H "Authorization: Bearer $PUB_RAT" \ | jq -c '{reported: .token_endpoint_auth_method}' POST_REG=$(register manual-28097-post client_secret_post) POST_ID=$(echo "$POST_REG" | jq -r '.client_id') echo "$POST_REG" | jq -c '{reported_on_register: .token_endpoint_auth_method}' curl -s "$BASE_URL/oauth2/clients/$POST_ID" -H "Authorization: Bearer $POST_RAT" \ | jq -c '.token_endpoint_auth_method' stored "$POST_ID" ``` ```text {"reported":"none"} {"reported_on_register":"client_secret_post"} "client_secret_post" confidential | client_secret_post ``` Registration and read both report the stored value. The update site is covered by scenarios 8 and 9. </details> <details> <summary>6. A legacy confidential row storing `none` reports `client_secret_basic`</summary> Seeded from a real confidential client so it genuinely holds the secret its exchange requires, with only the method column rewritten. This is the pre-#28043 shape and cannot be produced through the API any more. ```bash LEGACY=$(register manual-28097-legacy "") LEGACY_ID=$(echo "$LEGACY" | jq -r '.client_id') LEGACY_SECRET=$(echo "$LEGACY" | jq -r '.client_secret') stored "$LEGACY_ID" psql "$PGURL" -c "update oauth2_provider_apps set token_endpoint_auth_method = 'none' where id = '$LEGACY_ID';" stored "$LEGACY_ID" psql "$PGURL" -At -c "select count(*) from oauth2_provider_app_secrets where app_id = '$LEGACY_ID';" curl -s "$BASE_URL/oauth2/clients/$LEGACY_ID" -H "Authorization: Bearer $LEGACY_RAT" \ | jq -c '{reported: .token_endpoint_auth_method}' stored "$LEGACY_ID" ``` ```text confidential | client_secret_basic UPDATE 1 confidential | none 1 {"reported":"client_secret_basic"} confidential | none ``` The response substitutes and the row is deliberately not rewritten. A read has no side effects. </details> <details> <summary>7. The reported method is the one actually enforced</summary> The token endpoint enforces on `client_type`, which is still confidential, so the secret is required regardless of what the method column says. ```bash exchange "$LEGACY_ID" "$LEGACY_SECRET" | jq -c '{has_access: has("access_token"), error, error_description}' exchange "$LEGACY_ID" "" | jq -c '{has_access: has("access_token"), error, error_description}' ``` ```text {"has_access":true,"error":null,"error_description":null} {"has_access":false,"error":"invalid_request","error_description":"Missing required parameter: client_secret"} ``` The second line is the pre-fix breakage reproduced. A client that read `"token_endpoint_auth_method": "none"` from `GET` and dropped its secret accordingly, which is what RFC 7592 tells it to do, would have hit exactly this on its next exchange. The old response was not internally inconsistent, it was actionable and wrong. Scenario 10 shows the same property in the opposite direction. </details> <details> <summary>8. An update resending the stored value is accepted and changes nothing</summary> ```bash show -X PUT "$BASE_URL/oauth2/clients/$LEGACY_ID" \ -H "Authorization: Bearer $LEGACY_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-legacy","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"none"}' stored "$LEGACY_ID" ``` ```text HTTP 200 "token_endpoint_auth_method": "client_secret_basic", confidential | none ``` Three things at once. The update is accepted, which is why the type-change guard requires the auth method to actually change: the requested `none` implies a public client while the row is confidential, so a guard keyed on the type mismatch alone would reject this and lock a legacy client out of managing its own registration. The `PUT` reports the same substituted value as the `GET`, covering the third call site. The row keeps what the client sent, so the divergence persists until scenario 9. </details> <details> <summary>9. An update resending the reported value heals the row</summary> ```bash show -X PUT "$BASE_URL/oauth2/clients/$LEGACY_ID" \ -H "Authorization: Bearer $LEGACY_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-legacy","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"client_secret_basic"}' stored "$LEGACY_ID" curl -s "$BASE_URL/oauth2/clients/$LEGACY_ID" -H "Authorization: Bearer $LEGACY_RAT" \ | jq -c '{reported: .token_endpoint_auth_method}' exchange "$LEGACY_ID" "$LEGACY_SECRET" | jq -c '{has_access: has("access_token")}' ``` ```text HTTP 200 "token_endpoint_auth_method": "client_secret_basic", confidential | client_secret_basic {"reported":"client_secret_basic"} {"has_access":true} ``` The row healed through an ordinary read-modify-write cycle, with no migration, backfill, or admin action. That works only because scenario 8 keeps the door open and this response gives the client a correct value to echo back; remove either and the row stays inconsistent indefinitely. </details> <details> <summary>10. The reverse mismatch reports `none`</summary> Registration cannot produce this direction either, but `reportedAuthMethod` branches on the general disagreement rather than the one shape known to exist, so it is covered. ```bash REVERSE=$(register manual-28097-reverse none) REVERSE_ID=$(echo "$REVERSE" | jq -r '.client_id') psql "$PGURL" -c "update oauth2_provider_apps set token_endpoint_auth_method = 'client_secret_basic' where id = '$REVERSE_ID';" stored "$REVERSE_ID" curl -s "$BASE_URL/oauth2/clients/$REVERSE_ID" -H "Authorization: Bearer $REVERSE_RAT" \ | jq -c '{reported: .token_endpoint_auth_method}' exchange "$REVERSE_ID" "" | jq -c '{has_access: has("access_token"), error}' ``` ```text public | client_secret_basic {"reported":"none"} {"has_access":true,"error":null} ``` This client has no secret row at all and the token endpoint does not ask for one, so `none` is what the response should say. Reporting the stored `client_secret_basic` would have instructed it to send a secret that does not exist. Both resend shapes behave symmetrically with scenarios 8 and 9: ```text resend stored (client_secret_basic) -> reported "none", row unchanged resend reported (none) -> reported "none", row now public | none ``` </details> <details> <summary>11. Empty or unrecognized stored methods fall back to the default</summary> `NULL` and `''` are indistinguishable once read into `sql.NullString`. A third case was added because the function gates on validity rather than emptiness. ```bash for v in "null" "''" "'private_key_jwt'"; do psql "$PGURL" -At -c "update oauth2_provider_apps set token_endpoint_auth_method = $v where id = '$CONF_ID';" stored "$CONF_ID" curl -s "$BASE_URL/oauth2/clients/$CONF_ID" -H "Authorization: Bearer $CONF_RAT" \ | jq -c '.token_endpoint_auth_method' done ``` ```text confidential | <NULL> "client_secret_basic" confidential | "client_secret_basic" confidential | private_key_jwt "client_secret_basic" ``` `private_key_jwt` is a real RFC 7591 method that Coder does not implement. Reporting it back would advertise an authentication scheme the token endpoint cannot honor, which is the same class of problem as the legacy `none` report in scenario 7. </details> <details> <summary>12. Type-changing updates are rejected both ways</summary> ```bash show -X PUT "$BASE_URL/oauth2/clients/$CONF_ID" \ -H "Authorization: Bearer $CONF_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-confidential","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"none"}' stored "$CONF_ID" show -X PUT "$BASE_URL/oauth2/clients/$PUB_ID" \ -H "Authorization: Bearer $PUB_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-public","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"client_secret_basic"}' stored "$PUB_ID" show -X PUT "$BASE_URL/oauth2/clients/$CONF_ID" \ -H "Authorization: Bearer $CONF_RAT" -H "Content-Type: application/json" \ -d '{"client_name":"manual-28097-confidential","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"client_secret_post"}' stored "$CONF_ID" ``` ```text HTTP 400 { "error": "invalid_client_metadata", "error_description": "token_endpoint_auth_method cannot move an existing client between public and confidential (stored \"client_secret_basic\", requested \"none\"); the client type is fixed at registration, so register a new client instead" } confidential | client_secret_basic HTTP 400 { "error": "invalid_client_metadata", "error_description": "token_endpoint_auth_method cannot move an existing client between public and confidential (stored \"none\", requested \"client_secret_basic\"); the client type is fixed at registration, so register a new client instead" } public | none HTTP 200 "token_endpoint_auth_method": "client_secret_post", confidential | client_secret_post ``` Both rejections leave the row and its secrets untouched. A partial application would be the dangerous outcome here, a client left confidential while its caller believes it went public. The third case is the control. Two `400`s alone would be equally consistent with a guard that rejects every `token_endpoint_auth_method` change, a considerably more disruptive rule than the documented one. The `200` pins the guard to the derived client type rather than the method string. The second case is also what backs scenario 1. If public to confidential were permitted, the workaround to a refused secret request would be to flip the type and ask again. The two guards close that loop, which is why the same remedy appears in both error messages. </details> <details> <summary>13. Regression sweep of the merged stack</summary> Nothing here is new in this PR, but the guards sit on top of it. ```text discovery token_endpoint_auth_methods_supported : ["client_secret_basic","client_secret_post","none"] public registration, client_secret key present : false confidential registration, secret issued : true, method defaulted to client_secret_basic PKCE-only exchange, no client_secret sent : access + refresh issued access token against /api/v2/users/me : 200 token row app_id / app_secret_id : populated / NULL RFC 7009 revoke of own refresh token : 200, access token then 401 authorized-apps listing while a token is live : [{"name":"manual-28097-public"}] bulk DELETE /oauth2/tokens : 204, listing then [] ``` The NULL `app_secret_id` is what makes the secrets guard necessary. That column is `ON DELETE CASCADE`, so deleting a confidential app's secret takes its tokens with it, while a public client's tokens would survive. An admin who deleted such a secret would believe access was cut off when it was not. PKCE failure modes were checked separately, since the token endpoint distinguishes two the previous runbook treated as one: ```text verifier omitted : invalid_request, RFC 7636 section 4.1 bounds well-formed 43 chars but wrong : invalid_grant, "The PKCE code verifier is invalid" correct verifier, replaying that code: invalid_grant, "invalid or expired" ``` The third confirms the failed check destroys the code, so a leaked code cannot absorb repeated verifier guesses. </details> <details> <summary>14. The new docs paragraphs match observed behavior</summary> `pnpm run lint-docs` reports 0 errors across 501 files, `pnpm run format-docs` rewrites nothing, and Vale reports 0 errors with 2 warnings, both on pre-existing gerund headings that the same file on `main` also produces. The new prose is one sentence per line. | Documented claim | Verified by | |---|---| | A client's type is fixed when it registers | 12 | | A type-changing update is rejected with `invalid_client_metadata` | 12, both directions | | The client either holds a secret that would stop being required or has none and no way to be issued one | 12 (2 secrets), 1 and 12 (0 secrets, issuance refused) | | Switching between `client_secret_basic` and `client_secret_post` is allowed | 12 | | To change type, register a new client | matches the remedy in both errors and in scenario 1's detail | | Legacy `none` clients are stored as confidential and still require their `client_secret` | 6, 7 | | Coder reports `client_secret_basic` for those clients | 6 | | So that what it reports matches what it enforces | 7, both directions | | The mismatch clears itself the next time the client updates its registration | 9 | One wording note. Scenario 8 shows the mismatch does not clear on an update that resends the stored value, so strictly it clears on the next update carrying the reported value. A read-modify-write client, which is what RFC 7592 section 2.2 prescribes, always carries the reported value, so the sentence holds for the client it describes. A client that hardcodes its own metadata would not self-heal, but it would also not be following RFC 7592. </details> <details> <summary>Cleanup</summary> ```bash for id in "$PUB_ID" "$CONF_ID" "$POST_ID" "$LEGACY_ID" "$REVERSE_ID" "$ADMIN_ID"; do curl -s -X DELETE "$BASE_URL/api/v2/oauth2-provider/apps/$id" -H "$AUTH_HEADER" \ -o /dev/null -w "%{http_code}\n" done curl -s -X PUT "$BASE_URL/api/v2/oauth2-provider/settings" -H "$AUTH_HEADER" \ -H "Content-Type: application/json" -d '{"dynamic_client_registration_enabled": false}' show -X POST "$BASE_URL/oauth2/register" -H "Content-Type: application/json" \ -d '{"client_name":"should-fail","redirect_uris":["http://localhost:9876/callback"],"token_endpoint_auth_method":"none"}' ``` ```text 204 (x6) {"dynamic_client_registration_enabled":false} HTTP 403 {"error":"invalid_request","error_description":"Dynamic client registration is disabled on this deployment"} ``` All six test apps deleted with no orphaned secret or token rows. Both seeded rows had already healed to a consistent state before deletion. DCR is back off, confirmed by a registration attempt rather than by reading the flag back. </details>

**TL;DR** Last of the stack, split out of #28045. #28178 decided what the scope is; this PR is everything that reports it, to the user on the consent page and to the client on rejection. | PR | What it does | |---|---| | #28007 | Schema: `codes.scope` and `tokens.scope`, so a negotiated scope has somewhere to live. Every code is still stamped unrestricted. | | #28167 | `ScopesCover` compares an allowlist against a request by permission coverage rather than by name, and fails closed on any shape it does not model. Added with no production callers. | | #28178 | The authorize endpoint checks the request against the scope catalog, grants it only if the app's filtered allowlist covers it, and persists the result on the code. | | **#28179 (this)** | The consent page lists the negotiated permissions, and `invalid_scope` reaches the client's own callback per RFC 6749 §4.1.2.1 instead of a Coder error page. Retires the interim plumbing #28178 left behind. | - #28178 has merged, so this diff is against main and reviews on its own. It is still worth reading first: nothing here decides a scope, it only reports the one `negotiateScope` already returns. - **The consent page HTML is not final.** Layout and wording in `site/static/oauth2allow.html` are still being refined. It is in this branch so the scope-enforcement flow can be exercised end to end in a browser, not because the markup is settled. The reviewable part of Half A is the Go side that decides what the page states; the presentation will change. - An unrestricted grant still reads as full access, not as a scope name. Nothing about what a token can do changes in this PR either. **Where in the flow** - Authorize endpoint only, both verbs. GET decides what the consent page states; both verbs send `invalid_scope` to the callback. The token exchange, refresh, and revocation are untouched. - `negotiateScope` is unchanged from #28178, so which requests succeed is unchanged. This PR reads its result twice: once to decide what the page lists, once to decide where the rejection goes. - The endpoint sits behind `apiKeyMiddlewareRedirect`, so the user is authenticated before any of this runs. That matters for the redirect, not just for the page. - Consent page rendering moves a decision into Go rather than the template: `RenderOAuthAllowData` carries `Scopes` and `Unrestricted` separately, and the template branches on `Unrestricted` alone. **What it satisfies** - RFC 6749 §4.1.2.1 — on a failure that is not a bad redirection URI, the server "informs the client by adding the following parameters to the query component of the redirection URI". `invalid_scope` now goes there. #28178's interim delivery, a static error page on GET and an error body on POST, is what this replaces. - RFC 6749 §4.1.2.1 — `state` is "REQUIRED if a `state` parameter was present in the client authorization request", so it is echoed only when the client sent one, never synthesized. Pinned by `OmittedStateNotEchoed`. - RFC 6749 §3.1.2.4 and §4.1.2.1 — on a "missing, invalid, or mismatching redirection URI" the server "MUST NOT automatically redirect the user-agent to the invalid redirection URI". This is the whole ordering argument, and the file's other error paths keep their existing delivery precisely because several of them are where that validation fails. - RFC 9700 §2.1 — pre-registered redirection URIs "MUST utilize exact string matching". `extractAuthorizeParams` already did this before the stack; Half B depends on it rather than adding it. - RFC 9700 §4.11.2 — names this exact hazard: an attacker who registers a client can "intentionally send an erroneous authorization request, e.g., by using an invalid scope value, thus instructing the authorization server to redirect the user agent to its phishing site". The destination is still only ever a URI that client registered, so this is a phishing surface that costs an attacker a registration, not an open redirect. The precaution the section requires, authenticating the user first, is what `apiKeyMiddlewareRedirect` provides. - RFC 9700 §4.12 — a server redirecting a request that may carry credentials "MUST NOT use the HTTP 307 status code". 302, matching the success redirect beside it. - RFC 6749 §4.1.2.1 — `server_error` exists "because a 500 Internal Server Error HTTP status code cannot be returned to the client via an HTTP redirect". An unusable registered callback is the one case with nowhere to redirect, so it answers 500 on both verbs and logs the app and URL for an operator to correlate by. - Not yet: §1.4.1 still wants `scope` in the token response when the grant differs from the request. Tokens are untouched here, as in #28178, so that lands with enforcement. --- Split out of #28045 ([PLAT-479](https://linear.app/codercom/issue/PLAT-479)). Last of three. #28178 has merged, so this reviews against main. #28178 decided what the scope is. This PR is everything that tells someone about it. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>

**TL;DR** Fourth of the stack, split out of #28045. #28179 sent `invalid_scope` to the client's own callback; this PR sends the rest of the errors raised after the callback has been verified, behind one URL builder they all now share. | PR | What it does | |---|---| | #28007 | Schema: `codes.scope` and `tokens.scope`, so a negotiated scope has somewhere to live. | | #28167 | `ScopesCover` compares an allowlist against a request by permission coverage rather than by name. | | #28178 | The authorize endpoint negotiates the scope against the app's allowlist and persists it on the code. | | #28179 | The consent page lists the negotiated permissions, and `invalid_scope` reaches the client's callback per RFC 6749 §4.1.2.1. | | **#28450 (this)** | The same delivery for every other error raised once the callback is trusted. | - #28179 has merged, so this diff is against main and reviews on its own. - Nothing about what a token can do changes here. This is delivery, not enforcement. **Contract change** Four failures now arrive at the app's registered callback instead of terminating on Coder, so an integrator with error handling on its callback starts seeing codes it never received. | Verb | Condition | Error code | Was | |------|-----------|------------|-----| | GET | `response_type != code` | `unsupported_response_type` | static "Unsupported Response Type" page, 400 | | POST | `response_type != code` | `unsupported_response_type` | `WriteOAuth2Error`, 400 | | POST | `code_challenge_method=plain` | `invalid_request` | `WriteOAuth2Error`, 400 | | GET | `code_challenge_method=plain` | `invalid_request` | consent page, 200, refused after Allow | The GET static error page loses its only caller. The last row is what review found: the method was checked on POST only, so the user approved a request that could never succeed. **Where in the OAuth Flow** <details> <summary>Diagram: where these errors are raised, and what licenses the redirect</summary> ```mermaid flowchart TD EX["GET or POST /oauth2/authorize extractAuthorizeParams exact-matches redirect_uri against the app's registered callback"] EX -->|"no match, rejected scheme, unparsable, other param failures"| CODER["Answers on Coder RFC 6749 3.1.2.4: MUST NOT redirect"] EX -->|"matched: a validatedCallbackURL, which is what licenses a redirect"| RT{"response_type == code?"} RT -->|no| E1["unsupported_response_type (this PR)"] RT -->|yes| PK{"code_challenge_method S256 or omitted?"} PK -->|no| E2["invalid_request (this PR, and newly checked on GET)"] PK -->|yes| SC{"scope negotiable?"} SC -->|no| E3["invalid_scope (previous PR)"] SC -->|yes| OK["GET: consent page, cancel link is access_denied POST: authorization code"] E1 --> OUT E2 --> OUT E3 --> OUT OK --> OUT["One builder produces every URL the flow emits: error redirect, cancel link, success code"] ``` </details> - Authorize endpoint only, both verbs. The token exchange, refresh, and revocation are untouched. - Every redirect is licensed by a `validatedCallbackURL`, a type produced only where `extractAuthorizeParams` exact-matches the URI against the app's registration. A guard, not a proof: no other package can fabricate one, this one still can. - One builder now produces every response URL, including the consent page's cancel link, which rolled its own and edited the shared URL in place rather than copying it. - Each rejection logs. The error leaves in a `Location` header, which `loggermw` does not record, so a failed authorization used to look exactly like a successful one. **Three smaller behavior changes** - A callback registered with `code`, `error`, `error_description`, or `state` in its own query no longer receives that value back. A registered `error=` rode out on the success redirect, where a client that reads `error` first discards a valid code. The rest of the registered query is kept, as §3.1.2 requires. - `error_description` is confined to the NQSCHAR set Appendix A permits. Descriptions name the offending value with `%q`, so every `invalid_scope` Coder emits today carries excluded characters. Quotes now render as apostrophes. - An unparsable `redirect_uri` returns 400 instead of 500. It reached a nil dereference in `httpapi.RedirectURL`, which `POST /oauth2/tokens` also uses, and that endpoint takes no API key. **What it satisfies** - RFC 6749 §4.1.2.1 — on a failure that is not a bad redirection URI, the server informs the client through the redirection URI. That is the whole PR. - RFC 6749 §3.1.2.4 and §4.1.2.1 — a missing, invalid, or mismatching redirection URI "MUST NOT" be redirected to. A mismatched URI, and a registered URI with a rejected scheme, still answer on Coder. Pinned by `MismatchedRedirectURINotRedirected` and `DangerousCallbackSchemeNotRedirected`. - RFC 6749 Appendix A — `error_description` is NQSCHAR, and the rule is on the decoded value, so percent-encoding on the wire does not satisfy it. - RFC 6749 §4.1.2 and §3.1.2 — the four reserved response parameters are dropped from the registered query; everything else in it is retained. - Not yet: the remaining parameter failures do have a trusted callback in hand, but the parser reports one verdict for every field at once, so the caller cannot say which field failed. A follow-up classifies them. The two `server_error` sites also stay put: sending an internal fault outbound with a Coder-written description wants its own review. **Docs and Swagger.** `docs/admin/integrations/oauth2-provider.md` gains a Common Issues entry per newly redirected code, and the implicit-grant limitation now says where the error arrives. Both authorize verbs document the 302, the GET side for the first time. --- Split out of #28045 ([PLAT-479](https://linear.app/codercom/issue/PLAT-479)). Fourth of the stack. #28179 has merged, so this reviews against main. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>

@uri

) **TL;DR** Fifth of the stack, split out of #28045. #28450 delivered the errors raised *after* the callback is trusted and left the parameter failures behind. This PR delivers those, and replaces the implicit ordering that decided where an answer went with an explicit classification. | PR | What it does | |---|---| | #28007 | Schema: `codes.scope` and `tokens.scope`, so a negotiated scope has somewhere to live. | | #28167 | `ScopesCover` compares an allowlist against a request by permission coverage rather than by name. | | #28178 | The authorize endpoint negotiates the scope against the app's allowlist and persists it on the code. | | #28179 | The consent page lists the negotiated permissions, and `invalid_scope` reaches the client's callback. | | #28450 | The same delivery for every other error raised once the callback is trusted. | | **#28736 (this)** | The parameter failures #28450 left behind, and the classification that decides where each answer goes. | - Stacked on #28450. Review that first. - Nothing about what a token can do changes here. This is delivery, not enforcement. **Contract change** A rejected parameter now arrives at the app's registered callback with `error=invalid_request`, a description naming every failing field, and the request's `state`. Both verbs. | Verb | Was | |---|---| | GET | static "Invalid Query Parameters" page, 400 | | POST | `WriteOAuth2Error`, 400 | An integrator watching for either now reads the error from its own callback instead. **Two failures still answer on Coder**, because in neither case is the callback trustworthy yet: - A `redirect_uri` that does not parse, or does not exactly match the registration. Redirecting to it would defeat the check that just rejected it. - A `client_id` sent more than once, or naming something other than the app the callback was matched against. Coder cannot tell whose registration it is about to redirect to. An absent query `client_id` is **not** in that group. `httpmw` also reads the POST form body and the §2.3.1 Basic credential, so an absent query parameter still names a client and its failure is deliverable. **Precedence change.** An app whose *registered* callback does not parse, or uses a rejected scheme, now answers 500 even when the request also carries parameter errors; that used to answer 400. Decided before any parameter is read, so the others are never detected. Only reachable for an app row that bypassed registration. `DangerousCallbackSchemeOutranksParseFailure` pins it. **Also in this PR** - **Unrecognized parameters are ignored**, as §3.1 requires, rather than rejected by `ErrorExcessParams`. An OIDC `nonce` or a vendor extension no longer fails the request. Repeats of parameters the endpoint does read are still rejected. - **Every unsupported `response_type` gets one code.** Read as text rather than through the SDK enum, so a value with no Go constant behind it answers `unsupported_response_type` like `token` does, instead of splitting on whether the SDK names it. - **A malformed `resource` answers `invalid_target`** (RFC 8707 §2), but only when nothing else failed. A client retrying on `invalid_target` would otherwise resend a request still broken in a field it never heard about. - **`error_description` is bounded and readable.** Capped at 2048 characters and marked `(truncated)` before the log field or the `Location` header is written. Entries read `field: reason` joined with `; `, not the parser's debug shape, whose details contain commas and cannot be split apart again. - **A repeated `state` is charged to the client, not the callback.** Reading `state` shares a function with the `redirect_uri` match, so it used to fall into that carve-out and answer on Coder. **Where in the OAuth Flow** <details> <summary>Diagram: the three-way dispatch, and what licenses a redirect</summary> ```mermaid flowchart TD NEW["newAuthorizeResponse parses the registered callback, checks its scheme, exact-matches any redirect_uri, reads state"] NEW -->|"registration unusable"| C500["500 on Coder server_error, value logged not echoed"] NEW --> PARSE["extractAuthorizeParams reads every parameter, collects all failures"] PARSE -->|"no failure"| OK["GET: consent page POST: authorization code"] PARSE -->|"failure"| KIND{"authorizeFailure.kind()"} KIND -->|"corrupt registration"| C500 KIND -->|"redirect_uri or client_id at fault"| CODER["400 on Coder RFC 6749 4.1.2.1 carve-out"] KIND -->|"anything else"| CLIENT["302 to the callback invalid_request or invalid_target, with state"] ``` </details> - Authorize endpoint only, both verbs. Token exchange, refresh, and revocation are untouched, and `ErrorExcessParams` still guards the token endpoint. - `validatedCallbackURL` becomes `authorizeResponse`, built only by `newAuthorizeResponse`, which runs the scheme check and the exact match in that order. Holding one is what licenses a redirect, so the ordering is structural rather than a convention each call site remembers. - The scheme is checked on the *registered* URL, since `p.RedirectURL` returns the client's URI on a mismatch and checking that would blame the app for a request it never made. - `state` moves onto the type, out of the parameter lists of `withQuery`, `errorURL`, `codeURL`, and `redirectAuthorizeError`, so no call site can emit a response the client cannot correlate. - `extractAuthorizeParams` returns one `authorizeFailure` instead of two trailing values, and both handlers dispatch on `kind()` rather than re-deriving precedence from field checks. **What it satisfies** - RFC 6749 §4.1.2.1: a failure that is not a bad redirection URI or client identifier is reported through the redirection URI. That is the whole PR. Its two exceptions "MUST NOT" be redirected to, hence the carve-outs. - RFC 6749 §3.1: unrecognized parameters MUST be ignored, and no parameter may appear more than once. Both now hold here. - RFC 6749 Appendix A: `error_description` is confined to the permitted set, on the decoded value. - RFC 8707 §2: a `resource` that is not an absolute URI without a fragment answers `invalid_target`. - RFC 7636 §4.4.1: a malformed `code_challenge` is rejected at the authorization request rather than deferred to token exchange, where the error would point at the `code_verifier`. - Not yet: CRF-26 (the `server_error` sites) and CRF-13 (fragment delivery), both as in #28450. **Docs and Swagger.** `docs/admin/integrations/oauth2-provider.md` gains entries for the redirected parameter errors, `invalid_target`, and the two failures that stay on Coder. Both authorize verbs document their 400 and 500 responses and stop advertising `response_type=token`, which meant declaring the parameter as a `string`, since `Enums` appends to what swaggo derives from the type. --- Split out of #28045 ([PLAT-479](https://linear.app/codercom/issue/PLAT-479)). Fifth of the stack, stacked on #28450. ## Manual Tests Verified by hand against a local dev deployment (`v2.37.0-devel+1145d35a47`, dev Postgres), in addition to the automated suite. The build string was checked first, because `develop.sh` builds from whatever the tree held when it started and every result below would otherwise be describing the wrong binary. Two scenario groups need state the API cannot produce. The scope allowlist is reachable only through dynamic client registration, which was enabled for the run and disabled again at the end. A registered callback that does not parse, or that uses a blocked scheme, is refused at registration, so those rows were planted directly with `psql` on a purpose-built client and restored afterwards. **28 scenarios, all passing.** No correctness or security defect was found in the code this PR changes. Seven observations came out of the run: six are consistency, documentation or diagnosability points, and one is a small defect that predates this PR. <details> <summary>Scenario summary, all 28</summary> | # | Scenario | Result | |---|----------|--------| | 1 | A well-formed request still succeeds on both verbs, and the consent page renders | Pass | | 2 | A rejected parameter now reaches the client's callback on `POST` | Pass | | 3 | The same on `GET`, and the consent page does not render first | Pass | | 4 | The description names every failing field, joined with `; ` | Pass | | 5 | A mismatched `redirect_uri` is answered on Coder with no `Location` on either verb | Pass | | 6 | An unparseable `redirect_uri` likewise, and the echoed value is HTML escaped | Pass | | 7 | A repeated `client_id` is answered on Coder although the callback was valid | Pass | | 8 | A single query `client_id` cannot disagree with the resolved app over HTTP | Pass | | 9 | A `client_id` supplied only in the `POST` form body is delivered, not withheld | Pass | | 10 | A `{UPPERCASE}` `client_id` is delivered, and also succeeds end to end | Pass | | 11 | A repeated `state` is delivered rather than charged to the redirect carve-out | Pass | | 12 | A registered callback with a blocked scheme answers 500 on both verbs | Pass | | 13 | An unparseable registered callback answers 500, indistinguishably to the caller | Pass | | 14 | Corrupt registration outranks a deliverable failure and both carve-outs | Pass | | 15 | The corrupt value is logged with the app ID and never appears in a response | Pass | | 16 | Restoring the row restores normal behaviour with no restart | Pass | | 17 | Every unsupported `response_type` gets one code; an empty one is `invalid_request` | Pass | | 18 | An unsupported `response_type` is not recast as a missing `code_challenge` | Pass | | 19 | Unrecognized parameters are ignored, including a case-variant `REDIRECT_URI` | Pass | | 20 | A misspelled `redirect_url` is ignored and cannot smuggle a destination | Pass | | 21 | Repeated known parameters are still rejected | Pass | | 22 | `resource` answers `invalid_target` alone and `invalid_request` in company | Pass | | 23 | A fragment in `resource` is rejected | Pass, with one gap | | 24 | A long description is capped at 2048 and marked `(truncated)`, on both verbs | Pass | | 25 | The description is sanitized to the RFC 6749 §4.1.2.1 character set | Pass | | 26 | The registered callback query is retained except the reserved names | Pass | | 27 | Scope rejections still reach the callback, and no consent page renders | Pass | | 28 | Declining consent carries `access_denied` and issues no code | Pass | </details> <details> <summary>Observations, all 7</summary> | # | Observation | Severity | |---|-------------|----------| | 1 | `invalid_request` is returned in three description shapes: the parser's `Invalid query params: field: reason; ...` aggregate, the PKCE validator's single message, and `scopeFailureResponse`'s. A client cannot parse all three with one rule. | Consistency, reviewer call | | 2 | The phase 2 runbook for #28045 asserts the old `field: x detail: y` message shape, which this PR replaces. That doc needs a one-line update. | Docs follow-up | | 3 | `clientIDInDoubt`'s `default` branch is unreachable over HTTP, because `httpmw` derives `app.ID` from the same query value the parser reads. Defensive rather than dead, but the comment does not say so. | Comment clarity | | 4 | A `POST` supplying `client_id` only in the form body always fails with `client_id ... is required and cannot be empty`, naming a parameter it did supply. `httpmw` reads the body, `RequiredNotEmpty` reads the query. Pre-existing. | Misleading diagnostic | | 5 | A misspelled parameter is invisible from every angle at default verbosity. The client gets a 302 with a code, its redirect is silently replaced by the registered one, and the `ignoring unrecognized authorization parameters` line is `logger.Debug` so it is not emitted. | Diagnosability | | 6 | `resource` sent twice answers `invalid_target` rather than `invalid_request`. RFC 8707 §2 frames `invalid_target` as being about the resource value; a duplicated parameter is a malformed request under RFC 6749 §3.1. | Low, error code choice | | 7 | `resource=https://api.example.com/#` (trailing `#`, empty fragment) is accepted and stored with the `#` intact, so the persisted audience is not textually equal to the fragment-free form. `url.Parse` maps empty and absent fragments both to `Fragment == ""`. Pre-existing, in `tokens.go`. | Defect, low severity | </details> **One note for anyone re-running this.** Every request below depends on `$CHALLENGE` from the most recent `new_pkce` call. Forgetting to call it sends an empty `code_challenge`, which fails with the fixed "is required and cannot be empty" message at 98 characters and looks exactly like a cap or a validator failing to fire. That cost one wrong measurement during this run before the numbers below were taken. <details> <summary>Shell helpers used throughout</summary> ```bash export BASE_URL=http://localhost:3000 export AUTH_HEADER="Coder-Session-Token: $(cat ./.coderv2/session)" export PGPASSWORD=$(cat ./.coderv2/postgres/password) export PGPORT=$(cat ./.coderv2/postgres/port) pg() { psql -h localhost -p "$PGPORT" -U coder -d coder -tAc "$1"; } plant_callback() { pg "UPDATE oauth2_provider_apps SET callback_url = '$2' WHERE id = '$1';"; } urlenc() { jq -rn --arg v "$1" '$v|@uri'; } # 43 unreserved characters every time. Base64url of the raw 32 bytes. new_pkce() { VERIFIER=$(openssl rand 32 | base64 | tr -d '\n=' | tr '+/' '-_') CHALLENGE=$(printf '%s' "$VERIFIER" | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_') STATE=$(openssl rand -hex 16) } authz_url() { local url="$BASE_URL/oauth2/authorize?client_id=$1&response_type=code" url="$url&redirect_uri=$(urlenc "$2")&state=$STATE" url="$url&code_challenge=$CHALLENGE&code_challenge_method=S256" printf '%s' "$url" } # The core assertion of the whole run: what answer, delivered where. answer() { curl -s -o /dev/null -D - -X "$1" "$2" -H "$AUTH_HEADER" | grep -iE '^HTTP/|^location:'; } show_callback() { python3 - "$1" <<'PY' import sys, urllib.parse q = urllib.parse.urlparse(sys.argv[1]).query for k, v in urllib.parse.parse_qsl(q, keep_blank_values=True): print(f"{k} = {v}") PY } ``` Fixtures, all registered through DCR: ```text plat479-plain http://localhost:9876/callback plat479-query http://localhost:9876/callback?tenant=acme&error=stale plat479-corrupt http://localhost:9876/callback (overwritten by psql in 12 to 16) plat479-reserved http://localhost:9876/callback?tenant=acme&code=FAKECODE&error=stale&error_description=oldmsg&state=oldstate&keep=yes plat479-ci http://localhost:9876/callback (scope allowlist coder:workspaces.access) ``` </details> <details> <summary>1. Baseline: a well-formed request still succeeds</summary> ```bash new_pkce; answer GET "$(authz_url "$APP_ID" http://localhost:9876/callback)" new_pkce; answer POST "$(authz_url "$APP_ID" http://localhost:9876/callback)" ``` ```text === GET === HTTP/1.1 200 OK === POST === HTTP/1.1 302 Found Location: http://localhost:9876/callback?code=coder_dzvaCPf9W1_...&state=e321bd5d5b594a1fae8a2d5da654b84a ``` `GET` carries no `Location` at all, so the consent page really rendered rather than redirecting. The `state` echoed is the one sent. The code persisted as `coder:all`, this app having no allowlist, with `redirect_uri` recorded because the request supplied one. Also driven through a real browser rather than curl, so the consent form's `nosurf` token is the one submitted rather than the session-token header alone. Clicking **Allow** delivered `code` and `state` to the callback. Both request shapes agree on the success case. Not to be misread: `oauth2_provider_app_codes` holds one row after both POSTs, not two. `ProcessAuthorize` opens its transaction with `DeleteOAuth2ProviderAppCodesByAppAndUserID`, so a row count is not a count of successful authorizations. That predates this PR. </details> <details> <summary>2 to 4. A rejected parameter now reaches the client's callback</summary> ```bash new_pkce BAD="$BASE_URL/oauth2/authorize?client_id=$APP_ID&response_type=code" BAD="$BAD&redirect_uri=$(urlenc http://localhost:9876/callback)&state=$STATE" BAD="$BAD&code_challenge=short&code_challenge_method=S256" answer POST "$BAD" answer GET "$BAD" ``` ```text HTTP/1.1 302 Found Location: http://localhost:9876/callback?error=invalid_request&error_description=Invalid+query+params%3A+code_challenge%3A+must+be+43+to+128+characters+from+the+unreserved+character+set+%5BA-Za-z0-9-._~%5D&state=e89f4d98d4c8c347e5ae25175748b731 ``` Identical on both verbs. Decoded: ```text error = invalid_request error_description = Invalid query params: code_challenge: must be 43 to 128 characters from the unreserved character set [A-Za-z0-9-._~] state = e89f4d98d4c8c347e5ae25175748b731 ``` The `GET` body confirms no page was rendered, against the baseline: | Request | Body size | Occurrences of "Allow" | |---|---|---| | Malformed `code_challenge` | 263 bytes | 0 | | Well formed (scenario 1) | 4928 bytes | 2 | 263 bytes is Go's redirect stub. There is no consent form in it, so there is no Allow button to press for a request the server has already refused. Multiple failures are reported together (scenario 4): ```text error_description = Invalid query params: code_challenge: must be 43 to 128 characters from the unreserved character set [A-Za-z0-9-._~]; resource: must be an absolute URI without fragment ``` The separator is `; `, and no entry uses the older `field: x detail: y` shape. The code is `invalid_request` rather than `invalid_target` even though `resource` is one of the two failures, which is the scenario 22 guard firing. The same request also carried `code_challenge_method=plain`, which is invalid and is *not* mentioned, because the method is validated after `extractAuthorizeParams` returns and a parse failure short-circuits first. Sent alone it is rejected as `code_challenge_method 'plain' is not supported; use 'S256'`, with no `Invalid query params: ` prefix. That difference is observation 1. </details> <details> <summary>5 and 6. The redirect_uri carve-out keeps the answer here</summary> ```bash new_pkce EVIL="$BASE_URL/oauth2/authorize?client_id=$APP_ID&response_type=code" EVIL="$EVIL&redirect_uri=$(urlenc http://evil.example/steal)&state=$STATE" EVIL="$EVIL&code_challenge=$CHALLENGE&code_challenge_method=S256" answer POST "$EVIL"; answer GET "$EVIL" ``` ```text HTTP/1.1 400 Bad Request HTTP/1.1 400 Bad Request ``` **The absence in that output is the assertion**: no `Location` on either verb, so nothing redirects to `evil.example`. Stronger, `evil.example` appears zero times in either full response, headers and body together, so the attacker-supplied host is not reflected into the page either. ```json {"error":"invalid_request","error_description":"Invalid query params: redirect_uri: Query param \"redirect_uri\" must exactly match http://localhost:9876/callback"} ``` The `GET` page names the *registered* callback as the value the parameter had to match, which is the app's own configuration and safe to display. An unparseable `redirect_uri` reaches the same outcome by a different route, having never parsed at all: ```json {"error":"invalid_request","error_description":"Invalid query params: redirect_uri: Query param \"redirect_uri\" must be a valid url: parse \"http://%zz\": invalid URL escape \"%zz\""} ``` That message echoes the caller's value, unlike the mismatch case, so the `GET` path was probed with markup in it: ```text input: http://%zz"><img src=x onerror=BAD> page: ...must be a valid url: parse "http://%zz\"><img src=x onerror=BAD>"... literal `<img` in body: 0 entity encoded `<img`: 1 ``` Escaped correctly by the template layer. Recorded as a negative result so a future change to the error page has something to regress against. One note for the docs: this detail contains colons, so a client splitting `field: reason` entries must split each on its **first** colon only. </details> <details> <summary>7 to 10. The client_id carve-out, and its negatives</summary> A repeated `client_id` in an otherwise perfect request: ```text HTTP/1.1 400 Bad Request (both verbs, no Location) {"error":"invalid_request","error_description":"Invalid query params: client_id: Query param \"client_id\" provided more than once, found 2 times. Only provide 1 instance of this query param."} ``` This is the one carve-out not implied by the response's shape. The `redirect_uri` matched the registration, so `canRedirect()` was true and there *was* somewhere to send the answer; it was withheld because `extractAuthorizeParams` assigns `failure.redirect` only when `clientIDInDoubt` is false. **The `default` branch of that function is unreachable over HTTP** (observation 3). `httpmw` resolves the app from `r.URL.Query().Get("client_id")`, so `app.ID` and the parsed value derive from the same string and cannot disagree. Three experiments: ```text one query client_id, bad challenge -> 302 delivered two DIFFERENT client_id values -> 400, caught by the repeated branch query names app A, form body names app B -> 302 to app A; the body is ignored ``` The negatives matter as much as the positives. An identifier absent from the query but present in the `POST` form body is **delivered**, correctly: ```text HTTP/1.1 302 Found Location: ...error=invalid_request&error_description=Invalid+query+params%3A+client_id%3A+...is+required+and+cannot+be+empty%3B+code_challenge%3A+... ``` That is observation 4: the description says `client_id` is required for a request that supplied it. With everything else valid the same shape still fails, while the identical request carrying `client_id` in the query succeeds with a code. `RequiredNotEmpty` reads the query; `httpmw` reads the body. Pre-existing, and arguably the parser is the layer in the right, since RFC 6749 §4.1.1 puts authorization parameters in the query string and §3.2.1 is the endpoint that uses a form body. A braced upper-case UUID is delivered, and succeeds end to end: ```text {UPPERCASE} client_id, bad challenge -> 302, description names ONLY code_challenge {UPPERCASE} client_id, all valid -> 302 with a real code GET, all valid -> 200, consent page, heading resolves to the app's real name ``` `uuid.Parse` accepts that spelling and `httpmw` resolved through it, so a string comparison here would withhold an answer the client is entitled to, for no reason it could diagnose. </details> <details> <summary>11. A repeated state is delivered, not withheld</summary> ```text HTTP/1.1 302 Found (both verbs) error = invalid_request error_description = Invalid query params: state: Query param 'state' provided more than once, found 2 times. Only provide 1 instance of this query param. ``` Reading `state` happens inside `newAuthorizeResponse`, the same function that runs the `redirect_uri` match. A count of errors across those two lines would charge this to the redirect carve-out and answer 400. It answered 302, so the field-specific test is doing the work. The asymmetry with scenario 7 is deliberate: a repeated `client_id` leaves the server unsure whose callback it holds, a repeated `state` leaves the callback fully settled. The response carries **no `state`**, since `parseSingle` collapsed the repeat to the empty string and `withQuery` only sets it when non-empty. Unavoidable rather than wrong: picking one of two values could satisfy a CSRF check the client meant to be strict. </details> <details> <summary>12 to 16. A corrupt registration outranks everything</summary> DCR refuses both planted values, which is why `psql` is needed and why 500 is the right answer: a stored one is not something a client did. ```json {"error":"invalid_client_metadata","error_description":"invalid redirect_uris: redirect URI at index 0: redirect URI uses dangerous scheme javascript which is not allowed"} {"error":"invalid_client_metadata","error_description":"invalid redirect_uris: redirect URI at index 0 is not a valid URL: parse \"http://%zz\": invalid URL escape \"%zz\""} ``` ```bash plant_callback "$APP_CORRUPT_ID" 'javascript:alert(1)' new_pkce CORRUPT="$BASE_URL/oauth2/authorize?client_id=$APP_CORRUPT_ID&response_type=code&state=$STATE" CORRUPT="$CORRUPT&code_challenge=$CHALLENGE&code_challenge_method=S256" answer POST "$CORRUPT"; answer GET "$CORRUPT" ``` ```text HTTP/1.1 500 Internal Server Error {"error":"server_error","error_description":"The application's registered callback URL is not usable"} GET: HTTP/1.1 500, page reads "500 - Invalid Callback URL", 3346 bytes, 0 Allow buttons ``` The unparseable callback is **indistinguishable to the caller**: same description, and a `GET` page of exactly 3346 bytes, matching byte for byte. Two different defects in stored state, one answer. Correct, since neither is actionable by the client. Note the request carried no `redirect_uri` of its own and still failed, because the scheme is checked on the *registered* URL before the client's value is consulted. Precedence was tested against all three competitors, not one: | Also wrong in the request | Would answer alone | Actually answered | |---|---|---| | `code_challenge=short` | 302 to the callback | **500** | | `client_id` repeated | 400 kept here | **500** | | `redirect_uri` mismatched | 400 kept here | **500** | Reading the source, this is stronger than precedence. `newAuthorizeResponse` runs at `authorize.go:250`, before any of `client_id`, `code_challenge`, `scope` or `resource` is read, and the early return discards whatever `p.Errors` already held. In rows two and three those failures are never detected at all, so `kind()` ranking `failureCorruptRegistration` first is belt and braces for a failure constructed some other way. **The value is logged and never echoed.** A distinctive marker was planted and five requests fired: ```text [erro] coderd: oauth2 app has an unusable registered callback URL request_id=01ca4796-c743-4923-a06b-afc1f3decc95 app_id=12f9ca3b-7a2c-4c54-8ed7-723698c62bd1 callback_url="javascript:alert(\"plat479-6d-marker\")" error= redirect URI uses dangerous scheme javascript which is not allowed: codersdk/oauth2_validation.go:116 ``` Five requests, five lines, in request order. Occurrences of the marker in the POST body, the rendered GET page, and the response headers: **0, 0, 0**. The `request_id` joins that line to the request log entry whose `response_body` field holds the vague message, so the server's own record confirms what it sent for the same request rather than a replay. The log is also where the two causes diverge, naming `validateScheme` and its source line, which the response cannot do. Restoring the row restores normal behaviour immediately, with no restart: ```text POST -> 302 with a code GET -> 200 consent page ``` That is a control rather than housekeeping: it rules out the app having been poisoned lastingly, and rules out a cached parse outliving the `UPDATE`. </details> <details> <summary>17 and 18. Every unsupported response_type gets one code</summary> | `response_type` sent | `error` | `error_description` | |---|---|---| | `token` | `unsupported_response_type` | Only response_type=code is supported | | `banana` | `unsupported_response_type` | Only response_type=code is supported | | `code token` | `unsupported_response_type` | Only response_type=code is supported | | `CODE` | `unsupported_response_type` | Only response_type=code is supported | | `code_extra` | `unsupported_response_type` | Only response_type=code is supported | | empty string | `invalid_request` | Invalid query params: response_type: Query param 'response_type' is required and cannot be empty | `token` has a Go constant behind it and `banana` does not, so their agreement is what reading the value as text bought. Three rows are more interesting than `banana`: `code token` is a legal RFC 6749 §3.1.1 space-delimited list used by OIDC's hybrid flow, `CODE` confirms the comparison is case sensitive, and `code_extra` confirms it is equality rather than a prefix match. The empty string is correctly the exception. A valueless parameter is *missing*, not unsupported, and RFC 6749 §3.1 requires it be treated as omitted. **The error is in the query, not the fragment.** `response_type=token` is the implicit grant's own value, so a fragment would be arguable, but this deployment advertises `"response_types_supported":["code"]` alone, and a fragment is never sent to the server, so it would be unreadable to the client's backend. PKCE is not recast, which the controls show: | `response_type`, no `code_challenge` sent | `error` | |---|---| | `token` | `unsupported_response_type`, zero mentions of `code_challenge` | | `banana` | `unsupported_response_type`, zero mentions of `code_challenge` | | `code` | `invalid_request`, naming `code_challenge` | Without the `if params.responseType == responseTypeCode` guard, a client sending `token` would be told its `code_challenge` was missing, add one, resend, and be told the same thing again. </details> <details> <summary>19 to 21. Unrecognized parameters ignored, repeats still rejected</summary> Eight unknown parameters, each on an otherwise valid request, every one issuing a code: | Unknown parameter | Result | |---|---| | `nonce`, `prompt`, `login_hint`, `acr_values`, `max_age`, `ui_locales` | code issued | | `code_challenge_methods=S256` | code issued | | `REDIRECT_URI=http://evil.example/steal` | code issued, **to the registered callback** | Five of those are OpenID Connect Core parameters, so an OIDC client pointed here degrades to plain OAuth2 rather than failing. The ignored parameters are **dropped, not forwarded**: the callback receives only `code` and `state`. The last two rows are the ones with teeth. Query parameter names are case sensitive, so `REDIRECT_URI` must be ignored, and a misspelled `redirect_url` likewise: ```text case variant alongside a valid redirect_uri : Location host localhost:9876 case variant as the only redirect parameter : Location host localhost:9876 redirect_url typo, no valid redirect_uri : Location host localhost:9876, code issued evil.example occurrences in any Location : 0 ``` The consent page renders for the typo case too, and even its cancel link targets the registered callback. A parser matching names case-insensitively would have redirected to `evil.example` with a valid code attached. **Observation 5.** The `ignoring unrecognized authorization parameters` line was never emitted. Three requests carrying a distinctive marker produced no new server output at all, because the call is `logger.Debug` (`authorize.go:307`) and the deployment runs with `verbose: null`. The behaviour is correct and required by RFC 6749 §3.1, but at default verbosity the misspelling is invisible from every angle: the client sees a 302 with a code, its redirect is silently replaced, and nothing is logged. The comment claiming the typo "surfaces here" is optimistic. Repeats of known parameters are still rejected, which the replacement of `ErrorExcessParams` had to leave alone: | Repeated parameter | HTTP | `error` | Answered at | |---|---|---|---| | `code_challenge` | 302 | `invalid_request`, two entries one field | client callback | | `code_challenge_method` | 302 | `invalid_request` | client callback | | `response_type` | 302 | `invalid_request` | client callback | | `redirect_uri` | 400 | withheld | **Coder** | | `scope` | 302 | `invalid_request` | client callback | | `resource` | 302 | `invalid_target` | client callback | `redirect_uri` being withheld confirms the carve-out keys on the field name rather than the kind of mistake. `code_challenge` collects two entries for one mistake, because `parseSingle` collapses the value to empty and the PKCE block then reports it missing. The `resource` row is observation 6. </details> <details> <summary>22 and 23. resource, its own code, and the fragment rule</summary> Both sides of the rule, since a check enforced too broadly would reject a valid URN: | `resource` sent | `error` | Stored `resource_uri` | |---|---|---| | `not a uri` | `invalid_target` | not stored | | `/api` relative | `invalid_target` | not stored | | `https://api.example.com` | none, code issued | as sent | | `https://api.example.com/v1?q=1` | none, code issued | as sent, query is not a fragment | | `urn:example:resource` | none, code issued | as sent | | empty string | none, code issued | `NULL`, same as omitted | `invalid_target` applies only when `resource` is the sole failure. Add any second failure in any other field and the code becomes `invalid_request` naming both, which is the retry-loop guard: ```text resource + bad code_challenge -> invalid_request, both named resource + repeated scope -> invalid_request, both named resource + repeated state -> invalid_request, both named resource alone (control) -> invalid_target ``` Fragments are rejected in all three placements, with or without a path: ```text https://api.example.com/#x -> invalid_target https://api.example.com#x -> invalid_target https://api.example.com/v1#frag -> invalid_target https://api.example.com/# -> code issued, stored as https://api.example.com/# ``` **The last row is observation 7, and it predates this PR.** Go cannot represent the distinction: ```text https://a.example.com/ Fragment="" String()="https://a.example.com/" https://a.example.com/# Fragment="" String()="https://a.example.com/" https://a.example.com/#x Fragment="x" String()="https://a.example.com/#x" ``` `url.Parse` collapses "no fragment" and "empty fragment", so `if u.Fragment != ""` in `validateResourceParameter` (`coderd/oauth2provider/tokens.go:591`) cannot see a trailing `#`. RFC 3986 §3.5 permits a zero-length fragment, which RFC 8707 §2's "MUST NOT include a fragment component" reads as forbidding. Validation parses the value but persistence stores the raw string, so the `#` reaches `resource_uri` and the stored audience is not textually equal to the fragment-free form. A client appending a harmless-looking `#` would get a token bound to an audience nothing matches. The authorize-side call to `validateResourceParameter` is context in this diff rather than an added line; this PR added the `invalid_target` code for the failure. </details> <details> <summary>24 and 25. The description cap and character set</summary> The obvious probe does not exercise the cap: a 4000-character `code_challenge` produces a 116-character description, because that message is fixed text and never quotes the offending value. The descriptions that echo caller input come from a validator's own error. | Request | `error` | Description length | Truncated | `Location` length | |---|---|---|---|---| | `code_challenge_method` = 4000 chars | `invalid_request` | 2060 | **yes** | 2176 | | `scope` = 4000 chars | `invalid_scope` | 2060 | **yes** | 2174 | | `code_challenge_method` = 2000 chars | `invalid_request` | 2035 | no | 2147 | | `code_challenge` = 4000 chars | `invalid_request` | 116 | no | 234 | 2060 is exactly `maxErrorDescription` plus `" (truncated)"`. Row three is the boundary control, so the cap fires on length rather than on the presence of echoed input. Measured from the `Location` header rather than a log line, since the stated reason for the cap is that the header must survive intermediary proxies; the longest observed was 2176 bytes. Both verbs agree, the cap living in the shared `redirectAuthorizeError`: ```text GET, long method : desc_len=2060 truncated=True GET, long scope : desc_len=2060 truncated=True ``` The character set is enforced independently, later, in `errorURL`: ```text input: pl"ain\back<newline>tab<tab>end~unicode description: unsupported code_challenge_method: pl'ainback tab end~ nicode chars outside the RFC 6749 §4.1.2.1 set: [] ``` `"` becomes `'`, `\` is dropped so it cannot escape the rewritten quote, and anything below 0x20 or above 0x7E becomes a space. Checked programmatically rather than by eye. The RFC sets no length limit, so the 2048 cap is policy; the character set is conformance. </details> <details> <summary>26. The registered callback query, retained except reserved names</summary> `plat479-reserved` registers a callback carrying all four reserved names plus two ordinary ones, and DCR stores it unchanged: ```text http://localhost:9876/callback?tenant=acme&code=FAKECODE&error=stale&error_description=oldmsg&state=oldstate&keep=yes ``` Success response, no `redirect_uri` sent: ```text code = coder_WXwBpvYR5u_... keep = yes state = 516abeea9976119eb33ddc10f0ca04ed tenant = acme ``` Failure response, same app: ```text error = invalid_request error_description = Invalid query params: code_challenge: must be 43 to 128 characters... keep = yes state = 3a17bdd75dfcf740bda36124cd161fb7 tenant = acme ``` Stale registered values in either response: `FAKECODE` 0, `stale` 0, `oldmsg` 0, `oldstate` 0. Without the deletion step a registered `error=stale` would ride out on the **success** response, and a client reading `error` before `code`, the conventional order, would discard a valid authorization code. `FAKECODE` is the mirror image on a failure response. The cancel link obeys the same rule, being built by the same `withQuery` path: ```text http://localhost:9876/callback?error=access_denied&error_description=The+resource+owner...&keep=yes&state=e15b5f0f...&tenant=acme ``` RFC 6749 §3.1.2 requires the registered query be retained when adding parameters, which is the `tenant` and `keep` half. It does not say what to do when the registered query collides with the response parameters §4.1.2 adds, so dropping the registered copies is the resolution that keeps the client's read unambiguous. </details> <details> <summary>27 and 28. Scope rejections, and declining consent</summary> `plat479-ci` carries the allowlist `coder:workspaces.access`. ```text scope=template:update (outside) -> error = invalid_scope 'template:update': scope requests permissions beyond this app's allowed scopes GET, same request -> 302, 225 bytes, 0 Allow buttons, no consent page scope=workspace:ssh (covered) -> code issued, persisted scope workspace:ssh GET, same request -> 200, 5182 bytes, consent page naming the scope ``` `workspace:ssh` is granted although the allowlist names the composite rather than that scope, which is the coverage-not-spelling rule from #28045. This run only checks that the changes here left it delivering the same answers. The consent page for a narrow scope reads differently from the unrestricted one: "to access your **admin** account with these permissions?", the scope as `<li role="listitem">workspace:ssh`, and the caution "These are technical permission names. Grant them only to an application you trust." The scope description shape differs from the parser's, coming from `scopeFailureResponse` rather than the field join, which is the third producer behind observation 1. Declining consent, clicked in a real browser: ```text error = access_denied error_description = The resource owner or authorization server denied the request state = a8bd538b365b61f54586892c73968b80 ``` The page carries exactly one `href` and it is the cancel link. The `state` matches, there is no `code`, and the code count for the app is unchanged, the link being a plain `GET` to the client's callback that never reaches `ProcessAuthorize`. The description is RFC 6749 §4.1.2.1's own definition of `access_denied`, word for word. That shared construction is what makes the `#nosec G203` annotation on `CancelURI` sound: the URL is injected as a trusted `htmltemplate.URL`, safe only because `newAuthorizeResponse` validated the registered scheme before the response object could exist. Scenario 12 is the other half, an app with a rejected scheme never reaching this page at all. </details> <details> <summary>Where the consent page renders, across every GET case</summary> Only `GET` can render anything, and body size identifies which of four renderers ran, so this is the table to check when a status code alone looks right. | Case | Status | Body bytes | Allow buttons | Rendered | |---|---|---|---|---| | well formed | 200 | 4928 | 1 | **consent page** | | bad `code_challenge` | 302 | 263 | 0 | redirect stub | | two bad fields | 302 | 319 | 0 | redirect stub | | mismatched `redirect_uri` | 400 | 3814 | 0 | 400 error page | | unparseable `redirect_uri` | 400 | 3846 | 0 | 400 error page | | repeated `client_id` | 400 | 3846 | 0 | 400 error page | | `client_id` in form body only | 400 | 77 | 0 | `httpmw` JSON, unstyled | | `{UPPERCASE}` `client_id` | 200 | 4940 | 1 | **consent page** | | repeated `state` | 302 | 239 | 0 | redirect stub | | corrupt callback, blocked scheme | 500 | 3346 | 0 | 500 error page | | corrupt callback, unparseable | 500 | 3346 | 0 | 500 error page | | `response_type=token` | 302 | 187 | 0 | redirect stub | | `banana` / `code token` / `CODE` | 302 | 187 | 0 | redirect stub | | empty `response_type` | 302 | 243 | 0 | redirect stub | | `token`, no PKCE | 302 | 187 | 0 | redirect stub | | `code`, no PKCE | 302 | 245 | 0 | redirect stub | | unknown params, valid request | 200 | 4932 | 1 | **consent page** | | `redirect_url` typo, valid request | 200 | 4932 | 1 | **consent page** | | `resource` malformed | 302 | 214 | 0 | redirect stub | | `resource` valid absolute URI | 200 | 4932 | 1 | **consent page** | | `resource=.../#` empty fragment | 200 | 4932 | 1 | **consent page**, observation 7 | | reserved params in registered callback | 200 | 4973 | 1 | **consent page** | | `scope` outside allowlist | 302 | 225 | 0 | redirect stub | | `scope` covered by allowlist | 200 | 5182 | 1 | **consent page** | The invariant: the page renders if and only if `extractAuthorizeParams` returned no failure, so no input asks the owner to approve a request the server has already refused. The `evil.example` rows are worth reading carefully rather than alarming: RFC 6749 §3.1 requires a case variant and a typo to be ignored, so from the server's view nothing was wrong with either request, and both cancel links target the registered callback. About 4930 bytes is the consent page, about 3800 the 400 page, 3346 the 500 page, 77 the unstyled `httpmw` JSON, and low hundreds is Go's redirect stub, whose length tracks the `Location` it embeds. Every `unsupported_response_type` answer is exactly 187 bytes because they share one description. Small variations within a renderer are expected: the consent page moves 4928 to 4940 with a fresh CSRF token and the app name. As a regression signal, body size is more sensitive than the status line. A change that rendered a page alongside a redirect would still report 302, but the stub would come back at thousands of bytes rather than hundreds. </details> <details> <summary>Cleanup</summary> ```bash for id in $(pg "SELECT id FROM oauth2_provider_apps WHERE name LIKE 'plat479%';"); do curl -s -X DELETE "$BASE_URL/api/v2/oauth2-provider/apps/$id" -H "$AUTH_HEADER" \ -o /dev/null -w "$id -> %{http_code}\n" done pg "SELECT count(*) FROM oauth2_provider_apps WHERE name LIKE 'plat479%';" ./scripts/coder-dev.sh oauth2-provider dcr disable ``` All planted callback rows were restored before deletion, so no fixture was left holding a value DCR would refuse. </details> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>

BobbyHo and others added 9 commits August 10, 2026 15:31

Merge branch 'main' into coder-oauth2-scope-enforcement-plat-470

5ca9beb

Merge branch 'main' into coder-oauth2-scope-enforcement-plat-470

08f2c9a

Merge remote-tracking branch 'origin/coder-oauth2-scope-enforcement-p…

e98cac8

…lat-470' into coder-oauth2-scope-enforcement-plat-470-phrase-2

github-actions Bot assigned BobbyHo Aug 11, 2026

coder-agents-review Bot suggested changes Aug 12, 2026

View reviewed changes

BobbyHo and others added 2 commits August 12, 2026 07:54

Merge branch 'main' into coder-oauth2-scope-enforcement-plat-470

852d983

BobbyHo added 3 commits August 12, 2026 11:26

Merge branch 'coder-oauth2-scope-enforcement-plat-470' of https://git…

a95174e

…hub.com/coder/coder into coder-oauth2-scope-enforcement-plat-470

BobbyHo mentioned this pull request Aug 12, 2026

feat: add oauth2 scope columns and single-use delete queries #28007

Merged

BobbyHo and others added 6 commits August 12, 2026 13:06

Merge branch 'main' into coder-oauth2-scope-enforcement-plat-470

02076e1

Base automatically changed from coder-oauth2-scope-enforcement-plat-470 to main August 13, 2026 16:58

coder-agents-review Bot reviewed Aug 13, 2026

View reviewed changes

BobbyHo added 2 commits August 13, 2026 18:35

BobbyHo and others added 2 commits August 13, 2026 21:29

Merge branch 'main' into coder-oauth2-scope-enforcement-plat-470-phra…

ab90213

…se-2

BobbyHo marked this pull request as ready for review August 13, 2026 23:24

BobbyHo requested a review from Emyrk August 13, 2026 23:25

This was referenced Aug 14, 2026

feat(coderd/rbac): compare scopes by permission coverage #28167

Merged

feat: negotiate and persist authorization scope #28178

Merged

feat: report the negotiated scope to the user and the client #28179

Merged

Merge branch 'main' into coder-oauth2-scope-enforcement-plat-470-phra…

7f84436

…se-2

BobbyHo marked this pull request as draft August 18, 2026 19:53

designertyler mentioned this pull request Aug 20, 2026

feat(site): OAuth2 device, consent and public-client admin screens (early sketches) #28390

Closed

github-actions Bot added the stale This issue is like stale bread. label Aug 27, 2026

github-actions Bot closed this Aug 31, 2026

BobbyHo mentioned this pull request Aug 31, 2026

feat: reject secret creation and fix reporting for public clients #28097

Merged

This was referenced Sep 1, 2026

fix: deliver four more authorize errors to the client #28450

Merged

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

Merged

#	Scenario	Result
1	No allowlist, no request, stays unrestricted	Pass
2	Request narrower than the allowlist is granted by permission coverage, not name matching	Pass
3	Request outside the allowlist is refused, including one sharing a resource prefix	Pass
4	Omitted scope defaults to the app's allowlist (RFC 6749 section 3.3)	Pass
5	Rejection is delivered to the client's own callback with `error`, `error_description`, and `state`, on both verbs	Pass
6	Names outside the external catalog are rejected, both unrecognized and internal-only	Pass
7	Legacy aliases are canonicalized and duplicates collapsed before persisting	Pass
8	Consent page lists the negotiated scope, and an unrestricted grant keeps the full-access wording	Pass
9	DCR apps with non-catalog scopes fail in both directions, with the registered names in the message	Pass, as designed
10	An unregistered `redirect_uri` is never redirected to, so the new error redirect cannot be aimed	Pass
11	Issued token is still unrestricted, confirming the phase boundary	Pass, expected

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

BobbyHo commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Manual Tests

Uh oh!

linear-code Bot commented Aug 11, 2026

Uh oh!

BobbyHo commented Aug 12, 2026

Uh oh!

coder-agents-review Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Findings

Contested and acknowledged

CRF-15 (Nit, authorize_test.go:262) - authorizeRequest retraces doAuthorizeRequest

CRF-9 (P3, registration.go:111) - DCR registration accepts non-catalog scopes

CRF-16 (Nit, authorize_internal_test.go:15) - Duplicated catalog-membership constants

CRF-17 (Note, authorize.go:39) - Single-use noScopeAllowlist abstraction

CRF-19 (Note, apps.go:102) - Admin-created apps hardcode sql.NullString{}

CRF-20 (Note, authorize.go:378) - Persisted grantedScope has no reader yet

CRF-22 (Note, validation_test.go:544) - TestOAuth2ClientScopeValidation duplicated across two files

Law analysis

Round log

Round 1

Round 2

Round 3

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BobbyHo commented Aug 12, 2026

Uh oh!

github-actions Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Docs preview

Uh oh!

BobbyHo commented Aug 12, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BobbyHo commented Aug 13, 2026

Round 3 close-out

CRF-26 has no thread, so recording it here

Not adopted

Uh oh!

BobbyHo commented Aug 13, 2026

Uh oh!

coderagents Bot commented Aug 13, 2026

Documentation Check

Updates Needed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

BobbyHo commented Aug 11, 2026 •

edited

Loading

coder-agents-review Bot commented Aug 12, 2026 •

edited

Loading

CRF-15 (Nit, `authorize_test.go:262`) - `authorizeRequest` retraces `doAuthorizeRequest`

CRF-9 (P3, `registration.go:111`) - DCR registration accepts non-catalog scopes

CRF-16 (Nit, `authorize_internal_test.go:15`) - Duplicated catalog-membership constants

CRF-17 (Note, `authorize.go:39`) - Single-use `noScopeAllowlist` abstraction

CRF-19 (Note, `apps.go:102`) - Admin-created apps hardcode `sql.NullString{}`

CRF-20 (Note, `authorize.go:378`) - Persisted `grantedScope` has no reader yet

CRF-22 (Note, `validation_test.go:544`) - `TestOAuth2ClientScopeValidation` duplicated across two files

github-actions Bot commented Aug 12, 2026 •

edited

Loading