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

Allow to render TrustedHTML objects similar to SafeString #1567

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Allow to render TrustedHTML objects similar to SafeString #1567

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
76e1d94
Allow to render TrustedHTML objects similar to SafeString
crypto Feb 21, 2024
b61fb07
Add pnpm-lock.yaml
crypto Feb 21, 2024
62d56b3
Use prettier for content.ts
crypto Feb 21, 2024
7e3b94c
Small fix in test
crypto Feb 21, 2024
eee2ef7
Update comment in test
crypto Feb 21, 2024
23a8b0d
Add type TrustedHTML to methods used to append HTML
crypto Feb 21, 2024
1abf285
Formatting
crypto Feb 21, 2024
ecc3230
Formatting
crypto Feb 21, 2024
d3f3793
Don't modify insertHTMLBefore before simple-dom
crypto Feb 21, 2024
1a21f68
Don't change types of interfaces
crypto Feb 22, 2024
af6ea25
Stringify TrustedHTML
crypto Feb 22, 2024
2c67820
Stringify TrustedHTML
crypto Feb 22, 2024
0d54eec
revert bin/opcodes.json
crypto Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@glimmer-workspace/integration-tests/package.json
View file
Open in desktop
Expand Up @@ -29,6 +29,7 @@
"@simple-dom/document": "^1.4.0",
"@simple-dom/serializer": "^1.4.0",
"@simple-dom/void-map": "^1.4.0",
"@types/trusted-types": "^2.0.7",
"js-reporters": "^2.1.0",
"qunit": "^2.19.4",
"simple-html-tokenizer": "^0.5.11"
Expand Down
49 changes: 49 additions & 0 deletions packages/@glimmer-workspace/integration-tests/test/trusted-html-test.ts
View file
Open in desktop
@@ -0,0 +1,49 @@
import type { TrustedTypePolicy, TrustedTypesWindow } from 'trusted-types/lib';

import { jitSuite, RenderTest, test } from '..';

let policy: TrustedTypePolicy | undefined;
if (typeof window !== 'undefined') {
let trustedTypes = (window as unknown as TrustedTypesWindow).trustedTypes;
if (trustedTypes?.createPolicy) {
policy = trustedTypes.createPolicy('test', {
createHTML: (s: string) => s,
createScript: (s: string) => s,
createScriptURL: (s: string) => s,
});
}
}

export class TrustedHTMLTests extends RenderTest {
static suiteName = 'TrustedHTML';

@test
'renders TrustedHTML similar to SafeString'() {
if (!policy) return;

let html = '<b>test\'"&quot;</b>';
this.registerHelper('trustedHTML', () => {
return policy?.createHTML(html);
});

this.render('<div>{{trustedHTML}}</div>');
this.assertHTML('<div><b>test\'""</b></div>');
this.assertStableRerender();
}

@test
'renders TrustedHTML in attribute context as string'() {
crypto marked this conversation as resolved.
Show resolved Hide resolved
if (!policy) return;

let html = '<b>test\'"&quot;</b>';
this.registerHelper('trustedHTML', () => {
return policy?.createHTML(html);
});

this.render('<a title="{{trustedHTML}}">{{trustedHTML}}</a>');
this.assertHTML('<a title="<b>test\'&quot;&amp;quot;</b>"><b>test\'""</b></a>');
this.assertStableRerender();
}
}

jitSuite(TrustedHTMLTests);
5 changes: 5 additions & 0 deletions packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/stdlib.ts
View file
Open in desktop
Expand Up @@ -74,6 +74,11 @@ export function StdAppend(
op(Op.AppendSafeHTML);
});

when(ContentType.TrustedHTML, () => {
op(Op.AssertSame);
op(Op.AppendHTML);
crypto marked this conversation as resolved.
Show resolved Hide resolved
});

when(ContentType.Fragment, () => {
op(Op.AssertSame);
op(Op.AppendDocumentFragment);
Expand Down
17 changes: 15 additions & 2 deletions packages/@glimmer/runtime/lib/compiled/opcodes/content.ts
View file
Open in desktop
Expand Up @@ -11,7 +11,14 @@ import { isObject } from '@glimmer/util';
import { ContentType, CurriedType, Op } from '@glimmer/vm';

import { isCurriedType } from '../../curried-value';
import { isEmpty, isFragment, isNode, isSafeString, shouldCoerce } from '../../dom/normalize';
import {
isEmpty,
isFragment,
isNode,
isSafeString,
isTrustedHTML,
shouldCoerce,
} from '../../dom/normalize';
import { APPEND_OPCODES } from '../../opcodes';
import DynamicTextContent from '../../vm/content/text';
import { CheckReference } from './-debug-strip';
Expand All @@ -32,6 +39,8 @@ function toContentType(value: unknown) {
return ContentType.Helper;
} else if (isSafeString(value)) {
return ContentType.SafeString;
} else if (isTrustedHTML(value)) {
return ContentType.TrustedHTML;
} else if (isFragment(value)) {
return ContentType.Fragment;
} else if (isNode(value)) {
Expand Down Expand Up @@ -87,7 +96,11 @@ APPEND_OPCODES.add(Op.AppendHTML, (vm) => {
let reference = check(vm.stack.pop(), CheckReference);

let rawValue = valueForRef(reference);
let value = isEmpty(rawValue) ? '' : String(rawValue);
let value = isEmpty(rawValue)
? ''
: isTrustedHTML(rawValue)
? (rawValue as string)
Copy link
Contributor

Choose a reason for hiding this comment

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

This cast seems suspect

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think, if we made it here, we should have already checked and unwrapped the value?

Copy link
Author

@crypto crypto Feb 21, 2024 •
edited

Choose a reason for hiding this comment

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

We can modify whole chain of functions to accept string | TrustedHTML type, but I thought we should keep it when decide to fully migrate Glimmer to Trusted Types.

Copy link
Author

Choose a reason for hiding this comment

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

I've added TrustedHTML for appendHTML methods, let's see if we have any issues

Copy link
Author

@crypto crypto Feb 21, 2024 •
edited

Choose a reason for hiding this comment

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

I think it goes down to
@simple-dom/interface that doesn't support Trusted Types

glimmer-vm/packages/@glimmer/runtime/lib/dom/operations.ts

Line 69 in 1cb493b

insertHTMLBefore(parent: SimpleElement, nextSibling: Nullable<SimpleNode>, html: string): Bounds {

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we definitely still have to plumb this through to simple-dom. I am actually not sure why TypeScript is not catching the TypeError, but essentially the problem is you changed the interface without changing the implementations, so for example, in places like these, it's all going to be problematic:

https://github.com/glimmerjs/glimmer-vm/blob/main/packages/%40glimmer/runtime/lib/dom/operations.ts#L69-L74

glimmer-vm/packages/@glimmer/node/lib/node-dom-helper.ts

Lines 23 to 25 in 1cb493b

html: string
): Bounds {
let raw = this.document.createRawHTMLSection!(html);

The code is a bit odd, because the primary implementation of insertHTMLBefore is on a class that doesn't claim to implement the interface you modified, so no type error there. But this extends and implement it, so it should be caught there, but it wasn't: https://github.com/glimmerjs/glimmer-vm/blob/main/packages/%40glimmer/runtime/lib/dom/api.ts#L18

Anyway missing type errors aside, there are some real, a bit tedious but solvable problems, there are also a few more things to think through around attributes (and then the grunt work to actually plumb it through, get simple-dom released, etc), so it may be a while before we can land this.

Copy link
Author

Choose a reason for hiding this comment

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

I've changed it back to not change interface of insertHTMLBefore.

Copy link
Author

Choose a reason for hiding this comment

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

If you think it will take a while, I suggest to taking small steps and in this PR just stringify TrusteHTML, but render it as HTML.

Copy link
Author

@crypto crypto Feb 23, 2024 •
edited

Choose a reason for hiding this comment

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

more things to think through around attributes

Let me explain why I'm against error on rendering TrustedHTML in attribute context.

I've tested LinkedIn where we use SafeString in attribute context, almost everywhere. It's related to our i18n system. Every i18n string is like small HTML template where all params HTML encoded, not every i18n string contains HTML tags, but potentially every string can contain HTML entities, like &nbsp;. For this reason i18n helper uses htmlSafe for every i18n string. And obviously same SafeStrings used in attribute context. If we are going to replace SafeString with TrustedHTML it will not change anything. Yes, we have some rare double encoding issues, but throwing errors would completely prevent migration to TrustedHTML.

Another point is DOM API, some attributes require TrustedHTML to work, for example iframe.srcdoc

trustedTypes.getAttributeType('iframe', 'srcdoc') // TrustedHTML

https://w3c.github.io/trusted-types/dist/spec/#validate-attribute-mutation
For this reason we should keep TrusteHTML and other trusted types unmodified for setAttribute, decoding can cause an injection, stringifying can cause default policy triggering.
https://developer.mozilla.org/en-US/docs/Web/API/TrustedTypePolicyFactory/getAttributeType

I would recommend to keep same behavior as SafeString, and in best case don't stringify TrustedHTML object.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for walking through those use cases. That's helpful, and I'm fine with not throwing

: String(rawValue);

vm.elements().appendDynamicHTML(value);
});
Expand Down
13 changes: 13 additions & 0 deletions packages/@glimmer/runtime/lib/dom/normalize.ts
View file
Open in desktop
@@ -1,4 +1,5 @@
import type { Dict, SimpleDocumentFragment, SimpleNode } from '@glimmer/interfaces';
import type { TrustedTypesWindow } from 'trusted-types/lib';

export interface SafeString {
toHTML(): string;
Expand Down Expand Up @@ -43,6 +44,18 @@ export function isEmpty(value: unknown): boolean {
return value === null || value === undefined || typeof (value as Dict).toString !== 'function';
}

let isHTML: ((value: unknown) => boolean) | undefined;
if (typeof window !== 'undefined') {
let trustedTypes = (window as unknown as TrustedTypesWindow).trustedTypes;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a good way to progressively allow the feature for browsers that have it and not break in browsers that don't

if (trustedTypes?.isHTML) {
isHTML = trustedTypes?.isHTML.bind(trustedTypes);
}
}

export function isTrustedHTML(value: unknown): boolean {
crypto marked this conversation as resolved.
Show resolved Hide resolved
return isHTML ? isHTML(value) : false;
}

export function isSafeString(value: unknown): value is SafeString {
return typeof value === 'object' && value !== null && typeof (value as any).toHTML === 'function';
}
Expand Down
3 changes: 2 additions & 1 deletion packages/@glimmer/runtime/package.json
View file
Open in desktop
Expand Up @@ -43,7 +43,8 @@
"@glimmer/util": "workspace:^",
"@glimmer/validator": "workspace:^",
"@glimmer/vm": "workspace:^",
"@glimmer/wire-format": "workspace:^"
"@glimmer/wire-format": "workspace:^",
"@types/trusted-types": "^2.0.7"
},
"devDependencies": {
"@glimmer-workspace/build-support": "workspace:^",
Expand Down
1 change: 1 addition & 0 deletions packages/@glimmer/vm/lib/content.ts
View file
Open in desktop
Expand Up @@ -7,4 +7,5 @@ export const ContentType = {
Fragment: 5,
Node: 6,
Other: 8,
TrustedHTML: 9,
} as const;