feat: support trusted-types api#3801
Conversation
lunadogbot
left a comment
There was a problem hiding this comment.
Thanks for this — Trusted Types support is a nice security improvement, and the change is appropriately scoped (the only DOMParser/innerHTML write in the client runtime is exactly this one).
One blocking concern and a couple of nits:
Blocking — defensive policy creation
trustedTypes.createPolicy(...) can throw in a few situations:
- A policy named
fresh-partialsalready exists (e.g. after HMR re-evaluates the module). - The page's CSP has
require-trusted-types-for 'script'but thetrusted-typesdirective doesn't includefresh-partials—createPolicythen throws aTypeError.
Because the call sits at module top level, either case throws during module evaluation and the whole partials.ts module fails to load — meaning client-side partial navigation breaks for all users on that page, not just the ones with strict CSP. Suggest wrapping in a try/catch and falling back to the passthrough:
const freshTrustedTypes = (() => {
if (!("trustedTypes" in globalThis)) return { createHTML: (s: string) => s };
try {
// deno-lint-ignore no-explicit-any
return (globalThis as any).trustedTypes.createPolicy("fresh-partials", {
createHTML: (s: string) => s,
});
} catch {
return { createHTML: (s: string) => s };
}
})();That keeps partials working for anyone whose page CSP doesn't list the policy name yet (which is a perfectly reasonable thing to do — they get the existing string-based behavior, exactly as they do today).
nit: The variable name reads a bit oddly — it's a TrustedTypePolicy, not trustedTypes. Something like partialsPolicy would be clearer at the call site.
nit: Worth a small test that exercises applyPartials with trustedTypes defined on globalThis (you can stub it) to confirm the policy path is hit and the result is still a parseable Document. Not a hard blocker, but it'd guard against a future regression that silently strips the wrap.
Let me know if you'd like a hand getting this over the finish line.
fibibot
left a comment
There was a problem hiding this comment.
createPolicy("fresh-partials", ...) runs eagerly at module evaluation, so it executes during import "./partials.ts" (packages/fresh/src/runtime/client/mod.ts:3). Per the Trusted Types spec, if a trusted-types CSP directive is set without fresh-partials in the allowlist, createPolicy throws TypeError — even when require-trusted-types-for 'script' is absent. That's a regression for any existing Fresh user who has a trusted-types <names> directive that doesn't already list fresh-partials: the partials module fails to import and the feature breaks. Wrap the call in try/catch and fall back to the identity object so the worst case matches today's behavior.
- nit: worth a docs note (e.g. under
docs/canary/) so users with strict CSPs know to addfresh-partialstotrusted-types.
43d1804 to
85879c0
Compare
|
After the last changes the CI test failure is caused by deno update and will be fixed in #3815 |
85879c0 to
2f09b86
Compare
https://developer.mozilla.org/en-US/docs/Web/API/Trusted_Types_API
I have only encountered only place where fresh uses untrusted input, and while this might be suboptimal, I do not think server-side untrusted input handling is actionable anyway.
I have not added trusted types policy to csp() midleware, as it might vary depending on the packages used, but with this change any user of fresh can configure it on its own. E.g https://delta.rocks has the following CSP, strictest possible for fresh:
While trying to test, I have encountered that
@astral/astraltriggers trusted types violation on its own by default, but was unable to to find the problem here yetEdit: Actually, for delta.rocks I have added a couple of more items here due to wasm usage and injected HTML for documentation for example, but the rest is the same