koajs / koa

Expressive middleware for node.js using ES2017 async functions
https://koajs.com
MIT License
35.2k stars 3.23k forks source link

[fix] Request.secure returns false for HTTP2 connections after recieving RST_STREAM #1736

Open ctusch opened 1 year ago

ctusch commented 1 year ago

Describe the bug

Node.js version: 19.4.0

OS version: Windows 10

Description: I'm using koa with http2.createSecureServer and during some requests Request.secure turns from initial true to false. I've seen that the property indirectly checks whether Request.req.socket.encrypted is truthy. This is only true if socket is of type TLSSocket which is initially the case. But when the bug occurs Http2ServerRequest.socket switches its underlying type to ServerHttp2Stream.

As far as I can tell it has to do with the client sending a RST_STREAM and node.js closing the HttpStream (aborted event is triggered on the stream). But I would argue this should either leave the secure flag true or throw an error when trying to access it. Solely relying on socket.encrypted doesn't seem to be enough for HTTP2.

I've stumbled upon this error while trying to set a secure cookie which lead to a pretty misleading error: Cannot send secure cookie over unencrypted connection.

Actual behavior

Request.secure is false.

Expected behavior

Request.secure should be true or throw an error.

Code to reproduce

const http2 = require('http2');
const fs = require('fs');
const Koa = require('koa');

const app = new Koa();

app.use(async (context) => {
  await new Promise(res => setTimeout(res, 100));
  console.log('socket: ' + context.socket.constructor.name)
  console.log('secure: ' + context.secure);
  return context.response.body = 'test';
});

const cert = fs.readFileSync('localhost.crt');
const key = fs.readFileSync('localhost.key');

http2.createSecureServer({ cert, key }, app.callback()).listen(1234);
const client = http2.connect('https://localhost:1234', { ca: cert });

const successfulRequest = client.request();
successfulRequest.on('data', (chunk) => console.log(chunk.toString()));

const cancelledRequest = client.request();
cancelledRequest.on('ready', () => cancelledRequest.destroy());

// output:
// socket: bound TLSSocket
// secure: true
// socket: bound ServerHttp2Stream
// secure: false
// test

Checklist

ctusch commented 1 year ago

I've added code to reproduce.