Page MenuHomePhabricator

GetBetaFeaturePreferences hook doesn't validate all the entries against $wgBetaFeaturesWhitelist
Closed, ResolvedPublic

Description

I can register a beta feature in this hook that is not in $wgBetaFeaturesWhitelist but it still shows when I visit Special:Preferences despite not working.

Expected:
When a feature is not whitelisted it should not show up in beta features.

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: BetaFeatures.
Jdlrobson added a subscriber: Jdlrobson.

I'm moderately sure this is a regression, but I can't say how.

if (
		is_array( $wgBetaFeaturesWhitelist ) &&
		!in_array( $key, $wgBetaFeaturesWhitelist )
	) {
	$success = false;
} elseif ( isset( $depHooks[$key] ) ) {
	$success = Hooks::run( $depHooks[$key] );
}

if ( $success !== true ) {
	// Skip this preference!
	continue;
}

Thoughts as to why this doesn't work any more appreciated.

if (
		is_array( $wgBetaFeaturesWhitelist ) &&
		!in_array( $key, $wgBetaFeaturesWhitelist )
	) {
	$success = false;
} elseif ( isset( $depHooks[$key] ) ) {
	$success = Hooks::run( $depHooks[$key] );
}

if ( $success !== true ) {
	// Skip this preference!
	continue;
}

Thoughts as to why this doesn't work any more appreciated.

The above block is encolsed in

if ( isset( $info['dependent'] ) && $info['dependent'] === true ) {

Clearly, if dependencies are not given, whitelist check is completely bypassed.

Change 258418 had a related patch set uploaded (by Sumit):
BetaFeatures only add whitelisted preferences

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

Thoughts as to why this doesn't work any more appreciated.

The above block is encolsed in

if ( isset( $info['dependent'] ) && $info['dependent'] === true ) {

Clearly, if dependencies are not given, whitelist check is completely bypassed.

Well, don't I feel a fool. :-)

Change 258418 merged by jenkins-bot:
BetaFeatures only add whitelisted preferences

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