🐛 Fixed CORS errors for embedded signup forms after a custom domain is added#26497
Merged
kevinansfield merged 1 commit intomainfrom Feb 24, 2026
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (10)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughReplaces direct use of the external cors invocation in the members app with a new local CORS middleware module (ghost/core/core/server/web/members/middleware/cors.js) that determines CORS behavior (enabled, disabled, wildcard) based on configured site/admin URLs and request origin. Wires the middleware into the members app and adds unit tests validating allowed, disallowed, missing/null, and wildcard origin handling. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
kevinansfield
changed the title
kevinansfield
changed the title
kevinansfield
force-pushed
the
codex/linear-mention-ber-3063-embeddable-sign-up-forms-break-w
branch
3 times, most recently
from
4845cb4 to
c9a561a
Compare
kevinansfield
force-pushed
the
codex/linear-mention-ber-3063-embeddable-sign-up-forms-break-w
branch
from
c9a561a to
084fe02
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
ghost/core/core/server/web/members/middleware/cors.js (1)
14-15: IPv6 loopback (::1) is not covered.The localhost allowlist covers
localhostand127.0.0.1but misses the IPv6 loopback::1. Local development environments using IPv6 stacks will receive wildcard CORS instead of the reflected-origin path.♻️ Proposed fix
-if (originUrl.hostname === 'localhost' || originUrl.hostname === '127.0.0.1') { +if (originUrl.hostname === 'localhost' || originUrl.hostname === '127.0.0.1' || originUrl.hostname === '::1') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/web/members/middleware/cors.js` around lines 14 - 15, The CORS allowlist only checks originUrl.hostname for 'localhost' and '127.0.0.1' and misses the IPv6 loopback; update the condition in cors.js (the branch that references originUrl.hostname) to also treat '::1' as allowed so IPv6 local requests follow the reflected-origin path instead of falling back to wildcard CORS.ghost/core/test/unit/server/web/members/middleware/cors.test.js (3)
56-56:res.getstubs appear to be dead code.The middleware uses
req.get('origin')(confirmed by the implementation snippet) and the mock already coversres.getHeader(Line 24). Theres.getstubs on the response object are never invoked and can be removed to reduce noise.♻️ Proposed cleanup
- res.get = sinon.stub().withArgs('origin').returns(origin);Remove this line from all four origin-bearing tests.
Also applies to: 75-75, 94-94, 113-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js` at line 56, The res.get stubs are dead code in the origin-bearing tests — remove the calls that stub res.get (e.g., the line that does res.get = sinon.stub().withArgs('origin').returns(origin)) from the four tests that already mock req.get('origin') and res.getHeader to clean up noise; update the tests in ghost/core/test/unit/server/web/members/middleware/cors.test.js by deleting those res.get stubbing lines in each origin-bearing test so only req.get and res.getHeader mocks remain.
13-20: OnlyOPTIONS(preflight) method is tested; actual-request CORS behavior is unverified.
req.methodis hardcoded to'OPTIONS'inbeforeEach. The middleware should also set CORS headers on regularGET/POSTrequests (callingnext()rather thanres.end()). A missing test here means a regression in non-preflight handling would go undetected.✅ Suggested additional test
+ it('should set CORS header and call next for non-OPTIONS requests with known origin', function () { + configUtils.set({url: 'https://my.blog'}); + req.method = 'GET'; + + const origin = 'http://my.blog'; + req.get = sinon.stub().withArgs('origin').returns(origin); + req.headers.origin = origin; + + cors(req, res, next); + + sinon.assert.calledOnce(next); + sinon.assert.notCalled(res.end); + assert.equal(res.headers['Access-Control-Allow-Origin'], origin); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js` around lines 13 - 20, The test only covers OPTIONS (preflight) because beforeEach hardcodes req.method = 'OPTIONS', so add a new test case for non-OPTIONS requests (e.g., 'GET' and/or 'POST') and/or change beforeEach to default to a non-OPTIONS method and override for the preflight test; in that new test, assert the CORS headers are set on res and that the middleware calls next() (instead of ending the response), referencing the test's req object and the cors middleware under test to locate the code to update.
8-120: No test forlocalhostorigin, which the PR explicitly documents as a known/reflected origin.The PR description states: "Known origins (site URL
config.url, admin URLconfig.admin.url, localhost) are reflected back." There is no test verifying that alocalhost(orhttp://localhost:*) origin receives its own origin echoed back rather than*.✅ Suggested additional test
+ it('should be enabled when origin is localhost', function () { + configUtils.set({url: 'https://my.blog'}); + + const origin = 'http://localhost:2368'; + req.get = sinon.stub().withArgs('origin').returns(origin); + req.headers.origin = origin; + + cors(req, res, next); + + sinon.assert.calledOnce(res.end); + assert.equal(res.headers['Access-Control-Allow-Origin'], origin); + assert.equal(res.headers['Access-Control-Allow-Credentials'], 'true'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js` around lines 8 - 120, Add a new unit test in the cors.test.js suite that verifies localhost origins are reflected back: set configUtils to a normal site/admin URL, stub req.get('origin') and res.get('origin') to return an origin like 'http://localhost:3000', set req.headers.origin accordingly, call cors(req, res, next), assert res.end was called once and assert res.headers['Access-Control-Allow-Origin'] === origin (not '*'), and ensure next was not called; use the same req/res stubbing pattern as the other tests and reference the cors middleware function and configUtils used elsewhere in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ghost/core/core/server/web/members/middleware/cors.js`:
- Line 6: ENABLE_CORS currently lacks the credentials flag so browsers will
still block credentialed cross-origin requests; update the ENABLE_CORS object
used by the cors middleware (the const ENABLE_CORS declaration) to include
credentials: true (e.g. { origin: true, maxAge, credentials: true }) so the
server returns Access-Control-Allow-Credentials: true while still reflecting the
request origin.
In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js`:
- Around line 51-82: The tests for the cors middleware are missing assertions
for Access-Control-Allow-Credentials and the middleware config lacks enabling
credentials; update the cors config object (ENABLE_CORS) to include credentials:
true so the cors npm middleware will emit Access-Control-Allow-Credentials, then
update the two tests that check known origins (the tests calling cors(req, res,
next) with config.url and config.admin.url) to assert
res.headers['Access-Control-Allow-Credentials'] === 'true'; also add an
assertion in the wildcard-origin test to confirm credentials header is not set,
and consider adding a separate test case for localhost origin to cover that
branch.
---
Nitpick comments:
In `@ghost/core/core/server/web/members/middleware/cors.js`:
- Around line 14-15: The CORS allowlist only checks originUrl.hostname for
'localhost' and '127.0.0.1' and misses the IPv6 loopback; update the condition
in cors.js (the branch that references originUrl.hostname) to also treat '::1'
as allowed so IPv6 local requests follow the reflected-origin path instead of
falling back to wildcard CORS.
In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js`:
- Line 56: The res.get stubs are dead code in the origin-bearing tests — remove
the calls that stub res.get (e.g., the line that does res.get =
sinon.stub().withArgs('origin').returns(origin)) from the four tests that
already mock req.get('origin') and res.getHeader to clean up noise; update the
tests in ghost/core/test/unit/server/web/members/middleware/cors.test.js by
deleting those res.get stubbing lines in each origin-bearing test so only
req.get and res.getHeader mocks remain.
- Around line 13-20: The test only covers OPTIONS (preflight) because beforeEach
hardcodes req.method = 'OPTIONS', so add a new test case for non-OPTIONS
requests (e.g., 'GET' and/or 'POST') and/or change beforeEach to default to a
non-OPTIONS method and override for the preflight test; in that new test, assert
the CORS headers are set on res and that the middleware calls next() (instead of
ending the response), referencing the test's req object and the cors middleware
under test to locate the code to update.
- Around line 8-120: Add a new unit test in the cors.test.js suite that verifies
localhost origins are reflected back: set configUtils to a normal site/admin
URL, stub req.get('origin') and res.get('origin') to return an origin like
'http://localhost:3000', set req.headers.origin accordingly, call cors(req, res,
next), assert res.end was called once and assert
res.headers['Access-Control-Allow-Origin'] === origin (not '*'), and ensure next
was not called; use the same req/res stubbing pattern as the other tests and
reference the cors middleware function and configUtils used elsewhere in the
file.
084fe02 to
c229c1c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/core/server/web/members/middleware/cors.js (1)
10-16: IPv6 loopback (::1) not covered by the localhost check.
originUrl.hostname === 'localhost' || originUrl.hostname === '127.0.0.1'misses::1. Any dev tool or browser that sendsOrigin: http://[::1]:PORTwill fall through toWILDCARD_CORSrather thanENABLE_CORS, so credentialed local requests from IPv6 stacks won't work. The fix is a one-liner:🛠️ Proposed fix
- if (originUrl.hostname === 'localhost' || originUrl.hostname === '127.0.0.1') { + if (originUrl.hostname === 'localhost' || originUrl.hostname === '127.0.0.1' || originUrl.hostname === '::1') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/core/server/web/members/middleware/cors.js` around lines 10 - 16, The isAllowedOrigin function currently treats localhost only as 'localhost' or '127.0.0.1', missing IPv6 loopback; update isAllowedOrigin to also accept '::1' by adding a check for originUrl.hostname === '::1' alongside the existing localhost and 127.0.0.1 checks so IPv6 local origins are allowed and credentialed local requests succeed.ghost/core/test/unit/server/web/members/middleware/cors.test.js (1)
42-120: No test for the localhost origin path.
isAllowedOriginincors.jsexplicitly whitelistslocalhostand127.0.0.1(lines 14–16), but no test case exercises this path. Adding coverage ensures regressions are caught if the localhost logic changes.🧪 Suggested localhost test
+ it('should be enabled when origin is localhost', function () { + configUtils.set({url: 'https://my.blog'}); + + const origin = 'http://localhost:2368'; + req.get = sinon.stub().withArgs('origin').returns(origin); + req.headers.origin = origin; + + cors(req, res, next); + + sinon.assert.calledOnce(res.end); + assert.equal(res.headers['Access-Control-Allow-Origin'], origin); + assert.equal(res.headers['Access-Control-Allow-Credentials'], 'true'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js` around lines 42 - 120, Add a unit test that exercises the localhost whitelist in isAllowedOrigin by stubbing req.get/res.get and setting req.headers.origin to a localhost origin (e.g. 'http://localhost:3000' or 'http://127.0.0.1:3000'), then call cors(req, res, next) and assert that res.end was called and res.headers['Access-Control-Allow-Origin'] equals the provided origin; place this alongside the existing tests (referencing the cors function and isAllowedOrigin behavior) to ensure the localhost path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js`:
- Around line 51-82: The two known-origin tests in cors.test.js that call
cors(req, res, next) (the "config.url" and "config.admin.url" it blocks)
currently assert Access-Control-Allow-Origin but do not assert the credentials
header; add assertions after the existing origin checks to verify that the
response includes the Access-Control-Allow-Credentials header and that its value
is true; additionally, update the wildcard-path test to assert that
Access-Control-Allow-Credentials is not set (undefined) for the wildcard case so
the negative case is covered.
---
Nitpick comments:
In `@ghost/core/core/server/web/members/middleware/cors.js`:
- Around line 10-16: The isAllowedOrigin function currently treats localhost
only as 'localhost' or '127.0.0.1', missing IPv6 loopback; update
isAllowedOrigin to also accept '::1' by adding a check for originUrl.hostname
=== '::1' alongside the existing localhost and 127.0.0.1 checks so IPv6 local
origins are allowed and credentialed local requests succeed.
In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js`:
- Around line 42-120: Add a unit test that exercises the localhost whitelist in
isAllowedOrigin by stubbing req.get/res.get and setting req.headers.origin to a
localhost origin (e.g. 'http://localhost:3000' or 'http://127.0.0.1:3000'), then
call cors(req, res, next) and assert that res.end was called and
res.headers['Access-Control-Allow-Origin'] equals the provided origin; place
this alongside the existing tests (referencing the cors function and
isAllowedOrigin behavior) to ensure the localhost path is covered.
e78bcb4 to
09b9342
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ghost/core/test/unit/server/web/members/middleware/cors.test.js (2)
13-20: All tests exercise only preflight (OPTIONS); missing coverage for actual requestsEvery test uses
req.method: 'OPTIONS', which causes thecorsmiddleware to callres.end(). The actual request (e.g. GET, POST) response must also includeAccess-Control-Allow-Credentials: true. Consider adding at least one test withmethod: 'POST'for a known origin, asserting the CORS headers are set andnextis called rather thanres.end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js` around lines 13 - 20, Add a test in cors.test.js that uses a non-OPTIONS method (e.g. set req.method = 'POST' in a new it block) so the cors middleware path for actual requests runs; for that test provide a known origin in req.headers.origin, stub res.setHeader and res.end, and assert that res.setHeader was called with "Access-Control-Allow-Credentials" set to "true" (and other CORS headers) and that next() was invoked while res.end was NOT called; update any existing beforeEach or add a new test-specific req override so other tests still exercise preflight behavior.
56-56:res.getstub is dead code
res.getis stubbed at lines 56 and 75, but neither the custom middleware (req.get('origin')at line 38 ofcors.js) nor thecorsnpm package (which readsreq.headers.origindirectly) ever callsres.get(). These stubs can be removed.🧹 Proposed cleanup
- res.get = sinon.stub().withArgs('origin').returns(origin); req.headers.origin = origin;Apply identically at line 75.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js` at line 56, Remove the dead res.get stubs in the unit test: delete the sinon.stub() calls that set res.get to return origin in cors.test.js (both occurrences where res.get is stubbed), because neither the middleware (cors.js uses req.get('origin')) nor the cors package (reads req.headers.origin) call res.get; simply remove those two res.get stubbing lines and run tests to confirm no other references rely on res.get.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js`:
- Around line 51-82: The tests for known-origin paths (the two it blocks that
call cors(req, res, next) with origins matching config.url and config.admin.url)
are missing assertions that Access-Control-Allow-Credentials is set to "true"
and the wildcard-origin tests are missing assertions that this header is not
present; update those tests to assert
res.headers['Access-Control-Allow-Credentials'] === 'true' for the
reflected-origin cases and assert
res.headers['Access-Control-Allow-Credentials'] is undefined (or not present)
for the wildcard-origin cases, and before relying on those assertions confirm
that the middleware's ENABLE_CORS configuration (in cors.js) includes
credentials: true or otherwise ensures credentials are enabled at request-time.
- Around line 8-121: Add a new unit test in the members cors middleware suite
that asserts localhost origins are reflected and credentials are enabled: in
ghost/core/test/unit/server/web/members/middleware/cors.test.js add a test
similar to the "should be enabled when origin matches config.url host" case that
sets req.get/res.get to return 'http://localhost:2368' (and req.headers.origin
accordingly), calls cors(req, res, next) and then asserts
res.headers['Access-Control-Allow-Origin'] equals that origin and
res.headers['Access-Control-Allow-Credentials'] === 'true', and still verifies
res.end was called; locate the test near the other origin-matching tests
referencing the cors function and existing req/res setup.
---
Nitpick comments:
In `@ghost/core/test/unit/server/web/members/middleware/cors.test.js`:
- Around line 13-20: Add a test in cors.test.js that uses a non-OPTIONS method
(e.g. set req.method = 'POST' in a new it block) so the cors middleware path for
actual requests runs; for that test provide a known origin in
req.headers.origin, stub res.setHeader and res.end, and assert that
res.setHeader was called with "Access-Control-Allow-Credentials" set to "true"
(and other CORS headers) and that next() was invoked while res.end was NOT
called; update any existing beforeEach or add a new test-specific req override
so other tests still exercise preflight behavior.
- Line 56: Remove the dead res.get stubs in the unit test: delete the
sinon.stub() calls that set res.get to return origin in cors.test.js (both
occurrences where res.get is stubbed), because neither the middleware (cors.js
uses req.get('origin')) nor the cors package (reads req.headers.origin) call
res.get; simply remove those two res.get stubbing lines and run tests to confirm
no other references rely on res.get.
09b9342 to
f121600
Compare
Member
Author
Context: CORS middleware comparisonWe now have three CommonalitiesAll three files:
Differences
Key observations
|
kevinansfield
force-pushed
the
codex/linear-mention-ber-3063-embeddable-sign-up-forms-break-w
branch
2 times, most recently
from
35ed990 to
38db64b
Compare
allouis
approved these changes
Feb 20, 2026
| const ENABLE_CORS = {origin: true, maxAge, credentials: true}; | ||
| const WILDCARD_CORS = {origin: '*', maxAge}; | ||
|
|
||
| function isAllowedOrigin(origin) { |
Collaborator
There was a problem hiding this comment.
If isAllowedOrigin handled the try/catch for URL construction internally and also handled null/missing origins (returning false), then the corsOptionsDelegate below could collapse into a single if statement, something like:
function corsOptionsDelegate(req, callback) {
const origin = req.get('origin');
if (isAllowedOrigin(origin)) {
return callback(null, ENABLE_CORS);
}
return callback(null, WILDCARD_CORS);
}Not a big deal tho!
closes https://linear.app/ghost/issue/BER-3063/ Replaced blanket wildcard CORS on the members API with an origin-aware policy so credentialed requests (integrity-token) work when making requests from `{url}` to `{admin.url}` as is the case for embedded signup forms after a hosted site adds a custom domain.
kevinansfield
force-pushed
the
codex/linear-mention-ber-3063-embeddable-sign-up-forms-break-w
branch
from
38db64b to
2ff89c9
Compare
kevinansfield
deleted the
codex/linear-mention-ber-3063-embeddable-sign-up-forms-break-w
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes https://linear.app/ghost/issue/BER-3063/
Problem
Embedded signup forms are used via copy/paste and include the currently configured URL in their data attributes. Any forms created whilst a site is on a hosted subdomain (e.g.
*.ghost.io) stop working when the site switches over to custom domain due to a CORS error on the/ghost/api/members/token-integrity/endpoint which requires a cookie to function. The request failed CORS because we returned anAccess-Control-Allow-Origin: *header on the Members API for which browsers refuse to make credentialed requests cross-origin.Summary
Replaced the blanket
cors({maxAge})middleware on the members API with an origin-aware policy. Known origins (site URL, admin URL, localhost) get the specific origin reflected back withAccess-Control-Allow-Credentials: trueso credentialed requests like integrity-token work correctly. All other requests (unknown, missing, or invalid origins) still get*so the public API remains as accessible as before from any site embedding Ghost widgets.access-control-allow-originbehaviourconfig.url)*config.admin.url)********