Incorrectly computes HTTP/2 client origin set

[szmarczak]

• Version: 14.12.0
• Platform: Linux solus 5.6.19-158.current #1 SMP PREEMPT Sun Jul 26 14:17:01 UTC 2020 x86_64 GNU/Linux
• Subsystem: http2

What steps will reproduce the bug?

const http2 = require('http2');
const createCert = require('create-cert');

(async () => {
	const server = http2.createSecureServer({
		...(await createCert()),
		origins: [
			'https://example.com'
		]
	});

	server.listen(error => {
		if (error) {
			throw error;
		}

		const session = http2.connect(
			new URL('https://example.com'),
			{
				host: 'localhost',
				port: server.address().port,
				rejectUnauthorized: false
			}
		);

		session.once('remoteSettings', () => {
			console.log('remoteSettings', session.originSet);
		});

		session.once('origin', () => {
			console.log('origin', session.originSet);

			session.close();
			server.close();
		});
	});
})();

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

Always.

What is the expected behavior?

remoteSettings [ 'https://example.com' ]
origin [ 'https://example.com' ]

or throw if there's a mismatch between origin.port and options.port?

What do you see instead?

remoteSettings [ 'https://example.com:38303' ]
origin [ 'https://example.com:38303', 'https://example.com' ]

[drewswanner]

I would be interested on helping with this

[drewswanner]

Hi, So what I am seeing is I believe this is the file that needs to be edited: lib/internal/http2/core.js

function initOriginSet(session) {
let originSet = session[kState].originSet;
if (originSet === undefined) {
const socket = session[kSocket];
session[kState].originSet = originSet = new Set();
if (socket.servername != null) {
let originString = https://${socket.servername};
if (socket.remotePort != null)
originString += :${socket.remotePort};
// We have to ensure that it is a properly serialized
// ASCII origin string. The socket.servername might not
// be properly ASCII encoded.
originSet.add((new URL(originString)).origin);
}
}
return originSet;
}

I am wondering if I should just remove the port from being included? I am thinking that maybe making it optional makes more sense. Does anyone have some guidance how best to resolve this one?

[drewswanner]

@szmarczak after looking at this more I see how this is expected use from what I can tell. I believe this is to allow origin witin this spec https://tools.ietf.org/html/rfc7230#section-2.3. Which it looks like it was implemented with this PR #17935. @jasnell can you confirm if this is correct I believe you would know for sure.

[mmomtchev]

I don't think the RFC applies here - the point of that option is to allow you to send an Origin frame that does not conform to what the RFC specifies from Node's point of view - most probably because of some special routing or tunneling or proxying or whatever.
I think @szmarczak 's is correct, if you overload the origin, Node should send that exact value, even if it violates the RFC from its point of view - leaving the ultimate decision on how it works in your hands.

[mmomtchev]

@szmarczak , I think that the server correctly sends to the client the origin you requested, but keeps an internal originSet with all values. Whether this is correct is surely debatable, but it does not really have an impact on the outside world.

[szmarczak]

That's why I used the word compute. While it has little impact on the outside world, it's got a huge impact on the inside.

[mmomtchev]

In fact, it is the server session that exhibits this debatable behavior. In your code you are referring to the client session originSet which is correct in that particular case according to https://tools.ietf.org/html/draft-ietf-httpbis-origin-frame-06#section-2.3
On the client side, you should get whatever you connected to (url with port) and whatever the server sent to you (url without port)
However the server originSet has both values which I agree that is not very logical, but it is a rather minor problem

Is this what you meant?

[szmarczak]

On the client side, you should get whatever you connected to (url with port)

If that's the case then it's still a bug since I connected to localhost but the requested origin is example.com.

[mmomtchev]

The RFC states that the client should initialize the originSet with the same host name that was used for the SNI for the TLS handshake if there was a SNI, otherwise it should use the DNS name or the IP address of the host it connected to. Once again, it is a debatable edge case - I don't think the code implements this to the letter - I think it will use the URL hostname even if there was not an SNI - but this case is so obscure I don't think it qualifies for a PR 😄

[szmarczak]

Once again, it is a debatable edge case

This still doesn't change the fact that it's a bug in Node.js. I see no reason why it's debatable.

[szmarczak]

Actually someone may think that the current state is a desired one, therefore I'm closing. I decided to internally check if there is a mismatch between the origin and the actual remote target.