Page MenuHomePhabricator

Response to api.php?action=login on Wikimedia wikis has some seriously sick Set-Cookie headings
Closed, ResolvedPublic

Description

Response to api.php?action=login on Wikimedia wikis has some seriously sick Set-Cookie headings.

curl -X POST -I "https://en.wikipedia.org/w/api.php?format=json&action=login" | grep -i Set-Cookie

Set-Cookie: forceHTTPS=true; path=/; httponly
Set-Cookie: enwikiSession=XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX; path=/; secure; httponly
Set-Cookie: enwikiUserID=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; httponly
Set-Cookie: enwikiToken=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; secure; httponly
Set-Cookie: centralauth_User=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; domain=.wikipedia.org; secure; httponly
Set-Cookie: centralauth_Token=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; domain=.wikipedia.org; secure; httponly
Set-Cookie: centralauth_Session=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; domain=.wikipedia.org; secure; httponly
Set-Cookie: forceHTTPS=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; httponly
Set-Cookie: forceHTTPS=true; path=/; domain=.wikipedia.org; httponly
Set-Cookie: forceHTTPS=true; path=/; httponly
Set-Cookie: forceHTTPS=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0; path=/; httponly
Set-Cookie: forceHTTPS=true; path=/; domain=.wikipedia.org; httponly
Set-Cookie: WMF-Last-Access=22-Jan-2016;Path=/;HttpOnly;Expires=Tue, 23 Feb 2016 12:00:00 GMT
Set-Cookie: GeoIP=YYYYYYYYYYYYYYYYYYYYYYYYYYY; Path=/; Domain=.wikipedia.org
  1. Why are we sending so many pre-expired cookies? I see that this is intentional, but I'm curious why it's needed.
  2. Why are we sending the 'forceHTTPS' cookies six times?

(The expired cookies serve as a good test of the client's cookie handling abilities, heh. My bot was unable to log in until I taught it to expire cookies.)

Event Timeline

matmarex created this task.Jan 22 2016, 2:57 PM
matmarex raised the priority of this task from to Needs Triage.
matmarex updated the task description. (Show Details)
matmarex added a subscriber: matmarex.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptJan 22 2016, 2:57 PM
Reedy set Security to None.
Reedy added subscribers: csteipp, dcausse.
Tgr added a subscriber: Tgr.Jan 22 2016, 6:09 PM

AIUI cookies need to be consistent now (see T124384#1954664).

Anomie added a comment.EditedJan 22 2016, 6:26 PM

Why are we sending so many pre-expired cookies? I see that this is intentional, but I'm curious why it's needed.

Before SessionManager, PHP set the session cookie and other code set/cleared the other cookies when that other code happened to be called. Now that handling of all authn cookies is in CookieSessionProvider (or CentralAuthSessionProvider), all those cookies get updated every time the session gets saved.

We could theoretically have WebResponse check whether the cookie was in $_COOKIE (or was set earlier in this request) before explicitly deleting it, if we decide that avoiding sending deletions for already-deleted cookies is something that would be useful.

Why are we sending the 'forceHTTPS' cookies six times?

  1. CookieSessionProvider setting the cookie.
  2. CentralAuthSessionProvider clearing the cookie that CookieSessionProvider set.
  3. CentralAuthSessionProvider setting the cookie with different parameters.
  4. CookieSessionProvider setting the cookie again, because step #2 cleared it and step #3 set the same name with different parameters anyway.
  5. CentralAuthSessionProvider clearing the cookie that CookieSessionProvider set.
  6. CentralAuthSessionProvider setting the cookie itself with different parameters.

We could cut out number 6 by adjusting the logic in WebResponse to more closely match the algorithm in RFC 6265 § 5.3, particularly step 11, although that could risk having broken clients fail in different ways. Cutting out the rest would be more difficult, unless we decide to let it have the 'forceHTTPS' cookie set on two different domains and rely on the code to make sure they don't get out of sync.

saper added a subscriber: saper.Jan 22 2016, 9:26 PM

Steps 1-5 indicate there is something wrong in the architecture, why can't they communicate more efficiently?

Change 265929 had a related patch set uploaded (by Anomie):
SessionManager: Abstract forceHTTPS cookie setting

https://gerrit.wikimedia.org/r/265929

Change 265931 had a related patch set uploaded (by Anomie):
SessionManager: Abstract forceHTTPS cookie setting

https://gerrit.wikimedia.org/r/265931

Change 265929 merged by jenkins-bot:
SessionManager: Abstract forceHTTPS cookie setting

https://gerrit.wikimedia.org/r/265929

Change 265931 merged by jenkins-bot:
SessionManager: Abstract forceHTTPS cookie setting

https://gerrit.wikimedia.org/r/265931

Anomie closed this task as Resolved.Jan 25 2016, 8:22 PM
Anomie claimed this task.

The task description provides the "before" here. Could someone please provide the "after"?

The "after" has fewer forceHTTPS cookies set (in most cases it should be two max), and also avoids sending "deleted" cookies if the cookie being deleted wasn't in the request to start with.

For future reference, the output from wmf.10, as seen right now for the same request, is:

Set-Cookie: enwikiSession=XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX; path=/; secure; httponly
Set-Cookie: WMF-Last-Access=25-Jan-2016;Path=/;HttpOnly;Expires=Fri, 26 Feb 2016 12:00:00 GMT
Set-Cookie: GeoIP=YYYYYYYYYYYYYYYYYYYYYYYYYYY; Path=/; Domain=.wikipedia.org

Change 266415 had a related patch set uploaded (by BryanDavis):
SessionManager: Abstract forceHTTPS cookie setting

https://gerrit.wikimedia.org/r/266415

Change 266423 had a related patch set uploaded (by BryanDavis):
SessionManager: Abstract forceHTTPS cookie setting

https://gerrit.wikimedia.org/r/266423

Change 266423 merged by jenkins-bot:
SessionManager: Abstract forceHTTPS cookie setting

https://gerrit.wikimedia.org/r/266423

Change 266415 merged by jenkins-bot:
SessionManager: Abstract forceHTTPS cookie setting

https://gerrit.wikimedia.org/r/266415

Change 266585 had a related patch set uploaded (by Anomie):
Avoid forceHTTPS cookie flapping if core and CA are setting the same cookie

https://gerrit.wikimedia.org/r/266585

Change 266585 merged by jenkins-bot:
Avoid forceHTTPS cookie flapping if core and CA are setting the same cookie

https://gerrit.wikimedia.org/r/266585

Change 266671 had a related patch set uploaded (by Gergő Tisza):
Avoid forceHTTPS cookie flapping if core and CA are setting the same cookie

https://gerrit.wikimedia.org/r/266671

Change 266671 merged by jenkins-bot:
Avoid forceHTTPS cookie flapping if core and CA are setting the same cookie

https://gerrit.wikimedia.org/r/266671

sbassett moved this task from Backlog to Done on the Security-Team board.Jun 11 2019, 7:08 PM