Skip to content

Commit 81a08c2

Browse files
committed
stream: improve Web Compression spec compliance
Pass rejectGarbageAfterEnd: true to createInflateRaw() and createBrotliDecompress(), matching the behavior already in place for deflate and gzip. The Compression Streams spec treats any data following a valid compressed payload as an error. When the underlying Node.js stream throws synchronously from write() (e.g. zlib rejects an invalid chunk type), destroy the stream so that the readable side is also errored. Without this, the readable side hangs forever waiting for data that will never arrive. Introduce a kValidateChunk callback option in the webstreams adapter layer. Compression streams use this to validate that chunks are BufferSource instances not backed by SharedArrayBuffer, replacing the previous monkey-patching of the underlying handle's write method. Unskip WPT compression bad-chunks tests which now run instead of hang and mark the remaining expected failures.
1 parent 41d5b41 commit 81a08c2

File tree

6 files changed

+322
-23
lines changed

6 files changed

+322
-23
lines changed

lib/internal/webstreams/adapters.js

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ const {
1111
SafePromiseAll,
1212
SafePromisePrototypeFinally,
1313
SafeSet,
14+
StringPrototypeStartsWith,
15+
Symbol,
1416
TypeError,
1517
TypedArrayPrototypeGetBuffer,
1618
TypedArrayPrototypeGetByteLength,
@@ -39,6 +41,7 @@ const {
3941
Writable,
4042
Readable,
4143
Duplex,
44+
Transform,
4245
destroy,
4346
} = require('stream');
4447

@@ -94,6 +97,8 @@ const { UV_EOF } = internalBinding('uv');
9497

9598
const encoder = new TextEncoder();
9699

100+
const kValidateChunk = Symbol('kValidateChunk');
101+
97102
// Collect all negative (error) ZLIB codes and Z_NEED_DICT
98103
const ZLIB_FAILURES = new SafeSet([
99104
...ArrayPrototypeFilter(
@@ -139,9 +144,10 @@ function handleKnownInternalErrors(cause) {
139144

140145
/**
141146
* @param {Writable} streamWritable
147+
* @param {object} [options]
142148
* @returns {WritableStream}
143149
*/
144-
function newWritableStreamFromStreamWritable(streamWritable) {
150+
function newWritableStreamFromStreamWritable(streamWritable, options = kEmptyObject) {
145151
// Not using the internal/streams/utils isWritableNodeStream utility
146152
// here because it will return false if streamWritable is a Duplex
147153
// whose writable option is false. For a Duplex that is not writable,
@@ -220,12 +226,26 @@ function newWritableStreamFromStreamWritable(streamWritable) {
220226
if (!streamWritable.writableObjectMode && isArrayBuffer(chunk)) {
221227
chunk = new Uint8Array(chunk);
222228
}
223-
if (streamWritable.writableNeedDrain || !streamWritable.write(chunk)) {
224-
backpressurePromise = PromiseWithResolvers();
225-
return SafePromisePrototypeFinally(
226-
backpressurePromise.promise, () => {
227-
backpressurePromise = undefined;
228-
});
229+
try {
230+
options[kValidateChunk]?.(chunk);
231+
if (streamWritable.writableNeedDrain || !streamWritable.write(chunk)) {
232+
backpressurePromise = PromiseWithResolvers();
233+
return SafePromisePrototypeFinally(
234+
backpressurePromise.promise, () => {
235+
backpressurePromise = undefined;
236+
});
237+
}
238+
} catch (error) {
239+
// When the underlying stream is a Transform (e.g.
240+
// CompressionStream), a sync throw must also destroy the
241+
// stream so the readable side is errored too. Without this
242+
// the readable side hangs forever. This replicates the
243+
// TransformStream semantics: error both sides on any throw
244+
// in the transform path.
245+
if (streamWritable instanceof Transform) {
246+
destroy(streamWritable, error);
247+
}
248+
throw error;
229249
}
230250
},
231251

@@ -662,9 +682,14 @@ function newReadableWritablePairFromDuplex(duplex, options = kEmptyObject) {
662682
return { readable, writable };
663683
}
664684

685+
const writableOptions = {
686+
__proto__: null,
687+
[kValidateChunk]: options[kValidateChunk],
688+
};
689+
665690
const writable =
666691
isWritable(duplex) ?
667-
newWritableStreamFromStreamWritable(duplex) :
692+
newWritableStreamFromStreamWritable(duplex, writableOptions) :
668693
new WritableStream();
669694

670695
if (!isWritable(duplex))
@@ -1064,4 +1089,5 @@ module.exports = {
10641089
newStreamDuplexFromReadableWritablePair,
10651090
newWritableStreamFromStreamBase,
10661091
newReadableStreamFromStreamBase,
1092+
kValidateChunk,
10671093
};

lib/internal/webstreams/compression.js

Lines changed: 35 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,27 @@ const {
77

88
const {
99
newReadableWritablePairFromDuplex,
10+
kValidateChunk,
1011
} = require('internal/webstreams/adapters');
1112

1213
const { customInspect } = require('internal/webstreams/util');
1314

15+
const {
16+
isArrayBufferView,
17+
isSharedArrayBuffer,
18+
} = require('internal/util/types');
19+
1420
const {
1521
customInspectSymbol: kInspect,
1622
kEnumerableProperty,
1723
} = require('internal/util');
1824

25+
const {
26+
codes: {
27+
ERR_INVALID_ARG_TYPE,
28+
},
29+
} = require('internal/errors');
30+
1931
const { createEnumConverter } = require('internal/webidl');
2032

2133
let zlib;
@@ -24,6 +36,18 @@ function lazyZlib() {
2436
return zlib;
2537
}
2638

39+
// Per the Compression Streams spec, chunks must be BufferSource
40+
// (ArrayBuffer or ArrayBufferView not backed by SharedArrayBuffer).
41+
function validateBufferSourceChunk(chunk) {
42+
if (isArrayBufferView(chunk) && isSharedArrayBuffer(chunk.buffer)) {
43+
throw new ERR_INVALID_ARG_TYPE(
44+
'chunk',
45+
['Buffer', 'TypedArray', 'DataView'],
46+
chunk,
47+
);
48+
}
49+
}
50+
2751
const formatConverter = createEnumConverter('CompressionFormat', [
2852
'deflate',
2953
'deflate-raw',
@@ -62,7 +86,9 @@ class CompressionStream {
6286
this.#handle = lazyZlib().createBrotliCompress();
6387
break;
6488
}
65-
this.#transform = newReadableWritablePairFromDuplex(this.#handle);
89+
this.#transform = newReadableWritablePairFromDuplex(this.#handle, {
90+
[kValidateChunk]: validateBufferSourceChunk,
91+
});
6692
}
6793

6894
/**
@@ -108,25 +134,23 @@ class DecompressionStream {
108134
});
109135
break;
110136
case 'deflate-raw':
111-
this.#handle = lazyZlib().createInflateRaw();
137+
this.#handle = lazyZlib().createInflateRaw({
138+
rejectGarbageAfterEnd: true,
139+
});
112140
break;
113141
case 'gzip':
114142
this.#handle = lazyZlib().createGunzip({
115143
rejectGarbageAfterEnd: true,
116144
});
117145
break;
118146
case 'brotli':
119-
this.#handle = lazyZlib().createBrotliDecompress();
147+
this.#handle = lazyZlib().createBrotliDecompress({
148+
rejectGarbageAfterEnd: true,
149+
});
120150
break;
121151
}
122-
this.#transform = newReadableWritablePairFromDuplex(this.#handle);
123-
124-
this.#handle.on('error', (err) => {
125-
if (this.#transform?.writable &&
126-
!this.#transform.writable.locked &&
127-
typeof this.#transform.writable.abort === 'function') {
128-
this.#transform.writable.abort(err);
129-
}
152+
this.#transform = newReadableWritablePairFromDuplex(this.#handle, {
153+
[kValidateChunk]: validateBufferSourceChunk,
130154
});
131155
}
132156

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
'use strict';
2+
// Flags: --no-warnings --expose-internals
3+
require('../common');
4+
const assert = require('assert');
5+
const test = require('node:test');
6+
const { Duplex, Transform, Writable } = require('stream');
7+
const {
8+
newWritableStreamFromStreamWritable,
9+
newReadableWritablePairFromDuplex,
10+
} = require('internal/webstreams/adapters');
11+
12+
// Verify that when the underlying Node.js stream throws synchronously from
13+
// write(), the writable web stream properly rejects.
14+
15+
test('WritableStream from Node.js stream handles sync write throw', async () => {
16+
const error = new TypeError('invalid chunk');
17+
const writable = new Writable({
18+
write() {
19+
throw error;
20+
},
21+
});
22+
23+
const ws = newWritableStreamFromStreamWritable(writable);
24+
const writer = ws.getWriter();
25+
26+
await assert.rejects(writer.write('bad'), (err) => {
27+
assert.strictEqual(err, error);
28+
return true;
29+
});
30+
31+
// Standalone writable should not be destroyed on sync write error
32+
assert.strictEqual(writable.destroyed, false);
33+
});
34+
35+
test('Transform-backed pair destroys on sync write throw', async () => {
36+
const error = new TypeError('invalid chunk');
37+
const transform = new Transform({
38+
transform() {
39+
throw error;
40+
},
41+
});
42+
43+
const { writable, readable } = newReadableWritablePairFromDuplex(transform);
44+
const writer = writable.getWriter();
45+
const reader = readable.getReader();
46+
47+
await assert.rejects(writer.write('bad'), (err) => {
48+
assert.strictEqual(err, error);
49+
return true;
50+
});
51+
52+
// The transform must be destroyed so the readable side is also errored
53+
assert.strictEqual(transform.destroyed, true);
54+
55+
// The readable side should also be errored, not hang
56+
await assert.rejects(reader.read(), (err) => {
57+
assert.strictEqual(err, error);
58+
return true;
59+
});
60+
});
61+
62+
test('Duplex-backed pair does NOT destroy on sync write throw', async () => {
63+
const error = new TypeError('invalid chunk');
64+
const duplex = new Duplex({
65+
read() {},
66+
write() {
67+
throw error;
68+
},
69+
});
70+
71+
const { writable, readable } = newReadableWritablePairFromDuplex(duplex);
72+
const writer = writable.getWriter();
73+
74+
await assert.rejects(writer.write('bad'), (err) => {
75+
assert.strictEqual(err, error);
76+
return true;
77+
});
78+
79+
// A plain Duplex should NOT be destroyed on sync write error
80+
assert.strictEqual(duplex.destroyed, false);
81+
82+
// The readable side should still be usable
83+
const reader = readable.getReader();
84+
reader.cancel();
85+
});
86+
87+
test('WritableStream from Node.js stream - valid writes still work', async () => {
88+
const chunks = [];
89+
const writable = new Writable({
90+
write(chunk, _encoding, cb) {
91+
chunks.push(chunk);
92+
cb();
93+
},
94+
});
95+
96+
const ws = newWritableStreamFromStreamWritable(writable);
97+
const writer = ws.getWriter();
98+
99+
await writer.write(Buffer.from('hello'));
100+
await writer.write(Buffer.from(' world'));
101+
await writer.close();
102+
103+
assert.strictEqual(Buffer.concat(chunks).toString(), 'hello world');
104+
});
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const test = require('node:test');
5+
const { CompressionStream, DecompressionStream } = require('stream/web');
6+
7+
// Verify that writing invalid (non-BufferSource) chunks to
8+
// CompressionStream and DecompressionStream properly rejects
9+
// on both the write and the read side, instead of hanging.
10+
11+
const badChunks = [
12+
{ name: 'undefined', value: undefined, code: 'ERR_INVALID_ARG_TYPE' },
13+
{ name: 'null', value: null, code: 'ERR_STREAM_NULL_VALUES' },
14+
{ name: 'number', value: 3.14, code: 'ERR_INVALID_ARG_TYPE' },
15+
{ name: 'object', value: {}, code: 'ERR_INVALID_ARG_TYPE' },
16+
{ name: 'array', value: [65], code: 'ERR_INVALID_ARG_TYPE' },
17+
{
18+
name: 'SharedArrayBuffer',
19+
value: new SharedArrayBuffer(1),
20+
code: 'ERR_INVALID_ARG_TYPE',
21+
},
22+
{
23+
name: 'Uint8Array backed by SharedArrayBuffer',
24+
value: new Uint8Array(new SharedArrayBuffer(1)),
25+
code: 'ERR_INVALID_ARG_TYPE',
26+
},
27+
];
28+
29+
for (const format of ['deflate', 'deflate-raw', 'gzip', 'brotli']) {
30+
for (const { name, value, code } of badChunks) {
31+
const expected = { name: 'TypeError', code };
32+
33+
test(`CompressionStream rejects bad chunk (${name}) for ${format}`, async () => {
34+
const cs = new CompressionStream(format);
35+
const writer = cs.writable.getWriter();
36+
const reader = cs.readable.getReader();
37+
38+
const writePromise = writer.write(value);
39+
const readPromise = reader.read();
40+
41+
await assert.rejects(writePromise, expected);
42+
await assert.rejects(readPromise, expected);
43+
});
44+
45+
test(`DecompressionStream rejects bad chunk (${name}) for ${format}`, async () => {
46+
const ds = new DecompressionStream(format);
47+
const writer = ds.writable.getWriter();
48+
const reader = ds.readable.getReader();
49+
50+
const writePromise = writer.write(value);
51+
const readPromise = reader.read();
52+
53+
await assert.rejects(writePromise, expected);
54+
await assert.rejects(readPromise, expected);
55+
});
56+
}
57+
}

0 commit comments

Comments
 (0)