Skip to content

Validate proxy requests in absolute-form - #169

Merged
WyriHaximus merged 1 commit into
reactphp:masterfrom
clue-labs:uris
Apr 21, 2017
Merged

Validate proxy requests in absolute-form#169
WyriHaximus merged 1 commit into
reactphp:masterfrom
clue-labs:uris

Conversation

@clue

@clueclue commented Apr 18, 2017

Copy link
Copy Markdown
Member

The absolute-formrequest-target was already supported in earlier version, but was neither tested nor documented. This PR makes sure that getUri() behaves consistent across different forms of request-target and adds matching documentation and an example for a plain HTTP proxy that relies on requests in absolute-form.

Example request as sent to a plain HTTP proxy:

GET http://www.google.com/ HTTP/1.1
Host: www.google.com

Builds on top of #167, #158 and #157.

@clueclue added this to the v0.7.0 milestone Apr 18, 2017
@clue
clueforce-pushed the uris branch 2 times, most recently from 4808542 to 708b143CompareApril 19, 2017 20:30
@clueclue changed the title Make getUri() behave consistent across different forms of request-targetValidate proxy requests in absolute-formApr 19, 2017
@clue

clue commented Apr 19, 2017

Copy link
Copy Markdown
MemberAuthor

Updated PR to remove unrelated changes so this now has a clear focus on requests in absolute-form:shipit:

@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, just a minor nitpick

Comment threadtests/ServerTest.php Outdated
$this->assertSame('http://example.com/test', $requestAssertion->getRequestTarget());
$this->assertEquals('http://example.com/test', $requestAssertion->getUri());
$this->assertSame('/test', $requestAssertion->getUri()->getPath());
//$this->assertSame(array(), $requestAssertion->getQueryParams());

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.

Any reason this is commented out? To me commented out code shouldn't be committed tbh

Comment threadtests/ServerTest.php Outdated
$this->assertSame('http://example.com:8080/test', $requestAssertion->getRequestTarget());
$this->assertEquals('http://example.com:8080/test', $requestAssertion->getUri());
$this->assertSame('/test', $requestAssertion->getUri()->getPath());
//$this->assertSame(array(), $requestAssertion->getQueryParams());

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.

Same as line 289

@clue

clue commented Apr 20, 2017

Copy link
Copy Markdown
MemberAuthor

Thanks for spotting! Updated and also rebased now that #170 is in, which did not preserve original request-target :shipit:

@WyriHaximus
WyriHaximus merged commit 194658a into reactphp:masterApr 21, 2017
@WyriHaximus

Copy link
Copy Markdown
Member

👍

@clue
clue deleted the uris branch April 21, 2017 07:06
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@clue@WyriHaximus@jsor