A valid JS code is considered as HTML comment

[nazarhussain]

Describe the bug

I am trying out a big bundled JS code in SES and faced with following error:

TypeError#1: Failed to load module "./html_comment_code.js" in package "test" (1 underlying failures: Possible HTML comment rejected at .../html_comment_code.js:5. (SES_HTML_COMMENT_REJECTED)

The file contains valid JS code, apparently it seems that SES is considering a specific JS expression as HTML comment.

Steps to reproduce

// html_comment_code.js
// That code is extracted from the big bundled package, so sorry for the variables names. 

const _0n$6 = 0;

export function pow2(x, power, modulo) {
  let res = x;
  while (power-- > _0n$6) {
    res *= res;
    res %= modulo;
  }
  return res;
}

Here is the code which I am trying to execute above in SES:

import "ses";
import url from "node:url";
import bundleSource from "@endo/bundle-source";
import {importBundle} from "@endo/import-bundle";
import {StaticModuleRecord} from "@endo/static-module-record";

lockdown();

const resolveHook = (spec, referrer) => {
  new URL(spec, referrer).toString();
};

const importHook = async (moduleSpecifier) => {
    const fullPath = import.meta.resolve(moduleSpecifier);

    if (fullPath.startsWith("file://")) {
      const sourceBundlePath = url.fileURLToPath(fullPath);
      const sourceBundleP = await bundleSource(sourceBundlePath);

      // To check if we had a valid bundle 
      const bundle = await importBundle(sourceBundleP, {endowments: {console}});
      console.log(bundle);

      return new StaticModuleRecord(sourceBundleP, moduleSpecifier);
    }
};

const c = new Compartment(
  {
    print: harden(console.log),
  },
  {},
  {
    name: "test",
    resolveHook,
    importHook,
  }
);

await c.import("./html_comment_code.js");

Expected behavior

This code is simple JS expression and should work well in SES.

Platform environment

  System:
    OS: macOS 14.0
    CPU: (8) x64 Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
    Memory: 645.75 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.7.0 - ~/.asdf/installs/nodejs/20.7.0/bin/node
    Yarn: 1.22.19 - ~/.asdf/installs/nodejs/20.7.0/bin/yarn
    npm: 10.1.0 - ~/.asdf/plugins/nodejs/shims/npm
  Browsers:
    Brave Browser: 118.1.59.122
    Chrome: 118.0.5993.88
    Firefox: 112.0.1
    Safari: 17.0

Additional context

Screenshots

[nazarhussain]

This is the bundled code, which clearly strips the space between -- and > and made it an HTML comment.

{
    moduleFormat: 'getExport',
    source: "function getExport() { 'use strict'; let exports = {}; const module = { exports }; 'use strict';\n" +
      '\n' +
      "Object.defineProperty(exports,'__esModule',{value:true});\n" +
      '\n' +
      'const _0n$6=0;\n' +
      '\n' +
      'function pow2(x,power,modulo){\n' +
      'let res=x;\n' +
      'while(power-->_0n$6){\n' +
      'res*=res;\n' +
      'res%=modulo;\n' +
      '}\n' +
      'return res;\n' +
      '}\n' +
      '\n' +
      'exports.pow2=pow2;\n' +
      '\n' +
      'return module.exports;\n' +
      '}\n' +
      '//# sourceURL=/bundled-source/.../html_comment_code.js\n',
    sourceMap: '//# sourceURL=/bundled-source/.../html_comment_code.js\n'
  }

[kriskowal]

The SES censor is working as expected. SES can’t parse JavaScript to distinguish HTML comments from the so-called “down-to operator” (-->). The StaticModuleRecord constructor does parse JavaScript to do the module-to-program transform necessary to stuff it into compartment.evaluate and stand on the confinement guarantees SES can currently only emulate with eval. We’re working in parallel with TC39 to make confined evaluation a language feature, which would not have this censorship wart.

However, we’ve largely been able to compensate for the censor using an “evasive transform” in bundleSource with the default “endoZipBase64” format. The bundle you’re showing in in getExport format which is intended to generate a bundle that can be passed to compartment.evaluate but not intended for importHook and StaticModuleRecord. The bundler for getExport format is old and uses Rollup. Rollup is almost certainly the culprit that is collapsing -- and > into --> and it almost certainly can be avoided, depending on what you’re trying to achieve.

The importHook should not need to use bundleSource. It should be safe to pass the original source for a single ESM module through the StaticModuleRecord constructor.

Aside: StaticModuleRecord is likely to be called ModuleSource in a future version of JavaScript.

[kriskowal]

Notably, the evasive transform does not yet inject a space between -- and > if it appears in the original source. That would be a worthwhile compensation and would only slightly mar our desire to minimize differences between the source and target text. We prefer to avoid source maps in production since they can be used by an attacker to confuse an auditor.

The evasive transform also does not yet break up strings with --> in them, but does rewrite --> to a similar expression using either X or a dash homograph when it appears in comments.

[kriskowal]

I’ve reproduced this bug locally. Thank you for reporting!

Steps to reproduce: (In endo/packages/bundle-source)

1. Write a temp.js from the issue description.

// html_comment_code.js
// That code is extracted from the big bundled package, so sorry for the variables names. 

const _0n$6 = 0;

export function pow2(x, power, modulo) {
  let res = x;
  while (power-- > _0n$6) {
    res *= res;
    res %= modulo;
  }
  return res;
}

2. Create a bundle `bundle-source --cache-json bundles temp.js temp
3. Extract the zip from the bundle: jq -r .endoZipBase64 | base64 -d > temp.zip
4. Unpack the zip: unzip temp.zip -d temp
5. Extract the generated output jq -r .__syncModuleProgram__ temp/\@endo/bundle-source-v3.0.1/temp.js

({   imports: $h‍_imports,   liveVar: $h‍_live,   onceVar: $h‍_once,   importMeta: $h‍____meta, }) => (function () {   $h‍_imports([]);Object.defineProperty(pow2, 'name', {value: "pow2"});$h‍_once.pow2(pow2);   /* html_comment_code.js*/
/* That code is extracted from the big bundled package, so sorry for the variables names. */

const _0n$6=0;

function        pow2(x,power,modulo){
let res=x;
while(power-->_0n$6){
res*=res;
res%=modulo;
 }
return res;
 }
})()

[kriskowal]

A really satisfying solution to this problem would be to convince Babel to preserve the whitespace between -- and > in the above transformation (the authorities @nicolo-ribaudo have been informed).

But, an adequate mitigation for this specific case might be to augment @endo/evasive-transforms to change all occurrences of x-- > y to (x--, x > y).

[gibson042]

But, an adequate mitigation for this specific case might be to augment @endo/evasive-transforms to change all occurrences of x-- > y to (x--, x > y).

That's not equivalent because the completion value of x-- is pre-decrement. It'd need to be something like [x--][0] > y.

[erights]

[x--][0] > y

I like it!

[kriskowal]

That's not equivalent because the completion value of x-- is pre-decrement. It'd need to be something like [x--][0] > y.

I do not understand. My understanding of the comma operator is that the completion value is the completion value of the final expression. What am I missing?

[edit]: Confirmed difference in behavior. I seem to be oblivious of a JavaScript complication to how expressions are traced and when values are captured from identifiers.

[edit]: Nope. I’d simply forgotten what @erights states below.

[erights]

I think the term "completion value" may be causing confusion.

In (x--, x > y), the value being compared to y is the value of x after decrementing. In x-- > y the value being compared to y is the value of x before decrementing.