IdentityPython / djangosaml2

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

Add additional RelayState URL validation #388

Closed tymees closed 8 months ago

tymees commented 9 months ago

A PR to fix #385;

I went for a different approach than I suggested in my issue. I noticed there was a validate_referral_url function, which seemed like a more logical place to check for URL validity.

In addition, when looking at that function I noticed that there was an additional (hypothetical) issue, in which a RelayState for logout would redirect to the fallback login url if the RelayState didn't pass the allowed-host check.

Thus, I decided to refactor that function to only do validation checks, and moved the logic for deciding what to do when it fails to the functions that use it.

Validating if URLs are valid is always tricky, and my choice to use Django's resolve_url is thus not perfect as well. It will fail on values that could be a valid redirect target (I used a plain 'home' as an example). I cannot figure out a solution that will handle that gracefully but also fix the issue I described in #385.

One could say those URLs are poorly-formatted, especially as Django by default enforces URLs have trailing slashes (which will be seen as valid). However, I don't want this stricter validation to enforce some idealistic idea of URL formatting. Thus, I decided to add a settings opt-out to my new check, as described in the additional docs I provided.

I also added some extra debug logging, these are similar to the ones I added when debugging where my error was coming from. Hopefully they help other devs figuring out why their app does not do what they think it should ;)

Please feel free to completely disagree with this approach, I'd be happy to make a different PR if requested :) Any other comments are also welcome of course.

--

As an aside, the docs say I should target the dev branch for PR's. However, that branch seems outdated, and misses the very commit that caused the issue. So I hope me targeting the master branch instead is fine?

peppelinux commented 9 months ago

I'd ask in this PR, if you agree, to bring the version up to 1.8.0 https://github.com/IdentityPython/djangosaml2/blob/master/setup.py#L30

tymees commented 9 months ago

Of course! I think I've addressed your comments and I have updated the version number.