Overzealous link header validation in writeEarlyHints

[khalsah]

Version

v19.5.0

Platform

Darwin Kernel Version 22.2.0: Fri Nov 11 02:03:51 PST 2022; root:xnu-8792.61.2~4/RELEASE_ARM64_T6000 arm64

Subsystem

http

What steps will reproduce the bug?

import { createServer } from "node:http";

const server = createServer((req, res) => {
  res.writeEarlyHints({
    link: "<https://fonts.gstatic.com/>; rel=preconnect; crossorigin",
  });
  res.end();
});

server.listen(3000);

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

No response

What is the expected behavior?

No response

What do you see instead?

node:internal/validators:473
    throw new ERR_INVALID_ARG_VALUE(
    ^

TypeError [ERR_INVALID_ARG_VALUE]: The argument 'hints' must be an array or string of format "</styles.css>; rel=preload; as=style". Received '<https://fonts.gstatic.com/>; rel=preconnect; crossorigin'
    at new NodeError (node:internal/errors:399:5)
    at validateLinkHeaderFormat (node:internal/validators:473:11)
    at validateLinkHeaderValue (node:internal/validators:493:5)
    at ServerResponse.writeEarlyHints (node:_http_server:310:16)
    at Server.<anonymous> (file:///Users/hargo/Code/wyyerd/i7n/test.mjs:4:7)
    at Server.emit (node:events:512:28)
    at parserOnIncoming (node:_http_server:1067:12)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17) {
  code: 'ERR_INVALID_ARG_VALUE'
}

Node.js v19.5.0

Additional information

This appears to be an overzelous validation that requires all link parameters to be followed by an =.

Per the ABNF in RFC8288 https://www.rfc-editor.org/rfc/rfc8288.html#section-3 the = is optional along with the parameter value.

Additionally it seems that the validation restricts parameters to a preset list of those headers currently defined by the HTML spec. This seems in conflict with RFC8288 which doesn't appear to place any restrictions on parameters, which seems more relevant that the parameters that happen to be specified in the current HTML specification.

[bnoordhuis]

PR welcome, although the regex used to validate the header is... wrgh.

writeEarlyHints() was added in #44180 and then its validation was updated #44874. Personally I don't see the value in being so rigid. @nodejs/http what was the thinking there?

[mcollina]

https://github.com/orgs/nodejs/teams/http what was the thinking there?

This is just a bug and no one caught it.
As for the script validation, the contributor proposed it and it seemed ok to me.

Can you take a look @anonrig @Uzlopak? You both discussed this problem in: https://github.com/nodejs/node/pull/44820/files#r983969846.

[climba03003]

I really doesn't know why Link header is the only exception to own a validation.
Maybe it provide a better DX, but in-consistence on behavior really cause a lot of confusion to the user.

[nithin-murali-arch]

Can this also be fixed on Node 18?

[ShogunPanda]

@nithin-murali-arch Isn't this fixed in 18.16.0? #46466 has been backported there.