515433 - (CSP) Implement core CSP elements

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P1)

Description

[Sid Stamm [:geekboy or :sstamm]]

Attachment: Core CSP modules

In splitting CSP into manageable pieces, this is the core of CSP upon which all other parts will depend.  It includes the parser and model data structure for policy representation.  This core piece shouldn't affect the operation of other Mozilla code, but simply provide structures and interfaces for CSP.

Initial patch (including some xpcshell unit tests) is attached.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Core Modules (v7)

Core modules for version 7 of CSP patch.  Updated in this version:
- all violation reports are triggered inside the contentSecurityPolicy.js XPCOM component (not from outside)

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Core Modules (v7.1)

Fixed a problem where directives would think they're implicit (not stated in the HTTP header, but rather calculated) when they really aren't.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Core Modules (v7.2)

Misc. bug fixes in this version.  Brought in support for the observer topic.

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Core Modules (v7.3)

Fixed some dotless hosts parsing issues and basics.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 407903 [details] [diff] [review]
CSP Core Modules (v7.3)

Some comments:

* I'm not well versed in JS style, but I prefer overbraced if statements and spaces after 'if' and 'while'.

* On the same vain spaces after open parens and before close parens are generally eschewed.

* There are a few uses of == when === would work.

* You use 'with' for (as far as I can tell) two identifiers. Please spell out the common identifier.

* In CSPSource.create, the aData instanceof CSPSource case doesn't return a source (it clones one and then returns null).

* strings have a .trim function, no need for the two .replace calls with regexps.

Comment 6

[Jeff Walden [:Waldo]]

(In reply to comment #5)
> * I'm not well versed in JS style, but I prefer overbraced if statements and
> spaces after 'if' and 'while'.

I don't think this is a common JS style.  I'm trying to think of idiomatic code I've seen that does this, and I can only come up with C++, not with JS, which is perhaps notable because that JS isn't limited to just Mozilla's JS.


> * You use 'with' for (as far as I can tell) two identifiers. Please spell out
> the common identifier.

Adding a little more detail, |with| causes any function which contains it to be deoptimized, because name lookups inside the body of the |with| can't be fast-pathed as accessing arguments, local variables, global variables, etc.  Abbreviation doesn't have this characteristic.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> (In reply to comment #5)
> > * I'm not well versed in JS style, but I prefer overbraced if statements and
> > spaces after 'if' and 'while'.
> 
> I don't think this is a common JS style.

Er, I referred only to overbracing there.  |if (foo)| and |while (foo)| and |for (var i in o)| and |for (var i = 0; i < n; i++)| and |do { } while (foo)| all have spaces between keyword and parenthesis in the most common Mozilla JS style.  (I'm not sure if I've seen the cramped-spacing style in Mozilla JS, to be honest.)

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Core Modules (v7.4)

> * I'm not well versed in JS style, but I prefer overbraced 
> if statements and spaces after 'if' and 'while'.
> * On the same vain spaces after open parens and before 
> close parens are generally eschewed.

I updated the bracing to be more uniform in the files and also fixed spacing after keywords and within parens.

> * You use 'with' for (as far as I can tell) two identifiers. 
> Please spell out the common identifier.

Not sure why I was using with() ... un-with()'ed it.

> * In CSPSource.create, the aData instanceof CSPSource case 
> doesn't return a source (it clones one and then returns null).

Good catch.  I must have stopped working on that mid-thought.  Fixed.

> * strings have a .trim function, no need for the two 
> .replace calls with regexps.

I think I got them all.  This is new in 1.9.0, right?  The version of smjs I have installed on my desktop must be the old one, for it doesn't know about trim().

I fixed a couple of minor bugs in this patch update including:
* Reports were not being sent out because of a parsing issue (well, intersecting issue that ignored the report-uri directive).  That's fixed now.
* ports are inherited differently (minor logical change in the parser for source expressions).

I requested sr=?mrbkap because of the IDL.  I don't think this new interface will change any existing stuff, but I want to make sure it's looked at carefully.  Thanks for the review!

Comment 9

[Brandon Sterne (:bsterne)]

Comment on attachment 419332 [details] [diff] [review]
CSP Core Modules (v7.4)

Flipping the superreview over to jst since mrbkap did the initial review.

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

Attachment: core 7.4 adjustment

This tiny patch should be added on top of csp-core v7.4 to fix some major report bustage that becomes evident in later mochitests.

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

> Created an attachment (id=420376) [details]
> core 7.4 adjustment

One of the hunks (js/src/jsobj.cpp) will fail in that patch, just ignore the failed hunk, it's not important for this change anyway.  (That's what I get for hastily uploading a patch!)

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

Looks like the adjustment patch missed a small bug:

content/base/src/contentSecurityPolicy.js:268

  "   <blocked-uri>" + blockedUri + "</blocked-uri>\n" +

should check the type of blockedUri first (could be string or nsIURI):

  "   <blocked-uri>" + (typeof blockedUri === "nsIURI" ?
                        blockedUri.asciiSpec :
                        blockedUri) + "</blocked-uri>\n" +

blockedUri is a string when the URI is "self", such as when a base restriction is violated.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 419332 [details] [diff] [review]
CSP Core Modules (v7.4)

Looks good!

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Core Modules (v7.5)

Minor tweaks as requested by reviewers, and ready to land. Carried over the r+ and sr+ from mrbkap and jst.

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Attachment: CSP Core Modules (v7.6) [Checkin: Comment 17]

Moved IContentSecurityPolicy into the mozilla namespace (nsIContentSecurityPolicy).

https://bugzilla.mozilla.org/show_bug.cgi?id=515437#c14

Also properly unwrap "blockedUri" when it's an nsIURI in sendReports (was using typeof and === instead of instanceof, and the type comparison wasn't working right).

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 422647 [details] [diff] [review]
CSP Core Modules (v7.6)
[Checkin: Comment 17]

r=dveditz on the incremental change

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 422647 [details] [diff] [review]
CSP Core Modules (v7.6)
[Checkin: Comment 17]


http://hg.mozilla.org/mozilla-central/rev/c551e8a012d7

Comment 18

[Daniel Veditz [:dveditz]]

Checked-in yesterday