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

Fixed #018555: Debug by ip not usable if being a proxy #104

Merged
merged 3 commits into from Sep 3, 2011

Conversation

alafon
Copy link
Contributor

@alafon alafon commented Aug 18, 2011

  • I first implemented the logic in the eZDebug::isAllowedByCurrentIP() method itself, but after reading a comment in a related issue of 018555, I decided to add a clientIP() method to eZSys in order to make the job.
  • I chose not to hard code the header name in the method because, even if commonly used by Squid and other proxy, X-Forwarded-For is not mentioned in any RFC (yet?).

@dpobel
Copy link
Contributor

dpobel commented Aug 18, 2011

Nice pull request Arnaud :-)
+1 for me. (nitpick: I would have done it with only one new INI settings that could contains the custom header to look for and if it's empty, use REMOTE_ADDR)

@alafon
Copy link
Contributor Author

alafon commented Aug 18, 2011

Thanks Damien.

nitpick: I would have done it with only one new INI settings that could contains the custom header to look for and if it's empty, use REMOTE_ADDR

It is done this way :)

@dpobel
Copy link
Contributor

dpobel commented Aug 18, 2011

doh! sorry the example mislead me :-/

@alafon
Copy link
Contributor Author

alafon commented Aug 22, 2011

Done :

  • method eZSys:clientIP() re-implemented as suggested by André
  • site.ini updated
  • doc updated

andrerom added a commit that referenced this pull request Sep 3, 2011
Fixed #018555: Debug by ip not usable if being a proxy
@andrerom andrerom merged commit 6dbb3f2 into ezsystems:master Sep 3, 2011
andrerom added a commit that referenced this pull request Sep 8, 2011
@andrerom
Copy link
Contributor

andrerom commented Sep 8, 2011

Sorry for asking stuff on a closed pull request, but #112 made me think about the setting introduced here.
Is it really needed to be able to specific the header name?
Isn't it always X-Forward-For?

What I'm thinking about is to instead have a enable/disable setting(aka only enable if you trust the source setting these headers), and potentially also reuse it for things like X-Forward-Host used in hostname().

@alafon
Copy link
Contributor Author

alafon commented Sep 8, 2011

The problem is that during my investigation about X-Forward-For, I've discovered that it's not in any official RFC. You're right, it (the header name) is commonly used by technologies such as Varnish, so maybe we could hard code the header name... My though are that having X-Forward-For as a default parameter is ok, and being able to change it on demand is also a good feature since sometime eZ publish is behind homemade proxys (such as the big customer for whom I've implemented this).

@andrerom
Copy link
Contributor

andrerom commented Sep 8, 2011

Yes, I see your point.
I'am just trying to keep number of settings down, and since these have always been hardcoded in the past (eg X-Forward-Host further up) and no one has complained about it, I thought we could simplify a bit before it ends up in a release :)

@alafon
Copy link
Contributor Author

alafon commented Sep 9, 2011

Alright.
Instead of using a setting, what about providing a way where anyone would be able to override the clientIP() function ?

peterkeung pushed a commit to peterkeung/ezpublish that referenced this pull request Oct 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants