#7265: Move Nunjucks template validation to sandbox (#7426)

---------
Co-authored-by: Todd Schiller <todd.schiller@gmail.com>

src/analysis/analysisVisitors/requestPermissionAnalysis.ts

@@ -106,13 +106,12 @@ class RequestPermissionAnalysis extends AnalysisVisitorABC {
 
       const permissionsValue = `${parsedURL.origin}/*`;
 
-      const permissionCheckPromise = browser.permissions
-        .contains({
-          origins: [parsedURL.href],
-        })
-        // eslint-disable-next-line promise/prefer-await-to-then -- need the complete Promise
-        .then((hasPermission) => {
-          if (!hasPermission) {
+      this.permissionCheckPromises.push(
+        (async () => {
+          const hasPermissions = await browser.permissions.contains({
+            origins: [parsedURL.href],
+          });
+          if (!hasPermissions) {
             this.annotations.push({
               position: nestedPosition(position, "config.url"),
               message:
@@ -134,17 +133,18 @@ class RequestPermissionAnalysis extends AnalysisVisitorABC {
               ],
             });
           }
-        });
-
-      this.permissionCheckPromises.push(permissionCheckPromise);
+        })(),
+      );
     }
   }
 
   override async run(extension: ModComponentFormState): Promise<void> {
     super.run(extension);
 
     // Use allSettled because `browser.permissions.contains` errors out for certain cases, e.g., malformed URLs
-    await allSettled(this.permissionCheckPromises, { catch: "ignore" });
+    await allSettled(this.permissionCheckPromises, {
+      catch: "ignore",
+    });
   }
 }
 

src/analysis/analysisVisitors/templateAnalysis.test.ts

@@ -18,11 +18,29 @@
 import { type BrickPosition } from "@/bricks/types";
 import TemplateAnalysis from "./templateAnalysis";
 import { toExpression } from "@/utils/expressionUtils";
+import { RemoteMethod } from "@/bricks/transformers/remoteMethod";
+import { triggerFormStateFactory } from "@/testUtils/factories/pageEditorFactories";
+import { brickConfigFactory } from "@/testUtils/factories/brickFactories";
+import { type Expression } from "@/types/runtimeTypes";
 
 const position: BrickPosition = {
   path: "test.path.property",
 };
 
+function blockExtensionFactory(expression: Expression<unknown>) {
+  const extension = triggerFormStateFactory();
+  extension.extension.blockPipeline = [
+    brickConfigFactory({
+      id: RemoteMethod.BLOCK_ID,
+      config: {
+        url: expression,
+      },
+    }),
+  ];
+
+  return extension;
+}
+
 describe("TemplateAnalysis", () => {
   const validNunjucksTemplates = [
     "{{ username }}",
@@ -46,19 +64,27 @@ describe("TemplateAnalysis", () => {
   ];
   test.each(mustacheOnlyTemplates)(
     "accepts valid mustache [%s]",
-    (template) => {
+    async (template) => {
       const analysis = new TemplateAnalysis();
-      analysis.visitExpression(position, toExpression("mustache", template));
+      const extension = blockExtensionFactory(
+        toExpression("mustache", template),
+      );
+
+      await analysis.run(extension);
 
       expect(analysis.getAnnotations()).toHaveLength(0);
     },
   );
 
   test.each(mustacheOnlyTemplates)(
     "rejects mustache template in non-mustache expression [%s]",
-    (template) => {
+    async (template) => {
       const analysis = new TemplateAnalysis();
-      analysis.visitExpression(position, toExpression("nunjucks", template));
+      const extension = blockExtensionFactory(
+        toExpression("nunjucks", template),
+      );
+
+      await analysis.run(extension);
 
       expect(analysis.getAnnotations()).toHaveLength(1);
     },
@@ -68,9 +94,14 @@ describe("TemplateAnalysis", () => {
 
   test.each(invalidNunjucksTemplates)(
     "rejects invalid nunjucks [%s]",
-    (template) => {
+    async (template) => {
       const analysis = new TemplateAnalysis();
-      analysis.visitExpression(position, toExpression("nunjucks", template));
+      const extension = blockExtensionFactory(
+        toExpression("nunjucks", template),
+      );
+
+      await analysis.run(extension);
+
       expect(analysis.getAnnotations()).toHaveLength(1);
     },
   );

src/analysis/analysisVisitors/templateAnalysis.ts

@@ -21,7 +21,6 @@ import {
 } from "@/analysis/analysisTypes";
 import { type BrickPosition } from "@/bricks/types";
 import { isMustacheOnly } from "@/components/fields/fieldUtils";
-import { Template } from "nunjucks";
 import PipelineExpressionVisitor from "@/bricks/PipelineExpressionVisitor";
 import { type ModComponentFormState } from "@/pageEditor/starterBricks/formStateTypes";
 import { type Expression } from "@/types/runtimeTypes";
@@ -30,6 +29,8 @@ import {
   isNunjucksExpression,
   isTemplateExpression,
 } from "@/utils/expressionUtils";
+import { validateNunjucksTemplate } from "@/sandbox/messenger/api";
+import { getErrorMessage } from "@/errors/errorHelpers";
 
 const TEMPLATE_ERROR_MESSAGE =
   "Invalid text template. Read more about text templates: https://docs.pixiebrix.com/developing-mods/developer-concepts/text-template-guide";
@@ -41,6 +42,10 @@ type PushAnnotationArgs = {
 };
 
 class TemplateAnalysis extends PipelineExpressionVisitor implements Analysis {
+  // XXX: for now we handle asynchronous pipeline traversal by gathering all the promises and awaiting them all
+  // see discussion https://github.com/pixiebrix/pixiebrix-extension/pull/4013#discussion_r944690969
+  private readonly nunjuckValidationPromises: Array<Promise<void>> = [];
+
   get id() {
     return "template";
   }
@@ -50,10 +55,12 @@ class TemplateAnalysis extends PipelineExpressionVisitor implements Analysis {
     return this.annotations;
   }
 
-  run(extension: ModComponentFormState): void {
+  async run(extension: ModComponentFormState): Promise<void> {
     this.visitRootPipeline(extension.extension.blockPipeline, {
       extensionPointType: extension.type,
     });
+
+    await Promise.all(this.nunjuckValidationPromises);
   }
 
   private pushErrorAnnotation({
@@ -74,7 +81,10 @@ class TemplateAnalysis extends PipelineExpressionVisitor implements Analysis {
     position: BrickPosition,
     expression: Expression<unknown>,
   ): void {
-    if (!isTemplateExpression(expression)) {
+    if (
+      !isTemplateExpression(expression) ||
+      expression.__value__.trim() === ""
+    ) {
       return;
     }
 
@@ -90,19 +100,19 @@ class TemplateAnalysis extends PipelineExpressionVisitor implements Analysis {
         expression,
       });
     } else if (isNunjucksExpression(expression)) {
-      try {
-        // eslint-disable-next-line no-new
-        new Template(expression.__value__, undefined, undefined, true);
-      } catch (error) {
-        // @ts-expect-error nunjucks error does have message property
-        const failureCause = (error.message as string)
-          ?.replace("(unknown path)", "")
-          .trim();
-
-        const message = `Invalid template: ${failureCause}.`;
-
-        this.pushErrorAnnotation({ position, message, expression });
-      }
+      this.nunjuckValidationPromises.push(
+        (async () => {
+          try {
+            await validateNunjucksTemplate(expression.__value__);
+          } catch (error) {
+            this.pushErrorAnnotation({
+              position,
+              message: getErrorMessage(error),
+              expression,
+            });
+          }
+        })(),
+      );
     }
   }
 }

src/contentScript/sidebarController.tsx

@@ -263,7 +263,7 @@ export function removeExtensions(extensionIds: UUID[]): void {
   console.debug("sidebarController:removeExtensions", { extensionIds });
 
   // `panels` is const, so replace the contents
-  const current = panels.splice(0, panels.length);
+  const current = panels.splice(0);
   panels.push(...current.filter((x) => !extensionIds.includes(x.extensionId)));
   void renderPanelsIfVisible();
 }
@@ -286,7 +286,7 @@ export function removeExtensionPoint(
   });
 
   // `panels` is const, so replace the contents
-  const current = panels.splice(0, panels.length);
+  const current = panels.splice(0);
   panels.push(
     ...current.filter(
       (x) =>

src/sandbox/messenger/api.ts

@@ -51,6 +51,8 @@ export type TemplateRenderPayload = {
   autoescape: boolean;
 };
 
+export type TemplateValidatePayload = string;
+
 export async function renderNunjucksTemplate(payload: TemplateRenderPayload) {
   return (await isSandboxed())
     ? postMessage({
@@ -61,6 +63,18 @@ export async function renderNunjucksTemplate(payload: TemplateRenderPayload) {
     : directApi.renderNunjucksTemplate(payload);
 }
 
+export async function validateNunjucksTemplate(
+  payload: TemplateValidatePayload,
+) {
+  return (await isSandboxed())
+    ? postMessage({
+        recipient: await loadSandbox(),
+        payload,
+        type: "VALIDATE_NUNJUCKS",
+      })
+    : directApi.validateNunjucksTemplate(payload);
+}
+
 export async function renderHandlebarsTemplate(payload: TemplateRenderPayload) {
   return (await isSandboxed())
     ? postMessage({

src/sandbox/messenger/executor.ts

@@ -15,7 +15,11 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-import { type JavaScriptPayload, type TemplateRenderPayload } from "./api";
+import {
+  type TemplateValidatePayload,
+  type JavaScriptPayload,
+  type TemplateRenderPayload,
+} from "./api";
 import { isErrorObject } from "@/errors/errorHelpers";
 import {
   BusinessError,
@@ -45,6 +49,27 @@ export async function renderNunjucksTemplate(
   }
 }
 
+export async function validateNunjucksTemplate(
+  template: TemplateValidatePayload,
+): Promise<void> {
+  // Webpack caches the module import, so doesn't need to cache via lodash's `once`
+  const { compile } = await import(
+    /* webpackChunkName: "nunjucks" */ "nunjucks"
+  );
+
+  try {
+    // @ts-expect-error -- The last parameter is not in the types yet
+    compile(template, undefined, undefined, true);
+  } catch (error) {
+    if (isErrorObject(error) && error.name === "Template render error") {
+      const failureCause = error.message?.replace("(unknown path)", "").trim();
+      throw new InvalidTemplateError(failureCause, template);
+    }
+
+    throw error;
+  }
+}
+
 export async function renderHandlebarsTemplate(
   payload: TemplateRenderPayload,
 ): Promise<string> {

src/sandbox/messenger/registration.ts

@@ -21,11 +21,13 @@ import { addPostMessageListener } from "@/utils/postMessage";
 import {
   renderHandlebarsTemplate,
   renderNunjucksTemplate,
+  validateNunjucksTemplate,
   runUserJs,
 } from "./executor";
 
 export default function registerMessenger(): void {
   addPostMessageListener("RENDER_NUNJUCKS", renderNunjucksTemplate);
+  addPostMessageListener("VALIDATE_NUNJUCKS", validateNunjucksTemplate);
   addPostMessageListener("RENDER_HANDLEBARS", renderHandlebarsTemplate);
   addPostMessageListener("RUN_USER_JS", runUserJs);
   addPostMessageListener("SANDBOX_PING", async (payload) => "pong");

src/starterBricks/contextMenu.ts

@@ -174,7 +174,7 @@ export abstract class ContextMenuStarterBrickABC extends StarterBrickABC<Context
 
   override uninstall({ global = false }: { global?: boolean }): void {
     // NOTE: don't uninstall the mouse/click handler because other context menus need it
-    const extensions = this.modComponents.splice(0, this.modComponents.length);
+    const extensions = this.modComponents.splice(0);
     if (global) {
       for (const extension of extensions) {
         void uninstallContextMenu({ extensionId: extension.id });

src/starterBricks/menuItemExtension.ts

@@ -323,7 +323,7 @@ export abstract class MenuItemStarterBrickABC extends StarterBrickABC<MenuItemSt
     const menus = [...this.menus.values()];
 
     // Clear so they don't get re-added by the onNodeRemoved mechanism
-    const extensions = this.modComponents.splice(0, this.modComponents.length);
+    const extensions = this.modComponents.splice(0);
     this.menus.clear();
 
     if (extensions.length === 0) {

src/starterBricks/sidebarExtension.ts

@@ -158,7 +158,7 @@ export abstract class SidebarStarterBrickABC extends StarterBrickABC<SidebarConf
   }
 
   public override uninstall(): void {
-    const extensions = this.modComponents.splice(0, this.modComponents.length);
+    const extensions = this.modComponents.splice(0);
     this.clearModComponentInterfaceAndEvents(extensions.map((x) => x.id));
     removeExtensionPoint(this.id);
     console.debug(

src/utils/postMessage.ts

@@ -38,7 +38,7 @@ import { type JsonValue } from "type-fest";
 
 const TIMEOUT_MS = 3000;
 
-type Payload = JsonValue;
+type Payload = JsonValue | void;
 
 // eslint-disable-next-line local-rules/persistBackgroundData -- Function
 const log = process.env.SANDBOX_LOGGING ? console.debug : () => {};
@@ -56,7 +56,7 @@ export interface PostMessageInfo {
   recipient: Window;
 }
 
-type PostMessageListener = (payload?: Payload) => Promise<Payload>;
+type PostMessageListener = (payload?: Payload) => Promise<Payload | void>;
 
 /** Use the postMessage API but expect a response from the target */
 export default async function postMessage({