CSP frame-ancestors test over-succeeds

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: geekboy, Assigned: geekboy)

Tracking

(Blocks 1 bug)

Trunk
mozilla33
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

content/base/test/csp/test_CSP_frame-ancestors.html passes 18 times despite only having 10 things to test.  This means duplicates and may indicate we're counting successes wrong, and missing a test that doesn't run.

We should fix the test to pass iff all things allowed load and all individual things disallowed are blocked.
This is possibly an issue with frame-ancestors looking at the test harness, not the virtual root for the purpose of the test.  (There's an additional ancestor when running the test as part of the suite and not alone).

We can fix this by skipping the checks on the test_CSP_frameancestors.html file:

  examiner.prototype  = {
    observe: function(subject, topic, data) {
      // subject should be an nsURI, and should be either allowed or blocked.
      if (!SpecialPowers.can_QI(subject))
        return;
  
 +    var asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIURI"), "asciiSpec");
 +
 +    // skip checks on the test harness
 +    if (asciiSpec.contains("test_CSP_frameancestors.html")) {
 +      return;
 +    }
 +
 +
      if (topic === "csp-on-violate-policy") {
        //these were blocked... record that they were blocked
 -      var asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject,  "nsIURI"), "asciiSpec");
        window.frameBlocked(asciiSpec, data);
      }
    },
The CSP spec made this a little harder (can't send violating URIs cross-origin for frame-ancestors violations, see bug 846978), but I put a lot of comments in the file explaining how to calculate the expected violations.

Garrett: does this change make sense?  The test over-succeeds still, but in a predictable fashion with the right number of test successes.
Assignee: nobody → sstamm
Status: NEW → ASSIGNED
Attachment #8446762 - Flags: review?(grobinson)
Comment on attachment 8446762 [details] [diff] [review]
frameancestor-test-over-succeeds

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

::: content/base/test/csp/test_CSP_frameancestors.html
@@ +77,5 @@
> +        return;
> +      }
> +    } catch (ex) {
> +      // was not an nsIURI, so it was probably a cross-origin report.
> +      asciiSpec = subject;

Nit: you don't have to assign asciiSpec twice - it already happens before the try. you could leave the catch empty :-)
Blocks: 1031658
Comment on attachment 8446762 [details] [diff] [review]
frameancestor-test-over-succeeds

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

r=me with the assignment issue fixed (either my or ckerschb's way). Thanks for taking the time to investigate, and comment the specific exceptions due to the cross-origin reporting limitation!

::: content/base/test/csp/test_CSP_frameancestors.html
@@ +77,5 @@
> +        return;
> +      }
> +    } catch (ex) {
> +      // was not an nsIURI, so it was probably a cross-origin report.
> +      asciiSpec = subject;

Or remove the prior assignment, since it's equivalent to this unique branch. Definitely leave the comment either way.
Attachment #8446762 - Flags: review?(grobinson) → review+
Posted patch fixSplinter Review
Fixed assignment, carried over r=grobinson.  Thanks guys!
Attachment #8446762 - Attachment is obsolete: true
Attachment #8449731 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9619b783fc
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/fb9619b783fc
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.