fix: __FRSH_STATE potentially being overwritten by user code (#2256)

As outlined in #2254 it was
possible to overwrite the script tag that is used by Fresh to pass
island props from the server to the client. This could be done by
injecting raw HTML via `dangerouslySetInnerHTML` and simply using the
same id that Fresh was using.

With this PR we generate a unique id per render that cannot be guessed
anymore. It's composed of `__FRSH_STATE_<uuid>`

src/runtime/entrypoints/main.ts

@@ -640,6 +640,7 @@ class NoPartialsError extends Error {}
  */
 export async function applyPartials(res: Response): Promise<void> {
   const contentType = res.headers.get("Content-Type");
+  const uuid = res.headers.get("X-Fresh-UUID");
   if (contentType !== "text/html; charset=utf-8") {
     throw new Error(partialErrorMessage);
   }
@@ -652,7 +653,7 @@ export async function applyPartials(res: Response): Promise<void> {
   // Preload all islands because they need to be available synchronously
   // for rendering later
   const islands: IslandRegistry = {};
-  const dataRaw = doc.getElementById("__FRSH_PARTIAL_DATA")!;
+  const dataRaw = doc.getElementById(`__FRSH_PARTIAL_DATA_${uuid}`)!;
   let data: {
     islands: Record<string, { export: string; url: string }>;
     signals: string | null;
@@ -669,7 +670,7 @@ export async function applyPartials(res: Response): Promise<void> {
     );
   }
 
-  const stateDom = doc.getElementById("__FRSH_STATE")?.textContent;
+  const stateDom = doc.getElementById(`__FRSH_STATE_${uuid}`)?.textContent;
   let state: SerializedState = [[], []];
 
   // Load all dependencies

src/server/context.ts

@@ -669,7 +669,7 @@ const createRenderNotFound = (
   return async (req, ctx) => {
     const notFound = extractResult.notFound;
     if (!notFound.component) {
-      return sendResponse(["Not found.", undefined], {
+      return sendResponse(["Not found.", "", undefined], {
         status: STATUS_CODE.NotFound,
         isDev: dev,
         statusText: undefined,
@@ -771,19 +771,20 @@ function collectEntrypoints(
 }
 
 function sendResponse(
-  resp: [string, ContentSecurityPolicy | undefined],
+  resp: [string, string, ContentSecurityPolicy | undefined],
   options: {
     status: number;
     statusText: string | undefined;
     headers?: HeadersInit;
     isDev: boolean;
   },
 ) {
+  const [body, uuid, csp] = resp;
   const headers: Record<string, string> = {
     "content-type": "text/html; charset=utf-8",
+    "x-fresh-uuid": uuid,
   };
 
-  const [body, csp] = resp;
   if (csp) {
     if (options.isDev) {
       csp.directives.connectSrc = [

src/server/render.ts

@@ -122,7 +122,7 @@ export function checkAsyncComponent<T>(
  */
 export async function render<Data>(
   opts: RenderOptions<Data>,
-): Promise<[string, ContentSecurityPolicy | undefined] | Response> {
+): Promise<[string, string, ContentSecurityPolicy | undefined] | Response> {
   const component = opts.route.component;
 
   // Only inherit layouts up to the nearest root layout.
@@ -242,6 +242,7 @@ export async function render<Data>(
   // ensures that each render request is associated with the same
   // data.
   const renderState = new RenderState(
+    crypto.randomUUID(),
     {
       url,
       route: opts.route.pattern,
@@ -393,5 +394,5 @@ export async function render<Data>(
     moduleScripts: result.moduleScripts,
     lang: ctx.lang,
   });
-  return [html, csp];
+  return [html, renderState.renderUuid, csp];
 }

src/server/rendering/fresh_tags.tsx

@@ -72,16 +72,20 @@ export function renderFreshTags(
   // The inline script that will hydrate the page.
   let script = "";
 
-  // Serialize the state into the <script id=__FRSH_STATE> tag and generate the
+  // Serialize the state into the <script id="__FRSH_STATE-<uuid>"> tag and generate the
   // inline script to deserialize it. This script starts by deserializing the
   // state in the tag. This potentially requires importing @preact/signals.
   let hasSignals = false;
   let requiresDeserializer = false;
   if (state[0].length > 0 || state[1].length > 0) {
+    // Careful: This must be unique per render to avoid injected content
+    // via `dangerouslySetInnerHTML` being able to overwrite our state.
+    const stateId = `__FRSH_STATE_${renderState.renderUuid}`;
+
     const res = serialize(state);
     const escapedState = htmlEscapeJsonString(res.serialized);
     opts.bodyHtml +=
-      `<script id="__FRSH_STATE" type="application/json" nonce="${renderState.getNonce()}">${escapedState}</script>`;
+      `<script id="${stateId}" type="application/json" nonce="${renderState.getNonce()}">${escapedState}</script>`;
 
     hasSignals = res.hasSignals;
     requiresDeserializer = res.requiresDeserializer;
@@ -94,7 +98,7 @@ export function renderFreshTags(
       const url = addImport("signals.js");
       script += `import { signal } from "${url}";`;
     }
-    script += `const ST = document.getElementById("__FRSH_STATE").textContent;`;
+    script += `const ST = document.getElementById("${stateId}").textContent;`;
     script += `const STATE = `;
     if (res.requiresDeserializer) {
       if (res.hasSignals) {
@@ -166,7 +170,7 @@ export function renderFreshTags(
     );
     const nonce = renderState.csp ? ` nonce="${renderState.getNonce()}` : "";
     opts.bodyHtml +=
-      `<script id="__FRSH_PARTIAL_DATA" type="application/json"${nonce}">${escapedData}</script>`;
+      `<script id="__FRSH_PARTIAL_DATA_${renderState.renderUuid}" type="application/json"${nonce}">${escapedData}</script>`;
   }
   if (script !== "") {
     opts.bodyHtml +=

src/server/rendering/state.ts

@@ -16,6 +16,7 @@ export interface RenderStateRouteOptions {
 }
 
 export class RenderState {
+  readonly renderUuid: string;
   // deno-lint-ignore no-explicit-any
   componentStack: any[];
   renderingUserTemplate = false;
@@ -48,12 +49,14 @@ export class RenderState {
   basePath: string;
 
   constructor(
+    renderUuid: string,
     routeOptions: RenderStateRouteOptions,
     // deno-lint-ignore no-explicit-any
     componentStack: any[],
     csp?: ContentSecurityPolicy,
     error?: unknown,
   ) {
+    this.renderUuid = renderUuid;
     this.routeOptions = routeOptions;
     this.csp = csp;
     this.componentStack = componentStack;

tests/fixture/fresh.gen.ts

@@ -74,6 +74,7 @@ import * as $route_groups_bar_boof_index from "./routes/route-groups/(bar)/boof/
 import * as $route_groups_foo_layout from "./routes/route-groups/(foo)/_layout.tsx";
 import * as $route_groups_foo_index from "./routes/route-groups/(foo)/index.tsx";
 import * as $signal_shared from "./routes/signal_shared.tsx";
+import * as $spoof_state from "./routes/spoof_state.tsx";
 import * as $state_in_props_middleware from "./routes/state-in-props/_middleware.ts";
 import * as $state_in_props_index from "./routes/state-in-props/index.tsx";
 import * as $state_middleware_middleware from "./routes/state-middleware/_middleware.ts";
@@ -85,6 +86,7 @@ import * as $std from "./routes/std.tsx";
 import * as $umlaut_äöüß from "./routes/umlaut-äöüß.tsx";
 import * as $wildcard from "./routes/wildcard.tsx";
 import * as $Counter from "./islands/Counter.tsx";
+import * as $DangerousIsland from "./islands/DangerousIsland.tsx";
 import * as $Foo_Bar from "./islands/Foo.Bar.tsx";
 import * as $FormIsland from "./islands/FormIsland.tsx";
 import * as $Greeter from "./islands/Greeter.tsx";
@@ -194,6 +196,7 @@ const manifest = {
     "./routes/route-groups/(foo)/_layout.tsx": $route_groups_foo_layout,
     "./routes/route-groups/(foo)/index.tsx": $route_groups_foo_index,
     "./routes/signal_shared.tsx": $signal_shared,
+    "./routes/spoof_state.tsx": $spoof_state,
     "./routes/state-in-props/_middleware.ts": $state_in_props_middleware,
     "./routes/state-in-props/index.tsx": $state_in_props_index,
     "./routes/state-middleware/_middleware.ts": $state_middleware_middleware,
@@ -208,6 +211,7 @@ const manifest = {
   },
   islands: {
     "./islands/Counter.tsx": $Counter,
+    "./islands/DangerousIsland.tsx": $DangerousIsland,
     "./islands/Foo.Bar.tsx": $Foo_Bar,
     "./islands/FormIsland.tsx": $FormIsland,
     "./islands/Greeter.tsx": $Greeter,

tests/fixture/islands/DangerousIsland.tsx

@@ -0,0 +1,10 @@
+import { useEffect, useState } from "preact/hooks";
+
+export default function RawIsland(props: { raw: string }) {
+  const [css, set] = useState("");
+  useEffect(() => {
+    set("raw_ready");
+  }, []);
+
+  return <div class={css} dangerouslySetInnerHTML={{ __html: props.raw }} />;
+}

tests/fixture/routes/spoof_state.tsx

@@ -0,0 +1,5 @@
+import DangerousIsland from "../islands/DangerousIsland.tsx";
+
+export default function SerializePrototype() {
+  return <DangerousIsland raw={`<h1 id="__FRSH_STATE">{.invalid.json}</h1>`} />;
+}

tests/fixture_partials/fresh.gen.ts

@@ -117,9 +117,12 @@ import * as $relative_link_index from "./routes/relative_link/index.tsx";
 import * as $scroll_restoration_index from "./routes/scroll_restoration/index.tsx";
 import * as $scroll_restoration_injected from "./routes/scroll_restoration/injected.tsx";
 import * as $scroll_restoration_update from "./routes/scroll_restoration/update.tsx";
+import * as $spoof_state_index from "./routes/spoof_state/index.tsx";
+import * as $spoof_state_partial from "./routes/spoof_state/partial.tsx";
 import * as $Counter from "./islands/Counter.tsx";
 import * as $CounterA from "./islands/CounterA.tsx";
 import * as $CounterB from "./islands/CounterB.tsx";
+import * as $DangerousIsland from "./islands/DangerousIsland.tsx";
 import * as $Fader from "./islands/Fader.tsx";
 import * as $InvalidSlot from "./islands/InvalidSlot.tsx";
 import * as $KeyExplorer from "./islands/KeyExplorer.tsx";
@@ -262,11 +265,14 @@ const manifest = {
     "./routes/scroll_restoration/index.tsx": $scroll_restoration_index,
     "./routes/scroll_restoration/injected.tsx": $scroll_restoration_injected,
     "./routes/scroll_restoration/update.tsx": $scroll_restoration_update,
+    "./routes/spoof_state/index.tsx": $spoof_state_index,
+    "./routes/spoof_state/partial.tsx": $spoof_state_partial,
   },
   islands: {
     "./islands/Counter.tsx": $Counter,
     "./islands/CounterA.tsx": $CounterA,
     "./islands/CounterB.tsx": $CounterB,
+    "./islands/DangerousIsland.tsx": $DangerousIsland,
     "./islands/Fader.tsx": $Fader,
     "./islands/InvalidSlot.tsx": $InvalidSlot,
     "./islands/KeyExplorer.tsx": $KeyExplorer,

tests/fixture_partials/islands/DangerousIsland.tsx

@@ -0,0 +1,10 @@
+import { useEffect, useState } from "preact/hooks";
+
+export default function DangerousIsland(props: { raw: string }) {
+  const [css, set] = useState("");
+  useEffect(() => {
+    set("raw_ready");
+  }, []);
+
+  return <div class={css} dangerouslySetInnerHTML={{ __html: props.raw }} />;
+}

tests/fixture_partials/routes/spoof_state/index.tsx

@@ -0,0 +1,12 @@
+import { Partial } from "$fresh/runtime.ts";
+
+export default function SerializePrototype() {
+  return (
+    <div>
+      <Partial name="content">
+        <p>initial</p>
+      </Partial>
+      <a href="/spoof_state/partial">Update</a>
+    </div>
+  );
+}

tests/fixture_partials/routes/spoof_state/partial.tsx

@@ -0,0 +1,11 @@
+import { Partial } from "$fresh/runtime.ts";
+import DangerousIsland from "../../islands/DangerousIsland.tsx";
+
+export default function Res() {
+  return (
+    <Partial name="content">
+      <DangerousIsland raw={`<h1 id="__FRSH_STATE">{.invalid.json}</h1>`} />
+      <p class="done">partial</p>
+    </Partial>
+  );
+}

tests/main_test.ts

@@ -949,7 +949,7 @@ Deno.test("Adds nonce to inline scripts", async () => {
   await withFakeServe("./tests/fixture/main.ts", async (server) => {
     const doc = await server.getHtml(`/nonce_inline`);
 
-    const stateScript = doc.querySelector("#__FRSH_STATE")!;
+    const stateScript = doc.querySelector("[id^=__FRSH_STATE]")!;
     const nonce = stateScript.getAttribute("nonce")!;
 
     const el = doc.querySelector("#inline-script")!;
@@ -1220,3 +1220,18 @@ Deno.test("empty string fallback for optional params", async () => {
     assertEquals(data, { path: "foo", version: "" });
   });
 });
+
+// See https://github.com/denoland/fresh/issues/2254
+Deno.test("should not be able to override __FRSH_STATE", async () => {
+  await withPageName("./tests/fixture/main.ts", async (page, address) => {
+    let didError = false;
+    page.on("pageerror", (ev) => {
+      didError = true;
+      console.log(ev);
+    });
+    await page.goto(`${address}/spoof_state`);
+    await page.waitForSelector(".raw_ready");
+
+    assert(!didError);
+  });
+});

tests/partials_test.ts

@@ -1569,3 +1569,23 @@ Deno.test("render partial without title", async () => {
     },
   );
 });
+
+// See https://github.com/denoland/fresh/issues/2254
+Deno.test("should not be able to override __FRSH_STATE", async () => {
+  await withPageName(
+    "./tests/fixture_partials/main.ts",
+    async (page, address) => {
+      let didError = false;
+      page.on("pageerror", (ev) => {
+        didError = true;
+        console.log(ev);
+      });
+      await page.goto(`${address}/spoof_state`);
+
+      await page.click("a");
+      await page.waitForSelector(".raw_ready");
+
+      assert(!didError);
+    },
+  );
+});

tests/render_test.ts

@@ -18,8 +18,8 @@ Deno.test("doesn't leak data across renderers", async () => {
     const resp = await handler(req);
     const doc = parseHtml(await resp.text());
 
-    assertSelector(doc, "#__FRSH_STATE");
-    const text = doc.querySelector("#__FRSH_STATE")?.textContent!;
+    assertSelector(doc, "[id^=__FRSH_STATE]");
+    const text = doc.querySelector("[id^=__FRSH_STATE]")?.textContent!;
     const json = JSON.parse(text);
     assertEquals(json, { "v": [[{ "site": name }], []] });
   }