991972 - CSP frame-ancestors test over-succeeds

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 1

[Sid Stamm [:geekboy or :sstamm]]

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);
      }
    },

Comment 2

[Sid Stamm [:geekboy or :sstamm]]

Attachment: frameancestor-test-over-succeeds

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.

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

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 :-)

Comment 4

[Garrett Robinson [:grobinson]]

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.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Attachment: fix

Fixed assignment, carried over r=grobinson.  Thanks guys!

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9619b783fc

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fb9619b783fc