Skip to content

Commit 9b5cc89

Browse files
shiradyguymguym
shirady
and
guymguym
committed
FIx Request Handling on Requests with Trailing Headers (Mainly in ChunkedContentDecoder)
1. In http_utils.js accept more types of content sha256 headers (STREAMING-UNSIGNED-PAYLOAD-TRAILER, STREAMING-AWS4-HMAC-SHA256-PAYLOAD-TRAILER), as without those headers on clients that add the checksum headers with trailing we would fail. 2. Change the state of the machine and add more states to support the trailing headers: STATE_READ_TRAILER (like we have STATE_READ_CHUNK_HEADER), STATE_WAIT_NL_TRAILER (like we have STATE_WAIT_NL_DATA) and STATE_WAIT_NL_END. 3. Set the following constants to limit the request (avoid the client from abuse): - MAX_CHUNK_SIZE - we want to have a lower number than Number.MAX_SAFE_INTEGER (reference), we expect lower number. - MAX_CHUNK_HEADER_SIZE - we don't expect that long (as it is saved in memory during the parsing). - MAX_TRAILER_SIZE - same, in the example we saw it was about ~30 (x-amz-checksum-crc32:uOMGCw==). - MAX_TRAILERS - currently we saw the trailer of checksum (x-amz-checksum-crc32:uOMGCw==), we expect to have a few trailers in a request. 4. Refactor and organize - add comments with explanations about the state machine, add helper functions, add separation between the parts, rename chunk_header_str to chunk_header, add members related to trailers, add the member this.stream_pos which we use for validation. 5. Improve building the string (either this.chunk_header, and this.trailer) so we won't build it byte by byte, but only after we find the CR ('\r`). 6. Replace buffer slice function with subarray as the function slice was deprecated (see reference). Co-authored-by: Guy Margalit <guymguym@gmail.com> Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
1 parent 56563a0 commit 9b5cc89

File tree

3 files changed

+476
-33
lines changed

3 files changed

+476
-33
lines changed

‎src/test/unit_tests/jest_tests/test_chunked_content_decoder.test.js

Lines changed: 259 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,259 @@
1+
/* Copyright (C) 2025 NooBaa */
2+
'use strict';
3+
4+
const stream = require('stream');
5+
const assert = require('assert');
6+
const ChunkedContentDecoder = require('../../../util/chunked_content_decoder');
7+
const buffer_utils = require('../../../util/buffer_utils');
8+
9+
describe('ChunkedContentDecoder', function() {
10+
11+
// Reminder about chunk structure:
12+
// <hex bytes of data>\r\n
13+
// <data>
14+
//....
15+
// the end of the chunk:
16+
// 0\r\n
17+
// \r\n
18+
//
19+
// The following example was copied from:
20+
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding
21+
// 7\r\n
22+
// Mozilla\r\n
23+
// 11\r\n
24+
// Developer Network\r\n
25+
// 0\r\n
26+
// \r\n
27+
28+
// for easier debugging you can set the number of iteration here:
29+
const NUMBER_OF_ITERATIONS_IMPORTANT_CASE = 100;
30+
const NUMBER_OF_ITERATIONS_DEFAULT = 2;
31+
32+
describe('expected to parse the input', function() {
33+
test_parse_output({
34+
name: 'one_chunk',
35+
input:
36+
'3\r\n' +
37+
'foo\r\n' +
38+
'0\r\n' +
39+
'\r\n',
40+
output: 'foo',
41+
iterations: NUMBER_OF_ITERATIONS_DEFAULT,
42+
});
43+
44+
test_parse_output({
45+
name: 'two_chunks',
46+
input:
47+
'3\r\n' +
48+
'foo\r\n' +
49+
'3\r\n' +
50+
'bar\r\n' +
51+
'0\r\n' +
52+
'\r\n',
53+
output: 'foobar',
54+
iterations: NUMBER_OF_ITERATIONS_IMPORTANT_CASE,
55+
});
56+
57+
test_parse_output({
58+
name: 'three_chunks_with_trailers',
59+
input:
60+
'3\r\n' +
61+
'foo\r\n' +
62+
'6\r\n' +
63+
'barbaz\r\n' +
64+
'ff\r\n' +
65+
'f'.repeat(255) + '\r\n' +
66+
'0\r\n' +
67+
'x-trailer-1:value\r\n' +
68+
'x-trailer-2:value\r\n' +
69+
'\r\n',
70+
output: 'foobarbaz' + 'f'.repeat(255),
71+
iterations: NUMBER_OF_ITERATIONS_IMPORTANT_CASE,
72+
check: decoder => {
73+
assert.deepStrictEqual(decoder.trailers, [
74+
'x-trailer-1:value',
75+
'x-trailer-2:value',
76+
]);
77+
},
78+
});
79+
80+
test_parse_output({
81+
name: 'no_chunk_with_trailers',
82+
input:
83+
'0\r\n' +
84+
'movie:trailer\r\n' +
85+
'semi:trailer\r\n' +
86+
'\r\n',
87+
output: '',
88+
iterations: NUMBER_OF_ITERATIONS_IMPORTANT_CASE,
89+
check: decoder => {
90+
assert.deepStrictEqual(decoder.trailers, [
91+
'movie:trailer',
92+
'semi:trailer',
93+
]);
94+
},
95+
});
96+
97+
test_parse_output({
98+
name: 'one_chunk_with_extension',
99+
input:
100+
'3;crc=1a2b3c4d\r\n' +
101+
'EXT\r\n' +
102+
'0\r\n' +
103+
'\r\n',
104+
output: 'EXT',
105+
iterations: NUMBER_OF_ITERATIONS_IMPORTANT_CASE,
106+
});
107+
108+
test_parse_output({
109+
name: 'one_chunk_with_extension_and_trailer',
110+
input:
111+
'3;crc=1a2b3c4d\r\n' +
112+
'EXT\r\n' +
113+
'0\r\n' +
114+
create_trailers(1) +
115+
'\r\n',
116+
output: 'EXT',
117+
iterations: NUMBER_OF_ITERATIONS_IMPORTANT_CASE,
118+
});
119+
120+
test_parse_output({
121+
name: 'one_chunk_with_trailers', // lower than MAX_CHUNK_HEADER_SIZE
122+
input:
123+
'3\r\n' +
124+
'foo\r\n' +
125+
'0\r\n' +
126+
create_trailers(19) +
127+
'\r\n',
128+
output: 'foo',
129+
iterations: NUMBER_OF_ITERATIONS_DEFAULT,
130+
});
131+
132+
});
133+
134+
describe('expected to have an error on parse', function() {
135+
136+
test_parse_error({
137+
name: 'chunk_size_not_hex',
138+
input: 'invalid\r\n\r\n',
139+
error_pos: 7, // end of header
140+
iterations: NUMBER_OF_ITERATIONS_IMPORTANT_CASE,
141+
});
142+
143+
test_parse_error({
144+
name: 'chunk_size_too_big', // according to MAX_CHUNK_SIZE
145+
input: '10000000001\r\n\r\n',
146+
error_pos: 11, // end of header
147+
iterations: NUMBER_OF_ITERATIONS_IMPORTANT_CASE,
148+
});
149+
150+
test_parse_error({
151+
name: 'header_too_long', // according to MAX_CHUNK_HEADER_SIZE
152+
input: '0' + ';'.repeat(1024) + '\r\n\r\n',
153+
error_pos: 1025, // end of header
154+
iterations: NUMBER_OF_ITERATIONS_IMPORTANT_CASE,
155+
});
156+
157+
test_parse_error({
158+
name: 'too_many_trailers', // according to MAX_CHUNK_HEADER_SIZE
159+
input:
160+
'3\r\n' +
161+
'foo\r\n' +
162+
'0\r\n' +
163+
create_trailers(21) +
164+
'\r\n',
165+
error_pos: 420, // last trailer position
166+
iterations: NUMBER_OF_ITERATIONS_DEFAULT,
167+
});
168+
169+
});
170+
171+
/**
172+
* @param {{
173+
* name: string,
174+
* input: string,
175+
* output: string,
176+
* iterations?: number
177+
* check?: (decoder: ChunkedContentDecoder) => void,
178+
* }} params
179+
*/
180+
function test_parse_output({ name, input, output, check, iterations = NUMBER_OF_ITERATIONS_DEFAULT}) {
181+
it(name, async function() {
182+
for (let i = 0; i < iterations; ++i) {
183+
const decoder = new ChunkedContentDecoder();
184+
console.log(`test_parse_output(${name}): decoder input`, input, decoder.get_debug_info());
185+
const readable = new stream.Readable({
186+
read() {
187+
// split at random position
188+
const sp = Math.floor(input.length * Math.random());
189+
this.push(input.slice(0, sp));
190+
this.push(input.slice(sp));
191+
this.push(null);
192+
}
193+
});
194+
const writable = buffer_utils.write_stream();
195+
await stream.promises.pipeline(readable, decoder, writable);
196+
const decoded = buffer_utils.join(writable.buffers, writable.total_length);
197+
console.log(`test_parse_output(${name}): decoder returned`, decoded, decoder.get_debug_info());
198+
assert.deepStrictEqual(decoded, Buffer.from(output));
199+
if (check) check(decoder);
200+
}
201+
});
202+
}
203+
204+
/**
205+
* @param {{
206+
* name: string,
207+
* input: string,
208+
* error_pos?: number,
209+
* iterations?: number
210+
* }} params
211+
*/
212+
function test_parse_error({ name, input, error_pos, iterations = NUMBER_OF_ITERATIONS_DEFAULT }) {
213+
it(name, async function() {
214+
for (let i = 0; i < iterations; ++i) {
215+
const decoder = new ChunkedContentDecoder();
216+
console.log(`test_parse_error(${name}): decoder input`, input, decoder.get_debug_info());
217+
console.log(name, 'decode', decoder);
218+
try {
219+
const readable = new stream.Readable({
220+
read() {
221+
// split at random position
222+
const sp = Math.floor(input.length * Math.random());
223+
this.push(input.slice(0, sp));
224+
this.push(input.slice(sp));
225+
this.push(null);
226+
}
227+
});
228+
const writable = buffer_utils.write_stream();
229+
await stream.promises.pipeline(readable, decoder, writable);
230+
const decoded = buffer_utils.join(writable.buffers, writable.total_length);
231+
console.log(`test_parse_error(${name}): decoder returned`, decoded, decoder.get_debug_info());
232+
assert.fail('Should have failed');
233+
} catch (err) {
234+
if (err.message === 'Should have failed') throw err;
235+
console.log(`test_parse_error(${name}): decoder caught`, err, decoder.get_debug_info());
236+
if (error_pos !== undefined) {
237+
assert.strictEqual(decoder.stream_pos, error_pos);
238+
}
239+
}
240+
}
241+
});
242+
}
243+
244+
245+
/**
246+
* create_trailers will return a single string with the number of trailers
247+
* @param {number} number_of_trailers
248+
* @returns string
249+
*/
250+
function create_trailers(number_of_trailers) {
251+
const trailers = [];
252+
for (let index = 1; index <= number_of_trailers; ++index) {
253+
const trailer = `x-trailer-${index}:value\r\n`;
254+
trailers.push(trailer);
255+
}
256+
return trailers.join('');
257+
}
258+
259+
});

0 commit comments

Comments
 (0)