From b776a14b46fc14a972d15d59473daa5f150d452c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Mon, 23 Feb 2026 12:18:44 +0100 Subject: [PATCH] fix(coderd): harden OAuth2 provider security (#22194) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 ---
📋 Implementation Plan # 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.
Why both exact match AND store+verify? 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.
### 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
--- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-6` • Thinking: `xhigh`_ --- coderd/database/dbgen/dbgen.go | 2 + coderd/database/dump.sql | 8 +- ...h2_provider_app_codes_add_columns.down.sql | 3 + ...uth2_provider_app_codes_add_columns.up.sql | 9 ++ coderd/database/models.go | 4 + coderd/database/queries.sql.go | 24 +++- coderd/database/queries/oauth2.sql | 8 +- coderd/httpapi/queryparams.go | 7 +- coderd/httpmw/csrf.go | 8 +- coderd/httpmw/csrf_test.go | 20 +++ coderd/mcp/mcp_e2e_test.go | 62 +++++--- coderd/oauth2_test.go | 135 ++++++++++++------ coderd/oauth2provider/authorize.go | 74 ++++++++-- .../oauth2provider/authorize_internal_test.go | 53 +++++++ coderd/oauth2provider/authorize_test.go | 32 +++++ .../oauth2providertest/oauth2_test.go | 84 ++++++----- coderd/oauth2provider/tokens.go | 27 +++- coderd/oauth2provider/tokens_internal_test.go | 29 ++++ codersdk/oauth2.go | 6 +- docs/admin/integrations/oauth2-provider.md | 25 +++- site/site.go | 6 + site/static/oauth2allow.html | 1 + 22 files changed, 484 insertions(+), 143 deletions(-) create mode 100644 coderd/database/migrations/000420_oauth2_provider_app_codes_add_columns.down.sql create mode 100644 coderd/database/migrations/000420_oauth2_provider_app_codes_add_columns.up.sql create mode 100644 coderd/oauth2provider/authorize_internal_test.go create mode 100644 coderd/oauth2provider/authorize_test.go diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index e9ae41391f..0fdc1ea522 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -1373,6 +1373,8 @@ func OAuth2ProviderAppCode(t testing.TB, db database.Store, seed database.OAuth2 ResourceUri: seed.ResourceUri, CodeChallenge: seed.CodeChallenge, CodeChallengeMethod: seed.CodeChallengeMethod, + StateHash: seed.StateHash, + RedirectUri: seed.RedirectUri, }) require.NoError(t, err, "insert oauth2 app code") return code diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 90467591c1..7a6e4ad8fc 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -1471,7 +1471,9 @@ CREATE TABLE oauth2_provider_app_codes ( app_id uuid NOT NULL, resource_uri text, code_challenge text, - code_challenge_method text + code_challenge_method text, + state_hash text, + redirect_uri text ); COMMENT ON TABLE oauth2_provider_app_codes IS 'Codes are meant to be exchanged for access tokens.'; @@ -1482,6 +1484,10 @@ COMMENT ON COLUMN oauth2_provider_app_codes.code_challenge IS 'PKCE code challen COMMENT ON COLUMN oauth2_provider_app_codes.code_challenge_method IS 'PKCE challenge method (S256)'; +COMMENT ON COLUMN oauth2_provider_app_codes.state_hash IS 'SHA-256 hash of the OAuth2 state parameter, stored to prevent state reflection attacks.'; + +COMMENT ON COLUMN oauth2_provider_app_codes.redirect_uri IS 'The redirect_uri provided during authorization, to be verified during token exchange (RFC 6749 §4.1.3).'; + CREATE TABLE oauth2_provider_app_secrets ( id uuid NOT NULL, created_at timestamp with time zone NOT NULL, diff --git a/coderd/database/migrations/000420_oauth2_provider_app_codes_add_columns.down.sql b/coderd/database/migrations/000420_oauth2_provider_app_codes_add_columns.down.sql new file mode 100644 index 0000000000..8538b13728 --- /dev/null +++ b/coderd/database/migrations/000420_oauth2_provider_app_codes_add_columns.down.sql @@ -0,0 +1,3 @@ +ALTER TABLE oauth2_provider_app_codes + DROP COLUMN state_hash, + DROP COLUMN redirect_uri; diff --git a/coderd/database/migrations/000420_oauth2_provider_app_codes_add_columns.up.sql b/coderd/database/migrations/000420_oauth2_provider_app_codes_add_columns.up.sql new file mode 100644 index 0000000000..9b343d9cfa --- /dev/null +++ b/coderd/database/migrations/000420_oauth2_provider_app_codes_add_columns.up.sql @@ -0,0 +1,9 @@ +ALTER TABLE oauth2_provider_app_codes + ADD COLUMN state_hash text, + ADD COLUMN redirect_uri text; + +COMMENT ON COLUMN oauth2_provider_app_codes.state_hash IS + 'SHA-256 hash of the OAuth2 state parameter, stored to prevent state reflection attacks.'; + +COMMENT ON COLUMN oauth2_provider_app_codes.redirect_uri IS + 'The redirect_uri provided during authorization, to be verified during token exchange (RFC 6749 §4.1.3).'; diff --git a/coderd/database/models.go b/coderd/database/models.go index 0dc4a48f21..24ab3608f9 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -4026,6 +4026,10 @@ type OAuth2ProviderAppCode struct { CodeChallenge sql.NullString `db:"code_challenge" json:"code_challenge"` // PKCE challenge method (S256) CodeChallengeMethod sql.NullString `db:"code_challenge_method" json:"code_challenge_method"` + // SHA-256 hash of the OAuth2 state parameter, stored to prevent state reflection attacks. + StateHash sql.NullString `db:"state_hash" json:"state_hash"` + // The redirect_uri provided during authorization, to be verified during token exchange (RFC 6749 §4.1.3). + RedirectUri sql.NullString `db:"redirect_uri" json:"redirect_uri"` } type OAuth2ProviderAppSecret struct { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 2268d6ad24..2eb8fdb0a2 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -6768,7 +6768,7 @@ func (q *sqlQuerier) GetOAuth2ProviderAppByRegistrationToken(ctx context.Context } const getOAuth2ProviderAppCodeByID = `-- name: GetOAuth2ProviderAppCodeByID :one -SELECT id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method FROM oauth2_provider_app_codes WHERE id = $1 +SELECT id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri FROM oauth2_provider_app_codes WHERE id = $1 ` func (q *sqlQuerier) GetOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.UUID) (OAuth2ProviderAppCode, error) { @@ -6785,12 +6785,14 @@ func (q *sqlQuerier) GetOAuth2ProviderAppCodeByID(ctx context.Context, id uuid.U &i.ResourceUri, &i.CodeChallenge, &i.CodeChallengeMethod, + &i.StateHash, + &i.RedirectUri, ) return i, err } const getOAuth2ProviderAppCodeByPrefix = `-- name: GetOAuth2ProviderAppCodeByPrefix :one -SELECT id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method FROM oauth2_provider_app_codes WHERE secret_prefix = $1 +SELECT id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri FROM oauth2_provider_app_codes WHERE secret_prefix = $1 ` func (q *sqlQuerier) GetOAuth2ProviderAppCodeByPrefix(ctx context.Context, secretPrefix []byte) (OAuth2ProviderAppCode, error) { @@ -6807,6 +6809,8 @@ func (q *sqlQuerier) GetOAuth2ProviderAppCodeByPrefix(ctx context.Context, secre &i.ResourceUri, &i.CodeChallenge, &i.CodeChallengeMethod, + &i.StateHash, + &i.RedirectUri, ) return i, err } @@ -7210,7 +7214,9 @@ INSERT INTO oauth2_provider_app_codes ( user_id, resource_uri, code_challenge, - code_challenge_method + code_challenge_method, + state_hash, + redirect_uri ) VALUES( $1, $2, @@ -7221,8 +7227,10 @@ INSERT INTO oauth2_provider_app_codes ( $7, $8, $9, - $10 -) RETURNING id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method + $10, + $11, + $12 +) RETURNING id, created_at, expires_at, secret_prefix, hashed_secret, user_id, app_id, resource_uri, code_challenge, code_challenge_method, state_hash, redirect_uri ` type InsertOAuth2ProviderAppCodeParams struct { @@ -7236,6 +7244,8 @@ type InsertOAuth2ProviderAppCodeParams struct { ResourceUri sql.NullString `db:"resource_uri" json:"resource_uri"` CodeChallenge sql.NullString `db:"code_challenge" json:"code_challenge"` CodeChallengeMethod sql.NullString `db:"code_challenge_method" json:"code_challenge_method"` + StateHash sql.NullString `db:"state_hash" json:"state_hash"` + RedirectUri sql.NullString `db:"redirect_uri" json:"redirect_uri"` } func (q *sqlQuerier) InsertOAuth2ProviderAppCode(ctx context.Context, arg InsertOAuth2ProviderAppCodeParams) (OAuth2ProviderAppCode, error) { @@ -7250,6 +7260,8 @@ func (q *sqlQuerier) InsertOAuth2ProviderAppCode(ctx context.Context, arg Insert arg.ResourceUri, arg.CodeChallenge, arg.CodeChallengeMethod, + arg.StateHash, + arg.RedirectUri, ) var i OAuth2ProviderAppCode err := row.Scan( @@ -7263,6 +7275,8 @@ func (q *sqlQuerier) InsertOAuth2ProviderAppCode(ctx context.Context, arg Insert &i.ResourceUri, &i.CodeChallenge, &i.CodeChallengeMethod, + &i.StateHash, + &i.RedirectUri, ) return i, err } diff --git a/coderd/database/queries/oauth2.sql b/coderd/database/queries/oauth2.sql index 8e177a2a34..30d3e08027 100644 --- a/coderd/database/queries/oauth2.sql +++ b/coderd/database/queries/oauth2.sql @@ -140,7 +140,9 @@ INSERT INTO oauth2_provider_app_codes ( user_id, resource_uri, code_challenge, - code_challenge_method + code_challenge_method, + state_hash, + redirect_uri ) VALUES( $1, $2, @@ -151,7 +153,9 @@ INSERT INTO oauth2_provider_app_codes ( $7, $8, $9, - $10 + $10, + $11, + $12 ) RETURNING *; -- name: DeleteOAuth2ProviderAppCodeByID :exec diff --git a/coderd/httpapi/queryparams.go b/coderd/httpapi/queryparams.go index d30244eaf0..d2653c9985 100644 --- a/coderd/httpapi/queryparams.go +++ b/coderd/httpapi/queryparams.go @@ -228,12 +228,11 @@ func (p *QueryParamParser) RedirectURL(vals url.Values, base *url.URL, queryPara }) } - // It can be a sub-directory but not a sub-domain, as we have apps on - // sub-domains and that seems too dangerous. - if v.Host != base.Host || !strings.HasPrefix(v.Path, base.Path) { + // OAuth 2.1 requires exact redirect URI matching. + if v.String() != base.String() { p.Errors = append(p.Errors, codersdk.ValidationError{ Field: queryParam, - Detail: fmt.Sprintf("Query param %q must be a subset of %s", queryParam, base), + Detail: fmt.Sprintf("Query param %q must exactly match %s", queryParam, base), }) } diff --git a/coderd/httpmw/csrf.go b/coderd/httpmw/csrf.go index 7196517119..6f9915f806 100644 --- a/coderd/httpmw/csrf.go +++ b/coderd/httpmw/csrf.go @@ -62,8 +62,12 @@ func CSRF(cookieCfg codersdk.HTTPCookieConfig) func(next http.Handler) http.Hand mw.ExemptRegexp(regexp.MustCompile("/organizations/[^/]+/provisionerdaemons/*")) mw.ExemptFunc(func(r *http.Request) bool { - // Only enforce CSRF on API routes. - if !strings.HasPrefix(r.URL.Path, "/api") { + // Enforce CSRF on API routes and the OAuth2 authorize + // endpoint. The authorize endpoint serves a browser consent + // form whose POST must be CSRF-protected to prevent + // cross-site authorization code theft (coder/security#121). + if !strings.HasPrefix(r.URL.Path, "/api") && + !strings.HasPrefix(r.URL.Path, "/oauth2/authorize") { return true } diff --git a/coderd/httpmw/csrf_test.go b/coderd/httpmw/csrf_test.go index 62e8150fb0..c1365b39f9 100644 --- a/coderd/httpmw/csrf_test.go +++ b/coderd/httpmw/csrf_test.go @@ -51,6 +51,26 @@ func TestCSRFExemptList(t *testing.T) { URL: "https://coder.com/api/v2/me", Exempt: false, }, + { + Name: "OAuth2Authorize", + URL: "https://coder.com/oauth2/authorize", + Exempt: false, + }, + { + Name: "OAuth2AuthorizeQuery", + URL: "https://coder.com/oauth2/authorize?client_id=test", + Exempt: false, + }, + { + Name: "OAuth2Tokens", + URL: "https://coder.com/oauth2/tokens", + Exempt: true, + }, + { + Name: "OAuth2Register", + URL: "https://coder.com/oauth2/register", + Exempt: true, + }, } mw := httpmw.CSRF(codersdk.HTTPCookieConfig{}) diff --git a/coderd/mcp/mcp_e2e_test.go b/coderd/mcp/mcp_e2e_test.go index f101cfbdd5..c1386363cc 100644 --- a/coderd/mcp/mcp_e2e_test.go +++ b/coderd/mcp/mcp_e2e_test.go @@ -2,6 +2,8 @@ package mcp_test import ( "context" + "crypto/sha256" + "encoding/base64" "encoding/json" "fmt" "io" @@ -10,6 +12,7 @@ import ( "strings" "testing" + "github.com/google/uuid" mcpclient "github.com/mark3labs/mcp-go/client" "github.com/mark3labs/mcp-go/client/transport" "github.com/mark3labs/mcp-go/mcp" @@ -24,6 +27,15 @@ import ( "github.com/coder/coder/v2/testutil" ) +// mcpGeneratePKCE creates a PKCE verifier and S256 challenge for MCP +// e2e tests. +func mcpGeneratePKCE() (verifier, challenge string) { + verifier = uuid.NewString() + uuid.NewString() + h := sha256.Sum256([]byte(verifier)) + challenge = base64.RawURLEncoding.EncodeToString(h[:]) + return verifier, challenge +} + func TestMCPHTTP_E2E_ClientIntegration(t *testing.T) { t.Parallel() @@ -553,31 +565,32 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { // In a real flow, this would be done through the browser consent page // For testing, we'll create the code directly using the internal API - // First, we need to authorize the app (simulating user consent) - authURL := fmt.Sprintf("%s/oauth2/authorize?client_id=%s&response_type=code&redirect_uri=%s&state=test_state", - api.AccessURL.String(), app.ID, "http://localhost:3000/callback") + // First, we need to authorize the app (simulating user consent). + staticVerifier, staticChallenge := mcpGeneratePKCE() + authURL := fmt.Sprintf("%s/oauth2/authorize?client_id=%s&response_type=code&redirect_uri=%s&state=test_state&code_challenge=%s&code_challenge_method=S256", + api.AccessURL.String(), app.ID, "http://localhost:3000/callback", staticChallenge) - // Create an HTTP client that follows redirects but captures the final redirect + // Create an HTTP client that follows redirects but captures the final redirect. client := &http.Client{ CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse // Stop following redirects }, } - // Make the authorization request (this would normally be done in a browser) + // Make the authorization request (this would normally be done in a browser). req, err := http.NewRequestWithContext(ctx, "GET", authURL, nil) require.NoError(t, err) - // Use RFC 6750 Bearer token for authentication + // Use RFC 6750 Bearer token for authentication. req.Header.Set("Authorization", "Bearer "+coderClient.SessionToken()) resp, err := client.Do(req) require.NoError(t, err) defer resp.Body.Close() - // The response should be a redirect to the consent page or directly to callback - // For testing purposes, let's simulate the POST consent approval + // The response should be a redirect to the consent page or directly to callback. + // For testing purposes, let's simulate the POST consent approval. if resp.StatusCode == http.StatusOK { - // This means we got the consent page, now we need to POST consent + // This means we got the consent page, now we need to POST consent. consentReq, err := http.NewRequestWithContext(ctx, "POST", authURL, nil) require.NoError(t, err) consentReq.Header.Set("Authorization", "Bearer "+coderClient.SessionToken()) @@ -588,7 +601,7 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { defer resp.Body.Close() } - // Extract authorization code from redirect URL + // Extract authorization code from redirect URL. require.True(t, resp.StatusCode >= 300 && resp.StatusCode < 400, "Expected redirect response") location := resp.Header.Get("Location") require.NotEmpty(t, location, "Expected Location header in redirect") @@ -600,13 +613,14 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { t.Logf("Successfully obtained authorization code: %s", authCode[:10]+"...") - // Step 2: Exchange authorization code for access token and refresh token + // Step 2: Exchange authorization code for access token and refresh token. tokenRequestBody := url.Values{ "grant_type": {"authorization_code"}, "client_id": {app.ID.String()}, "client_secret": {secret.ClientSecretFull}, "code": {authCode}, "redirect_uri": {"http://localhost:3000/callback"}, + "code_verifier": {staticVerifier}, } tokenReq, err := http.NewRequestWithContext(ctx, "POST", api.AccessURL.String()+"/oauth2/tokens", @@ -868,41 +882,44 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { t.Logf("Successfully registered dynamic client: %s", clientID) - // Step 3: Perform OAuth2 authorization code flow with dynamically registered client - authURL := fmt.Sprintf("%s/oauth2/authorize?client_id=%s&response_type=code&redirect_uri=%s&state=dynamic_state", - api.AccessURL.String(), clientID, "http://localhost:3000/callback") + // Step 3: Perform OAuth2 authorization code flow with dynamically registered client. + dynamicVerifier, dynamicChallenge := mcpGeneratePKCE() + authURL := fmt.Sprintf("%s/oauth2/authorize?client_id=%s&response_type=code&redirect_uri=%s&state=dynamic_state&code_challenge=%s&code_challenge_method=S256", + api.AccessURL.String(), clientID, "http://localhost:3000/callback", dynamicChallenge) - // Create an HTTP client that captures redirects + // Create an HTTP client that captures redirects. authClient := &http.Client{ CheckRedirect: func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse // Stop following redirects }, } - // Make the authorization request with authentication + // Make the authorization request with authentication. authReq, err := http.NewRequestWithContext(ctx, "GET", authURL, nil) require.NoError(t, err) authReq.Header.Set("Cookie", fmt.Sprintf("coder_session_token=%s", coderClient.SessionToken())) + authReq.Header.Set("Authorization", "Bearer "+coderClient.SessionToken()) authResp, err := authClient.Do(authReq) require.NoError(t, err) defer authResp.Body.Close() - // Handle the response - check for error first + // Handle the response - check for error first. if authResp.StatusCode == http.StatusBadRequest { - // Read error response for debugging + // Read error response for debugging. bodyBytes, err := io.ReadAll(authResp.Body) require.NoError(t, err) t.Logf("OAuth2 authorization error: %s", string(bodyBytes)) t.FailNow() } - // Handle consent flow if needed + // Handle consent flow if needed. if authResp.StatusCode == http.StatusOK { - // This means we got the consent page, now we need to POST consent + // This means we got the consent page, now we need to POST consent. consentReq, err := http.NewRequestWithContext(ctx, "POST", authURL, nil) require.NoError(t, err) consentReq.Header.Set("Cookie", fmt.Sprintf("coder_session_token=%s", coderClient.SessionToken())) + consentReq.Header.Set("Authorization", "Bearer "+coderClient.SessionToken()) consentReq.Header.Set("Content-Type", "application/x-www-form-urlencoded") authResp, err = authClient.Do(consentReq) @@ -910,7 +927,7 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { defer authResp.Body.Close() } - // Extract authorization code from redirect + // Extract authorization code from redirect. require.True(t, authResp.StatusCode >= 300 && authResp.StatusCode < 400, "Expected redirect response, got %d", authResp.StatusCode) location := authResp.Header.Get("Location") @@ -923,13 +940,14 @@ func TestMCPHTTP_E2E_OAuth2_EndToEnd(t *testing.T) { t.Logf("Successfully obtained authorization code: %s", authCode[:10]+"...") - // Step 4: Exchange authorization code for access token + // Step 4: Exchange authorization code for access token. tokenRequestBody := url.Values{ "grant_type": {"authorization_code"}, "client_id": {clientID}, "client_secret": {clientSecret}, "code": {authCode}, "redirect_uri": {"http://localhost:3000/callback"}, + "code_verifier": {dynamicVerifier}, } tokenReq, err := http.NewRequestWithContext(ctx, "POST", api.AccessURL.String()+"/oauth2/tokens", diff --git a/coderd/oauth2_test.go b/coderd/oauth2_test.go index f8b203581f..f898d38e55 100644 --- a/coderd/oauth2_test.go +++ b/coderd/oauth2_test.go @@ -2,6 +2,8 @@ package coderd_test import ( "context" + "crypto/sha256" + "encoding/base64" "encoding/json" "fmt" "net/http" @@ -289,7 +291,6 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { authError: "Invalid query params:", }, { - // TODO: This is valid for now, but should it be? name: "DifferentProtocol", app: apps.Default, preAuth: func(valid *oauth2.Config) { @@ -297,6 +298,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { newURL.Scheme = "https" valid.RedirectURL = newURL.String() }, + authError: "Invalid query params:", }, { name: "NestedPath", @@ -306,6 +308,7 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { newURL.Path = path.Join(newURL.Path, "nested") valid.RedirectURL = newURL.String() }, + authError: "Invalid query params:", }, { // Some oauth implementations allow this, but our users can host @@ -481,11 +484,12 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { } var code string + var verifier string if test.defaultCode != nil { code = *test.defaultCode } else { var err error - code, err = authorizationFlow(ctx, userClient, valid) + code, verifier, err = authorizationFlow(ctx, userClient, valid) if test.authError != "" { require.Error(t, err) require.ErrorContains(t, err, test.authError) @@ -500,8 +504,12 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) { test.preToken(valid) } - // Do the actual exchange. - token, err := valid.Exchange(ctx, code, test.exchangeMutate...) + // Do the actual exchange. Include PKCE code_verifier when + // we obtained a code through the authorization flow. + exchangeOpts := append([]oauth2.AuthCodeOption{ + oauth2.SetAuthURLParam("code_verifier", verifier), + }, test.exchangeMutate...) + token, err := valid.Exchange(ctx, code, exchangeOpts...) if test.tokenError != "" { require.Error(t, err) require.ErrorContains(t, err, test.tokenError) @@ -683,10 +691,11 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) { } type exchangeSetup struct { - cfg *oauth2.Config - app codersdk.OAuth2ProviderApp - secret codersdk.OAuth2ProviderAppSecretFull - code string + cfg *oauth2.Config + app codersdk.OAuth2ProviderApp + secret codersdk.OAuth2ProviderAppSecretFull + code string + verifier string } func TestOAuth2ProviderRevoke(t *testing.T) { @@ -730,11 +739,13 @@ func TestOAuth2ProviderRevoke(t *testing.T) { name: "OverrideCodeAndToken", fn: func(ctx context.Context, client *codersdk.Client, s exchangeSetup) { // Generating a new code should wipe out the old code. - code, err := authorizationFlow(ctx, client, s.cfg) + code, verifier, err := authorizationFlow(ctx, client, s.cfg) require.NoError(t, err) // Generating a new token should wipe out the old token. - _, err = s.cfg.Exchange(ctx, code) + _, err = s.cfg.Exchange(ctx, code, + oauth2.SetAuthURLParam("code_verifier", verifier), + ) require.NoError(t, err) }, replacesToken: true, @@ -770,14 +781,15 @@ func TestOAuth2ProviderRevoke(t *testing.T) { } // Go through the auth flow to get a code. - code, err := authorizationFlow(ctx, testClient, cfg) + code, verifier, err := authorizationFlow(ctx, testClient, cfg) require.NoError(t, err) return exchangeSetup{ - cfg: cfg, - app: app, - secret: secret, - code: code, + cfg: cfg, + app: app, + secret: secret, + code: code, + verifier: verifier, } } @@ -794,12 +806,16 @@ func TestOAuth2ProviderRevoke(t *testing.T) { test.fn(ctx, testClient, testEntities) // Exchange should fail because the code should be gone. - _, err := testEntities.cfg.Exchange(ctx, testEntities.code) + _, err := testEntities.cfg.Exchange(ctx, testEntities.code, + oauth2.SetAuthURLParam("code_verifier", testEntities.verifier), + ) require.Error(t, err) // Try again, this time letting the exchange complete first. testEntities = setup(ctx, testClient, test.name+"-2") - token, err := testEntities.cfg.Exchange(ctx, testEntities.code) + token, err := testEntities.cfg.Exchange(ctx, testEntities.code, + oauth2.SetAuthURLParam("code_verifier", testEntities.verifier), + ) require.NoError(t, err) // Validate the returned access token and that the app is listed. @@ -872,25 +888,38 @@ func generateApps(ctx context.Context, t *testing.T, client *codersdk.Client, su } } -func authorizationFlow(ctx context.Context, client *codersdk.Client, cfg *oauth2.Config) (string, error) { - state := uuid.NewString() - authURL := cfg.AuthCodeURL(state) +// generatePKCE creates a PKCE verifier and S256 challenge for testing. +func generatePKCE() (verifier, challenge string) { + verifier = uuid.NewString() + uuid.NewString() + h := sha256.Sum256([]byte(verifier)) + challenge = base64.RawURLEncoding.EncodeToString(h[:]) + return verifier, challenge +} - // Make a POST request to simulate clicking "Allow" on the authorization page - // This bypasses the HTML consent page and directly processes the authorization - return oidctest.OAuth2GetCode( +func authorizationFlow(ctx context.Context, client *codersdk.Client, cfg *oauth2.Config) (code, codeVerifier string, err error) { + state := uuid.NewString() + codeVerifier, challenge := generatePKCE() + authURL := cfg.AuthCodeURL(state, + oauth2.SetAuthURLParam("code_challenge", challenge), + oauth2.SetAuthURLParam("code_challenge_method", "S256"), + ) + + // Make a POST request to simulate clicking "Allow" on the authorization page. + // This bypasses the HTML consent page and directly processes the authorization. + code, err = oidctest.OAuth2GetCode( authURL, func(req *http.Request) (*http.Response, error) { - // Change to POST to simulate the form submission + // Change to POST to simulate the form submission. req.Method = http.MethodPost - // Prevent automatic redirect following + // Prevent automatic redirect following. client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse } return client.Request(ctx, req.Method, req.URL.String(), nil) }, ) + return code, codeVerifier, err } func must[T any](value T, err error) T { @@ -997,11 +1026,15 @@ func TestOAuth2ProviderResourceIndicators(t *testing.T) { Scopes: []string{}, } - // Step 1: Authorization with resource parameter + // Step 1: Authorization with resource parameter and PKCE. state := uuid.NewString() - authURL := cfg.AuthCodeURL(state) + verifier, challenge := generatePKCE() + authURL := cfg.AuthCodeURL(state, + oauth2.SetAuthURLParam("code_challenge", challenge), + oauth2.SetAuthURLParam("code_challenge_method", "S256"), + ) if test.authResource != "" { - // Add resource parameter to auth URL + // Add resource parameter to auth URL. parsedURL, err := url.Parse(authURL) require.NoError(t, err) query := parsedURL.Query() @@ -1030,7 +1063,7 @@ func TestOAuth2ProviderResourceIndicators(t *testing.T) { // Step 2: Token exchange with resource parameter // Use custom token exchange since golang.org/x/oauth2 doesn't support resource parameter in token requests - token, err := customTokenExchange(ctx, ownerClient.URL.String(), apps.Default.ID.String(), secret.ClientSecretFull, code, apps.Default.CallbackURL, test.tokenResource) + token, err := customTokenExchange(ctx, ownerClient.URL.String(), apps.Default.ID.String(), secret.ClientSecretFull, code, apps.Default.CallbackURL, test.tokenResource, verifier) if test.expectTokenError { require.Error(t, err) require.Contains(t, err.Error(), "invalid_target") @@ -1127,9 +1160,13 @@ func TestOAuth2ProviderCrossResourceAudienceValidation(t *testing.T) { Scopes: []string{}, } - // Authorization with resource parameter for server1 + // Authorization with resource parameter for server1 and PKCE. state := uuid.NewString() - authURL := cfg.AuthCodeURL(state) + verifier, challenge := generatePKCE() + authURL := cfg.AuthCodeURL(state, + oauth2.SetAuthURLParam("code_challenge", challenge), + oauth2.SetAuthURLParam("code_challenge_method", "S256"), + ) parsedURL, err := url.Parse(authURL) require.NoError(t, err) query := parsedURL.Query() @@ -1149,8 +1186,11 @@ func TestOAuth2ProviderCrossResourceAudienceValidation(t *testing.T) { ) require.NoError(t, err) - // Exchange code for token with resource parameter - token, err := cfg.Exchange(ctx, code, oauth2.SetAuthURLParam("resource", resource1)) + // Exchange code for token with resource parameter and PKCE verifier. + token, err := cfg.Exchange(ctx, code, + oauth2.SetAuthURLParam("resource", resource1), + oauth2.SetAuthURLParam("code_verifier", verifier), + ) require.NoError(t, err) require.NotEmpty(t, token.AccessToken) @@ -1226,9 +1266,11 @@ func TestOAuth2RefreshExpiryOutlivesAccess(t *testing.T) { } // Authorization and token exchange - code, err := authorizationFlow(ctx, ownerClient, cfg) + code, verifier, err := authorizationFlow(ctx, ownerClient, cfg) require.NoError(t, err) - tok, err := cfg.Exchange(ctx, code) + tok, err := cfg.Exchange(ctx, code, + oauth2.SetAuthURLParam("code_verifier", verifier), + ) require.NoError(t, err) require.NotEmpty(t, tok.AccessToken) require.NotEmpty(t, tok.RefreshToken) @@ -1253,7 +1295,7 @@ func TestOAuth2RefreshExpiryOutlivesAccess(t *testing.T) { // customTokenExchange performs a custom OAuth2 token exchange with support for resource parameter // This is needed because golang.org/x/oauth2 doesn't support custom parameters in token requests -func customTokenExchange(ctx context.Context, baseURL, clientID, clientSecret, code, redirectURI, resource string) (*oauth2.Token, error) { +func customTokenExchange(ctx context.Context, baseURL, clientID, clientSecret, code, redirectURI, resource, codeVerifier string) (*oauth2.Token, error) { data := url.Values{} data.Set("grant_type", "authorization_code") data.Set("code", code) @@ -1263,6 +1305,9 @@ func customTokenExchange(ctx context.Context, baseURL, clientID, clientSecret, c if resource != "" { data.Set("resource", resource) } + if codeVerifier != "" { + data.Set("code_verifier", codeVerifier) + } req, err := http.NewRequestWithContext(ctx, "POST", baseURL+"/oauth2/tokens", strings.NewReader(data.Encode())) if err != nil { @@ -1637,17 +1682,21 @@ func TestOAuth2CoderClient(t *testing.T) { // Make a new user client, user := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID) - // Do an OAuth2 token exchange and get a new client with an oauth token + // Do an OAuth2 token exchange and get a new client with an oauth token. state := uuid.NewString() + verifier, challenge := generatePKCE() - // Get an OAuth2 code for a token exchange + // Get an OAuth2 code for a token exchange. code, err := oidctest.OAuth2GetCode( - cfg.AuthCodeURL(state), + cfg.AuthCodeURL(state, + oauth2.SetAuthURLParam("code_challenge", challenge), + oauth2.SetAuthURLParam("code_challenge_method", "S256"), + ), func(req *http.Request) (*http.Response, error) { - // Change to POST to simulate the form submission + // Change to POST to simulate the form submission. req.Method = http.MethodPost - // Prevent automatic redirect following + // Prevent automatic redirect following. client.HTTPClient.CheckRedirect = func(req *http.Request, via []*http.Request) error { return http.ErrUseLastResponse } @@ -1656,7 +1705,9 @@ func TestOAuth2CoderClient(t *testing.T) { ) require.NoError(t, err) - token, err := cfg.Exchange(ctx, code) + token, err := cfg.Exchange(ctx, code, + oauth2.SetAuthURLParam("code_verifier", verifier), + ) require.NoError(t, err) // Use the oauth client's authentication diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 241547702c..15e85e8352 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -1,7 +1,9 @@ package oauth2provider import ( + "crypto/sha256" "database/sql" + "encoding/hex" "errors" "net/http" "net/url" @@ -9,6 +11,7 @@ import ( "time" "github.com/google/uuid" + "github.com/justinas/nosurf" "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/database" @@ -22,6 +25,7 @@ import ( type authorizeParams struct { clientID string redirectURL *url.URL + redirectURIProvided bool responseType codersdk.OAuth2ProviderResponseType scope []string state string @@ -34,11 +38,13 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar p := httpapi.NewQueryParamParser() vals := r.URL.Query() + // response_type and client_id are always required. p.RequiredNotEmpty("response_type", "client_id") params := authorizeParams{ clientID: p.String(vals, "", "client_id"), redirectURL: p.RedirectURL(vals, callbackURL, "redirect_uri"), + redirectURIProvided: vals.Get("redirect_uri") != "", responseType: httpapi.ParseCustom(p, vals, "", "response_type", httpapi.ParseEnum[codersdk.OAuth2ProviderResponseType]), scope: strings.Fields(strings.TrimSpace(p.String(vals, "", "scope"))), state: p.String(vals, "", "state"), @@ -46,6 +52,15 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar codeChallenge: p.String(vals, "", "code_challenge"), codeChallengeMethod: p.String(vals, "", "code_challenge_method"), } + + // PKCE is required for authorization code flow requests. + if params.responseType == codersdk.OAuth2ProviderResponseTypeCode && params.codeChallenge == "" { + p.Errors = append(p.Errors, codersdk.ValidationError{ + Field: "code_challenge", + Detail: `Query param "code_challenge" is required and cannot be empty`, + }) + } + // Validate resource indicator syntax (RFC 8707): must be absolute URI without fragment if err := validateResourceParameter(params.resource); err != nil { p.Errors = append(p.Errors, codersdk.ValidationError{ @@ -112,6 +127,22 @@ func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc { return } + if params.responseType != codersdk.OAuth2ProviderResponseTypeCode { + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ + Status: http.StatusBadRequest, + HideStatus: false, + Title: "Unsupported Response Type", + Description: "Only response_type=code is supported.", + Actions: []site.Action{ + { + URL: accessURL.String(), + Text: "Back to site", + }, + }, + }) + return + } + cancel := params.redirectURL cancelQuery := params.redirectURL.Query() cancelQuery.Add("error", "access_denied") @@ -122,6 +153,7 @@ func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc { AppName: app.Name, CancelURI: cancel.String(), RedirectURI: r.URL.String(), + CSRFToken: nosurf.Token(r), Username: ua.FriendlyName, }) } @@ -147,16 +179,23 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { return } - // Validate PKCE for public clients (MCP requirement) - if params.codeChallenge != "" { - // If code_challenge is provided but method is not, default to S256 - if params.codeChallengeMethod == "" { - params.codeChallengeMethod = string(codersdk.OAuth2PKCECodeChallengeMethodS256) - } - if err := codersdk.ValidatePKCECodeChallengeMethod(params.codeChallengeMethod); err != nil { - httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error()) - return - } + // OAuth 2.1 removes the implicit grant. Only + // authorization code flow is supported. + if params.responseType != codersdk.OAuth2ProviderResponseTypeCode { + httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, + codersdk.OAuth2ErrorCodeUnsupportedResponseType, + "Only response_type=code is supported") + return + } + + // code_challenge is required (enforced by RequiredNotEmpty above), + // but default the method to S256 if omitted. + if params.codeChallengeMethod == "" { + params.codeChallengeMethod = string(codersdk.OAuth2PKCECodeChallengeMethodS256) + } + if err := codersdk.ValidatePKCECodeChallengeMethod(params.codeChallengeMethod); err != nil { + httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error()) + return } // TODO: Ignoring scope for now, but should look into implementing. @@ -194,6 +233,8 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { ResourceUri: sql.NullString{String: params.resource, Valid: params.resource != ""}, CodeChallenge: sql.NullString{String: params.codeChallenge, Valid: params.codeChallenge != ""}, CodeChallengeMethod: sql.NullString{String: params.codeChallengeMethod, Valid: params.codeChallengeMethod != ""}, + StateHash: hashOAuth2State(params.state), + RedirectUri: sql.NullString{String: params.redirectURL.String(), Valid: params.redirectURIProvided}, }) if err != nil { return xerrors.Errorf("insert oauth2 authorization code: %w", err) @@ -218,3 +259,16 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { http.Redirect(rw, r, params.redirectURL.String(), http.StatusFound) } } + +// hashOAuth2State returns a SHA-256 hash of the OAuth2 state parameter. If +// the state is empty, it returns a null string. +func hashOAuth2State(state string) sql.NullString { + if state == "" { + return sql.NullString{} + } + hash := sha256.Sum256([]byte(state)) + return sql.NullString{ + String: hex.EncodeToString(hash[:]), + Valid: true, + } +} diff --git a/coderd/oauth2provider/authorize_internal_test.go b/coderd/oauth2provider/authorize_internal_test.go new file mode 100644 index 0000000000..2e23b96188 --- /dev/null +++ b/coderd/oauth2provider/authorize_internal_test.go @@ -0,0 +1,53 @@ +//nolint:testpackage // Internal test for unexported hashOAuth2State helper. +package oauth2provider + +import ( + "crypto/sha256" + "encoding/hex" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestHashOAuth2State(t *testing.T) { + t.Parallel() + + t.Run("EmptyState", func(t *testing.T) { + t.Parallel() + result := hashOAuth2State("") + assert.False(t, result.Valid, "empty state should return invalid NullString") + assert.Empty(t, result.String, "empty state should return empty string") + }) + + t.Run("NonEmptyState", func(t *testing.T) { + t.Parallel() + state := "test-state-value" + result := hashOAuth2State(state) + require.True(t, result.Valid, "non-empty state should return valid NullString") + + // Verify it's a proper SHA-256 hash. + expected := sha256.Sum256([]byte(state)) + assert.Equal(t, hex.EncodeToString(expected[:]), result.String, + "state hash should be SHA-256 hex digest") + }) + + t.Run("DifferentStatesProduceDifferentHashes", func(t *testing.T) { + t.Parallel() + hash1 := hashOAuth2State("state-a") + hash2 := hashOAuth2State("state-b") + require.True(t, hash1.Valid) + require.True(t, hash2.Valid) + assert.NotEqual(t, hash1.String, hash2.String, + "different states should produce different hashes") + }) + + t.Run("SameStateProducesSameHash", func(t *testing.T) { + t.Parallel() + hash1 := hashOAuth2State("deterministic") + hash2 := hashOAuth2State("deterministic") + require.True(t, hash1.Valid) + assert.Equal(t, hash1.String, hash2.String, + "same state should produce identical hash") + }) +} diff --git a/coderd/oauth2provider/authorize_test.go b/coderd/oauth2provider/authorize_test.go new file mode 100644 index 0000000000..018ac1a02f --- /dev/null +++ b/coderd/oauth2provider/authorize_test.go @@ -0,0 +1,32 @@ +package oauth2provider_test + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/site" +) + +func TestOAuthConsentFormIncludesCSRFToken(t *testing.T) { + t.Parallel() + + const csrfFieldValue = "csrf-field-value" + req := httptest.NewRequest(http.MethodGet, "https://coder.com/oauth2/authorize", nil) + rec := httptest.NewRecorder() + + site.RenderOAuthAllowPage(rec, req, site.RenderOAuthAllowData{ + AppName: "Test OAuth App", + CancelURI: "https://coder.com/cancel", + RedirectURI: "https://coder.com/oauth2/authorize?client_id=test", + CSRFToken: csrfFieldValue, + Username: "test-user", + }) + + require.Equal(t, http.StatusOK, rec.Result().StatusCode) + assert.Contains(t, rec.Body.String(), `name="csrf_token"`) + assert.Contains(t, rec.Body.String(), `value="`+csrfFieldValue+`"`) +} diff --git a/coderd/oauth2provider/oauth2providertest/oauth2_test.go b/coderd/oauth2provider/oauth2providertest/oauth2_test.go index f3a5f514a5..22d8ac0534 100644 --- a/coderd/oauth2provider/oauth2providertest/oauth2_test.go +++ b/coderd/oauth2provider/oauth2providertest/oauth2_test.go @@ -158,7 +158,9 @@ func TestOAuth2InvalidPKCE(t *testing.T) { ) } -func TestOAuth2WithoutPKCE(t *testing.T) { +// TestOAuth2WithoutPKCEIsRejected verifies that authorization requests without +// a code_challenge are rejected now that PKCE is mandatory. +func TestOAuth2WithoutPKCEIsRejected(t *testing.T) { t.Parallel() client := coderdtest.New(t, &coderdtest.Options{ @@ -166,15 +168,15 @@ func TestOAuth2WithoutPKCE(t *testing.T) { }) _ = coderdtest.CreateFirstUser(t, client) - // Create OAuth2 app - app, clientSecret := oauth2providertest.CreateTestOAuth2App(t, client) + // Create OAuth2 app. + app, _ := oauth2providertest.CreateTestOAuth2App(t, client) t.Cleanup(func() { oauth2providertest.CleanupOAuth2App(t, client, app.ID) }) state := oauth2providertest.GenerateState(t) - // Perform authorization without PKCE + // Authorization without code_challenge should be rejected. authParams := oauth2providertest.AuthorizeParams{ ClientID: app.ID.String(), ResponseType: "code", @@ -182,21 +184,9 @@ func TestOAuth2WithoutPKCE(t *testing.T) { State: state, } - code := oauth2providertest.AuthorizeOAuth2App(t, client, client.URL.String(), authParams) - require.NotEmpty(t, code, "should receive authorization code") - - // Exchange code for token without PKCE - tokenParams := oauth2providertest.TokenExchangeParams{ - GrantType: "authorization_code", - Code: code, - ClientID: app.ID.String(), - ClientSecret: clientSecret, - RedirectURI: oauth2providertest.TestRedirectURI, - } - - token := oauth2providertest.ExchangeCodeForToken(t, client.URL.String(), tokenParams) - require.NotEmpty(t, token.AccessToken, "should receive access token") - require.NotEmpty(t, token.RefreshToken, "should receive refresh token") + oauth2providertest.AuthorizeOAuth2AppExpectingError( + t, client, client.URL.String(), authParams, http.StatusBadRequest, + ) } func TestOAuth2TokenExchangeClientSecretBasic(t *testing.T) { @@ -212,13 +202,16 @@ func TestOAuth2TokenExchangeClientSecretBasic(t *testing.T) { oauth2providertest.CleanupOAuth2App(t, client, app.ID) }) + codeVerifier, codeChallenge := oauth2providertest.GeneratePKCE(t) state := oauth2providertest.GenerateState(t) authParams := oauth2providertest.AuthorizeParams{ - ClientID: app.ID.String(), - ResponseType: "code", - RedirectURI: oauth2providertest.TestRedirectURI, - State: state, + ClientID: app.ID.String(), + ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, + State: state, + CodeChallenge: codeChallenge, + CodeChallengeMethod: "S256", } code := oauth2providertest.AuthorizeOAuth2App(t, client, client.URL.String(), authParams) @@ -229,6 +222,7 @@ func TestOAuth2TokenExchangeClientSecretBasic(t *testing.T) { data.Set("grant_type", "authorization_code") data.Set("code", code) data.Set("redirect_uri", oauth2providertest.TestRedirectURI) + data.Set("code_verifier", codeVerifier) req, err := http.NewRequestWithContext(ctx, "POST", client.URL.String()+"/oauth2/tokens", strings.NewReader(data.Encode())) require.NoError(t, err, "failed to create token request") @@ -265,13 +259,16 @@ func TestOAuth2TokenExchangeClientSecretBasicInvalidSecret(t *testing.T) { oauth2providertest.CleanupOAuth2App(t, client, app.ID) }) + codeVerifier, codeChallenge := oauth2providertest.GeneratePKCE(t) state := oauth2providertest.GenerateState(t) authParams := oauth2providertest.AuthorizeParams{ - ClientID: app.ID.String(), - ResponseType: "code", - RedirectURI: oauth2providertest.TestRedirectURI, - State: state, + ClientID: app.ID.String(), + ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, + State: state, + CodeChallenge: codeChallenge, + CodeChallengeMethod: "S256", } code := oauth2providertest.AuthorizeOAuth2App(t, client, client.URL.String(), authParams) @@ -282,6 +279,7 @@ func TestOAuth2TokenExchangeClientSecretBasicInvalidSecret(t *testing.T) { data.Set("grant_type", "authorization_code") data.Set("code", code) data.Set("redirect_uri", oauth2providertest.TestRedirectURI) + data.Set("code_verifier", codeVerifier) wrongSecret := clientSecret + "x" @@ -349,26 +347,30 @@ func TestOAuth2ResourceParameter(t *testing.T) { }) state := oauth2providertest.GenerateState(t) + codeVerifier, codeChallenge := oauth2providertest.GeneratePKCE(t) - // Perform authorization with resource parameter + // Perform authorization with resource parameter. authParams := oauth2providertest.AuthorizeParams{ - ClientID: app.ID.String(), - ResponseType: "code", - RedirectURI: oauth2providertest.TestRedirectURI, - State: state, - Resource: oauth2providertest.TestResourceURI, + ClientID: app.ID.String(), + ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, + State: state, + CodeChallenge: codeChallenge, + CodeChallengeMethod: "S256", + Resource: oauth2providertest.TestResourceURI, } code := oauth2providertest.AuthorizeOAuth2App(t, client, client.URL.String(), authParams) require.NotEmpty(t, code, "should receive authorization code") - // Exchange code for token with resource parameter + // Exchange code for token with resource parameter. tokenParams := oauth2providertest.TokenExchangeParams{ GrantType: "authorization_code", Code: code, ClientID: app.ID.String(), ClientSecret: clientSecret, RedirectURI: oauth2providertest.TestRedirectURI, + CodeVerifier: codeVerifier, Resource: oauth2providertest.TestResourceURI, } @@ -392,13 +394,16 @@ func TestOAuth2TokenRefresh(t *testing.T) { }) state := oauth2providertest.GenerateState(t) + codeVerifier, codeChallenge := oauth2providertest.GeneratePKCE(t) - // Get initial token + // Get initial token. authParams := oauth2providertest.AuthorizeParams{ - ClientID: app.ID.String(), - ResponseType: "code", - RedirectURI: oauth2providertest.TestRedirectURI, - State: state, + ClientID: app.ID.String(), + ResponseType: "code", + RedirectURI: oauth2providertest.TestRedirectURI, + State: state, + CodeChallenge: codeChallenge, + CodeChallengeMethod: "S256", } code := oauth2providertest.AuthorizeOAuth2App(t, client, client.URL.String(), authParams) @@ -409,6 +414,7 @@ func TestOAuth2TokenRefresh(t *testing.T) { ClientID: app.ID.String(), ClientSecret: clientSecret, RedirectURI: oauth2providertest.TestRedirectURI, + CodeVerifier: codeVerifier, } initialToken := oauth2providertest.ExchangeCodeForToken(t, client.URL.String(), tokenParams) diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index fcd031f8e5..8380d307a5 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -254,16 +254,29 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database return codersdk.OAuth2TokenResponse{}, errBadCode } - // Verify PKCE challenge if present - if dbCode.CodeChallenge.Valid && dbCode.CodeChallenge.String != "" { - if req.CodeVerifier == "" { - return codersdk.OAuth2TokenResponse{}, errInvalidPKCE - } - if !VerifyPKCE(dbCode.CodeChallenge.String, req.CodeVerifier) { - return codersdk.OAuth2TokenResponse{}, errInvalidPKCE + // Verify redirect_uri matches the one used during authorization + // (RFC 6749 §4.1.3). + if dbCode.RedirectUri.Valid && dbCode.RedirectUri.String != "" { + if req.RedirectURI != dbCode.RedirectUri.String { + return codersdk.OAuth2TokenResponse{}, errBadCode } } + // PKCE is mandatory for all authorization code flows + // (OAuth 2.1). Verify the code verifier against the stored + // challenge. + if req.CodeVerifier == "" { + return codersdk.OAuth2TokenResponse{}, errInvalidPKCE + } + if !dbCode.CodeChallenge.Valid || dbCode.CodeChallenge.String == "" { + // Code was issued without a challenge — should not happen + // with authorize endpoint enforcement, but defend in depth. + return codersdk.OAuth2TokenResponse{}, errInvalidPKCE + } + if !VerifyPKCE(dbCode.CodeChallenge.String, req.CodeVerifier) { + return codersdk.OAuth2TokenResponse{}, errInvalidPKCE + } + // Verify resource parameter consistency (RFC 8707) if dbCode.ResourceUri.Valid && dbCode.ResourceUri.String != "" { // Resource was specified during authorization - it must match in token request diff --git a/coderd/oauth2provider/tokens_internal_test.go b/coderd/oauth2provider/tokens_internal_test.go index 5a6dcf3d93..7f25b68827 100644 --- a/coderd/oauth2provider/tokens_internal_test.go +++ b/coderd/oauth2provider/tokens_internal_test.go @@ -318,6 +318,7 @@ func TestExtractAuthorizeParams_Scopes(t *testing.T) { query.Set("response_type", "code") query.Set("client_id", "test-client") query.Set("redirect_uri", "http://localhost:3000/callback") + query.Set("code_challenge", "test-challenge") if tc.scopeParam != "" { query.Set("scope", tc.scopeParam) } @@ -341,6 +342,34 @@ func TestExtractAuthorizeParams_Scopes(t *testing.T) { } } +// TestExtractAuthorizeParams_TokenResponseTypeDoesNotRequirePKCE ensures +// response_type=token is parsed without requiring PKCE fields so callers can +// return unsupported_response_type instead of invalid_request. +func TestExtractAuthorizeParams_TokenResponseTypeDoesNotRequirePKCE(t *testing.T) { + t.Parallel() + + callbackURL, err := url.Parse("http://localhost:3000/callback") + require.NoError(t, err) + + query := url.Values{} + query.Set("response_type", string(codersdk.OAuth2ProviderResponseTypeToken)) + query.Set("client_id", "test-client") + query.Set("redirect_uri", "http://localhost:3000/callback") + + reqURL, err := url.Parse("http://localhost:8080/oauth2/authorize?" + query.Encode()) + require.NoError(t, err) + + req := &http.Request{ + Method: http.MethodGet, + URL: reqURL, + } + + params, validationErrs, err := extractAuthorizeParams(req, callbackURL) + require.NoError(t, err) + require.Empty(t, validationErrs) + require.Equal(t, codersdk.OAuth2ProviderResponseTypeToken, params.responseType) +} + // TestRefreshTokenGrant_Scopes tests that scopes can be requested during refresh func TestRefreshTokenGrant_Scopes(t *testing.T) { t.Parallel() diff --git a/codersdk/oauth2.go b/codersdk/oauth2.go index 3d86de2271..3f0db4d75c 100644 --- a/codersdk/oauth2.go +++ b/codersdk/oauth2.go @@ -217,11 +217,7 @@ const ( ) func (e OAuth2ProviderResponseType) Valid() bool { - switch e { - case OAuth2ProviderResponseTypeCode, OAuth2ProviderResponseTypeToken: - return true - } - return false + return e == OAuth2ProviderResponseTypeCode || e == OAuth2ProviderResponseTypeToken } type OAuth2TokenEndpointAuthMethod string diff --git a/docs/admin/integrations/oauth2-provider.md b/docs/admin/integrations/oauth2-provider.md index e85055ec0c..3d1ff10988 100644 --- a/docs/admin/integrations/oauth2-provider.md +++ b/docs/admin/integrations/oauth2-provider.md @@ -128,9 +128,16 @@ If client authentication fails, the token endpoint returns **HTTP 401** with an "$CODER_URL/api/v2/users/me" ``` -### PKCE Flow (Recommended) +> [!NOTE] +> The PKCE flow below is the **required** integration path. The example +> above is shown for reference but omits the mandatory `code_challenge` +> parameter. See [PKCE Flow](#pkce-flow-required) for the complete flow. -Use PKCE for enhanced security (recommended for both public and confidential clients): +### PKCE Flow (Required) + +PKCE is **required** for all OAuth2 authorization code flows. Coder enforces +PKCE in compliance with the OAuth 2.1 specification. Both public and +confidential clients must include PKCE parameters: 1. Generate a code verifier and challenge: @@ -251,7 +258,8 @@ Verify that the `code_verifier` used in the token request matches the one used t ## Security Considerations - **Use HTTPS**: Always use HTTPS in production to protect tokens in transit -- **Implement PKCE**: Use PKCE for all public clients (mobile apps, SPAs) +- **Implement PKCE**: PKCE is mandatory for all authorization code clients + (public and confidential) - **Validate redirect URLs**: Only register trusted redirect URIs for your applications - **Rotate secrets**: Periodically rotate client secrets using the management API @@ -261,11 +269,20 @@ As an experimental feature, the current implementation has limitations: - No scope system - all tokens have full API access - No client credentials grant support +- Implicit grant (`response_type=token`) is not supported; OAuth 2.1 + deprecated this flow due to token leakage risks, and requests return + `unsupported_response_type` - Limited to opaque access tokens (no JWT support) ## Standards Compliance -This implementation follows established OAuth2 standards including [RFC 6749](https://datatracker.ietf.org/doc/html/rfc6749) (OAuth2 core), [RFC 7636](https://datatracker.ietf.org/doc/html/rfc7636) (PKCE), and related specifications for discovery and client registration. +This implementation follows established OAuth2 standards including +[RFC 6749](https://datatracker.ietf.org/doc/html/rfc6749) (OAuth2 core), +[RFC 7636](https://datatracker.ietf.org/doc/html/rfc7636) (PKCE), and the +[OAuth 2.1 draft](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-v2-1-12). +Coder enforces OAuth 2.1 requirements including mandatory PKCE for all +authorization code grants, exact redirect URI string matching, rejection +of the implicit grant, and CSRF protections on consent pages. ## Next Steps diff --git a/site/site.go b/site/site.go index 0ac7d97ee1..bb1e74bc18 100644 --- a/site/site.go +++ b/site/site.go @@ -719,6 +719,7 @@ type RenderOAuthAllowData struct { AppName string CancelURI string RedirectURI string + CSRFToken string Username string } @@ -731,6 +732,11 @@ type RenderOAuthAllowData struct { 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") + err := oauthTemplate.Execute(rw, data) if err != nil { httpapi.Write(r.Context(), rw, http.StatusOK, codersdk.Response{ diff --git a/site/static/oauth2allow.html b/site/static/oauth2allow.html index 313cac30dd..76fb4927c1 100644 --- a/site/static/oauth2allow.html +++ b/site/static/oauth2allow.html @@ -119,6 +119,7 @@ links */}}

+
Cancel