Page MenuHomePhabricator

Cannot suppress pages while deleting following change to page deletion interface
Closed, ResolvedPublic

Description

Prior to the change to the page deletion interface, oversighters had the option to suppress the page when deleting it. This option is not available on the new interface.

This is a significant problem when the page title itself is part of the policy violation that is leading to suppression.

Event Timeline

Risker created this task.Jul 23 2017, 4:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 23 2017, 4:37 PM
Risker triaged this task as High priority.Jul 23 2017, 4:47 PM
Ks0stm added a subscriber: Ks0stm.Jul 23 2017, 5:03 PM
Jalexander set Security to Software security bug.Jul 23 2017, 8:23 PM
Jalexander added a project: Security.
Jalexander changed the visibility from "Public (No Login Required)" to "Custom Policy".

Pulling this in to the security zone because the attack vector it exposes.

jrbs added a subscriber: jrbs.Jul 23 2017, 8:32 PM

Is this a regression?

@Jalexander: the workaround is to suppress the log event manually, no? Doesn't seem worth keeping private accordingly unless I am misunderstanding...

lfaraone merged a task: Restricted Task.Jul 23 2017, 9:53 PM
lfaraone removed subscribers: Liuxinyu970226, Jay8g, TerraCodes.

@Jalexander: the workaround is to suppress the log event manually, no? Doesn't seem worth keeping private accordingly unless I am misunderstanding..

Well. The workaround is complicated and fraught with opportunity for error, because it requires (a) suppressing all of the revisions of the page after deleting it and (b) hunting down and suppressing the log event. Both of these increase the complexity significantly. It is also difficult to ascertain that all of the instances of the title are suppressed that way - the previous iteration was supposed to do that, and I am not 100% certain that happens with suppressing the deletion log entry.

Should we be treating this as unbreak now then?

@jrbs wrt. regression. Seems so.

@lfaraone I'd say to keep it private until fixed given the potential abuse vector this could be. I'm quoting myself from the dupe task to signal the vulnerabilities I've identified:

After the latest MediaWiki update (or so it seems as the deletion form design has changed) the option to directly suppress pages have dissapeared. This doubles oversighters' work since then they'll have to suppress individual deletion log entries (leaving traces in logs and leaks of private data for those parsing IRC feeds, EventStreams, etc. where you cannot remove them afterwards) and revisions of the suppressed title so they ain't viewable to administrators either. Please restore the functionality to directly suppress (oversight) a page when deleting it. Thanks.

Although yes, the page title once it was created appeared on IRC/EventStreams.

@Legoktm UBN seems appropriate to me.

Legoktm claimed this task.Jul 23 2017, 10:13 PM
Legoktm raised the priority of this task from High to Unbreak Now!.

OK, I've figured out what's broken. Working on a patch now.

@Jalexander: the workaround is to suppress the log event manually, no? Doesn't seem worth keeping private accordingly unless I am misunderstanding...

Yeah, there are workarounds but they're not guaranteed and requires multiple steps. While I'll have no problem pulling it public again once fixed but until then I think it's better to be safe since it is easy to miss something or not realize you need to do the workaround especially on smaller wikis where it's likely someone will point out the error.

Should we be treating this as unbreak now then?

I know already answered/done but for the record, yes, I think so. Thanks for working on the patch

matmarex added a comment.EditedJul 23 2017, 10:41 PM

I can't test right now but code looks good. Issues/thoughts:

  • The change of getVal() to getCheck() at the beginning looks unrelated to the rest of the changes?
  • PHPCS complains about the double newline at the end of the patch.

I can't test right now but code looks good. Issues/thoughts:

  • The change of getVal() to getCheck() at the beginning looks unrelated to the rest of the changes?

The checkbox is being posted as &wpSuppress= so it has no value, meaning getVal() is falsey. So I switched it to getCheck().

  • PHPCS complains about the double newline at the end of the patch.

Fixed.

[16:04:56] <MatmaRex> legoktm: but ok, either way works. fine by me. ship it

Deploying it now...

Legoktm closed this task as Resolved.Jul 23 2017, 11:30 PM

Deployed and @Risker confirmed it works now.

Legoktm changed the visibility from "Custom Policy" to "Public (No Login Required)".Jul 23 2017, 11:30 PM

Thanks! I confirm it works as well.

Change 367339 merged by jenkins-bot:
[mediawiki/core@master] [SECURITY] Restore ability to suppress pages while deleting

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