Skip to content

fix: harden against confirmed review findings - #2

Merged
conorbronsdon merged 1 commit into
mainfrom
fix/hardening-review
Jul 7, 2026
Merged

fix: harden against confirmed review findings#2
conorbronsdon merged 1 commit into
mainfrom
fix/hardening-review

Conversation

@conorbronsdon

Copy link
Copy Markdown
Owner

From an automated multi-agent review (personal-context#62); implemented + verified by Claude Code.

Two confirmed hardening findings against the public repo. Each fix was reproduced on the current source, fixed, and re-verified; a regression test was added. Built and tested against Mojo 1.0.0b3.dev2026070506.

1. (security) image-alt breakout in _strip_tags — FIXED

Raw inline HTML passes through verbatim (CommonMark). When such HTML appears in an image description, _strip_tags scanned for the tag-closing > without honoring quoted attribute values. A raw > inside a quoted attribute (e.g. title="a>b") terminated the tag early, and the trailing bytes leaked into — and broke out of — the generated alt="…" attribute, emitting unbalanced, injection-prone HTML.

Repro / evidence (input ![<span title="a>b">x](/i.png)):

  • Before: <p><img src="/i.png" alt="b">x" /></p> ← the > closes the img early; x" leaks as markup
  • After: <p><img src="/i.png" alt="x" /></p> ← matches CommonMark reference

Fix: made the tag scan in _strip_tags quote-aware (skips over "/'-delimited attribute values). Regression test test_image_alt_raw_html_gt_no_breakout covers double-quoted, single-quoted, and nested-<img> cases.

2. (security/doc) output is not sanitized — DOCUMENTED in SECURITY.md

Raw HTML and javascript:/data: URLs pass through verbatim (spec-compliant), but SECURITY.md previously framed the only risk as crashes/hangs — a stored-XSS trap for callers. Added an explicit "Output is NOT sanitized" section: rendering untrusted markdown into a live page is a stored-XSS risk and callers must sanitize (e.g. DOMPurify-equivalent) themselves. Behavior is intentionally unchanged (spec compliance preserved); documented rather than changed. A safe-mode/sanitizing flag was considered but left out to keep the change low-risk and API-stable.

pixi pin

Checked: the repo's pixi.toml already pins mojo = ">=1.0.0b3.dev2026070506,<2", which solves correctly (pixi install succeeds). The >=1.0.0b3 sort-below-nightlies bug does not exist here — no change needed.

Verification

  • Unit suite: 49/49 passing (48 prior + 1 new regression test).
  • Conformance: 643/652 — identical before and after the fix (no regression; verified by stashing the fix).
  • Fuzz runner: no crash/hang on adversarial alt-breakout inputs.

Overlap note

A separate agent is editing README.md and CHANGELOG.md in parallel PRs; this PR deliberately does not touch either file. Based off origin/main.

Make _strip_tags quote-aware so a raw `>` inside a quoted attribute of
raw HTML in an image description cannot terminate the tag early and break
out of the generated alt="…" attribute (injection-prone unbalanced HTML).
Add a regression test covering double/single-quoted attributes and a
nested <img>.

Document in SECURITY.md that output is NOT sanitized: raw HTML and
javascript:/data: URLs pass through verbatim per CommonMark, so rendering
untrusted markdown into a live page is a stored-XSS risk that callers must
sanitize themselves.

Co-Authored-By: Claude <noreply@anthropic.com>
@conorbronsdon

Copy link
Copy Markdown
Owner Author

🤖 Independent Claude review:

Verdict: Ready to mark for review. Adversarial second look — no blocking correctness/security issue found.

Verified (by hand-tracing bytes, not just re-running the author's tests):

  • Fix closes the reported breakout AND its class, not just the one input. A breakout of alt="…" requires a literal " to survive into the output. The only unescaped bytes reaching _strip_tags are (a) raw-HTML tag interiors — dropped; (b) the nested-<img> alt extraction — its inner loop stops at the first QUOTE, so extracted text can't contain "; (c) normal text — escape_html maps "&quot; (common.mojo:140). So there's no residual quote-breakout vector, not just the demoed title="a>b" case.
  • Quote-aware scan is sound (inline.mojo:412+): tracks quote_ch so ' inside "…" is literal and vice-versa; the scan is byte-level but ASCII quote bytes (0x22/0x27) can't collide with UTF-8 multibyte (all ≥0x80), so no false quote/GT detection.
  • Regression test is non-vacuous: on the pre-fix source the double-quoted case renders alt="b">x" and the assert fails — it genuinely reproduces the bug. Single-quote + nested-<img> cases add real coverage. Not tautological.
  • No over/under-reach: _strip_tags has a single caller (image alt, inline.mojo:557) so there's no sibling site left unfixed; link text passes HTML through as element content (not an attribute), which is spec-compliant and not a breakout.
  • pixi pin: pixi.toml:15 already pins mojo = ">=1.0.0b3.dev2026070506,<2" (full nightly suffix → avoids the sort-below-nightlies trap). No change is the right call.
  • SECURITY.md claim is accurate: encode_url only percent-encodes, no scheme filtering, so javascript:/data: do pass through as documented.

Non-blocking nits (optional):

  • Unbalanced quote in raw HTML inside an image description (e.g. ![<span title="a>b>x](/i.png)) makes the unterminated 'tag' swallow the rest of the input — differs from the CommonMark reference but is safe (no breakout). Edge case, fine to leave.
  • Nested-<img> alt extraction only recognizes double-quoted alt="…"; a single-quoted alt='…' on a nested img isn't extracted. Pre-existing (not introduced here).

@conorbronsdon
conorbronsdon marked this pull request as ready for review July 6, 2026 07:58
@conorbronsdon

Copy link
Copy Markdown
Owner Author

Code review (opus, static — verify CI): SHIP-WITH-NITS

Fixes a real HTML-attribute breakout: image alt text flows into alt="…" unescaped (inline.mojo:574-575), and the old tag scan stopped at the first >, so a > inside a quoted attribute leaked ">x into the attribute. The quote-aware scan (inline.mojo:421-431) closes it cleanly and is the right layer. Verified the " breakout is fully closed (literal alt escaped upstream at :104; nested <img> alt extraction stops at first QUOTE at :443; unclosed-quote bounded). Conformance unchanged (643/652), so 98.6% holds.

Nit: inline.mojo:436-445 — nested-<img> alt-value insertion doesn't re-escape </>/& (not a breakout — those don't terminate a quoted attribute, and " can't be captured; spec-cosmetic only). (low)

@conorbronsdon
conorbronsdon merged commit 8b4eacd into main Jul 7, 2026
1 check passed
@conorbronsdon
conorbronsdon deleted the fix/hardening-review branch July 7, 2026 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant