[DevTools] Move sanitize url to devtools_ui.cc.

[DevTools] Move sanitize url to devtools_ui.cc.

Compatibility script is not reliable enough.

BUG=653134
Committed: https://crrev.com/f865c2dfddb1d95af3a2467587c62566e3f7dfe4
Cr-Commit-Position: refs/heads/master@{#425814}

[dgozman, 2016-10-08 00:42:25]

dgozman@chromium.org changed reviewers:
+ pfeldman@chromium.org

[dgozman, 2016-10-08 00:42:25]

Could you please take a look?

[pfeldman, 2016-10-08 01:01:44]

https://codereview.chromium.org/2403633002/diff/1/chrome/browser/ui/webui/dev...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2403633002/diff/1/chrome/browser/ui/webui/dev...
chrome/browser/ui/webui/devtools_ui.cc:183: GURL fullUrl = FullURLForPath(path);
Lets introduce

GURL sanitizeFrontendURL(GURL)
{
   .. that would
   1) enforce our domain and path
   2) only pass whitelist of params
   3) sanitize these query params
}

https://codereview.chromium.org/2403633002/diff/1/chrome/browser/ui/webui/dev...
chrome/browser/ui/webui/devtools_ui.cc:186: if (it.GetKey() == "settings")
Extract each of these and provide descriptions on why these are validated.

https://codereview.chromium.org/2403633002/diff/1/content/renderer/devtools/d...
File content/renderer/devtools/devtools_client.cc (right):

https://codereview.chromium.org/2403633002/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_client.cc:32: #if defined(DEBUG_DEVTOOLS)
open it, it is for us.

[dgozman, 2016-10-10 23:27:27]

Please take another look.

https://codereview.chromium.org/2403633002/diff/1/chrome/browser/ui/webui/dev...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2403633002/diff/1/chrome/browser/ui/webui/dev...
chrome/browser/ui/webui/devtools_ui.cc:183: GURL fullUrl = FullURLForPath(path);
On 2016/10/08 01:01:44, pfeldman wrote:
> Lets introduce
> 
> GURL sanitizeFrontendURL(GURL)
> {
>    .. that would
>    1) enforce our domain and path
>    2) only pass whitelist of params
>    3) sanitize these query params
> }

Done.

https://codereview.chromium.org/2403633002/diff/1/content/renderer/devtools/d...
File content/renderer/devtools/devtools_client.cc (right):

https://codereview.chromium.org/2403633002/diff/1/content/renderer/devtools/d...
content/renderer/devtools/devtools_client.cc:32: #if defined(DEBUG_DEVTOOLS)
On 2016/10/08 01:01:44, pfeldman wrote:
> open it, it is for us.

Done.

[pfeldman, 2016-10-11 20:51:34]

lgtm

https://codereview.chromium.org/2403633002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/2403633002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:108: std::string
ConvertFlagsToQueryParams(const std::string flags) {
Lets assume it is already of the target form.

[dgozman, 2016-10-11 21:09:14]

https://codereview.chromium.org/2403633002/diff/60001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_window.cc (right):

https://codereview.chromium.org/2403633002/diff/60001/chrome/browser/devtools...
chrome/browser/devtools/devtools_window.cc:108: std::string
ConvertFlagsToQueryParams(const std::string flags) {
On 2016/10/11 20:51:34, pfeldman wrote:
> Lets assume it is already of the target form.

Done.

[dgozman, 2016-10-11 21:10:22]

dgozman@chromium.org changed reviewers:
+ tsepez@chromium.org

[dgozman, 2016-10-11 21:10:22]

Tom, could you please take a look at sanitization code in devtools_ui.cc?

[Tom Sepez, 2016-10-11 22:52:18]

https://codereview.chromium.org/2403633002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2403633002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/devtools_ui.cc:61: if (revision[i] != '@'
nit: if you want to be really picky, you might insist that @ occurs at 0, and
nowhere else.

https://codereview.chromium.org/2403633002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/devtools_ui.cc:148: it.GetUnescapedValue());
suppose we have ws=1%26evil%3dtrue
presumably it.GetKey() returns "ws"
presumably it.GetUnescapedValue() returns "1&evil=true"
this passes through SanitizeFrontendQueryParam unmolested.
then the sprintf result is ws=1&evil=true

and we've injected a parameter.

[dgozman, 2016-10-12 22:00:38]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 22:01:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dgozman, 2016-10-12 22:10:33]

Tom, please take another look.

https://codereview.chromium.org/2403633002/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/devtools_ui.cc (right):

https://codereview.chromium.org/2403633002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/devtools_ui.cc:61: if (revision[i] != '@'
On 2016/10/11 22:52:18, Tom Sepez wrote:
> nit: if you want to be really picky, you might insist that @ occurs at 0, and
> nowhere else.

Nice! Done.

https://codereview.chromium.org/2403633002/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/devtools_ui.cc:148: it.GetUnescapedValue());
On 2016/10/11 22:52:18, Tom Sepez wrote:
> suppose we have ws=1%26evil%3dtrue
> presumably it.GetKey() returns "ws"
> presumably it.GetUnescapedValue() returns "1&evil=true"
> this passes through SanitizeFrontendQueryParam unmolested.
> then the sprintf result is ws=1&evil=true
> 
> and we've injected a parameter.

Right! We should not unescape ever and just pass it as is. Fixed and added a
test.

I've also added validation of endpoints - they should not contain & and ?.

The only unescaping we do now is for remoteFrontendUrl, which we escape right
back in the same function.

[commit-bot: I haz the power, 2016-10-12 22:24:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-12 22:24:54]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dgozman, 2016-10-13 23:58:03]

The CQ bit was checked by dgozman@chromium.org to run a CQ dry run

[dgozman, 2016-10-13 23:58:32]

@tsepez: friendly ping

[commit-bot: I haz the power, 2016-10-13 23:58:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-14 01:42:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 01:42:40]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2016-10-17 19:18:38]

Sorry about the delay. LGTM.

[dgozman, 2016-10-17 20:19:12]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-10-17 20:19:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2403633002/#ps120001
(title: "disable test on android and ios - there is no devtools_ui")

[commit-bot: I haz the power, 2016-10-17 20:20:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 23:36:03]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-17 23:37:50]

Description was changed from

==========
[DevTools] Move sanitize url to devtools_ui.cc.

Compatibility script is not reliable enough.

BUG=653134
==========

to

==========
[DevTools] Move sanitize url to devtools_ui.cc.

Compatibility script is not reliable enough.

BUG=653134

Committed: https://crrev.com/f865c2dfddb1d95af3a2467587c62566e3f7dfe4
Cr-Commit-Position: refs/heads/master@{#425814}
==========

[commit-bot: I haz the power, 2016-10-17 23:37:52]

Patchset 7 (id:??) landed as
https://crrev.com/f865c2dfddb1d95af3a2467587c62566e3f7dfe4
Cr-Commit-Position: refs/heads/master@{#425814}