✨ Optionally validate DigiD mock IDP callback urls #86
Conversation
swrichards
force-pushed
the
85-validate-mock-idp-callbacks
branch
from
be766a1 to
7abfb4e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #86 +/- ##
==========================================
+ Coverage 92.31% 92.50% +0.19%
==========================================
Files 48 48
Lines 1691 1734 +43
Branches 107 116 +9
==========================================
+ Hits 1561 1604 +43
Misses 96 96
Partials 34 34
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
swrichards
force-pushed
the
85-validate-mock-idp-callbacks
branch
3 times, most recently
from
2194959 to
97b9b9e
Compare
swrichards
force-pushed
the
85-validate-mock-idp-callbacks
branch
2 times, most recently
from
d7e593b to
b16210d
Compare
digid_eherkenning/views/base.py
Outdated
| def is_relative_path(uri: str) -> bool: | ||
| """Check if `uri` is a relative path.""" | ||
| parsed = urlparse(uri) | ||
| return not (parsed.scheme or parsed.netloc) |
There was a problem hiding this comment.
here be dragons - please check the comments in the django source code for _url_has_allowed_host_and_scheme!
There was a problem hiding this comment.
Fair, there's some gnarly edge cases to consider, clearly. In light of that, I think life's too short (in the context of this story) to care much about the relative callbacks. Delegating this fully to the Django library, which means callbacks will have to be fully qualified if the flag is set.
There was a problem hiding this comment.
@sergei-maertens I fully delegated the safety checks to the existing utility helper, with the caveat that non-https callbacks are allowed if the mock IDP is currently hosted under regular HTTP. Related to this, the default setting for the flag is taken from settings.DEBUG, on the assumption that the callback validation is not required for (local) dev, and if somebody is using the IDP in a world-facing environment, the validation should be enabled (and thus follow the DEBUG setting, further assuming that best practices wrt disabling DEBUG are followed).
There was a problem hiding this comment.
f the mock IDP is currently hosted under regular HTTP
hosting anything not under HTTPS and throwing pentests at it is an exercise in futility anyway, so acknowledged but dismissed :-)
swrichards
force-pushed
the
85-validate-mock-idp-callbacks
branch
from
9c58811 to
484e3e9
Compare
| """ | ||
| Whether to validate that the `next_url` and `cancel_urls` are safe | ||
| """ | ||
| flag = getattr(settings, "DIGID_MOCK_IDP_VALIDATE_CALLBACK_URLS", settings.DEBUG) |
There was a problem hiding this comment.
@sergei-maertens I clearly had a brain fade here, this should have been not settings.DEBUG 🤦♂️ I'll fix that up separately for a future patch release, don't think this warrants any other drastic action.
There was a problem hiding this comment.
Feel to just patch it on master and bump the version + tag it, then we don't forget about it.
Closes #85