Skip to content

Add additional RelayState URL validation #388

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

Merged
merged 4 commits into from
Oct 25, 2023

Conversation

tymees
Copy link
Contributor

@tymees tymees commented Oct 2, 2023

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?

Also, some additional debug logging. Also fixed an issue which would
cause logout requests to redirect to the fallback login redirect url
@peppelinux
Copy link
Member

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 and others added 3 commits October 2, 2023 15:26
This will prevent redirect loops with the strict validation disabled

Co-authored-by: Giuseppe De Marco <[email protected]>
@tymees
Copy link
Contributor Author

tymees commented Oct 2, 2023

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

@peppelinux peppelinux merged commit d815b5c into IdentityPython:master Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logout redirecting to non-url RelayState
2 participants