SpecialUserLogin extends LoginSignupSpecialPage... SpecialPasswordReset extends FormSpecialPage

LoginSignupSpecialPage calls OutputPage::disallowUserJs... SpecialPasswordReset and FormSpecialPage don't.

Is this good for now?

In T192134#4293721, @Ladsgroup wrote:

Is this good for now?

0001-Do-not-allow-user-scripts-on-Special-PasswordReset.patch877 BDownload

Maybe I should call that method in alterForm instead of construct. Can't say for sure tbh.

ping @Bawolff

@Ladsgroup - patch above still applies to MW 1.35. I'm personally fine with this as a stop-gap measure or even if it becomes more of a permanent mitigation. Though I'm not really sure there's a demonstrated security issue here. Yes, certain user css/js can do unintentional or bad things on wiki pages where it's run, but that's a fundamental part of such functionality existing in the first place. Anyhow, I'm happy to deploy this as a core security patch and backport it to supported release branches, though IMO, it might barely meet the security patch threshold.

In T192134#5682990, @sbassett wrote:

@Ladsgroup - patch above still applies to MW 1.35. I'm personally fine with this as a stop-gap measure or even if it becomes more of a permanent mitigation. Though I'm not really sure there's a demonstrated security issue here. Yes, certain user css/js can do unintentional or bad things on wiki pages where it's run, but that's a fundamental part of such functionality existing in the first place. Anyhow, I'm happy to deploy this as a core security patch and backport it to supported release branches, though IMO, it might barely meet the security patch threshold.

So the biggest part of the problem is that people can steal others' email addresses that is considered private data. It's not as bad as stealing their passwords (that's what I initially thought, it's PasswordReset) but I realized it's email address only. I tested it locally and it works.

In T192134#4321709, @Ladsgroup wrote:

Maybe I should call that method in alterForm instead of construct. Can't say for sure tbh.

[[ https://codesearch.wmflabs.org/core/?q=disallowUserJs%5C(%5C)&i=nope&files=&repos= | Most other specials seem to call it in execute() ]] though SpecialPasswordReset.php doesn't call execute() right now. I'm not sure there's anything wrong with calling disallowUserJs() within the constructor, though I'm sure that could be modified to look more like what is done in SpecialChangeEmail for consistency's sake. Anyhow, whatever you want to do, I'd like to try to deploy this during the security deployment window this Monday, if possible.

In T192134#5719482, @sbassett wrote:

In T192134#4321709, @Ladsgroup wrote:

Maybe I should call that method in alterForm instead of construct. Can't say for sure tbh.

[[ https://codesearch.wmflabs.org/core/?q=disallowUserJs%5C(%5C)&i=nope&files=&repos= | Most other specials seem to call it in execute() ]] though SpecialPasswordReset.php doesn't call execute() right now. I'm not sure there's anything wrong with calling disallowUserJs() within the constructor, though I'm sure that could be modified to look more like what is done in SpecialChangeEmail for consistency's sake. Anyhow, whatever you want to do, I'd like to try to deploy this during the security deployment window this Monday, if possible.

Okay, I made another patch that makes me feel better (I was worried that construct would have been ran in other contexts like in Special:SpecialPages)

Thanks, @Ladsgroup. Patch applies and tests fine locally on MW master. Will deploy shortly.

Change 556073 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@master] Do not allow user scripts on Special:PasswordReset

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

Change 556074 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@REL1_34] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556075 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@REL1_33] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556076 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@REL1_32] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556077 had a related patch set uploaded (by SBassett; owner: Ladsgroup):
[mediawiki/core@REL1_31] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556077 merged by jenkins-bot:
[mediawiki/core@REL1_31] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556076 merged by jenkins-bot:
[mediawiki/core@REL1_32] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556075 merged by jenkins-bot:
[mediawiki/core@REL1_33] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556074 merged by jenkins-bot:
[mediawiki/core@REL1_34] SECURITY: Do not allow user scripts on Special:PasswordReset

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

Change 556073 merged by jenkins-bot:
[mediawiki/core@master] SECURITY: Do not allow user scripts on Special:PasswordReset

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