mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
Bobby HoandClaude Opus 5
16c58770f8
Extracted from #27873 so the schema change can be reviewed for migration safety on its own. #27873 will rebase onto this. `client_type` decides whether the token endpoint validates a client secret at all, and the column accepts any text: nullable, no `CHECK`, no enum. No Go path can write a bad value today, and `IsPublic` fails 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. `000565` adds `CHECK (client_type IN ('confidential', 'public'))` and `NOT NULL`. The `UPDATE` ahead of it should touch zero rows, since migration `000344` added the column with a default of `'confidential'` and backfilled with `COALESCE`; it is there so `SET NOT NULL` cannot fail on an unexpected row. Both `ALTER`s take `ACCESS EXCLUSIVE` and 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_method` is 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_type` is Coder's derived copy, and it is what the token endpoint enforces on. RFC 7591 defines no `client_type` metadata field; the column exists only as a denormalization. Registration used to persist the declaration verbatim while hardcoding `client_type` to `'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. `000566` aligns 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 NULL` changes the generated field from `sql.NullString` to `string`, 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 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
32 lines
1.7 KiB
SQL
32 lines
1.7 KiB
SQL
-- token_endpoint_auth_method is the client's declared auth method (RFC 7591
|
|
-- section 2); client_type is Coder's own field, and it is what the token
|
|
-- endpoint actually enforces. The two can disagree: registration used to
|
|
-- persist a client's "none" declaration while hardcoding client_type to
|
|
-- confidential, so some clients declare themselves public while still
|
|
-- holding, and needing, a real secret.
|
|
--
|
|
-- Align the declaration to what is enforced, not the reverse: deriving
|
|
-- enforcement from the declaration would silently stop requiring the secret
|
|
-- those clients already hold.
|
|
--
|
|
-- Each branch repairs anything outside the valid set for its client_type,
|
|
-- not just the one bad value seen so far, so a stray '' or unrecognized
|
|
-- method can't slip through and later pass a client_type/auth_method
|
|
-- consistency check unnoticed.
|
|
--
|
|
-- The IS NULL arm matters: NULL NOT IN (...) evaluates to NULL, and WHERE
|
|
-- only admits true, so without it NULL rows would stop being repaired.
|
|
UPDATE oauth2_provider_apps
|
|
SET token_endpoint_auth_method = 'client_secret_basic' -- the RFC 7591 section 2 default for a client with a secret
|
|
WHERE client_type = 'confidential'
|
|
AND (token_endpoint_auth_method IS NULL
|
|
OR token_endpoint_auth_method NOT IN ('client_secret_basic', 'client_secret_post'));
|
|
|
|
-- The mirror case is not reachable through any current code path, since a public
|
|
-- client is only ever created by requesting 'none'. Included so the invariant
|
|
-- holds for the whole table rather than for the half that had a known bug.
|
|
UPDATE oauth2_provider_apps
|
|
SET token_endpoint_auth_method = 'none'
|
|
WHERE client_type = 'public'
|
|
AND (token_endpoint_auth_method IS NULL OR token_endpoint_auth_method <> 'none');
|