Skip to content

Fix checking maximum header size, do not take start of body into account - #88

Merged
clue merged 1 commit into
reactphp:masterfrom
nopolabs:fix/header-size-in-RequestHeaderParser
Feb 6, 2017
Merged

Fix checking maximum header size, do not take start of body into account#88
clue merged 1 commit into
reactphp:masterfrom
nopolabs:fix/header-size-in-RequestHeaderParser

Conversation

@nopolabs

Copy link
Copy Markdown
Contributor

Fixed check for exceeding maximum header size in RequestHeaderParser::feed()

@clue

clue commented Feb 5, 2017

Copy link
Copy Markdown
Member

Thanks for filing this PR 👍

Unfortunately it has no tests, see also #82.

@WyriHaximusWyriHaximus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As @clue mentions could you add tests?

Comment threadsrc/Server.php Outdated
});
$parser->on('error', function($exception) {
$this->emit('error', [$exception]);
});

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure why this is in this PR? (It does make sense to do this non the less.)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FYI this has been addressed in #83

@WyriHaximusWyriHaximus added this to the v0.4.3 milestone Feb 5, 2017
@nopolabs
nopolabsforce-pushed the fix/header-size-in-RequestHeaderParser branch from 5fb8f9e to c4c1aecCompareFebruary 5, 2017 19:10
@nopolabs

nopolabs commented Feb 5, 2017

Copy link
Copy Markdown
ContributorAuthor

added test

removed bubble up of error from Server.php, not related to this issue

@WyriHaximus

Copy link
Copy Markdown
Member

added test

Test look good, but now this test is failing

removed bubble up of error from Server.php, not related to this issue

Great, thanks 👍

@nopolabs
nopolabsforce-pushed the fix/header-size-in-RequestHeaderParser branch 2 times, most recently from 6e3c57d to 97ac89cCompareFebruary 5, 2017 19:50
@WyriHaximus
WyriHaximus requested review from clue and jsorFebruary 5, 2017 19:53

@WyriHaximusWyriHaximus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 👍

Comment threadtests/ServerTest.php Outdated

$data = $this->createGetRequest();
$data = str_pad($data, 4096 * 4);
$data = chop($this->createGetRequest());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks okay, but could probably be a bit clearer. What do you think, should this be updated to use a literal request string here instead?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I agree, thanks for the suggestion.

@nopolabs
nopolabsforce-pushed the fix/header-size-in-RequestHeaderParser branch from 97ac89c to 5dc78e7CompareFebruary 5, 2017 20:39
jsor
jsor approved these changes Feb 6, 2017

@jsorjsor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

clue
clue approved these changes Feb 6, 2017
@clueclue added the bug label Feb 6, 2017
@clueclue changed the title check max header sizeFix checking maximum header size, do not take start of body into accountFeb 6, 2017
@clue
clue merged commit 8d6272a into reactphp:masterFeb 6, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nopolabs@clue@WyriHaximus@jsor