Skip to content

docs(web/guides): document CORS allow-list defaults drift on 3.x→4.x migration - #2721

Merged
bpamiri merged 3 commits into
developfrom
docs/bot-2708-wheels-middleware-cors-defaults-differ-in-spelling
May 15, 2026
Merged

docs(web/guides): document CORS allow-list defaults drift on 3.x→4.x migration#2721
bpamiri merged 3 commits into
developfrom
docs/bot-2708-wheels-middleware-cors-defaults-differ-in-spelling

Conversation

@wheels-bot

@wheels-bot wheels-bot Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

This PR adds documentation for the silent allow-list shrinkage that occurs when a 3.x app migrates from set(accessControlAllow*) global settings to the wheels.middleware.Cors constructor. The middleware's allowHeaders default (Content-Type,Authorization,X-Requested-With) is narrower than the legacy global default (Origin, Content-Type, X-Auth-Token, X-Requested-By, X-Requested-With), causing preflight rejections for any client that sends X-Auth-Token or X-Requested-By — with no server-side log entry to diagnose it.

Changes

  • Upgrade guide (3x-to-4x.mdx) — new #### Migrating from global settings to the middleware subsection inside the "CORS — two paths in 4.0" section: side-by-side defaults table, explanation of the failure mode, and an explicit-constructor-args code example. New entry added to the "Common issues" list.
  • CORS reference page (cors.mdx)<Aside type="caution"> block after the configuration options table, cross-linking to the upgrade guide section for users who land on the reference page first.
  • Both changes applied to both the v4-0-0 and v4-0-1-snapshot doc versions.
  • CHANGELOG.md [Unreleased] entry added.

Fixes #2708

Screenshots needed

None — this is a prose and table documentation change with no visual components.

…migration

Add a "Migrating from global settings to the middleware" subsection to the
3.x→4.x upgrade guide comparing the legacy accessControlAllowHeaders default
to the Cors constructor default, calling out the silently-dropped X-Auth-Token,
X-Requested-By, and Origin headers, and showing the explicit-constructor-args
fix. Add a matching common-issues entry and a migration callout Aside on both
the CORS reference page and upgrade guide for both v4-0-0 and v4-0-1-snapshot.

Fixes #2708

Signed-off-by: wheels-bot[bot] <wheels-bot[bot]@users.noreply.github.com>
Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot Reviewer A - Full review: The PR documents silent header allow-list shrinkage from 3.x set(accessControlAllow*) to 4.x wheels.middleware.Cors. Two nits: (1) Failure mode description is slightly inaccurate - the server DOES return Access-Control-Allow-Headers on preflights (with the origin in allowlist), it just omits the legacy headers like X-Auth-Token. Developer would see the header in curl -v output. Accurate framing: server returns the header but without the missing headers. The cors.mdx Aside already says silently drops those headers - upgrade guide should match. (2) Methods difference prose under-describes the table - there is also a PATCH/PUT order change (legacy: PATCH, PUT; middleware: PUT, PATCH) that prose does not mention. Suggest: The methods difference (spaces and a minor PATCH/PUT reordering) is cosmetic -- HTTP implementations trim list values and CORS method checks are presence-based. No action is needed unless you pattern-match the exact string. Commits clean. Security: no concerns. Key facts verified against Cors.cfc and security.cfm. Both v4-0-0 and v4-0-1-snapshot in sync. CHANGELOG present.

@wheels-bot

wheels-bot Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

Wheels Bot — Reviewer B (round 1)

A's review is substantively sound. Both findings are accurate, no sycophancy detected. I verified the core claim directly against Cors.cfc line 74 before writing this. Convergence with changes: finding #1 is a real accuracy issue with a clear fix.

Sycophancy

None detected. A cited specific source files (Cors.cfc, security.cfm), checked both doc versions, confirmed the CHANGELOG, and gave a COMMENTED verdict rather than an uncritical approval.

False positives

None detected.

  • Finding New master #1 (failure mode wording): A claims the upgrade guide's phrase "return no Access-Control-Allow-Headers" is inaccurate because the middleware always emits that header. Verified: Cors.cfc line 74 unconditionally calls cfheader(name="Access-Control-Allow-Headers", value=variables.allowHeaders). The header IS returned on every preflight — it just doesn't include the dropped headers in its value. A's correction is accurate.
  • Finding New master #2 (PATCH/PUT ordering): The table diff shows legacy order is PATCH, PUT and middleware order is PUT, PATCH. A notes the prose only mentions spacing, not the ordering flip. A then correctly concludes it is cosmetic (CORS method checks are presence-based per spec). The finding is factually accurate and A's no-action conclusion is correct.

Missed issues

One minor gap A did not flag: the middleware default adds Authorization that was absent from the legacy accessControlAllowHeaders default. The PR's "like-for-like swap" framing focuses on what is dropped (X-Auth-Token, X-Requested-By, Origin) but does not note that the middleware also adds Authorization. A developer who wants to replicate the exact legacy list would include Authorization unnecessarily. Low severity — extra allowed headers break nothing — but a one-line note under the table would be complete. Not blocking.

Verdict alignment

A's COMMENTED state is consistent with two minor nits on a docs-only PR, though finding #1 is concrete enough that request-changes would also have been defensible.

Convergence

Aligned, changes needed. The failure mode description in both v4-0-0/upgrading/3x-to-4x.mdx and v4-0-1-snapshot/upgrading/3x-to-4x.mdx should change from:

preflight OPTIONS requests from clients that send those headers return no Access-Control-Allow-Headers

to something like:

preflight OPTIONS requests from clients that send those headers receive an Access-Control-Allow-Headers response that omits those headers

This matches cors.mdx's accurate phrasing ("silently drops those headers") and aligns with actual Cors.cfc behavior (cfheader is called unconditionally on line 74). No other changes required. Finding #2 needs no action.

Fix inaccurate failure-mode description in the CORS migration section:
the middleware always emits Access-Control-Allow-Headers (Cors.cfc:74
calls cfheader unconditionally) — the response header just omits the
dropped values rather than being absent.

- web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx:114
- web/sites/guides/src/content/docs/v4-0-1-snapshot/upgrading/3x-to-4x.mdx:114

Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
@wheels-bot

wheels-bot Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor Author

Wheels Bot — Address Review (round 1)

Applied consensus findings from Reviewer A and Reviewer B's convergence (round 1):

  • Finding New master #1 (failure-mode wording) — fixed in both versions of the upgrade guide. Replaced "return no Access-Control-Allow-Headers" with "receive an Access-Control-Allow-Headers response that omits those headers". Reviewer B verified that Cors.cfc:74 calls cfheader unconditionally, so the header is always present — only its value differs. New wording aligns with cors.mdx's "silently drops those headers" phrasing.
    • web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx:114
    • web/sites/guides/src/content/docs/v4-0-1-snapshot/upgrading/3x-to-4x.mdx:114

Skipped (not in consensus):

  • Finding New master #2 (PATCH/PUT method ordering) — Reviewer A flagged the prose under-describing the table; Reviewer A and Reviewer B both concluded no action is needed since CORS method matching is presence-based per spec. B explicitly stated "Finding New master #2 needs no action."
  • Reviewer B missed-issue note (Authorization added by middleware default) — B flagged this as "Not blocking" and "low severity" and the convergence summary explicitly said "No other changes required."

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on approve or the outer-loop cap (5 rounds) is reached.

@wheels-bot wheels-bot Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheels Bot — Reviewer A

This PR adds documentation for the CORS allow-list defaults drift that occurs when migrating from 3.x set(accessControlAllow*) global settings to wheels.middleware.Cors. The round-1 fix (latest commit) corrects the inaccurate failure-mode description flagged in the previous review cycle. Both v4-0-0 and v4-0-1-snapshot are kept in sync. Overall this is clean and accurate; a couple of minor nits remain.

Verdict: comment — no blocking correctness, cross-engine, or security findings.


Correctness

Nit 1 — Methods prose omits the PATCH/PUT ordering change

File: web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx, line 128 (same line in the v4-0-1-snapshot copy)

The table shows two differences between the legacy and middleware method defaults, not one:

Difference Legacy Middleware
Whitespace GET, POST, PATCH, PUT, … GET,POST,PUT,PATCH,…
PATCH/PUT order …PATCH, PUT… …PUT,PATCH…

But the prose only names one: "The methods difference (spaces vs no spaces between list items) is cosmetic." A reader who notices the reordering will be unsure whether they misread the table or the prose is incomplete. Both differences are cosmetic, so the conclusion stands — but the prose should name both:

The methods difference (no spaces after commas, and PATCH/PUT order swapped) is cosmetic —
HTTP implementations trim list values and CORS method checks are presence-based, not
order-sensitive. No action is needed unless you pattern-match the exact string.

This was raised by Reviewer A on the previous SHA and was not addressed by the round-1 fix.

Nit 2 — "no entry in your server logs" could mislead on what is logged

File: web/sites/guides/src/content/docs/v4-0-0/upgrading/3x-to-4x.mdx, line 114 (same line in the v4-0-1-snapshot copy)

The sentence ends: "… and the browser blocks the real request with no entry in your server logs."

This is technically accurate — the real (non-preflight) request is blocked browser-side, so the server never receives it. However, the OPTIONS preflight does appear in HTTP access logs (Cors.cfc line 74 emits Access-Control-Allow-Headers and returns 200 normally). A developer tailing access logs will see the OPTIONS 200 but may not connect it to the client-side CORS failure.

Suggested rewording: "… and the browser blocks the actual request (the POST, PUT, etc.) before sending it, leaving no server-side error to diagnose — only a CORS error in the browser console."

The current text is not wrong; this is a clarity suggestion only.


Docs

Verified facts against source:

  • Legacy allowHeaders default: vendor/wheels/events/init/security.cfm line 22 → "Origin, Content-Type, X-Auth-Token, X-Requested-By, X-Requested-With"
  • Legacy allowMethods default: same file line 19 → "GET, POST, PATCH, PUT, DELETE, OPTIONS"
  • Middleware allowHeaders default: vendor/wheels/middleware/Cors.cfc line 22 → "Content-Type,Authorization,X-Requested-With"
  • Middleware allowMethods default: same file line 21 → "GET,POST,PUT,PATCH,DELETE,OPTIONS"
  • Aside component imported at cors.mdx line 9 — no missing import ✓
  • Anchor #migrating-from-global-settings-to-the-middleware matches the #### heading slug in both doc versions ✓
  • CHANGELOG [Unreleased] entry present ✓

Commits

Both commits conform to commitlint.config.js:

  • docs(web/guides): document CORS allow-list defaults drift on 3.x→4.x migration
  • docs(web/guides): address Reviewer A/B consensus findings (round 1)

DCO Signed-off-by trailers present on both commits ✓


Security

No concerns. Pure documentation change; the code example is a recommended middleware constructor call with no injectable values.

@bpamiri
bpamiri marked this pull request as ready for review May 15, 2026 19:44
Manually triggered baseline refresh via
.github/workflows/refresh-visual-baselines.yml on branch docs/bot-2708-wheels-middleware-cors-defaults-differ-in-spelling.

Run when an intentional content/layout change makes the visual-regression
check fail. The new PNG(s) under web/tests/visual-baselines/ are now the
expected rendering; re-run the failing visual-regression job to flip the
check green.
@bpamiri bpamiri closed this May 15, 2026
@bpamiri bpamiri reopened this May 15, 2026
@bpamiri
bpamiri merged commit dd2c8ec into develop May 15, 2026
10 of 11 checks passed
@bpamiri
bpamiri deleted the docs/bot-2708-wheels-middleware-cors-defaults-differ-in-spelling branch May 15, 2026 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

wheels.middleware.Cors defaults differ in spelling from legacy global settings — silent drift on swap

1 participant