Prevent open redirects when a malformed URL is passed to ?next=

[boydgreenfield]

Example: "/login?next=http:///google.com" (note 3rd slash)

This is due to the way urlparse.urlsplit splits URLs. Specifically:

# URL with only a path (no scheme, no netloc)
urlparse.urlsplit("/index.html")
SplitResult(scheme='', netloc='', path='/index.html', query='', fragment='')

# Standard URL has a scheme and a netloc
urlparse.urlsplit("http://www.google.com")
SplitResult(scheme='http', netloc='www.google.com', path='', query='', fragment='')

# Malformed URL has no netloc, but has a scheme
urlparse.urlsplit("ftp:///evil.com")
SplitResult(scheme='ftp', netloc='', path='/evil.com', query='', fragment='')

Consequently, in the old validate_redirect_url function, which did this:

def validate_redirect_url(url):
    if url is None or url.strip() == '':
        return False
    url_next = urlsplit(url)
    url_base = urlsplit(request.host_url)
    if url_next.netloc and url_next.netloc != url_base.netloc:
        return False
    return True

This condition would not be met for malformed URLs and it would return True (valid URL).

The included patch changes the code to return False if the url_next variable has a scheme and url_next.netloc != url_base.netloc (it's insufficient to simply check if url_next.netloc != url_base.netloc because then relative redirects, such as ?next=/index.html fail). The fix is simply:

def validate_redirect_url(url):
    if url is None or url.strip() == '':
        return False
    url_next = urlsplit(url)
    url_base = urlsplit(request.host_url)
    if url_next.scheme and url_next.netloc != url_base.netloc:  # Changed line
        return False
    return True

This passes all the tests, and I can't think of any other nasty edge cases it might cause, but an alternative fix could be:

def validate_redirect_url(url):
    if url is None or url.strip() == '':
        return False
    url_next = urlsplit(url)
    url_base = urlsplit(request.host_url)
    if url_next.scheme and not url_next.netloc:
        return False
    elif url_next.netloc and url_next.netloc != url_base.netloc:
        return False
    return True

Credit to Muhammad Talha Khan for finding this.

[coveralls]

Coverage remained the same when pulling 8b036f2 on boydgreenfield:develop into 76ad77a on mattupstate:develop.

[boydgreenfield]

Note: This passed the tests for everything except PyPy, which looks like some kind of unrelated non-deterministic failure. Unfortunately, I don't believe I have the ability to tell Travis CI to re-run the test (there's nothing where the re-run button normally appears). Please feel free to re-run the PyPy build, I believe everything should pass / be 100% OK.

[jamesonjlee]

ah yes the http:/// is http:// not http://localhost/ thing.
How about if (url_next.netloc or url_next.scheme) and url_next.netloc != url_base.netloc: which should be equivalent?

[boydgreenfield]

That should work – a bit harder to read, but does preserve any edge cases that rely on url_next.netloc existing in a case where url_next.scheme doesn't (this can only arise locally? or not at all?).

Travis is building and testing now.

[coveralls]

Coverage remained the same when pulling 5bc37ad on boydgreenfield:develop into 76ad77a on mattupstate:develop.

[mattupstate]

Great catch. Thanks for this.