False positive: Confusing "... may be at an arbitrary position in the sanitized URL."

[The-Compiler]

Description of the false positive

To be honest I don't didn't really understand the aim of the rule at all - but it is reporting code like:

• assert 'http://www.example.com/' in web_history
• assert 'https://github.com' in bookmarks.marks
• assert 'https://python.org' in web_history_populated
• assert 'http://example.com' in bm.marks

All of those are checking whether a string (which happens to be an URL) is in a set or list. This doesn't seem to be related to sanitization at all...

Looking at the query and help I think flagging things like the above isn't the intention of the rule? The help says:

Sanitizing untrusted URLs is an important technique for
preventing attacks such as request forgeries and malicious
redirections. Usually, this is done by checking that the host of a URL
is in a set of allowed hosts.

However, it is notoriously error-prone to treat the URL as
a string and check if one of the allowed hosts is a substring of the
URL. Malicious URLs can bypass such security checks by embedding one
of the allowed hosts in an unexpected location.

Even if the substring check is not used in a
security-critical context, the incomplete check may still cause
undesirable behaviors when the check succeeds accidentally.

So I assume what the query actually indends to check is something like if 'https://example.com' in url - but then the RHS must be a string, which is not the case here.

cc @markshannon who added the rule in #820.

URL to the alert on the project page on LGTM.com

I'm using CodeQL via GitHub's security scanning (which is in Beta) - so I hope I'm reporting this in the right place, given that the rules behind the two sites seem to be the same...

Here are the alerts I think are false-positives: https://github.com/qutebrowser/qutebrowser/security/code-scanning?query=tool%3ACodeQL+is%3Aclosed+id%3Apy%2Fincomplete-url-substring-sanitization

[tausbn]

Thank you for your report. I agree that these are likely false positives (though curiously I do not seem to have permissions to look at the Code Scanning results. I'll have to look into this tomorrow.)

In general, this query is quite noisy, as it relies on a fairly coarse heuristic for guessing what's an instance of URL sanitisation (and does relatively little to see whether this supposed lack of sanitisation really is important). You're not the first person to report an issue with it, and I expect we'll revisit this in the not-too-distant future to eliminate these (and other) false positives.

[RasmusWL]

Since this project is also on LGTM.com, I found one of the alerts in a test file (which makes the alert hidden by default on LGTM.com): https://lgtm.com/projects/g/qutebrowser/qutebrowser/latest/files/tests/unit/completion/test_models.py?sort=name&dir=ASC&mode=heatmap&showExcluded=true#L598