Closed Bug 977188 Opened 7 years ago Closed 7 years ago

Display CSP errors separately from JS errors in the Developer HUD

Categories

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

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S4 (28mar)

People

(Reporter: janx, Assigned: janx)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → All
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?
Flags: needinfo?(stephouillon)
Depends on: 983710
Attachment #8391255 - Attachment is obsolete: true
Attached image 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).
Attachment #8391298 - Flags: review?(21)
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.
Attachment #8391298 - Flags: review?(21)
Jan, reporting other security errors along with CSP sounds like a very good idea to me.
Flags: needinfo?(stephouillon)
(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).
I agree with Freddy.
Ok, thanks for your input Stéphanie and Frederik! I'll go ahead with the current patch.
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 :)
Attachment #8391298 - Flags: review?(21)
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 ?
Attachment #8391298 - Flags: review?(21) → feedback+
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.
Attachment #8392164 - Flags: review?(21)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef4ee2255eeb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → 1.4 S4 (28mar)
You need to log in before you can comment on or make changes to this bug.