dgtlmoon
Fix static redirect url
Currently the app is always redirecting to the root path after login. This is kind of annoying and defeats the purpose of using the {{ diff_url }} placeholder in notification templates.
Expected behavior I click on the link, login and I am getting redirected to the generated preview page.
Actual behavior I click on the link, login and I am on the watch list page. In order to see the preview page I either have to open the link again or I manually navigate there.
This PR should fix the issue.
there was some "safe flask redirect" setup to prevent various XSS hacks through this, do you know anything about it? thats mainly why it wasnt there
I did a quick research on the "safe flask redirect" setup and added the is_safe_url() method. Like this only local paths of the host the application is running on are allowed. Redirect tampering to external urls such as https://example.com or external apps (myapp://some/path) won't load anymore. Instead the watch list opens.
can you add a test in the test_security.py ? theres a login part, we could copy that to a new test in that file and then check the redirect lands on the right page (follow_redirect=true arg can be seen in other examples)
and also that redirect doesnt bounce us out to google or something
Heres a machine generated security report of the current PR...
1. Open Redirect Vulnerability in is_safe_url() Implementation ⚠️
The is_safe_url() function has a critical flaw:
def is_safe_url(path):
ref_url = urlparse(request.host_url)
test_url = urlparse(urljoin(request.host_url, path))
return test_url.scheme in ('http', 'https') and ref_url.netloc == test_url.netloc
Problem: This validation can be bypassed using protocol-relative URLs and absolute URLs with path confusion. Here are attack vectors:
Attack Vector 1: Protocol-relative URL
//evil.com
When passed to urljoin(), this becomes a valid absolute URL to evil.com while passing the validation if not carefully checked.
Attack Vector 2: URL with embedded newlines/CRLF
/some/path%0D%0ALocation:%20https://evil.com
Attack Vector 3: URL with @ symbol
//@evil.com
Depending on how urljoin processes this, it might interpret it as a netloc.
2. Missing Input Sanitization
The redirect parameter is taken directly from user input without length limits or character validation:
redirect_arg = request.args.get('redirect') or request.form.get('redirect')
This could allow:
- Very long URLs that could cause memory issues
- Special characters that might cause issues in downstream processing
- URL encoding bypasses (double-encoding attacks)
3. Unsafe Fallback Behavior
When redirect_arg is None or fails validation, it falls back to:
redirect_url = redirect_arg or url_for('watchlist.index')
However, if redirect_arg is an empty string "", it's falsy in Python but might have already been validated. This could lead to unexpected behavior.
def is_safe_redirect_url(target):
"""Validate redirect URLs are internal relative paths only."""
if not target or not isinstance(target, str):
return False
target = target.strip()
# Only allow relative paths starting with /
# Allow fragments (#) and query params (?)
return target.startswith('/') and not target.startswith('//')
Examples that work:
- /dashboard ✅
- /settings#profile ✅ (fragment)
- /page?foo=bar ✅ (query param)
- /page?foo=bar#section ✅ (query + fragment)
- /api/users#top ✅
Examples that are blocked:
- //evil.com#fake ❌ (starts with //)
- http://evil.com#fake ❌ (doesn't start with /)
- #fragment ❌ (doesn't start with /)
The only edge case is if someone wants to redirect to just a fragment on the current page like #section, but that's not really a redirect - that's a client-side anchor navigation that should be handled differently (usually just href="#section" in HTML, not a server redirect).
The current implementation works very similar. The difference is that sections as redirects are allowed. Passing a section as redirect has the same effect as /#section, what imo is not a problem. I added more tests for the other scenarios provided by the security report.
it really needs these extra checks
"""Validate redirect URLs are internal relative paths only."""
if not target or not isinstance(target, str):
return False
target = target.strip()
# Only allow relative paths starting with /
# Allow fragments (#) and query params (?)
return target.startswith('/') and not target.startswith('//')
@Genva when the redirect field is ////evil.com it takes me to evil.com, but that was before d7ac0cb, please add a test for the quadslash attack
in your test, it should prove that accessing /login?redirect=settings or whatever should work too, then all notifiactions can always include that /login as default but it will work even if theres no password or they are already logged in
it needs test that this works both with and without password setup
these kinds of edge cases are exactly what i deal with every day, i cannot just accept a PR without this stuff covered, or i'm the one who has to deal with, its not fair on me, is it?