Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create internal trustedTypes policy only if not specified via config object #801

Merged
merged 1 commit into from
May 6, 2023

Conversation

dejang
Copy link
Contributor

@dejang dejang commented May 3, 2023

Summary

Before #800:

  • DOMPurify would attempt to create an internal Trusted Types policy when loaded on the page.
  • No possibility to configure the policy used by DOMPurify.

After #800:

  • DOMPurify would attempt to create an internal Trusted Types policy when loaded on the page.
  • sanitize and setConfig allow switching from the internal policy to a user provided one when TRUSTED_TYPES_POLICY configuration option is passed via the configuration object.

This PR:

Background & Context

See #798 for the background discussion.

Note: since the original loadDOMPurify test helper was performing a call to sanitize without any configuration option this triggered the creation of the internal policy and I could not write a test to validate that there is no policy created when supplying TRUSTED_TYPES_POLICY configuration option. I had to refactor the helper to allow writing a new test which specifically looks for the number of policies created when DOMPurify is loaded on the page and after calls to sanitize.

Tasks

  • internal Trusted Types policy creation is skipped if configuration object supplies a Trusted Types policy

originalDocument
);
let emptyHTML = trustedTypesPolicy ? trustedTypesPolicy.createHTML('') : '';
let trustedTypesPolicy;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would move let trustedTypesPolicy declaration just before if (cfg.TRUSTED_TYPES_POLICY) { line

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that wouldn't be possible in the present form of the code, the if condition is part of the _parseConfig function and let trustedTypesPolicy is in the upper scope. Moving it before the if would make it a local variable to _parseConfig and inaccessible to the rest of the methods that look for it on the upper scope. This would be a move involved effort.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep current change then if it's intentional.

);
let emptyHTML = trustedTypesPolicy ? trustedTypesPolicy.createHTML('') : '';
let trustedTypesPolicy;
let emptyHTML = '';
Copy link
Contributor

@tosmolka tosmolka May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I would revert let emptyHTML = ''; to original declaration before PR 800

const emptyHTML = trustedTypesPolicy ? trustedTypesPolicy.createHTML('') : '';

and move it just after either default or custom trustedTypesPolicy is initialized.

Copy link
Contributor Author

@dejang dejang May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we would be missing some functionality if we make it a const and initialized one time. Particularly this kind of usage:

try {
  DOMPurify.sanitize(dirty1);
} catch(e) {
  DOMPurify.sanitize(dirty1, {TRUSTED_TYPES_POLICY: policy});
}

This would be something that developers can use to detect if their host, which they may not control the configuration of, allows the usage of DOMPurify with TT by default or if they need to declare a policy or even create a new instance of DOMPurify for themselves.

There is, however, an alternative to initializing this. Looking at the existing trustedTypes API in Chrome, there seems to be a trustedTypes.emptyHTML signed value built into the API so this declaration would not be needed anymore. We could simply declare it as

const emptyHTML = trustedTypes? trustedTypes.emptyHTML : ''

I did not want to make that change because I do not have enough information to know if the API is considered stable at the moment and if this will be supported in the future. Should be an easy improvement for a follow up PR in case it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep current change then if it's intentional.

@cure53
Copy link
Owner

cure53 commented May 5, 2023

Cool, are we ready for a merge? 😊

@dejang
Copy link
Contributor Author

dejang commented May 5, 2023

@cure53 let's do it! I'm ready to upgrade whenever the new release is published.

@cure53 cure53 merged commit ad1bdd4 into cure53:main May 6, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants