Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: relax writeEarlyHints validations #46464

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+206 −136
Open

http: relax writeEarlyHints validations #46464

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78dd37e
http: relax writeEarlyHints validations
khalsah Feb 1, 2023
8eacdb7
Fix lint error
khalsah Feb 2, 2023
9d71829
Don't send 103 if empty, add tests
khalsah Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions lib/_http_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ const { ConnectionsList } = internalBinding('http_parser');
const {
kUniqueHeaders,
parseUniqueHeadersOption,
OutgoingMessage
OutgoingMessage,
validateHeaderName,
validateHeaderValue,
} = require('_http_outgoing');
const {
kOutHeaders,
Expand Down Expand Up @@ -84,7 +86,6 @@ const {
const {
validateInteger,
validateBoolean,
validateLinkHeaderValue,
validateObject
} = require('internal/validators');
const Buffer = require('buffer').Buffer;
Expand Down Expand Up @@ -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);
anonrig marked this conversation as resolved.
Show resolved Hide resolved

if (ArrayIsArray(hint)) {
for (let i = 0; i < hint.length; i++) {
head += key + ': ' + hint[i] + '\r\n';
}
anonrig marked this conversation as resolved.
Show resolved Hide resolved
} else {
head += key + ': ' + hints[key] + '\r\n';
}
}
Expand Down
18 changes: 1 addition & 17 deletions lib/internal/http2/compat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ const {
const {
validateFunction,
validateString,
validateLinkHeaderValue,
validateObject,
} = require('internal/validators');
const {
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the need to remove regexp but why don't we keep the primitive checks as it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check skips sending 103 Early Hints all together if no link header is present. Which would be confusing if you're trying to send early hints with some other header. Admittedly I'm not aware of any current need to send 103 Early Hints without a link header, but by my reading the RFC doesn't mandate the link header in any way.

}

const stream = this[kStream];

if (stream.headersSent || this[kState].closed)
return false;

stream.additionalHeaders({
...headers,
...hints,
anonrig marked this conversation as resolved.
Show resolved Hide resolved
[HTTP2_HEADER_STATUS]: HTTP_STATUS_EARLY_HINTS,
'Link': linkHeaderValue,
});

return true;
Expand Down
56 changes: 0 additions & 56 deletions lib/internal/validators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 "</styles.css>; 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 "</styles.css>; rel=preload; as=style"'
);
}

module.exports = {
isInt32,
isUint32,
Expand All @@ -545,6 +490,5 @@ module.exports = {
validateUndefined,
validateUnion,
validateAbortSignal,
validateLinkHeaderValue,
validateInternalField,
};
45 changes: 3 additions & 42 deletions test/parallel/test-http-early-hints.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,47 +99,6 @@ const testResBody = 'response content\n';
}));
}

{
// Happy flow - empty array

const server = http.createServer(common.mustCall((req, res) => {
anonrig marked this conversation as resolved.
Show resolved Hide resolved
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

Expand Down Expand Up @@ -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 = '';
Expand Down
42 changes: 0 additions & 42 deletions test/parallel/test-http2-compat-write-early-hints-invalid-argument-value.js
View file
Open in desktop

This file was deleted.

4 changes: 3 additions & 1 deletion test/parallel/test-http2-compat-write-early-hints.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
anonrig marked this conversation as resolved.
Show resolved Hide resolved
}));

req.on('response', common.mustCall((headers) => {
assert.strictEqual(headers[':status'], 200);
Expand Down
13 changes: 0 additions & 13 deletions test/parallel/test-validators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const {
validateString,
validateInt32,
validateUint32,
validateLinkHeaderValue,
} = require('internal/validators');
const { MAX_SAFE_INTEGER, MIN_SAFE_INTEGER } = Number;
const outOfRangeError = {
Expand Down Expand Up @@ -155,15 +154,3 @@ const invalidArgValueError = {
code: 'ERR_INVALID_ARG_TYPE'
}));
}

{
// validateLinkHeaderValue type validation.
[
['</styles.css>; rel=preload; as=style', '</styles.css>; rel=preload; as=style'],
['</styles.css>; rel=preload; title=hello', '</styles.css>; rel=preload; title=hello'],
['</styles.css>; rel=preload; crossorigin=hello', '</styles.css>; rel=preload; crossorigin=hello'],
['</styles.css>; rel=preload; disabled=true', '</styles.css>; rel=preload; disabled=true'],
['</styles.css>; rel=preload; fetchpriority=high', '</styles.css>; rel=preload; fetchpriority=high'],
['</styles.css>; rel=preload; referrerpolicy=origin', '</styles.css>; rel=preload; referrerpolicy=origin'],
].forEach(([value, expected]) => assert.strictEqual(validateLinkHeaderValue(value), expected));
}