Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Django strategy should respect X-Forwarded-Port #841

Closed
wants to merge 1 commit into from
Closed

Django strategy should respect X-Forwarded-Port #841

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions social/strategies/django_strategy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ def request_data(self, merge=True):
return data

def request_host(self):
if self.request:
return self.request.get_host()
return self.request.get_host()

def request_is_secure(self):
"""Is the request using HTTPS?"""
Expand All @@ -63,7 +62,14 @@ def request_path(self):

def request_port(self):
"""Port in use for this request"""
return self.request.META['SERVER_PORT']
try: # django >= 1.9
return self.request.get_port()
except AttributeError: # django < 1.9
host_parts = self.request.get_host().split(':')
try:
return host_parts[1]
except IndexError:
return None
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't 80 be the default? Or self.request.META['SERVER_PORT']?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should only hit this code path if the port is 80 or 443, in which case it will be stripped anyway: https://github.com/onelogin/python-saml/blob/efb2515e0a12fb43271d5d6b8bddcbaa4a61f451/src/onelogin/saml2/utils.py#L268-L271

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So shouldn't the except AttributeError: path just return self.request.META['SERVER_PORT'] then instead of splitting the host?

Or does looking at the port from get_host() emulate using USE_X_FORWARDED_PORT? In that case, it seems like the except IndexError: should still fall back to self.request.META['SERVER_PORT'] so we get at least 80 or 443 rather than None?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

USE_X_FORWARDED_PORT was introduced in django 1.9, and if we're on the except AttributeError: path it means we're using an older version so we can't use that setting. In this case, get_host() will respect the USE_X_FORWARDED_HOST setting, and the reverse proxy can be configured to pass the port along with the X-Forwarded-Host header.

If you prefer we can fallback to using self.request.META['SERVER_PORT'] from except IndexError:. The behaviour would be the same, as the port would just be stripped in the python-saml code later on.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should note - I have not authority over this repo whatsoever, I'm just a fellow user of PSA, trying to help out if I can :)

I'm worried about the behavior staying the same because things other than python-saml might be relying on get_port(). Seems like the behavior in PSA shouldn't change dramatically (possibly returning None now) because of starting to use USE_X_FORWARDED_HOST. So yeah it seems like falling back to old behavior when nothing else works might be more likely to result in the PR getting merged, but that's just my guess.


def request_get(self):
"""Request GET data"""
Expand Down