unoplatform / Uno.Wasm.Bootstrap

A simple nuget package to run C# code in a WASM-compatible browser
Other
371 stars 57 forks source link

Include Single Quotes in EscapeJS? #773

Closed SerratedSharp closed 1 year ago

SerratedSharp commented 1 year ago

Consider adding single quotes as a character escaped in WebAssemblyRuntime.EscapeJS

It might be non-obvious that code like this would allow a string to break out of the quotes and run arbitrary JS. It's also pretty tempting to use single quotes in the embedded JS to ease conflict with the C# double quotes, and even some of these examples use single quotes as the flip back and forth between examples, demonstrating use of EscapeJS where appropriate, but don't necessarily note the danger: https://platform.uno/docs/articles/interop/wasm-javascript-1.html#invoke-javascript-code-from-c

  string userText = """
      ");alert("bad stuff");alert("
      """;
  // attempt to break out of double quotes defeated by EscapeJs
  userText = WebAssemblyRuntime.EscapeJs(userText);
  WebAssemblyRuntime.InvokeJS($"""
      var d = document.getElementById("first");                
      d.textContent("{userText}");
      """);

  // Not safe
  string userText = "');alert('bad stuff');alert('";
  userText = WebAssemblyRuntime.EscapeJs(userText);
  WebAssemblyRuntime.InvokeJS($@"
      var d = document.getElementById('first');                
      d.textContent('{userText}');
      ");

Obviously caller is responsible for appropriate sanitization based on the context of where user data is being embedded, and we can't protect from all possible scenarios, but in terms of appropriate sanitization in the appropriate place, typically single quotes would be included in a JS encoding.

Could potentially be a breaking change if someone is already doing WebAssemblyRuntime.EscapeJs(html).Replace("'", @"\'") themselves, as the change would then result in double encoding that would still be safe, but mangled.

jeromelaban commented 1 year ago

Thanks for the report. Indeed, I don't think we'll change this behavior because it is a breaking change, but in general it is now recommended to use JSImport instead of InvokeJS.