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

Consider an append/insert variant #132

Closed
tjcrowder opened this issue Oct 7, 2021 · 11 comments
Closed

Consider an append/insert variant #132

tjcrowder opened this issue Oct 7, 2021 · 11 comments
Labels

Comments

@tjcrowder
Copy link

tjcrowder commented Oct 7, 2021

Very excited to see this Sanitizer API!

If .setHTML(x) is, in some sense, the new .innerHTML = x, it probably makes sense to handle the common use case that is currently often (unfortunately) handled with .innerHTML += x (but without the antipattern round-trip through markup).

Perhaps insertHTML, accepting the same position argument that insertAdjacentHTML takes:

partial interface Element {
  undefined insertHTML(DOMString position, DOMString input, Sanitizer? sanitizer);
};

It may need to throw if called with position set to "beforebegin" or "afterend" if the element it's called on doesn't have a parent (yet), since as I understand it the sanitizer needs to know what element it's sanitizing for.

Example:

document.getElementById("x").insertHTML("beforeend", userInput);

It's tempting "just" to add a third optional argument to insertAdjacentHTML, but setHTML sanitizes whether it has that sanitizer argument or not, whereas insertAdjacentHTML doesn't, so probably best if it's separate.

(Apologies if I've missed previous discussion on this, as seems likely. Didn't find anything relevant in the issues or Q&A.)

@mozfreddyb mozfreddyb added the sanitizer-api issues with the API label Oct 7, 2021
@mozfreddyb
Copy link
Collaborator

Hm, not sure yet. Though I do want to point out that some of the this could be easily done without adding yet another function:

  • foo + beforeend: foo.setHTML(foo.innerHTML + input),
  • foo + afterbegin: foo.setHTML(input + foo.innerHTML),

Let's hear what the others think..

@koto
Copy link

koto commented Oct 7, 2021

In practice, it's probably only innerHTML += cases that would be interesting to cover, which @mozfreddyb expression solves. There's a slightly bigger hidden problem in that setHTML only covers one sink, and there's many (like insertAdjacentHTML), but I don't think Sanitizer API itself is a good fit for 'let's cover all risky sinks' issue.

@tjcrowder
Copy link
Author

@mozfreddyb You could do that, but it has issues:

  • It drops event handlers, state that doesn't serialize (ex: form field state, active element, user selection), and non-DOM data (jQuery-like expandos, etc.) from existing elements
  • It's an unnecessary round-trip through markup for the elements that are already there
  • It drops existing content that would be unsafe if provided by a user (but was safe when it was added originally, because it didn't come from a user)

You can also work around it by using serializeFor and the relevant insertBefore or similar, which is much more complicated and thus (IMHO) less likely to be adopted.

@koto
Copy link

koto commented Oct 7, 2021

innerHTML += has these side effects too, no? I think it mostly exemplifies why dynamically appending to a node is not a best practice, and sanitizer just doesn't make this easier than it could.

@tjcrowder
Copy link
Author

tjcrowder commented Oct 7, 2021

@koto - innerHTML += is generally a bad idea, which is why we have insertAdjacentHTML. (But no, innerHTML += wouldn't have the third issue listed above -- but it would have the issue of not sanitizing! :-D ) All I'm suggesting is a sanitizing version of insertAdjacentHTML, not least in hopes that people will learn to use it (if it's nice and simple) where they would have used innerHTML +=.

@mozfreddyb
Copy link
Collaborator

So, I think there is a need for better HTML APIs regardless of the Sanitizer API.
This is true for the HTML modification use cases you outline as well as HTML-creation use cases (i.e., a cooler way to do createElement, setAttribute, addEventListener, append). I don't think this API is the right place to solve these problems and I also don't think that the Sanitizer ought to solve it today, but I 100% agree they are worth solving.

@mozfreddyb
Copy link
Collaborator

In the meantime, I think developers might call sanitizeFor and then use Element.before or Element.prepend

@tjcrowder
Copy link
Author

@mozfreddyb - I don't understand your point about better HTML APIs and solving this elsewhere. The DOM already has this (insertAdjacentHTML), it's not a new concept. Other than the awkwardness of the name, it works well enough -- except that it doesn't sanitize. All I'm suggesting is a sanitizing equivalent of what's already there. (I certainly agree there's other work to do in the DOM [and respect the difficulty of doing it].)

Re sanitizeFor + before/prepend, sure, I mentioned that above. Playing Devil's advocate: They could also use sanitizeFor in combination with replaceChildren, so why do we need setHTML? Because easy = better adoption. The user should always be part of the design of a system.

Let's hear what others think.

@nightpool
Copy link

Personally, as a user, using sanitizeFor with the existing DOM APIs makes more sense to me—it's more composable and utilizes the patterns and APIs I already understand, instead of expecting me to learn a whole new set of primitives. The downside is that it might be hard to explicitly manage the sanitizeFor tag context, but I don't think there's a way to introduce a security bug by doing so? e.g. using a DOM node sanitized for a div in a table instead doesn't have any negative security consequences, because the parsing has already happened. If my assumption in the previous sentence is correct, then I don't see any downside to using sanitizeFor. (And, given that, maybe it's better to give it a different name, like sanitizeWithin?)

@annevk
Copy link
Collaborator

annevk commented Oct 18, 2023

Adding beforeHTML() and such make sense to me, not sure we need to track it here.

@annevk
Copy link
Collaborator

annevk commented Oct 18, 2023

Oh, let's fold this into #184.

@annevk annevk closed this as not planned Won't fix, can't repro, duplicate, stale Oct 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants