iframe-proxy

kylecarbs · 2022-01-20T14:21:21Z

Due to misconfiguration of a linting rules directory, our linter has not been
working properly. This change fixes the configuration issue, and all remaining
linting errors.

Due to misconfiguration of a linting rules directory, our linter has not been working properly. This change fixes the configuration issue, and all remaining linting errors.

codecov · 2022-01-20T14:23:10Z

bryphe-coder

Thanks for catching the lint misconfiguration and fixing these!

f0b975f rolls in PRs #33 and #34 on coder/agents-chat-action, which unbreak action.yaml load after the identity-model refactor (a literal ${{ github.actor }} in an input description). No workflow input changes.

) Follows #28340, now merged. `smtp.go` renders the notification title through `PlaintextFromMarkdown`, which strips Markdown **and decodes HTML entities**, then stores the result in `Labels["_subject"]`. `html.gotmpl` interpolated that raw into `<title>` and `<h1>`, so an entity-encoded payload in a user-controlled label arrived as live markup: ``` template_display_name = <a href="https://attacker.example/login">Re-authenticate now</a> -> <title>Template "<a href="https://attacker.example/login">Re-authenticate now</a>" deleted</title> ``` Markdown escaping cannot reach this. `&` is not backslash-escapable in either renderer, and this path never enters gomarkdown, so neither `html.SkipHTML` nor the `Safelink` added in #28340 sees the string. `{{ .UserName }}` was interpolated raw at the same template, straight from the unescaped payload the dispatcher receives. This PR adds `| html` to seven values across eleven positions in `html.gotmpl`: | Value | Positions | Why | |---|---|---| | `.Labels._subject` | 2 | the injection above, in `<title>` and `<h1>` | | `.UserName` | 1 | reaches the template straight from the unescaped payload | | `$action.URL` | 1 | rendered from user data at `enqueuer.go:201`; `EscapedForMarkdown` does not touch `Actions` | | `$action.Label` | 1 | static today, escaped so it stays safe if that changes | | `base_url` | 4 | `--access-url` is scheme-checked only, so a `"` closes the `href` | | `current_year` | 1 | cannot carry markup, escaped so the rule has no exceptions | | `.NotificationTemplateID` | 1 | same | `logo_url` and `app_name` were already escaped in #28340. `{{ .Labels._body }}` stays unescaped: it is intentionally gomarkdown output, and it is the only value in the file that is not escaped. The action and `base_url` values are defense in depth rather than open holes. A `"` in an action URL fails closed at enqueue, because the rendered actions JSON is unmarshalled before use and the quote breaks that parse; `<`, `>`, `&` and `'` survive but are inert inside a double-quoted attribute. `base_url` requires an operator to set a hostile `--access-url`. Every value is guarded by a test. Removing `| html` from any of the nine escaped values now fails a named test, verified by removing each pipe in turn: - `TestSMTPHTMLTemplateEscapesUntrustedValues` covers `_subject`, `UserName` and both action values. - `TestSMTPHTMLTemplateEscapesTrustedValues` covers `base_url`, `current_year` and `.NotificationTemplateID`, none of which can carry markup in production, so no golden file would catch their regression. - `TestSMTPHTMLTemplateEscapesAppearanceHelpers` covers `logo_url` and `app_name`. That last point is why the trusted values needed tests rather than goldens: escaping them costs zero golden churn, so nothing already in the tree fails when it is removed. Before this PR the same was true of `$action.Label`, whose escaping could be deleted with no golden diff and no failing test at all. The 36 golden files change by entity encoding only, mostly `"` to `"` and `'` to `'` in subjects. Verified by quoted-printable decoding every file before and after and confirming the two are identical once HTML entities are decoded: 36/36 with no semantic difference. Escaping `base_url`, `current_year` and `.NotificationTemplateID` added no further churn. **NOTE**: `$action.URL | html` turns the `&` in the one-time passcode reset link into `&`. That is the correct encoding of a literal `&` in an attribute value, and every conformant client decodes it before navigating, so the request the server receives is unchanged. It is the only golden change with behavior attached. Migrating this template to `html/template` was considered and declined; the reasoning and the conditions that would reverse it are on the CRF-3 review thread. Refs https://linear.app/codercom/issue/PLAT-273/markdown-link-injection-into-admin-notification-emails-sec-93

) Follows #28340, now merged. `smtp.go` renders the notification title through `PlaintextFromMarkdown`, which strips Markdown **and decodes HTML entities**, then stores the result in `Labels["_subject"]`. `html.gotmpl` interpolated that raw into `<title>` and `<h1>`, so an entity-encoded payload in a user-controlled label arrived as live markup: ``` template_display_name = <a href="https://attacker.example/login">Re-authenticate now</a> -> <title>Template "<a href="https://attacker.example/login">Re-authenticate now</a>" deleted</title> ``` Markdown escaping cannot reach this. `&` is not backslash-escapable in either renderer, and this path never enters gomarkdown, so neither `html.SkipHTML` nor the `Safelink` added in #28340 sees the string. `{{ .UserName }}` was interpolated raw at the same template, straight from the unescaped payload the dispatcher receives. This PR adds `| html` to seven values across eleven positions in `html.gotmpl`: | Value | Positions | Why | |---|---|---| | `.Labels._subject` | 2 | the injection above, in `<title>` and `<h1>` | | `.UserName` | 1 | reaches the template straight from the unescaped payload | | `$action.URL` | 1 | rendered from user data at `enqueuer.go:201`; `EscapedForMarkdown` does not touch `Actions` | | `$action.Label` | 1 | static today, escaped so it stays safe if that changes | | `base_url` | 4 | `--access-url` is scheme-checked only, so a `"` closes the `href` | | `current_year` | 1 | cannot carry markup, escaped so the rule has no exceptions | | `.NotificationTemplateID` | 1 | same | `logo_url` and `app_name` were already escaped in #28340. `{{ .Labels._body }}` stays unescaped: it is intentionally gomarkdown output, and it is the only value in the file that is not escaped. The action and `base_url` values are defense in depth rather than open holes. A `"` in an action URL fails closed at enqueue, because the rendered actions JSON is unmarshalled before use and the quote breaks that parse; `<`, `>`, `&` and `'` survive but are inert inside a double-quoted attribute. `base_url` requires an operator to set a hostile `--access-url`. Every value is guarded by a test. Removing `| html` from any of the nine escaped values now fails a named test, verified by removing each pipe in turn: - `TestSMTPHTMLTemplateEscapesUntrustedValues` covers `_subject`, `UserName` and both action values. - `TestSMTPHTMLTemplateEscapesTrustedValues` covers `base_url`, `current_year` and `.NotificationTemplateID`, none of which can carry markup in production, so no golden file would catch their regression. - `TestSMTPHTMLTemplateEscapesAppearanceHelpers` covers `logo_url` and `app_name`. That last point is why the trusted values needed tests rather than goldens: escaping them costs zero golden churn, so nothing already in the tree fails when it is removed. Before this PR the same was true of `$action.Label`, whose escaping could be deleted with no golden diff and no failing test at all. The 36 golden files change by entity encoding only, mostly `"` to `"` and `'` to `'` in subjects. Verified by quoted-printable decoding every file before and after and confirming the two are identical once HTML entities are decoded: 36/36 with no semantic difference. Escaping `base_url`, `current_year` and `.NotificationTemplateID` added no further churn. **NOTE**: `$action.URL | html` turns the `&` in the one-time passcode reset link into `&`. That is the correct encoding of a literal `&` in an attribute value, and every conformant client decodes it before navigating, so the request the server receives is unchanged. It is the only golden change with behavior attached. Migrating this template to `html/template` was considered and declined; the reasoning and the conditions that would reverse it are on the CRF-3 review thread. Refs https://linear.app/codercom/issue/PLAT-273/markdown-link-injection-into-admin-notification-emails-sec-93 (cherry picked from commit 2236710)

@uri

) **TL;DR** Fifth of the stack, split out of #28045. #28450 delivered the errors raised *after* the callback is trusted and left the parameter failures behind. This PR delivers those, and replaces the implicit ordering that decided where an answer went with an explicit classification. | PR | What it does | |---|---| | #28007 | Schema: `codes.scope` and `tokens.scope`, so a negotiated scope has somewhere to live. | | #28167 | `ScopesCover` compares an allowlist against a request by permission coverage rather than by name. | | #28178 | The authorize endpoint negotiates the scope against the app's allowlist and persists it on the code. | | #28179 | The consent page lists the negotiated permissions, and `invalid_scope` reaches the client's callback. | | #28450 | The same delivery for every other error raised once the callback is trusted. | | **#28736 (this)** | The parameter failures #28450 left behind, and the classification that decides where each answer goes. | - Stacked on #28450. Review that first. - Nothing about what a token can do changes here. This is delivery, not enforcement. **Contract change** A rejected parameter now arrives at the app's registered callback with `error=invalid_request`, a description naming every failing field, and the request's `state`. Both verbs. | Verb | Was | |---|---| | GET | static "Invalid Query Parameters" page, 400 | | POST | `WriteOAuth2Error`, 400 | An integrator watching for either now reads the error from its own callback instead. **Two failures still answer on Coder**, because in neither case is the callback trustworthy yet: - A `redirect_uri` that does not parse, or does not exactly match the registration. Redirecting to it would defeat the check that just rejected it. - A `client_id` sent more than once, or naming something other than the app the callback was matched against. Coder cannot tell whose registration it is about to redirect to. An absent query `client_id` is **not** in that group. `httpmw` also reads the POST form body and the §2.3.1 Basic credential, so an absent query parameter still names a client and its failure is deliverable. **Precedence change.** An app whose *registered* callback does not parse, or uses a rejected scheme, now answers 500 even when the request also carries parameter errors; that used to answer 400. Decided before any parameter is read, so the others are never detected. Only reachable for an app row that bypassed registration. `DangerousCallbackSchemeOutranksParseFailure` pins it. **Also in this PR** - **Unrecognized parameters are ignored**, as §3.1 requires, rather than rejected by `ErrorExcessParams`. An OIDC `nonce` or a vendor extension no longer fails the request. Repeats of parameters the endpoint does read are still rejected. - **Every unsupported `response_type` gets one code.** Read as text rather than through the SDK enum, so a value with no Go constant behind it answers `unsupported_response_type` like `token` does, instead of splitting on whether the SDK names it. - **A malformed `resource` answers `invalid_target`** (RFC 8707 §2), but only when nothing else failed. A client retrying on `invalid_target` would otherwise resend a request still broken in a field it never heard about. - **`error_description` is bounded and readable.** Capped at 2048 characters and marked `(truncated)` before the log field or the `Location` header is written. Entries read `field: reason` joined with `; `, not the parser's debug shape, whose details contain commas and cannot be split apart again. - **A repeated `state` is charged to the client, not the callback.** Reading `state` shares a function with the `redirect_uri` match, so it used to fall into that carve-out and answer on Coder. **Where in the OAuth Flow** <details> <summary>Diagram: the three-way dispatch, and what licenses a redirect</summary> ```mermaid flowchart TD NEW["newAuthorizeResponse parses the registered callback, checks its scheme, exact-matches any redirect_uri, reads state"] NEW -->|"registration unusable"| C500["500 on Coder server_error, value logged not echoed"] NEW --> PARSE["extractAuthorizeParams reads every parameter, collects all failures"] PARSE -->|"no failure"| OK["GET: consent page POST: authorization code"] PARSE -->|"failure"| KIND{"authorizeFailure.kind()"} KIND -->|"corrupt registration"| C500 KIND -->|"redirect_uri or client_id at fault"| CODER["400 on Coder RFC 6749 4.1.2.1 carve-out"] KIND -->|"anything else"| CLIENT["302 to the callback invalid_request or invalid_target, with state"] ``` </details> - Authorize endpoint only, both verbs. Token exchange, refresh, and revocation are untouched, and `ErrorExcessParams` still guards the token endpoint. - `validatedCallbackURL` becomes `authorizeResponse`, built only by `newAuthorizeResponse`, which runs the scheme check and the exact match in that order. Holding one is what licenses a redirect, so the ordering is structural rather than a convention each call site remembers. - The scheme is checked on the *registered* URL, since `p.RedirectURL` returns the client's URI on a mismatch and checking that would blame the app for a request it never made. - `state` moves onto the type, out of the parameter lists of `withQuery`, `errorURL`, `codeURL`, and `redirectAuthorizeError`, so no call site can emit a response the client cannot correlate. - `extractAuthorizeParams` returns one `authorizeFailure` instead of two trailing values, and both handlers dispatch on `kind()` rather than re-deriving precedence from field checks. **What it satisfies** - RFC 6749 §4.1.2.1: a failure that is not a bad redirection URI or client identifier is reported through the redirection URI. That is the whole PR. Its two exceptions "MUST NOT" be redirected to, hence the carve-outs. - RFC 6749 §3.1: unrecognized parameters MUST be ignored, and no parameter may appear more than once. Both now hold here. - RFC 6749 Appendix A: `error_description` is confined to the permitted set, on the decoded value. - RFC 8707 §2: a `resource` that is not an absolute URI without a fragment answers `invalid_target`. - RFC 7636 §4.4.1: a malformed `code_challenge` is rejected at the authorization request rather than deferred to token exchange, where the error would point at the `code_verifier`. - Not yet: CRF-26 (the `server_error` sites) and CRF-13 (fragment delivery), both as in #28450. **Docs and Swagger.** `docs/admin/integrations/oauth2-provider.md` gains entries for the redirected parameter errors, `invalid_target`, and the two failures that stay on Coder. Both authorize verbs document their 400 and 500 responses and stop advertising `response_type=token`, which meant declaring the parameter as a `string`, since `Enums` appends to what swaggo derives from the type. --- Split out of #28045 ([PLAT-479](https://linear.app/codercom/issue/PLAT-479)). Fifth of the stack, stacked on #28450. ## Manual Tests Verified by hand against a local dev deployment (`v2.37.0-devel+1145d35a47`, dev Postgres), in addition to the automated suite. The build string was checked first, because `develop.sh` builds from whatever the tree held when it started and every result below would otherwise be describing the wrong binary. Two scenario groups need state the API cannot produce. The scope allowlist is reachable only through dynamic client registration, which was enabled for the run and disabled again at the end. A registered callback that does not parse, or that uses a blocked scheme, is refused at registration, so those rows were planted directly with `psql` on a purpose-built client and restored afterwards. **28 scenarios, all passing.** No correctness or security defect was found in the code this PR changes. Seven observations came out of the run: six are consistency, documentation or diagnosability points, and one is a small defect that predates this PR. <details> <summary>Scenario summary, all 28</summary> | # | Scenario | Result | |---|----------|--------| | 1 | A well-formed request still succeeds on both verbs, and the consent page renders | Pass | | 2 | A rejected parameter now reaches the client's callback on `POST` | Pass | | 3 | The same on `GET`, and the consent page does not render first | Pass | | 4 | The description names every failing field, joined with `; ` | Pass | | 5 | A mismatched `redirect_uri` is answered on Coder with no `Location` on either verb | Pass | | 6 | An unparseable `redirect_uri` likewise, and the echoed value is HTML escaped | Pass | | 7 | A repeated `client_id` is answered on Coder although the callback was valid | Pass | | 8 | A single query `client_id` cannot disagree with the resolved app over HTTP | Pass | | 9 | A `client_id` supplied only in the `POST` form body is delivered, not withheld | Pass | | 10 | A `{UPPERCASE}` `client_id` is delivered, and also succeeds end to end | Pass | | 11 | A repeated `state` is delivered rather than charged to the redirect carve-out | Pass | | 12 | A registered callback with a blocked scheme answers 500 on both verbs | Pass | | 13 | An unparseable registered callback answers 500, indistinguishably to the caller | Pass | | 14 | Corrupt registration outranks a deliverable failure and both carve-outs | Pass | | 15 | The corrupt value is logged with the app ID and never appears in a response | Pass | | 16 | Restoring the row restores normal behaviour with no restart | Pass | | 17 | Every unsupported `response_type` gets one code; an empty one is `invalid_request` | Pass | | 18 | An unsupported `response_type` is not recast as a missing `code_challenge` | Pass | | 19 | Unrecognized parameters are ignored, including a case-variant `REDIRECT_URI` | Pass | | 20 | A misspelled `redirect_url` is ignored and cannot smuggle a destination | Pass | | 21 | Repeated known parameters are still rejected | Pass | | 22 | `resource` answers `invalid_target` alone and `invalid_request` in company | Pass | | 23 | A fragment in `resource` is rejected | Pass, with one gap | | 24 | A long description is capped at 2048 and marked `(truncated)`, on both verbs | Pass | | 25 | The description is sanitized to the RFC 6749 §4.1.2.1 character set | Pass | | 26 | The registered callback query is retained except the reserved names | Pass | | 27 | Scope rejections still reach the callback, and no consent page renders | Pass | | 28 | Declining consent carries `access_denied` and issues no code | Pass | </details> <details> <summary>Observations, all 7</summary> | # | Observation | Severity | |---|-------------|----------| | 1 | `invalid_request` is returned in three description shapes: the parser's `Invalid query params: field: reason; ...` aggregate, the PKCE validator's single message, and `scopeFailureResponse`'s. A client cannot parse all three with one rule. | Consistency, reviewer call | | 2 | The phase 2 runbook for #28045 asserts the old `field: x detail: y` message shape, which this PR replaces. That doc needs a one-line update. | Docs follow-up | | 3 | `clientIDInDoubt`'s `default` branch is unreachable over HTTP, because `httpmw` derives `app.ID` from the same query value the parser reads. Defensive rather than dead, but the comment does not say so. | Comment clarity | | 4 | A `POST` supplying `client_id` only in the form body always fails with `client_id ... is required and cannot be empty`, naming a parameter it did supply. `httpmw` reads the body, `RequiredNotEmpty` reads the query. Pre-existing. | Misleading diagnostic | | 5 | A misspelled parameter is invisible from every angle at default verbosity. The client gets a 302 with a code, its redirect is silently replaced by the registered one, and the `ignoring unrecognized authorization parameters` line is `logger.Debug` so it is not emitted. | Diagnosability | | 6 | `resource` sent twice answers `invalid_target` rather than `invalid_request`. RFC 8707 §2 frames `invalid_target` as being about the resource value; a duplicated parameter is a malformed request under RFC 6749 §3.1. | Low, error code choice | | 7 | `resource=https://api.example.com/#` (trailing `#`, empty fragment) is accepted and stored with the `#` intact, so the persisted audience is not textually equal to the fragment-free form. `url.Parse` maps empty and absent fragments both to `Fragment == ""`. Pre-existing, in `tokens.go`. | Defect, low severity | </details> **One note for anyone re-running this.** Every request below depends on `$CHALLENGE` from the most recent `new_pkce` call. Forgetting to call it sends an empty `code_challenge`, which fails with the fixed "is required and cannot be empty" message at 98 characters and looks exactly like a cap or a validator failing to fire. That cost one wrong measurement during this run before the numbers below were taken. <details> <summary>Shell helpers used throughout</summary> ```bash export BASE_URL=http://localhost:3000 export AUTH_HEADER="Coder-Session-Token: $(cat ./.coderv2/session)" export PGPASSWORD=$(cat ./.coderv2/postgres/password) export PGPORT=$(cat ./.coderv2/postgres/port) pg() { psql -h localhost -p "$PGPORT" -U coder -d coder -tAc "$1"; } plant_callback() { pg "UPDATE oauth2_provider_apps SET callback_url = '$2' WHERE id = '$1';"; } urlenc() { jq -rn --arg v "$1" '$v|@uri'; } # 43 unreserved characters every time. Base64url of the raw 32 bytes. new_pkce() { VERIFIER=$(openssl rand 32 | base64 | tr -d '\n=' | tr '+/' '-_') CHALLENGE=$(printf '%s' "$VERIFIER" | openssl dgst -sha256 -binary | base64 | tr -d "=" | tr '+/' '-_') STATE=$(openssl rand -hex 16) } authz_url() { local url="$BASE_URL/oauth2/authorize?client_id=$1&response_type=code" url="$url&redirect_uri=$(urlenc "$2")&state=$STATE" url="$url&code_challenge=$CHALLENGE&code_challenge_method=S256" printf '%s' "$url" } # The core assertion of the whole run: what answer, delivered where. answer() { curl -s -o /dev/null -D - -X "$1" "$2" -H "$AUTH_HEADER" | grep -iE '^HTTP/|^location:'; } show_callback() { python3 - "$1" <<'PY' import sys, urllib.parse q = urllib.parse.urlparse(sys.argv[1]).query for k, v in urllib.parse.parse_qsl(q, keep_blank_values=True): print(f"{k} = {v}") PY } ``` Fixtures, all registered through DCR: ```text plat479-plain http://localhost:9876/callback plat479-query http://localhost:9876/callback?tenant=acme&error=stale plat479-corrupt http://localhost:9876/callback (overwritten by psql in 12 to 16) plat479-reserved http://localhost:9876/callback?tenant=acme&code=FAKECODE&error=stale&error_description=oldmsg&state=oldstate&keep=yes plat479-ci http://localhost:9876/callback (scope allowlist coder:workspaces.access) ``` </details> <details> <summary>1. Baseline: a well-formed request still succeeds</summary> ```bash new_pkce; answer GET "$(authz_url "$APP_ID" http://localhost:9876/callback)" new_pkce; answer POST "$(authz_url "$APP_ID" http://localhost:9876/callback)" ``` ```text === GET === HTTP/1.1 200 OK === POST === HTTP/1.1 302 Found Location: http://localhost:9876/callback?code=coder_dzvaCPf9W1_...&state=e321bd5d5b594a1fae8a2d5da654b84a ``` `GET` carries no `Location` at all, so the consent page really rendered rather than redirecting. The `state` echoed is the one sent. The code persisted as `coder:all`, this app having no allowlist, with `redirect_uri` recorded because the request supplied one. Also driven through a real browser rather than curl, so the consent form's `nosurf` token is the one submitted rather than the session-token header alone. Clicking **Allow** delivered `code` and `state` to the callback. Both request shapes agree on the success case. Not to be misread: `oauth2_provider_app_codes` holds one row after both POSTs, not two. `ProcessAuthorize` opens its transaction with `DeleteOAuth2ProviderAppCodesByAppAndUserID`, so a row count is not a count of successful authorizations. That predates this PR. </details> <details> <summary>2 to 4. A rejected parameter now reaches the client's callback</summary> ```bash new_pkce BAD="$BASE_URL/oauth2/authorize?client_id=$APP_ID&response_type=code" BAD="$BAD&redirect_uri=$(urlenc http://localhost:9876/callback)&state=$STATE" BAD="$BAD&code_challenge=short&code_challenge_method=S256" answer POST "$BAD" answer GET "$BAD" ``` ```text HTTP/1.1 302 Found Location: http://localhost:9876/callback?error=invalid_request&error_description=Invalid+query+params%3A+code_challenge%3A+must+be+43+to+128+characters+from+the+unreserved+character+set+%5BA-Za-z0-9-._~%5D&state=e89f4d98d4c8c347e5ae25175748b731 ``` Identical on both verbs. Decoded: ```text error = invalid_request error_description = Invalid query params: code_challenge: must be 43 to 128 characters from the unreserved character set [A-Za-z0-9-._~] state = e89f4d98d4c8c347e5ae25175748b731 ``` The `GET` body confirms no page was rendered, against the baseline: | Request | Body size | Occurrences of "Allow" | |---|---|---| | Malformed `code_challenge` | 263 bytes | 0 | | Well formed (scenario 1) | 4928 bytes | 2 | 263 bytes is Go's redirect stub. There is no consent form in it, so there is no Allow button to press for a request the server has already refused. Multiple failures are reported together (scenario 4): ```text error_description = Invalid query params: code_challenge: must be 43 to 128 characters from the unreserved character set [A-Za-z0-9-._~]; resource: must be an absolute URI without fragment ``` The separator is `; `, and no entry uses the older `field: x detail: y` shape. The code is `invalid_request` rather than `invalid_target` even though `resource` is one of the two failures, which is the scenario 22 guard firing. The same request also carried `code_challenge_method=plain`, which is invalid and is *not* mentioned, because the method is validated after `extractAuthorizeParams` returns and a parse failure short-circuits first. Sent alone it is rejected as `code_challenge_method 'plain' is not supported; use 'S256'`, with no `Invalid query params: ` prefix. That difference is observation 1. </details> <details> <summary>5 and 6. The redirect_uri carve-out keeps the answer here</summary> ```bash new_pkce EVIL="$BASE_URL/oauth2/authorize?client_id=$APP_ID&response_type=code" EVIL="$EVIL&redirect_uri=$(urlenc http://evil.example/steal)&state=$STATE" EVIL="$EVIL&code_challenge=$CHALLENGE&code_challenge_method=S256" answer POST "$EVIL"; answer GET "$EVIL" ``` ```text HTTP/1.1 400 Bad Request HTTP/1.1 400 Bad Request ``` **The absence in that output is the assertion**: no `Location` on either verb, so nothing redirects to `evil.example`. Stronger, `evil.example` appears zero times in either full response, headers and body together, so the attacker-supplied host is not reflected into the page either. ```json {"error":"invalid_request","error_description":"Invalid query params: redirect_uri: Query param \"redirect_uri\" must exactly match http://localhost:9876/callback"} ``` The `GET` page names the *registered* callback as the value the parameter had to match, which is the app's own configuration and safe to display. An unparseable `redirect_uri` reaches the same outcome by a different route, having never parsed at all: ```json {"error":"invalid_request","error_description":"Invalid query params: redirect_uri: Query param \"redirect_uri\" must be a valid url: parse \"http://%zz\": invalid URL escape \"%zz\""} ``` That message echoes the caller's value, unlike the mismatch case, so the `GET` path was probed with markup in it: ```text input: http://%zz"><img src=x onerror=BAD> page: ...must be a valid url: parse "http://%zz\"><img src=x onerror=BAD>"... literal `<img` in body: 0 entity encoded `<img`: 1 ``` Escaped correctly by the template layer. Recorded as a negative result so a future change to the error page has something to regress against. One note for the docs: this detail contains colons, so a client splitting `field: reason` entries must split each on its **first** colon only. </details> <details> <summary>7 to 10. The client_id carve-out, and its negatives</summary> A repeated `client_id` in an otherwise perfect request: ```text HTTP/1.1 400 Bad Request (both verbs, no Location) {"error":"invalid_request","error_description":"Invalid query params: client_id: Query param \"client_id\" provided more than once, found 2 times. Only provide 1 instance of this query param."} ``` This is the one carve-out not implied by the response's shape. The `redirect_uri` matched the registration, so `canRedirect()` was true and there *was* somewhere to send the answer; it was withheld because `extractAuthorizeParams` assigns `failure.redirect` only when `clientIDInDoubt` is false. **The `default` branch of that function is unreachable over HTTP** (observation 3). `httpmw` resolves the app from `r.URL.Query().Get("client_id")`, so `app.ID` and the parsed value derive from the same string and cannot disagree. Three experiments: ```text one query client_id, bad challenge -> 302 delivered two DIFFERENT client_id values -> 400, caught by the repeated branch query names app A, form body names app B -> 302 to app A; the body is ignored ``` The negatives matter as much as the positives. An identifier absent from the query but present in the `POST` form body is **delivered**, correctly: ```text HTTP/1.1 302 Found Location: ...error=invalid_request&error_description=Invalid+query+params%3A+client_id%3A+...is+required+and+cannot+be+empty%3B+code_challenge%3A+... ``` That is observation 4: the description says `client_id` is required for a request that supplied it. With everything else valid the same shape still fails, while the identical request carrying `client_id` in the query succeeds with a code. `RequiredNotEmpty` reads the query; `httpmw` reads the body. Pre-existing, and arguably the parser is the layer in the right, since RFC 6749 §4.1.1 puts authorization parameters in the query string and §3.2.1 is the endpoint that uses a form body. A braced upper-case UUID is delivered, and succeeds end to end: ```text {UPPERCASE} client_id, bad challenge -> 302, description names ONLY code_challenge {UPPERCASE} client_id, all valid -> 302 with a real code GET, all valid -> 200, consent page, heading resolves to the app's real name ``` `uuid.Parse` accepts that spelling and `httpmw` resolved through it, so a string comparison here would withhold an answer the client is entitled to, for no reason it could diagnose. </details> <details> <summary>11. A repeated state is delivered, not withheld</summary> ```text HTTP/1.1 302 Found (both verbs) error = invalid_request error_description = Invalid query params: state: Query param 'state' provided more than once, found 2 times. Only provide 1 instance of this query param. ``` Reading `state` happens inside `newAuthorizeResponse`, the same function that runs the `redirect_uri` match. A count of errors across those two lines would charge this to the redirect carve-out and answer 400. It answered 302, so the field-specific test is doing the work. The asymmetry with scenario 7 is deliberate: a repeated `client_id` leaves the server unsure whose callback it holds, a repeated `state` leaves the callback fully settled. The response carries **no `state`**, since `parseSingle` collapsed the repeat to the empty string and `withQuery` only sets it when non-empty. Unavoidable rather than wrong: picking one of two values could satisfy a CSRF check the client meant to be strict. </details> <details> <summary>12 to 16. A corrupt registration outranks everything</summary> DCR refuses both planted values, which is why `psql` is needed and why 500 is the right answer: a stored one is not something a client did. ```json {"error":"invalid_client_metadata","error_description":"invalid redirect_uris: redirect URI at index 0: redirect URI uses dangerous scheme javascript which is not allowed"} {"error":"invalid_client_metadata","error_description":"invalid redirect_uris: redirect URI at index 0 is not a valid URL: parse \"http://%zz\": invalid URL escape \"%zz\""} ``` ```bash plant_callback "$APP_CORRUPT_ID" 'javascript:alert(1)' new_pkce CORRUPT="$BASE_URL/oauth2/authorize?client_id=$APP_CORRUPT_ID&response_type=code&state=$STATE" CORRUPT="$CORRUPT&code_challenge=$CHALLENGE&code_challenge_method=S256" answer POST "$CORRUPT"; answer GET "$CORRUPT" ``` ```text HTTP/1.1 500 Internal Server Error {"error":"server_error","error_description":"The application's registered callback URL is not usable"} GET: HTTP/1.1 500, page reads "500 - Invalid Callback URL", 3346 bytes, 0 Allow buttons ``` The unparseable callback is **indistinguishable to the caller**: same description, and a `GET` page of exactly 3346 bytes, matching byte for byte. Two different defects in stored state, one answer. Correct, since neither is actionable by the client. Note the request carried no `redirect_uri` of its own and still failed, because the scheme is checked on the *registered* URL before the client's value is consulted. Precedence was tested against all three competitors, not one: | Also wrong in the request | Would answer alone | Actually answered | |---|---|---| | `code_challenge=short` | 302 to the callback | **500** | | `client_id` repeated | 400 kept here | **500** | | `redirect_uri` mismatched | 400 kept here | **500** | Reading the source, this is stronger than precedence. `newAuthorizeResponse` runs at `authorize.go:250`, before any of `client_id`, `code_challenge`, `scope` or `resource` is read, and the early return discards whatever `p.Errors` already held. In rows two and three those failures are never detected at all, so `kind()` ranking `failureCorruptRegistration` first is belt and braces for a failure constructed some other way. **The value is logged and never echoed.** A distinctive marker was planted and five requests fired: ```text [erro] coderd: oauth2 app has an unusable registered callback URL request_id=01ca4796-c743-4923-a06b-afc1f3decc95 app_id=12f9ca3b-7a2c-4c54-8ed7-723698c62bd1 callback_url="javascript:alert(\"plat479-6d-marker\")" error= redirect URI uses dangerous scheme javascript which is not allowed: codersdk/oauth2_validation.go:116 ``` Five requests, five lines, in request order. Occurrences of the marker in the POST body, the rendered GET page, and the response headers: **0, 0, 0**. The `request_id` joins that line to the request log entry whose `response_body` field holds the vague message, so the server's own record confirms what it sent for the same request rather than a replay. The log is also where the two causes diverge, naming `validateScheme` and its source line, which the response cannot do. Restoring the row restores normal behaviour immediately, with no restart: ```text POST -> 302 with a code GET -> 200 consent page ``` That is a control rather than housekeeping: it rules out the app having been poisoned lastingly, and rules out a cached parse outliving the `UPDATE`. </details> <details> <summary>17 and 18. Every unsupported response_type gets one code</summary> | `response_type` sent | `error` | `error_description` | |---|---|---| | `token` | `unsupported_response_type` | Only response_type=code is supported | | `banana` | `unsupported_response_type` | Only response_type=code is supported | | `code token` | `unsupported_response_type` | Only response_type=code is supported | | `CODE` | `unsupported_response_type` | Only response_type=code is supported | | `code_extra` | `unsupported_response_type` | Only response_type=code is supported | | empty string | `invalid_request` | Invalid query params: response_type: Query param 'response_type' is required and cannot be empty | `token` has a Go constant behind it and `banana` does not, so their agreement is what reading the value as text bought. Three rows are more interesting than `banana`: `code token` is a legal RFC 6749 §3.1.1 space-delimited list used by OIDC's hybrid flow, `CODE` confirms the comparison is case sensitive, and `code_extra` confirms it is equality rather than a prefix match. The empty string is correctly the exception. A valueless parameter is *missing*, not unsupported, and RFC 6749 §3.1 requires it be treated as omitted. **The error is in the query, not the fragment.** `response_type=token` is the implicit grant's own value, so a fragment would be arguable, but this deployment advertises `"response_types_supported":["code"]` alone, and a fragment is never sent to the server, so it would be unreadable to the client's backend. PKCE is not recast, which the controls show: | `response_type`, no `code_challenge` sent | `error` | |---|---| | `token` | `unsupported_response_type`, zero mentions of `code_challenge` | | `banana` | `unsupported_response_type`, zero mentions of `code_challenge` | | `code` | `invalid_request`, naming `code_challenge` | Without the `if params.responseType == responseTypeCode` guard, a client sending `token` would be told its `code_challenge` was missing, add one, resend, and be told the same thing again. </details> <details> <summary>19 to 21. Unrecognized parameters ignored, repeats still rejected</summary> Eight unknown parameters, each on an otherwise valid request, every one issuing a code: | Unknown parameter | Result | |---|---| | `nonce`, `prompt`, `login_hint`, `acr_values`, `max_age`, `ui_locales` | code issued | | `code_challenge_methods=S256` | code issued | | `REDIRECT_URI=http://evil.example/steal` | code issued, **to the registered callback** | Five of those are OpenID Connect Core parameters, so an OIDC client pointed here degrades to plain OAuth2 rather than failing. The ignored parameters are **dropped, not forwarded**: the callback receives only `code` and `state`. The last two rows are the ones with teeth. Query parameter names are case sensitive, so `REDIRECT_URI` must be ignored, and a misspelled `redirect_url` likewise: ```text case variant alongside a valid redirect_uri : Location host localhost:9876 case variant as the only redirect parameter : Location host localhost:9876 redirect_url typo, no valid redirect_uri : Location host localhost:9876, code issued evil.example occurrences in any Location : 0 ``` The consent page renders for the typo case too, and even its cancel link targets the registered callback. A parser matching names case-insensitively would have redirected to `evil.example` with a valid code attached. **Observation 5.** The `ignoring unrecognized authorization parameters` line was never emitted. Three requests carrying a distinctive marker produced no new server output at all, because the call is `logger.Debug` (`authorize.go:307`) and the deployment runs with `verbose: null`. The behaviour is correct and required by RFC 6749 §3.1, but at default verbosity the misspelling is invisible from every angle: the client sees a 302 with a code, its redirect is silently replaced, and nothing is logged. The comment claiming the typo "surfaces here" is optimistic. Repeats of known parameters are still rejected, which the replacement of `ErrorExcessParams` had to leave alone: | Repeated parameter | HTTP | `error` | Answered at | |---|---|---|---| | `code_challenge` | 302 | `invalid_request`, two entries one field | client callback | | `code_challenge_method` | 302 | `invalid_request` | client callback | | `response_type` | 302 | `invalid_request` | client callback | | `redirect_uri` | 400 | withheld | **Coder** | | `scope` | 302 | `invalid_request` | client callback | | `resource` | 302 | `invalid_target` | client callback | `redirect_uri` being withheld confirms the carve-out keys on the field name rather than the kind of mistake. `code_challenge` collects two entries for one mistake, because `parseSingle` collapses the value to empty and the PKCE block then reports it missing. The `resource` row is observation 6. </details> <details> <summary>22 and 23. resource, its own code, and the fragment rule</summary> Both sides of the rule, since a check enforced too broadly would reject a valid URN: | `resource` sent | `error` | Stored `resource_uri` | |---|---|---| | `not a uri` | `invalid_target` | not stored | | `/api` relative | `invalid_target` | not stored | | `https://api.example.com` | none, code issued | as sent | | `https://api.example.com/v1?q=1` | none, code issued | as sent, query is not a fragment | | `urn:example:resource` | none, code issued | as sent | | empty string | none, code issued | `NULL`, same as omitted | `invalid_target` applies only when `resource` is the sole failure. Add any second failure in any other field and the code becomes `invalid_request` naming both, which is the retry-loop guard: ```text resource + bad code_challenge -> invalid_request, both named resource + repeated scope -> invalid_request, both named resource + repeated state -> invalid_request, both named resource alone (control) -> invalid_target ``` Fragments are rejected in all three placements, with or without a path: ```text https://api.example.com/#x -> invalid_target https://api.example.com#x -> invalid_target https://api.example.com/v1#frag -> invalid_target https://api.example.com/# -> code issued, stored as https://api.example.com/# ``` **The last row is observation 7, and it predates this PR.** Go cannot represent the distinction: ```text https://a.example.com/ Fragment="" String()="https://a.example.com/" https://a.example.com/# Fragment="" String()="https://a.example.com/" https://a.example.com/#x Fragment="x" String()="https://a.example.com/#x" ``` `url.Parse` collapses "no fragment" and "empty fragment", so `if u.Fragment != ""` in `validateResourceParameter` (`coderd/oauth2provider/tokens.go:591`) cannot see a trailing `#`. RFC 3986 §3.5 permits a zero-length fragment, which RFC 8707 §2's "MUST NOT include a fragment component" reads as forbidding. Validation parses the value but persistence stores the raw string, so the `#` reaches `resource_uri` and the stored audience is not textually equal to the fragment-free form. A client appending a harmless-looking `#` would get a token bound to an audience nothing matches. The authorize-side call to `validateResourceParameter` is context in this diff rather than an added line; this PR added the `invalid_target` code for the failure. </details> <details> <summary>24 and 25. The description cap and character set</summary> The obvious probe does not exercise the cap: a 4000-character `code_challenge` produces a 116-character description, because that message is fixed text and never quotes the offending value. The descriptions that echo caller input come from a validator's own error. | Request | `error` | Description length | Truncated | `Location` length | |---|---|---|---|---| | `code_challenge_method` = 4000 chars | `invalid_request` | 2060 | **yes** | 2176 | | `scope` = 4000 chars | `invalid_scope` | 2060 | **yes** | 2174 | | `code_challenge_method` = 2000 chars | `invalid_request` | 2035 | no | 2147 | | `code_challenge` = 4000 chars | `invalid_request` | 116 | no | 234 | 2060 is exactly `maxErrorDescription` plus `" (truncated)"`. Row three is the boundary control, so the cap fires on length rather than on the presence of echoed input. Measured from the `Location` header rather than a log line, since the stated reason for the cap is that the header must survive intermediary proxies; the longest observed was 2176 bytes. Both verbs agree, the cap living in the shared `redirectAuthorizeError`: ```text GET, long method : desc_len=2060 truncated=True GET, long scope : desc_len=2060 truncated=True ``` The character set is enforced independently, later, in `errorURL`: ```text input: pl"ain\back<newline>tab<tab>end~unicode description: unsupported code_challenge_method: pl'ainback tab end~ nicode chars outside the RFC 6749 §4.1.2.1 set: [] ``` `"` becomes `'`, `\` is dropped so it cannot escape the rewritten quote, and anything below 0x20 or above 0x7E becomes a space. Checked programmatically rather than by eye. The RFC sets no length limit, so the 2048 cap is policy; the character set is conformance. </details> <details> <summary>26. The registered callback query, retained except reserved names</summary> `plat479-reserved` registers a callback carrying all four reserved names plus two ordinary ones, and DCR stores it unchanged: ```text http://localhost:9876/callback?tenant=acme&code=FAKECODE&error=stale&error_description=oldmsg&state=oldstate&keep=yes ``` Success response, no `redirect_uri` sent: ```text code = coder_WXwBpvYR5u_... keep = yes state = 516abeea9976119eb33ddc10f0ca04ed tenant = acme ``` Failure response, same app: ```text error = invalid_request error_description = Invalid query params: code_challenge: must be 43 to 128 characters... keep = yes state = 3a17bdd75dfcf740bda36124cd161fb7 tenant = acme ``` Stale registered values in either response: `FAKECODE` 0, `stale` 0, `oldmsg` 0, `oldstate` 0. Without the deletion step a registered `error=stale` would ride out on the **success** response, and a client reading `error` before `code`, the conventional order, would discard a valid authorization code. `FAKECODE` is the mirror image on a failure response. The cancel link obeys the same rule, being built by the same `withQuery` path: ```text http://localhost:9876/callback?error=access_denied&error_description=The+resource+owner...&keep=yes&state=e15b5f0f...&tenant=acme ``` RFC 6749 §3.1.2 requires the registered query be retained when adding parameters, which is the `tenant` and `keep` half. It does not say what to do when the registered query collides with the response parameters §4.1.2 adds, so dropping the registered copies is the resolution that keeps the client's read unambiguous. </details> <details> <summary>27 and 28. Scope rejections, and declining consent</summary> `plat479-ci` carries the allowlist `coder:workspaces.access`. ```text scope=template:update (outside) -> error = invalid_scope 'template:update': scope requests permissions beyond this app's allowed scopes GET, same request -> 302, 225 bytes, 0 Allow buttons, no consent page scope=workspace:ssh (covered) -> code issued, persisted scope workspace:ssh GET, same request -> 200, 5182 bytes, consent page naming the scope ``` `workspace:ssh` is granted although the allowlist names the composite rather than that scope, which is the coverage-not-spelling rule from #28045. This run only checks that the changes here left it delivering the same answers. The consent page for a narrow scope reads differently from the unrestricted one: "to access your **admin** account with these permissions?", the scope as `<li role="listitem">workspace:ssh`, and the caution "These are technical permission names. Grant them only to an application you trust." The scope description shape differs from the parser's, coming from `scopeFailureResponse` rather than the field join, which is the third producer behind observation 1. Declining consent, clicked in a real browser: ```text error = access_denied error_description = The resource owner or authorization server denied the request state = a8bd538b365b61f54586892c73968b80 ``` The page carries exactly one `href` and it is the cancel link. The `state` matches, there is no `code`, and the code count for the app is unchanged, the link being a plain `GET` to the client's callback that never reaches `ProcessAuthorize`. The description is RFC 6749 §4.1.2.1's own definition of `access_denied`, word for word. That shared construction is what makes the `#nosec G203` annotation on `CancelURI` sound: the URL is injected as a trusted `htmltemplate.URL`, safe only because `newAuthorizeResponse` validated the registered scheme before the response object could exist. Scenario 12 is the other half, an app with a rejected scheme never reaching this page at all. </details> <details> <summary>Where the consent page renders, across every GET case</summary> Only `GET` can render anything, and body size identifies which of four renderers ran, so this is the table to check when a status code alone looks right. | Case | Status | Body bytes | Allow buttons | Rendered | |---|---|---|---|---| | well formed | 200 | 4928 | 1 | **consent page** | | bad `code_challenge` | 302 | 263 | 0 | redirect stub | | two bad fields | 302 | 319 | 0 | redirect stub | | mismatched `redirect_uri` | 400 | 3814 | 0 | 400 error page | | unparseable `redirect_uri` | 400 | 3846 | 0 | 400 error page | | repeated `client_id` | 400 | 3846 | 0 | 400 error page | | `client_id` in form body only | 400 | 77 | 0 | `httpmw` JSON, unstyled | | `{UPPERCASE}` `client_id` | 200 | 4940 | 1 | **consent page** | | repeated `state` | 302 | 239 | 0 | redirect stub | | corrupt callback, blocked scheme | 500 | 3346 | 0 | 500 error page | | corrupt callback, unparseable | 500 | 3346 | 0 | 500 error page | | `response_type=token` | 302 | 187 | 0 | redirect stub | | `banana` / `code token` / `CODE` | 302 | 187 | 0 | redirect stub | | empty `response_type` | 302 | 243 | 0 | redirect stub | | `token`, no PKCE | 302 | 187 | 0 | redirect stub | | `code`, no PKCE | 302 | 245 | 0 | redirect stub | | unknown params, valid request | 200 | 4932 | 1 | **consent page** | | `redirect_url` typo, valid request | 200 | 4932 | 1 | **consent page** | | `resource` malformed | 302 | 214 | 0 | redirect stub | | `resource` valid absolute URI | 200 | 4932 | 1 | **consent page** | | `resource=.../#` empty fragment | 200 | 4932 | 1 | **consent page**, observation 7 | | reserved params in registered callback | 200 | 4973 | 1 | **consent page** | | `scope` outside allowlist | 302 | 225 | 0 | redirect stub | | `scope` covered by allowlist | 200 | 5182 | 1 | **consent page** | The invariant: the page renders if and only if `extractAuthorizeParams` returned no failure, so no input asks the owner to approve a request the server has already refused. The `evil.example` rows are worth reading carefully rather than alarming: RFC 6749 §3.1 requires a case variant and a typo to be ignored, so from the server's view nothing was wrong with either request, and both cancel links target the registered callback. About 4930 bytes is the consent page, about 3800 the 400 page, 3346 the 500 page, 77 the unstyled `httpmw` JSON, and low hundreds is Go's redirect stub, whose length tracks the `Location` it embeds. Every `unsupported_response_type` answer is exactly 187 bytes because they share one description. Small variations within a renderer are expected: the consent page moves 4928 to 4940 with a fresh CSRF token and the app name. As a regression signal, body size is more sensitive than the status line. A change that rendered a page alongside a redirect would still report 302, but the stub would come back at thousands of bytes rather than hundreds. </details> <details> <summary>Cleanup</summary> ```bash for id in $(pg "SELECT id FROM oauth2_provider_apps WHERE name LIKE 'plat479%';"); do curl -s -X DELETE "$BASE_URL/api/v2/oauth2-provider/apps/$id" -H "$AUTH_HEADER" \ -o /dev/null -w "$id -> %{http_code}\n" done pg "SELECT count(*) FROM oauth2_provider_apps WHERE name LIKE 'plat479%';" ./scripts/coder-dev.sh oauth2-provider dcr disable ``` All planted callback rows were restored before deletion, so no fixture was left holding a value DCR would refuse. </details> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>

chore: Fix golangci-lint configuration and patch errors

1e62837

Due to misconfiguration of a linting rules directory, our linter has not been working properly. This change fixes the configuration issue, and all remaining linting errors.

kylecarbs requested a review from bryphe-coder January 20, 2022 14:21

kylecarbs self-assigned this Jan 20, 2022

kylecarbs added 4 commits January 20, 2022 14:43

Fix race in peer logging

7327507

Fix race and return

5034ca3

Lock on bufferred amount low

f6b76a6

Fix mutex lock

3ca4a35

bryphe-coder approved these changes Jan 20, 2022

View reviewed changes

kylecarbs merged commit 2654a93 into main Jan 20, 2022

kylecarbs deleted the fixlint branch January 20, 2022 16:00

DanielleMaywood mentioned this pull request Mar 27, 2026

perf(site): defer off-screen messages and cache message parsing #23721

Closed

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Uh oh!

chore: Fix golangci-lint configuration and patch errors - #34

chore: Fix golangci-lint configuration and patch errors#34
kylecarbs merged 5 commits into
mainfrom
fixlint

kylecarbs commented Jan 20, 2022

Uh oh!

codecov Bot commented Jan 20, 2022 •

edited

Loading

Uh oh!

bryphe-coder left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

kylecarbs commented Jan 20, 2022

Uh oh!

codecov Bot commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bryphe-coder left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jan 20, 2022 •

edited

Loading