feat: validate and persist OAuth2 authorization scope by BobbyHo · Pull Request #28045 · coder/coder

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.

#	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

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.

BobbyHo · 2026-08-13T15:57:49Z

/coder-agents-review

coder-agents-review

Round 2 blocked.

Round 1 landed cleanly on 20 of 22 findings. Fifteen were fixed in code (CRF-1, CRF-2, CRF-3, CRF-4, CRF-5, CRF-6, CRF-7, CRF-8, CRF-10, CRF-11, CRF-12, CRF-13, CRF-14, CRF-18, CRF-21). Four were accepted with rationale (CRF-15, CRF-19, CRF-20, CRF-22); two are contested and would be judged by the panel (CRF-16, CRF-17). Notable work: rbac.CanonicalScopeName in c15059f930, the typed sentinels + errors.Is in the same commit, redirectAuthorizeError in 339714cb30, rbac.ScopesCover in 32275ac699, and the consent-page Scopes field plumbed through the render data in bcd9e9f302. TestOAuth2AuthorizeMismatchedRedirectURINotRedirected pins the precondition that keeps redirectAuthorizeError safe.

One open finding blocks this round.

CRF-9 (P3, coderd/oauth2provider/registration.go:111) — DCR registration accepts non-catalog scopes without validation.
No code change since round 1, and no substantive follow-up. Comment IC_kwDOGkVX1s8AAAABOgOLEg groups CRF-9 with CRF-6 and CRF-18 as "need decisions before implementation"; the other two in that group were subsequently implemented, CRF-9 was not, and no ticket has been linked.

The finding stands: a client can DCR-register today with scope: "openid" and immediately fail every /oauth2/authorize, either explicitly or by omission, with the compat-break error message this PR now ships. The read-side redirect makes that error reach the client cleanly, which is progress, but it does not change that the failure was avoidable at the write boundary. A one-line Valid: req.Scope != "" plus IsExternalScope (or CanonicalScopeName) check at registration.go:111/:329 rejects with RFC 7591 §3.2.1 invalid_client_metadata before a client_id is issued.

One of three responses is needed to unblock further review: land the write-side check in this PR; file a follow-up ticket and defer with the ticket linked here; or explain in a reply why registration-time catalog rejection is the wrong choice on its merits and why the current authorize-time failure is preferable. A bare "won't fix" is not a resolution, especially given this is the only finding from round 1 that received neither code nor a defense.

No Netero, no panel this round: per the review process, a silent open finding blocks reviewer spawn until the author responds or pushes fixes. Once CRF-9 is disposed one of the three ways above, the next round will pick up CRF-16 and CRF-17 (contested, need panel judgment) alongside the new-diff coverage.

🤖 This review was automatically generated with Coder Agents.

BobbyHo · 2026-08-13T16:28:49Z

CRF-9: deferred to PLAT-503, with the reasoning below rather than a bare deferral.

Taking the "file a follow-up and link it" option, plus the explanation the third option asks for, because I don't think either candidate remedy is ready to land as described.

Where the risk actually sits. The read side already holds the safety property. An app whose allowlist filters to nothing is rejected, not granted, and that branch exists specifically because falling through would grant strictly more than the allowlist ever permitted. So what CRF-9 describes is a delayed and confusing failure rather than an authorization hole, which is consistent with it being filed P3. The redirect from CRF-6 and the message from CRF-8 also moved that failure to the app's own callback, naming the registered scopes and the remedy. That was most of the original harm, and it is addressed.

The proposed fix is not one line, and the citation is off by a section. invalid_client_metadata is defined in RFC 7591 section 3.2.2, not 3.2.1. Section 3.2.1 is the Client Information Response, and it is the section that permits an authorization server to "reject or replace any of the client's requested metadata values", which supports the alternative below rather than the rejection being proposed. Mechanically, registration metadata validation runs through req.Validate() in codersdk/oauth2_validation.go; putting a catalog check there would pull coderd/rbac into the client SDK that every CLI binary links, so it belongs in registration.go instead. That is a split-and-check over the scope string, an error naming the offending names, both the create and update paths, a decision about apps already holding non-catalog values, and tests.

And the obvious alternative is unsafe as usually described. Filtering scope through the catalog at registration and echoing the narrowed value back, per section 3.2.1, sounds like the least breaking option. It is not, because noScopeAllowlist treats "" and NULL as the same state, "no allowlist configured", which grants unrestricted access when the client omits scope. Filtering openid profile email down to "" at the write boundary would convert today's hard rejection into the most permissive grant the server can issue, for exactly the apps this PR's compatibility break exists to catch. Making it safe requires either rejecting when the filter empties the list, which collapses back into registration-time rejection for the case CRF-9 actually names, or introducing a distinct empty-allowlist state that the read side understands as "grant nothing".

Worth noting what filtering would and would not change: a partially valid allowlist such as openid workspace:read already works today, because the read side filters it to workspace:read and grants that. The only case where write-side filtering changes behavior is the all-junk case, which is the one it cannot handle safely without that extra state.

So the real decision is between rejecting at registration, which is a genuine compatibility break for clients that register non-catalog scopes and never authorize, and adding an empty-allowlist state so that filtering is safe. Both are public API contract changes that need their own compatibility note, in a PR whose stated scope is authorize-time negotiation. PLAT-503 captures the above, including the escalation trap, so the next person does not rediscover it.

Happy to be argued out of the deferral. I would rather not pick between those two shapes under review pressure and ship the escalation by accident.

BobbyHo · 2026-08-13T16:30:45Z

/coder-agents-review

…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

coder-tasks · 2026-08-13T23:27:03Z

Documentation Check

This PR implements OAuth2 authorization-code scope negotiation and enforcement (the app allowlist now bounds granted scope, out-of-catalog/over-broad requests are rejected with invalid_scope, and the consent page lists the negotiated permissions). The OAuth2 provider page has a published limitation that this change makes false.

Updates Needed

docs/admin/integrations/oauth2-provider.md (Limitations, line ~370) - The bullet No scope system - all tokens have full API access is now inaccurate. Scopes are negotiated against the app's scope allowlist and enforced at /oauth2/authorize; requesting a scope outside the app's allowlist or the external scope catalog is rejected with invalid_scope, and the consent page now lists the permissions being granted. Remove/replace this limitation and describe the new behavior (allowlist as an upper bound, RFC 6749 §3.3 default when no scope is requested, and the consent page listing).

Note

The oauth2 provider experiment is not in ExperimentsSafe, so this is arguably a below-the-bar experiment. This is flagged only because the page is already published and now carries a factually incorrect limitation, not to request net-new documentation. If maintainers prefer to hold all scope docs until the experiment graduates, dismiss this.

Automated review via Coder Agents

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

coder-agents-review Bot reviewed Aug 13, 2026

View reviewed changes

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: compare scopes by permission coverage #28167

Open

feat: negotiate and persist authorization scope #28178

Draft

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

Draft

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

Docs preview

Uh oh!

BobbyHo commented Aug 12, 2026

Uh oh!

BobbyHo commented Aug 13, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

BobbyHo commented Aug 13, 2026

Uh oh!

BobbyHo commented Aug 13, 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!

coder-tasks Bot commented Aug 13, 2026

Documentation Check

Updates Needed

Uh oh!

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