fix(middleware): Cors no longer emits raw comma list as Access-Control-Allow-Origin#2725
Conversation
Wheels Bot — Docs updatedAdded a doc commit to this PR:
|
![wheels-bot[bot]](https://avatars.githubusercontent.com/in/3657588?s=60&v=4){kind=small}
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly fixes a real CORS spec violation — a multi-origin allowOrigins comma list was flowing through as the raw Access-Control-Allow-Origin value on requests without an Origin header. The extraction into $resolveAllowOrigin() is clean and the new BDD specs cover the key edge cases. No cross-engine, security, or commitlint issues. Two non-blocking findings: a missing Vary: Origin header (pre-existing but this PR is the right place to fix it) and a small gap in integration-level test coverage for the regression scenario. Verdict: comment — no blocking issues, but the Vary concern is worth tracking.
Correctness
Finding 1: Missing Vary: Origin header — pre-existing, but worth fixing here
File: vendor/wheels/middleware/Cors.cfc, lines 77-87
if (Len(local.allowOrigin)) {
try {
cfheader(name = "Access-Control-Allow-Origin", value = local.allowOrigin);
cfheader(name = "Access-Control-Allow-Methods", value = variables.allowMethods);
...
} catch (any e) {
}
}When allowOrigin is a specific reflected origin (i.e., not "*"), the CORS spec (Fetch §3.2.3) requires the response to carry Vary: Origin. Without it, a shared CDN/reverse-proxy can cache the response with Access-Control-Allow-Origin: https://portal.pai.com and serve it to a subsequent request from https://portal.paiindustries.com, which the browser will then block. This bug predates this PR, but since the PR already touches the header-emission block and the PR body explicitly mentions "can poison CDN/proxy caches", this is a natural spot to add it.
Suggested addition inside the same try block, after the Access-Control-Allow-Origin line, conditional on non-wildcard:
if (local.allowOrigin != "*") {
cfheader(name = "Vary", value = "Origin");
}Finding 2: PR body is slightly misleading about wildcard behavior
The summary states "same-origin and S2S responses no longer carry it at all." This is true when allowOrigins is a specific origin or comma-list. However, when allowOrigins = "*", $resolveAllowOrigin("") returns "*" (line 54-56), so requests without an Origin header still receive Access-Control-Allow-Origin: *. This matches the old behavior (the original code defaulted local.allowOrigin = variables.allowOrigins which was already "*"), so it is not a regression, but the description overpromises. The code is fine — only the PR body needs a qualifier.
Tests
Finding 3: No integration test for the primary regression scenario
The new specs (lines 85-126 in CorsSpec.cfc) call $resolveAllowOrigin() directly — good for unit coverage — but there is no end-to-end test that drives the regression through handle():
- No Origin header + comma-list
allowOrigins→handle()does NOT set theAccess-Control-Allow-Originresponse header.
The existing integration-style tests (lines 10-83) verify the handle() return value but do not inspect headers. Since cfheader calls in test mode do not write real HTTP headers, a mock-based approach (overriding $setHeader or capturing headers via a test-helper wrapper) or at minimum a comment explaining why unit coverage of $resolveAllowOrigin is sufficient would close this gap. Without it, a future refactor that breaks the wiring between handle() and $resolveAllowOrigin() might not be caught by this test suite.
Commits
Both commits conform to commitlint rules:
| SHA | Subject | Type | Scope | DCO |
|---|---|---|---|---|
48a4030 |
fix(middleware): Cors no longer emits raw comma list as Access-Control-Allow-Origin |
fix |
middleware |
present |
4f80380 |
docs(web/guides): note that requests without Origin header omit Access-Control-Allow-Origin |
docs |
web/guides |
present |
No issues.
Docs
Both doc versions correctly updated:
web/sites/guides/src/content/docs/v4-0-0/digging-deeper/cors.mdxweb/sites/guides/src/content/docs/v4-0-1-snapshot/digging-deeper/cors.mdx
The added bullet point under the troubleshooting checklist is accurate and helpful. No issues.
Summary
The core change is correct and well-scoped. The two actionable items:
- Add
Vary: Originwhen emitting a specific (non-wildcard) origin — prevents cache poisoning, which the PR body itself calls out as a concern. - Consider an integration-level test through
handle()for the comma-list + no-Origin regression path to guard the wiring, not just the logic.
Neither is blocking for a bug-fix PR, but (1) in particular would be a cheap addition that completes the correctness story the PR body promises.
Wheels Bot — Reviewer B (round 1)A's review is solid on all three findings. No sycophancy, no false positives, no missed issues on the diff I re-read. Verdict alignment holds. SycophancyNone detected. A's "comment" verdict is earned, not given — it names specific findings and explicitly marks neither as blocking. False positivesNone detected.
Missed issuesNone detected. Cross-engine compatibility is clean — Verdict alignment"Comment" is consistent with two non-blocking findings. A does not approve over gaps — both are named and present — but correctly judges that neither warrants blocking a well-scoped bug fix. ConvergenceAligned. A's findings are accurate and appropriately weighted. The |
bpamiri
force-pushed
the
fix/bot-2704-wheels-middleware-cors-emits-raw-comma-list-as-acc
branch
from
2c74384 to
5e788e8
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is an incremental update to a CORS bug-fix PR whose substantive changes (the $resolveAllowOrigin() extraction and BDD specs) were already reviewed in the prior Reviewer A pass. The only material change since that review is a CI-automated visual baseline refresh commit (5e788e80d), unrelated to the CORS fix itself. The previous substantive findings were properly addressed: the PR body now clarifies wildcard behavior, and the Vary: Origin gap is correctly deferred to PR #2724. No correctness, cross-engine, or security issues in either the original change or the new commit. Verdict: comment — one minor housekeeping note on the new commit.
Commits
Finding: chore(web): refresh visual baseline(s) (blog) — no DCO Signed-off-by: trailer
The commit 5e788e80d was authored by github-actions[bot] via .github/workflows/refresh-visual-baselines.yml and has no Signed-off-by: trailer:
commit 5e788e80d21f438f627663d7dc7ec8d305aa50c5
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
chore(web): refresh visual baseline(s) (blog)
The DCO App exempts bots if the repo's .github/dco.yml includes allowRemediationCommits: true or an explicit bot allowlist. If the DCO check goes red on this commit, the fix is to configure the DCO App to exempt github-actions[bot], not to push a manual Signed-off-by: from a human.
Separate concern — unrelated commit in a bug-fix PR: a visual baseline update for the blog page has no logical connection to a CORS header emission fix. It should land on develop independently or be targeted at a visual-regression sweep PR. This is a housekeeping nit, not a merge blocker, but it muddies git log --oneline for the CORS topic.
The other two commits (48a403058, 4f8038014) conform to commitlint and carry DCO sign-offs — no issues there.
Prior findings — resolved
| Finding | Disposition |
|---|---|
A1: Missing Vary: Origin |
Tracked in #2707, addressed in #2724. Acceptable deferral for a focused bug-fix PR. |
| A2: PR body overpromised on wildcard behavior | Addressed in the updated Summary section. Code unchanged. |
A3: No handle()-level integration test for regression path |
Acknowledged: cfheader() in test mode does not write real HTTP headers; $resolveAllowOrigin() unit coverage is the assertable surface. Reviewer B concurred. Accepted. |
Everything else — clean
- Correctness:
$resolveAllowOrigin()correctly returns""for a comma-list config with noOriginheader,"*"for wildcard, and the reflected origin when matched. The wiring inhandle()(line 75) and the guard at line 77 (if (Len(local.allowOrigin))) are correct. - Cross-engine: No
struct.map()collisions, noapplicationscope function members, no bracket-notation calls, noclientscope, noLeft(str, 0). All code paths use vanilla CFML. - Mixin access:
$resolveAllowOrigin()is declaredpublicwith$prefix — correct for a standalone instantiated component where tests need to call it directly. Not a mixin CFC, so theprivate-integration rule (CLAUDE.md §10) does not apply. - Tests: BDD syntax throughout (
describe/it/expect). Six new assertions in theAccess-Control-Allow-Origin header resolutionblock cover all meaningful combinations of the absent-Origin path. - Docs: Both
v4-0-0andv4-0-1-snapshotCORS guides updated with the accurate troubleshooting bullet. - Security: The fix tightens security — stops emitting a spec-violating multi-origin header value that could have been cached and replayed incorrectly by proxies.
Reviewer A round 2 — dispositionThanks for the re-review on
CI is now fully green on |
Wheels Bot -- Reviewer B (round 1)A's review is accurate for the new HEAD SHA. The two findings (DCO trailer on the SycophancyNone detected. A's 'comment' verdict is earned: A identifies two concrete issues and does not rubber-stamp the PR despite the prior substantive review having already converged-approve. False positivesNone detected.
Missed issuesNone detected. A's cross-engine check is thorough: Verdict alignment'Comment' is consistent with the findings. Neither the DCO concern (conditional, repo-config resolution) nor the unrelated commit (nit) warrants 'request-changes' on an otherwise correct and well-tested fix. ConvergenceAligned. A's two findings are accurate and appropriately non-blocking. The core CORS fix is correct, cross-engine clean, covered by six targeted unit tests, and documented in both snapshot guides. The DCO concern is a repo-configuration matter rather than a PR-code matter; maintainers can address it via |
bpamiri
force-pushed
the
fix/bot-2704-wheels-middleware-cors-emits-raw-comma-list-as-acc
branch
from
5e788e8 to
e14f59d
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is the third Reviewer A pass on PR #2725, which fixes a CORS spec violation in wheels.middleware.Cors. The only change since the second pass is removal of the unrelated chore(web): refresh visual baseline(s) commit (5e788e80d) that was flagged as out-of-scope. The branch now contains exactly two commits — the fix(middleware) implementation and the docs(web/guides) troubleshooting note — both reviewed and found clean in prior passes. All findings from both previous reviews have been addressed or explicitly deferred. No correctness, cross-engine, or security issues remain. Verdict: approve (self-approval blocked by GitHub; human maintainer approval unblocked).
Prior findings — final disposition
| Finding | Pass | Status |
|---|---|---|
A1: Missing Vary: Origin header |
A/B round 1 | Deferred to #2724. Acceptable for a focused bug-fix PR. |
| A2: PR body overpromised on wildcard behavior | A round 1 | Addressed — wildcard qualifier added to Summary. |
A3: No handle()-level integration test for regression path |
A round 1 | Accepted — cfheader() does not write real HTTP headers in test mode; $resolveAllowOrigin() unit coverage is the assertable surface. |
| B1: Unrelated visual-baseline commit with no DCO sign-off | A round 2 | Resolved — 5e788e80d (chore(web): refresh visual baseline(s) (blog)) removed from the branch. |
Current state — clean
Correctness: $resolveAllowOrigin() returns "" for a comma-list config with no Origin header, "*" for wildcard, and the reflected origin when matched. The wiring in handle() (line 75) and the guard at line 77 (if (Len(local.allowOrigin))) are correct.
Cross-engine: All code uses vanilla CFML (Len(), ListFindNoCase(), StructKeyExists()). No struct.map() collisions, no application scope members, no client scope, no bracket-notation calls, no Left(str, 0). $resolveAllowOrigin() is declared public with $ prefix — correct access level for a standalone instantiated component (the private-integration rule in CLAUDE.md applies to mixin CFCs injected via $integrateComponents(), not to middleware components).
Tests: Six new BDD specs under describe("Access-Control-Allow-Origin header resolution") cover all meaningful combinations of the absent-Origin path. Syntax is correct (describe/it/expect, extending wheels.WheelsTest).
Commits: Both commits conform to commitlint, carry DCO Signed-off-by: trailers, and no unrelated commits remain on the branch.
Docs: Both v4-0-0 and v4-0-1-snapshot CORS guides updated with the accurate troubleshooting bullet.
Security: The fix tightens security — stops emitting a spec-violating multi-origin header value that could be cached and incorrectly replayed by proxies.
Wheels Bot — Reviewer B (round 1)A's third-pass review is accurate and appropriately scoped. All prior findings are correctly marked resolved or deferred. No sycophancy, no false positives, no missed issues on the diff. Aligning on approve. SycophancyNone detected. A's approve verdict is earned: every prior finding is dispositioned with evidence, not waved away. False positivesNone detected. I verified the key claims against the diff:
Missed issuesNone detected. Cross-engine compat is clean: Verdict alignment"Approve" (expressed under a ConvergenceAligned, no changes needed. The fix is correct, cross-engine clean, covered by six targeted unit tests, and documented in both snapshot guides. Joint recommendation: approve and merge. |
…l-Allow-Origin When `allowOrigins` was configured with a comma-delimited list and the request arrived with no `Origin` header (same-origin, server-to-server, curl-without-`-H`), the default `local.allowOrigin = variables.allowOrigins` flowed the raw list through to the response header, violating the CORS spec requirement that `Access-Control-Allow-Origin` be a single origin or `*`. Extract origin resolution into a pure `$resolveAllowOrigin()` method that returns `"*"` for wildcard configs, the reflected origin when the request `Origin` is in the allowlist, or `""` otherwise. The header is now only emitted when there is a value to emit — same-origin and S2S responses no longer carry it at all. Fixes #2704 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
bpamiri
force-pushed
the
fix/bot-2704-wheels-middleware-cors-emits-raw-comma-list-as-acc
branch
from
e14f59d to
40d5502
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is the fourth Reviewer A pass on PR #2725. The branch was force-pushed to rebase onto develop after PR #2724 (Vary: Origin fix) merged — the only reason the head SHA changed from the third-pass SHA (e14f59dbd). The two remaining commits are byte-identical in diff to those reviewed and passed in the third pass. All prior findings are closed. No correctness, cross-engine, or security issues. Verdict: approve (GitHub blocks self-approval by the bot; this unblocks a human maintainer merge).
Prior findings — final disposition
| Finding | Origin | Status |
|---|---|---|
| A1: Missing Vary: Origin header | Pass 1 | Resolved — #2724 merged into develop; $headersFor() now emits Vary: Origin when local.reflected is true. |
| A2: PR body overpromised on wildcard behavior | Pass 1 | Resolved — Summary qualifier added in pass 2. |
A3: No handle()-level integration test for regression path |
Pass 1 | Accepted — cfheader() does not write real HTTP headers in test mode; $resolveAllowOrigin() unit coverage is the assertable surface. Reviewer B concurred. |
| B1: Unrelated visual-baseline commit with no DCO sign-off | Pass 2 | Resolved — commit 5e788e80d removed; branch now has exactly two commits. |
Current state — clean
Correctness: $resolveAllowOrigin() correctly handles all three cases:
allowOrigins == "*"— returns"*"(wildcard, no Origin needed)- non-empty
requestOriginin the comma-list — returns the reflected origin - anything else (no Origin header, disallowed origin, empty allowlist) — returns
""
The guard if (Len(local.allowOrigin)) in $headersFor() ensures the header is only emitted when there is a value to emit. The wiring from handle() through $headersFor() through $resolveAllowOrigin() is correct.
Cross-engine: All code paths use Len(), ListFindNoCase(), StructKeyExists() — vanilla CFML with no engine-specific gotchas. No struct.map() member-function collision, no application scope members, no client scope, no bracket-notation function calls, no Left(str, 0). $resolveAllowOrigin() is declared public with $ prefix — correct for a standalone instantiated middleware component (the private-integration rule in CLAUDE.md applies to mixin CFCs injected via $integrateComponents(), not to middleware).
Tests: Six new BDD specs under describe("Access-Control-Allow-Origin header resolution") cover the regression case (comma-list + no Origin => empty string), single-origin + no Origin, matched origin, non-matched origin, wildcard (both present and absent Origin), and empty allowlist. Syntax is correct (describe/it/expect, extending wheels.WheelsTest). No RocketUnit legacy syntax.
Commits:
| SHA | Subject | Type | Scope | DCO |
|---|---|---|---|---|
8878dd55d |
fix(middleware): Cors no longer emits raw comma list as Access-Control-Allow-Origin |
fix |
middleware |
present |
40d550223 |
docs(web/guides): note that requests without Origin header omit Access-Control-Allow-Origin |
docs |
web/guides |
present |
Both conform to commitlint, carry DCO Signed-off-by: trailers, and are scoped appropriately.
Docs: Both v4-0-0 and v4-0-1-snapshot CORS guides updated with the "No Origin header, no CORS header" troubleshooting bullet. CHANGELOG [Unreleased] / Fixed entry is accurate.
Security: The fix tightens the spec-compliance surface — stops emitting a multi-origin string that violates the CORS spec and could be cached and incorrectly replayed by CDN/proxy intermediaries.
Wheels Bot — Reviewer B (round 1)A's fourth-pass review is accurate on all code, test, and cross-engine claims. One missed issue: the docs commit carries the same wildcard-overpromise that A caught and fixed in the PR body (A2, pass 1), but the guide bullet was not qualified to match. Everything else checks out; the code is correct and the tests are solid. SycophancyNone detected. A's approve verdict (expressed as COMMENTED due to GitHub's bot self-approval block) is earned: every prior finding is dispositioned with evidence, the two commits are verified, and A does not rubber-stamp. False positivesNone detected. I verified A's key claims against the diff:
Missed issuesBoth guide files — wildcard qualifier missing from docs bullet A found and fixed the identical issue in the PR body (A2, pass 1): "PR body overpromised — 'same-origin and S2S responses no longer carry it at all' is only true for specific-origin configurations." That qualifier was added to the PR Summary. The docs commit (
Tracing the code: for The fix is a one-clause qualifier, e.g.: "...never receive an Note: prior B passes reviewed these guide files and did not catch this. That was an oversight on those rounds; flagging it now is the right call regardless. Verdict alignmentA's approve verdict is consistent with finding no blocking code or test issues. The docs inaccuracy is the only open item. ConvergenceNot aligned. The wildcard qualifier issue is concrete, fixable, and directly analogous to A2 which A already proved it could catch and fix. Once A adds the qualifier to both guide files, there are no remaining open items and B will converge-approve. |
…s-Control-Allow-Origin Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
bpamiri
force-pushed
the
fix/bot-2704-wheels-middleware-cors-emits-raw-comma-list-as-acc
branch
from
40d5502 to
60eb197
Compare
There was a problem hiding this comment.
Wheels Bot — Reviewer A response (round 1)
B's critique on this SHA is correct and I concede the missed issue. The docs bullet introduced by commit 40d550223 states that no-Origin requests "never receive an Access-Control-Allow-Origin response header" — but that claim is false for allowOrigins=\"*\" configurations. I verified this against the diff: $resolveAllowOrigin() lines 46-48 return \"*\" unconditionally when variables.allowOrigins == \"*\", with no guard on arguments.requestOrigin. The new test at lines 165-170 of CorsSpec.cfc ("returns '*' when allowOrigins is wildcard, regardless of Origin header presence") confirms this explicitly by asserting both $resolveAllowOrigin(\"\") and $resolveAllowOrigin(\"https://anything.com\") return \"*\". I missed this in all four review passes despite having caught the identical overpromise in the PR body summary (A2, pass 1). B was right to escalate it.
Conceded points
- B: both guide files use "never" which is false for wildcard configs. Confirmed. The troubleshooting bullet in both
v4-0-0/digging-deeper/cors.mdxandv4-0-1-snapshot/digging-deeper/cors.mdxreads: "Requests that arrive without anOriginheader ... never receive anAccess-Control-Allow-Originresponse header." ForallowOrigins=\"*\",$resolveAllowOrigin(\"\")returns\"*\",Len(\"*\")is truthy, andAccess-Control-Allow-Origin: *is emitted regardless of whether the request carries anOriginheader. The "never" is only true for specific-origin and comma-list configurations. The fix B suggests is correct: add a qualifier such as "(except onallowOrigins=\"*\"configurations, which always emitAccess-Control-Allow-Origin: *)" to both guide files. This is directly analogous to A2 from pass 1, which I caught in the PR body but failed to apply consistently to the docs.
Defended points
None. B raised no false-positive claims against prior A findings on this SHA. The only issue B raised is the missed docs inaccuracy, which is a genuine concession.
Updated verdict
Request changes — the "never" qualifier in both guide files (v4-0-0 and v4-0-1-snapshot) must be corrected to scope the claim to non-wildcard configurations before this PR can be approved. The code and tests are correct; this is a documentation accuracy issue only, but it is concrete and fixable.
Wheels Bot — Reviewer B (round 1)A's concession on the docs inaccuracy is correct, but the "request changes" verdict is stale: the fix A describes as needed is already present at the current HEAD. Both guide files in the current PR diff already include the wildcard qualifier. Converging on approve. SycophancyNone detected. A correctly conceded B's finding without hedging. False positivesOne — A's verdict. A's review was submitted at Both A's concession about what was wrong is accurate; the "request changes" call does not reflect the actual state of the PR at the commit ID the review was submitted against. Missed issuesNone detected. Code and tests remain correct and unchanged from prior passing reviews. Verdict alignmentA's "request changes" verdict is inconsistent with the current PR state. The change A is requesting is already applied. ConvergenceAligned, no changes needed. The code fix is correct, the six BDD specs cover the regression path, and both guide files now correctly qualify the wildcard behavior. There are no open findings. Joint recommendation: approve and merge. |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This is the fifth Reviewer A pass on PR #2725. The only change since the fourth pass (SHA 40d55022318e1d9966633e854295e8da02b36b43) is a docs amendment to commit 60eb19782 that addresses the single outstanding "request changes" finding: the troubleshooting bullet in both guide files now correctly qualifies the "never receive an Access-Control-Allow-Origin response header" claim to non-wildcard configurations and explicitly calls out the wildcard exception. The conceded finding from the Reviewer A round-1 response is resolved. All prior findings are closed. No correctness, cross-engine, or security issues remain. Verdict: approve (GitHub blocks self-approval; this unblocks a human maintainer merge).
Prior findings — final disposition
| Finding | Pass | Status |
|---|---|---|
A1: Missing Vary: Origin header |
A/B round 1 | Resolved — #2724 merged into develop. |
| A2: PR body overpromised on wildcard behavior | A round 1 | Resolved — wildcard qualifier added to Summary. |
A3: No handle()-level integration test for regression path |
A round 1 | Accepted — cfheader() does not write real HTTP headers in test mode; $resolveAllowOrigin() unit coverage is the assertable surface. |
| B1: Unrelated visual-baseline commit with no DCO sign-off | A round 2 | Resolved — commit 5e788e80d removed from the branch. |
| A-response round 1: "never" in guide files inaccurate for wildcard configs | A round 5 (this pass) | Resolved — see below. |
Correctness
Resolved: wildcard qualifier in both guide files
Both v4-0-0/digging-deeper/cors.mdx and v4-0-1-snapshot/digging-deeper/cors.mdx now read:
On specific-origin and comma-list
allowOriginsconfigurations, requests that arrive without anOriginheader — same-origin browser navigation, server-to-server calls, andcurlwithout-H "Origin: ..."— never receive anAccess-Control-Allow-Originresponse header. This is correct CORS spec behavior. (Wildcard configurationsallowOrigins="*"are the exception: they emitAccess-Control-Allow-Origin: *on every response regardless of whetherOriginis set.)
The "never" is now correctly scoped to non-wildcard configurations. The parenthetical exception is accurate: $resolveAllowOrigin("") returns "*" unconditionally when variables.allowOrigins == "*" (Cors.cfc lines 47-49), and the Len(local.allowOrigin) guard at line 94 is truthy for "*", so the header is emitted on every wildcard response regardless of the incoming Origin. The guide and code are consistent.
Current state — clean
Correctness: $resolveAllowOrigin() handles all three cases correctly: wildcard → "*"; matching listed origin → reflected origin; everything else → "". The local.reflected recomputation at Cors.cfc line 94 (Len(local.allowOrigin) && local.allowOrigin != "*") correctly identifies when a specific origin is being reflected, preserving the Vary: Origin emission introduced by #2724.
Cross-engine: All code paths use Len(), ListFindNoCase(), StructKeyExists() — vanilla CFML with no engine-specific gotchas. No struct.map() member-function collision, no application scope members, no client scope, no bracket-notation function calls, no Left(str, 0). $resolveAllowOrigin() is declared public with $ prefix — correct for a standalone instantiated middleware component (the private-integration rule in CLAUDE.md applies to mixin CFCs injected via $integrateComponents(), not to middleware components).
Tests: Six new BDD specs under describe("Access-Control-Allow-Origin header resolution") cover all meaningful paths: comma-list + no Origin, single origin + no Origin, matched origin, non-matched origin, wildcard (both present and absent Origin), and empty allowlist. Syntax is correct (describe/it/expect, extending wheels.WheelsTest). No RocketUnit legacy syntax.
Commits:
| SHA | Subject | Type | Scope | DCO |
|---|---|---|---|---|
8878dd55d |
fix(middleware): Cors no longer emits raw comma list as Access-Control-Allow-Origin |
fix |
middleware |
present |
60eb19782 |
docs(web/guides): note that requests without Origin header omit Access-Control-Allow-Origin |
docs |
web/guides |
present |
Both conform to commitlint, carry DCO Signed-off-by: trailers, and are scoped appropriately. No unrelated commits on the branch.
Docs: Both v4-0-0 and v4-0-1-snapshot CORS guides updated with the accurate troubleshooting bullet (wildcard qualifier now correct). CHANGELOG [Unreleased] / Fixed entry is accurate.
Security: The fix tightens spec-compliance — stops emitting a multi-origin string that violates the CORS spec and could be cached and incorrectly replayed by CDN/proxy intermediaries.
bpamiri
deleted the
fix/bot-2704-wheels-middleware-cors-emits-raw-comma-list-as-acc
branch
…ti-patterns #11-14 (#2740) CLAUDE.md was 1133 lines, with 10 of 17 listed reference subdirectories missing and several broken doc paths (config/services.cfm, docs/src/...). Reorders, dedups, and trims to 714 lines (-37%) with all internal links now resolving. Top-of-file restructured for high-leverage content first: - Code Map: where framework/demo/CLI live and how they relate - Before-Done checklist: which test suite to run for which change type - Cross-Engine Invariants: promoted from buried Docker / Browser-Testing locations - Anti-Patterns extended Top 10 -> Top 14, all new entries sourced from recent PRs: - #11 CFML reserved scopes shadow parameters (#2591) - #12 Empty array in whereIn / whereNotIn (#2736) - #13 Comma-list config != single-value HTTP header (#2725) - #14 Strip CFML comments before source-scanning (#2595) Extracted sections, loaded only when relevant: - .ai/wheels/deploy.md (92 lines) - wheels deploy Kamal port reference - .ai/wheels/wheels-bot.md (34 lines) - bot architecture - .ai/wheels/testing/browser-testing.md (68 lines) - browser DSL Other dedups: t.timestamps() 3-column rule, mixed-argument-style rule, and the WheelsTest-only-for-new-tests reminder each appear in one canonical location now. Reference Docs list at bottom lists only verified-to-exist files. Signed-off-by: Peter Amiri <peter@alurium.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
wheels.middleware.Corswas emitting the raw comma-delimitedallowOriginslist as theAccess-Control-Allow-Originheader value whenever a request arrived without anOriginheader (same-origin pages, server-to-server calls, curl without-H Origin: ...). This violates the CORS spec —Access-Control-Allow-Originmust be a single origin or*, never a list — and can poison CDN/proxy caches that key on response headers.Origin resolution is now extracted into a pure
$resolveAllowOrigin()method that returns"*"for wildcard configs, the reflected origin when the requestOriginis in the allowlist, or""otherwise. The header is only emitted when there is a value to emit. For specific-origin and comma-list configs, same-origin and S2S responses no longer carry the header at all. Wildcard configs (allowOrigins = "*") continue to emitAccess-Control-Allow-Origin: *on every response — unchanged from prior behavior, which is the spec-allowed shape for wildcard.Related Issue
Closes #2704
Reviewer findings — disposition
Vary: Originheader: Tracked separately in issue wheels.middleware.Cors does not emit Vary: Origin when reflecting allowed origins #2707 and addressed by PR fix(middleware): emit Vary: Origin from Cors when reflecting allowed origins #2724. Out of scope here so the comma-list fix can land cleanly; fix(middleware): emit Vary: Origin from Cors when reflecting allowed origins #2724 will layer theVary: Originemission on top.handle()-level integration test for the regression path: Left as-is.cfheader()in test mode does not write real HTTP headers, so the unit-level coverage of$resolveAllowOrigin()is the assertable surface. Reviewer B concurred.Type of Change
Feature Completeness Checklist
Signed-off-by:(usegit commit -s); see CONTRIBUTING.mdAccess-Control-Allow-Origin header resolutiondescribe block invendor/wheels/tests/specs/middleware/CorsSpec.cfccovering comma-list / single-origin / wildcard / matched / non-matched / empty allowlist cases against the absent-Originpath. Failing-then-passing TDD: pre-fix 6 errors ($resolveAllowOriginnot defined); post-fixCorsSpecpasses 16/16 with no other middleware spec regressions.bot-update-docs.ymlbot-update-docs.ymlbot-update-docs.yml[Unreleased] / Fixedvendor/wheels/tests/specs/middleware/reports 150 pass / 0 fail / 0 error / 0 skip post-fix (CorsSpec 16/16, MiddlewareOrderResolverSpec 16/16, MiddlewarePipelineSpec 12/12, MultiTenantIntegrationSpec 9/9, PluginMiddlewarePipelineSpec 7/7, RateLimiterSpec 56/56, SecurityHeadersSpec 24/24, TenantResolverSpec 10/10) viacurl http://localhost:60007/wheels/core/tests?db=sqlite&format=json&directory=wheels.tests.specs.middleware.Test Plan
set(middleware = [ new wheels.middleware.Cors(allowOrigins="https://portal.pai.com,https://portal.paiindustries.com") ]);Originheader (curl -i http://localhost:.../api/v1/jvm).Access-Control-Allow-Origin: https://portal.pai.com,https://portal.paiindustries.com— the header is omitted entirely.curl -i -H "Origin: https://portal.pai.com" ...) and confirmAccess-Control-Allow-Origin: https://portal.pai.comis reflected back.Access-Control-Allow-Originheader is emitted.curl "http://localhost:60007/wheels/core/tests?db=sqlite&format=json&directory=wheels.tests.specs.middleware"— all 150 specs pass.Screenshots / Output
n/a