Replies: 1 comment
|
Hey, thanks for raising this, quick background before I have time to dive more into this in detail: There are numerous ways that html content gets rendered and can be supplied to windshift. We got the classic webview, the Tauri app as well as the portal layer - but most importantly - outbound email renderers. The best way would probably as you suggested to rely on rendering sanitization. The problem is that DOMPurify itself has vulnerabilities that need to be patched on an ongoing basis. So bluemonday is the second line of defence here. I went the safe route and just stripped html with very few exceptions (exceptions which are necessary for certain features in the MilkdownEditor to work) I will get back to this discussion later with some ideas how to be more consistent and permissive |
Uh oh!
There was an error while loading. Please reload this page.
The bug
The server sanitizer deletes tag-shaped text inside inline code spans. If I save a description or comment like:
it comes back as:
<port>is gone. Anything that looks like an HTML tag gets stripped even inside backticks, where it's just text.I tried to fix it and kept hitting the same wall, so I'd rather ask about the intended trust boundary before writing a PR.
What makes it awkward
These fields are stored as Markdown and rendered on the client. The read view runs
markedthenDOMPurify; the editor just keeps the Markdown source. The server sanitizer is a second layer. It wants to keep real code content (<port>is code, not HTML) but still strip anything dangerous, and doing that in Markdown on the server is where it gets messy. I had two attempts working and threw both away once I looked hard at them.Attempt 1: pull code spans out to placeholders, sanitize the rest, put them back
Round-trips code fine. But it kept springing leaks, all the same shape: the placeholder boundary depends on Markdown context that shifts while you sanitize.
\`escaped backticks aren't a code span in CommonMark, but a backtick scanner counts them and protects a span that isn't there.Each is fixable on its own. Together they told me the approach was wrong.
Attempt 2: parse once, keep code verbatim, cut the dangerous nodes out of the source
Parse with a CommonMark parser, walk the tree, delete the source ranges of dangerous nodes (raw HTML, bad URLs), copy everything else byte for byte. No placeholders, byte-exact. Also broken, two ways:
<scrandipt>are text and the<i>between them is raw HTML, deleting<i>leaves<scrandipt>next to each other, and a later parse reads that as a tag. Cutting bytes out isn't safe when the result gets parsed again.marked. What one treats as text the other can treat as markup, so the server's decision doesn't cover the surface that actually matters, which is what the client renders.What I'm actually asking
For the read view,
markedplusDOMPurifyis the thing that decides what runs. The server stripping is both what's eating<port>and something I can't prove safe against a different parser downstream. So is client-side sanitization meant to be the real boundary, or is the server supposed to be authoritative?If the client is the boundary, the clean fix is to stop stripping HTML on the body fields (keep the length caps) and leave the strict policies alone for the plain-text fields (names, IDs, URLs). That only works if every first-party place that renders these as HTML actually sanitizes on the way out: read view, notification and email bodies, exports, search highlighting, admin and mobile. The REST API is a separate case. A JSON response doesn't execute anything, so there I'd rather the field just be documented as untrusted Markdown than pre-scrubbed.
So:
markedplusDOMPurify) the intended XSS boundary, with the server sanitizer as backup?Things I skipped on purpose
One thing to watch if you do stop stripping: it changes stored data, so a rolling deploy or a rollback means an older client, worker, or export can read a record that still assumes the old stripped shape. So it needs a minimum client version and a rollout plan, not just "today's consumers are fine."
Happy to send a PR once there's a direction, and to hand over the specifics: the sanitizer and parser, the
markedandDOMPurifyversions and config, the write paths, and the tests from the prototypes. Current behavior deletes the brackets but it's safe, so there's no rush to ship a half fix.All reactions