Page MenuHomePhabricator

HTMLForms using POST might be broken if the form action is a URL containing "?title="
Open, Needs TriagePublicBUG REPORT

Description

Discovered for AbuseFilter (Special:AbuseFilter/import) while testing apache builds in CI (example).

The form has the action set to the full URL of Special:AbuseFilter/new, where it POSTs the import data. If this url uses the ?title= variant (e.g. 127.0.0.1:9413/index.php?title=Special:AbuseFilter/new), as opposed to e.g. //localhost/pedia/index.php/Special:AbuseFilter/new, submitting the form will leave the user on the import page.

This happens because the form has a hidden title input whose value is Special:AbuseFilter/import, and this takes precedence over the query string in the action.

You can easily reproduce this bug by:

  • Going to Special:AbuseFilter/import
  • Manually changing the form action to use the "title=" variant
  • Add any text into the field and submit

In particular, an exception will be thrown due to HTMLForm mishandling, but the root cause is that we're actually showing the wrong special page (/import).


I think one possible fix might be to conditionally set the title input like we do for GET forms, see here. I'm not very familiar with HTMLForm though, so deferring to someone else.

Event Timeline

I'm not sure where the relevant code is in AbuseFilter, there's a lot of forms there…

Does the problem still occur if you replace the calls to addHiddenField() and setAction() with a call to setTitle()?

I'm not sure where the relevant code is in AbuseFilter, there's a lot of forms there…

The AbuseFilter one is here -- a fairly simple one.

Does the problem still occur if you replace the calls to addHiddenField() and setAction() with a call to setTitle()?

The addHiddenField thing happens in HTMLForm::getHiddenFields and there seems to be no way around it. OTOH, if I use setTitle instead of setAction it works. This might resolve the immediate problem, but not the whole thing IMHO. I think this should be either documented or, better, fixed.

Change 702903 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/AbuseFilter@master] ViewList: Use setTitle instead of addHiddenField/setAction

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

Change 702904 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/Wikibase@master] [DNM] Test for T285464

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

Change 702903 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] ViewImport/ViewList: Use setTitle instead of addHiddenField/setAction

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

kostajh claimed this task.

Thanks for the tips @matmarex & for the code review @Daimona.

Daimona removed kostajh as the assignee of this task.
Daimona removed a project: Patch-For-Review.

@kostajh I'd like to keep this task open since I feel that other might fall for this bug. HTMLForm should either not let you set an action that is not going to work (or at least document that), or avoid setting the title param if the action already has it.

@kostajh I'd like to keep this task open since I feel that other might fall for this bug. HTMLForm should either not let you set an action that is not going to work (or at least document that), or avoid setting the title param if the action already has it.

Ah, yeah fair enough. Thanks @Daimona.

Change 759955 had a related patch set uploaded (by Umherirrender; author: Kosta Harlan):

[mediawiki/extensions/AbuseFilter@REL1_36] ViewImport/ViewList: Use setTitle instead of addHiddenField/setAction

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

Change 759955 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@REL1_36] ViewImport/ViewList: Use setTitle instead of addHiddenField/setAction

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

Change 761091 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] HTMLForm: Disallow setting internal field names

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

Change 772026 had a related patch set uploaded (by Func; author: Func):

[mediawiki/extensions/AbuseFilter@master] ViewRevert: Adjust use cases of HTMLForm

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

Change 772026 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] ViewRevert: Adjust use cases of HTMLForm

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

Change 772013 had a related patch set uploaded (by Func; author: Func):

[mediawiki/core@master] HTMLForm: Add title field if the action is overridden to script path

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

Change 772013 merged by jenkins-bot:

[mediawiki/core@master] HTMLForm: Add title field if the action is overridden to script path

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