[codex] Require CSRF for state-changing APIs#442
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12f4ff62c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stmts = append(stmts, apiCSRFStmts(csrf)...) | ||
| stmts = append(stmts, | ||
| assign([]ast.Expr{selExpr(id("request"), "Body")}, call(sel("http", "MaxBytesReader"), id("response"), selExpr(id("request"), "Body"), id("maxAPIBodyBytes"))), |
There was a problem hiding this comment.
When a state-changing API request has the CSRF cookie but omits the CSRF header, CSRF.Validate falls back to request.ParseForm() (addons/actions/csrf.go:163-168). Because this new API CSRF block runs before wrapping request.Body with http.MaxBytesReader, form-encoded cross-site requests bypass the configured maxAPIBodyBytes cap during CSRF validation before the request is rejected. Move the MaxBytesReader assignment ahead of CSRF validation or make the validator avoid parsing an uncapped API body.
Useful? React with 👍 / 👎.
| func csrfEnabled(options Options) bool { | ||
| adapter := backendAdapterIR(options) | ||
| return options.Config.Build.CSRF.EnabledForGeneratedEndpoints() && (adapter.HasEndpointKind(BackendEndpointAction) || contractExposuresParseForm(executableContractExposures(adapter.ContractExposures))) | ||
| return options.Config.Build.CSRF.EnabledForGeneratedEndpoints() && (adapter.HasEndpointKind(BackendEndpointAction) || adapter.HasCSRFSensitiveAPI() || contractExposuresParseForm(executableContractExposures(adapter.ContractExposures))) |
There was a problem hiding this comment.
Enabling generated CSRF solely because a state-changing API exists makes browser fetch/XHR clients send X-GOWDK-CSRF, but the current token source is only exercised by CSRFInjectHTML, which returns without minting a cookie unless a GET HTML response contains a POST form (runtime/app/app.go:381-388,400-408), and the cookie itself is HttpOnly. In an API-only flow or a page that posts via JS without a generated POST form, clients never receive a token to echo, so every legitimate POST/PUT/DELETE API call is rejected with 403; add a generated token bootstrap such as a meta/header/endpoint for API CSRF.
Useful? React with 👍 / 👎.
| if csrfRule.Code == "" { | ||
| csrfRule.Code = csrfCodeForKind(endpoint.Kind) | ||
| } | ||
| if policy.Builtin && endpoint.Kind == "api" && csrfRule.Code == "audit_api_missing_csrf" && !gwdkir.HTTPMethodRequiresCSRF(endpoint.Method) { |
There was a problem hiding this comment.
When a project declares a policy that extends "baseline.api", the inherited audit_api_missing_csrf rule is evaluated under the child policy with Builtin == false, so this guard no longer skips GET/HEAD-style APIs and gowdk audit starts reporting missing CSRF for read-only APIs. Since docs/language/audit.md documents extending built-ins as normal composition, keep the state-changing-method exemption attached to the API CSRF rule itself rather than only to policies marked built-in.
Useful? React with 👍 / 👎.
1 participant
Summary
audit_api_missing_csrfbaseline diagnostic when CSRF is disabled.Closes #433.
Verification
go test ./internal/gwdkir ./internal/gwdkanalysis ./internal/appgen ./internal/compiler ./internal/auditspec ./internal/diagnosticsgo build ./cmd/gowdkscripts/test-go-modules.shNotes
I checked #431, #432, #434, #435, #436, and #437 against current
main; those fixes are already present there. PR #440 is already merged with green checks.