595305 - Factor cookie third-party URI code into separate API

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

Other code is going to start needing third-party checking code -- specifically, what nsCookiePermission::GetOriginatingURI and nsCookieService::IsForeign do (and their associated dependencies).

We should factor this out into a common helper service, or stash the APIs somewhere appropriate. A separate service is probably the way to go here.

Comment 1

[Shawn Wilsher :sdwilsh]

(In reply to comment #0)
> Other code is going to start needing third-party checking code -- specifically,
> what nsCookiePermission::GetOriginatingURI and nsCookieService::IsForeign do
> (and their associated dependencies).
Like what?  Are bugs filed (and can they be added to the dependency list)?

Comment 2

[[e-mail]]

See dependency. :)

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This blocks a betaN+ blocker and therefore needs to block betaN.

Comment 4

[[e-mail]]

Attachment: patch

OK, here it is. It's fairly self-explanatory. This really isn't as big as it looks; just jiggering stuff around. All the interesting stuff is in one file.

A few notes:

1) This changes the definition of "third party cookie" in that we now test against the URI of every window in the hierarchy; not just the topmost. I think this is better.

2) This optimizes under what situations we compute foreignness. (If the user has it preffed off, which is the default, we don't do any work.)

3) This breaks binary compat with nsICookiePermission. I think that's OK since it's for internal use only.

Tests forthcoming.

Comment 5

[[e-mail]]

Attachment: tests

This adds to our existing third party cookie tests, which basically cover all the cases we're interested in. It adds an additional test for the case where we have three windows involved:

* test_different_domain_in_hierarchy.html opens a popup window with toplevel URL example.org/file_domain_hierarchy_inner.html
* This loads in an iframe example.com/file_domain_hierarchy_inner_inner.html
* This loads in an iframe example.org/file_domain_hierarchy_inner_inner_inner.html

So this tests the case where the top and bottommost window URIs are same-domain, but the middle one is not.

The existing tests cover both isThirdPartyChannel and isThirdPartyWindow, since the former calls into the latter for the window hierarchy bits.

The additional xpcshell test covers isThirdPartyURI, which is simple to test, and the various corner cases of the other two methods. The mochitests cover the rest.

Comment 6

[Benjamin Smedberg]

bent, can you prioritize this review? It's making me nervous. Also, since we're past API freeze, the interface change will need to be un-done.

Comment 7

[[e-mail]]

Attachment: patch v2

This avoids interface change and has nsICookiePermission.getOriginatingURI return NS_ERROR_NOT_IMPLEMENTED. I think this is fine since this really is an internal-use interface...

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 482739 [details] [diff] [review]
patch v2

>+ThirdPartyUtil::Init()

I think that you should NS_ASSERT or NS_ENSURE that you're on the main thread here. Refcount assertions should help protect you elsewhere.

>+  // Block any URIs without a host that aren't file:// URIs. This makes it the

Typo

>+  if (NS_FAILED(rv)) return rv;

Blegh! Two lines please. And elsewhere.

>+  // Check strict equality.
>+  *aResult = aFirstDomain != secondDomain;

Shouldn't you ToLowercase() these before comparing?

>+  NS_ENSURE_ARG(aFirstURI);
>+  NS_ENSURE_ARG(aSecondURI);

NS_ENSURE_ARG_POINTER

>+  nsCAutoString firstHost;

Just nsCString.

>+ThirdPartyUtil::IsThirdPartyWindow(nsIDOMWindow* aWindow,
>+                                   nsIURI* aURI,
>+                                   PRBool* aResult)
>+{
>+  NS_ENSURE_ARG(aWindow);

NS_ENSURE_ARG_POINTER. And NS_ASSERTION(aResult)?

>+  nsCAutoString bottomDomain;

nsCString.

>+    rv = current->GetParent(getter_AddRefs(parent));
>+    NS_ENSURE_SUCCESS(rv, NS_ERROR_INVALID_ARG);

Why not just return rv here? Switching error codes seems weird.

Also, nsGlobalWindow::GetParent can return NS_OK and leave |parent| null, so you need to handle that case. Maybe change the |while(1)| to |while(current)| ?

>+    if (parent == current) {

Use SameCOMIdentity here.

>+ThirdPartyUtil::IsThirdPartyChannel(nsIChannel* aChannel,
>+                                    nsIURI* aURI,
>+                                    PRBool* aResult)
>+{
>+  NS_ENSURE_ARG(aChannel);

NS_ENSURE_ARG_POINTER. And NS_ASSERTION(aResult)?

>+  PRBool doForce = false;

Either use |bool| or |PR_FALSE|.

>+  nsCOMPtr<nsIHttpChannelInternal> httpChannelInternal =
>+    do_QueryInterface(aChannel);
>+  if (httpChannelInternal)
>+  {

Your earlier braces were always on the previous line. What changed? Below as well.

>+    nsresult rv = httpChannelInternal->GetForceAllowThirdPartyCookie(&doForce);

You need |rv| below, why not declare |rv| next to |doForce|?

>+  nsCAutoString channelDomain;

nsCString

>+  nsCOMPtr<nsILoadContext> ctx;
>+  NS_QueryNotificationCallbacks(aChannel, ctx);
>+  if (!ctx) return NS_ERROR_INVALID_ARG;

NS_ENSURE_TRUE here? Seems like something we'd want a warning about, right?

>+  nsCOMPtr<nsIDOMWindow> ourWin, parentWin;
>+  ctx->GetAssociatedWindow(getter_AddRefs(ourWin));
>+  if (!ourWin) return NS_ERROR_INVALID_ARG;

Here too?

>+  if (ourWin == parentWin) {

SameCOMIdentity

>+    nsLoadFlags flags;
>+    aChannel->GetLoadFlags(&flags);

You either need to check success or initialize |flags| here.

>+#undef  LIMIT

You need to wrap this in an |#ifdef LIMIT|

>+#define LIMIT(x, low, high, default) ((x) >= (low) && (x) <= (high) ? (x) : (default))

Past 80 chars here. And you were doing so well in all the other files! ;)

>+static const PRUint32 BEHAVIOR_ACCEPT        = 0;
>+static const PRUint32 BEHAVIOR_REJECTFOREIGN = 1;
>+static const PRUint32 BEHAVIOR_REJECT        = 2;

Sigh. These are defined in three places already according to MXR. Any chance you'd like to consolidate?

>+static const char kPrefThirdPartySession[] = "network.cookie.thirdparty.sessionOnly";

80 char limit.

>+    prefBranch->AddObserver(kPrefCookieBehavior,    this, PR_TRUE);
>+    prefBranch->AddObserver(kPrefThirdPartySession, this, PR_TRUE);

I'm not a fan of space padding like that... Won't kill me though.

>+CookieServiceChild::PrefChanged(nsIPrefBranch *aPrefBranch)
>+{
>+  PRInt32 val;
>+  if (NS_SUCCEEDED(aPrefBranch->GetIntPref(kPrefCookieBehavior, &val)))
>+    mCookieBehavior = (PRUint8) LIMIT(val, 0, 2, 0);

Shouldn't that be |BEHAVIOR_ACCEPT| and |BEHAVIOR_REJECT|? No magic numbers here!

Then you're going to have signed vs. unsigned comparison warnings, though. I think you should just test manually.

>+    mThirdPartyUtil = do_GetService(THIRDPARTYUTIL_CONTRACTID);
>+    NS_ASSERTION(mThirdPartyUtil, "require ThirdPartyUtil service");

Hm... This can fail for any number of reasons, right? Simply asserting isn't enough. You need to test mThirdPartyUtil where you use it.

>+CookieServiceChild::Observe(nsISupports     *aSubject,
> ...
>+  nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(aSubject);
>+  if (prefBranch)
>+    PrefChanged(prefBranch);

Seems like we'd want a warning if that ever fails.

>@@ -1311,16 +1316,22 @@ nsCookieService::PrefChanged(nsIPrefBran

Er, hm. There's a lot of code duplicated here... You've got the same problem with mThirdPartyUtil not being guaranteed to exist.

>diff --git a/netwerk/cookie/nsICookiePermission.idl b/netwerk/cookie/nsICookiePermission.idl

Yeah, we can't change this interface until we branch for 2.0. I don't think you can even change it to just return NS_ERROR_NOT_IMPLEMENTED. Why not just leave it like it was and mark it deprecated?

Comment 9

[[e-mail]]

(In reply to comment #8)
> >+  // Check strict equality.
> >+  *aResult = aFirstDomain != secondDomain;
> 
> Shouldn't you ToLowercase() these before comparing?

No need; the base domain strings are ACE'd and normalized.

> >+  NS_ENSURE_ARG(aFirstURI);
> >+  NS_ENSURE_ARG(aSecondURI);
> 
> NS_ENSURE_ARG_POINTER

I believe convention is for NS_ENSURE_ARG to be used for actual args (which these are), and NS_ENSURE_ARG_POINTER for outparam pointers.

> >+    rv = current->GetParent(getter_AddRefs(parent));
> >+    NS_ENSURE_SUCCESS(rv, NS_ERROR_INVALID_ARG);
> 
> Why not just return rv here? Switching error codes seems weird.
> 
> Also, nsGlobalWindow::GetParent can return NS_OK and leave |parent| null, so
> you need to handle that case. Maybe change the |while(1)| to |while(current)| ?

The GetURIFromWindow() call a few lines below will return NULL in that case, and we'll bail. (Which is fine, right? The window having no docshell should definitely be a bail condition.)

> >+  PRBool doForce = false;
> 
> Either use |bool| or |PR_FALSE|.

PR_* are going the way of the dinosaurs, and jsengine style already allows 'JSBool foo = true;'-type statements. And I can't use 'bool' here because it's an outparam to an IDL method. Do you really want me to change it? :)

> >+  nsCOMPtr<nsILoadContext> ctx;
> >+  NS_QueryNotificationCallbacks(aChannel, ctx);
> >+  if (!ctx) return NS_ERROR_INVALID_ARG;
> 
> NS_ENSURE_TRUE here? Seems like something we'd want a warning about, right?

I'm trying to walk a fine line between debug spew and useful warnings. I chose the conditions that can commonly fail to not warn. In this case, not having a ctx just means someone manually kicked off a channel (which we do all over the place, e.g. favicon code IIRC, extensions, chrome code, etc.). Sound OK?

> >+static const PRUint32 BEHAVIOR_ACCEPT        = 0;
> >+static const PRUint32 BEHAVIOR_REJECTFOREIGN = 1;
> >+static const PRUint32 BEHAVIOR_REJECT        = 2;
> 
> Sigh. These are defined in three places already according to MXR. Any chance
> you'd like to consolidate?

They mean different things in all those 3 files. :)

> >+    mThirdPartyUtil = do_GetService(THIRDPARTYUTIL_CONTRACTID);
> >+    NS_ASSERTION(mThirdPartyUtil, "require ThirdPartyUtil service");
> 
> Hm... This can fail for any number of reasons, right? Simply asserting isn't
> enough. You need to test mThirdPartyUtil where you use it.

How would it fail, other than OOM?

Not having this should be a hard (as in fatal) failure. If you think there are conditions where we could legitimately not have it, though, then I could make all the places we require it just bail out with error codes.

All other comments addressed, new patch coming up!

Comment 10

[[e-mail]]

Attachment: patch v3

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 483357 [details] [diff] [review]
patch v3

Looks good!

Comment 12

[[e-mail]]

Thanks!

Did you want to look at the tests too, or do I have an implicit plus?

Comment 13

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/56815e37d436
http://hg.mozilla.org/mozilla-central/rev/d35efa6966ac

Comment 14

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Could this have caused bug 605835?