Security review for AdvancedSearch extension
Closed, ResolvedPublic

Description

Project Information

Description of the tool/project

Creates an improved advanced search interface for MediaWiki and aims for a user-friendly integration of search keywords.

Description of how the tool will be used at WMF

If installed, the extension enhances Special:Search by an advanced parameters form and improves the way how to configure namespaces.
The extension should be available as a beta-feature at the beginning.

Dependencies

  • None

Has this project been reviewed before?

Only review inside the TCB-Team.

Working test environment

wfLoadExtension( 'AdvancedSearch' );
  • After enabling the extension, go to Special:Search.

Post-deployment

TCB-Team

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 26 2017, 1:16 PM
Tobi_WMDE_SW updated the task description. (Show Details)Jun 26 2017, 1:18 PM
Tobi_WMDE_SW updated the task description. (Show Details)
Tobi_WMDE_SW updated the task description. (Show Details)Jul 4 2017, 3:06 PM
gabriel-wmde updated the task description. (Show Details)Jul 11 2017, 3:28 PM
Reedy added a subscriber: Reedy.EditedJul 11 2017, 9:58 PM

Non security little things

If installed, the extension enhances Special:Search by an advanced parameters form and improves the way how to configure namespaces.

and also on https://www.mediawiki.org/wiki/Extension:AdvancedSearch

I'm struggling to grok that as a whole sentence. Configure namespaces? Configure them for what? (I'm presuming it doesn't mean configure namespaces akin to adding them etc in LocalSettings)

  • onSpecialSearchPowerBox hook subscriber is a noop, should be removed
  • Indenting in getBetaFeaturePreferences for info-link and discussion-link values, should either be on the same line, or should be an extra tab before =>
  • No COPYING/LICENCE file (but licence is in extension.json)
  • Vagrant role (should be goodfirstbug ) would be useful
  • Screenshot on https://www.mediawiki.org/wiki/Extension:AdvancedSearch is out of date
  • advancedsearch-field-or should be "these" not "those"
  • Has this extension had i18n review?
  • I noticed on Special:Search?uselang=qqx that a few messages didn't change to message keys. Having then found the javascript below, it's clear that these messages can't be localised. This need fixing before deployment on WMF wikis
	var i18n = {
		de: {
			'advanced-search': 'Erweiterte Suchoptionen:',

			// TODO Move these to i18n
			text: 'Seite enthält …',
			structure: 'Struktur',
			files: 'Dateien und Bilder'
		},
		en: {
			'advanced-search': 'Advanced parameters:',
			text: 'The page should include …',
			structure: 'Structure',
			// FIXME: Why does this need to mention both, like images are not files?
			files: 'Files and images'
		}
	};
  • 'The page should include …' - Should this be an ellipses? I don't think we use this syntax anywhere else
  • RL loader code that looks wrong module is defined for each file. Could be moved to ResourceFileModulePaths to save some duplication/words

Non security little things

i18n: Also note that splitting strings like ("The page should include..." "this word") currently does not tell translators that the second string requires accusative declension when trying to translate the second string properly...

Non security little things

i18n: Also note that splitting strings like ("The page should include..." "this word") currently does not tell translators that the second string requires accusative declension when trying to translate the second string properly...

And should generally be avoided for sure!

https://www.mediawiki.org/wiki/Localisation#Avoid_fragmented_or_.27patchwork.27_messages

I guess also... "The page should include... Not this word" isn't very good english.

"The page should not include this word" obviously makes a lot more sense

"The page should include … Exactly this text:" doesn't really make sense. Include exactly this text? What do the other options do?

I wonder if these should also have tooltips with much more detailed information about what the options do... Or links

@Reedy @Aklapper Thanks for your review so far! We'll follow up on it soon.

Change 365261 had a related patch set uploaded (by Gabriel Birke; owner: Gabriel Birke):
[mediawiki/extensions/AdvancedSearch@master] Address some non-security issues from T168860

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

Change 365261 merged by jenkins-bot:
[mediawiki/extensions/AdvancedSearch@master] Address some non-security issues from T168860

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

Reedy added a comment.EditedJul 18 2017, 2:21 PM

I think those are the only outstanding ones for the non security stuff

Vagrant roles added at https://gerrit.wikimedia.org/r/#/c/365964/

PHP code is obviously fine from a security PoV

Reedy added a comment.Jul 18 2017, 3:45 PM
	// TODO initialize with API call instead
	var templateModel = new mw.libs.advancedSearch.dm.MenuDataModel( {
		infoboxNature: 'Infobox Nature',
		infoboxArchitecture: 'Infobox Architecture',
		commonsLink: 'Link to Commons'
	} );

Specifically the last one there... Should that be i18n-ified? Not that any of them actually seem to be used anywhere?

	mw.loader.load( '//de.wikipedia.org/w/index.php?title=MediaWiki:Gadget-DeepCat.js&action=raw&ctype=text/javascript' );

Uh... what? Why? This doesn't feel very non WMF friendly among other things. Never mind any comments/similar explaining why it's there.

On Timeless skin... Things look rather broken in terms of layout of the subheadings

On Vector/Monobook... There seems to be some control overlap below the "Search in:" header. Doesn't look the same as the "Advanced parameters:" header in collapsed/expanded states

Reedy moved this task from Scheduled to In Progress on the Security-Reviews board.Jul 18 2017, 3:56 PM
Reedy triaged this task as Normal priority.Jul 18 2017, 6:17 PM

@Reedy Thank you for pointing out two vestiges of the early demo prototype!

  • The templateModel variable is just a placeholder for an unfinished feature. This will be implemented in T165302 and the variable will be removed. No need to i18n the placeholder.
  • The loading of the DeepCat gadget will also be removed. Instead, DeepCat-like functionality will be implemented for all wikis that are indexed in CatGraph. See T165297 and T170533 .

Could someone give an update on the status of the security review? I hope nobody is waiting for anything from our side.

Tobi_WMDE_SW added a subscriber: Bawolff.

@Bawolff @Reedy do you have any updates on this? We're aiming for a deployment as a beta feature in October or November 2017, so we should resolve any security review related issues rather soonish. Can you give a brief timeline for this?

Reedy closed this task as Resolved.Sep 21 2017, 11:03 PM
Reedy claimed this task.

I blame Wikimania!

Looks like the scary arbitrary inclusion of js from dewiki is gone, so I don't see anything else that's outstanding at this point.

Probably worth enabling codesniffer (either version) so commits like https://github.com/wikimedia/mediawiki-extensions-AdvancedSearch/commit/2aa01dc9c3e367c7202a541c93cd1221ce989ee4 don't need to be made manually. PHP linting/

And then jslint/whatever we're using these days too

Oh PhpStorm is bitching about the * @return boolean on onSpecialPageBeforeExecute