Skip to content

fix(cli): stop boss mcp ledger printing forged rows and terminal escapes - #995

Closed
Antriksh1984 wants to merge 1 commit into
risa-labs-inc:devfrom
Antriksh1984:fix/mcp-ledger-terminal-injection
Closed

Antriksh1984 wants to merge 1 commit into
risa-labs-inc:devfrom
Antriksh1984:fix/mcp-ledger-terminal-injection

Conversation

@Antriksh1984

Copy link
Copy Markdown
Contributor

Summary

boss mcp ledger tail|search|verify is how an operator reads the MCP audit trail. Its human output printed strings that came out of the ledger file raw:

  • a tool's errorSnippet (McpArgumentSanitizer.sanitizeMessage only redacts credentials, so control characters reach the file untouched),
  • a tool's toolName,
  • and, in a verify break report, the recordId, both hashes and the file names of a record that was just found to be altered.

Two consequences, both in the one view an operator trusts:

  1. Forged audit rows. A newline in an error snippet starts a second line that is formatted exactly like a ledger record (2099-01-01T00:00:00Z approved_tool ALLOW/AUTO_ALLOWED ... ok hash ...) but was never written by the ledger.
  2. Terminal control. An ESC sequence acts on the terminal itself: clearing the screen over what the operator was reading, retitling the window, or writing the clipboard on terminals that honour OSC 52. A tampered ledger controls every field verify prints for the record it reports as altered, so the report that says "this file was tampered with" was itself attacker-writable.

JSON output is unaffected: its serializer already escapes control characters.

Fix

A new shared TerminalText (safe, safeLines, safeIndented) writes anything that could act on a terminal or on the reader out as a visible escape:

  • C0 and C1 controls, DEL, and line breaks and tabs (as \n, \r, \t, or \uXXXX),
  • bidi embeddings, overrides and isolates (which can make one tool name read as another), zero-width characters, and the line and paragraph separators.

It escapes rather than drops, so the operator can see that something was there. Ordinary text, including accents, CJK and emoji, is returned unchanged. McpLedgerFormat applies it to every ledger-derived string in the text renderings (records, break report, file names, and the read-error messages, which can quote ledger content).

The helper is shared on purpose: the same class of problem exists in the other human renderings (boss mcp list prints plugin-supplied tool names and descriptions), and I will follow up on those separately rather than widen this diff.

Tests

Test-first: the three new ledger tests fail on unfixed dev (run before the change) and pass after.

  • tail cannot be made to print a forged record row or a terminal escape sequence - an error snippet carrying a newline, a forged row, ESC[2J and an OSC title; asserts no control characters, no line starting with the forged timestamp, the escape stays visible, and one record still yields exactly one error line.
  • tail neutralises control and direction-override characters in a tool name
  • verify neutralises control characters read back from a tampered ledger - alters a record id in the file and checks the break report.
  • TerminalTextTest pins the helper, including that ordinary text is untouched and that a multi-line value cannot start a line that reads as a heading.

McpLedgerCliTest 18/18, TerminalTextTest 4/4, BossMcpCliTest 23/23. ktlintCheck and detekt clean.

boss mcp ledger tail/search/verify printed record fields (tool name,
provider id, arguments, error snippets) raw. A record can carry a
terminal escape sequence or a newline chosen by whoever supplied that
field, so the CLI's own output could move the cursor, hide text or
forge a line that looks like a different record.

TerminalText.safe renders every non-printing character as a visible
escape (\n, \t, \uXXXX) rather than passing it through, and
McpLedgerCli routes every field through it before printing.
@Antriksh1984
Antriksh1984 force-pushed the fix/mcp-ledger-terminal-injection branch from 2536f24 to d15c9fa Compare September 22, 2026 09:59
@Antriksh1984

Copy link
Copy Markdown
Contributor Author

Rebased onto current dev as one clean commit (was carrying stale duplicate history that made the diff unreviewable and the merge status CONFLICTING, the same issue found on #1406/#1407/#1417/#1420). Content unchanged. Tests, ktlintCheck and detekt pass on the rebased head.

@fluck-boss

Copy link
Copy Markdown
Collaborator

Thanks so much for this contribution, @Antriksh1984 - the work here is genuinely appreciated, and it's clear real care went into it.

This overlaps with #1001 ("fix(cli): neutralise plugin manifest text in boss plugin validate and link"), which is further along in review and will carry this change forward. To keep review attention focused, I'm closing this one in favor of #1001 - please do review and follow that PR, and you're very welcome to pick up any part of it that still needs help or take on another open issue.

Your contribution credit is noted here, and anything unique in this branch that #1001 misses is still welcome as a follow-up. Thanks again for contributing to BossConsole!

@fluck-boss fluck-boss closed this Sep 23, 2026
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