Let me know when you have code ready.

Also, please follow my suggestion about policy here https://www.mediawiki.org/wiki/Requests_for_comment/HTML_templating_library#Security, and make sure the team has a policy that,

• If substitutions are used in html attributes, those attributes must be quoted with double quotes.
• Make sure any SafeString objects have their escaping as close to the creation of the SafeString as possible, and that should be as close to the output as possible. It would be really helpful if I don't have to trace back more than 1 (or at most 2) function calls to see the escaping.

What is the timeline around this?

Hi Chris! We aim to merge the new Flow frontend in two weeks.

The code is in the Flow frontend-rewrite branch.

• It depends on the new Mantle extension for the third-party JS library HandlebarsJS, in Mantle/javascripts/externals/handlebars.js. Mantle also has ResourceLoader and JS code to use this templating library from MobileFrontend. The Mantle extension is ready for review, bug 66094.
• The third-party PHP library lightncandy is in Flow/vendor/lightncandy.php.
• The way Flow invokes templates from PHP code is set. Shahyar Ghobadpour is refining declarative ways to invoke templates from JS code. We have a small set of helper functions that templates can invoke.

Some documentation of our approach is at https://www.mediawiki.org/wiki/Flow/Templating. Note we disable run-time server compilation to PHP in production, instead we compile handlebars templates to PHP in advance and check them into git. Handlebars templates can similarly be compiled to JS in advance, but we aren't planning that initially (bug 64735) and the user's browser will compile them to JS at runtime.

I'll pass on your comment #1 suggestions to the Flow team.

I'm still working on the review, but a couple of comments:

In includes/TemplateHelper.php

• getTemplateFilenames() should check for directory traversal
• progressiveEnhancement() should escape $insertionType and $sectionId
• Can you whitelist function names in pipelist, eachPost?
• diffRevision: update @param comments to indicate diffContent isn't escaped
• Seems like addReturnTo shouldn't setup the parameters if the request was a POST

• flow_block_header.handlebars and flow_block_header_single_view.handlebars assume revision.content is safe html. flow_preview.handlebars assumes content is safe. It's hard to find where I can prove that-- maybe add a comment in the template to where that is generated, so it's easy to check the correctness?

• form_element's setting the {{tag}} seems like it could be abused. Maybe adding a comment that it really shouldn't be used except in FlowHandlebars.prototype.formElement, and having the default in the switch raise an exception or reset tag to a sane value?

In includes/TemplateHelper.php

• getTemplateFilenames() should check for directory traversal

new patch: https://gerrit.wikimedia.org/r/140938

• progressiveEnhancement() should escape $insertionType and $sectionId

new patch: https://gerrit.wikimedia.org/r/140940

• Can you whitelist function names in pipelist, eachPost?

pipelist turns out to be unused, removed: https://gerrit.wikimedia.org/r/140943

eachPost (and two new block helpers, ifEquals and ifAnonymous) can't be directly whitelisted. Basically lightncandy takes this syntax:

{{#eachPost foo}}
    ...
{{else}}
    ...
{{/eachPost}}

The first ... gets converted into a closure as $options['fn'] and the second ... after the {{else}} gets converted into a closure as $options['inv']. The block helper then decides based on parameters to call those functions one or more times.

I suspect your primarily worried about users being able to specify the function that is called through a tainted variable. To prevent this i've added a patch which ensures these callbacks are Closure instances which cannot be directly provieded by user input.

new patch: https://gerrit.wikimedia.org/r/140944

• diffRevision: update @param comments to indicate diffContent isn't escaped

@TODO

• Seems like addReturnTo shouldn't setup the parameters if the request was a POST

@TODO

• flow_block_header.handlebars and flow_block_header_single_view.handlebars assume revision.content is safe html. flow_preview.handlebars assumes content is safe. It's hard to find where I can prove that-- maybe add a comment in the template to where that is generated, so it's easy to check the correctness?

Matt also noticed this problem, the following patch makes the decision of to escape or not escape revision content more specific with less assumptions. Perhaps the comments between me and matt on PS2 also make it more clear how this should work.

new patch: https://gerrit.wikimedia.org/r/140831/

• form_element's setting the {{tag}} seems like it could be abused. Maybe adding a comment that it really shouldn't be used except in FlowHandlebars.prototype.formElement, and having the default in the switch raise an exception or reset tag to a sane value?

form_element looks to be unused, removed here: https://gerrit.wikimedia.org/r/140957

There are still two aboved marked as todo, i need to check with another flow dev about those.

• diffRevision: update @param comments to indicate diffContent isn't escaped

new patch: https://gerrit.wikimedia.org/r/140962

Thanks for the quick fixes Erik!

I've looked through the majority of the rest of the template handling, and I don't see anything that makes me too nervous.

Architecturally, I think with just a little cleanup, this could be a good addition in core. But that's probably a discussion for another time.

In T65445#680738, @csteipp wrote:

Thanks for the quick fixes Erik!

I've looked through the majority of the rest of the template handling, and I don't see anything that makes me too nervous.

Are you comfortable with us marking this resolved? There has been iteration, of course (including using the core infrastructure for template delivery), but I think the basic architecture is the same as when you reviewed.