fix: canonicalize API key scope aliases at ingress - #28322
Conversation
IsExternalScope accepts `all` and `application_connect`, which are not
api_key_scope enum members. The plural Scopes field appended the requested
name verbatim, so POST /users/{user}/keys/tokens with {"scopes":["all"]}
passed validation and then failed inside apikey.Generate, answering HTTP 500
with `invalid API key scope: "all"`. codersdk still exports APIKeyScopeAll and
APIKeyScopeApplicationConnect, so a caller reaches this by passing the
constants the SDK offers for exactly this purpose.
Route every accepted name through rbac.CanonicalScopeName. That fixes the
plural path and deletes the two open-coded alias switches, which restated a
mapping the rbac package already owns and had to be kept in step by hand. The
singular Scope field behaves as before, now by the shared table.
78cfafa to
4a402d3
Compare
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 18 findings (1 P2, 9 P3, 3 P4, 5 Nit), COMMENT. Review Finding inventoryFinding inventory - PR #28322Findings
Round logRound 2Churn guard: PROCEED (8 addressed, 2 deferred with tickets, 0 silent). Head 4a402d3 -> cdc233e via d9753b5 plus a merge of main. Reviewed against c029869..cdc233e. New diff +213 -33, 5 files (31 production, 182 test). Panel re-review pending. Round 1Netero first pass (P3 + Note, below gate) then 14-reviewer panel (Kurapika, Razor, ging-go, Ryosuke, Chopper, Komugi, Gon, Leorio, Bisky, Hisoka, Mafu-san, Mafuuu, Pariston, Meruem). Reviewed against eab5e28..4a402d3. 1 P2, 5 P3, 1 P4, 3 Nit posted. Komugi: no findings. Event COMMENT (no P0/P1). The fix is real and well-tested for the two ingress sites (multiple reviewers verified by reverting production files and rerunning TestTokenLegacyPluralScopeCompat). 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.
This is a clean, proportional fix: 13 production lines delete two hand-copied alias switches and route every accepted name through the one alias table in coderd/rbac, and the new TestTokenLegacyPluralScopeCompat is a genuine regression test. Several reviewers independently reverted the two production files to base and watched the test fail with the exact 500 from the PR description, then pass at HEAD. The fix does what the title says for the two ingress sites it touches, and it removes duplication rather than adding it.
One P2, five P3, one P4, three Nit. No P0/P1, so this is a COMMENT.
The headline, converged on by nearly the whole panel: apikey.Generate teaches the alias lesson to the deprecated singular Scope field but leaves the plural Scopes field verbatim, so the exact bug this PR fixes still lives one layer down, on the field callers are being moved toward. No in-tree caller hits it today (ingress canonicalizes first), which is why it is P2 and not P1, but Generate is the exported choke point every key creation passes through, and the OAuth2 token paths will fill Scopes when their TODO: ignoring scopes resolves. As Hisoka put it: "One asymmetry, and it is the same asymmetry you came to fix. Shall we finish it?" Pick one layer to own canonicalization; the per-element handler calls then become belt-and-braces or disappear.
The other P3s cluster around the same seam: a duplicate-scope persistence the canonicalization newly makes reachable, a missing test binding the rbac catalog to the api_key_scope enum (the class this bug belongs to), missing negative-path coverage on the plural rejection branch, and a structural option (a single ParseExternalScope that validates and returns the canonical name, unexporting IsExternalScope) that would make the wrong call-order unwriteable. The package already documents that call-order hazard four times; four warnings is the shape of a missing function.
Two process notes, not blocking. First, the PR description says "#28178 already routes OAuth2 scope validation through CanonicalScopeName," but on this base coderd/oauth2provider still ignores scopes entirely (TODOs at tokens.go:377,520, authorize.go:237); nothing there is wired to CanonicalScopeName yet. Two reviewers could not verify #28178's own state externally, so this is a report of what main contains. Second, the description reproduces the bug with a raw JSON body and only names the SDK path; cli/tokens.go:104 funnels coder tokens create --scope into the plural field, so coder tokens create --scope all was a first-party CLI command hitting the 500 on shipped releases. Worth one sentence in the body so reviewers size this as a documented user flow, not an SDK edge case.
docs/admin/users/sessions-tokens.md:148
P3 [CRF-6] The doc that teaches operators to pass application_connect is the reproducer for this bug, and it stays inconsistent after the fix. (Leorio)
coder tokens create --scope populates the plural field (cli/tokens.go:104), so coder tokens create --scope application_connect, copied from this page, went down the un-canonicalized plural path and 500'd. After this PR the command succeeds, then coder tokens list prints coder:application_connect because the table renders the plural field while convertAPIKey only rewrites the deprecated singular one. The operator types one name and sees another. Same one paragraph up (:125, the all default lists as coder:all). codersdk marks both aliases Deprecated:. Use the canonical spellings in the prose and examples, and add a line noting that all/application_connect are accepted for backward compatibility and stored/displayed as coder:all/coder:application_connect.
🤖
coderd/database/modelmethods.go:262
P4 [CRF-7] APIKeyScope.ToRBAC is a fourth open-coded copy of the same two-name mapping, and both arms are now the identity. (Ryosuke P4, Meruem P4)
ApiKeyScopeCoderAll and rbac.ScopeAll are both "coder:all"; likewise application_connect. The switch returns exactly what its default returns, so the whole function is return rbac.ScopeName(s) with two decorative cases, the same shape as the switches this PR deletes, left behind when a migration renamed the enum values. Outside this diff's files, so not this PR's obligation; flagging as the same family the PR is cleaning up.
🤖
🤖 This review was automatically generated with Coder Agents.
Generate taught the deprecated singular Scope field to accept alias
spellings and left the plural Scopes field passing its input to the enum
check unchanged, so the alias bug this branch fixes at the handler still
existed one layer down, on the field callers are being moved toward. No
caller hits it today, but Generate is the choke point every key creation
passes through, and the OAuth2 token paths will fill Scopes once their
scope TODOs resolve.
Canonicalize in the loop that already checks each name, so all three
cases are covered by one statement, and build a new slice so the
caller's is left alone. Deduplicate afterwards: an alias and its
canonical spelling are two names on the way in and one name here, and
without this "coder tokens create --scope all --scope coder:all" stored
coder:all twice and listed it twice.
Cover the rejection path, which no test watched. Both handler guards
could be deleted with the suite still green, while
{"scopes":["debug_info:read"]} would have persisted an internal-only
scope that IsExternalScope deliberately refuses and the enum check
accepts.
Add TestExternalScopesAreStorable to pin the class rather than the two
instances: any public scope name the api_key_scope enum cannot store
fails inside Generate after the handler has accepted the request. The
rbac package cannot check this itself, since database imports rbac.
Use the canonical spellings in the token docs, which taught the commands
that reproduced the original 500 and now report back a different name
than the operator typed.
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. |
BobbyHo
changed the title
|
Round 1 responses are in Three items are deliberately deferred and are now tracked in Linear:
One correction on CRF-2: duplicate scope persistence was not introduced by this PR. |
There was a problem hiding this comment.
Round 2 is a strong response. Eight of the ten round-1 findings are fixed in d9753b5, and the fixes are the good kind: canonicalization moved into apikey.Generate at the choke point every key creation passes through, dedup added there, negative-path coverage (unknown scope, internal debug_info:read) added so both handler guards can no longer be deleted with the suite green, and a class test added. Reviewers verified the fixes by mutation rather than by reading, and Mafu-san checked all four checkable claims in your replies and every one held, including the CRF-2 correction that duplicate persistence predates this PR. The three deferrals (CRF-5, CRF-6 display half, CRF-7) all carry Linear tickets filed before the round closed, which is a deferral, not a drop.
Seven new findings, none blocking (no P0/P1, so COMMENT): four P3, two P4, two Nit.
Two dominate, both verified by revert or reproduction across many reviewers. First, CRF-11: the fix moved canonicalization into Generate but left the handler's own rbac.CanonicalScopeName calls in place, so the alias table now has two consumers on the same request path and the handler copy is behaviorally dead. Seven reviewers independently replaced the handler calls with the raw name and watched the whole HTTP suite stay green; nothing distinguishes those two lines from their absence. Round 1's own advice was 'then the handler's per-element calls become redundant', so this is the second half of that fix. Keep the IsExternalScope guards (they are the handler's real, covered job), drop the CanonicalScopeName calls, and reword the header comment.
Second, CRF-3 is re-raised: TestExternalScopesAreStorable pins the class by walking ExternalScopeNames(), but the set the handler actually admits is what IsExternalScope accepts, and that set is strictly larger. ExternalScopeNames() filters externalLowLevel through parseLowLevelScope; IsExternalScope does a bare map lookup. Hisoka added 'workspace:reed' to externalLowLevel and Zoro added 'not_a_resource:read'; in both cases the storability test passed and POST /keys/tokens returned the exact 500 this PR exists to remove, with the full suite green. Latent today (nothing is filtered out of the 57 names), but scopes_catalog.go is being edited by three PRs right now, which is when a curated entry picks up a typo. The alias sub-case is genuinely closed (TestScopeAliases requires each alias's canonical target to be listed, and I verified that against scopes_internal_test.go:56); the hole is the low-level filter. Fix is one loop in coderd/rbac, either asserting every externalLowLevel key parses or dropping the filter in ExternalScopeNames.
The rest: an empty scopes/allow_list array is read as 'unset' and mints a full-authority coder:all / wildcard token with no error (pre-existing, but on the block you rewrote; worth a ticket if not fixed here), and Generate's exported doc comment never states the contract that just moved into it, including that it checks enum membership, not public-ness, so it will store debug_info:read for the OAuth2 caller who fills Scopes once that TODO resolves. Two P4s and two Nits are minor.
One process note: comment quality. Gon found 5 of 13 in-scope comments restate the code or the PR description, concentrated in the new test doc comments. Leorio, by contrast, called the commit messages on this branch exactly what chart notes should be: 'A reviewer who reads only the commit log understands the bug, the blast radius, and the reasoning behind each of the four changes.' The gap is that the reasoning lives in the commit log and inline body comments but not in the one place a future caller looks, the Generate godoc. And a false alarm to clear: one reviewer flagged the TestExternalScopesAreStorable comment as overstating TestScopeAliases coverage; that is incorrect, the composition holds, and I dropped it.
coderd/apikey/apikey.go:46
P3 [CRF-13] Generate's doc comment says nothing about the scope contract this PR moved into it, and the dangerous part is what it does not do. (Leorio P2, Meruem Nit, Mafuuu Nit; Kurapika/Ryosuke/Hisoka Note)
The godoc still reads as if Generate only builds a row. As of d9753b5 it accepts alias spellings, rewrites them, collapses duplicates, defaults an empty request to coder:all, and errors on any name the enum cannot store. Critically, it validates with canonical.Valid() (enum membership), not rbac.IsExternalScope (public-ness), so it will mint a key scoped debug_info:read, which the HTTP handler deliberately refuses with a 400. Your own test comment at coderd/apikey_test.go:188 states this, but it lives in a test file. The patient is coderd/oauth2provider/tokens.go:377,520, whose TODO: We are ignoring scopes for now your commit body says will fill Scopes; the person who resolves that TODO sees Generate validates and canonicalizes and ships it, and nothing they read says the enum check is a storage check, not an authorization one. The structural fix (a parse/guard the caller cannot skip) is tracked as CRF-5/PLAT-528; this finding is just the cheap half: state the contract on the exported function, including that Generate checks storability, not requestability, and callers taking external input must run IsExternalScope first.
🤖
🤖 This review was automatically generated with Coder Agents.
The two other server-flag links in this file point at ../../reference/cli/server.md. This one used an absolute coder.com URL with no .md extension, so it always resolved to the published docs rather than the version being read. Relative links follow the branch preview and the offline docs build.
IsExternalScope admits any key in externalLowLevel, but ExternalScopeNames dropped the keys parseLowLevelScope rejects. A curated entry that does not parse was therefore accepted by the token handler and absent from every list-driven check, so it reached the api_key_scope enum and failed there. Adding "workspace:reed" to the catalog reproduces the 500 this branch exists to remove, with the whole suite green. The filter also made an existing assertion unreachable: TestExternalScopeNames requires every entry to parse but iterates the already-filtered list, so it could never see a bad one. Dropping the filter makes the accepted set and the listed set the same, and that assertion plus TestExternalScopesAreStorable then both fail on the poisoned entry. Output is unchanged today: 57 names before and after, and codersdk/apikey_scopes_gen.go regenerates identically.
…ation postToken canonicalized each accepted name and apikey.Generate canonicalized every element again, so the handler copy decided nothing: replacing both calls with the raw name leaves the token suite green. Two sites answering the same question is how a stored name drifts from the validated one, so the handler now only decides which names may be requested and Generate owns the spelling. The 400 also covered two problems with one sentence. A name that is no scope at all is a typo the caller can fix; an internal api_key_scope member is not, and no re-spelling makes it requestable. Both rejection sites now go through one helper that says which case the caller is in and points at the docs. Adds a case for a caller sending both scope and scopes. Nothing sends both today, so an inverted precedence would widen a workspace:read request to coder:all with every other case still green.
The test cloned params.Scopes with the pre-1.21 append idiom; the repo already uses slices.Clone in 59 other places, and it names the operation instead of leaving the reader to recognize it. The comment above the canonicalization loop said Generate leaves params as the caller passed it, which is not true: it also assigns ExpiresAt, LifetimeSeconds and AllowList on its local copy. State the property that actually matters, which is that params.Scopes shares a backing array with the caller, so canonicalizing in place would rewrite the caller's slice.
The page pointed at "the API reference documentation" with no link, and the only complete list there is the codersdk.APIKeyScope enum, a superset that includes debug_info:read and every other internal scope. An operator following that sentence copies one and gets a 400, which TestTokenLegacyPluralScopeCompat now pins as permanent behavior. Link the schema and say which half of it a token may not ask for.
Rename TestTokenLegacyPluralScopeCompat to TestCreateTokenScopes and TestGenerateScopeNames to TestGenerateCanonicalizesScopeAliases, and drop their doc comments. The subtest names now state the behavior under test, and the rationale the comments carried already lives next to the code it describes: postToken documents the field precedence, apikey.Generate the canonicalization, and convertAPIKey the derived legacy field. Rename the table fields in TestCreateTokenScopes so the per-field comments are unnecessary, and keep only the two case comments whose expectations are surprising on their own. Also tighten two comments in the handler: scopeDocsURL describes the constant rather than the page it links, and the default coder:all scope is already explained where apikey.Generate applies it.
Rewrite three comments flagged in review as narrating history instead of behavior. writeUnrequestableScope now says what it distinguishes rather than which mistake each case came from, and the two ExternalScopeNames comments say the same thing in about half the words.
Co-authored-by: McKayla はな <mckayla@hey.com>
Co-authored-by: McKayla はな <mckayla@hey.com>
2 participants
rbac.IsExternalScopeacceptsallandapplication_connect, neither of which is anapi_key_scopeenum member. The pluralScopesfield stored the name as given, so{"scopes":["all"]}passed validation and then 500'd insideapikey.Generatewithinvalid API key scope: "all". Reachable fromcoder tokens create --scope all, from the spellingsdocs/admin/users/sessions-tokens.mdtaught, and from thecodersdkconstants exported for exactly this purpose.apikey.Generate, in the loop that already validates each name. One statement covers every caller and all three input paths: pluralScopes, deprecated singularScope, and the default. Replaces two open-coded switches and the handler's own per-element copy, so one place decides the stored spelling and the handler only decides what may be requested.ExternalScopeNames()lists every nameIsExternalScopeaccepts, instead of dropping catalog entries that fail to parse. A curated entry that cannot be stored used to pass every test and 500 at runtime; it now fails two.api_key_scopemember need different words, since no re-spelling makes the second requestable. Both rejection sites share one helper that names the case and links the docs.not_a_real_scopeanddebug_info:read. The second is a valid enum member the rbac catalog treats as internal, so before this both handler guards could be deleted with the suite still green. A further case pins that pluralScopeswins when a caller sets both fields.TestExternalScopesAreStorablepins the class rather than the two known instances: every public rbac scope name must be storable.coderd/rbaccannot assert this itself, sincedatabaseimportsrbacand not the reverse.codersdk.APIKeyScopeschema, and say which scopes a token cannot request.Egress is unchanged:
convertAPIKeystill derives a legacy singular name on the way out.ExternalScopeNames()returns the same 57 names as before, andcodersdk/apikey_scopes_gen.goregenerates identically.rbac.CanonicalScopeNamemerged with #28167, so this applies tomainand reviews on its own. The OAuth2 provider ignores scopes entirely today (authorize.go:237,tokens.go:377,tokens.go:520), so it is unaffected until those TODOs resolve.Deferred from review: PLAT-528, PLAT-529, PLAT-530, and PLAT-532. PLAT-532 covers
{"scopes":[]}and{"allow_list":[]}defaulting open, which predates this PR.