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

Remove ProxyMiddleware #5607

Merged
merged 5 commits into from May 9, 2019
Merged

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Apr 18, 2019

Closes #5605


return ip_address
return None
Copy link
Contributor

@davidfischer davidfischer Apr 18, 2019

There's two cases here and these changes neglect one of them:

  • If X-Forwarded-For is present, this function should return the first value from the comma separated list. Arguably, it could validate that it is a real IP address, but that isn't that important.
  • If X-Forwarded-For is not present, this function should return the value from REMOTE_ADDR. This implementation returns None.

This function is used in advertising code for geo-targeting as well as being used for server side analytics (currently used in advertising but in the future might replace Google Analytics JS) but the middleware can be safely removed.

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 19, 2019

@davidfischer
Thank you for the information.
I have updated the PR.

Will there be any case in which both of these headers are not found? Currently the implementation returns None in that case.

if x_forwarded_for:
ip_address = x_forwarded_for.rsplit(':')[0]
Copy link
Contributor

@davidfischer davidfischer Apr 19, 2019

This logic was removed and also shouldn't be. The X-Forwarded-For header is not exactly a standard but some implementations include a port number. This line strips that port number.

This function is correct and working as-is. Is there a reason to change it? I do appreciate comments which capture the reasoning though.

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 20, 2019

@davidfischer

The X-Forwarded-For header is not exactly a standard but some implementations include a port number.

Thank you for this information. I didn't found this while searching about the header and removed this line thinking that it can produce bugs.
I have updated the code.

Is there a reason to change it?

The only reason was the readability and to improve comments.

Copy link
Member

@ericholscher ericholscher Apr 22, 2019

We should add tests for this also.

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 23, 2019

@ericholscher
I have added the tests.

@humitos humitos merged commit 227da1f into readthedocs:master May 9, 2019
1 check passed
@dojutsu-user dojutsu-user deleted the remove-proxy-middleware branch May 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants