Skip to content
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

Support Tolerance Provision when parsing headers #449

Merged
merged 1 commit into from Dec 6, 2016

Conversation

kwgithubusername
Copy link
Contributor

@kwgithubusername kwgithubusername commented Dec 5, 2016

The change to fix this issue #422 has created another issue for our app on the iPhone 4s running iOS 8.2 and iOS 9.3. The impact prevents these users from using one of two methods to login into their accounts.

More specifically, the result of splitting by \r\n and not both \r\n and \n is a malformed response header with all the actual headers put into the 'pragma' key and a content type of text/plain;charset=UTF-8 instead of application/json.

Source:
http://stackoverflow.com/a/5757349
more specifically, https://www.w3.org/Protocols/rfc2616/rfc2616-sec19.html#sec19.3

@mislav
Copy link
Contributor

mislav commented Dec 6, 2016

Thanks for reporting. Why support the single \r, though, when the tolerance provision doesn't specify it?

@kwgithubusername
Copy link
Contributor Author

Changed to remove support for single \r.

@@ -355,7 +355,7 @@

function parseHeaders(rawHeaders) {
var headers = new Headers()
rawHeaders.split('\r\n').forEach(function(line) {
rawHeaders.replace('\r\n', '\n').split('\n').forEach(function(line) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work as simply split(/\r?\n/) to avoid the replace?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that works - thanks for your quick response!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushing a change now

@mislav mislav merged commit d7989c1 into JakeChampion:master Dec 6, 2016
@kwgithubusername
Copy link
Contributor Author

Thanks!

@kwgithubusername
Copy link
Contributor Author

Edited scope of impact in original post.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants