Closed Bug 595305 Opened 9 years ago Closed 9 years ago

Factor cookie third-party URI code into separate API

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: dwitte, Assigned: dwitte)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
(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)?
Blocks: 595307
See dependency. :)
This blocks a betaN+ blocker and therefore needs to block betaN.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #481886 - Flags: review?(bent.mozilla)
Attached patch testsSplinter Review
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.
Attachment #481890 - Flags: review?(bent.mozilla)
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.
Attached patch patch v2 (obsolete) — Splinter Review
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...
Attachment #481886 - Attachment is obsolete: true
Attachment #482739 - Flags: review?(bent.mozilla)
Attachment #481886 - Flags: review?(bent.mozilla)
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?
(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!
Attached patch patch v3Splinter Review
Attachment #482739 - Attachment is obsolete: true
Attachment #483357 - Flags: review?(bent.mozilla)
Attachment #482739 - Flags: review?(bent.mozilla)
Comment on attachment 483357 [details] [diff] [review]
patch v3

Looks good!
Attachment #483357 - Flags: review?(bent.mozilla) → review+
Thanks!

Did you want to look at the tests too, or do I have an implicit plus?
Attachment #481890 - Flags: review?(bent.mozilla) → review+
http://hg.mozilla.org/mozilla-central/rev/56815e37d436
http://hg.mozilla.org/mozilla-central/rev/d35efa6966ac
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.