Skip to content

fix(security): v0.35.0 — escape Markdown report table cells; old-DB upgrade regression test#121

Merged
hxddh merged 1 commit into
mainfrom
claude/architecture-code-docs-review-k41dn6
Jul 17, 2026
Merged

fix(security): v0.35.0 — escape Markdown report table cells; old-DB upgrade regression test#121
hxddh merged 1 commit into
mainfrom
claude/architecture-code-docs-review-k41dn6

Conversation

@hxddh

@hxddh hxddh commented Jul 17, 2026

Copy link
Copy Markdown
Owner

What changed

A focused release from a report-generation audit.

🔴 Security — Markdown report table injection / stored XSS

The report renderer's _table (sidecar/app/runs/analysis_report.py) interpolated cell values straight between | delimiters with no escaping — and the cells carry the most attacker-influenceable data in the product: object keys and user-agents (an S3 key legally contains |, newlines, and angle brackets). Unescaped:

  • a | in a key misaligns the row's columns (size/storage-class shift under the wrong headers);
  • a newline splits one row into two, corrupting the rest of the table;
  • a key like '<img src=x onerror=...>' lands verbatim in the saved .md → stored HTML/script in any viewer that renders inline HTML.

Credential redaction (applied to the whole document on write) only scrubs secret patterns — it does not touch markdown metacharacters, so it didn't cover this. Fixed with a _cell() escaper (| \r \n \ < >`) applied at the render boundary, covering all four report types (access-log, inventory, config-review, account-discovery) at once. Backward-compatible with app-generated content.

🟢 Regression coverage — old-DB upgrade survival

Added a test proving an old-schema DB upgrades cleanly: rows in the tables that later migrations rebuild (tool_calls@m002, datasets@m004) survive with data intact (kind→dataset_type, source_path→stored_path, counts preserved). The path was sound but had no coverage.

What checks were run

  • cd sidecar && pytest -q596 passed (+3 in tests/test_v0350_fixes.py).
  • ruff check app — clean.
  • Existing report tests (test_config_review, test_runs, test_account_discovery) — green (escaping is backward-compatible).

Note

This round's other two mining angles (skills-content drift, migration deep-dive) were interrupted by a session limit. I completed the skills-drift audit inline — it came back clean (every tool/param a SKILL.md references still resolves to a real tool) — and verified the migration happy-path is sound (hence the regression test above). A fuller re-run of those angles is in progress separately.

Security invariants

No new writes, no shell, no destructive S3 op. The fix only escapes rendered output — it doesn't change what data reaches the report (redaction still runs). No behavior change for legitimate content.

🤖 Generated with Claude Code

https://claude.ai/code/session_01M3YojFPzjkfMj6YYSwvzdx


Generated by Claude Code

…DB upgrade regression test

Report tables interpolated object keys and user-agents (attacker-influenceable)
straight between `|` delimiters with no escaping: a `|` misaligned columns, a
newline split a row and corrupted the rest of the table, and a key like
`<img src=x onerror=...>` landed as stored HTML in the saved .md (credential
redaction doesn't cover markdown metacharacters). All four report types now
escape every cell (| CR LF ` < >) at the render boundary.

Also added a regression test proving an old-schema DB upgrades cleanly — rows in
the rebuilt tables (tool_calls@M002, datasets@M004) survive with data intact —
a path that was sound but had no coverage.

Found by a report-generation audit. Two sibling mining agents (skills-content
drift, migration deep-dive) were cut off by a session limit and will be re-run.

Tests: +3 (test_v0350_fixes.py). Full sidecar suite 596 passed; ruff clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3YojFPzjkfMj6YYSwvzdx

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 240a77b1ac

ℹ️ 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 +39 to +40
return (str(v).replace("\\", "\\\\").replace("|", "\\|")
.replace("\r", " ").replace("\n", " ").replace("`", "\\`")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Teach the report parser to honor escaped pipes

When a key or user-agent contains |—the exact input this change targets—_cell emits \|, but the built-in report viewer's cells() in frontend/src/components/Markdown.tsx line 176 unconditionally calls .split("|") without recognizing escapes. Consequently, a row such as | a\|b | 5 GB | is still parsed as three cells in the report overlay and RunDetail, shifting columns and corrupting the displayed results despite the new backend test passing; update that parser or encode pipes in a form it does not treat as a delimiter.

Useful? React with 👍 / 👎.

@hxddh
hxddh merged commit 47cae38 into main Jul 17, 2026
6 checks passed
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.

2 participants