[Antriksh1984]

Stacked on #996 (which is stacked on #995; it introduces TerminalText). Until those land the diff also shows their commits; this PR is the last two.

Summary

Third and last of the TerminalText sweep (#995 the MCP ledger, #996 status / doctor / mcp list). This one is boss plugin, and it is the sharpest of the three, because validating a plugin you did not write is the whole point of boss plugin validate, and its output quotes that plugin's own files:

• A malformed plugin.json is reported as Malformed plugin.json: <exception message>, and kotlinx's exception messages quote the offending JSON.
• Other checks quote manifest fields and archive entry names.
• boss plugin link prints the plugin id from the manifest. reloadDevPlugin refuses an id containing control characters, and its refusal message quotes the id it just refused - so the one input that trips the guard is the one printed raw.

Printed raw, a hostile manifest writes escape sequences to the developer's terminal (clear the screen over the [x] lines, retitle the window, OSC 52 clipboard write) and can start lines of its own that read as passing checks.

Fix

Every string that comes from the plugin under inspection goes through TerminalText: a check's name and message, error messages, the plugin id in the "staged" messages, and the reload result. The check and error lines move into two small internal formatters (formatCheckLine, formatErrorLine) so they are testable; that extraction is its own behavior-neutral commit ahead of the fix. JSON output is unchanged.

Tests

Test-first: 3 of the 4 new PluginCommandTerminalSafetyTest cases fail on the unfixed formatters and pass after; the fourth pins that a null message still reads Error: null.

• a check line neutralises its name and message and stays one line,
• an error line cannot forge a [ok] Validation passed line,
• end to end: a plugin directory with a hostile plugin.json is run through the real PluginValidator, and every printed check is exactly one line with no control characters.

All of ai.rever.boss.cli.* passes; ktlintCheck and detekt clean. Control characters are built from code points in the test source, not embedded in it.

[Antriksh1984]

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]

@claude review this PR - security-sensitive changes, part of the backlog sweep. Focus on auth, signing, path handling, secrets, and injection surfaces.

[claude]

Claude finished @fluck-boss's task in 3m 41s —— View job

---

Security review - PR #1001 (TerminalText sweep, part 3)

• Read TerminalText and its test
• Read HumanReports formatters
• Read BossPluginCommand / BossDoctorCommand / McpLedgerCli call sites
• Audit for unsanitized sinks left behind (traced reachability through PluginValidator and SingleInstanceManager)
• Post review

Reviewed as a security change: the threat model (a developer runs boss plugin validate/link on a repo they did not write, and the command quotes that repo's manifest and filenames back at their terminal) is correct and under-appreciated, and the fix is the right shape - escape rather than drop, human output only, JSON untouched. The test that drives the real PluginValidator over a hostile plugin.json and asserts one-line-per-check is the part that makes this hold up; a formatter-only test would have passed against a sink that never called the formatter.

One real gap, in the same function the PR fixes.

---

1. boss plugin link still prints an attacker-chosen filename raw - Medium

composeApp/src/desktopMain/kotlin/ai/rever/boss/cli/BossPluginCommand.kt:214-224

val errMsg = "Cannot link invalid plugin at ${targetJarFile.absolutePath}"
...
echo("$errMsg:", err = true)          // <- no TerminalText
validation.checks.filter { !it.passed }.forEach {
    echo(formatCheckLine("  [✗]", it.name, it.message), err = true)   // <- fixed
}

targetJarFile is not the operator's argument. PluginValidator.resolveStagingTarget (PluginValidator.kt:68-100) lists <plugin>/build/libs and picks a *.jar out of it, so the filename comes from the plugin repo that was cloned. ESC, CR and LF are all legal in a POSIX filename, so build/libs/x<ESC>[2J.jar is enough - and this is the invalid-plugin branch, i.e. precisely the branch a hostile plugin lands on, printed immediately above the [✗] lines the PR just hardened. Every other error path in this same run() goes through formatErrorLine; this one was missed.

echo(formatErrorLine(errMsg), err = true)
// or, to keep the trailing colon: echo(TerminalText.safe("$errMsg:"), err = true)

The --json branch above it is fine - the serializer escapes. Fix this →

2. boss plugin validate has one raw line left - Low

BossPluginCommand.kt:138: echo("Validating plugin at ${target.absolutePath}..."). This one is the operator's own argument (or cwd), so it is a much weaker case than #1 - but it is the only unneutralised line in a command whose whole output is otherwise neutralised, and a tab-completed path inside a cloned repo is not impossible. Cheap to wrap.

3. TerminalText iterates UTF-16 chars, so supplementary format characters pass - Low

TerminalText.kt:52,65-72. for (ch in text) plus a Char-keyed needsEscaping cannot see the astral Cf blocks - the tag characters U+E0000..U+E007F and the musical format controls U+1D173..U+1D17A, both of which render invisibly and are the standard text-hiding trick. They cannot drive the terminal (no ESC), so the severity is "text the operator cannot see", not "terminal control".

Worth closing mainly for consistency: this branch already carries c77599c fix(cli): reject supplementary format controls for the deep-link filter, so the two guards in the same repo currently disagree about what a hidden character is. Iterating code points and asking Character.getType(cp) == Character.FORMAT.toInt() would subsume most of the hand-listed ranges as well.

4. boss mcp invoke prints plugin tool output raw - deliberate? Please say so

BossCommand.kt:439,447 - echo(content), where content is whatever the plugin's MCP tool returned. That is the largest remaining sink of plugin-controlled text in the CLI, and it is the one place I'd guess escaping is wrong: boss mcp invoke ... | jq needs byte fidelity, and --raw exists for that. If that is the reasoning, a sentence in TerminalText's KDoc ("tool payloads on stdout are out of scope: they are a pipeline, not a report") would stop the next sweep either breaking pipes or leaving the reader thinking it was missed. A System.console() != null gate is the other option, but it makes output non-reproducible, so I'd prefer the documented exclusion.

5. BossDoctorCommand.kt:33 - Low

fail("Error: ${it.message}") prints an exception message raw while the rest of that file carefully sanitizes. Same one-line fix; belongs to #996's surface rather than this PR, so noted only.

---

Things I checked that are fine

• Path injection via the manifest id is bounded, but not by this file. pluginId is interpolated into File(DevPluginArtifacts.stagingRoot(), pluginId) and "$pluginId.jar" (:250,268-269) and the resulting stagedJar.absolutePath is printed raw at :324,343,362 right beside a TerminalText.safe(pluginId). That is safe today, twice over: ID_REGEX (^[a-zA-Z][a-zA-Z0-9_-]*(?:\.[a-zA-Z0-9_-]+)+$) is a check, and isValid = checks.all { it.passed }, so link bails at :213 before the staging path is built; and validPluginDevId independently rejects controls, whitespace, /, \, . and ... But the mixed escaping in one string reads as "the path was considered and is trusted" when really it is trusted by a regex two files away - one comment at :250 naming ID_REGEX would keep that from decaying.
• The .jar.part + ATOMIC_MOVE staging, the AtomicMoveNotSupportedException fallback, and the JSON payloads are untouched by this PR.
• No auth, signing, secret or trust decision changes. link still stages under DevPluginArtifacts.stagingRoot() and still signals over the token-authenticated single-instance channel; validate remains advisory and does not check signatures, unchanged. formatErrorLine(null) → "Error: null" is pinned.
• HumanReports: the safeIndented treatment of descriptions closes the "second line reads as another tool" forgery, the parse-failure fallbacks (:48,109) are sanitized too, and the compact-JSON inputSchema note at :141-143 is right - toString() has no raw newlines but leaves DEL/C1/bidi alone.
• safeLines normalises \r\n first and then escapes a lone \r, so a bare CR cannot return the cursor and overwrite the line above. That is the detail this kind of code usually gets wrong.

I did not run ./gradlew test here (full desktop test compile is beyond this job's budget); the author reports ai.rever.boss.cli.*, ktlintCheck and detekt green, and CI on the PR is the check for that.

Verdict: approve in spirit once #1 is addressed - it is the same bug class in the same function, so landing without it leaves the sweep's headline command exploitable by the exact input the PR describes. #2-#5 are follow-up sized.
• fix/cli-plugin-output-terminal-safety

[fluck-boss]

"Verified finding 1 against the real code - CONFIRMED and fixed (57ea2b3). BossPluginCommand.kt:220 printed the invalid-plugin header raw, and the filename is discovered inside the cloned plugin repo, so ESC/CR/LF in it are attacker-chosen - on exactly the branch a hostile plugin lands. It now goes through formatErrorLine (TerminalText.safe) like every other error path, with a test pinning the neutralisation. #2 (the validate commands own-argument line) is a fair cheap wrap but weaker - left for the contributor. #3 (astral Cf in TerminalText) is a real text-hiding gap, no terminal control - follow-up sized."

[fluck-boss]

@claude please review this exact head (57ea2b3) for merge into dev. Check that the change is correct and complete for its stated goal, that tests cover the risk being fixed (a test that passes without the fix is a finding), and flag any security issues (secrets, injection, auth or approval bypass, path handling). Keep the review focused on this diff.

[claude]

Claude finished @fluck-boss's task in 5m 19s —— View job

---

Review of 57ea2b3 for merge into dev

• Read the diff (TerminalText, HumanReports, BossPluginCommand, BossDoctorCommand, McpLedgerCli)
• Check correctness/completeness against the stated goal
• Verify each new test fails without the fix
• Audit for remaining unsanitized sinks / injection / path handling
• Post review

The threat model is right and the shape of the fix is right: escape rather than drop, human renderings only, JSON untouched. I confirmed the premise behind the head commit independently - PluginValidator.resolveStagingTarget (PluginValidator.kt:64-103) lists build/libs inside the cloned repo and returns a discovered filename, and validateDirectory/parseManifest (:496) put kotlinx's message - which quotes the offending JSON - into a printed check. Both are attacker-chosen.

Two findings, and the first one means the head commit does not actually achieve its stated goal.

---

1. The escaped echo is defeated by the raw logger.error directly above it - Medium

composeApp/src/desktopMain/kotlin/ai/rever/boss/cli/BossPluginCommand.kt:214-223

val errMsg = "Cannot link invalid plugin at ${targetJarFile.absolutePath}"
logger.error(LogCategory.SYSTEM, errMsg)        // <- raw, and this goes to the terminal
...
echo(formatErrorLine("$errMsg:"), err = true)   // <- the fix

In headless CLI mode the logger is the operator's terminal:

• CliBootstrap.isHeadlessCli (:42-46) includes "plugin", so boss plugin link takes the headless path.
• dispatchHeadless (:67) calls configureHeadlessLogging(), which is System.setProperty("org.slf4j.simpleLogger.logFile", "System.err") (BossCommand.kt:562-565).
• simplelogger.properties:5 sets defaultLogLevel=info, so ERROR is emitted.
• plugin-logging/.../BossLogger.kt:463 is slf4jLogger.error(formattedMessage, entry.error) - the message verbatim.

So against a hostile plugin the operator gets, in order:

… ERROR … Cannot link invalid plugin at /tmp/x<ESC>[2J.jar      <- screen cleared here
Error: Cannot link invalid plugin at /tmp/x[2J.jar:

The ESC reaches the terminal one line before the line the commit hardened, on exactly the branch the commit message names. Two more in the same run(): :236-237 builds "Failed to extract plugin manifest from JAR: ${e.message}" and logs it with error = e (message and stack trace both), and :372-373 logs msg embedding pluginId and reloadResult.reason raw before the echo escapes it.

Cheapest correct shape is to log the neutralised string - the log file is also read in a terminal, and LogSanitizer is already the house pattern for "do not put raw foreign text in a log line":

val errMsg = "Cannot link invalid plugin at ${TerminalText.safe(targetJarFile.absolutePath)}"

which then feeds both the log and the echo. Fix this →

2. The head commit's test passes without the head commit's fix - Medium (coverage)

You asked specifically for this. PluginCommandTerminalSafetyTest:

fun `the invalid-plugin header neutralises a hostile discovered filename`() {
    val line = formatErrorLine("Cannot link invalid plugin at $hostile:")

formatErrorLine was introduced in the parent commit e5ab2b7 with exactly this behaviour; what 57ea2b3 changed is that BossPluginCommand.kt:223 now calls it instead of interpolating raw. The test never references BossPluginLinkCommand, so it passes verbatim on e5ab2b7. It is a guard for code that was already correct, not for the defect that was fixed.

The same structural gap applies to the case the PR description calls "end to end":

val result = PluginValidator.validate(dir)
val printed = result.checks.joinToString("\n") { formatCheckLine("[x]", it.name, it.message) }

That drives the real validator (good - it proves the hostile manifest really does reach the check messages) but the test applies the formatter. Revert :141, :151, :223 and :225 to raw interpolation and the whole file stays green. Every sink this PR fixes is a call site, and no test covers a call site.

Clikt 5.1.0 has com.github.ajalt.clikt.testing.test, and the KMP test compilation is associated with desktopMain, so internal declarations and clikt itself are already visible from desktopTest - one BossPluginValidateCommand().test(arrayOf(dir.absolutePath)) asserting no control characters in stderr + stdout would cover both call sites and finding 1's log line at the same time. Fix this →

---

Smaller

3. BossPluginCommand.kt:138 is still raw - Low. echo("Validating plugin at ${target.absolutePath}..."). Raised last round and left; worth noting it is printed immediately before the [✓]/[✗] lines, so it is the one line positioned to clear or scroll away the checks it introduces. Same commit-message rationale ("ESC/CR/LF are legal in POSIX names") applies to a tab-completed directory name inside a cloned repo, just with the operator in the loop.

4. CliBootstrap.kt:74 - Low. System.err.println("Error: ${e.message ?: …}") is the top-level catch for every headless command, printing an exception message raw. Not in this diff, but it is the last uncovered sink of the sweep.

5. McpLedgerCli.kt:335 - nit. record.hash?.take(12)?.let(::terminalSafe) truncates then escapes, while toolName two lines up escapes then pads. Hashes are hex so nothing breaks today; escape-then-take would make the two agree.

6. TerminalText astral Cf gap - Low, unchanged. for (ch in text) over UTF-16 with a Char-keyed predicate still misses U+E0000..U+E007F and U+1D173..U+1D17A. No terminal control, only hidden text - but c77599c on this same branch rejects exactly those for deep links, so the repo has two disagreeing definitions of "hidden character". Follow-up sized, as before.

7. Adjacent, out of diff. PluginValidator.validateDirectory:163 reads the manifest with an unbounded manifestFile.readText(), while the archive path uses DevPluginArtifacts.readBoundedUtf8String with MAX_MANIFEST_BYTES (:281-292). The directory path is the one this PR's new test drives. A hostile plugin.json of arbitrary size OOMs boss plugin validate <dir>. Worth an issue rather than a change here.

Checked and fine

• Path handling is sound and I verified the chain rather than assuming it. stagedJar.absolutePath is printed raw at :327/346/365 beside a TerminalText.safe(pluginId), which looks inconsistent but is correct: PluginValidator.validate sets isValid = checks.all { it.passed } and id-format (:507) applies ID_REGEX = ^[a-zA-Z][a-zA-Z0-9_-]*(?:\.[a-zA-Z0-9_-]+)+$, and link bails at :213 before the staging path is ever built. No traversal, no control characters, and the staging root and v<timestamp> are host-generated. A one-line comment at :253 naming ID_REGEX would stop that decaying.
• BossPluginInitCommand:84 prints result.pluginId raw, and that is safe: PluginScaffolder.derivePluginId reduces to [a-z0-9-].
• The HumanReports half is properly regression-guarded, and that is the difference from finding 2: those formatters were extracted out of BossCommand's private methods and are the whole render path, so HumanReportsTerminalSafetyTest fails against the pre-fix bodies. The • -count and single-Access:/Health:-line assertions are the right shape for forgery.
• safeLines normalises \r\n before splitting, and U+0085, U+2028, U+2029 are all inside the escaped ranges, so split('\n') cannot miss a break the terminal would honour. The double escaping where healthSummaryOf (already escaped via areaNames) is re-safed in formatHumanStatus:46 is idempotent.
• policyOf is enum-derived, so the ledger's padEnd widths cannot be driven by foreign text; the verificationJson/recordsJson paths are untouched and the serializer escapes.
• No auth, signing, secret, trust or approval-path change. link still stages under DevPluginArtifacts.stagingRoot() behind the token-authenticated single-instance channel; validate stays advisory. formatErrorLine(null) → "Error: null" is pinned.
• Behaviour change worth a line in the PR body: the invalid-plugin header now gains an Error: prefix it did not have. Cosmetic, and --json is the machine contract.

I did not run ./gradlew test (full desktop test compile is beyond this job's budget) - CI on the PR is the check for the author's reported green run.

Verdict: the one-line change in 57ea2b3 is correct, but it does not close the hole it names - finding 1 leaves the same ESC reaching the same terminal from the log line above it, and finding 2 means nothing would catch either the original defect or its return. I'd hold the merge for those two; 3-7 are follow-ups.
• fix/cli-plugin-output-terminal-safety

[fluck-boss]

Thanks @Antriksh1984 - neutralising the plugin manifest text is the right fix. Two points from Claude's review of 57ea2b3 that I checked:

1. The escaped echo is undercut by a raw logger.error just above it that still logs the attacker-chosen text. Log the neutralised string (or run it through LogSanitizer) - the log is read in terminals too.
2. The head commit's test passes without the head commit's fix: formatErrorLine already behaved this way in the parent commit, and the test never goes through BossPluginCommand. Drive the test through the command path at BossPluginCommand.kt:223 so it fails if that call reverts to raw interpolation.

Changes requested + draft until then. Push, get the ubuntu and windows legs green, mark it ready for review, and it goes straight back into the merge queue.