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

Implemented RFC 7239 - "Forwarded HTTP Extension" #94

Merged
merged 8 commits into from Jun 17, 2016
Merged

Conversation

@patrickkusebauch
Copy link
Contributor

patrickkusebauch commented May 21, 2016

  • feature
  • issues - #20
  • documentation - not needed - just see RFC 7239
  • BC break - no

RequestFactory can now take "RemoteAddress", "RemoteHost", "port" and "scheme" from the proxy "FORWARDED" header.

$url->setPort((int) $_SERVER['HTTP_X_FORWARDED_PORT']);
}
if (isset($proxyParams['for'])) {
$remoteAddr = explode(':', $proxyParams['for'][0])[0];

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 22, 2016

Contributor

This will not work with IPv6.

This comment has been minimized.

Copy link
@patrickkusebauch

patrickkusebauch May 22, 2016

Author Contributor

IPv6 support in commit 11461e6

$forwardParams = preg_split('/[,]|[;]/', $_SERVER['HTTP_FORWARDED']);
foreach ($forwardParams as $forwardParam) {
$param = explode("=", $forwardParam);
$proxyParams[trim($param[0])][] = trim($param[1]); //e.g. array['for'][0] = 192.168.0.1

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 22, 2016

Contributor

Parameter names must be treated in case-insensitive manner.

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 22, 2016

Contributor

Also this doesn't properly handle quoted values.

This comment has been minimized.

Copy link
@patrickkusebauch

patrickkusebauch May 22, 2016

Author Contributor

Parameter names must be treated in case-insensitive manner.
solved by commit 44660bd

This comment has been minimized.

Copy link
@patrickkusebauch

patrickkusebauch May 22, 2016

Author Contributor

Quoted values in commit 11461e6

$url->setScheme(strcasecmp($_SERVER['HTTP_X_FORWARDED_PROTO'], 'https') === 0 ? 'https' : 'http');
}
if(!empty($_SERVER['HTTP_FORWARDED'])) {
$forwardParams = preg_split('/[,]|[;]/', $_SERVER['HTTP_FORWARDED']);

This comment has been minimized.

Copy link
@Majkl578

Majkl578 May 22, 2016

Contributor

Semicolin is used as a forward-pair delimiter whereas comma is used as a field separator. I'm afraid current implementation won't be able to distinguish it...

This comment has been minimized.

Copy link
@patrickkusebauch

patrickkusebauch May 22, 2016

Author Contributor

It is not. However it should not be a problem, as I believe there is no need for the distinction.
These two headers would produce the same result array:
for=192.0.2.43, for=198.51.100.17
for=192.0.2.43; for=198.51.100.17

I just need to split the "key-value" pairs and put them in the proper sub-array.

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 22, 2016

Contributor

This can be simplified to preg_split('#[,;]#')

This comment has been minimized.

Copy link
@patrickkusebauch

patrickkusebauch May 22, 2016

Author Contributor

Integrated in commit a2c7a79

Will now work with IPv6.

Added tests for port and scheme of the URL
 - Split test into "x-forwarded" and "forwarded" files for proxy
 - Added tests for default scheme.
 - Added tests for every combination of IPv4/IPv6 with and without port for both "host" and "for" headers
@JanTvrdik

This comment has been minimized.

Copy link
Contributor

JanTvrdik commented May 22, 2016

possible related #92

foreach ($forwardParams as $forwardParam) {
$param = explode("=", $forwardParam);
$proxyParams[strtolower(trim($param[0]))][] = trim($param[1], "\"\t\n\r\0\x0B"); //e.g. array['for'][0] = 192.168.0.1
}

This comment has been minimized.

Copy link
@JanTvrdik

JanTvrdik May 22, 2016

Contributor

This can be simplified to

foreach ($forwardParams as $forwardParam) {
    list($key, $value) = explode('=', $forwardParam, 2) + [1 => NULL];
    $proxyParams[strtolower(trim($key))] = trim($value, " \t\"");
}

This comment has been minimized.

Copy link
@patrickkusebauch

patrickkusebauch May 22, 2016

Author Contributor

Integrated in commit a2c7a79

@patrickkusebauch

This comment has been minimized.

Copy link
Contributor Author

patrickkusebauch commented May 22, 2016

I know that it keeps failing on php5.6 with composer update --no-interaction --prefer-dist --prefer-lowest --prefer-stable. However this cannot be redeemed as the tests have to change to keep up with the versions. Furthermore they are test unrelated to this pull request.

@patrickkusebauch

This comment has been minimized.

Copy link
Contributor Author

patrickkusebauch commented May 23, 2016

Any other problems to be fixed with this PR?
Missing tests?
Or can I safely squash it to be merged?

@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 17, 2016

Is it ready for merge?

@patrickkusebauch

This comment has been minimized.

Copy link
Contributor Author

patrickkusebauch commented Jun 17, 2016

Since there were no comments to the PR for more than 3 weeks, I would assume nobody has any more problems with it.
So yes, it is ready for merge.

@dg dg merged commit e819eff into nette:master Jun 17, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage increased (+0.6%) to 79.293%
Details
@dg

This comment has been minimized.

Copy link
Member

dg commented Jun 17, 2016

So thanks!

@patrickkusebauch

This comment has been minimized.

Copy link
Contributor Author

patrickkusebauch commented Jun 17, 2016

I thank you for letting me be a part of the development of Nette. :)

dg added a commit that referenced this pull request Jun 17, 2016
* Implemented RFC 7239 - " Forwarded HTTP Extension" handling in RequestFactory

* Implemented RFC 7239 - " Forwarded HTTP Extension" handling in RequestFactory

* Deleted echo statement

* case- insensitive handling of  tokens

* Proper handle of quoted strings.
Will now work with IPv6.

Added tests for port and scheme of the URL [Closes #94]

* Tests refactoring:

 - Split test into "x-forwarded" and "forwarded" files for proxy
 - Added tests for default scheme.
 - Added tests for every combination of IPv4/IPv6 with and without port for both "host" and "for" headers

* Code simplifications

* Fixed coding standards for tests
dg added a commit that referenced this pull request Jun 17, 2016
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.