Boris Zbarsky [:bzbarsky] Assignee
	Description • 16 years ago

There's cookie permission code that depends on getInterface of docshell interfaces from the loadgroup.  Ideally, this code would just have an interface it could request and interrogate that could be implemented even by non-docshell callbacks (e.g. the ones being added in bug 433616).  The current setup means that I had to expose docshell interfaces on the loadgroup for external resources, which is highly suboptimal.  Requesting blocking because I'd really like to remove those interfaces there for 1.9.1 for security reasons, and that will break cookies in external resource documents.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 16 years ago

This introduces nsILoadContext and uses it in cookie code.  It also changes external resource docs to not ever hand out direct docshell pointers and instead hand back shims for interfaces that docshell implements directly.  Consumers who want something docshell-like should be using nsILoadContext.

dwitte, if you could review the cookie changes that would rock.  I couldn't find any 3rd-party cookie tests; if there are none, would you mind putting some together?  I think if external references allowed cross-site loads (which they don't yet) we could have a testcase that fails with the code as it used to be... ;)

	dwitte@gmail.com
	Comment 2 • 16 years ago

Comment on attachment 348818 [details] [diff] [review]
Proposed fix

>-       // XXXbz evil hack for cookies for now

yay!

>diff --git a/extensions/cookie/nsCookiePermission.cpp > #ifdef MOZ_MAIL_NEWS

> // returns PR_TRUE if URI appears to be the URI of a mailnews protocol
>+// XXXbz this should be a protocol flag, not a scheme list, dammit!
> static PRBool
> IsFromMailNews(nsIURI *aURI)

thanks, i'll file a followup on that...

>@@ -345,18 +342,23 @@ nsCookiePermission::CanSetCookie(nsIURI 
>-      if (aChannel)
>-        NS_QueryNotificationCallbacks(aChannel, parent);
>+      if (aChannel) {
>+	nsCOMPtr<nsILoadContext> ctx;
>+        NS_QueryNotificationCallbacks(aChannel, ctx);
>+	if (ctx) {
>+	  ctx->GetAssociatedWindow(getter_AddRefs(parent));
>+	}
>+      }

some tab funkiness going on here, and elsewhere in this file - not sure if it's a modeline gone awry (if it is, please feel free to fix).

>@@ -459,46 +461,57 @@ nsCookiePermission::GetOriginatingURI(ns

>   // find the docshell and its root

are we dealing with docshells or windows now? (does the big comment block with the case descriptions need massaging too?)

>   // cases 2) and 3)
>-  if (!root || type != nsIDocShellTreeItem::typeContent)
>+  // XXXbz why not enforce this for non-content docshells too?  Either they
>+  // have the system principal, and then they'd fail down in case 5 anyway (but
>+  // allow through case 4) or they're not the system principal and can't do
>+  // much harm.  Or am I missing something here?

what do you mean by "this"?

>   // case 4)
>-  if (docshell == root) {
>-    nsLoadFlags flags;
>-    aChannel->GetLoadFlags(&flags);
>+  if (ourWin == topWin) {
>+    // Check whether this is the document channel for this window (representing
>+    // a load of a new page).  This is a bit of a nasty hack; perhaps it should
>+    // live on nsILoadContext?
>+    nsCOMPtr<nsIDocumentLoader> loader = do_QueryInterface(webnav);
>+    if (loader) {
>+      nsCOMPtr<nsIChannel> docChannel;
>+      loader->GetDocumentChannel(getter_AddRefs(docChannel));
>+      if (docChannel == aChannel) {
>+	// get the channel URI - the docshell's will be bogus
>+	aChannel->GetURI(aURI);
>+	if (!*aURI)
>+	  return NS_ERROR_NULL_POINTER;

i'm curious, why do we need the (ourWin == topWin) check? (i don't understand how this check is functionally equivalent to the docshell & loadflag check, so i'm trying to get an idea of how it works.)

there's also a method with some docshell-crawling goo hanging out in http://mxr.mozilla.org/mozilla/source/mailnews/base/src/nsMsgContentPolicy.cpp#629, but since that's not part of mozilla-central, not sure if anything needs to be done here.

there are third-party cookie mochitests in extensions/cookie/test... you can run those to exercise the various cases in GetOriginatingURI. (note that one of the tests was disabled due to intermittent failures, pending a fix - http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/test/Makefile.in#76 - please enable it locally.) we should definitely have something to exercise the external resource doc case if possible; i can write one at a later point (with some help).

r=me with nits on the cookie bits.

thanks bz!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 16 years ago

> some tab funkiness going on here, and elsewhere in this file

Will fix.

> does the big comment block with the case descriptions need massaging too?

Good catch.  I'll massage the comments some.

> what do you mean by "this"?

Actually returning a URI from this method.  That is, why is the non-system chrome case special-cased here?  The system chrome is special-cased because it has no principal URI; that part makes sense to me.

> i'm curious, why do we need the (ourWin == topWin) check?

It's the equivalent of the docshell == root check that used to be there.  It's checking that the window involved is the root same-type window.  Then the next check is checking that the channel is in fact the document channel currently being loaded in that window.

> there's also a method with some docshell-crawling goo hanging out in [mail]

Good catch.  I'll get a bug filed (and maybe patch).

> there are third-party cookie mochitests in extensions/cookie/test...

Huh.  I skimmed those tests and none jumped out at me.  Do some of them set the third-party pref, then?

	dwitte@gmail.com
	Comment 4 • 16 years ago

(In reply to comment #3)
> > what do you mean by "this"?
> 
> Actually returning a URI from this method.  That is, why is the non-system
> chrome case special-cased here?  The system chrome is special-cased because it
> has no principal URI; that part makes sense to me.

ah. well, i'm not sure under what conditions loads are from system or non-system chrome, but this is intended to catch the case of loads kicked off from chrome that didn't result from user interaction (e.g. favicons, safebrowsing updates, ocsp certs and such). one may wonder what relation the two bear, but empirically i found this to work. (content docshells presumably mean something's being displayed to the user, which in firefox at least means they clicked on something - unless it's a rogue popup - whereas content docshells mean it's probably coming from the browser internals and has a good chance of being hidden to the user, and we're considering silent loads to be third party. there's a bug filed to give chrome loads the ability to opt-in to third party cookies, if they really do need them.)

does that answer your question? or raise more? ;)

> > there are third-party cookie mochitests in extensions/cookie/test...
> 
> Huh.  I skimmed those tests and none jumped out at me.  Do some of them set the
> third-party pref, then?

yeah, they all do (i think), via the js files... see e.g. http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/test/browser_test_favicon.js#10

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 5 • 16 years ago

> but this is intended to catch the case of loads kicked off
> from chrome that didn't result from user interaction

I understand that, but why do those need to be caught specially?  Unless they're same-origin with the chrome that started loading them we'll block the cookies anyway, no?

	Johnny Stenback (:jst)
	Comment 6 • 16 years ago

Comment on attachment 348818 [details] [diff] [review]
Proposed fix

r+sr=jst

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 16 years ago

OK.  So the test_loadflags test failed with my original patch.  There was at least one call to GetOriginatingURI that had the DOCUMENT_URI flag on the channel but did NOT have it set as the document channel of the docloader yet (in fact had a null document channel).  Is this perhaps a call coming from inside AsyncOpen or something?

In any case, I just undid that change and went back to checking loadflags.  As long as we don't do cross-site external document references, or at least don't flag those channels as DOCUMENT_URI (which so far we don't), we should be fine.

With that, all the tests pass, and the http://www.grc.com/siteoptions.htm bit (check the two "enable tpc" boxes and then browse around) works fine.

Patch with comments addressed coming up.  dwitte was saying he'll try to write some tests this weekend.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 16 years ago

Comment on attachment 349271 [details] [diff] [review]
Updated to comments

Since this changes things for extensions that might want to get docshells, we probably want this for b2.  It does pass existing tests.  I've done manual testing to verify that the code works correctly, and we're working on creating some automated tests using external document references...

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 10 • 16 years ago

Comment on attachment 349271 [details] [diff] [review]
Updated to comments

a191b2=beltzner, please watch closely after landing

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 16 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/d584db13034e and then http://hg.mozilla.org/mozilla-central/rev/371b210baaa4 to fix build bustage.

As far as dev-doc-needed, what needs documenting are the following:

1) The right way to get a load context from a request is to get an
   nsILoadContext, not to query various docshell APIs.
2) The right way to get a load context in C++ is:

     nsCOMPtr<nsILoadContext> loadContext;
     nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
     NS_QueryNotificationCallbacks(channel, loadContext);

3) The right way to get a load context in JS is:

     var loadContext;
     try {
       loadContext = 
         aRequest.queryInterface(Components.interfaces.nsIChannel)
                 .notificationCallbacks
                 .getInterface(Components.interfaces.nsILoadContext);
     } catch (ex) {
       try {
         loadContext =
           aRequest.loadgroup.notificationCallbacks
                   .getInterface(Components.interfaces.nsILoadContext);
       } catch (ex) {
         loadContext = null;
       }
     }

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 16 years ago

Er, that should be:

  aRequest.loadGroup.notificationCallbacks
          .getInterface(Components.interfaces.nsILoadContext);

in the second try block in the JS code.

	Benjamin Smedberg
	Comment 13 • 15 years ago

Fixed before branching.

	Eric Shepherd [:sheppy]
	Comment 14 • 15 years ago

https://developer.mozilla.org/En/Updating_extensions_for_Firefox_3.1#Getting_a_load_context_from_a_request

	Nickolay_Ponomarev
	Comment 15 • 13 years ago

bz, in comment 11 you say "not to query various docshell APIs" - does it include "notificationCallbacks.getInterface(Ci.nsIDOMWindow)"?

I see you changed the code in cookies that did that to use nsILoadContext, but I assume the older way still works, at least it's used at https://developer.mozilla.org/en/Code_snippets/Tabbed_browser#Getting_the_browser_that_fires_the_http-on-modify-request_notification and re-posted quite a few times on other sites (also after this change was released).

Will the nsIDOMWindow way of doing this stay or should we stop recommending it? Is there any difference between the two?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 16 • 13 years ago

> does it include "notificationCallbacks.getInterface(Ci.nsIDOMWindow)"?

Yes.

> I assume the older way still works,

It may or it may not, depending on e10s and whether you're talking about SVG resource loads.

> at least it's used at

I thought I fixed that at some point...

> Will the nsIDOMWindow way of doing this stay

It's already gone, for SVG resource documents.  Has been for years.

	Nickolay_Ponomarev
	Comment 17 • 13 years ago

Thanks for the quick clarification!

I updated https://developer.mozilla.org/En/Updating_extensions_for_Firefox_3.5#Getting_a_load_context_from_a_request to be more explicit about this and mentioned this on the tab snippets page. (Didn't update the code for the fear of introducing typos.)

I also mentioned this on a few of the top google hits for the incorrect pattern.