r78585 MediaWiki - Code Review archive

• For instance, a user might

* revert mass vandalism from a user by loading their contribs and
* ctrl+clicking each rollback link. If we cleared the Token from
* session after each rollback, they'd have to reload the contribs
* page each time, which would be annoying. 

This is a quite bad example, since the rollback links are prefixed with the page name and the rolledback user precisely to make them unique so that they only work on that exact page. There's not point on keeping them. They are for using once and discard. I also miss a mention on why those kind of tokens must be discarded (since they are GET request, they get logged in proxys and are it's thus unsafe to reuse them).

+	/**
+	 * String the action which is being protected by the token 
+	 * ('edit', 'login', 'rollback', etc)
+	 */
+	protected $type = self::ANONYMOUS;

There seems to be a mixture between actions and security levels. From reading the code, $type identifies the security level, and all 'login', 'rollback', etc. would be of type PERSISTENT. The comment should be fixed.

+ global $wgUser; Would be nice if the user was handled similar to Token::request. Make the user the third parameter and move $request to the fourth.

I'd expect that a UNIQUE token would be automatically cleared once it matches.

A user should be able to load two different Unique actions on different tabs and perform both.

This is a quite bad example, since the rollback links are prefixed with the page name and the rolledback user precisely to make them unique so that they only work on that exact page. There's not point on keeping them.

But for all the rollback links on the page, there is only one key stored in the session, in both the old and new systems. It's salted in different ways for each instance so that they're all unique, but for each link it's retrieving the same token from the session, and comparing it in different ways. If you cleared the session token after the first rollback, all the other rollback links would stop working because there was no session value to retrieve.

They are for using once and discard. I also miss a mention on why those kind of tokens must be discarded (since they are GET request, they get logged in proxys and are it's thus unsafe to reuse them).

The part of the token which is sent in the GET request is the non-session part; it's the bit which would be set in a hidden field in a POSTed form. When the user clicks on the rollback link, the checker compares the token in the GET to token-in-session-hashed-with-the-unique-salt.

There seems to be a mixture between actions and security levels. From reading the code, $type identifies the security level, and all 'login', 'rollback', etc. would be of type PERSISTENT.

I made a change partway through to three different types of tokens rather than a different type for each and every action.

+ global $wgUser; Would be nice if the user was handled similar to Token::request. Make the user the third parameter and move $request to the fourth.

You can't really get tokens for anyone other than the current user, because you can only access the current user's session. Is there anywhere in code where this would be needed?

I'd expect that a UNIQUE token would be automatically cleared once it matches.

Good idea.

A user should be able to load two different Unique actions on different tabs and perform both.

That could be a challenge... Might need different session elements for each UNIQUE token; maybe based on the salt...?

But for all the rollback links on the page, there is only one key stored in the session, in both the old and new systems. It's salted in different ways for each instance so that they're all unique, but for each link it's retrieving the same token from the session, and comparing it in different ways. If you cleared the session token after the first rollback, all the other rollback links would stop working because there was no session value to retrieve.

You are right. The relationship between token instances as seen from the user and Token objects is not injective. Maybe we could rename the token class to avoid confusing them?

Also, prepare() is a three items singleton since all Token objects with the same type are equivalent. So you create several token objects but they are magically the same (that bit must be kept into account regarding clearing). And if you want to use the same master token with a different salt, you must create a new object. This doesn't seem a good design. The salt could be a member parameter, or there could be a master token class, which generates token instances which then have different salts...

You can't really get tokens for anyone other than the current user, because you can only access the current user's session. Is there anywhere in code where this would be needed?

The objective was to reduce usage of globals in new code.

That could be a challenge... Might need different session elements for each UNIQUE token; maybe based on the salt...?

There could be a class in the session which generates new tokens based on the required security and tracks the issued ones.

Or we could scrap all of this. Can you make a list of the 'problems it tries to solve'? There are some shortcoming with the current approach (eg. actions needing a token even for anons), but so far it has worked quite well. This may be making things too much complex.

You are right. The relationship between token instances as seen from the user and Token objects is not injective. Maybe we could rename the token class to avoid confusing them?

I don't entirely understand what you're saying here. Are you saying that the implementation is different between the old and new, or between two facets of the new? Why do you think a name change would help?

And if you want to use the same master token with a different salt, you must create a new object. This doesn't seem a good design. The salt could be a member parameter, or there could be a master token class, which generates token instances which then have different salts...

But where do we ever need to do this? We don't get multiple tokens at the same level in the callstack; either we only need one token for the page, or we need to get one token in the course of preparing one object, which we may do multiple times. There's no need to be able to reuse Token objects. Or am I missing a usecase?

The objective was to reduce usage of globals in new code.

But it doesn't reduce the usage of globals at all, because you either have to get $wgUser in the constructor, or get $wgUser in every caller and pass it to the constructor. I don't see how that has practical advantages.

Or we could scrap all of this. Can you make a list of the 'problems it tries to solve'? There are some shortcoming with the current approach (eg. actions needing a token even for anons), but so far it has worked quite well. This may be making things too much complex.

Primarily it's a refactoring to try and improve readability; but it should also make it clearer when and where tokens are required, and how sensitive they are. There is a security consideration in that many tokens are currently unsalted: if you sniff a copy of an admin's Special:MovePage form, you can use the same token to CSRF a Special:BlockIP form, as they're both unsalted. And virtually no extensions use tokens at all.

I'm genuinely unsure about whether the best approach is the $type system in there now, or the $action method (different session tokens for 'rollback', 'login', etc) that I had before; thoughts are welcome. Tim's closure of the CSRF in Special:UserLogin indicated that it's valuable for the login token not to be the same as the edit token, but I don't know whether it's better to extend that to having different tokens for each action, or to keep the session a bit cleaner by not having so many different tokens.

Putting an object in the session to act as a 'token jar' might work...

For me the token is the aabbccddee... string sent to the user. When making a list of rollback links, you are creating several tokens.

With $tokenJar->generate( $salt ) I think it's clearer to see that you are creating a token from a given source.

than noting that (new Token( Token::PERSISTENT, 'Main Page|Happy-melon') )->clean(); will also delete new Token( Token::PERSISTENT, 'WP:RFA|Jimmy Wales')

I'm genuinely unsure about whether the best approach is the $type system in there now, or the $action method (different session tokens for 'rollback', 'login', etc) that I had before; thoughts are welcome. Tim's closure of the CSRF in Special:UserLogin indicated that it's valuable for the login token not to be the same as the edit token, but I don't know whether it's better to extend that to having different tokens for each action, or to keep the session a bit cleaner by not having so many different tokens.

Note that for Special:UserLogin, $wgUser->editToken() wouldn't have worked since it would always return +\

I don't see where you are getting that indication from r64677. Changing tokens for each action is indeed a bit more secure, but try not to be obnoxious. I'm sure it will break bots and scripts, so too much different tokens (eg. onj moving: one per source page) wouldn't be appropiate.

For me the token is the aabbccddee... string sent to the user. When making a list of rollback links, you are creating several tokens.

I think that way of looking at it is a mistake, and masks the whole point of the tokens which is that they are in two parts, one in the session and one in the HTML request.

Currently if you get given a rollback link with &token=f0ObAr, that's because you have a session token "baZqu0k", and hashing that session token together with the title of the page and the user to rollback gives you the HTML token. Currently there is no method to clear the session part of the token, because that would invalidate pretty much every token the user has, for rollback, block, delete, etc; login is the only exception. But without that invalidation, the token &token=f0ObAr can still be successfully used to CSRF a rollback to the same page from the same user for as long as the user stays logged in, which is obviously undesirable. But it's a mistake to only think about the HTML part of the token, because you can't "invalidate" that; the way you 'invalidate' a token is to change or remove the session component of it, which invalidates all the HTML tokens using it (which is currently everything).

A solution would be to switch to a one-to-one mapping between session tokens and HTML tokens - so every rollback link generated would store a new session token. That doesn't change the behaviour from the perspective of bots and scripts, since they already need to ask for the HTML token for every individual rollback they perform. Since the session data is stored on the server, not the client, end-users wouldn't see any change at all. But on the server side we'd need to have a TokenJar to garbage-collect unused tokens after a timeout, because they wouldn't be cleared unless they were 'used'.

Essentially there are two elements to this: making sure that each HTML token given out is unique, which is done by salting correctly; and making HTML tokens single-use, which is done by having separate corresponding session tokens and clearing them after use. Which do you think are important?

I think that way of looking at it is a mistake, and masks the whole point of the tokens which is that they are in two parts, one in the session and one in the HTML request.

Only calling 'f0ObAr' a token just means that I want to call 'baZqu0k' something different: Master token, TokenGenerator. token jar...

I agree with your following paragraph. Note that the rollback issue can be fixed with a minor funcionality change by adding the revision id to the salt, but it's not so easy for other forms. Basically everything which the user can change in the final form (eg. the target user to block), means that we can't use it for token salting.

By breaking scripts I wasn't refering rollback links. I doubt any change on them them will break any script.

We could easily make tokens expire by adding them in the objectache prefixed by the userid and a token prefix. Now, what would be a sensible time for that?

Breaks the ApiWatchTest::testWatchRollback phpunit test: Received error badtoken

Hmm, $wgUser->editToken() is called in ApiMain::setupModule(), the same way for all modules. So it wouldn't be (easily) possible to separate rollback from the other token-controlled actions in the API and have some on the new system and some on the old.

Reverted rollback implementation in r78631.