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

Support configurable TrustedTypes policy #798

Closed
dejang opened this issue Apr 27, 2023 · 10 comments
Closed

Support configurable TrustedTypes policy #798

dejang opened this issue Apr 27, 2023 · 10 comments

Comments

@dejang
Copy link
Contributor

dejang commented Apr 27, 2023

Background & Context

We use DOMPurify extensively in a multi tenant web application that serves millions of users. We have also began moving towards TrustedTypes but we find ourselves limited by how the TrustedTypes policy is used within DOMPurify. For starters, we never include DOMPurify via a <script> tag, we always rollup the dependencies in a bundle that is shipped to the browser and is loaded as a <script> tag. Our code is ESM. Additionally, because performance is an influencing factor in our architecture, we use multiple preconfigured instances of DOMPurify in order to skip configuration logic with repeated invocations. The multiple DOMPurify instances have different usages and we'd like to maintain that flexibility in our system. We find that with our current CSP configurations which does not allow duplicates we cannot use more than 1 instance of DOMPurify for the entire application. Given the size of the application and the scenario we are in this affects our ability to maintain our flexibility and, given that with each invocation we need to reset the configuration and the hooks, we end up spending compute cycles that we feel could be avoided.

We would like to request the ability for DOMPurify to support passing in our own TT policy via configuration when instantiated or when the configuration is being set.

Feature

Given a configuration object passed to sanitize or to setConfig allow an entry to specify the TT policy that DOMPurify should use.

I've worked on the DOMPurify source code before and would be happy to contribute if this feature is being accepted.

The new configuration option could be named trustedTypesPolicy and it must define a createHTML hook. The code must validate the object's shape to prevent passing in policies which do not define the minimum requirement.

@cure53
Copy link
Owner

cure53 commented Apr 28, 2023

Thank you, that sounds like a very interesting and relevant feature and we'd be happy to review a PR implementing it.

@dejang
Copy link
Contributor Author

dejang commented Apr 28, 2023

Great, I'll get a PR ready for you to review.

@cure53
Copy link
Owner

cure53 commented May 2, 2023

The PR looks great, thanks, tests are all green as far as I can see. Ready for a new 3.x release? 😊

@tosmolka
Copy link
Contributor

tosmolka commented May 2, 2023

@dejang , I know this is already merged but if I do read the PR correctly then DOMPurify will now always attempt to create default TT policy (dompurify + suffix) even if the new TRUSTED_TYPES_POLICY option is set.

Would you mind minor refactoring to reverse the order and create default policy only if option is not set (or maybe if its invalid)?

Again, if I read it right, current behavior will cause unnecessary CSP violations when DOMPurify fails to create duplicate default policy on every bundle load. Thanks.

@cure53
Copy link
Owner

cure53 commented May 2, 2023

Hmmm, good point @tosmolka! We will wait with a release until clarified & resolved.

@dejang
Copy link
Contributor Author

dejang commented May 2, 2023

Yes, I maintained the existing behavior but I can move the instantiation of the policy.

@dejang
Copy link
Contributor Author

dejang commented May 3, 2023

@tosmolka, hopefully I understood correctly your ask, I'm linking the PR here #801. Let me know if we need to make adjustments. But if everything looks good I am ready to upgrade on my side.

@tosmolka
Copy link
Contributor

tosmolka commented May 3, 2023

Thank you @dejang for quick update, PR looks good to me, I left only minor comments.

@cure53
Copy link
Owner

cure53 commented May 10, 2023

Heya, did the latest release address the issue as expected? 😄

@cure53 cure53 closed this as completed May 15, 2023
@dejang
Copy link
Contributor Author

dejang commented May 15, 2023

Yes, it worked as expected! Thank you for the support and please accept my apologies for getting back to you so late, we only had time to integrate the new version over the weekend.

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

No branches or pull requests

3 participants