Here is my WIP patch that I need to hand off. It seems to be basically working. I tried to generate some tests with OpenCode and wasn't able to complete this, so the tests are failing, and I haven't closely reviewed the generated tests. I think some of them are failing because of the change itself, not only the new test that I've written. Please feel free to scrap the generated tests if they aren't any good.

Note that in order to link to the AccountRecovery page from the email auth challenge page, the EmailAuth en.json should be updated with this:

	"emailauth-login-help": "If you are having trouble receiving the code, you can [[Special:AccountRecovery|recover your account]].",

+	"emailauth-accountrecovery-no-emailauth": "Please try to [[Special:UserLogin | log in]] or [[Special:PasswordReset | reset your password]] before attempting account recovery.",

Generally we don't have leading/trailing whitespace in wiki links, so that's a minor nit that would need fixing too.

From discussions, this has the side effect that we confirm that the person requesting it know the password.

From discussions, this has the side effect that we confirm that the person requesting it know the password.

At least for me, I'd say that is the primary effect. Another effect that is valuable is that we would also get an IP address (and perhaps JA3/4 fingerprints?) on record for that EmailAuth session, which could be compared to information from past sessions known to be associated with the "real" user.

Here is a new patch. Tests are passing, and the commit message includes testing instructions. Would be great to get some code review. Perhaps @sbassett or @Reedy?

I'm a little bit concerned with the size of the patch, given that we do not want to push this through Gerrit. Do we think this is too big for a security patch, or are we comfortable with this size? Note that a significant number of changed lines are in the tests.

% git diff --stat HEAD^..
 extension.json                                     |  3 +-
 i18n/en.json                                       |  1 +
 .../Special/SpecialAccountRecovery.php             | 38 +++++++++-
 .../EmailAuthSecondaryAuthenticationProvider.php   |  1 +
 ...mailAuthSecondaryAuthenticationProviderTest.php | 17 +++++
 .../Integration/SpecialAccountRecoveryTest.php     | 88 ++++++++++++++++++++++
 6 files changed, 145 insertions(+), 3 deletions(-)

It's not a bundled extension, so there is no need to keep it embargoed for that purpose. And I suspect few to no third parties probably use this part of EmailAuth anyway as it's a bit WMF specific.

Roan's message (linked above) was

No please do not put this on Gerrit for now
I would like us to put our legal and communication obligations to bed before anything like this becomes public

AFAIK, both of those are done.

It could be enough that we can just push it through gerrit now, or possibly if we want to keep a bit more caution, push it as a security patch, do some double testing in prod, and when we're happy, push it through gerrit within the next week or so as a followup so it doesn't sit as a security patch for an extended period.

I don't think it's too big, but in many cases, it depends what repo it's in, how many files it changes, complexity of the changes... And basically, "how likely is this to cause other failures" (ie stuff not tested directly in this repo alone) and "how likely is it to cause merge conflicts of the patch in the near future".

And certainly for a WMF deployed security patch in the deployment, the tests are mostly useless when it's not going through our CI, so could be split to a seperate patch if it was a major concern.

I'll try and test it on Friday at some point.

I don't see why this and the other related patches cannot just go through gerrit as code hardening efforts. The incident is resolved, the account recovery process is still disabled and we will be putting in place a much stronger account recovery process early next week.

As discussed at today's clinic, @Catrope et al are fine with this going through gerrit now, as we should not be blocked on any communications issues.

Perfect, will do 👍

Patch is ready here - https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EmailAuth/+/1289409

Change #1289409 had a related patch set uploaded (by Alex.sanford; author: Alex.sanford):

[mediawiki/extensions/EmailAuth@master] Update Special:AccountRecovery to ensure there is an active emailauth challenge

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

Change #1289409 merged by jenkins-bot:

[mediawiki/extensions/EmailAuth@master] Update Special:AccountRecovery to ensure there is an active emailauth challenge

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

I agree the page isn't the best, but it doesn't direct people:

If you have lost access to your Wikimedia account, the simplest solution is to create a new account and link to your old one on its user page. If you would like to regain access, you can request help from the Wikimedia Foundation's Trust and Safety team. This process uses Zendesk, a Help Center platform.

And then has a seemingly outwardly unrelated section of "Using the Account Recovery form"

But yes, the documentation should be updated.

"request help from the Wikimedia Foundation's Trust and Safety team" - if user see Help:Account_recovery page they will not know other ways to contact it other than the Account Recovery form. Also if we decide to narrow the scope of the help page, remind other on-wiki pages may link to it (or link to Special:AccountRecovery directly).

Account recovery page is now linked from https://foundation.wikimedia.org/wiki/Legal:Wikimedia_Foundation_Legal_and_Safety_Contact_Information#Security which will be linked from footer of every Wikimedia site.