Page MenuHomePhabricator

Security review for FormWizard extension
Open, Stalled, NormalPublic

Description

Project Information

Description of the tool/project

A mediawiki extension to implement the functionalities of FormWizard Gadget.

Description of how the tool will be used at WMF

This extension will be installed on Mediawiki and can be used on all mediaWiki Wikis.

Dependencies

The extension depends on *oojs* and *oojs-ui* as it uses these components to facilitate use on mobile devices.

Has this project been reviewed before?

No

Working test environment

Test Wikipedia

Post-deployment

Meta-Wiki

Event Timeline

Eugene233 created this task.Aug 8 2018, 9:02 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 8 2018, 9:02 AM
Reedy removed Eugene233 as the assignee of this task.Aug 8 2018, 9:32 AM

This seems to be blocked on T201488: Move FormWizard extension to new Gerrit repository. if you want any reviewer to actually find code to review :P

Eugene233 updated the task description. (Show Details)Aug 13 2018, 3:06 PM
Harej added a comment.Sep 5 2018, 9:11 PM

The extension is ready to be reviewed, having since been moved to Gerrit.

Bawolff moved this task from Backlog to Scheduled on the Security-Team-Reviews board.
Bawolff removed Bawolff as the assignee of this task.
Bawolff added a subscriber: Bawolff.

Review of revision 988d242afdd2

Major concerns

  • There doesn't seem to be any restriction on formWizardPageName. What if it is MediaWiki:Common.js or a protected page (e.g. Vandal tricks people into clicking form wizard links).
  • [minor] formWizardConfig isn't verified to actually be a JSON page.
  • Either Hooks::showProjectButton needs to call $parser->getOutput()->preventClickjacking( true ), or the extension js needs to disable itself when frames are present

Non-security concerns

One overall comment - This seems to be very specialized to the grant's process with how it formats the resulting saved page. I don't think it should be hard coded like that but instead pull that sort of information from the config page. I would also like to note, it would be nice if the json schema the config page uses was better documented.

Hooks.php

  • showProjectButton - May want to consider wfEscapeWikiText() depending on if you want formatted text inside the button.
  • Why are you calling addExtensionData() instead of $parser->getOutput()->addJsConfigVars() directly?
  • What happens if multiple form wizards on a page?
  • what is the color attribute? Should this be style or something or is OOUI making up its own attribute names.
    • For that matter, should probably consider using OOUI directly (e.g. new OOUI\ButtonWidget(...). And using $parser->getOutput()->setEnableOOUI( true ); instead of directly including the module. OTOH, if you do that, you probably cannot output directly as wikitext, but must use html instead.
  • I think maybe this should use named parameters instead of numbered parameters. Its hard to remember which parameter is which.
  • should probably register config pages as a "template" dependency.
    1. SpecialFormWizard.php
  • What is this for?
    1. ext.formWizard.js
  • "ext.formWizard.js" line 160 - action=parse seems like an odd way to get the page's content. I think query=revisions would be better
    • More to the point, I find it odd its fetching the content at all, why not include it as a JS config variable?
  • Some weird indenting in all the docblock comments.
  • Mixing config/wikitext templates and programming stuff (e.g. getRapidGrantsProbox()). Ideally I'd like to see this more generic.
  • "ext.formWizard.js" line 432 - This doesn't look right. You probably do not want both text and appendText
  • It would be nice to have some error handling with the page edit process, even if its very basic. Ditto for error handling when given an invalid schema
  • "modules/ext.formWizard.js" line 438 (mw.notify) - This needs to be i18n-ized. I'm also not sure how well it works to give the user a notice and then immediately reload the page (Thus making the notice disappear before they could read it)
  • "modules/ext.formWizard.js" line 431 - edit summary should probably be an i18n message (preferrably in content lang)
  • "modules/ext.formWizard.js" line 4 - doc comment is wrong.
  • "modules/ext.formWizard.js" - is this meant to be fieldSetContentData instead of fieldSetContendData?

FormWizard.test.js

The test suit is a little empty :P

Harej added a comment.Nov 6 2018, 4:03 PM

Would it be possible to take another look at the extension? The concerns originally raised should be addressed by now.

Harej added a comment.Apr 16 2019, 9:40 PM

Would a re-review of this extension be possible? I would like to begin testing the extension in a production environment.

sbassett triaged this task as Normal priority.Apr 17 2019, 2:10 PM
sbassett added a comment.EditedApr 17 2019, 2:16 PM

@Harej - I believe @Bawolff currently has this assigned as a lower-priority review. Maybe we can address two things:

  1. Have all of the issues from his initial review (T201492#4587298) been addressed and resolved or marked WONTFIX? I see some subtasks above - a few of them are resolved w/ corresponding gerrit patch sets, but some still seem open. I'm not sure if that's all of them though.
  2. Is there a more firm date you have in mind for production testing or a deploy? If so, we should note that here, as it will help us with our scheduling.

Thanks.

Harej added a comment.Apr 19 2019, 3:30 AM

I hope to get back to you soon on this. Currently we are seeking other sorts of code review, so we are not quite ready yet.

sbassett changed the task status from Open to Stalled.Apr 19 2019, 1:46 PM

So I guess this isn't quite ready for a security review given previous comment, but some thoughts

  • Weird how basically every line of the css file has a style-lint disable line
  • Its weird that there is a method named setConfigData whose job is to get config data
  • "ext.formWizard.js" line 395 [and several other places] - targetRootName + $( '#subpage-name' ).val(); - may want to consider having mw- prefixing id names if you want to make sure that users can't create elements with a conflicting id.
  • "ext.formWizard.js" line 407 - using window.location.replace() is a bit odd, as i don't think you'd want to get rid of the entry in the history list.
  • ext.formWizard.js" line 433 - i18n message formwizard-subpage-request-text need to be html escaped
  • What about more than one formwizard on a single page, how would each have its own separate config?
  • Special:FormWizard should either be fixed or removed.
  • getRapidGrantsProbox() seems like deadcode should probably be removed
  • Be nice if extension did some content-modely stuff on server side to enforce that the json config files were valid.
Bawolff removed Bawolff as the assignee of this task.Fri, May 10, 10:46 PM