Closed Bug 1380755 Opened 7 years ago Closed 6 years ago

Support frame-ancestors in Content-Security-Policy-Report-Only


(Core :: DOM: Security, defect, P1)

55 Branch



Tracking Status
firefox58 --- fixed


(Reporter: bugzilla, Assigned: bugzilla)


(Keywords: dev-doc-complete, Whiteboard: [domsecurity-active])


(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170710085521

Steps to reproduce:

- Create a web page that contains a iframe containing another page
- Add a header to the iframed page: "Content-Security-Policy: frame-ancestors 'none'; report-uri /csp/report;"
- Load the parent page in Firefox, and observe the console and network log
  - Notice that an error is logged in the console, and a request is made to the URL specified by `report-uri`
- Change the name of the header on the iframed page to "Content-Security-Policy-Report-Only"
- Reload the parent page, and observe the console & network log

Actual results:

When the header is set to Report-Only:
- No errors are logged in the console
- No request is made to the URL specified by `report-uri`

Expected results:

When the header is set to Report-Only:
- Errors are logged in the console
- Request is made to the URL specified by `report-uri`
It should be noted that the MDN page for `frame-ancestors`[0] states:

> "This directive is not supported in the <meta> element or by the Content-Security-policy-Report-Only header field."

However reading the W3C spec[1] there is no mention of this. The closest I can find is about using CSP in <meta> elements[2], which states:

> "Note: The Content-Security-Policy-Report-Only header is not supported inside a meta element. Neither are the report-uri, frame-ancestors, and sandbox directives."

This leads me to believe that the MDN page was either referring to an older version of the spec, or the spec was misread. This is further backed up by the fact that Chrome acts on `frame-ancestors` when used in a `Content-Security-Policy-Report-Only` header.

Component: Untriaged → DOM: Security
Product: Firefox → Core
Attached patch bug-1380755.patch (obsolete) — Splinter Review
I figured I'd take a look, and after finding the offending code fairly quickly, I decided I'd submit a fix for this myself.

This change removes a check for `frame-ancestors` while in report-only mode, so instead of simply skipping it, it will instead perform the analysis & report violations.

I tested this with my [example web pages][0]; without the change, report-only mode does not report on `frame-ancestors` violations. With the change, `frame-ancestors` violations are reported when in report-only mode.

I've run CPP tests locally and there are no failures. I'm not sure if there are other tests that I should be running.

We ought to be reporting these violations in report-only mode, but we shouldn't prevent the child frame from loading in that case. We'll need to write tests to check in with the patch for this bug to prove it's working correctly.
Priority: -- → P3
Whiteboard: [domsecurity-backlog2]
(In reply to Jason Tarka from comment #2)
> I've run CPP tests locally and there are no failures. I'm not sure if there
> are other tests that I should be running.

Thanks for taking a look at this bug. In fact I agree, we could/should send report violations for frame-ancestors in report-only mode. I am willing to accept that change if you could write a test to make sure it's working correctly. You can find similar tests within dom/security/test/csp. You can run the whole suite using:
./mach test dom/security/test/csp

Thanks for working on this!
My C++ is rusty, and it'll take me a little while to get to it, but I'll take a look at the existing tests and see what I can do.
(In reply to Jason Tarka from comment #5)
> My C++ is rusty

You are already done with the C++ part :-)

What you would need is a mochitest (HTML + JS). Here is an example of how you would set that up using our testing infrastructure:
Attached patch bug1380755.patch (obsolete) — Splinter Review
It took longer to get to this than I'd hoped, but now the patch included a mochitest to verify that when a CSP violation occurs in report-only mode a report is submitted, and the frame content still loads.
Attachment #8887170 - Attachment is obsolete: true
Attachment #8899499 - Flags: review?(ckerschb)
Comment on attachment 8899499 [details] [diff] [review]

Review of attachment 8899499 [details] [diff] [review]:

Thanks for putting this together. There are really only a few nitpicks. Please address them before we can check those changes in.

::: dom/security/test/csp/file_bug1380755_child.html
@@ +4,5 @@
> +          href='/tests/dom/security/test/csp/file_CSP.sjs?testid=css_self&type=text/css' />
> +
> +  </head>
> +  <body>
> +    <img src="/tests/dom/security/test/csp/file_CSP.sjs?testid=img_self&type=img/png"> </img>

Please remove the external stylesheet as well as the external image loading. I think this can really be a dummy file like:

::: dom/security/test/csp/mochitest.ini
@@ +90,5 @@
>    file_bug941404.html
>    file_bug941404_xhr.html
>    file_bug941404_xhr.html^headers^
> +  file_bug1380755_child.html
> +  file_bug1380755_child.html^headers^

please rename to file_frame_ancestors_ro.html and file_frame_ancestors_ro.html^headers^. We are trying to get away from using bug numbers as file names.

@@ +247,4 @@
>  [test_bug910139.html]
>  [test_bug909029.html]
>  [test_bug1229639.html]
> +[test_bug1380755.html]

please rename to test_frame_ancestors_ro.html]

::: dom/security/test/csp/test_bug1380755.html
@@ +30,5 @@
> +
> +  is(cspReport["original-policy"], "frame-ancestors 'none'; report-uri http://mochi.test:8888/foo.sjs",
> +     "Incorrect original-policy");
> +     
> +  testResults.reportFired = true;

nit: please remove trailing whitespace, or even better, just remove all empty lines from within checkResults()

@@ +47,5 @@
> +      ok(false, "Could not parse JSON (exception: " + e + ")");
> +    }
> +    try {
> +      // test for the proper values in the report object
> +      checkResults(reportObj);

I would be fine with moving checkResults() within the above try-block to avoid the additonal try-catch; up to you though.
Attachment #8899499 - Flags: review?(ckerschb) → review+
Assignee: nobody → bugzilla
Ever confirmed: true
Priority: P3 → P1
Whiteboard: [domsecurity-backlog2] → [domsecurity-active]
Attached patch bug1380755.patchSplinter Review
- Renamed files
- Removed most of the content from the child file
- Removed extraneous whitespace & try...catch
- All CSP tests pass on my local build
Attachment #8899499 - Attachment is obsolete: true
Attachment #8915277 - Flags: review?(ckerschb)
Comment on attachment 8915277 [details] [diff] [review]

Review of attachment 8915277 [details] [diff] [review]:

Sorry this took so long, but I was out on PTO. But you already had my r+. Setting the checkin-need flag for you.
Attachment #8915277 - Flags: review?(ckerschb) → review+
No worries, and thanks for the feedback.

Since I don't have committer access to the main repos, is there anything I can/need to do from here?
(In reply to Jason Tarka from comment #11)
> Since I don't have committer access to the main repos, is there anything I
> can/need to do from here?

Nope, I'll make sure it gets into the codebase. Thanks for fixing!
Pushed by
Examine & report on frame-ancestors CSP in report-only mode. r=ckerschb
Keywords: checkin-needed
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
It looks like Jason has updated the actual ref page already - thanks for that!

I've just added a note to the Fx58 rel notes:
You need to log in before you can comment on or make changes to this bug.