[IMP] runtime/utils: export htmlEscape and add tests

markup tag function requires markup awareness to determine whether a
given parameter should be escaped or not.

This implies that pre-escaped content should be properly marked'ed up to
avoid double escaping. Having to manually wrap all calls to escape
with markup is cumbersome and prone to issues (on top of having to be
validated by the security team for no reason).

This commit introduces a markup-aware escape function to resolve those
issues.

Author: seb-odoo

src/runtime/index.ts

@@ -41,7 +41,7 @@ export { useComponent, useState } from "./component_node";
 export { status } from "./status";
 export { reactive, markRaw, toRaw } from "./reactivity";
 export { useEffect, useEnv, useExternalListener, useRef, useChildSubEnv, useSubEnv } from "./hooks";
-export { batched, EventBus, whenReady, loadFile, markup } from "./utils";
+export { batched, EventBus, htmlEscape, whenReady, loadFile, markup } from "./utils";
 export {
   onWillStart,
   onMounted,

src/runtime/utils.ts

@@ -111,15 +111,15 @@ export async function loadFile(url: string): Promise<string> {
  */
 export class Markup extends String {}
 
-function _escapeHtml(str: any): string | Markup {
+export function htmlEscape(str: any): Markup {
   if (str instanceof Markup) {
     return str;
   }
   if (str === undefined) {
-    return "";
+    return markup("");
   }
   if (typeof str === "number") {
-    return String(str);
+    return markup(String(str));
   }
   [
     ["&", "&amp;"],
@@ -131,7 +131,7 @@ function _escapeHtml(str: any): string | Markup {
   ].forEach((pairs) => {
     str = String(str).replace(new RegExp(pairs[0], "g"), pairs[1]);
   });
-  return str;
+  return markup(str);
 }
 
 /*
@@ -153,7 +153,7 @@ export function markup(
   let acc = "";
   let i = 0;
   for (; i < placeholders.length; ++i) {
-    acc += strings[i] + _escapeHtml(placeholders[i]);
+    acc += strings[i] + htmlEscape(placeholders[i]);
   }
   acc += strings[i];
   return new Markup(acc);

tests/utils.test.ts

@@ -1,4 +1,4 @@
-import { batched, EventBus, markup } from "../src/runtime/utils";
+import { batched, EventBus, htmlEscape, markup } from "../src/runtime/utils";
 import { nextMicroTick } from "./helpers";
 
 describe("event bus behaviour", () => {
@@ -78,6 +78,61 @@ describe("markup", () => {
     const html = markup("<blink>Hello</blink>");
     expect(html).toBeInstanceOf(Markup);
   });
+  describe("htmlEscape", () => {
+    test("htmlEscape escapes text", () => {
+      const res = htmlEscape("<p>test</p>");
+      expect(res.toString()).toBe("&lt;p&gt;test&lt;/p&gt;");
+      expect(res).toBeInstanceOf(Markup);
+    });
+    test("htmlEscape keeps html markup", () => {
+      const res = htmlEscape(markup("<p>test</p>"));
+      expect(res.toString()).toBe("<p>test</p>");
+      expect(res).toBeInstanceOf(Markup);
+    });
+    test("htmlEscape produces empty string on undefined", () => {
+      const res = htmlEscape(undefined);
+      expect(res.toString()).toBe("");
+      expect(res).toBeInstanceOf(Markup);
+    });
+    test("htmlEscape produces string from number", () => {
+      const res = htmlEscape(10);
+      expect(res.toString()).toBe("10");
+      expect(res).toBeInstanceOf(Markup);
+    });
+    test("htmlEscape produces string from boolean", () => {
+      const res = htmlEscape(false);
+      expect(res.toString()).toBe("false");
+      expect(res).toBeInstanceOf(Markup);
+    });
+    test("htmlEscape correctly escapes various links", () => {
+      expect(htmlEscape("<a>this is a link</a>").toString()).toBe(
+        "&lt;a&gt;this is a link&lt;/a&gt;"
+      );
+      expect(htmlEscape(`<a href="https://www.odoo.com">odoo<a>`).toString()).toBe(
+        `&lt;a href=&quot;https://www.odoo.com&quot;&gt;odoo&lt;a&gt;`
+      );
+      expect(htmlEscape(`<a href='https://www.odoo.com'>odoo<a>`).toString()).toBe(
+        `&lt;a href=&#x27;https://www.odoo.com&#x27;&gt;odoo&lt;a&gt;`
+      );
+      expect(htmlEscape("<a href='https://www.odoo.com'>Odoo`s website<a>").toString()).toBe(
+        `&lt;a href=&#x27;https://www.odoo.com&#x27;&gt;Odoo&#x60;s website&lt;a&gt;`
+      );
+    });
+    test("htmlEscape doesn't escape already escaped content", () => {
+      const res = htmlEscape("<p>test</p>");
+      expect(res.toString()).toBe("&lt;p&gt;test&lt;/p&gt;");
+      expect(res).toBeInstanceOf(Markup);
+      const res2 = htmlEscape(res);
+      expect(res2.toString()).toBe("&lt;p&gt;test&lt;/p&gt;");
+      expect(res2).toBeInstanceOf(Markup);
+      expect(res2).toBe(res);
+    });
+    test("htmlEscape returns markup even for only-safe text", () => {
+      const res = htmlEscape("safe");
+      expect(res.toString()).toBe("safe");
+      expect(res).toBeInstanceOf(Markup);
+    });
+  });
   describe("tag function", () => {
     test("interpolated values are escaped", () => {
       const maliciousInput = "<script>alert('💥💥')</script>";
@@ -99,5 +154,11 @@ describe("markup", () => {
       const html = markup`<img src="${imgUrl}">`;
       expect(html.toString()).toBe(`<img src="lol&quot; onerror=&quot;alert(&#x27;xss&#x27;)">`);
     });
+    test("already escaped content is not escaped again", () => {
+      const res = htmlEscape("<p>test</p>");
+      expect(res.toString()).toBe("&lt;p&gt;test&lt;/p&gt;");
+      const html = markup`${res}`;
+      expect(html.toString()).toBe("&lt;p&gt;test&lt;/p&gt;");
+    });
   });
 });