Page MenuHomePhabricator

Allow cookie-block tracking from any uncached web request
Closed, ResolvedPublic

Description

This approach is more consistent than the current approach of adding the cookie on specific actions, causing bugs like T196575 when the logic wasn't added for a particular action.

See T196575#5472917 and the associated discussion for more details.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 23 2019, 10:00 AM

Assuming this task is about MediaWiki-User-management, hence adding project tag so others can find this task when searching for tasks under that project or looking at that project workboard.

Change 537714 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] exception: Let MediaWiki.php control final output for ErrorPageError

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

Change 538716 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] exception: Add missing early return for UserNotLoggedIn error page

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

Change 534933 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] block: Allow cookie-block tracking from any uncached web request

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

Krinkle triaged this task as High priority.Sep 24 2019, 2:21 AM
Krinkle added a project: Performance-Team.
Krinkle moved this task from Inbox to Blocked or Needs-CR on the Performance-Team board.

Change 538716 merged by jenkins-bot:
[mediawiki/core@master] exception: Add missing early return for UserNotLoggedIn error page

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

Tagging CPT for CR on this particular patch, blocks the other patch and parent task which could all be reviewed by AHT.

Change 537714 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] exception: Let MediaWiki.php control final output for ErrorPageError
https://gerrit.wikimedia.org/r/537714

Change 537714 merged by jenkins-bot:
[mediawiki/core@master] exception: Let MediaWiki.php control final output for ErrorPageError

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

Change 534933 merged by jenkins-bot:
[mediawiki/core@master] block: Allow cookie-block tracking from any uncached web request

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

Krinkle closed this task as Resolved.Oct 3 2019, 1:35 AM

@Krinkle After this change, on my local machine I am seeing more pages return Cache-Control: no-cache, no-store, max-age=0, must-revalidate (where before they would return something like private, must-revalidate, max-age=0).

However, I am not seeing this change on beta.

For example, going to https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Special:UserLogin (which should have the change) and https://test.wikipedia.org/w/index.php?title=Special:UserLogin (which should not) both return: cache-control: private, s-maxage=0, max-age=0, must-revalidate

Just checking that this is expected. Perhaps due to how the Varnish caches are setup?

Tchanders reopened this task as Open.Oct 7 2019, 4:41 PM

Just re-opening this until signed off by QA

@Krinkle After this change, on my local machine I am seeing more pages return Cache-Control: no-cache, no-store, max-age=0, must-revalidate (where before they would return something like private, must-revalidate, max-age=0).

Well-spotted. I did not realise the header would be different for the login use-case. Previously it went down the "maybe cachable bt private due to maxage" branch, but it now goes down the "not cacheable" branch. In OutputPage, these two are two subtly different branches. The only difference is that for the private branch we do allow re-use of the HTML payload after a server validation. This difference is important for article views. E.g. if you're a logged-in user and view an article twice in a short amount of time (and have not received notifications, changed preferences, and noone edited the article since) the browser will receive a 304-Not-Modified response and the browser will then re-use what it had cached. We don't want to go down the no-cache path there, as that would regress repeat view performance.

For the login page this isn't much of an issue, though. It's not critical in terms of repeat view perf, but worth optimising. I'll file a task for that.

However, I am not seeing this change on beta.
For example, going to https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Special:UserLogin (which should have the change) and https://test.wikipedia.org/w/index.php?title=Special:UserLogin (which should not) both return: cache-control: private, s-maxage=0, max-age=0, must-revalidate

Yeah in beta and in production the UseCdn flag is enabled. This isn't something you should test locally though, as enabling that also sets expectation on other things (in particular, it sets the expectation MediaWiki responses are proxied through something like Varnish where more stuff is set). One of the things that Varnish layer does it normalise all private responses to private, must-revalidate, max-age=0. Hence, this distinction is not visible in beta and prod. Not a blocking concern, but I'll file a follow-up task. We'll probably want enableClientCache() to be a tri-state flag instead of boolean, allowing callers to say "Make public", "Make private" or "Turn off". Right now it only has "Make public" or "Turn off".

Krinkle removed Krinkle as the assignee of this task.Oct 8 2019, 12:22 AM
Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Krinkle added a subscriber: Krinkle.

@Krinkle @Tchanders @dbarratt For logged in users with autoblocks against their accounts, I think there are a few places we are missing setting block cookies:

  • Special:AllMyUploads
  • Special:MyContributions
  • Special:MyLanguage
  • Special:MyTalk
  • Special:MyPage
  • action=raw

This is when I compare beta to test. I haven't tried before and after on the same environment.

For logged in users with autoblocks against their accounts

Is the block against the user or against the IP address? I was under the impression that logging in would bypass the autoblock (unless your user account is the target of the block).

For logged in users with autoblocks against their accounts

Is the block against the user or against the IP address? I was under the impression that logging in would bypass the autoblock (unless your user account is the target of the block).

Against the user.

It looks like instances of RedirectSpecialPage and ActionRaw are not getting the block cookies. For RedirectSpecialPage I'm not sure this is an issue because the user will get the cookies after the redirect is complete. For ActionRaw it looks like it sets its own headers rather than using OutputPage, perhaps a new task should be created for one or both of these issues.

I don't think either of these are a big deal, but I defer to @Niharika. :)

It looks like instances of RedirectSpecialPage and ActionRaw are not getting the block cookies. For RedirectSpecialPage I'm not sure this is an issue because the user will get the cookies after the redirect is complete...

Sorry, bit of miscommunication. The cookie is not set even after the redirect completes.

It looks like instances of RedirectSpecialPage and ActionRaw are not getting the block cookies. For RedirectSpecialPage I'm not sure this is an issue because the user will get the cookies after the redirect is complete...

Sorry, bit of miscommunication. The cookie is not set even after the redirect completes.

Good catch, @dom_walden. Let's file a task for that.

It looks like instances of RedirectSpecialPage and ActionRaw are not getting the block cookies. For RedirectSpecialPage I'm not sure this is an issue because the user will get the cookies after the redirect is complete. For ActionRaw it looks like it sets its own headers rather than using OutputPage, perhaps a new task should be created for one or both of these issues.
I don't think either of these are a big deal, but I defer to @Niharika. :)

For ActionRaw, I am inclined to leave it be unless it is a really trivial amount of work to set the cookies.

Comparing the headers returned by https://en.wikipedia.beta.wmflabs.org and http://test.wikipedia.org/ for as many pages as I could find (mostly special pages).

While logged out with a block against your IP, there are a number of new places where block cookies are set (non-exhaustive list):


Including in various error conditions, such as going to Special:ConfirmEmail while not logged in.

The block cookie gets set in all these places regardless of block parameters, including if it is a partial block. As pointed out by Thalia, if the block does not apply to creating accounts you will still get a block cookie when going to Special:CreateAccount. Similarly, if you are blocked from Page_A and try to edit Page_B, it will still set a block cookie (block won't be applied though).

While logged in with an autoblock against the user, cookie setting is more-or-less unchanged, with the exceptions in T233594#5555784 (and possibly more). These have been logged as bugs T235047 and T235050.

On my local environment (no Varnish caches like on beta/test) there are some changes to cache headers ("Cache-Control", "Pragma"), mainly when logged out, as mentioned in T233594#5552616. Affecting:

  • Special:UserLogin
  • Special:CreateAccount
  • Special:EmailUser
  • Editing
  • Page mentioned in T233594#5555784 (which return Cache-Control: no-cache)
  • Various error conditions (e.g. pages where you need to be logged in like Special:BotPassword)

We also modified BlockManager::trackBlockWithCookie which has caused us problems in the past (e.g. T226777 and T227901). Just in case, I attempted to reproduce the bugs in T227901 and T226777, but could not.

Niharika closed this task as Resolved.Oct 10 2019, 4:38 AM

We also modified BlockManager::trackBlockWithCookie which has caused us problems in the past (e.g. T226777 and T227901). Just in case, I attempted to reproduce the bugs in T227901 and T226777, but could not.

@dom_walden Should we close T227901: PHP warning on attempting to set a block cookie after CentralAuth auto login in that case?

@Niharika We should be able to close T227901 once all WMF sites are on 1.35.0-wmf.1. The logs will still be recording that warning until then.

@Niharika We should be able to close T227901 once all WMF sites are on 1.35.0-wmf.1. The logs will still be recording that warning until then.

Done.