Page MenuHomePhabricator

Clear block cookie when tracking block, not when checking block
Closed, ResolvedPublic

Description

Since T233594, a block can be tracked with a cookie from any uncached web request. However, the cookie is only removed when a user attempts to perform an action that triggers a block check. Problems with this:

  • Cookies are added and removed asymmetrically, meaning that they persist longer than they should. E.g. if a user is unblocked earlier than expected, their cookie will still persist even when making uncached web requests. (This will not currently have a damaging effect, since it will be removed if the user tried to do anything that a blocked user cannot do; however, it pollutes their cookies.)
  • In order to clear the cookie when the block is being checked, the BlockManager must access the response object from the user object. Ideally the user would not provide access to this; in fact, this is one of the few cases where access is needed, as discussed in T231930.

A solution to both of these problems would be to clear invalid block cookies at the point when block cookies are being set.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 23 2019, 10:02 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.

Doesn't the cookie expire anyways?

@dbarratt It will expire, but the block may be removed first.

See also T231930#5496232 for another reason to do this.

@Tchanders Is there a patch for this? (sorry I went to this in the opposite direction)

Change 537099 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Clear block cookie when tracking block, not when checking block

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

Follow up from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/537099/1/includes/block/BlockManager.php#467

@Niharika should block cookies be a rolling 24 hours or a should it be a hard 24 hours? Right now, the block cookie is set for 24 hours, and regardless of how many attempts you make, it will expire in 24 hours. With this change, if you make an uncached request to MediaWiki, the block cookie expiration will be reset.

Tchanders added a comment.EditedOct 8 2019, 10:53 PM

@Niharika should block cookies be a rolling 24 hours or a should it be a hard 24 hours? Right now, the block cookie is set for 24 hours, and regardless of how many attempts you make, it will expire in 24 hours. With this change, if you make an uncached request to MediaWiki, the block cookie expiration will be reset.

@Niharika Just to clarify, the cookie is only set once a user attempts to do something, not when the block is made. The two scenarios we are choosing between are:

  1. If two users are blocked at the same time, and one of them attempts to do something 1, 2 and then 3 hours later, but the other one only attempts to do something 3 hours later, the first user's cookie expires sooner (because the cookies expire 24 hours after the first attempt)
  2. If two users are blocked at the same time, and one of them attempts to do something 1, 2 and then 3 hours later, but the other one only attempts to do something 3 hours later, both cookies expire at the same time (because the cookies expire 24 hours after the latest attempt)

The current patch does #2.

Thanks for the explanation, @dbarratt and @Tchanders. Is there a reason we want to change the existing behavior (hard 24 hours expiry)?

Thanks for the explanation, @dbarratt and @Tchanders. Is there a reason we want to change the existing behavior (hard 24 hours expiry)?

I don't have a reason to change it.

Thanks for the explanation, @dbarratt and @Tchanders. Is there a reason we want to change the existing behavior (hard 24 hours expiry)?

I don't have a reason to change it.

I imagine the existing behavior is documented and known, both on our wikis and 3rd-party wikis. I'd refrain from changing it if we don't need to.

@Niharika I'd argue that the behaviour should be changed.

In the original tasks for cookie blocks (T5233, T153347, T152462), the length of 24 hours is discussed, but whether it's 24 hours from the first attempt or the latest attempt doesn't appear to be discussed. There seems to be a general consensus that cookie should be set for 24 hours once a user attempts to edit (or some other action), but nowhere does it seem to be argued that subsequent attempts should not reset the timer. I'd argue that by common sense, it would be expected to be reset on each attempt.

In the two examples I gave above, the first user's cookie expires earlier than the second user's cookie, even though their latest attempts to edit happened at the same time. In other words, the current behaviour arbitrarily rewards the first user for having also made earlier attempts.

@Tchanders Looking at this another way -

  • User A makes an attempt to edit every 6 hours, hoping their block has been lifted
  • User B makes an attempt once but doesn't try again for a couple days

Three days days later, User A is still blocked but User B is not. In this scenario, one could say that we are punishing User A for trying to repeatedly edit and rewarding User B for not attempting to edit for 24 hours. User A might be trying to edit in good faith but because the cookie is repeatedly reset, they can potentially be blocked for days/weeks just because they keep trying.

Yet another way to look at this would be that, as a whole, users would be blocked for much longer if we keep resetting the cookie but not if don't reset them after the first attempt. My gut instinct is to block them for lesser time if we can because it is possible the user has changed, like on a university computer. Or the user might have mended their ways.

@Niharika Thanks for providing the alternative perspective.

I think I'd still disagree with treating the first attempt as potentially malicious, but subsequent attempts as potentially collateral damage. To my mind, it would make more sense for all attempts to be treated the same; i.e. it seems odd that the cookie should "remember" whether any previous attempts were made.

But in the interest of moving forward let's update the patch to preserve the existing behaviour. Will also make the documentation clearer to aid any future conversations about this.

@Tchanders, I agree that all attempts should be treated as equal. My first thought when reading this ticket was that the cookie should be set when the block is and the edit/other attempts should not affect it at all. Is that possible to do?

@Niharika That's an interesting point.

We wouldn't be able to send a cookie on setting the block, since we wouldn't have access to the blocked user's response object at that point. However, we may be able to set a cookie expiry time to be some amount of time relative to the start of the block, since the timestamp when the block was created is stored in the database. We could file a separate task about that?

@Tchanders That sounds like it would be ideal. Let's file a ticket. I would prioritize it low at this time since we need to get on to CheckUser soon, but having this documented is desirable.

Tchanders updated the task description. (Show Details)Oct 9 2019, 1:35 PM

Change 537099 merged by jenkins-bot:
[mediawiki/core@master] Clear block cookie when tracking block, not when checking block

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

@Tchanders please confirm what I am seeing below:

  • If you are logged in with an autoblock against your username, when you log out the block cookie is immediately removed. Previously, the cookie would remain, even after you perform a blockable action (e.g. editing). This seems to be a regression.
  • If logged out with a block against your IP, when you log in the block cookie is immediately removed. Previously, it would only be removed when you attempt a blockable action. This is probably does not make a difference to block behaviour.

I notice that previously BlockManager::getBlockFromCookieValue() used shouldApplyCookieBlock() to decide whether to clear a cookie.

Now, trackBlockWithCookie() uses shouldTrackBlockWithCookie(), which has slightly different logic for deciding.

  • If you are logged in with an autoblock against your username, when you log out the block cookie is immediately removed. Previously, the cookie would remain, even after you perform a blockable action (e.g. editing). This seems to be a regression.

@dom_walden Agreed this is a regression caused by calling shouldTrackBlockWithCookie, as you point out. We can fix this by calling shouldApplyCookieBlock to decide whether to remove an existing cookie, rather than shouldTrackBlockWithCookie.

Adding the details for posterity...

For a block against a user account, shouldTrackBlockWithCookie returns:

!$isAnon && $wgCookieSetOnAutoblock && $block->isAutoblocking()

whereas shouldApplyCookieBlock returns:

$wgCookieSetOnAutoblock && $block->isAutoblocking()

Presumably this is because we only want to add a cookie if the user is logged into the blocked account, but once a cookie is added, we want to apply it to anyone. (I believe @dbarratt and @dmaza were more involved with implementing cookie blocks, so they may be able to confirm this.)

  • If logged out with a block against your IP, when you log in the block cookie is immediately removed. Previously, it would only be removed when you attempt a blockable action. This is probably does not make a difference to block behaviour.

This is intentional, and that's correct that it shouldn't affect block behaviour.

Change 543140 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/core@master] Ensure block cookie is not removed early if blocked user logs out

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

Change 543140 merged by jenkins-bot:
[mediawiki/core@master] Ensure block cookie is not removed early if blocked user logs out

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

  • If you are logged in with an autoblock against your username, when you log out the block cookie is immediately removed. Previously, the cookie would remain, even after you perform a blockable action (e.g. editing). This seems to be a regression.

This regression no longer happens. The block cookie remains (even after doing actions like edit).

I also tested that the block cookie is cleared in the cases of 1) block expiry and 2) block removed from the database.

Testing on https://test.wikipedia.org that:

Block cookies get removed when:

  • block expires
  • block is removed
  • cookie hash value is invalid
  • For IP blocks: when you login

and that this happens in more places (as per T233594).

Block cookies don't get removed when:

  • For autoblocks: logging out, removing session cookies or switching accounts
  • For IP blocks: switching IPs

Tested IP, range and autoblocks.

When you have an autoblock against your user and an IP block against your IP, when you log in the IP block cookie gets unset and the autoblock cookie gets set. The autoblock cookie remains set when you log out.

Tested potential issues with CentralAuth. With an autoblock on wiki A, when you log in on connected wiki B and refresh wiki A the autoblock cookie gets set. Did this locally so I could check error logs.

dbarratt closed this task as Resolved.Oct 17 2019, 4:29 PM