Conversation
Admin-created OAuth2 apps had no way to restrict what scope their tokens could be granted. `PostOAuth2ProviderAppRequest`/`PutOAuth2ProviderAppRequest` had no `scope` field, so `CreateApp`/`UpdateApp` always stored the app unrestricted, and `codersdk.OAuth2ProviderApp` didn't echo the value back. DCR-registered apps could already set this at registration; admin-created apps could not. Add `Scope` to both request types and to the response type. `CreateApp` and `UpdateApp` now store the submitted value, canonicalized and deduplicated with the existing `canonicalScopes` helper, and `PUT` fully replaces the allowlist (an empty value clears it back to unrestricted). No change to scope enforcement itself: the existing authorize-time negotiation already reads this column correctly for any app. Includes tests covering create/update/clear round-trips and an end-to-end check that an app created through this API is narrowed by the authorize endpoint the same way a database-seeded app already is. Generated files (swagger, typesGenerated.ts, api docs) updated via `make gen`.
Docs previewCheck 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. |
|
/coder-agents-review |
|
Chat: Review in progress (19/19 reviewers complete) | View chat deep-review v0.9.0 | Round 3 | Last posted: Round 3, 14 findings (2 P2, 2 P3, 6 Nit, 4 Note), COMMENT. Review Finding inventoryFinding inventory - PR #29313Findings
Contested and acknowledgedCRF-4 (P3, coderd/oauth2provider/apps.go:77) - Partial-invalid admin scope list silently dropped, echoed as success
CRF-3 (Note, coderd/oauth2provider/apps.go:75) - scopeAllowlist stores admin scopes without catalog validation
Round logRound 1Netero-only first pass (pre-panel gate). 2 P2, 1 Note. Reviewed against 46c904d..acd0d60. CRF-1 (DCR scope clearing) verified by orchestrator: PUT /oauth2-provider/apps/{app} uses ExtractOAuth2ProviderApp with no dynamically_registered guard (coderd.go:2016, httpmw/oauth2.go:357), and pre-PR UpdateApp preserved Scope. CRF-2 posted inline anchored at the codersdk Scope field. Panel deferred until mechanical floor is clean. Round 2Churn guard: PROCEED (2 addressed, 1 acknowledged, 0 silent). CRF-1 fixed in b3545d0 (PutOAuth2ProviderAppRequest.Scope is now *string; omit keeps existing allowlist, empty string clears). CRF-2 fixed by dropping the title scope. CRF-3 accepted by the human author (defer scope-catalog error to first authorize). Panel round to follow. Round 2 panelNetero: no findings, CRF-1 fix verified. Panel of 15 (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Kurapika, Razor, Ging-go, Ryosuke, Chopper, Gon, Leorio, Komugi, Meruem, Knov). Kurapika, Pariston, and the process reviewers confirmed the fix is at the root cause and enforcement cannot be widened (fail-closed via grantableScopes/IsExternalScope). New: CRF-4 P3 (partial-invalid scope silently dropped + GET false confirmation; two reviewers, re-raises CRF-3's acceptance premise for the mixed case), CRF-5 P3 (admin-vs-DCR canonicalization divergence in stored/echoed scope; seven reviewers converged, Ryosuke set P3), CRF-6 Note (PUT replace/patch semantic mix, structural hazard for future sensitive fields). CRF-7 (Bisky test-redundancy Note) dropped. Reviewed against 46c904d..6119cf2 (round base f5be7ae..6119cf2). Event COMMENT (no P0-P1). Round 3 panelNetero: no findings; CRF-1 and CRF-5 fixes verified. Panel of 17 (Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Kurapika, Killua, Knov, Razor, Ging-go, Ryosuke, Chopper, Komugi, Gon, Leorio; wildcards Kite, Zoro). CRF-5 confirmed fixed by all who traced it. Perf/DoS caps and linear CanonicalScopeList verified by Killua, Kurapika, Hisoka, Kite (caps on all 4 write paths, byte-check-before-split, set-based dedup). CRF-4 re-raised at P3 by 8 reviewers with new evidence: round 3 did not resolve it but shipped 3 factually-false statements that the mixed valid/invalid case disproves (codersdk/oauth2_validation.go:24 "an unknown name is rejected at authorization"; docs/admin/integrations/oauth2-provider.md:319 "a name it does not offer fails at authorization"; codersdk/oauth2.go:22 "scopes this app's tokens may be granted"). Orchestrator verified all three texts and the authorize branch (negotiateScope: grantableScopes drops unknown, non-empty allowlist proceeds, no error). Sharp structural point (Knov/Ryosuke/Chopper/Kite): writeValidScopeAllowlist is admin-only and has no non-Coder-vocabulary caller, so the DCR justification does not apply to the admin path; firstUnknownScope would close it there. Fail-closed, so P3, needs a human decision. New Nits: CRF-8 (stale noScopeAllowlist comment, 3 reviewers), CRF-9 (writeValidScopeAllowlist naming/polarity), CRF-10 (error msg repeats field name), CRF-11 (pre-1.22 counting loops), CRF-12 (reimplements slice.Unique), CRF-13 (scopeAllowlist duplicates inline spelling). New Notes: CRF-14 (whitespace-only literal echo / three-state UI trap), CRF-15 (perf-guard test has no time bound). Reviewed against 800629c..0a6a3ac. Event COMMENT (no P0-P1). About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
First-pass review only. These are mechanical findings from Netero; the full review panel has not yet reviewed this PR. The panel will review after these findings are addressed. Posting now because there is a defect the author should fix before the panel spends parallel review time.
The change is small and well-tested (73% test density), and the tests are real: they round-trip scope through GET and assert actual authorization narrowing against a live app rather than just echoing the stored field. Canonicalization and dedup are covered, and the clear-to-unrestricted path is asserted.
Severity count: 2 P2, 1 Note.
CI is red: the title job fails because the feat(coderd) scope does not name every changed file. Details inline (CRF-2).
The headline concern is CRF-1: UpdateApp now writes Scope unconditionally. Before this PR it preserved the stored value. As Netero put it, an admin editing such an app's name or icon without resending scope "widens its allowlist from restricted to unrestricted, a silent privilege escalation." The PUT route (coderd.go:2016) uses ExtractOAuth2ProviderApp with no dynamically_registered guard, so it can target DCR-registered apps, which are exactly the population that could already carry a scope allowlist. I verified the middleware and route: no client-type filter exists. Whether a current UI/client issues such a PUT is unconfirmed, but the endpoint permits it and the diff changes the behavior.
🤖 This review was automatically generated with Coder Agents.
… update
- PUT /oauth2-provider/apps/{app} rebuilt Scope from the request on
every update, so editing name/icon without resending scope reset
the allowlist to unrestricted
- other fields on this endpoint already keep their existing value
when omitted; Scope now matches
- PutOAuth2ProviderAppRequest.Scope is now *string: omit to keep the
allowlist, send "" to clear it
BobbyHo
changed the title
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2. The two P2s from the first pass are fixed and I verified both against the current tree. CRF-1 is closed at the root cause: PutOAuth2ProviderAppRequest.Scope is now a *string, so UpdateApp keeps the existing allowlist when scope is omitted and only clears it on an explicit empty string. The panel independently confirmed the fix and its test coverage (omit-keeps, replace, empty-clears), and Kurapika and Pariston both tried to widen privilege through the new write path and could not: enforcement re-filters through grantableScopes/IsExternalScope on every authorize, so an admin allowlist can only narrow, never grant. The string (POST) vs *string (PUT) split is the right type shape, not an inconsistency to harmonize later.
Severity count this round: 2 P3, 1 Note (plus the two prior P2s verified fixed).
Two things need a human decision, both fail-closed (the app ends up narrower than intended, never wider), which is why they are P3 and not higher:
- CRF-4 re-opens the premise CRF-3 was accepted on. "Defer the error to first authorize" holds only when every stored scope name is invalid. For a mix of valid and invalid names, no error ever fires: the invalid name is stored, echoed back by GET as if accepted, then silently dropped at authorize while the allowlist stays non-empty. As Hisoka put it, "there is no UI, so the API response is the only feedback channel, and it lies." Two reviewers reached this independently. Either validate names at config time (firstUnknownScope already exists and is the exact check) or restate the disposition to explicitly cover the mixed case.
- CRF-5: the admin path canonicalizes before storing while the DCR path stores raw, and this PR is the first to echo the stored bytes back. Seven reviewers converged on it. Enforcement is unaffected, but the announced scope-picker UI will read this field and see two spellings for the same allowlist.
Process note: the PR description still says "Sending an empty or no scope clears it back to unrestricted." After the CRF-1 fix that is only half true, omitting scope now keeps the allowlist. The field doc comment is correct; please update the description, since it becomes the squash-merge commit body.
🤖 This review was automatically generated with Coder Agents.
- admin writes cleaned the scope string, DCR writes stored it as sent, so the same allowlist read back two ways depending on origin - both write paths now store what the caller sent - rbac.CanonicalScopeList owns the display form, applied in db2sdk - enforcement is unchanged; grantableScopes already canonicalized on read
…oauth-app-scope-allowlist # Conflicts: # coderd/database/db2sdk/db2sdk.go # site/src/api/typesGenerated.ts # site/src/testHelpers/entities.ts
Documentation CheckThe API reference pages in this PR ( Updates Needed
Automated review via Coder Agents |
|
When DCR is enabled, an unauthenticated client can register a scope containing many distinct unknown names. Those names are retained here, so every app list/get response scans a growing slice for each token. A roughly 800 KB list of 100,000 names causes about five billion comparisons on each read, while the request limit is 4 MiB. Consider using map-based deduplication while preserving first-seen order and unknown names, and add regression coverage for a large distinct scope list. |
|
A stored scope of |
|
The OAuth2 admin guide still says management-API apps declare no allowlist, and its troubleshooting and limitations sections say administrators cannot configure one. Please update those statements for optional |
CanonicalScopeList checked each name against the names kept so far with slices.Contains, so canonicalizing a stored allowlist was quadratic in the number of distinct names. Dynamic client registration lets an unauthenticated client store a scope list of up to the 4 MiB request body limit without validating its contents, and every app list and get response canonicalizes that list, so many distinct unknown names made every read expensive. Track seen names in a set instead. First-seen order and unknown names are preserved. Add coverage for the display form and for a large distinct list.
Dynamic client registration is unauthenticated and stored the scope list as given, bounded only by the 4 MiB request body limit. Admin create and update did the same. Reject lists over 4096 bytes or 100 names on every write path. Names are still not checked against the catalog, since an unknown name is rejected at authorization where the client learns which name to drop. The admin handlers check explicitly rather than through a validate tag so the response names the limit instead of echoing the oversized value back.
…cope-allowlist' into coder-plat-475-admin-oauth-app-scope-allowlist
|
Fixed in 7e0a0fa: Also added a cap in 3aba8a6: scope lists over 4096 bytes or 100 names are rejected at both DCR registration and admin create/update, so oversized lists are no longer stored at all. Names are still not checked against the catalog, so clients sending |
…able from empty The admin app converter canonicalized the stored scope by splitting on whitespace, so a stored " " read back as "". Empty is documented as unrestricted, but authorization treats a whitespace-only allowlist as configured and rejects every request, so the response reported the opposite of how the app behaves. Return a list with no names as given. Document the state on the API field and cover it from admin create, admin update, and DCR registration.
|
Fixed in e4bf42e: |
…lowlist The OAuth2 provider guide said only Dynamic Client Registration could declare a scope allowlist and that administrators could not configure one. Both are now wrong: scope is optional on the create endpoint, and the update endpoint keeps, replaces, or clears it. Update the Scopes, troubleshooting, and limitations sections, and link the API reference. The web UI limitation stays until the form lands.
|
Updated in 12ab5a8: the Scopes section now describes the optional |
|
/coder-agents-review review the latest commits |
There was a problem hiding this comment.
Round 3. The response to feedback here has been strong. CRF-5 is fixed at the root (both write paths store raw, rbac.CanonicalScopeList normalizes once at read time in db2sdk), verified by every reviewer who traced it. The three findings from the human reviewer are addressed and hold up under fresh review: the O(n^2) read scan is gone (set-based dedup, linear), byte/name caps are enforced on all four write paths with the byte check running before the split so an oversized body is never tokenized, and the whitespace-only allowlist now reads back non-empty and still fails closed. The size-cap tests and the cross-origin spelling test are real and specific.
Severity count: 1 P3 (contested, re-raised), 6 Nit, 2 Note. No P0-P2. CRF-1's *string PUT semantics remain intact.
The one thing that needs a human decision is CRF-4, which eight reviewers independently re-raised this round. The contested premise is that catalog validation is deferred so "an unknown name is rejected at authorization." That is true for the all-invalid case (CRF-3, accepted) but false for a list that mixes valid and invalid names: the invalid name is stored, GET echoes it as configured, and at authorize grantableScopes drops it while the allowlist stays non-empty, so no error fires at config time or authorize time. The app permanently enforces a narrower allowlist than its own API advertises, with no signal on any channel. As Hisoka put it, the code "was not ready on CRF-4, and round 3 chose to leave it that way."
Round 3 hardened this into three shipped statements that the mixed case disproves, which I verified verbatim: the ValidateOAuth2ScopeList comment ("an unknown name is rejected at authorization"), docs/admin/integrations/oauth2-provider.md line 319 ("a name it does not offer fails at authorization"), and the codersdk.OAuth2ProviderApp.Scope field doc ("scopes this app's tokens may be granted"). Several reviewers noted that writeValidScopeAllowlist is admin-only and has no non-Coder-vocabulary caller, so the DCR justification does not transfer to the admin path; firstUnknownScope would close it there without touching DCR. This is fail-closed (narrower, never wider), so it is P3, not a security break. It is a human's call: validate at config time, or accept the gap and correct the three statements so the code stops asserting a guarantee it does not provide. Either way the false statements should be fixed.
The rest are Nits and forward-looking Notes for the scope-picker UI PR; details inline.
coderd/oauth2provider/authorize.go:72
Nit [CRF-8] The noScopeAllowlist comment says "admin-created apps store NULL," which this PR makes false. (Pariston, Knov, Ryosuke)
CreateApp now stores scopeAllowlist(req.Scope), and scopeAllowlist always sets Valid: true, so a newly admin-created app with no scope stores {Valid: true, String: ""}, not NULL. The function still behaves correctly (it tests String == ""), but the parenthetical now describes only pre-PR rows. Update it to say both paths may store an empty valid string and NULL only survives on pre-existing rows.
🤖
🤖 This review was automatically generated with Coder Agents.
Rename the scope size guard so it reads like the other write* helpers, drop the field name from its error strings, store the allowlist through one helper on every write path, dedupe with slice.Unique, use range-over-int loops, and fix the stale comment on noScopeAllowlist. The large-list test now claims only correctness; a benchmark covers the cost.
…cope-allowlist' into coder-plat-475-admin-oauth-app-scope-allowlist
|
CRF-8: fixed in 7b690c5. The noScopeAllowlist comment now says pre-column apps store NULL and every write path since stores a possibly empty string. |
…oauth-app-scope-allowlist # Conflicts: # coderd/oauth2provider/apps.go # coderd/oauth2provider/provider_test.go
2 participants
Admin-created OAuth2 apps had no way to set a scope allowlist. Only apps registered through DCR could. Part of PLAT-475.
Backend only. The UI for this (a scope picker in the app form) is a separate follow-up PR.
What changed
PostOAuth2ProviderAppRequestandPutOAuth2ProviderAppRequestgain an optionalscopefield (space separated, same format as the existing DCR field).CreateAppandUpdateAppnow store it, instead of always leaving it unrestricted.OAuth2ProviderAppnow returnsscope. Responses canonicalize it and drop duplicates, so a stored allowlist has one display form no matter which spelling the caller used, and an admin-set allowlist reads back the same as a DCR-registered one.scopeonPUTreplaces the app's current allowlist rather than merging into it. Omittingscopeleaves the existing allowlist untouched, and sending an empty string clears it back to unrestricted. OnPOST, an empty or omittedscopemeans unrestricted.Tests
POST,PUT, andGET: setting it, replacing it, omitting it on update, and clearing it with an empty string./oauth2/authorize, the same as an app seeded directly in the database.make genoutput included for the SDK and docs changes.