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

Fix X-Forwarded-For not supported properly #17785

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

davidglezz
Copy link
Contributor

@davidglezz davidglezz commented Feb 22, 2020

Questions Answers
Branch? develop
Description? Range must be 172.16/12 instead of 172.16/16 (https://tools.ietf.org/html/rfc1918#section-3)
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #13074.
How to test? No need QA, it's related to server configuration.

Some tests

172.15.255.255 -> use REMOTE_ADDR
172.16.0.0    ->  use HTTP_X_FORWARDED_FOR
172.16.255.255 -> use HTTP_X_FORWARDED_FOR
172.22.255.0  ->  use HTTP_X_FORWARDED_FOR
172.31.0.0   ->   use HTTP_X_FORWARDED_FOR
172.31.255.255 -> use HTTP_X_FORWARDED_FOR
172.32.0.0   ->   use REMOTE_ADDR

This change is Reviewable

@davidglezz davidglezz requested a review from a team as a code owner February 22, 2020 16:12
@prestonBot prestonBot added develop Branch Bug Type: Bug labels Feb 22, 2020
@PierreRambaud
Copy link
Contributor

PierreRambaud commented Feb 24, 2020

Unfortunately there is no unit tests for this method, can you add them?

@davidglezz
Copy link
Contributor Author

No, sorry, 0 experience doing test. I know I should know.

@PierreRambaud
Copy link
Contributor

So, we need a better scenario to test 😅

@davidglezz
Copy link
Contributor Author

davidglezz commented Feb 24, 2020

I only modify the behavior for 172 range.

Before: preg_match('/^172\.16.*/i', ..)

  • 172.16.x.x → true → use HTTP_X_FORWARDED_FOR ✅
  • 172.16x.x.x → true → use HTTP_X_FORWARDED_FOR ❌
  • 172.17.x.x to 172.31.x.x → false → use REMOTE_ADDR ❌

After: preg_match('/^172\.(1[6-9]|2\d|30|31)\..*/i', ..)

  • 172.16.x.x to 172.31.x.x → true → use HTTP_X_FORWARDED_FOR ✅
  • 172.x.x.x but not in 172.16.x.x to 172.31.x.x → false → use REMOTE_ADDR ✅

@PierreRambaud
Copy link
Contributor

On more review and merge without QA.

@PierreRambaud PierreRambaud added this to the 1.7.8.0 milestone Feb 28, 2020
@PierreRambaud PierreRambaud merged commit 5397822 into PrestaShop:develop Feb 28, 2020
@PierreRambaud
Copy link
Contributor

Thanks @davidglezz

@Jogai
Copy link

Jogai commented Oct 9, 2020

Is there a way I can convince people to cherry pick this in an earlier release?

@PierreRambaud
Copy link
Contributor

Is there a way I can convince people to cherry pick this in an earlier release?

You should contact an agency or a freelance to do it for you 😉

@Jogai
Copy link

Jogai commented Oct 9, 2020

I'm a freelancer, and I'm comfortable with git, but I'm guessing its generally not accepted to make a pull request against a release branch.

@PierreRambaud
Copy link
Contributor

PierreRambaud commented Oct 9, 2020

No it will not be accepted ^^ We are only working on the develop branch. You will have to wait for the 1.7.8.0 if you need this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Type: Bug develop Branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

X-Forwarded-For not supported properly
5 participants