Merge lp:~abreu-alexandre/webbrowser-app/harden-accepted-set-of-webapp-url-pattern into lp:webbrowser-app

Proposed by Alexandre Abreu
Status: Merged
Approved by: Olivier Tilloy
Approved revision: 331
Merged at revision: 361
Proposed branch: lp:~abreu-alexandre/webbrowser-app/harden-accepted-set-of-webapp-url-pattern
Merge into: lp:webbrowser-app
Diff against target: 138 lines (+61/-3)
3 files modified
src/app/commandline-parser.cpp (+28/-1)
src/app/commandline-parser.h (+9/-0)
tests/unittests/commandline-parser/tst_CommandLineParserTests.cpp (+24/-2)
To merge this branch: bzr merge lp:~abreu-alexandre/webbrowser-app/harden-accepted-set-of-webapp-url-pattern
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve
Olivier Tilloy Approve
Review via email: mp+188186@code.launchpad.net

Commit message

Harden the set of accepted url patterns.

Description of the change

Harden the set of accepted url patterns.

Tests for the validity of a given webapp url pattern. It follows the following grammar:

<url-pattern> := <scheme>://<host><path>
<scheme> := 'http' | 'https'
<host> := <any char except '/' and '.'>+ <hostpart> <hostpart>+
<hostpart> := '.' <any char except '/', '*', '?' and '.'>+
<path> := '/' <any chars>

e.g.

https?://*.mydomain.com/my/path/*

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Since you have a pretty well-defined grammar to validate, why not use a regular expression to match it? That would make for much less verbose code (and would thus be easier to maintain or update).

Revision history for this message
Olivier Tilloy (osomon) wrote :

63 + QStringList parts = hostname.split(".");
64 + if (parts.count() < 3)
65 + {
66 + return false;
67 + }

I don’t understand this condition: isn’t "ubuntu.com" a valid hostname? It only has two parts.

review: Needs Information
Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> Since you have a pretty well-defined grammar to validate, why not use a
> regular expression to match it? That would make for much less verbose code
> (and would thus be easier to maintain or update).

hah! indeed the grammar is fairly regular :), work for me ... done

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> 63 + QStringList parts = hostname.split(".");
> 64 + if (parts.count() < 3)
> 65 + {
> 66 + return false;
> 67 + }
>
> I don’t understand this condition: isn’t "ubuntu.com" a valid hostname? It
> only has two parts.

yes but we are dealing with patterns, not validating URLs; ubuntu.com is not a pattern
*.ubuntu.com is (and pretty much sums up the intended behavior),

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Francis Ginther (fginther) wrote :
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good to me. A couple of minor comments on style and performance:

23 + qDebug() << "Ignoring empty or invalid webapp url pattern: '"
24 + << url
25 + << "'";

qDebug() inserts spaces between each stream it’s being passed, so you probably want to remove the single quotes surrounding url.

48 +bool CommandLineParser::isValidWebappUrlPattern(const QString & urlPattern) const
49 +{
50 + QRegularExpression urlPatternTemplate(…);
51 + return urlPatternTemplate.match(urlPattern).hasMatch();
52 +}

Instead of instantiating the regexp everytime the method is called, you should make it a member variable so that it’s compiled only once at startup.

Revision history for this message
Olivier Tilloy (osomon) wrote :

And another style comment (picking nits, really): I usually try to keep include statements sorted alphabetically (I know the rule is not written anywhere). Can you please re-order this one?

7 #include <QtCore/QTextStream>
8 +#include <QtCore/QRegularExpression>

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> Looks good to me. A couple of minor comments on style and performance:
>
> 23 + qDebug() << "Ignoring empty or
> invalid webapp url pattern: '"
> 24 + << url
> 25 + << "'";
>
> qDebug() inserts spaces between each stream it’s being passed, so you probably
> want to remove the single quotes surrounding url.

done

>
> 48 +bool CommandLineParser::isValidWebappUrlPattern(const QString &
> urlPattern) const
> 49 +{
> 50 + QRegularExpression urlPatternTemplate(…);
> 51 + return urlPatternTemplate.match(urlPattern).hasMatch();
> 52 +}
>
> Instead of instantiating the regexp everytime the method is called, you should
> make it a member variable so that it’s compiled only once at startup.

done

Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> And another style comment (picking nits, really): I usually try to keep
> include statements sorted alphabetically (I know the rule is not written
> anywhere). Can you please re-order this one?
>
> 7 #include <QtCore/QTextStream>
> 8 +#include <QtCore/QRegularExpression>

done

Revision history for this message
Olivier Tilloy (osomon) wrote :

> > Instead of instantiating the regexp everytime the method is called, you
> should
> > make it a member variable so that it’s compiled only once at startup.
>
> done

But you didn’t remove the local variable, and you’re still matching against it (the static member is unused).

review: Needs Fixing
Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

> > > Instead of instantiating the regexp everytime the method is called, you
> > should
> > > make it a member variable so that it’s compiled only once at startup.
> >
> > done
>
> But you didn’t remove the local variable, and you’re still matching against it
> (the static member is unused).

:/ done

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

Looks good now, thanks!

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/commandline-parser.cpp'
2--- src/app/commandline-parser.cpp 2013-09-24 21:05:55 +0000
3+++ src/app/commandline-parser.cpp 2013-10-01 13:19:15 +0000
4@@ -29,6 +29,10 @@
5 // stdlib
6 #include <cstdio>
7
8+QRegularExpression
9+CommandLineParser::m_webappUrlPatternTemplate("^http(s|s\\?)?://[^\\.]+\\.[^\\.\\*\\?]+\\.[^\\.\\*\\?]+(\\.[^\\.\\*\\?/]+)*/.*$");
10+
11+
12 CommandLineParser::CommandLineParser(QStringList arguments, QObject* parent)
13 : QObject(parent)
14 , m_help(false)
15@@ -65,10 +69,14 @@
16 Q_FOREACH(const QString & includePattern, includePatterns)
17 {
18 QString url = includePattern.trimmed();
19- if ( ! url.isEmpty())
20+ if ( ! url.isEmpty() && isValidWebappUrlPattern(url))
21 {
22 m_webappUrlPatterns.append(url);
23 }
24+ else
25+ {
26+ qDebug() << "Ignoring empty or invalid webapp url pattern:" << url;
27+ }
28 }
29 }
30 } else if (argument == "--enable-back-forward") {
31@@ -148,6 +156,25 @@
32 out << " --enable-addressbar enable the display of the address bar" << endl;
33 }
34
35+
36+/**
37+ * Tests for the validity of a given webapp url pattern. It follows
38+ * the following grammar:
39+ *
40+ * <url-pattern> := <scheme>://<host><path>
41+ * <scheme> := 'http' | 'https'
42+ * <host> := '*' <any char except '/' and '.'>+ <hostpart> <hostpart>+
43+ * <hostpart> := '.' <any char except '/', '*', '?' and '.'>+
44+ * <path> := '/' <any chars>
45+ *
46+ * @param urlPattern pattern that is to be tested for validity
47+ * @return if the url is valid or not
48+ */
49+bool CommandLineParser::isValidWebappUrlPattern(const QString & urlPattern) const
50+{
51+ return m_webappUrlPatternTemplate.match(urlPattern).hasMatch();
52+}
53+
54 CommandLineParser::ChromeElementFlags CommandLineParser::chromeFlags() const
55 {
56 return m_chromeFlags;
57
58=== modified file 'src/app/commandline-parser.h'
59--- src/app/commandline-parser.h 2013-09-17 14:46:16 +0000
60+++ src/app/commandline-parser.h 2013-10-01 13:19:15 +0000
61@@ -21,10 +21,12 @@
62
63 // Qt
64 #include <QtCore/QObject>
65+#include <QtCore/QRegularExpression>
66 #include <QtCore/QString>
67 #include <QtCore/QStringList>
68 #include <QtCore/QUrl>
69
70+
71 class CommandLineParser : public QObject
72 {
73 Q_OBJECT
74@@ -67,6 +69,11 @@
75 ChromeElementFlags chromeFlags() const;
76
77 private:
78+
79+ bool isValidWebappUrlPattern(const QString & pattern) const;
80+
81+
82+private:
83 bool m_help;
84 bool m_fullscreen;
85 bool m_maximized;
86@@ -78,6 +85,8 @@
87 QString m_appid;
88 QStringList m_webappUrlPatterns;
89 ChromeElementFlags m_chromeFlags;
90+
91+ static QRegularExpression m_webappUrlPatternTemplate;
92 };
93
94 Q_DECLARE_OPERATORS_FOR_FLAGS(CommandLineParser::ChromeElementFlags)
95
96=== modified file 'tests/unittests/commandline-parser/tst_CommandLineParserTests.cpp'
97--- tests/unittests/commandline-parser/tst_CommandLineParserTests.cpp 2013-09-25 05:18:39 +0000
98+++ tests/unittests/commandline-parser/tst_CommandLineParserTests.cpp 2013-10-01 13:19:15 +0000
99@@ -251,8 +251,8 @@
100 QTest::addColumn<QStringList>("patterns");
101
102 QString BINARY("webbrowser-app");
103- QString INCLUDE_PATTERN("http://www.ubuntu.*/*");
104- QString INCLUDE_PATTERN2("http://www.bbc.*/*");
105+ QString INCLUDE_PATTERN("https?://*.ubuntu.com/*");
106+ QString INCLUDE_PATTERN2("https?://www.bbc.co.uk/*");
107
108 QTest::newRow("no switch") << (QStringList() << BINARY) << QStringList();
109
110@@ -266,6 +266,28 @@
111 QTest::newRow("switch and multiple trimmed pattern")
112 << (QStringList() << BINARY << (QString("--webappUrlPatterns=") + INCLUDE_PATTERN + " , " + INCLUDE_PATTERN2 + " , "))
113 << (QStringList() << INCLUDE_PATTERN << INCLUDE_PATTERN2);
114+
115+#define WEBAPP_INVALID_URL_PATTERN_TEST(id,invalid_url_pattern) \
116+ QTest::newRow("switch and invalid pattern " #id) \
117+ << (QStringList() << BINARY << (QString("--webappUrlPatterns=") + INCLUDE_PATTERN + "," + invalid_url_pattern)) \
118+ << (QStringList() << INCLUDE_PATTERN)
119+
120+ WEBAPP_INVALID_URL_PATTERN_TEST(1, "http");
121+ WEBAPP_INVALID_URL_PATTERN_TEST(2, "://");
122+ WEBAPP_INVALID_URL_PATTERN_TEST(3, "file://");
123+ WEBAPP_INVALID_URL_PATTERN_TEST(4, "https?://");
124+ WEBAPP_INVALID_URL_PATTERN_TEST(5, "https?://*");
125+ WEBAPP_INVALID_URL_PATTERN_TEST(6, "https?://foo.*");
126+ WEBAPP_INVALID_URL_PATTERN_TEST(7, "https?://foo.ba*r.com");
127+ WEBAPP_INVALID_URL_PATTERN_TEST(8, "https?://foo.*.com/");
128+ WEBAPP_INVALID_URL_PATTERN_TEST(9, "https?://foo.bar.*/");
129+ WEBAPP_INVALID_URL_PATTERN_TEST(10, "https?://*.bar.*");
130+ WEBAPP_INVALID_URL_PATTERN_TEST(11, "https?://*.bar.*/");
131+ WEBAPP_INVALID_URL_PATTERN_TEST(12, "https?://*.bar.*/");
132+ WEBAPP_INVALID_URL_PATTERN_TEST(13, "httpsfoo?://*.bar.com/");
133+ WEBAPP_INVALID_URL_PATTERN_TEST(14, "httppoo://*.bar.com/");
134+
135+#undef WEBAPP_INVALID_URL_PATTERN_TEST
136 }
137
138 void shouldUseIncludes()

Subscribers

People subscribed via source and target branches

to status/vote changes: