fix(api): drop CORS credentials + skip same-origin decoration

[EricAndrechek]

Summary

• Drops Access-Control-Allow-Credentials: true from the API CORS middleware (closes Protect CSRF (Cross-Site Request Forgery) #30 — WaveHouse is a Bearer-token API, never used cookies, so credentials mode is unneeded and the prior combination of Allow-Credentials: true + Allow-Origin: * violated the CORS spec).
• Skips CORS header decoration entirely on requests with no Origin (closes CORS Setup (Cross-Origin Resource Sharing) #29's remaining gap — same-origin/server-to-server callers don't need stamped CORS headers).
• Preflight from a disallowed origin returns 204 with no CORS headers — the browser treats that as preflight failure, but we no longer leak the Allow-Methods / Allow-Headers list to origins outside the allowlist.

#29 and #30 were both already most-of-the-way done — config knob, env var, allowlist mode, JWT middleware, no cookies anywhere in the tree. This PR fills the remaining holes and documents the design decision in code, config, and docs so it doesn't drift later.

Why

Reading both issues against the current code:

Issue	Status before this PR
#29 — CORS middleware, config knob, allowlist, dev recipe	All present in corsMiddleware + config.Server.CORSAllowedOrigins + env var WH_SERVER_CORS_ALLOWED_ORIGINS; bug: Allow-Credentials: true paired with Allow-Origin: * is a spec violation browsers reject
#30 — CSRF via Bearer (not cookies)	Already enforced — JWTAuthMiddleware reads Authorization: Bearer … only; no http.Cookie/SetCookie anywhere in internal/ or clients/ts/src/; SDK never sets credentials: 'include'

The credentials fix has two motivations: (a) it removes the spec violation, and (b) it explicitly closes the door on future cookie-based auth being added without thought, which would re-introduce the CSRF surface #30 is trying to keep closed.

What changed

• internal/api/router.go — corsMiddleware rewritten with explicit policy: no Origin → passthrough; wildcard → echo *; allowlist hit → echo origin + Vary: Origin; allowlist miss → no headers (preflight still gets 204). Credentials header dropped across all branches.
• internal/api/router_test.go — existing cases preserved + table-driven TestCORSMiddleware_NoCredentialsHeader (wildcard, empty-allowlist, allowlist-hit) + TestCORSMiddleware_NoOriginIsPassthrough + TestCORSMiddleware_BlockedOriginPreflight.
• config.yaml — comment block explaining the Bearer-token rationale + dev recipe (http://localhost:3000).
• docs/configuration.md, docs/deployment.md — note the credentials decision so operators don't try to "fix" it by re-adding the header.
• CHANGELOG.md — [Unreleased] / Fixed entry.

Test plan

• make verify (tidy + fmt + vulncheck + lint) — clean
• make test-unit — 451 tests, unit coverage 73.4% (gate: 70%)
• make ci — full pipeline (unit + integration + sdk + e2e + merged 80% gate) all green
• Inspected internal/api/stream_ws.go — WS handler's own OriginPatterns allowlist agrees with the middleware policy
• Grepped for Cookie / SetCookie across internal/ and clients/ — none found, confirming the no-cookies invariant

Summary by CodeRabbit

• Bug Fixes

  • CORS behavior tightened: credentials header removed, requests without Origin bypass CORS, disallowed origins receive no CORS headers while responses include Vary: Origin, and disallowed preflight requests return 204 with no CORS headers.

• Documentation

  • Updated config and deployment docs with clearer guidance on allowlist vs wildcard, development vs production examples, and bearer-token API behavior (no cookies).

• Tests

  • Added and strengthened CORS tests to cover these behaviors.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request refines the API's CORS middleware to align with security best practices and technical specifications. By removing credential-related headers and optimizing header decoration for same-origin requests, the changes prevent potential spec violations and reduce the attack surface. The update also clarifies the intended use of the CORS allowlist through updated documentation and robust unit tests.

Highlights

• CORS Middleware Refinement: Removed 'Access-Control-Allow-Credentials' header to comply with CORS specifications and improve security for this Bearer-token based API.
• Same-Origin Optimization: Updated the middleware to skip CORS header decoration entirely for requests lacking an 'Origin' header, reducing unnecessary response noise.
• Preflight Security: Modified preflight handling to return a 204 status without CORS headers for disallowed origins, preventing the leakage of internal method and header lists.
• Documentation and Testing: Added comprehensive documentation in config and docs files, alongside new table-driven tests to verify the updated CORS behavior.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[claude]

0 [MUST], 0 [SHOULD], 1 [MAY] — see the inline thread for detail.

The CORS rewrite is correct and complete. Quick rundown of what I checked:

• Correctness: The allowAll sentinel, the continue that keeps "*" out of allowedSet, the origin == "" early-return, the allowed bool gating the methods/headers block, and the unconditional OPTIONS 204 short-circuit all compose correctly. Disallowed origins get no CORS headers on any method; allowed origins get the full set; no-Origin requests are untouched.
• Security (NONE): Dropping Access-Control-Allow-Credentials fixes a real CORS spec violation (wildcard origin + credentials = browser rejection). No new surface introduced. The t.Fatal guard in TestCORSMiddleware_BlockedOriginPreflight will catch a future refactor that accidentally moves the OPTIONS short-circuit inside the if allowed branch.
• Doc sync: docs/configuration.md, docs/deployment.md, config.yaml, and CHANGELOG.md are all updated. The one wrinkle — a docs/development.md typo in the CHANGELOG file list where it should read docs/deployment.md — is flagged inline as [MAY].
• Tests: Table-driven no-credentials test (wildcard / empty-allowlist / allowlist-hit), no-Origin passthrough test, and blocked-origin preflight test together give exhaustive coverage of the three new behavior branches.

Ship it — fix the one-word CHANGELOG typo (already suggested inline) and merge.

@claude

[gemini-code-assist]

Code Review

This pull request refactors the CORS middleware to improve spec compliance and security by removing the Access-Control-Allow-Credentials header, skipping CORS decoration for same-origin requests, and short-circuiting preflights for disallowed origins. Documentation and tests have been updated to reflect these changes. Feedback focuses on ensuring these architectural shifts are recorded in AGENTS.md, improving cache consistency by sending the Vary: Origin header even for disallowed origins, and utilizing t.Context() in tests for better lifecycle management.

[claude]

1 [MUST], 0 [SHOULD], 0 [MAY] — see the inline thread for detail.

The CORS rewrite is correct and well-tested. The implementation (corsMiddleware rewrite, allowAll sentinel, continue preventing "*" from entering allowedSet, early-return for no-Origin, allowed gating the methods/headers block, unconditional OPTIONS 204 short-circuit), the security rationale (spec violation removed, credentials header gone, CSRF surface documented), and the four new table-driven test functions are all solid. Doc sync to docs/configuration.md, docs/deployment.md, and config.yaml is complete.

One issue was missed in my first pass (6bdd4cac): the CHANGELOG.md entry is garbled — the old Coverage badge fix bullet was replaced in-place rather than a new bullet being inserted, so the coverage-badge text was spliced onto the end of the CORS entry and the standalone coverage-badge record was deleted from [Unreleased]. Details in the inline thread on CHANGELOG.md line 14.

Iterate — truncate the CORS CHANGELOG entry at the dev-recipe sentence and restore the coverage-badge bullet as its own line.

(Supersedes my earlier Ship-it verdict at #issuecomment-4435405141, which reviewed commit 6bdd4cac and missed the garbled entry.)

[claude]

1 [MUST], 0 [SHOULD], 0 [MAY] — see the inline thread for detail.

The implementation at HEAD (b9de0a9) is correct. The Vary: Origin fix on allowlist rejects (addressing Gemini's caching concern), AGENTS.md item #15 documenting the Bearer-token CORS posture, and the four test functions are all solid. No new issues in this push.

The sole outstanding item is the garbled CHANGELOG.md entry, re-flagged inline. The coverage-badge text from the old 4d4a3c4a fix is still appended to the CORS bullet, and the standalone coverage-badge record is still missing from [Unreleased]. The inline suggestion splits the line at the correct boundary.

Iterate — apply the one-line CHANGELOG suggestion and this is done.

@claude

[claude]

0 [MUST], 0 [SHOULD], 0 [MAY]

The f72a5ada push resolves the one outstanding [MUST] (garbled CHANGELOG — the CORS bullet and coverage-badge bullet are now properly split into separate entries). No new issues in this push.

Full implementation summary for the record:

• Correctness: All branches correct — allowAll sentinel, continue keeping "*" out of allowedSet, origin == "" early-return for non-browser callers, Vary: Origin on both allowlist hits and rejects (shared-cache safety), credentials header unconditionally absent, OPTIONS 204 short-circuit fires for all origins including disallowed ones.
• Security (NONE): Spec violation fixed (wildcard + credentials → browser rejection). No new attack surface.
• Tests: Four new functions (TestCORSMiddleware_NoCredentialsHeader table-driven / TestCORSMiddleware_NoOriginIsPassthrough / TestCORSMiddleware_BlockedOriginPreflight / extended TestCORSMiddleware_BlockedOrigin) give exhaustive branch coverage.
• Doc sync: AGENTS.md item 15, CHANGELOG.md, config.yaml, docs/configuration.md, docs/deployment.md all updated.

Ship it — ready to merge.

(Supersedes Iterate verdict at #issuecomment-4435483441.)

@claude

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 53a5a798-6565-4f1d-bc9c-f4c5c18b182a

📥 Commits

Reviewing files that changed from the base of the PR and between 2107b8e and 4f1cf07.

📒 Files selected for processing (6)

• AGENTS.md
• CHANGELOG.md
• config.yaml
• docs/src/content/docs/configuration.md
• docs/src/content/docs/deployment.md
• internal/api/router.go

---

📝 Walkthrough

Walkthrough

WaveHouse CORS middleware refactored to enforce a Bearer-token-only authentication posture by removing the Access-Control-Allow-Credentials header, skipping CORS decoration for requests lacking an Origin header, setting Vary: Origin in allowlist mode, and properly handling origin validation and wildcard configurations. Test coverage expanded and documentation updated.

Changes

CORS Bearer-Token-Only Enforcement

Layer / File(s)	Summary
CORS Middleware Behavior Refactor internal/api/router.go	Updated corsMiddleware to remove Access-Control-Allow-Credentials, bypass CORS decoration for non-CORS requests (no Origin header), set Vary: Origin unconditionally in allowlist mode, conditionally emit CORS allow headers only when origin is accepted, treat "*" as allowAll without inserting it into the allow set, and short-circuit disallowed OPTIONS with 204 (omitting CORS headers).
CORS Middleware Test Coverage internal/api/router_test.go	Enhanced TestCORSMiddleware_BlockedOrigin to assert rejected origins receive no Access-Control-Allow-Methods while Vary: Origin is still emitted. Added TestCORSMiddleware_NoCredentialsHeader verifying credentials header is never set across wildcard, empty, and allowlist-hit configs. Added TestCORSMiddleware_NoOriginIsPassthrough ensuring requests without Origin header receive no CORS headers. Added TestCORSMiddleware_BlockedOriginPreflight verifying blocked-origin OPTIONS returns 204 No Content without invoking the handler.
CORS Documentation and Design Rationale AGENTS.md, CHANGELOG.md, config.yaml, docs/src/content/docs/configuration.md, docs/src/content/docs/deployment.md	Documented the Bearer-token-only CORS posture (agent design decision #16), clarified cors_allowed_origins/WH_SERVER_CORS_ALLOWED_ORIGINS semantics and * behavior, added config comments and deployment docs noting that cookies are not used and Access-Control-Allow-Credentials is intentionally omitted.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I nibble through headers with careful delight,
Tokens in pockets, no cookies in sight.
Origins inspected, Vary kept in view,
OPTIONS may pause, but responses stay true.
Hops of approval — secure skies, good night.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and concisely summarizes the two main changes: removing CORS credentials and skipping CORS decoration for same-origin requests without Origin headers.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 fix/cors-credentials-wildcard

---

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

[claude]

0 [MUST], 0 [SHOULD], 0 [MAY]

Re-review of HEAD bab51eb (merge of origin/main into the PR branch). No new issues introduced.

Everything from the prior f72a5ada review remains correct: corsMiddleware rewrite (allowAll sentinel, continue keeping "*" out of allowedSet, early-return on no-Origin, Vary: Origin on allowlist hits and rejects, credentials header unconditionally absent, OPTIONS 204 short-circuit), four comprehensive test functions, and complete doc sync across AGENTS.md / CHANGELOG.md / config.yaml / docs/configuration.md / docs/deployment.md.

The merge of main brought in no conflicting changes to the CORS path. All prior inline threads are resolved.

Ship it — ready to merge.

@claude

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/api/router_test.go`:
- Around line 184-200: The TestCORSMiddleware_BlockedOriginPreflight currently
only asserts Access-Control-Allow-Origin is empty; update the test
(TestCORSMiddleware_BlockedOriginPreflight) that uses corsMiddleware to also
assert that all other CORS response headers are absent/empty — e.g. assert.Empty
for "Access-Control-Allow-Methods", "Access-Control-Allow-Headers",
"Access-Control-Allow-Credentials" (and optionally "Access-Control-Max-Age") in
addition to the existing assertions so the test pins the full "no CORS headers"
contract.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f5b3ef91-3ef2-4c0f-8ad4-3f90034fce22

📥 Commits

Reviewing files that changed from the base of the PR and between c22780d and bab51eb.

📒 Files selected for processing (7)

• AGENTS.md
• CHANGELOG.md
• config.yaml
• docs/src/content/docs/configuration.md
• docs/src/content/docs/deployment.md
• internal/api/router.go
• internal/api/router_test.go

[claude]

0 [MUST], 0 [SHOULD], 0 [MAY]

Re-review of HEAD 2107b8eb (full merge-base diff reviewed).

Quick summary of what I verified:

• Correctness — All branches compose correctly: allowAll sentinel initialised from empty-slice, continue keeps "*" out of allowedSet, origin == "" early-return for non-browser callers, Vary: Origin on both allowlist hits and rejects (shared-cache coherence), allowed bool correctly gates the methods/headers block, OPTIONS 204 short-circuit fires unconditionally before next.ServeHTTP for all origins including disallowed ones.
• Security (NONE) — Net improvement: spec violation fixed (wildcard + credentials = browser rejection), Allow-Methods/Allow-Headers/etc. no longer leaked to disallowed origins on preflight, no new attack surface.
• Testing — Four new functions give exhaustive branch coverage. The CodeRabbit CHANGES_REQUESTED concern (full no-CORS-headers contract on blocked-origin preflight) is addressed by 2107b8eb — the test now pins all six CORS response headers plus Vary: Origin on the reject path.
• Doc sync — AGENTS.md item 15, CHANGELOG.md, config.yaml, docs/configuration.md, docs/deployment.md all updated. docs/architecture.md and docs/api.md don't need changes — this is a behaviour fix inside an existing middleware component, not a new package or endpoint.

Ship it — CodeRabbit's CHANGES_REQUESTED is resolved; no remaining issues.

@claude

[claude]

0 [MUST], 0 [SHOULD], 0 [MAY] — no inline threads.

Ship it — CORS spec-compliance fix is correct, Vary: Origin cache-coherence invariant is right for allowlist mode (set on both hits and rejects), wildcard mode correctly omits Vary, and test coverage is thorough. Documentation sync is complete per AGENTS.md §Documentation & Consistency Sync. CodeRabbit's earlier CHANGES_REQUESTED (full CORS header assertions in TestCORSMiddleware_BlockedOriginPreflight) is addressed in the final diff.

[taitelee]

Good. Good catch on context life cycle.