Skip to content

fix(router): harden CSRF — constant-time compare + mandatory EncryptionKey#60

Merged
jcsvwinston merged 1 commit into
mainfrom
feature/csrf-hardening
May 14, 2026
Merged

fix(router): harden CSRF — constant-time compare + mandatory EncryptionKey#60
jcsvwinston merged 1 commit into
mainfrom
feature/csrf-hardening

Conversation

@jcsvwinston
Copy link
Copy Markdown
Owner

Summary

Closes the two CSRF security gaps from the 2026-05-14 post-sprint audit §7. Designed in ADR-006. Lands on top of v0.7.0.

pkg/router is a stable surface — the change is additive on the API and deliberate on behaviour (see BREAKING below).

Security fixes

  • Constant-time token comparison. The old submitted != token short-circuited on the first mismatched byte — a timing side-channel that leaks how many leading bytes of the CSRF token an attacker guessed correctly. Now crypto/subtle.ConstantTimeCompare.
  • EncryptionKey is no longer derived from the cookie name. CSRFOptions.defaults() filled an empty EncryptionKey with sha256(CookieName) — a globally-predictable AES key (the cookie name is public, defaults to "_csrf"). Every deployment that enabled EnableXSRFCookie without an explicit key had a forgeable XSRF-TOKEN cookie. The weak-key derivation is removed; the key is now mandatory and validated.
  • encryptToken/decryptToken no longer slice the key with key[:32] (panicked at request time on a short key, silently truncated a long one) — the key goes straight to aes.NewCipher, which validates length.
  • Fixed a latent bug where a too-short ciphertext decrypted to "" with a nil error.
  • generateCSRFToken panics on a crypto/rand failure rather than issuing a half-filled token.
  • The X-XSRF-TOKEN header is only read when EnableXSRFCookie is true.

API (additive — contract baseline updated)

  • router.NewCSRFMiddleware(CSRFOptions) (func(http.Handler) http.Handler, error) — error-returning constructor; returns router.ErrCSRFEncryptionKey on a misconfiguration.
  • router.ErrCSRFEncryptionKey — sentinel error.
  • CSRFMiddleware keeps its exact signature and becomes the regexp.MustCompile-style wrapper: it panics at construction (never on the request path) when EnableXSRFCookie is set without a 32-byte key.

⚠️ BREAKING (pre-v1.0, stable surface)

An app calling CSRFMiddleware with EnableXSRFCookie: true and no (or a non-32-byte) EncryptionKey previously started successfully with a weak/truncated key; it now panics at startup. Migration: set EncryptionKey to exactly 32 bytes, sourced from the environment or a secret manager — see docs/guides/CSRF_GUIDE.md. Apps with EnableXSRFCookie: false (the default) are unaffected. Documented in CHANGELOG.md under Changed; per contract-guardian, no DEP- entry is needed (no symbol removed or renamed).

Iteration review loop

architect-reviewer PASS (1 WARN), code-reviewer NITS, security-auditor PASS (2 LOW), contract-guardian PASS — no blockers. In-scope review fixes applied in the same PR: removed the dead OriginOnly status-code branch, guarded the X-XSRF-TOKEN header read, corrected an inaccurate godoc comment, added tamper-rejection tests. Deferred follow-ups (CSRF middleware logger, EncryptionKey []byte type, Secure cookie default) recorded in CURRENT_ITERATION.md.

Test plan

  • pkg/router/csrf_hardening_test.go — key validation (missing/short/long/valid), CSRFMiddleware panic path, defaults() no longer derives a key, constant-time accept/reject (same-length and different-length wrong tokens), XSRF-cookie round-trip, tampered X-XSRF-TOKEN rejection (bit-flip / garbage / truncated — must reject, must not panic), X-XSRF-TOKEN ignored when EnableXSRFCookie is false, encryptToken/decryptToken defensive behaviour.
  • Existing TestCSRFMiddleware_* in router_test.go — still pass.
  • go test ./... — clean.
  • go test ./contracts/ — freeze green; NewCSRFMiddleware + ErrCSRFEncryptionKey added to the baseline, correctly sorted.
  • go vet ./pkg/router/ — clean.

🤖 Generated with Claude Code

…onKey

Closes the two CSRF security gaps from the 2026-05-14 audit §7. See
ADR-006.

Security fixes:

- Token comparison is now constant-time (crypto/subtle.ConstantTimeCompare).
  The old `submitted != token` short-circuited on the first mismatched
  byte, leaking — through response latency — how many leading bytes an
  attacker guessed correctly.
- EncryptionKey is no longer derived from the cookie name.
  CSRFOptions.defaults() previously filled an empty EncryptionKey with
  sha256(CookieName) — a globally-predictable AES key, since the cookie
  name is public and defaults to a constant. Any deployment that enabled
  EnableXSRFCookie without an explicit key had a forgeable XSRF-TOKEN
  cookie. The weak-key derivation is removed; the key is now mandatory
  and validated.
- encryptToken/decryptToken no longer slice the key with key[:32] (which
  panicked at request time on a short key, silently truncated a long
  one) — the key goes straight to aes.NewCipher, which validates length.
- Fixed a latent bug where a too-short ciphertext decrypted to "" with a
  nil error.
- generateCSRFToken panics on a crypto/rand failure rather than issuing
  a half-filled, low-entropy token.
- The X-XSRF-TOKEN header is only read when EnableXSRFCookie is true.

API (additive, contract baseline updated):

- NewCSRFMiddleware(CSRFOptions) (func(http.Handler) http.Handler, error)
  — error-returning constructor; returns ErrCSRFEncryptionKey on a
  misconfiguration.
- ErrCSRFEncryptionKey sentinel.
- CSRFMiddleware keeps its signature and becomes the
  regexp.MustCompile-style wrapper: it panics at construction (not on
  the request path) when EnableXSRFCookie is set without a 32-byte key.

BREAKING (pre-v1.0, stable surface): an app calling CSRFMiddleware with
EnableXSRFCookie: true and no (or non-32-byte) EncryptionKey previously
started with a weak key; it now panics at startup. Migration: set a
32-byte EncryptionKey from the environment / a secret manager. Apps with
EnableXSRFCookie: false (the default) are unaffected. Documented in
CHANGELOG under Changed; no DEP entry needed (no symbol removed/renamed).

Iteration review loop: architect-reviewer PASS (1 WARN), code-reviewer
NITS, security-auditor PASS (2 LOW), contract-guardian PASS — no
blockers. In-scope review fixes applied: dead OriginOnly branch removed,
X-XSRF header guarded, comment accuracy, tamper-rejection tests. Deferred
follow-ups recorded in CURRENT_ITERATION.md (CSRF logger, EncryptionKey
[]byte type, Secure-flag default).

Tests: pkg/router/csrf_hardening_test.go — key validation, panic path,
constant-time accept/reject, XSRF round-trip, tampered-header rejection,
encrypt/decrypt defensive behaviour. Full go test ./... and contract
freeze green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jcsvwinston jcsvwinston merged commit 643aee7 into main May 14, 2026
9 checks passed
jcsvwinston added a commit that referenced this pull request May 14, 2026
* chore(state): close CSRF hardening iteration

Session End Protocol for the CSRF hardening iteration (PR #60, ADR-006).

- Archive the iteration at docs/iterations/2026-05-14-csrf-hardening.md
  — constant-time comparison, mandatory EncryptionKey, NewCSRFMiddleware,
  defensive crypto fixes, the review-loop outcome, and three deferred
  follow-ups.
- Reset CURRENT_ITERATION.md to an empty slate; secrets redaction in
  slog is the top-ranked next candidate.
- Refresh HANDOFF.md: main @ 643aee7, no active iteration.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(observe): redact secrets in the structured logger by default

Closes the secrets-in-logs gap from the 2026-05-14 audit §7 item 6. See
ADR-007.

observe.NewLogger previously built a slog.Handler with no ReplaceAttr —
any code that logged a secret-bearing attribute (authorization,
password, token, a session cookie, …) emitted it verbatim. NewLogger
now redacts: the value of any attribute whose key is in a curated,
case-insensitive denylist is replaced with RedactionPlaceholder
("[REDACTED]"). The key and log-line shape are unchanged. Pure stdlib
(slog.HandlerOptions.ReplaceAttr); no new dependency.

API (additive, contract baseline updated):

- NewLoggerWithRedaction(level, format string, RedactionConfig) — the
  explicit-control constructor. RedactionConfig{Disabled, ExtraKeys,
  Placeholder}.
- DefaultRedactedKeys() — exposes the built-in denylist for auditing.
- RedactionPlaceholder — the default masked value.
- NewLogger keeps its signature and delegates to NewLoggerWithRedaction
  with a zero-value config (redaction on).
- log_redact_extra_keys config key (transitional) threads ExtraKeys
  through App.New.

There is deliberately NO config key to disable redaction — turning it
off requires an explicit code-level opt-out via NewLoggerWithRedaction,
so the decision surfaces in code review (the ADR-004 / WithOpenAuthz
discipline).

BREAKING (pre-v1.0, stable surface): a deployment that intentionally
logged a field under a denylisted key now sees [REDACTED] there.
Documented in CHANGELOG under Changed; contract-guardian confirmed no
DEP entry is needed (no symbol removed/renamed) — same governance trail
as ADR-006.

Review-loop fixes applied in this PR:

- security MED: the auto-generated admin bootstrap password was logged
  under the key "password" — now [REDACTED], which would lock the
  operator out. The password is now written once to stderr, deliberately
  bypassing the logger; the structured log records only that it
  happened. (pkg/app/app.go)
- security MED: expanded the denylist with framework-relevant keys —
  DSN/connection strings (database_url, dsn, redis_url, …), smtp_pass,
  aws_secret_access_key, aws_session_token, private-key-material names,
  provider tokens (oauth_token, github_token, …).
- code-review: ExtraKeys can no longer silence slog's built-in attrs
  (time/level/msg/source) — guarded explicitly so a stray ExtraKeys
  entry cannot break log pipelines.
- godoc: NewLogger documents the key-based-only limitation (msg-string
  interpolation and slog.Any structs are not redacted).

Review loop: architect-reviewer PASS, code-reviewer NITS,
security-auditor PASS (2 MED addressed above), contract-guardian PASS.

Tests: pkg/observe/redact_test.go — default-key redaction, case
-insensitivity, value-type independence, group nesting, WithContext /
With() paths, ExtraKeys, custom placeholder, disabled, built-in
collision guard, DefaultRedactedKeys copy semantics. Full go test ./...
and contract freeze green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jcsvwinston added a commit that referenced this pull request May 14, 2026
Session End Protocol for the CSRF hardening iteration (PR #60, ADR-006).

- Archive the iteration at docs/iterations/2026-05-14-csrf-hardening.md
  — constant-time comparison, mandatory EncryptionKey, NewCSRFMiddleware,
  defensive crypto fixes, the review-loop outcome, and three deferred
  follow-ups.
- Reset CURRENT_ITERATION.md to an empty slate; secrets redaction in
  slog is the top-ranked next candidate.
- Refresh HANDOFF.md: main @ 643aee7, no active iteration.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jcsvwinston jcsvwinston deleted the feature/csrf-hardening branch May 15, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant