[DevTools] Whitelist remoteFrontendUrl and remoteBase params.

[DevTools] Whitelist remoteFrontendUrl and remoteBase params.

This also fixes loadScriptsPromise to not normalize hostname.

BUG=619414, 618333
Committed: https://crrev.com/554517a4587bfb0071bcd3c7eff6645a0b06d72a
Cr-Commit-Position: refs/heads/master@{#400768}

[dgozman, 2016-06-15 13:50:06]

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

[dgozman, 2016-06-15 13:50:07]

Could you please take a look?

[caseq, 2016-06-16 12:51:16]

lgtm

https://codereview.chromium.org/2065823004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2065823004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/devtools.js:988: var
remoteBaseRegexp =
/^https:\/\/chrome-devtools-frontend\.appspot\.com\/serve_file\/@[0-9a-zA-Z]+\/?$/;
nit: consider extracting a constant. also, perhaps just use [\w] for brevity?

[dgozman, 2016-06-20 19:05:56]

https://codereview.chromium.org/2065823004/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/devtools.js (right):

https://codereview.chromium.org/2065823004/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/devtools.js:988: var
remoteBaseRegexp =
/^https:\/\/chrome-devtools-frontend\.appspot\.com\/serve_file\/@[0-9a-zA-Z]+\/?$/;
On 2016/06/16 12:51:16, caseq wrote:
> nit: consider extracting a constant. also, perhaps just use [\w] for brevity?

These two files cannot share any code.

[dgozman, 2016-06-20 19:06:00]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2016-06-20 19:06:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2065823004/1

[commit-bot: I haz the power, 2016-06-20 20:33:43]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-20 20:41:11]

Description was changed from

==========
[DevTools] Whitelist remoteFrontendUrl and remoteBase params.

This also fixes loadScriptsPromise to not normalize hostname.

BUG=619414,618333
==========

to

==========
[DevTools] Whitelist remoteFrontendUrl and remoteBase params.

This also fixes loadScriptsPromise to not normalize hostname.

BUG=619414,618333

Committed: https://crrev.com/554517a4587bfb0071bcd3c7eff6645a0b06d72a
Cr-Commit-Position: refs/heads/master@{#400768}
==========

[commit-bot: I haz the power, 2016-06-20 20:41:12]

Patchset 1 (id:??) landed as
https://crrev.com/554517a4587bfb0071bcd3c7eff6645a0b06d72a
Cr-Commit-Position: refs/heads/master@{#400768}

[Rafał Chłodnicki, 2016-06-30 14:36:39]

I wish that the URL would be constructed from constants defined in
chrome/browser/ui/webui/devtools_ui.cc and exposed to JS instead of hardcoding
them in two separate places.

We in Opera have to apply more patches if things are hardcoded like that.
If Google is potentially interested then I could improve that (although I'm not
very familiar with this code).

[dgozman, 2016-06-30 16:25:53]

On 2016/06/30 14:36:39, Rafał Chłodnicki wrote:
> I wish that the URL would be constructed from constants defined in
> chrome/browser/ui/webui/devtools_ui.cc and exposed to JS instead of hardcoding
> them in two separate places.
> 
> We in Opera have to apply more patches if things are hardcoded like that.
> If Google is potentially interested then I could improve that (although I'm
not
> very familiar with this code).

I see the problem. The two locations serve different purposes though:
devtools_ui constructs it, and devtools.js validates it for security reasons.
I think it's actually possible to do the validation in devtools_ui, and just
prevent navigation or not create DevToolsUIBindings when the url is suspicious.
It would be a bit harder to implement, but we can go for it. Are you interested
to contribute?

[Rafał Chłodnicki, 2016-07-01 10:02:42]

On 2016/06/30 16:25:53, dgozman wrote:
> On 2016/06/30 14:36:39, Rafał Chłodnicki wrote:
> > I wish that the URL would be constructed from constants defined in
> > chrome/browser/ui/webui/devtools_ui.cc and exposed to JS instead of
hardcoding
> > them in two separate places.
> > 
> > We in Opera have to apply more patches if things are hardcoded like that.
> > If Google is potentially interested then I could improve that (although I'm
> not
> > very familiar with this code).
> 
> I see the problem. The two locations serve different purposes though:
> devtools_ui constructs it, and devtools.js validates it for security reasons.
> I think it's actually possible to do the validation in devtools_ui, and just
> prevent navigation or not create DevToolsUIBindings when the url is
suspicious.
> It would be a bit harder to implement, but we can go for it. Are you
interested
> to contribute?

I wouldn't mind but I'm a bit overloaded right now. My managers have other
assignments for me.
And also you seem to have much better idea of how to fix that. :)
And I don't have access to related security bugs (although our security group
does and explained to me what are those about) so it's hard for me to
communicate about this issue.


(Also I can't CC myself on this review as it's closed so I'm not getting
notifications.)

[dgozman, 2016-07-06 17:08:00]

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

[dgozman, 2016-07-06 17:08:01]

+lushnikov, who was interested in this area.