Page MenuHomePhabricator

TitlesMultiselectWidget for Partial blocks suggests and accepts special pages and other invalid namespaces
Closed, ResolvedPublic3 Story Points

Description

Problem: The 'Pages' text input box suggests and accepts special pages (e.g. Special:RecentChanges) or Media namespace

These are rejected on submission, but this is bad design.


Acceptance criteria

  • The error messages should continue to appear if the admin somehow manages to get these through
  • The 'Pages' input field should not suggest Special pages
  • The 'Pages' input field should not accept Special pages

Notes: Some of these are redirects or "junk" pages on Test Wiki. Might not be a big problem on other wikis.

Event Timeline

Restricted Application added subscribers: MGChecker, Aklapper. ยท View Herald TranscriptOct 30 2018, 8:47 PM
TBolliger renamed this task from Partial block page input field should not allow Special pages and redlink pages to TitlesMultiselectWidget for Partial blocks should not suggest or accept Special pages and redlink pages.Nov 1 2018, 6:16 PM
TBolliger triaged this task as Normal priority.
TBolliger renamed this task from TitlesMultiselectWidget for Partial blocks should not suggest or accept Special pages and redlink pages to TitlesMultiselectWidget for Partial blocks has several usability problems .Nov 1 2018, 6:29 PM
TBolliger updated the task description. (Show Details)
TBolliger updated the task description. (Show Details)
TBolliger updated the task description. (Show Details)Nov 1 2018, 6:33 PM
dbarratt edited projects, added MediaWiki-General; removed OOUI.Nov 2 2018, 4:19 PM
dbarratt added a subscriber: dbarratt.

The TitlesMultiselectWidget is part of MediaWiki, not part of OOUI

TBolliger updated the task description. (Show Details)Nov 2 2018, 6:53 PM

All of these seem things to be fixed (made parametrizable) at TitlesMultiselectWidget level.

Regarding issue #1 note that it not only happens with Special: pages, but also with Media: I expect that if there are other similar non-editable namespaces or models it would fail similarly, too.

TBolliger renamed this task from TitlesMultiselectWidget for Partial blocks has several usability problems to TitlesMultiselectWidget for Partial blocks suggests and accepts special pages and other invalid namespaces.Nov 2 2018, 8:58 PM
TBolliger updated the task description. (Show Details)

All of these seem things to be fixed (made parametrizable) at TitlesMultiselectWidget level.
Regarding issue #1 note that it not only happens with Special: pages, but also with Media: I expect that if there are other similar non-editable namespaces or models it would fail similarly, too.


Good find. I've split this task into the 4 different issues and added your findings to this ticket. Thank you!

TBolliger lowered the priority of this task from Normal to Low.Nov 2 2018, 9:00 PM
TBolliger updated the task description. (Show Details)
TBolliger moved this task from Cards ready to be discussed to Snackbox on the Anti-Harassment board.

This seems to be a problem with bad data in testwiki's database rather than an issue with the widget.

TBolliger moved this task from Snackbox to Blocked on the Anti-Harassment board.Nov 15 2018, 11:39 PM
TBolliger raised the priority of this task from Low to Normal.Jan 30 2019, 11:17 PM
TBolliger moved this task from Blocked to Cards ready to be discussed on the Anti-Harassment board.

I was able to add Speciale:UltimeModifiche on Italian Wikipedia, and it still suggests special pages:

I was able to add Speciale:UltimeModifiche on Italian Wikipedia, and it still suggests special pages:

The API returns that value:
https://it.wikipedia.org/w/api.php?action=query&prop=info%7Cpageprops&generator=prefixsearch&gpssearch=Speciale%3AUltimeModifiche&gpslimit=10&ppprop=disambiguation&redirects=true
but there doesn't appear to be a way to restrict the namespace if you are specifying it in the query... but maybe MediaWiki-API has an alternative that can be used.

Anomie added a subscriber: Anomie.

The API allows for specifying the namespaces to search (the psnamespace parameter). But the underlying SearchEngine code ignores the specified namespaces if, among other things, the entered text looks like it begins with a namespace name. Which is why entering something like "User:Foo" into the box actually works to find page "Foo" in the User namespace instead of trying to find a page invalidly named "User:Foo" in the main namespace.

Probably the thing to do here would be to ignore any results coming back from the API with negative namespaces, since they're not what you want.

Thank you Brad!

TBolliger set the point value for this task to 3.Feb 7 2019, 6:16 PM

Change 489334 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/core@master] mw.widgets.TitleWidget: Add a custom preprocessItems function

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

Change 489335 had a related patch set uploaded (by Mooeypoo; owner: Mooeypoo):
[mediawiki/core@master] Remove suggestions for negative namespaces in Page Restrictions

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

This resolves "The 'Pages' input field should not suggest Special pages" and retains the "The error messages should continue to appear if the admin somehow manages to get these through"

The ability to mark any such tag as invalid is a bit more complex, and might require some creativity here. (re acceptance criteria "The 'Pages' input field should not accept Special pages")
TagMultiselectWidget accepts "allowedValues" as an array but that is not relevant for an API search (because the values can be arbitrary). TagMultiselectWidget in itself does not have an array (or test specifically) of "invalid" tags. That seems like an architectural oversight, honestly, and should be upstreamed -- but I'm not 100% sure how to do taht best yet. Adding another method seems overkill seeing how many ways to validate items the widget already has. I am wondering if it might be better to override checkValidity in TitlesMultiselect but if we do that, we should still allow for configurability in case the widget is used elsewhere in mediawiki.

This is the only followup to do here, I will investigate a little further and see if there's a straight forward way to invalidate these tag values. Either way, the suggestion list will not offer them and the back-end will reject them, so the behavior should be close to what we want.

For the validation of the specific tags, we need to look into allowing for validation in the original ooui widget. I created T215760: TagMultiselectWidget: Add validation method rule for arbitrary tags to look into this in OOUI.

We might be able to get away with something temporary in TitlesMultiselectWidget while the official OOUI method is built, but since TitlesMultiselectWidget is general (and is supposed to be used not just in Special:Block) we should be careful of making sure it's generalizable throughout.

Because there are error messages, If we need to remove the The 'Pages' input field should not accept Special pages bullet from the acceptance criteria, we can. It's the least essential of the points.

The patch seems to resolve all the acceptance criteria.

In the TitlesMultiselectWidget, allowArbitrary is false, so it won't accept pages prefixed by Special or Media (with the patch applied). If the user enters such a page, it can't become a tag, and stays in the input. If the input contains something invalid when it blurs, it styles as invalid (due to onInputBlur in TagMultiselectWidget):

Change 489334 merged by jenkins-bot:
[mediawiki/core@master] mw.widgets.TitleWidget: Add 'excludeDynamicNamespaces' config

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

Change 489335 merged by jenkins-bot:
[mediawiki/core@master] Remove suggestions for negative namespaces in Page Restrictions

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

dom_walden added a subscriber: dom_walden.

When I type something like "Special:B", I see the api call returns pages in the Special namespace, but these don't appear in the dropdown.

I can submit the block with a page like "Special:BlockList" and this creates the block, but "Special:BlockList" is not in the BlockList nor in the database (ipblocks_restrictions).

If there are valid pages in the "Pages" input field before the "Special:BlockList", these are added to the block as per usual. Anything that comes after "Special:BlockList", however, are considered invalid and not included in the block.

I notice that, with javascript disabled, if I try to submit with something like "Special:BlockList" it will not allow me and show an error: "Special:BlockList does not exist". I don't know why this does not happen with javascript enabled. See:

I tested the mobile version of Special:Block as well, and the behaviour was the same as desktop.

I can submit the block with a page like "Special:BlockList" and this creates the block, but "Special:BlockList" is not in the BlockList nor in the database (ipblocks_restrictions).
If there are valid pages in the "Pages" input field before the "Special:BlockList", these are added to the block as per usual. Anything that comes after "Special:BlockList", however, are considered invalid and not included in the block.

It seems like Dom and Thalia's last two comments state opposite things.

As I mentioned earlier, as long as the Special namespace doesn't appear in the suggestions I think we can mark this ticket as resolved, assuming we're handling submitted Special: pages gracefully.

The data loss of items submitted after the invalid page (Sp:BlockList) is not graceful or ideal โ€” is this behavior that was introduced by this patch?

The data loss of items submitted after the invalid page (Sp:BlockList) is not graceful or ideal โ€” is this behavior that was introduced by this patch?

I does not look that way. I can reproduce on test.wikipedia.org.

If you enter something like "NonExistentPage" and the API cannot find any matches, if you keep typing, e.g. "NonExistentPage AnotherPage", then the API searches for a page with the string "NonExistentPage AnotherPage", rather than searching for them as separate pages. In fairness, as we allow spaces in page names it is hard to see how the api/ui would know how the user would intend the spaces to be interpreted.

Probably an invalid bug on my part.

@TBolliger They're not actually opposite, though I can see how it sounds that way...

When a user types into the Pages input, the text exists as text in the input until the user presses enter or clicks away. At this point, if the text in the input is a valid title, it will become a tag (which is what I mean by being accepted); if not, it will style as invalid and remain as text in the input.

If something invalid is typed after an accepted tag, it will look like this:

If an invalid page is typed before a valid page, neither one will become a tag, because the contents of the input will be checked together. In this example, the widget will try to validate "Special:Block Foo" - as @dom_walden points out:

When the form is submitted, only the accepted tags are submitted, not the text left in the input - so the invalid contents aren't actually being submitted at this point. On the non JS version, however, all the text entered is submitted - hence the error message. I suppose in the first example, we might hope that the invalid styling would indicate that the page Special:Block was not accepted, so will not be submitted, even though the form itself doesn't complain... Do we think this UI needs improving?

TBolliger closed this task as Resolved.Feb 22 2019, 6:54 PM

When the form is submitted, only the accepted tags are submitted, not the text left in the input - so the invalid contents aren't actually being submitted at this point. On the non JS version, however, all the text entered is submitted - hence the error message. I suppose in the first example, we might hope that the invalid styling would indicate that the page Special:Block was not accepted, so will not be submitted, even though the form itself doesn't complain... Do we think this UI needs improving?

Thank you for clarifying โ€” this is perfect to ship!

Looks like everything is merged, so closing.

Restricted Application added a project: User-Ryasmeen. ยท View Herald TranscriptFeb 22 2019, 6:54 PM