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

Validate proxy requests in absolute-form #169

Merged
merged 1 commit into from
Apr 21, 2017

Conversation

clue
Copy link
Member

@clue clue commented Apr 18, 2017

The absolute-form request-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.

@clue clue added this to the v0.7.0 milestone Apr 18, 2017
@clue clue force-pushed the uris branch 2 times, most recently from 4808542 to 708b143 Compare April 19, 2017 20:30
@clue clue changed the title Make getUri() behave consistent across different forms of request-target Validate proxy requests in absolute-form Apr 19, 2017
@clue
Copy link
Member Author

clue commented Apr 19, 2017

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

@WyriHaximus WyriHaximus requested review from jsor and WyriHaximus April 20, 2017 12:05
Copy link
Member

@WyriHaximus WyriHaximus left a comment

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

$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
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

$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
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
Copy link
Member Author

clue commented Apr 20, 2017

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:master Apr 21, 2017
@WyriHaximus
Copy link
Member

👍

Sign up for free to 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