http: check for existance in resetHeadersTimeoutOnReqEnd

[mcollina]

socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: #26366

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2744/pipeline

[mcollina]

cc @nodejs/http

@nodejs/lts @nodejs/release we would likely have to backport this down to 6 for safety, given that we do not know how this condition is triggered.

[mcollina]

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

[richardlau]

socket.parser can be undefined under unknown circumstances.
This is a fix for a bug I cannot reproduce but it is affecting
people.

Fixes: #26357

Shouldn't that be #26366? (Commit message too.)

[mcollina]

@richardlau good spot! Fixed.

[Trott]

Optional typo fix for commit title: s/existance/existence/

[Trott]

Here's a test that reproduces the error in #26366 in current master.

'use strict';

require('../common');

const http = require('http');

const server = http.createServer((req, res) => {
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.write('okay', () => { delete res.socket.parser });
  res.end();
});

server.listen(1337, '127.0.0.1');

const req = http.request({
  port: 1337,
  host: '127.0.0.1',
  method: 'GET',
});

req.end();

[Trott]

Is it worth adding the code in the previous comment (or something like it) as a test?

[mcollina]

I think so. However it’s not clear if we are doing it in core or not, or it is just user specific (somehow).

[Trott]

By the way, #26404 is basically the same thing but on the client end rather than the server end.

[jasnell]

Changing to parser != null would work also and be a bit safer

[lpinca]

I think we need to understand why this happens or we could mask a bug instead of fixing it. It would be great if @bjjenson or @jasine could provide a test case to reproduce the issue.

[mcollina]

The overall problem with supporting a “delete” case is that it could trigger the vulnerability we are trying to protect against.

[mcollina]

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

[mcollina]

Landed in 3c83f93

[mcollina]

@nodejs/lts this should be backported asap to all lines.

[richardlau]

@nodejs/lts this should be backported asap to all lines.

Probably too late for 11.11.0, but ping @BridgeAR.

[BridgeAR]

@richardlau I would rather pull that into the release afterwards.

[lpinca]

I've finally found the root issue behind #26366 or better in https://github.com/eggjs/egg-socket.io.

The problem is that our request.socket is replaced by egg-socket.io with the socket.io Socket. See https://github.com/eggjs/egg-socket.io/blob/9e7f71d835930d3a63c69635488737066f779661/lib/connectionMiddlewareInit.js#L8-L9.

There is nothing wrong with this fix but the problem is in egg-socket.io and may arise again.

I think the regression test added here does not make much sense.