rejectHtmlComments throws on bignumber.js comment

[kumavis]

https://github.com/MikeMcl/bignumber.js/blob/986fd70e514e58e86d43bc9944547d82658e47ae/bignumber.js#L2427

const s = '// e.g 0.0009999 (e-4) --> 0.001 (e-3), so adjust s so the rounding digits'
const htmlCommentPattern = new RegExp(`(?:${'<'}!--|--${'>'})`);

function rejectHtmlComments(s) {
  const index = s.search(htmlCommentPattern);
  if (index !== -1) {
    const linenum = s.slice(0, index).split('\n').length; // more or less
    throw new SyntaxError(
      `possible html comment syntax rejected around line ${linenum}`
    );
  }
}

rejectHtmlComments(s)
/*
Thrown:
SyntaxError: possible html comment syntax rejected around line 1
*/

[kumavis]

There's doesn't seem to be a way to disable this rejection. Silly too as we never inline our js into html, only via script tag's src attribute.

[erights]

There's doesn't seem to be a way to disable this rejection. Silly too as we never inline our js into html, only via script tag's src attribute.

The bizarreness of JS's treatment of so-called html-like comments occurs in all browser-based JS engines even when used outside the browser. For example, Node uses v8. On Node:

> 2 + <!-- 3
... 4
6
> 

[erights]

The only JS engine that currently does not do this is Moddable's XS (attn @phoddie ). At the Berlin tc39 mtg, I proposed that this horrible syntax become standard, because the only thing worse than horrible syntax is optional horrible syntax. If we can't kill it, we should make it mandatory, or better, allow (but not require) that it be rejected as a SyntaxError. We will be debating this again at the next tc39 mtg (attn @waldemarhorwat ).

[kumavis]

@erights could rejectHtmlComments be adjusted to only reject comment starts (<!--) and not comment terminals (-->) ?

[erights]

We could add an option to control this, ala https://github.com/Agoric/realms-shim/issues/34 . Like that one, any such option should default to the safe setting. However, unlike that one, the unsafe setting on this one is so invisibly dangerous that I nervous about providing such an option. Perhaps if the name of the option were sufficiently alarming ;)

[erights]

@erights could rejectHtmlComments be adjusted to only reject comment starts () ?

We could also do that by an option. But the option would again need to default to the safe setting. Perhaps this finer distinction would be less dangerous? I am confused by the behaviors I'm seeing where these two different html-like comments act differently.

[kumavis]

Look at this pretty graph / html comment https://npmfs.com/package/react-dom/15.6.2/lib/Transaction.js

[michaelfig]

@kumavis, You could make your life less strict with a rewrite transform from apparent HTML comments into something that usually throws an error outside a comment block (but does not trigger the aggressive rejectDangerousSources):

const htmlCommentReplacement = ';@@HTML_COMMENT@@;';
const htmlCommentPattern = new RegExp(`(?:${'<'}!--|--${'>'})`, 'g');
const htmlCommentTransform = {
  rewrite(rs) {
    const src = rs.src.replace(htmlCommentPattern, htmlCommentReplacement);
    return { ...rs, src };
  },
};
const transforms = [htmlCommentTransform];
// Test it out:
const s = SES.makeSESRootRealm({ transforms });
s.evaluate(`<!-- throws with syntaxerror -->`);
s.evaluate(`123 // <!-- no problem nested in comments -->`);
// or in a compartment:
const c = s.global.Realm.makeCompartment({ transforms });
c.evaluate(`<!-- throws with syntaxerror -->`);
c.evaluate(`123 // <!-- no problem in comments -->`);

This approach is kludgey enough that I wouldn't want to expose it as an SES flag, but it may be worthy of consideration for people like you who need to evaluate sources that contain ASCII-art. 😉