Skip to content

http: fix validation of "Link" header #46466

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

Merged
nodejs-github-bot merged 6 commits into from
Feb 23, 2023
Merged

http: fix validation of "Link" header #46466

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
revert error message, improve regex, add test
  • Loading branch information
@SRHerzog
SRHerzog committed Feb 17, 2023
commit a0281cf927088b0dfcb8d6f1597b4b72ab88d660
2 changes: 1 addition & 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,7 @@ function validateUnion(value, name, union) {
}
}

const linkValueRegExp = /^(?:<[^>]*>)(?:\s*;\s*[^;"]+(?:=(")?[^;"]*\1)?)*$/;
const linkValueRegExp = /^(?:<[^>]*>)(?:\s*;\s*[^;"\s]+(?:=(")?[^;"\s]*\1)?)*$/;
Comment thread
SRHerzog marked this conversation as resolved.
Copy link
Copy Markdown
Member

@lpinca lpinca Feb 8, 2023

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
Copy Markdown
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
Copy Markdown
Contributor

@ShogunPanda ShogunPanda Feb 13, 2023

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
Copy Markdown
Member

@lpinca lpinca Feb 13, 2023

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
Copy Markdown
Contributor

@ShogunPanda ShogunPanda Feb 13, 2023

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
Copy Markdown
Member

@Trott Trott Feb 18, 2023

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
Copy Markdown
Member

@Trott Trott Feb 18, 2023

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
Copy Markdown
Member

@Trott Trott Feb 18, 2023

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
Copy Markdown
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
Copy Markdown
Member

@lpinca lpinca Feb 18, 2023

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
35 changes: 35 additions & 0 deletions test/parallel/test-http-early-hints-invalid-argument-value.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
'use strict';
const common = require('../common');
const assert = require('node:assert');
const http = require('node:http');
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({
link: '</>; '
});

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) => {
Comment thread
SRHerzog marked this conversation as resolved.
Outdated
debug(`Caught an exception: ${JSON.stringify(err)}`);
if (err.name === 'AssertionError') throw err;
assert.strictEqual(err.code, 'ERR_INVALID_ARG_VALUE');
process.exit();
});
}));
4 changes: 2 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,7 @@ const testResBody = 'response content\n';
res.writeEarlyHints({
link: [
'</styles.css>; rel=preload; as=style',
'</scripts.js>; rel=preload; as=script',
'</scripts.js>; rel=preload; as=script; crossorigin',
Comment thread
SRHerzog marked this conversation as resolved.
]
});

Expand All @@ -75,7 +75,7 @@ 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>; rel=preload; as=script; crossorigin'
);
}));

Expand Down