Page MenuHomePhabricator

Security review for FormWizard extension
Closed, ResolvedPublic

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 claimed this task.Sep 11 2018, 7:59 PM
Bawolff moved this task from Backlog to Scheduled on the Security-Team-Reviews board.
Bawolff removed Bawolff as the assignee of this task.Sep 17 2018, 4:15 AM
Bawolff moved this task from In Progress to Awaiting remediation on the Security-Team-Reviews board.
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.

Bawolff claimed this task.Nov 13 2018, 1:17 PM
Bawolff moved this task from Awaiting remediation to Scheduled on the Security-Team-Reviews board.
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 Medium 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.May 10 2019, 10:46 PM

@Bawolff-alt, @sbassett After the previous comments, the following have been done:

Its weird that there is a method named setConfigData whose job is to get config data (resolved)

"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. (resolved)

"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. ( Resolved using window.location.assign() )

ext.formWizard.js" line 433 - i18n message formwizard-subpage-request-text need to be html escaped (resolved)

What about more than one formwizard on a single page, how would each have its own separate config? (WONTFIX) - TODO

Special:FormWizard should either be fixed or removed. (resolved)

getRapidGrantsProbox() seems like deadcode should probably be removed (resolved)

Be nice if extension did some content-modely stuff on server side to enforce that the json config files were valid. TODO

The following have not been addressed yet for the following reasons

Weird how basically every line of the css file has a style-lint disable line

This was inserted to disable some styles (disable-next-line selector-class-pattern) which are seen by stylelint as errors if these comments were absent(Tests fail without comments - even though comments don't affect code)

sbassett reassigned this task from Bawolff-alt to Bawolff.Jun 12 2019, 6:30 PM

So yeah, I guess this counts as passes security review as none of those issues were security related. May need additional security review if the extension changes significantly. Should still get approval from Rel engineering before deploy.

Bawolff removed Bawolff as the assignee of this task.Jun 25 2019, 2:18 PM
sbassett closed this task as Resolved.Jun 25 2019, 5:18 PM
sbassett claimed this task.

Resolving for now, can re-open if more issues surface.