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

http,https: align options of https's server with http's #38992

Closed

Conversation

@Ayase-252
Copy link
Member

@Ayase-252 Ayase-252 commented Jun 10, 2021

Add maxHeaderSize options for https server. The underlying logic has been implemented in #30570, but https has not adapt to that change.

Fixes: #38954
Refs: #30570

Copy link
Member

@addaleax addaleax left a comment

We should not special-case maxHeaderSize, or copy the validation code, imo.

https://nodejs.org/api/https.html#https_https_createserver_options_requestlistener claims that all “options from tls.createServer(), tls.createSecureContext() and http.createServer()” are supported – if that’s not already the case, then the goal should be to make that the case (in particular, all of the options supported in http.createServer() should be supported, and ideally by validating and setting them using the same code).

@Ayase-252
Copy link
Member Author

@Ayase-252 Ayase-252 commented Jun 10, 2021

@addaleax

Thanks,
After a cursory looking, IncomingMessage and ServerResponse are supported in #15752.
insecureHTTPParser and maxHeaderSize options are only two option unsupported in HTTPS. IIUC, we should set them as the same way as http do, and test them under the same test case?

@addaleax
Copy link
Member

@addaleax addaleax commented Jun 10, 2021

@Ayase-252 Maybe https and http could just share their code for validating the options and setting them on this in a separate function?

function storeHttpOptions(options) {
  if (options.maxHeaderSize) {
    validate(options.maxHeaderSize);
    this.maxHeaderSize = options.maxHeaderSize;
  }
  // ... for the other options
}

// in http:
function Server(options) {
  // ....
  storeHttpOptions.call(this, options);
  net.Server.call(this, options);
  // ....
}

// in https:
function Server(options) {
  // ....
  storeHttpOptions.call(this, options);
  tls.Server.call(this, options);
  // ....
}
@Ayase-252 Ayase-252 added the wip label Jun 10, 2021
@Ayase-252 Ayase-252 changed the title https: add maxHeaderSize option for https server http,https: align options of https's server with http's Jun 11, 2021
@Ayase-252 Ayase-252 force-pushed the Ayase-252:feature/https/add-max-header-size branch from 2243db8 to 146ce9a Jun 17, 2021
@Ayase-252 Ayase-252 added http and removed wip labels Jun 17, 2021
@Ayase-252
Copy link
Member Author

@Ayase-252 Ayase-252 commented Jun 17, 2021

I think it is ready for review. @addaleax PTAL.

Copy link
Member

@addaleax addaleax left a comment

Thank you :)

@targos
targos approved these changes Jun 20, 2021
@github-actions
Copy link

@github-actions github-actions bot commented Jun 26, 2021

Landed in 8954e03...8a8c7f4

@github-actions github-actions bot closed this Jun 26, 2021
nodejs-github-bot added a commit that referenced this pull request Jun 26, 2021
Fixes: #38954

PR-URL: #38992
Refs: #30570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@Ayase-252 Ayase-252 deleted the Ayase-252:feature/https/add-max-header-size branch Jun 29, 2021
@Ayase-252 Ayase-252 restored the Ayase-252:feature/https/add-max-header-size branch Jul 2, 2021
@Ayase-252 Ayase-252 deleted the Ayase-252:feature/https/add-max-header-size branch Jul 2, 2021
targos added a commit that referenced this pull request Jul 11, 2021
Fixes: #38954

PR-URL: #38992
Refs: #30570
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants