Sanitize plugin-supplied HTML and validate icon names - #107
Merged
Conversation
The html widget assigned plugin content straight to innerHTML inside a shadow root. Shadow DOM isolates styles, not scripts, and the app sets no CSP, so an inline event handler in plugin markup ran in the host realm with access to the Tauri invoke bridge — letting a plugin that declared only ui.panel reach every capability the app has. iconHtml had the same shape: a plugin-supplied icon name was interpolated into an img src with no escaping, so a name could close the attribute and add a handler. Add an allowlist sanitizer that parses untrusted markup into an inert document and rebuilds it node by node, keeping only vetted elements, attributes, and URL schemes. Inline SVG is namespace-aware so icons still render and case-sensitive names like viewBox survive. Icon names are validated as bare filenames rather than escaped. Extract both into their own modules and add vitest + jsdom, the first JS test coverage in the repo. 57 tests, including a regression case built from the tmux manager example's real markup.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4726b938e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+115
to
+117
| dest.setAttribute(attribute.name, value); | ||
| // A link that opens elsewhere must not hand the opener to the target. | ||
| if (name === 'target') dest.setAttribute('rel', 'noopener noreferrer'); |
There was a problem hiding this comment.
noopener after all link attributes
When a plugin supplies a link with target before rel, such as <a target="_blank" rel="opener" ...>, this sets the safe rel value while processing target, then the later rel attribute overwrites it. The opened page can consequently retain window.opener and tabnab the application; enforce or merge noopener noreferrer after the attribute loop (or reject plugin-provided rel for targeted links).
Useful? React with 👍 / 👎.
Brings in the CI pipeline fixes and the markdown preview work. The only conflict was .gitignore, where main added the markdown vendor bundle and this branch added the root node_modules for the vitest harness — both entries kept. main's markdown preview sanitizes through a script-less sandboxed iframe, a different surface and mechanism from this branch's shadow-DOM plugin widget sanitizer, so the two do not overlap.
an0nn30
added a commit
that referenced
this pull request
Sep 3, 2026
Brings in the plugin HTML sanitizer from #107. No conflicts: that branch's docs/plugin-sdk.md edits cover the html widget's sanitization allowlist, while this branch's cover payload conversion behavior for app.publish, app.query_plugin, app.register_settings_section, ui.form, ui.open_docked_view, session.current and session.exec_active. Different sections, no contradiction. Full workspace suite green after the merge, including the 93 termlab_plugin tests covering the new bridge.
an0nn30
added a commit
that referenced
this pull request
Sep 3, 2026
Step 1 of the core is merged, so the spec now records it as done and the plan is marked executed rather than reading as outstanding work. Three corrections to the spec, all found while implementing it: - It named three hand-rolled converters; there were four. The fourth, set_lua_table_from_json_map in session.rs, copied only scalars, so nested objects and arrays were silently dropped from session.current() and session.exec_active(). - The vitest dependency note described fix/plugin-widget-html-xss as pending; it merged in #107. - handle_query is recorded as the last hand-rolled JSON edge still open: it hands on_query a JSON string and discards a malformed reply with .ok(). Verified still present on main; belongs with step 4.
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two injection vulnerabilities in the plugin widget renderer, both letting a plugin escape its declared capabilities.
1. The
htmlwidget was a sandbox escape.renderHtmlWidgetassigned plugin content straight toinnerHTMLinside a shadow root, and the app sets"csp": null. Shadow DOM isolates styles, not scripts — so whileinnerHTMLwon't run inline<script>, an event-handler attribute like<img src=x onerror="...">executes in the main window's realm, wherewindow.__TAURI__.invokelives. A plugin declaring onlyui.panelcould call any Tauri command without ever passing throughcheck_capability.2.
iconHtmlhad the same shape. A plugin-supplied icon name was interpolated into animg srcwith no escaping, so a name likex.png" onerror="…broke out of the attribute. Reachable fromui.panel_icon_label,ui.panel_button{icon}, tree nodes, and table cells.Both contradict
docs/plugin-security-model.md, which promises least privilege and denied-by-default gates.How
Untrusted markup is parsed into an inert document via
DOMParserand rebuilt node by node against an allowlist, rather than scrubbed in place. Only text and vetted attributes cross over, and nothing is re-serialized — which is what makes mutation-XSS round-trips possible in string-based sanitizers.Inline SVG is supported but namespace-aware: elements are created with
createElementNS, and attributes keep their original casing soviewBoxsurvives.script,foreignObject,use, and the animation elements are excluded.Icon names are validated as bare filenames rather than escaped — a name that isn't a bare filename can't address the bundled icon set anyway.
Notes
lua-tmux-manager.lua, which builds its whole UI from<button>and inline<svg>. There's a regression test built from that plugin's real markup.cssargument is deliberately not sanitized; CSS cannot execute script."csp": nullis unchanged. Adding a CSP would be worthwhile defense-in-depth but is a riskier change that didn't belong in this branch.docs/plugin-sdk.mddocuments exactly what survives sanitization.Test plan
Verified at the unit level; I did not launch the app to confirm the tmux panel renders visually.
🤖 Generated with Claude Code