-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: allow numeric header values #48
Conversation
a288ad3
to
1ca4ee2
Compare
@@ -127,7 +127,8 @@ function verifyState(xhr) { | |||
function normalizeHeaderValue(value) { | |||
// Ref: https://fetch.spec.whatwg.org/#http-whitespace-bytes | |||
/*eslint no-control-regex: "off"*/ | |||
return value.replace(/^[\x09\x0A\x0D\x20]+|[\x09\x0A\x0D\x20]+$/g, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All header values should be strings. I think you might as well just handle the case by wrapping the value
in String(value)
. When getting header values you should always get a string, anyway (so update the test as well). Any conversion to numbers in your code need to happen after header extraction. XHR can't know if the value is a number or string when sent on the wire, so this is client knowledge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ref #37 (comment)
I changed my mind. Wrapping this in a string doesn't make sense, as the format is not decided. For instance, wrapping
We should throw on non-string vals
ref https://en.wikipedia.org/wiki/List_of_HTTP_header_fields#General_format |
According to RFC7230, http fields have used to be text and new headers should continue to do so, restricting the values to consist of US-ASCII octets. This test is not so strict, but we avoid a whole range of errors by just checking if the value is a string. Ref sinonjs#51 and sinonjs#48
According to RFC7230, http fields have used to be text and new headers should continue to do so, restricting the values to consist of US-ASCII octets. This test is not so strict, but we avoid a whole range of errors by just checking if the value is a string. Ref sinonjs#51, sinonjs#48 and https://tools.ietf.org/html/rfc7230#section-3.2.4
According to RFC7230, http fields have used to be text and new headers should continue to do so, restricting the values to consist of US-ASCII octets. This test is not so strict, but we avoid a whole range of errors by just checking if the value is a string. Ref #51, #48 and https://tools.ietf.org/html/rfc7230#section-3.2.4
No description provided.