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

Jump to bottom

http: fix validation of "Link" header #46466

Merged
merged 6 commits into from
Feb 23, 2023
Merged

http: fix validation of "Link" header #46466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9947fd9
http: fix validation of "Link" header
SRHerzog Feb 17, 2023
a0281cf
revert error message, improve regex, add test
SRHerzog Feb 17, 2023
88eaafd
minor test refactor
SRHerzog Feb 3, 2023
e24f2fd
update tests and comments
SRHerzog Feb 6, 2023
2327a58
remove redundant test
SRHerzog Feb 6, 2023
8c49924
fix overlong string
SRHerzog Feb 9, 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
10 changes: 9 additions & 1 deletion lib/internal/validators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,15 @@ function validateUnion(value, name, union) {
}
}

const linkValueRegExp = /^(?:<[^>]*>;)\s*(?:rel=(")?[^;"]*\1;?)\s*(?:(?:as|anchor|title|crossorigin|disabled|fetchpriority|rel|referrerpolicy)=(")?[^;"]*\2)?$/;
/*
The rules for the Link header field are described here:
https://www.rfc-editor.org/rfc/rfc8288.html#section-3

This regex validates any string surrounded by angle brackets
(not necessarily a valid URI reference) followed by zero or more
link-params separated by semicolons.
*/
const linkValueRegExp = /^(?:<[^>]*>)(?:\s*;\s*[^;"\s]+(?:=(")?[^;"\s]*\1)?)*$/;
SRHerzog marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

It probably does not matter as the header is sent by the server but this regex is vulnerable to ReDoS.

Copy link
Member

@lpinca lpinca Feb 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

FWIW the original is also vulnerable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not questioning your point, I'm asking as I want to learn on this matter:

  1. How do you know?
  2. How can we fix it?

Copy link
Member

Choose a reason for hiding this comment

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

  1. How do you know?

https://github.com/makenowjust-labs/recheck

  1. How can we fix it?

Usually it is possible to tweak the regex. I'm not sure if it is possible in this case. I did not spend time on it. The input is "trusted" so I think it does not worth the effort.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for reference.

I also think it's not worth the effort. Shall we just insert a comment for future knowledge?

Copy link
Member

Choose a reason for hiding this comment

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

The input is "trusted" so I think it does not worth the effort.

I'd be concerned that we'd start leveraging this at a future time for something and expose it to end users. There should at least be a comment, IMO.

Copy link
Member

Choose a reason for hiding this comment

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

(To be 100% clear: If we're certain that this is not something that will accept user input, yeah, we don't need to fix it. But let's add a comment explaining.)

Copy link
Member

Choose a reason for hiding this comment

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

This regex (and the existing one) will fail to do the right thing if a quotation mark is backslash-escaped inside the string, right? (I only skimmed the spec so I apologize if I'm wrong!) Are we doing the whole "using a regexp when a parsing algorithm is what is needed" thing? (This is a question, but not a blocking objection or anything. The current regexp would have the same issue if this one has that issue.)

Copy link
Member

@lpinca lpinca Feb 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You could have a for loop that loops backward from the string examining each character. That might actually be more performant than a regex.

It could also start from the beginning of the string, but yes, I think that it would be better.

Copy link
Member

Choose a reason for hiding this comment

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

We can add a TODO comment and do this in a follow-up PR.


/**
* @param {any} value
Expand Down
66 changes: 41 additions & 25 deletions test/parallel/test-http-early-hints-invalid-argument.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,44 @@ const debug = require('node:util').debuglog('test');

const testResBody = 'response content\n';

const server = http.createServer(common.mustCall((req, res) => {
debug('Server sending early hints...');
res.writeEarlyHints('bad argument type');

debug('Server sending full response...');
res.end(testResBody);
}));

server.listen(0, common.mustCall(() => {
const req = http.request({
port: server.address().port, path: '/'
});

req.end();
debug('Client sending request...');

req.on('information', 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_TYPE');
process.exit(0);
});
}));
{
const server = http.createServer(common.mustCall((req, res) => {
debug('Server sending early hints...');
assert.throws(() => {
res.writeEarlyHints('bad argument type');
}, (err) => err.code === 'ERR_INVALID_ARG_TYPE');

assert.throws(() => {
res.writeEarlyHints({
link: '</>; '
});
}, (err) => err.code === 'ERR_INVALID_ARG_VALUE');

assert.throws(() => {
res.writeEarlyHints({
link: 'rel=preload; </scripts.js>'
});
}, (err) => err.code === 'ERR_INVALID_ARG_VALUE');

assert.throws(() => {
res.writeEarlyHints({
link: 'invalid string'
});
}, (err) => err.code === 'ERR_INVALID_ARG_VALUE');

debug('Server sending full response...');
res.end(testResBody);
server.close();
}));

server.listen(0, common.mustCall(() => {
const req = http.request({
port: server.address().port, path: '/'
});

req.end();
debug('Client sending request...');

req.on('information', common.mustNotCall());
}));
}
6 changes: 4 additions & 2 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 @@ -58,7 +58,8 @@ const testResBody = 'response content\n';
res.writeEarlyHints({
link: [
'</styles.css>; rel=preload; as=style',
'</scripts.js>; rel=preload; as=script',
'</scripts.js>; crossorigin; rel=preload; as=script',
'</scripts.js>; rel=preload; as=script; crossorigin',
SRHerzog marked this conversation as resolved.
Show resolved Hide resolved
]
});

Expand All @@ -75,7 +76,8 @@ const testResBody = 'response content\n';
req.on('information', common.mustCall((res) => {
assert.strictEqual(
res.headers.link,
'</styles.css>; rel=preload; as=style, </scripts.js>; rel=preload; as=script'
'</styles.css>; rel=preload; as=style, </scripts.js>; crossorigin; ' +
'rel=preload; as=script, </scripts.js>; rel=preload; as=script; crossorigin'
);
}));

Expand Down