shishantbiswas/coder
1
0
Fork 0
mirror of https://github.com/coder/coder.git synced 2026-06-04 13:38:21 +00:00
Code Issues Packages Projects Releases Wiki Activity
12,434 Commits 1,154 Branches 344 Tags
1a774ab7ce99063a2e01beb94de3fcbccaf84dbe
Commit Graph

4 Commits

Author SHA1 Message Date
Thomas Kosiewski b776a14b46 fix(coderd): harden OAuth2 provider security (#22194) 
## Summary

Harden the OAuth2 provider with multiple security fixes addressing
`coder/security#121` (CSRF session takeover) and converge on OAuth 2.1
compliance.

### Security Fixes

| Fix | Description | Commits |
|-----|-------------|---------|
| **CSRF on `/oauth2/authorize`** | Enforce CSRF protection on the
authorize endpoint POST (consent form submission) | `ba7d646`, `b94a64e`
|
| **Clickjacking: `frame-ancestors` CSP** | Prevent consent page from
being iframed (`Content-Security-Policy: frame-ancestors 'none'` +
`X-Frame-Options: DENY`) | `597aeb2` |
| **Exact redirect URI matching** | Changed from prefix matching to full
string exact matching per OAuth 2.1 §4.1.2.1 | `73d64b1`, `93897f1` |
| **Store & verify `redirect_uri`** | Store redirect_uri with auth code
in DB, verify at token exchange matches exactly (RFC 6749 §4.1.3) |
`50569b9`, `d7ca315` |
| **Mandatory PKCE** | Require `code_challenge` at authorization (for
`response_type=code`) + unconditional `code_verifier` verification at
token exchange | `d7ca315`, `1cda1a9` |
| **Reject implicit grant** | `response_type=token` now returns
`unsupported_response_type` error page (OAuth 2.1 removes implicit flow)
| `d7ca315`, `91b8863` |

### Changes by File

**`coderd/httpmw/csrf.go`** — Extended the CSRF `ExemptFunc` to enforce
CSRF on `/oauth2/authorize` in addition to `/api` routes. The consent
form POST is now CSRF-protected to prevent cross-site authorization code
theft.

**`site/site.go`** — Added `Content-Security-Policy: frame-ancestors
'none'` and `X-Frame-Options: DENY` headers to `RenderOAuthAllowPage`
(consent page only — does not affect the SPA/global CSP used by AI
tasks).

**`coderd/httpapi/queryparams.go`** — Changed `RedirectURL` from prefix
matching (`strings.HasPrefix(v.Path, base.Path)`) to full URI exact
matching (`v.String() != base.String()`), comparing scheme, host, path,
and query.

**`coderd/oauth2provider/authorize.go`** — Added PKCE enforcement:
`code_challenge` is required when `response_type=code` (via a
conditional check, not `RequiredNotEmpty`, so `response_type=token` can
reach the explicit rejection path). `ShowAuthorizePage` (GET) validates
`response_type` before rendering and returns a 400 error page for
unsupported types. `ProcessAuthorize` (POST) stores the `redirect_uri`
with the auth code when explicitly provided.

**`coderd/oauth2provider/tokens.go`** — PKCE verification is now
unconditional (not gated on `code_challenge` being present in DB). If
the stored code has a `redirect_uri`, the token endpoint verifies it
matches exactly — mismatch returns `errBadCode` → `invalid_grant`.
Missing `code_verifier` returns `invalid_grant`.

**`codersdk/oauth2.go`** — `OAuth2ProviderResponseTypeToken` constant
and `Valid()` acceptance are **kept** so the authorize handler can parse
`response_type=token` and return the proper `unsupported_response_type`
error rather than failing at parameter validation.

**`coderd/database/migrations/000421_*`** — Added `redirect_uri text`
column to `oauth2_provider_app_codes`.

### Design Decisions

**`state` parameter remains optional** — The plan initially required
`state` via `RequiredNotEmpty`, but this was reverted in `376a753` to
avoid breaking existing clients. The `state` is still hashed and stored
when provided (via `state_hash` column), securing clients that opt in.

**`response_type=token` kept in `Valid()`** — Removing it from `Valid()`
would cause the parameter parser to reject the request before the
authorize handler can return the proper `unsupported_response_type`
error. The constant is kept for correct error handling flow.

**CSP scoped to consent page only** — `frame-ancestors 'none'` is set
only on the OAuth consent page renderer, not globally. The SPA/global
CSP was previously changed to allow framing for AI tasks
([#18102](https://github.com/coder/coder/pull/18102)); this change does
not regress that.

### Out of Scope (follow-up PRs)

- Bearer tokens in query strings (needs internal caller audit)
- Scope enforcement on OAuth2 tokens
- Rate limiting on dynamic client registration


---

<details>
<summary>📋 Implementation Plan</summary>

# Plan: Harden OAuth2 Provider — Security Fixes + OAuth 2.1 Compliance

## Context & Why

Security issue `coder/security#121` reports a critical session takeover
via CSRF on the OAuth2 provider. This plan covers all remaining security
fixes from that issue **plus** convergence on OAuth 2.1 requirements.
The goal is a single PR that closes all actionable gaps.

## Current State (already committed on branch `csrf-sjx1`)

| Fix | Status | Commits |
|-----|--------|---------|
| Fix 1: CSRF on `/oauth2/authorize` | ✅ Done | `ba7d646`, `b94a64e` |
| CSRF token in consent form HTML | ✅ Done | `b94a64e` |
| `state_hash` column + storage | ✅ Done (hash stored, but state still
optional) | `9167d83`, `b94a64e` |
| Tests for CSRF + state hash | ✅ Done | `e4119b5` |

## Remaining Work

### ~~Fix 2 — Require `state` parameter~~ (DROPPED)

> **Decision:** Do not enforce `state` as required. The `state`
parameter is still hashed and stored when provided (via
`hashOAuth2State` / `state_hash` column from prior commits), but clients
are not forced to supply it. This avoids breaking existing integrations
that omit state.

**Rollback:** Remove `"state"` from the `RequiredNotEmpty` call in
`coderd/oauth2provider/authorize.go:42`:

```go
// BEFORE (current on branch)
p.RequiredNotEmpty("response_type", "client_id", "state", "code_challenge")

// AFTER
p.RequiredNotEmpty("response_type", "client_id", "code_challenge")
```

No test changes needed — tests already pass `state` voluntarily.

### Fix 4 — Exact redirect URI matching

Currently `coderd/httpapi/queryparams.go:233` uses prefix matching:

```go
// CURRENT — prefix match
if v.Host != base.Host || !strings.HasPrefix(v.Path, base.Path) {
```

OAuth 2.1 requires **exact string matching**. Change to:

```go
// AFTER — exact match (OAuth 2.1 §4.1.2.1)
if v.Host != base.Host || v.Path != base.Path {
```

**File: `coderd/httpapi/queryparams.go` — `RedirectURL` method**

Also update the error message from "must be a subset of" to "must
exactly match".

**Additionally**, store `redirect_uri` with the auth code and verify at
the token endpoint (RFC 6749 §4.1.3):

1. **New migration** (same migration file or a new `000421`): Add
`redirect_uri text` column to `oauth2_provider_app_codes`
2. **Update INSERT query** in `coderd/database/queries/oauth2.sql` to
include `redirect_uri`
3. **`coderd/oauth2provider/authorize.go`**: Store
`params.redirectURL.String()` when inserting the code
4. **`coderd/oauth2provider/tokens.go`**: After retrieving the code from
DB, verify that `redirect_uri` from the token request matches the stored
value exactly. Currently `tokens.go:103` calls `p.RedirectURL(vals,
callbackURL, "redirect_uri")` for prefix validation only — it must
compare against the stored redirect_uri from the code, not just the
app's callback URL.

<details>
<summary>Why both exact match AND store+verify?</summary>

Exact matching at the authorize endpoint prevents open redirectors
(attacker can't use a sub-path).
Storing and verifying at the token endpoint prevents code injection — an
attacker who steals a code can't exchange it with a different
redirect_uri than was originally authorized. This is required by RFC
6749 §4.1.3 and OAuth 2.1.
</details>

### Fix 7 — `frame-ancestors` CSP on consent page

The consent page can be iframed by a workspace app (same-site), which is
the attack vector. Add a `Content-Security-Policy` header to prevent
framing.

**File: `site/site.go` — `RenderOAuthAllowPage` function (~line 731)**

Before writing the response, add:

```go
func RenderOAuthAllowPage(rw http.ResponseWriter, r *http.Request, data RenderOAuthAllowData) {
    rw.Header().Set("Content-Type", "text/html; charset=utf-8")
    // Prevent the consent page from being framed to mitigate
    // clickjacking attacks (coder/security#121).
    rw.Header().Set("Content-Security-Policy", "frame-ancestors 'none'")
    rw.Header().Set("X-Frame-Options", "DENY")
    ...
```

Both headers for defense-in-depth (CSP for modern browsers,
X-Frame-Options for legacy).

### OAuth 2.1 — Mandatory PKCE

Currently PKCE is checked only when `code_challenge` was provided during
authorization (`tokens.go:258`):

```go
// CURRENT — conditional check
if dbCode.CodeChallenge.Valid && dbCode.CodeChallenge.String != "" {
    // verify PKCE
}
```

OAuth 2.1 requires PKCE for ALL authorization code flows. Change to:

**File: `coderd/oauth2provider/authorize.go`** — Add `"code_challenge"`
to required params:

```go
p.RequiredNotEmpty("response_type", "client_id", "code_challenge")
```

**File: `coderd/oauth2provider/tokens.go:257-265`** — Make PKCE
verification unconditional:

```go
// AFTER — PKCE always required (OAuth 2.1)
if req.CodeVerifier == "" {
    return codersdk.OAuth2TokenResponse{}, errInvalidPKCE
}
if !dbCode.CodeChallenge.Valid || dbCode.CodeChallenge.String == "" {
    // Code was issued without a challenge — should not happen
    // with the authorize endpoint enforcement, but defend in
    // depth.
    return codersdk.OAuth2TokenResponse{}, errInvalidPKCE
}
if !VerifyPKCE(dbCode.CodeChallenge.String, req.CodeVerifier) {
    return codersdk.OAuth2TokenResponse{}, errInvalidPKCE
}
```

**File: `codersdk/oauth2.go`** — Remove
`OAuth2ProviderResponseTypeToken` from the enum or reject it explicitly
in the authorize handler. Currently it's defined at line 216 but the
handler ignores `response_type` and always issues a code. We should
either:
- (a) Remove the `"token"` variant from the enum and reject it with
`unsupported_response_type`, OR
- (b) Add an explicit check in `ProcessAuthorize` that rejects
`response_type=token`

Option (b) is simpler and more backwards-compatible:

```go
// In ProcessAuthorize, after extracting params:
if params.responseType != codersdk.OAuth2ProviderResponseTypeCode {
    httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest,
        codersdk.OAuth2ErrorCodeUnsupportedResponseType,
        "Only response_type=code is supported")
    return
}
```

### OAuth 2.1 — Bearer tokens in query strings

`coderd/httpmw/apikey.go:743` accepts `access_token` from URL query
parameters. OAuth 2.1 prohibits this. However, this may be used
internally (e.g., workspace apps, DERP). Need to audit callers before
removing.

**Approach:** This is a larger change with potential breakage. Mark as a
**separate follow-up issue** rather than including in this PR. Document
the finding.

### OAuth 2.1 — Removed flows

✅ **Already compliant.** `tokens.go` only supports `authorization_code`
and `refresh_token` grant types. The implicit grant
(`response_type=token`) will be explicitly rejected per the PKCE section
above.

### OAuth 2.1 — Refresh token rotation

✅ **Already compliant.** `tokens.go:442` deletes the old API key when a
refresh token is used.

## Migration Plan

All DB changes can go in a single new migration (or extend 000420 if the
branch is rebased before merge). Columns to add:
- `redirect_uri text` on `oauth2_provider_app_codes`

The `state_hash` column is already added by migration 000420.

## Implementation Order

1. **Fix 7** — CSP headers on consent page (isolated, no deps)
2. ~~**Fix 2** — Require `state` parameter~~ (DROPPED — state stays
optional)
3. **Fix 4** — Exact redirect URI matching + store/verify redirect_uri
4. **PKCE mandatory** — Require `code_challenge` + reject
`response_type=token`
5. **Rollback** — Remove `"state"` from `RequiredNotEmpty` in
`authorize.go`
6. **Tests** — Update/add tests for all changes
7. **`make gen`** after DB changes

## Out of Scope (separate PRs)

- Bearer tokens in query strings (needs internal caller audit)
- Scope enforcement on OAuth2 tokens
- Rate limiting / quota on dynamic client registration

</details>

---
_Generated with [`mux`](https://github.com/coder/mux) • Model:
`anthropic:claude-opus-4-6` • Thinking: `xhigh`_
 2026-02-23 12:18:44 +01:00
Thomas Kosiewski dd6aec04d7 fix(coderd/oauth2provider): support client_secret_basic client auth (#21793) 2026-02-02 16:01:33 +01:00
Ehab Younes 6683d807ac refactor: add RFC-compliant enum types and use SDK as source of truth (#21468) 
Add comprehensive OAuth2 enum types to codersdk following RFC specifications:
- OAuth2ProviderGrantType (RFC 6749)
- OAuth2ProviderResponseType (RFC 6749)
- OAuth2TokenEndpointAuthMethod (RFC 7591)
- OAuth2PKCECodeChallengeMethod (RFC 7636)
- OAuth2TokenType (RFC 6749, RFC 9449)
- OAuth2RevocationTokenTypeHint (RFC 7009)
- OAuth2ErrorCode (RFC 6749, RFC 7009, RFC 8707)

Add OAuth2TokenRequest, OAuth2TokenResponse, OAuth2TokenRevocationRequest,
and OAuth2Error structs to the SDK. Update OAuth2ClientRegistrationRequest,
OAuth2ClientRegistrationResponse, OAuth2ClientConfiguration, and
OAuth2AuthorizationServerMetadata to use typed enums instead of raw strings.

This makes codersdk the single source of truth for OAuth2 types, eliminating
duplication between SDK and server-side structs.

Closes #21476
 2026-01-15 12:41:28 +03:00
Thomas Kosiewski c65013384a refactor: move OAuth2 provider code to dedicated package (#18746) 
# Refactor OAuth2 Provider Code into Dedicated Package

This PR refactors the OAuth2 provider functionality by moving it from the main `coderd` package into a dedicated `oauth2provider` package. The change improves code organization and maintainability without changing functionality.

Key changes:

- Created a new `oauth2provider` package to house all OAuth2 provider-related code
- Moved existing OAuth2 provider functionality from `coderd/identityprovider` to the new package
- Refactored handler functions to follow a consistent pattern of returning `http.HandlerFunc` instead of being handlers directly
- Split large files into smaller, more focused files organized by functionality:
  - `app_secrets.go` - Manages OAuth2 application secrets
  - `apps.go` - Handles OAuth2 application CRUD operations
  - `authorize.go` - Implements the authorization flow
  - `metadata.go` - Provides OAuth2 metadata endpoints
  - `registration.go` - Handles dynamic client registration
  - `revoke.go` - Implements token revocation
  - `secrets.go` - Manages secret generation and validation
  - `tokens.go` - Handles token issuance and validation

This refactoring improves code organization and makes the OAuth2 provider functionality more maintainable while preserving all existing behavior.
 2025-07-03 20:24:45 +02:00