Page MenuHomePhabricator

[Regression] revision delete bitfield get lost on page deletion
Closed, ResolvedPublic

Description

https://gerrit.wikimedia.org/r/#/c/305627/3 refactored the page delete, but in that case the revision delete bitfield is not correctly transfered to the archive table and get lost. This allows admins to delete pages and see revision deleted content or suppressed deleted content (oversight)

In sql strict mode this fails with
1366 Incorrect integer value: 'rev_deleted' for column 'ar_deleted' at row 1 (localhost)

but in non strict mode the text in the column is valid and gets converted to a 0, which means no revision delete

The patch set is already live on wmf/1.28.0-wmf.17, so this is created as security task. Please mark this as blocker for the today deployment. Thanks

Details

Related Gerrit Patches:
mediawiki/core : masterFix deletion handling of rev_deleted
mediawiki/core : wmf/1.28.0-wmf.17Fix deletion handling of rev_deleted

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 1 2016, 11:38 AM
aaron added a comment.EditedSep 1 2016, 12:45 PM

I just made a patch above to deal with that.

Thanks for the report!

aaron added a subscriber: Bawolff.Sep 1 2016, 12:46 PM
hashar added a subscriber: hashar.Sep 1 2016, 1:08 PM

@aaron poked me about it. I am not comfortable with that patch and I am really rusty with MW developement

Namely a select field is passed:

[ 'rev_deleted' => $bitfield ]

With $bitfield set above to either rev_deleted or an integer. So I am very confused about it.

aaron added a comment.Sep 1 2016, 1:19 PM

In the above patch, the syntax for the DB layer is [ alias => column]. If alias=column, then just column is used. So it's either:

  • rev_deleted
  • 15 AS rev_deleted

The later is for suppression.

I just tested round tripping this (delete/restore) and it looks fine.

hashar added a comment.Sep 1 2016, 1:22 PM

[alias => column] for column AS alias. Yeah I guess that is what confused me :]

hashar added a comment.EditedSep 1 2016, 1:29 PM

I have put patch on tin in /srv/patches/1.28.0-wmf.17/core/04-T144484.patch and applied it to 1.28.0-wmf.17.

synced after aaron reviewed the state on tin.

13:34:23 !log hashar@tin Synchronized php-1.28.0-wmf.17/includes/page/WikiPage.php: T144484 (duration: 00m 49s)
hashar assigned this task to aaron.Sep 1 2016, 1:56 PM

Aaron did the patch and it is deployed. Removing this task from 1.28.0-wmf.17 blockers.

aaron added a comment.Sep 2 2016, 7:04 PM

Master patch is https://gerrit.wikimedia.org/r/#/c/308220/

wmf17 (for anyone using out branch) is publicly at https://gerrit.wikimedia.org/r/#/c/308222/2

aaron changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 2 2016, 7:27 PM
Restricted Application added a subscriber: JEumerus. · View Herald TranscriptSep 2 2016, 7:27 PM

Change 308220 merged by jenkins-bot:
Fix deletion handling of rev_deleted

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

Change 308222 merged by jenkins-bot:
Fix deletion handling of rev_deleted

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

aaron closed this task as Resolved.Sep 3 2016, 11:00 AM