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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
May 6, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 19 additions & 9 deletions dist/purify.cjs.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/purify.cjs.js.map
View file
Open in desktop

Large diffs are not rendered by default.

28 changes: 19 additions & 9 deletions dist/purify.es.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/purify.es.js.map
View file
Open in desktop

Large diffs are not rendered by default.

28 changes: 19 additions & 9 deletions dist/purify.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/purify.js.map
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/purify.min.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/purify.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

37 changes: 23 additions & 14 deletions src/purify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,11 @@ const getGlobal = () => (typeof window === 'undefined' ? null : window);
* Creates a no-op policy for internal use only.
* Don't export this function outside this module!
* @param {?TrustedTypePolicyFactory} trustedTypes The policy factory.
* @param {Document} document The document object (to determine policy name suffix)
* @param {HTMLScriptElement} purifyHostElement The Script element used to load DOMPurify (to determine policy name suffix).
* @return {?TrustedTypePolicy} The policy created (or null, if Trusted Types
* are not supported).
* are not supported or creating the policy failed).
*/
const _createTrustedTypesPolicy = function (trustedTypes, document) {
const _createTrustedTypesPolicy = function (trustedTypes, purifyHostElement) {
if (
typeof trustedTypes !== 'object' ||
typeof trustedTypes.createPolicy !== 'function'
Expand All @@ -43,11 +43,8 @@ const _createTrustedTypesPolicy = function (trustedTypes, document) {
// Policy creation with duplicate names throws in Trusted Types.
let suffix = null;
const ATTR_NAME = 'data-tt-policy-suffix';
if (
document.currentScript &&
document.currentScript.hasAttribute(ATTR_NAME)
) {
suffix = document.currentScript.getAttribute(ATTR_NAME);
if (purifyHostElement && purifyHostElement.hasAttribute(ATTR_NAME)) {
suffix = purifyHostElement.getAttribute(ATTR_NAME);
}

const policyName = 'dompurify' + (suffix ? '#' + suffix : '');
Expand Down Expand Up @@ -96,6 +93,7 @@ function createDOMPurify(window = getGlobal()) {
}

const originalDocument = window.document;
const currentScript = originalDocument.currentScript;

let { document } = window;
const {
Expand Down Expand Up @@ -130,11 +128,8 @@ function createDOMPurify(window = getGlobal()) {
}
}

let trustedTypesPolicy = _createTrustedTypesPolicy(
trustedTypes,
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 = '';
Copy link
Contributor

@tosmolka tosmolka May 3, 2023 •
edited
Loading

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 •
edited
Loading

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.


const {
implementation,
Expand Down Expand Up @@ -603,8 +598,22 @@ function createDOMPurify(window = getGlobal()) {

// Overwrite existing TrustedTypes policy.
trustedTypesPolicy = cfg.TRUSTED_TYPES_POLICY;
// Sign local variables in case using the internal policy did not succeed.

// Sign local variables required by `sanitize`.
emptyHTML = trustedTypesPolicy.createHTML('');
} else {
// Uninitialized policy, attempt to initialize the internal dompurify policy.
if (trustedTypesPolicy === undefined) {
trustedTypesPolicy = _createTrustedTypesPolicy(
trustedTypes,
currentScript
);
}

// If creating the internal policy succeeded sign internal variables.
if (trustedTypesPolicy !== null && typeof emptyHTML === 'string') {
emptyHTML = trustedTypesPolicy.createHTML('');
}
}

// Prevent further manipulation of configuration.
Expand Down
Loading