IdentityPython / djangosaml2

Django SAML2 Service Provider based on pySAML2
Apache License 2.0
254 stars 143 forks source link

next_path is URL encoded #360

Closed Gomes117 closed 1 year ago

Gomes117 commented 1 year ago

This commit introduced a breaking change where the next_path is now being encoded. That's an issue if your next_path is a full URL since the schema will be encoded (https:// -> https%3A//) resulting in an error on redirect.

Can that change be reverted?

daggaz commented 1 year ago

I have this issue too.

LoginView.get() calls self.get_next_path() (here), which calls validate_referral_url() (here), which has been changed to encode the URL. The resulting value eventually becomes the RelayState sent in the SAML Login Request to the IDP.

On the way back, the ACS (here) gets the RelayState from the SAML Response and again calls validate_referral_url(), which actually double encodes the final URL that is used in the returned HttpResponseRedirect().

peppelinux commented 1 year ago

Yes, my bad!

Instead of reverting that commit, what do you think It would a good workarounds to keep the security of the solution and avoid this breaking change?

If you have one consider me available in doing a revisione on your PR and do another release After this

prauscher commented 1 year ago

The same issue applies with Query-strings in next_url, e.g. /fetch?source=123. Is there a reason to escape the data here, e.g. what are we protecting from? If it is manipulation in the Identity Provider or in transfer a better solution would be to use https://docs.djangoproject.com/en/4.1/topics/signing/

peppelinux commented 1 year ago

I can revert that commit in the next hours and issue a new release

Is there some security or implementativo considerations that you would like to express for this issue?

prauscher commented 1 year ago

Thanks for your fast response! Actually I'm quite new to SAML2, but I think the most important security features are host-checking which is already implemented and that next_url will only use GET-Requests. If you can think of any security considerations let's tackle them here :)

peppelinux commented 1 year ago

Ok, I revert the commit to get a new release without this bug and I ask to @Gee19 ti join in this discussion as author of the commit and the considerations behind that

peppelinux commented 1 year ago

Reverted here https://github.com/IdentityPython/djangosaml2/releases/tag/v1.5.5