Closed emmerich closed 2 weeks ago
ijjk
commented
2 weeks ago Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer
ijjk
commented
2 weeks ago Commit: b9e7042b2ca753cf792602891cb0a2ac84842897
__NEXT_EXPERIMENTAL_PPR=true pnpm test-start test/e2e/app-dir/css-client-side-nav-parallel-routes/css-client-side-nav-parallel-routes.test.ts (PPR)
Read more about building and testing Next.js in contributing.md.
TURBOPACK=1 pnpm test-start test/e2e/prerender.test.ts (turbopack)
Read more about building and testing Next.js in contributing.md.
__NEXT_EXPERIMENTAL_PPR=true pnpm test-dev test/e2e/app-dir/actions-allowed-origins/app-action-allowed-origins.test.ts (PPR)
Read more about building and testing Next.js in contributing.md.
ijjk
commented
2 weeks ago Commit: b9e7042b2ca753cf792602891cb0a2ac84842897
styfle
commented
2 weeks ago @emmerich One thing I don't understand is how the url your shared could serve html at all.
I can't reproduce this. Instead, I see Unable to optimize image and unable to fallback to upstream image when I run next dev without remotePatterns configured.
Is there specific config that would cause it to serve html instead of 400? Or perhaps this is only impacting older versions of Next.js and is already fixed in the latest version?
emmerich
commented
2 weeks ago @emmerich One thing I don't understand is how the url your shared could serve html at all.
I can't reproduce this. Instead, I see
Unable to optimize image and unable to fallback to upstream imagewhen I runnext devwithoutremotePatternsconfigured.Is there specific config that would cause it to serve html instead of 400? Or perhaps this is only impacting older versions of Next.js and is already fixed in the latest version?
@styfle I'm also a bit baffled by it. Our Next config is fairly straight-forward:
module.exports = {
env: { ... },
webpack() { ... },
assetPrefix: process.env.GITBOOK_ASSETS_PREFIX, // GitBook static assets CDN
poweredByHeader: false,
images: {
remotePatterns: [
{
protocol: 'https',
hostname: '*.gitbook.io',
port: ''
}
]
}
}Maybe a couple things to note:
remotePatterns config.We're on Next 14.1.3, so not an old version.
I suppose it could be something related to the combination of Cloudflare Pages, next-on-pages, and Nextjs that causes the endpoint to serve HTML, but I didn't dig deeper into next-server.
styfle
commented
2 weeks ago It looks like the issue only reproduces with Cloudflare Pages, not with next dev or next start.
This is likely a vulnerability in the custom Cloudflare code and not Next.js
Its probably best to contact them to get a more complete fix.
emmerich
commented
2 weeks ago After some digging in Cloudflare's next-on-pages project I've found that it has been fixed in a recent release: https://github.com/cloudflare/next-on-pages/commit/8da9da2535356836a7091776d5be84c80cc8091d
Thanks for the help @styfle !
This PR introduces a breaking change that returns a 400 error if the Image Optimization API is given a protocol-relative URL.
The Image Optimization API currently checks whether the given image URL is relative by checking
url.startsWith('/'). This means that protocol-relative URLs, such as//example.com, pass the check and are treated as relative. They in turn skip any kind of validation provided when matching againstremotePatternsand are passed back to the optimation logic as a relative URL.My knowledge of the stack stops there, but in our case at GitBook it led to a nasty attack where non-GitBook content could be served over this URL: https://docs.gitbook.com/_next/image?url=//example.com&w=1200&q=100 - even though we have configured
remotePatternsto protect against it.I originally went into the problem wanting to handle the URL properly (treating it as an absolute URL and potentially using the protocol of the Optimization API itself as the relative protocol), but after seeing the code in
https://github.com/vercel/next.js/blob/canary/packages/next/src/client/legacy/image.tsx#L135
and
https://github.com/vercel/next.js/blob/canary/packages/next/src/shared/lib/image-loader.ts#L26
it feels that protocol-relative URLs are just not really supported anywhere. My understanding is that very few uses of
next/imagewill be allowed to use protocol-relative URLs, so the impact of this breaking change should be quite low? If others disagree I am happy to modify and to use the protocol of the request as a stand-in for the relative protocol.