Imagine a.com has a form that posts to b.com that we want CSP to block (CSP 1.1 has this feature currently). Alternatively, a.com has a link to b.com that we want to block (CSP might consider in the future). Currently, the NS_CheckContentLoadPolicy calls from nsdocshell do not send the principal that caused the request (in this case a.com). Since the CSP is attached to the principal, it is not possible for CSPService to know that there is a CSP for this request. I suggest using the "extra" parameter in the call to attach the principal causing the request, so that CSP can use it.

Or we could just actually start passing the request principal along as part of the normal content policy API... Pretty sure we have bugs on that. The point of aExtra is that it's an extension mechanism that we promise to never use ourselves, so we can't use it here.

Modified to just add a new argument to ContentPolicy, as discussed with bz. This patch is only for making changes to the IDL, haven't modified CSP to make use of this, nor have I modified the individual NS_CheckContentPolicy calls to pass a relevant argument. Should those be on this bug or a new bug ?

Comment on attachment 635594 [details] [diff] [review] Add a requestPrincipal to nsIcontentpolicy NS_CheckContentLoadPolicy already had a originPrincipal argument. No need to add a new argument to that method. Same for NS_CheckContentProcessPolicy. Please follow local argument-naming style (aRequestPrincipal in various places where that's the style). Missing space before "decision" in nsContentPolicy::CheckPolicy. For nsContentPolicy::CheckPolicy, please pass the requestPrincipal as the arg _before_ decision, since decision is the out param. Missing space after ',' in the NS_STDCALL_FUNCPROTO gunk in nsContentPolicy.h I think you should just change loadingPrincipal in the docshell code to be the QI from aOwner if that's not null. If it's null, then fall back to what it does right now. Please don't name locals with names starting with "a": those indicate arguments in that code. Please fix the indent in nsContentBlocker::ShouldLoad. r=me with those nits (and the one substantive change to loadingPrincipal) picked.

Thanks for the quick review. I incorporated all your changes. The spacing in nsContentBlocker::ShouldLoad was already broken, and I tried fixing it, but it still looks weird in the diff (the actual file looks all right in vim).

> but it still looks weird in the diff That's because there were some stray tabs in the file. Just replace tab with 8 spaces globally on it? ;)

Comment on attachment 635796 [details] [diff] [review] Add a requestPrincipal to nsIcontentpolicy >+++ b/docshell/base/nsDocShell.cpp >+ if(ownerPrincipal){ Space before '(', please. Also, probably better to do this earlier like so: nsCOMPtr<nsIPrincipal> loadingPrincipal = do_QueryInterface(aOwner); and do the various loadingPrincipal stuff we do right now only if the result is null. r=me

thanks. Fixed.

Comment on attachment 635874 [details] [diff] [review] Add requestPrincipal to nsIContentPolicy r=me

Made the argument optional since it seems calling this code is actually pretty common. Anyone know how I can carry over the r+ from bz?

Also, here's a link to the Try https://tbpl.mozilla.org/?tree=Try&rev=9a046282d0e6

Comment on attachment 637333 [details] [diff] [review] Add requestPrincipal to nsIContentPolicy Carrying over the previous r=bz.

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/2777ec78a2b5 For future patches, dev, please make sure your user is set in HG and the bug number is in the summary for the patch file.

FYI this commit "breaks" the RequestPolicy add-on. When you (manually) enter a new URL on a existing page RequestPolicy always prompts "This webpage has asked to redirect to (new URL) Allow, Deny, More...". See: https://github.com/RequestPolicy/requestpolicy/issues/318

Weird, the requestPolicy code uses the JS interface to nsIContentPolicy, which hasn't changed at all. I am going to wait for Justin to comment on the bug over there.

This patch "fixes" the issue: diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index c497410..9760c22 100644 --- a/docshell/base/nsDocShell.cpp +++ b/docshell/base/nsDocShell.cpp @@ -8162,8 +8162,8 @@ nsDocShell::InternalLoad(nsIURI * aURI, } // XXXbz would be nice to know the loading principal here... but we don't - nsCOMPtr<nsIPrincipal> loadingPrincipal = do_QueryInterface(aOwner); - if (!loadingPrincipal && aReferrer) { + nsCOMPtr<nsIPrincipal> loadingPrincipal; + if (aReferrer) { nsCOMPtr<nsIScriptSecurityManager> secMan = do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv); NS_ENSURE_SUCCESS(rv, rv);

> which hasn't changed at all. Well, it has. We now send a different principal for pageloads, no? That will affect the aRequestOrigin in the JS API, to the extent that we have an owner and its URI does not match the aReferrer value... It sounds like we used to send an empty referrer for loads from the URL bar but the principal for those loads is the currently-loaded page? Have you tried to look at the callstack to InternalLoad with the steps to reproduce and see what aOwner is and where it comes from? It sounds like we need to get a bug filed on this, blocking this one, by the way.

There was a previous version of the patch that modified NS_CheckContentPolicy (adding 1 more argument) but Comment 4 suggested that we just reuse the existing argument. How about we go back to adding 1 more argument to the NS_CheckContentPolicy ? That should fix this, right?

Well, yes, at the cost of making most consumers of NS_Check* more complicated.... We could make the one more argument optional (defaulting to null) and when it's null use the existing principal argument. Then have only docshell pass the optional argument. But this case: > Alternatively, a.com has a link to b.com that we want to block (CSP might consider in > the future). would then break for loads from the browser UI, no? Because they would look like they're coming from a.com. So it might be better to really figure out what's going on here. And again, it's best to do it in a separate bug.