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

maxHeaderSize option on HTTPS server is not working #38954

Closed
SantanM opened this issue Jun 7, 2021 · 8 comments
Closed

maxHeaderSize option on HTTPS server is not working #38954

SantanM opened this issue Jun 7, 2021 · 8 comments
Labels
https Issues or PRs related to the https subsystem.

Comments

@SantanM
Copy link

SantanM commented Jun 7, 2021

Darwin Santans-MacBook-Pro.local 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64
  • Version: v12.19.0
  • Platform: MacOS
  • Subsystem: HTTPS module

What steps will reproduce the bug?

Creating a server using HTTPS module with option maxHeaderSize, like below. While increasing maxHeaderSize, the server still shows 431 error for large header.

const server = https.createServer({
  key: process.env.SSL_PRIVATE_KEY,
  cert: process.env.SSL_PUBLIC_CERT,
  maxHeaderSize: 8192*6
}, app).listen(PORT, () => {
  log.info(`🚀 Approuter started on port ${PORT}`);
console.log(server.maxHeaderSize); // outputs undefined
});

How often does it reproduce? Is there a required condition?

It seems (according to the documentation) the options of HTTP server are applicable to HTTPS, but the value does not seem to work in my case (seeing 431 error always)

What is the expected behavior?

In doing so, I am expecting the large header sent to the server is accepted and does not return 431 error.

What do you see instead?

HTTP 431 error

Additional information

The test cases of maxHeaderSize option are covered only for HTTP Server module and not HTTPS. I don't think it has been tested with HTTPS module.

@SantanM SantanM changed the title add maxHeaderSize to HTTPS server not working maxHeaderSize option on HTTPS server is not working Jun 7, 2021
@Ayase-252 Ayase-252 added the https Issues or PRs related to the https subsystem. label Jun 7, 2021
@bl-ue
Copy link
Contributor

bl-ue commented Jun 7, 2021

Linking for reference: nodejs/help#3401

@Ayase-252
Copy link
Member

Hi, @SantanM
Thanks for bug reporting,

How do you generate headers for your request? If possible, could you provide a code snippet containing both server and request? It would be very helpful.

I've written a case with testing tools of Node.js, it passes on v12.19.0 (interestingly, fails on v14 and above, it seems maxHeaderSize is ignored (always 200)? but it may be different issue I guess...)

'use strict'

const assert = require('assert');
const common = require('../common');
const https = require('https');
const fixtures = require('../common/fixtures');

const options = {
  key: fixtures.readKey('agent1-key.pem'),
  cert: fixtures.readKey('agent1-cert.pem')
};

const maxHeaderSize = 8192

const body = 'hello world\n';
const serverCallback = function (req, res) {
  res.writeHead(200, { 'content-type': 'text/plain' });
  res.end(body);
};

// test header size is larger than maxHeaderSize
{
  const server = https.createServer({
    ...options,
    maxHeaderSize
  }, serverCallback)

  server.listen(0, common.mustCall(() => {
    const serverPort = server.address().port
    const reqOptions = {
      hostname: '127.0.0.1',
      port: serverPort,
      path: '/',
      method: 'GET',
      rejectUnauthorized: false,
      headers: {
        "h": 'a'.repeat(maxHeaderSize + 1)
      }
    };

    const req = https.request(reqOptions, common.mustCall((res) => {
      assert.strictEqual(res.statusCode, 431)

      res.on('data', function (d) {
      });

      res.on('end', common.mustCall(() => {
        server.close()
      }))
    })).end()
  }))
}

// test header size is in the range
{
  const server = https.createServer({
    ...options,
    maxHeaderSize
  }, serverCallback)

  server.listen(0, common.mustCall(() => {
    const serverPort = server.address().port
    const reqOptions = {
      hostname: '127.0.0.1',
      port: serverPort,
      path: '/',
      method: 'GET',
      rejectUnauthorized: false,
      headers: {
        // some other header overhead
        "h": 'a'.repeat(maxHeaderSize - 200)
      }
    };

    const req = https.request(reqOptions, common.mustCall((res) => {
      assert.strictEqual(res.statusCode, 200)

      res.on('data', function (d) {
      });

      res.on('end', common.mustCall(() => {
        server.close()
      }))
    })).end()
  }))
}

@Ayase-252
Copy link
Member

Well, never mind, maxHeaderSize is unconfigurable for https.createServer, investigating further.

@SantanM
Copy link
Author

SantanM commented Jun 10, 2021

Thanks for checking. Keep us updated on the next plan.

@richardlau
Copy link
Member

Being able to set maxHeaderSize on a server was added by #30570 but that was introduced in Node.js 13.3.0 and is not present in Node.js 12. There was a suggestion that it was "backportable to Node 12 with a bit of work" (#30570 (comment)) but that "bit of work" never happened and is unlikely to now that Node.js 12 is in maintenance and does not get new features.

@Ayase-252
Copy link
Member

Ayase-252 commented Jun 10, 2021

@SantanM

I think maxHeaderSize has never been implemented in https Server. #30570 added maxHeaderSize for http.Server but not for https. Therefore,

https.createServer
options Accepts options from tls.createServer(), tls.createSecureContext() and http.createServer().

The doc is wrong. https.createServer([options][, requestListener]) will not accept all options of http.createServer(). At least, maxHeaderSize is not supported.

For now, I think the only way to control the maximun header size of a HTTPS Server is via CLI option --max-http-header-size.

Also, it seems fairly simple to bring maxHeaderSize into https.Server, I may open a PR in days.

Ayase-252 added a commit to Ayase-252/node that referenced this issue Jun 10, 2021
Ayase-252 added a commit to Ayase-252/node that referenced this issue Jun 10, 2021
Ayase-252 added a commit to Ayase-252/node that referenced this issue Jun 17, 2021
targos pushed a commit that referenced this issue Jul 11, 2021
Fixes: #38954

PR-URL: #38992
Refs: #30570
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@SantanM
Copy link
Author

SantanM commented Aug 6, 2021

@Ayase-252 - The changes you made will be replicated to lower versions, more particularly to v14.XX? Could you confirm?

@Ayase-252
Copy link
Member

@SantanM I think it has been released in v16.5.0. It has not been backported to v14.x yet. Hopefully soon.

targos pushed a commit that referenced this issue Sep 4, 2021
Fixes: #38954

PR-URL: #38992
Refs: #30570
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants