Implement CSP devtool using GCLI

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

({dev-doc-complete})

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(8 attachments, 7 obsolete attachments)

741.32 KB, image/png
Details
427.12 KB, image/png
Details
117.90 KB, image/png
bram
: feedback+
Details
948 bytes, application/zip
Details
82.91 KB, image/png
Details
3.43 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
16.61 KB, patch
bholley
: review+
Details | Diff | Splinter Review
9.17 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Attachment #8577875 - Flags: review?(jgriffiths)
Attachment #8577876 - Flags: review?(sstamm)
Attachment #8577877 - Flags: review?(sstamm)
Attachment #8577877 - Flags: review?(jgriffiths)
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.
Flags: needinfo?(jwalker)
Attachment #8577877 - Flags: review?(jgriffiths) → review-
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.
Attachment #8577875 - Flags: review?(jgriffiths) → review-
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/
Blocks: 1143922
No longer blocks: 1143922
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?
Attachment #8577877 - Flags: review-
(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.
Depends on: 1128988
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
(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.
Flags: needinfo?(jwalker)
Ok, then I'll wait till the dependency for this bug cleared. Once bug 1128988 landed, i will reflag you for review.
Chris, do you still want me to review this or are you going to rewrite it first?
Flags: needinfo?(mozilla)
Flags: needinfo?(mozilla)
Attachment #8577877 - Flags: review?(sstamm)
(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 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?
Attachment #8577876 - Flags: review?(sstamm) → review-
Attachment #8577875 - Attachment is obsolete: true
Attachment #8577876 - Attachment is obsolete: true
Attachment #8577877 - Attachment is obsolete: true
Attachment #8601207 - Flags: review?(jwalker)
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 {}".
Attachment #8601208 - Flags: review?(sstamm)
It's using the updated syntax for e10s now. Let me know what you think!
Attachment #8601209 - Flags: review?(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?
Attachment #8601209 - Flags: review?(jwalker) → review+
> > +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.
Attachment #8601209 - Attachment is obsolete: true
Attachment #8601768 - Flags: review+
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?
Posted image 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.
Flags: needinfo?(jwalker)
Comment on attachment 8603405 [details]
csp_devtool_gcli.png

Hi Bram, how do you feel about the UX design?
Attachment #8603405 - Flags: feedback?(bram)
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.
Attachment #8601208 - Flags: superreview?(bobbyholley)
Attachment #8601208 - Flags: review?(sstamm)
Attachment #8601208 - Flags: review+
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?
Attachment #8601208 - Flags: superreview?(bobbyholley) → superreview-
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.
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?
(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.
Depends on: 1163254
Posted file 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.
(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.
Attachment #8603405 - Flags: feedback?(bram) → feedback+
Depends on: 1164292
No longer depends on: 1163254
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?
Attachment #8601207 - Flags: review?(jwalker) → review+
(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.
Flags: needinfo?(jwalker)
Thanks ntim for the new icons, going to use those now. Carrying over r+ from jwalker.
Attachment #8601207 - Attachment is obsolete: true
Attachment #8601208 - Attachment is obsolete: true
Attachment #8601768 - Attachment is obsolete: true
Attachment #8608231 - Flags: review+
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.
Attachment #8608232 - Flags: review?(bobbyholley)
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/
Attachment #8608232 - Flags: review?(bobbyholley) → review+
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.
Attachment #8608235 - Flags: review+
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)
Depends on: 1169722
Depends on: 1185072
Depends on: 1186308
Depends on: 1204414
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
(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!
You need to log in before you can comment on or make changes to this bug.