1394681 - network monitor replaced with web content

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect)

Description

[:glob ✱]

Attachment: network tab replaced

nightly 57.0a1 (2017-08-28) (64-bit), osx

if you perform the follow steps, the network monitor dev tools content is replaced with a url of your choosing:

1. new tab, open dev tools, switch to network tab
2. navigate to a url that returns json containing a url
   eg https://drop.glob.com.au/landfill/sample.json
   which is just {"url":"https://glob.com.au/"}
3. show request details -> response tab
4. click on the url under the JSON section

expected result

- url field changes to <input> or similar, so you can copy the url

actual result

- entire network monitor tab changed to the url
- refreshing the page doesn't restore the original network tab content

Comment 1

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Im not sure how serious this is, but a couple of observations from testing:
 - the loaded page opens in the Chrome process (this would be a way to bypass sandboxing if it was trigger-able from a web content process
 - For the loaded web page, window.top is chrome://browser/content/browser.xul (not dangerous in itself, afaik, but not normal either)

Neither of those things are great, and its likely you could interfere with devtools from web content. (if you could coerce a user to follow the STR)

Comment 2

[Daniel Veditz [:dveditz]]

Is the content itself regular web content, or does it inherit chrome privileges from its parent? Does it have some kind of intermediate dev tools privileged access?

Rating sec-high just because this is a sandbox escape at least.

Comment 3

[:Gijs (he/him)]

Honza, can you take a look?

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1394681.patch

Try looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=546546ad0e34a265df0e2f0f20db7d71d189f812

Honza

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8904517 [details] [diff] [review]
bug1394681.patch

Review of attachment 8904517 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Honza!

I've verified that the current patch has been fixed the security issue. Follow the STR from description will open the url link in a new tab.

I just see one usability broken after clicking on the url link in response panel, then the url value will be changed into editable state, but it is unable to be changed back (to be an url link again).

Is this design intentional?

::: devtools/client/netmonitor/index.html
@@ +56,3 @@
>            const App = createFactory(require("./src/components/app"));
>            const sourceMapService = toolbox.sourceMapURLService;
> +          const app = App({sourceMapService, openLink });

nit: add whitespace before sourceMapService

::: devtools/client/netmonitor/src/components/app.js
@@ +20,5 @@
>  /*
>   * App component
>   * The top level component for representing main panel
>   */
> +function App({ statisticsOpen, sourceMapService, openLink }) {

nit: format component properties into multiple line with alphabetical order.

function App({
  openLink,
  statisticsOpen,
  sourceMapService,
}) {

@@ +25,3 @@
>    return (
>      div({ className: "network-monitor" },
> +      !statisticsOpen ? MonitorPanel({sourceMapService, openLink}) : StatisticsPanel()

nit: add whitespace between properties and define them in alphabetical order in possible.

{ openLink, sourceMapService }

@@ +34,5 @@
>  App.propTypes = {
>    statisticsOpen: PropTypes.bool.isRequired,
>    // Service to enable the source map feature.
>    sourceMapService: PropTypes.object,
> +  openLink: PropTypes.func,

nit: define propTypes in alphabetical order if possible.

::: devtools/client/netmonitor/src/components/params-panel.js
@@ +33,5 @@
>  /*
>   * Params panel component
>   * Displays the GET parameters and POST data of a request
>   */
> +function ParamsPanel({ request, openLink }) {

nit: Please convert these properties into multiline.

::: devtools/client/netmonitor/src/components/security-panel.js
@@ +23,5 @@
>   * If the site is being served over HTTPS, you get an extra tab labeled "Security".
>   * This contains details about the secure connection used including the protocol,
>   * the cipher suite, and certificate details
>   */
> +function SecurityPanel({ request, openLink }) {

nit: Please convert these properties into multiline.

::: devtools/client/netmonitor/src/components/stack-trace-panel.js
@@ +15,5 @@
>  
>  // Components
>  const StackTrace = createFactory(require("devtools/client/shared/components/stack-trace"));
>  
> +function StackTracePanel({ request, sourceMapService, openLink }) {

nit: Please convert these properties into multiline.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #5)
> I just see one usability broken after clicking on the url link in response
> panel, then the url value will be changed into editable state, but it is
> unable to be changed back (to be an url link again).
> 
> Is this design intentional?
Nice catch!

This isn't caused by this patch, the problem was there before.
I filled bug 1397228 for it.

All nits fixed.

Thanks for the review!

Honza

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: bug1394681.patch

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8904967 [details] [diff] [review]
bug1394681.patch

Review of attachment 8904967 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. thanks!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13af2a4eee010f9c8861ebb9292bd80febfd33f3

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13af2a4eee01