http2: add support for sensitive headers

[addaleax]

Add support for “sensitive”/“never-indexed” HTTP2 headers.

Fixes: #34091

@jasnell @clshortfuse

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

[addaleax]

Fwiw, if we do decide that we need more granularity for multi-value headers like set-cookie, I would suggest allowing the property to be a function ({ key, value }) => boolean instead of an Array, to give the user full control. But as mentioned in the issue, I suspect that’s rare enough that we might not need to implement that?

[nodejs-github-bot]

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

[clshortfuse]

Nice work!

Just in case you find some issue testing, it appears as though all 'Authorization' and cookie headers under 20-length appear to be forced as not-indexed via nghttp2

https://github.com/nghttp2/nghttp2/blob/5ae9bb8925c1cb1e945b41d140367f27b3dd5c98/lib/nghttp2_hd.c#L1358-L1367

So, if you see cookie always being not-index despite never setting it on NodeJS, that might why.

[addaleax]

@clshortfuse Thanks, that’s very good to know – because the test just tested this for cookie. I’ve updated this a bit, please take a look. :)

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[clshortfuse]

Confirmed via Wireshark that it works! Nice job @addaleax

[ruyadorno]

This does not land cleanly on v14.x since it touches files related to the QUIC implementation. Should this be backported?

[pimterry]

A note here: this is marked as a semver-minor change, but I think it's actually a breaking change (and because of that, I don't think it should be backported to v14!). Certainly it breaks my application, and may well break things for anybody else who iterates over request.headers. Specifically:

• Before this change, keys in incoming HTTP headers were always strings - now one is a symbol (I currently use headerName.startsWith(':') to look for psuedoheaders, and my code now crashes because you can't do that with symbols).
• Before this change, every key in the headers object represented a received header, and could be interpreted as such, whilst now some keys are instead in-band special values. That matters because it might not be correct to forward this value when proxying requests, it is definitely not correctly to forward this value when translating HTTP/2 requests to HTTP/1, and it probably shouldn't be treated as a header when validating incoming headers, in metrics & logging contexts, etc etc etc.

Ofc this is a manageable breaking change, and I can still see arguments for doing this in-band rather than as a separate option, but it is going to break things and it should be advertised as such imo.

[clshortfuse]

@pimterry How are you iterating through keys? I've just checked and Symbols don't appear in

• Object.keys(obj)
• for (var key in obj)
• Object.entries(obj)
• Object.getOwnPropertyNames(obj)

Edit: TIL Reflect.ownKeys(obj) will.

[pimterry]

Ah, interesting! In this specific instance I'm using lodash (latest release). Like so:

const _ = require('lodash');
const s = Symbol('s')
const headers = { [s]: 'value' };
_.omitBy(headers, (v, k) => k.startsWith(':')); // k.startsWith is not a function

I can't speak for the reasoning on their end, but it seems that they include enumerable symbols in all methods like this intentionally, and they have done for many years: https://github.com/lodash/lodash/blob/master/.internal/getAllKeysIn.js

This does work fine if sensitiveHeaders is set as a non-enumerable property, like so:

const _ = require('lodash');
const s = Symbol('s')
const headers = { };
Object.defineProperty(headers, s, {
  value: 'value',
  enumerable: false
});
_.omitBy(headers, (v, k) => k.startsWith(':')); // All good

I'm not sure whether lodash is correct here, but I think there's an argument that others will hit this, and since conceptually it shouldn't be considered as a header key, there's an argument that this shouldn't be an enumerable property.