Add maxHeadersCount support for http(s)2 server

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

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

I found maxHeaderListPairs in docs accidentally. Are they same?

@LongTengDao

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

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.

@LongTengDao

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

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

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

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

add doc is a good idea

doc: specify maxHeaderCount alias for maxHeaderListPairs

Navigation Menu

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

addaleax commented on Mar 20, 2020

LongTengDao commented on Mar 20, 2020

addaleax commented on Mar 20, 2020

LongTengDao commented on Mar 20, 2020

himself65 commented on Mar 24, 2020

himself65 commented on Mar 25, 2020

himself65 commented on Mar 26, 2020

LongTengDao commented on Mar 27, 2020

himself65 commented on Mar 27, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions