fix(cloudflare): handle app.baseURL, cross-zone origin detection and external sources#2126
Conversation
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2126 +/- ##
========================================
- Coverage 6.83% 6.79% -0.05%
========================================
Files 80 80
Lines 3729 3753 +24
Branches 142 142
========================================
Hits 255 255
- Misses 3424 3448 +24
Partials 50 50 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adamkasper
force-pushed
the
fix/cloudflare-provider-base-url-and-cross-zone
branch
from
6cb5eb3 to
598918f
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe Cloudflare image provider was changed: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/runtime/providers/cloudflare.ts (1)
48-51: Inline comment<SOURCE-IMAGE>is now misleading.After this change, the path segment is
sourcePathwhich for internal images isjoinURL(nuxt.baseURL, src)— e.g./admin/images/test.png, not justsrc. The comment should be updated to reflect this.📝 Suggested comment update
- // https://<ZONE>/cdn-cgi/image/<OPTIONS>/<SOURCE-IMAGE> + // https://<ZONE>/cdn-cgi/image/<OPTIONS>/<NUXT-BASE><SOURCE-IMAGE> + // For external srcs the sourcePath is the original absolute URL (cross-zone resizing)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/providers/cloudflare.ts` around lines 48 - 51, Update the misleading inline comment above the URL construction: it currently shows "<SOURCE-IMAGE>" but the code builds the final path using joinURL(baseURL, ...) so sourcePath can be a full path like "/admin/images/test.png" (not just a bare filename). Edit the comment near the operations ? joinURL(baseURL, 'cdn-cgi/image', operations, sourcePath) : sourcePath expression to describe that the resulting URL uses the full sourcePath (which may include nuxt.baseURL) rather than a simple image filename.test/nuxt/providers.test.ts (2)
138-150: No-modifier cross-zone silently ignores the CDNbaseURL— worth documenting.When
modifiersis empty for a cross-zone request (line 146–149), the implementation falls through tourl = sourcePath, discardingbaseURL('https://cdn.example.com') entirely. The image is therefore served from the originating Nuxt host, not the configured CDN. This is consistent with the code, but the expectation is non-obvious to library users and is not called out in a comment. Consider adding a short comment incloudflare.tsor the provider's JSDoc to explain this design decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt/providers.test.ts` around lines 138 - 150, The test shows that cloudflare().getImage returns the original sourcePath when modifiers is empty even if a cross-zone baseURL is provided; add a short explanatory comment in the cloudflare provider (e.g., in cloudflare.ts near the cloudflare() factory and the getImage implementation) clarifying that when modifiers is empty the provider intentionally returns the sourcePath (url = sourcePath) and therefore ignores the baseURL for cross-zone requests; reference the cloudflare() function, getImage method and the url = sourcePath branch in the comment so future readers understand this design choice.
112-176: Source coverage ofcloudflare.tschanges is 0%.All provider tests import from
../../dist/runtime/providers/cloudflare(line 11), so the new lines insrc/runtime/providers/cloudflare.tsare never instrumented. The PR description acknowledges this as a pre-existing architecture issue, but it means none of the bug-fix logic (lines 45–51) is covered by the instrumented test suite. Changing the imports to use the TypeScript source paths (with appropriate tsconfig/vitest path mapping) would give real coverage for future regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt/providers.test.ts` around lines 112 - 176, The tests import the compiled file ('../../dist/runtime/providers/cloudflare') so coverage/reporting never instruments the new source lines in src/runtime/providers/cloudflare.ts; update providers.test.ts to import the TypeScript source module instead (e.g. point the import to src/runtime/providers/cloudflare or use a path alias) and ensure your test runner can resolve TS source by adding the corresponding tsconfig/vitest path mapping or enabling TS resolution (e.g. vitest/ts-node or alias mapping for "runtime/providers/*" -> "src/runtime/providers/*") so the cloudflare.ts lines (including the bug-fix logic) are executed and measured by coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/providers/cloudflare.ts`:
- Around line 48-51: Update the misleading inline comment above the URL
construction: it currently shows "<SOURCE-IMAGE>" but the code builds the final
path using joinURL(baseURL, ...) so sourcePath can be a full path like
"/admin/images/test.png" (not just a bare filename). Edit the comment near the
operations ? joinURL(baseURL, 'cdn-cgi/image', operations, sourcePath) :
sourcePath expression to describe that the resulting URL uses the full
sourcePath (which may include nuxt.baseURL) rather than a simple image filename.
In `@test/nuxt/providers.test.ts`:
- Around line 138-150: The test shows that cloudflare().getImage returns the
original sourcePath when modifiers is empty even if a cross-zone baseURL is
provided; add a short explanatory comment in the cloudflare provider (e.g., in
cloudflare.ts near the cloudflare() factory and the getImage implementation)
clarifying that when modifiers is empty the provider intentionally returns the
sourcePath (url = sourcePath) and therefore ignores the baseURL for cross-zone
requests; reference the cloudflare() function, getImage method and the url =
sourcePath branch in the comment so future readers understand this design
choice.
- Around line 112-176: The tests import the compiled file
('../../dist/runtime/providers/cloudflare') so coverage/reporting never
instruments the new source lines in src/runtime/providers/cloudflare.ts; update
providers.test.ts to import the TypeScript source module instead (e.g. point the
import to src/runtime/providers/cloudflare or use a path alias) and ensure your
test runner can resolve TS source by adding the corresponding tsconfig/vitest
path mapping or enabling TS resolution (e.g. vitest/ts-node or alias mapping for
"runtime/providers/*" -> "src/runtime/providers/*") so the cloudflare.ts lines
(including the bug-fix logic) are executed and measured by coverage.
598918f to
3a2035c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/providers/cloudflare.ts`:
- Around line 43-44: The code currently defaults proto to 'https' using
headers.get('x-forwarded-proto'), which yields incorrect URLs when that header
is missing in plain HTTP environments; update the logic in the Cloudflare
provider (around the proto/header lookup and where imageSource is built) to
determine scheme by checking event.node?.req.socket?.encrypted (use 'https' when
encrypted true, otherwise 'http') and/or accept an optional scheme override on
CloudflareOptions (e.g., options.scheme) so callers can force 'http' in local
setups; ensure the fallback order is headers.get('x-forwarded-proto') ->
options.scheme -> event.node?.req.socket?.encrypted -> 'http' and use that proto
when building the host URL.
- Around line 63-69: The cross-zone guard can silently produce a wrong CDN URL
when getRequestOrigin(ctx.options.event) returns falsy, so update the block that
assigns imageSource (the code using getRequestOrigin, isExternal, hasProtocol,
joinURL and baseURL) to detect a missing origin and emit a clear warning: if
!isExternal && hasProtocol(baseURL) and origin is falsy, call console.warn with
a message that includes baseURL, sourcePath and guidance to set an explicit app
origin (suggest exposing appOrigin on CloudflareOptions) or run with SSR-enabled
context; optionally fail fast in dev by throwing; ensure the warning references
getRequestOrigin, imageSource and CloudflareOptions so maintainers can locate
and act on it.
- Around line 39-49: getRequestOrigin currently trusts unvalidated
x-forwarded-host/header values, enabling SSRF via Cloudflare; add an optional
appOrigin property to CloudflareOptions and make getRequestOrigin use that
pinned origin when provided, falling back to header/window only if appOrigin is
unset. Update the CloudflareOptions type to include appOrigin?: string, change
any constructors or init logic that build Cloudflare provider instances to
accept/apply options.appOrigin, and modify getRequestOrigin(event) to
immediately return options.appOrigin when present (and validate it is a
well-formed origin), otherwise preserve the existing header/window fallback
behavior.
3a2035c to
1640ccd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/providers/cloudflare.ts`:
- Around line 47-49: The origin construction uses
headers.get('x-forwarded-proto') directly which can be a comma-separated list;
update the proto extraction (where proto is assigned) to parse the header value
from headers.get('x-forwarded-proto'), split on ',' and use the first trimmed
token (falling back to 'https' if missing), then build the origin with host as
before (the variables to change are proto and the existing host usage in the
return `${proto}://${host}` expression).
1640ccd to
df0a14d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/runtime/providers/cloudflare.ts (1)
44-55:x-forwarded-hostcan also contain comma-separated values behind multiple proxies.Same treatment applied to
x-forwarded-protoon line 48 should be considered forx-forwarded-hoston line 47. Some proxy chains append to this header, producing"original.example.com, proxy.internal".Low priority since
appOriginis the recommended production path, but worth hardening for consistency.🛡️ Proposed fix
- const host = headers.get('x-forwarded-host') || headers.get('host') + const host = (headers.get('x-forwarded-host') || headers.get('host') || '').split(',')[0].trim() || undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/providers/cloudflare.ts` around lines 44 - 55, In getRequestOrigin, harden extraction of the host header (currently reading 'x-forwarded-host' or 'host') by treating 'x-forwarded-host' like 'x-forwarded-proto': split on ',' and take the first value trimmed before using it; update the logic around headers.get('x-forwarded-host') (in function getRequestOrigin) to parse comma-separated values, fall back to headers.get('host') if empty, and then build the origin with the already-handled proto value.test/nuxt/providers.test.ts (1)
112-247: Comprehensive test coverage for the new Cloudflare provider behavior.The new tests cover the critical paths:
app.baseURLprepending, external images passthrough, cross-zone origin resolution,appOriginconfiguration, and the SSRF mitigation (header override test on line 228). Well-structured with clear scenario separation.Two gaps worth considering for a follow-up:
- Console warn path: No test verifies that
console.warnfires when cross-zone is active but neitherappOriginnor event headers are available. Avi.spyOn(console, 'warn')test would prevent regressions on that diagnostic path.- Multi-value
x-forwarded-proto: No test for the comma-separated header fix (e.g.'https, http'), which was specifically addressed in the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt/providers.test.ts` around lines 112 - 247, Add two tests to cover missing diagnostic and multi-value proto parsing: (1) a test that simulates cross-zone CDN without appOrigin and without event headers and asserts console.warn is called (use vi.spyOn(console, 'warn') and cloudflare().getImage with baseURL set to an external CDN and modifiers to trigger cross-zone logic), and (2) a test that sets event.headers 'x-forwarded-proto' to a comma-separated value like 'https, http' and asserts cloudflare().getImage resolves the protocol correctly (e.g., uses https for app origin resolution). Target the same test suite and call sites using cloudflare().getImage and the ctx object creation used in existing tests so the new cases exercise the SSRF-warning and multi-value header parsing paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/runtime/providers/cloudflare.ts`:
- Around line 44-55: In getRequestOrigin, harden extraction of the host header
(currently reading 'x-forwarded-host' or 'host') by treating 'x-forwarded-host'
like 'x-forwarded-proto': split on ',' and take the first value trimmed before
using it; update the logic around headers.get('x-forwarded-host') (in function
getRequestOrigin) to parse comma-separated values, fall back to
headers.get('host') if empty, and then build the origin with the already-handled
proto value.
In `@test/nuxt/providers.test.ts`:
- Around line 112-247: Add two tests to cover missing diagnostic and multi-value
proto parsing: (1) a test that simulates cross-zone CDN without appOrigin and
without event headers and asserts console.warn is called (use vi.spyOn(console,
'warn') and cloudflare().getImage with baseURL set to an external CDN and
modifiers to trigger cross-zone logic), and (2) a test that sets event.headers
'x-forwarded-proto' to a comma-separated value like 'https, http' and asserts
cloudflare().getImage resolves the protocol correctly (e.g., uses https for app
origin resolution). Target the same test suite and call sites using
cloudflare().getImage and the ctx object creation used in existing tests so the
new cases exercise the SSRF-warning and multi-value header parsing paths.
df0a14d to
36c1ccd
Compare
adamkasper
force-pushed
the
fix/cloudflare-provider-base-url-and-cross-zone
branch
from
5cd9413 to
c7f2733
Compare
2 participants
🔗 Linked issue
❓ Type of change
📚 Description
The Cloudflare provider had several issues when constructing
/cdn-cgi/image/URLs:app.baseURLnot respected — when a Nuxt app is served under a sub-path (e.g.app.baseURL: '/admin/'), local images like/images/photo.jpgneed to resolve as/admin/images/photo.jpginside the Cloudflare transformation URL. Without this fix, the base path was omitted and Cloudflare could not find the image on the origin.Cross-zone origin not resolved — when the Cloudflare
baseURL(zone) differs from the app's domain (e.g. zone ishttps://cdn.example.combut the app runs onhttps://app.example.com), relative image paths like/images/photo.jpgwould resolve against the CDN zone's origin, which doesn't host the images. The provider now auto-detects the app's origin from request headers (host,x-forwarded-proto) on the server orwindow.location.originon the client, producing correct absolute source URLs likehttps://cdn.example.com/cdn-cgi/image/w=200/https://app.example.com/admin/images/photo.jpg.External sources handled correctly — when
srcis already an absolute URL (e.g.https://other.example.com/photo.jpg), it is passed through as-is without prependingbaseURLor the detected origin.Changes:
hasProtocolfromufoto detect absolute source URLsctx.options.nuxt.baseURLonly for relative image pathsgetRequestOrigin()helper that reads the app's origin from H3 event headers (server) orwindow.location(client)baseURLis an absolute URL), resolve relative sources to absolute URLs using the detected originapp.baseURL, external images, cross-zone, and cross-zone with origin detection