feat(powershell-format): disclose formatter rewrites on the user channel

[kyle-sexton]

No linked issue

Partially addresses #1596

Related

Refs #1596

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ad88dc7534

ℹ️ 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".

[claude]

Claude finished @kyle-sexton's task in 2m 39s —— View job

---

Security Review

• Gather context (diff, changed files)
• Review powershell-format.sh changes for injection / trust-boundary issues
• Review plugin.json / CHANGELOG.md changes
• Post findings

Reviewed the 3 changed files (plugin.json version bump, CHANGELOG.md, and the new _ps_before/maybe_disclose_ps_rewrite snapshot-and-disclose logic in powershell-format.sh). Scope: security only (injection, authz, credential/token handling, trust-boundary logic, GitHub Actions risk) — no workflow files are touched by this PR, so the Actions-specific checks don't apply here.

Finding: Full-file snapshot in system temp isn't cleaned up on the trust-gate and tool-break exit paths

File: plugins/powershell-format/hooks/powershell-format.sh

The new snapshot logic copies the entire content of the edited file into a mktemp file before formatting runs:

_ps_before=""
if _ps_before=$(mktemp 2>/dev/null); then
  cp "$FILE" "$_ps_before" 2>/dev/null || _ps_before=""
fi

maybe_disclose_ps_rewrite (the only code path that rm -fs it after a successful comparison) is called only from the exit-0 and exit-1 branches (L634, L657). Two other paths reach exit 0 via emit_skipped (which calls exit 0 directly, L122-L125) without ever touching $_ps_before:

• Trust-gate skip (exit 6) — L674-L733: no rm -f "$_ps_before" anywhere in this arm.
• Tool-break / default (*, covers exit 4 and any unrecognized exit) — L734-L748: same, no cleanup.

By contrast the exit-3 and exit-5 arms were given an explicit rm -f "$_ps_before" (L664, L671), so the omission on the trust-gate and tool-break arms looks like an oversight rather than an intentional choice.

Impact: every edit that hits the trust gate (a state this hook is specifically designed to reach — repos with CustomRulePath-bearing settings that aren't yet approved) or an Invoke-Formatter/Invoke-ScriptAnalyzer throw leaves a full, unmanaged copy of the source file's content behind in the system temp directory, indefinitely. mktemp creates the file 0600, so it isn't readable by other OS users by default, but it is readable by any other process running as the same local user — a real adjacent-threat consideration in an agentic coding environment where MCP servers, plugin hooks, and third-party package scripts routinely execute with that same identity. Because the trust-gate path is exercised precisely when a repo's settings are untrusted/unapproved, this quietly grows a pile of un-cleaned, undiscoverable snapshots of exactly the files a user is actively editing (which may contain secrets, connection strings, or other sensitive content never intended to leave the working tree), with no TTL or bound on accumulation.

Severity: IMPORTANT
Confidence: Medium (the code-path gap is confirmed by reading; real-world exploitability depends on another same-user process being present to read /tmp, which is a plausible but not certain threat model here)

Suggested fix: install a single trap 'rm -f "$_ps_before"' EXIT right after the snapshot is taken (around L208-L211) instead of relying on each case arm to remember cleanup — that also removes the need for the per-arm rm -f calls on the exit-3/5 paths. Fix this →

---

No injection, authorization, credential-handling, or GitHub Actions concerns found in this diff — plugin.json/CHANGELOG.md changes are non-functional (version bump and docs), and the new shell code doesn't introduce any command/path injection (all interpolation stays inside quoted arguments, and hook::json_escape is used correctly for the systemMessage text).

Note: Codex's review already flagged a separate, non-security correctness issue on this PR (composing the rewrite notice with lint context can emit two JSON documents on stdout in the exit-1 path) — that's a hook-protocol/output-format bug, not a security vulnerability, so it's out of scope for this review and I'm deferring to that comment rather than duplicating it.

[claude]

Claude finished @kyle-sexton's task in 1m 49s —— View job

---

Security Review

• Gather context (diff, changed files)
• Review powershell-format.sh changes for injection / trust-boundary / info-disclosure issues
• Review plugin.json / CHANGELOG.md changes
• Post findings

Reviewed the 3 changed files (plugin.json version bump, CHANGELOG.md, and the new _ps_before / maybe_disclose_ps_rewrite snapshot-and-disclose logic in powershell-format.sh). Scope: security only. No GitHub Actions workflows are touched by this PR, so the pinning/permissions/trigger checks that zizmor already covers statically don't apply here.

---

Finding: Full-file snapshot in system temp isn't cleaned up on the trust-gate and tool-break exit paths

File: plugins/powershell-format/hooks/powershell-format.sh#L208-L220

The new pre-format snapshot copies the entire content of the edited file into a mktemp file:

_ps_before=""
if _ps_before=$(mktemp 2>/dev/null); then
  cp "$FILE" "$_ps_before" 2>/dev/null || _ps_before=""
fi

maybe_disclose_ps_rewrite — the only routine that rm -fs this snapshot after use — is invoked solely from the exit‑0 (L634) and exit‑1 (L657) branches. Two other termination paths never touch $_ps_before:

• Trust-gate skip, exit 6 — L674-L733: falls through to emit_skipped with no rm -f "$_ps_before" anywhere in the arm.
• Tool-break / default * arm (covers exit 4 and any unrecognized exit) — L734-L748: same gap.

By contrast, the exit‑3 and exit‑5 arms were explicitly given rm -f "$_ps_before" (L664, L671), so omitting it on the trust-gate and tool-break arms reads as an oversight rather than a deliberate choice.

There's a second, narrower leak in the same block: if mktemp succeeds but cp fails (disk pressure, permission error), _ps_before is reset to "" on L210 — discarding the only reference to the file mktemp just created, so that empty temp file is orphaned too (low impact, since it's empty, but still unbounded accumulation).

Impact: the trust gate (exit 6) is precisely the state this hook is designed to reach for repos with unapproved CustomRulePath settings — i.e., repos whose content the hook explicitly does not trust yet. Every edit that lands there (or hits an Invoke-Formatter/Invoke-ScriptAnalyzer throw) leaves a full, 0600-permissioned copy of the source file's content behind in the shared system temp directory, indefinitely, with no TTL or accumulation bound. mktemp's 0600 mode blocks other OS users, but not other processes running as the same local user — a realistic adjacent-threat model in an agentic coding environment where MCP servers, plugin hooks, and third-party package/build scripts routinely execute under that same identity. Files a developer is actively editing can carry secrets, connection strings, or other sensitive content never intended to leave the working tree, and this path silently mirrors them into /tmp.

Severity: IMPORTANT
Confidence: Medium (the code-path gap is confirmed by static reading; real-world exploitability depends on a co-resident same-user process reading /tmp, which is plausible but not guaranteed in every deployment)

Suggested fix: install a single trap 'rm -f "$_ps_before"' EXIT immediately after the snapshot is taken (right after L211), and retain the mktemp path across a failed cp instead of discarding it, so cleanup is unconditional regardless of which exit path is taken — this also lets you drop the now-redundant per-arm rm -f calls on the exit-3/5 paths. Fix this →

---

No injection, authorization, credential-handling, or GitHub Actions concerns found otherwise: plugin.json/CHANGELOG.md changes are non-functional (version bump and docs), and the new shell code keeps all interpolation inside quoted arguments with hook::json_escape correctly applied to the systemMessage text.

Note: Codex already flagged a separate issue on this PR — composing the rewrite notice with lint context can print two JSON documents on stdout in the exit-1 path (hook::ctx_flush at L650 followed by maybe_disclose_ps_rewrite's own hook::emit_channels call at L657). That's a hook-protocol/output-format correctness bug, not a security vulnerability (no injection or trust-boundary implication — worst case is malformed hook output), so it's out of scope for this security-only review and I'm deferring to that comment rather than duplicating it.

[claude]

Claude finished @kyle-sexton's task in 3m 8s —— View job

---

Code Review

• Gather context (diff, changed files, CLAUDE.md / REVIEW.md)
• Review powershell-format.sh changes for correctness, error handling, maintainability
• Review plugin.json / CHANGELOG.md changes
• Post findings

Reviewed the 3 changed files against this repo's REVIEW.md code-review-lane scope (correctness, design, conventions, error handling, observability, tests, docs). This repo has a claude-security-review lane, so the temp-file cleanup gap on the exit-6/tool-break paths (already flagged by both the security-review bot and Codex's P2 comment) is that lane's finding, not this one's — omitted here per the lane split.

Finding posted inline (powershell-format.sh:657)

Confirmed, independently of Codex's P1 flag on the same line: when Invoke-Formatter rewrites the file and Invoke-ScriptAnalyzer still reports findings, hook::ctx_flush (line 650) prints one JSON document via hook::emit_additional_context, and the new maybe_disclose_ps_rewrite → hook::emit_system_message call then prints a second, independent JSON document. Claude Code parses a hook's whole stdout as one JSON document, so this combination is invalid output.

I traced this against the sibling markdown-format hook, which already solves the identical problem: it accumulates CTX/SYSMSG and composes them into one hook::emit_channels PostToolUse "$CTX" "$SYSMSG" call at the end, with an explicit comment explaining why ("Compose ONE stdout document..." — markdown-format.sh:1099-1112). This PR should follow that established convention rather than introducing a second, independent emit call.

Test-coverage gap (no line in this diff to anchor it to — no test file was touched)

powershell-format.test.sh was not updated by this PR, so the new disclosure feature — the entire point of the change — has zero test assertions:

• No test checks that a systemMessage is actually emitted when the formatter rewrites a file (neither the exit-0 clean-rewrite path nor the exit-1 rewrite+findings path).
• The existing fixture (SETTINGS_BODY at L45-L51) enables both PSUseCorrectCasing (formatter-fixable) and PSAvoidGlobalVars (semantic finding), but Case 3 (L267-L279) and Case 4 (L281-L298) exercise them in separate files, never together in one file. A test combining a casing fix with a surviving lint finding in a single file would have caught the double-JSON-document bug above directly.

Suggest adding: (1) a case asserting systemMessage content on a clean rewrite, and (2) a case with both a formatter-fixable rule and a surviving finding in the same file, asserting the output parses as a single valid JSON document.

Not flagged

• plugin.json version bump (0.7.7 → 0.7.8) and CHANGELOG.md entry are consistent with each other and with the change.
• No security, authz, or injection concerns reviewed here — that's the security lane's scope per REVIEW.md, and it already posted its findings on this PR.

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @cursor[bot]'s task in 2m 20s —— View job

---

CI Security Review

• Gather context (diff, changed files, prior review history)
• Check skip gate (already-reviewed head, closed PR, no security surface)
• Review changed files for security issues
• Post findings

Reviewed the 3 changed files (plugin.json version bump, CHANGELOG.md, and the new _ps_before / maybe_disclose_ps_rewrite snapshot-and-disclose logic in powershell-format.sh) at HEAD 7a856c9. Scope: security only. No GitHub Actions workflows are touched, so the pinning/permissions/trigger checks zizmor already covers statically don't apply.

Skip-gate note: this PR's only content commit is 1c800cf — already reviewed twice by this lane on earlier base SHAs (e8c4c27d, 89c4c907). The current HEAD commit 7a856c9 (chore: retrigger CI after main pin merge) is an empty commit with zero file changes, confirmed via git show 7a856c9 --numstat. So the prior finding still applies unchanged, and I re-posted it anchored to the current head/permalinks rather than re-deriving it from scratch.

Finding (posted inline at powershell-format.sh:210): full-file snapshot in system temp isn't cleaned up on the trust-gate and tool-break exit paths

The _ps_before snapshot (L208-211) copies the entire edited file into system temp. Cleanup only happens via maybe_disclose_ps_rewrite, called from the exit-0 (L635) and exit-1 (L658) branches, or via explicit rm -f on the exit-3/5 arms (L665, L672). The trust-gate arm (exit 6, L675-734) and the default/tool-break arm (*, covers exit 4, L735-749) never remove it — and the trust gate is precisely the state this hook is designed to reach for repos with unapproved CustomRulePath settings, i.e. content it explicitly does not yet trust. Every such edit leaves a full, 0600-permissioned copy of the source file's content behind in the shared system temp directory indefinitely, with no TTL or bound — a real information-disclosure concern in an agentic environment where other same-user processes (MCP servers, other hooks, package scripts) can read /tmp. There's also a narrower orphan case: if mktemp succeeds but cp fails, _ps_before is reset to empty on L210, discarding the only reference to the file mktemp just created.

Severity: IMPORTANT · Confidence: Medium (path gap confirmed by reading; exploitability depends on a co-resident same-user process reading /tmp). Full detail and suggested fix (a single trap 'rm -f "$_ps_before"' EXIT right after the snapshot) posted as an inline comment.

Not re-flagged (out of scope for this lane): Codex's P1 comment on the exit-1 path printing two JSON documents on stdout when a rewrite and surviving lint findings coincide is a hook-protocol/output-format correctness bug, not a security vulnerability (no injection or trust-boundary implication) — deferring to the code-review lane, consistent with this lane's prior passes on this PR.

No injection, authorization, credential-handling, or other GitHub Actions concerns found: plugin.json/CHANGELOG.md changes are non-functional (version bump and docs), and all shell interpolation stays inside quoted arguments with hook::json_escape correctly applied to the systemMessage text.
· branch: cursor/powershell-format-mutation-disclosure-63e1

[github-actions]

Last security-reviewed head: b08c0f6ed178c9d64a214f676b21b984c646a2c5. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[claude]

Claude finished @cursor[bot]'s task in 1m 9s —— View job

---

CI Security Review

• Gather context (diff since last-reviewed head, changed files, prior review history)
• Check skip gate
• Post findings

Skip-gate result: no new security-relevant surface.

The only commit added since the last security review (at HEAD 7a856c923781fa85b26e885d68a4e07b17593db0, an empty CI-retrigger) is b08c0f6 — test(powershell-format): expect mutation disclosure when formatter rewrites. It touches only plugins/powershell-format/hooks/powershell-format.test.sh, updating test assertions (adding a has_format_disclosure helper and switching several gate-path checks to expect a systemMessage disclosure instead of empty output) to match the hook behavior this PR already introduced. powershell-format.sh itself, plugin.json, and CHANGELOG.md are byte-identical to the previously-reviewed state — confirmed via git diff against the last-reviewed head, no changes to the hook logic. Test-only assertion changes carry no injection, authz, credential-handling, or trust-boundary surface, so per this lane's skip gate (no security-relevant surface in the new content) I'm not re-deriving a full review.

Prior finding status: the previously-reported IMPORTANT finding — the _ps_before mktemp snapshot (powershell-format.sh#L208-L211) not being cleaned up on the trust-gate (exit 6) and tool-break/default (*, exit 4) arms, leaking a full copy of the edited file's content into system temp — is still present, unchanged, since the underlying hook file has not been modified since that finding was posted (twice as a PR comment, most recently on commit 7a856c9). Not re-posting the full writeup again here to avoid duplicate noise; see the earlier comments on this PR for the complete finding and suggested fix (trap 'rm -f "$_ps_before"' EXIT right after the snapshot).

No new injection, authorization, credential-handling, or GitHub Actions concerns found in this commit.
· branch: cursor/powershell-format-mutation-disclosure-63e1

[claude]

Claude finished @cursor[bot]'s task in 31s —— View job

---

Code Review (/review:code-review)

• Gather context (PR state, diff, changed files)
• Apply skip gate
• Post result

Skip gate hit: PR is not open.

This PR's state is MERGED (confirmed from the pre-fetched context), which matches skip-gate condition 1 in the code-review lane skill ("PR is closed or not open"). Per that skill, deep review is skipped and nothing else is posted — no inline comments were added.

For reference, this PR was already reviewed by this lane and the security lane while it was open:

• Code-review lane flagged the exit-1 double-JSON-output bug (powershell-format.sh:657-658, matching Codex's P1) and a test-coverage gap for the new disclosure feature — see the 2026-08-12T17:50:57Z comment above.
• Security lane flagged unbounded temp-snapshot leakage on the trust-gate/tool-break exit paths (powershell-format.sh:210) across three passes.

Since the PR is already merged, no further action is taken here.