From 7e1827a011a94cf2ae672949a6ef46c6c671c7e2 Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Thu, 15 Apr 2021 07:47:36 +0000 Subject: [PATCH] [IMP] web: prepare for de-traw-ification A note concerning the attributes which I will probably need to take a look at in the vdom version: the Python version has to process attf in order to stringify individual elements, and separately stringify the attribute value so it gets forcefully escaped even if it's markup-safe (because markup-safety and attributes-safety are different). The first should not need to be performed on the JS side, because Markup can not overload addition, therefore in JS String + Markup is String whereas in Python it's Markup (and the String gets forcefully escaped). In general, js!markup is currently much simpler than py!markup, both by necessity (can't overload operators) and simplicity (we might want to overload some of the operations e.g. String#replace, but that's complicated and it's not been strictly necessary for now). Also wrt Markup / _Markup: `class` ctors can only be invoked with `new` meaning they can't be used as template strings or regular functions. Here `class` is useful to avoid the mess of calling the super's constructor explicitly (which may not even be possible for `String`), however it means we need a facade function to support our use-cases. Also update `utils.sprintf` to be Markup-aware: if the format string is a Markup object, interpolated values get automatically escaped (if necessary) and the result remains a Markup object. If we need to perform explicit instance check we can always set `Markup.prototype = _Markup.prototype` (I think), however in theory that's not necessary: there are protocols in place for the relevant pseudo-escaping operations and they ought suffice. qweb/js divergence from qweb/py =============================== Unlike qweb/py, qweb/js will *not* return a Markup object. That is because in js a primitive `string` and a boxed `String` object don't match when typechecking, and while `markup instanceof String` passes, `typeof markup === 'string'` does not. The overwhelming majority of string typechecks are the latter: there are all of 6 `instanceof String` in the entire codebase, all in dependencies, while there are hundreds of `typeof $X === 'string'`, several of which get fed the output of template rendering e.g. `jQuery.parseXML` or `AbstractView#init` (some widgets will render a template then use it as the `arch` of a subview, so `viewInfo.arch` can be the output of a qweb template rendering). This makes for very annoying and somewhat gnarly debugging. Plus jQuery in particular really doesn't like being fed a boxed String, as it will interpret said boxed string as an array, and assume it's an array of DOM elements to wrap, leading to a rather strange jQuery object as output. Since feeding the result of a template rendering to jQuery is a major use-case in non-vdom widgets... that's a bit of an issue. For the same reason while `_.escape` is `Markup`-aware, unlike `markupsafe-escape` it does not *produce*, though it is `Markup`-transparent. --- addons/web/static/lib/qweb/qweb2.js | 39 ++- addons/web/static/src/legacy/js/core/utils.js | 107 ++++++- addons/web/static/tests/legacy/qweb_tests.js | 261 ++++++++++++------ 3 files changed, 296 insertions(+), 111 deletions(-) diff --git a/addons/web/static/lib/qweb/qweb2.js b/addons/web/static/lib/qweb/qweb2.js index d2a367f1c33..60a7c0beea5 100644 --- a/addons/web/static/lib/qweb/qweb2.js +++ b/addons/web/static/lib/qweb/qweb2.js @@ -28,7 +28,7 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. var QWeb2 = { expressions_cache: { }, RESERVED_WORDS: 'true,false,NaN,null,undefined,debugger,console,window,in,instanceof,new,function,return,this,typeof,eval,void,Math,RegExp,Array,Object,Date'.split(','), - ACTIONS_PRECEDENCE: 'foreach,if,elif,else,call,set,tag,esc,raw,js,debug,log'.split(','), + ACTIONS_PRECEDENCE: 'foreach,if,elif,else,call,set,tag,out,esc,raw,js,debug,log'.split(','), WORD_REPLACEMENT: { 'and': '&&', 'or': '||', @@ -65,15 +65,14 @@ var QWeb2 = { js_escape: function(s, noquotes) { return (noquotes ? '' : "'") + s.replace(/\r?\n/g, "\\n").replace(/'/g, "\\'") + (noquotes ? '' : "'"); }, - html_escape: function(s, attribute) { + html_escape: function(s) { if (s == null) { return ''; } - s = String(s).replace(/&/g, '&').replace(//g, '>'); - if (attribute) { - s = s.replace(/"/g, '"'); - } - return s; + return _.escape(s); + }, + markup(s) { + return new _Markup(s); }, gen_attribute: function(o) { if (o !== null && o !== undefined) { @@ -94,7 +93,10 @@ var QWeb2 = { return ''; }, format_attribute: function(name, value) { - return ' ' + name + '="' + this.html_escape(value, true) + '"'; + // we want to ensure `value` is *not* a `Markup`, because markup-safe + // strings are not necessarily attributes-safe. + const attrvalue = value == null ? '' : this.html_escape(String(value)); + return ` ${name}="${attrvalue}"`; }, extend: function(dst, src, exclude) { for (var p in src) { @@ -149,9 +151,9 @@ var QWeb2 = { var new_dict = this.extend({}, old_dict); new_dict['__caller__'] = old_dict['__template__']; if (callback) { - new_dict[0] = callback(context, new_dict); + new_dict[0] = this.markup(callback(context, new_dict)); } - return context.engine._render(template, new_dict); + return this.markup(context.engine._render(template, new_dict)); }, foreach: function(context, enu, as, old_dict, callback) { if (enu != null) { @@ -689,7 +691,7 @@ QWeb2.Element = (function() { s && r.push(_this.engine.tools.js_escape(s)); } - var re = /(?:#{(.+?)}|{{(.+?)}})/g, start = 0, r = [], m; + var re = /#{(.+?)}|{{(.+?)}}/g, start = 0, r = [], m; while (m = re.exec(s)) { // extract literal string between previous and current match append_literal(s.slice(start, re.lastIndex - m[0].length)); @@ -800,7 +802,7 @@ QWeb2.Element = (function() { this.bottom("}));"); this.indent(); this.top("var r = [];"); - return this.bottom("return r.join('');"); + return this.bottom("return context.engine.tools.markup(r.join(''));"); } }, compile_action_set : function(value) { @@ -822,11 +824,14 @@ QWeb2.Element = (function() { this.bottom("})(dict);"); this.indent(); this.top("var r = [];"); - this.bottom("return r.join('');"); + this.bottom("return context.engine.tools.markup(r.join(''));"); } } }, compile_action_esc : function(value) { + return this.compile_action_out(value); + }, + compile_action_out(value) { this.top("var t = " + this.format_str(value) + ";"); this.top("if (t != null) r.push(context.engine.tools.html_escape(t));"); this.top("else {"); @@ -834,6 +839,14 @@ QWeb2.Element = (function() { this.indent(); }, compile_action_raw : function(value) { + let e = this.node; + while (e.parentElement && !e.getAttribute('t-name')) { + e = e.parentElement; + } + console.warn( + "Found deprecated directive '@t-raw=\"%s\"' in " + + "template '%s'. Replace by @t-out, and explicitely wrap content in " + + "utils.Markup if necessary.", value, e.getAttribute('t-name')); this.top("var t = " + this.format_str(value) + ";"); this.top("if (t != null) r.push(t);"); this.top("else {"); diff --git a/addons/web/static/src/legacy/js/core/utils.js b/addons/web/static/src/legacy/js/core/utils.js index 05d2f14b358..95ad897c9a1 100644 --- a/addons/web/static/src/legacy/js/core/utils.js +++ b/addons/web/static/src/legacy/js/core/utils.js @@ -240,9 +240,81 @@ class UnknownPatchError extends Error { } } +const __escape = _.escape; +_.escapeMethod = Symbol('html') +_.escape = function escape(s) { + return s[_.escapeMethod] ? s[_.escapeMethod]() : __escape(s); +} + +// notable issues: +// * objects can't be negative in JS, so !!"" -> false but +// !!(new String) -> true, likewise markup +// TODO (?) +// * Markup.join / Markup#join => escapes items and returns a Markup +// * Markup#replace => automatically escapes the replacements (difficult impl) +class _Markup extends String { + [_.escapeMethod]() { + return this; + } +} +// exposed for qweb2.js +window._Markup = _Markup; + +/** + * Returns a markup object, which acts like a String but is considered safe by + * `_.escape`, and will therefore be injected as-is (without additional + * escaping) in templates. Can be used to inject dynamic HTML in templates + * (where the template itself can't), see first example. + * + * Can also be used as a *template tag*, in which case the literal content + * won't be escaped but the substitutions which are not already markup objects + * will be. + * + * ## WARNINGS: + * * A markup object is a `String` (boxed) but not a `string` (primitive), they + * typecheck differently which can be relevant. + * * To strip out the "markupness", just call `String(markup)`. + * * Most string operations (e.g. concatenation, `String#replace`, ...) will + * also strip out markupness + * * If the input is empty, returns a regular string (that way boolean tests + * work as expected). + * + * @returns a markup object + * + * @example regular function + * let h; + * if (someTest) { + * h = Markup(_t("This is a success")); + * } else { + * h = Markup(_t("Things did not work out")); + * } + * qweb.render("some_template", { message: h }); + * + * @example template tag + * const escaped = " text"; + * const asis = Markup`some text`; + * const h = Markup`Regular strings get ${escaped} but markup is injected ${asis}`; + */ +function Markup(v, ...exprs) { + if (!(v instanceof Array)) { + return v ? new _Markup(v) : ''; + } + const elements = []; + let i = 0; + for(; i < exprs.length; ++i) { + elements.push(v[i], _.escape(exprs[i])); + } + elements.push(v[i]); + + const s = elements.join(''); + if (!s) { return '' } + return new _Markup(s); +} + var utils = { AlreadyDefinedPatchError, UnknownPatchError, + Markup, /** * Throws an error if the given condition is not true @@ -818,25 +890,40 @@ var utils = { }, /** * Returns a string formatted using given values. + * * If the value is an object, its keys will replace `%(key)s` expressions. * If the values are a set of strings, they will replace `%s` expressions. * If no value is given, the string will not be formatted. * - * @param {string} string - * @param {(Object|...string)} values + * `Markup`-aware: if a `Markup` object is provided as format string, + * automatically escapes injected values and returns a new `Markup` + * object. + * + * @param {string|Markup} string format string + * @param values values injected into the format string, can be either a + * sequence of positional parameters (for positional + * placeholders) or a single Object acting a as map (for named + * placeholders). + * @returns {string|Markup} */ - sprintf: function (string, ...values) { + sprintf(string, ...values) { + let finalizer, mapper; + finalizer = mapper = a => a; + if (string instanceof _Markup) { + string = String(string); + finalizer = Markup; + mapper = _.escape; + } if (values.length === 1 && typeof values[0] === 'object') { const valuesDict = values[0]; - for (const value in valuesDict) { - string = string.replace(`%(${value})s`, valuesDict[value]); - } + string = string.replace( + /%\((\w+)\)s/g, + (_, group) => mapper(valuesDict[group])); } else { - for (const value of values) { - string = string.replace(/%s/, value); - } + let i = 0; + string = string.replace(/%s/g, () => mapper(values[i++])); } - return string; + return finalizer(string); }, /** * Sort an array in place, keeping the initial order for identical values. diff --git a/addons/web/static/tests/legacy/qweb_tests.js b/addons/web/static/tests/legacy/qweb_tests.js index dd2b0c527ec..2880988ad4a 100644 --- a/addons/web/static/tests/legacy/qweb_tests.js +++ b/addons/web/static/tests/legacy/qweb_tests.js @@ -1,6 +1,8 @@ odoo.define('web.qweb_tests', function (require) { "use strict"; +const {Markup} = require('web.utils'); + var qwebPath = '/web/static/lib/qweb/'; const {hushConsole} = require('@web/../tests/helpers/utils'); @@ -9,99 +11,182 @@ function trim(s) { } /** - * Loads the template file, and executes all the test template in a - * qunit module $title + * Promise-based wrapper for QWeb2.Engine#add_template + * + * The base version is callbacks-based which is a bit shit, and it also has + * variable asynchronicity: it'll be async if passed a URL but not if passed + * either a Document or an XML string. + * + * Either way this converts the `(error, doc)` callback to a `Promise` + * as in it literally returns the parsed DOM Document. + * + * @param qweb the qweb instance to load the template into + * @param {String|Document} template qweb template (Document, template file, or template URL) */ -function loadTest(assert, template, context) { - var done = assert.async(); - assert.expect(1); - - var qweb = new window.QWeb2.Engine(); - var prom = new Promise(function (resolve, reject) { - qweb.add_template(qwebPath + template, function (error, doc) { - if (error) { - return reject(error); - } - resolve({ - qweb: qweb, - doc: doc - }); - }); +function add_template(qweb, template) { + return new Promise(function (resolve, reject) { + qweb.add_template(template, (error, doc) => error ? reject(error) : resolve(doc)); }); - - prom.then(function (r) { - var qweb = r.qweb; - var doc = r.doc; - assert.expect(doc.querySelectorAll('result').length); - - var templates = qweb.templates; - for (var template in templates) { - try { - if (!templates.hasOwnProperty(template)) { - continue; - } - // ignore templates whose name starts with _, they're - // helpers/internal - if (/^_/.test(template)) { - continue; - } - - var params = doc.querySelector('params#' + template); - var args = params ? JSON.parse(params.textContent) : (context ? _.clone(context) : {}); - - var results = doc.querySelector('result#' + template); - - assert.equal( - trim(qweb.render(template, args)), - trim(results.textContent.replace(new RegExp(String.fromCharCode(13), 'g'), '')), - template); - } catch (error) { - assert.notOk(error.stack || error, 'Rendering error'); - } - } - done(); - }); - return prom; } -QUnit.module('QWeb', { - before() { - this.oldConsole = window.console; - window.console = hushConsole; +/** + * Loads the template file, and executes all the test template in a + * qunit module $title + * + * @param assert QUnit assertion module + * @param {String|Document} templateFile template container to load + * @param {Object} [context] additional rendering context + */ +async function loadTest(assert, templateFile, context) { + const qweb = new window.QWeb2.Engine(); + const doc = await add_template(qweb, qwebPath + templateFile); + + assert.expect(doc.querySelectorAll('result').length); + + const templates = qweb.templates; + for (const template in templates) { + if (!templates.hasOwnProperty(template)) { + continue; + } + // ignore templates whose name starts with _, they're + // helpers/internal + if (/^_/.test(template)) { + continue; + } + + const results = doc.querySelector(`result#${template}`).textContent.replace(/\r/g, ''); + const params = doc.querySelector(`params#${template}`) || {textContent: 'null'}; + const args = {...JSON.parse(params.textContent), ...context}; + + try { + assert.equal(trim(qweb.render(template, args)), trim(results), template); + } catch (error) { + assert.notOk(error.stack || error, `Rendering error for ${template} (in ${templateFile} with context ${JSON.stringify(args)}).`); + } + } +} + +const table = [ + {name: 'Output', file: 'qweb-test-output.xml'}, + {name: 'Context-setting', file: 'qweb-test-set.xml'}, + {name: 'Conditionals', file: 'qweb-test-conditionals.xml'}, + {name: 'Attributes manipulation', file: 'qweb-test-attributes.xml'}, + {name: 'Templates calling (to the faraway pages)', file: 'qweb-test-call.xml', context: {True: true}}, + {name: 'Foreach', file: 'qweb-test-foreach.xml'}, + { + name: 'Global', file: 'qweb-test-global.xml', + // test uses python syntax + context: {bool: (v) => !!v ? 'True' : 'False'}, + fixture: { + before() { + this.WORD_REPLACEMENT = window.QWeb2.WORD_REPLACEMENT; + window.QWeb2.WORD_REPLACEMENT = _.extend( + {not: '!', None: 'undefined'}, + this.WORD_REPLACEMENT + ) + }, + after() { + window.QWeb2.WORD_REPLACEMENT = this.WORD_REPLACEMENT; + } + } }, - after() {window.console = this.oldConsole;} -}, function () { - QUnit.test('Output', function (assert) { - loadTest(assert, 'qweb-test-output.xml'); - }); - QUnit.test('Context-setting', function (assert) { - loadTest(assert, 'qweb-test-set.xml'); - }); - QUnit.test('Conditionals', function (assert) { - loadTest(assert, 'qweb-test-conditionals.xml'); - }); - QUnit.test('Attributes manipulation', function (assert) { - loadTest(assert, 'qweb-test-attributes.xml'); - }); - QUnit.test('Template calling (to the faraway pages)', function (assert) { - loadTest(assert, 'qweb-test-call.xml', {True: true}); - }); - QUnit.test('Foreach', function (assert) { - loadTest(assert, 'qweb-test-foreach.xml'); - }); - QUnit.test('Global', function (assert) { - // test use python syntax - var WORD_REPLACEMENT = window.QWeb2.WORD_REPLACEMENT; - window.QWeb2.WORD_REPLACEMENT = _.extend({not: '!', None: 'undefined'}, WORD_REPLACEMENT); - loadTest(assert, 'qweb-test-global.xml', {bool: function (v) { return !!v ? 'True' : 'False';}}) - .then(function () { - window.QWeb2.WORD_REPLACEMENT = WORD_REPLACEMENT; - }, function () { - window.QWeb2.WORD_REPLACEMENT = WORD_REPLACEMENT; - }); - }); - QUnit.test('Template inheritance', function (assert) { - loadTest(assert, 'qweb-test-extend.xml'); + {name: 'Template Inheritance', file: 'qweb-test-extend.xml'}, +]; +QUnit.module('QWeb', { + beforeEach() { + this.oldConsole = window.console; + window.console = hushConsole; + }, + afterEach() {window.console = this.oldConsole;} +}, () => { + for(const {name, file, context, fixture} of table) { + QUnit.test(name, async assert => { + // fake expect to avoid qunit being a pain in the ass, loadTest will + // update it + assert.expect(1); + if (fixture && 'before' in fixture) { await fixture.before(); } + try { + await loadTest(assert, file, context); + } finally { + if (fixture && 'after' in fixture) { await fixture.after(); } + } + }); + } + QUnit.test('escape', assert => { + // not strictly about qweb... + assert.expect(8); + assert.equal(_.escape('a'), 'a'); + assert.equal(_.escape(''), '<a>'); + assert.equal(_.escape({[_.escapeMethod]() { return 'a'; }}), 'a'); + assert.equal(_.escape({[_.escapeMethod]() { return ''; }}), ''); + assert.equal(_.escape(Markup('a')), 'a'); + assert.equal(_.escape(Markup('')), ''); + assert.equal(_.escape(Markup`a`), 'a'); + assert.equal(_.escape(Markup``), ''); }); + QUnit.module('t-out', {}, () => { + QUnit.test("basics", async assert => { + assert.expect(5); + const qweb = new QWeb2.Engine; + await add_template(qweb, ` +

+ `); + + assert.equal( + qweb.render('t-out', {value: 'test'}), + '

<i>test</i>

', + "regular t-out should just escape the contents" + ); + assert.equal( + qweb.render('t-out', {value: Markup('test')}), + '

test

', + "Markup contents should not be escaped" + ); + assert.equal( + qweb.render('t-out', {value: Markup`test ${1}`}), + '

test 1

', + "markup template string should not be escaped" + ); + const teststr = 'test'; + assert.equal( + qweb.render('t-out', {value: Markup`${teststr}`}), + '

<i>test</i>

', + "the markup template should not be escaped but what it uses should be" + ); + const testMarkup = Markup(teststr); + assert.equal( + qweb.render('t-out', {value: Markup`${testMarkup}`}), + '

test

', + "markupception" + ); + }); + QUnit.test("Set", async assert => { + assert.expect(4); + const qweb = new QWeb2.Engine; + await add_template(qweb, ` + + + + + + + + + + + + + + + + + `); + + assert.equal(trim(qweb.render('litval')), '<a/>'); + assert.equal(trim(qweb.render('body')), ''); + assert.equal(trim(qweb.render('value', {val: ''})), '<a/>'); + assert.equal(trim(qweb.render('bodyout', {val: ''})), '<a/>'); + }); + }) }); });