Compare Engine reporting's privateHeaders case-insensitively, as documented.
#2276
Conversation
…cumented. The documentation for `privateHeaders`[[0]] suggests that it is case-insensitive. While that statement is true, and the incoming header is lower-cased before checking it against the `privateHeaders` configuration, it assumed that the headers in the `privateHeaders` object were specified in lower-case. This changes the comparison to lower-case both sides prior to determining equality. [0]: https://github.com/apollographql/apollo-server/blob/abb8dc58/packages/apollo-engine-reporting/src/agent.ts#L67-L70 Fixes: #2273
abernix
force-pushed
the
abernix/allow-any-case-for-privateHeaders
branch
from
927fe47
to
5ee9845
Compare
| this.options.privateHeaders.includes(key.toLowerCase()) | ||
| this.options.privateHeaders.some(privateHeader => { | ||
| // Headers are case-insensitive, and should be compared as such. | ||
| return privateHeader.toLowerCase() === key.toLowerCase(); |
There was a problem hiding this comment.
This makes sense to me! We can do things to improve performance of course, like not calling key.toLowerCase() for every privateHeader, making sure to store private headers in lower case, or even using something like a Set for easy lookup (which I think is what that comment also alludes to), but none of that is likely to matter in practice, and this makes the intention immediately clear.
There was a problem hiding this comment.
Agree. I tried to avoid doing any micro-optimization right now since we'll (probably soon-ish?) want to introduce the ability to define this as a function and its less clear how beneficial it would be to build that out given that direction (and such an implementation could benefit from its own optimizations). Unless there are a large number of privateHeaders defined here, I don't think this relatively simple string manipulation would benefit, noticeably, right now.
The documentation for
privateHeaders[0] suggests that it is case-insensitive. While that statement is true, and the incoming header is lower-cased before checking it against theprivateHeadersconfiguration, it assumed that the headers in theprivateHeadersobject were specified in lower-case.This changes the comparison to lower-case both sides prior to determining equality.
Fixes: #2273