http: fix validation of "Link" header by SRHerzog · Pull Request #46466 · nodejs/node · GitHub

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

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

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!

Can you also add a test?

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

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

lgtm

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

LGTM with a nit about expanding the tests a bit more

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.

linting is failing

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

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

@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?

@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: #46453 Refs: https://www.rfc-editor.org/rfc/rfc8288.html#section-3 Refs: #46464

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

lgtm

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

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

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 <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

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

climba03003 reviewed Feb 2, 2023

View reviewed changes

Comment thread lib/internal/validators.js Outdated

bnoordhuis reviewed Feb 2, 2023

View reviewed changes

Comment thread lib/internal/validators.js Outdated

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 outstanding review comments, 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

Closed

ShogunPanda reviewed Feb 3, 2023

View reviewed changes

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

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

Comment thread lib/internal/validators.js Outdated

jasnell reviewed Feb 4, 2023

View reviewed changes

Comment thread test/parallel/test-http-early-hints.js Outdated

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

ShogunPanda approved these changes Feb 6, 2023

View reviewed changes

lpinca reviewed Feb 8, 2023

View reviewed changes

lpinca approved these changes Feb 8, 2023

View reviewed changes

Comment thread test/parallel/test-http-early-hints.js Outdated

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

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

http: fix validation of "Link" header by SRHerzog · Pull Request #46466 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: fix validation of "Link" header - #46466

http: fix validation of "Link" header#46466nodejs-github-bot merged 6 commits intonodejs:mainnodejs/node:mainfrom SRHerzog:mainCopy head branch name to clipboard

Conversation

SRHerzog commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anonrig left a comment

Choose a reason for hiding this comment

Uh oh!

SRHerzog commented Feb 2, 2023

Uh oh!

anonrig commented Feb 2, 2023

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 2, 2023

Uh oh!

nodejs-github-bot commented Feb 3, 2023

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 3, 2023

Uh oh!

Uh oh!

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpinca Feb 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina commented Feb 9, 2023

Uh oh!

nodejs-github-bot commented Feb 17, 2023

Uh oh!

mcollina commented Feb 17, 2023

Uh oh!

richardlau commented Feb 17, 2023

Uh oh!

mcollina commented Feb 17, 2023

Uh oh!

SRHerzog commented Feb 17, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 21, 2023

Uh oh!

nodejs-github-bot commented Feb 21, 2023

Uh oh!

nodejs-github-bot commented Feb 23, 2023

Uh oh!

Reviewers

http: fix validation of "Link" header#46466
nodejs-github-bot merged 6 commits into
nodejs:mainfrom
SRHerzog:main

SRHerzog commented Feb 1, 2023 •
edited
Loading

lpinca Feb 8, 2023 •
edited
Loading

lpinca Feb 18, 2023 •
edited
Loading