feat: constrain the OAuth2 client type column - #27931
Conversation
client_type decides whether the token endpoint validates a client secret at all, and the column accepted any text: nullable, no CHECK, no enum. No Go path can write a bad value, and IsPublic fails closed on anything unrecognized, so the read side is safe today. The point is what the schema still permits: a future migration writing 'public' onto a row that holds a secret turns off client authentication for that app with nothing to catch it, no constraint, no log, no audit entry, no test. Two columns also describe the same fact and could contradict each other. token_endpoint_auth_method is the client's own declaration, registered metadata under RFC 7591 §2 where "none" means the client is public and has no secret; client_type is the derived value enforced on. Registration used to persist the declaration verbatim while hardcoding client_type, so rows exist declaring "none" on a client stored confidential that holds a real secret. A client reading its own metadata concludes it is public, drops the secret, and stops being able to exchange codes. The backfill aligns the declaration to what is enforced; deriving enforcement from the declaration instead would reclassify those clients as public and stop requiring the secret they were issued. SET NOT NULL changes the generated field from sql.NullString to string, so the three write sites are updated to match. That is the whole application change; no behavior depends on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An RFC 7591 registration requesting `token_endpoint_auth_method: "none"` now produces a public client: no secret is minted, `client_type` is persisted as `public`, and the token endpoint accepts that client's authorization_code exchange with PKCE alone. Discovery advertises `"none"` as a supported auth method. PKCE was already mandatory for every authorization_code flow, so public clients inherit it unchanged. That makes the code ownership check (`dbCode.AppID != app.ID`) the sole binding between the exchange and the app named by `client_id` for a public client, where it was defense in depth for confidential ones. It is retained and covered with a public client, along with the refresh and revocation paths, which are the first to see a NULL `app_secret_id`. Verifier length is now checked against RFC 7636 §4.1, since for a public client PKCE is the only client authentication and a one-character verifier hashes to a well-formed challenge. Registration writes the app and its secret in one transaction. Two independently committed inserts could leave a permanently committed app that can never authenticate while still holding a registration access token. An RFC 7592 update can no longer move a client between public and confidential, and secret creation is rejected for a public app: the token endpoint would never validate that secret, and deleting it revokes nothing because a public client's tokens carry a NULL `app_secret_id` instead of cascading from the secret. Clients registered with `"none"` before it was honored are stored confidential and still require their secret, so an update that resends their own metadata is accepted rather than rejected, and the auth method reported back is the one the server actually enforces. Depends on #27931 for the client_type constraint and the schema-level alignment of the two columns. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.9.0 | Round 2 | Last posted: Round 2, 17 findings (3 P2, 4 P3, 7 Nit, 3 Note), COMMENT. Review Finding inventoryFinding inventory: PR 27931Findings
Contested and acknowledgedCRF-2 (P3, 000566_oauth2_auth_method_backfill.up.sql:24) - contradiction recreated by registration until #27873
CRF-3 (Note, coderd/oauth2provider/apps.go:95) - client_type bare string literals
Round logRound 1Netero-only (P2 present, panel gated). 1 P2, 1 P3, 1 Note. Reviewed against 5064006..8b0eb45. Round 2Churn guard: PROCEED. CRF-1 author fixed (c258668, +227 test lines), CRF-2 acknowledged, CRF-3 deferred to #27873. 0 silent. Round 2 panelNetero (1 new Note, CRF-4) then full panel: 18 trigger-matched + 2 wildcards (Zoro, Killua). CRF-1 fix verified by 12 reviewers running the tests; CRF-2 closed by unanimous panel acceptance with residual spun to CRF-8. New: 2 P2, 3 P3, 7 Nit, 1 Note (CRF-5..17). Gon's 10 comment-bloat P2s consolidated and downgraded to one Nit (CRF-10). Reviewed against 5064006..c258668. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
This is a first-pass review only: the findings below are mechanical checks from the automated first-pass reviewer. The full review panel has not yet reviewed this PR and will review after these findings are addressed.
The change itself is well shaped: the migration ordering in 000565 (UPDATE, ADD CONSTRAINT, SET NOT NULL) fails loudly on an unexpected value instead of rewriting it, the PR description's factual claims about migration 000344 check out, all three sql.NullString write sites were updated, and build, vet, migration round-trip, and audit tests all pass. As the first-pass reviewer put it: "the 000565 ordering (UPDATE, ADD CONSTRAINT, SET NOT NULL) fails the migration loudly on an unexpected value rather than rewriting it, which is correct."
Severity count: 1 P2, 1 P3, 1 Note.
🤖 This review was automatically generated with Coder Agents.
…d backfill Addresses CRF-1 on #27931. Neither migration had a dedicated test, and the shared fixture cannot reach the case that matters: testdata/fixtures/ 000182_oauth2_provider.up.sql inserts one app with no token_endpoint_auth_method, so the backfill's IS NULL branch matches it while the '= none' branch, the one repairing the actual bug, matches zero rows in CI. Follows the existing Stepper-to-prior-version, seed, migrate, assert pattern of TestMigration000542ChatReasoningEffortBackfill, TestMigration000562OAuth2PublicClientTokensBackfill and TestMigration000563TemplateAgentsAllowedBackfill. The backfill test seeds all six combinations that matter: a confidential client declaring "none" (the known bug), one with no declaration at all, a public client declaring a secret-based method, and the three already consistent shapes. It asserts client_secret_post is not flattened to basic, that client_type itself is never rewritten, and states the resulting invariant directly by counting rows where the declaration contradicts the enforced type. The constraint test asserts a NULL client_type is backfilled to confidential rather than blocking SET NOT NULL, that the column becomes NOT NULL, and that the CHECK actually rejects "Public", "PUBLIC", "public ", "bogus", "" and NULL while both canonical values remain insertable. Both are mutation-checked. Removing the backfill's '= none' branch fails the legacy case and the invariant count; removing the CHECK fails the rejection assertions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2: full panel review (Netero first pass, then 20 reviewers). The round-1 findings resolved well: CRF-1's fix was verified by twelve reviewers independently running both migration tests against Postgres, with the mutation checks the author claimed reproduced by one of them; CRF-2's accepted window was evaluated by thirteen reviewers and the defense holds unanimously, since the failure direction is fail-closed, the window strictly shrinks the contradictory-row population versus base, and a forgotten backfill in the future cross-column CHECK migration fails loudly at ADD CONSTRAINT rather than shipping silently. As Knuckle put it: "The interest rate on this loan is low and the balloon payment is enforced by the database itself."
The change itself drew repeated praise: correct alignment direction (declaration follows enforcement, never the reverse), honest irreversible down migration, a constraint test that proves rejection instead of trusting existence, and per-case reason strings that make failures self-diagnosing.
New findings this round: 2 P2, 3 P3, 8 Nit, 2 Note.
One item needs a human decision rather than agent acceptance: the permanent fix that the CRF-2 closure leans on (cross-column CHECK, SET NOT NULL on token_endpoint_auth_method, and the backfill for window-created rows) is tracked nowhere except a PR comment (CRF-8). Either confirm ENG-3029 itemizes that third migration or file a ticket.
Process observation (Leorio): the subject of commit c258668 is 79 characters and truncates in log and GitHub views; "test(coderd/database/migrations): cover client_type constraint and backfill" fits within 72. The commit bodies, by contrast, are exemplary: alternatives considered, mutation checks documented.
🤖 This review was automatically generated with Coder Agents.
| seed(ids["publicMismatched"], "test-565-public-mismatch", "public", strPtr("client_secret_basic")) | ||
| seed(ids["confidentialBasic"], "test-565-basic", "confidential", strPtr("client_secret_basic")) | ||
| seed(ids["confidentialPost"], "test-565-post", "confidential", strPtr("client_secret_post")) | ||
| seed(ids["publicNone"], "test-565-public", "public", strPtr("none")) |
There was a problem hiding this comment.
P3 [CRF-5] The fixture seeds no public + NULL row, so the second UPDATE's IS NULL branch matches zero rows in the test, and the closing invariant query is NULL-blind, so neither assertion can ever catch that branch being lost. (Bisky P3, Meruem P3, Hisoka Note, Knuckle Note)
Bisky verified by mutation:
I removed
token_endpoint_auth_method IS NULL ORfrom the second UPDATE in000566_oauth2_auth_method_backfill.up.sqlandTestMigration000566OAuth2AuthMethodBackfillstill passed (worktree restored afterward).
Meruem verified the NULL semantics against Postgres 13: for the invariant query at line 2953, (NULL = 'none') is NULL, NULL <> bool is NULL, and WHERE drops the row, so VALUES ('confidential', NULL), ('confidential','none'), ('public','none') yields a count of 1, not 2.
The fix is two lines of intent: seed a seventh shape (public + NULL, expecting none), and make the invariant honest about NULL with IS DISTINCT FROM instead of <>. Hisoka and Knuckle both traced the second part's consequence beyond this test: the NULL-blind predicate is the obvious candidate to become the permanent cross-column CHECK after #27873, and a CHECK is even more forgiving than count(*) (NULL means not violated), so copying this form forward copies the blind spot into a migration gate unless SET NOT NULL lands first.
🤖
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.key, func(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
P2 [CRF-7] The six parallel subtests of TestMigration000566OAuth2AuthMethodBackfill query through the parent's 60-second ctx (testutil.Context at line 2762), and the clock runs while the subtests sit in the -parallel queue behind every other top-level test in the package. (Komugi)
Measured, not guessed:
At
-parallel=1(a legal setting; CI race jobs run-parallel=4on shared 16-core runners) on this idle box,go test -jsonshows the subtests registered at 22:49:57.73 and continued at 22:50:29.79: 32 seconds of the 60-second budget consumed purely by queue wait, for queries that then took under 20ms.
The project's mechanical guard for this hazard (paralleltestctx) exits 0 here because the ctx arrives through the setupMigration000565Apps return value instead of a direct call in the subtest, so nothing catches this before CI does. When it fires, the failure is context deadline exceeded on a single-row SELECT, unattributable to any code change in the failing PR, and every future migration test added to the package widens the gap. Every other test in this file creates its ctx inside the function that uses it (e.g. line 338). Either create the ctx inside each subtest, or drop t.Parallel() from the six subtests, which are six sub-20ms single-row reads and gain nothing from parallelism.
🤖
There was a problem hiding this comment.
Fixed in c28798d. The measurement is what made this actionable rather than theoretical, so thank you for taking it.
Each subtest now creates its own context. The parent's is still used for the setup and the trailing assertions, which run promptly in the parent's own body.
Two things worth recording about how this got in. The t.Parallel() calls were not a design choice: golangci-lint's tparallel rule failed the build on the first version of this test, and I added them mechanically without asking whether parallelism was correct for six sub-20ms reads sharing one connection. Second, your note that paralleltestctx exits 0 because the context arrives through the helper's return value is the useful generalisation. The guard checks the shape of the call, not the lifetime of the value, so any test in this package that takes its context from a setup helper has the same hole.
Verified the fix under the condition you measured: the full package passes at -parallel=1, which is the worst case for queue wait.
| @@ -0,0 +1,31 @@ | |||
| -- Two columns describe how an OAuth2 client authenticates, and they can | |||
There was a problem hiding this comment.
Nit [CRF-10] Comment duplication pattern: the declaration-vs-enforcement rationale appears four times in this diff (000566 header, TestMigration000566 doc, seed-map comment, require messages) and the 000565 rationale twice; ten comment sites restate what an adjacent require message, map literal, or the owning migration header already says. (Gon)
The migration headers are the owning location; every other copy is the same "why" repeated across layers. Ten bloat findings on one PR is a comment-quality pattern, not ten independent accidents.
Instances with the suggested minimum at each: 000565...up.sql:1 (derivable "authentication rules are undefined" clause restates the concrete failure two sentences later), 000566...up.sql:1 (opening two-line preview of paragraph two; denormalization fact stated twice), 000566...down.sql:3 (narrates the up migration before explaining why this one is empty), migrate_test.go:2765 ("The shapes that matter." carries nothing the map does not show), 2798 (sentence two duplicates the 000565 header the test executes), 2840 (keep only "Prove the CHECK rejects rather than trusting that it exists."), 2861 (restates the loop and its require message; delete), 2872 (paragraph two duplicates the 000566 header; also "the shared fixture" resolves to setupMigration000565Apps rather than the intended testdata/fixtures run), 2949 and 2960 (each restates the require message two lines away; the message is the canonical copy because it prints on failure).
Downgraded from Gon's P2: the code works and these are project comment-rule violations, which is the Nit category; the substance stands as one pattern to fix in a pass.
🤖
| (id, created_at, updated_at, name, icon, callback_url, client_type) | ||
| VALUES ($1, $2, $2, 'test-565-null-after', '', 'http://localhost/callback', NULL) | ||
| `, uuid.New(), now) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
Nit [CRF-14] The NULL-insert probe asserts require.Error(t, err) with no check of which error, while the sibling assertions three lines up pin the constraint name with require.ErrorContains. (Chopper)
On a fresh per-test database the only plausible error is the not-null violation, so the false-positive window is small; flagged for consistency with the test's own assertion style. One line: require.ErrorContains(t, err, "not-null").
🤖
| @@ -0,0 +1,11 @@ | |||
| -- Deliberately empty. | |||
There was a problem hiding this comment.
Nit [CRF-15] The file opens with "Deliberately empty." and then executes SELECT 1;, so its first line is false. (Leorio)
000506_ai_provider_type_copilot_value.down.sql is comment-only and migrates fine, so the statement is not needed. Drop the SELECT 1;, or if you keep it, say "Deliberately a no-op." The justification itself drew praise from two reviewers (consequence, not convenience: values not recorded, and restoring them would reinstate metadata the server rejects).
🤖
| now := time.Now().UTC().Truncate(time.Microsecond) | ||
|
|
||
| // The shapes that matter. "legacy" is the row the known bug produced: the | ||
| // client asked for "none", which RFC 7591 §2 defines as public, but |
There was a problem hiding this comment.
Nit [CRF-16] "RFC 7591 §2" here, "RFC 7591 section 2" in both migration files of the same PR. (Gon)
Pick one; the SQL files' spelled-out "section 2" is the majority form in this diff.
🤖
| -- | ||
| -- This should touch zero rows: migration 000344 added the column with a default | ||
| -- of 'confidential' and backfilled existing rows with COALESCE. | ||
| UPDATE oauth2_provider_apps SET client_type = 'confidential' WHERE client_type IS NULL; |
There was a problem hiding this comment.
Note [CRF-17] The UPDATE repairs only NULL; an unrecognized non-NULL value (say, a hand-edited 'Public') makes ADD CONSTRAINT fail and, because all migrations share one transaction, blocks the entire upgrade. (Meruem)
This is the right direction.
client_typedecides whether a secret is validated at all, so silently coercing an unexplained value to'confidential'would hide tampering; failing the deploy forces an operator to look.
Worth knowing because the migration comment documents the zero-rows expectation for NULL but not the deploy-blocking consequence of a non-NULL stranger. No change requested.
🤖
| `, id, now, name, clientType, authMethod) | ||
| require.NoError(t, err) | ||
| } | ||
| strPtr := func(s string) *string { return &s } |
There was a problem hiding this comment.
Note [CRF-4] The local strPtr closure duplicates the existing coderd/util/ptr.Ref generic. (Netero)
ptr.Ref[string] does the same thing and is the shared utility; this file just does not import it. One test-local line, no behavior at stake, not worth a standalone change; fold it in if you touch the file for other findings.
🤖
… this tree Addresses part of CRF-6 on #27931. The comment named IsPublic, which is added in #27873 and greps to nothing here, so a reader during the window between the two PRs goes looking for a function that does not exist. Ten panel reviewers flagged it independently. States the property directly instead, which is true at this commit and stays true afterwards. The finding's other four instances describe enforcement that arrives with #27873 and are deliberately left: unlike a dangling symbol they are accurate statements about where the column is headed, and each becomes true when that PR lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… method Addresses CRF-9 on #27931. The two UPDATEs were asymmetric: the public branch excluded the one valid value and repaired everything else, while the confidential branch enumerated the two bad shapes known today, so a confidential row holding '' or an unrecognized method passed through untouched. That row would then be invisible permanently, not just unrepaired. The cross-column constraint this is heading towards, (method = 'none') = (client_type = 'public'), evaluates false = false for it and passes, so nothing looks at it again while RFC 7592 GET keeps handing the client a declaration it cannot use. Both branches now name the values valid for their client type and repair everything else, which makes the migration idempotent against history it cannot inspect rather than against the one bug we know about. Reachability is unverified and stated as such in the comment: ApplyDefaults maps "" to client_secret_basic and Valid() rejects unknown methods, so such a row needs a write path predating those guards. The IS NULL arm is kept deliberately. NULL NOT IN (...) evaluates to NULL and WHERE admits only true, so the widening alone would have stopped repairing NULL rows, which the narrow predicate did handle. Both regressions are mutation-checked: restoring the narrow predicate fails the empty and unrecognized cases, and dropping the IS NULL arm fails the NULL case. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… assertions Addresses CRF-7 and CRF-5 on #27931. CRF-7: the parallel subtests queried through the parent's context. Its deadline starts when it is created, but a parallel subtest does not run until a -parallel slot frees, so most of the budget can be spent queued behind other tests in the package before the first query runs. The reported failure would be "context deadline exceeded" on a sub-20ms single-row read, naming no code and worsening as the package grows. paralleltestctx does not catch it because the context arrives through a helper's return value rather than a direct call in the subtest. Each subtest now creates its own. CRF-5, first half: no public + NULL row was seeded, so the second UPDATE's IS NULL arm matched nothing and removing it left the test green. That shape is now seeded and asserted. CRF-5, second half: the closing invariant used <>, which is NULL-blind. With a NULL declaration the comparison is NULL, WHERE drops the row, and an unrepaired NULL counts as consistent. Now IS DISTINCT FROM. This matters past this test: the predicate is the obvious candidate for the permanent cross-column CHECK after #27873, and a CHECK is more forgiving still, since NULL reads as not-violated. Mutation-checked. Removing the second UPDATE's IS NULL arm now fails both the publicNull case and the invariant count; with the old <> form only the former fires, which is what made the blind spot invisible. Full package passes at -parallel=1, the worst case for queue wait. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nt type migrations Addresses CRF-4 and CRF-10 through CRF-16 on #27931. CRF-11: the CHECK-violation assertion matched the constraint name as a raw string, while this migration generates a typed constant for it. Now uses database.IsCheckViolation with CheckOauth2ProviderAppsClientTypeCheck, the established idiom, which matches the pq error code and constraint name rather than error text, and which make gen keeps correct through a rename. CRF-13: the stepper loop treated exhaustion as success, so a renumbered or squashed 564 would silently apply 000565 and 000566 during setup and fail later on a confusing "constraint already exists". Now t.Fatalf, matching every other stepper loop in the file. CRF-14: the NULL-insert probe asserted only that some error occurred, while its siblings pin the constraint. Now pins the not-null violation. CRF-15: the down migration opened with "Deliberately empty." and then ran SELECT 1. Comment-only down migrations run fine (000506 is the precedent), so the statement is gone and the first line is true. CRF-12: the helper doc claimed it seeds every combination the two migrations care about, but 000565's NULL client_type row is seeded in that test. Narrowed to what it actually covers. CRF-10: the declaration-versus-enforcement rationale was written four times and 000565's twice. Kept the migration headers as the owning location and removed the copies that restated an adjacent require message, map literal, or header. Also folded in CRF-17's deploy-blocking consequence, which was the one thing the 000565 header did not say. CRF-16: "section 2" throughout, the majority form in the diff. CRF-4: the local strPtr closure replaced with the shared ptr.Ref. Re-verified after the assertion changes: dropping the CHECK still fails the constraint test, and the full package passes at -parallel=1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extracted from #27873 so the schema change can be reviewed for migration safety on its own. #27873 will rebase onto this.
client_typedecides whether the token endpoint validates a client secret at all, and the column accepts any text: nullable, noCHECK, no enum. No Go path can write a bad value today, andIsPublicfails closed on anything unrecognized, so the read side is safe. What the schema still permits is the problem: a future migration writing'public'onto a row that holds a secret turns off client authentication for that app with nothing to catch it, no constraint, no log, no audit entry, no test.000565addsCHECK (client_type IN ('confidential', 'public'))andNOT NULL. TheUPDATEahead of it should touch zero rows, since migration000344added the column with a default of'confidential'and backfilled withCOALESCE; it is there soSET NOT NULLcannot fail on an unexpected row. BothALTERs takeACCESS EXCLUSIVEand scan a table holding one row per registered OAuth2 client, so the lock is brief.The second migration, and why it aligns the way it does
Two columns describe the same fact and can currently contradict each other.
token_endpoint_auth_methodis the client's own declaration: registered client metadata under RFC 7591 §2, where"none"is defined to mean the client is public and has no secret.client_typeis Coder's derived copy, and it is what the token endpoint enforces on. RFC 7591 defines noclient_typemetadata field; the column exists only as a denormalization.Registration used to persist the declaration verbatim while hardcoding
client_typeto'confidential', so rows exist declaring"none"on a client stored confidential that was issued, and still requires, a real secret. A client that reads its own metadata and believes it is public will drop that secret and stop being able to exchange codes.000566aligns the declaration to what is enforced, not the reverse. Deriving enforcement from the declaration would reclassify every such client as public and stop requiring the secret it holds, which is a silent authentication downgrade. The down migration is deliberately empty: the previous values are not recorded, and restoring them would only reinstate metadata that tells a client to authenticate in a way the server rejects.Application changes
SET NOT NULLchanges the generated field fromsql.NullStringtostring, so the three write sites are updated to match. That is the entire application diff and no behavior depends on it.Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client