Page MenuHomePhabricator

PageHistory.php deleterevision <form> breaks on ugly URL wikis
Closed, ResolvedPublic

Description

Big problems at PageHistory.php's

if( $this->linesonpage > 1 && $wgUser->isAllowed('deleterevision') ) {...

which makes
<form action="http://example.org/index.php?title=Special:Revisiondelete"

method="get" id="mw-history-revdeleteform"
style="visibility:hidden;float:right;">
  <input name="target" type="hidden" value="A" />
  <input name="oldid" type="hidden" value=""
  id="revdel-oldid" />
  <input type="submit"
  value="Show/hide selected revisions" />
</form>

Let's examine this one by one:

  1. action="http://example.org/index.php?title=Special:Revisiondelete"

This should just be action="/index.php". What is a "?" doing in
action? Maybe you fellows were just testing with "pretty URLs" wikis.

  1. You depend on Javascript to put the values into the form, as

apparently this is the only way you can deal with "dueling forms" here.

Why not combine the two forms into one? Just have a different <input
type="submit" ...> for the second.

I would be willing to write a patch, if you were willing to not insist
on Javascript.

P.S., I don't know what that style="visibility:hidden" stuff is hiding.

By the way,
the above test should be reversed, to

if( $wgUser->isAllowed('deleterevision') && $this->linesonpage > 1  ) {...

considering most views are from normal users, so quit early.

And here's the background of how I found the bug:

Noting SpecialRevisiondelete.php is the second largest special page,
$ ls -S specials|nl|sed 3q

1	SpecialUpload.php
2	SpecialRevisiondelete.php
3	SpecialSearch.php

I decided to give it a try.
We see in the HISTORY file

  • Further work on rev_deleted; changed to a bitfield with several data-hiding options. Not yet ready for production use; Special:Revisiondelete is incomplete, and the flags are not preserved across page deletion/undeletion. To try it; add the 'deleterevision' permission to a privileged group.

OK, we do
$wgGroupPermissions['sysop']['deleterevision']=true;
and proceed to browse the history of some page (with more than 1 revision),

http://example.org/index.php?title=A&action=history ... and the

rest is, well, history.


Version: 1.16.x
Severity: normal

Details

Reference
bz18827

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:39 PM
bzimport set Reference to bz18827.

mike.lifeguard+bugs wrote:

Could you please provide an accurate bug summary? Clearly the form /does/ work as written, since we're using it.

(In reply to comment #0)

Big problems at PageHistory.php's

if( $this->linesonpage > 1 && $wgUser->isAllowed('deleterevision') ) {...

which makes
<form action="http://example.org/index.php?title=Special:Revisiondelete"

method="get" id="mw-history-revdeleteform"
style="visibility:hidden;float:right;">
  <input name="target" type="hidden" value="A" />
  <input name="oldid" type="hidden" value=""
  id="revdel-oldid" />
  <input type="submit"
  value="Show/hide selected revisions" />
</form>

Let's examine this one by one:

  1. action="http://example.org/index.php?title=Special:Revisiondelete"

This should just be action="/index.php". What is a "?" doing in
action? Maybe you fellows were just testing with "pretty URLs" wikis.

POSTing to URLs with query strings works for me (haven't tested this particular form, but I've used it in other web apps).

  1. You depend on Javascript to put the values into the form, as

apparently this is the only way you can deal with "dueling forms" here.

Why not combine the two forms into one? Just have a different <input
type="submit" ...> for the second.

Yeah, this looks evil to me.

I would be willing to write a patch, if you were willing to not insist
on Javascript.

Nobody's insisting on anything. The fact that there's Javascript in that form doesn't automatically mean that everyone agrees with it and will defend its existence, as you seem to be suggesting. Go right ahead and write a patch, nobody's discouraging you.

P.S., I don't know what that style="visibility:hidden" stuff is hiding.

It seems to be hiding the form; a piece of Javascript is probably unhiding it though.

By the way,
the above test should be reversed, to

if( $wgUser->isAllowed('deleterevision') && $this->linesonpage > 1  ) {...

considering most views are from normal users, so quit early.

There's no real gain in that, since the $this->linesonpage > 1 check isn't slow. If anything, isAllowed() is probably slower.

The summary still doesn't look right to me: I'm pretty sure the form will work on ugly URL wikis (will test when I have a chance), but it seems the form is relying on Javascript while that's probably not necessary. Assigning to Aaron as RevisionDelete is his baby IIRC.

POSTing to URLs with query strings works for me

I'll believe it when I see it.
Re: visibility:hidden, and also bug 18674: just be sure to not
violate (the spirit too of)
http://en.wikipedia.org/wiki/Wikipedia:Accessibility
http://en.wikipedia.org/wiki/Wikipedia:HiddenStructure
even if only for administrators. OK, leaving this in
Aaron's hands.

  1. Works for me. Can anybody reproduce?
  2. One thing in one bug please!

The form works with ugly URLs since r51279, but other issues are still not fixed.

Tim removed the JS dependencies.