Skip to content

Commit d712561

Browse files
ferhatelmasferhatelmas
committed
fix: more sanitization for imgproxy bad requests
Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
1 parent 79f7c44 commit d712561

2 files changed

Lines changed: 141 additions & 14 deletions

File tree

‎src/storage/renderer/image.test.ts‎

Lines changed: 67 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -515,14 +515,17 @@ describe('ImageRenderer fetch client', () => {
515515
})
516516

517517
it.each([
518-
[408, 'image request timed out', 'image request timed out'],
519-
[429, 'too many image requests', 'too many image requests'],
518+
[408, 'image request timed out', 408, 400, 'Image request timed out'],
519+
[429, 'too many image requests', 429, 400, 'Too many requests'],
520520
[
521521
500,
522522
"Can't download source image: https://internal.example/private.png?X-Amz-Signature=secret",
523+
500,
524+
500,
523525
'Internal error',
524526
],
525-
])('maps exhausted retry response for imgproxy %i', async (statusCode, body, expectedMessage) => {
527+
[503, 'temporary imgproxy failure', 503, 400, 'Internal error'],
528+
])('maps exhausted retry response for imgproxy %i', async (statusCode, body, expectedStatusCode, expectedUserStatusCode, expectedMessage) => {
526529
const mockUndici = await useUndiciMockAgent()
527530

528531
try {
@@ -554,7 +557,8 @@ describe('ImageRenderer fetch client', () => {
554557
const result = await resultPromise
555558

556559
expect(result).toMatchObject({
557-
httpStatusCode: statusCode,
560+
httpStatusCode: expectedStatusCode,
561+
userStatusCode: expectedUserStatusCode,
558562
message: expectedMessage,
559563
})
560564
mockAgent.assertNoPendingInterceptors()
@@ -607,6 +611,25 @@ describe('ImageRenderer fetch client', () => {
607611
400,
608612
'The source image is invalid or unsupported for rendering',
609613
],
614+
[422, 'Invalid source image', 400, 'The source image is invalid or unsupported for rendering'],
615+
[
616+
422,
617+
'Invalid source image \n',
618+
400,
619+
'The source image is invalid or unsupported for rendering',
620+
],
621+
[
622+
422,
623+
'Broken or unsupported image',
624+
400,
625+
'The source image is invalid or unsupported for rendering',
626+
],
627+
[
628+
422,
629+
'Broken or unsupported image \t',
630+
400,
631+
'The source image is invalid or unsupported for rendering',
632+
],
610633
])('maps imgproxy source-image validation error %# (%i)', async (upstreamStatusCode, body, expectedStatusCode, expectedMessage) => {
611634
const fetchMock = vi.fn<typeof fetch>().mockResolvedValue(
612635
new Response(body, {
@@ -622,6 +645,7 @@ describe('ImageRenderer fetch client', () => {
622645
expect(result).toMatchObject({
623646
code: 'InvalidRequest',
624647
httpStatusCode: expectedStatusCode,
648+
userStatusCode: expectedStatusCode,
625649
message: expectedMessage,
626650
})
627651
})
@@ -647,10 +671,43 @@ describe('ImageRenderer fetch client', () => {
647671
expect(result).toMatchObject({
648672
code: 'InvalidRequest',
649673
httpStatusCode: upstreamStatusCode,
674+
userStatusCode: 400,
650675
message: 'Unable to download source image',
651676
})
652677
})
653678

679+
it.each([
680+
[404, 'Invalid URL', 404, 400, 'Invalid image request'],
681+
[404, 'Invalid source', 404, 400, 'Invalid image source'],
682+
[404, 'Invalid source \n', 404, 400, 'Invalid image source'],
683+
[403, 'Forbidden', 403, 400, 'Image transformation request was rejected'],
684+
[404, 'Not found', 404, 400, 'Not found'],
685+
[404, 'Source image is unreachable', 404, 400, 'Unable to download source image'],
686+
[404, 'Source image is unreachable \n', 404, 400, 'Unable to download source image'],
687+
[429, 'Too many requests', 429, 400, 'Too many requests'],
688+
[429, 'Too many requests \t', 429, 400, 'Too many requests'],
689+
[503, 'Timeout', 503, 400, 'Image request timed out'],
690+
[422, '<html>upstream debug</html>', 422, 400, 'Invalid image request'],
691+
[502, '<html>upstream failure</html>', 502, 400, 'Internal error'],
692+
])('maps imgproxy production public error %# (%i)', async (upstreamStatusCode, body, expectedStatusCode, expectedUserStatusCode, expectedMessage) => {
693+
const fetchMock = vi.fn<typeof fetch>().mockResolvedValue(
694+
new Response(body, {
695+
status: upstreamStatusCode,
696+
})
697+
)
698+
vi.stubGlobal('fetch', fetchMock)
699+
700+
const { ImageRenderer } = await loadRendererModule()
701+
const renderer = new ImageRenderer(createBackend('local:///tmp/cat.png'))
702+
const result = await renderer.getAsset(createRequest(), createRenderOptions()).catch((e) => e)
703+
704+
expect(result).toMatchObject({
705+
httpStatusCode: expectedStatusCode,
706+
userStatusCode: expectedUserStatusCode,
707+
message: expectedMessage,
708+
})
709+
})
710+
654711
it('clamps imgproxy Retry-After waits before retrying', async () => {
655712
const mockUndici = await useUndiciMockAgent()
656713

@@ -1139,7 +1196,7 @@ describe('ImageRenderer fetch client', () => {
11391196
await expect(readStream(result.body)).resolves.toBe('rendered-body')
11401197
})
11411198

1142-
it('does not retry non-retryable imgproxy failures and preserves the response body', async () => {
1199+
it('does not retry non-retryable imgproxy failures and returns a controlled message', async () => {
11431200
const fetchMock = vi.fn<typeof fetch>().mockResolvedValue(
11441201
new Response('invalid image request', {
11451202
status: 400,
@@ -1154,15 +1211,15 @@ describe('ImageRenderer fetch client', () => {
11541211

11551212
await expect(renderer.getAsset(createRequest(), createRenderOptions())).rejects.toMatchObject({
11561213
httpStatusCode: 400,
1157-
message: 'invalid image request',
1214+
message: 'Invalid image request',
11581215
metadata: {
11591216
transformations: ['height:50', 'resizing_type:fill'],
11601217
},
11611218
})
11621219
expect(fetchMock).toHaveBeenCalledTimes(1)
11631220
})
11641221

1165-
it('falls back to the request error message when imgproxy returns an empty error body', async () => {
1222+
it('returns a controlled message when imgproxy returns an empty error body', async () => {
11661223
const fetchMock = vi.fn<typeof fetch>().mockResolvedValue(new Response(null, { status: 400 }))
11671224
vi.stubGlobal('fetch', fetchMock)
11681225

@@ -1171,7 +1228,7 @@ describe('ImageRenderer fetch client', () => {
11711228

11721229
await expect(renderer.getAsset(createRequest(), createRenderOptions())).rejects.toMatchObject({
11731230
httpStatusCode: 400,
1174-
message: 'Request failed with status code 400',
1231+
message: 'Invalid image request',
11751232
})
11761233
expect(fetchMock).toHaveBeenCalledTimes(1)
11771234
})
@@ -1213,7 +1270,7 @@ describe('ImageRenderer fetch client', () => {
12131270
const errorBody = new Readable({
12141271
objectMode: true,
12151272
read() {
1216-
this.push(new TextEncoder().encode('invalid image request'))
1273+
this.push(new TextEncoder().encode('Invalid source image'))
12171274
this.push(null)
12181275
},
12191276
})
@@ -1228,7 +1285,7 @@ describe('ImageRenderer fetch client', () => {
12281285
} as never)
12291286
).resolves.toMatchObject({
12301287
httpStatusCode: 400,
1231-
message: 'invalid image request',
1288+
message: 'The source image is invalid or unsupported for rendering',
12321289
})
12331290
})
12341291

‎src/storage/renderer/image.ts‎

Lines changed: 74 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ const IMAGE_RENDERER_RESPONSE_HEADERS = ['content-length', 'content-type', 'last
5454
const IMGPROXY_INTERNAL_ERROR_MESSAGE = 'Internal error'
5555
const IMGPROXY_SOURCE_IMAGE_ERROR_PATTERN = /Can't download source image\b/i
5656
const IMGPROXY_SOURCE_IMAGE_ERROR_MESSAGE = 'Unable to download source image'
57+
const IMGPROXY_INVALID_TRANSFORMATION_MESSAGE = 'Invalid image request'
58+
const IMGPROXY_INVALID_SOURCE_MESSAGE = 'Invalid image source'
59+
const IMGPROXY_REJECTED_REQUEST_MESSAGE = 'Image transformation request was rejected'
60+
const IMGPROXY_NOT_FOUND_MESSAGE = 'Not found'
61+
const IMGPROXY_TOO_MANY_REQUESTS_MESSAGE = 'Too many requests'
62+
const IMGPROXY_REQUEST_TIMED_OUT_MESSAGE = 'Image request timed out'
5763
const IMGPROXY_SOURCE_IMAGE_INVALID_OR_UNSUPPORTED_MESSAGE =
5864
'The source image is invalid or unsupported for rendering'
5965
const IMGPROXY_SOURCE_IMAGE_BAD_REQUESTS = [
@@ -69,6 +75,14 @@ const IMGPROXY_SOURCE_IMAGE_BAD_REQUESTS = [
6975
message: IMGPROXY_SOURCE_IMAGE_INVALID_OR_UNSUPPORTED_MESSAGE,
7076
pattern: /invalid TIFF format:/i,
7177
},
78+
{
79+
message: IMGPROXY_SOURCE_IMAGE_INVALID_OR_UNSUPPORTED_MESSAGE,
80+
pattern: /^Invalid source image$/i,
81+
},
82+
{
83+
message: IMGPROXY_SOURCE_IMAGE_INVALID_OR_UNSUPPORTED_MESSAGE,
84+
pattern: /^Broken or unsupported image$/i,
85+
},
7286
{
7387
message: 'The source image resolution is too large to process',
7488
pattern: /Source image resolution is too big/i,
@@ -82,6 +96,36 @@ const IMGPROXY_SOURCE_IMAGE_BAD_REQUESTS = [
8296
pattern: /Source image file is too big/i,
8397
},
8498
]
99+
const IMGPROXY_PUBLIC_ERRORS = [
100+
{
101+
message: IMGPROXY_SOURCE_IMAGE_ERROR_MESSAGE,
102+
pattern: /^Source image is unreachable$/i,
103+
},
104+
{
105+
message: IMGPROXY_INVALID_TRANSFORMATION_MESSAGE,
106+
pattern: /^Invalid URL$/i,
107+
},
108+
{
109+
message: IMGPROXY_INVALID_SOURCE_MESSAGE,
110+
pattern: /^Invalid source$/i,
111+
},
112+
{
113+
message: IMGPROXY_REJECTED_REQUEST_MESSAGE,
114+
pattern: /^Forbidden$/i,
115+
},
116+
{
117+
message: IMGPROXY_NOT_FOUND_MESSAGE,
118+
pattern: /^Not found$/i,
119+
},
120+
{
121+
message: IMGPROXY_TOO_MANY_REQUESTS_MESSAGE,
122+
pattern: /^Too many requests$/i,
123+
},
124+
{
125+
message: IMGPROXY_REQUEST_TIMED_OUT_MESSAGE,
126+
pattern: /^Timeout$/i,
127+
},
128+
] as const
85129

86130
const dispatcher: Dispatcher = new Agent({
87131
bodyTimeout: IMGPROXY_REQUEST_TIMEOUT_MS,
@@ -455,7 +499,7 @@ export class ImageRenderer extends Renderer {
455499

456500
const processingError = getImageProcessingError(
457501
error.response?.status || 500,
458-
errorResponse || error.message
502+
errorResponse.trim() || error.message
459503
)
460504
return ERRORS.ImageProcessingError(processingError.statusCode, processingError.message)
461505
}
@@ -470,9 +514,24 @@ function getImageProcessingError(statusCode: number, message: string) {
470514
}
471515
}
472516

473-
if (isImgProxySourceImageError(message) && statusCode < 500) {
517+
if (statusCode === 408) {
474518
return {
475-
message: IMGPROXY_SOURCE_IMAGE_ERROR_MESSAGE,
519+
message: IMGPROXY_REQUEST_TIMED_OUT_MESSAGE,
520+
statusCode,
521+
}
522+
}
523+
524+
if (statusCode === 429) {
525+
return {
526+
message: IMGPROXY_TOO_MANY_REQUESTS_MESSAGE,
527+
statusCode,
528+
}
529+
}
530+
531+
const publicError = getImgProxyPublicError(message)
532+
if (publicError) {
533+
return {
534+
message: publicError.message,
476535
statusCode,
477536
}
478537
}
@@ -484,8 +543,15 @@ function getImageProcessingError(statusCode: number, message: string) {
484543
}
485544
}
486545

546+
if (isImgProxySourceImageError(message)) {
547+
return {
548+
message: IMGPROXY_SOURCE_IMAGE_ERROR_MESSAGE,
549+
statusCode,
550+
}
551+
}
552+
487553
return {
488-
message,
554+
message: IMGPROXY_INVALID_TRANSFORMATION_MESSAGE,
489555
statusCode,
490556
}
491557
}
@@ -494,6 +560,10 @@ function getImgProxySourceImageValidationError(message: string) {
494560
return IMGPROXY_SOURCE_IMAGE_BAD_REQUESTS.find((badRequest) => badRequest.pattern.test(message))
495561
}
496562

563+
function getImgProxyPublicError(message: string) {
564+
return IMGPROXY_PUBLIC_ERRORS.find((publicError) => publicError.pattern.test(message))
565+
}
566+
497567
function isImgProxySourceImageError(message: string) {
498568
return IMGPROXY_SOURCE_IMAGE_ERROR_PATTERN.test(message)
499569
}

0 commit comments

Comments
 (0)