Skip to content

feat: support trusted-types api#3801

Open
CertainLach wants to merge 2 commits into
freshframework:mainfrom
CertainLach:push-rzoxxqrnruku
Open

feat: support trusted-types api#3801
CertainLach wants to merge 2 commits into
freshframework:mainfrom
CertainLach:push-rzoxxqrnruku

Conversation

@CertainLach
Copy link
Copy Markdown
Contributor

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:

csp({
    useNonce: true,
    csp: [
      "script-src 'self' 'unsafe-inline'",
      "style-src 'self' 'unsafe-inline'",
      "require-trusted-types-for 'script'",
      // webpack policy is created by freshpack: trustedTypes.policyName = "webpack"
      //     for webpack own runtime script imports
      // fresh-partials used for fresh own partial loading
      "trusted-types webpack fresh-partials",
      "img-src 'self' data:",
      "font-src 'self'",
      "connect-src 'self'",
      "base-uri 'self'",
      "frame-ancestors 'none'",
    ],
  })

While trying to test, I have encountered that @astral/astral triggers trusted types violation on its own by default, but was unable to to find the problem here yet

Edit: 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

Copy link
Copy Markdown
Contributor

@lunadogbot lunadogbot left a comment

Choose a reason for hiding this comment

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

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:

  1. A policy named fresh-partials already exists (e.g. after HMR re-evaluates the module).
  2. The page's CSP has require-trusted-types-for 'script' but the trusted-types directive doesn't include fresh-partialscreatePolicy then throws a TypeError.

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.

Copy link
Copy Markdown
Contributor

@fibibot fibibot left a comment

Choose a reason for hiding this comment

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

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 add fresh-partials to trusted-types.

@CertainLach
Copy link
Copy Markdown
Contributor Author

After the last changes the CI test failure is caused by deno update and will be fixed in #3815

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants