Url regex changes #302
Url regex changes #302
Conversation
The old URL regex had a few issues which were revealed by fuzzing, the biggest being that it accepted non-printable characters (e.g. 0x00 or 0x01) as part of the URL. This created the scenario where a url of https://example.com/[0x00] would be rendered as %2 (and attempting to open the link would give a value like "https://example.com https://example.com " due to some odd iteraction with the regex that I haven't quite worked out. The new regex appears to work with all the iterations I have tried and rejects non-printable characters.
Prevent attempts at phishing through unicode direction controls by forcing left-to-right display for links through html entity ‪ This is a fairly minor risk as a victim would have to go through many hoops and not see the obvious url issues. But better fixed than not.
|
Snapshot of new behavior.
|
| @@ -41,7 +41,7 @@ LinkedText::LinkedText(QObject *parent) | |||
| : QObject(parent) | |||
| { | |||
| // Select things that look like URLs of some kind and allow QUrl::fromUserInput to validate them | |||
| linkRegex = QRegularExpression(QStringLiteral("([a-z]{3,9}:|www\\.)([^\\s,.);!>]|[,.);!>](?!\\s|$))+"), QRegularExpression::CaseInsensitiveOption); | |||
| linkRegex = QRegularExpression(QStringLiteral("([a-z]{3,9}:|www\\.)((([\\p{L}\\p{N}\\?#/~=]+)|([\\-\\._:&%][^\\p{Zs}])+)+)"), QRegularExpression::CaseInsensitiveOption); | |||
There was a problem hiding this comment.
This expression isn't quite right; some common symbols won't be included in the URL, and the trailing punctuation logic worked better before. I can also break it with some combining characters or emoji (although this is going far beyond normal).
What do you think of blacklisting \p{C} to remove control characters instead? Is that suffcient?
Something like:
([a-z]{3,9}:|www\\.)([^\\p{Z}\\p{C}<>"',.)\\[\\];!]|[,.)\\[\\];!](?!\\s|$))+
See https://regex101.com/r/tL7wM3/1 - but I didn't try to get BIDI or control characters in there yet.
There was a problem hiding this comment.
Yeah, looks like my proposed one misses certain url params, not good. The new one looks better - and looks like control characters are blocked as expected - but will confirm through recoil later this evening.
On a related note, can we get a formalized spec of how we think this should work, for future reference.
- URLs are detected if they start with a protocol specifier or with "www." and are followed by at least one non-unicode character.
- A space always terminates the URL
- ". " always terminates the URL and excludes the final period.
- "! " always terminates the URL and excludes the final exclamation.
- ", " always terminates the URL and excludes the final comma.
- Unicode Control characters are not permitted inside the url and will terminate the URL.
This spec seems incomplete as www.?())@($3423#@$@#$# qualifies as a URL - although it is likely better to fall on the side of forgiveness - we can always refine it later.
Combined PR with new Regex Changes to fixup some URL issues parsing odd control characters and prevent phishing through unicode bidi characters in links. Will close the old PRs.