1302824 - path matching added in bug 1264192 does the wrong thing

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Daniel Veditz [:dveditz]]

+++ This bug was initially created as a clone of Bug #1264192 +++

The changes to the eviction algorithm in bug 1264192 confused cookie "path" and nsIURI "path" which are not the same thing.

The new code sets sourcePath using GetPath() from nsIURI, which is everything after the origin -- nsIURI knows nothing about specific http-defined sub-parts. This code should have done the same thing as CheckPath(). In fact I recommend creating a GetPathFromURI() helper function and calling it in both places so we're sure to be consistent and will be so if people add more code later. In CheckPath we do:

    // strip down everything after the last slash to get the path,
    // ignoring slashes in the query string part.
    // if we can QI to nsIURL, that'll take care of the query string portion.
    // otherwise, it's not an nsIURL and can't have a query string, so just find the last slash.
    nsCOMPtr<nsIURL> hostURL = do_QueryInterface(aHostURI);
    if (hostURL) {
      hostURL->GetDirectory(aCookieAttributes.path);
    } else {
      aHostURI->GetPath(aCookieAttributes.path);
      int32_t slash = aCookieAttributes.path.RFindChar('/');
      if (slash != kNotFound) {
        aCookieAttributes.path.Truncate(slash + 1);
      }
    }

This code will make sure the path ends with a slash so you can remove the slash-stripping length check later on in the comparison. This is good because I think there's another bug lurking there: an existing cookie with path "/foo/" will match a sourcePath of "/foobar". Should add a test to make sure "/foo/" and "/foo/bar/" match but "/foobar/" doesn't. Testing "starts with" including the trailing slash prevents this.

Comment 1

[Daniel Veditz [:dveditz]]

The code does NOT make sure path ends with a slash -- I've found many that don't, although also many that do. The ones that don't are mostly google session cookies, from docs.google.com and mail.google.com for instance.

I have a mail.google.com cookie with the path "/mail/u/1" that comes from a URL like
https://mail.google.com/mail/u/1/#inbox/<someid> -- no real "file" part to that URL. I just checked these in Chrome and the cookie paths don't end with a slash either so I guess we're correct. Maybe the ones that _do_ end with a slash are a problem?

In any case, we do still have to be careful with prefix-matching paths.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Avoid duplicating domain-matching and path-matching logic in cookie service

I like reading this code a lot more after consolidating it. I'm going to write some tests for the broken eviction logic that these changes should fix.

Comment 3

[Josh Matthews [:jdm]]

Attachment: Avoid duplicating domain-matching and path-matching logic in cookie service

Turns out the existing eviction test already contained some incorrect tests\!

Comment 4

[(no longer active)]

Comment on attachment 8792103 [details] [diff] [review]
Avoid duplicating domain-matching and path-matching logic in cookie service

Review of attachment 8792103 [details] [diff] [review]:
-----------------------------------------------------------------

No need to escape exclamation marks in commit messages.  Thanks, bourne shell!

::: netwerk/cookie/nsCookieService.cpp
@@ +3033,5 @@
>    }
>  };
>  
> +bool
> +DomainMatches(nsCookie* aCookie, nsACString& aHost) {

Nit: please make all of these helper functions static.

@@ +3042,5 @@
> +      (aCookie->IsDomain() && StringEndsWith(aHost, aCookie->Host()));
> +}
> +
> +bool
> +PathMatches(nsCookie* aCookie, nsACString& aPath) {

It doesn't look like you're mutating aPath.  Please make it a const reference.

Comment 5

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0c0ab47f0ff

Comment 6

[Josh Matthews [:jdm]]

Attachment: Avoid duplicating domain-matching and path-matching logic in cookie service

Comments addressed. Waiting on try results.

Comment 7

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=61c69038a48c

Comment 8

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=df17c5dabddf

Comment 9

[Josh Matthews [:jdm]]

Failures in the last job all look unrelated.

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 8792635 [details] [diff] [review]
Avoid duplicating domain-matching and path-matching logic in cookie service

I'm requesting for both 50 and 51, since it's release day and this is only just merging to mozilla-inbound today. I'm not actually sure that it's worth taking this on 50, since it's an edge case of an edge case.

Approval Request Comment
[Feature/regressing bug #]: 1264192
[User impact if declined]: Potential for non-optimal cookies to be evicted on sites that set more than 165 cookies.
[Describe test coverage new/current, TreeHerder]: A bunch of new tests exercising this code.
[Risks and why]: These changes refactor the code that decides which cookies to send; if it's wrong somehow, arbitrary sites could start behaving oddly. The refactoring is just consolidating duplicated code into a single place, so the risk is low imo.
[String/UUID change made/needed]: None

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f261677780f
Avoid duplicating domain-matching and path-matching logic in cookie service. r=ehsan

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f261677780f

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8792635 [details] [diff] [review]
Avoid duplicating domain-matching and path-matching logic in cookie service

Given the risk of this fix, I'd prefer to wontfix for 50 and uplift only for Aurora51.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c4b72a980594