Skip to content

Commit

Permalink
fix(next): reject protocol-relative URLs in image optimization (#65752)
Browse files Browse the repository at this point in the history
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 against `remotePatterns` and 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 `remotePatterns` to 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/image` will 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.

---------

Co-authored-by: Steven <steven@ceriously.com>
  • Loading branch information
emmerich and styfle authored May 15, 2024
1 parent fdd8f0c commit d9ce336
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 1 deletion.
6 changes: 6 additions & 0 deletions packages/next/src/server/image-optimizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,12 @@ export class ImageOptimizerCache {
return { errorMessage: '"url" parameter is too long' }
}

if (url.startsWith('//')) {
return {
errorMessage: '"url" parameter cannot be a protocol-relative URL (//)',
}
}

let isAbsolute: boolean

if (url.startsWith('/')) {
Expand Down
5 changes: 5 additions & 0 deletions test/integration/image-optimizer/app/pages/api/no-header.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export default function handler(_req, res) {
// Intentionally missing Content-Type header so that
// the fallback is not served when optimization fails
res.end('foo')
}
11 changes: 10 additions & 1 deletion test/integration/image-optimizer/test/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -987,8 +987,17 @@ export function runTests(ctx: RunTestsCtx) {
expect(await res.text()).toBe(`"url" parameter is too long`)
})

it('should fail when url is protocol relative', async () => {
const query = { url: `//example.com`, w: ctx.w, q: 1 }
const res = await fetchViaHTTP(ctx.appPort, '/_next/image', query, {})
expect(res.status).toBe(400)
expect(await res.text()).toBe(
`"url" parameter cannot be a protocol-relative URL (//)`
)
})

it('should fail when internal url is not an image', async () => {
const url = `//<h1>not-an-image</h1>`
const url = `/api/no-header`
const query = { url, w: ctx.w, q: 39 }
const opts = { headers: { accept: 'image/webp' } }
const res = await fetchViaHTTP(ctx.appPort, '/_next/image', query, opts)
Expand Down

0 comments on commit d9ce336

Please sign in to comment.