Skip to content

Ignore excessive whitespace in chunk header #116

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.

Sign up for GitHub

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

Jump to bottom
Closed
DangerLifter wants to merge 2 commits into from
Closed

Ignore excessive whitespace in chunk header #116

DangerLifter wants to merge 2 commits into from

Conversation

@DangerLifter
Copy link

@DangerLifter DangerLifter commented Dec 10, 2017

@legionth
Copy link

legionth commented Dec 11, 2017

Hey thank you for your PR 👍

While I see your problem, I am pretty sure that a 2a \r\n... or similar is not valid HTTP, according to the HTTP RFC.

Any idea how other clients handle this behavior?
Could you also provide some unit tests?

@DangerLifter
Copy link
Author

DangerLifter commented Dec 12, 2017 •
edited
Loading

Any idea how other clients handle this behavior?

Sorry, know nothing here.

Could you also provide some unit tests?

Done.

@clue clue added the bug label Jan 16, 2018
@clue clue added this to the v0.5.7 milestone Jan 16, 2018
@clue clue changed the title * try to fix issue with chunk length header error Ignore excessive whitespace in chunk header Jan 16, 2018
clue
clue requested changes Jan 16, 2018
View reviewed changes
Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

@DangerLifter Thank you for looking into this and filing this PR! 👍

I've found some minor issues below, I'm happy to get this in once this PR is updated :shipit:

tests/DecodeChunkedStreamTest.php
],
'extra-space-in-length-chunk' => [
["04 \r\nWiki\r\n5\r\npedia\r\nE\r\n in\r\n\r\nchunks.\r\n0\r\n\r\n"],
]
Copy link
Member

@clue clue Jan 16, 2018

Choose a reason for hiding this comment

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

Minor CS issue.

Also, can you add a test a chunk header that consists of a single space " \r\n" will be rejected?

@clue clue mentioned this pull request Feb 7, 2018
Merged
@clue clue removed this from the v0.5.7 milestone Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clue clue clue requested changes

Assignees

No one assigned

Labels

bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@DangerLifter @legionth @clue