dom-expressions icon indicating copy to clipboard operation
dom-expressions copied to clipboard

Invalid double escaping of nested arrays in SSR

Open axel-habermaier opened this issue 1 year ago • 4 comments

Consider the following code:

function Test() {
  const a = ["<"];
  return <div>{[a, a]}</div>;
}

During SSR, this incorrectly renders the HTML <div>&amp;lt;&amp;lt;</div>, i.e., the < is escaped twice and is thus shown as &lt;&lt; in the browser.

I expected SSR to generate the single-escaped HTML <div>&lt;&lt;</div> which then renders as << in the browser.

axel-habermaier avatar Jan 10 '25 19:01 axel-habermaier

Update: The underlying reason seems to be that the array a is unexpectedly modified during rendering! So if a were at module scope, there would be additional escaping for each render.

axel-habermaier avatar Jan 14 '25 08:01 axel-habermaier

Yeah, that can also be reproduced using only jsx

import { renderToString } from "solid-js/web"

let x = "<"
let a = <>{x}{x}</>
let v = <>{a}{a}</>

console.log(v)
console.log(renderToString(() => <>{v}</>))
console.log(v)

we definitely should create a new array here before escaping its elements https://github.com/ryansolid/dom-expressions/blob/8f9f43254cf313d2cda653502da1097c76b991af/packages/dom-expressions/src/server.js#L412-L418

also created https://github.com/mdynnl/solid-ssr-escape-bench to benchmark which approach is faster although it may vary depending on the runtime

after trying with both bun(1.2.3-canary) and node(v20.17.0), i honestly don't know which one is faster as the results are entirely different, but .slice() and Array(length) seems to be on top in most cases which makes sense somewhat. and although i don't know enough js engine internals, i'd expect .slice() to only allocate memory for the array itself, so it's probably the answer here.

mdynnl avatar Feb 09 '25 22:02 mdynnl

@mdynnl Can you benchmark return s.map(escape)? That seems like the obvious way to write it, and hopefully map is smart enough to preallocate the array and maybe do the loop slightly more efficiently.

edemaine avatar Feb 09 '25 22:02 edemaine

@mdynnl Can you benchmark return s.map(escape)? That seems like the obvious way to write it, and hopefully map is smart enough to preallocate the array and maybe do the loop slightly more efficiently.

No, but be very careful with shorting map as array.map(escape) !

map passes to the callback three arguments. Same as doing array.map((element, index, thisArg) => escape(element, index, thisArg)) !!

The escape function signature is escape(string, attribute) so attribute will be truthy when index > 0

This is easy to forget, I don't even know how I notice that.

titoBouzout avatar May 27 '25 02:05 titoBouzout

PR in https://github.com/ryansolid/dom-expressions/pull/444

titoBouzout avatar Jul 12 '25 11:07 titoBouzout