Compatibility with Symfony 3.3+, with backwards compatibility #70
Conversation
…rding chain of IP addresses
… on the config, or the default if the config is not set
with backwards compatibility for Symfony < 3.3
src/TrustProxies.php
Outdated
|
|
||
| /** | ||
| * Retrieve trusted header names | ||
| * @return mixed |
There was a problem hiding this comment.
we can probably be more specific here
There was a problem hiding this comment.
Thinking something like "get trusted header names, falling back to defaults if not found in config" ?
There was a problem hiding this comment.
I was meaning with the return type. :)
There was a problem hiding this comment.
While it could technically return anything, we should document it to return what we actually expect it to return.
| @@ -83,36 +86,59 @@ protected function setTrustedProxyIpAddresses($request) | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * We specify the IP addresses to trust explicitly | |||
There was a problem hiding this comment.
Missing newline and full stop in each case.
|
Other than cs, this looks good. Symfony 3.3 is out now, so people's apps will break when they upgrade. Probably would be good to merge and tag soon. :P |
|
I need this, my app is down. |
|
Yeahr, really soon! My App is down too! |
src/TrustProxies.php
Outdated
| * Set the trusted header names based on teh content of trustedproxy.headers | ||
| * | ||
| * Set the trusted header names based on the content of trustedproxy.headers | ||
| * Note: Depreciated in Symfony 3.3+, but available for backwards compatibility |
There was a problem hiding this comment.
Typo (Depreciated) 😄
There was a problem hiding this comment.
You can use the deprecated tag for this kind of info
https://phpdoc.org/docs/latest/references/phpdoc/tags/deprecated.html
|
well done guys! Let's see when this PR is merged :) |
|
Merging today I think! Sidenote: your apps are down? Are y'all using Laravel 5.5 before it's released? (latest stable Laravel seems to use Symfony 3.2 specifically, based on the composer.json file). Just wondering if there's another way people end up with Symfony 3.3. |
|
|
Request expects a bitfield of headers rather than array.
|
I'm pulling in #73 which builds on top of this, fixes tests, and passes the correct argument into |
Add getTrustedHeaderSet(), fix tests.
|
Thanks @fideloper |
|
Before Symfony 3.3 we could distrust headers by setting them to Instead of nulling, I removed Not sure if this is the right "fix" or this needs to be fixed in |
|
Definitely an interesting issue, thanks for letting us know!
…On Tue, Jun 6, 2017 at 4:38 PM, Jarno van Leeuwen ***@***.***> wrote:
Before Symfony 3.3 we could distrust headers by setting them to null in
the config. However, with this PR all header keys are processed by the
getTrustedHeaderSet method. I started retrieving the following exception
on some requests:
Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException: The request has both a trusted "FORWARDED" header and a trusted "X_FORWARDED_FOR" header, conflicting with each other. You should either configure your proxy to remove one of them, or configure your project to distrust the offending one.
Instead of nulling, I removed \Illuminate\Http\Request::HEADER_FORWARDED
from the headers array in the config, which resolved the problem.
Not sure if this is the right "fix" or this needs to be fixed in
getTrustedHeaderSet. If this is the right way, the docs might need an
update on distrusting header names.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#70 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAch0yHphei91VogGu0NvpvaoLnspLIdks5sBcbIgaJpZM4NlB6T>
.
|
|
@jarnovanleeuwen See PR #77 which I've pulled in, it should address that issue. |
|
Looks great! Thank you for the quick response. |
See #68