fix(security): patch six Claude Security scan findings (F1–F6)#1097
fix(security): patch six Claude Security scan findings (F1–F6)#1097kyle-sexton wants to merge 4 commits into
Conversation
… 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 encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| @@ -247,22 +255,42 @@ claim_trust_advisory() { | |||
| printf '%s\t%s\n' "$config" "$digest" | |||
There was a problem hiding this comment.
Once a user approves a risky markdownlint configuration, this signature hashes only the configuration files. If a referenced customRules, markdownItPlugins, outputFormatters, or a module required by an approved .cjs/.mjs config changes while the config itself stays unchanged (for example, after switching branches), the existing marker remains valid and the next Markdown edit executes the changed repository-supplied module without a new approval. Include the resolved code-loading inputs in the approval state, or revoke approval when they cannot be tracked.
Useful? React with 👍 / 👎.
| printf '%s\n' "$REPO_ROOT" | ||
| digest=$(git hash-object "$SETTINGS_FOUND" 2>/dev/null) || return 1 | ||
| printf '%s\t%s\n' "$SETTINGS_FOUND" "$digest" | ||
| } | git hash-object --stdin 2>/dev/null |
There was a problem hiding this comment.
The approval signature covers only PSScriptAnalyzerSettings.psd1, not the modules named by its CustomRulePath. Therefore, after approving an unchanged settings file, a later change to any referenced rule module leaves PSSA_APPROVED=1; the hook skips the gate and Invoke-ScriptAnalyzer loads and executes that changed module on the next PowerShell edit. Track the referenced rule contents in the approval state, or require approval again whenever they cannot be verified unchanged.
Useful? React with 👍 / 👎.
kyle-sexton
added
the
needs-human
1 participant
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.
.cjs/.mjsconfig as Node code on any markdown edit (CWE-94)CustomRulePathmodule as PowerShell on any.ps1edit (CWE-94)javascript:/data:source URL reached a deck anchorhrefunfiltered — stored XSS on click (CWE-79)lib/hook-utils.shNAME=valueBash command leaked its value into the telemetry/audit subject (CWE-532)file://UNC URL embedded as a PPTX hyperlink — NTLM leak on click (CWE-20)Fix families:
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 configchange revokes it; fails closed when the plugin-data dir is unavailable. The
edit itself is never blocked.
lib/url-policy.js: anhttp/https/mailto/telscheme allowlistenforced at schema time and at every href/hyperlink sink, plus private/
loopback/link-local/reserved literal-host filtering in the reachability gate.
hook::extract_bash_subjectnow bails a resolved assignment-shapedtoken to the bare
Bashsubject; synced to all per-plugin copies viascripts/sync-hook-utils.sh, which is why this PR bumps every carryingplugin (the coupled
sync-hook-utils/changelog-paritygates).Deliberate, documented behaviour changes: ai-briefing now rejects
non-allowlisted URL schemes (
javascript:/data:/file:and rarer ones likeftp:); a repo whose linter config can execute code is no longer auto-rununtil 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 matchbash scripts/sync-hook-utils.sh --check-bump origin/main→ passbash scripts/check-changelog-parity.sh --checkand--check-bump origin/main→ passbash scripts/validate-plugins.shandnode scripts/generate-catalog.mjs --check→ passbash scripts/check-silent-skips.sh,check-cross-plugin-source-drift.sh,check-changed-skills.sh origin/main,check-skill-portability.sh origin/main→ passshellcheck -x lib/hook-utils.sh plugins/markdown-format/hooks/markdown-format.sh plugins/powershell-format/hooks/powershell-format.sh→ cleannode --test27/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.