Studio: fix two Codex findings on PR 5717 head
P1 -- ``scripts/check_new_install_scripts.py``: the head-only
rejection only refused new HEAD entries, not deletions. That left a
two-step bypass open:
1. PR A removes ``studio/frontend/.install-script-allowlist`` on
main (passes, since the lockfile has no new install-script
deps).
2. PR B then hits the bootstrap path (base allowlist missing)
and self-allowlists any newly introduced install-script
dependency, because bootstrap mode accepts head as-is.
Now also fail when head DROPS trusted base entries. Allowlist
deletions must land via their own reviewed commit instead of
chaining into the bootstrap window.
P2 -- ``html-svg-renderer.tsx``: ``<style>`` is removed from the
SVG sanitizer's FORBID_TAGS. The original justification was "inline
CSS would leak to the host page selectors", but the SVG preview
runs inside ``sandbox=""`` plus ``default-src 'none'`` -- the inner
``<style>`` cannot reach host page selectors and cannot fetch
external URLs (the CSP blocks ``@import`` and ``url(...)``).
Stripping ``<style>`` was breaking legitimate class-styled SVG
exports from real diagram tools. The existing
"strips inline <style>" test is replaced with one that proves
class-styled SVG renders as authored.
This commit is contained in:
parent
e3c7948874
commit
17afdfeb8d
3 changed files with 42 additions and 6 deletions
|
|
@ -372,6 +372,31 @@ def main(argv: list[str] | None = None) -> int:
|
|||
)
|
||||
return 1
|
||||
|
||||
# Also refuse a PR that DROPS trusted base entries. Without
|
||||
# this, an attacker could land a two-step bypass:
|
||||
# 1. PR A removes ``.install-script-allowlist`` from base
|
||||
# (passes -- no new lockfile findings).
|
||||
# 2. PR B then hits the bootstrap path (base allowlist
|
||||
# missing) and self-allowlists a newly introduced
|
||||
# install-script dependency.
|
||||
# Removing an allowlist entry is a security-sensitive change
|
||||
# and must land via the same review path that added it.
|
||||
removed_from_head = sorted(base_allowlist - head_allowlist)
|
||||
if removed_from_head:
|
||||
print(
|
||||
"[install-script-diff] FAIL: PR removes trusted base "
|
||||
"allowlist entries. Allowlist deletions must land in a "
|
||||
"separate, isolated commit so a follow-up PR cannot "
|
||||
"exploit the bootstrap path.",
|
||||
file = sys.stderr,
|
||||
)
|
||||
for entry in removed_from_head:
|
||||
print(
|
||||
f" dropped allowlist entry: {entry}",
|
||||
file = sys.stderr,
|
||||
)
|
||||
return 1
|
||||
|
||||
# Only the trusted base allowlist participates in the skip set.
|
||||
allowlist = base_allowlist
|
||||
|
||||
|
|
|
|||
|
|
@ -268,12 +268,19 @@ describe("sanitizeSvgSource", () => {
|
|||
expect(clean).toContain("<rect");
|
||||
});
|
||||
|
||||
it("strips inline <style> blocks so SVG CSS cannot retarget host selectors", () => {
|
||||
const svg = `<svg xmlns="http://www.w3.org/2000/svg"><style>body{display:none!important}</style><rect/></svg>`;
|
||||
it("keeps inline <style> blocks so class-styled SVG exports still render", () => {
|
||||
// The SVG preview iframe is fully sandboxed (sandbox="") and the
|
||||
// inner CSP is default-src 'none', so the iframe's <style> cannot
|
||||
// reach the host page selectors or fetch external URLs (CSP blocks
|
||||
// @import / url(...)). Stripping <style> broke legitimate class-
|
||||
// styled SVG exports from many diagram tools, which is the bigger
|
||||
// real-world cost than the (already-mitigated) selector leak.
|
||||
const svg = `<svg xmlns="http://www.w3.org/2000/svg"><style>.fg{fill:red}</style><rect class="fg"/></svg>`;
|
||||
const clean = sanitizeSvgSource(svg).toLowerCase();
|
||||
expect(clean).not.toContain("<style");
|
||||
expect(clean).not.toContain("display:none");
|
||||
expect(clean).toContain("<style");
|
||||
expect(clean).toContain(".fg");
|
||||
expect(clean).toContain("<rect");
|
||||
expect(clean).toContain('class="fg"');
|
||||
});
|
||||
|
||||
it("strips style attributes so inline CSS cannot fire url()/@import requests", () => {
|
||||
|
|
|
|||
|
|
@ -53,13 +53,17 @@ const HEURISTIC_UNSAFE_SVG_RE =
|
|||
// https://developer.mozilla.org/en-US/docs/Web/SVG/Guides/SVG_as_an_image
|
||||
const SVG_PURIFY_CONFIG = {
|
||||
USE_PROFILES: { svg: true, svgFilters: true },
|
||||
// ``style`` -- inline CSS would otherwise leak to the host page selectors.
|
||||
// ``image`` / ``use`` -- carry ``href``/``xlink:href`` and would let an
|
||||
// assistant fetch attacker-controlled URLs from the user's browser.
|
||||
// ``foreignObject`` -- can embed HTML inside the SVG and re-introduce XSS.
|
||||
// ``script`` / ``link`` / ``meta`` / ``iframe`` / ``embed`` / ``object``
|
||||
// are unconditional XSS / network surfaces. ``<style>`` is kept --
|
||||
// the SVG preview iframe is fully sandboxed (``sandbox=""``) with a
|
||||
// ``default-src 'none'`` CSP, so class-based styling that real
|
||||
// diagram exporters emit cannot leak to the host page or fetch
|
||||
// external URLs (the CSP blocks ``@import`` and ``url(...)``).
|
||||
FORBID_TAGS: [
|
||||
"script",
|
||||
"style",
|
||||
"foreignObject",
|
||||
"iframe",
|
||||
"embed",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue