Handle multiply defined headers #1
Conversation
This is to associate specific headers with values factored into the signature, in case any headers are defined more than once in the request.
mbland
force-pushed
the
handle-multiply-defined-headers
branch
from
3f4af68
to
4f0aeba
Compare
|
OK, 18F/hmacauth#1 is in. I've kept this PR in tight sync with that one. Either of y'all mind taking a peek sometime? |
|
cc: @ccostino |
| return headers.map(function(header) { return req.get(header) || ''; }); | ||
| return headers.map(function(header) { | ||
| var value = req.headers[header]; | ||
| if (typeof value === Array) { value = value.join(','); } |
There was a problem hiding this comment.
If value is an array, typeof value would return 'object' (a string). How about testing for what you intend to use, in this case:
if (typeof value !== 'string' && typeof value.join === 'function') {
value = value.join(',');
}There was a problem hiding this comment.
Actually, that would miss undefined values, and since this is specifically for Node and we don't need to worry about older browsers, how about using if (Array.isArray(value)) { ... }?
There was a problem hiding this comment.
Hmm, it passed my multiply-defined header test, but I agree Array.isArray() is the way to roll. Done.
There was a problem hiding this comment.
I think the test passed because typeof value === Array fails, so the
join() call is skipped, but later the value array is being coerced to a
string and ['foo', 'bar'].toString() => "foo,bar" — the same output as
the join method.
|
Looks great! I'd say 🚢 away |
|
Thanks, @dhcole! |
Maintains parity with 18F/hmacauth#1. We should wait for that PR to go in before reviewing/submitting this one (unless you want to check it out for style ahead of time).
cc: @dhcole @jeremiak