When a page has CSP enabled and it is requested by a site that will put it in a frame, the policy's "frame-ancestors" directive must be satisfied before it is loaded as a sub-document. Initial patch is attached.

This version is updated to check for the CSP instance in the document's principal (not in nsDocument) as the rest of the v7 patches are written.

Added mochitests (10) for checking efficacy of the frame-ancestors restriction. Also tweaked contentSecurityPolicy.js component to properly report violations via the observer service.

Fixed mochitests to play nice with the main test harness (subframes were using window.top instead of finding the appropriate intermediate frame to alert when they're loaded).

Includes some bug fixes and modifications based on brief, informal reviews. (Also had pretty nasty bit rot.)

Comment on attachment 420199 [details] [diff] [review] CSP Frame Ancestor Check (v7.4) This is a pretty short patch -- most of it is unit tests. jst, would you take a peek?

Comment on attachment 420199 [details] [diff] [review] CSP Frame Ancestor Check (v7.4) - In nsDocShell::DisplayLoadError(): + else if (NS_ERROR_CSP_FRAME_ANCESTOR_VIOLATION == aError) { + // CSP error + error.AssignLiteral("forbiddenFrameAncestor"); + cssClass.AssignLiteral("neterror"); + // TODO: localize this, or at least put the message into a resource + messageStr.AssignLiteral("CSP Blocked loading of this page because it doesn't trust the page embedding it."); Is there a bug on file to localize this? r=jst assuming that bug exists and is dependent on this bug.

Moved the strings into localizable .properties and .dtd files. Carrying over jst's review, since that's all I changed and johnath wanted to peek at it anyway. johnath: would you do a UI review on the strings and the use of neterror?

Comment on attachment 423083 [details] [diff] [review] CSP Frame Ancestor Check (v7.5) >diff -r d660f235e5ad browser/locales/en-US/chrome/overrides/appstrings.properties >+cspFrameAncestorBlocked=This page did not load because it doesn't trust the page embedding it. We generally don't anthropomorphize web pages like this. :) I think we want to talk about the policy error, not the trust of the page. Maybe something like: This page cannot be loaded because it has a security policy which prevents embedding in this way. or This page has a content security policy which prevents it from being embedded in this way. Still not something a user is going to understand, but maybe more explicit about the cause? (Ditto for the other .properties file with identical text) >diff -r d660f235e5ad browser/locales/en-US/chrome/overrides/netError.dtd >+<!ENTITY cspFrameAncestorBlocked.title "CSP Blocked This Load"> We shouldn't use unspecified acronyms in our error messages. Something like: "Blocked by Security Policy" or "Blocked by Content Security Policy" >+<!ENTITY cspFrameAncestorBlocked.longDesc "<p>CSP blocked this load because the protected page is embedded on another page in a way not allowed by this page's policy.</p>"> Likewise, let's avoid the acronym. When we need to refer to an agent doing a thing in these messages, we typically use "Firefox", so maybe: "&brandShortName; prevented this page from loading in this way because the page has a content security policy which prevents it." (And again, ditto with the other versions of these strings). ui-r- but it should be a quick + with the changes - I mostly just want another crack at them. :) Thanks Sid - really excited to see this stuff landing soon.

here's another whack, johnath.

Comment on attachment 423094 [details] [diff] [review] CSP Frame Ancestor Check (v7.5 with revised strings) >diff -r d660f235e5ad browser/locales/en-US/chrome/overrides/appstrings.properties >+cspFrameAncestorBlocked=This page has a content security policy that prevents it from being embedded in this way. >diff -r d660f235e5ad browser/locales/en-US/chrome/overrides/netError.dtd >+<!ENTITY cspFrameAncestorBlocked.title "Blocked by Content Security Policy"> >+<!ENTITY cspFrameAncestorBlocked.longDesc "<p>&brandShortName; prevented this page from loading in this way because the page has a content security policy that disallows it.</p>"> These look good - thanks, Sid. However: >diff -r d660f235e5ad dom/locales/en-US/chrome/netError.dtd >--- a/dom/locales/en-US/chrome/netError.dtd Tue Jan 05 13:08:54 2010 -0800 >+++ b/dom/locales/en-US/chrome/netError.dtd Fri Jan 22 16:01:12 2010 -0800 >@@ -75,13 +75,16 @@ > "> > > <!ENTITY phishingBlocked.title "Suspected Web Forgery!"> > <!ENTITY phishingBlocked.longDesc " > <p>Entering any personal information on this page may result in identity theft or other fraud.</p> > <p>These types of web forgeries are used in scams known as phishing attacks, in which fraudulent web pages and emails are used to imitate sources you may trust.</p> > "> > >+<!ENTITY cspFrameAncestorBlocked.title "Blocked by Content Security Policy"> >+<!ENTITY cspFrameAncestorBlocked.longDesc "<p>&brandShortName; prevented this page from loading in this way because the page has a content security policy that disallows it.</p>"> I think that the dom/ version of this DTD doesn't source brand.dtd, and hence can't use &brandShortName; which is a bit insidious - this would pass all tests in Firefox, but give xhtml parse errors on other gecko consumers. IIRC, the dom/ version just uses "The browser" everywhere instead. ui-r=me with that fixed to match whatever the prevailing style is in that file.

Thanks johnath, updated and ready to land.