From 78dd37e2502974d5145db956af44c1f8393c2634 Mon Sep 17 00:00:00 2001 From: "Hargobind S. Khalsa" Date: Wed, 1 Feb 2023 10:04:20 -0800 Subject: [PATCH 1/3] http: relax writeEarlyHints validations Removes the requirement that every call to writeEarlyHints include a `link` header. While the `link` header is clearly the most common usage of `103 Early Hints`, I could find no requirement to include a `link` header as part of [RFC8297](https://www.rfc-editor.org/rfc/rfc8297.html). Additionally this removes the existing incorrect validation of the Link header format in favor of only validating that it is a valid header value. While the validation could be updated to better match [RFC8288 Section 3](https://www.rfc-editor.org/rfc/rfc8288.html#section-3), it appears it would be the only place in the node.js code base where we proactively validate header values beyond verifying they are valid at the HTTP protocol layer. Fixes: https://github.com/nodejs/node/issues/46453 --- lib/_http_server.js | 27 ++++----- lib/internal/http2/compat.js | 18 +----- lib/internal/validators.js | 56 ------------------- test/parallel/test-http-early-hints.js | 45 +-------------- ...rite-early-hints-invalid-argument-value.js | 42 -------------- .../test-http2-compat-write-early-hints.js | 4 +- test/parallel/test-validators.js | 13 ----- 7 files changed, 19 insertions(+), 186 deletions(-) delete mode 100644 test/parallel/test-http2-compat-write-early-hints-invalid-argument-value.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 7e96a0b2bb3397..6fecbbbd21bd82 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -51,7 +51,9 @@ const { ConnectionsList } = internalBinding('http_parser'); const { kUniqueHeaders, parseUniqueHeadersOption, - OutgoingMessage + OutgoingMessage, + validateHeaderName, + validateHeaderValue, } = require('_http_outgoing'); const { kOutHeaders, @@ -84,7 +86,6 @@ const { const { validateInteger, validateBoolean, - validateLinkHeaderValue, validateObject } = require('internal/validators'); const Buffer = require('buffer').Buffer; @@ -305,20 +306,16 @@ ServerResponse.prototype.writeEarlyHints = function writeEarlyHints(hints, cb) { validateObject(hints, 'hints'); - if (hints.link === null || hints.link === undefined) { - return; - } - - const link = validateLinkHeaderValue(hints.link); - - if (link.length === 0) { - return; - } - - head += 'Link: ' + link + '\r\n'; - for (const key of ObjectKeys(hints)) { - if (key !== 'link') { + const hint = hints[key]; + validateHeaderName(key); + validateHeaderValue(key, hint); + + if (ArrayIsArray(hint)) { + for (let i = 0; i < hint.length; i++) { + head += key + ': ' + hint[i] + '\r\n'; + } + } else { head += key + ': ' + hints[key] + '\r\n'; } } diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index dec5b734a2490f..f48cee4c2cb159 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -55,7 +55,6 @@ const { const { validateFunction, validateString, - validateLinkHeaderValue, validateObject, } = require('internal/validators'); const { @@ -850,29 +849,14 @@ class Http2ServerResponse extends Stream { writeEarlyHints(hints) { validateObject(hints, 'hints'); - const headers = { __proto__: null }; - - const linkHeaderValue = validateLinkHeaderValue(hints.link); - - for (const key of ObjectKeys(hints)) { - if (key !== 'link') { - headers[key] = hints[key]; - } - } - - if (linkHeaderValue.length === 0) { - return false; - } - const stream = this[kStream]; if (stream.headersSent || this[kState].closed) return false; stream.additionalHeaders({ - ...headers, + ...hints, [HTTP2_HEADER_STATUS]: HTTP_STATUS_EARLY_HINTS, - 'Link': linkHeaderValue, }); return true; diff --git a/lib/internal/validators.js b/lib/internal/validators.js index 50b3016ab78ec2..05c1f2a02a3c57 100644 --- a/lib/internal/validators.js +++ b/lib/internal/validators.js @@ -459,67 +459,12 @@ function validateUnion(value, name, union) { } } -const linkValueRegExp = /^(?:<[^>]*>;)\s*(?:rel=(")?[^;"]*\1;?)\s*(?:(?:as|anchor|title|crossorigin|disabled|fetchpriority|rel|referrerpolicy)=(")?[^;"]*\2)?$/; - -/** - * @param {any} value - * @param {string} name - */ -function validateLinkHeaderFormat(value, name) { - if ( - typeof value === 'undefined' || - !RegExpPrototypeExec(linkValueRegExp, value) - ) { - throw new ERR_INVALID_ARG_VALUE( - name, - value, - 'must be an array or string of format "; rel=preload; as=style"' - ); - } -} - const validateInternalField = hideStackFrames((object, fieldKey, className) => { if (typeof object !== 'object' || object === null || !ObjectPrototypeHasOwnProperty(object, fieldKey)) { throw new ERR_INVALID_ARG_TYPE('this', className, object); } }); -/** - * @param {any} hints - * @return {string} - */ -function validateLinkHeaderValue(hints) { - if (typeof hints === 'string') { - validateLinkHeaderFormat(hints, 'hints'); - return hints; - } else if (ArrayIsArray(hints)) { - const hintsLength = hints.length; - let result = ''; - - if (hintsLength === 0) { - return result; - } - - for (let i = 0; i < hintsLength; i++) { - const link = hints[i]; - validateLinkHeaderFormat(link, 'hints'); - result += link; - - if (i !== hintsLength - 1) { - result += ', '; - } - } - - return result; - } - - throw new ERR_INVALID_ARG_VALUE( - 'hints', - hints, - 'must be an array or string of format "; rel=preload; as=style"' - ); -} - module.exports = { isInt32, isUint32, @@ -545,6 +490,5 @@ module.exports = { validateUndefined, validateUnion, validateAbortSignal, - validateLinkHeaderValue, validateInternalField, }; diff --git a/test/parallel/test-http-early-hints.js b/test/parallel/test-http-early-hints.js index 7183d9516f6dda..6cd017eb59f753 100644 --- a/test/parallel/test-http-early-hints.js +++ b/test/parallel/test-http-early-hints.js @@ -99,47 +99,6 @@ const testResBody = 'response content\n'; })); } -{ - // Happy flow - empty array - - const server = http.createServer(common.mustCall((req, res) => { - debug('Server sending early hints...'); - res.writeEarlyHints({ - link: [] - }); - - debug('Server sending full response...'); - res.end(testResBody); - })); - - server.listen(0, common.mustCall(() => { - const req = http.request({ - port: server.address().port, path: '/' - }); - debug('Client sending request...'); - - req.on('information', common.mustNotCall()); - - req.on('response', common.mustCall((res) => { - let body = ''; - - assert.strictEqual(res.statusCode, 200, `Final status code was ${res.statusCode}, not 200.`); - - res.on('data', (chunk) => { - body += chunk; - }); - - res.on('end', common.mustCall(() => { - debug('Got full response.'); - assert.strictEqual(body, testResBody); - server.close(); - })); - })); - - req.end(); - })); -} - { // Happy flow - object argument with string @@ -256,7 +215,9 @@ const testResBody = 'response content\n'; }); debug('Client sending request...'); - req.on('information', common.mustNotCall()); + req.on('information', common.mustCall((info) => { + assert.strictEqual(info.statusCode, 103) + })); req.on('response', common.mustCall((res) => { let body = ''; diff --git a/test/parallel/test-http2-compat-write-early-hints-invalid-argument-value.js b/test/parallel/test-http2-compat-write-early-hints-invalid-argument-value.js deleted file mode 100644 index d640f13fae6f5e..00000000000000 --- a/test/parallel/test-http2-compat-write-early-hints-invalid-argument-value.js +++ /dev/null @@ -1,42 +0,0 @@ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) common.skip('missing crypto'); - -const assert = require('node:assert'); -const http2 = require('node:http2'); -const debug = require('node:util').debuglog('test'); - -const testResBody = 'response content'; - -{ - // Invalid link header value - - const server = http2.createServer(); - - server.on('request', common.mustCall((req, res) => { - debug('Server sending early hints...'); - res.writeEarlyHints({ link: BigInt(100) }); - - debug('Server sending full response...'); - res.end(testResBody); - })); - - server.listen(0); - - server.on('listening', common.mustCall(() => { - const client = http2.connect(`http://localhost:${server.address().port}`); - const req = client.request(); - - debug('Client sending request...'); - - req.on('headers', common.mustNotCall()); - - process.on('uncaughtException', (err) => { - debug(`Caught an exception: ${JSON.stringify(err)}`); - if (err.name === 'AssertionError') throw err; - assert.strictEqual(err.code, 'ERR_INVALID_ARG_VALUE'); - process.exit(0); - }); - })); -} diff --git a/test/parallel/test-http2-compat-write-early-hints.js b/test/parallel/test-http2-compat-write-early-hints.js index d1f26d7c20bbc0..29e63d1c186b31 100644 --- a/test/parallel/test-http2-compat-write-early-hints.js +++ b/test/parallel/test-http2-compat-write-early-hints.js @@ -128,7 +128,9 @@ const testResBody = 'response content'; debug('Client sending request...'); - req.on('headers', common.mustNotCall()); + req.on('headers', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 103); + })); req.on('response', common.mustCall((headers) => { assert.strictEqual(headers[':status'], 200); diff --git a/test/parallel/test-validators.js b/test/parallel/test-validators.js index a40139678eee65..63cf42e306605c 100644 --- a/test/parallel/test-validators.js +++ b/test/parallel/test-validators.js @@ -12,7 +12,6 @@ const { validateString, validateInt32, validateUint32, - validateLinkHeaderValue, } = require('internal/validators'); const { MAX_SAFE_INTEGER, MIN_SAFE_INTEGER } = Number; const outOfRangeError = { @@ -155,15 +154,3 @@ const invalidArgValueError = { code: 'ERR_INVALID_ARG_TYPE' })); } - -{ - // validateLinkHeaderValue type validation. - [ - ['; rel=preload; as=style', '; rel=preload; as=style'], - ['; rel=preload; title=hello', '; rel=preload; title=hello'], - ['; rel=preload; crossorigin=hello', '; rel=preload; crossorigin=hello'], - ['; rel=preload; disabled=true', '; rel=preload; disabled=true'], - ['; rel=preload; fetchpriority=high', '; rel=preload; fetchpriority=high'], - ['; rel=preload; referrerpolicy=origin', '; rel=preload; referrerpolicy=origin'], - ].forEach(([value, expected]) => assert.strictEqual(validateLinkHeaderValue(value), expected)); -} From 8eacdb7ac73150d9a37a6ce52caf0205ec85f7f8 Mon Sep 17 00:00:00 2001 From: "Hargobind S. Khalsa" Date: Thu, 2 Feb 2023 01:28:49 -0800 Subject: [PATCH 2/3] Fix lint error --- test/parallel/test-http-early-hints.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-http-early-hints.js b/test/parallel/test-http-early-hints.js index 6cd017eb59f753..81aff945b22b2c 100644 --- a/test/parallel/test-http-early-hints.js +++ b/test/parallel/test-http-early-hints.js @@ -216,7 +216,7 @@ const testResBody = 'response content\n'; debug('Client sending request...'); req.on('information', common.mustCall((info) => { - assert.strictEqual(info.statusCode, 103) + assert.strictEqual(info.statusCode, 103); })); req.on('response', common.mustCall((res) => { From 9d71829430c144041fa34211c217834f250a3a27 Mon Sep 17 00:00:00 2001 From: "Hargobind S. Khalsa" Date: Fri, 3 Feb 2023 07:48:44 -0800 Subject: [PATCH 3/3] Don't send 103 if empty, add tests --- lib/_http_server.js | 11 ++ lib/internal/http2/compat.js | 11 ++ test/parallel/test-http-early-hints.js | 131 +++++++++++++++++- .../test-http2-compat-write-early-hints.js | 88 ++++++++++++ 4 files changed, 239 insertions(+), 2 deletions(-) diff --git a/lib/_http_server.js b/lib/_http_server.js index 6fecbbbd21bd82..d118d870f64100 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -23,9 +23,11 @@ const { ArrayIsArray, + ArrayPrototypeEvery, Error, MathMin, ObjectKeys, + ObjectValues, ObjectSetPrototypeOf, RegExpPrototypeExec, ReflectApply, @@ -306,6 +308,15 @@ ServerResponse.prototype.writeEarlyHints = function writeEarlyHints(hints, cb) { validateObject(hints, 'hints'); + const empty = ArrayPrototypeEvery( + ObjectValues(hints), + (hint) => ArrayIsArray(hint) && hint.length === 0 + ); + + if (empty) { + return false; + } + for (const key of ObjectKeys(hints)) { const hint = hints[key]; validateHeaderName(key); diff --git a/lib/internal/http2/compat.js b/lib/internal/http2/compat.js index f48cee4c2cb159..eacbf362b71275 100644 --- a/lib/internal/http2/compat.js +++ b/lib/internal/http2/compat.js @@ -2,11 +2,13 @@ const { ArrayIsArray, + ArrayPrototypeEvery, ArrayPrototypePush, Boolean, FunctionPrototypeBind, ObjectAssign, ObjectKeys, + ObjectValues, ObjectPrototypeHasOwnProperty, Proxy, ReflectApply, @@ -849,6 +851,15 @@ class Http2ServerResponse extends Stream { writeEarlyHints(hints) { validateObject(hints, 'hints'); + const empty = ArrayPrototypeEvery( + ObjectValues(hints), + (hint) => ArrayIsArray(hint) && hint.length === 0 + ); + + if (empty) { + return false; + } + const stream = this[kStream]; if (stream.headersSent || this[kState].closed) diff --git a/test/parallel/test-http-early-hints.js b/test/parallel/test-http-early-hints.js index 81aff945b22b2c..57968c89f19cad 100644 --- a/test/parallel/test-http-early-hints.js +++ b/test/parallel/test-http-early-hints.js @@ -99,6 +99,47 @@ const testResBody = 'response content\n'; })); } +{ + // Happy flow - empty array + + const server = http.createServer(common.mustCall((req, res) => { + debug('Server sending early hints...'); + res.writeEarlyHints({ + link: [] + }); + + debug('Server sending full response...'); + res.end(testResBody); + })); + + server.listen(0, common.mustCall(() => { + const req = http.request({ + port: server.address().port, path: '/' + }); + debug('Client sending request...'); + + req.on('information', common.mustNotCall()); + + req.on('response', common.mustCall((res) => { + let body = ''; + + assert.strictEqual(res.statusCode, 200, `Final status code was ${res.statusCode}, not 200.`); + + res.on('data', (chunk) => { + body += chunk; + }); + + res.on('end', common.mustCall(() => { + debug('Got full response.'); + assert.strictEqual(body, testResBody); + server.close(); + })); + })); + + req.end(); + })); +} + { // Happy flow - object argument with string @@ -215,8 +256,94 @@ const testResBody = 'response content\n'; }); debug('Client sending request...'); - req.on('information', common.mustCall((info) => { - assert.strictEqual(info.statusCode, 103); + req.on('information', common.mustNotCall()); + + req.on('response', common.mustCall((res) => { + let body = ''; + + assert.strictEqual(res.statusCode, 200, `Final status code was ${res.statusCode}, not 200.`); + + res.on('data', (chunk) => { + body += chunk; + }); + + res.on('end', common.mustCall(() => { + debug('Got full response.'); + assert.strictEqual(body, testResBody); + server.close(); + })); + })); + + req.end(); + })); +} + +{ + // Happy flow - capitalized header name + + const server = http.createServer(common.mustCall((req, res) => { + debug('Server sending early hints...'); + res.writeEarlyHints({ + Link: '; rel=preload; as=style' + }); + + debug('Server sending full response...'); + res.end(testResBody); + })); + + server.listen(0, common.mustCall(() => { + const req = http.request({ + port: server.address().port, path: '/' + }); + + debug('Client sending request...'); + + req.on('information', common.mustCall((res) => { + assert.strictEqual(res.headers.link, '; rel=preload; as=style'); + })); + + req.on('response', common.mustCall((res) => { + let body = ''; + + assert.strictEqual(res.statusCode, 200, `Final status code was ${res.statusCode}, not 200.`); + + res.on('data', (chunk) => { + body += chunk; + }); + + res.on('end', common.mustCall(() => { + debug('Got full response.'); + assert.strictEqual(body, testResBody); + server.close(); + })); + })); + + req.end(); + })); +} + +{ + // Happy flow - non-link header + + const server = http.createServer(common.mustCall((req, res) => { + debug('Server sending early hints...'); + res.writeEarlyHints({ + 'x-hint': 'value' + }); + + debug('Server sending full response...'); + res.end(testResBody); + })); + + server.listen(0, common.mustCall(() => { + const req = http.request({ + port: server.address().port, path: '/' + }); + + debug('Client sending request...'); + + req.on('information', common.mustCall((res) => { + assert.strictEqual(res.headers['x-hint'], 'value'); })); req.on('response', common.mustCall((res) => { diff --git a/test/parallel/test-http2-compat-write-early-hints.js b/test/parallel/test-http2-compat-write-early-hints.js index 29e63d1c186b31..0a581cc0428bf9 100644 --- a/test/parallel/test-http2-compat-write-early-hints.js +++ b/test/parallel/test-http2-compat-write-early-hints.js @@ -122,6 +122,92 @@ const testResBody = 'response content'; server.listen(0); + server.on('listening', common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + + debug('Client sending request...'); + + req.on('headers', common.mustNotCall()); + + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 200); + })); + + let data = ''; + req.on('data', common.mustCallAtLeast((d) => data += d)); + + req.on('end', common.mustCall(() => { + debug('Got full response.'); + assert.strictEqual(data, testResBody); + client.close(); + server.close(); + })); + })); +} + +{ + // Happy flow - capitalized header name + + const server = http2.createServer(); + + server.on('request', common.mustCall((req, res) => { + debug('Server sending early hints...'); + res.writeEarlyHints({ + Link: '; rel=preload; as=style' + }); + + debug('Server sending full response...'); + res.end(testResBody); + })); + + server.listen(0); + + server.on('listening', common.mustCall(() => { + const client = http2.connect(`http://localhost:${server.address().port}`); + const req = client.request(); + + debug('Client sending request...'); + + req.on('headers', common.mustCall((headers) => { + assert.notStrictEqual(headers, undefined); + assert.strictEqual(headers[':status'], 103); + assert.strictEqual(headers.link, '; rel=preload; as=style'); + })); + + req.on('response', common.mustCall((headers) => { + assert.strictEqual(headers[':status'], 200); + })); + + let data = ''; + req.on('data', common.mustCallAtLeast((d) => data += d)); + + req.on('end', common.mustCall(() => { + debug('Got full response.'); + assert.strictEqual(data, testResBody); + client.close(); + server.close(); + })); + })); +} + +{ + // Happy flow - non-link header + + const server = http2.createServer(); + + server.on('request', common.mustCall((req, res) => { + debug('Server sending early hints...'); + res.writeEarlyHints({ + 'x-hint': 'value' + }); + + debug('Server sending full response...'); + res.end(testResBody); + })); + + server.listen(0); + server.on('listening', common.mustCall(() => { const client = http2.connect(`http://localhost:${server.address().port}`); const req = client.request(); @@ -129,7 +215,9 @@ const testResBody = 'response content'; debug('Client sending request...'); req.on('headers', common.mustCall((headers) => { + assert.notStrictEqual(headers, undefined); assert.strictEqual(headers[':status'], 103); + assert.strictEqual(headers['x-hint'], 'value'); })); req.on('response', common.mustCall((headers) => {