diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index 30e2a0bfb6..6d1bd965ef 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -50,7 +50,7 @@ { "name": "explanatory-output-style", "description": "Adds educational insights about implementation choices and codebase patterns (mimics the deprecated Explanatory output style)", - "version": "1.0.0", + "version": "1.0.1", "author": { "name": "Dickson Tsai", "email": "dickson@anthropic.com" @@ -94,7 +94,7 @@ { "name": "learning-output-style", "description": "Interactive learning mode that requests meaningful code contributions at decision points (mimics the unshipped Learning output style)", - "version": "1.0.0", + "version": "1.0.1", "author": { "name": "Boris Cherny", "email": "boris@anthropic.com" @@ -127,7 +127,7 @@ { "name": "ralph-wiggum", "description": "Interactive self-referential AI loops for iterative development. Claude works on the same task repeatedly, seeing its previous work, until completion.", - "version": "1.0.0", + "version": "1.0.1", "author": { "name": "Daisy Hollman", "email": "daisy@anthropic.com" diff --git a/.devcontainer/Dockerfile b/.devcontainer/Dockerfile index 8b48f6ad72..1037516d81 100644 --- a/.devcontainer/Dockerfile +++ b/.devcontainer/Dockerfile @@ -6,7 +6,7 @@ ENV TZ="$TZ" ARG CLAUDE_CODE_VERSION=latest # Install basic development tools and iptables/ipset -RUN apt-get update && apt-get install -y --no-install-recommends \ +RUN apt-get update && apt-get install poppler-utils -y --no-install-recommends \ less \ git \ procps \ diff --git a/.devcontainer/init-firewall.sh b/.devcontainer/init-firewall.sh index 361d2aac65..288f8dcb8c 100644 --- a/.devcontainer/init-firewall.sh +++ b/.devcontainer/init-firewall.sh @@ -13,6 +13,22 @@ iptables -t nat -X iptables -t mangle -F iptables -t mangle -X ipset destroy allowed-domains 2>/dev/null || true +ipset destroy allowed-domains-v6 2>/dev/null || true + +# Detect IPv6 support. Without matching ip6tables rules, dual-stack networks +# let IPv6 egress bypass the allowlist below entirely. On hosts where IPv6 is +# disabled (e.g. ipv6.disable=1), ip6tables cannot operate - there is no IPv6 +# traffic to filter, so skip IPv6 rules rather than fail container startup. +if ip6tables -L -n >/dev/null 2>&1; then + IPV6_ENABLED=true + ip6tables -F + ip6tables -X + ip6tables -t mangle -F 2>/dev/null || true + ip6tables -t mangle -X 2>/dev/null || true +else + IPV6_ENABLED=false + echo "WARNING: ip6tables unavailable - skipping IPv6 firewall rules" +fi # 2. Selectively restore ONLY internal Docker DNS resolution if [ -n "$DOCKER_DNS_RULES" ]; then @@ -37,8 +53,21 @@ iptables -A INPUT -p tcp --sport 22 -m state --state ESTABLISHED -j ACCEPT iptables -A INPUT -i lo -j ACCEPT iptables -A OUTPUT -o lo -j ACCEPT +# Same DNS/SSH/localhost allowances for IPv6 +if [ "$IPV6_ENABLED" = true ]; then + ip6tables -A OUTPUT -p udp --dport 53 -j ACCEPT + ip6tables -A INPUT -p udp --sport 53 -j ACCEPT + ip6tables -A OUTPUT -p tcp --dport 22 -j ACCEPT + ip6tables -A INPUT -p tcp --sport 22 -m state --state ESTABLISHED -j ACCEPT + ip6tables -A INPUT -i lo -j ACCEPT + ip6tables -A OUTPUT -o lo -j ACCEPT +fi + # Create ipset with CIDR support ipset create allowed-domains hash:net +if [ "$IPV6_ENABLED" = true ]; then + ipset create allowed-domains-v6 hash:net family inet6 +fi # Fetch GitHub meta information and aggregate + add their IP ranges echo "Fetching GitHub IP ranges..." @@ -61,34 +90,105 @@ while read -r cidr; do fi echo "Adding GitHub range $cidr" ipset add allowed-domains "$cidr" -done < <(echo "$gh_ranges" | jq -r '(.web + .api + .git)[]' | aggregate -q) - -# Resolve and add other allowed domains -for domain in \ - "registry.npmjs.org" \ - "api.anthropic.com" \ - "sentry.io" \ - "statsig.com" \ - "marketplace.visualstudio.com" \ - "vscode.blob.core.windows.net" \ - "update.code.visualstudio.com"; do +done < <(echo "$gh_ranges" | jq -r '(.web + .api + .git)[]' | grep -v ':' | aggregate -q) + +if [ "$IPV6_ENABLED" = true ]; then + echo "Processing GitHub IPv6 ranges..." + while read -r cidr; do + if [[ ! "$cidr" =~ ^[0-9a-fA-F:]+/[0-9]{1,3}$ ]]; then + echo "ERROR: Invalid IPv6 CIDR range from GitHub meta: $cidr" + exit 1 + fi + echo "Adding GitHub IPv6 range $cidr" + ipset add allowed-domains-v6 "$cidr" + done < <(echo "$gh_ranges" | jq -r '(.web + .api + .git)[]' | grep ':' | sort -u) +fi + +# Resolve and add other allowed domains. +# +# Domains listed in REQUIRED_DOMAINS must resolve - without them the container +# cannot do its job, so failing loudly is correct. Everything else is +# best-effort: telemetry and marketplace endpoints come and go, and a single +# NXDOMAIN there used to abort the whole script and leave the container +# unusable. Those are now skipped with a warning. +REQUIRED_DOMAINS=( + "api.anthropic.com" + "registry.npmjs.org" +) + +OPTIONAL_DOMAINS=( + "sentry.io" + "statsig.com" + "marketplace.visualstudio.com" + "vscode.blob.core.windows.net" + "update.code.visualstudio.com" +) + +skipped_domains=() + +for domain in "${REQUIRED_DOMAINS[@]}" "${OPTIONAL_DOMAINS[@]}"; do + required=false + for req in "${REQUIRED_DOMAINS[@]}"; do + if [ "$domain" = "$req" ]; then + required=true + break + fi + done + echo "Resolving $domain..." - ips=$(dig +noall +answer A "$domain" | awk '$4 == "A" {print $5}') + ips=$(dig +noall +answer A "$domain" | awk '$4 == "A" {print $5}' || true) if [ -z "$ips" ]; then - echo "ERROR: Failed to resolve $domain" - exit 1 + if [ "$required" = true ]; then + echo "ERROR: Failed to resolve required domain $domain" + exit 1 + fi + echo "WARNING: Failed to resolve $domain - skipping (not required)" + skipped_domains+=("$domain") + continue fi - + while read -r ip; do if [[ ! "$ip" =~ ^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}$ ]]; then - echo "ERROR: Invalid IP from DNS for $domain: $ip" - exit 1 + if [ "$required" = true ]; then + echo "ERROR: Invalid IP from DNS for $domain: $ip" + exit 1 + fi + echo "WARNING: Invalid IP from DNS for $domain: $ip - skipping" + continue fi echo "Adding $ip for $domain" ipset add allowed-domains "$ip" done < <(echo "$ips") + + # Also add AAAA records so allowed domains work first-class over IPv6. + # A missing AAAA record is not an error: IPv6 attempts are rejected fast + # below and clients fall back to IPv4. + if [ "$IPV6_ENABLED" = true ]; then + ipv6s=$(dig +noall +answer AAAA "$domain" | awk '$4 == "AAAA" {print $5}' || true) + while read -r ip; do + if [ -n "$ip" ]; then + if [[ ! "$ip" =~ ^[0-9a-fA-F:]+$ ]]; then + if [ "$required" = true ]; then + echo "ERROR: Invalid IPv6 from DNS for $domain: $ip" + exit 1 + fi + echo "WARNING: Invalid IPv6 from DNS for $domain: $ip - skipping" + continue + fi + echo "Adding $ip for $domain (IPv6)" + ipset add allowed-domains-v6 "$ip" + fi + done < <(echo "$ipv6s") + fi done +if [ ${#skipped_domains[@]} -gt 0 ]; then + echo "NOTE: ${#skipped_domains[@]} optional domain(s) could not be resolved and were not allowlisted:" + for domain in "${skipped_domains[@]}"; do + echo " - $domain" + done +fi + # Get host IP from default route HOST_IP=$(ip route | grep default | cut -d" " -f3) if [ -z "$HOST_IP" ]; then @@ -118,6 +218,32 @@ iptables -A OUTPUT -m set --match-set allowed-domains dst -j ACCEPT # Explicitly REJECT all other outbound traffic for immediate feedback iptables -A OUTPUT -j REJECT --reject-with icmp-admin-prohibited +# IPv6: same default-deny posture, so IPv6 cannot bypass the IPv4 allowlist +if [ "$IPV6_ENABLED" = true ]; then + # Link-local and ICMPv6 are required for neighbor discovery; without + # them IPv6 breaks entirely, even for allowed destinations + ip6tables -A INPUT -s fe80::/10 -j ACCEPT + ip6tables -A OUTPUT -d fe80::/10 -j ACCEPT + ip6tables -A INPUT -p ipv6-icmp -j ACCEPT + ip6tables -A OUTPUT -p ipv6-icmp -j ACCEPT + + # Set default policies to DROP + ip6tables -P INPUT DROP + ip6tables -P FORWARD DROP + ip6tables -P OUTPUT DROP + + # Allow established connections for already approved traffic + ip6tables -A INPUT -m state --state ESTABLISHED,RELATED -j ACCEPT + ip6tables -A OUTPUT -m state --state ESTABLISHED,RELATED -j ACCEPT + + # Then allow only specific outbound traffic to allowed domains + ip6tables -A OUTPUT -m set --match-set allowed-domains-v6 dst -j ACCEPT + + # REJECT (not DROP) so blocked IPv6 attempts fail fast and clients + # fall back to IPv4 instead of hanging + ip6tables -A OUTPUT -j REJECT --reject-with icmp6-adm-prohibited +fi + echo "Firewall configuration complete" echo "Verifying firewall rules..." if curl --connect-timeout 5 https://example.com >/dev/null 2>&1; then @@ -127,6 +253,17 @@ else echo "Firewall verification passed - unable to reach https://example.com as expected" fi +# Verify the block also holds over IPv6 (in IPv4-only environments curl -6 +# cannot connect at all, so this check passes there too) +if [ "$IPV6_ENABLED" = true ]; then + if curl -6 --connect-timeout 5 https://example.com >/dev/null 2>&1; then + echo "ERROR: Firewall verification failed - was able to reach https://example.com over IPv6" + exit 1 + else + echo "Firewall verification passed - unable to reach https://example.com over IPv6 as expected" + fi +fi + # Verify GitHub API access if ! curl --connect-timeout 5 https://api.github.com/zen >/dev/null 2>&1; then echo "ERROR: Firewall verification failed - unable to reach https://api.github.com" diff --git a/.github/workflows/log-issue-events.yml b/.github/workflows/log-issue-events.yml index f23d4fbe98..c117137d2e 100644 --- a/.github/workflows/log-issue-events.yml +++ b/.github/workflows/log-issue-events.yml @@ -10,14 +10,16 @@ jobs: permissions: issues: read steps: - - name: Log issue creation to Statsig + - name: Log issue event to Statsig env: STATSIG_API_KEY: ${{ secrets.STATSIG_API_KEY }} + EVENT_NAME: ${{ github.event.action == 'closed' && 'github_issue_closed' || 'github_issue_created' }} ISSUE_NUMBER: ${{ github.event.issue.number }} REPO: ${{ github.repository }} ISSUE_TITLE: ${{ github.event.issue.title }} AUTHOR: ${{ github.event.issue.user.login }} CREATED_AT: ${{ github.event.issue.created_at }} + CLOSED_AT: ${{ github.event.issue.closed_at }} run: | # All values are now safely passed via environment variables # No direct templating in the shell script to prevent injection attacks @@ -27,14 +29,15 @@ jobs: -H "statsig-api-key: $STATSIG_API_KEY" \ -d '{ "events": [{ - "eventName": "github_issue_created", + "eventName": "'"$EVENT_NAME"'", "metadata": { "issue_number": "'"$ISSUE_NUMBER"'", "repository": "'"$REPO"'", "title": "'"$(echo "$ISSUE_TITLE" | sed "s/\"/\\\\\"/g")"'", "author": "'"$AUTHOR"'", - "created_at": "'"$CREATED_AT"'" + "created_at": "'"$CREATED_AT"'", + "closed_at": "'"$CLOSED_AT"'" }, "time": '"$(date +%s)000"' }] - }' \ No newline at end of file + }' diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cb306befa..83fea05573 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7383,7 +7383,7 @@ ## 1.0.124 -- Set `CLAUDE_BASH_NO_LOGIN` environment variable to 1 or true to to skip login shell for BashTool +- Set `CLAUDE_BASH_NO_LOGIN` environment variable to 1 or true to skip login shell for BashTool - Fix Bedrock and Vertex environment variables evaluating all strings as truthy - No longer inform Claude of the list of allowed tools when permission is denied - Fixed security vulnerability in Bash tool permission checks diff --git a/examples/gateway/aws/setup.sh b/examples/gateway/aws/setup.sh index 6c8ce85b49..2208ed2e85 100755 --- a/examples/gateway/aws/setup.sh +++ b/examples/gateway/aws/setup.sh @@ -63,7 +63,8 @@ DOCKERFILE="${DOCKERFILE:-./Dockerfile}" CLAUDE_BINARY="${CLAUDE_BINARY:-./claude}" # prebuilt linux-x64 Claude Code release binary (includes the gateway subcommand) DIST_URL="${DIST_URL:-}" # optional: download URL, used only if $CLAUDE_BINARY is missing DIST_SHA256="${DIST_SHA256:-}" # REQUIRED with DIST_URL: expected sha256 of the binary (verified fail-closed) -DIST_SHA256="${DIST_SHA256,,}" # normalize to lowercase — openssl emits lowercase hex; some tools (PowerShell Get-FileHash) publish uppercase +# normalize to lowercase — openssl emits lowercase hex; some tools (PowerShell Get-FileHash) publish uppercase +DIST_SHA256="$(printf '%s' "${DIST_SHA256}" | LC_ALL=C tr '[:upper:]' '[:lower:]')" # Obtain DIST_SHA256 out-of-band — never from the server that serves DIST_URL. # For binaries from the standard Claude Code release channel, verify the # release's GPG-signed manifest.json and copy the platform checksum from it: diff --git a/examples/gateway/gcp/setup.sh b/examples/gateway/gcp/setup.sh index 782de6fd0d..b44a53de66 100755 --- a/examples/gateway/gcp/setup.sh +++ b/examples/gateway/gcp/setup.sh @@ -28,7 +28,7 @@ set -euo pipefail # ---- configuration (env-overridable) ---------------------------------------- -PROJECT_ID="${PROJECT_ID:-$(gcloud config get-value project 2>/dev/null)}" +PROJECT_ID="${PROJECT_ID:-$(gcloud config get-value project 2>/dev/null || true)}" REGION="${REGION:-${CLOUDSDK_COMPUTE_REGION:-us-east5}}" # guide §1 uses us-east5 (Agent Platform model region) SA_NAME="${SA_NAME:-claude-gateway}" # §2 service account diff --git a/examples/hooks/bash_command_validator_example.py b/examples/hooks/bash_command_validator_example.py index 53ab7a829e..a1c879a820 100644 --- a/examples/hooks/bash_command_validator_example.py +++ b/examples/hooks/bash_command_validator_example.py @@ -6,7 +6,7 @@ It validates bash commands against a set of rules before execution. In this case it changes grep calls to using rg. -Read more about hooks here: https://docs.anthropic.com/en/docs/claude-code/hooks +Read more about hooks here: https://code.claude.com/docs/en/hooks Make sure to change your path to your actual script. diff --git a/examples/settings/README.md b/examples/settings/README.md index 34e60cae78..36cd490f6e 100644 --- a/examples/settings/README.md +++ b/examples/settings/README.md @@ -25,6 +25,7 @@ These may be applied at any level of the [settings hierarchy](https://code.claud - Settings files must be valid JSON - Before deploying configuration files to your organization, test them locally by applying to `managed-settings.json`, `settings.json` or `settings.local.json` - The `sandbox` property only applies to the `Bash` tool; it does not apply to other tools (like Read, Write, WebSearch, WebFetch, MCPs), hooks, or internal commands +- The sandbox is available on macOS, Linux, and WSL2 only. When it cannot initialize (e.g. native Windows, or Linux hosts without bubblewrap), Claude Code warns and runs Bash commands unsandboxed unless `failIfUnavailable` is set to `true` — [`settings-bash-sandbox.json`](./settings-bash-sandbox.json) sets it so the sandbox requirement fails closed ## Deploying via MDM diff --git a/examples/settings/settings-bash-sandbox.json b/examples/settings/settings-bash-sandbox.json index 65d66dcf81..b69174e0fb 100644 --- a/examples/settings/settings-bash-sandbox.json +++ b/examples/settings/settings-bash-sandbox.json @@ -2,6 +2,7 @@ "allowManagedPermissionRulesOnly": true, "sandbox": { "enabled": true, + "failIfUnavailable": true, "autoAllowBashIfSandboxed": false, "allowUnsandboxedCommands": false, "excludedCommands": [], diff --git a/plugins/README.md b/plugins/README.md index cf4a21ecc5..e941977670 100644 --- a/plugins/README.md +++ b/plugins/README.md @@ -24,7 +24,7 @@ Learn more in the [official plugins documentation](https://docs.claude.com/en/do | [plugin-dev](./plugin-dev/) | Comprehensive toolkit for developing Claude Code plugins with 7 expert skills and AI-assisted creation | **Command:** `/plugin-dev:create-plugin` - 8-phase guided workflow for building plugins
**Agents:** `agent-creator`, `plugin-validator`, `skill-reviewer`
**Skills:** Hook development, MCP integration, plugin structure, settings, commands, agents, and skill development | | [pr-review-toolkit](./pr-review-toolkit/) | Comprehensive PR review agents specializing in comments, tests, error handling, type design, code quality, and code simplification | **Command:** `/pr-review-toolkit:review-pr` - Run with optional review aspects (comments, tests, errors, types, code, simplify, all)
**Agents:** `comment-analyzer`, `pr-test-analyzer`, `silent-failure-hunter`, `type-design-analyzer`, `code-reviewer`, `code-simplifier` | | [ralph-wiggum](./ralph-wiggum/) | Interactive self-referential AI loops for iterative development. Claude works on the same task repeatedly until completion | **Commands:** `/ralph-loop`, `/cancel-ralph` - Start/stop autonomous iteration loops
**Hook:** Stop - Intercepts exit attempts to continue iteration | -| [security-guidance](./security-guidance/) | Security reminder hook that warns about potential security issues when editing files | **Hook:** PreToolUse - Monitors 9 security patterns including command injection, XSS, eval usage, dangerous HTML, pickle deserialization, and os.system calls | +| [security-guidance](./security-guidance/) | Security reminder hook that warns about potential security issues when editing files | **Hook:** PostToolUse - Regex pattern warnings (~25 patterns) on Edit/Write/MultiEdit/NotebookEdit, plus an agentic multi-file review triggered by `git commit`/`git push`
**Hook:** Stop - LLM review of the full session diff | ## Installation diff --git a/plugins/commit-commands/commands/clean_gone.md b/plugins/commit-commands/commands/clean_gone.md index 57f0b6e3ea..cb999bcf08 100644 --- a/plugins/commit-commands/commands/clean_gone.md +++ b/plugins/commit-commands/commands/clean_gone.md @@ -25,15 +25,27 @@ You need to execute the following bash commands to clean up stale local branches 3. **Finally, remove worktrees and delete [gone] branches (handles both regular and worktree branches)** Execute this command: ```bash - # Process all [gone] branches, removing '+' prefix if present - git branch -v | grep '\[gone\]' | sed 's/^[+* ]//' | awk '{print $1}' | while read branch; do + # Process all branches whose upstream has been deleted + git for-each-ref --format='%(refname:short)%09%(upstream:track)' refs/heads | while IFS=$'\t' read -r branch tracking; do + [ "$tracking" = "[gone]" ] || continue echo "Processing branch: $branch" - # Find and remove worktree if it exists - worktree=$(git worktree list | grep "\\[$branch\\]" | awk '{print $1}') - if [ ! -z "$worktree" ] && [ "$worktree" != "$(git rev-parse --show-toplevel)" ]; then - echo " Removing worktree: $worktree" - git worktree remove --force "$worktree" - fi + # Find the associated worktree using Git's stable machine-readable format + root_worktree=$(git rev-parse --show-toplevel) + current_worktree= + while IFS= read -r -d '' field; do + case "$field" in + "worktree "*) + current_worktree=${field#worktree } + ;; + "branch refs/heads/$branch") + if [ -n "$current_worktree" ] && [ "$current_worktree" != "$root_worktree" ]; then + echo " Removing worktree: $current_worktree" + git worktree remove --force "$current_worktree" + fi + break + ;; + esac + done < <(git worktree list --porcelain -z) # Delete the branch echo " Deleting branch: $branch" git branch -D "$branch" @@ -50,4 +62,3 @@ After executing these commands, you will: - Provide feedback on which worktrees and branches were removed If no branches are marked as [gone], report that no cleanup was needed. - diff --git a/plugins/explanatory-output-style/.claude-plugin/plugin.json b/plugins/explanatory-output-style/.claude-plugin/plugin.json index a70cbf97c1..4b643de211 100644 --- a/plugins/explanatory-output-style/.claude-plugin/plugin.json +++ b/plugins/explanatory-output-style/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "explanatory-output-style", - "version": "1.0.0", + "version": "1.0.1", "description": "Adds educational insights about implementation choices and codebase patterns (mimics the deprecated Explanatory output style)", "author": { "name": "Dickson Tsai", diff --git a/plugins/explanatory-output-style/hooks/hooks.json b/plugins/explanatory-output-style/hooks/hooks.json index d1fb8a5734..2478dadf7e 100644 --- a/plugins/explanatory-output-style/hooks/hooks.json +++ b/plugins/explanatory-output-style/hooks/hooks.json @@ -6,7 +6,7 @@ "hooks": [ { "type": "command", - "command": "${CLAUDE_PLUGIN_ROOT}/hooks-handlers/session-start.sh" + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks-handlers/session-start.sh\"" } ] } diff --git a/plugins/hookify/skills/writing-rules/SKILL.md b/plugins/hookify/skills/writing-rules/SKILL.md index 008168a4c9..ea630dbd5b 100644 --- a/plugins/hookify/skills/writing-rules/SKILL.md +++ b/plugins/hookify/skills/writing-rules/SKILL.md @@ -1,5 +1,5 @@ --- -name: Writing Hookify Rules +name: writing-rules description: This skill should be used when the user asks to "create a hookify rule", "write a hook rule", "configure hookify", "add a hookify rule", or needs guidance on hookify rule syntax and patterns. version: 0.1.0 --- diff --git a/plugins/learning-output-style/.claude-plugin/plugin.json b/plugins/learning-output-style/.claude-plugin/plugin.json index 3f798c518c..506bf0f0e8 100644 --- a/plugins/learning-output-style/.claude-plugin/plugin.json +++ b/plugins/learning-output-style/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "learning-output-style", - "version": "1.0.0", + "version": "1.0.1", "description": "Interactive learning mode that requests meaningful code contributions at decision points (mimics the unshipped Learning output style)", "author": { "name": "Boris Cherny", diff --git a/plugins/learning-output-style/hooks/hooks.json b/plugins/learning-output-style/hooks/hooks.json index b3ab7ce9b7..7f66d5ec31 100644 --- a/plugins/learning-output-style/hooks/hooks.json +++ b/plugins/learning-output-style/hooks/hooks.json @@ -6,7 +6,7 @@ "hooks": [ { "type": "command", - "command": "${CLAUDE_PLUGIN_ROOT}/hooks-handlers/session-start.sh" + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks-handlers/session-start.sh\"" } ] } diff --git a/plugins/plugin-dev/skills/agent-development/SKILL.md b/plugins/plugin-dev/skills/agent-development/SKILL.md index 36830932de..ca0e183bee 100644 --- a/plugins/plugin-dev/skills/agent-development/SKILL.md +++ b/plugins/plugin-dev/skills/agent-development/SKILL.md @@ -1,5 +1,5 @@ --- -name: Agent Development +name: agent-development description: This skill should be used when the user asks to "create an agent", "add an agent", "write a subagent", "agent frontmatter", "when to use description", "agent examples", "agent tools", "agent colors", "autonomous agent", or needs guidance on agent structure, system prompts, triggering conditions, or agent development best practices for Claude Code plugins. version: 0.1.0 --- diff --git a/plugins/plugin-dev/skills/agent-development/scripts/validate-agent.sh b/plugins/plugin-dev/skills/agent-development/scripts/validate-agent.sh index ca4dfd4b92..5fa5b5a6f1 100755 --- a/plugins/plugin-dev/skills/agent-development/scripts/validate-agent.sh +++ b/plugins/plugin-dev/skills/agent-development/scripts/validate-agent.sh @@ -56,74 +56,82 @@ error_count=0 warning_count=0 # Check name field -NAME=$(echo "$FRONTMATTER" | grep '^name:' | sed 's/name: *//' | sed 's/^"\(.*\)"$/\1/') +# The "|| true" on each field extraction keeps grep's no-match exit status from +# killing the script under set -e; a missing field is reported below instead. +NAME=$(echo "$FRONTMATTER" | grep '^name:' | sed 's/name: *//' | sed 's/^"\(.*\)"$/\1/' || true) if [ -z "$NAME" ]; then echo "❌ Missing required field: name" - ((error_count++)) + error_count=$((error_count + 1)) else echo "✅ name: $NAME" # Validate name format if ! [[ "$NAME" =~ ^[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]$ ]]; then echo "❌ name must start/end with alphanumeric and contain only letters, numbers, hyphens" - ((error_count++)) + error_count=$((error_count + 1)) fi # Validate name length name_length=${#NAME} if [ $name_length -lt 3 ]; then echo "❌ name too short (minimum 3 characters)" - ((error_count++)) + error_count=$((error_count + 1)) elif [ $name_length -gt 50 ]; then echo "❌ name too long (maximum 50 characters)" - ((error_count++)) + error_count=$((error_count + 1)) fi # Check for generic names if [[ "$NAME" =~ ^(helper|assistant|agent|tool)$ ]]; then echo "⚠️ name is too generic: $NAME" - ((warning_count++)) + warning_count=$((warning_count + 1)) fi fi # Check description field -DESCRIPTION=$(echo "$FRONTMATTER" | grep '^description:' | sed 's/description: *//') +# The description may span multiple lines (unquoted prose plus blocks), +# so capture everything from "description:" until the next top-level agent key. +DESCRIPTION=$(echo "$FRONTMATTER" | awk ' + /^description:/ { in_description=1; sub(/^description:[[:space:]]*/, ""); print; next } + /^(name|model|color|tools):/ { in_description=0 } + in_description { print } +') if [ -z "$DESCRIPTION" ]; then echo "❌ Missing required field: description" - ((error_count++)) + error_count=$((error_count + 1)) else desc_length=${#DESCRIPTION} echo "✅ description: ${desc_length} characters" if [ $desc_length -lt 10 ]; then echo "⚠️ description too short (minimum 10 characters recommended)" - ((warning_count++)) + warning_count=$((warning_count + 1)) elif [ $desc_length -gt 5000 ]; then echo "⚠️ description very long (over 5000 characters)" - ((warning_count++)) + warning_count=$((warning_count + 1)) fi # Check for example blocks if ! echo "$DESCRIPTION" | grep -q ''; then echo "⚠️ description should include blocks for triggering" - ((warning_count++)) + warning_count=$((warning_count + 1)) fi # Check for "Use this agent when" pattern if ! echo "$DESCRIPTION" | grep -qi 'use this agent when'; then echo "⚠️ description should start with 'Use this agent when...'" - ((warning_count++)) + warning_count=$((warning_count + 1)) fi fi # Check model field -MODEL=$(echo "$FRONTMATTER" | grep '^model:' | sed 's/model: *//') +MODEL=$(echo "$FRONTMATTER" | grep '^model:' | sed 's/model: *//' || true) if [ -z "$MODEL" ]; then echo "❌ Missing required field: model" - ((error_count++)) + error_count=$((error_count + 1)) else echo "✅ model: $MODEL" @@ -133,17 +141,17 @@ else ;; *) echo "⚠️ Unknown model: $MODEL (valid: inherit, sonnet, opus, haiku)" - ((warning_count++)) + warning_count=$((warning_count + 1)) ;; esac fi # Check color field -COLOR=$(echo "$FRONTMATTER" | grep '^color:' | sed 's/color: *//') +COLOR=$(echo "$FRONTMATTER" | grep '^color:' | sed 's/color: *//' || true) if [ -z "$COLOR" ]; then echo "❌ Missing required field: color" - ((error_count++)) + error_count=$((error_count + 1)) else echo "✅ color: $COLOR" @@ -153,13 +161,13 @@ else ;; *) echo "⚠️ Unknown color: $COLOR (valid: blue, cyan, green, yellow, magenta, red)" - ((warning_count++)) + warning_count=$((warning_count + 1)) ;; esac fi # Check tools field (optional) -TOOLS=$(echo "$FRONTMATTER" | grep '^tools:' | sed 's/tools: *//') +TOOLS=$(echo "$FRONTMATTER" | grep '^tools:' | sed 's/tools: *//' || true) if [ -n "$TOOLS" ]; then echo "✅ tools: $TOOLS" @@ -173,23 +181,23 @@ echo "Checking system prompt..." if [ -z "$SYSTEM_PROMPT" ]; then echo "❌ System prompt is empty" - ((error_count++)) + error_count=$((error_count + 1)) else prompt_length=${#SYSTEM_PROMPT} echo "✅ System prompt: $prompt_length characters" if [ $prompt_length -lt 20 ]; then echo "❌ System prompt too short (minimum 20 characters)" - ((error_count++)) + error_count=$((error_count + 1)) elif [ $prompt_length -gt 10000 ]; then echo "⚠️ System prompt very long (over 10,000 characters)" - ((warning_count++)) + warning_count=$((warning_count + 1)) fi # Check for second person if ! echo "$SYSTEM_PROMPT" | grep -q "You are\|You will\|Your"; then echo "⚠️ System prompt should use second person (You are..., You will...)" - ((warning_count++)) + warning_count=$((warning_count + 1)) fi # Check for structure diff --git a/plugins/plugin-dev/skills/agent-development/scripts/validate-agent.test.sh b/plugins/plugin-dev/skills/agent-development/scripts/validate-agent.test.sh new file mode 100755 index 0000000000..f35acb97dd --- /dev/null +++ b/plugins/plugin-dev/skills/agent-development/scripts/validate-agent.test.sh @@ -0,0 +1,70 @@ +#!/bin/bash +# Regression tests for validate-agent.sh (anthropics/claude-code#83803): +# - warnings must not abort the run: under `set -e`, `((x++))` returns nonzero +# when x was 0, so the first warning killed the script with exit 1 +# - multi-line descriptions (prose plus blocks) must not be +# false-flagged as missing examples +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +VALIDATOR="$SCRIPT_DIR/validate-agent.sh" +PLUGIN_ROOT="$(cd "$SCRIPT_DIR/../../.." && pwd)" +TMP_DIR="$(mktemp -d)" || exit 1 +trap 'rm -rf "$TMP_DIR"' EXIT + +failures=0 + +check() { + local label="$1" expected="$2" file="$3" + local actual=0 + bash "$VALIDATOR" "$file" > "$TMP_DIR/out.txt" 2>&1 || actual=$? + if [ "$actual" -eq "$expected" ]; then + echo "PASS: $label (exit $actual)" + else + echo "FAIL: $label (expected exit $expected, got $actual)" + cat "$TMP_DIR/out.txt" + failures=$((failures + 1)) + fi +} + +# The plugin's own agents are valid and must pass. +for agent in "$PLUGIN_ROOT"/agents/*.md; do + check "own agent $(basename "$agent")" 0 "$agent" +done + +# A valid agent whose fields only trigger warnings must still exit 0, +# and must reach the summary line instead of aborting at the first warning. +cat > "$TMP_DIR/warning-agent.md" <<'EOF' +--- +name: warning-agent +description: A valid description that has no example blocks and no trigger phrase +model: sonnet +color: blue +--- + +You are a test agent. Your job is to exist so the validator has something to warn about. +EOF +check "valid agent with warnings" 0 "$TMP_DIR/warning-agent.md" +if ! grep -q "Validation passed" "$TMP_DIR/out.txt"; then + echo "FAIL: summary line missing (script aborted before finishing)" + failures=$((failures + 1)) +fi + +# An invalid agent (bad name, missing color) must still fail with exit 1. +cat > "$TMP_DIR/invalid-agent.md" <<'EOF' +--- +name: x +description: A valid description for an otherwise invalid agent file +model: sonnet +--- + +You are a test agent with an invalid name and no color field. +EOF +check "invalid agent" 1 "$TMP_DIR/invalid-agent.md" + +echo "" +if [ "$failures" -gt 0 ]; then + echo "$failures test(s) failed" + exit 1 +fi +echo "All tests passed" diff --git a/plugins/plugin-dev/skills/command-development/SKILL.md b/plugins/plugin-dev/skills/command-development/SKILL.md index e39435e14d..57802118bd 100644 --- a/plugins/plugin-dev/skills/command-development/SKILL.md +++ b/plugins/plugin-dev/skills/command-development/SKILL.md @@ -1,5 +1,5 @@ --- -name: Command Development +name: command-development description: This skill should be used when the user asks to "create a slash command", "add a command", "write a custom command", "define command arguments", "use command frontmatter", "organize commands", "create command with file references", "interactive command", "use AskUserQuestion in command", or needs guidance on slash command structure, YAML frontmatter fields, dynamic arguments, bash execution in commands, user interaction patterns, or command development best practices for Claude Code. version: 0.2.0 --- diff --git a/plugins/plugin-dev/skills/hook-development/SKILL.md b/plugins/plugin-dev/skills/hook-development/SKILL.md index d1c0c199c7..094bf14b9a 100644 --- a/plugins/plugin-dev/skills/hook-development/SKILL.md +++ b/plugins/plugin-dev/skills/hook-development/SKILL.md @@ -1,6 +1,6 @@ --- -name: Hook Development -description: This skill should be used when the user asks to "create a hook", "add a PreToolUse/PostToolUse/Stop hook", "validate tool use", "implement prompt-based hooks", "use ${CLAUDE_PLUGIN_ROOT}", "set up event-driven automation", "block dangerous commands", or mentions hook events (PreToolUse, PostToolUse, Stop, SubagentStop, SessionStart, SessionEnd, UserPromptSubmit, PreCompact, Notification). Provides comprehensive guidance for creating and implementing Claude Code plugin hooks with focus on advanced prompt-based hooks API. +name: hook-development +description: This skill should be used when the user asks to "create a hook", "add a PreToolUse/PostToolUse/Stop/MessageDisplay hook", "validate tool use", "implement prompt-based hooks", "use ${CLAUDE_PLUGIN_ROOT}", "set up event-driven automation", "block dangerous commands", or mentions hook events (PreToolUse, PostToolUse, Stop, SubagentStop, SessionStart, SessionEnd, UserPromptSubmit, PreCompact, Notification, MessageDisplay). Provides comprehensive guidance for creating and implementing Claude Code plugin hooks with focus on advanced prompt-based hooks API. version: 0.1.0 --- @@ -15,6 +15,7 @@ Hooks are event-driven automation scripts that execute in response to Claude Cod - React to tool results (PostToolUse) - Enforce completion standards (Stop, SubagentStop) - Load project context (SessionStart) +- Transform streamed assistant text for display (MessageDisplay) - Automate workflows across the development lifecycle ## Hook Types @@ -275,6 +276,44 @@ Execute before context compaction. Use to add critical information to preserve. Execute when Claude sends notifications. Use to react to user notifications. +### MessageDisplay + +Run while Claude streams assistant text to the screen. Use it for display-only +transformations such as redacting sensitive values or adapting text for a +terminal UI. It has no matcher and fires for every assistant message that +streams text. + +Claude Code waits for this hook before rendering each batch, so keep the +handler fast. If it fails or times out, Claude Code renders the original text. + +**Streaming input:** + +- `message_id` identifies one assistant message across its batches. +- `index` is the zero-based position of the batch in that message. +- `final` is true exactly once, on the final batch. Use it for cleanup even + when `delta` is empty. +- `delta` contains newly streamed text, usually as completed lines. The final + batch can end mid-line, so do not assume a particular grouping when carrying + state between calls. + +Return `displayContent` to replace only the current batch on screen: + +```json +{ + "hookSpecificOutput": { + "hookEventName": "MessageDisplay", + "displayContent": "Replacement text" + } +} +``` + +MessageDisplay is display-only: it cannot block the message or change the +transcript or what Claude sees. Store any streaming state per `message_id` and +remove it when `final` is true. + +See the [MessageDisplay hook reference](https://code.claude.com/docs/en/hooks#messagedisplay) +for the complete input and output schema. + ## Hook Output Format ### Standard Output (All Hooks) @@ -642,6 +681,7 @@ echo "$output" | jq . | SessionEnd | Session ends | Cleanup, logging | | PreCompact | Before compact | Preserve context | | Notification | User notified | Logging, reactions | +| MessageDisplay | Assistant text streams | Display-only transformations | ### Best Practices @@ -653,6 +693,7 @@ echo "$output" | jq . - ✅ Set appropriate timeouts - ✅ Return structured JSON output - ✅ Test hooks thoroughly +- ✅ Keep MessageDisplay handlers fast and state scoped to `message_id` **DON'T:** - ❌ Use hardcoded paths diff --git a/plugins/plugin-dev/skills/hook-development/examples/load-context.sh b/plugins/plugin-dev/skills/hook-development/examples/load-context.sh index 9754f321a7..b9713e9632 100755 --- a/plugins/plugin-dev/skills/hook-development/examples/load-context.sh +++ b/plugins/plugin-dev/skills/hook-development/examples/load-context.sh @@ -9,6 +9,12 @@ cd "$CLAUDE_PROJECT_DIR" || exit 1 echo "Loading project context..." +# Prevent symlink credential overwrite attack +if [[ -L "$CLAUDE_ENV_FILE" ]]; then + echo "Error: CLAUDE_ENV_FILE is a symlink, refusing to overwrite for security" >&2 + exit 1 +fi + # Detect project type and set environment if [ -f "package.json" ]; then echo "📦 Node.js project detected" diff --git a/plugins/plugin-dev/skills/hook-development/scripts/validate-hook-schema.sh b/plugins/plugin-dev/skills/hook-development/scripts/validate-hook-schema.sh index fed0a1f1d4..d657b449b9 100755 --- a/plugins/plugin-dev/skills/hook-development/scripts/validate-hook-schema.sh +++ b/plugins/plugin-dev/skills/hook-development/scripts/validate-hook-schema.sh @@ -40,7 +40,16 @@ echo "" echo "Checking root structure..." VALID_EVENTS=("PreToolUse" "PostToolUse" "UserPromptSubmit" "Stop" "SubagentStop" "SessionStart" "SessionEnd" "PreCompact" "Notification") -for event in $(jq -r 'keys[]' "$HOOKS_FILE"); do +# Check if hooks are defined under a top-level "hooks" key or directly at root +if jq -e 'has("hooks") and (.hooks | type == "object")' "$HOOKS_FILE" >/dev/null 2>&1; then + JQ_BASE=".hooks" +else + JQ_BASE="." +fi + +EVENT_KEYS=$(jq -r "${JQ_BASE} | keys[] | select(. != \"description\" and . != \"\$schema\" and . != \"title\" and . != \"version\")" "$HOOKS_FILE") + +for event in $EVENT_KEYS; do found=false for valid_event in "${VALID_EVENTS[@]}"; do if [ "$event" = "$valid_event" ]; then @@ -62,20 +71,12 @@ echo "Validating individual hooks..." error_count=0 warning_count=0 -for event in $(jq -r 'keys[]' "$HOOKS_FILE"); do - hook_count=$(jq -r ".\"$event\" | length" "$HOOKS_FILE") +for event in $EVENT_KEYS; do + hook_count=$(jq -r "${JQ_BASE}.\"$event\" | length" "$HOOKS_FILE") for ((i=0; i&2 + exit 1 +fi + # Extract frontmatter FRONTMATTER=$(sed -n '/^---$/,/^---$/{ /^---$/d; p; }' "$FILE") diff --git a/plugins/plugin-dev/skills/plugin-structure/SKILL.md b/plugins/plugin-dev/skills/plugin-structure/SKILL.md index 6fb8a3baa1..6bcae94a52 100644 --- a/plugins/plugin-dev/skills/plugin-structure/SKILL.md +++ b/plugins/plugin-dev/skills/plugin-structure/SKILL.md @@ -1,5 +1,5 @@ --- -name: Plugin Structure +name: plugin-structure description: This skill should be used when the user asks to "create a plugin", "scaffold a plugin", "understand plugin structure", "organize plugin components", "set up plugin.json", "use ${CLAUDE_PLUGIN_ROOT}", "add commands/agents/skills/hooks", "configure auto-discovery", or needs guidance on plugin directory layout, manifest configuration, component organization, file naming conventions, or Claude Code plugin architecture best practices. version: 0.1.0 --- diff --git a/plugins/plugin-dev/skills/skill-development/SKILL.md b/plugins/plugin-dev/skills/skill-development/SKILL.md index ac75eedde7..e720adc438 100644 --- a/plugins/plugin-dev/skills/skill-development/SKILL.md +++ b/plugins/plugin-dev/skills/skill-development/SKILL.md @@ -1,5 +1,5 @@ --- -name: Skill Development +name: skill-development description: This skill should be used when the user wants to "create a skill", "add a skill to plugin", "write a new skill", "improve skill description", "organize skill content", or needs guidance on skill structure, progressive disclosure, or skill development best practices for Claude Code plugins. version: 0.1.0 --- diff --git a/plugins/pr-review-toolkit/agents/code-reviewer.md b/plugins/pr-review-toolkit/agents/code-reviewer.md index 462f2e01b8..a16f18952f 100644 --- a/plugins/pr-review-toolkit/agents/code-reviewer.md +++ b/plugins/pr-review-toolkit/agents/code-reviewer.md @@ -1,6 +1,35 @@ --- name: code-reviewer -description: Use this agent when you need to review code for adherence to project guidelines, style guides, and best practices. This agent should be used proactively after writing or modifying code, especially before committing changes or creating pull requests. It will check for style violations, potential issues, and ensure code follows the established patterns in CLAUDE.md. Also the agent needs to know which files to focus on for the review. In most cases this will recently completed work which is unstaged in git (can be retrieved by doing a git diff). However there can be cases where this is different, make sure to specify this as the agent input when calling the agent. \n\nExamples:\n\nContext: The user has just implemented a new feature with several TypeScript files.\nuser: "I've added the new authentication feature. Can you check if everything looks good?"\nassistant: "I'll use the Task tool to launch the code-reviewer agent to review your recent changes."\n\nSince the user has completed a feature and wants validation, use the code-reviewer agent to ensure the code meets project standards.\n\n\n\nContext: The assistant has just written a new utility function.\nuser: "Please create a function to validate email addresses"\nassistant: "Here's the email validation function:"\n\nassistant: "Now I'll use the Task tool to launch the code-reviewer agent to review this implementation."\n\nProactively use the code-reviewer agent after writing new code to catch issues early.\n\n\n\nContext: The user is about to create a PR.\nuser: "I think I'm ready to create a PR for this feature"\nassistant: "Before creating the PR, I'll use the Task tool to launch the code-reviewer agent to ensure all code meets our standards."\n\nProactively review code before PR creation to avoid review comments and iterations.\n\n +description: | + Use this agent when you need to review code for adherence to project guidelines, style guides, and best practices. This agent should be used proactively after writing or modifying code, especially before committing changes or creating pull requests. It will check for style violations, potential issues, and ensure code follows the established patterns in CLAUDE.md. Also the agent needs to know which files to focus on for the review. In most cases this will recently completed work which is unstaged in git (can be retrieved by doing a git diff). However there can be cases where this is different, make sure to specify this as the agent input when calling the agent. + + Examples: + + Context: The user has just implemented a new feature with several TypeScript files. + user: "I've added the new authentication feature. Can you check if everything looks good?" + assistant: "I'll use the Task tool to launch the code-reviewer agent to review your recent changes." + + Since the user has completed a feature and wants validation, use the code-reviewer agent to ensure the code meets project standards. + + + + Context: The assistant has just written a new utility function. + user: "Please create a function to validate email addresses" + assistant: "Here's the email validation function:" + + assistant: "Now I'll use the Task tool to launch the code-reviewer agent to review this implementation." + + Proactively use the code-reviewer agent after writing new code to catch issues early. + + + + Context: The user is about to create a PR. + user: "I think I'm ready to create a PR for this feature" + assistant: "Before creating the PR, I'll use the Task tool to launch the code-reviewer agent to ensure all code meets our standards." + + Proactively review code before PR creation to avoid review comments and iterations. + + model: opus color: green --- diff --git a/plugins/pr-review-toolkit/agents/code-simplifier.md b/plugins/pr-review-toolkit/agents/code-simplifier.md index 7d6cfd81a8..89a01c0b97 100644 --- a/plugins/pr-review-toolkit/agents/code-simplifier.md +++ b/plugins/pr-review-toolkit/agents/code-simplifier.md @@ -1,37 +1,42 @@ --- name: code-simplifier -description: Use this agent when code has been written or modified and needs to be simplified for clarity, consistency, and maintainability while preserving all functionality. This agent should be triggered automatically after completing a coding task or writing a logical chunk of code. It simplifies code by following project best practices while retaining all functionality. The agent focuses only on recently modified code unless instructed otherwise.\n\nExamples:\n\n -Context: The assistant has just implemented a new feature that adds user authentication to an API endpoint. -user: "Please add authentication to the /api/users endpoint" -assistant: "I've implemented the authentication for the /api/users endpoint. Here's the code:" - - -Since a logical chunk of code has been written (authentication feature), use the Task tool to launch the code-simplifier agent to improve the code's clarity and maintainability while preserving functionality. - -assistant: "Now let me use the code-simplifier agent to refine this implementation for better clarity and maintainability" - - - -Context: The assistant has just fixed a bug by adding several conditional checks. -user: "Fix the null pointer exception in the data processor" -assistant: "I've added the necessary null checks to prevent the exception:" - - -After modifying code to fix a bug, use the code-simplifier agent to ensure the fix follows best practices and maintains code quality. - -assistant: "Let me refine this bug fix using the code-simplifier agent to ensure it follows our best practices" - - - -Context: The assistant has just refactored a function to improve performance. -user: "Optimize the data sorting algorithm for better performance" -assistant: "I've optimized the sorting algorithm. Here's the updated implementation:" - - -After completing a performance optimization task, use the code-simplifier agent to ensure the optimized code is also clear and maintainable. - -assistant: "Now I'll use the code-simplifier agent to ensure the optimized code is also clear and follows our coding standards" - +description: | + Use this agent when code has been written or modified and needs to be simplified for clarity, consistency, and maintainability while preserving all functionality. This agent should be triggered automatically after completing a coding task or writing a logical chunk of code. It simplifies code by following project best practices while retaining all functionality. The agent focuses only on recently modified code unless instructed otherwise. + + Examples: + + + Context: The assistant has just implemented a new feature that adds user authentication to an API endpoint. + user: "Please add authentication to the /api/users endpoint" + assistant: "I've implemented the authentication for the /api/users endpoint. Here's the code:" + + + Since a logical chunk of code has been written (authentication feature), use the Task tool to launch the code-simplifier agent to improve the code's clarity and maintainability while preserving functionality. + + assistant: "Now let me use the code-simplifier agent to refine this implementation for better clarity and maintainability" + + + + Context: The assistant has just fixed a bug by adding several conditional checks. + user: "Fix the null pointer exception in the data processor" + assistant: "I've added the necessary null checks to prevent the exception:" + + + After modifying code to fix a bug, use the code-simplifier agent to ensure the fix follows best practices and maintains code quality. + + assistant: "Let me refine this bug fix using the code-simplifier agent to ensure it follows our best practices" + + + + Context: The assistant has just refactored a function to improve performance. + user: "Optimize the data sorting algorithm for better performance" + assistant: "I've optimized the sorting algorithm. Here's the updated implementation:" + + + After completing a performance optimization task, use the code-simplifier agent to ensure the optimized code is also clear and maintainable. + + assistant: "Now I'll use the code-simplifier agent to ensure the optimized code is also clear and follows our coding standards" + model: opus --- diff --git a/plugins/pr-review-toolkit/agents/comment-analyzer.md b/plugins/pr-review-toolkit/agents/comment-analyzer.md index e214620a3f..24c4bf81fa 100644 --- a/plugins/pr-review-toolkit/agents/comment-analyzer.md +++ b/plugins/pr-review-toolkit/agents/comment-analyzer.md @@ -1,6 +1,34 @@ --- name: comment-analyzer -description: Use this agent when you need to analyze code comments for accuracy, completeness, and long-term maintainability. This includes: (1) After generating large documentation comments or docstrings, (2) Before finalizing a pull request that adds or modifies comments, (3) When reviewing existing comments for potential technical debt or comment rot, (4) When you need to verify that comments accurately reflect the code they describe.\n\n\nContext: The user is working on a pull request that adds several documentation comments to functions.\nuser: "I've added documentation to these functions. Can you check if the comments are accurate?"\nassistant: "I'll use the comment-analyzer agent to thoroughly review all the comments in this pull request for accuracy and completeness."\n\nSince the user has added documentation comments and wants them checked, use the comment-analyzer agent to verify their accuracy against the actual code.\n\n\n\n\nContext: The user just asked to generate comprehensive documentation for a complex function.\nuser: "Add detailed documentation for this authentication handler function"\nassistant: "I've added the documentation. Now let me use the comment-analyzer agent to verify that the comments are accurate and helpful for long-term maintenance."\n\nAfter generating large documentation comments, proactively use the comment-analyzer to ensure quality.\n\n\n\n\nContext: The user is preparing to create a pull request with multiple code changes and comments.\nuser: "I think we're ready to create the PR now"\nassistant: "Before creating the pull request, let me use the comment-analyzer agent to review all the comments we've added or modified to ensure they're accurate and won't create technical debt."\n\nBefore finalizing a PR, use the comment-analyzer to review all comment changes.\n\n +description: | + Use this agent when you need to analyze code comments for accuracy, completeness, and long-term maintainability. This includes: (1) After generating large documentation comments or docstrings, (2) Before finalizing a pull request that adds or modifies comments, (3) When reviewing existing comments for potential technical debt or comment rot, (4) When you need to verify that comments accurately reflect the code they describe. + + + Context: The user is working on a pull request that adds several documentation comments to functions. + user: "I've added documentation to these functions. Can you check if the comments are accurate?" + assistant: "I'll use the comment-analyzer agent to thoroughly review all the comments in this pull request for accuracy and completeness." + + Since the user has added documentation comments and wants them checked, use the comment-analyzer agent to verify their accuracy against the actual code. + + + + + Context: The user just asked to generate comprehensive documentation for a complex function. + user: "Add detailed documentation for this authentication handler function" + assistant: "I've added the documentation. Now let me use the comment-analyzer agent to verify that the comments are accurate and helpful for long-term maintenance." + + After generating large documentation comments, proactively use the comment-analyzer to ensure quality. + + + + + Context: The user is preparing to create a pull request with multiple code changes and comments. + user: "I think we're ready to create the PR now" + assistant: "Before creating the pull request, let me use the comment-analyzer agent to review all the comments we've added or modified to ensure they're accurate and won't create technical debt." + + Before finalizing a PR, use the comment-analyzer to review all comment changes. + + model: inherit color: green --- diff --git a/plugins/pr-review-toolkit/agents/pr-test-analyzer.md b/plugins/pr-review-toolkit/agents/pr-test-analyzer.md index 9b2de05b90..16208f3796 100644 --- a/plugins/pr-review-toolkit/agents/pr-test-analyzer.md +++ b/plugins/pr-review-toolkit/agents/pr-test-analyzer.md @@ -1,6 +1,34 @@ --- name: pr-test-analyzer -description: Use this agent when you need to review a pull request for test coverage quality and completeness. This agent should be invoked after a PR is created or updated to ensure tests adequately cover new functionality and edge cases. Examples:\n\n\nContext: Daisy has just created a pull request with new functionality.\nuser: "I've created the PR. Can you check if the tests are thorough?"\nassistant: "I'll use the pr-test-analyzer agent to review the test coverage and identify any critical gaps."\n\nSince Daisy is asking about test thoroughness in a PR, use the Task tool to launch the pr-test-analyzer agent.\n\n\n\n\nContext: A pull request has been updated with new code changes.\nuser: "The PR is ready for review - I added the new validation logic we discussed"\nassistant: "Let me analyze the PR to ensure the tests adequately cover the new validation logic and edge cases."\n\nThe PR has new functionality that needs test coverage analysis, so use the pr-test-analyzer agent.\n\n\n\n\nContext: Reviewing PR feedback before marking as ready.\nuser: "Before I mark this PR as ready, can you double-check the test coverage?"\nassistant: "I'll use the pr-test-analyzer agent to thoroughly review the test coverage and identify any critical gaps before you mark it ready."\n\nDaisy wants a final test coverage check before marking PR ready, use the pr-test-analyzer agent.\n\n +description: | + Use this agent when you need to review a pull request for test coverage quality and completeness. This agent should be invoked after a PR is created or updated to ensure tests adequately cover new functionality and edge cases. Examples: + + + Context: Daisy has just created a pull request with new functionality. + user: "I've created the PR. Can you check if the tests are thorough?" + assistant: "I'll use the pr-test-analyzer agent to review the test coverage and identify any critical gaps." + + Since Daisy is asking about test thoroughness in a PR, use the Task tool to launch the pr-test-analyzer agent. + + + + + Context: A pull request has been updated with new code changes. + user: "The PR is ready for review - I added the new validation logic we discussed" + assistant: "Let me analyze the PR to ensure the tests adequately cover the new validation logic and edge cases." + + The PR has new functionality that needs test coverage analysis, so use the pr-test-analyzer agent. + + + + + Context: Reviewing PR feedback before marking as ready. + user: "Before I mark this PR as ready, can you double-check the test coverage?" + assistant: "I'll use the pr-test-analyzer agent to thoroughly review the test coverage and identify any critical gaps before you mark it ready." + + Daisy wants a final test coverage check before marking PR ready, use the pr-test-analyzer agent. + + model: inherit color: cyan --- diff --git a/plugins/pr-review-toolkit/agents/silent-failure-hunter.md b/plugins/pr-review-toolkit/agents/silent-failure-hunter.md index b8a8dfa41e..02376e271d 100644 --- a/plugins/pr-review-toolkit/agents/silent-failure-hunter.md +++ b/plugins/pr-review-toolkit/agents/silent-failure-hunter.md @@ -1,6 +1,28 @@ --- name: silent-failure-hunter -description: Use this agent when reviewing code changes in a pull request to identify silent failures, inadequate error handling, and inappropriate fallback behavior. This agent should be invoked proactively after completing a logical chunk of work that involves error handling, catch blocks, fallback logic, or any code that could potentially suppress errors. Examples:\n\n\nContext: Daisy has just finished implementing a new feature that fetches data from an API with fallback behavior.\nDaisy: "I've added error handling to the API client. Can you review it?"\nAssistant: "Let me use the silent-failure-hunter agent to thoroughly examine the error handling in your changes."\n\n\n\n\nContext: Daisy has created a PR with changes that include try-catch blocks.\nDaisy: "Please review PR #1234"\nAssistant: "I'll use the silent-failure-hunter agent to check for any silent failures or inadequate error handling in this PR."\n\n\n\n\nContext: Daisy has just refactored error handling code.\nDaisy: "I've updated the error handling in the authentication module"\nAssistant: "Let me proactively use the silent-failure-hunter agent to ensure the error handling changes don't introduce silent failures."\n\n +description: | + Use this agent when reviewing code changes in a pull request to identify silent failures, inadequate error handling, and inappropriate fallback behavior. This agent should be invoked proactively after completing a logical chunk of work that involves error handling, catch blocks, fallback logic, or any code that could potentially suppress errors. Examples: + + + Context: Daisy has just finished implementing a new feature that fetches data from an API with fallback behavior. + Daisy: "I've added error handling to the API client. Can you review it?" + Assistant: "Let me use the silent-failure-hunter agent to thoroughly examine the error handling in your changes." + + + + + Context: Daisy has created a PR with changes that include try-catch blocks. + Daisy: "Please review PR #1234" + Assistant: "I'll use the silent-failure-hunter agent to check for any silent failures or inadequate error handling in this PR." + + + + + Context: Daisy has just refactored error handling code. + Daisy: "I've updated the error handling in the authentication module" + Assistant: "Let me proactively use the silent-failure-hunter agent to ensure the error handling changes don't introduce silent failures." + + model: inherit color: yellow --- diff --git a/plugins/pr-review-toolkit/agents/type-design-analyzer.md b/plugins/pr-review-toolkit/agents/type-design-analyzer.md index f720f0fcec..a9d4852d55 100644 --- a/plugins/pr-review-toolkit/agents/type-design-analyzer.md +++ b/plugins/pr-review-toolkit/agents/type-design-analyzer.md @@ -1,6 +1,25 @@ --- name: type-design-analyzer -description: Use this agent when you need expert analysis of type design in your codebase. Specifically use it: (1) when introducing a new type to ensure it follows best practices for encapsulation and invariant expression, (2) during pull request creation to review all types being added, (3) when refactoring existing types to improve their design quality. The agent will provide both qualitative feedback and quantitative ratings on encapsulation, invariant expression, usefulness, and enforcement.\n\n\nContext: Daisy is writing code that introduces a new UserAccount type and wants to ensure it has well-designed invariants.\nuser: "I've just created a new UserAccount type that handles user authentication and permissions"\nassistant: "I'll use the type-design-analyzer agent to review the UserAccount type design"\n\nSince a new type is being introduced, use the type-design-analyzer to ensure it has strong invariants and proper encapsulation.\n\n\n\n\nContext: Daisy is creating a pull request and wants to review all newly added types.\nuser: "I'm about to create a PR with several new data model types"\nassistant: "Let me use the type-design-analyzer agent to review all the types being added in this PR"\n\nDuring PR creation with new types, use the type-design-analyzer to review their design quality.\n\n +description: | + Use this agent when you need expert analysis of type design in your codebase. Specifically use it: (1) when introducing a new type to ensure it follows best practices for encapsulation and invariant expression, (2) during pull request creation to review all types being added, (3) when refactoring existing types to improve their design quality. The agent will provide both qualitative feedback and quantitative ratings on encapsulation, invariant expression, usefulness, and enforcement. + + + Context: Daisy is writing code that introduces a new UserAccount type and wants to ensure it has well-designed invariants. + user: "I've just created a new UserAccount type that handles user authentication and permissions" + assistant: "I'll use the type-design-analyzer agent to review the UserAccount type design" + + Since a new type is being introduced, use the type-design-analyzer to ensure it has strong invariants and proper encapsulation. + + + + + Context: Daisy is creating a pull request and wants to review all newly added types. + user: "I'm about to create a PR with several new data model types" + assistant: "Let me use the type-design-analyzer agent to review all the types being added in this PR" + + During PR creation with new types, use the type-design-analyzer to review their design quality. + + model: inherit color: pink --- diff --git a/plugins/ralph-wiggum/.claude-plugin/plugin.json b/plugins/ralph-wiggum/.claude-plugin/plugin.json index ec19b4012a..e94fa59a18 100644 --- a/plugins/ralph-wiggum/.claude-plugin/plugin.json +++ b/plugins/ralph-wiggum/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "ralph-wiggum", - "version": "1.0.0", + "version": "1.0.1", "description": "Implementation of the Ralph Wiggum technique - continuous self-referential AI loops for interactive iterative development. Run Claude in a while-true loop with the same prompt until task completion.", "author": { "name": "Daisy Hollman", diff --git a/plugins/ralph-wiggum/commands/ralph-loop.md b/plugins/ralph-wiggum/commands/ralph-loop.md index 3b4dbcb62a..88ba99b35c 100644 --- a/plugins/ralph-wiggum/commands/ralph-loop.md +++ b/plugins/ralph-wiggum/commands/ralph-loop.md @@ -10,7 +10,9 @@ hide-from-slash-command-tool: "true" Execute the setup script to initialize the Ralph loop: ```! -"${CLAUDE_PLUGIN_ROOT}/scripts/setup-ralph-loop.sh" $ARGUMENTS +"${CLAUDE_PLUGIN_ROOT}/scripts/setup-ralph-loop.sh" <<'RALPH_LOOP_ARGS_EOF' +$ARGUMENTS +RALPH_LOOP_ARGS_EOF ``` Please work on the task. When you try to exit, the Ralph loop will feed the SAME PROMPT back to you for the next iteration. You'll see your previous work in files and git history, allowing you to iterate and improve. diff --git a/plugins/ralph-wiggum/hooks/hooks.json b/plugins/ralph-wiggum/hooks/hooks.json index 2e5f697934..bfdcddd7ce 100644 --- a/plugins/ralph-wiggum/hooks/hooks.json +++ b/plugins/ralph-wiggum/hooks/hooks.json @@ -6,7 +6,7 @@ "hooks": [ { "type": "command", - "command": "${CLAUDE_PLUGIN_ROOT}/hooks/stop-hook.sh" + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/stop-hook.sh\"" } ] } diff --git a/plugins/ralph-wiggum/scripts/setup-ralph-loop.sh b/plugins/ralph-wiggum/scripts/setup-ralph-loop.sh index ac5491f4b8..3845409429 100755 --- a/plugins/ralph-wiggum/scripts/setup-ralph-loop.sh +++ b/plugins/ralph-wiggum/scripts/setup-ralph-loop.sh @@ -2,19 +2,16 @@ # Ralph Loop Setup Script # Creates state file for in-session Ralph loop +# +# Arguments arrive as raw text on stdin (fed via a quoted heredoc from +# ralph-loop.md) so prompt text is never parsed as shell code. Quotes, +# semicolons, $(), and newlines in the prompt are all treated literally. +# Direct invocation with positional arguments still works for manual use. set -euo pipefail -# Parse arguments -PROMPT_PARTS=() -MAX_ITERATIONS=0 -COMPLETION_PROMISE="null" - -# Parse options and positional arguments -while [[ $# -gt 0 ]]; do - case $1 in - -h|--help) - cat << 'HELP_EOF' +show_help() { + cat << 'HELP_EOF' Ralph Loop - Interactive self-referential development loop USAGE: @@ -56,61 +53,101 @@ MONITORING: # View full state: head -10 .claude/ralph-loop.local.md HELP_EOF - exit 0 - ;; - --max-iterations) - if [[ -z "${2:-}" ]]; then - echo "❌ Error: --max-iterations requires a number argument" >&2 - echo "" >&2 - echo " Valid examples:" >&2 - echo " --max-iterations 10" >&2 - echo " --max-iterations 50" >&2 - echo " --max-iterations 0 (unlimited)" >&2 - echo "" >&2 - echo " You provided: --max-iterations (with no number)" >&2 - exit 1 - fi - if ! [[ "$2" =~ ^[0-9]+$ ]]; then - echo "❌ Error: --max-iterations must be a positive integer or 0, got: $2" >&2 - echo "" >&2 - echo " Valid examples:" >&2 - echo " --max-iterations 10" >&2 - echo " --max-iterations 50" >&2 - echo " --max-iterations 0 (unlimited)" >&2 - echo "" >&2 - echo " Invalid: decimals (10.5), negative numbers (-5), text" >&2 - exit 1 - fi - MAX_ITERATIONS="$2" - shift 2 - ;; - --completion-promise) - if [[ -z "${2:-}" ]]; then - echo "❌ Error: --completion-promise requires a text argument" >&2 - echo "" >&2 - echo " Valid examples:" >&2 - echo " --completion-promise 'DONE'" >&2 - echo " --completion-promise 'TASK COMPLETE'" >&2 - echo " --completion-promise 'All tests passing'" >&2 - echo "" >&2 - echo " You provided: --completion-promise (with no text)" >&2 - echo "" >&2 - echo " Note: Multi-word promises must be quoted!" >&2 - exit 1 - fi - COMPLETION_PROMISE="$2" - shift 2 - ;; - *) - # Non-option argument - collect all as prompt parts - PROMPT_PARTS+=("$1") - shift - ;; - esac -done +} + +# Read raw argument text: positional arguments when invoked directly, +# otherwise stdin (the heredoc from ralph-loop.md) +if [[ $# -gt 0 ]]; then + RAW="$*" +elif [[ ! -t 0 ]]; then + RAW="$(cat)" +else + RAW="" +fi + +# Normalize Windows line endings +RAW="${RAW//$'\r'/}" + +if [[ "$RAW" =~ (^|[[:space:]])(-h|--help)([[:space:]]|$) ]]; then + show_help + exit 0 +fi -# Join all prompt parts with spaces -PROMPT="${PROMPT_PARTS[*]}" +MAX_ITERATIONS=0 +COMPLETION_PROMISE="null" + +# Extract --completion-promise from the raw text. +# Quotes are literal characters here, so recognize 'quoted', "quoted", +# and bare single-word forms explicitly. Parsed before --max-iterations +# so a quoted promise containing flag-like text is consumed first. +RE_PROMISE_SQ="--completion-promise[[:space:]]+'([^']*)'" +RE_PROMISE_DQ='--completion-promise[[:space:]]+"([^"]*)"' +RE_PROMISE_BARE='--completion-promise[[:space:]]+([^[:space:]]+)' +if [[ "$RAW" == *--completion-promise* ]]; then + if [[ "$RAW" =~ $RE_PROMISE_SQ ]] || [[ "$RAW" =~ $RE_PROMISE_DQ ]] || [[ "$RAW" =~ $RE_PROMISE_BARE ]]; then + PROMISE_MATCH="${BASH_REMATCH[0]}" + COMPLETION_PROMISE="${BASH_REMATCH[1]}" + fi + if [[ -z "$COMPLETION_PROMISE" ]] || [[ "$COMPLETION_PROMISE" == "null" ]]; then + echo "❌ Error: --completion-promise requires a text argument" >&2 + echo "" >&2 + echo " Valid examples:" >&2 + echo " --completion-promise 'DONE'" >&2 + echo " --completion-promise 'TASK COMPLETE'" >&2 + echo " --completion-promise 'All tests passing'" >&2 + echo "" >&2 + echo " You provided: --completion-promise (with no text)" >&2 + echo "" >&2 + echo " Note: Multi-word promises must be quoted!" >&2 + exit 1 + fi + RAW="${RAW/"$PROMISE_MATCH"/ }" +fi + +# Extract --max-iterations from the raw text +if [[ "$RAW" == *--max-iterations* ]]; then + if [[ "$RAW" =~ --max-iterations[[:space:]]+([^[:space:]]+) ]]; then + MAX_ITER_MATCH="${BASH_REMATCH[0]}" + MAX_ITER_VALUE="${BASH_REMATCH[1]}" + if ! [[ "$MAX_ITER_VALUE" =~ ^[0-9]+$ ]]; then + echo "❌ Error: --max-iterations must be a positive integer or 0, got: $MAX_ITER_VALUE" >&2 + echo "" >&2 + echo " Valid examples:" >&2 + echo " --max-iterations 10" >&2 + echo " --max-iterations 50" >&2 + echo " --max-iterations 0 (unlimited)" >&2 + echo "" >&2 + echo " Invalid: decimals (10.5), negative numbers (-5), text" >&2 + exit 1 + fi + MAX_ITERATIONS="$MAX_ITER_VALUE" + RAW="${RAW/"$MAX_ITER_MATCH"/ }" + else + echo "❌ Error: --max-iterations requires a number argument" >&2 + echo "" >&2 + echo " Valid examples:" >&2 + echo " --max-iterations 10" >&2 + echo " --max-iterations 50" >&2 + echo " --max-iterations 0 (unlimited)" >&2 + echo "" >&2 + echo " You provided: --max-iterations (with no number)" >&2 + exit 1 + fi +fi + +# Whatever remains is the prompt; trim surrounding whitespace +PROMPT="$RAW" +PROMPT="${PROMPT#"${PROMPT%%[![:space:]]*}"}" +PROMPT="${PROMPT%"${PROMPT##*[![:space:]]}"}" + +# Strip one pair of surrounding quotes (shell used to consume these when +# arguments were passed on the command line, e.g. /ralph-loop "Build X") +if [[ ${#PROMPT} -ge 2 ]]; then + case "$PROMPT" in + \"*\") PROMPT="${PROMPT:1:${#PROMPT}-2}" ;; + \'*\') PROMPT="${PROMPT:1:${#PROMPT}-2}" ;; + esac +fi # Validate prompt is non-empty if [[ -z "$PROMPT" ]]; then @@ -132,7 +169,7 @@ mkdir -p .claude # Quote completion promise for YAML if it contains special chars or is not null if [[ -n "$COMPLETION_PROMISE" ]] && [[ "$COMPLETION_PROMISE" != "null" ]]; then - COMPLETION_PROMISE_YAML="\"$COMPLETION_PROMISE\"" + COMPLETION_PROMISE_YAML=$(jq -n --arg p "$COMPLETION_PROMISE" '$p') else COMPLETION_PROMISE_YAML="null" fi diff --git a/plugins/security-guidance/.claude-plugin/plugin.json b/plugins/security-guidance/.claude-plugin/plugin.json index 1e4165bb15..f0891b687d 100644 --- a/plugins/security-guidance/.claude-plugin/plugin.json +++ b/plugins/security-guidance/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "security-guidance", - "version": "2.0.0", + "version": "2.1.0", "description": "Security review for Claude-generated code. Pattern-based warnings on edits, LLM-powered diff review on Stop, and an agentic commit reviewer that catches injection, XSS, SSRF, hardcoded secrets, and 25+ other vulnerability classes.", "author": { "name": "David Dworken", diff --git a/plugins/security-guidance/README.md b/plugins/security-guidance/README.md index 485f22fbb5..cadcb6a858 100644 --- a/plugins/security-guidance/README.md +++ b/plugins/security-guidance/README.md @@ -50,6 +50,7 @@ SECURITY_REVIEW_MODEL=claude-opus-4-7@20260218 | `ENABLE_CODE_SECURITY_REVIEW=0` | on | Disable all LLM reviews (Stop hook + commit/push) | | `ENABLE_STOP_REVIEW=0` | on | Disable only the Stop-hook diff review, keeping commit/push reviews. Useful for multi-agent / shared-worktree setups where another agent can move HEAD between a worker's turns | | `ENABLE_COMMIT_REVIEW=0` | on | Disable layer 3 (agentic commit review) | +| `SG_SKIP_SECRET_FILES=0` | on | Stop skipping well-known secret files (`.env*`, private keys, `secrets.yaml`, `credentials.json`, `.netrc`, …) so they are reviewed like any other file. Files your `Read` deny rules cover stay excluded either way — see [Privacy and data handling](#privacy-and-data-handling) | ### Higher-recall mode @@ -86,6 +87,15 @@ Built-in rules cover common web-vulnerability classes without it — `claude-sec The plugin sends data to a model endpoint to perform its reviews. Specifically, each Stop-hook diff review transmits the changed file paths, the diff hunks, and the relevant file contents in the diff; each agentic commit review additionally transmits any files the reviewer pulls in via `Read`/`Grep`/`Glob` while tracing data flow. Your `claude-security-guidance.md` contents (user, project, and local) are appended to the prompt on every review, so don't put secrets in it. +Two kinds of file are kept out of every review, both from the diff the plugin sends and from what the agentic reviewer's sub-agent may `Read`/`Grep`/`Glob` (the sub-agent gets no shell tool, so `cat` or `git show` are not a way around this): + +- **Files your session's `Read` permission rules deny or ask about.** The plugin reads `permissions.deny` and `permissions.ask` from the same settings files Claude Code loads — managed (`managed-settings.json` and `managed-settings.d/`), user (`~/.claude/settings.json`), project (`.claude/settings.json`) and local (`.claude/settings.local.json`) — and applies the `Read(...)` rules with Claude Code's path conventions (`//abs`, `~/home`, `/project-relative`, gitignore-style globs). The sub-agent receives the same rules as `--disallowedTools`. Rules that exist only in MDM/registry policy, server-managed settings, or `--settings` / `--disallowedTools` command-line flags are not visible to a hook; put them in one of the settings files above if the reviewer must honor them. +- **Well-known secret files**, on by default: `.env` / `.env.*` / `*.env`, private keys and keystores (`*.pem`, `*.key`, `*.p12`, `*.jks`, `id_rsa*`, …), credential stores (`.netrc`, `.npmrc`, `.pypirc`, `.git-credentials`, `credentials`, `kubeconfig`) and `secret(s)` / `credential(s)` data files (`.json`, `.yaml`, `.toml`, …). Source files such as `secrets.py` are still reviewed. In the sub-agent these names follow permission-rule (gitignore) semantics, so a directory that shares one — `credentials/`, `.env/` — is off-limits to it as well. Set `SG_SKIP_SECRET_FILES=0` to review these too. + +The flip side: the reviewer cannot flag a problem inside a file it never sees, and a project's own `.claude/settings.json` decides what is hidden — the same trust Claude Code itself places in project settings. Each withheld path is named in the debug log (`secretpaths: withheld from review …`). + +Anything else that changed in the working tree during the turn is reviewed, including files your own `PreToolUse` hooks would have stopped the main session from reading: the content reaches the reviewer as a diff, not through a tool call, so those hooks never run. What comes back into the main conversation is the finding, not the file, and before a finding is emitted or kept in session state the plugin masks the credential values it can recognize in the quoted code — the value after a `password` / `secret` / `token` / `api_key`-style key (code, JSON, YAML, `.env`, XML, Dockerfile `ENV`), passwords in connection URLs, `Authorization` and `curl -u` credentials, well-known API-key formats, private-key blocks, and, in a finding classed as a hardcoded secret, any other quoted literal or long opaque token — to `****` plus the last two characters, so the main session learns where the secret is without receiving it. This is pattern matching, not a guarantee: add a `Read(...)` deny rule for any file whose content must not reach the review endpoint at all. + Where that data goes depends on your Claude Code configuration: - **Default (Anthropic API / subscription):** sent to `api.anthropic.com` and handled under Anthropic's [Commercial Terms](https://www.anthropic.com/legal/commercial-terms) and [Privacy Policy](https://www.anthropic.com/legal/privacy). - **LLM gateway** (`ANTHROPIC_BASE_URL` set): sent to your gateway URL instead. The gateway operator's terms apply. diff --git a/plugins/security-guidance/hooks/extensibility.py b/plugins/security-guidance/hooks/extensibility.py index a9c7f8fe5f..997c1ca379 100644 --- a/plugins/security-guidance/hooks/extensibility.py +++ b/plugins/security-guidance/hooks/extensibility.py @@ -32,7 +32,6 @@ all pattern checks; there is no per-rule kill switch in v1. """ -import fnmatch import json import os import re @@ -156,7 +155,7 @@ def _load_user_patterns(cwd: Optional[str]) -> List[Dict[str, Any]]: if data is None: continue for entry in (data or {}).get("patterns", []): - rule = _validate_pattern(entry, source=label) + rule = _validate_pattern(entry, source=label, cwd=cwd) if rule: rules.append(rule) break # found one extension; don't double-load .yaml AND .json @@ -196,7 +195,7 @@ def _read_config(path: str) -> Optional[Dict[str, Any]]: return None -def _validate_pattern(entry: Any, source: str) -> Optional[Dict[str, Any]]: +def _validate_pattern(entry: Any, source: str, cwd: Optional[str] = None) -> Optional[Dict[str, Any]]: """Validate one user pattern entry. Returns a rule dict in the same shape as the built-in SECURITY_PATTERNS, or None if invalid (logged).""" if not isinstance(entry, dict): @@ -239,19 +238,131 @@ def _validate_pattern(entry: Any, source: str) -> Optional[Dict[str, Any]]: return None # Capture as defaults so the lambda doesn't share state across rules. rule["path_filter"] = ( - lambda p, _inc=tuple(paths), _exc=tuple(exclude): _glob_match(p, _inc, _exc) + lambda p, _inc=tuple(paths), _exc=tuple(exclude), _cwd=cwd: _glob_match(p, _inc, _exc, _cwd) ) return rule -def _glob_match(path: str, include: Tuple[str, ...], exclude: Tuple[str, ...]) -> bool: +def _glob_to_regex(glob: str) -> "re.Pattern[str]": + """Translate a glob to a regex matching fnmatch's semantics exactly + (``*`` and ``?`` both cross ``/`` — that recursion is relied on by + existing patterns and is preserved deliberately, not a bug), except for + one addition: ``**/`` also matches zero directories, so ``**/*.ts`` + covers a top-level file the same way it covers a nested one. Plain + ``fnmatch`` can't express that — a literal ``/`` in the pattern always + requires a literal ``/`` in the string, no matter how many ``*`` sit + next to it.""" + out = [] + i, n = 0, len(glob) + while i < n: + if glob[i:i + 3] == "**/": + out.append("(?:.*/)?") + i += 3 + elif glob[i] == "*": + out.append(".*") + i += 1 + elif glob[i] == "?": + out.append(".") + i += 1 + elif glob[i] == "[": + # fnmatch class syntax: leading '!' negates (regex '^', not '!'), + # and a ']' immediately after '[' or '[!' is a literal member + # rather than the closing bracket. + j = i + 1 + if j < n and glob[j] == "!": + j += 1 + if j < n and glob[j] == "]": + j += 1 + while j < n and glob[j] != "]": + j += 1 + if j >= n: + out.append(re.escape("[")) + i += 1 + else: + body = glob[i + 1:j].replace("\\", "\\\\") + if body.startswith("!"): + body = "^" + body[1:] + elif body[:1] in ("^", "["): + body = "\\" + body + out.append("[" + body + "]") + i = j + 1 + else: + out.append(re.escape(glob[i])) + i += 1 + # fnmatch.fnmatch() case-normalizes via os.path.normcase before matching, + # which lowercases on Windows and is a no-op elsewhere (including macOS, + # despite its default case-insensitive filesystem — normcase only knows + # about nt vs. posix). Match that platform split so a rule doesn't + # silently stop firing depending on file casing. + flags = re.IGNORECASE if os.name == "nt" else 0 + return re.compile("(?s:" + "".join(out) + ")\\Z", flags) + + +def _segment_match(pat_segs: List[str], path_segs: List[str]) -> bool: + """Match a glob split on ``/`` against a path split on ``/``, where a + standalone ``**`` segment consumes zero or more whole path segments. + + A single regex with an optional ``(?:.*/)?`` for ``**/`` is not enough: + since the segment after it (e.g. ``[!t]*.ts``) may itself contain an + unrestricted ``*`` that also crosses ``/`` (required for fnmatch + compatibility — see ``_glob_to_regex``), the engine can backtrack into + skipping the ``**/`` match entirely and let that later ``*`` swallow the + real directory boundary instead. That lets a per-segment check like a + negated class end up applied to the wrong character — e.g. + ``**/[!t]*.ts`` would wrongly match ``src/t.ts``, silently admitting the + exact file the rule meant to exclude. Matching segment-by-segment, with + ``**`` structurally bounded to whole segments, has no such backtracking + escape hatch.""" + if not pat_segs: + return not path_segs + head, rest = pat_segs[0], pat_segs[1:] + if head == "**": + if _segment_match(rest, path_segs): + return True + return bool(path_segs) and _segment_match(pat_segs, path_segs[1:]) + if not path_segs: + return False + return bool(_glob_to_regex(head).match(path_segs[0])) and _segment_match(rest, path_segs[1:]) + + +def _pattern_matches(glob: str, s: str) -> bool: + """Match one glob against one string, routing standalone ``**`` segments + through the segment-aware matcher and everything else through the + single-regex translator (which already matches fnmatch exactly).""" + segs = glob.split("/") + if "**" in segs: + return _segment_match(segs, s.split("/")) + return bool(_glob_to_regex(glob).match(s)) + + +def _project_relative(path: str, cwd: Optional[str]) -> str: + """Canonicalize a hook-delivered path to project-relative form, so that + an absolute payload (the normal case: tool hooks report edited files by + absolute path) and a relative payload for the same file produce the same + include/exclude decision against project-relative globs like ``src/*.ts``. + + Paths outside the project root, or when ``cwd`` is unknown, are left as + delivered (normalized to ``/`` separators) rather than guessing via + prefix-stripping.""" + if cwd and os.path.isabs(path): + try: + root = os.path.abspath(cwd) + rel = os.path.relpath(os.path.abspath(path), root) + except ValueError: + rel = None # e.g. different drive on Windows + if rel is not None and rel != os.pardir and not rel.startswith(os.pardir + os.sep): + return rel.replace(os.sep, "/") + return path.replace(os.sep, "/") + + +def _glob_match( + path: str, include: Tuple[str, ...], exclude: Tuple[str, ...], cwd: Optional[str] = None +) -> bool: """Match a path against include/exclude globs. ``**`` matches any depth.""" - norm = path.replace(os.sep, "/") + norm = _project_relative(path, cwd) base = os.path.basename(norm) def _hit(globs: Tuple[str, ...]) -> bool: - return any( - fnmatch.fnmatch(norm, g) or fnmatch.fnmatch(base, g) for g in globs - ) + return any(_pattern_matches(g, norm) or _pattern_matches(g, base) for g in globs) if include and not _hit(include): return False if exclude and _hit(exclude): diff --git a/plugins/security-guidance/hooks/gitutil.py b/plugins/security-guidance/hooks/gitutil.py index 1a549d76c6..bee7b7303f 100644 --- a/plugins/security-guidance/hooks/gitutil.py +++ b/plugins/security-guidance/hooks/gitutil.py @@ -20,6 +20,7 @@ import subprocess from _base import debug_log +import secretpaths GIT_CMD = [ @@ -548,6 +549,10 @@ def _score(item): def _is_reviewable_source(file_path): + # The session's Read deny rules and well-known secret files come first: + # a denied `config/prod.json` has a reviewable extension. + if secretpaths.is_excluded(file_path): + return False # Normalize for component matching: a path like `.next/x.js` or # `pkg/node_modules/y.ts` should both be excluded; matching against # `'/' + path` lets each pattern be checked as `'/' + p in '/' + path` diff --git a/plugins/security-guidance/hooks/llm.py b/plugins/security-guidance/hooks/llm.py index eff56de741..f1a3904f3f 100644 --- a/plugins/security-guidance/hooks/llm.py +++ b/plugins/security-guidance/hooks/llm.py @@ -27,6 +27,7 @@ import extensibility import review_api +import secretpaths from _base import debug_log, _record_usage, _PV, PROVENANCE_TAG # noqa: F401 from session_state import with_locked_state @@ -276,8 +277,9 @@ def _call_claude_via_sdk(prompt, output_schema, *, max_tokens=16000, model=None) 3P providers. Uses the same `output_format` JSON-schema contract so the return value shape is identical (parsed dict or None). - No tools (`allowed_tools=[]`) — the security review only needs structured - output, not Read/Grep/Glob. Single turn keeps cost predictable. + No tools — the security review only needs structured output, so the + file and shell tools are passed as `disallowed_tools`. Single turn keeps + cost predictable. """ global _last_call_claude_http_error _last_call_claude_http_error = None @@ -330,6 +332,11 @@ async def _arun(): system_prompt=CLAUDE_CODE_SYSTEM_PROMPT, cli_path=cli_path, allowed_tools=[], + # allowed_tools=[] grants nothing but removes nothing either: + # Read inside cwd and read-only shell commands are auto-approved + # in default mode. This call needs no tools, so take the file and + # shell tools away outright. + disallowed_tools=secretpaths.no_file_tools(), setting_sources=[], max_turns=2, model=chosen_model, @@ -607,28 +614,7 @@ def _format_vulns_guidance(vulns: List[Dict[str, Any]]) -> Optional[str]: """ if not vulns: return None - severity_order = {"critical": 0, "high": 1, "medium": 2} - vulns = sorted(vulns, key=lambda v: severity_order.get(v.get("severity", "medium"), 2)) - by_file: Dict[str, list] = {} - for v in vulns: - by_file.setdefault(v.get("filePath", "unknown"), []).append(v) - lines = [ - "Security Review: Potential vulnerabilities detected", - "", - f"Affected files: {', '.join(by_file)}", - "The following issues were flagged by automated security review. Address each, or briefly note why it doesn't apply. Valid reasons to proceed without changes: the user explicitly asked for this and you've already surfaced the security tradeoffs, or the pattern isn't actually exploitable in this context. Do not dismiss findings solely because the service is internal-only — internal services are common SSRF/IDOR targets:", - "", - ] - n = 1 - for fp, vs in by_file.items(): - lines.append(f" {fp}:") - for v in vs: - sev = (v.get("severity") or "medium").upper() - lines.append(f" {n}. [{sev}] [{v.get('category', 'Unknown')}] {v.get('vulnerableCode', 'N/A')}") - lines.append(f" Suggested fix: {v.get('fix', 'N/A')}") - lines.append("") - n += 1 - return "\n".join(lines) + return review_api.format_findings(vulns) # CC truncates the rewakeSummary override at 300 chars. Cap a little under so @@ -1126,6 +1112,8 @@ def agentic_review( # trace cross-file data flow. The harness sets SG_AGENTIC_CONTEXT_DIR to a # full repo (worktree at the commit, or the live clone at HEAD). context_dir = os.environ.get("SG_AGENTIC_CONTEXT_DIR") or repo_dir + disallowed = secretpaths.subagent_disallowed_tools(context_dir) + debug_log(f"agentic_review: {len(disallowed)} disallowed Read pattern(s) for the sub-agent") context_note = "" if context_dir != repo_dir: context_note = ( @@ -1219,6 +1207,13 @@ async def _arun(system: str, prompt: str, *, schema: Dict[str, Any], # would trip our own agent-permission-bypass guidance). Leaving # permission_mode unset means an accidental future addition of # a write/exec tool to allowed_tools is caught by the gate. + # + # setting_sources=[] keeps the parent's plugins and hooks (this + # one included) from loading recursively, but it also drops the + # parent's permission rules. The session's Read deny/ask rules + # and the well-known secret-file globs are re-applied here so the + # reviewer cannot pull a denied file into model context. + disallowed_tools=disallowed, setting_sources=[], max_turns=turns if turns is not None else max_turns, model=model, @@ -1688,9 +1683,10 @@ def analyze_security_concerns(files: List[Tuple[str, str]], is_diff: bool = Fals lines.append("") for i, concern in enumerate(concerns, 1): severity = concern.get('severity', 'high').upper() - lines.append(f" {i}. [{severity}] [{concern.get('category', 'Unknown')}] {concern.get('area', '')}") - lines.append(f" Evidence: {concern.get('evidenceLine', 'N/A')}") - lines.append(f" Check: {concern.get('concern', '')}") + cat = concern.get('category', 'Unknown') + lines.append(f" {i}. [{severity}] [{cat}] {review_api.redact_secret_values(concern.get('area', ''), category=cat)}") + lines.append(f" Evidence: {review_api.redact_secret_values(concern.get('evidenceLine', 'N/A'), category=cat)}") + lines.append(f" Check: {review_api.redact_secret_values(concern.get('concern', ''), category=cat)}") lines.append("") return "\n".join(lines) diff --git a/plugins/security-guidance/hooks/patterns.py b/plugins/security-guidance/hooks/patterns.py index b7f6f10a60..737532645e 100644 --- a/plugins/security-guidance/hooks/patterns.py +++ b/plugins/security-guidance/hooks/patterns.py @@ -94,6 +94,7 @@ }, { "ruleName": "new_function_injection", + "path_filter": lambda p: not p.endswith(_DOC_EXTS), "substrings": ["new Function"], "reminder": "\u26a0\ufe0f Security Warning: Using new Function() with string interpolation is a CODE INJECTION vulnerability. If any variable is concatenated or interpolated into the function body string, an attacker controlling that variable can execute arbitrary code. Use safe alternatives: for property access use obj[key] or array.reduce((o, k) => o[k], root); for computation use a safe expression parser. NEVER interpolate untrusted strings into new Function() bodies.", }, @@ -107,16 +108,19 @@ }, { "ruleName": "react_dangerously_set_html", + "path_filter": lambda p: not p.endswith(_DOC_EXTS), "substrings": ["dangerouslySetInnerHTML"], "reminder": "⚠️ Security Warning: dangerouslySetInnerHTML can lead to XSS vulnerabilities if used with untrusted content. Ensure all content is properly sanitized using an HTML sanitizer library like DOMPurify, or use safe alternatives.", }, { "ruleName": "document_write_xss", + "path_filter": lambda p: not p.endswith(_DOC_EXTS), "substrings": ["document.write"], "reminder": "⚠️ Security Warning: document.write() can be exploited for XSS attacks and has performance issues. Use DOM manipulation methods like createElement() and appendChild() instead.", }, { "ruleName": "innerHTML_xss", + "path_filter": lambda p: not p.endswith(_DOC_EXTS), "substrings": [".innerHTML =", ".innerHTML="], "reminder": "⚠️ Security Warning: Setting innerHTML with untrusted content can lead to XSS vulnerabilities. Use textContent for plain text or safe DOM methods for HTML content. If you need HTML support, consider using an HTML sanitizer library such as DOMPurify.", }, diff --git a/plugins/security-guidance/hooks/review_api.py b/plugins/security-guidance/hooks/review_api.py index 499336b96f..923c296fec 100644 --- a/plugins/security-guidance/hooks/review_api.py +++ b/plugins/security-guidance/hooks/review_api.py @@ -16,6 +16,7 @@ import json import os +import re from typing import Any import extensibility @@ -344,6 +345,362 @@ def _intersects(cand: dict[str, Any]) -> bool: return candidates +# --------------------------------------------------------------------------- +# Secret-value redaction for feedback text +# --------------------------------------------------------------------------- +# +# Findings quote the diff (`vulnerableCode`, `fix`, `evidenceLine`), and the +# formatted block is fed back into the main conversation and kept in session +# state. For a hardcoded-secret finding that quote IS the secret, so a value +# that only ever lived in a working-tree file gets copied into model context, +# the transcript and ~/.claude. The reviewer is deliberately NOT asked to +# elide values itself: diff anchoring, dedupe and the refute pass all compare +# the quoted code against the diff. Masking happens here, on the way out. +# Keep the key name and the last two characters so the finding still points +# at the right line; when unsure, mask: a garbled evidence line costs less +# than an echoed credential. Pattern matching, not a guarantee — the README +# says so and points at Read(...) deny rules for files that must never leave. + +# Identifier segments that make an assignment's left-hand side a credential. +_SECRET_KEY_WORDS = frozenset(( + "password", "passwd", "pwd", "pass", "passphrase", "secret", "secrets", + "token", "apikey", "apitoken", "authtoken", "accesstoken", "refreshtoken", + "secretkey", "privatekey", "accesskey", "signingkey", "masterkey", + "credential", "credentials", "creds", "authorization", "bearer", "dsn", + "connectionstring", "connstr", "connstring", +)) +# Joined spellings: PGPASSWORD, DBPASSWORD, CLIENTSECRET, GHTOKEN. +_SECRET_KEY_SUFFIXES = ("password", "passwd", "passphrase", "secret", "token", "apikey") +_SECRET_KEY_PAIRS = frozenset(( + ("api", "key"), ("private", "key"), ("secret", "key"), ("access", "key"), + ("signing", "key"), ("master", "key"), ("encryption", "key"), + ("client", "secret"), ("auth", "token"), ("auth", "key"), + ("conn", "str"), ("connection", "string"), +)) +# ...unless another segment says the value is metadata about a credential. +_NOT_SECRET_KEY_WORDS = frozenset(( + "timeout", "expiry", "expires", "expire", "expiration", "ttl", "age", + "length", "len", "max", "min", "size", "count", "limit", "attempts", + "url", "uri", "endpoint", "host", "path", "file", "filename", "dir", + "name", "names", "id", "ids", "type", "field", "fields", "header", "param", + "hash", "hashed", "hasher", "hashers", "digest", "alg", "algorithm", + "reset", "csrf", "xsrf", "enabled", "disabled", "required", "use", + "policy", "rotation", "validator", "validators", "regex", "pattern", +)) +# A dotted "key" ending in one of these is a file name (`secrets.yml:12`). +_FILE_EXTENSIONS = frozenset(( + "yml", "yaml", "json", "toml", "ini", "cfg", "conf", "env", "properties", + "xml", "txt", "md", "py", "js", "ts", "jsx", "tsx", "go", "rb", "java", + "kt", "rs", "ex", "exs", "php", "cs", "sh", "swift", "scala", "c", "h", + "cpp", "tf", "tfvars", "sql", "lock", "pem", "key", +)) +_BARE_VALUE_SKIP = frozenset(( + "true", "false", "null", "none", "nil", "yes", "no", "required", + "optional", "string", "str", "redacted", "await", "new", +)) +_CAMEL_RE = re.compile(r"(?<=[a-z0-9])(?=[A-Z])") +_SEGMENT_SPLIT_RE = re.compile(r"[_\-.$\s]+") +# Assignment/comparison operator. The key before it is found by walking back +# over identifier characters, which keeps the scan linear on hostile input +# (the quoted code comes from the repository under review). +_ASSIGN_OP_RE = re.compile(r"(?:[!=]?==|!=|=>|:=|[:=])[ \t]*") +_KEY_CHARS = frozenset("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_$.-") +# `password: str = "..."`, `apiKey: string = '...'`, `password: &str = "..."`. +_TYPE_ANNOTATION_RE = re.compile(r"[&*]?[A-Za-z_][\w.<>\[\]&|?]*(?:[ \t]+=(?!=)[ \t]*|=(?=[\"'bfru@]))") +_STRING_PREFIX_RE = re.compile(r"(?:[bBfFrRuU]{1,2}|@)(?=[\"'])") +_AUTH_SCHEME_RE = re.compile(r"(?i)(?:basic|bearer|token|digest|apikey|negotiate)[ \t]+") +_BARE_TOKEN_RE = re.compile(r"[^\s,;&)}\]\"'`]+") +_DOTTED_REFERENCE_RE = re.compile(r"[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)+$") +_CONSTANT_NAME_RE = re.compile(r"[A-Z][A-Z0-9_]*$") +# What may precede a key that opens a config line: indentation, a diff or +# list marker, a quote, a `file:line:` prefix, `export`/`ENV`/`set`. +_LINE_LEAD_RE = re.compile( + r"[\s+>\"'-]*(?:[\w./\\-]+:\d+:[ \t]*)?(?:(?:export|ENV|ARG|set|SET|setx)[ \t]+)?$" +) +# Dockerfile `ENV DB_PASSWORD value` (the `=`-less form). +_DOCKER_ENV_RE = re.compile(r"(?m)^([ \t+-]*(?:ENV|ARG)[ \t]+([A-Za-z_]\w*)[ \t]+)([^\s=\"']+)$") +# value style markup. +_XML_VALUE_RE = re.compile(r"(<([A-Za-z_][\w.-]*)[^>]*>)([^<]{3,})( outside an Authorization assignment (prose, logs). +_BEARER_RE = re.compile(r"(?i)(\bbearer\s+)((?=[A-Za-z0-9._~+/=-]*\d)[A-Za-z0-9._~+/=-]{8,})") +# scheme://user:password@host and scheme://:password@host +_URL_CRED_RE = re.compile( + r"((? str: + return "****" + value[-2:] if len(value) >= 8 else "****" + + +def _looks_like_path(text: str) -> bool: + if text.startswith(("/", "./", "../", "~/")) or "://" in text: + return True + segments = [seg for seg in re.split(r"[/\\]", text) if seg] + return len(segments) > 1 and ( + all(_PATH_SEGMENT_RE.match(seg) for seg in segments) + or any(_FILE_EXT_RE.search(seg) for seg in segments) + ) + + +def _keep_assigned(value: str) -> bool: + """A quoted right-hand side is a literal; keep it only when it is a + template/reference placeholder or a path, never for its shape.""" + return bool( + value.startswith(("****", "$", "%(", "<", "{{", "~/")) + or ("{" in value and "}" in value) or "%s" in value or "$(" in value + or ("/" in value and _looks_like_path(value)) + or value.lower() in _BARE_VALUE_SKIP + ) + + +def _keep_quoted(inner: str) -> bool: + """True for a quoted string that reads as a name, reference, path, URL + or template rather than a value: those carry the finding's location.""" + return bool( + inner.startswith("****") + or _LABEL_RE.match(inner) + or ("{" in inner and "}" in inner) or "%s" in inner or "$(" in inner + or _QUOTED_KEEP_RE.match(inner) + or _DOTTED_REFERENCE_RE.match(inner) + or _FILE_EXT_RE.search(inner) + or (("/" in inner or "\\" in inner) and _looks_like_path(inner)) + ) + + +def _is_secret_key(key: str) -> bool: + segments = [ + seg for seg in _SEGMENT_SPLIT_RE.split(_CAMEL_RE.sub("_", key).lower()) if seg + ] + if not segments or any(seg in _NOT_SECRET_KEY_WORDS for seg in segments): + return False + if "." in key and key.rsplit(".", 1)[-1].lower() in _FILE_EXTENSIONS: + return False + for seg in segments: + if seg in _SECRET_KEY_WORDS: + return True + if not seg.startswith(("csrf", "xsrf")) and seg.endswith(_SECRET_KEY_SUFFIXES) \ + and any(0 < len(seg) - len(w) <= 6 for w in _SECRET_KEY_SUFFIXES if seg.endswith(w)): + return True + return any(pair in _SECRET_KEY_PAIRS for pair in zip(segments, segments[1:])) + + +def _is_secret_category(category: str) -> bool: + return bool(_SECRET_CATEGORY_RE.search(category)) and not _NOT_SECRET_CATEGORY_RE.search(category) + + +def _mask_assignments(s: str) -> str: + """`key = value`, `key: value`, `"key": "value"`, `cfg['key'] = 'value'`, + `key: Type = "value"`, `key == 'value'` where the key names a credential. + A quoted value is masked whole (spaces, punctuation and all); a bare + value is masked as one token, or to end of line when the key opens the + line (YAML, .env, ini, a quoted `+` diff line).""" + out: list[str] = [] + pos = 0 + length = len(s) + while True: + m = _ASSIGN_OP_RE.search(s, pos) + if not m: + out.append(s[pos:]) + return "".join(out) + i = m.end() + out.append(s[pos:i]) + pos = i + # Walk back: spaces, an optional quote/bracket closing a subscript or + # JSON key, then the identifier itself. + j = m.start() + while j > 0 and s[j - 1] in " \t": + j -= 1 + key_quote = "" + if j > 0 and s[j - 1] == "]": + j -= 1 + if j > 0 and s[j - 1] in "\"'": + key_quote = s[j - 1] + j -= 1 + key_end = key_start = j + while key_start > 0 and s[key_start - 1] in _KEY_CHARS: + key_start -= 1 + while key_start < key_end and s[key_start] in "-.$0123456789": + key_start += 1 + key = s[key_start:key_end] + if i >= length or not key or not _is_secret_key(key): + continue + ch = s[i] + line_start = s.rfind("\n", 0, key_start) + 1 + # `log("password=" + pw)`, `"...?api_key=" + key`: an odd number of + # quotes before this one means it closes the literal the key sits in, + # and what follows is code. + if not key_quote and ch in "\"'" and s.count(ch, line_start, i) % 2 == 1: + continue + if m.group(0).rstrip() == ":": + typed = _TYPE_ANNOTATION_RE.match(s, i) + if typed: + out.append(typed.group(0)) + pos = i = typed.end() + if i >= length: + continue + ch = s[i] + prefix_m = _STRING_PREFIX_RE.match(s, i) + if prefix_m: + out.append(prefix_m.group(0)) + pos = i = prefix_m.end() + ch = s[i] + if ch in "\"'`": + close = s.find(ch, i + 1) + newline = s.find("\n", i + 1) + if close == -1 or (newline != -1 and newline < close): + close = newline if newline != -1 else length + inner = s[i + 1:close] + scheme = _AUTH_SCHEME_RE.match(inner) + prefix = scheme.group(0) if scheme else "" + value = inner[len(prefix):] + if value and not _keep_assigned(value): + value = _mask(value) + out.append(ch + prefix + value) + pos = close + continue + scheme = _AUTH_SCHEME_RE.match(s, i) + if scheme: + out.append(scheme.group(0)) + pos = i = scheme.end() + token_m = _BARE_TOKEN_RE.match(s, i) + if not token_m: + continue + token = token_m.group(0) + end = token_m.end() + if token.startswith(("****", "$", "{", "<", "%", "&", "*", "@")) or "(" in token \ + or "[" in token or token.lower() in _BARE_VALUE_SKIP \ + or _DOTTED_REFERENCE_RE.match(token): + continue + opens_line = bool(_LINE_LEAD_RE.match(s[line_start:key_start])) + # `-H 'X-Api-Key: value'`: the key opens a quoted string, the value ends it. + opens_string = not key_quote and key_start > line_start and s[key_start - 1] in "\"'" + has_digit = any(c.isdigit() for c in token) + # In code a bare right-hand side is a reference; bare literals live in + # config lines (YAML, .env, ini, Dockerfile), where the key opens the + # line or a quoted string. Mid-line, only a token carrying digits + # reads as a value, and an ALL_CAPS one reads as a constant's name. + if not (opens_line or opens_string) and (_CONSTANT_NAME_RE.match(token) or not has_digit): + continue + # `key: v a l u e` runs on to the end of the line (or of the string); + # `KEY=value cmd`, a shell prefix assignment, is one token. + if (opens_line or opens_string) and (m.group(0)[-1] in " \t" or m.group(0).startswith(":")): + line_end = s.find("\n", end) + line_end = length if line_end == -1 else line_end + if opens_string: + close = s.find(s[key_start - 1], end, line_end) + line_end = close if close != -1 else line_end + rest = s[end:line_end] + comment = rest.find(" #") + if comment != -1: + rest = rest[:comment] + if len(rest.split()) > 3 and not opens_string: + if not has_digit: + continue # more than a few words after `Key:` is a sentence + elif "(" not in rest and "[" not in rest: + end += len(rest.rstrip()) + token = s[i:end] + out.append(_mask(token)) + pos = end + + +def _mask_docker_env(m: "re.Match[str]") -> str: + value = m.group(3) + if not _is_secret_key(m.group(2)) or value.startswith(("$", "{", "<", "****")): + return m.group(0) + return m.group(1) + _mask(value) + + +def _mask_xml(m: "re.Match[str]") -> str: + value = m.group(3).strip() + if not _is_secret_key(m.group(2)) or value.lower() in _BARE_VALUE_SKIP \ + or value.startswith(("$", "{", "%", "<", "****")): + return m.group(0) + return m.group(1) + _mask(value) + m.group(4) + + +def _mask_quoted(m: "re.Match[str]") -> str: + inner, text, start, end = m.group(2), m.string, m.start(), m.end() + after = text[end:end + 4].lstrip(" \t") + is_key = after[:1] in (":", "=") and after[1:2] not in ("=", ">", ":") + is_subscript = text[max(0, start - 1):start] == "[" and text[end:end + 1] == "]" + is_attribute_name = _ATTRIBUTE_NAME_RE.search(text[max(0, start - 8):start]) + if is_key or is_subscript or is_attribute_name or _keep_quoted(inner): + return m.group(0) + return m.group(1) + _mask(inner) + m.group(3) + + +def _mask_opaque(m: "re.Match[str]") -> str: + run, text = m.group(0), m.string + if text[m.end():m.end() + 1] == "(" or text[max(0, m.start() - 7):m.start()].lower() in ("commit ", "sha "): + return run # a call site / a commit reference, not a value + digits = sum(ch.isdigit() for ch in run) + letters = sum(ch.isalpha() for ch in run) + mixed_case = run.lower() != run and run.upper() != run + opaque = letters >= 2 and (digits >= 2 or (digits and mixed_case and len(run) >= 32)) + if not opaque or ("/" in run and _looks_like_path(run)): + return run + return _mask(run) + + +def redact_secret_values(text: Any, *, category: Any = "") -> str: + """Mask credential-shaped values in a finding field before it is echoed + into the main conversation or stored. ``category`` is the finding's own + category; one that names an exposed credential turns on the broad masks.""" + s = str(text or "") + if not s: + return s + s = _PEM_BODY_RE.sub(lambda m: m.group(1) + " **** " + m.group(2), s) + s = _TOKEN_SHAPES_RE.sub(lambda m: _mask(m.group(0)), s) + s = _URL_CRED_RE.sub(lambda m: m.group(1) + _mask(m.group(2)) + m.group(3), s) + s = _CURL_USER_RE.sub(lambda m: m.group(1) + _mask(m.group(2)), s) + s = _CLI_FLAG_RE.sub(lambda m: m.group(1) + _mask(m.group(2)), s) + s = _BEARER_RE.sub(lambda m: m.group(1) + _mask(m.group(2)), s) + s = _mask_assignments(s) + s = _DOCKER_ENV_RE.sub(_mask_docker_env, s) + s = _XML_VALUE_RE.sub(_mask_xml, s) + if _is_secret_category(str(category or "")): + s = _QUOTED_RE.sub(_mask_quoted, s) + s = _OPAQUE_RUN_RE.sub(_mask_opaque, s) + return s + + _SEVERITY_ORDER = {"critical": 0, "high": 1, "medium": 2, "low": 3} @@ -366,7 +723,9 @@ def filter_by_severity( def format_findings(findings: list[dict[str, Any]]) -> str: - """Render findings as the same text block the CC plugin emits to Claude.""" + """Render findings, most severe first, as the text block the CC plugin + emits to Claude. Quoted code goes through ``redact_secret_values``.""" + findings = sorted(findings, key=lambda v: _SEVERITY_ORDER.get(v.get("severity", "medium"), 2)) by_file: dict[str, list[dict[str, Any]]] = {} for v in findings: by_file.setdefault(v.get("filePath", "unknown"), []).append(v) @@ -388,11 +747,14 @@ def format_findings(findings: list[dict[str, Any]]) -> str: lines.append(f" {fp}:") for v in vs: sev = (v.get("severity") or "medium").upper() + cat = v.get("category", "Unknown") + lines.append( + f" {n}. [{sev}] [{cat}] " + f"{redact_secret_values(v.get('vulnerableCode', 'N/A'), category=cat)}" + ) lines.append( - f" {n}. [{sev}] [{v.get('category', 'Unknown')}] " - f"{v.get('vulnerableCode', 'N/A')}" + f" Suggested fix: {redact_secret_values(v.get('fix', 'N/A'), category=cat)}" ) - lines.append(f" Suggested fix: {v.get('fix', 'N/A')}") lines.append("") n += 1 return "\n".join(lines) diff --git a/plugins/security-guidance/hooks/secretpaths.py b/plugins/security-guidance/hooks/secretpaths.py new file mode 100644 index 0000000000..327e2e9615 --- /dev/null +++ b/plugins/security-guidance/hooks/secretpaths.py @@ -0,0 +1,371 @@ +"""Paths the security reviewer must not see. + +Two sources, one answer: + +1. The session's own ``Read`` deny/ask permission rules, read from the same + settings files Claude Code loads (managed, user, project, project-local). + The main session enforces these, but the review prompt is assembled here + from ``git diff`` and the reviewer's SDK sub-agents run with + ``setting_sources=[]``, so neither knew about them. +2. A built-in list of well-known secret-file names (``.env``, private keys, + credential stores). On by default; ``SG_SKIP_SECRET_FILES=0`` opts out. + +``is_excluded()`` drops such files from every review diff (via +``gitutil._is_reviewable_source``) and ``subagent_disallowed_tools()`` hands +the same rules to the SDK sub-agents as ``disallowed_tools`` so they cannot +``Read``/``Grep``/``Glob`` them, nor reach them through a shell. + +Not covered: rules that only exist in MDM/registry policy, server-managed +settings, or the parent's ``--settings``/``--disallowedTools`` flags — a hook +subprocess cannot see those. The README says so. +""" + +import fnmatch +import json +import os +import re +import sys +from typing import List, Optional, Pattern, Tuple + +from _base import debug_log + +# Basename globs (case-insensitive) for files whose purpose is to hold +# credentials. Deliberately conservative: names that are secret stores by +# convention, not names that merely sound sensitive. Users extend this with +# ordinary `Read(...)` deny rules. +SECRET_BASENAME_GLOBS = ( + ".env", ".env.*", "*.env", ".envrc", + "*.pem", "*.key", "*.p12", "*.pfx", "*.jks", "*.keystore", "*.ppk", "*.kdbx", + "id_rsa*", "id_dsa*", "id_ecdsa*", "id_ed25519*", + ".netrc", "_netrc", ".pgpass", ".git-credentials", ".npmrc", ".pypirc", + ".htpasswd", "credentials", "kubeconfig", +) +# `secrets.yaml` / `credentials.json` style stores. Restricted to data +# extensions so `secrets.py` / `credentials.ts` (code that HANDLES secrets — +# exactly what the reviewer should read) stay reviewable. +_SECRET_STEMS = ("secret", "secrets", "credential", "credentials") +_DATA_EXTS = ( + ".json", ".yaml", ".yml", ".toml", ".ini", ".cfg", ".conf", ".env", + ".properties", ".xml", ".txt", +) + +# Shell tools reach file contents without consulting Read(...) rules — their +# read-only commands (`cat`, `git show HEAD:path`) are auto-approved — and the +# reviewer needs none of them. +_SHELL_TOOLS = ["Bash"] + (["PowerShell"] if sys.platform == "win32" else []) +_READ_TOOLS = ("Read", "Grep", "Glob") +_RULE_RE = re.compile(r"^\s*([A-Za-z_][\w-]*)\s*(?:\((.*)\))?\s*$", re.DOTALL) + +# Session state: load_for_session() only records the cwd; the settings files +# are read on first use so hook events that never review (per-edit pattern +# checks) pay nothing. +_cwd: Optional[str] = None +_loaded = False +# (kind, root, pattern, compiled). kind "cwd": root-less rule, relative to the +# session cwd (also tried against the repo toplevel, where diff paths live); +# "project": `/x`, anchored at `root`; "abs": `//x` or `~/x`, anchored at `root`. +_rules: List[Tuple[str, str, str, Optional[Pattern[str]]]] = [] +_forward: List[str] = [] # rule strings for the sub-agent, `/x` made absolute +_deny_all_reads = False +_project_dir: Optional[str] = None +_toplevel: Optional[str] = None +_reported = set() # paths already named in the debug log + + +def skip_secret_files_enabled() -> bool: + return os.environ.get("SG_SKIP_SECRET_FILES", "1").strip().lower() not in ("0", "off", "false", "no") + + +def load_for_session(cwd: Optional[str]) -> None: + """Point the module at this hook invocation's cwd; rules load lazily.""" + global _cwd, _loaded, _rules, _forward, _deny_all_reads, _project_dir, _toplevel + _cwd = _norm_abs(cwd) if cwd else None + _loaded, _rules, _forward, _deny_all_reads = False, [], [], False + _project_dir = _toplevel = None + + +def is_excluded(file_path: str) -> bool: + """True if the reviewer must not see this file: a well-known secret file + (unless opted out) or one the session's Read deny/ask rules cover. + `file_path` is repo-toplevel-relative (as `git diff` prints it) or absolute. + Each withheld path is named once in the debug log, so a rule that hides + code from review is at least visible there.""" + reason = _exclusion_reason(file_path) + if reason and file_path not in _reported: + _reported.add(file_path) + debug_log(f"secretpaths: withheld from review ({reason}): {file_path}") + return reason is not None + + +def subagent_disallowed_tools(context_dir: str) -> List[str]: + """`disallowed_tools` for an SDK sub-agent rooted at `context_dir`: the + shell tools, the session's Read/Grep/Glob deny+ask rules, and the + secret-file globs. + + The sub-agent anchors root-less and `/x` patterns at its own cwd, which + can differ from the session cwd and project dir (a git toplevel above + them, or an alternate checkout), so those rules are also sent as absolute + copies for every root they should cover.""" + _ensure_loaded() + out = _SHELL_TOOLS + _forward + context = _norm_abs(context_dir) + session_cwd = _cwd or _project_dir + for kind, root, pattern, _rx in _rules: + if kind == "cwd": # verbatim copy already covers `context` itself + roots = {session_cwd, _mirrored(session_cwd, context)} - {context} + elif kind == "project": # _forward already covers the project dir + roots = {_mirrored(root, context)} - {root} + else: + continue + out.extend(f"Read(/{r}/{pattern})" for r in sorted(x for x in roots if x)) + if skip_secret_files_enabled(): + out.extend(f"Read({g})" for g in SECRET_BASENAME_GLOBS) + out.extend(f"Read({stem}{ext})" for stem in _SECRET_STEMS for ext in _DATA_EXTS) + return list(dict.fromkeys(out)) + + +def no_file_tools() -> List[str]: + """`disallowed_tools` for a sub-agent that must not touch the filesystem at all.""" + return _SHELL_TOOLS + list(_READ_TOOLS) + + +# ── internals ─────────────────────────────────────────────────────────────── + + +def _exclusion_reason(file_path: str) -> Optional[str]: + if skip_secret_files_enabled() and _is_secret_basename(os.path.basename(file_path)): + return "well-known secret file" + _ensure_loaded() + if _deny_all_reads: + return "Read denied" + if not _rules: + return None + abs_path = _norm_abs(file_path if os.path.isabs(file_path) else os.path.join(_toplevel or os.getcwd(), file_path)) + for kind, root, _pattern, rx in _rules: + if rx is None: + continue + for base in ({_cwd or _norm_abs(os.getcwd()), _toplevel} if kind == "cwd" else {root}): + if not base: + continue + if not _outside(abs_path, base) and rx.match(os.path.relpath(abs_path, base).replace(os.sep, "/")): + return "Read deny/ask rule" + return None + + +def _is_secret_basename(base: str) -> bool: + low = base.lower() + if any(fnmatch.fnmatchcase(low, g) for g in SECRET_BASENAME_GLOBS): + return True + stem, ext = os.path.splitext(low) + return stem in _SECRET_STEMS and ext in _DATA_EXTS + + +def _ensure_loaded() -> None: + """Read Read/Grep/Glob deny+ask rules from every settings file the parent + session loads. A malformed file is debug-logged and skipped (Claude Code + does not enforce rules from a file it cannot parse either); any other + failure hides every file from the reviewer rather than risk showing it a + denied one.""" + global _loaded, _deny_all_reads + if _loaded: + return + _loaded = True + try: + seen = set() + for path, anchor in _settings_files(): + try: + # utf-8-sig: Claude Code strips a BOM before parsing, so must we. + with open(path, encoding="utf-8-sig") as f: + data = json.load(f) + except OSError: + continue + except ValueError as e: + debug_log(f"secretpaths: cannot parse {path}: {e}") + continue + perms = data.get("permissions") if isinstance(data, dict) else None + if not isinstance(perms, dict): + continue + # `ask` means "a human decides" — there is no human in the review + # sub-agent, so it counts as a deny there. + for key in ("deny", "ask"): + rules = perms.get(key) + for rule in rules if isinstance(rules, list) else []: + if isinstance(rule, str) and (rule, anchor) not in seen: + seen.add((rule, anchor)) + _add_rule(rule, anchor) + except Exception as e: + debug_log(f"secretpaths: failed to load permission rules ({e}); excluding all files") + _deny_all_reads = True + if _rules or _deny_all_reads: + debug_log(f"secretpaths: {len(_rules)} Read deny/ask pattern(s) loaded" + + (" (+ deny-all Read)" if _deny_all_reads else "")) + + +def _settings_files() -> List[Tuple[str, str]]: + """(settings file, anchor dir for its `/`-prefixed patterns) — the files + Claude Code itself loads: managed (+ drop-ins), user, the project dir's + settings.json / settings.local.json, and settings.local.json at the + canonical repo root (where Claude Code keeps it for linked worktrees). + Managed, project and local rules anchor at the project dir, user rules + at the config dir.""" + global _project_dir, _toplevel + from gitutil import _git_dir, _git_toplevel # gitutil imports this module + + config_dir = _norm_abs(os.environ.get("CLAUDE_CONFIG_DIR") or os.path.expanduser("~/.claude")) + _project_dir = _norm_abs(os.environ.get("CLAUDE_PROJECT_DIR") or _cwd or os.getcwd()) + top = _git_toplevel(_cwd or _project_dir) + _toplevel = _norm_abs(top) if top else (_cwd or _project_dir) + canonical = None + if top: + common = _git_dir(top) + if common and os.path.basename(os.path.normpath(common)) == ".git": + canonical = _norm_abs(os.path.dirname(os.path.normpath(common))) + managed = _managed_settings_dir() + files = [(os.path.join(managed, "managed-settings.json"), _project_dir)] + try: + dropins = sorted(os.listdir(os.path.join(managed, "managed-settings.d"))) + except OSError: + dropins = [] + files.extend((os.path.join(managed, "managed-settings.d", n), _project_dir) + for n in dropins if n.endswith(".json")) + files.append((os.path.join(config_dir, "settings.json"), config_dir)) + files.append((os.path.join(_project_dir, ".claude", "settings.json"), _project_dir)) + files.append((os.path.join(_project_dir, ".claude", "settings.local.json"), _project_dir)) + if canonical and canonical != _project_dir: + files.append((os.path.join(canonical, ".claude", "settings.local.json"), _project_dir)) + return files + + +def _mirrored(directory: Optional[str], context: str) -> Optional[str]: + """`directory`'s counterpart inside the checkout rooted at `context` (the + sub-agent's cwd): itself when the sub-agent runs inside this repo, else + the same toplevel-relative location under `context`.""" + if not directory or not _toplevel or not _outside(context, _toplevel): + return directory + rel = os.path.relpath(directory, _toplevel) + return None if _outside(directory, _toplevel) else _norm_abs(os.path.join(context, rel)) + + +def _outside(path: str, root: str) -> bool: + try: + rel = os.path.relpath(path, root) + except ValueError: # different Windows drive + return True + return rel == ".." or rel.startswith(".." + os.sep) + + +def _managed_settings_dir() -> str: + if sys.platform == "darwin": + return "/Library/Application Support/ClaudeCode" + if sys.platform == "win32": + return r"C:\Program Files\ClaudeCode" + return "/etc/claude-code" + + +def _norm_abs(path: str) -> str: + """Absolute path; on Windows in the `/c/Users/...` form Claude Code + matches against, so rules and paths compare in one notation.""" + p = os.path.abspath(path) + if os.name == "nt": + p = p.replace("\\", "/") + if re.match(r"^[A-Za-z]:", p): + p = "/" + p[0].lower() + p[2:] + return p + + +def _add_rule(rule: str, anchor: str) -> None: + global _deny_all_reads + m = _RULE_RE.match(rule) + if not m or m.group(1) not in _READ_TOOLS: + return + tool, pattern = m.group(1), (m.group(2) or "").strip() + if not pattern: + # Bare `Read` denies every read; bare `Grep`/`Glob` only remove a tool. + if tool == "Read": + _deny_all_reads = True + _forward.append(tool) + return + if tool != "Read": + # Grep(...)/Glob(...) content rules are not path rules; forward as-is. + _forward.append(rule.strip()) + return + # Same prefix conventions as Claude Code's patternWithRootFor. + if os.name == "nt" and re.match(r"^(~\\|\\(?![!#]))", pattern): + pattern = pattern.replace("\\", "/") + if os.name == "nt" and re.match(r"^[A-Za-z]:[/\\]", pattern): + pattern = "//" + pattern[0].lower() + pattern[2:].replace("\\", "/") + if pattern.startswith("//"): + kind, root, rel = "abs", "/", pattern[2:] + if os.name == "nt" and re.match(r"^[A-Za-z]/", rel): + root, rel = "/" + rel[0].lower(), rel[2:] + elif pattern.startswith("~/"): + kind, root, rel = "abs", _norm_abs(os.path.expanduser("~")), pattern[2:] + elif pattern.startswith("/"): + kind, root, rel = "project", anchor, pattern[1:] + rule = f"Read(/{anchor}/{rel})" + else: + kind, root, rel = "cwd", "", (pattern[2:] if pattern.startswith("./") else pattern) + # All three prefixed forms stay anchored at their root, as in Claude Code. + _rules.append((kind, root, rel, _compile(rel if kind == "cwd" else "/" + rel))) + _forward.append(rule.strip()) + + +def _compile(pattern: str) -> Optional[Pattern[str]]: + """gitignore-style pattern → regex over a root-relative POSIX path. + Approximates the `ignore` matcher Claude Code uses; where it differs it + errs toward matching (excluding) more: negations are skipped, directory + patterns also match a same-named file, and — as Claude Code itself does + for deny rules — a trailing `/**` is dropped, so `dir/**` means `dir` at + any depth.""" + p = pattern.strip() + if p.startswith("!") or p.startswith("#"): + return None + if p.endswith("/**"): + p = p[:-3] + p = p.rstrip("/") + if p.startswith("\\"): + p = p[1:] + anchored = "/" in p + p = p.lstrip("/") + if not p: + return None # `./`, `/`, `~/`: matches nothing, as in Claude Code + if p == "**": + return re.compile("") # everything under the root + out, i = "", 0 + while i < len(p): + c = p[i] + if p.startswith("**/", i): + out += "(?:.*/)?" + i += 3 + elif p.startswith("**", i): + out += ".*" + i += 2 + elif c == "*": + out += "[^/]*" + i += 1 + elif c == "?": + out += "[^/]" + i += 1 + elif c == "[": + j = p.find("]", i + 2) + if j == -1: + out += re.escape(c) + i += 1 + else: + body = p[i + 1:j] + if body.startswith("!"): + body = "^" + body[1:] + out += "[" + body.replace("\\", "\\\\") + "]" + i = j + 1 + elif c == "\\" and i + 1 < len(p): + out += re.escape(p[i + 1]) + i += 2 + else: + out += re.escape(c) + i += 1 + prefix = "^" if anchored else "^(?:.*/)?" + try: + return re.compile(prefix + out + "(?:/.*)?$", re.IGNORECASE) + except re.error: + debug_log(f"secretpaths: unusable pattern {pattern!r}; treating as match-all") + return re.compile("") diff --git a/plugins/security-guidance/hooks/security_reminder_hook.py b/plugins/security-guidance/hooks/security_reminder_hook.py index ffc9ba3c7d..273238d0ab 100755 --- a/plugins/security-guidance/hooks/security_reminder_hook.py +++ b/plugins/security-guidance/hooks/security_reminder_hook.py @@ -84,6 +84,7 @@ _PRICE_PER_MTOK, _PRICE_DEFAULT, _record_usage, _usage_metrics, ) import extensibility # noqa: E402 +import secretpaths # noqa: E402 from patterns import ( # noqa: E402,F401 _JS_EXTS, _PY_EXTS, _DOC_EXTS, _UNSAFE_DESERIALIZATION_REMINDER, _UNSAFE_YAML_LOAD_REMINDER, @@ -394,7 +395,13 @@ def check_patterns(file_path, content): # positive match condition (e.g. .github/workflows/). if "path_filter" in pattern: try: - if not pattern["path_filter"](normalized_path): + # Pass the un-stripped path (normalized_path has already lost + # its leading '/', which breaks os.path.isabs() for callers + # like extensibility._glob_match that need to tell an + # absolute payload from a relative one to canonicalize it + # against the project root). Existing path_filter callbacks + # are all endswith()-based and unaffected by a leading '/'. + if not pattern["path_filter"](file_path): continue except Exception: continue @@ -829,6 +836,20 @@ def is_commit_review_enabled(): COMMIT_REVIEW_ENABLED = is_commit_review_enabled() + +def _finding_snapshot(v): + """What previous_findings keeps of a finding. Stop and commit review + dedupe on (filePath, category); vulnerableCode only feeds the reviewer's + do-not-re-flag list, so it is stored masked — the state file must not + become a copy of a credential that lived in the working tree.""" + category = v.get("category", "Unknown") + return { + "filePath": v.get("filePath", ""), + "category": category, + "vulnerableCode": review_api.redact_secret_values(v.get("vulnerableCode", ""), category=category), + } + + def _agentic_review_with_race( repo_root: str, diff_files: List[Tuple[str, str]], @@ -1334,14 +1355,7 @@ def _read_previous(state): # Record new findings into shared state. Key on (filePath, category) — # vulnerableCode bytes drift between fires (diff context lines shift) so # matching on it under-dedupes; this aligns with Stop's _record_fire. - finding_snapshots = [ - { - "filePath": v.get("filePath", ""), - "category": v.get("category", "Unknown"), - "vulnerableCode": v.get("vulnerableCode", ""), - } - for v in new_vulns - ] + finding_snapshots = [_finding_snapshot(v) for v in new_vulns] def _record_findings(state): existing = [f for f in state.get("previous_findings", []) if isinstance(f, dict)] @@ -1662,12 +1676,7 @@ def handle_push_sweep_posttooluse(input_data): # push-sweep itself won't re-find them; leaving them out of # previous_findings keeps the door open for the per-commit hook to # surface them later if the code is touched again. - snapshots = [ - {"filePath": v.get("filePath", ""), - "category": v.get("category", "Unknown"), - "vulnerableCode": v.get("vulnerableCode", "")} - for v in reported - ] + snapshots = [_finding_snapshot(v) for v in reported] def _record(state): existing = [f for f in state.get("previous_findings", []) if isinstance(f, dict)] @@ -1881,14 +1890,7 @@ def _skip(reason, restore=False, **extra): concrete_guidance = _format_vulns_guidance(vulns) if concrete_guidance: - finding_snapshots = [ - { - "filePath": v.get("filePath", ""), - "category": v.get("category", "Unknown"), - "vulnerableCode": v.get("vulnerableCode", ""), - } - for v in vulns - ] + finding_snapshots = [_finding_snapshot(v) for v in vulns] # Update baseline so next stop hook iteration only sees new changes new_sha = capture_git_baseline(cwd) new_untracked_baseline = _list_untracked(cwd) if new_sha else None @@ -2051,6 +2053,7 @@ def main(): # per invocation. Failures are non-fatal (debug-logged) so a malformed # config never prevents the built-in checks from running. extensibility.load_for_session(input_data.get("cwd")) + secretpaths.load_for_session(input_data.get("cwd")) # Remote-pod SDK-bootstrap rescue: PostToolUse is the earliest hook event # that is guaranteed to fire *after* async plugin sync (its firing proves diff --git a/plugins/security-guidance/hooks/session_state.py b/plugins/security-guidance/hooks/session_state.py index 4ccd15e8e9..8ec5bf7301 100644 --- a/plugins/security-guidance/hooks/session_state.py +++ b/plugins/security-guidance/hooks/session_state.py @@ -109,7 +109,9 @@ def save_state(session_id, state): if state_dir: os.makedirs(state_dir, exist_ok=True) - with open(state_file, "w") as f: + # Write safely without following symlinks + fd = os.open(state_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC | getattr(os, "O_NOFOLLOW", 0), 0o600) + with os.fdopen(fd, "w") as f: json.dump(state, f) except (IOError, OSError) as e: debug_log(f"Failed to save state file {state_file}: {e}") diff --git a/plugins/security-guidance/hooks/sg-python.sh b/plugins/security-guidance/hooks/sg-python.sh index 5a1e8032e4..4deced991d 100755 --- a/plugins/security-guidance/hooks/sg-python.sh +++ b/plugins/security-guidance/hooks/sg-python.sh @@ -22,10 +22,46 @@ # "${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py" set -e +# Fast path: reuse the candidate a previous run already probed. Every probe is +# an extra interpreter launch, and on Windows the Python Install Manager +# aliases (py, python, python3 under WindowsApps) start an AppX update per +# activation that leaks memory in AppXSvc, so probing and then exec'ing doubles +# that cost on every hook (anthropics/claude-code#98929). Only the candidate +# *name* is cached, and only if it is one of the fixed names below, so the +# file's contents are never run as-is. The entry expires after a day, and is +# ignored when the command is no longer on PATH, so a changed or removed +# interpreter falls back to probing. +cache_file="${SG_PYTHON_CACHE:-${HOME:+$HOME/.claude/security/python-cmd}}" +if [ -n "$cache_file" ] && [ -f "$cache_file" ] \ + && [ -n "$(find "$cache_file" -mmin -1440 2>/dev/null)" ]; then + cached=$(head -n 1 "$cache_file" 2>/dev/null || true) + case "$cached" in + "python3"|"python"|"py -3") + if command -v "${cached%% *}" >/dev/null 2>&1; then + # shellcheck disable=SC2086 + exec $cached "$@" + fi + ;; + esac +fi + +# Capture probe stderr so the all-candidates-failed path can report useful +# diagnostics. Logging is best-effort: if the temp file cannot be created, +# fall back to the previous stderr-suppression behavior. +errlog="" +errlog=$(mktemp 2>/dev/null) || true +if [ -n "$errlog" ]; then + trap 'rm -f "$errlog"' EXIT +fi + probe() { # $1..N: the interpreter command (may be multi-word like `py -3`) # Probe writes the major version to stdout and exits 0 iff it's >=3. - "$@" -c 'import sys; print(sys.version_info[0])' 2>/dev/null + if [ -n "$errlog" ]; then + "$@" -c 'import sys; print(sys.version_info[0])' 2>>"$errlog" + else + "$@" -c 'import sys; print(sys.version_info[0])' 2>/dev/null + fi } for cmd in "python3" "python" "py -3"; do @@ -33,6 +69,17 @@ for cmd in "python3" "python" "py -3"; do # shellcheck disable=SC2086 v=$(probe $cmd) || continue if [ "$v" = "3" ]; then + if [ -n "$errlog" ]; then + rm -f "$errlog" + fi + # Best-effort: write via a temp name so concurrent hooks never read a + # half-written file, and never let a cache failure block the hook. + if [ -n "$cache_file" ]; then + { mkdir -p "$(dirname "$cache_file")" \ + && printf '%s\n' "$cmd" > "$cache_file.$$" \ + && mv -f "$cache_file.$$" "$cache_file"; } 2>/dev/null \ + || rm -f "$cache_file.$$" 2>/dev/null || true + fi # shellcheck disable=SC2086 exec $cmd "$@" fi @@ -40,5 +87,9 @@ done echo "security-guidance: no working Python 3 interpreter found." >&2 echo " tried: python3, python, py -3" >&2 +if [ -n "$errlog" ] && [ -s "$errlog" ]; then + echo " probe errors:" >&2 + sed 's/^/ /' "$errlog" >&2 +fi echo " on Windows, install Python from https://python.org (NOT the Microsoft Store)" >&2 exit 1 diff --git a/plugins/security-guidance/hooks/test_extensibility.py b/plugins/security-guidance/hooks/test_extensibility.py new file mode 100644 index 0000000000..03857bd6ab --- /dev/null +++ b/plugins/security-guidance/hooks/test_extensibility.py @@ -0,0 +1,188 @@ +"""Tests for the security-patterns.json glob matcher in extensibility.py. + +Run with: python3 test_extensibility.py (or: python3 -m unittest test_extensibility) + +Covers the two independent axes from +https://github.com/anthropics/claude-code/issues/86545 : + (a) an absolute, in-project payload and a relative payload for the same + file must produce the same include/exclude decision against a + project-relative glob (path canonicalization). + (b) ``**/`` must match zero path segments, not just one-or-more + (depth semantics). +""" + +import os +import sys +import tempfile +import unittest +from unittest import mock + +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +from extensibility import _glob_match # noqa: E402 + + +class DepthSemantics(unittest.TestCase): + """Axis (b): ``**`` matches any depth, including zero directories.""" + + def test_double_star_matches_top_level_file(self): + self.assertTrue(_glob_match("config.ts", ("**/*.ts",), ())) + + def test_double_star_matches_nested_file(self): + self.assertTrue(_glob_match("src/a.ts", ("**/*.ts",), ())) + + def test_double_star_does_not_match_other_extension(self): + self.assertFalse(_glob_match("config.py", ("**/*.ts",), ())) + + def test_mid_pattern_double_star_matches_zero_and_more_segments(self): + self.assertTrue(_glob_match("utils/x.ts", ("utils/**/*.ts",), ())) + self.assertTrue(_glob_match("utils/sub/x.ts", ("utils/**/*.ts",), ())) + self.assertFalse(_glob_match("other/x.ts", ("utils/**/*.ts",), ())) + + def test_exclude_still_applies_on_top_of_double_star_include(self): + self.assertTrue(_glob_match("config.ts", ("**/*.ts",), ("**/*.test.ts",))) + self.assertFalse( + _glob_match("config.test.ts", ("**/*.ts",), ("**/*.test.ts",)) + ) + + +class PathCanonicalization(unittest.TestCase): + """Axis (a): absolute in-project payloads and relative payloads for the + same file must agree, for directory-anchored (non-``**``) globs.""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = self._tmp.name + os.makedirs(os.path.join(self.root, "src"), exist_ok=True) + + def tearDown(self): + self._tmp.cleanup() + + def test_absolute_in_project_path_matches_directory_anchored_glob(self): + abs_path = os.path.join(self.root, "src", "f.ts") + self.assertTrue(_glob_match(abs_path, ("src/*.ts",), (), cwd=self.root)) + + def test_relative_payload_for_same_file_gets_same_decision(self): + rel_path = os.path.join("src", "f.ts") + self.assertTrue(_glob_match(rel_path, ("src/*.ts",), (), cwd=self.root)) + + def test_absolute_and_relative_payloads_agree(self): + abs_path = os.path.join(self.root, "src", "f.ts") + rel_path = os.path.join("src", "f.ts") + include = ("src/*.ts",) + self.assertEqual( + _glob_match(abs_path, include, (), cwd=self.root), + _glob_match(rel_path, include, (), cwd=self.root), + ) + + def test_double_star_and_directory_anchor_together_absolute(self): + nested = os.path.join(self.root, "src", "sub", "f.ts") + self.assertTrue(_glob_match(nested, ("src/**/*.ts",), (), cwd=self.root)) + + def test_absolute_path_outside_project_root_is_not_forced_to_match(self): + outside = os.path.join(tempfile.gettempdir(), "elsewhere", "src", "f.ts") + # Not under self.root, so canonicalization is skipped and the glob + # is compared against the path as delivered (normalized separators + # only) rather than a guessed relative form. + self.assertFalse(_glob_match(outside, ("src/*.ts",), (), cwd=self.root)) + + def test_no_cwd_falls_back_to_matching_path_as_delivered(self): + rel_path = os.path.join("src", "f.ts") + self.assertTrue(_glob_match(rel_path, ("src/*.ts",), (), cwd=None)) + abs_path = os.path.join(self.root, "src", "f.ts") + self.assertFalse(_glob_match(abs_path, ("src/*.ts",), (), cwd=None)) + + +class CharacterClassFnmatchCompat(unittest.TestCase): + """``[...]`` classes must keep fnmatch semantics: a leading ``!`` + negates (regex uses ``^``, which fnmatch gives no special meaning), and + a ``]`` immediately after ``[`` or ``[!`` is a literal member, not the + closing bracket. Each case is checked against real ``fnmatch`` so this + stays pinned to the semantics the plugin's existing patterns were + written against.""" + + CASES = [ + ("[!t]*.ts", "t.ts"), + ("[!t]*.ts", "x.ts"), + ("[]t]*.ts", "]x.ts"), + ("[]t]*.ts", "t.ts"), + ("[]t]*.ts", "a.ts"), + ("[![]*.ts", "[.ts"), + ("[![]*.ts", "a.ts"), + ("[a-z]*.ts", "a.ts"), + ("[a-z]*.ts", "A.ts"), + ("[^abc]*.ts", "^.ts"), + ("[^abc]*.ts", "a.ts"), + ("[!abc]*.ts", "a.ts"), + ("[!abc]*.ts", "x.ts"), + ] + + def test_matches_fnmatch_for_each_case(self): + import fnmatch + + for pattern, path in self.CASES: + with self.subTest(pattern=pattern, path=path): + expected = fnmatch.fnmatch(path, pattern) + actual = _glob_match(path, (pattern,), ()) + self.assertEqual(actual, expected) + + def test_negated_class_excludes_matching_file_from_security_rule(self): + # The motivating case: a rule meant to cover every .ts file except + # ones starting with 't' must not silently include t.ts. + self.assertFalse(_glob_match("t.ts", ("**/[!t]*.ts",), ())) + self.assertTrue(_glob_match("x.ts", ("**/[!t]*.ts",), ())) + + def test_negated_class_excludes_nested_matching_file_too(self): + # Regression: a single regex with an optional (?:.*/)? for '**/' let + # a later unrestricted '*' backtrack around the '**/' entirely, so + # the [!t] check landed on the wrong character and t.ts one level + # down (or more) was wrongly admitted even though bare t.ts was + # correctly excluded above. + self.assertFalse(_glob_match("src/t.ts", ("**/[!t]*.ts",), ())) + self.assertFalse(_glob_match("a/b/t.ts", ("**/[!t]*.ts",), ())) + self.assertTrue(_glob_match("src/x.ts", ("**/[!t]*.ts",), ())) + + +class StarAndQuestionMarkStayRecursive(unittest.TestCase): + """A bare ``*`` and ``?`` must keep crossing ``/`` exactly like + ``fnmatch`` always did — that recursion is relied on by real deployed + configs (see the #86545 thread) and is deliberately preserved. Only + ``**/`` gets new (zero-depth) behavior fnmatch can't express; nothing + about plain ``*``/``?`` should change.""" + + CASES = [ + ("*.ts", "src/deep/f.ts"), + ("src/*.ts", "src/deep/f.ts"), + ("src/*.ts", "src/sub/deep/f.ts"), + ("src/*", "src/deep/f.ts"), + ("*/*.ts", "src/deep/f.ts"), + ("a*b*c.ts", "aXbYc.ts"), + ("a?c.ts", "abc.ts"), + ] + + def test_matches_fnmatch_for_each_case(self): + import fnmatch + + for pattern, path in self.CASES: + with self.subTest(pattern=pattern, path=path): + expected = fnmatch.fnmatch(path, pattern) + actual = _glob_match(path, (pattern,), ()) + self.assertEqual(actual, expected) + + +class CaseSensitivity(unittest.TestCase): + """fnmatch.fnmatch() case-normalizes via os.path.normcase, which + lowercases on Windows and is a no-op elsewhere. Matching must follow + the same split rather than being unconditionally case-sensitive.""" + + def test_case_sensitive_on_posix(self): + with mock.patch("extensibility.os.name", "posix"): + self.assertFalse(_glob_match("FOO.TS", ("*.ts",), ())) + + def test_case_insensitive_on_windows(self): + with mock.patch("extensibility.os.name", "nt"): + self.assertTrue(_glob_match("FOO.TS", ("*.ts",), ())) + + +if __name__ == "__main__": + unittest.main() diff --git a/plugins/security-guidance/tests/test-sg-python.sh b/plugins/security-guidance/tests/test-sg-python.sh new file mode 100755 index 0000000000..dc46787e87 --- /dev/null +++ b/plugins/security-guidance/tests/test-sg-python.sh @@ -0,0 +1,245 @@ +#!/usr/bin/env bash +# Regression tests for plugins/security-guidance/hooks/sg-python.sh +# +# Covers anthropics/claude-code issue #86709: probe stderr was discarded, so +# when every Python candidate failed the shim reported a generic +# "no working Python 3 interpreter found" and hid the real reason (a broken +# pyenv shim, the Microsoft Store stub, a missing `py` launcher, a transient +# fork failure, ...). +# +# These tests drive the shim with fake interpreters on a synthetic PATH and +# assert on the exit code and the stdout/stderr split. The suite never invokes +# a real Python interpreter, so it behaves identically whether or not the host +# machine has Python installed. +# +# Usage: bash plugins/security-guidance/tests/test-sg-python.sh +set -u + +SELF_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +SHIM="$SELF_DIR/../hooks/sg-python.sh" + +FAKE_BIN="$(mktemp -d)" +T7_TMP="$(mktemp -d)" +trap 'rm -rf "$FAKE_BIN" "$T7_TMP"' EXIT + +pass=0 +fail=0 + +# fake — writes a verbatim shell body into $FAKE_BIN/ +fake() { + local name=$1 + local f="$FAKE_BIN/$name" + shift + { + echo '#!/bin/sh' + printf '%s\n' "$@" + } > "$f" + chmod +x "$f" +} + +# reset_bin — drop all fake interpreters so each test starts from a known state +reset_bin() { + rm -f "$FAKE_BIN"/* +} + +# run — runs the shim, captures RC/OUT/ERR +run() { + local extra_env=$1 path=$2 + shift 2 + : > "$FAKE_BIN/run.out" + : > "$FAKE_BIN/run.err" + # SG_PYTHON_CACHE points the shim's interpreter cache into $FAKE_BIN so no + # test reads or writes the real ~/.claude/security/python-cmd. reset_bin + # clears it, so every test starts cold; runs inside one test share it. + if [ -n "$extra_env" ]; then + env "$extra_env" SG_PYTHON_CACHE="${SG_TEST_CACHE:-$FAKE_BIN/py-cache}" PATH="$path" bash "$SHIM" "$@" > "$FAKE_BIN/run.out" 2> "$FAKE_BIN/run.err" + else + env SG_PYTHON_CACHE="${SG_TEST_CACHE:-$FAKE_BIN/py-cache}" PATH="$path" bash "$SHIM" "$@" > "$FAKE_BIN/run.out" 2> "$FAKE_BIN/run.err" + fi + RC=$? + OUT="$(cat "$FAKE_BIN/run.out")" + ERR="$(cat "$FAKE_BIN/run.err")" +} + +ok() { pass=$((pass + 1)); echo "ok - $1"; } +bad() { fail=$((fail + 1)); echo "FAIL - $1"; } + +assert_rc() { [ "$RC" -eq "$1" ] && ok "$2" || { bad "$2 (rc=$RC, want $1)"; } ; } +assert_out() { printf '%s' "$OUT" | grep -Fq -- "$1" && ok "$2" || { bad "$2 (missing: $1)"; }; } +assert_err() { printf '%s' "$ERR" | grep -Fq -- "$1" && ok "$2" || { bad "$2 (missing in stderr: $1)"; }; } +assert_no_err() { printf '%s' "$ERR" | grep -Fq -- "$1" && { bad "$2 (unexpected in stderr: $1)"; } || ok "$2"; } + +### +# T1 — every candidate fails: stderr from each probe is surfaced. +### +reset_bin +fake python3 'echo "pyenv: version 3.12.1 is not installed" >&2' 'exit 1' +fake python 'echo "simulated Store-stub failure" >&2' 'exit 49' +fake py 'echo "py launcher failed" >&2' 'exit 1' +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("unreachable")' +assert_rc 1 "T1: exits 1 when every candidate fails" +assert_err "no working Python 3 interpreter found" "T1: generic message present" +assert_err "probe errors:" "T1: probe-errors section header present" +assert_err "pyenv: version 3.12.1 is not installed" "T1: python3 probe error surfaced" +assert_err "simulated Store-stub failure" "T1: python probe error surfaced" +assert_err "py launcher failed" "T1: py probe error surfaced" + +### +# T2 — first candidate fails, second succeeds: failure stays hidden, shim falls back. +### +reset_bin +fake python3 'echo "bad pyenv" >&2' 'exit 1' +fake python 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "received:$*"; fi' +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("ran via python")' +assert_rc 0 "T2: falls back to python and succeeds" +assert_out 'received:-c print("ran via python")' "T2: python receives the payload" +assert_no_err "bad pyenv" "T2: earlier probe failure stays hidden on fallback" +assert_no_err "probe errors:" "T2: no probe-errors section on successful fallback" + +### +# T3 — Python 2 candidate is rejected, not selected. +### +reset_bin +fake python3 'exit 1' +fake python 'echo 2' +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("must-not-run")' +assert_rc 1 "T3: exits 1 when only python2 is present" +assert_err "no working Python 3 interpreter found" "T3: generic message present" +assert_no_err "must-not-run" "T3: payload never executed" + +### +# T4 — multi-word `py -3` candidate still resolves after a `python3`/`python` failure. +### +reset_bin +fake python3 'exit 1' +fake python 'exit 1' +fake py 'if [ "$3" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "received:$*"; fi' +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("ran via py -3")' +assert_rc 0 "T4: py -3 launcher works" +assert_out 'received:-3 -c print("ran via py -3")' "T4: py -3 receives the payload" + +### +# T5 — arguments passed to the shim reach the selected interpreter verbatim. +### +reset_bin +fake python3 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else printf "<%s>\n" "$@"; fi' +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("argv:", sys.argv[1:])' hello "two words" +assert_rc 0 "T5: first candidate (python3) selected" +assert_out "" "T5: plain arg passed verbatim" +assert_out "" "T5: space-containing arg passed verbatim as one argument" + +### +# T6 — diagnostics are best-effort: with TMPDIR unusable, mktemp fails but the +# shim must still find and exec a working interpreter (no crash from set -e). +### +reset_bin +fake python3 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "still works"; fi' +run "TMPDIR=/nonexistent-sgtest-dir" "$FAKE_BIN:/usr/bin:/bin" -c 'print("still works")' +assert_rc 0 "T6: interpreter selection works when mktemp fails" +assert_out "still works" "T6: payload executes despite missing diagnostics" +reset_bin +fake python3 'echo "no tmp for errors" >&2' 'exit 1' +fake python 'echo "no tmp for errors" >&2' 'exit 1' +fake py 'echo "no tmp for errors" >&2' 'exit 1' +run "TMPDIR=/nonexistent-sgtest-dir" "$FAKE_BIN:/usr/bin:/bin" -c 'print("unreachable")' +assert_rc 1 "T6b: all-fail path still reports failure without a temp file" +assert_err "no working Python 3 interpreter found" "T6b: generic message present" +assert_no_err "unreachable" "T6b: payload never executed" + +### +# T7 - no temp files leak: shim temp logs land in $T7_TMP (via TMPDIR), an +# isolated dir owned by this test, so the count is immune to unrelated +# activity in the shared /tmp. +### +before="$(ls "$T7_TMP"/tmp.* 2>/dev/null | wc -l)" +reset_bin +fake python3 'exit 1' +fake python 'exit 1' +fake py 'exit 1' +run "TMPDIR=$T7_TMP" "$FAKE_BIN:/usr/bin:/bin" -c 'print("unreachable")' +reset_bin +fake python3 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "ok"; fi' +run "TMPDIR=$T7_TMP" "$FAKE_BIN:/usr/bin:/bin" -c 'print("ok")' +after="$(ls "$T7_TMP"/tmp.* 2>/dev/null | wc -l)" +[ "$before" = "$after" ] && ok "T7: no temp files leaked (before=$before after=$after)" \ + || bad "T7: temp files leaked (before=$before after=$after)" + +### +# T8 — #98929: the shim must not launch the interpreter twice per hook. The +# first run probes then execs (2 launches) and records the candidate; later +# runs reuse it and launch the interpreter exactly once. +### +reset_bin +fake python3 'echo x >> "$(dirname "$0")/launches"' \ + 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "hook ran"; fi' +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("hook")' +assert_rc 0 "T8: first run succeeds" +[ "$(wc -l < "$FAKE_BIN/launches")" -eq 2 ] && ok "T8: cold run launches twice (probe + hook)" \ + || bad "T8: cold run launch count is $(wc -l < "$FAKE_BIN/launches"), want 2" +[ "$(cat "$FAKE_BIN/py-cache" 2>/dev/null)" = "python3" ] && ok "T8: candidate name cached" || bad "T8: cache not written" +: > "$FAKE_BIN/launches" +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("hook")' +assert_rc 0 "T8: cached run succeeds" +assert_out "hook ran" "T8: cached run executes the hook" +[ "$(wc -l < "$FAKE_BIN/launches")" -eq 1 ] && ok "T8: warm run launches once" \ + || bad "T8: warm run launch count is $(wc -l < "$FAKE_BIN/launches"), want 1" + +### +# T9 — the multi-word `py -3` candidate is cached and reused with its flag. +### +reset_bin +fake python3 'exit 1' +fake python 'exit 1' +fake py 'echo x >> "$(dirname "$0")/launches"' \ + 'if [ "$3" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "received:$*"; fi' +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("one")' +[ "$(cat "$FAKE_BIN/py-cache")" = "py -3" ] && ok "T9: 'py -3' cached as one candidate" || bad "T9: cache holds '$(cat "$FAKE_BIN/py-cache" 2>/dev/null)'" +: > "$FAKE_BIN/launches" +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("two")' +assert_out 'received:-3 -c print("two")' "T9: cached py -3 keeps its flag" +[ "$(wc -l < "$FAKE_BIN/launches")" -eq 1 ] && ok "T9: warm run launches py once" || bad "T9: warm run launch count is $(wc -l < "$FAKE_BIN/launches"), want 1" + +### +# T10 — a cache entry is never trusted blindly: unknown content is ignored and +# never run, a command that left PATH falls back to probing, and an entry older +# than a day is re-probed. +### +reset_bin +fake python3 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "real run"; fi' +printf '%s\n' 'touch /tmp/sg-cache-injection' > "$FAKE_BIN/py-cache" +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("x")' +assert_rc 0 "T10: unrecognised cache content is ignored" +assert_out "real run" "T10: probing still selects the interpreter" +[ ! -e /tmp/sg-cache-injection ] && ok "T10: cache content never executed" || { bad "T10: cache content was executed"; rm -f /tmp/sg-cache-injection; } +[ "$(cat "$FAKE_BIN/py-cache")" = "python3" ] && ok "T10: bad entry replaced by a probed candidate" || bad "T10: entry not replaced" + +reset_bin +# `py` is absent from PATH. python3 is stubbed to fail so a real /usr/bin/python3 +# on the host cannot be picked up by the fallback probe. +fake python3 'exit 1' +fake python 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "via python"; fi' +printf '%s\n' 'py -3' > "$FAKE_BIN/py-cache" +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("x")' +assert_out "via python" "T10: cached command missing from PATH falls back to probing" +[ "$(cat "$FAKE_BIN/py-cache")" = "python" ] && ok "T10: stale entry refreshed" || bad "T10: stale entry kept" + +reset_bin +fake python3 'echo x >> "$(dirname "$0")/launches"' \ + 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "ok"; fi' +printf '%s\n' 'python3' > "$FAKE_BIN/py-cache" +touch -d '2 days ago' "$FAKE_BIN/py-cache" 2>/dev/null || touch -t 200001010000 "$FAKE_BIN/py-cache" +run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("x")' +[ "$(wc -l < "$FAKE_BIN/launches")" -eq 2 ] && ok "T10: expired entry is re-probed" || bad "T10: expired entry launch count is $(wc -l < "$FAKE_BIN/launches"), want 2" + +### +# T11 — an unwritable cache location never breaks the hook. +### +reset_bin +fake python3 'if [ "$2" = "import sys; print(sys.version_info[0])" ]; then echo 3; else echo "still ok"; fi' +SG_TEST_CACHE="/nonexistent-sgtest-dir/sub/python-cmd" run "" "$FAKE_BIN:/usr/bin:/bin" -c 'print("x")' +assert_rc 0 "T11: hook runs when the cache cannot be written" +assert_out "still ok" "T11: payload executes without a cache" + +echo +echo "passed: $pass, failed: $fail" +[ "$fail" -eq 0 ] || exit 1 diff --git a/plugins/security-guidance/tests/test_doc_path_filters.py b/plugins/security-guidance/tests/test_doc_path_filters.py new file mode 100644 index 0000000000..e99b761fde --- /dev/null +++ b/plugins/security-guidance/tests/test_doc_path_filters.py @@ -0,0 +1,64 @@ +"""Regression tests for documentation path filters on substring rules.""" + +import sys +import unittest +from pathlib import Path + + +HOOKS_DIR = Path(__file__).resolve().parents[1] / "hooks" +sys.path.insert(0, str(HOOKS_DIR)) + +from patterns import _DOC_EXTS # noqa: E402 +from security_reminder_hook import check_patterns # noqa: E402 + + +XSS_FAMILY_CASES = ( + ("new_function_injection", "const fn = new Function(body);", "app.js"), + ( + "react_dangerously_set_html", + "return
;", + "Component.jsx", + ), + ("document_write_xss", "document.write(userInput);", "legacy.ts"), + ("innerHTML_xss", "element.innerHTML = userInput;", "render.tsx"), +) + + +def matched_rule_names(path, content): + return {rule_name for rule_name, _ in check_patterns(path, content)} + + +class DocumentationPathFilterTests(unittest.TestCase): + def test_xss_family_rules_ignore_all_documentation_extensions(self): + for rule_name, content, _ in XSS_FAMILY_CASES: + for extension in _DOC_EXTS: + with self.subTest(rule=rule_name, extension=extension): + matches = matched_rule_names(f"docs/security-guide{extension}", content) + self.assertNotIn(rule_name, matches) + + def test_xss_family_rules_still_match_executable_source(self): + for rule_name, content, source_path in XSS_FAMILY_CASES: + with self.subTest(rule=rule_name, path=source_path): + self.assertIn(rule_name, matched_rule_names(source_path, content)) + + def test_only_a_terminal_documentation_extension_is_filtered(self): + for rule_name, content, _ in XSS_FAMILY_CASES: + with self.subTest(rule=rule_name): + self.assertNotIn( + rule_name, matched_rule_names("example.js.md", content) + ) + self.assertIn( + rule_name, matched_rule_names("example.md.js", content) + ) + + def test_existing_eval_filter_remains_the_control(self): + self.assertNotIn( + "eval_injection", matched_rule_names("guide.md", "eval(input)") + ) + self.assertIn( + "eval_injection", matched_rule_names("app.js", "eval(input)") + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/plugins/security-guidance/tests/test_finding_redaction.py b/plugins/security-guidance/tests/test_finding_redaction.py new file mode 100644 index 0000000000..ab7f93ccd8 --- /dev/null +++ b/plugins/security-guidance/tests/test_finding_redaction.py @@ -0,0 +1,301 @@ +"""Findings are fed back into the main conversation and kept in session +state; a hardcoded-secret finding quotes the secret itself. These tests pin +that credential-shaped values are masked at every formatting and storage +boundary while code quotes and location strings survive +(anthropics/claude-code#96276, the reporter's Stop-hook case). + +Run: python3 -m unittest discover -s plugins/security-guidance/tests -v +""" + +import os +import sys +import tempfile +import time +import unittest +from unittest import mock + +HOOKS_DIR = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "hooks") +sys.path.insert(0, HOOKS_DIR) + +_tmp_state = tempfile.mkdtemp(prefix="sg-test-state-") +os.environ.setdefault("SECURITY_WARNINGS_STATE_DIR", _tmp_state) +os.environ.setdefault("SECURITY_GUIDANCE_DEBUG_LOG", os.path.join(_tmp_state, "log.txt")) + +import llm # noqa: E402 +import review_api # noqa: E402 +import security_reminder_hook # noqa: E402 + +MARKER = "SG_FAKE_MARKER_A2_7f3a91" +SECRET_FINDING = { + "filePath": "secrets.yml", + "category": "Hardcoded Secrets", + "vulnerableCode": f"password: {MARKER}", + "explanation": "A plaintext credential is committed to the working tree.", + "fix": f"Move {MARKER} into a secret manager and read it from the environment.", + "severity": "high", +} +SQLI_FINDING = { + "filePath": "app.py", + "category": "SQL Injection", + "vulnerableCode": 'cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")', + "explanation": "user_id flows from the request into the query string.", + "fix": "Use a parameterized query: cursor.execute(sql, (user_id,)).", + "severity": "high", +} + + +def R(text, category="Hardcoded Secrets"): + return review_api.redact_secret_values(text, category=category) + + +class MaskedValuesTests(unittest.TestCase): + def test_reporter_case_keeps_key_and_masks_value(self): + self.assertEqual(R(f"password: {MARKER}"), "password: ****91") + self.assertEqual(R(f"db:\n host: db.example.invalid\n password: {MARKER}"), + "db:\n host: db.example.invalid\n password: ****91") + + def test_bare_marker_in_fix_prose_is_masked_for_secret_findings(self): + out = R(f"Move {MARKER} out of secrets.yml") + self.assertNotIn(MARKER, out) + self.assertEqual(out, "Move ****91 out of secrets.yml") + + def test_quoted_value_is_masked_whole_whatever_it_contains(self): + # Punctuation, spaces and brackets inside the quotes must not cut the mask short. + self.assertEqual(R("SECRET_KEY = 'django-insecure-)x7@k!p#q2wz&8'"), "SECRET_KEY = '****&8'") + self.assertEqual(R("password: 'my secret pass phrase'"), "password: '****se'") + self.assertEqual(R('api_key = "abcd1234ijkl"', "SSRF"), 'api_key = "****kl"') + + def test_config_line_bare_value_is_masked_to_end_of_line(self): + for text, want in ( + ("password: correct horse battery staple", "password: ****le"), + (" - password: correct horse # dev only", " - password: ****se # dev only"), + ("+ password: prodpassword", "+ password: ****rd"), # quoted `+` diff line + ("secrets.yml:3: password: prodpassword", "secrets.yml:3: password: ****rd"), + ("export API_TOKEN=abcdefgh1", "export API_TOKEN=****h1"), + ("DB_PASSWORD=SUPERSECRET99", "DB_PASSWORD=****99"), + ("PGPASSWORD=s3cretvalue psql -h db", "PGPASSWORD=****ue psql -h db"), + ("PASSWORD=hunter2\nUSER=bob", "PASSWORD=****\nUSER=bob"), + ): + with self.subTest(text=text): + self.assertEqual(R(text, "X"), want) + + def test_key_forms(self): + for text, want in ( + ("app.config['SECRET_KEY'] = 'p9Zxnotsosecret'", "app.config['SECRET_KEY'] = '****et'"), + ("$config['db']['password'] = 'Pr0duction!';", "$config['db']['password'] = '****n!';"), + ("password => 'perlish123'", "password => '****23'"), + ('{"password": "hunter2", "user": "bob"}', '{"password": "****", "user": "bob"}'), + ("hunter22x", "****2x"), + ("X-Api-Key: abcd1234efgh", "X-Api-Key: ****gh"), + ("mysql --password=hunter2 -u root", "mysql --password=**** -u root"), + ("if token == 'abc123secret':", "if token == '****et':"), + ('if (token === "abc123") {', 'if (token === "****") {'), + ('if password != "S3cr3tAdm1n":', 'if password != "****1n":'), + ('password: str = "hunter2"', 'password: str = "****"'), + ('const apiKey: string = "abcd1234efgh5678";', 'const apiKey: string = "****78";'), + ('key = b"0123456789abcdef"', 'key = b"0123456789abcdef"'), # `key` alone is not a credential name + ('password = b"hunter2xyz"', 'password = b"****yz"'), + ("app.secret_key = 'super secret key'", "app.secret_key = '****ey'"), + ("self.api_key = 'abcdefgh1234'", "self.api_key = '****34'"), + ("password: cGFzc3dvcmQ=", "password: ****Q="), # base64, not a type annotation + ('password = "ABCDEF123456"', 'password = "****56"'), # a quoted RHS is a literal + ('token = "Zq9.Lm3xTv8pQw2n"', 'token = "****2n"'), + ("--token abcdef123456 --verbose", "--token ****56 --verbose"), + ("-H 'X-Api-Key: abcdefghijklmnop'", "-H 'X-Api-Key: ****op'"), + ("ENV DB_PASSWORD s3cret", "ENV DB_PASSWORD ****"), + ("POSTGRES_PASSWORD: postgrespass", "POSTGRES_PASSWORD: ****ss"), + ("db.password=changeitplease", "db.password=****se"), + ): + with self.subTest(text=text): + self.assertEqual(R(text, "Insecure Configuration"), want) + + def test_known_token_shapes_masked_in_any_category(self): + for value in ( + "sk_live_DUMMYENVMARKER111abc", + "AKIAABCDEFGHIJKLMNOP", + "ghp_abcdefghijklmnopqrstuvwx0123456789", + "eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.abcdefghijklmnop", + ): + with self.subTest(value=value): + out = R(f"requests.get(url + '{value}')", "SSRF") + self.assertNotIn(value, out) + self.assertIn("****" + value[-2:], out) + + def test_http_credentials(self): + self.assertEqual(R('connect("postgres://admin:s3cr3tpass@db/prod")', "SQL Injection"), + 'connect("postgres://admin:****ss@db/prod")') + self.assertEqual(R("redis://:sup3rs3cretlongvalue99@cache.internal:6379/0", "SSRF"), + "redis://:****99@cache.internal:6379/0") + self.assertEqual(R("mongodb://root:P@ssw0rd!2024@mongo:27017/db", "SSRF"), + "mongodb://root:****24@mongo:27017/db") + self.assertEqual(R('{"Authorization": "Bearer abcdEFGH1234ijkl"}', "SSRF"), + '{"Authorization": "Bearer ****kl"}') + self.assertEqual(R('{"Authorization": "Basic YWRtaW46cGFzc3dvcmQ="}', "SSRF"), + '{"Authorization": "Basic ****Q="}') + self.assertEqual(R("Authorization: Token 9944b09199c62bcf9418ad846dd0e4bbdfc6ee4b", "IDOR"), + "Authorization: Token ****4b") + self.assertEqual(R("curl -u admin:Passw0rd! https://host/api", "Command Injection"), + "curl -u admin:****d! https://host/api") + self.assertEqual(R("curl --user 'admin:Passw0rd!' https://host/api", "Command Injection"), + "curl --user 'admin:****d!' https://host/api") + + def test_private_key_bodies_are_dropped(self): + for kind in ("RSA PRIVATE KEY", "PRIVATE KEY", "PGP PRIVATE KEY BLOCK"): + pem = f"-----BEGIN {kind}-----\nlQOYBGRk3xkBCAC7n0v9Zq1mX2pL8sT4uY\nabcd\n-----END {kind}-----" + with self.subTest(kind=kind): + out = R(pem, "Key management") + self.assertNotIn("lQOYBGRk", out) + self.assertIn(f"-----BEGIN {kind}-----", out) + + def test_positional_secret_in_secret_category(self): + out = R("client = Client('AKIAABCDEFGHIJKLMNOP', 'wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY')", + "Hardcoded AWS credentials") + self.assertEqual(out, "client = Client('****OP', '****EY')") + self.assertEqual(R("jwt.decode(tok, 'mysupersecretkey', algorithms=['HS256'])", "Hardcoded JWT secret"), + "jwt.decode(tok, '****ey', algorithms=['HS256'])") + self.assertEqual(R('API_KEYS = ["key1abcdx", "key2efghx"]'), 'API_KEYS = ["****dx", "****hx"]') + self.assertEqual(R('if ("Adm1nP@ssw0rd!" == pw) {'), 'if ("****d!" == pw) {') + self.assertEqual(R("Rotate wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY now"), "Rotate ****EY now") + + +class UnmaskedTextTests(unittest.TestCase): + """The finding must still say where the problem is.""" + + def test_code_quotes_survive(self): + for code, category in ( + (SQLI_FINDING["vulnerableCode"], "SQL Injection"), + ("subprocess.run(cmd, shell=True)", "Command Injection"), + ("password_hash = bcrypt.hashpw(pw, salt)", "Weak crypto"), + ("password = derive_key(salt)", "Hardcoded Secrets"), + ("token = await getToken()", "Hardcoded Secrets"), + ("auth_token=request.args['t']", "IDOR"), + ("token = request.headers['X-Token']", "Hardcoded Secrets"), + ("if token == expected:", "Timing attack"), + ("password: ${DB_PASS}", "Hardcoded Secrets"), + ("${DB_PASS}", "Hardcoded Secrets"), + ('logger.debug("password=" + password)', "Secrets in logs"), + ('pw = getpass.getpass("Password: ")', "Hardcoded Secrets"), + ): + with self.subTest(code=code): + self.assertEqual(R(code, category), code) + + def test_fix_advice_survives(self): + for text in ( + "Replace with password = os.environ['DB_PASSWORD']", + "const apiKey = process.env.API_KEY;", + "os.environ.get('DB_PASSWORD')", + "Rotate the password: anyone with repo access already has it", + "Credentials: move the database password into the environment and rotate it.", + "Token: see line 12 of auth.py", + "const t = fetchOAuth2AccessTokenForUser(req)", + "see commit 9944b09199c62bcf9418ad846dd0e4bbdfc6ee4b", + 'password_file = "/run/secrets/db"', + "--password $DB_PASS", + "Move the value out of `config/database.yml` (use `bin/rails credentials:edit`)", + "client.get_secret_value(SecretId='prod/db')['SecretString']", + "Replace with `process.env.STRIPE_SECRET_KEY` from `.env.production`", + 'headers = {"Content-Type": ctype, "X-Api-Key": key}', + ): + with self.subTest(text=text): + self.assertEqual(R(text, "Hardcoded Secrets"), text) + self.assertEqual(R("os.environ.get('DB_PASSWORD', 'changeme123')"), + "os.environ.get('DB_PASSWORD', '****23')") + + def test_locations_survive_in_secret_findings(self): + for text in ( + "config/settings_v2.py, /api/v1/auth/login endpoint", + "src/server/routes/auth2fa.ts — POST /api/v2/sessions", + "deploy/k8s/prod-secrets-2024.yaml", + "see 'src/Components/App.tsx' and '/etc/app/conf'", + "config/secrets.yml:12", + "src/auth/token.py:45 — GET /api/v2/users/12345/tokens", + "`docker-compose.prod.yml`", + ): + with self.subTest(text=text): + self.assertEqual(R(text, "Hardcoded credential"), text) + + def test_credential_metadata_keys_are_not_values(self): + for text, category in ( + ("OAUTH_CALLBACK = 'https://attacker.example.net/cb'", "Open Redirect"), + ("auth_url = 'http://sso.example.com/login'", "Cleartext Transmission"), + ("author_id = 12345", "IDOR"), + ("PASSWORD_RESET_TIMEOUT = 259200", "Auth"), + ("MAX_TOKENS = 4096", "LLM"), + ("Require bearer authentication on this route", "Missing Authentication"), + ): + with self.subTest(text=text): + self.assertEqual(R(text, category), text) + + def test_token_named_categories_that_are_not_about_secret_values(self): + for text, category in ( + ('
', "CSRF token missing"), + ("fetch('/api/v1/users/delete', {method: 'POST'})", "Missing CSRF token"), + ("headers['X-CSRF-Token'] = getCookie('csrftoken')", "CSRF Token"), + ("user = User.objects.get(reset_token=request.GET['t'])", "Password reset token not invalidated"), + ): + with self.subTest(text=text): + self.assertEqual(R(text, category), text) + + def test_non_string_fields_do_not_raise(self): + self.assertEqual(R(None), "") + self.assertEqual(R(12345, ["Secrets"]), "12345") + out = review_api.format_findings([{"filePath": "a", "category": ["Secrets"], "vulnerableCode": 7, + "fix": None, "severity": "high"}]) + self.assertIn("[['Secrets']] 7", out) + + def test_hostile_input_stays_linear(self): + # The quoted code comes from the repository under review, so no input + # shape may make the scan quadratic: 4x the input stays well under + # 16x the time. + def timed(probe): + started = time.perf_counter() + R(probe, "Hardcoded Secrets") + return time.perf_counter() - started + + for unit in ("auth.", "token-", "a", "'b", "a:", " ", "\t", "=", "x.]", '"'): + with self.subTest(unit=unit): + small = min(timed(unit * 2500) for _ in range(3)) + large = min(timed(unit * 10000) for _ in range(3)) + self.assertLess(large, small * 10 + 0.02) + + +class BoundaryTests(unittest.TestCase): + """Every block that reaches the main conversation, and the state file, + goes through the mask.""" + + def test_guidance_block_masks_secret_but_keeps_location(self): + out = llm._format_vulns_guidance([SQLI_FINDING, SECRET_FINDING]) + self.assertNotIn(MARKER, out) + self.assertIn("secrets.yml:", out) + self.assertIn("[Hardcoded Secrets] password: ****91", out) + self.assertIn("Suggested fix: Move ****91 into a secret manager", out) + self.assertIn(SQLI_FINDING["vulnerableCode"], out) # code quote untouched + self.assertEqual(out, review_api.format_findings([SQLI_FINDING, SECRET_FINDING])) + + def test_concerns_review_masks_evidence_line(self): + analysis = { + "hasConcerns": True, + "concerns": [{ + "category": "Hardcoded credential", + "area": "secrets.yml", + "concern": f"Confirm whether {MARKER} is a live credential.", + "evidenceLine": f" password: {MARKER}", + "severity": "high", + }], + } + with mock.patch.object(llm, "HAS_API_CREDENTIALS", True), \ + mock.patch.object(llm, "_call_claude_dual_or", return_value=analysis): + out = llm.analyze_security_concerns([("secrets.yml", f"+ password: {MARKER}\n")], is_diff=True) + self.assertIsNotNone(out) + self.assertNotIn(MARKER, out) + self.assertIn("secrets.yml", out) + self.assertIn("Evidence: password: ****91", out) + + def test_state_snapshot_stores_masked_code(self): + snap = security_reminder_hook._finding_snapshot(SECRET_FINDING) + self.assertEqual(snap, {"filePath": "secrets.yml", "category": "Hardcoded Secrets", + "vulnerableCode": "password: ****91"}) + + +if __name__ == "__main__": + unittest.main() diff --git a/plugins/security-guidance/tests/test_secretpaths.py b/plugins/security-guidance/tests/test_secretpaths.py new file mode 100644 index 0000000000..5ba66710cc --- /dev/null +++ b/plugins/security-guidance/tests/test_secretpaths.py @@ -0,0 +1,317 @@ +"""Regression tests for anthropics/claude-code#96276: the reviewer must not +put files the session's Read deny/ask rules cover (or well-known secret +files) into a review prompt, and its SDK sub-agents must carry the same +rules as disallowed_tools. + +Run: python3 -m unittest discover -s plugins/security-guidance/tests -v +""" + +import json +import os +import shutil +import subprocess +import sys +import tempfile +import types +import unittest + +HOOKS_DIR = os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "hooks") +sys.path.insert(0, HOOKS_DIR) + +_tmp_state = tempfile.mkdtemp(prefix="sg-test-state-") +os.environ["SECURITY_WARNINGS_STATE_DIR"] = _tmp_state +os.environ["SECURITY_GUIDANCE_DEBUG_LOG"] = os.path.join(_tmp_state, "log.txt") + +import gitutil # noqa: E402 +import llm # noqa: E402 +import secretpaths # noqa: E402 + +SHELL_TOOLS = ["Bash"] + (["PowerShell"] if sys.platform == "win32" else []) + + +def tearDownModule(): + shutil.rmtree(_tmp_state, ignore_errors=True) + + +def _diff(*paths): + """A minimal unified diff touching each path with one added line.""" + out = [] + for p in paths: + out.append( + f"diff --git a/{p} b/{p}\n" + f"--- a/{p}\n+++ b/{p}\n" + "@@ -0,0 +1,1 @@\n" + f"+value_for_{p.replace('/', '_')} = \"DUMMY-SECRET\"\n" + ) + return "".join(out) + + +class _Env: + """Isolated project, config and managed dirs; restores env and module state.""" + + def __init__(self, project_settings=None, user_settings=None, local_settings=None, + managed_settings=None): + self.project_settings = project_settings + self.user_settings = user_settings + self.local_settings = local_settings + self.managed_settings = managed_settings + + def __enter__(self): + self._saved = dict(os.environ) + self._saved_managed_dir = secretpaths._managed_settings_dir + self.root = os.path.realpath(tempfile.mkdtemp(prefix="sg-test-")) + self.project = os.path.join(self.root, "project") + self.config = os.path.join(self.root, "config-dir") + self.managed = os.path.join(self.root, "managed") + os.makedirs(os.path.join(self.project, ".claude")) + os.makedirs(self.config) + os.makedirs(os.path.join(self.managed, "managed-settings.d")) + secretpaths._managed_settings_dir = lambda: self.managed + if self.managed_settings is not None: + with open(os.path.join(self.managed, "managed-settings.d", "10-sec.json"), "w") as f: + json.dump(self.managed_settings, f) + if self.project_settings is not None: + with open(os.path.join(self.project, ".claude", "settings.json"), "w") as f: + json.dump(self.project_settings, f) + if self.local_settings is not None: + with open(os.path.join(self.project, ".claude", "settings.local.json"), "w") as f: + json.dump(self.local_settings, f) + if self.user_settings is not None: + with open(os.path.join(self.config, "settings.json"), "w") as f: + json.dump(self.user_settings, f) + os.environ["CLAUDE_CONFIG_DIR"] = self.config + os.environ["CLAUDE_PROJECT_DIR"] = self.project + os.environ.pop("SG_SKIP_SECRET_FILES", None) + secretpaths.load_for_session(self.project) + return self + + def __exit__(self, *exc): + os.environ.clear() + os.environ.update(self._saved) + secretpaths._managed_settings_dir = self._saved_managed_dir + secretpaths.load_for_session(None) + shutil.rmtree(self.root, ignore_errors=True) + + +class DiffFilterTests(unittest.TestCase): + DIFF = _diff( + "app.py", + "config/prod.json", # denied by Read(./config/**) + "docs/config/notes.json", # also denied: Claude Code reads `x/**` as `x`, any depth + "docs/settings/ui.json", # Read(./settings/*.json) is anchored: NOT denied + "private/report.json", # ask rule Read(/private/**) → excluded + "secrets.yaml", # well-known secret store name + "deploy/credentials", # well-known secret store name (extensionless) + "keys/server.pem", # already non-reviewable by extension + "src/secrets.py", # code that handles secrets: reviewable + ) + + def _reviewed(self): + return [p for p, _ in gitutil.parse_diff_into_files(self.DIFF)] + + def test_denied_and_secret_files_are_dropped_from_review_diff(self): + settings = {"permissions": {"deny": ["Read(./config/**)", "Read(./settings/*.json)", + "Bash(curl:*)"], + "ask": ["Read(/private/**)"]}} + with _Env(project_settings=settings): + self.assertEqual( + self._reviewed(), + ["app.py", "docs/settings/ui.json", "src/secrets.py"], + ) + + def test_opt_out_restores_secret_named_files_but_not_denied_ones(self): + settings = {"permissions": {"deny": ["Read(./config/**)"]}} + with _Env(project_settings=settings): + os.environ["SG_SKIP_SECRET_FILES"] = "0" + self.assertEqual( + self._reviewed(), + ["app.py", "docs/settings/ui.json", "private/report.json", + "secrets.yaml", "deploy/credentials", "src/secrets.py"], + ) + + def test_no_settings_still_skips_well_known_secret_files(self): + with _Env(): + reviewed = self._reviewed() + self.assertIn("config/prod.json", reviewed) + self.assertNotIn("secrets.yaml", reviewed) + self.assertNotIn("deploy/credentials", reviewed) + + def test_bare_read_deny_excludes_everything(self): + with _Env(local_settings={"permissions": {"deny": ["Read"]}}): + self.assertEqual(self._reviewed(), []) + + def test_user_settings_slash_rule_anchors_at_config_dir_not_project(self): + # `/x` in user settings means /x, so it must not hide the + # project's reports/ — but `~/` and `//` rules reach anywhere. + user = {"permissions": {"deny": ["Read(/reports/**)", "Read(//**/vault/*.json)"]}} + with _Env(user_settings=user): + reviewed = [p for p, _ in gitutil.parse_diff_into_files( + _diff("reports/q3.json", "app/vault/keys.json", "app.py"))] + self.assertEqual(reviewed, ["reports/q3.json", "app.py"]) + + def test_managed_dropin_rules_and_utf8_bom_are_honored(self): + with _Env(managed_settings={"permissions": {"deny": ["Read(/private/**)"]}}) as env: + # PowerShell 5 writes settings.json with a BOM; Claude Code strips it. + with open(os.path.join(env.project, ".claude", "settings.json"), "w", encoding="utf-8-sig") as f: + json.dump({"permissions": {"deny": ["Read(./config/**)"]}}, f) + secretpaths.load_for_session(env.project) + reviewed = self._reviewed() + self.assertNotIn("private/report.json", reviewed) + self.assertNotIn("config/prod.json", reviewed) + self.assertIn("app.py", reviewed) + + def test_empty_patterns_match_nothing_instead_of_everything(self): + with _Env(project_settings={"permissions": {"deny": ["Read(./)", "Read(/)", "Read(~/)"]}}): + self.assertIn("app.py", self._reviewed()) + + def test_worktree_session_reads_local_settings_at_canonical_root(self): + with _Env() as env: + git = ["git", "-c", "user.email=t@example.com", "-c", "user.name=t"] + run = lambda *a, cwd=env.project: subprocess.run([*git, *a], cwd=cwd, check=True, + capture_output=True) + run("init", "-q") + run("commit", "-q", "--allow-empty", "-m", "init") + worktree = os.path.join(env.root, "wt") + run("worktree", "add", "-q", worktree) + # Claude Code keeps settings.local.json at the main repo root. + with open(os.path.join(env.project, ".claude", "settings.local.json"), "w") as f: + json.dump({"permissions": {"deny": ["Read(./config/**)"]}}, f) + os.environ["CLAUDE_PROJECT_DIR"] = worktree + secretpaths.load_for_session(worktree) + self.assertNotIn("config/prod.json", self._reviewed()) + self.assertIn("app.py", self._reviewed()) + + def test_malformed_settings_file_is_ignored(self): + with _Env() as env: + with open(os.path.join(env.project, ".claude", "settings.json"), "w") as f: + f.write("{not json") + secretpaths.load_for_session(env.project) + self.assertIn("app.py", self._reviewed()) + + +class PatternSemanticsTests(unittest.TestCase): + CASES = [ + # (pattern, root-relative path, matches) + ("secrets.yaml", "secrets.yaml", True), + ("secrets.yaml", "a/b/secrets.yaml", True), # slashless → any depth + ("secrets.yaml", "SECRETS.YAML", True), # case-insensitive, like Claude Code + ("config/*.json", "config/a.json", True), + ("config/*.json", "x/config/a.json", False), # contains slash → anchored + ("config/*.json", "config/sub/a.json", False), # * does not cross / + ("config/**", "config/sub/a.json", True), + ("config/**", "docs/config/a.json", True), # Claude Code strips /** → slashless + ("/config/**", "docs/config/a.json", False), + ("config", "config/sub/a.json", True), # dir name covers contents + ("/private", "private/x", True), + ("/private", "docs/private/x", False), + ("**/vault/*.json", "a/b/vault/k.json", True), + ("**/vault/*.json", "vault/k.json", True), + ("*.pem", "keys/a.PEM", True), + ("id_?sa", "home/id_rsa", True), + ("key[0-9]", "key7", True), + ("key[!0-9]", "key7", False), + (r"a\*b", "a*b", True), + (r"a\*b", "axb", False), + ("docs/", "docs/readme.md", True), + ("!keep.json", "keep.json", False), # negation never excludes + ] + + def test_gitignore_style_matching(self): + for pattern, path, expected in self.CASES: + with self.subTest(pattern=pattern, path=path): + rx = secretpaths._compile(pattern) + self.assertEqual(bool(rx and rx.match(path)), expected) + + +class SubagentDisallowedToolsTests(unittest.TestCase): + def test_rules_are_forwarded_with_absolute_anchors(self): + settings = {"permissions": { + "deny": ["Read(./config/**)", "Read(/private/**)", "Read(~/.ssh/**)", + "Read(//etc/shadow)", "Grep", "Bash(curl:*)", "Edit(**)"], + "ask": ["Read(*.tfstate)"], + }} + with _Env(project_settings=settings) as env: + tools = secretpaths.subagent_disallowed_tools(env.project) + proj = env.project.strip("/") + for expected in (*SHELL_TOOLS, "Read(./config/**)", f"Read(//{proj}/private/**)", + "Read(~/.ssh/**)", "Read(//etc/shadow)", "Grep", + "Read(*.tfstate)", "Read(.env)", "Read(id_rsa*)", + "Read(secrets.yaml)", "Read(credentials.json)"): + self.assertIn(expected, tools) + # Only read-path rules are forwarded; other tools' rules are not ours. + self.assertFalse(any(t.startswith(("Bash(", "Edit")) for t in tools)) + # Sub-agent rooted at an alternate checkout: the verbatim relative + # rule covers that root, the session cwd gets an absolute copy, and + # the project-anchored rule is mirrored into the checkout. + alt = os.path.join(env.root, "alt-checkout") + elsewhere = secretpaths.subagent_disallowed_tools(alt) + for expected in (f"Read(//{proj}/config/**)", f"Read(//{alt.strip('/')}/private/**)"): + self.assertIn(expected, elsewhere) + self.assertNotIn(expected, tools) + + def test_opt_out_drops_only_the_builtin_globs(self): + with _Env(project_settings={"permissions": {"deny": ["Read(./config/**)"]}}) as env: + os.environ["SG_SKIP_SECRET_FILES"] = "0" + self.assertEqual(secretpaths.subagent_disallowed_tools(env.project), + SHELL_TOOLS + ["Read(./config/**)"]) + + def test_diff_only_call_gets_no_file_or_shell_tools(self): + self.assertEqual(secretpaths.no_file_tools(), SHELL_TOOLS + ["Read", "Grep", "Glob"]) + + +class _FakeSDK(types.ModuleType): + """Stand-in for claude_agent_sdk that records the options each query got.""" + + def __init__(self): + super().__init__("claude_agent_sdk") + self.options_seen = [] + sdk = self + + class ClaudeAgentOptions: + def __init__(self, **kwargs): + self.__dict__.update(kwargs) + sdk.options_seen.append(kwargs) + + class AssistantMessage: + pass + + class ResultMessage: + subtype = "success" + structured_output = {"findings": []} + usage = None + total_cost_usd = None + + async def query(prompt, options): + async for _ in prompt: + pass + yield ResultMessage() + + self.ClaudeAgentOptions = ClaudeAgentOptions + self.AssistantMessage = AssistantMessage + self.ResultMessage = ResultMessage + self.query = query + + +class AgenticReviewWiringTests(unittest.TestCase): + def test_agentic_review_passes_deny_rules_as_disallowed_tools(self): + fake = _FakeSDK() + saved = sys.modules.get("claude_agent_sdk") + sys.modules["claude_agent_sdk"] = fake + try: + with _Env(project_settings={"permissions": {"deny": ["Read(./config/**)"]}}) as env: + os.environ["SG_AGENTIC_CLI_PATH"] = os.path.join(env.root, "no-such-claude") + llm.agentic_review(env.project, [("app.py", _diff("app.py"))], ["app.py"]) + finally: + if saved is None: + del sys.modules["claude_agent_sdk"] + else: + sys.modules["claude_agent_sdk"] = saved + self.assertTrue(fake.options_seen, "agentic_review never built ClaudeAgentOptions") + for opts in fake.options_seen: + self.assertEqual(opts.get("setting_sources"), []) # still no recursive plugins/hooks + for expected in ("Bash", "Read(./config/**)", "Read(.env)"): + self.assertIn(expected, opts.get("disallowed_tools", [])) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/auto-close-duplicates.ts b/scripts/auto-close-duplicates.ts index 2ad3bd3112..dcb5a1ed85 100644 --- a/scripts/auto-close-duplicates.ts +++ b/scripts/auto-close-duplicates.ts @@ -46,6 +46,28 @@ async function githubRequest(endpoint: string, token: string, method: string return response.json(); } +// List endpoints return at most 100 items per page (30 by default), so +// follow pagination until a short page signals the end of the collection. +async function githubRequestAllPages( + endpoint: string, + token: string +): Promise { + const results: T[] = []; + const perPage = 100; + const separator = endpoint.includes("?") ? "&" : "?"; + + for (let page = 1; page <= 20; page++) { + const pageItems: T[] = await githubRequest( + `${endpoint}${separator}per_page=${perPage}&page=${page}`, + token + ); + results.push(...pageItems); + if (pageItems.length < perPage) break; + } + + return results; +} + function extractDuplicateIssueNumber(commentBody: string): number | null { // Try to match #123 format first let match = commentBody.match(/#(\d+)/); @@ -153,7 +175,7 @@ async function autoCloseDuplicates(): Promise { ); console.log(`[DEBUG] Fetching comments for issue #${issue.number}...`); - const comments: GitHubComment[] = await githubRequest( + const comments: GitHubComment[] = await githubRequestAllPages( `/repos/${owner}/${repo}/issues/${issue.number}/comments`, token ); @@ -217,7 +239,7 @@ async function autoCloseDuplicates(): Promise { console.log( `[DEBUG] Issue #${issue.number} - checking reactions on duplicate comment...` ); - const reactions: GitHubReaction[] = await githubRequest( + const reactions: GitHubReaction[] = await githubRequestAllPages( `/repos/${owner}/${repo}/issues/comments/${lastDupeComment.id}/reactions`, token );