1691113 - Commented out block relating to ancient cookie Path requirements should be removed

Status: RESOLVED FIXED

(Core :: Networking: Cookies, enhancement, P2)

Description

[Mike Taylor [:miketaylr]]

https://searchfox.org/mozilla-central/source/netwerk/cookie/CookieService.cpp#1791-1806 has the following commented out code:

#if 0
  } else {
    /**
     * The following test is part of the RFC2109 spec.  Loosely speaking, it says that a site
     * cannot set a cookie for a path that it is not on.  See bug 155083.  However this patch
     * broke several sites -- nordea (bug 155768) and citibank (bug 156725).  So this test has
     * been disabled, unless we can evangelize these sites.
     */
    // get path from aHostURI
    nsAutoCString pathFromURI;
    if (NS_FAILED(aHostURI->GetPathQueryRef(pathFromURI)) ||
        !StringBeginsWith(pathFromURI, aCookieData.path())) {
      return false;
    }
#endif

Chromium has a similar comment at https://source.chromium.org/chromium/chromium/src/+/master:net/cookies/canonical_cookie.cc;l=310-314;drc=c6259cb544738517e7f1e9400ac8ce9372f8c9a1

  // The RFC says the path should be a prefix of the current URL path.
  // However, Mozilla allows you to set any path for compatibility with
  // broken websites.  We unfortunately will mimic this behavior.  We try
  // to be generous and accept cookies with an invalid path attribute, and
  // default the path to something reasonable.

The relevant spec text at the time stated:

(From https://tools.ietf.org/html/rfc2109:)

4.3.2  Rejecting Cookies

   To prevent possible security or privacy violations, a user agent
   rejects a cookie (shall not store its information) if any of the
   following is true:

   * The value for the Path attribute is not a prefix of the request-
     URI.

But rfc6265 states:

the Path attribute does not provide any integrity protection because the
user agent will accept an arbitrary Path attribute in a Set-Cookie header.

So this block can be removed.

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: 0001-Bug-1691113-Remove-obsolete-and-commented-out-cookie.patch

Awkwardly, if I try to log into Phabricator with my Bugzilla account, it says my account has been disabled.

Comment 2

[Mike Taylor [:miketaylr]]

Comment on attachment 9201691 [details] [diff] [review]
0001-Bug-1691113-Remove-obsolete-and-commented-out-cookie.patch

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

r? :dragana

(I have no idea if code is even landable via splinter anymore)

Comment 3

[:glob ✱]

(I have no idea if code is even landable via splinter anymore)

It isn't; I've re-enabled your Phabricator account, please resubmit there.

Comment 4

[Mike Taylor [:miketaylr]]

Thanks glob!

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Chrome seems to have fixed their bug. We should do that too.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

https://searchfox.org/mozilla-central/rev/ac497ae160e664617e2f4286cdce3b03ef912d90/netwerk/cookie/CookieService.cpp#1928

Comment 9

[Ed Guloien [:edgul]]

Attachment: Bug 1691113 - Remove unused ancient cookie setting path requirement r?valentin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e2f00ee20ccc
Remove unused ancient cookie setting path requirement r=valentin,cookie-reviewers

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/e2f00ee20ccc