Skip to content

Commit 91a070d

Browse files
mcollinaaduh95
authored andcommitted
http2: handle errors on compat push response streams
`response.createPushResponse()` wrapped the pushed stream in an `Http2ServerResponse` without attaching an 'error' listener. Request streams get one from `Http2ServerRequest`, but pushed streams have no request, so a peer refusing the push with a non-clean RST_STREAM code (e.g. REFUSED_STREAM) before the response finished emitted an unhandled 'error' on `response.stream` unless the application listened on it. Attach `onStreamError` to the pushed stream before invoking the callback, and make `onStreamError` tolerate the `null` request that core sets on every stream. Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: #67053 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 236355a commit 91a070d

2 files changed

Lines changed: 54 additions & 2 deletions

File tree

‎lib/internal/http2/compat.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,9 +162,9 @@ function onStreamEnd() {
162162

163163
function onStreamError(error) {
164164
// Mirror HTTP/1's IncomingMessage._destroy: forward 'error' to req
165-
// only when a listener is attached.
165+
// only when a listener is attached. Pushed streams have no request.
166166
const request = this[kRequest];
167-
if (request !== undefined && request.listenerCount('error') > 0)
167+
if (request != null && request.listenerCount('error') > 0)
168168
request.emit('error', error);
169169
}
170170

@@ -928,6 +928,10 @@ class Http2ServerResponse extends Stream {
928928
callback(err);
929929
return;
930930
}
931+
// Pushed streams have no Http2ServerRequest to install the stream
932+
// 'error' handler, so attach it here. Otherwise a peer refusing the
933+
// push (e.g. RST_STREAM with REFUSED_STREAM) would crash the process.
934+
stream.on('error', onStreamError);
931935
callback(null, new Http2ServerResponse(stream));
932936
});
933937
}
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
if (!common.hasCrypto)
5+
common.skip('missing crypto');
6+
7+
// A client refusing a push created through the compat API with
8+
// RST_STREAM(REFUSED_STREAM) must not crash the server with an unhandled
9+
// 'error' event on the pushed stream.
10+
11+
const assert = require('assert');
12+
const http2 = require('http2');
13+
14+
const server = http2.createServer(common.mustCall((req, res) => {
15+
res.createPushResponse({ ':path': '/style.css' }, common.mustSucceed((pushRes) => {
16+
pushRes.on('close', common.mustCall());
17+
pushRes.setHeader('content-type', 'text/css');
18+
pushRes.end('body { color: red }');
19+
}));
20+
res.end('hello');
21+
}));
22+
23+
server.listen(0, common.mustCall(() => {
24+
// A zero initial window keeps the pushed body from completing before the
25+
// client refuses it, so the server sees a peer reset with its writable
26+
// side still pending.
27+
const client = http2.connect(`http://localhost:${server.address().port}`, {
28+
settings: { initialWindowSize: 0 },
29+
});
30+
31+
client.on('stream', common.mustCall((pushed) => {
32+
pushed.on('error', common.expectsError({
33+
code: 'ERR_HTTP2_STREAM_ERROR',
34+
}));
35+
pushed.close(http2.constants.NGHTTP2_REFUSED_STREAM);
36+
// Reopen the window so the main response can complete.
37+
client.settings({ initialWindowSize: 65535 });
38+
}));
39+
40+
const req = client.request({ ':path': '/' });
41+
req.resume();
42+
req.on('close', common.mustCall(() => {
43+
assert.strictEqual(req.rstCode, http2.constants.NGHTTP2_NO_ERROR);
44+
client.close();
45+
server.close();
46+
}));
47+
req.end();
48+
}));

0 commit comments

Comments
 (0)