Slice 3 of 4 for runtime experiment rules: evaluate stored rules per
user in discovery, the frontend experiments query, and the pilot
`mcp-tool-search` gate in chatd. There is still no way to edit rules
through the product; that comes in slice 4.
With no rules stored and a healthy database, backend decisions match
today. This PR deliberately changes three things:
1. **Fail closed on rule reads.** If the rules read fails,
`mcp-tool-search` is off, even when it is enabled at startup.
2. **Frontend freshness.** The experiments query now refetches.
3. **Caching.** `/api/v2/experiments` and rendered HTML are sent with
`Cache-Control: no-store`. Static assets keep their long-lived caching.
## What's in it
- **coderd** builds `api.ExperimentEvaluator` from
`experiments.NewDBStore` plus the startup `--experiments` list.
- **Discovery:** `GET /api/v2/experiments` and the `<meta
property="experiments">` tag both use `EnabledExperiments(ctx, caller)`.
- **chatd:** `mcp-tool-search` is now user-scoped.
`chatd.Config.ExperimentEvaluator` is required, so `chatd.New` returns
an error without it. `prepareGeneration` decides once per turn for the
chat owner (`chat.OwnerID`); child chats inherit the owner.
- **Ruleguard** (`userScopedExperimentStaticCheck` in
`scripts/rules.go`): rejects static `Experiments.Enabled(...)` and
`httpmw.RequireExperiment*(...)` checks of user-scoped experiments in
server packages. A codersdk test keeps its list equal to
`codersdk.ExperimentsUserScoped`.
- **Frontend:** the experiments query is keyed by user ID and seeded
from the meta tag only when the embedded user matches. It is stale 60 s
after page load and refetches on mount, window focus and reconnect.
Logout also clears the experiments metadata. The dashboard keeps the
last loaded list when a background refetch fails, instead of replacing
the page with an error.
- **Once per turn:** chatd decides `mcp-tool-search` on a turn's first
step and reuses that decision for the turn's later steps, so a rule
change applies from the next turn and never withdraws a `find_tools`
call the model already made. The decision lives on the chat runner,
keyed by the turn's prompt row, like the stop-nudge tracker. A runner
handoff in the middle of a turn evaluates again.
- **Reference audit:** the only non-test server check of
`mcp-tool-search` is in `generation_preparer.go`, and it now goes
through the evaluator.
## Testing
- **Go:** per-user results and `no-store` on `/api/v2/experiments`; the
per-user meta tag and the HTML cache headers in `site`; group and org
conditions end to end in enterprise; chatd owner eligibility, on → off →
on across turns including a child chat, and a failed rules read.
- **Vitest:** user-keyed seeding, staleness from page load after a
remount, refetch on focus, logout clearing, and the dashboard surviving
a failed refetch.
Remote dogfood UAT through Coder Agents passed on a licensed test
deployment:
- per-user discovery in both the API and the meta tag;
- widening a rule, and turning it off, with no restart;
- `off` beating a startup default, and `inherit` restoring it;
- fail closed on malformed JSON and on an unknown mode, with no
condition text in the logs;
- the `no-store` headers;
- real-model chatd enforcement: `find_tools` offered on turn 1, gone
after `off`, back on turn 3.
That round tested `b57207d4ee`. The later commits change only tests, and
a follow-up round will cover the final head.
**For the author:** `coderd/x/chatd/ARCHITECTURE.md`'s list of the
runner's local state doesn't mention the per-turn trackers (debug turn,
session start, stop nudges, and now this experiment decision). I did not
edit that document.
**Known behavior:** a broken rule logs one error line per evaluation
(per request or turn), with only the experiment, revision, category and
position.
## Stack
1. #29869: evaluator library
2. #29898: storage and audit types
3. **This PR:** enforcement and discovery
4. Control plane (experimental API, `coder exp experiment-rules` CLI,
docs)
> [!NOTE]
> This PR was written by an AI agent (Xum).
---
_Generated with [`xum`](https://github.com/coder/xum) • Model:
`anthropic:claude-opus-5-5` • Thinking: `high`_
---------
Signed-off-by: Thomas Kosiewski <tk@coder.com>
When `DisableUserSecretFilePath` is set, coderd strips file targets
while building the agent manifest: env-only secrets are unchanged,
dual-target secrets keep the env var with an empty file path, and
file-only secrets are omitted so their plaintext never crosses DRPC.
Stored paths stay in the database and reappear on the next manifest
fetch once the option is cleared. Agents converge on reconnect; no
protobuf, provisioner, or agent-side changes are needed, and files
already written to workspace disks are left alone.
Also exposes the positive capability as the
`user-secret-file-path-enabled` embedded metadata tag, which the next PR
in the stack consumes in the dashboard. Builds on #28518. Refs #27488.
Reviewed and updated by Coder Agents on behalf of @dylanhuff-at-coder.
Adds a `codernauts_enabled` runtime setting (default: enabled) that
controls whether the Codernauts game link appears in the user dropdown
menu. A toggle at the bottom of the Deployment > Appearance page
controls it, and the value persists in `site_configs`, so it survives
restarts and upgrades without any CLI flags or environment variables.
The setting works on all licenses: `GET /api/v2/appearance` reports it
from both the enterprise and the default (AGPL) appearance fetchers, and
`PUT /api/v2/appearance` persists it gated only by the deployment-config
RBAC permission, not by an entitlement. On the Appearance page the
toggle renders outside the Premium paywall introduced in #27948; the
other appearance fields keep their existing licensing behavior.
Note: a pure AGPL-only build of coderd does not register the
`/api/v2/appearance` routes at all, so the toggle cannot be saved there.
The standard `coder server` binary is unaffected regardless of license
state.
<details>
<summary>Implementation notes</summary>
- `coderd/database/queries/siteconfig.sql`: new `GetCodernautsEnabled` /
`UpsertCodernautsEnabled` queries backed by a `codernauts_enabled` key
in `site_configs`; defaults to `true` when unset (no migration needed).
- `coderd/database/dbauthz`: read has no authz checks (matching other
appearance reads); write requires `ResourceDeploymentConfig` update.
Coverage added to `dbauthz_test.go`.
- `codersdk`: `codernauts_enabled` added to `AppearanceConfig` and
`UpdateAppearanceConfig`.
- `coderd/appearance`: the default fetcher takes a `database.Store` and
reads the setting from the database so unlicensed deployments serve it
too.
- `enterprise/coderd/appearance.go`: fetches the value in `Fetch` and
persists it in `putAppearance`.
- Frontend: "Codernauts game" toggle section on Deployment > Appearance
saving immediately via the existing appearance mutation;
`codernauts_enabled` threaded from `useDashboard().appearance` through
Navbar to `UserDropdownContent`, which renders the menu item only when
enabled. Storybook stories with `play` functions cover toggling
(entitled and not) and the dropdown hiding the link when disabled.
</details>
---
🤖 This PR was generated by Coder Agents on behalf of @bartekgatzcoder.
**TL;DR**
Last of the stack, split out of #28045. #28178 decided what the scope
is; this PR is everything that reports it, to the user on the consent
page and to the client on rejection.
| PR | What it does |
|---|---|
| #28007 | Schema: `codes.scope` and `tokens.scope`, so a negotiated
scope has somewhere to live. Every code is still stamped unrestricted. |
| #28167 | `ScopesCover` compares an allowlist against a request by
permission coverage rather than by name, and fails closed on any shape
it does not model. Added with no production callers. |
| #28178 | The authorize endpoint checks the request against the scope
catalog, grants it only if the app's filtered allowlist covers it, and
persists the result on the code. |
| **#28179 (this)** | The consent page lists the negotiated permissions,
and `invalid_scope` reaches the client's own callback per RFC 6749
§4.1.2.1 instead of a Coder error page. Retires the interim plumbing
#28178 left behind. |
- #28178 has merged, so this diff is against main and reviews on its
own. It is still worth reading first: nothing here decides a scope, it
only reports the one `negotiateScope` already returns.
- **The consent page HTML is not final.** Layout and wording in
`site/static/oauth2allow.html` are still being refined. It is in this
branch so the scope-enforcement flow can be exercised end to end in a
browser, not because the markup is settled. The reviewable part of Half
A is the Go side that decides what the page states; the presentation
will change.
- An unrestricted grant still reads as full access, not as a scope name.
Nothing about what a token can do changes in this PR either.
**Where in the flow**
- Authorize endpoint only, both verbs. GET decides what the consent page
states; both verbs send `invalid_scope` to the callback. The token
exchange, refresh, and revocation are untouched.
- `negotiateScope` is unchanged from #28178, so which requests succeed
is unchanged. This PR reads its result twice: once to decide what the
page lists, once to decide where the rejection goes.
- The endpoint sits behind `apiKeyMiddlewareRedirect`, so the user is
authenticated before any of this runs. That matters for the redirect,
not just for the page.
- Consent page rendering moves a decision into Go rather than the
template: `RenderOAuthAllowData` carries `Scopes` and `Unrestricted`
separately, and the template branches on `Unrestricted` alone.
**What it satisfies**
- RFC 6749 §4.1.2.1 — on a failure that is not a bad redirection URI,
the server "informs the client by adding the following parameters to the
query component of the redirection URI". `invalid_scope` now goes there.
#28178's interim delivery, a static error page on GET and an error body
on POST, is what this replaces.
- RFC 6749 §4.1.2.1 — `state` is "REQUIRED if a `state` parameter was
present in the client authorization request", so it is echoed only when
the client sent one, never synthesized. Pinned by
`OmittedStateNotEchoed`.
- RFC 6749 §3.1.2.4 and §4.1.2.1 — on a "missing, invalid, or
mismatching redirection URI" the server "MUST NOT automatically redirect
the user-agent to the invalid redirection URI". This is the whole
ordering argument, and the file's other error paths keep their existing
delivery precisely because several of them are where that validation
fails.
- RFC 9700 §2.1 — pre-registered redirection URIs "MUST utilize exact
string matching". `extractAuthorizeParams` already did this before the
stack; Half B depends on it rather than adding it.
- RFC 9700 §4.11.2 — names this exact hazard: an attacker who registers
a client can "intentionally send an erroneous authorization request,
e.g., by using an invalid scope value, thus instructing the
authorization server to redirect the user agent to its phishing site".
The destination is still only ever a URI that client registered, so this
is a phishing surface that costs an attacker a registration, not an open
redirect. The precaution the section requires, authenticating the user
first, is what `apiKeyMiddlewareRedirect` provides.
- RFC 9700 §4.12 — a server redirecting a request that may carry
credentials "MUST NOT use the HTTP 307 status code". 302, matching the
success redirect beside it.
- RFC 6749 §4.1.2.1 — `server_error` exists "because a 500 Internal
Server Error HTTP status code cannot be returned to the client via an
HTTP redirect". An unusable registered callback is the one case with
nowhere to redirect, so it answers 500 on both verbs and logs the app
and URL for an operator to correlate by.
- Not yet: §1.4.1 still wants `scope` in the token response when the
grant differs from the request. Tokens are untouched here, as in #28178,
so that lands with enforcement.
---
Split out of #28045
([PLAT-479](https://linear.app/codercom/issue/PLAT-479)). Last of three.
#28178 has merged, so this reviews against main.
#28178 decided what the scope is. This PR is everything that tells
someone about it.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
This PR removes the now-dead direct-routing code:
- Deletes the direct routing implementation.
- Collapses the resolvedModelRoute discriminated union into aiGatewayModelRoute.
- Removes the dead providerKeys cascade.
- Deletes the preferredShortTextCandidates quickgen function.
- Simplifies the advisor override error handling.
- Deprecates the AIGatewayRoutingEnabled deployment option. It is now a no-op so as to not break existing deployments on upgrade.
Once direct routing was gone, the AI Gateway became mandatory for chat, which surfaced gaps in how the product behaves with the gateway disabled:
- Exposes ai-gateway-enabled to the frontend via embedded page metadata.
- Disables the chat composer via the existing AgentSetupNotice when the gateway is disabled, for both new and existing chats.
- Fixes nil/typed-nil chatDaemon panics on startup and shutdown when gateway is disabled.
- Fixes chat WebSocket from retrying the still-gated stream endpoint forever when the gateway is disabled.
Part 1: Backend portion of a change broken into 2 PRs.
Part 2: #25077
Adds three new UserAppearanceSettings fields (theme_mode, theme_light,
theme_dark) on top of the existing theme_preference and terminal_font.
Replaces GetUserThemePreference and GetUserTerminalFont with a single
GetUserAppearanceSettings aggregate query. The PUT handler is wrapped in
db.InTx so sync-mode's mode + slot writes can never half-apply.
Remove the `ExperimentAgents` feature flag so the Agents feature is
always available without requiring `--experiments=agents`. The feature
is now in beta.
Existing deployments that still pass `--experiments=agents` will get a
harmless "ignoring unknown experiment" warning on startup.
### Changes
**Backend:**
- Remove `RequireExperimentWithDevBypass` middleware from chat and MCP
server routes
- Always include `AgentsAccessRole` in assignable site roles (later
refactored to org-scoped on main; rebase keeps that)
- Always set `AgentsTabVisible = true`, then drop the entire dead
`AgentsTabVisible` metadata pipeline (Go htmlState field,
populateHTMLState goroutine, HTML meta tag, useEmbeddedMetadata
registration, mock); no production consumer reads it. `AgentsNavItem`
already gates on `permissions.createChat`.
- Make `blob:` CSP `img-src` addition unconditional
- Remove `ExperimentAgents` constant, `DisplayName` case, and
`ExperimentsKnown` entry
**CLI:**
- Graduate the agents TUI from `coder exp agents` to `coder agents`
(moved from `AGPLExperimental()` to `CoreSubcommands()`)
- Drop the `agent` alias so it does not collide with the hidden
workspace-agent command
- Rename implementation files `cli/exp_agents_*.go` -> `cli/agents_*.go`
and internal identifiers (`expChatsTUIModel` -> `chatsTUIModel`,
`newExpChatsTUIModel` -> `newChatsTUIModel`, `setupExpAgentsBackend` ->
`setupAgentsBackend`, `startExpAgentsSession` -> `startAgentsSession`,
`expAgentsPtr` -> `agentsPtr`, `expAgentsSession` -> `agentsSession`,
`TestExpAgents*` -> `TestAgents*`). `expClient` (the
`*codersdk.ExperimentalClient` local) is kept; `coderd/exp_chats*.go`
and other still-experimental `cli/exp_*.go` commands are intentionally
untouched.
**Frontend:**
- Remove experiment check from `AgentsNavItem` - render when
`canCreateChat` is true
- Remove `agentsEnabled` experiment check from `WorkspacesPage`, then
gate `chatsByWorkspace` on `permissions.createChat` so users without
chat access don't trigger the per-page DB query (Copilot review
feedback)
- Add `FeatureStageBadge` (beta) next to the Coder logo in the Agents
sidebar (desktop + mobile)
**Docs:**
- Remove experiment flag setup instructions from `early-access.md` and
`getting-started.md` (and rename `early-access.md`'s "Enable Coder
Agents" heading to "Set up Coder Agents", since there is no enablement
step left)
- Update `chats-api.md` and `getting-started.md`'s Chats API note to say
"beta" instead of "experimental"
- `docs/manifest.json`: drop "experimental" from the Chats API sidebar
description
- `make gen` regenerated `docs/reference/cli/agents.md` and the CLI
index
- `scripts/check_emdash.sh`: exclude `cli/testdata/*.golden` and
`enterprise/cli/testdata/*.golden` from the new repo-wide emdash lint,
since serpent emits emdash borders in every generated `--help` golden
file
**Tests:**
- Remove `ExperimentAgents` setup from all test files (14 occurrences
across 7 files)
- Update stale "with the agents experiment" comments in
`coderd/x/chatd/integration_test.go` and `coderd/mcp_test.go`
<img width="1185" height="900" alt="image"
src="https://github.com/user-attachments/assets/b420bc8f-41d6-42c6-abd8-ad572533d651"
/>
> 🤖 Generated by Coder Agents
Currently when a user clicks either the Cancel or Allow button on the
authorization page the client app URI is executed but the page does not
land to the main dashboard page, leaving the two buttons open for
multiple clicks from the user. Aside from the potential problems it
might cause by activating the callback URI multiple times, the page also
provides poor UX because users usually expect the authorization tab to
return to the dashboard.
The consent page now executes the OAuth2 callback (auth code on Allow,
`access_denied` on Cancel) and hides the two buttons and updates the
existing description with a user instruction to close the window.
Initial implementation relied on a pop-up window executing the callback
while the main window was redirected to the dashboard main page.
- resolves https://github.com/coder/coder/issues/20323
<!--
If you have used AI to produce some or all of this PR, please ensure you
have read our [AI Contribution
guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING)
before submitting.
-->
Go's html/template has a built-in security filter (urlFilter) that only
allows http, https, and mailto URL schemes. Any other scheme gets
replaced with #ZgotmplZ.
The OAuth2 app's callback URL uses custom URI scheme which the filter
considers unsafe. For example the Coder JetBrains plugin exposes a
callback URI with the scheme jetbrains:// - which was effectively
changed by the template engine into #ZgotmplZ. Of course this is not an
actual callback. When users clicked the cancel button nothing happened.
The fix was simple - we now wrap the apps registered callback URI into
htmltemplate.URL. Usually this needs some validation otherwise the
linter will complain about it. The callback URI used by the Cancel logic
is actually validated by our backend when the client app
programmatically registered via the dynamic OAuth2 registration
endpoints, so we refactored the validation around that code and re-used
some of it in the Cancel handling to make sure we don't allow URIs like
`javascript` and `data`, even though in theory these URIs were already
validated.
In addition, while testing this PR with
https://github.com/coder/coder-jetbrains-toolbox/pull/209 I discovered
that we are also not compliant with
https://www.rfc-editor.org/rfc/rfc6749#section-4.1.2.1 which requires
the server to attach the local state if it was provided by the client in
the original request. Also it is optional but generally a good practice
to include `error_description` in the error responses. In fact we follow
this pattern for the other types of error responses. So this is not a
one off.
- resolves#20323
<img width="1485" height="771" alt="Cancel_page_with_invalid_uri"
src="https://github.com/user-attachments/assets/5539d234-9ce3-4dda-b421-d023fc9aa99e"
/>
<img width="486" height="746" alt="Coder Toolbox handling the Cancel
button"
src="https://github.com/user-attachments/assets/acab71a6-d29c-4fa9-80ba-3c0095bbdc8f"
/>
<!--
If you have used AI to produce some or all of this PR, please ensure you
have read our [AI Contribution
guidelines](https://coder.com/docs/about/contributing/AI_CONTRIBUTING)
before submitting.
-->
Replaces the generic red `ErrorAlert` ("Forbidden.") with a proactive
permission check and friendly info alert when a user lacks the
`agents-access` role.
- Add `createChat` permission check to `permissions.json` using
`owner_id: "me"`
- Handle `"me"` owner substitution in `renderPermissions` (SSR path)
- Pass `canCreateChat` from `useAuthenticated().permissions` into
`AgentCreateForm`
- Show `ChatAccessDeniedAlert` and disable input immediately (no need to
trigger a 403 first)
- Also catch 403 errors as a fallback in case permissions aren't yet
loaded
- Add `ForbiddenNoAgentsRole` Storybook story with `play` assertions
- Add `TestRenderPermissionsResolvesMe` Go test to pin the `"me"`
sentinel substitution
<details><summary>Implementation plan & decision log</summary>
- Uses the existing `permissions.json` + `checkAuthorization` system
rather than a separate API call
- `owner_id: "me"` is resolved to the actor's ID by both the auth-check
API endpoint and the SSR `renderPermissions` function
- Go test uses a real `rbac.StrictCachingAuthorizer` (not a mock) so it
verifies both the sentinel substitution and the RBAC role evaluation
end-to-end
- Alert follows the exact same `Alert` pattern as the 409 usage-limit
block
- Uses `severity="info"` and links to the getting-started docs Step 3
- Textarea is disabled proactively so the user never sees the scary
generic error
</details>
> 🤖 Created by a Coder Agent and will be reviewed by a human.
## Problem
The `/agents` page has a voice input feature that uses the Web Speech
API (`webkitSpeechRecognition`), but clicking the mic button always
results in a `"not-allowed"` error — even when the browser has
microphone permission granted.
## Root Cause
The `secureHeaders()` function in `site/site.go` sets a
`Permissions-Policy` header with `microphone=()`, which completely
disables microphone access for the page at the HTTP level. This
overrides any browser-level mic permission grants and causes the Web
Speech API to immediately fire an `onerror` event with `error:
"not-allowed"`.
## Fix
Change `microphone=()` to `microphone=(self)`, which:
- Allows the Coder origin itself to use the microphone (enabling the Web
Speech API voice input)
- Still blocks cross-origin iframes from accessing the microphone
This is the minimal permission change needed — `(self)` is more
restrictive than removing the policy entirely, maintaining the security
intent of the original header.
## Testing
1. Navigate to `/agents`
2. Click the mic button in the chat input
3. Verify voice input works (browser will prompt for mic permission if
not already granted)
4. Verify `Permissions-Policy` response header now shows
`microphone=(self)` instead of `microphone=()`
---------
Co-authored-by: blink-so[bot] <211532188+blink-so[bot]@users.noreply.github.com>
## Problem
Two network requests were blocking the initial page render with
fullscreen `<Loader fullscreen />` spinners:
1. **`POST /api/v2/authcheck`** (permissions) — blocked in `RequireAuth`
via `AuthProvider.isLoading`
2. **`GET /api/v2/organizations`** — blocked in `DashboardProvider`
All other bootstrap queries (`user`, `entitlements`, `appearance`,
`experiments`, `build-info`, `regions`) already used server-side
metadata injection via `index.html` meta tags and resolved instantly.
These two did not.
## Solution
Follow the existing `cachedQuery` + `<meta>` tag pattern to inject both
datasets server-side:
### Server-side (`site/site.go`)
- Add `Permissions` and `Organizations` fields to `htmlState`
- Fetch organizations via `GetOrganizationsByUserID` in parallel with
existing queries
- Evaluate all `permissionChecks` using the RBAC authorizer directly
- Inject results as HTML-escaped JSON into `<meta>` tags
### Frontend
- Register `permissions` and `organizations` in `useEmbeddedMetadata`
- Update `checkAuthorization()` to accept optional metadata and use
`disabledRefetchOptions` when available
- Update `organizations()` to accept optional metadata and use
`cachedQuery` when available
- Wire metadata through `AuthProvider` and `DashboardProvider`
### Note
The Go `permissionChecks` map in `site/site.go` mirrors
`site/src/modules/permissions/index.ts` and must be kept in sync.
## 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`_
relates to: https://github.com/coder/internal/issues/1300
Refactors the options to the site handler to take the cache directory, rather than expecting the caller to call `ExtractOrReadBinFS` and pass the results.
This is important in this stack because we need direct access to the cache directory for compressed file caching.
relates to: https://github.com/coder/internal/issues/1300
Refactors the bin handler to be a `struct` instead of a handlerfunc. The reason we want this is because we are going to introduce a cache of compressed files, so we need somewhere to put this cache.
relates to: https://github.com/coder/internal/issues/1300
Refactors the site binary handler routines to their own file. The `site.go` was getting pretty long and I want to do some refactoring on how the binary handler works.
This PR is literally just moving code from file to file; at the package level nothing is changed.
Fixes all our Go file imports to match the preferred spec that we've _mostly_ been using. For example:
```
import (
"context"
"time"
"github.com/prometheus/client_golang/prometheus"
"golang.org/x/xerrors"
"gopkg.in/natefinch/lumberjack.v2"
"cdr.dev/slog/v3"
"github.com/coder/coder/v2/codersdk/agentsdk"
"github.com/coder/serpent"
)
```
3 groups: standard library, 3rd partly libs, Coder libs.
This PR makes the change across the codebase. The PR in the stack above modifies our formatting to maintain this state of affairs, and is a separate PR so it's possible to review that one in detail.
Upgrades to slog v3 which includes a small, but backward incompatible API change to the acceptible call arguments when logging. This change allows us to verify via compile time type checking that arguments are correct and won't cause a panic, as was possible in slog v1, which this replaces (v2 was tagged but never used in coder/coder).
It also updates dependencies that also use slog and were updated.
I've left the `aibridge` dependency as a commit SHA, under the assumption that the team there (cc @pawbana @dannykopping ) will tag and update the dependency soon and on their own schedule.
Other dependencies, I pushed new tags.
Closes#19984
As part of this, I refactored the error template to take in a slice of
actions rather than using individual booleans and strings to control the
behavior.
We decided a link resolves the issue for now so that is what I added,
although we may want to consider a way to start the workspace and follow
the logs dynamically on that page and then show the app when finished
(similar to the tasks page), or at least make the link automatically
start the workspace instead of only taking you to the dashboard where
you have to then start the workspace.
This will be used in the extensions and desktop apps to enable
compression AND progress reporting for the download by comparing the
original content length to the amount of bytes written to disk.
Closes#16340
The experimental functions in `golang.org/x/exp/slices` are now
available in the standard library since Go 1.21.
Reference: https://go.dev/doc/go1.21#slices
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
Underscores and double hyphens are now blocked. The regex is almost the
exact same as the `coder_app` `slug` regex, but uppercase characters are
still permitted.
Addresses https://github.com/coder/nexus/issues/175.
## Changes
- Adds the `telemetry_items` database table. It's a key value store for
telemetry events that don't fit any other database tables.
- Adds a telemetry report when HTML is served for the first time in
`site.go`.
Before, if a file was not found we would serve the app.
This would cause either the login page or the workspace
page to load (and consequently error because `assets` is
likely not a valid user).
* chore: refactor entitlements to be passable as an argument
Previously, all usage of entitlements requires mutex usage on the
api struct directly. This prevents passing the entitlements to
a sub package. It also creates the possibility for misuse.
* chore: merge authorization contexts
Instead of 2 auth contexts from apikey and dbauthz, merge them to
just use dbauthz. It is annoying to have two.
* fixup authorization reference
* feat: disable directory listings for static files
Static file server handles serving static asset files (js, css, etc).
The default file server would also list all files in a directory.
This has been changed to only serve files.
The new Agent API needs an interface for ServiceBanners, so this PR creates it and refactors the AGPL and Enterprise code to achieve it.
Before we depended on the fact that the HTTP endpoint was missing to serve an empty ServiceBanner on AGPL deployments, but that won't work with dRPC, so we need a real interface to call.
* chore: add /v2 to import module path
go mod requires semantic versioning with versions greater than 1.x
This was a mechanical update by running:
```
go install github.com/marwan-at-work/mod/cmd/mod@latest
mod upgrade
```
Migrate generated files to import /v2
* Fix gen
Thomas Kosiewski