Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Url regex changes #302

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Url regex changes #302

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a10ce3
Updating URL Regex
s-rah Oct 13, 2015
c62a7d1
Prevents Bidi Phishing
s-rah Nov 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/ui/LinkedText.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


allowedSchemes << QStringLiteral("http")
<< QStringLiteral("https")
Expand All @@ -64,7 +64,12 @@ QString LinkedText::parsed(const QString &input)

if (start > p)
re.append(input.mid(p, start - p).toHtmlEscaped().replace(QLatin1Char('\n'), QStringLiteral("<br/>")));
re.append(QStringLiteral("<a href=\"%1\">%2</a>").arg(QString::fromLatin1(url.toEncoded()).toHtmlEscaped()).arg(match.capturedRef().toString().toHtmlEscaped()));

// Surround link with &#8234; (LEFT-TO-RIGHT EMBEDDING) and &#8236; (POP DIRECTIONAL FORMATTING )
// this will force URI's to be rendered left to right, while preserving the direction of the overall
// text. This prevents phising attacks where the attacker tries obscure the URI by using unicode
// bidi control characters.
re.append(QStringLiteral("<a href=\"%1\">&#8234;%2&#8236;</a>").arg(QString::fromLatin1(url.toEncoded()).toHtmlEscaped()).arg(match.capturedRef().toString().toHtmlEscaped()));
p = match.capturedEnd();
}

Expand Down