Skip to content

Commit 8104705

Browse files
petebacondarwinpetebacondarwin
authored
Allow "plain text" images when blocking vulnerable non-image responses (#9824)
1 parent 56dc5c4 commit 8104705

File tree

3 files changed

+146
-119
lines changed

3 files changed

+146
-119
lines changed

‎.changeset/fluffy-comics-study.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@cloudflare/workers-shared": patch
3+
---
4+
5+
Allow "plain text" images when blocking vulnerable non-image responses

‎packages/workers-shared/router-worker/src/worker.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -132,10 +132,12 @@ export default {
132132

133133
if (shouldBlockNonImageResponse) {
134134
const resp = await env.USER_WORKER.fetch(maybeSecondRequest);
135-
if (
136-
!resp.headers.get("content-type")?.startsWith("image/") &&
137-
resp.status !== 304
138-
) {
135+
const isImage = resp.headers
136+
.get("content-type")
137+
?.startsWith("image/");
138+
const isPlainText =
139+
resp.headers.get("content-type") === "text/plain";
140+
if (!isImage && !isPlainText && resp.status !== 304) {
139141
analytics.setData({ abuseMitigationBlocked: true });
140142
return new Response("Blocked", { status: 403 });
141143
}

‎packages/workers-shared/router-worker/tests/index.test.ts‎

Lines changed: 135 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -247,123 +247,143 @@ describe("unit tests", async () => {
247247
expect(await response.text()).toEqual("hello from user worker");
248248
});
249249

250-
it.each([
251-
{
252-
description:
253-
"blocks /_next/image requests with remote URLs when not fetched as image",
254-
url: "https://example.com/_next/image?url=https://evil.com/ssrf",
255-
},
256-
{
257-
description:
258-
"blocks /_next/image requests with remote URLs when not fetched as image for next apps host not at the root",
259-
url: "https://example.com/some/subpath/_next/image?url=https://evil.com/ssrf",
260-
},
261-
])("$description", async ({ url }) => {
262-
const request = new Request(url);
263-
const ctx = createExecutionContext();
264-
265-
const env = {
266-
CONFIG: {
267-
has_user_worker: true,
268-
invoke_user_worker_ahead_of_assets: true,
269-
},
270-
USER_WORKER: {
271-
async fetch(_: Request): Promise<Response> {
272-
return new Response("<!DOCTYPE html><html></html>", {
250+
describe.each(["/some/subpath/", "/"])(
251+
"blocking /_next/image requests hosted at %s with remote URLs",
252+
(subpath) => {
253+
it("blocks /_next/image requests with remote URLs when not fetched as image", async () => {
254+
const request = new Request(
255+
`https://example.com${subpath}_next/image?url=https://evil.com/ssrf`
256+
);
257+
const ctx = createExecutionContext();
258+
259+
const env = {
260+
CONFIG: {
261+
has_user_worker: true,
262+
invoke_user_worker_ahead_of_assets: true,
263+
},
264+
USER_WORKER: {
265+
async fetch(_: Request): Promise<Response> {
266+
return new Response("<!DOCTYPE html><html></html>", {
267+
headers: { "content-type": "text/html" },
268+
});
269+
},
270+
},
271+
} as Env;
272+
273+
const response = await worker.fetch(request, env, ctx);
274+
expect(response.status).toBe(403);
275+
expect(await response.text()).toBe("Blocked");
276+
});
277+
278+
it.each([
279+
{
280+
description:
281+
"allows /_next/image requests with remote URLs when fetched as image",
282+
url: `https://example.com${subpath}_next/image?url=https://example.com/image.jpg`,
283+
headers: { "sec-fetch-dest": "image" } as HeadersInit,
284+
userWorkerResponse: {
285+
body: "fake image data",
286+
headers: { "content-type": "image/jpeg" },
287+
status: 200,
288+
},
289+
expectedStatus: 200,
290+
expectedBody: "fake image data",
291+
},
292+
{
293+
description:
294+
"allows /_next/image requests with remote URLs that have content type starting with image/...",
295+
url: `https://example.com${subpath}_next/image?url=https://example.com/image.jpg`,
296+
userWorkerResponse: {
297+
body: "fake image data",
298+
headers: { "content-type": "image/jpeg" },
299+
status: 200,
300+
},
301+
expectedStatus: 200,
302+
expectedBody: "fake image data",
303+
},
304+
{
305+
description:
306+
"allows /_next/image requests with remote URLs that have content type text/plain",
307+
url: `https://example.com${subpath}_next/image?url=https://example.com/image.jpg`,
308+
userWorkerResponse: {
309+
body: "fake image data",
310+
headers: { "content-type": "text/plain" },
311+
status: 200,
312+
},
313+
expectedStatus: 200,
314+
expectedBody: "fake image data",
315+
},
316+
{
317+
description:
318+
"allows /_next/image with remote URL and image header regardless of response content",
319+
url: `https://example.com${subpath}_next/image?url=https://example.com/image.jpg`,
320+
headers: { "sec-fetch-dest": "image" } as HeadersInit,
321+
userWorkerResponse: {
322+
body: "<!DOCTYPE html><html></html>",
273323
headers: { "content-type": "text/html" },
274-
});
275-
},
276-
},
277-
} as Env;
278-
279-
const response = await worker.fetch(request, env, ctx);
280-
expect(response.status).toBe(403);
281-
expect(await response.text()).toBe("Blocked");
282-
});
283-
284-
it.each([
285-
{
286-
description:
287-
"allows /_next/image requests with remote URLs when fetched as image",
288-
url: "https://example.com/_next/image?url=https://example.com/image.jpg",
289-
headers: { "sec-fetch-dest": "image" } as HeadersInit,
290-
userWorkerResponse: {
291-
body: "fake image data",
292-
headers: { "content-type": "image/jpeg" },
293-
status: 200,
294-
},
295-
expectedStatus: 200,
296-
expectedBody: "fake image data",
297-
},
298-
{
299-
description:
300-
"allows /_next/image with remote URL and image header regardless of response content",
301-
url: "https://example.com/_next/image?url=https://example.com/image.jpg",
302-
headers: { "sec-fetch-dest": "image" } as HeadersInit,
303-
userWorkerResponse: {
304-
body: "<!DOCTYPE html><html></html>",
305-
headers: { "content-type": "text/html" },
306-
status: 200,
307-
},
308-
expectedStatus: 200,
309-
expectedBody: "<!DOCTYPE html><html></html>",
310-
},
311-
{
312-
description: "allows /_next/image requests with local URLs",
313-
url: "https://example.com/_next/image?url=/local/image.jpg",
314-
headers: {} as HeadersInit,
315-
userWorkerResponse: {
316-
body: "local image data",
317-
headers: { "content-type": "image/jpeg" },
318-
status: 200,
319-
},
320-
expectedStatus: 200,
321-
expectedBody: "local image data",
322-
},
323-
{
324-
description: "allows /_next/image requests with 304 status",
325-
url: "https://example.com/_next/image?url=https://example.com/image.jpg",
326-
headers: {} as HeadersInit,
327-
userWorkerResponse: {
328-
body: null,
329-
headers: { "content-type": "text/html" },
330-
status: 304,
331-
},
332-
expectedStatus: 304,
333-
expectedBody: null,
334-
},
335-
])(
336-
"$description",
337-
async ({
338-
url,
339-
headers,
340-
userWorkerResponse,
341-
expectedStatus,
342-
expectedBody,
343-
}) => {
344-
const request = new Request(url, { headers });
345-
const ctx = createExecutionContext();
346-
347-
const env = {
348-
CONFIG: {
349-
has_user_worker: true,
350-
invoke_user_worker_ahead_of_assets: true,
351-
},
352-
USER_WORKER: {
353-
async fetch(_: Request): Promise<Response> {
354-
return new Response(userWorkerResponse.body, {
355-
status: userWorkerResponse.status,
356-
headers: userWorkerResponse.headers,
357-
});
324+
status: 200,
358325
},
359-
},
360-
} as Env;
361-
362-
const response = await worker.fetch(request, env, ctx);
363-
expect(response.status).toBe(expectedStatus);
364-
if (expectedBody !== null) {
365-
expect(await response.text()).toBe(expectedBody);
366-
}
326+
expectedStatus: 200,
327+
expectedBody: "<!DOCTYPE html><html></html>",
328+
},
329+
{
330+
description: "allows /_next/image requests with local URLs",
331+
url: `https://example.com${subpath}_next/image?url=/local/image.jpg`,
332+
headers: {} as HeadersInit,
333+
userWorkerResponse: {
334+
body: "local image data",
335+
headers: { "content-type": "image/jpeg" },
336+
status: 200,
337+
},
338+
expectedStatus: 200,
339+
expectedBody: "local image data",
340+
},
341+
{
342+
description: "allows /_next/image requests with 304 status",
343+
url: `https://example.com${subpath}_next/image?url=https://example.com/image.jpg`,
344+
headers: {} as HeadersInit,
345+
userWorkerResponse: {
346+
body: null,
347+
headers: { "content-type": "text/html" },
348+
status: 304,
349+
},
350+
expectedStatus: 304,
351+
expectedBody: null,
352+
},
353+
])(
354+
"$description",
355+
async ({
356+
url,
357+
headers,
358+
userWorkerResponse,
359+
expectedStatus,
360+
expectedBody,
361+
}) => {
362+
const request = new Request(url, { headers });
363+
const ctx = createExecutionContext();
364+
365+
const env = {
366+
CONFIG: {
367+
has_user_worker: true,
368+
invoke_user_worker_ahead_of_assets: true,
369+
},
370+
USER_WORKER: {
371+
async fetch(_: Request): Promise<Response> {
372+
return new Response(userWorkerResponse.body, {
373+
status: userWorkerResponse.status,
374+
headers: userWorkerResponse.headers,
375+
});
376+
},
377+
},
378+
} as Env;
379+
380+
const response = await worker.fetch(request, env, ctx);
381+
expect(response.status).toBe(expectedStatus);
382+
if (expectedBody !== null) {
383+
expect(await response.text()).toBe(expectedBody);
384+
}
385+
}
386+
);
367387
}
368388
);
369389

0 commit comments

Comments
 (0)