Maniphest T201492

Security review for FormWizard extension
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Eugene233
	Aug 8 2018, 9:02 AM

Description

Project Information

Name of tool/project: FormWizard
Project home page: https://phabricator.wikimedia.org/tag/formwizard/
Name of team requesting review: TBA
Primary contact: @Harej
Target date for deployment: End of September
Link to code repository / patchset: https://gerrit.wikimedia.org/r/#/admin/projects/mediawiki/extensions/FormWizard

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?

Working test environment

Test Wikipedia

Post-deployment

Meta-Wiki

Related Objects

Mentioned In: T204661: Edit summary should be an i18n message
T204651: Config pages should be registered as a "template" dependency.
T204648: showProjectButton should call addJsConfigVars() directly instead of setExtensionData()
T204647: showProjectButton should call wfEscapeWikiText() to get formatted text inside the button.
T204644: Verify that FormWizard config page is actually a JSON page
T201771: Request repository ownership for FormWizard Extension,
Mentioned Here: rEFWZ988d242afdd2: build: Updating mediawiki/mediawiki-codesniffer to 22.0.0
T201488: Move FormWizard extension to new Gerrit repository.

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

Eugene233 added a project: FormWizard.Aug 8 2018, 9:04 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

Peachey88 mentioned this in T201771: Request repository ownership for FormWizard Extension,.Aug 11 2018, 10:10 PM

Eugene233 updated the task description. (Show Details)Aug 13 2018, 3:06 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 Incoming to Scheduled on the deprecated-security-team-reviews board.

Bawolff moved this task from Scheduled to In Progress on the deprecated-security-team-reviews board.Sep 16 2018, 6:04 PM

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

Eugene233 mentioned this in T204644: Verify that FormWizard config page is actually a JSON page.Sep 18 2018, 7:48 AM

Eugene233 mentioned this in T204647: showProjectButton should call wfEscapeWikiText() to get formatted text inside the button..Sep 18 2018, 8:04 AM

Eugene233 mentioned this in T204648: showProjectButton should call addJsConfigVars() directly instead of setExtensionData().Sep 18 2018, 8:10 AM

Eugene233 mentioned this in T204651: Config pages should be registered as a "template" dependency..Sep 18 2018, 8:30 AM

Eugene233 mentioned this in T204661: Edit summary should be an i18n message.Sep 18 2018, 9:10 AM

sbassett subscribed.Sep 24 2018, 6:30 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 Waiting to Scheduled on the deprecated-security-team-reviews board.

• chasemp moved this task from Scheduled to Incoming on the deprecated-security-team-reviews board.Dec 17 2018, 6:36 PM

• charlotteportero edited projects, added Security-Team-Review-Active; removed deprecated-security-team-reviews.Jan 14 2019, 6:18 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

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

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.
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.

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 assigned this task to Bawolff-alt.Jun 11 2019, 7:25 PM

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