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.

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

Add maxHeadersCount support for http(s)2 server #32388

Closed
LongTengDao opened this issue Mar 20, 2020 · 9 comments
Closed

Add maxHeadersCount support for http(s)2 server #32388

LongTengDao opened this issue Mar 20, 2020 · 9 comments
Labels
feature request Issues that request new features to be added to Node.js. http2 Issues or PRs related to the http2 subsystem.

Comments

@LongTengDao
Copy link
Contributor

LongTengDao commented Mar 20, 2020

Is your feature request related to a problem? Please describe.

In http(s)1, user can limit server.maxHeadersCount for server, to avoid malicious request (like hash collision attack), because normal headers wont be more than 20 in fact.

But in http(s)2, user has no chance to prevent parse that, even if user check request.rawHeaders.length/2>2000 && response.writeHead(400), the request.headers already been parsed.

Describe the solution you'd like

Add server.maxHeadersCount, just like http(s)1 did.

Describe alternatives you've considered

@addaleax addaleax added feature request Issues that request new features to be added to Node.js. http2 Issues or PRs related to the http2 subsystem. labels Mar 20, 2020
@addaleax
Copy link
Member

Sounds good to me – maxHeaderSize could be added along with that. 👍

@LongTengDao
Copy link
Contributor Author

I found maxHeaderListPairs in docs accidentally. Are they same?

@addaleax
Copy link
Member

@LongTengDao Yeah, actually, that’s the same thing. It’s really unfortunate how the naming mismatches – I guess that means adding an alias makes sense here?

@LongTengDao
Copy link
Contributor Author

LongTengDao commented Mar 20, 2020

I didn't found that, not only because the name, but also the way to set... one is property of instance, one is in creating options.

I guess that means adding an alias makes sense here?

I have no idea. Maybe deprecate server.maxHeadersCount in http1 to maxHeaderListPairs also works?

By the way, via reading the source, I found the effects of server.maxHeadersCount seems to prevent excessive request.headers assigning:

  // Propagate headers limit from server instance to parser
  if (typeof server.maxHeadersCount === 'number') {
    parser.maxHeaderPairs = server.maxHeadersCount << 1;
  }
  let n = headers.length;

  // If parser.maxHeaderPairs <= 0 assume that there's no limit.
  if (parser.maxHeaderPairs > 0)
    n = MathMin(n, parser.maxHeaderPairs);

  incoming._addHeaderLines(headers, n);

and prevent excessive request.rawHeaders pushing which depends on if the headers are recieved in two times or not:

function parserOnHeaders(headers, url) {
  // Once we exceeded headers limit - stop collecting them
  if (this.maxHeaderPairs <= 0 ||
      this._headers.length < this.maxHeaderPairs) {
    this._headers = this._headers.concat(headers);
  }
  this._url += url;
}

Does maxHeaderListPairs do exact same? I couldn't found more useful infomation by search in source.

@himself65
Copy link
Member

@LongTengDao You should read this PR #16676. And use git blame instead of global text search

@himself65
Copy link
Member

himself65 commented Mar 25, 2020

I read the source code these days.
And I found that http use maxHeadersCount, but http2 use maxHeaderListPairs, which they use the different C++ library and implement.
Unify them not easy work. 😟

@himself65
Copy link
Member

I've done this issue which adds aliases for http1/2 named maxHeadersCount and maxHeaderListPairs,
but I'm doubting that if we really need this kind of thing.
Adding the aliases will add the complexity of the code, that we have two option mange the one thing. Actually, I disapprove of this feature.
🤔

@LongTengDao
Copy link
Contributor Author

I've done this issue which adds aliases for http1/2 named maxHeadersCount and maxHeaderListPairs,
but I'm doubting that if we really need this kind of thing.
Adding the aliases will add the complexity of the code, that we have two option mange the one thing. Actually, I disapprove of this feature.
🤔

Maybe just add explain in docs will resolve this (if we can't remove one of them, and just reserve anothor)?

@himself65
Copy link
Member

add doc is a good idea

rexagod added a commit to rexagod/node that referenced this issue Jun 6, 2020
codebytere pushed a commit that referenced this issue Jun 27, 2020
Fixes: #32388

PR-URL: #33519
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jun 30, 2020
Fixes: #32388

PR-URL: #33519
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jul 10, 2020
Fixes: #32388

PR-URL: #33519
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jul 12, 2020
Fixes: #32388

PR-URL: #33519
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this issue Jul 14, 2020
Fixes: #32388

PR-URL: #33519
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants