796740 - CSP violations warnings in System app

Status: RESOLVED FIXED

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

Description

[GH to BZ]

[GitHub issue by fabricedesre on 2012-09-25T21:34:05Z, https://github.com/mozilla-b2g/gaia/issues/5177]
Since bug 768029 landed, the system app doesn't start anymore because of this:

E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onload attribute on UNKNOWN element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onsubmit attribute on FORM element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on SECTION element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on BUTTON element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on DIV element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on DIV element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on DIV element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on DIV element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on BUTTON element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on DIV element"}]
E/GeckoConsole(  106): [JavaScript Warning: "CSP WARN:  Directive inline script base restriction violated
E/GeckoConsole(  106): " {file: "app://system.gaiamobile.org/index.html" line: 0 column: 0 source: "onmousedown attribute on BUTTON element"}]

Comment 1

[GH to BZ]

[GitHub comment by autonome on 2012-09-26T06:44:46Z]
how does this get fixed? who can do it?

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

System app obviously works so that means we have not turn on the pref yet. Is there a bug number that we could reference?

I can take this bug if this is indeed P1.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ah, there is a bug.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What we did was that we changed the CSP policy to be much more permissive. So you shouldn't be seeing the warnings in comment 0 any more. However we should turn the pref back to the more restrictive policy, which would show those warnings again, and likely also break the system app.

So what we need to do here is:

1. Change the "security.apps.privileged.CSP.default" pref to
   "default-src *; script-src 'self'; object-src 'none'; style-src 'self'
    'unsafe-inline'"

2. See what warnings start showing up.

3. Fix code so that warnings no longer show up.

4. Check in new code as well as new pref value.


Note that it's very likely that this affects more apps than the system app. So we need to test and fix all built-in apps in steps 2 and 3.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: WIP

Work in progress, looks like the only thing I broke is the scrolling of the value selector. Rudy, can you find out what's wrong?



Feedback: [e-mail]

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 669082 [details] [diff] [review]
WIP

Ah, |git bz| doesn't support set up feedback bit.

Comment 7

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Hi Tim,

Sorry for the late reply.
I am not sure how about combine my changes with yours, so I put my patch to value_seletor.js here, https://gist.github.com/3857440

1. remove the "evt.stopPropagation();" in mousedown event handler.
2. override onmousedown for 'value-picker-hours' and other value pickers used in the time picker.

Thanks.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1

I am setting r? to Vivien but I think everyone familiar with value selector or system app can do the review.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jonas Sicking (:sicking) from comment #4)
> What we did was that we changed the CSP policy to be much more permissive.
> So you shouldn't be seeing the warnings in comment 0 any more. However we
> should turn the pref back to the more restrictive policy, which would show
> those warnings again, and likely also break the system app.
> 
> So what we need to do here is:
> 
> 1. Change the "security.apps.privileged.CSP.default" pref to
>    "default-src *; script-src 'self'; object-src 'none'; style-src 'self'
>     'unsafe-inline'"
> 
> 2. See what warnings start showing up.
> 
> 3. Fix code so that warnings no longer show up.
> 
> 4. Check in new code as well as new pref value.
> 
> 
> Note that it's very likely that this affects more apps than the system app.
> So we need to test and fix all built-in apps in steps 2 and 3.

Cool, I will test this against my patch. For other apps you should file bugs against them.

Comment 10

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

Comment on attachment 669453 [details] [diff] [review]
Patch v1

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

::: apps/system/js/value_selector/value_selector.js
@@ +128,5 @@
>          }
>          break;
>  
> +      case 'submit':
> +        // Prevent the form from submit.

Does it miss a break or is it intentional?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jonas Sicking (:sicking) from comment #4)
> What we did was that we changed the CSP policy to be much more permissive.
> So you shouldn't be seeing the warnings in comment 0 any more. However we
> should turn the pref back to the more restrictive policy, which would show
> those warnings again, and likely also break the system app.
> 
> So what we need to do here is:
> 
> 1. Change the "security.apps.privileged.CSP.default" pref to
>    "default-src *; script-src 'self'; object-src 'none'; style-src 'self'
>     'unsafe-inline'"
> 
> 2. See what warnings start showing up.
> 

Jonas, I don't see any warning after putting this into custom-prefs.js because what you listed here is exactly the same permission we give in b2g.js!

https://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#363

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Vivien Nicolas (:vingtetun) from comment #10)
> Comment on attachment 669453 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 669453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/system/js/value_selector/value_selector.js
> @@ +128,5 @@
> >          }
> >          break;
> >  
> > +      case 'submit':
> > +        // Prevent the form from submit.
> 
> Does it miss a break or is it intentional?

That's intentional... Do you think this is bad coding style?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > So what we need to do here is:
> > 
> > 1. Change the "security.apps.privileged.CSP.default" pref to
> >    "default-src *; script-src 'self'; object-src 'none'; style-src 'self'
> >     'unsafe-inline'"
> > 
> > 2. See what warnings start showing up.
> > 
> 
> Jonas, I don't see any warning after putting this into custom-prefs.js
> because what you listed here is exactly the same permission we give in
> b2g.js!

Sorry, I meant the "security.apps.certified.CSP.default" pref!

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Jonas Sicking (:sicking) from comment #13)
> Sorry, I meant the "security.apps.certified.CSP.default" pref!

Right. I can verified that the patch does suppress the warnings. However switching this pref on doesn't prevent unpatched System app to start. There are a lot of warnings generated by other apps so we would need some other bugs to fix them too.

I am going to land the patch and close this bug.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

https://github.com/mozilla-b2g/gaia/commit/12a76c897aea24bdb3dee1cad0eb090bd3ffa2d2