mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
27e3d0fb00f346759aa4aa3f518461bde185cc41
30
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Bobby Ho
|
1aa3553b52 |
fix(coderd): set Cache-Control: no-store on OAuth2 responses (#28143)
No response from the `/oauth2` route tree set `Cache-Control` at all, so
an intermediary cache or customer-operated reverse proxy was free to
apply a heuristic freshness lifetime to a response carrying a live
credential. RFC 6749 §5.1 and OAuth 2.1 §3.2.3 both make an affirmative
`no-store` directive a MUST for the authorization server.
Adds `httpmw.NoStore`, mounted on the `/oauth2` and
`/api/v2/oauth2-provider` trees, setting `Cache-Control: no-store` and
`Pragma: no-cache` on every response from them. OAuth 2.1 drops `Pragma`
because RFC 9111 §5.4 deprecates it as a request-only field, so sending
both is conformant under either reading. Not operator-configurable,
since both specs say MUST.
## Scope
- **Both trees, not just `POST /oauth2/tokens`.** The mount is one line
either way, and the wider scope also covers DCR registration, client
configuration read and update, the authorize 302 whose `Location` query
carries the code, and `POST /oauth2-provider/apps/{app}/secrets`, which
returns a plaintext client secret. A route added later inherits the
headers, which matters for PLAT-449.
- **A middleware, not a hook in `httpapi.Write`.** Three write paths
never call it: `POST /oauth2/revoke` and `DELETE
/oauth2/clients/{client_id}` write a bare status, and
`writeOAuth2RegistrationError` encodes its own JSON.
- **`/.well-known/*` deliberately excluded.** Public discovery metadata,
and RFC 9728 §5 asks for the opposite treatment. Assertions pin the
exclusion so a later hoist onto a higher router fails CI.
- **Session-credential routes left alone.** `/users/login`,
`/users/otp/change-password`, and `/users/{user}/keys/*` have the same
gap, but PLAT-448 is scoped to OAuth2 and reaching into session auth
changes the risk profile.
Every credential-returning route here is a `POST`, and RFC 9111 §3 bars
heuristic caching of `POST` responses, so this is defense-in-depth
against a non-conformant intermediary rather than a live caching bug.
Both specs say MUST regardless of what caches would actually do.
## Note for PLAT-498
`DELETE /oauth2/tokens` now carries `no-store` and is wrapped in
`apiKeyMiddleware`, which is mounted inside the `/oauth2` tree and
therefore runs after this middleware. It is the one route where both can
write `Cache-Control`, and PLAT-498's write must not replace `no-store`
with something weaker such as `private`. `POST /oauth2/tokens` cannot
overlap, since it deliberately has no `apiKeyMiddleware`.
## Two assumptions testing corrected
- `GET /oauth2/does-not-exist` returns **200**, not 404. Chi runs the
subrouter's middleware chain for unmatched paths, so both headers are
present, but the request falls through to the root router's SPA handler.
The test asserts the headers and deliberately not the status.
- The experiment-disabled case is unreachable from a test binary, since
`RequireExperimentWithDevBypass` short-circuits on `buildinfo.IsDev()`.
A unit test covers the consequence against the `RequireExperiment` it
delegates to.
No schema, `codersdk`, or serpent option changes, so `make gen` produces
no diff. Rollback is a revert.
Refs PLAT-448
|
||
|
Bobby HoandClaude Opus 5
|
990d24dc42 |
feat: add oauth2 scope columns and single-use delete queries (#28007)
OAuth2 tokens issued by Coder ignore scope entirely. The authorize endpoint parses the `scope` parameter and then discards it, and both grant paths mint API keys with full API access regardless of what the client requested or what the app's allowlist permits. There is also nowhere to put a negotiated scope: nothing carries one from the authorize step to the token it produces. Schema and query groundwork for that pipeline. No behavior change on its own. - Migration `000569` adds a `scope` column to `oauth2_provider_app_codes` and `oauth2_provider_app_tokens`, so a negotiated scope can travel from a code to the token it is exchanged for, and from a token to its refreshed successor. - Existing rows are backfilled to `coder:all`, then both columns become NOT NULL with a non-empty CHECK. Every OAuth2 key is unrestricted in fact today, so the backfill only writes that down, and a caller that omits the column now fails instead of silently issuing full access. - Adds `DeleteOAuth2ProviderAppCodeByIDReturningRow` and `DeleteAPIKeyByIDReturningRow`, which return `sql.ErrNoRows` when the row is already gone. Postgres serializes concurrent deletes on the row lock, so exactly one caller gets a row back, which is what will let the grant paths enforce single use of a code or refresh token without a read-then-write race. - No callers yet. The existing blind deletes and all of their call sites are untouched, and codes and tokens record `coder:all` until a later phase negotiates a real value. Phase 1 of [PLAT-470](https://linear.app/codercom/issue/PLAT-470), tracked as [PLAT-478](https://linear.app/codercom/issue/PLAT-478/phase-1-schema-and-queries). Scope validation at authorize, applying the negotiated scope in the code grant, and refresh narrowing follow as separate PRs. Verified locally: `make gen` and `make lint` clean, the migrations suite passes both up and down, and dbauthz's `TestMethodTestSuite` passes. <details> <summary>End-to-end scope enforcement flow (green marks what this PR touches)</summary> ```mermaid flowchart TD subgraph authorize["/oauth2/authorize"] AZ1["ShowAuthorizePage (GET)<br/>renders consent page"] AZ2["ProcessAuthorize (POST)<br/>scope parsed, then discarded"] Q1["InsertOAuth2ProviderAppCode<br/>gains a Scope param"] AZ1 --> AZ2 --> Q1 end Q1 --> CODES[("oauth2_provider_app_codes<br/>new column: scope text NOT NULL")] subgraph codegrant["POST /oauth2/token, grant_type=authorization_code"] G1["authorizationCodeGrant"] Q2["GetOAuth2ProviderAppCodeByPrefix<br/>now returns Scope"] Q4["DeleteOAuth2ProviderAppCodeByIDReturningRow<br/>added, no caller yet"] G2["apikey.Generate + UserRBACSubject<br/>hardcoded to full access"] G1 --> Q2 --> G2 G1 -.-> Q4 end CODES --> G1 G2 --> Q3 Q3["InsertOAuth2ProviderAppToken<br/>gains a Scope param"] Q3 --> TOKENS[("oauth2_provider_app_tokens<br/>new column: scope text NOT NULL")] subgraph refresh["POST /oauth2/token, grant_type=refresh_token"] G3["refreshTokenGrant"] Q5["GetOAuth2ProviderAppTokenByPrefix<br/>now returns Scope"] Q6["DeleteAPIKeyByIDReturningRow<br/>added, no caller yet"] G3 --> Q5 G3 -.-> Q6 end TOKENS --> G3 Q5 --> Q3 subgraph enforce["Every authenticated API request"] E1["httpmw ExtractAPIKey"] --> E2["APIKey.ScopeSet()"] --> E3["UserRBACSubject"] --> E4["dbauthz authorize"] end TOKENS --> E1 classDef changed fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,color:#1b3c1e classDef dormant fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,stroke-dasharray:5 3,color:#1b3c1e class Q1,Q2,Q3,Q5,CODES,TOKENS changed class Q4,Q6 dormant ``` Solid green is added or changed here. Dashed green exists but has no caller yet. Everything else is unchanged, including the enforcement engine at the bottom, which already reads a key's scopes correctly and only needs real data fed into it. </details> <details> <summary>Suggested reading order</summary> Most of the diff is generated. `dump.sql`, `models.go`, `querier.go`, `queries.sql.go`, `check_constraint.go`, and the dbmock and dbmetrics packages all come from `make gen`. 1. `migrations/000569_oauth2_scope_columns.{up,down}.sql`: additive column, backfill, NOT NULL, CHECK, and a `COMMENT ON COLUMN` on each. 2. `queries/oauth2.sql` and `queries/apikeys.sql`: `scope` added to both insert column lists, plus the two new returning-row deletes alongside the untouched originals. The `Get...ByPrefix` selects needed no edit, since they are `SELECT *`. 3. `dbauthz/dbauthz.go`: hand-written wrappers for the two new queries, each fetching by ID, authorizing delete against the fetched object, then delegating. The generic `deleteQ` helper does not fit, since it requires the delete to return only `error`. 4. `oauth2provider/authorize.go` and `oauth2provider/tokens.go`: the only production changes, all behavior-neutral. 5. `dbgen/dbgen.go` and `dbauthz/dbauthz_test.go`: seed threading, plus a case per new query. `MethodTestSuite` fails with "Method never called" for anything untested. Neither type needs to become auditable, which `make lint` confirms by not erroring on `enterprise/audit/table.go`. </details> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Bobby Ho
|
209d1ca498 |
fix: reject PKCE code_verifier below RFC 7636 length floor (#28003)
The token endpoint accepted any non-empty `code_verifier`, so a one-character verifier was enough to authenticate. RFC 7636 §4.1 requires 43 to 128 characters from the unreserved set. That fix plus the related gaps review surfaced in the same path: - Enforce the length and charset floor on the verifier before the S256 comparison runs. - Validate the challenge at the authorize endpoint too. It was only checked for non-emptiness, so a malformed challenge was stored and then failed late at token exchange, blaming the wrong parameter. - A malformed verifier now returns `invalid_request` (RFC 6749 §5.2); a well-formed but wrong one still returns `invalid_grant` (RFC 7636 §4.6). Both looked identical before, so a client had no way to tell a syntax error from a hash mismatch and would retry the same bad verifier forever. - Revoke the authorization code when a PKCE check fails. Without that, a leaked code could be replayed with unlimited verifier guesses for its remaining lifetime, and RFC 6749 §10.5 requires codes to be single use. - Fix verifier generation in `scripts/oauth2/*.sh` and the docs example. They deleted reserved base64 characters instead of translating them to the URL-safe alphabet, so most runs produced verifiers under the new floor. Also carries #28041, which merged into this branch: public clients may register bare custom schemes such as `vscode://` again, with `mailto`, `tel`, and `sms` rejected. Split out of #27873 (public OAuth2 client support). PKCE is already mandatory for every client, so this stands on its own. <details> <summary>Manual verification</summary> Ran against a local dev server on this branch, using a session token and a throwaway app from `scripts/oauth2/setup-test-app.sh`. 1. Happy path unchanged: HTTP 200, verifier length 43. 2. `code_verifier=short`, and a 43-character verifier ending in `!`: both HTTP 400 `invalid_request`, so charset is enforced and not just length. 3. `code_challenge=tooshort` at authorize: HTTP 400 `invalid_request`, no code issued. An empty challenge still hits the older "required and cannot be empty" message. 4. Well-formed but wrong verifier: HTTP 400 `invalid_grant`, distinct from the cases above. 5. Retrying that same code with the correct verifier: HTTP 400, code already revoked by the failed check. 6. `generate-pkce.sh` produces a 43-character verifier (20 out of 20 runs); the docs example produces 128. 7. `scripts/oauth2/test-mcp-oauth2.sh` passes end to end. The two bearer-token failures in its output are a pre-existing script bug (`09c50559f3`, July 2025) that reuses a resource-scoped token against the real API, not a regression here. </details> |
||
|
Bobby HoandClaude Opus 5
|
16c58770f8 |
feat: constrain the OAuth2 client type column (#27931)
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> |
||
|
Bobby HoandClaude Sonnet 5
|
d814dfad88 |
feat(coderd): support public OAuth2 client tokens at the schema layer (#27712)
Layer 1 of a multi-PR split of #27195 (public/secretless PKCE-only OAuth2 clients), broken up for easier review: **database schema (this PR)** → oauth2provider handler logic → API/e2e integration tests. ## Goal Coder's OAuth2 provider only works correctly for confidential clients today. Public clients — native apps that can't safely hold a shared secret, such as the CLI's browser-based login flow, IDE plugins (VS Code, JetBrains), desktop apps, and MCP clients — cannot complete a real OAuth2 flow against Coder, even though OAuth 2.1 §2.1 explicitly defines this client type and RFC 8252 §8.5 requires PKCE alone to be sufficient authentication for it. Every MCP client, CLI login flow, and IDE plugin is a public client by construction, and none of them can complete a secretless flow against Coder today: dynamic registration always classifies a client as confidential regardless of what it asks for, the token endpoint unconditionally requires a `client_secret`, and discovery metadata never advertises `"none"` as a supported auth method. Full write-up: [ENG-3029](https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client) ### Overall design (end state across the full PR stack) `[PR2]` marks handler-layer changes landing in the next PR in this stack. The green box is what this PR implements. ```mermaid sequenceDiagram autonumber participant C as Public Client (CLI/MCP/IDE plugin) participant S as coderd (chi router) participant H as oauth2provider handlers participant DB as PostgreSQL Note over C,S: Discovery C->>S: GET /.well-known/oauth-authorization-server S->>H: GetAuthorizationServerMetadata() Note over H: [PR2] add "none" to<br/>the returned auth methods list H-->>C: [PR2] 200 { token_endpoint_auth_methods_supported:<br/>[..., "none"] } Note over C,S: Dynamic Client Registration C->>S: POST /oauth2/register<br/>{redirect_uris, token_endpoint_auth_method: "none"} S->>H: CreateDynamicClientRegistration() Note over H: [PR2] client type now reads<br/>the request -> "public" Note over H: [PR2] skip secret generation<br/>for public clients H->>DB: [PR2] INSERT app row<br/>(client_type = 'public') DB-->>H: app row Note over H: [PR2] skip secret insert entirely H-->>C: [PR2] 201 { client_id }<br/>(no client_secret field) Note over C,S: Authorization Code + PKCE flow C->>S: GET /oauth2/authorize?client_id=...&code_challenge=... C->>S: POST /oauth2/tokens (grant_type=authorization_code)<br/>no client_secret S->>H: extractTokenRequest() Note over H: [PR2] client_secret no longer required<br/>for public clients H->>H: authorizationCodeGrant() Note over H: [PR2] skip secret lookup for public clients Note over H: PKCE verification — already mandatory, unchanged rect rgb(198, 239, 206) Note over H,DB: [THIS PR] oauth2_provider_app_tokens.app_id<br/>column added (NOT NULL, populated at insert<br/>time from app.ID) and app_secret_id loosened<br/>to nullable. Revocation now checks app_id<br/>directly. Confidential-client behavior is<br/>unchanged — no public client can be created yet. H->>DB: [PR2] INSERT refresh token row<br/>(no secret reference, for public clients) end DB-->>H: token row H-->>C: 200 { access_token, refresh_token } ``` ## This PR: database schema A public client has no `client_secret`, so it has nothing to put in `oauth2_provider_app_tokens.app_secret_id`, which was `NOT NULL`. This PR makes that column nullable and instead attributes a token to its owning app through a new, always-populated `app_id` column — so ownership checks (e.g. revocation) work identically for public and confidential clients without joining through a secret that may not exist. | Column | Before | After (this PR) | |---|---|---| | `app_secret_id` | `uuid NOT NULL` | **nullable** | | `app_id` | — | **new**: `uuid NOT NULL`, `FOREIGN KEY → oauth2_provider_apps(id) ON DELETE CASCADE`, backfilled for every existing row and populated on every new insert from that point on | This is a single, complete migration — not staged across multiple PRs. An earlier version of this branch deferred `app_secret_id`'s nullability and the insert-time population of `app_id` to a later PR, keeping this PR's diff limited to `coderd/database`. [Automated review](https://github.com/coder/coder/pull/27712#discussion_r3686851911) correctly flagged that as unsafe: the migration would backfill existing rows once, but nothing would populate `app_id` for rows written afterward, so the moment this PR merged, new tokens would start accumulating a permanently `NULL` app_id — and if a release happened to be cut before the follow-up PR landed, that gap could ship to customers and would need a second, later backfill to close. Doing the full migration now avoids that: `app_id` is correct from the first row written, and the promised `NOT NULL` constraint requires no data repair because it's already enforced. Closing that gap requires a few mechanical, non-branching touches outside `coderd/database`: - `revoke.go`'s two ownership checks now compare `dbToken.AppID` directly instead of looking up the app through `app_secret_id` — a genuine simplification (and slightly less code), not a temporary shim. - `tokens.go`'s two `InsertOAuth2ProviderAppToken` call sites supply the new `app_id` column and wrap `app_secret_id` as a `NullUUID`. - `oauth2_test.go`'s one direct-insert test fixture does the same. None of these introduce client-type branching or new capability — every client today is still confidential-only, still always presents a secret, and behavior is unchanged. The full repo builds, vets, and all existing tests pass unmodified in behavior. ## Coming next - **PR2 (handler layer)**: `codersdk`'s `DetermineClientType()` reading the requested `token_endpoint_auth_method`; `registration.go` skipping secret generation for public clients (and wrapping the app+secret insert in a single transaction, fixing a pre-existing orphan-row/visibility-race gap); `tokens.go` making the secret check conditional so PKCE alone authenticates a public client; `metadata.go` advertising `"none"` in discovery. No further migration is needed — the schema this PR ships is already final. - **PR3 (API/e2e layer)**: integration tests through the real HTTP API (`coderd/oauth2_test.go`), the MCP OAuth2 e2e flow (`coderd/mcp/mcp_e2e_test.go`), and the manual test script (`scripts/oauth2/test-mcp-oauth2.sh`). Depends on: #27195 (original combined PR, being superseded by this stack) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Bobby Ho
|
fbac602456 |
feat!: add admin-controlled dynamic client registration toggle (#27316)
`POST /oauth2/register` (RFC 7591 Dynamic Client Registration) has exactly one gate today: `ExperimentOAuth2`, a static, process-lifetime flag that wraps the entire `/oauth2/*` route tree as an all-or-nothing switch. That flag is scheduled for removal at GA, which would leave DCR with zero admin control at all once it is gone. Add a persistent, DCR-specific `oauth2_dcr_enabled` deployment setting, independent of the experiment system, so admin control over DCR survives GA. `POST /oauth2/register` checks the flag and rejects new registrations with an RFC 7591-shaped `403` when disabled; discovery metadata (`GET /.well-known/oauth-authorization-server`) conditionally omits `registration_endpoint`. A new audited `GET`/`PUT /api/v2/oauth2-provider/settings` endpoint lets an owner toggle it live, no restart required. The setting defaults to disabled, matching the canonical design proposal; disabling only stops new self-registrations, clients that already registered continue to authorize and exchange tokens normally. Address issue described in [ENG-3056](https://linear.app/codercom/issue/ENG-3056/oauth2-dcr-admin-configurable-enabledisable). ## Where this sits in the request path ```mermaid sequenceDiagram autonumber participant A as Admin participant S as coderd participant DB as site_configs<br/>(oauth2_dcr_enabled) participant C as OAuth2/MCP Client Note over A,S: Admin toggles DCR (new) A->>S: PUT /api/v2/oauth2-provider/settings<br/>{dynamic_client_registration_enabled: false} S->>S: authorizeContext(ActionUpdate, ResourceDeploymentConfig) S->>DB: UPSERT oauth2_dcr_enabled = false S-->>A: 200 OK (audited) Note over C,S: Client discovery + registration afterward C->>S: GET /.well-known/oauth-authorization-server S->>DB: GetOAuth2DCREnabled (system ctx, every request, no cache) DB-->>S: false S-->>C: 200 metadata, registration_endpoint omitted C->>S: POST /oauth2/register S->>DB: GetOAuth2DCREnabled (system ctx, every request, no cache) DB-->>S: false S-->>C: 403 invalid_request,<br/>"Dynamic client registration is disabled" Note over C,S: A client that registered before the change is unaffected C->>S: GET /oauth2/authorize?client_id=... Note over S: no DCR-enabled check on this path S-->>C: 200 (proceeds normally) C->>S: PUT/DELETE /oauth2/clients/{client_id} (RFC 7592 self-management) Note over S: no DCR-enabled check on this path either S-->>C: 200 (proceeds normally) ``` ## Files changed: manual vs. generated Reviewers should focus on the **manual** files. The **generated** ones are `make gen` output that follows mechanically from the manual changes and don't need direct review. <details> <summary><b>Manual files (26)</b> — click to expand, grouped the same way as "Suggested review order" below</summary> **1. Database** | File | What changed | |---|---| | `coderd/database/queries/siteconfig.sql` | New `GetOAuth2DCREnabled`/`UpsertOAuth2DCREnabled` query pair on the existing generic `site_configs` table. No schema change. | | `coderd/database/dbauthz/dbauthz.go` | RBAC check (`rbac.ResourceDeploymentConfig`) on the two new query methods; extends the `subjectSystemOAuth2` system-actor role with read-only `ResourceDeploymentConfig` access, needed so the public discovery/registration endpoints can read the flag via `dbauthz.AsSystemOAuth2`. | | `coderd/database/dbauthz/dbauthz_test.go` | RBAC assertion coverage for `GetOAuth2DCREnabled`/`UpsertOAuth2DCREnabled` in the method-coverage test suite. | **2. Request gating (the actual feature)** | File | What changed | |---|---| | `coderd/oauth2provider/registration.go` | The actual gate: `CreateDynamicClientRegistration` reads the flag first and returns an RFC 7591-shaped `403` when disabled (defaults disabled if never configured). | | `coderd/oauth2provider/registration_test.go` | New unit test, `TestCreateDynamicClientRegistration_DCREnabled`: calls the handler directly (no HTTP server), covering enabled / explicitly disabled / never-configured. | | `coderd/oauth2provider/metadata.go` | `GetAuthorizationServerMetadata` conditionally omits `registration_endpoint` from discovery metadata when DCR is disabled. | | `coderd/oauth2provider/metadata_test.go` | New unit test, `TestGetAuthorizationServerMetadata_DCREnabled`: same three states, for the discovery handler. | **3. Admin settings endpoint** | File | What changed | |---|---| | `codersdk/oauth2.go` | New `OAuth2ProviderSettings` SDK type plus `Client.OAuth2ProviderSettings`/`PutOAuth2ProviderSettings` methods. | | `coderd/oauth2.go` | New `oauth2ProviderSettings`/`putOAuth2ProviderSettings` admin handlers (audited via `audit.InitRequest`); updates the `GetAuthorizationServerMetadata` call site to pass `api.Database`. | | `coderd/coderd.go` | Registers `GET`/`PUT /api/v2/oauth2-provider/settings`. | | `coderd/oauth2_provider_settings_test.go` | New test file: admin `GET`/`PUT` round-trip, default-disabled-before-any-`PUT`, and `403` for a non-owner on both `GET` and `PUT`. | **4. Audit wiring** | File | What changed | |---|---| | `coderd/database/types.go` | New `database.OAuth2ProviderSettings` audit-only struct (mirrors `NotificationsSettings`). | | `coderd/audit/diff.go` | Adds the new struct to the `Auditable` type union. | | `coderd/audit/request.go` | Adds the new struct to all four dispatch switches (`ResourceTarget`, `ResourceID`, `ResourceType`, `ResourceRequiresOrgID`). | | `codersdk/audit.go` | New API-facing `ResourceTypeOAuth2ProviderSettings` constant and its `FriendlyString` case. | | `enterprise/audit/table.go` | Field-level audit action map (`ActionTrack`/`ActionIgnore`) for the new struct. | | `coderd/database/migrations/000546_audit_oauth2_provider_settings.up.sql` | Adds `oauth2_provider_settings` to the `resource_type` Postgres enum, required for the audit wiring above (`resource_type` is a real enum, not a Go-only value). | | `coderd/database/migrations/000546_audit_oauth2_provider_settings.down.sql` | No-op (`ALTER TYPE ... ADD VALUE` can't be reverted). | **5. Test-suite ripple from the disabled-by-default flip** | File | What changed | |---|---| | `coderd/oauth2provider/oauth2providertest/helpers.go` | New shared test helper, `EnableDCR`, since DCR now defaults to disabled and many pre-existing tests need it turned on to register a client. | | `coderd/oauth2_test.go` | Adds `TestOAuth2DynamicClientRegistrationDisabled` (registers a client, disables DCR, verifies new registration is rejected while the existing client's self-management, authorize, and token exchange all keep working); calls `EnableDCR` in every pre-existing test that registers a client. | | `coderd/oauth2_error_compliance_test.go` | Calls `EnableDCR` in every test that registers a client, so RFC-error-format assertions aren't masked by the new disabled-by-default gate. | | `coderd/oauth2_metadata_validation_test.go` | Same: `EnableDCR` added to every registration-dependent test. | | `coderd/oauth2_security_test.go` | Same. | | `coderd/oauth2provider/validation_test.go` | Same (near-duplicate of `oauth2_metadata_validation_test.go` in a different package). | | `coderd/oauth2provider/provider_test.go` | Same. | | `coderd/mcp/mcp_e2e_test.go` | Same, for the MCP end-to-end dynamic-registration flow test. | </details> <details> <summary><b>Generated files (12)</b> — from <code>make gen</code>, no need to review directly</summary> `coderd/apidoc/docs.go`, `coderd/apidoc/swagger.json`, `coderd/database/dbmetrics/querymetrics.go`, `coderd/database/dbmock/dbmock.go`, `coderd/database/dump.sql`, `coderd/database/models.go`, `coderd/database/querier.go`, `coderd/database/queries.sql.go`, `docs/admin/security/audit-logs.md`, `docs/reference/api/enterprise.md`, `docs/reference/api/schemas.md`, `site/src/api/typesGenerated.ts`. </details> ## Suggested review order ### 1. Database Establishes the persisted setting and its RBAC rule; everything else builds on `GetOAuth2DCREnabled`/`UpsertOAuth2DCREnabled`. 1. `coderd/database/queries/siteconfig.sql` — the two new queries. Same boolean-encoding pattern as the existing `oauth2_github_default_eligible` key right above them in the same file. 2. `coderd/database/dbauthz/dbauthz.go` — the RBAC wrapper for those two queries, plus the `subjectSystemOAuth2` role extension (search this file for `ResourceDeploymentConfig`, it appears in both spots). 3. `coderd/database/dbauthz/dbauthz_test.go` — asserts the RBAC checks from (2) actually fire. ### 2. Request gating (the actual feature) Where `POST /oauth2/register` and discovery metadata change behavior. 1. `coderd/oauth2provider/registration.go` — the primary gate. Read this first; it's the feature. 2. `coderd/oauth2provider/registration_test.go` — its new unit test, exercising the gate's three states directly against the handler. 3. `coderd/oauth2provider/metadata.go` — the same gating pattern applied to the discovery `GET` endpoint. 4. `coderd/oauth2provider/metadata_test.go` — its new unit test. ### 3. Admin settings endpoint How an owner flips the setting live. 1. `codersdk/oauth2.go` — the `OAuth2ProviderSettings` SDK type and `Client` methods first; this is the public contract everything below implements against. 2. `coderd/oauth2.go` — the `GET`/`PUT` handlers themselves. 3. `coderd/coderd.go` — route registration, to see where those handlers get wired in. 4. `coderd/oauth2_provider_settings_test.go` — round-trip and permission tests. ### 4. Audit wiring Plumbing required so step 3's `PUT` is auditable; mechanical except for (3). 1. `coderd/database/types.go` — the audit-only struct; everything else in this layer exists to plumb it through. 2. `coderd/audit/diff.go` — adds it to the `Auditable` type union (the compiler enforces this one). 3. `coderd/audit/request.go` — the four dispatch switches; the one part of this layer worth reading closely. 4. `codersdk/audit.go` — the API-facing resource type constant. 5. `enterprise/audit/table.go` — the field-action map. 6. `coderd/database/migrations/000546_audit_oauth2_provider_settings.{up,down}.sql` — read last; a consequence of needing a new `resource_type` enum value for (1)-(5), not a design decision of its own. ### 5. Test-suite ripple from the disabled-by-default flip 1. `coderd/oauth2provider/oauth2providertest/helpers.go` — the new `EnableDCR` helper. Read first to understand the fix pattern before seeing it applied repeatedly. 2. `coderd/oauth2_test.go` — next, since it also contains the new `TestOAuth2DynamicClientRegistrationDisabled`, not just `EnableDCR` call sites. 3. The rest, in any order, they're mechanical repeats of the same one-line addition: `coderd/oauth2_error_compliance_test.go`, `coderd/oauth2_metadata_validation_test.go`, `coderd/oauth2_security_test.go`, `coderd/oauth2provider/validation_test.go`, `coderd/oauth2provider/provider_test.go`, `coderd/mcp/mcp_e2e_test.go`. ## Explicitly out of scope Per the design proposal: rate limiting on `POST /oauth2/register` (tracked separately), retroactively affecting already-registered clients when DCR is disabled (this only gates new self-registration), and an Initial Access Token requirement (a separate, follow-up ticket). |
||
|
Ethan
|
c650aabbef |
chore: standardize on *_internal_test.go for white-box tests (#25601)
My agent added `//nolint:testpackage` to a test file on one of my PRs. Again. This PR cleans it up across the entire repo and updates the in-repo conventions so future agents stop doing it. The repo already has a precedent for white-box tests that need to touch unexported symbols: `*_internal_test.go` (145+ existing files). The `testpackage` linter's default `skip-regexp` exempts that filename suffix, so the `//nolint:testpackage` directive is unnecessary in every case where someone reached for it. This PR renames 51 such files to `*_internal_test.go` via `git mv` so blame and history follow, and strips the dead directive from 2 files that were already correctly named (`coderd/oauth2provider/authorize_internal_test.go`, `coderd/x/chatd/advisor_internal_test.go`). `.claude/docs/TESTING.md` now documents the rule explicitly under *Test Package Naming*, which is imported into the root `AGENTS.md` via `@.claude/docs/TESTING.md`. The rule: prefer `package foo_test`; if you need internal access, rename the file to `*_internal_test.go` rather than adding a nolint directive. |
||
|
Faur Ioan-Aurel
|
a8e7f329ac |
fix: redirect OAuth2 authorization page to dashboard (#24499)
Currently when a user clicks either the Cancel or Allow button on the authorization page the client app URI is executed but the page does not land to the main dashboard page, leaving the two buttons open for multiple clicks from the user. Aside from the potential problems it might cause by activating the callback URI multiple times, the page also provides poor UX because users usually expect the authorization tab to return to the dashboard. The consent page now executes the OAuth2 callback (auth code on Allow, `access_denied` on Cancel) and hides the two buttons and updates the existing description with a user instruction to close the window. Initial implementation relied on a pop-up window executing the callback while the main window was redirected to the dashboard main page. - resolves https://github.com/coder/coder/issues/20323 <!-- If you have used AI to produce some or all of this PR, please ensure you have read our [AI Contribution guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING) before submitting. --> |
||
|
Faur Ioan-Aurel
|
83fd4cf5c2 |
fix: OAuth2 cancel button in the authorization page not working (#24058)
Go's html/template has a built-in security filter (urlFilter) that only allows http, https, and mailto URL schemes. Any other scheme gets replaced with #ZgotmplZ. The OAuth2 app's callback URL uses custom URI scheme which the filter considers unsafe. For example the Coder JetBrains plugin exposes a callback URI with the scheme jetbrains:// - which was effectively changed by the template engine into #ZgotmplZ. Of course this is not an actual callback. When users clicked the cancel button nothing happened. The fix was simple - we now wrap the apps registered callback URI into htmltemplate.URL. Usually this needs some validation otherwise the linter will complain about it. The callback URI used by the Cancel logic is actually validated by our backend when the client app programmatically registered via the dynamic OAuth2 registration endpoints, so we refactored the validation around that code and re-used some of it in the Cancel handling to make sure we don't allow URIs like `javascript` and `data`, even though in theory these URIs were already validated. In addition, while testing this PR with https://github.com/coder/coder-jetbrains-toolbox/pull/209 I discovered that we are also not compliant with https://www.rfc-editor.org/rfc/rfc6749#section-4.1.2.1 which requires the server to attach the local state if it was provided by the client in the original request. Also it is optional but generally a good practice to include `error_description` in the error responses. In fact we follow this pattern for the other types of error responses. So this is not a one off. - resolves #20323 <img width="1485" height="771" alt="Cancel_page_with_invalid_uri" src="https://github.com/user-attachments/assets/5539d234-9ce3-4dda-b421-d023fc9aa99e" /> <img width="486" height="746" alt="Coder Toolbox handling the Cancel button" src="https://github.com/user-attachments/assets/acab71a6-d29c-4fa9-80ba-3c0095bbdc8f" /> <!-- If you have used AI to produce some or all of this PR, please ensure you have read our [AI Contribution guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING) before submitting. --> |
||
|
Cian Johnston
|
3f55b35f68 |
refactor: replace AsSystemRestricted with narrower actors (#23712)
Replace overly-broad `AsSystemRestricted` with purpose-built actors:
- **OAuth2 provider paths** → `AsSystemOAuth2` (13 call sites across
`tokens.go`, `registration.go`, `apikey.go`)
- **Provisioner daemon health read** → `AsSystemReadProvisionerDaemons`
(1 site in `healthcheck/provisioner.go`)
- **Provisionerd file cache paths** → `AsProvisionerd` (2 sites in
`provisionerdserver.go`, matching existing usage nearby)
<details>
<summary>Implementation notes</summary>
Each replacement actor is a strict subset of `AsSystemRestricted`. Every
DB method
at each call site is already covered by the narrower actor's
permissions:
- `subjectSystemOAuth2`: OAuth2App/Secret/CodeToken (all), ApiKey (Read,
Delete), User (Read), Organization (Read)
- `subjectSystemReadProvisionerDaemons`: ProvisionerDaemon (Read)
- `subjectProvisionerd`: File (Create, Read) plus provisionerd-scoped
resources
No new permissions added. `nolint:gocritic` comments updated to reflect
the new actors.
</details>
> 🤖 Created by a Coder Agent, reviewed by me.
|
||
|
Cian Johnston
|
9e4c283370 |
test: share coderdtest instances in OAuth2 validation tests (#23455)
Consolidates invocations of `coderdtest.New` to a single shared instance per parent for the following tests: - `TestOAuth2ClientMetadataValidation` - `TestOAuth2ClientNameValidation` - `TestOAuth2ClientScopeValidation` - `TestOAuth2ClientMetadataEdgeCases` > 🤖 This PR was created with the help of Coder Agents, and was reviewed by my human. 🧑💻 |
||
|
Thomas Kosiewski
|
b776a14b46 |
fix(coderd): harden OAuth2 provider security (#22194)
## Summary Harden the OAuth2 provider with multiple security fixes addressing `coder/security#121` (CSRF session takeover) and converge on OAuth 2.1 compliance. ### Security Fixes | Fix | Description | Commits | |-----|-------------|---------| | **CSRF on `/oauth2/authorize`** | Enforce CSRF protection on the authorize endpoint POST (consent form submission) | `ba7d646`, `b94a64e` | | **Clickjacking: `frame-ancestors` CSP** | Prevent consent page from being iframed (`Content-Security-Policy: frame-ancestors 'none'` + `X-Frame-Options: DENY`) | `597aeb2` | | **Exact redirect URI matching** | Changed from prefix matching to full string exact matching per OAuth 2.1 §4.1.2.1 | `73d64b1`, `93897f1` | | **Store & verify `redirect_uri`** | Store redirect_uri with auth code in DB, verify at token exchange matches exactly (RFC 6749 §4.1.3) | `50569b9`, `d7ca315` | | **Mandatory PKCE** | Require `code_challenge` at authorization (for `response_type=code`) + unconditional `code_verifier` verification at token exchange | `d7ca315`, `1cda1a9` | | **Reject implicit grant** | `response_type=token` now returns `unsupported_response_type` error page (OAuth 2.1 removes implicit flow) | `d7ca315`, `91b8863` | ### Changes by File **`coderd/httpmw/csrf.go`** — Extended the CSRF `ExemptFunc` to enforce CSRF on `/oauth2/authorize` in addition to `/api` routes. The consent form POST is now CSRF-protected to prevent cross-site authorization code theft. **`site/site.go`** — Added `Content-Security-Policy: frame-ancestors 'none'` and `X-Frame-Options: DENY` headers to `RenderOAuthAllowPage` (consent page only — does not affect the SPA/global CSP used by AI tasks). **`coderd/httpapi/queryparams.go`** — Changed `RedirectURL` from prefix matching (`strings.HasPrefix(v.Path, base.Path)`) to full URI exact matching (`v.String() != base.String()`), comparing scheme, host, path, and query. **`coderd/oauth2provider/authorize.go`** — Added PKCE enforcement: `code_challenge` is required when `response_type=code` (via a conditional check, not `RequiredNotEmpty`, so `response_type=token` can reach the explicit rejection path). `ShowAuthorizePage` (GET) validates `response_type` before rendering and returns a 400 error page for unsupported types. `ProcessAuthorize` (POST) stores the `redirect_uri` with the auth code when explicitly provided. **`coderd/oauth2provider/tokens.go`** — PKCE verification is now unconditional (not gated on `code_challenge` being present in DB). If the stored code has a `redirect_uri`, the token endpoint verifies it matches exactly — mismatch returns `errBadCode` → `invalid_grant`. Missing `code_verifier` returns `invalid_grant`. **`codersdk/oauth2.go`** — `OAuth2ProviderResponseTypeToken` constant and `Valid()` acceptance are **kept** so the authorize handler can parse `response_type=token` and return the proper `unsupported_response_type` error rather than failing at parameter validation. **`coderd/database/migrations/000421_*`** — Added `redirect_uri text` column to `oauth2_provider_app_codes`. ### Design Decisions **`state` parameter remains optional** — The plan initially required `state` via `RequiredNotEmpty`, but this was reverted in `376a753` to avoid breaking existing clients. The `state` is still hashed and stored when provided (via `state_hash` column), securing clients that opt in. **`response_type=token` kept in `Valid()`** — Removing it from `Valid()` would cause the parameter parser to reject the request before the authorize handler can return the proper `unsupported_response_type` error. The constant is kept for correct error handling flow. **CSP scoped to consent page only** — `frame-ancestors 'none'` is set only on the OAuth consent page renderer, not globally. The SPA/global CSP was previously changed to allow framing for AI tasks ([#18102](https://github.com/coder/coder/pull/18102)); this change does not regress that. ### Out of Scope (follow-up PRs) - Bearer tokens in query strings (needs internal caller audit) - Scope enforcement on OAuth2 tokens - Rate limiting on dynamic client registration --- <details> <summary>📋 Implementation Plan</summary> # Plan: Harden OAuth2 Provider — Security Fixes + OAuth 2.1 Compliance ## Context & Why Security issue `coder/security#121` reports a critical session takeover via CSRF on the OAuth2 provider. This plan covers all remaining security fixes from that issue **plus** convergence on OAuth 2.1 requirements. The goal is a single PR that closes all actionable gaps. ## Current State (already committed on branch `csrf-sjx1`) | Fix | Status | Commits | |-----|--------|---------| | Fix 1: CSRF on `/oauth2/authorize` | ✅ Done | `ba7d646`, `b94a64e` | | CSRF token in consent form HTML | ✅ Done | `b94a64e` | | `state_hash` column + storage | ✅ Done (hash stored, but state still optional) | `9167d83`, `b94a64e` | | Tests for CSRF + state hash | ✅ Done | `e4119b5` | ## Remaining Work ### ~~Fix 2 — Require `state` parameter~~ (DROPPED) > **Decision:** Do not enforce `state` as required. The `state` parameter is still hashed and stored when provided (via `hashOAuth2State` / `state_hash` column from prior commits), but clients are not forced to supply it. This avoids breaking existing integrations that omit state. **Rollback:** Remove `"state"` from the `RequiredNotEmpty` call in `coderd/oauth2provider/authorize.go:42`: ```go // BEFORE (current on branch) p.RequiredNotEmpty("response_type", "client_id", "state", "code_challenge") // AFTER p.RequiredNotEmpty("response_type", "client_id", "code_challenge") ``` No test changes needed — tests already pass `state` voluntarily. ### Fix 4 — Exact redirect URI matching Currently `coderd/httpapi/queryparams.go:233` uses prefix matching: ```go // CURRENT — prefix match if v.Host != base.Host || !strings.HasPrefix(v.Path, base.Path) { ``` OAuth 2.1 requires **exact string matching**. Change to: ```go // AFTER — exact match (OAuth 2.1 §4.1.2.1) if v.Host != base.Host || v.Path != base.Path { ``` **File: `coderd/httpapi/queryparams.go` — `RedirectURL` method** Also update the error message from "must be a subset of" to "must exactly match". **Additionally**, store `redirect_uri` with the auth code and verify at the token endpoint (RFC 6749 §4.1.3): 1. **New migration** (same migration file or a new `000421`): Add `redirect_uri text` column to `oauth2_provider_app_codes` 2. **Update INSERT query** in `coderd/database/queries/oauth2.sql` to include `redirect_uri` 3. **`coderd/oauth2provider/authorize.go`**: Store `params.redirectURL.String()` when inserting the code 4. **`coderd/oauth2provider/tokens.go`**: After retrieving the code from DB, verify that `redirect_uri` from the token request matches the stored value exactly. Currently `tokens.go:103` calls `p.RedirectURL(vals, callbackURL, "redirect_uri")` for prefix validation only — it must compare against the stored redirect_uri from the code, not just the app's callback URL. <details> <summary>Why both exact match AND store+verify?</summary> Exact matching at the authorize endpoint prevents open redirectors (attacker can't use a sub-path). Storing and verifying at the token endpoint prevents code injection — an attacker who steals a code can't exchange it with a different redirect_uri than was originally authorized. This is required by RFC 6749 §4.1.3 and OAuth 2.1. </details> ### Fix 7 — `frame-ancestors` CSP on consent page The consent page can be iframed by a workspace app (same-site), which is the attack vector. Add a `Content-Security-Policy` header to prevent framing. **File: `site/site.go` — `RenderOAuthAllowPage` function (~line 731)** Before writing the response, add: ```go func RenderOAuthAllowPage(rw http.ResponseWriter, r *http.Request, data RenderOAuthAllowData) { rw.Header().Set("Content-Type", "text/html; charset=utf-8") // Prevent the consent page from being framed to mitigate // clickjacking attacks (coder/security#121). rw.Header().Set("Content-Security-Policy", "frame-ancestors 'none'") rw.Header().Set("X-Frame-Options", "DENY") ... ``` Both headers for defense-in-depth (CSP for modern browsers, X-Frame-Options for legacy). ### OAuth 2.1 — Mandatory PKCE Currently PKCE is checked only when `code_challenge` was provided during authorization (`tokens.go:258`): ```go // CURRENT — conditional check if dbCode.CodeChallenge.Valid && dbCode.CodeChallenge.String != "" { // verify PKCE } ``` OAuth 2.1 requires PKCE for ALL authorization code flows. Change to: **File: `coderd/oauth2provider/authorize.go`** — Add `"code_challenge"` to required params: ```go p.RequiredNotEmpty("response_type", "client_id", "code_challenge") ``` **File: `coderd/oauth2provider/tokens.go:257-265`** — Make PKCE verification unconditional: ```go // AFTER — PKCE always required (OAuth 2.1) if req.CodeVerifier == "" { return codersdk.OAuth2TokenResponse{}, errInvalidPKCE } if !dbCode.CodeChallenge.Valid || dbCode.CodeChallenge.String == "" { // Code was issued without a challenge — should not happen // with the authorize endpoint enforcement, but defend in // depth. return codersdk.OAuth2TokenResponse{}, errInvalidPKCE } if !VerifyPKCE(dbCode.CodeChallenge.String, req.CodeVerifier) { return codersdk.OAuth2TokenResponse{}, errInvalidPKCE } ``` **File: `codersdk/oauth2.go`** — Remove `OAuth2ProviderResponseTypeToken` from the enum or reject it explicitly in the authorize handler. Currently it's defined at line 216 but the handler ignores `response_type` and always issues a code. We should either: - (a) Remove the `"token"` variant from the enum and reject it with `unsupported_response_type`, OR - (b) Add an explicit check in `ProcessAuthorize` that rejects `response_type=token` Option (b) is simpler and more backwards-compatible: ```go // In ProcessAuthorize, after extracting params: if params.responseType != codersdk.OAuth2ProviderResponseTypeCode { httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeUnsupportedResponseType, "Only response_type=code is supported") return } ``` ### OAuth 2.1 — Bearer tokens in query strings `coderd/httpmw/apikey.go:743` accepts `access_token` from URL query parameters. OAuth 2.1 prohibits this. However, this may be used internally (e.g., workspace apps, DERP). Need to audit callers before removing. **Approach:** This is a larger change with potential breakage. Mark as a **separate follow-up issue** rather than including in this PR. Document the finding. ### OAuth 2.1 — Removed flows ✅ **Already compliant.** `tokens.go` only supports `authorization_code` and `refresh_token` grant types. The implicit grant (`response_type=token`) will be explicitly rejected per the PKCE section above. ### OAuth 2.1 — Refresh token rotation ✅ **Already compliant.** `tokens.go:442` deletes the old API key when a refresh token is used. ## Migration Plan All DB changes can go in a single new migration (or extend 000420 if the branch is rebased before merge). Columns to add: - `redirect_uri text` on `oauth2_provider_app_codes` The `state_hash` column is already added by migration 000420. ## Implementation Order 1. **Fix 7** — CSP headers on consent page (isolated, no deps) 2. ~~**Fix 2** — Require `state` parameter~~ (DROPPED — state stays optional) 3. **Fix 4** — Exact redirect URI matching + store/verify redirect_uri 4. **PKCE mandatory** — Require `code_challenge` + reject `response_type=token` 5. **Rollback** — Remove `"state"` from `RequiredNotEmpty` in `authorize.go` 6. **Tests** — Update/add tests for all changes 7. **`make gen`** after DB changes ## Out of Scope (separate PRs) - Bearer tokens in query strings (needs internal caller audit) - Scope enforcement on OAuth2 tokens - Rate limiting / quota on dynamic client registration </details> --- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-6` • Thinking: `xhigh`_ |
||
|
Danielle Maywood
|
92a6d6c2c0 |
chore: remove unnecessary loop variable captures (#22180)
Since Go 1.22, the loop variable capture issue is resolved. Variables declared by for loops are now per-iteration rather than per-loop, making the 'v := v' pattern unnecessary. |
||
|
Jake Howell
|
a2ab7e6519 |
fix: marshal OAuth2ProviderApp into [] not null (#21992)
This pull-request makes it so that when we `json.Marshal` an empty set of responses from `ListApps(...)` we return an empty array (`[]`) instead of `null`. This ensures that the array is non-nil 🙂 | Old | New | | --- | --- | | <img width="839" height="177" alt="OAUTH2_APPS_BORKED" src="https://github.com/user-attachments/assets/c264be1a-6260-405a-bf07-50a533e48ed5" /> | <img width="839" height="177" alt="OAUTH2_APPS_WORKING" src="https://github.com/user-attachments/assets/483a46b1-f5fd-496e-bfcb-4193a3ca8ec3" /> | |
||
|
Thomas Kosiewski
|
dd6aec04d7 | fix(coderd/oauth2provider): support client_secret_basic client auth (#21793) | ||
|
Ehab Younes
|
6683d807ac |
refactor: add RFC-compliant enum types and use SDK as source of truth (#21468)
Add comprehensive OAuth2 enum types to codersdk following RFC specifications: - OAuth2ProviderGrantType (RFC 6749) - OAuth2ProviderResponseType (RFC 6749) - OAuth2TokenEndpointAuthMethod (RFC 7591) - OAuth2PKCECodeChallengeMethod (RFC 7636) - OAuth2TokenType (RFC 6749, RFC 9449) - OAuth2RevocationTokenTypeHint (RFC 7009) - OAuth2ErrorCode (RFC 6749, RFC 7009, RFC 8707) Add OAuth2TokenRequest, OAuth2TokenResponse, OAuth2TokenRevocationRequest, and OAuth2Error structs to the SDK. Update OAuth2ClientRegistrationRequest, OAuth2ClientRegistrationResponse, OAuth2ClientConfiguration, and OAuth2AuthorizationServerMetadata to use typed enums instead of raw strings. This makes codersdk the single source of truth for OAuth2 types, eliminating duplication between SDK and server-side structs. Closes #21476 |
||
|
Spike Curtis
|
bddb808b25 |
chore: arrange imports in a standard way (#21452)
Fixes all our Go file imports to match the preferred spec that we've _mostly_ been using. For example: ``` import ( "context" "time" "github.com/prometheus/client_golang/prometheus" "golang.org/x/xerrors" "gopkg.in/natefinch/lumberjack.v2" "cdr.dev/slog/v3" "github.com/coder/coder/v2/codersdk/agentsdk" "github.com/coder/serpent" ) ``` 3 groups: standard library, 3rd partly libs, Coder libs. This PR makes the change across the codebase. The PR in the stack above modifies our formatting to maintain this state of affairs, and is a separate PR so it's possible to review that one in detail. |
||
|
Spike Curtis
|
49b34a716a |
fix: fix slog to always use array of Fields (#21426)
Upgrades to slog v3 which includes a small, but backward incompatible API change to the acceptible call arguments when logging. This change allows us to verify via compile time type checking that arguments are correct and won't cause a panic, as was possible in slog v1, which this replaces (v2 was tagged but never used in coder/coder). It also updates dependencies that also use slog and were updated. I've left the `aibridge` dependency as a commit SHA, under the assumption that the team there (cc @pawbana @dannykopping ) will tag and update the dependency soon and on their own schedule. Other dependencies, I pushed new tags. |
||
|
Asher
|
3a0e8af6e3 |
feat: add view workspace button to app error page (#20960)
Closes #19984 As part of this, I refactored the error template to take in a slice of actions rather than using individual booleans and strings to control the behavior. We decided a link resolves the issue for now so that is what I added, although we may want to consider a way to start the workspace and follow the logs dynamically on that page and then show the app when finished (similar to the tasks page), or at least make the link automatically start the workspace instead of only taking you to the dashboard where you have to then start the workspace. |
||
|
Steven Masley
|
54497f4f6b |
chore: add revocation endpoint to oauth well-known (#20561)
Was added to apps endpoints, but not the wider site ones. This is a site wide oauth route |
||
|
Steven Masley
|
13ca9ead3a |
chore!: ensure consistent secret token generation and hashing (#20388)
This PR uses the same sha256 hashing technique as we use for APIKeys. So now all randomly generated secrets will be hashed with sha256 for consistency. This is a breaking change for the oauth tokens. Since oauth is only allowed for dev builds and experimental, this is ok. |
||
|
Steven Masley
|
4bd7c7b7e0 |
feat: implement oauth2 RFC 7009 token revocation endpoint (#20362)
Adds RFC 7009 token revocation endpoint |
||
|
Thomas Kosiewski
|
251f787743 | fix: normalize oauth2 scope parsing (#20359) | ||
|
Thomas Kosiewski
|
05537c1894 |
feat: publish RBAC scopes in OAuth2 metadata endpoints (#19942)
<!-- If you have used AI to produce some or all of this PR, please ensure you have read our [AI Contribution guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING) before submitting. --> Publish supported OAuth2 scopes from RBAC external scope names This PR updates the OAuth2 metadata endpoints to publish the supported scopes from the RBAC external scope names. Previously, the `ScopesSupported` field was empty with a TODO to implement a scope system. Now, both the authorization server metadata and protected resource metadata endpoints return the list of scopes from `rbac.ExternalScopeNames()`. The tests have been updated to verify that the correct scopes are being returned in the metadata responses. |
||
|
Thomas Kosiewski
|
6fb4cc6b82 |
fix: add retry logic to OAuth2 metadata tests to avoid race conditions (#19813)
This PR adds a readiness wait to OAuth2 metadata endpoint tests to avoid rare races with server startup. Instead of immediately making HTTP requests, the tests now use `testutil.Eventually` to retry the requests until they succeed, with a short interval between attempts. This helps prevent flaky tests that might fail due to timing issues during server initialization. Fixes: https://github.com/coder/internal/issues/996 |
||
|
Thomas Kosiewski
|
088d14933c | feat: ensure OAuth2 refresh tokens outlive access tokens (#19769) | ||
|
Thomas Kosiewski
|
7b06fc77ae |
refactor: simplify OAuth2 authorization flow and use 302 redirects (#18923)
# Refactor OAuth2 Provider Authorization Flow This PR refactors the OAuth2 provider authorization flow by: 1. Removing the `authorizeMW` middleware and directly implementing its functionality in the `ShowAuthorizePage` handler 2. Simplifying function signatures by removing unnecessary parameters: - Removed `db` parameter from `ShowAuthorizePage` - Removed `accessURL` parameter from `ProcessAuthorize` 3. Changing the redirect status code in `ProcessAuthorize` from 307 (Temporary Redirect) to 302 (Found) to improve compatibility with external OAuth2 apps and browsers. (Technical explanation: we replied with a 307 to a POST request, thus the browser performs a redirect to that URL as a POST request, but we need it to be a GET request to be compatible. Thus, we use the 302 redirect so that browsers turn it into a GET request when redirecting back to the redirect_uri.) The changes maintain the same functionality while simplifying the code and improving compatibility with external systems. |
||
|
Thomas Kosiewski
|
071383bbe8 |
feat: add RFC 9728 OAuth2 resource metadata support (#18920)
# Enhanced OAuth2 and MCP Compliance for API Authentication This PR improves OAuth2 and MCP (Microsoft Cloud for Sovereignty) compliance by: 1. Adding RFC 9728 compliant `WWW-Authenticate` headers with resource metadata URLs 2. Passing the configured `AccessURL` to API key middleware for proper audience validation 3. Creating specialized CORS handling for OAuth2 and MCP endpoints with appropriate headers 4. Making the `state` parameter optional in OAuth2 authorization requests These changes ensure proper OAuth2 token audience validation against the configured access URL and improve interoperability with OAuth2 clients by providing better error responses and metadata discovery. Signed-off-by: Thomas Kosiewski <tk@coder.com> |
||
|
Thomas Kosiewski
|
4607e5113b |
refactor: organize OAuth2 provider tests into dedicated packages (#18747)
# OAuth2 Provider Code Reorganization This PR reorganizes the OAuth2 provider code to improve separation of concerns and maintainability. The changes include: 1. Migrating OAuth2 provider app validation tests from `coderd/oauth2_test.go` to `oauth2provider/provider_test.go` 2. Moving OAuth2 client registration validation tests to `oauth2provider/validation_test.go` 3. Adding new comprehensive test files for metadata and validation edge cases 4. Renaming `OAuth2ProviderAppSecret` to `AppSecret` for better naming consistency 5. Simplifying the main integration test in `oauth2_test.go` to focus on core functionality The PR maintains all existing test coverage while organizing the code more logically, making it easier to understand and maintain the OAuth2 provider implementation. This reorganization will help with future enhancements to the OAuth2 provider functionality. |
||
|
Thomas Kosiewski
|
c65013384a |
refactor: move OAuth2 provider code to dedicated package (#18746)
# Refactor OAuth2 Provider Code into Dedicated Package This PR refactors the OAuth2 provider functionality by moving it from the main `coderd` package into a dedicated `oauth2provider` package. The change improves code organization and maintainability without changing functionality. Key changes: - Created a new `oauth2provider` package to house all OAuth2 provider-related code - Moved existing OAuth2 provider functionality from `coderd/identityprovider` to the new package - Refactored handler functions to follow a consistent pattern of returning `http.HandlerFunc` instead of being handlers directly - Split large files into smaller, more focused files organized by functionality: - `app_secrets.go` - Manages OAuth2 application secrets - `apps.go` - Handles OAuth2 application CRUD operations - `authorize.go` - Implements the authorization flow - `metadata.go` - Provides OAuth2 metadata endpoints - `registration.go` - Handles dynamic client registration - `revoke.go` - Implements token revocation - `secrets.go` - Manages secret generation and validation - `tokens.go` - Handles token issuance and validation This refactoring improves code organization and makes the OAuth2 provider functionality more maintainable while preserving all existing behavior. |