(CSP 1.1) Implement form-action directive

RESOLVED FIXED in mozilla36

Status

()

defect
P3
normal
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: bsterne, Assigned: francois)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

Trunk
mozilla36
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [CSP 1.1], )

Attachments

(5 attachments, 30 obsolete attachments)

26.69 KB, patch
francois
: review+
Details | Diff | Splinter Review
1.72 KB, text/html
Details
9.28 KB, patch
francois
: review+
Details | Diff | Splinter Review
3.42 KB, patch
francois
: review+
Details | Diff | Splinter Review
5.54 KB, patch
francois
: review+
Details | Diff | Splinter Review
Reporter

Description

10 years ago
CSP needs to restrict the sites to which forms can be submitted.
Reporter

Updated

10 years ago
Assignee: nobody → bsterne
Depends on: 515437
Summary: (CSP) Implement form-action base restriction → (CSP) Implement form-action directive
Duplicate of this bug: 659023

Updated

6 years ago
Blocks: CSP
Whiteboard: [CSP 1.1]
Summary: (CSP) Implement form-action directive → (CSP 1.1) Implement form-action directive
Assignee: brandon → sstamm
Priority: -- → P3
Assignee

Updated

5 years ago
Assignee: sstamm → francois
Assignee

Comment 4

5 years ago
Assignee

Comment 5

5 years ago
I've attached my initial implementation here just to check whether or not I'm on the right track.

It's split logically in three pieces, but I assume I'd land this all at once in a single patch once it's finished and includes tests.
Assignee

Comment 6

5 years ago
Posted file Test page (obsolete) —
Here's the test page I used to verify that this is working correctly.
Assignee

Updated

5 years ago
Attachment #8506661 - Attachment description: Patch 1 of 3 → Patch 1 of 3 (marking the directive as unsupported)
Attachment #8506661 - Flags: feedback?(sstamm)
Assignee

Updated

5 years ago
Attachment #8506662 - Flags: feedback?(sstamm)
Assignee

Updated

5 years ago
Attachment #8506663 - Flags: feedback?(sstamm)
Comment on attachment 8506663 [details] [diff] [review]
Patch 3 of 3 (form submission)

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

I think you are right on track; we really need an additional layer where we can map types to directives though. Sid, what is the plan for frame_ancestors (currently mapped to TYPE_DOCUMENT) where we face the same problem?

::: content/html/content/src/HTMLFormElement.cpp
@@ +1641,5 @@
> +                         EmptyCString(), // mime type
> +                         nullptr,        // Extra parameter
> +                         &permitsFormAction);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (permitsFormAction != nsIContentPolicy::ACCEPT) {

Nit: please use !NS_CP_ACCEPTED(aDecision), see:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCSPService.cpp#304
Assignee

Comment 8

5 years ago
Fixed the nit mentioned in comment 8.
Attachment #8506663 - Attachment is obsolete: true
Attachment #8506663 - Flags: feedback?(sstamm)
Assignee

Updated

5 years ago
Attachment #8506662 - Attachment description: Patch 2 of 3 (support for hyperlink auditing) → Patch 2 of 4 (support for hyperlink auditing)
Attachment #8506662 - Flags: feedback?(sstamm)
Assignee

Updated

5 years ago
Attachment #8506661 - Attachment description: Patch 1 of 3 (marking the directive as unsupported) → Patch 1 of 4 (marking the directive as unsupported)
Attachment #8506661 - Flags: feedback?(sstamm)
Assignee

Comment 9

5 years ago
I think this is all that's needed to add a new IContentPolicy::TYPE.
Attachment #8507664 - Flags: feedback?(sstamm)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> I think you are right on track; we really need an additional layer where we
> can map types to directives though. Sid, what is the plan for
> frame_ancestors (currently mapped to TYPE_DOCUMENT) where we face the same
> problem?

Well, frame-ancestors is a different beast, kind of, because it's checking already loaded documents.  Really we're abusing the content types here to map backwards from CP type to CSP directive, and that doesn't take into account the "activity" (am I loading a resource, checking ancestors, etc).  It probably shouldn't use all the same logic as content loads and navigations.  It probably makes sense to add a new IContentPolicy type for form actions since we have one for beacon, ping, etc.

If I could boil the ocean, I'd separate the activity from the requested content type so that each request would have an activity/type pair; thus a ping would be ACTIVITY_PING+TYPE_NULL or something and image loads would be ACTIVITY_SUBRESOURCE+TYPE_IMAGE.  Then a form action would be ACTIVITY_FORM_SUBMIT+TYPE_DOCUMENT.

What we should probably do is make something alongside ShouldLoad that takes the CSP directive itself (CSP::PolicyAllows() or something) and then use that for things that aren't subresource loads.  In the cases where we already know the directive we want to query, we should do this instead of shoehorning things into a content policy and *then* into CSP.  

But here I've gone off on a rant. I can probably take care of this mapping repair in bug 999656.

Francois: I assume you'll get this feature done before bug 999656 lands.  Can you add a new method to nsIContentSecurityPolicy.idl, say permitsFormAction() that does the policy checking we need without having to do mapping stuff (and is much like permitsAncestry or permitsBaseURI)?  I can factor these things out later to agree with the new mapping stuff or a new PolicyAllows kind of function call.
Comment on attachment 8507664 [details] [diff] [review]
Patch 4 of 4 (add TYPE_FORM_ACTION)

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

I'm gonna clear the feedback flag here... I think we should do something in nsIContentSecurityPolicy instead of IContentPolicy for this new "activity" (form posts are not really a content type).  See my rant in comment 10.
Attachment #8507664 - Flags: feedback?(sstamm)
Assignee

Comment 12

5 years ago
Sid: is that what you had in mind?
Attachment #8507663 - Attachment is obsolete: true
Attachment #8507664 - Attachment is obsolete: true
Attachment #8510102 - Flags: feedback?(sstamm)
Assignee

Comment 13

5 years ago
Attachment #8506662 - Attachment is obsolete: true
Assignee

Comment 14

5 years ago
Attachment #8506661 - Attachment is obsolete: true
Comment on attachment 8510102 [details] [diff] [review]
Patch 3 of 3 (form submission)

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

This looks almost exactly like I had imagined--great!  I've only got a few minor high-level comments; please address them and then flag me for review when you think it's ready.

::: content/base/src/nsCSPContext.cpp
@@ +1107,5 @@
> +{
> +  // Can't perform check without aURI
> +  if (aURI == nullptr) {
> +    return NS_ERROR_FAILURE;
> +  }

You can use NS_ENSURE_ARG_POINTER here.

@@ +1126,5 @@
> +                                 i,             /* policy index        */
> +                                 EmptyString(), /* no observer subject */
> +                                 EmptyString(), /* no source file      */
> +                                 EmptyString(), /* no script sample    */
> +                                 0);            /* no line number      */

I'm noticing that there are a bunch of pieces of context missing.  Like we do for inline scripts and such, what if we decouple reporting and have the _caller_ of PermitsFormAction() generate context and send violation reports?  Or can we pass all the necessary context in here?  (This is obviously not required for a first iteration of this subfeature.)

::: content/base/src/nsCSPUtils.h
@@ +359,5 @@
>                                            nsAString& outDirective) const;
>  
>      void getDirectiveStringForBaseURI(nsAString& outDirective) const;
>  
> +    void getDirectiveStringForFormAction(nsAString& outDirective) const;

At this point, maybe it makes more sense to create a generic "getPolicyDirectiveAsString()" function for form-action, base-uri, etc.

::: docshell/base/nsDocShell.cpp
@@ +4815,3 @@
>          // CSP error
>          cssClass.AssignLiteral("neterror");
> +        error.AssignLiteral("cspFrameAncestorBlocked"); // TODO: rename to something generic?

Or better yet, create another string for NS_ERROR_CSP_FORM_ACTION_VIOLATION that says "CSP blocked submission of a form to X".
Attachment #8510102 - Flags: feedback?(sstamm) → feedback+
Comment on attachment 8510102 [details] [diff] [review]
Patch 3 of 3 (form submission)

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

::: content/base/src/nsCSPContext.cpp
@@ +1107,5 @@
> +{
> +  // Can't perform check without aURI
> +  if (aURI == nullptr) {
> +    return NS_ERROR_FAILURE;
> +  }

Not sure, but I think NS_ENSURE_ARG_POINTER only works for double pointers (outgoing arguments). To be compliant with the coding standard you could use if (!aURI), see; https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

::: content/html/content/src/HTMLFormElement.cpp
@@ +1635,5 @@
> +    bool permitsFormAction = true;
> +    rv = csp->PermitsFormAction(actionURL, &permitsFormAction);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    if (!permitsFormAction) {
> +      rv = NS_ERROR_CSP_FORM_ACTION_VIOLATION;

Drive by comment/question: Do we just need to assign to rv and still allow the assignment of the aActionURL further down or do we want to return the error right away?
Attachment #8510102 - Flags: feedback+
Assignee

Comment 17

5 years ago
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #15)
> ::: docshell/base/nsDocShell.cpp
> @@ +4815,3 @@
> >          // CSP error
> >          cssClass.AssignLiteral("neterror");
> > +        error.AssignLiteral("cspFrameAncestorBlocked"); // TODO: rename to something generic?
> 
> Or better yet, create another string for NS_ERROR_CSP_FORM_ACTION_VIOLATION
> that says "CSP blocked submission of a form to X".

cspFrameAncestorBlocked currently maps to this:

  <title>Blocked by Content Security Policy</title>
  <p>&brandShortName; prevented this page from loading in this way
  because the page has a content security policy that disallows it.</p>

which could work as a generic "CSP network error" message.

If I create a new form-action-specific message, it will need to be translated in:

  browser/locales/en-US/chrome/overrides/appstrings.properties
  browser/locales/en-US/chrome/overrides/netError.dtd
  mobile/locales/en-US/overrides/appstrings.properties
  mobile/locales/en-US/overrides/netError.dtd
  dom/locales/en-US/chrome/appstrings.properties
  dom/locales/en-US/chrome/netError.dtd
  b2g/locales/en-US/chrome/overrides/appstrings.properties
  webapprt/locales/en-US/webapprt/overrides/appstrings.properties

I'm happy to do it, but is it worth it?
Flags: needinfo?(sstamm)
Assignee

Comment 18

5 years ago
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #16)
> ::: content/html/content/src/HTMLFormElement.cpp
> @@ +1635,5 @@
> > +    bool permitsFormAction = true;
> > +    rv = csp->PermitsFormAction(actionURL, &permitsFormAction);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +    if (!permitsFormAction) {
> > +      rv = NS_ERROR_CSP_FORM_ACTION_VIOLATION;
> 
> Drive by comment/question: Do we just need to assign to rv and still allow
> the assignment of the aActionURL further down or do we want to return the
> error right away?

The check with the SecurityManager (https://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLFormElement.cpp?rev=9e7dff9e4404#1626) only assigns to rv and allows the assignment to aActionURL to happen so I'm guessing the CSP check should do the same?
Assignee

Comment 19

5 years ago
Posted file Test page (obsolete) —
I've updated the test page to include submitting forms via JavaScript and submitting a form that will be redirected.

The JavaScript submisions are not working properly right now because the form-action directive is looking at the content of the "action" attribute in the HTML source as opposed to when the form is actually submitted. I'm looking into this.

For the redirect case, I'd like to make sure that I understand the expected behavior correctly. Given a policy of "form-action 'http://here.example.com'":

1. form -> http://here.example.com/redirect -> http://elsewhere.example.com/submit
2. form -> http://elsewhere.example.com/redirect -> http://here.example.com/submit
3. form -> http://here.example.com/redirect -> http://here.example.com/submit

should only the last case be allowed?
Attachment #8506665 - Attachment is obsolete: true
Assignee

Comment 20

5 years ago
Posted patch Work in progress (obsolete) — Splinter Review
This patch addresses some of the points raised in comment 15 and comment 16 and includes all of the changes in a single patch.

The JavaScript/redirect issues are still outstanding.
Attachment #8510102 - Attachment is obsolete: true
Attachment #8510104 - Attachment is obsolete: true
Attachment #8510106 - Attachment is obsolete: true
(In reply to François Marier [:francois] from comment #17)
> cspFrameAncestorBlocked currently maps to this:
> 
>   <title>Blocked by Content Security Policy</title>
>   <p>&brandShortName; prevented this page from loading in this way
>   because the page has a content security policy that disallows it.</p>
> 
> which could work as a generic "CSP network error" message.

Yeah, looks fine for any required display of "blocked content would have been here" for things like form-action, child-src, etc.

> If I create a new form-action-specific message, it will need to be
> translated in:
[snip]
> I'm happy to do it, but is it worth it?

Nope.  Not worth it.  Bummer about the string naming, though.  Can you put a comment next to the definition(s) of the string to say that it is used for both frame ancestors and for form action (so it's easy to tell in the future if we change the string)?
Flags: needinfo?(sstamm)
Assignee

Comment 22

5 years ago
Posted patch WIP with tests (obsolete) — Splinter Review
This addresses comment 21 and adds mochitests for the cases that work (standard form submission and pings).
Attachment #8510840 - Attachment is obsolete: true
Assignee

Comment 23

5 years ago
Posted file Test page (obsolete) —
This new test page correctly tests for the JS-submitted forms (they work fine after all) and adds two new hyperlink auditing tests: redirected pings and imagemap pings.
Attachment #8510831 - Attachment is obsolete: true
Assignee

Comment 24

5 years ago
Posted patch For review, v1 (obsolete) — Splinter Review
This patch covers all cases except the redirected form submissions.
Attachment #8514771 - Attachment is obsolete: true
(In reply to François Marier [:francois] from comment #24)
> Created attachment 8515825 [details] [diff] [review]
> WIP with tests
> 
> This patch covers all cases except the redirected form submissions.

Talked to Sid about the redirect case, probably we have to add some kind of tag/information to the LoadInfo when we create the channel so that we know that we have to call ::PermitsFormAction() instead of ::permits() whenever a channel gets redirected [1]. I think you should try to get this landed and file a follow-up for the redirect case.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsCSPService.cpp#228
Assignee

Comment 26

5 years ago
Comment on attachment 8515825 [details] [diff] [review]
For review, v1

Sid, are you happy to review this patch?

If so, I'll file a follow-up bug to address the form submission redirects.
Attachment #8515825 - Attachment description: WIP with tests → For review, v1
Attachment #8515825 - Flags: review?(sstamm)
Assignee

Comment 27

5 years ago
Posted patch Patch 1 of 4 (CSP changes) (obsolete) — Splinter Review
Attachment #8515825 - Attachment is obsolete: true
Attachment #8515825 - Flags: review?(sstamm)
Attachment #8518641 - Flags: review?(sstamm)
Assignee

Comment 29

5 years ago
Attachment #8518644 - Flags: review?(sstamm)
Assignee

Comment 30

5 years ago
Posted patch Patch 4 of 4 (tests) (obsolete) — Splinter Review
Attachment #8518646 - Flags: review?(mozilla)
Comment on attachment 8518646 [details] [diff] [review]
Patch 4 of 4 (tests)

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

Looks really good to me. Also browsed over the other patches in this bug as well - I think that's it. Please push it to try and ask Sid for the final approval since he was the one who r- you initially.

::: dom/base/test/csp/mochitest.ini
@@ +122,5 @@
>  [test_CSP_bug909029.html]
>  [test_policyuri_regression_from_multipolicy.html]
>  [test_nonce_source.html]
>  [test_CSP_bug941404.html]
> +[test_form-action.html]

Have you had this on try yet?
I think you have to disable the test on b2g and e10s:
* skip-if = e10s || buildapp == 'b2g'
because if I remember correclty http-on-opening-request observers are not available in child processes.

::: dom/base/test/csp/test_form-action.html
@@ +96,5 @@
> +function loadNextTest() {
> +  counter++;
> +  if (counter == tests.length) {
> +    window.ConnectSrcExaminer.remove();
> +    SimpleTest.finish();

Just fixing that problem in Bug 1096396, please add a return statement here.
Attachment #8518646 - Flags: review?(mozilla) → review+
Comment on attachment 8518641 [details] [diff] [review]
Patch 1 of 4 (CSP changes)

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

r=me with the appropriate documentation.  I don't particularly like where the spec has put ping, but it is probably too late to get the spec fixed.

::: dom/base/nsCSPUtils.cpp
@@ +708,5 @@
>        return CSP_FRAME_ANCESTORS;
>  
> +    case nsIContentPolicy::TYPE_PING:
> +      return CSP_FORM_ACTION;
> +

This is counter-intuitive.  Please add a comment explaining why this is here (that pings are subject to the form-action directive).

According to the current draft spec, a ping is subject to form-action.  But sendBeacon is subject to connect-src.  This is probably something that needs to be fixed in the spec.  I disagree with how ping is classified, but maybe there's a good reason for it.

@@ +817,3 @@
>    // TODO: currently [frame-ancestors] is mapped to TYPE_DOCUMENT (needs to be fixed)
> +  if (aContentType == nsIContentPolicy::TYPE_DOCUMENT ||
> +      aContentType == nsIContentPolicy::TYPE_PING) {

Would it make sense to bypass permits() entirely in this case and have the general permits() call (which comes from ShouldLoad) pick the right specific one, like permitsFormAction, when the respective content type is presented?  Maybe not, but either way we should document in the code why "PING" is here.
Attachment #8518641 - Flags: review?(sstamm) → review+
Chris: do you know the history behind the classification of ping as a form-action and sendBeacon as a connect-src?  That seems weird to me.
Flags: needinfo?(mozilla)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #33)
> Chris: do you know the history behind the classification of ping as a
> form-action and sendBeacon as a connect-src?  That seems weird to me.

No, I can't recall. I guess Dan is the person who might know.
Flags: needinfo?(mozilla) → needinfo?(dveditz)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #34)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #33)
> > Chris: do you know the history behind the classification of ping as a
> > form-action and sendBeacon as a connect-src?  That seems weird to me.
> 
> No, I can't recall. I guess Dan is the person who might know.

See https://bugzilla.mozilla.org/show_bug.cgi?id=936340#c21 and associated w3c thread
Assignee

Comment 36

5 years ago
Posted patch Patch 1 of 4 (CSP changes) (obsolete) — Splinter Review
Adding documentation requested by Sid in comment 32. Carrying over his r+.
Attachment #8518641 - Attachment is obsolete: true
Attachment #8522668 - Flags: review+
Assignee

Comment 37

5 years ago
Rebased patch, no changes.
Attachment #8518643 - Attachment is obsolete: true
Assignee

Comment 38

5 years ago
Rebased patch, no changes.
Attachment #8518644 - Attachment is obsolete: true
Attachment #8518644 - Flags: review?(sstamm)
Assignee

Comment 39

5 years ago
Posted patch Patch 4 of 4 (tests) (obsolete) — Splinter Review
Fixing two small things requested by Christoph in comment 31. Carrying over his r+.
Attachment #8518646 - Attachment is obsolete: true
Attachment #8522672 - Flags: review+
Assignee

Comment 41

5 years ago
Posted patch Patch 1 of 4 (CSP changes) (obsolete) — Splinter Review
Fixing commit message only, keeping r+ from Sid.
Attachment #8522668 - Attachment is obsolete: true
Attachment #8522695 - Flags: review+
Assignee

Comment 42

5 years ago
Fixing commit message only.
Attachment #8522669 - Attachment is obsolete: true
Assignee

Comment 43

5 years ago
As recommended by Unfocused, I'm renaming this l10n string after making it generic, in case it needs to be updated for some languages.
Attachment #8522670 - Attachment is obsolete: true
Attachment #8522703 - Flags: review?(bmcbride)
Assignee

Comment 44

5 years ago
Posted patch Patch 4 of 4 (tests) (obsolete) — Splinter Review
Fixing commit message only, keeping r+ from Christoph.
Attachment #8522672 - Attachment is obsolete: true
Attachment #8522705 - Flags: review+
Assignee

Comment 45

5 years ago
Comment on attachment 8522698 [details] [diff] [review]
Patch 2 of 4 (DocShell and DOM changes)

Olli, is this something you'd be willing/able to review?
Attachment #8522698 - Flags: review?(bugs)
Attachment #8522703 - Flags: review?(bmcbride) → review+
Assignee

Comment 46

5 years ago
New try run to account for the changes requested by Unfocused: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=aff126273b18
Comment on attachment 8522698 [details] [diff] [review]
Patch 2 of 4 (DocShell and DOM changes)

> #define MODULE NS_ERROR_MODULE_SECURITY
>   /* Error code for CSP */
>   ERROR(NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION,     FAILURE(99)),
>+  ERROR(NS_ERROR_CSP_FORM_ACTION_VIOLATION,        FAILURE(99)),
This doesn't look right. Reusing the same value.
Use some other value.
Attachment #8522698 - Flags: review?(bugs) → review+
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #34)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #33)
> > Chris: do you know the history behind the classification of ping as a
> > form-action and sendBeacon as a connect-src?  That seems weird to me.
> 
> No, I can't recall. I guess Dan is the person who might know.

My reading of https://fetch.spec.whatwg.org/#requests shows them both being connect-src. The only form-action in that table is for forms. This makes sense to me, and since we want to make CSP built on fetch the two obviously need to agree. I'll raise this on the WG but for now why don't you assume Fetch is going to win and make them both connect-src.

The classification of ping is pretty arbitrary, but form-action (which didn't even exist in CSP 1.0) seems like an odd choice.
Flags: needinfo?(dveditz)
Oh, one reason form-action might make sense is because for backwards compatibility reasons it does not use default-src as a fallback when it's not specified. By sticking it under form-action the spec may be trying to make sure old pings don't go un-pung. CSP currently does not affect <a href> in any way so this makes ping sort of match that model.

You could make an equally convincing case that sendBeacon() is as much like a form post (minus the response) as it is an XHR.
(except that in Gecko we had <a ping> directly under default-src in our original implementation so not breaking pages because of default-src worries isn't a compelling argument for us)

WG thread starts here: http://lists.w3.org/Archives/Public/public-webappsec/2014Nov/0259.html
Assignee

Comment 51

5 years ago
Updating commit message, no code changes. Carrying over Unfocused's r+.
Attachment #8522703 - Attachment is obsolete: true
Attachment #8523398 - Flags: review+
Assignee

Comment 52

5 years ago
Fix the duplicate error code pointed out by Olli in comment 47.
Attachment #8522698 - Attachment is obsolete: true
Attachment #8523400 - Flags: review?(bugs)
Attachment #8523400 - Flags: review?(bugs) → review+
Assignee

Comment 53

5 years ago
Changing the error code from 100 to 98 as suggested by Sid and updating the commit message to note r+ from Olli.
Attachment #8523400 - Attachment is obsolete: true
Attachment #8523502 - Flags: review+
Assignee

Comment 54

5 years ago
Posted file Test page
Attachment #8515818 - Attachment is obsolete: true
Assignee

Comment 55

5 years ago
Posted patch Patch 1 of 4 (CSP changes) (obsolete) — Splinter Review
Removed PING related code as suggested by dveditz.
Attachment #8522695 - Attachment is obsolete: true
Attachment #8523587 - Flags: review+
Assignee

Comment 56

5 years ago
Posted patch Patch 4 of 4 (tests) (obsolete) — Splinter Review
Removed PING related tests as suggested by dveditz.
Attachment #8522705 - Attachment is obsolete: true
Attachment #8523589 - Flags: review+
Assignee

Comment 57

5 years ago
Comment on attachment 8523587 [details] [diff] [review]
Patch 1 of 4 (CSP changes)

Sid, can I get you to r+ my patch without the PING code?
Attachment #8523587 - Flags: review+ → review?(sstamm)
Assignee

Comment 58

5 years ago
Comment on attachment 8523589 [details] [diff] [review]
Patch 4 of 4 (tests)

Christoph, can I get you to r+ my patch without the PING tests?
Attachment #8523589 - Flags: review+ → review?(mozilla)
Comment on attachment 8523589 [details] [diff] [review]
Patch 4 of 4 (tests)

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

Yep, looks good to me.
Attachment #8523589 - Flags: review?(mozilla) → review+
Assignee

Comment 60

5 years ago
I have moved the PING code to bug 1100181.
Comment on attachment 8523587 [details] [diff] [review]
Patch 1 of 4 (CSP changes)

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

Looks good to me.

::: dom/security/nsCSPContext.cpp
@@ +1151,5 @@
> +                                 i,             /* policy index        */
> +                                 EmptyString(), /* no observer subject */
> +                                 EmptyString(), /* no source file      */
> +                                 EmptyString(), /* no script sample    */
> +                                 0);            /* no line number      */

Someday it would be nice to give more context to the violations.  Can you file a follow-up to do this?  (It's not priority one, but we should help out devs as much as we can if we have time.)
Attachment #8523587 - Flags: review?(sstamm) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Assignee

Comment 62

5 years ago
I filed bug 1100630 to improve the context info around CSP violations.
Assignee

Updated

5 years ago
Status: NEW → ASSIGNED
Hi, seems you have some commit messages in part 1,2,4 that are git style and cause problems when commited to hg. Could you clean this commit messages up in a little hg style ? thanks !
Assignee

Comment 64

5 years ago
Fixing patch metadata, no code changes.
Attachment #8523587 - Attachment is obsolete: true
Attachment #8524403 - Flags: review+
Assignee

Comment 65

5 years ago
Fixing patch metadata, no code changes.
Attachment #8523502 - Attachment is obsolete: true
Attachment #8524405 - Flags: review+
Assignee

Comment 66

5 years ago
Fixing patch metadata, no code changes.
Attachment #8523589 - Attachment is obsolete: true
Attachment #8524407 - Flags: review+
Assignee

Comment 67

5 years ago
Carsten, sorry for the badly formatted patches. I've recreated them all and they now all look like patch #3.
Thanks François!

I've completed the page with a spec and compat tables.
Also added a link in https://developer.mozilla.org/en-US/Firefox/Releases/36#Security
You need to log in before you can comment on or make changes to this bug.