1129999 - Implement CSP devtool using GCLI

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

What we have in mind is a devtool that supports 2 features:
* provide feedback about a CSP deployed with a page
* allow users to browse a page in 'recording' mode and suggest a CSP for that page

In the future we potentially could provide more such features including suggestions for SRI, CORS, MCB, etc.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: screenshot_4.png

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: screenshot_3.png

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_icons.patch

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_csp_to_json.patch

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_gcli.patch

Comment 6

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Comment on attachment 8577877 [details] [diff] [review]
bug_1129999_csp_devtool_gcli.patch

Declining review here - I think you should ask Joe for a devtools engineer to look at this.

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Comment on attachment 8577875 [details] [diff] [review]
bug_1129999_csp_devtool_icons.patch

Declining this one too, should get an eng to look at it.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Jeff, sorry I was not sure anymore whether you, or Jeff volunteered to review the changes here. Anyway, what I forgot to mention yesterday - I prepared a testpage that tests 2 policies at the same time (one of them is in report-only mode).
See: http://www.christophkerschbaumer.com/devtool/

Comment 9

[Joe Walker [:jwalker]]

Comment on attachment 8577877 [details] [diff] [review]
bug_1129999_csp_devtool_gcli.patch

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

I'm looking at the command-line parts of this. 2 thoughts.

1. We're about to land a significant change to GCLI for e10s. As a consequence all commands should be marked runAt:'server' (in which case they get access to the DOM) or runAt:'client' (in which case they can mess with the devtool toolbox). Since your command does context.environment.document, it looks like it should be runAt:'server'. Converters are always run in the client process.

2. One reason for the separation of model from presentation is e10s, but there is an obvious deeper reason. We're doing a fair amount of processing before we serialize the data to the converter, and I wondered if that processing was there to support the view rather than because the data was 'wrong' in some way before the processing. Does that make sense?

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #9)
> Comment on attachment 8577877 [details] [diff] [review]
> bug_1129999_csp_devtool_gcli.patch
> 
> Review of attachment 8577877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm looking at the command-line parts of this. 2 thoughts.
> 
> 1. We're about to land a significant change to GCLI for e10s. As a
> consequence all commands should be marked runAt:'server' (in which case they
> get access to the DOM) or runAt:'client' (in which case they can mess with
> the devtool toolbox). Since your command does context.environment.document,
> it looks like it should be runAt:'server'. Converters are always run in the
> client process.

It's great to see that you are fixing the problem with e10s. I was already experiencing that the document is not available in e10s. I will rewrite and flag you again - thanks for the update. Have you already rewritten some of GCLI to use runAt:'server' so I could take a look how it's done properly?

> 2. One reason for the separation of model from presentation is e10s, but
> there is an obvious deeper reason. We're doing a fair amount of processing
> before we serialize the data to the converter, and I wondered if that
> processing was there to support the view rather than because the data was
> 'wrong' in some way before the processing. Does that make sense?

I am not sure I can completely follow, but the e10s reason already convinces me to rewrite anyway.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Joe, just chatted with bgrins who patiently answered all my questions. It seems you are adding support for runAtServer at the moment in bug 1128988 (or also [1]). When is that going to land on mc? I would really like to get the devtool landed for 39. Any chance I could land on mc without runAtServer support for now and then file a Github pull request to change those few lines to use l10n, runAt, and such?

In general, is GCLI code always landing on github first and then merged into mc? Or can I also have my code landed on mc, right away?

[1] https://github.com/joewalker/gecko-dev/pull/10

Comment 12

[Joe Walker [:jwalker]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> Joe, just chatted with bgrins who patiently answered all my questions. It
> seems you are adding support for runAtServer at the moment in bug 1128988
> (or also [1]). When is that going to land on mc?

I'd like to get it done this week, maybe I will, but I'm not sure it would be a good idea to land a large change just before a merge, so I suspect it will have to wait for 40.

> I would really like to get
> the devtool landed for 39. Any chance I could land on mc without runAtServer
> support for now and then file a Github pull request to change those few
> lines to use l10n, runAt, and such?

Yes, that's fine

> In general, is GCLI code always landing on github first and then merged into
> mc? Or can I also have my code landed on mc, right away?

Generally that's true for things in /toolkit/devtools/gcli/source/...
But you're in /toolkit/devtools/gcli/commands/... so you're fine.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Ok, then I'll wait till the dependency for this bug cleared. Once bug 1128988 landed, i will reflag you for review.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Chris, do you still want me to review this or are you going to rewrite it first?

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #14)
> Chris, do you still want me to review this or are you going to rewrite it
> first?

Hey Sid, cleared the r? for the gcli patch, but it would be great if you could have a look at the JSON patch. It's debatable whether we want toJson or rather fall back to the ::toString() method already available. I personally think it might be nice to have a toJSON. Let me know how you feel about providing that functionality.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8577876 [details] [diff] [review]
bug_1129999_csp_devtool_csp_to_json.patch

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

r- for the interface comment below.  I'd rather make the JSON CSP value a readonly attribute of the principal than require callers to create an instance of nsIContentSecurityPolicy in order to extract a string from nsIPrincipal.

If you can find a better way that doesn't require making an instance of nsIContentSecurityPolicy, I'd be happy with that too.

::: dom/interfaces/security/nsIContentSecurityPolicy.idl
@@ +292,5 @@
> +   * Please note, that nsIPrincipal is not exposing
> +   * the CSP to JS. For security reasons we rather provide
> +   * that interface to query a CSP in JSON notation.
> +   */
> +  AString toJSONFromPrincipal(in nsIPrincipal aPrincipal);

I see what you're doing, but this feels awkward.  We have to create an instance of nsIContentSecurityPolicy in order to get the JSONified CSP out of aPrincipal...  

Why not just add a readonly attribute to nsIPrincipal called cspAsJSON?

::: dom/security/nsCSPContext.cpp
@@ +1187,5 @@
>    return NS_OK;
>  }
>  
> +NS_IMETHODIMP
> +nsCSPContext::ToJSON(nsAString& aCSPinJSON)

If this is an outparam, prefix it with "out" instead of "a".

@@ +1208,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsCSPContext::ToJSONFromPrincipal(nsIPrincipal* aPrincipal,
> +                                  nsAString& aCSPinJSON)

Same here, please prefix outparams with "out" as the rest of the file does.

@@ +1219,5 @@
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  aPrincipal->GetCsp(getter_AddRefs(csp));
> +
> +  if (!csp) {
> +    jsonPolicies.ToJSON(aCSPinJSON);

I don't think your jsonPolicies variable does anything, but maybe I'm reading it wrong.  Don't you just create the variable above, then call ToJSON on something that's empty?  Is there a better way to return an empty JSON string?

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_icons.patch

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_csp_to_json.patch

Hey Sid, I followed your suggestion and extended nsIPrincipal by a "cspJSON" attribute which we can query from JS.

> > +  if (!csp) {
> > +    jsonPolicies.ToJSON(aCSPinJSON);
> 
> I don't think your jsonPolicies variable does anything, but maybe I'm
> reading it wrong.  Don't you just create the variable above, then call
> ToJSON on something that's empty?  Is there a better way to return an empty
> JSON string?

That's the way to do it apparently :-) Btw, it's not a completely empty String, it's a JSON that looks something like "csp-policies {}".

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_gcli.patch

It's using the updated syntax for e10s now. Let me know what you think!

Comment 20

[Joe Walker [:jwalker]]

Comment on attachment 8601209 [details] [diff] [review]
bug_1129999_csp_devtool_gcli.patch

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

Looks good from a GCLI point of view

::: toolkit/devtools/gcli/commands/security.js
@@ +24,5 @@
> +const DIR_UNSAFE_INLINE = "'unsafe-inline'";
> +const DIR_UNSAFE_EVAL = "'unsafe-eval'";
> +const POLICY_REPORT_ONLY = "report-only"
> +
> +const WILDCARD_MSG = "can you remove the wildcard?";

I think these need l10n-ing, right?

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_gcli.patch

> > +const WILDCARD_MSG = "can you remove the wildcard?";
> 
> I think these need l10n-ing, right?

Yes, that is correct - thanks for catching. Updated! Carrying over r+ from jwalker.

Comment 22

[Joe Walker [:jwalker]]

Comment on attachment 8601207 [details] [diff] [review]
bug_1129999_csp_devtool_icons.patch

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

I'm not sure my skills as an artist are top notch, but I'll take a look. Could you make it easy and attach a screenshot please?

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: csp_devtool_gcli.png

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #22)
> Comment on attachment 8601207 [details] [diff] [review]
> bug_1129999_csp_devtool_icons.patch
> 
> Review of attachment 8601207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure my skills as an artist are top notch, but I'll take a look.
> Could you make it easy and attach a screenshot please?

Sure can. I think the graphics itself are fine (unless you think otherwise). I was more interested in the way they are integrated (bundled) into Firefox.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8603405 [details]
csp_devtool_gcli.png

Hi Bram, how do you feel about the UX design?

Comment 25

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8601208 [details] [diff] [review]
bug_1129999_csp_devtool_csp_to_json.patch

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

Looks ok.  r=me with a few naming repairs (please address before landing).  

In addition, you will need a superreview or double-check on the nsIPrincipal interface changes (from bholley, I think; it looks like he owns caps).  Flagging bholley...

::: caps/nsIPrincipal.idl
@@ +144,5 @@
>  
>      /**
> +     * The CSP of the principal in JSON notation.
> +     * Note, that the CSP itself is not exposed to JS, but JS
> +     * should be able to query the CSP in JSON.

Nit: "... but script should be able to obtain a JSON representation of the CSP."

::: caps/nsNullPrincipal.cpp
@@ +173,5 @@
> +  dom::CSPPolicies jsonPolicies;
> +
> +  if (!mCSP) {
> +    jsonPolicies.ToJSON(outCSPinJSON);
> +    return NS_OK;

I still think this is weird.  Can we do this once and store the constant for "no CSP policies" somewhere?

::: dom/security/nsCSPUtils.cpp
@@ +781,5 @@
>    }
>  }
>  
> +void
> +nsCSPDirective::toJSON(mozilla::dom::CSP& outCSP) const

See my note in the .h: this isn't creating a JSON blob, it's creating a dom::CSP struct thing.  Needs renaming.

@@ +1038,5 @@
>  }
>  
> +void
> +nsCSPPolicy::toJSON(mozilla::dom::CSP& outCSP) const
> +{

Same note here about the name.

::: dom/security/nsCSPUtils.h
@@ +290,5 @@
>      bool permits(nsIURI* aUri, const nsAString& aNonce, bool aWasRedirected) const;
>      bool permits(nsIURI* aUri) const;
>      bool allows(enum CSPKeyword aKeyword, const nsAString& aHashOrNonce) const;
>      void toString(nsAString& outStr) const;
> +    void toJSON(mozilla::dom::CSP& outCSP) const;

This doesn't convert the policy into JSON, it converts it into a webidl "object," right?  Please rename this toDomCSPStruct or something that describes what it's doing more accurately.  Or make it create a JSON string and not a dom::CSP struct.

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 8601208 [details] [diff] [review]
bug_1129999_csp_devtool_csp_to_json.patch

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

I'm not wild about the code duplication here. Can you make an abstract class called mozilla::AbstractPrincipal that sits between all these principal implementations and nsIPrincipal, and implement this there?

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 8601208 [details] [diff] [review]
bug_1129999_csp_devtool_csp_to_json.patch

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

::: dom/webidl/CSPInfo.webidl
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> +  * Dictionary used to display CSP info.

Also, I think we should call this file CSPDictionaries, given the lack of any actual interfaces here.

Comment 28

[Bobby Holley (:bholley)]

I'm about to refactor CAPS in bug 1163254, and could add AbstractPrincipal while I'm at it. Those changes shouldn't take long, just a day or two. Do you want to take care of it in this bug, or would you prefer that I do it?

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Bobby Holley (:bholley) from comment #28)
> I'm about to refactor CAPS in bug 1163254, and could add AbstractPrincipal
> while I'm at it. Those changes shouldn't take long, just a day or two. Do
> you want to take care of it in this bug, or would you prefer that I do it?

Since you are already on it :-) Thanks for the review and also taking on the work.
Adding dependency to bug 1163254.

Comment 30

[Tim Nguyen :ntim]

Attachment: sec_icons.zip

Here are some SVG icons that fit better with the devtools UI. Let me know if you'd like to see some tweaks.

Comment 31

[Bram Pitoyo [:bram]]

Attachment: CSP panel - text alignment

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #24)
> Hi Bram, how do you feel about the UX design?

The interface looks alright. I do have four feedbacks about it:

1. The CSP panel should sit lower than the toolbar and be no higher than the browser’s content area. Either make it as tall as content, or have it sit 20–30px lower.

2. Panel shouldn’t scroll horizontally. If the content doesn’t fit, either move it to the next line or increase the panel width.

3. Text inside panel should have baselines that align. See attachment.

4. Following DevTools theme, the console background color should be #ffffff on light theme, and #252c33 on dark. The panel background color should be #fcfcfc on light, and #14171a on dark.

Comment 32

[Joe Walker [:jwalker]]

Comment on attachment 8601207 [details] [diff] [review]
bug_1129999_csp_devtool_icons.patch

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

I'm not sure my skills as an artist are top notch, but I'll take a look. Could you make it easy and attach a screenshot please?

Comment 33

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] (overloaded - needinfo me or ping on irc) from comment #32)
> Comment on attachment 8601207 [details] [diff] [review]
> bug_1129999_csp_devtool_icons.patch
> 
> Review of attachment 8601207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure my skills as an artist are top notch, but I'll take a look.
> Could you make it easy and attach a screenshot please?

For some reason bugzilla brought that comment back from when I submitted comment 22. Please ignore.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_icons.patch

Thanks ntim for the new icons, going to use those now. Carrying over r+ from jwalker.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_csp_to_json.patch

Hey Bobby, thanks for adding the BasePrincipal; I rebased the patches and incorporated your suggestions. Also incorporated Sid's suggestions - should be ready to go now.

Comment 36

[Bobby Holley (:bholley)]

Comment on attachment 8608232 [details] [diff] [review]
bug_1129999_csp_devtool_csp_to_json.patch

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

r=me on the stuff in caps/

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1129999_csp_devtool_gcli.patch

I think I am going to land this as is and then we file a follow up; maybe Bram, or Ntim can have a look and make this more pretty if needed. I think it's fine as is given that we have a lot of other GCLI stuff that would need some polishing as well.

Carrying over r+ from jwalker.

Comment 38

[Tim Nguyen :ntim]

Comment on attachment 8608231 [details] [diff] [review]
bug_1129999_csp_devtool_icons.patch

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

::: browser/base/content/gcli_sec_bad.svg
@@ +1,4 @@
> +<svg width="30" height="30" xmlns="http://www.w3.org/2000/svg">
> +  <circle cx="15" cy="15" r="15" fill="#e74c3c"/>
> +  <g stroke="white" stroke-width="3">
> +  	<line x1="9" y1="9" x2="21" y2="21"/>

nit : this is tab indented (should be 2 spaces)

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5924130fdaa5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d157cf63a5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6983ac38193

Comment 40

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/5924130fdaa5
https://hg.mozilla.org/mozilla-central/rev/4d157cf63a5e
https://hg.mozilla.org/mozilla-central/rev/c6983ac38193

Comment 41

[Sebastian Zartner [:sebo]]

Added general info about the 'security' command to https://developer.mozilla.org/en-US/docs/Tools/GCLI, more detailed info at https://developer.mozilla.org/en-US/docs/Tools/GCLI/Display_security_information and linked to it from https://developer.mozilla.org/en-US/Firefox/Releases/41.

Christoph, please note that I made screenshots of your example page to explain the feature. Please let me know in case I should use another page.

Sebastian

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Sebastian Zartner [:sebo] from comment #41)
> Added general info about the 'security' command to
> https://developer.mozilla.org/en-US/docs/Tools/GCLI, more detailed info at
> https://developer.mozilla.org/en-US/docs/Tools/GCLI/
> Display_security_information and linked to it from
> https://developer.mozilla.org/en-US/Firefox/Releases/41.

Great - thanks!