-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Allow X-Forwarded-For to have multiple values #8409
Conversation
Would be great to have some unit test for this case |
@kassner Hi! Unfortunately, delivery of PR to the |
@kassner I am closing this PR for now due to lack of activity. |
@ishakhsuvarov @vrann can you reopen? GitHub allows to rebase the PR from the interface, so I changed the target branch here. |
@vrann Done. |
Hi @kassner |
@@ -64,6 +64,11 @@ public function getRemoteAddress($ipToLong = false) | |||
return false; | |||
} | |||
|
|||
if (strpos($this->remoteAddress, ',') !== false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this check. explode(',', 'abc')
will give you an array nonetheless.
@ishakhsuvarov - XFF can have multiple IPs and I don't think M2 can handle that situation. Same problem here. |
@ishakhsuvarov we can see that happening when we have HAProxy -> Varnish -> Apache. It is appended by Varnish (rightfully) before reaching Apache:
I guess you can simulate it by forcing the header |
@kassner thank you for your contribution to Magento 2 project |
@magento-team This is merged but doesn't appear in 2.2.7. Why? |
Hi @Ctucker9233, |
@ihor-sviziev Perfect! Thank you. |
PR #1546 was closed because CLA issues, thus I'm resubmitting the changes.
Preconditions
Steps to reproduce
Expected result
Actual result