mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
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`_
This commit is contained in:
@@ -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
|
||||
|
||||
Generated
+7
-1
@@ -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,
|
||||
|
||||
@@ -0,0 +1,3 @@
|
||||
ALTER TABLE oauth2_provider_app_codes
|
||||
DROP COLUMN state_hash,
|
||||
DROP COLUMN redirect_uri;
|
||||
@@ -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).';
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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),
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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{})
|
||||
|
||||
+40
-22
@@ -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",
|
||||
|
||||
+93
-42
@@ -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
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
})
|
||||
}
|
||||
@@ -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+`"`)
|
||||
}
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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()
|
||||
|
||||
+1
-5
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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{
|
||||
|
||||
@@ -119,6 +119,7 @@ links */}}
|
||||
</p>
|
||||
<div class="button-group">
|
||||
<form method="POST" style="display: inline;">
|
||||
<input type="hidden" name="csrf_token" value="{{ .CSRFToken }}" />
|
||||
<button type="submit" class="primary-button">Allow</button>
|
||||
</form>
|
||||
<a href="{{ .CancelURI }}">Cancel</a>
|
||||
|
||||
Reference in New Issue
Block a user