Skip to content

fix(security): patch six Claude Security scan findings (F1–F6) - #1097

Merged
kyle-sexton merged 28 commits into
mainfrom
feat/security-hardening-scan-findings
Jul 26, 2026
Merged

fix(security): patch six Claude Security scan findings (F1–F6)#1097
kyle-sexton merged 28 commits into
mainfrom
feat/security-hardening-scan-findings

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Summary

Patches all six findings from the first Claude Security scan of this
marketplace (4 MEDIUM, 2 LOW), on one branch. Each fix was generated in an
isolated scratch workspace and verified by a panel of agents — an
independent verifier that reviewed the diff and ran the project's tests,
plus a fresh adversarial reviewer asked what the change newly enables —
before it was accepted. Grouped into four commits by plugin.

id sev plugin issue
F1 MEDIUM markdown-format auto-run markdownlint-cli2 executed a repo .cjs/.mjs config as Node code on any markdown edit (CWE-94)
F2 MEDIUM powershell-format auto-run PSScriptAnalyzer executed a repo settings CustomRulePath module as PowerShell on any .ps1 edit (CWE-94)
F3 MEDIUM ai-briefing javascript:/data: source URL reached a deck anchor href unfiltered — stored XSS on click (CWE-79)
F4 MEDIUM ai-briefing linkinator reachability gate fetched attacker URLs with no private-host filter — blind SSRF (CWE-918)
F5 LOW shared lib/hook-utils.sh a bare trailing NAME=value Bash command leaked its value into the telemetry/audit subject (CWE-532)
F6 LOW ai-briefing unvalidated file:// UNC URL embedded as a PPTX hyperlink — NTLM leak on click (CWE-20)

Fix families:

  • F1/F2 — fail-closed per-repo/per-config-state trust gate: a code-loading
    config/settings file skips the auto-run (with a once-per-session notice
    naming the approval command) until the user records a content-addressed
    approval marker under ${CLAUDE_PLUGIN_DATA}/trust-approvals. Any config
    change revokes it; fails closed when the plugin-data dir is unavailable. The
    edit itself is never blocked.
  • F3/F4/F6 — one shared URL-policy seam in ai-briefing's
    lib/url-policy.js: an http/https/mailto/tel scheme allowlist
    enforced at schema time and at every href/hyperlink sink, plus private/
    loopback/link-local/reserved literal-host filtering in the reachability gate.
  • F5hook::extract_bash_subject now bails a resolved assignment-shaped
    token to the bare Bash subject; synced to all per-plugin copies via
    scripts/sync-hook-utils.sh, which is why this PR bumps every carrying
    plugin (the coupled sync-hook-utils / changelog-parity gates).

Deliberate, documented behaviour changes: ai-briefing now rejects
non-allowlisted URL schemes (javascript:/data:/file: and rarer ones like
ftp:); a repo whose linter config can execute code is no longer auto-run
until approved. Residual: ai-briefing's SSRF gate filters literal private
hosts only — a public hostname resolving to a private address at fetch time
(DNS rebind) is not gated offline, since linkinator resolves DNS itself
(documented in code and CHANGELOG).

Test plan

Run locally against the merge base, all green:

  • bash lib/hook-utils.test.sh → 90/90 (7 new subject cases)
  • bash scripts/sync-hook-utils.sh --check → all 12 copies match
  • bash scripts/sync-hook-utils.sh --check-bump origin/main → pass
  • bash scripts/check-changelog-parity.sh --check and --check-bump origin/main → pass
  • bash scripts/validate-plugins.sh and node scripts/generate-catalog.mjs --check → pass
  • bash scripts/check-silent-skips.sh, check-cross-plugin-source-drift.sh, check-changed-skills.sh origin/main, check-skill-portability.sh origin/main → pass
  • shellcheck -x lib/hook-utils.sh plugins/markdown-format/hooks/markdown-format.sh plugins/powershell-format/hooks/powershell-format.sh → clean
  • Per-plugin contract suites (run during verification): markdown-format 69/69, powershell-format 54/54 (real pwsh + PSScriptAnalyzer), ai-briefing node --test 27/27 (new schema + sink coverage)

Related

No linked issue — these are findings from a local Claude Security scan of this
repository (finding ids F1–F6); there is no tracked GitHub issue to close.

kyle-sexton and others added 4 commits July 23, 2026 01:32
… build (F3, F4, F6)

Source URLs parsed from briefing markdown (parse-briefing.js) flowed to
every URL sink with no scheme allowlist and no private-host filter,
enabling three issues found by the Claude Security scan:

- F3 (XSS, CWE-79): a javascript:/data: bullet source URL reached the
  deck anchor href unfiltered (escape() encodes only & < > ").
- F4 (SSRF, CWE-918): the linkinator reachability gate fetched
  attacker-controlled URLs (e.g. http://169.254.169.254, 127.0.0.1,
  RFC1918) from the build host with no private-host filter.
- F6 (input validation, CWE-20): a file:// UNC URL was embedded as a
  clickable PPTX hyperlink (NTLM leak on click).

Fix, via one shared seam in lib/url-policy.js reused at every sink:
- isAllowedUrlScheme() allowlists http/https/mailto/tel; the schema
  (lib/schema.js) refines Url with it (root defense), and the HTML
  (build-sections.js) and PPTX (build-pptx.js buildNews/buildCondensed)
  sinks filter bullet URLs before emitting (defense in depth).
- shouldSkipLinkCheck() now also skips literal private/loopback/
  link-local/reserved hosts (incl. decimal/hex/octal IPv4 and
  IPv4-mapped IPv6 via WHATWG canonicalization), so linkinator never
  fetches them.

Benign http/https/mailto/tel links are preserved; javascript:/data:/
file: and rarer schemes (ftp:, etc.) are rejected/dropped as deliberate,
documented hardening. Residual: a public hostname that resolves to a
private address at fetch time (DNS rebind) is not gated offline, since
linkinator resolves DNS itself — documented in the CHANGELOG and code.

Verified by a panel of agents (independent verifier + fresh adversarial
reviewer of the diff); `node --test` in the build dir passes 27/27,
including new schema and sink coverage.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…-utils redaction (F1, F5)

F1 (code injection, CWE-94): editing any markdown file auto-ran
markdownlint-cli2, which loads and executes a repo-supplied
.markdownlint-cli2.cjs/.mjs (or customRules/markdownItPlugins/
outputFormatters module identifiers) as Node code. The hook only emitted
a one-time non-blocking advisory and ran the linter anyway, so a
malicious repo's checked-in config achieved arbitrary code execution on a
routine edit.

Fix: a fail-closed trust gate ahead of the linter. When a code-loading
config is discovered, the lint run is skipped — with a visible
once-per-session notice naming the exact approval command — unless a
content-addressed approval marker exists under
${CLAUDE_PLUGIN_DATA}/trust-approvals. Any config change revokes the
approval; the gate fails closed when CLAUDE_PLUGIN_DATA is unavailable.
Declarative rule-only configs are unaffected; the edit is never blocked
(hook always exits 0).

Also carries the shared lib/hook-utils.sh fix (F5): the per-plugin
hook-utils.sh copy is re-synced so a bare/trailing unquoted NAME=value
Bash command no longer leaks its value into the telemetry/audit subject.

Verified by a panel of agents; the plugin's contract suite passes 69/69
(11 new trust-gate assertions) and shellcheck is clean.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…c hook-utils redaction (F2, F5)

F2 (code injection, CWE-94): editing a PowerShell file auto-ran
PSScriptAnalyzer with the repo's PSScriptAnalyzerSettings.psd1. A
settings file declaring CustomRulePath makes the analyzer load and
execute repo-supplied rule modules, so a malicious repo achieved
arbitrary PowerShell execution on a routine edit.

Fix: a fail-closed trust gate inside the pwsh invocation, before
Set-Location/Invoke-Formatter. CustomRulePath is detected with
Import-PowerShellDataFile (PowerShell's restricted, non-executing
data-file parser — a textual scan is evadable by backtick-escaped keys);
the run is skipped unless a content-addressed approval marker exists
under ${CLAUDE_PLUGIN_DATA}/trust-approvals. Any settings change revokes
the approval; a settings file the restricted parser cannot read also
fails closed. Settings without CustomRulePath run exactly as before; the
edit is never blocked (hook always exits 0).

Also carries the shared lib/hook-utils.sh fix (F5): the per-plugin
hook-utils.sh copy is re-synced so a bare/trailing unquoted NAME=value
Bash command no longer leaks its value into the telemetry/audit subject.

Verified by a panel of agents; the plugin's contract suite passes 54/54
(new gate + escaped-key evasion cases) against real pwsh + PSScriptAnalyzer,
and shellcheck is clean.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…(F5)

F5 (info disclosure, CWE-532): hook::extract_bash_subject in the shared
lib/hook-utils.sh builds a privacy-safe telemetry/audit subject for Bash
commands. Its prefix-strip loop removed a leading NAME=value token only
when a further command word followed it, so a command whose LAST token
was an unquoted assignment (e.g. the whole command is TOKEN=ghp_secret)
survived to the subject and was emitted verbatim as Bash:TOKEN=ghp_secret
into .claude/observability/hook-events.jsonl and any wired
HOOK_TELEMETRY_SINK — leaking the credential value into the same
observability store these hooks otherwise redact paths for.

Fix: a resolved first_token still shaped like a shell assignment
(^[a-zA-Z_][a-zA-Z0-9_]*=) now bails to the bare `Bash` subject, matching
the existing quoted-value bail. Placed before the basename strip so a
path-valued assignment (TOKEN=/a/b/secret) cannot leak via the tail.
VAR=x cmd still reduces to Bash:cmd; the subject feeds only telemetry/
audit emission, so no guard's block/allow decision changes.

lib/hook-utils.sh is the source of truth; scripts/sync-hook-utils.sh
propagates it to every per-plugin copy, and the repo's coupled
sync/changelog gates require each carrying plugin to bump its version and
add a CHANGELOG entry. This commit carries the lib fix and the remaining
carrying plugins (markdown-format and powershell-format ride with their
own security fixes in the preceding commits); guardrails is bumped to
0.12.3 over the version it already carries on main.

Verified by a panel of agents; lib/hook-utils.test.sh passes 90/90 (7 new
subject cases), sync-hook-utils.sh --check confirms all 12 copies match,
gitleaks is clean, and shellcheck is clean.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@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: 5debbb7166

ℹ️ 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 thread plugins/markdown-format/hooks/markdown-format.sh
Comment thread plugins/powershell-format/hooks/powershell-format.sh Outdated

@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: 50e1ee750d

ℹ️ 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 thread plugins/markdown-format/hooks/markdown-format.sh
Comment thread plugins/ai-briefing/skills/generate/output/build/lib/url-policy.js Outdated
kyle-sexton and others added 3 commits July 25, 2026 20:37
# Conflicts:
#	plugins/ai-briefing/CHANGELOG.md
#	plugins/claude-ops/.claude-plugin/plugin.json
#	plugins/claude-ops/CHANGELOG.md
#	plugins/guardrails/.claude-plugin/plugin.json
#	plugins/guardrails/CHANGELOG.md
#	plugins/markdown-format/.claude-plugin/plugin.json
#	plugins/markdown-format/CHANGELOG.md
#	plugins/rate-limit-guard/.claude-plugin/plugin.json
#	plugins/rate-limit-guard/CHANGELOG.md
…SRF gate to all non-global blocks

Review-round hardening of the F1/F2/F4 patches:

- markdown-format (F1): the approval signature is now content-addressed over
  the risky configuration AND every repository file its string literals
  resolve to (transitively, bounded 64), so a referenced rule module changing
  under an unchanged config revokes the approval. Module-key detection drops
  the key-colon anchor (YAML explicit-key syntax splits key and colon) and
  classifies escape/tag constructs able to hide a key spelling (JSONC \uXXXX;
  YAML \x/\u/\U, escaped line joins, !! tags) as unverifiable: gated with no
  approval route.
- powershell-format (F2): the approval signature moves into the pwsh block
  where the restricted parser resolves CustomRulePath, and covers the settings
  file plus every file reachable under each declared entry (recursive for
  directories, bounded 512). An entry that does not resolve to hashable
  content leaves the state unverifiable and fail-closed. The pwsh block hands
  the verdict to the shell as structured PSSA_TRUST lines.
- ai-briefing url-policy (F4): the literal-host SSRF gate now refuses every
  non-global IANA special-purpose block, not just RFC1918 - shared address
  space 100.64/10, benchmarking 198.18/15, TEST-NETs, 192.0.0/24, multicast,
  reserved, broadcast; IPv6 ff00::/8, 100::/64, 2001:db8::/32, 3fff::/20, and
  NAT64 64:ff9b::/96 judged by its embedded IPv4 address.
- autonomy: hook-utils.sh sync of the F5 fix reached the copy added on main
  (0.11.1).

Suites: markdown-format 78/78, powershell-format 56/56, ai-briefing node
--test 28/28, hook-utils 92/92; sync + changelog-parity + validate + catalog
+ silent-skips + portability gates green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012SV88ZgGDKdUKEPJaJtDpG
# Conflicts:
#	plugins/rate-limit-guard/.claude-plugin/plugin.json
#	plugins/rate-limit-guard/CHANGELOG.md

@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: 26fefc258e

ℹ️ 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 thread plugins/markdown-format/hooks/markdown-format.sh Outdated
Comment thread plugins/powershell-format/hooks/powershell-format.sh Outdated
Comment thread plugins/markdown-format/hooks/markdown-format.sh Outdated
# Conflicts:
#	plugins/claude-ops/.claude-plugin/plugin.json
#	plugins/claude-ops/CHANGELOG.md

@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: 5d17b0e386

ℹ️ 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 thread plugins/ai-briefing/skills/generate/output/build/lib/url-policy.js Outdated
Comment thread plugins/powershell-format/hooks/powershell-format.sh
# Conflicts:
#	plugins/actionlint/CHANGELOG.md
#	plugins/ai-briefing/CHANGELOG.md
#	plugins/bash-format/CHANGELOG.md
#	plugins/biome-format/CHANGELOG.md
#	plugins/claude-ops/CHANGELOG.md
#	plugins/desktop-notification/CHANGELOG.md
#	plugins/eol-normalizer/CHANGELOG.md
#	plugins/go-format/CHANGELOG.md
#	plugins/guardrails/CHANGELOG.md
#	plugins/markdown-format/.claude-plugin/plugin.json
#	plugins/markdown-format/CHANGELOG.md
#	plugins/powershell-format/.claude-plugin/plugin.json
#	plugins/powershell-format/CHANGELOG.md
#	plugins/ruff-format/CHANGELOG.md
#	plugins/typos-format/CHANGELOG.md
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

@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: 35a2e60b4d

ℹ️ 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 thread lib/hook-utils.sh Outdated
kyle-sexton and others added 2 commits July 25, 2026 21:52
…oor, bash 3.2, DNS gate, assignment forms

Addresses the six round-two Codex findings on the F1/F2/F4/F5 patches:

- markdown-format: module tracking resolves Node CommonJS candidates
  (.cjs/.mjs/.js/.json/.node and directory package.json/index.*) so an
  extensionless require("./rules/local-rule") pins local-rule.cjs; the trust
  gate is bash 3.2-clean (no associative arrays; every empty-array expansion
  guarded for set -u).
- powershell-format: the signature adds transitive string-literal references
  of each collected rule file (bounded; unreadable file or overflow is
  UNVERIFIABLE), and hashing uses instance SHA256.ComputeHash + x2 formatting
  (the static HashData/ToHexString shortcuts are .NET 5+, absent on the
  PowerShell 7.0 floor) with any failure emitting UNVERIFIABLE - an empty
  signature can never mint a shared marker.
- ai-briefing url-policy: DNS-name hosts are resolved at gate time (all
  A/AAAA records) and refused when any resolved address is non-global;
  unresolvable or unreadable answers fail closed. Resolver injectable for
  tests. Residual (rebind between gate and fetch; redirect hops inside the
  checker) documented.
- hook-utils: the trailing-assignment bail recognizes every valid Bash
  assignment form - NAME=v, NAME+=v, NAME[i]=v, NAME[i]+=v - so append and
  subscripted assignments no longer leak values into the telemetry subject.
  Synced to all 14 copies.

Suites: markdown-format 79/79, powershell-format 58/58, ai-briefing node
--test 31/31, hook-utils 95/95; shellcheck clean; parity/sync/validate/
catalog/silent-skips gates green.

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

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: fix(security): patch six Claude Security scan findings (F1–F6)

  • Read PR diff and key changed files
  • Review F1/F2: markdown-format and powershell-format trust gates
  • Review F3/F4/F6: ai-briefing URL policy
  • Review F5: hook-utils.sh assignment forms
  • Check Codex findings and Kyle's responses against current code
  • Post comprehensive review

Summary

This PR patches all six original findings (F1–F6). After reviewing the full diff at HEAD (f517ff4) against origin/main, including the multi-round Codex exchange and the responses in commits 285a037 and 90d9ccb, I found no blocking issues — all P1 findings from the Codex rounds are addressed in the current code. Notes and a few minor observations follow.


F5 — hook-utils.sh assignment redaction

The new regex ^[a-zA-Z_][a-zA-Z0-9_]*(\[[^]]*\])?\+?= in lib/hook-utils.sh:322 correctly covers all four Bash assignment forms:

  • NAME=value (plain)
  • NAME+=value (append)
  • NAME[idx]=value (subscripted)
  • NAME[idx]+=value (subscripted append)

The multi-assignment case (VAR=x TOKEN=secret) is correctly handled: the strip loop advances past VAR=x, leaving TOKEN=secret as first_token, which the new check catches. The check runs before ##*/ so a path-valued assignment (TOKEN=/a/b/secret) can't shed its = via basename stripping. The test coverage in lib/hook-utils.test.sh covers all these forms with specific regression fixtures. ✓


F3/F4/F6 — ai-briefing URL policy

The scheme allowlist is enforced at three independent layers:

  1. Schema (lib/schema.js): z.string().url().refine(isAllowedUrlScheme) rejects javascript:/data:/file: at validation time
  2. PPTX/HTML sinks (build-pptx.js, build-sections.js): .filter(isAllowedUrlScheme) applied before each hyperlink site
  3. Reachability gate (shouldSkipLinkCheck): adds DNS resolution on top of literal-host filtering

The isPrivateIPv4 function correctly covers the full IANA IPv4 Special-Purpose Address Registry including 100.64.0.0/10 (CGN/shared address space). The isPrivateIPv6 covers loopback, unique-local, link-local, multicast, documentation prefixes, and importantly handles IPv4-mapped (::ffff:a.b.c.d) and NAT64 (64:ff9b::/96) by extracting and testing the embedded IPv4 address. I traced through the expandIPv6 logic on all the edge cases in the test suite and the math is correct.

The DNS resolution layer in shouldSkipLinkCheck uses dns.lookup({ all: true }) (all A/AAAA records) and refuses the link if any resolved address is non-global — the right fail-safe semantics. The test suite injects a stub resolver to cover the "ANY non-global record fails" and "unresolvable fails closed" cases without touching real DNS.

One observation (non-blocking): shouldSkipLinkCheck returns false (don't skip) for mailto: and tel: URLs because the DNS resolution block is only entered for http:/https:. For mailto:person@example.com, url.hostname is "", so isPrivateHost("") returns false, and the early-return path url.protocol !== "http:" && url.protocol !== "https:" fires and returns false. This means mailto: links are handed to Linkinator for checking; Linkinator natively skips these, so there's no SSRF exposure, but the function's contract ("skip when private") is silently not applied to non-HTTP schemes with non-empty hosts. Not a correctness issue given Linkinator's behavior, but worth noting in case the function is ever reused.


F1 — markdown-format trust gate

The two-tier textual over-approximation for JSONC/YAML key detection is the right call over building a second parser (which would open a differential-parsing gap). Specifically:

  • Tier 1: bare keywords (customRules|markdownItPlugins|outputFormatters) anywhere in the file, without a key-colon anchor, which also catches YAML explicit-key form
  • Tier 2: JSONC \uXXXX escapes, YAML \x/\u/\U escapes, escaped line joins, and !! tags all set RISK_UNVERIFIABLE=1 → fail closed with no approval route

The collect_module_files function now resolves extensionless CommonJS paths by trying "$base", "$base.cjs", "$base.mjs", "$base.js", "$base.json", "$base.node" for each string literal, which addresses the Codex finding about require("./rules/local-rule") missing local-rule.cjs. The transitive rescan (scanning each collected module for its own string literals) is bounded at 64 files and fails closed on overflow.

Bash 3.2 compatibility is maintained throughout: dedup state uses newline-delimited strings rather than local -A, and every array expansion is guarded by a ((${#arr[@]} > 0)) check. ✓

One edge note (non-blocking): The string-literal extractor grep -oE "\"[^\"]+\"|'[^']+'" will mis-parse a string containing an escaped quote (e.g., require("./it's/fine")), potentially producing a truncated path. In practice, module paths rarely contain quotes, and the consequence is a missed entry (fail-open for that specific edge) rather than a security bypass (the gate still fires for the risky config; only that one dependency is unpinned). This is the inherent limitation of text-based static approximation without a real parser, already acknowledged in the code's comment block.


F2 — powershell-format trust gate

The PS 7.0 compatibility fix — using [System.Security.Cryptography.SHA256]::Create() (instance method, .NET Core 3.1+) with $b.ToString("x2") hex formatting instead of the .NET 5-only static SHA256.HashData / Convert.ToHexString — addresses the Codex P1 about empty signatures silently minting a shared marker. The catch { $signature = "" } + empty-string check before emitting the marker ensures any failure is UNVERIFIABLE. ✓

The transitive scanning loop collects string literals from each rule file and rescans any file hits, bounded at 256 scans / 512 files. An unreadable file → UNVERIFIABLE guard prevents partial signatures. The documented residual (a rule module using a computed dot-source path like $PSScriptRoot\$helper) is correctly noted in both the PR description and the code.


Test coverage

The test suites are thorough:

  • IP boundary tests assert both sides of every new block (e.g., 100.63.x vs 100.128.x for the CGN range)
  • DNS resolver injection allows testing non-global lookup results without real DNS calls
  • Regression fixtures for specific bypass cases: "customRules" (JSONC escape), YAML !!binary, branch-switch module mutation, etc.
  • The build-sinks test drives the real build-pptx.js entrypoint and reads PPTX relationship XML to confirm file:// hyperlinks are filtered end-to-end

Verdict

All six original findings are patched and all Codex P1s from the multi-round review are addressed in the current code. The implementation is careful and the documented residuals (DNS rebinding window, computed dot-source transitive deps) are appropriately scoped. No changes required from this review — the mailto: observation and the escaped-quote edge case in collect_module_files are informational only.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 9m 13s —— View job


Security Review — PR #1097

Tasks

  • Gather context and understand the PR
  • Read PR diff and key changed files
  • Audit hook-utils.sh (F5 — assignment-form credential leak)
  • Audit markdown-format.sh (F1 — trust gate for code-loading configs)
  • Audit powershell-format.sh (F2 — trust gate for PSScriptAnalyzer)
  • Audit url-policy.js + sink files (F3/F4/F6 — XSS, SSRF, NTLM)
  • Post findings

Reviewed commit f517ff4. Focused on the 60 changed files only. I audited each fix family independently against the original finding and looked for new attack surface introduced by the changes.


Findings

IMPORTANT — Residual credential-leak bypass in the F5 assignment-form regex

Severity: IMPORTANT · Confidence: CONFIRMED

The fix in lib/hook-utils.sh adds:

if [[ "$first_token" =~ ^[a-zA-Z_][a-zA-Z0-9_]*(\[[^]]*\])?\+?= ]]; then

The subscript pattern \[[^]]*\] uses [^]]* — "any character that is not ]" — which stops at the first ] it encounters. When the subscript itself contains a ] (which is valid Bash for dynamic subscripts), the overall pattern fails to match:

first_token = A[${#arr[@]}]=ghp_secret
                          ^
              [^]]* stops here (first ] after @)
Remaining: }]=ghp_secret — \+?= can't match }, pattern fails

Result: A[${#arr[@]}]=ghp_secret is emitted as Bash:A[${#arr[@]}]=ghp_secret, leaking ghp_secret into the telemetry/audit subject — the exact class of exposure F5 is closing.

The common forms (NAME[0]=val, NAME[idx]=val) do work correctly. The escape requires ${#arr[@]} or similar expressions as the subscript, which is uncommon as a standalone top-level command but is valid Bash. The regex can be made robust by using the POSIX ERE character class [^][]* (matches any char that is not [ or ]), or alternatively by detecting = anywhere after the variable name and before whitespace without attempting to validate the subscript structure. All downstream copies in plugins/*/hooks/hook-utils.sh carry the same code via the sync script.

Fix this →


SUGGESTION — Mixed-notation IPv4-mapped IPv6 (::ffff:192.168.1.1) is not classified private

Severity: SUGGESTION · Confidence: PLAUSIBLE (low exploitability in practice)

isPrivateIPv6 in url-policy.js delegates to expandIPv6, which splits on : and parses each group with parseInt(h, 16). For the mixed-notation address ::ffff:192.168.1.1, the last segment is 192.168.1.1; parseInt("192.168.1.1", 16) = 402 (stops at .), which exceeds 0xffff, causing expandIPv6 to return null and isPrivateIPv6 to return false — not classified private.

The two paths where private-host classification happens:

  1. Literal URL hostname — WHATWG new URL("http://[::ffff:192.168.1.1]/") canonicalizes to [::ffff:c0a8:101] (full hex). expandIPv6 handles this correctly.
  2. DNS resolutiondns.lookup() returns IPv6 addresses in RFC-5952 abbreviated hex form, not mixed notation. Private IPv4 A records are returned as plain 192.168.1.1 and routed through isPrivateIPv4.

In practice the gap is not reachable via either path. It becomes reachable if a custom resolveHost implementation or a future Node.js change returns mixed-notation AAAA records. Worth patching to eliminate the theoretical exposure.


What was checked and found clean

Area Verdict
F1 — markdown-format trust gate Well-implemented. Content-addressed signature covers configs + resolved module graph. Fail-closed on unverifiable state. Bash 3.2 compatibility guards are correct.
F2 — powershell-format trust gate Well-implemented. Signature computed inside pwsh using Import-PowerShellDataFile (prevents backtick/quoting bypasses a textual grep would miss). SHA256 uses .NET 3.1-compatible instance API. PSSA_TRUST line parsing is safe (pwsh never writes user-controlled content to stdout in that block).
F3 — XSS via javascript:/data: href isAllowedUrlScheme allowlist applied at schema validation and every HTML/PPTX sink. escape(u) encodes " so attribute injection is not possible.
F4 — SSRF in link reachability gate DNS pre-resolution gate covers all A/AAAA records; fail-closed on non-resolving names. isPrivateIPv4 covers the full IANA special-purpose registry. Residual (DNS rebind, redirect hops) is documented.
F6 — file:// UNC NTLM leak file: scheme not in allowlist; blocked at schema time and all sinks.
F5 assignment forms (common) NAME=val, NAME+=val, NAME[0]=val, NAME[idx]+=val are all caught. The narrow residual for subscripts containing ] is noted above.
Trust-marker path traversal Signature is a hex string; pssa-<64hex> and sha1<40hex> forms contain no path separators.
pwsh env-var injection File/settings/state-base paths pass via env vars (not interpolated into the script string); -LiteralPath used throughout.
APPROVE_HINT shell quoting $TRUST_DIR is display-only (user runs mkdir manually); no execution risk. Single-quote edge case only affects UX.

@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: f517ff4fc8

ℹ️ 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 thread plugins/markdown-format/hooks/markdown-format.sh Outdated
Comment thread plugins/powershell-format/hooks/powershell-format.sh
Comment thread plugins/ai-briefing/skills/generate/output/build/lib/url-policy.js
# Conflicts:
#	plugins/claude-ops/CHANGELOG.md
#	plugins/desktop-notification/CHANGELOG.md
#	plugins/markdown-format/.claude-plugin/plugin.json
#	plugins/markdown-format/CHANGELOG.md
#	plugins/rate-limit-guard/CHANGELOG.md
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: fix(security): patch six Claude Security scan findings (F1–F6)

Tasks

  • Load tools and begin review
  • Read changed files (hook-utils.sh, url-policy.js, markdown-format.sh, powershell-format.sh)
  • Check open findings from latest Codex round (computed paths, NAT64, regex)
  • Check security review IMPORTANT finding (subscript ] in regex)
  • Post comprehensive review

Reviewed current HEAD (d859fc6). The prior review rounds have been thorough. Three P1s from the final Codex round (f517ff4fc8, posted after Kyle's last round of responses) remain open in the current code, and one IMPORTANT finding from the concurrent security review is likewise unaddressed. The fixes below are all targeted and small.


1 — Open P1: NAT64 local-use prefix not classified private

File: plugins/ai-briefing/skills/generate/output/build/lib/url-policy.js:91-95

The embedsIPv4 check covers the IANA well-known NAT64 prefix 64:ff9b::/96 by requiring g.slice(2, 6).every((h) => h === 0). The local-use NAT64 prefix 64:ff9b:1::/48 (RFC 8215) has g[2] === 0x0001, which fails that condition.

Trace for http://[64:ff9b:1::1]/expandIPv6("64:ff9b:1::1")g = [0x0064, 0xff9b, 0x0001, 0, 0, 0, 0, 0x0001]:

  • g[0] === 0x0064 ✓ and g[1] === 0xff9b
  • g.slice(2, 6).every((h) => h === 0)g[2] = 1FALSE
  • embedsIPv4 = false, function returns false — link passes as public

Fix: add a branch that blocks the entire 64:ff9b:1::/48 range directly, before the embedsIPv4 check:

// 64:ff9b:1::/48 local-use NAT64 (RFC 8215) — non-global, treat as private
if (g[0] === 0x0064 && g[1] === 0xff9b && g[2] === 0x0001) return true;

Also add 64:ff9b:1::1 as a boundary test case in url-policy.test.js (and 64:ff9b:0::1 asserting it still passes through the embedsIPv4 branch to isPrivateIPv4).

Fix this →


2 — Open IMPORTANT: subscript-with-] bypass in hook-utils.sh assignment regex

File: lib/hook-utils.sh:322

if [[ "$first_token" =~ ^[a-zA-Z_][a-zA-Z0-9_]*(\[[^]]*\])?\+?= ]]; then

[^]]* means "chars that are not ]". For A[${#arr[@]}]=ghp_secret, the subscript body is ${#arr[@]} which contains ] (after @). The match stops at that inner ], then \] tries to match the following } — fails. The group backtracks to optional, and \+?= tries [ — also fails. The whole pattern fails, so A[${#arr[@]}]=ghp_secret is emitted as the subject, leaking the value.

Fix: replace [^]]* with .* so the subscript body matches greedy-until-last-]:

if [[ "$first_token" =~ ^[a-zA-Z_][a-zA-Z0-9_]*(\[.*\])?\+?= ]]; then

With greedy .*, for A[${#arr[@]}]=ghp_secret: \[.*\] matches [${#arr[@]}] (greedy then backtracks to the last ] before =), then \+?= matches = — correctly classified as an assignment. Sync the fix to all 14 per-plugin hook-utils.sh copies and add A[${#arr[@]}]=ghp_secret as a regression fixture in lib/hook-utils.test.sh.

Fix this →


3 — Open P1: computed module paths in markdown-format trust gate

File: plugins/markdown-format/hooks/markdown-format.sh:332 (collect_module_files)

The string-literal extractor (grep -oE "$quoted_string_re") pulls out "rules" and "local.cjs" individually from require(path.join(__dirname, "rules", "local.cjs")). Neither maps to rules/local.cjs"rules" is tried as a directory (only its entry points are enqueued, not all files), and "local.cjs" is tried only in config_dir/ and CONFIG_ROOT/, not in rules/. The module is absent from the signature; changing rules/local.cjs after approval executes the changed code without reapproval.

Recommended fix: in collect_risky_configs, or as an extra scan pass immediately before calling collect_module_files, grep the .cjs/.mjs config for known dynamic path constructs (path.join, path.resolve, path.join\|__dirname\|__filename\|process.cwd). If any match, set RISK_UNVERIFIABLE=1 — consistent with the existing Tier 2 (JSONC/YAML escape) fail-closed strategy. This means: a config that computes paths at runtime cannot be statically reviewed and linting stays disabled, which is the same outcome as the JSONC \uXXXX case.

Fix this →


4 — Open P1 (partial residual): interpolated PowerShell variable paths

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

The transitive scan extracts string literals with $litPattern, finding $PSScriptRoot/helper.ps1 from . "$PSScriptRoot/helper.ps1". Join-Path $curDir "$PSScriptRoot/helper.ps1" produces a path where $PSScriptRoot is a literal directory name — Test-Path fails, the file is missed, and changing helper.ps1 after approval executes the changed dependency undetected.

Kyle documented this as a statically unpinnable residual. The Codex finding is that this could be addressed by detecting it as unverifiable: if an extracted literal contains a $ character (PowerShell variable interpolation), it cannot be resolved statically and should emit PSSA_TRUST UNVERIFIABLE rather than silently skipping. This matches the pattern for computed-path handling in finding #3. The fix would be: in the transitive scan loop at line 302, if $lit contains $, output PSSA_TRUST UNVERIFIABLE and exit 6.

Decision point for maintainer: this is more conservative than the current documented residual. Whether the UX cost (configs using $PSScriptRoot in dot-source paths are entirely blocked) is acceptable is a policy call, but accepting it as silent residual means an attacker who can write to helper.ps1 after approval can execute arbitrary code.

Fix this →


What is clean

Area Status
F5 common assignment forms (NAME=v, NAME+=v, NAME[0]=v, NAME[idx]+=v) ✓ Addressed; subscript-with-] is the residual
F3 — javascript:/data: XSS via href ✓ Scheme allowlist enforced at schema and all sinks
F4 — SSRF literal hosts + DNS pre-resolution gate ✓ Correct; all-records lookup, fail-closed on unresolvable
F6 — file:// UNC NTLM leak file: blocked at schema + sinks
F1 — markdown trust gate (extensionless CJS, Bash 3.2, JSONC/YAML escapes) ✓ Addressed; computed paths are the residual
F2 — powershell trust gate (PS 7.0 .NET floor, transitive scan) ✓ Addressed; interpolated vars are the residual
Mixed-notation IPv4-mapped ::ffff:192.168.1.1 ✓ WHATWG new URL() canonicalizes to full hex before expandIPv6
100.64.0.0/10, multicast, reserved IANA ranges ✓ Covered in isPrivateIPv4
Trust-marker path traversal ✓ Hex-only signature format

Verdict

Two targeted fixes are needed before merge — NAT64 64:ff9b:1::/48 (#1) and the subscript-with-] regex (#2) — both small and non-architectural. Findings #3 and #4 are the "computed path" class that the Codex has flagged across both plugins; the right mitigation (fail-closed on detection) is consistent with the existing UNVERIFIABLE strategy and I'd recommend addressing them rather than accepting as silent residuals, but that is a policy call for the maintainer.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@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: 6552b0ca01

ℹ️ 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 thread plugins/ai-briefing/skills/generate/output/build/lib/url-policy.js
Comment thread plugins/markdown-format/hooks/markdown-format.sh Outdated
Comment thread plugins/powershell-format/hooks/powershell-format.sh Outdated
…ed loaders

Two more mechanisms, both new classes rather than instances of one already
closed.

markdown-format: a reference that RESOLVES outside the repository was silently
skipped. A symlink inside the tree aimed out of it, or a `../` escape, names
code no signature over repository content can cover — re-aiming the symlink at a
different existing external target leaves the approval state byte-identical
while Node follows the new one and executes different code. Hashing the external
file instead would extend the signature beyond the repository the approval is
scoped to, so the state is refused. CONFIG_ROOT is already a physical path
(`pwd -P`), so this compares like with like and a symlinked checkout does not
read as an escape. Covered by a `../` fixture that runs everywhere plus a
symlink fixture gated on host symlink support, matching the suite's existing
idiom.

powershell-format: a loader fed by a pipeline takes its source from the upstream
element, not from its own arguments, so the AST walk saw only a constant command
name and accepted. `Get-Content (Join-Path $PSScriptRoot "deps" "helper.ps1")
-Raw | Invoke-Expression` executed a file the scan never reconstructed, and
`Get-ChildItem *.psm1 | Import-Module` has the same shape. Any loader that is
not the first element of its own pipeline is now refused.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

kyle-sexton and others added 2 commits July 26, 2026 00:42
…ule symlink

The out-of-repository guard added alongside this depends on
hook::physical_path, which degrades to the unchanged lexical path when neither
realpath nor readlink -f resolves. On such a host an escaping symlink reads as
in-repository and gets pinned by its lexical path, leaving the external target
free to change under a live approval — the guard failing open on exactly the
hosts it matters most on.

A symlink whose physical path came back unchanged is the observable signature of
that degradation, since a symlink never canonicalizes to itself. That is the
same test the file's membership scope already uses for the same reason, and it
gets the same fail-closed answer.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ng-scan-findings

plugins/guardrails/CHANGELOG.md: both sides added a distinct bullet to the same
0.16.2 "Fixed" section and neither removed anything, so both are kept — main's
skill-reference-verify anchoring fix (#1453) and this branch's hook-utils.sh
telemetry-subject redaction sync.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

The plugin version is the update cache key, so `sync-hook-utils.sh --check-bump`
requires every plugin carrying `lib/hook-utils.sh` to bump when the lib changes.
This branch had already bumped guardrails to 0.16.2 for the telemetry-subject
redaction, but main then released its own 0.16.2 for the skill-reference-verify
anchoring fix, so the version no longer cleared the base and an unbumped plugin
would never deliver the lib change.

0.16.3 carries the redaction sync; main's 0.16.2 entry keeps the release note it
was published with.
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@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: 4b669c71a5

ℹ️ 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 thread plugins/markdown-format/hooks/markdown-format.sh
Comment thread plugins/powershell-format/hooks/powershell-format.sh
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: fix(security): patch six Claude Security scan findings (F1–F6)

Tasks

  • Load tools
  • Read key changed files at HEAD (4b669c71)
  • Assess latest Codex P1s (eval-wrapped loaders, constant iex bodies)
  • Verify prior-round findings remain addressed
  • Post comprehensive review

Reviewed HEAD commit 4b669c71. This branch has accumulated seven additional security commits (ff5fd80 through 0641962) after the most recent concurrent agent reviews at ebdfb77. The Codex review at the current HEAD raises two P1s that remain open.


Open P1 — eval-wrapped Markdown loader bypasses pinnability check

File: plugins/markdown-format/hooks/markdown-format.sh:404–421

The unpinnable_js_specifier function uses a two-tier test. Tier 2 (loader-residue) strips plainly-written loader calls with sed then greps for remaining loader tokens:

sed -E "s/(require|import)[[:space:]]*\([[:space:]]*(\"[^\"]*\"|'[^']*')/ /g" "$file" |
  grep -Eq "(^|[^A-Za-z0-9_\$])(require($|[^A-Za-z0-9_\$])|import[[:space:]]*\()"

For module.exports = eval('require("./real-config.cjs")'):

  • The sed pattern matches require("./real-config.cjs") inside the eval argument and replaces it with a space
  • After stripping: module.exports = eval(' ')
  • No require/import token remains — grep fails
  • unpinnable_js_specifier returns false: not unpinnable

collect_module_files then extracts string literals from the file: 'require("./real-config.cjs")' → stripped → require("./real-config.cjs"). No path $dir/require("./real-config.cjs") exists, so real-config.cjs is never added to MODULE_FILES. Changing only real-config.cjs after approval executes the changed code on the next markdown edit without revocation.

Tier 1 also does not catch it: eval involves neither path.join, __dirname, process., ${, string concat, nor an escape sequence.

Fix: add eval to the unpinnable patterns (one line in Tier 1):

-e 'eval[[:space:]]*\('

Any config containing eval( is marked unverifiable → fail closed with no approval route. This is consistent with the existing JSONC \uXXXX and computed path.join(...) strategies.

[Fix this →](https://claude.ai/code?q=In%20plugins%2Fmarkdown-format%2Fhooks%2Fmarkdown-format.sh%20on%20branch%20feat%2Fsecurity-hardening-scan-findings%2C%20the%20unpinnable_js_specifier%20function%20at%20line%20404%20does%20not%20detect%20eval().%20For%20module.exports%20%3D%20eval('require(%22.%2Freal-config.cjs%22)')%2C%20the%20sed%20pattern%20at%20line%20419%20strips%20require(%22...%22)%20from%20inside%20the%20eval%20argument%20string%2C%20leaving%20module.exports%20%3D%20eval('').%20The%20grep%20then%20finds%20no%20loader%20token%20and%20the%20function%20returns%20false%20(not%20unpinnable)%2C%20so%20real-config.cjs%20is%20never%20added%20to%20MODULE_FILES.%20Add%20-e%20'eval%5B%5B%3Aspace%3A%5D%5D*%5C('%20to%20the%20Tier%201%20grep%20at%20lines%20406-416%20so%20any%20file%20containing%20eval(%20is%20classified%20RISK_UNPINNABLE.%20Add%20a%20regression%20fixture%20for%20module.exports%20%3D%20eval('require(%22.%2Freal-config.cjs%22)')%20in%20markdown-format.test.sh%20asserting%20it%20gates%20with%20no%20approval%20route.&repo=melodic-software/claude-code-plugins)


Open P1 — Constant Invoke-Expression body misses its dot-sourced dependency

File: plugins/powershell-format/hooks/powershell-format.sh:387–406

For iex '. "$PSScriptRoot/helper.ps1"', the AST walk identifies iex as a loader (in $loaders) and the argument as a StringConstantExpressionAst with .Value = '. "$PSScriptRoot/helper.ps1"'. This literal string is added to $pending:

if ($el -is [System.Management.Automation.Language.StringConstantExpressionAst]) {
    [void]$pending.Add($el.Value)   # adds: . "$PSScriptRoot/helper.ps1"
    continue
}

Candidate resolution then tries Join-Path $baseDir '. "$PSScriptRoot/helper.ps1"' — a path containing a literal . " prefix and embedded " — which no Test-Path check will satisfy. helper.ps1 is not added to $allFiles.

The text-based $litPattern scan runs on the same file. $litPattern = '([^']+)'|"([^"]+)". It matches the outer single-quoted string '. "$PSScriptRoot/helper.ps1"', capturing . "$PSScriptRoot/helper.ps1" as group 1. After expandKnownVars, the result is . "/actual/path/helper.ps1" — still with the . " prefix and trailing " — which again can't be resolved. The inner "$PSScriptRoot/helper.ps1" is NOT independently extracted because [regex]::Matches is non-overlapping and the outer single-quoted match already consumed that span.

Result: helper.ps1 is absent from $allFiles; changing it after approval executes changed code without revocation.

Note: b2ca7f1 addressed pipeline-fed iex (Get-Content ... | iex) by checking pipeline position. The constant-body form is distinct and unaddressed.

Fix: emit PSSA_TRUST UNPINNABLE for any iex/Invoke-Expression call, since reconstructing what the body executes requires parsing the argument value as a new script — equivalent to the recursive analysis the main scan already performs. Any legitimate iex use in a PSA rule module has a structural equivalent (Import-Module, . $path) that the scan handles correctly:

if ($cmdName -in @("iex", "Invoke-Expression")) {
    Write-Output "PSSA_TRUST UNPINNABLE"
    exit 6
}

Place this check immediately after the pipeline-position check (line 380). Add a regression fixture in powershell-format.test.sh.

Fix this →


All prior-round findings: confirmed addressed at HEAD

Finding Status Key mechanism
F1 markdown-format CWE-94 Content-addressed trust gate over config + module graph
F2 powershell-format CWE-94 pwsh-computed SHA256 signature over settings + transitive deps
F3 ai-briefing XSS CWE-79 isAllowedUrlScheme at schema + both HTML/PPTX sinks
F4 ai-briefing SSRF CWE-918 Full IANA special-purpose registry + DNS pre-resolution gate
F5 hook-utils CWE-532 Greedy (\[.*\])? subscript; all assignment forms; TOKEN[1+INDEX[0]]=v fixture
F6 ai-briefing NTLM CWE-20 file: not in allowlist; blocked at schema + sinks
Subscript [^]]*.* Fixed in ec17b15; regression at test:895
Escaped JS specifiers \uXXXX Fixed in ec17b15; regression fixture
RFC 8215 64:ff9b:1::/48 NAT64 Fixed in 9bfaec7
2001::/23, 2002::/16, 5f00::/16 IPv6 Fixed in 9bfaec7 and 117ba32
Computed path.join() module paths Non-literal require/import arg → fail closed
$PSScriptRoot relative dot-sources Fixed in 9bfaec7
$PSScriptRootFoo variable-name prefix collision Fixed in 0aba2b4; bounded expansion pattern
Alias of path module (const { join } = require("path")) Fixed in ff5fd80; import refused at import not call sites
comment-separated loader require/*c*/(...) Fixed in 117ba32; residue test needs no window
JSONC/YAML if-elif ordering (escape tier suppressed) Fixed in 6552b0c; independent tests
YAML plain scalars in module paths Fixed in 6552b0c
using module (not CommandAst) Fixed in 6552b0c; UsingStatementAst scan
using assembly Documented: .dll via using assembly is a .NET assembly, not a repo PS module; Add-Type is in $loaders
Extensionless PS module resolution Fixed in 6552b0c; .psd1/.psm1 + versioned layout candidates
Out-of-repository module targets (symlinks/../) Fixed in b2ca7f1; CONFIG_ROOT boundary check
Pipeline-fed iex (Get-Content ... | iex) Fixed in b2ca7f1; pipeline-position check
Symlink canonicalization degradation Fixed in 0641962; unchanged lexical path → fail closed
PS 7.0 / .NET 3.1 SHA256 floor Instance API [SHA256]::Create() + ComputeHash
Bash 3.2 compatibility local -A replaced; empty-array expansion guards
Trust-marker path traversal 40/64-char hex signatures; no separators

Documented residuals (appropriate scope)

  • DNS rebinding: The pre-resolution gate runs at validation time; Linkinator re-resolves for the actual fetch. Rebind between those two points is outside an offline tool's threat model.
  • Redirect hops: Linkinator follows HTTP redirects internally; a chain ending at a private host after the pre-check is not interceptable without replacing Linkinator's HTTP stack.
  • Arbitrary $var-relative PS dot-sources (not $PSScriptRoot/$PSCommandPath): e.g. . "$MyCustomRoot/helper.ps1" — static expansion not possible; the $ check in expandKnownVars leaves the $ in the value, which then fails path resolution and the dependency is silently omitted (weaker than fail-closed). This residual predates this PR's scope and is explicitly documented.

Verdict

Two targeted fixes are needed before merge. Both are small and self-contained:

  1. eval in JS configs (markdown-format) — add eval[[:space:]]*\( to the Tier 1 grep in unpinnable_js_specifier. One pattern line + one regression fixture.
  2. Constant iex body (powershell-format) — emit UNPINNABLE for any iex/Invoke-Expression call after the pipeline-position check. Four lines + one regression fixture.

The rest of the implementation is thorough and correct. The testing across all six original findings plus the 20+ subsequent review-round findings is comprehensive. These two items are the only remaining gaps at HEAD.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 10m 11s —— View job


Security Review — PR #1097

Tasks

  • Load tools
  • Get PR diff at HEAD (4b669c7)
  • Read key changed files: hook-utils.sh, markdown-format.sh, powershell-format.sh, url-policy.js
  • Audit all security-relevant changes at HEAD for new vulnerabilities
  • Cross-check latest Codex findings (eval-wrapped loaders, constant iex bodies)
  • Post findings

Reviewed HEAD commit 4b669c71a5. The four rounds of prior security review addressed the bulk of the original six findings plus the follow-on Codex rounds, but three confirmed bypasses and one plausible gap remain open at the current tip.


IMPORTANT — eval-wrapped require bypasses the markdown trust gate

Severity: IMPORTANT · Confidence: CONFIRMED

unpinnable_js_specifier in markdown-format.sh:404–421

For a .cjs config containing:

module.exports = eval('require("./real-config.cjs")')

Step through the function:

  1. First grep — no pattern covers eval, so none of the patterns match.
  2. sed stripper (line 419): the pattern (require|import)[[:space:]]*\([[:space:]]*(\"[^\"]*\"|'[^']*') matches require("./real-config.cjs") inside the eval string and replaces it with a space. After stripping: module.exports = eval(' ').
  3. Residue grep (line 420): looks for a remaining require or import( token in eval(' ') — finds none. Returns false (exit 1).
  4. unpinnable_js_specifier returns 1 — not unpinnable.

collect_module_files then extracts quoted strings from the original file:

grep -oE "\"[^\"]+\"|'[^']+'"

Starting from the left, 'require("./real-config.cjs")' matches the single-quoted pattern first. The inner "./real-config.cjs" is consumed inside that outer match and is not returned as a separate hit. After stripping the outer quote chars the extracted string is require("./real-config.cjs"), which doesn't resolve to any file. real-config.cjs is not added to MODULE_FILES and is not in the approval signature.

Exploit path: create a .cjs config that eval-requires another repo module containing customRules config or malicious code; get the initial approval; then edit only the inner module. The config's text is unchanged, so the signature is unchanged, and the approval marker is still valid when the inner module's changed code runs on the next markdown edit.

Fix: add eval to the first grep in unpinnable_js_specifier:

-e '(^|[^A-Za-z0-9_])eval[[:space:]]*\('

This is the fail-closed direction (a JS config that uses eval gets no approval route).

[Fix this →](https://claude.ai/code?q=In%20plugins%2Fmarkdown-format%2Fhooks%2Fmarkdown-format.sh%20on%20branch%20feat%2Fsecurity-hardening-scan-findings%2C%20the%20function%20unpinnable_js_specifier%20(lines%20404-421)%20does%20not%20detect%20eval()%20calls.%20For%20a%20config%20like%20module.exports%20%3D%20eval('require(%22.%2Freal-config.cjs%22)')%3A%20(1)%20no%20first-grep%20pattern%20matches%20eval%3B%20(2)%20the%20sed%20pattern%20strips%20require(%22./real-config.cjs%22)%20inside%20the%20eval%20string%2C%20leaving%20eval('%20')%3B%20(3)%20the%20residue%20grep%20finds%20no%20remaining%20require%20token%3B%20so%20the%20function%20returns%20%22pinnable%22.%20The%20literal%20extractor%20then%20finds%20'require(%22.%2Freal-config.cjs%22)'%20as%20one%20string%20which%20doesn't%20resolve%20to%20a%20file.%20real-config.cjs%20is%20not%20pinned%20in%20the%20signature.%20Fix%3A%20add%20-e%20'(^%7C%5B%5EA-Za-z0-9_%5D)eval%5B%5B%3Aspace%3A%5D%5D*%5C('%20to%20the%20first%20grep%20in%20unpinnable_js_specifier%20so%20any%20config%20using%20eval()%20is%20refused%20approval.%20Add%20a%20regression%20fixture%20for%20this%20case%20in%20markdown-format.test.sh.&repo=melodic-software/claude-code-plugins)


IMPORTANT — Constant-string iex body not parsed in the PowerShell trust gate

Severity: IMPORTANT · Confidence: CONFIRMED

$cmdAst.CommandElements loop in powershell-format.sh:387–391

For a custom rule module containing:

iex '. "$PSScriptRoot/helper.ps1"'

iex is in $loaders. The command is NOT pipeline-fed (the pipeline check passes). The argument '. "$PSScriptRoot/helper.ps1"' is a StringConstantExpressionAst with value . "$PSScriptRoot/helper.ps1". This value is added directly to $pending at line 390:

[void]$pending.Add($el.Value)   // adds `. "$PSScriptRoot/helper.ps1"`

In the resolution loop that follows, $base = Join-Path $baseDir '. "$PSScriptRoot/helper.ps1"'. No file at that path exists — the string is a PowerShell dot-source command, not a file path. helper.ps1 is never found and is not added to $allFiles. It is not included in the signature.

Exploit path: a custom rule module that uses iex with a constant dot-source string is approved; changing only helper.ps1 after approval leaves the signature unchanged and the malicious helper executes on the next PS edit.

The pipeline-fed guard at lines 380–386 covers Get-Content ... | iex but not iex 'command string'. The distinction is load-bearing: the argument IS a constant, but its VALUE is PS script code rather than a path, and the resolution loop cannot know that.

Fix options:

  • Treat any iex/Invoke-Expression call whose argument is a StringConstantExpressionAst as UNPINNABLE (fail closed — iex with a constant string is a dynamic-execution form this scan cannot trace).
  • OR recursively parse the constant string body as PS code (complex, risk of a parser gap).

The simpler and safer option is the fail-closed one: emit PSSA_TRUST UNPINNABLE and exit 6 when iex/Invoke-Expression is called with a string-constant argument.

Fix this →


IMPORTANT — Unquoted YAML scalar module paths not captured in signature

Severity: IMPORTANT · Confidence: CONFIRMED

collect_module_files in markdown-format.sh:428

The string-literal extractor used throughout collect_module_files is:

local quoted_string_re="\"[^\"]+\"|'[^']+'"
done < <(grep -oE "$quoted_string_re" "$item" 2>/dev/null)

This matches only double- or single-quoted strings. An unquoted YAML scalar is valid YAML and is a common authoring pattern:

# .markdownlint-cli2.yaml
customRules:
  - ./rules/local.cjs

grep -oE "\"[^\"]+\"|'[^']+' on this file returns no matches. ./rules/local.cjs is not in MODULE_FILES. The approval signature covers only the YAML config text. Changing local.cjs after approval doesn't revoke it — the signature is unchanged and the module executes on the next markdown edit.

The path IS detected as risky (the customRules tier-1 check adds the YAML file to RISK_CONFIGS), but the referenced module escapes the signature because only the config file's OWN text is hashed, not the content of the unquoted module it names.

Fix: Extend the string extractor for .yaml / .yml files to also capture unquoted YAML scalars that look like relative paths (e.g., with a heuristic like [[:space:]-]+([./][^[:space:]#]+) to capture list entries that are relative paths), OR set RISK_UNVERIFIABLE=1 when a YAML file contains a module-loading key and the scan finds no quoted module paths — treating unquoted-path YAML configs as unprovably safe, consistent with the Tier 2 strategy.

Fix this →


SUGGESTION — using assembly with repository-relative path skipped

Severity: SUGGESTION · Confidence: PLAUSIBLE

$usingAsts loop in powershell-format.sh:419–421

The code comment at lines 411–413 states:

"Only the Module kind loads repository code; a using namespace or using assembly statement names no repository file and is left alone."

This is incorrect for a repository-relative path:

using assembly ./deps/CustomRuleHelpers.dll

PowerShell loads the DLL from that relative path, but the UsingStatementKind -ne Module check fires continue, skipping this entry entirely. CustomRuleHelpers.dll is not added to $allFiles and not included in the signature. If the DLL is replaced after approval, the changed assembly executes on the next PowerShell lint run.

Low exploitability in practice (DLL-backed PSSA custom rules are uncommon), but the code comment is factually wrong and the gap is real. The safest fix is to emit PSSA_TRUST UNPINNABLE for Assembly-kind using statements rather than silently skipping them.


What was checked and found clean

Area Status
F5 assignment forms — plain, append, nested-subscript ((\[.*\])?) (\[.*\])?\+?= greedy match correctly handles A[${#arr[@]}]=secret. Test at line 895 ✓
F3 — XSS via javascript:/data: href Schema + both sinks enforce isAllowedUrlScheme; escape() encodes " in attributes ✓
F6 — file:// UNC NTLM leak file: not in allowedSchemes; blocked at schema and sinks ✓
F4 SSRF — literal private IPv4 (RFC 1918, CGN, loopback, link-local, multicast) Full IANA special-purpose registry in isPrivateIPv4
F4 SSRF — IPv6 allowlist inversion (2000::/3) (g[0] & 0xe000) !== 0x2000 classifies everything outside global unicast as private ✓
F4 SSRF — RFC 8215 NAT64 64:ff9b:1::/48 Explicit g[2] === 0x0001 check ✓
F4 SSRF — 2001::/23, 2002::/16, 3fff::/20 Each enumerated check correct; 2001:db8::/32 has separate explicit check outside /23 range ✓
F4 SSRF — [4000::1] and unassigned space Allowlist inversion: 0x4000 & 0xe000 = 0x4000 ≠ 0x2000 → private ✓
F4 SSRF — DNS pre-resolution gate All A/AAAA records resolved; any non-global or unresolvable → skip; fail-closed ✓
F1 — markdown trust gate, extensionless CJS (./rules/local-rulelocal-rule.cjs) .cjs/.mjs/.js/.json/.node candidates tried in resolution loop ✓
F1 — computed path.join() / non-literal require arguments unpinnable_js_specifier detects path import and residue loader; refuses approval ✓
F1 — escaped JS specifiers ("./rules.js") Backslash-u/x/octal pattern in first grep → RISK_UNPINNABLE ✓
F1 — escaped JSONC keys ("customRules") Independent JSONC \\u tier sets RISK_UNVERIFIABLE regardless of literal tier ✓
F2 — PowerShell 7.0 / .NET 3.1 SHA256 floor Instance [SHA256]::Create() + ComputeHash + ToString("x2"); empty → UNVERIFIABLE ✓
F2 — $PSScriptRoot dot-source expansion expandKnownVars handles both $PSScriptRoot/ and ${PSScriptRoot}/ forms ✓
F2 — transitive rule dependency pinning Recursive scan bounded at 256/512; unreadable file → UNVERIFIABLE ✓
F2 — using module extensionless loads (Import-Module "$PSScriptRoot/MyModule") .psd1/.psm1/.ps1/.dll candidates + versioned-directory layout tried ✓
Pipeline-fed loaders (Get-Content ... | iex) PipelineElements.Count > 1 && not first element → UNPINNABLE ✓
Out-of-repository symlinks in module graph physical_path + $CONFIG_ROOT/* check → UNPINNABLE ✓
Symlink canonicalization failure [[ -L "$candidate" && "$resolved" == "$candidate" ]] → UNPINNABLE ✓
Trust-marker path traversal 40/64-char hex signatures; no separators possible ✓

Verdict

Three targeted fixes are needed before merge. The eval-wrapped JS loader and the constant-string iex body are both confirmed trust-gate bypasses; the unquoted YAML scalar path is a confirmed signature gap. All three are straightforward to close at the pattern level. The using assembly observation is low-severity and can be deferred if DLL-backed custom rules are considered out of scope.

…mbly loads

markdown-format: the collector read only QUOTED strings, so a YAML plain scalar
was invisible — `customRules: [./rules/local.cjs]` never put local.cjs in the
signature, and a config-only marker stayed valid while the rule file changed
underneath it. Path-shaped bare tokens are now harvested alongside quoted ones,
restricted to tokens carrying a `/` or `.` so an ordinary key name or rule id is
not tried as a path. The quoted harvest keeps its exact prior behavior; the two
streams are tagged so neither filter leaks into the other.

powershell-format: `using assembly <path>` loads a repository DLL exactly as
`using module` loads a .psm1, and the previous branch skipped every non-Module
using statement — a comment that claimed those "name no repository file", which
was true for namespace and type and wrong for assembly. Assembly paths are now
collected too. Both directions are closed: the parser reports an assembly it
cannot load as a parse error, which the gate already treats as unverifiable, so a
loadable assembly is pinned and an unloadable one refuses approval.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


PR Review: fix(security): patch six Claude Security scan findings (F1–F6)

Tasks

  • Load tools
  • Read latest commits and changed files at HEAD (98f367e)
  • Verify status of the two remaining IMPORTANT findings from prior round (eval-wrapped loader, constant iex body)
  • Check 98f367e commit — unquoted YAML paths + using-assembly loads
  • Verify test coverage for all new cases
  • Post comprehensive review

Reviewed HEAD commit 98f367e. The prior Codex round at 4b669c71a5 flagged three P1s: unquoted YAML scalar paths (markdown-format), using assembly DLL loads (powershell-format), and constant-string iex body (powershell-format), plus the eval-wrapped loader in markdown-format. The 98f367e commit addresses the first two; one confirmed bypass remains open.


Open IMPORTANT — Constant-body iex/Invoke-Expression still evades the PS trust gate

File: plugins/powershell-format/hooks/powershell-format.sh:387–406

When a custom rule contains iex '. "$PSScriptRoot/helper.ps1"', the AST walk (line 389–391) classifies the argument as a StringConstantExpressionAst and adds its .Value (. "$PSScriptRoot/helper.ps1") to $pending. The litPattern scan (line 435–445) independently extracts the same single-quoted literal, runs expandKnownVars which substitutes $PSScriptRoot with $curDir, and adds both the raw and expanded forms (e.g., . "/repo/rules/helper.ps1") to $pending.

Neither form survives path resolution: Join-Path $baseDir '. "$PSScriptRoot/helper.ps1"' produces a path whose head is the literal characters . " — no file matches Test-Path -LiteralPath. The expanded form . "/repo/rules/helper.ps1" also fails (Join-Path prepends the baseDir, producing a path with . " still as a head segment). helper.ps1 is absent from $allFiles and absent from the signature.

Consequence: approve a rule module containing this iex call, then change only helper.ps1. The signature is unchanged; the hook skips the gate and Invoke-ScriptAnalyzer loads the modified helper on the next PS edit.

The pipeline-fed guard (lines 380–386) covers Get-Content ... | iex but not the constant-body form.

Fix: emit PSSA_TRUST UNPINNABLE immediately when the loader is iex or Invoke-Expression and the argument is a StringConstantExpressionAst. The body is a PowerShell command string, not a path; resolving it as a path never works and silently omits the dependency. Failing closed is consistent with the existing strategy for pipeline-fed iex:

# After the pipeline check (line 386), before the CommandElements loop:
if ($cmdName -in @("iex", "Invoke-Expression")) {
    Write-Output "PSSA_TRUST UNPINNABLE"
    exit 6
}

Add a regression fixture: a rule containing iex '. "$PSScriptRoot/helper.ps1"' should gate with UNPINNABLE and refuse the approval hint.

Fix this →


Observation (non-blocking) — eval in markdown JS configs: simple literal cases now incidentally covered

File: plugins/markdown-format/hooks/markdown-format.sh:404–421

The prior security review at 4b669c71a5 flagged module.exports = eval('require("./real-config.cjs")') as a bypass: unpinnable_js_specifier doesn't detect eval, and the sed stripper removes the loader before the residue grep, so unpinnable_js_specifier returns false. The concern was that real-config.cjs would not be pinned in the signature.

The 98f367e commit's plain_token_re extraction (added for YAML scalars) also runs on all queue items including .cjs files. For the eval-wrapper config, grep -oE "[A-Za-z0-9_.@~/+-]+" extracts ./real-config.cjs as a plain token — it contains both . and /, passes the path-shape filter, and is tried as $dir/real-config.cjs. If that file exists, it enters MODULE_FILES and is included in the approval signature. Changing it after approval revokes the signature. This means the simple literal-path-in-eval case that the prior review identified is now incidentally covered by the plain token extraction.

What is still unhandled: eval with a computed or encoded path (e.g., eval(Buffer.from('<base64>','base64').toString())). The unpinnable_js_specifier patterns don't flag this (no process\., no \$\{, no path\.join), and no plain token matching a real path would be extracted from a base64 blob. These exotic forms remain a theoretical gap. Adding eval[[:space:]]*\( to unpinnable_js_specifier Tier 1 would close it at the cost of rejecting any JS config that calls eval — an extreme rarity in a linter config — which is the right tradeoff.

This is informational only; the most common real-world eval usage is now covered.


What was fixed in 98f367e and is clean at HEAD

Unquoted YAML module pathsmarkdown-format.sh:428–537

The plain_token_re harvest ([A-Za-z0-9_.@~/+-]+) runs alongside the quoted-string harvest on every file in the queue. Tokens without / or . are dropped, preventing ordinary YAML keys and rule identifiers from being tried as paths. For customRules: [./rules/local.cjs], the bare scalar ./rules/local.cjs is extracted, passes the shape filter, and resolves to the module file. Regression fixture: approve a YAML config naming an unquoted module path, mutate only that module, gate re-blocks (suite 82/82). ✓

using assembly DLL loadspowershell-format.sh:416–432

Assembly is now included alongside Module in $loadKinds, so using assembly ./deps/helper.dll paths are collected and resolved exactly like using module paths. The parser reports an unloadable assembly as a parse error, which the gate already treats as UNVERIFIABLE — both directions (loadable → pinned, unloadable → refuses approval) are covered. Regression fixture: using assembly ./deps/UsingAsm.dll with a non-DLL placeholder refuses approval (suite 65/65). ✓


Full finding matrix at HEAD

Finding Status Round
F1 markdown-format CWE-94 285a037
F2 powershell-format CWE-94 285a037
F3 ai-briefing XSS CWE-79 285a037
F4 ai-briefing SSRF CWE-918 285a037 + 9bfaec7
F5 hook-utils CWE-532 90d9ccb + ec17b15
F6 ai-briefing NTLM CWE-20 285a037
Subscript [^]]* → greedy .* ec17b15
Escaped JS specifiers \uXXXX in loader arg ec17b15
RFC 8215 64:ff9b:1::/48 NAT64 9bfaec7
2001::/23, 2002::/16, 3fff::/20, 5f00::/16 IPv6 9bfaec7 + 117ba32
IPv6 allowlist inversion — (g[0] & 0xe000) !== 0x2000 117ba32
Computed path.join() module paths (markdown) 9bfaec7
$PSScriptRoot relative dot-sources 9bfaec7
$PSScriptRootFoo variable-name prefix collision 0aba2b4
Path module alias (const { join } = require("path")) ff5fd80
Comment-separated loader require/*c*/(...) 117ba32
JSONC/YAML if-elif ordering (escape tier suppressed) 6552b0c
YAML plain scalars in keys 6552b0c
using module extensionless loads 6552b0c
Out-of-repository module targets (symlinks / ../) b2ca7f1
Pipeline-fed iex (Get-Content ... | iex) b2ca7f1
Symlink canonicalization degradation 0641962
PS 7.0 / .NET 3.1 SHA256 floor 90d9ccb
Bash 3.2 local -A compatibility 90d9ccb
Trust-marker path traversal All rounds
Unquoted YAML module paths 98f367e
using assembly DLL loads 98f367e
Constant-body iex bypass OPEN

Verdict

One targeted fix is needed before merge: the constant-body iex/Invoke-Expression case in powershell-format.sh (above). The fix is four lines of PowerShell after the existing pipeline guard. The eval-in-JS observation is non-blocking given the incidental coverage by plain token extraction.

@claude

claude Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

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


Security Review — PR #1097

Tasks

  • Load tools
  • Get PR diff at HEAD (98f367e)
  • Read changed files for the latest commits
  • Verify the three open P1s from the prior round are addressed (eval-wrapped loader, constant iex body, unquoted YAML scalars)
  • Check the using assembly suggestion resolution
  • Look for any new issues introduced by the latest commits
  • Post findings

Reviewed HEAD commit 98f367e. The 98f367e commit addresses unquoted YAML module paths and using assembly collection. Two IMPORTANT/CONFIRMED findings from the prior round remain open at HEAD.


IMPORTANT — eval-wrapped JS loader still bypasses the markdown trust gate

Confidence: CONFIRMED

unpinnable_js_specifier in markdown-format.sh:404–421 is unchanged since 4b669c7.

For a .cjs config containing module.exports = eval('require("./real-config.cjs")'):

  1. Tier 1 grep: no pattern covers eval — all nine checks (path imports, process., __dirname, template literals, concatenation, escape sequences) pass.
  2. sed stripper (line 419): matches require("./real-config.cjs") inside the eval string argument and replaces it with a space. Result: module.exports = eval(' )').
  3. Residue grep (line 420): no remaining require or import( token → returns false.
  4. unpinnable_js_specifier returns 1 — not unpinnable.

collect_module_files then extracts quoted strings from the original file. The outer single-quoted string 'require("./real-config.cjs")' is matched first; the inner "./real-config.cjs" is consumed by that outer match and never returned as a separate hit. The raw string require("./real-config.cjs") fails all path resolution attempts. real-config.cjs is not added to MODULE_FILES and not included in the approval signature.

Exploit path: approve a .cjs config that eval-requires another repo module; edit only the inner module; the approval marker remains valid and the changed code executes on the next markdown edit.

Fix: add -e '(^|[^A-Za-z0-9_])eval[[:space:]]*\(' to the Tier 1 grep in unpinnable_js_specifier (line 406). Any config using eval( is refused approval (fail-closed). Add a regression fixture in markdown-format.test.sh.

Fix this →.%20The%20residue%20grep%20finds%20no%20remaining%20loader%20token%20and%20returns%20false%20(not%20unpinnable).%20real-config.cjs%20is%20never%20added%20to%20MODULE_FILES.%20Fix%3A%20add%20-e%20'(^%7C%5B%5EA-Za-z0-9_%5D)eval%5B%5B%3Aspace%3A%5D%5D*%5C('%20to%20the%20Tier%201%20grep%20at%20lines%20406-416%20so%20any%20file%20containing%20eval(%20is%20classified%20RISK_UNPINNABLE.%20Add%20a%20regression%20fixture%20in%20markdown-format.test.sh.&repo=melodic-software/claude-code-plugins)


IMPORTANT — Constant iex body with dot-source still bypasses the PowerShell trust gate

Confidence: CONFIRMED

CommandElements loop in powershell-format.sh:387–406 is unchanged since 4b669c7.

For a custom rule module containing iex '. "$PSScriptRoot/helper.ps1"':

  1. iex is in $loaders. The pipeline check passes (this is not pipeline-fed).
  2. $cmdAst.CommandElements[1] is the single-quoted string '. "$PSScriptRoot/helper.ps1"' — a StringConstantExpressionAst. Its .Value = . "$PSScriptRoot/helper.ps1" (literal content between the outer single-quotes, where $PSScriptRoot is a literal dollar sign, not a PowerShell variable).
  3. Line 390: [void]$pending.Add($el.Value). "$PSScriptRoot/helper.ps1" is added to $pending.
  4. The text scan ($litPattern) on the same file matches the outer '...' first; the inner "$PSScriptRoot/helper.ps1" is consumed inside that match and NOT extracted separately.
  5. expandKnownVars on . "$PSScriptRoot/helper.ps1". "/curDir/helper.ps1" (replaces $PSScriptRoot but leaves the . " prefix and " suffix).
  6. [System.IO.Path]::IsPathRooted('. "/curDir/helper.ps1"') → false (starts with . ").
  7. Join-Path $baseDir '. "/curDir/helper.ps1"' → nonsense path that does not exist.

helper.ps1 is not added to $allFiles and not in the approval signature. After approval, modifying helper.ps1 executes the changed code without revocation.

Note: the existing $PSScriptRoot expansion test at line 494 uses . "$PSScriptRoot/helper2.ps1" directly as a double-quoted string (an ExpandableStringExpressionAst), which the code handles correctly. The iex-wrapped form is a different AST shape and is not covered by any test fixture.

Fix: emit PSSA_TRUST UNPINNABLE for any iex/Invoke-Expression command whose argument is a StringConstantExpressionAst — the value is PS script code, not a path, and cannot be statically traced:

if ($cmdName -in @("iex", "Invoke-Expression") -and
    $el -is [System.Management.Automation.Language.StringConstantExpressionAst]) {
    Write-Output "PSSA_TRUST UNPINNABLE"
    exit 6
}

Place this check at the top of the foreach ($el in $cmdAst.CommandElements) loop (before the current CommandParameterAst check), scoped to the iex/Invoke-Expression command name. Add a regression fixture.

Fix this →


What was fixed in 98f367e and is clean at HEAD

Unquoted YAML module pathsmarkdown-format.sh:436, 458–539

Plain token stream (P-tagged) harvests unquoted YAML scalars. Filtered by / or . so bare key names and rule IDs are not tried as paths. The out-of-repository boundary check (lines 504–519) causes any token resolving outside $CONFIG_ROOT to set RISK_UNPINNABLE=1 — path-escape attempts via ../ or out-of-tree symlinks fail closed rather than being silently included or silently skipped. Regression test at line 968 confirms: approve a config with a YAML plain scalar module path, change only the module, approval is revoked. ✓

using assembly trackingpowershell-format.sh:415–432

Assembly is now in $loadKinds alongside Module. For a loadable assembly the path is added to $pending and resolved/pinned like any other dependency; for an unloadable assembly the PowerShell parser reports a parse error, which the gate already treats as UNVERIFIABLE → no approval route. The test at line 621 confirms the unloadable direction; the loadable direction is pinned through the same resolution loop that handles using module. ✓


Full prior-round finding matrix (not re-examined — last verified at ebdfb77/c7aefd7, confirmed unchanged for security-relevant files)

Finding Round closed Status
F3 XSS via javascript:/data: href 285a037 ✓ Scheme allowlist at schema + sinks
F4 SSRF literal private hosts (full IANA) 285a037+9bfaec7+117ba32 ✓ IPv4 full registry, IPv6 allowlist inversion
F5 hook-utils assignment regex (nested subscripts) ec17b15 ✓ Greedy (\[.*\])?; fixture at test:895
F6 file:// NTLM leak 285a037 file: not in allowlist
RFC 8215 64:ff9b:1::/48 NAT64 9bfaec7 ✓ Explicit g[2]===0x0001 check
2001::/23, 2002::/16, 5f00::/16 IPv6 9bfaec7+117ba32 ✓ Allowlist inversion covers all
Escaped JS specifiers ec17b15 ✓ Backslash-u/x/octal → UNPINNABLE
Computed path.join() module paths 9bfaec7 ✓ Non-literal require → UNPINNABLE
$PSScriptRoot double-quoted dot-sources 9bfaec7 ✓ Expansion + resolution correct
Pipeline-fed iex b2ca7f1 ✓ Pipeline position check → UNPINNABLE
Out-of-repository module targets b2ca7f1 CONFIG_ROOT boundary check
Symlink canonicalization failure 0641962 ✓ Lexical-path degradation → UNPINNABLE
PS 7.0 / .NET 3.1 SHA256 floor 90d9ccb ✓ Instance API; empty → UNVERIFIABLE
using module extensionless loads 6552b0c .psd1/.psm1/.ps1/.dll candidates
Bash 3.2 compatibility 90d9ccb local -A replaced
Trust-marker path traversal Hex-only signatures

Verdict

Two targeted fixes are needed before merge, both unchanged since the prior round. The eval-wrapped loader (#1) and the constant iex body (#2) are confirmed trust-gate bypasses; both allow code that executes on the next lint invocation to change after the approval is granted, without revoking it. Each fix is a few lines — add eval( to Tier 1's grep patterns, and emit UNPINNABLE for iex/Invoke-Expression with a string-constant argument.

The 98f367e changes (unquoted YAML paths, using assembly) are correct and introduce no new security issues.

@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: 98f367e1e4

ℹ️ 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 thread plugins/markdown-format/hooks/markdown-format.sh
Comment thread plugins/powershell-format/hooks/powershell-format.sh
@kyle-sexton
kyle-sexton merged commit d3df460 into main Jul 26, 2026
27 checks passed
@kyle-sexton
kyle-sexton deleted the feat/security-hardening-scan-findings branch July 26, 2026 05:36
kyle-sexton added a commit that referenced this pull request Jul 26, 2026
…sed to 0.16.4)

#1097 merged first and took guardrails 0.16.3 — the version this branch was
claiming — so the version collided again and had to be recomposed.

Conflict, composed rather than side-picked:

- plugins/guardrails/CHANGELOG.md — the same trap as the previous merge: both
  sides had created a `## [0.16.3]` / `### Fixed` heading pair, so git matched
  those two lines as COMMON CONTEXT and presented only the bullets as
  conflicting. Resolving the bullets in place would have silently merged this
  branch's entries into main's already-released 0.16.3 section with no marker to
  notice. This branch's entries move to a new `## [0.16.4]` above main's
  `## [0.16.3]`, whose bullets are kept verbatim.
- plugins/guardrails/.claude-plugin/plugin.json — 0.16.3 -> 0.16.4.

main's hook-utils.sh change (telemetry-subject assignment-value leak, F1-F6) is
unrelated to this branch's guards and needed no composition. Verified after
resolving: no conflict markers, no `## ` section from main dropped, only
`## [0.16.4]` added, markdownlint clean, changelog-parity --check-bump and
cross-plugin-source-drift green, shellcheck clean on the merged hook-utils.sh.
kyle-sexton added a commit that referenced this pull request Jul 26, 2026
This reverts commit f3c221d.

Following a `cd` correctly means implementing a shell interpreter inside a static
guard. Within an hour of that commit, four findings landed against the machinery
it added — control-flow reachability (`false && cd x` must not relocate), wrapped
builtins (`command cd`), subshell scoping (`(cd x; true)`), and explicit-global
replay — which are not four bugs but the first four items of shell semantics.
Quoting, `eval`, `$(…)`, `trap` and `set -e` are next.

Two of those were regressions against `main`, measured with real-git ground truth
(want=2 means the guard must block; branch vs origin/main's guard):

    F1 unreachable cd (false &&)        real=commits  want=2  branch=0  main=2
    F4 cd inside a subshell             real=commits  want=2  branch=0  main=2
    F3 --git-dir/--work-tree canonical  real=no-op    want=0  branch=2  main=0

Moving the base on an unreachable or subshell-scoped `cd` sent the analysis into
the child, where the alias was harmless, so the guard allowed — while `main`,
which never moves the base, resolved the outer alias and blocked correctly. F3
refused a VALID canonical commit. A half-interpreted shell is worse than a
documented gap: it produced a guard weaker than the one it replaced on two shapes
and a false positive on a third.

The relocation gap returns to being documented rather than half-closed, and is
tracked in #1486 together with this commit and the four findings, which are a map
of what a real fix must handle.

Kept from 8984d93: the invocation-prefix slice and the git-resolved
`repo_identity` primitive, neither of which is implicated. Also kept the 0.16.4
version composed when #1097 took 0.16.3.

Note for whoever re-runs the suite mid-revert: `git revert --no-commit` leaves
REVERT_HEAD, which `sequencer_in_progress` correctly treats as an in-progress
sequencer, so every commit-path fixture is exempted and reports 96 passed /
30 failed. That is #942, not this change.
kyle-sexton added a commit that referenced this pull request Jul 26, 2026
)

*This was generated by AI during work-loop execution.*

## Summary

- No CI gate covers shell portability: `shellcheck` lints syntax/style,
and `portability-lint`
(#531) matches skill-coupling tokens against changed *skill* files only.
A GNU-only construct —
like the `\brequire\b` word-boundary escape that nearly shipped a
fail-open security predicate in
`markdown-format.sh` (fixed on `main` via #1097) — passes both gates
silently on BSD userland
(macOS system `grep`/`sed`/`date`/`stat`/`mktemp`/`sort`), which no
runner in this repo's CI covers
(a Windows runner's Git Bash still ships GNU `grep`/`sed`, so it would
not help either).
- Adds `scripts/check-shell-portability.sh`, a changed-file-scoped gate
over `**/*.sh` mirroring
  `check-skill-portability.sh`'s shape: an external ERE token list
(`scripts/shell-portability-tokens.txt`), a same-line auto-guard for a
co-located BSD counterpart,
a per-site `portability-ok: <reason>` opt-out, and a whole-file
`portability-scope: <reason>`
declaration (used on the gate's own test file, which necessarily
contains the constructs it
  detects as fixture data).
- Wires a new `shell-portability-lint` job into `ci.yml` (self-test on
every push, diff-gated on
  pull requests) and adds it to the `ci-status` required-check list.
- **Active** classes today (zero real corpus impact, or auto-guarded):
the regex-escape family
(`\b \< \> \s \S \w \W`), `grep -P`/`--perl-regexp`, `echo -e`, `sort
-V`, unsuffixed `sed -i`, and
`readlink -f` (guarded when a `realpath` attempt sits on the same line —
the shape
`lib/hook-utils.sh` already uses). All four flag-based classes (`grep
-P`, `sort -V`, `echo -e`,
plus `sed -i`) match the target letter anywhere in a combined
short-option cluster (`-Pn`, `-Vr`,
`-ne`), not only as the cluster's last letter, and `sed -i`'s portable
BSD-safe empty-suffix idiom
  (`-i ''` / `-i ""`) is auto-guarded rather than flagged.
- **Staged** (commented, inactive) classes: `date -d`, `stat -c`,
`mktemp -p`. A corpus survey during
this change found real, already-legitimate uses (a cross-statement
GNU-then-BSD dialect function in
`morning-brief.sh`; ~20 shared test-scaffolding `mktemp -p` sites with
no BSD counterpart) that the
same-line auto-guard doesn't yet cover — enabling them is tracked in the
follow-up below, the same
staged-rollout posture `scripts/skill-portability-tokens.txt` already
documents for its own classes.

## Triage note

#1491's triage marked the token-list-vs-BSD-container design fork as
decision-defaulted (token list,
vetoable) and separately delegated "the starter token list's exact
membership" to the implementer as
reversible/low-stakes. The ACTIVE/STAGED split above is that delegated,
reversible call, made from an
actual corpus survey rather than guesswork — not a second judgment call
requiring escalation.

## Review response

An automated Codex review left 6 findings. Two risked flagging the
CORRECT portable form and were
fixed directly (the combined-short-option-cluster gap on `grep -P`/`sort
-V`/`echo -e`, and the
`sed -i ''`/`sed -i ""` empty-suffix idiom being wrongly flagged) plus a
guard-scoping tightening (the
`realpath` auto-guard now applies only to the readlink pattern match,
not the whole line). The
remaining three lower-severity findings (additional `sed -i` spellings,
`portability-scope:`
substring-match precision — shared with the sibling gate, not unique to
this PR — and an `awk`
operand edge case on a pathological filename) are deferred to #1513. See
the threaded replies on each
finding for the per-finding classification.

Two further review rounds followed and the unresolved-thread count grew
6 to 11 without net decrease —
including one finding that asks to REVERSE the `sed -i ''` auto-guard
added in response to round one.
Per this repo's convergence posture, the fix loop is cut off here: the
five new findings are grouped
and deferred to #1517 with per-item re-open triggers, and each thread
carries the reasoning. None is a
defect in the shipped behavior — four are false-negative detection gaps
(before this gate they all
passed silently), and the one false positive is the token file's own
documented over-flag posture,
which ships a per-site `portability-ok:` opt-out. Absorbing them would
re-widen the change and
invalidate the corpus survey the ACTIVE/STAGED split rests on.

## Test plan

- [x] `bash scripts/check-shell-portability.test.sh` — 35/35 passing,
including: the literal `\b`
token actually fires (verified against the real awk resolved in this
environment, gawk 5.4.0 —
not assumed; this is a distinct, POSIX-fundamental escape from the
sibling token list's
documented `\b`-as-boundary-anchor pitfall, which this gate does not
use), each of
`\< \> \s \S \w \W`, `grep -P`/`-riP`/`-Pn` (and that a comment merely
naming `grep -P` does not
fire), `echo -e`/`-ne`, `sort -V`/`-Vr`, unsuffixed `sed -i` vs. `sed
-i.bak` vs. the guarded
`sed -i ''`/`-i ""`, `readlink -f` bare vs. `realpath`-guarded (and that
the guard does not leak
to an unrelated token on the same line),
same-line/comment-block-above/leak-boundary
`portability-ok:` annotation behavior, the whole-file
`portability-scope:` declaration,
fail-closed behavior (malformed token, missing token file, invalid base
ref), `--all` scope
exclusion, a Git-quoted non-ASCII changed path, and — against the real
corpus — that the
shipped list does not flag `markdown-format.sh`'s known-good reference
implementation and that
      the staged classes stay inactive.
- [x] `scripts/check-shell-portability.sh origin/main` run directly
against this PR's own diff — the
      new gate's own source files (2 shell files in scope) pass clean.
- [x] `shellcheck --rcfile=.shellcheckrc` on both new scripts — clean.
- [x] `actionlint .github/workflows/ci.yml` — clean.
- [x] `bash scripts/check-skill-portability.test.sh` (sibling gate)
still passes — no cross-gate
      regression.
- [x] Full CI run green, including the new `shell-portability-lint` job
and the required `ci-status`
      aggregate.

## Related

Closes #1491. Follow-ups: #1510 (enabling the staged classes), #1513
(detection-precision findings
from review round 1), #1517 (detection-precision findings from review
rounds 2-3).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant