security: eliminate OAUTH_STATE_SECRET fallback + fix ASSETS binding

[chitcommit]

Summary

• CSRF fix: Remove all 'default-secret-change-in-production' fallbacks from OAuth routes (wave.ts, google.ts, oauth-state.ts). Routes now return 503 when OAUTH_STATE_SECRET is not configured, preventing CSRF bypass in production.
• ASSETS binding: Add binding = "ASSETS" to [assets] block in both wrangler.toml and deploy/system-wrangler.toml — fixes c.env.ASSETS.fetch() crash on static asset requests.
• Secret deployed: OAUTH_STATE_SECRET set via wrangler secret put for default, staging, and production environments.
• Timing-safe comparison: oauth-state-edge.ts now uses tokenEqual() for constant-time signature verification.

Audit context

Items 4 and 38 from the credential alignment audit (2026-03-25). Both were classified as Immediate priority — active security/availability vulnerabilities in production.

Test plan

• npx tsc --noEmit passes (verified locally)
• Wave OAuth /authorize returns 503 without secret (correct)
• Google OAuth /authorize returns 503 without secret (correct)
• Static asset serving works on finance.chitty.cc after deploy
• No remaining default-secret-change strings in codebase (verified via grep)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • OAuth authentication now requires OAUTH_STATE_SECRET environment variable; missing configuration returns appropriate error responses (503 for authorization, error redirect for callback).

• Bug Fixes

  • Enhanced OAuth state signature validation with timing-safe comparison for improved cryptographic security.

• Chores

  • Updated assets binding configuration in deployment settings.

[github-actions]

@coderabbitai review

Please evaluate:

• Security implications
• Credential exposure risk
• Dependency supply chain concerns
• Breaking API changes

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9f2746b7-1256-4f54-8492-e6693f9ff1ee

📥 Commits

Reviewing files that changed from the base of the PR and between 7afd4d8 and 7accf78.

📒 Files selected for processing (7)

• deploy/system-wrangler.toml
• server/lib/oauth-state-edge.ts
• server/lib/oauth-state.ts
• server/lib/password.ts
• server/routes/google.ts
• server/routes/wave.ts
• wrangler.toml

---

📝 Walkthrough

Walkthrough

The changes enforce OAuth CSRF protection by requiring explicit OAUTH_STATE_SECRET configuration in Wrangler environments instead of hardcoded fallbacks, update token comparison to use constant-time hex string comparison, and configure asset bindings in deployment configurations.

Changes

Cohort / File(s)	Summary
Configuration & Assets deploy/system-wrangler.toml, wrangler.toml	Added OAUTH_STATE_SECRET as required secret in deployment config and configured asset bindings with binding = "ASSETS" in both Wrangler configs.
OAuth State Secret Enforcement server/lib/oauth-state.ts, server/routes/google.ts, server/routes/wave.ts	Changed from hardcoded fallback secrets to requiring c.env.OAUTH_STATE_SECRET. Routes now return 503/redirect errors when secret is missing and pass explicit secrets to OAuth functions.
Token Validation & Comparison server/lib/oauth-state-edge.ts, server/lib/password.ts	Updated OAuth state signature validation to use async tokenEqual() with constant-time hex string comparison instead of bytewise XOR comparison.

Possibly related PRs

• feat: ChittyRegister CI/CD registration pipeline #49 — Modifies Cloudflare Worker deployment configs to add asset bindings and required worker secrets.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A secret emerges from the warren's deep store,
No hardcoded shadows to stumble upon anymore,
With constant-time whispers and bindings so clear,
OAuth flows safer—the path is now dear! 🔐

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch security/oauth-csrf-fix

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cloudflare-workers-and-pages]

Deploying with    Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status	Name	Latest Commit	Updated (UTC)
✅ Deployment successful! View logs	chittyfinance	7accf78	Mar 25 2026, 04:37 AM

[claude]

PR Review: security: eliminate OAUTH_STATE_SECRET fallback + fix ASSETS binding

This is a solid, high-priority security fix. The core changes are correct and well-scoped. A few things to call out:

---

✅ What's good

CSRF fix is correct and necessary. The || 'default-secret-change-in-production' pattern is a classic footgun — it silently degrades to a well-known secret if the env var is missing, defeating CSRF protection entirely. The new 503 behavior is the right call.

tokenEqual refactor in password.ts is sound. The new implementation (hash both inputs → hex → timingSafeEqual) is functionally equivalent to the old byte-by-byte XOR and is arguably cleaner. SHA-256 always produces 32 bytes = 64 hex chars, so timingSafeEqual always sees equal-length strings — constant-time comparison holds.

oauth-state-edge.ts timing-safe comparison is a real improvement. Replacing signature !== expected with await tokenEqual(...) upgrades from a string equality shortcut (timing-leaky) to a constant-time comparison. Good catch.

503 error messaging is descriptive. reason=server_misconfigured on the callback redirect is cleaner than a generic error page.

---

⚠️ Issues

1. oauth-state.ts (Node.js path) still uses direct string comparison — not timing-safe

In server/lib/oauth-state.ts, the validateOAuthState function was updated to guard against a missing secret, but the actual HMAC comparison was not updated to use constant-time comparison. The comparison signature !== expected leaks timing information.

// server/lib/oauth-state.ts — validateOAuthState (not changed in this PR)
const expected = createHmac('sha256', STATE_TOKEN_SECRET)...
if (signature !== expected) { // ← still a timing-leaky string comparison
  return null;
}

This is the Node.js standalone path (not the Workers/Hono path), so production isn't directly affected — but oauth-state.ts could be called during local development or testing. Consider a fix:

import { timingSafeEqual } from 'crypto';

const sigBuf = Buffer.from(signature, 'hex');
const expBuf = Buffer.from(expected, 'hex');
if (sigBuf.length !== expBuf.length || !timingSafeEqual(sigBuf, expBuf)) {
  return null;
}

2. generateOAuthState throws rather than returning a typed error

In oauth-state.ts, generateOAuthState now throws if the secret is missing. Any caller that doesn't wrap it in try/catch will surface an unhandled 500. The Hono routes check c.env.OAUTH_STATE_SECRET before calling generateOAuthState (from the edge version), so production is fine — but any standalone/Express route calling generateOAuthState from oauth-state.ts is at risk of an uncaught exception.

Not a blocker for this PR since standalone is dev-only, but worth a follow-up to harmonize error handling.

3. No automated test coverage for the security-critical path

The test plan is entirely manual. The 503-on-missing-secret behavior is exactly the kind of regression that's easy to re-introduce (e.g., someone adds a fallback "for local dev"). A simple route-level test asserting that GET /api/integrations/wave/authorize returns 503 when OAUTH_STATE_SECRET is unset would protect this permanently.

---

📝 Minor

• The console.warn in oauth-state.ts at module load time is fine for Node.js. In a Worker context, this runs per-isolate-startup rather than per-request, which is the right behavior — just noting it in case this file ever gets imported in the edge bundle.
• The wrangler.toml / deploy/system-wrangler.toml binding = "ASSETS" change is straightforward and correct.

---

Verdict

Approve with follow-up on item 1. The CSRF fix and timing-safe comparison in the edge path are the critical items and both are correct. The Node.js timing-safety gap (oauth-state.ts) is low-risk given standalone is dev-only, but worth fixing in a follow-up to keep the two implementations consistent. This PR should merge.

[Copilot]

Pull request overview

This PR hardens OAuth CSRF protections by removing insecure OAUTH_STATE_SECRET fallbacks and ensures Cloudflare static asset requests don’t crash by correctly binding ASSETS in Wrangler configs.

Changes:

• Remove default-secret-change-in-production fallbacks and fail closed when OAUTH_STATE_SECRET is not configured (Wave + Google OAuth routes; Node oauth-state helper).
• Add [assets] binding = "ASSETS" to Wrangler configs to support c.env.ASSETS.fetch() for static asset serving.
• Use constant-time signature verification in the edge OAuth state validator via tokenEqual().

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
wrangler.toml	Binds static assets to ASSETS Fetcher for worker runtime.
deploy/system-wrangler.toml	Mirrors the ASSETS binding and documents OAUTH_STATE_SECRET as required for OAuth flows.
server/routes/wave.ts	Removes secret fallback; returns 503/redirects when OAuth state secret is missing.
server/routes/google.ts	Removes secret fallback; returns 503/redirects when OAuth state secret is missing.
server/lib/password.ts	Refactors tokenEqual() to reuse the existing constant-time string comparator.
server/lib/oauth-state.ts	Removes fallback secret; adds explicit failure behavior when secret is unset.
server/lib/oauth-state-edge.ts	Switches signature verification to tokenEqual() for timing-safe comparison.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

validateOAuthState() still verifies the HMAC with a direct string comparison (signature !== expectedSignature). That’s not timing-safe and can leak information about the expected signature over many requests. Since this module is used by the Express routes, consider switching to a constant‑time compare (e.g., using the existing tokenEqual() helper or Node’s crypto.timingSafeEqual on decoded bytes).