fix: accept Shopify preview/myshopify origins on personaliser submit CORS#395
Conversation
…CORS The /api/personaliser/submit endpoint only echoed Access-Control-Allow-Origin for a static list (brightivy.com + the dev-store). Testing the storefront widget through an unpublished theme's *.shopifypreview.com preview host was blocked by the browser (Safari 'Load failed') because the preflight returned no ACAO for that origin. Pattern-match Shopify-owned storefront domains (*.myshopify.com, *.shopifypreview.com) in addition to the explicit prod hosts, while still rejecting look-alike/suffix-spoof origins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughCORS origin validation in the personaliser submit endpoint is refactored to replace a hardcoded allowlist containing a specific dev-store hostname with a smaller explicit list of production domains plus a case-insensitive regex pattern that dynamically permits any ChangesCORS Origin Pattern Validation
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@src/app/api/personaliser/submit/route.ts`:
- Around line 25-35: getCorsHeaders currently mirrors the request Origin into
Access-Control-Allow-Origin when isAllowedOrigin(origin) is true but doesn't add
a Vary header; update getCorsHeaders to add "Vary": "Origin" whenever you set
headers['Access-Control-Allow-Origin'] (i.e., inside the isAllowedOrigin(origin)
branch) so caches/CDNs won't reuse responses for different origins—modify the
getCorsHeaders function accordingly and ensure the header key matches "Vary"
with value "Origin".
🪄 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: 571546c0-be08-4ccc-99b9-09f2649271fd
📒 Files selected for processing (1)
src/app/api/personaliser/submit/route.ts
| function getCorsHeaders(origin: string | null): Record<string, string> { | ||
| const headers: Record<string, string> = { | ||
| 'Access-Control-Allow-Methods': 'POST, OPTIONS', | ||
| 'Access-Control-Allow-Headers': 'Content-Type', | ||
| 'Access-Control-Max-Age': '86400', | ||
| }; | ||
| if (origin && ALLOWED_ORIGINS.includes(origin)) { | ||
| headers['Access-Control-Allow-Origin'] = origin; | ||
| if (isAllowedOrigin(origin)) { | ||
| headers['Access-Control-Allow-Origin'] = origin as string; | ||
| } | ||
| return headers; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Should responses with dynamic Access-Control-Allow-Origin include Vary: Origin to avoid cache issues?
💡 Result:
Yes, responses that dynamically generate the Access-Control-Allow-Origin (ACAO) header based on the request's Origin header should include Vary: Origin to prevent significant caching issues [1][2][3][4]. When a server uses a dynamic ACAO policy (e.g., reflecting the request origin or checking it against a whitelist), it must inform shared caches—such as CDNs, proxy servers, and even the browser's own cache—that the response is dependent on the Origin request header [1][2][3]. Without the Vary: Origin header, these caches may incorrectly serve a cached response generated for one origin to a subsequent request from a different origin [2][4]. This can lead to CORS errors where the browser blocks the response because the cached ACAO header does not match the current request's origin, or even potential cache poisoning scenarios [5][2][4]. Best practices include: - Always include Vary: Origin when the ACAO header is dynamic (i.e., not a static string or the * wildcard) [1][2][3]. - Apply this header to all responses that are susceptible to CORS-related caching, including non-CORS requests (requests missing an Origin header), to ensure that a non-CORS response is not mistakenly served to a later CORS request [6][7]. - Do not use Vary: Origin if your ACAO response is always identical, such as when using the * wildcard or a single hardcoded origin string [8]. In these cases, including the header is unnecessary and can reduce cache efficiency [9][8].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/HTTP/Guides/CORS
- 2: https://claude.ai/public/artifacts/860f4896-69c0-4416-8066-ff9bd73c1a37
- 3: https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Access-Control-Allow-Origin
- 4: https://medium.com/@xinquanyip/cors-best-practices-securing-cross-origin-requests-the-right-way-0e58b4b19b33
- 5: https://security.stackexchange.com/questions/151590/vary-origin-response-header-and-cors-exploitation
- 6: Fix: Always set Vary: Origin header according to CORS spec to avoid caching issues expressjs/cors#348
- 7:
Vary: Originshould be set on non-CORS request expressjs/cors#330 - 8: https://stackoverflow.com/questions/59887931/should-you-set-vary-origin-when-allowing-any-cors-origin
- 9: https://dev.to/immanuel_nonzo/scaling-to-1-million-users-load-balancing-caching-strategies-2cdh
Add Vary: Origin to CORS headers when Access-Control-Allow-Origin is computed dynamically
Since getCorsHeaders reflects the request’s Origin into Access-Control-Allow-Origin, include Vary: Origin so shared caches/CDNs don’t reuse a cached response with one origin’s ACAO for a different origin.
♻️ Emit Vary: Origin
function getCorsHeaders(origin: string | null): Record<string, string> {
const headers: Record<string, string> = {
'Access-Control-Allow-Methods': 'POST, OPTIONS',
'Access-Control-Allow-Headers': 'Content-Type',
'Access-Control-Max-Age': '86400',
+ Vary: 'Origin',
};
if (isAllowedOrigin(origin)) {
headers['Access-Control-Allow-Origin'] = origin as string;
}
return headers;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getCorsHeaders(origin: string | null): Record<string, string> { | |
| const headers: Record<string, string> = { | |
| 'Access-Control-Allow-Methods': 'POST, OPTIONS', | |
| 'Access-Control-Allow-Headers': 'Content-Type', | |
| 'Access-Control-Max-Age': '86400', | |
| }; | |
| if (origin && ALLOWED_ORIGINS.includes(origin)) { | |
| headers['Access-Control-Allow-Origin'] = origin; | |
| if (isAllowedOrigin(origin)) { | |
| headers['Access-Control-Allow-Origin'] = origin as string; | |
| } | |
| return headers; | |
| } | |
| function getCorsHeaders(origin: string | null): Record<string, string> { | |
| const headers: Record<string, string> = { | |
| 'Access-Control-Allow-Methods': 'POST, OPTIONS', | |
| 'Access-Control-Allow-Headers': 'Content-Type', | |
| 'Access-Control-Max-Age': '86400', | |
| 'Vary': 'Origin', | |
| }; | |
| if (isAllowedOrigin(origin)) { | |
| headers['Access-Control-Allow-Origin'] = origin as string; | |
| } | |
| return headers; | |
| } |
🤖 Prompt for 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.
In `@src/app/api/personaliser/submit/route.ts` around lines 25 - 35,
getCorsHeaders currently mirrors the request Origin into
Access-Control-Allow-Origin when isAllowedOrigin(origin) is true but doesn't add
a Vary header; update getCorsHeaders to add "Vary": "Origin" whenever you set
headers['Access-Control-Allow-Origin'] (i.e., inside the isAllowedOrigin(origin)
branch) so caches/CDNs won't reuse responses for different origins—modify the
getCorsHeaders function accordingly and ensure the header key matches "Vary"
with value "Origin".
#395) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… on submit (#398) * feat(personaliser): store block photo_crop + high-res reference image on submit Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(personaliser): add Vary: Origin to dynamic CORS headers (CodeRabbit #395) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(personaliser): stable reference path + upsert (no orphans); clarify crop sanitiser + EXIF (CodeRabbit) - reference image now block-references/{id}.{ext} + upsert:true (service-role key), mirroring the photo path — overwrites in place, no orphaned 300dpi composites. Drops immutable cacheControl so the stable URL never serves a stale composite. - soften sanitiseBlockPhotoCrop comment (it's stricter than the case path, not a mirror). - note the reference is a canvas composite (no EXIF) so no sharp() strip is needed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(personaliser): log when block reference image is skipped (oversize/wrong-type) (CodeRabbit) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Joel Garthwaite <info@joelgarthwaite.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
The personaliser storefront widget's photo upload hits
/api/personaliser/submitcross-origin. The endpoint only echoedAccess-Control-Allow-Originfor a static list (brightivy.com,www.brightivy.com, the dev store). Testing the widget through an unpublished theme's*.shopifypreview.compreview host was blocked by the browser — Safari surfaced it as a generic "Load failed" on the Add-to-Cart step — because the CORS preflight returned noAccess-Control-Allow-Originfor that dynamic origin.Fix
Pattern-match Shopify-owned storefront domains (
*.myshopify.com,*.shopifypreview.com) in addition to the explicit production hosts, while still rejecting look-alike / suffix-spoof origins (e.g.shopifypreview.com.evil.com, plainhttp://).Verification
tsc --noEmitclean.brightivy.combehaviour unchanged.🤖 Generated with Claude Code
Summary by CodeRabbit