Django strategy should respect X-Forwarded-Port #841
Conversation
This change modifies the django strategy's request_port method to respect the USE_X_FORWARDED_HOST setting, and the USE_X_FORWARDED_PORT setting in django >= 1.9.
| try: | ||
| return host_parts[1] | ||
| except IndexError: | ||
| return None |
There was a problem hiding this comment.
Wouldn't 80 be the default? Or self.request.META['SERVER_PORT']?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
It would solve a problem of mine if this were merged. |
omab
force-pushed
the
master
branch
2 times, most recently
from
1078c07 to
de9f179
Compare
|
I've ported this PR into social-app-django. Thanks! |
This change modifies the django strategy's
request_portmethod to respect theUSE_X_FORWARDED_HOSTsetting, and theUSE_X_FORWARDED_PORTsetting in django >= 1.9.It should achieve the same thing as #741 but without introducing new settings.