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

remove absolute URI from $_SERVER['REQUEST_URI'] (issue #91) #93

Merged
merged 3 commits into from Jun 17, 2016

Conversation

@ondrej-tuhacek
Copy link
Contributor

ondrej-tuhacek commented May 6, 2016

No description provided.

@@ -81,7 +81,7 @@ public function createHttpRequest()
}

// path & query
$requestUrl = isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : '/';
$requestUrl = isset($_SERVER['REQUEST_URI']) ? Strings::replace($_SERVER['REQUEST_URI'], '#^\w+://[^/]+#i') : '/';

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 6, 2016

Contributor

I think it's better to make it more readable by splitting it into lines.

$requestUrl = isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : '/';
$requestUrl = Strings::replace($requestUrl, '#^\w+://[^/]+#i');

The pattern could be optimized by removing pointless case-insensitive flag and avoiding any possible backtracking

$requestUrl = isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : '/';
$requestUrl = Strings::replace($requestUrl, '#^\w++://[^/]++#');

With removed backtracking, we may probably replace Strings::replace with faster preg_replace

$requestUrl = isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : '/';
$requestUrl = preg_replace('#^\w++://[^/]++#', '', $requestUrl);

This comment has been minimized.

Copy link
@ondrej-tuhacek

ondrej-tuhacek May 6, 2016

Author Contributor

OK, you're right. I have modified it. Thanks.

@@ -82,6 +82,7 @@ public function createHttpRequest()

// path & query
$requestUrl = isset($_SERVER['REQUEST_URI']) ? $_SERVER['REQUEST_URI'] : '/';
$requestUrl = preg_replace('#^\w++://[^/]++#', '', $requestUrl);

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 6, 2016

Contributor

Is this actually RFC compliant? What if I send mismatching Host header or use different protocol than used in here? Shouldn't we strip it only if it is a match and throw exception otherwise?

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 7, 2016

Contributor

See how Symfony handles this. They replace it only if it matches current schema and host. But I haven't seen any specification which would define how this should actually be handled.

@dg dg force-pushed the nette:master branch from 6286f91 to d523f35 May 9, 2016
@dg dg merged commit dff9775 into nette:master Jun 17, 2016
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 78.776%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.