I agree to sanitize the output in the pipeline (see also my comment in adobe/helix-pipeline#263 (comment)).

But I think we should still aim for reach maximum security in htlengine. Someone might use it independently and expect the output to be safe.

from htlengine.

I think he meant to use it in the pipeline on the vdom. I don't think that we need to apply it again on the HTL output.
If we wanted to guard the output again after all, we should do it in the pipeline in order to support all kind of engines, not just htl.

from htlengine.

I tested the performance with some larger files:

describe('Engine sanitizer Performance test', async () => {
  // todo: enable large tests once performance issues are addressed
  const TEST_FILES = ['simple2.html', '400kb.htm', '700kb.htm'];

  TEST_FILES.forEach((filename) => {
    it(`produces htl output for ${filename}`, async () => {
      const now = Date.now();
      const filePath = path.resolve(__dirname, 'templates', filename);
      const source = await fse.readFile(filePath, 'utf-8');
      await engine({
        source,
        // eslint-disable-next-line no-template-curly-in-string
      }, '${source @ context="html"}');
      console.log(`Time for ${filename}: ${Date.now()-now}ms`);
    }).timeout(30000);
  });
});

with dom purify:

Time for simple2.html: 598ms
Time for 400kb.htm: 923ms
Time for 700kb.htm: 1005ms

with original sanitizer:

Time for simple2.html: 78ms
Time for 400kb.htm: 64ms
Time for 700kb.htm: 104ms

Given the fact, that we output a lot of HTML through the html context, I'm reluctant to use dompurify here. can we find the edge cases, where the current sanitizer doesn't work?

from htlengine.

I agree that performance is an important criterion. But in the one case where a customer gets hacked, it becomes moot :)

After a quick test using the examples from a08b0e7, I wasn't able to break our current sanitizer. But I'm not an expert security researcher by any means, and my concern regarding the dead codebase and community remains. The next new attack vector might not be covered...

from htlengine.

So... how should we proceed with this issue :)

If the majority thinks the current sanitizer is safe enough, I suggest we close this issue and the corresponding PR...

from htlengine.

I'd close it for now, but there is a slightly crazy idea I want to discuss with @tripodsan at the hackathon that could make it viable again (but this idea would break the rest of htlengine).

from htlengine.

there is a slightly crazy idea I want to discuss with @tripodsan at the hackathon that could make it viable again (but this idea would break the rest of htlengine).

:-) DOM based HTL engine ?

from htlengine.

:-) DOM based HTL engine ?

DOM based everything!

If we build HTLEngine the same way the JSX processor is built (by using h, or React.createElement or a compatible API) we'd get a proper DOM to work with in the pipeline (I'd change the downstream part of the HTML pipeline to work entirely on DOM instead of strings or HTAST) and we'd get a couple of features that make HTL in the browser more interesting such as support for Virtual DOM diffing.

from htlengine.