happy.melon.wiki wrote:

In installations where ParserFunctions is installed but Variables is not, $wgExtVariables is undefined, and so referencing it is a register_globals vulnerability.

The ParserFunctions extension should not contain references to the Variables extension in the same way that core code should not contain references to extensions. You should include hooks in ParserFunctions and hook functions in Variables which handle the actual processing (and could happily be used by other extensions). It's fine to tweak the code flow in ParserFunctions to ensure you can pass the right things to the hook.

van.de.bugger wrote:

Sorry, I am not an experienced PHP developer, so I did not get everything you wrote.

As I understand, vulnerability is only possible if Variables extension is not installed AND register_globals is ON. register_globals is OFF by default and I saw a lot of warnings that it should remain be OFF.

However, I do not object to introduce a hook if you point me an example what it is.

Another thing I though about is fully emulating Variables in ParserFunctions. Variables is trivial extension and is in public domain, so it is possible to just incorporate Variables into ParserFunctions.

john wrote:

Some information about hooks: http://www.mediawiki.org/wiki/Hooks

Basically you need a wfRunHooks call somewhere that lets you modify the behaviour of the ParserFunctions call from Variables.

van.de.bugger wrote:

Sorry, did not catch. Manual:Hooks says:

Hooks allow custom code to be executed when one of many defined events (like saving an article or a user logging in) occur.

In case of ParserFunctions/Variables integration, I do not see events. In this case one extension (namely, ParserFunctions) directly uses service provided by another extension (namely, Variables).

Happy-melon said:

The ParserFunctions extension should not contain references to the Variables

extension in the same way that core code should not contain references to
extensions.

?? Extensions use services provided by core directly. Extensions use global variables defined in core, like $wgAutoloadClasses, $wgGroupPermissions, $wgContLang, etc.

In this particular case, what is "core" and what is "extension"? (1) ParserFunctions extends Variables or (2) Variables extends ParserFunctions?

In the first case I do not see any issue if ParserFunctions uses services provided by Variables by using global variable $wgExtVariables.

In the seconds case ParserFunctions (namely, expression parser/evaluator) can be extended with plugins to introduce new functions, operators, names... It is an interesting idea -- extensions for extensions... It is suitable for big and complex extensions like SemanticMediaWiki, but ParserFunctions is rather simple extension... Do you really want make a monster from ParserFunctions? To me it is much simpler to move Variables functionality into ParserFunctions.

van.de.bugger wrote:

BTW, I just scanned extension sources checked out from svn. DynamicPageList extension uses Variables directly via $wgExtVariables, as well as another extension, Loops.

rm patch-need-review, this patch isn't going to be accepted. We should switch to Lua anyway instead of inviting new exciting ways to make wikitext even slower.

Oh, even six years later I want to note that Variables can double parsing speed.

This is unlikely to be resolved any time soon, as it would require either Hooks in ExprParser at really specific places - neither good style nor convenient, a patch providing them is very unlikely to get merged, or a possibility to replace ExprParser with an inheriting custom parser, which seems really complicated.

Furthermore, I'm not even sure this is a good idea at all, as this can lead to unexpected behavior of the parser becausec of unconventional variable names. No matter what, it would make ExprParser much more fragile.

I agree with declining this; there's just too much potential for unwanted Variables expansion caused by lazy/careless naming of Variables, or by intentionally malicious ParserFunction input. To prevent this, you'd have to have some feature for clearly marking what's meant to be a variable name and what's meant to just be text, and at that point you're back to the present situation.

In T32039#4167157, @MGChecker wrote:

Oh, even six years later I want to note that Variables can double parsing speed.

Can you clarify what you meant here? Are you saying that Variables can speed up page parsing, or that it can cause parsing to take twice as long?

In T32039#4490421, @Dinoguy1000 wrote:

I agree with declining this; there's just too much potential for unwanted Variables expansion caused by lazy/careless naming of Variables, or by intentionally malicious ParserFunction input. To prevent this, you'd have to have some feature for clearly marking what's meant to be a variable name and what's meant to just be text, and at that point you're back to the present situation.

Indeed. Additionally, it's highly improbable that we would get alterations to the ParserFunctions extension merged, that would be required to do this task.

In T32039#4167157, @MGChecker wrote:

Oh, even six years later I want to note that Variables can double parsing speed.

Can you clarify what you meant here? Are you saying that Variables can speed up page parsing, or that it can cause parsing to take twice as long?

Variables can speed up page parsing. Imagine templates that use page names and multiple switches with around 1000 entries to get some data, for example about template colors, and imagine these templates over 100 times on a single page. If you just have to query this information once, you are much faster.