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

toString is reserved identifier. #30

Closed
otherdaniel opened this issue Aug 20, 2020 · 9 comments
Closed

toString is reserved identifier. #30

otherdaniel opened this issue Aug 20, 2020 · 9 comments
Labels
sanitizer-api issues with the API

Comments

@otherdaniel
Copy link
Collaborator

toString is a reserved identifier in WebIDL. (See sentence starting with "The identifier of any".) We need to pick a different name.

@otherdaniel otherdaniel added the sanitizer-api issues with the API label Aug 20, 2020
@otherdaniel otherdaniel changed the title toString is reserved identigier. toString is reserved identifier. Aug 20, 2020
@otherdaniel
Copy link
Collaborator Author

Candidates:

  • saneFromString / saneFromFragment
  • sanitze / sanitizeToFragment
  • stringFrom / fragmentFrom (Since this is called on a sanitizer instance, the reader already has the context that this is about sanitzing)

@shhnjk
Copy link

shhnjk commented Aug 21, 2020

I like sanitze 😊 I always feel bad about assigning string to innerHTML after working on Trusted Types, so something that is different from word string would be nice 😊

@mozfreddyb
Copy link
Collaborator

We had agreed on sanitize (to DocFragment) and sanitizeToString (to String) in #22.

The reason for a default-to-DocFragment is that the Sanitizer will create a DOM Tree and traverse it internally. Returning a copy of that DOM tree is more performant than serializing it to a String and then using this tree to create another DOM tree when the developer ends up using innerHTML.

i.e.,
Good: foo.append(Sanitizer.sanitize(evil)) saves you a parsing/serialization roundtip
OK (but inherently slower): foo.innerHTML = Sanitizer.sanitizeToString(evil) creates two DOM trees (once for sanitizing and then another one for innerHTML).

@shhnjk
Copy link

shhnjk commented Aug 21, 2020

Hmm, I really don’t like the word string. We are assigning things to innerHTML, and we should really call out that we are assigning HTML, not string.

This is my personal preference though.

@shhnjk
Copy link

shhnjk commented Aug 27, 2020

Can people vote if you all prefer sanitizeToString or sanitizeToHTML?
If you like sanitizeToHTML please add a 👍 reaction in this comment. If you like sanitizeToString, then add a 👎 reaction in this comment.
If you hate both, add a comment 😊

CC: @koto, @mikewest, @cure53

@koto
Copy link

koto commented Aug 27, 2020

This depends on #20 (comment). If TT would allow creating a DocumentFragment for a given sanitizer config, there ideally should be a way to return a TrustedHTML instance from said config too.

I don't think we should even be offering strings as a return value for HTML in new APIs. We know what that led to.

@shhnjk
Copy link

shhnjk commented Aug 27, 2020

This depends on #20 (comment). If TT would allow creating a DocumentFragment for a given sanitizer config, there ideally should be a way to return a TrustedHTML instance from said config too.

Agreed 😊

I don't think we should even be offering strings as a return value for HTML in new APIs. We know what that led to.

I'm against this idea for Sanitizer API (though this might make sense for other APIs)🙁 The reason being, because we need to be flexible about whatever people decides to add in their SanitizerConfig, we do need to return string when we are unsure if the output would cause an XSS. This will ensure that the output is string, and therefore it's still unsafe to assign it as an HTML.

However I have to agree that this would make a confusion that sanitizeToHTML might return a string, which is weird 😂
Maybe indeed we have to create 2 different methods, one with arbitrary config called sanitizeToString, and one with only default config or stricter called sanitizeToHTML. And calling sanitizeToHTML with weak SanitizerConfig would just return emptyHTML.

@koto
Copy link

koto commented Aug 27, 2020

Agreed. I guess a right way to put it, there should ideally be 3 distinct methods - for fragment, a string, and TrustedHTML. These methods might throw - for example, if a conversion to fragments or TrustedHTML from a given sanitizer instance was violating TT restrictions on a page, but I imagine there might be other reasons for throwing too.

@otherdaniel
Copy link
Collaborator Author

Marking as closed, since there's no more toString left in the current spec draft. Jun's/Koto's requests for TT methods is best handled with #31

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sanitizer-api issues with the API
Projects
None yet
Development

No branches or pull requests

4 participants