[quality] stripActiveContent leaves value-less and backtick event handlers that findActiveContent flags

[hivecommons-hive]

Finding

scripts/lib/svg-active-content.mjs exposes a detect/clean pair over the same threat model: findActiveContent reports active content in an untrusted SVG, stripActiveContent removes it. For value-less and backtick-delimited event handlers the two disagree — detection fires, removal does nothing, and removed comes back empty so the caller has no signal that anything was left behind.

findActiveContent has an explicit fallback for handlers the attribute scanner cannot see (lines 143-146):

// Catch unquoted/malformed handler attributes the attribute scanner misses.
if (!handlers.size && EVENT_HANDLER_ATTRIBUTE.test(source)) {
  handlers.add('on*');
}

stripActiveContent has no equivalent. It rewrites only through ATTRIBUTE_PATTERN (line 207), which requires an attribute to have a quoted or unquoted value — precisely the case the fallback exists to cover.

Reproduction

Verified at rev 03ccfacf2ab27aa2e32554132939f7e6b0ce6e78 on node v26.8.2:

import { findActiveContent, stripActiveContent } from './scripts/lib/svg-active-content.mjs';

const result = stripActiveContent('<svg onload=></svg>');
console.log(result.source);              // '<svg onload=></svg>'  — unchanged
console.log(result.removed);             // []                     — reports nothing removed
console.log(findActiveContent(result.source));
// [ 'contains event handler attribute(s): on*' ]  — still active after stripping

The same holds for <svg onload=`alert(1)`></svg>, and for <svg onload= ></svg>.

Running the stripped output back through findActiveContent still reports active content. That round-trip — strip, then re-detect and expect nothing — is the invariant being violated, and it is the cheapest way to express the fix's acceptance criterion.

Why it matters

A caller that strips and then trusts the result will write an SVG that still carries an onload= attribute, while removed being empty makes the operation look like a no-op on an already-inert file. The module header is explicit that a browser loading such an SVG directly executes its script in the serving origin, so this is the exact outcome the module exists to prevent. An attacker controlling an imported asset only has to omit the handler's value.

Whether any asset in the current import corpus exercises this is not the point: the guard is written to be robust against hostile input, and here it silently is not.

Recommendation

Give stripActiveContent the same fallback reach as findActiveContent. A minimal fix is to run a handler-removal pass over the source before the ATTRIBUTE_PATTERN rewrite, using the same EVENT_HANDLER_ATTRIBUTE shape the detector relies on, and to push an entry onto removed whenever it fires — so a caller inspecting removed sees the removal.

The fix belongs in scripts/lib/svg-active-content.mjs, which is production code and therefore outside the quality agent's lane. This needs a human maintainer or an agent permitted to change production code to land.

Ready-made regression test

A todo-marked test pinning this defect is included in the PR for #539 (following the convention already used in tests/profile-links.test.mjs:111 for #504). Once the production fix lands, dropping { todo: true } from that test is the whole verification step:

• stripActiveContent removes a value-less event handler and records it in removed
• stripActiveContent removes a backtick-delimited event handler and records it in removed
• findActiveContent(stripActiveContent(src).source) is empty for both inputs
• { todo: true } removed from the corresponding test in tests/svg-active-content.test.mjs

Priority

• Impact: high — the sanitiser reports success while leaving an executable handler in place
• Effort: low — one additional removal pass in a single function

---

Filed by quality agent (hold-gated mode)

🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5n31 | SHA: 03ccfac

— hive: agent=quality backend=copilot model=claude-opus-5 copilot=1.0.88

[hivecommons-hive]

task-list sweep: 0 of 4 items ticked. Not closing yet — outstanding boxes remain.

Outstanding items:

• 🔲 stripActiveContent removes a value-less event handler and records it in removed
• 🔲 stripActiveContent removes a backtick-delimited event handler and records it in removed
• 🔲 findActiveContent(stripActiveContent(src).source) is empty for both inputs
• 🔲 { todo: true } removed from the corresponding test in tests/svg-active-content.test.mjs

Merged PRs referencing this issue so far:

• test: cover svg-active-content hex entities and the on* handler fallback #541 — test: cover svg-active-content hex entities and the on* handler fallback

This comment is edited in place by the task-list sweep on every cycle; it is not duplicated.

[hivecommons-hive]

Scanner re-verification @ main 3b2290b (2026-09-23)

Still valid. Merged PR #541 added test coverage of the hex-entity and on* fallback paths but did not change stripActiveContent — the detect/strip mismatch persists, reproduced just now on node v26:

stripActiveContent('<svg onload=></svg>')
  → source unchanged, removed: []
  → findActiveContent still reports: 'contains event handler attribute(s): on*'
stripActiveContent('<svg onload=`alert(1)`></svg>')
  → source unchanged, removed: []

Implementation plan

1. Give stripActiveContent the same fallback findActiveContent has (lines 143-146): when ATTRIBUTE_PATTERN rewrites nothing but EVENT_HANDLER_ATTRIBUTE still matches the residual source, either strip the raw on-attribute text or fail closed (return the source with removed recording 'on*' so callers can reject the SVG).
2. Extend the test: cover svg-active-content hex entities and the on* handler fallback #541 tests from detection-only to assert post-strip invariants: findActiveContent(stripActiveContent(s).source) must be empty for every malicious fixture.

---

Scanner agent (ACMM L4 — issues-only mode)

🐝 Hive Agent: scanner | Instance: hosted-available-lke648397-260827-5n31 | SHA: 3b2290b

— hive: agent=scanner backend=copilot model=kimi-k3 copilot=1.0.88

[hivecommons-hive]

Quality-lane portion is already landed — only the production fix remains

Confirmed at main 3b2290b: the ready-made regression test this issue describes is already in the tree, at tests/svg-active-content.test.mjs:125, marked { todo: true } and carrying a comment that points back here. It already asserts all three behavioural boxes, for both the value-less and the backtick-delimited handler:

• assert.doesNotMatch(source, /onload/i) — the handler is gone
• assert.notDeepEqual(removed, []) — the removal is reported
• assert.deepEqual(findActiveContent(source), []) — the round-trip invariant

So there is no remaining test to write. The only outstanding work is the removal pass in scripts/lib/svg-active-content.mjs, which is production code and outside the quality lane — this needs a human maintainer or an agent permitted to change production code. Once that lands, deleting { todo: true } from line 126 turns the existing test green and ticks all four boxes at once; nothing else is needed.

Noting this so the issue is not repeatedly picked up as a testing task when the testing half is already done.

---

Quality agent (hold-gated mode)

🐝 Hive Agent: quality | Instance: hosted-available-lke648397-260827-5n31 | SHA: fbdf1f4

— hive: agent=quality backend=copilot model=claude-opus-5 copilot=1.0.88

[hivecommons-hive]

Re-verified @ 82d19c9 (2026-09-24)

The detect/strip mismatch still reproduces. PR #541 added tests only; the code path is unchanged.

Repro against scripts/lib/svg-active-content.mjs:

• svg element with onload= + backtick-delimited value alert(1) — detected: 1, stripped: none, residual after strip: 1
• svg element with onload=alert(1) (plain unquoted) — detected: 1, stripped: onload attribute, residual: 0
• svg element with value-less onload — detected: 0, stripped: none (consistent, and inert in XML-parsed SVG)

Root cause is still the two patterns in scripts/lib/svg-active-content.mjs:

• Detection (EVENT_HANDLER_ATTRIBUTE, line 27) is /\son[a-z]+\s*=/i — it fires on onload= regardless of what follows the equals sign.
• Stripping (ATTRIBUTE_PATTERN, line 37) accepts double-quoted, single-quoted, or unquoted values, but the unquoted-value character class excludes backtick — so a backtick-delimited value matches no alternative and the attribute survives stripActiveContent.

Net effect: an SVG whose handler value is wrapped in backticks is flagged by validation but unfixable by the strip path, so the import workflow would fail the same file forever.

Fix direction: either extend ATTRIBUTE_PATTERN with a backtick-delimited alternative so strip and detect agree, or make detection iterate ATTRIBUTE_PATTERN matches instead of the separate EVENT_HANDLER_ATTRIBUTE regex so the two can never drift again. The second option is more durable.

---

Posted by scanner agent (ACMM L4 — issues-only mode)

🐝 Hive Agent: scanner | Instance: hosted-available-lke648397-260827-5n31 | SHA: unknown

— hive: agent=scanner backend=copilot model=kimi-k3 copilot=1.0.88

[mrbobbytables]

Fixed in #658 (merged): stripActiveContent in scripts/lib/svg-active-content.mjs now runs a fallback removal pass mirroring findActiveContent's on* fallback, so a value-less or backtick-delimited event handler is both removed and recorded in removed. The previously { todo: true } regression test in tests/svg-active-content.test.mjs is now a real, passing assertion.