Some settings (e.g., `HTML_INTEGRATION_POINTS`, and likely others) are "sticky"

[Zlatkovsky]

When running DOMPurify.sanitize(<...>), some settings are "sticky".

Specifically, if passing in the HTML_INTEGRATION_POINTS: {'foreignobject': true} on the config the first time, the setting will carry over even to future invocations of sanitize that do not have the HTML_INTEGRATION_POINTS on the config. That shouldn't be the case... right?

Running example:

1. Open https://zlatkovsky.github.io/DOMPurifyMystery/
2. Select version 3.2.4 or 3.2.6 in the "Select a version" dropdown. Leave the "HTML integration points" checkbox as OFF for now.
3. Click "Sanitize!". Note that in the "After sanitization" section, the text in the blue box is missing. This is a known issue due to DOMPurify 3.1.7 breaks Mermaid diagrams using foreignObject #1002 .
   
4. Now turn ON the "Enable HTML integration points" checkbox and press "Sanitize!" again. This time the label will show up in the blue box, as expected.
5. Finally, turn the "Enable HTML integration points" OFF again and press "Sanitize!"

I would have expected the result of #5 to be the same as #3, i.e., the label should be missing again since the checkbox is OFF. But instead, the label is still visible because the HTML_INTEGRATION_POINTS isn't reset.

The source code for the above site can be found at https://github.com/Zlatkovsky/DOMPurifyMystery/blob/main/index.html.

Tracing through DOMPurify code, it appears that:

1. On invocation of DOMPurify.sanitize, _parseConfig is called:
   
2. Specifically, this means that the following line is invoked:

    HTML_INTEGRATION_POINTS = cfg.HTML_INTEGRATION_POINTS || HTML_INTEGRATION_POINTS;

3. If the first call to DOMPurify.sanitize had HTML_INTEGRATION_POINTS: {'foreignobject': true} in the config, the bundle-scoped HTML_INTEGRATION_POINTS variable will be set.
4. On follow-up invocations of DOMPurify.sanitize that do NOT have HTML_INTEGRATION_POINTS set, the line from #2 is hit again. Because cfg.HTML_INTEGRATION_POINTS is undefined, HTML_INTEGRATION_POINTS gets set to just itself. BUT, since it never gets reset across DOMPurify.sanitize invocations, it just stays as whatever the last config that had this setting set it to.

[cure53]

Hey, thanks for filing this. And I think you are right, this setting should not be sticky.

Do you want to spin up a PR per chance? We plan to release 3.2.7 soon, then this change could be a part of it 😄

[Zlatkovsky]

Is it fair to say that all variables should be reset each time (unless a “setConfig” had been called)?

[cure53]

I am tempted to say yes.

[Zlatkovsky]

@cure53 , sure, I can make the change. I believe it was only two variables that were sticky, HTML_INTEGRATION_POINTS and MATHML_TEXT_INTEGRATION_POINTS

The interesting question is… is this a patch/minor level change, or should it be part of a major breaking change? Technically, it will be a difference in behavior. And it actually does have a chance to break applications: The reason I noticed this in the first place is that the project I was working on was apparently relying on the happy little accident of HTML_INTEGRATION_POINTS being sticky, and would be broken by the proposed bug fix of “un-sticking” the sticky settings!

In a way, it’s similar to to #1002. There in one of the comments you say that it was a patch/minor change because “we didn't break anything knowingly. Our tests were green, our API unchanged - so no breakage we can observe”. But here we know — from me — that at least in one case it would have broken an application. Then again, it’s a pretty fringe case, in code that was written incorrectly in the first place…

Curious to hear your opinion.

And I’ll prepare a PR in the meantime, in the next couple days.

[Zlatkovsky]

As a counter-argument to waiting, though, maybe it is patching a (small) security hole?.. That some part of the application was initially setting “ HTML_INTEGRATION_POINTS”, but during later sanitization having it set is actually harmful (too permissive)?

I’m not entirely sure what this setting does and how much of a hole it opens up. You or others would be the much better judge.

[cure53]

I think we would want to jump up to version 3.3.x with this one as it might indeed break things for power-users 🙂

As a counter-argument to waiting, though, maybe it is patching a (small) security hole?

Just as Schrödinger's breakage, it might be also Schrödinger's security hole. We don't know and likely won't until we have a look, and it depends on now folks use the library and how they curated their config file.

So, my current thinking goes direction 3.3.x but no security release needed.

[Zlatkovsky]

Sounds good.

One more question: today’s behavior is that unless specified, the code uses the defaults:

let MATHML_TEXT_INTEGRATION_POINTS = addToSet({}, [
    'mi',
    'mo',
    'mn',
    'ms',
    'mtext',
  ]);

 let HTML_INTEGRATION_POINTS = addToSet({}, ['annotation-xml']);

But then once something is specified for these properties on the config, the defaults are dropped. This seems different than some of the other properties, that seem to keep the defaults and just add to them:

ALLOWED_TAGS = objectHasOwnProperty(cfg, 'ALLOWED_TAGS')
      ? addToSet({}, cfg.ALLOWED_TAGS, transformCaseFunc)
      : DEFAULT_ALLOWED_TAGS;

Which should it be?

[cure53]

I think it should be dropping the defaults, as otherwise we might not have any way to actually get rid of existing integration points, no?

[Zlatkovsky]

Right... I was somehow misreading the code :-) for ALLOWED_TAGS, etc. Sounds good, we should indeed be dropping the defaults.

[mhmdhed1111-arch]

1> When running DOMPurify.sanitize(<...>), some settings are "sticky".

Specifically, if passing in the HTML_INTEGRATION_POINTS: {'foreignobject': true} on the config the first time, the setting will carry over even to future invocations of sanitize that do not have the HTML_INTEGRATION_POINTS on the config. That shouldn't be the case... right?

Running example:

1. Open https://zlatkovsky.github.io/DOMPurifyMystery/
2. Select version 3.2.4 or 3.2.6 in the "Select a version" dropdown. Leave the "HTML integration points" checkbox as OFF for now.
3. Click "Sanitize!". Note that in the "After sanitization" section, the text in the blue box is missing. This is a known issue due to DOMPurify 3.1.7 breaks Mermaid diagrams using foreignObject #1002 .
   
4. Now turn ON the "Enable HTML integration points" checkbox and press "Sanitize!" again. This time the label will show up in the blue box, as expected.
5. Finally, turn the "Enable HTML integration points" OFF again and press "Sanitize!"

I would have expected the result of #5 to be the same as #3, i.e., the label should be missing again since the checkbox is OFF. But instead, the label is still visible because the HTML_INTEGRATION_POINTS isn't reset.

The source code for the above site can be found at https://github.com/Zlatkovsky/DOMPurifyMystery/blob/main/index.html.

Tracing through DOMPurify code, it appears that:

1. On invocation of DOMPurify.sanitize, _parseConfig is called:
   
2. Specifically, this means that the following line is invoked:

    HTML_INTEGRATION_POINTS = cfg.HTML_INTEGRATION_POINTS || HTML_INTEGRATION_POINTS;

3. If the first call to DOMPurify.sanitize had HTML_INTEGRATION_POINTS: {'foreignobject': true} in the config, the bundle-scoped HTML_INTEGRATION_POINTS variable will be set.
4. On follow-up invocations of DOMPurify.sanitize that do NOT have HTML_INTEGRATION_POINTS set, the line from #2 is hit again. Because cfg.HTML_INTEGRATION_POINTS is undefined, HTML_INTEGRATION_POINTS gets set to just itself. BUT, since it never gets reset across DOMPurify.sanitize invocations, it just stays as whatever the last config that had this setting set it to.