path matching added in bug 1264192 does the wrong thing

RESOLVED FIXED in Firefox 51

Status

()

Core
Networking: Cookies
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dveditz, Assigned: jdm)

Tracking

({regression})

50 Branch
mozilla52
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 unaffected, firefox50+ wontfix, firefox51+ fixed, firefox52 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
+++ 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.
Whiteboard: [necko-active]
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Updated

2 years ago
Flags: needinfo?(josh)
(Assignee)

Comment 2

2 years ago
Created attachment 8792076 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Flags: needinfo?(josh)
(Assignee)

Comment 3

2 years ago
Created attachment 8792103 [details] [diff] [review]
Avoid duplicating domain-matching and path-matching logic in cookie service

Turns out the existing eviction test already contained some incorrect tests\!
Attachment #8792103 - Flags: review?(ehsan)
(Assignee)

Updated

2 years ago
Attachment #8792076 - Attachment is obsolete: true

Comment 4

2 years ago
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.
Attachment #8792103 - Flags: review?(ehsan) → review+
(Assignee)

Comment 6

2 years ago
Created attachment 8792635 [details] [diff] [review]
Avoid duplicating domain-matching and path-matching logic in cookie service

Comments addressed. Waiting on try results.
(Assignee)

Updated

2 years ago
Attachment #8792103 - Attachment is obsolete: true
(Assignee)

Comment 9

2 years ago
Failures in the last job all look unrelated.
Keywords: checkin-needed
(Assignee)

Comment 10

2 years ago
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
Attachment #8792635 - Flags: approval-mozilla-beta?
Attachment #8792635 - Flags: approval-mozilla-aurora?

Comment 11

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f261677780f
Avoid duplicating domain-matching and path-matching logic in cookie service. r=ehsan
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5f261677780f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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.
Attachment #8792635 - Flags: approval-mozilla-beta?
Attachment #8792635 - Flags: approval-mozilla-beta-
Attachment #8792635 - Flags: approval-mozilla-aurora?
Attachment #8792635 - Flags: approval-mozilla-aurora+

Updated

2 years ago
status-firefox50: affected → wontfix

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/c4b72a980594
status-firefox51: affected → fixed
Flags: in-testsuite+
Version: unspecified → 50 Branch
You need to log in before you can comment on or make changes to this bug.