Page MenuHomePhabricator

Misleading warning raised by the login API: "Fetching a token via action=login is deprecated"
Closed, ResolvedPublic

Description

Hello everyone!

I noticed that some people is seeing this warning in various occasions:

Fetching a token via "action=login" is deprecated. Use "action=query&meta=tokens&type=login" instead.

See for example:

The problem about this warning, is that it's quite misleading when it's actually raised during this API query:

action=login
lgname=ItwikiBot@mark_admins
lgpassword=a_very_long_bot_password
lgtoken=a_token_obtained_from_api_token_ending_with_+\

With the username and password generated from [[Special:BotPasswords]].

If it can help, note that the token was obtained with such request:

action=query
meta=tokens
type=login

Now, I've also seen a discussion in T137805: API action=login is deprecated and it seems that some developers already know that this warning is raised in some circumstances:

That's an unrelated warning.
@Anomie in T137805#2786123 - Nov 10 2016, 5:19 PM

So. Why action=login gives a warning about Fetching a token when you are already providing that token and anyway when the API call is not deprecated?

This is even more confusing, because AFAIK action=login is only deprecated for main account login and not for the bot passwords authentication.


P.S.

I forgot to say that this happens when the login fails and the server replies with:

{"login":{
  "result":"NeedToken",
  "token":"that-token+\"
}

Anyway the bot password is correct.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 6 2020, 5:49 PM
valerio.bozzolan renamed this task from ApiLogin.php gives a Misleading API warning on action=login to Misleading warning raised by the login API: "Fetching a token via action=login is deprecated".Apr 6 2020, 6:06 PM
valerio.bozzolan updated the task description. (Show Details)
valerio.bozzolan added a subscriber: Anomie.
Restricted Application added a project: Core Platform Team. · View Herald TranscriptApr 6 2020, 6:06 PM
Anomie moved this task from Unsorted to Needs Review on the MediaWiki-API board.Apr 6 2020, 9:30 PM

The "misleading" case occurs when the session is lost, resulting in a new token being created. While this is historical behavior, it's probably about time to change it. It's much more likely that the client is having a bug in cookie handling than that there was a transient session loss on the server side.

Change 586448 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] api: Report Failed rather than NeedToken on session loss for action=login

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

This issue has broken logins on VicuñaUploader, which still uses the deprecated method. The last upload from the tool was at 2020-04-05 21:27:08 UTC.

valerio.bozzolan added a comment.EditedApr 7 2020, 6:59 AM

Thank you @Anomie,

You mentioned the cookies. As far I can see when requesting action=query&meta=tokens&type=login the server returns these cookies:

set-cookie: itwikiSession=asdasd; path=/; secure; HttpOnly
set-cookie: forceHTTPS=true; path=/; HttpOnly
Set-Cookie: WMF-Last-Access=07-Apr-2020;Path=/;HttpOnly;secure;Expires=Sat, 09 May 2020 00:00:00 GMT
Set-Cookie: WMF-Last-Access-Global=07-Apr-2020;Path=/;Domain=.wikipedia.org;HttpOnly;secure;Expires=Sat, 09 May 2020 00:00:00 GMT
Set-Cookie: GeoIP=IT:21:Turin:1.2:3.4:v4; Path=/; secure; Domain=.wikipedia.org

Having said I'm not 100.00% sure about my inspection - I think that one of my problems is that the server was replying some headers with "not-canonical" fields. See the set-cookie instead of the traditional Set-Cookie. This is maybe weird but legitimate as per RFC2616 so it was a client code fault.

What do you think about that "not-canonical" set-cookie? Can you reproduce it?

4nn1l2 added a subscriber: 4nn1l2.Apr 7 2020, 11:04 AM
daniel assigned this task to Anomie.Apr 7 2020, 12:50 PM
daniel triaged this task as Medium priority.
CDanis added a subscriber: CDanis.Apr 7 2020, 3:49 PM

(...)
Having said I'm not 100.00% sure about my inspection - I think that one of my problems is that the server was replying some headers with "not-canonical" fields. See the set-cookie instead of the traditional Set-Cookie. This is maybe weird but legitimate as per RFC2616 so it was a client code fault.

What do you think about that "not-canonical" set-cookie? Can you reproduce it?

I faced this issue at T224712#6026486, my client is not prepared to handle set-cookie fields (it expects Set-Cookie instead) and thus fails to log in. By the way, letter case inconsistency affects several header fields (stringified Java Map with List values):

{date=[Tue, 07 Apr 2020 13:40:32 GMT],
null=[HTTP/1.1 200 OK],
X-Cache=[cp3054 miss, cp3060 pass],
server=[mw1400.eqiad.wmnet],
vary=[Accept-Encoding],
Server-Timing=[cache;desc="pass"],
x-frame-options=[DENY],
Connection=[keep-alive],
p3p=[CP="See https://pl.wiktionary.org/wiki/Special:CentralAutoLogin/P3P for more info."],
X-Client-IP=[<...>],
Accept-Ranges=[bytes],
set-cookie=[forceHTTPS=true; path=/; HttpOnly, plwiktionarySession=<...>; path=/; secure; HttpOnly],
Strict-Transport-Security=[max-age=106384710; includeSubDomains; preload],
X-Cache-Status=[pass],
content-disposition=[inline; filename=api-result.xml],
x-content-type-options=[nosniff],
Content-Encoding=[gzip],
Set-Cookie=[GeoIP=<...>; Path=/; secure; Domain=.wiktionary.org, WMF-Last-Access-Global=07-Apr-2020;Path=/;Domain=.wiktionary.org;HttpOnly;secure;Expires=Sat, 09 May 2020 12:00:00 GMT, WMF-Last-Access=07-Apr-2020;Path=/;HttpOnly;secure;Expires=Sat, 09 May 2020 12:00:00 GMT],
content-type=[text/xml; charset=utf-8],
Content-Length=[134],
cache-control=[private, must-revalidate, max-age=0],
Age=[0]}
Anomie added a comment.Apr 7 2020, 4:22 PM

This issue has broken logins on VicuñaUploader, which still uses the deprecated method. The last upload from the tool was at 2020-04-05 21:27:08 UTC.

This issue can't have broken anything. This task is requesting a change, not reporting on something that broke.

What do you think about that "not-canonical" set-cookie? Can you reproduce it?

Per the RFCs, header names are case-insensitive. If some client code is expecting a particular case, that client code is broken.

The cookie issue caused VicunaUploader to break as well. Rewriting the header using mitmproxy made it work again.

Anomie added a comment.Apr 7 2020, 4:57 PM

This task is about the API reporting NeedToken when a token was supplied but the session was lost. Session loss may be caused by cookie handling issues, but this task isn't about that.

Juandev added a subscriber: Juandev.Apr 7 2020, 8:42 PM
Skim added a subscriber: Skim.Apr 7 2020, 11:37 PM
Skim added a comment.Apr 8 2020, 12:35 AM

I fixed VicunaUploader by this commit: https://github.com/michal-josef-spacek/vicuna/commit/c4640dda4ad8b99bd88d38547afc9d7e27c3ccbd
Problem was with case-insensitive parsing of Set-Cookie.

Pintoch added a subscriber: Pintoch.EditedApr 8 2020, 8:22 AM

The fix for this issue might also be the cause for T249680, Wikidata-Toolkit not being able to log in anymore.

daniel added a subscriber: daniel.Apr 8 2020, 9:32 AM

The fix for this issue might also be the cause for T249680, Wikidata-Toolkit not being able to log in anymore.

Since the fix isn't merged yet, it can't be the cause.

Change 586448 merged by jenkins-bot:
[mediawiki/core@master] api: Report Failed rather than NeedToken on session loss for action=login

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

daniel closed this task as Resolved.Apr 8 2020, 11:26 AM

Per the RFCs, header names are case-insensitive. If some client code is expecting a particular case, that client code is broken.

Having said I 100% agree with your sentence I just asked if—from the MediaWiki perspective—it's that good to adopt not-canonical header fields and... oh, brilliant! It seems the right occasion to quote the holy Postel's law:

[…] be conservative in what you do, be liberal in what you accept from others.

That's why—having said that it's 100% client side fault—I still think that the server should not act like this.

@valerio.bozzolan I suggest you move this comment to T249680 instead, as this ticket is about something else.

Anomie added a comment.Apr 8 2020, 4:08 PM

@valerio.bozzolan: FYI, in HTTP/2 "set-cookie" is the canonical casing of the header field, and it turns out the change is the result of an upgrade to new software using the HTTP/2 semantics. More details are available in T249680: Clients failing API login due to dependence on "Set-Cookie" header name casing.

Reedy added a subscriber: Reedy.Apr 9 2020, 12:22 PM

Change 586448 merged by jenkins-bot:
[mediawiki/core@master] api: Report Failed rather than NeedToken on session loss for action=login

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

Should we be backporting this?

Anomie added a comment.Apr 9 2020, 1:15 PM

Should we be backporting this?

To 1.35.0-wmf.27, or to previous releases, or both?

The main effect it'll have is that clients running into issues like T249680 or T145545 will be more likely to just report an error rather than winding up looping because they're no more likely to pick up the correct session cookies that they missed the first time around. OTOH, if there's an actual server-side session loss it'll also make clients more likely to report an error rather than retrying with the new session.

Unless we're getting hammered by looping clients from T249680, I see no particular reason to backport it to wmf.27 instead of letting it ride the wmf.28 train. I have no reason to recommend for or against backporting it to previous releases either.

Reedy added a comment.Apr 9 2020, 1:49 PM

Yeah, I was meaning release branches. Thanks for clarifying!