🦋 Changeset detected

Latest commit: 3207e78

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Preview: https://svelte-dev-git-preview-svelte-16271-svelte.vercel.app/

pnpm add https://pkg.pr.new/svelte@16271

Btw, I think configuring CSP should be added to the docs. Or adding trusted-types svelte-trusted-html integrated to SvelteKit.

Thank you, but I don't think this is the right fix — it's saying that all HTML is trusted regardless of its provenance, which is the opposite of the intent of TrustedTypes.

What if {@html ...} accepted either a string or an instance of TrustedHTML? That way people can define their own policies with their own sanitization logic. It becomes slightly more cumbersome to use component libraries that inject HTML — those libraries would need to either also accept TrustedHTML or a function that created TrustedHTML given some input — but that's a feature rather than a bug.

(admittedly not sure how to support SVG and MathML in this case; might not be possible. Perhaps that's just the cost of CSP support though, to be borne by apps that need CSP support)

I agree that this shouldn't affect {@html ...}, but it's also needed for all of our internal template generation which is known to be safe. So if we can adjust it so that {@html ...} does not use the svelte-html trusted types then this should be a good addition.

@html and bind:innerHTML can use a separate policy name. Or implement #15528, so users can sanitize and trustify html in it.

Another CSP thing is that require-trusted-types-for 'script' breaks dynamic creation of scripts:

{#if condition}
  <div>
    <script>console.log("inline")</script>
    <script src="/static/my-script.js"></script>
  </div>
{/if}

Thank you, but I don't think this is the right fix — it's saying that all HTML is trusted regardless of its provenance, which is the opposite of the intent of TrustedTypes.

I think its okay to trust the output of Svelte's internal template generation - its what the other Frameworks do (i.e. Polymer/Lit). I'll try and work out how to carve out {@html ...} 😄

@html and bind:innerHTML can use a separate policy name. Or implement #15528, so users can sanitize and trustify html in it.

@7nik - I think I'll try and get it working with a separate policy for now Actually, thinking about this, its untrusted so it should go through the default policy - the ideal would be if users could do:

{@html customPolicy.createHTML('<span>Hello</span>')}

but I'm not going to dig into that here.

Another CSP thing is that require-trusted-types-for 'script' breaks dynamic creation of scripts:

I think you should be able to allow dynamic script creation via some of the other policies (i.e. unsafe-inline or whitelisting the script URL). Either way, I'd prefer it to be tackled separately.

Okay, @html doesn't go through policy now. I tested and bind:innerHTML already wasn't going through the policy so that should be fine.

FWIW, I think this a pretty useful improvement as is because it means that apps which don't use bind:innerHTML and @html will be able to use Svelte with require-trusted-types-for 'script' now.

I think the ideal solution for @html and bind:innerHTML would be to allow authors to pass in TrustedHTML rather than creating a custom policy for them (which would throw an error if we try to register it and it isn't supported).

What about using trustedTypes?.isHTML(html) to check if the passed value is TrustedHTML?

Offtop: it's funny that window.trustedTypes is protected from writing, but trustedTypes.isHTML = () => true and TrustedTypePolicyFactory.prototype.isHTML = () => true; are still allowed 😄

What about using trustedTypes?.isHTML(html) to check if the passed value is TrustedHTML?

You mean to be able to set {@html ...} to TrustedHTML? Yeah, that's probably a good approach - tbh, I didn't really dig into why it wasn't working, and maybe we won't even need to check 😆

Just wondering if the Svelte team would consider merging without it? This PR fixes Svelte for our use case (so we don't need to create a default policy).

Offtop: it's funny that window.trustedTypes is protected from writing, but trustedTypes.isHTML = () => true and TrustedTypePolicyFactory.prototype.isHTML = () => true; are still allowed

Huh, that is kind of weird - I guess it'll still fail when you try and assign a sink to the non-trusted HTML though.

Look at https://github.com/sveltejs/svelte/blob/5ebbedf2051715f1df10a884976d7d23b1d7f5fc/packages/svelte/src/internal/client/dom/blocks/html.js#L93-L95 In the case of SVG, to create elements with the correct namespace, code is wrapped in <svg> so it loses its trustiness and the trustiness should be checked before it somehow. But SVG has <foreignObject> that allows to include arbitrary HTML.

Okay, I think something like

var html = trustedTypes.isTrusted(value) ? value : (value + '') should work for the non-SVG / non-MATH elements - before going and changing that, just want to confirm with @dummdidumm that this is something Svelte would consider merging.

I think this should allow creating fragments with the desired namespace without losing trustiness:

export function create_fragment_from_html(html, untrusted = false, namespace = null) {
	var template = document.createElement('template');
        if (namespace) {
                var container = namespace == 'svg'
                	? document.createElementNS(NAMESPACE_SVG, 'svg')
                	: document.createElementNS(NAMESPACE_MATHML, 'mathml');
                container.innerHTML = untrusted ? html : create_trusted_html(html.replaceAll('<!>', '<!---->'));
                template.append(...container.childNodes);
        } else {
		template.innerHTML = untrusted ? html : create_trusted_html(html.replaceAll('<!>', '<!---->'));
	}
	return template.content;
}

plus a bit simplifies html() function

I believe @7nik's snippet could be simplified further to something like the following (using the same logic everywhere, which avoids repeating the replace bit for <!>).

I've removed the redundant template element, since the call to .append negates the benefit of having a fragment created by just calling .content. I'm also not sure how reliant the code truly is on having a document fragment (from a quick search, it'd seem it's not), in which case the call to .append could likely be avoided too by simply returning the container.

export function create_fragment_from_html(html, untrusted = false, namespace = NAMESPACE_HTML) {
        var fragment = document.createDocumentFragment();
        var containerEl;
        switch (namespace) {
            case NAMESPACE_SVG:
              containerEl = 'svg';
              break;
            case NAMESPACE_MATHML:
              containerEl = 'mathml';
              break;
            default:
              containerEl = 'div';
              break;
        }
        var container = document.createElementNS(namespace, containerEl);
        html = html.replaceAll('<!>', '<!---->'); // XHTML compliance
        container.innerHTML = untrusted ? html : create_trusted_html(html);
        fragment.append(...container.childNodes);
        return fragment;
}

However, unless I'm missing something, the special NS handling (which would indeed simplify html()) doesn't seem to have an effect, since <svg><path/><circle/></svg> seems to parse fine as HTML, see (I didn't try in XML mode, it could be that using createElementNS is indeed better in this case, as <svg>${html}</svg> might fail due to missing xmlns).

As for this PR specifically, the changes seem to work, although I'd suggest that the auto-added policy should be optional and configurable (e.g., it'd be nice to be able to provide a custom policy, for example when creating a component).

pinging @dummdidumm @Rich-Harris - is this something that Svelte would consider landing? It lets us set a stricter CSP, so its definitely useful as-is.

although I'd suggest that the auto-added policy should be optional and configurable

yeah agreed, this would be ideal - I think its useful without it though, and easy enough to add later if people need it.

FWIW, this is pretty similar to how it was implemented in Polymer Polymer/polymer@10220c9 so there is precedent

Wohoo, thanks @Rich-Harris!