http: fix validation of "Link" header #46466

SRHerzog · 2023-02-01T20:39:10Z

Updated regex for "Link" header validation to better match the
specification in RFC 8288 section 3. Does not check for valid URI
format but handles the rest of the header more permissively than
before. Alternative to another outstanding PR that disables validation
entirely.

Fixes: #46453
Refs: https://www.rfc-editor.org/rfc/rfc8288.html#section-3
Refs: #46464

anonrig

I'm +1 on this. Can you fix the linting issue as well as your commit message?

SRHerzog · 2023-02-02T05:02:50Z

I'm +1 on this. Can you fix the linting issue as well as your commit message?

I updated the commit message and changed the string from backticks to single quotes with escapes to pass linting. Thanks for the help!

anonrig · 2023-02-02T05:10:08Z

Can you also add a test?

lib/internal/validators.js

nodejs-github-bot · 2023-02-02T17:00:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49312/

nodejs-github-bot · 2023-02-03T03:21:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49325/

test/parallel/test-http-early-hints-invalid-argument-value.js

mcollina

lgtm

nodejs-github-bot · 2023-02-03T16:43:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49340/

lib/internal/validators.js

test/parallel/test-http-early-hints.js

jasnell

LGTM with a nit about expanding the tests a bit more

lpinca · 2023-02-08T19:54:22Z

lib/internal/validators.js

+  (not necessarily a valid URI reference) followed by zero or more
+  link-params separated by semicolons.
+*/
+const linkValueRegExp = /^(?:<[^>]*>)(?:\s*;\s*[^;"\s]+(?:=(")?[^;"\s]*\1)?)*$/;


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

FWIW the original is also vulnerable.

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

How do you know?

How can we fix it?

How do you know?

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

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.

Thanks for reference.

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

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.

(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.)

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.)

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.

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

mcollina · 2023-02-17T11:43:16Z

@nodejs/build could you take a look? This seems an odd failure: https://ci.nodejs.org/job/node-test-commit/59852/console

richardlau · 2023-02-17T15:02:55Z

@nodejs/build could you take a look? This seems an odd failure: https://ci.nodejs.org/job/node-test-commit/59852/console

11:18:44 + git rev-parse origin/main
11:18:44 + REBASE_ONTO=96c720e98f4ea80103a9d240ae8072190a226729
11:18:44 + git rebase --committer-date-is-author-date 96c720e98f4ea80103a9d240ae8072190a226729
11:18:45 Rebasing (1/6)
Auto-merging lib/internal/validators.js
11:18:45 CONFLICT (content): Merge conflict in lib/internal/validators.js
11:18:45 error: could not apply 2e6e618a56b... http: fix validation of "Link" header

It has failed to rebase this PR onto main (96c720e at the time). Possibly fe514bf has changed the file enough to confuse git?

mcollina · 2023-02-17T17:00:57Z

@SRHerzog could you review?

Updated regex for "Link" header validation to better match the specification in RFC 8288 section 3. Does not check for valid URI format but handles the rest of the header more permissively than before. Alternative to another outstanding PR that disables validation entirely. Fixes: nodejs#46453 Refs: https://www.rfc-editor.org/rfc/rfc8288.html#section-3 Refs: nodejs#46464

SRHerzog · 2023-02-17T17:29:53Z

There was a new comma in validators.js in main. Rebased successfully.

mcollina

lgtm

nodejs-github-bot · 2023-02-21T17:31:55Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49844/

nodejs-github-bot · 2023-02-21T22:47:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49859/

nodejs-github-bot · 2023-02-23T09:54:36Z

Landed in d0531eb

Updated regex for "Link" header validation to better match the specification in RFC 8288 section 3. Does not check for valid URI format but handles the rest of the header more permissively than before. Alternative to another outstanding PR that disables validation entirely. Fixes: #46453 Refs: https://www.rfc-editor.org/rfc/rfc8288.html#section-3 Refs: #46464 PR-URL: #46466 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 1, 2023

anonrig approved these changes Feb 2, 2023

View reviewed changes

SRHerzog force-pushed the main branch from 1dfd09c to ffdb710 Compare February 2, 2023 04:58

climba03003 reviewed Feb 2, 2023

View reviewed changes

lib/internal/validators.js Outdated Show resolved Hide resolved

bnoordhuis reviewed Feb 2, 2023

View reviewed changes

lib/internal/validators.js Outdated Show resolved Hide resolved

anonrig added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 2, 2023

anonrig approved these changes Feb 2, 2023

View reviewed changes

anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 2, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 2, 2023

ShogunPanda mentioned this pull request Feb 3, 2023

http: relax writeEarlyHints validations #46464

Open

ShogunPanda reviewed Feb 3, 2023

View reviewed changes

test/parallel/test-http-early-hints-invalid-argument-value.js Outdated Show resolved Hide resolved

mcollina approved these changes Feb 3, 2023

View reviewed changes

aymen94 approved these changes Feb 3, 2023

View reviewed changes

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2023

anonrig approved these changes Feb 3, 2023

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2023

jasnell reviewed Feb 4, 2023

View reviewed changes

lib/internal/validators.js Show resolved Hide resolved

jasnell reviewed Feb 4, 2023

View reviewed changes

test/parallel/test-http-early-hints.js Show resolved Hide resolved

jasnell approved these changes Feb 4, 2023

View reviewed changes

This was referenced Feb 5, 2023

CI Reliability 2023-02-05 nodejs/reliability#495

Open

CI Reliability 2023-02-06 nodejs/reliability#497

Open

SRHerzog force-pushed the main branch from eec8b6d to 185ee01 Compare February 6, 2023 19:08

ShogunPanda approved these changes Feb 6, 2023

View reviewed changes

lpinca reviewed Feb 8, 2023

View reviewed changes

SRHerzog added 6 commits February 17, 2023 11:11

revert error message, improve regex, add test

a0281cf

minor test refactor

88eaafd

update tests and comments

e24f2fd

remove redundant test

2327a58

fix overlong string

8c49924

SRHerzog force-pushed the main branch from 507fb5a to 8c49924 Compare February 17, 2023 17:28

This was referenced Feb 18, 2023

CI Reliability 2023-02-18 nodejs/reliability#499

Open

CI Reliability 2023-02-19 nodejs/reliability#500

Open

mcollina approved these changes Feb 21, 2023

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2023

This was referenced Feb 22, 2023

CI Reliability 2023-02-22 nodejs/reliability#503

Open

CI Reliability 2023-02-23 nodejs/reliability#504

Open

mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed needs-ci PRs that need a full CI run. labels Feb 23, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 23, 2023

nodejs-github-bot merged commit d0531eb into nodejs:main Feb 23, 2023

targos mentioned this pull request Mar 14, 2023

v19.8.0 release proposal #47087

Merged

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

ShogunPanda mentioned this pull request Jun 5, 2023

Overzealous link header validation in writeEarlyHints #46453

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: fix validation of "Link" header #46466

http: fix validation of "Link" header #46466

SRHerzog commented Feb 1, 2023 •

edited

Loading

anonrig left a comment

SRHerzog commented Feb 2, 2023

anonrig commented Feb 2, 2023

nodejs-github-bot commented Feb 2, 2023

nodejs-github-bot commented Feb 3, 2023

mcollina left a comment

nodejs-github-bot commented Feb 3, 2023

jasnell left a comment

lpinca Feb 8, 2023

lpinca Feb 8, 2023 •

edited

Loading

ShogunPanda Feb 13, 2023

lpinca Feb 13, 2023

ShogunPanda Feb 13, 2023

Trott Feb 18, 2023

Trott Feb 18, 2023

Trott Feb 18, 2023

lpinca Feb 18, 2023 •

edited

Loading

lpinca Feb 18, 2023

mcollina commented Feb 17, 2023

richardlau commented Feb 17, 2023

mcollina commented Feb 17, 2023

SRHerzog commented Feb 17, 2023

mcollina left a comment

nodejs-github-bot commented Feb 21, 2023

nodejs-github-bot commented Feb 21, 2023

nodejs-github-bot commented Feb 23, 2023

http: fix validation of "Link" header #46466

http: fix validation of "Link" header #46466

Conversation

SRHerzog commented Feb 1, 2023 • edited Loading

anonrig left a comment

Choose a reason for hiding this comment

SRHerzog commented Feb 2, 2023

anonrig commented Feb 2, 2023

nodejs-github-bot commented Feb 2, 2023

nodejs-github-bot commented Feb 3, 2023

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 3, 2023

jasnell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpinca Feb 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Feb 17, 2023

richardlau commented Feb 17, 2023

mcollina commented Feb 17, 2023

SRHerzog commented Feb 17, 2023

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 21, 2023

nodejs-github-bot commented Feb 21, 2023

nodejs-github-bot commented Feb 23, 2023

SRHerzog commented Feb 1, 2023 •

edited

Loading

lpinca Feb 8, 2023 •

edited

Loading

lpinca Feb 18, 2023 •

edited

Loading