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

DOM lib: Add support for Trusted Types API #1246

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

Conversation

tosmolka
Copy link
Member

This PR adds support for Trusted Types APIs and Sinks in dom and worker libraries. These APIs are so far only supported by Blink. In microsoft/TypeScript#30024 we seem to be getting an agreement to include this change anyway.

@github-actions
Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@saschanaz
Copy link
Contributor

This needs multiple implementor interest.

@shicks
Copy link

shicks commented Jan 14, 2022

My understanding is that there are polyfill options for other browsers, making appropriate typings still pretty important.

@orta
Copy link
Contributor

orta commented Jan 14, 2022

You're welcome to use DefinitelyTyped if you'd like to ship types which only work on a single engine 👍🏻 - though it might be quite difficult given how much existing code this affects.

@shicks
Copy link

shicks commented Jan 14, 2022

Either way, this needs to take advantage of the new differently-typed getter/setter feature in order to be properly backwards-compatible. For example,

interface Element {
  get outerHTML(): string;
  set outerHTML(html: string|TrustedHTML);
}

@tosmolka
Copy link
Member Author

@shicks , can you please take another look? Do you think this would be backwards-compatible? I ended up introducing setterType as I could not find a way to do what we wanted with current implementation of emitProperty. Happy to choose different approach if needed.

@shicks
Copy link

shicks commented Mar 29, 2022

This looks much better. IIUC, the TypeScript team has a corpus of real world code that they can compile against - presumably they'd want to test this against that corpus to make sure it doesn't break anything significant.

The main breakage I anticipate at this point is if anybody is implementing the DOM API (i.e. like a NodeJS virtual DOM) - but at this point anybody who's just using the DOM should be fine.

@lgarron
Copy link

lgarron commented May 18, 2022

We're prototyping the use of TrustedHTML at GitHub, and would really love to see this land!

Comment on lines +14212 to +14215
interface TrustedHTML {
toJSON(): string;
toString(): string;
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you give me some more insight into TrustedHTML nodes? Is it really this simple of an interface, or is there something "more" to them?

The reason I ask is that in a structural language like TypeScript, you could accidentally assign lots of stuff to TrustedHTML. Is there some internal runtime bit set, or is this really anything with a toJSON and a toString?

Copy link

Choose a reason for hiding this comment

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

TrustedHTML is interesting, in that it's both an interface and also the name of a global object.

So it's sort of like a class (you can do value instanceof TrustedHTML), but the main point of the API is that it can't be constructed without the Trusted Types API. Sort of like you have to do document.createElement("div") instead of new HTMLDivElement().

Each TrustedHTML instance is also associated with a specific string policy. So conceptually it might make sense to have something like TrustedHTML<"dompurify">. I don't know if it makes sense to include that in TypeScript, though. The "dompurify" policy in that case is something that would be enforced by CSP for e.g. innerHTML assignment, which seems a bit outside the scope of type checking.

https://developer.mozilla.org/en-US/docs/Web/API/TrustedHTML#methods

Copy link
Member

@weswigham weswigham May 25, 2022

Choose a reason for hiding this comment

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

Maybe consider emitting a class with a private constructor instead of an interface with only these very-widespread-methods (and the static pseudo-constructor)? That'd be allowable in instanceof and a private member should enable nominal checking (and thus forbid structural matches).

Copy link

Choose a reason for hiding this comment

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

How would that interact with something like DefinitelyTyped/DefinitelyTyped#60417 ?

Element classes like HTMLDivElement are declared using the same interface+var pattern as this PR. Is there a particular reason to diverge from that entirely?

For what it's worth, new HTMLDivElement() also doesn't error in TypeScript today.

@lgarron
Copy link

lgarron commented Aug 16, 2022

Is there any chance of moving this forward in the near future?

DefinitelyTyped/DefinitelyTyped#61160 was making some progress last month, but my understanding is that it will still be hard to use if the Element.innerHTML setter still only allows string assignment in lib.dom.d.ts. This makes it rather tricky to use trusted types in practice, without working around lots of errors. 😔

@saschanaz
Copy link
Contributor

So far no implementer interest outside of Google, so a progress can happen when something happens there.

@lgarron
Copy link

lgarron commented Aug 16, 2022

So far no implementer interest outside of Google, so a progress can happen when something happens there.

🙋 We are actively working on exploring trusted types on the github.com site, and are specifically running into challenges with the concrete implementation due to this. 🤓

By implementer, I'm guessing you might mean just browsers, though?

Is there any way to make the innerHTML setter type at least compatible with TrustedHTML from @types/trusted-types? That would make a big difference.
(For example, one blunt possibility would be for .innerHTML to accept any string or any object with toString().)

@saschanaz
Copy link
Contributor

saschanaz commented Aug 16, 2022

By implementer, I'm guessing you might mean just browsers, though?

Yes.

Is there any way to make the innerHTML setter type at least compatible with TrustedHTML from @types/trusted-types? That would make a big difference.

That's really a thing that needs a Microsoft decision, but I think TS should support overloading the setter declaration to make it compatible.

facebook-github-bot pushed a commit to facebook/flow that referenced this pull request May 26, 2023
Summary:
As a follow up to D46007012, this diff modifies the type declarations of XSS DOM sinks to accept Trusted Type objects as well.

Annoyingly, the spec does not fully enumerate the list of sensitive surfaces. I took some hints from microsoft/TypeScript-DOM-lib-generator#1246 and manual inspection, but left out some of the more ambiguous hints.

Changelog: [new] Updated dom libdefs to allow Trusted Type objects

Reviewed By: SamChou19815

Differential Revision: D46085621

fbshipit-source-id: d10bf667849560319ad69edc639090a9ddd35f9f
@koto
Copy link

koto commented May 16, 2024

So far no implementer interest outside of Google, so a progress can happen when something happens there.

There is now multi implementor interest: mozilla/standards-positions#20 (comment), with a Gecko implementation progressing. Implementation in WebKit is also ongoing. What are the next best steps?

@saschanaz
Copy link
Contributor

When the feature ships, MDN data will be updated and that will be automatically reflected here.

@koto
Copy link

koto commented May 16, 2024

When the feature ships, MDN data will be updated and that will be automatically reflected here.

Do you mean browser compat data on MDN? I think browser compat data on MDN is manually updated, at least that was the case when the Trusted Types shipped in Chrome. Once that happens (which, I guess, is after shipping in Gecko or WebKit), what is actually happening automatically in this repo? Trusted Types APIs are already largely integrated in HTML, DOM and other specs (e.g. https://dom.spec.whatwg.org/#parentnode).

Sorry for those basic questions, I'm just trying to figure out what needs manual work and when is it best to do that work. It seems like the 'when' part is after 2nd implementation part ships, but I don't know yet the 'what' part.

@HolgerJeromin
Copy link
Contributor

Do you mean browser compat data on MDN? I think browser compat data on MDN is manually updated, at least that was the case when the Trusted Types shipped in Chrome.

Yes. After that a friendly bot will come along and create here a PR like this: #1726

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants