977188 - Display CSP errors separately from JS errors in the Developer HUD

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Jan Keromnes [:janx]]

In Firefox's Developer HUD, CSP errors need to be counted into a different category than JS errors. Their number could be displayed in a black widget on the far right of the screen, next to the red JS errors.

Comment 1

[Jan Keromnes [:janx]]

I'm about to separate the following error types into a new "security" category:

- Mixed Content Blocker
- Mixed Content Message
- CSP
- Invalid HSTS Headers
- Insecure Password Field
- SSL
- CORS

Stephanie, would that work for you, or did you just want CSP errors counted separately?

Comment 2

[Jan Keromnes [:janx]]

Attachment: Display security errors separately in the Developer HUD. r=ochameau

Comment 3

[Jan Keromnes [:janx]]

Attachment: Report security errors separately in the Developer HUD. r=21

Comment 4

[Jan Keromnes [:janx]]

Attachment: screenshot

Here is a screenshot of the code in action. The test app "quirks" is privileged and tries to do different nasty things:

- warning (CSP): "Content Security Policy: The page's settings blocked the loading of a resource at http://d3js.org/d3.v3.min.js"
- warning (CSP): "Content Security Policy: The page's settings blocked the loading of a resource: An attempt to execute inline scripts has been blocked"
- error (X-Frame-Options): "Load denied by X-Frame-Options: http://www.google.com/ does not permit cross-origin framing."

Note that only the two CSP warnings were counted as security issues (black widget).

The X-Frame-Options was counted with a SyntaxError as a regular error (red widget), I'm not sure if we want to count X-Frame errors as security issues.

The absence of a meta-viewport caused the single warning (orange widget).

Comment 5

[Jan Keromnes [:janx]]

Comment on attachment 8391298 [details] [diff] [review]
Report security errors separately in the Developer HUD. r=21

Actually let's wait for Stephanie's opinion.

Comment 6

[Stephanie Ouillon [:arroway] (needinfo me)]

Jan, reporting other security errors along with CSP sounds like a very good idea to me.

Comment 7

[Frederik Braun [:freddy]]

(In reply to Jan Keromnes [:janx] from comment #4)
> The X-Frame-Options was counted with a SyntaxError as a regular error (red
> widget), I'm not sure if we want to count X-Frame errors as security issues.

If you ask me, I wouldn't count it as a security error and argue as follows:
It's not like you (the developer) are doing something that violates browser or web security principles. You're doing something the website owner of the framed document would like to prevent. It's basically an error with the HTML you have written (or created via JS).

Comment 8

[Stephanie Ouillon [:arroway] (needinfo me)]

I agree with Freddy.

Comment 9

[Jan Keromnes [:janx]]

Ok, thanks for your input Stéphanie and Frederik! I'll go ahead with the current patch.

Comment 10

[Jan Keromnes [:janx]]

Comment on attachment 8391298 [details] [diff] [review]
Report security errors separately in the Developer HUD. r=21

Vivien, if you have a minute please take a look :)

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8391298 [details] [diff] [review]
Report security errors separately in the Developer HUD. r=21

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

Just want the question about indexOf to be answered before giving a r+.

::: b2g/chrome/content/devtools.js
@@ +39,5 @@
>    _frames: new Map(),
>    _client: null,
>    _webappsActor: null,
>    _watchers: [],
> +  _logging: false,

Basically same thing than bug 983710. I would like to keep this one to true by default.

@@ +87,5 @@
>          }
>        });
>      });
>  
> +    SettingsListener.observe('hud.logging', false, enabled => {

s/false/true

@@ +368,5 @@
>            metric = 'errors';
>            output += 'error (';
>          }
>  
> +        if (this._security.indexOf(pageError.category) > 0) {

Is it not >= 0 ?

Comment 12

[Jan Keromnes [:janx]]

Attachment: Report security errors separately in the Developer HUD. r=21

Comment 13

[Jan Keromnes [:janx]]

Comment on attachment 8392164 [details] [diff] [review]
Report security errors separately in the Developer HUD. r=21

Rebased, nits addressed, round #2.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #11)
> > +  _logging: false,
> 
> Basically same thing than bug 983710. I would like to keep this one to true
> by default.

Done.

> > +    SettingsListener.observe('hud.logging', false, enabled => {
> 
> s/false/true/

Replaced fixed value with `this._logging` to ensure default values stay coherent. Did likewise for the `'hud.' + metric` observers.

> > +        if (this._security.indexOf(pageError.category) > 0) {
> 
> Is it not >= 0 ?

Good catch, fixed to > -1.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ef4ee2255eeb

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef4ee2255eeb