Merge lp:~abreu-alexandre/webbrowser-app/harden-accepted-set-of-webapp-url-pattern into lp:webbrowser-app
- harden-accepted-set-of-webapp-url-pattern
- Merge into trunk
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 | ||||
Related bugs: |
|
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>
<scheme> := 'http' | 'https'
<host> := <any char except '/' and '.'>+ <hostpart> <hostpart>+
<hostpart> := '.' <any char except '/', '*', '?' and '.'>+
<path> := '/' <any chars>
e.g.
https?:
PS Jenkins bot (ps-jenkins) wrote : | # |
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:328
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:328
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:328
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:328
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:328
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:328
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:328
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
FAILURE: http://
FAILURE: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
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).
Olivier Tilloy (osomon) wrote : | # |
63 + QStringList parts = hostname.
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.
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
Alexandre Abreu (abreu-alexandre) wrote : | # |
> 63 + QStringList parts = hostname.
> 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),
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:329
http://
Executed test runs:
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
Click here to trigger a rebuild:
http://
Francis Ginther (fginther) wrote : | # |
Rerun of failed builed PASSED:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Continuous integration, rev:329
http://
Executed test runs:
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
FAILURE: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:329
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
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 CommandLinePars
49 +{
50 + QRegularExpression urlPatternTempl
51 + return urlPatternTempl
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.
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/
8 +#include <QtCore/
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 CommandLinePars
> urlPattern) const
> 49 +{
> 50 + QRegularExpression urlPatternTempl
> 51 + return urlPatternTempl
> 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
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/
> 8 +#include <QtCore/
done
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).
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
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:330
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
Olivier Tilloy (osomon) wrote : | # |
Looks good now, thanks!
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
PS Jenkins bot (ps-jenkins) wrote : | # |
PASSED: Continuous integration, rev:331
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild:
http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
SUCCESS: http://
PS Jenkins bot (ps-jenkins) wrote : | # |
FAILED: Autolanding.
More details in the following jenkins job:
http://
Executed test runs:
UNSTABLE: http://
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
UNSTABLE: http://
SUCCESS: http://
PS Jenkins bot (ps-jenkins) : | # |
Preview Diff
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() |
FAILED: Continuous integration, rev:328 jenkins. qa.ubuntu. com/job/ webbrowser- app-ci/ 414/ jenkins. qa.ubuntu. com/job/ generic- mediumtests- saucy/4143 jenkins. qa.ubuntu. com/job/ generic- mediumtests- touch/1788 jenkins. qa.ubuntu. com/job/ webbrowser- app-saucy- amd64-ci/ 297 jenkins. qa.ubuntu. com/job/ webbrowser- app-saucy- armhf-ci/ 297 jenkins. qa.ubuntu. com/job/ webbrowser- app-saucy- armhf-ci/ 297/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ webbrowser- app-saucy- i386-ci/ 297 jenkins. qa.ubuntu. com/job/ autopilot- testrunner- otto-saucy/ 444 jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- saucy-amd64/ 10 jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- saucy-amd64/ 10/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- saucy-armhf/ 1790 jenkins. qa.ubuntu. com/job/ generic- mediumtests- builder- saucy-armhf/ 1790/artifact/ work/output/ *zip*/output. zip jenkins. qa.ubuntu. com/job/ generic- mediumtests- runner- maguro/ 1492 jenkins. qa.ubuntu. com/job/ generic- mediumtests- runner- mako/1504
http://
Executed test runs:
UNSTABLE: http://
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
UNSTABLE: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
deb: http://
SUCCESS: http://
SUCCESS: http://
Click here to trigger a rebuild: s-jenkins: 8080/job/ webbrowser- app-ci/ 414/rebuild
http://