Skip to content

OCPEDGE-2576: Add security-review plugin with credential and safety hooks#74

Draft
jeff-roche wants to merge 5 commits intoopenshift-eng:mainfrom
jeff-roche:feat/security-review-plugin
Draft

OCPEDGE-2576: Add security-review plugin with credential and safety hooks#74
jeff-roche wants to merge 5 commits intoopenshift-eng:mainfrom
jeff-roche:feat/security-review-plugin

Conversation

@jeff-roche
Copy link
Copy Markdown
Contributor

@jeff-roche jeff-roche commented Apr 22, 2026

Summary

  • Adds security-review Claude Code plugin with automatic PreToolUse hooks
  • Blocks git commit/git add when staged content contains credentials (AWS keys, private keys, passwords, tokens)
  • Blocks destructive commands (rm -rf /, git push --force, git reset --hard) with safer alternatives
  • Scans file content for credential patterns before Write operations
  • All hooks use fast-path exit for non-matching commands (<2s execution)

Test plan

  • Install plugin via marketplace
  • Verify echo '{"toolInput":{"command":"rm -rf /"}}' | bash plugins/security-review/scripts/block-destructive.sh exits 1
  • Verify echo '{"toolInput":{"command":"rm -rf ./build"}}' | bash plugins/security-review/scripts/block-destructive.sh exits 0
  • Verify echo '{"toolInput":{"file_path":"x","content":"password = \"secret\""}}' | bash plugins/security-review/scripts/check-file-secrets.sh exits 1
  • Verify echo '{"toolInput":{"command":"ls"}}' | bash plugins/security-review/scripts/check-secrets.sh exits 0

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added security-review plugin (v1.0.0) to the plugin marketplace to detect/block credential leaks and unsafe writes.
    • Installs PreToolUse hooks for shell commands and file writes to scan staged changes and file content for secrets and destructive commands.
    • Blocks common destructive operations (force pushes, hard resets, recursive deletions) and offers safer alternatives.
  • Documentation

    • Added user-facing README describing hook behavior, detected patterns, false-positive approval flow, and installation.

…hooks

Adds PreToolUse hooks that block credential commits (AWS keys, private
keys, passwords, tokens), destructive commands (force push, rm -rf /,
hard reset), and credential patterns in file writes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 22, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeff-roche

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 22, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds a new Claude plugin "security-review": manifest and marketplace entry, README, PreToolUse hook configuration, and three Bash hook scripts that scan staged diffs and file writes for credentials and block destructive shell commands.

Changes

Cohort / File(s) Summary
Plugin manifest & marketplace
plugins/security-review/.claude-plugin/plugin.json, .claude-plugin/marketplace.json
Adds security-review plugin metadata (name, version 1.0.0, author, homepage, license) and registers it in the marketplace.
Documentation
plugins/security-review/README.md
Adds README describing installed PreToolUse guard hooks, detection categories, false-positive approval flow, and installation instructions.
Hook configuration
plugins/security-review/hooks/hooks.json
Adds PreToolUse hook entries: for Bash run check-secrets.sh then block-destructive.sh; for Write run check-file-secrets.sh (with timeouts and status messages).
Guard scripts
plugins/security-review/scripts/check-secrets.sh, plugins/security-review/scripts/block-destructive.sh, plugins/security-review/scripts/check-file-secrets.sh
Adds three executable Bash scripts that parse hook JSON (prefer jq, fallback python3), scan staged git diffs or file contents for credential patterns, annotate sensitive paths, and block destructive command patterns by exiting nonzero and printing messages to stderr.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding a security-review plugin with credential and safety hooks, which is the primary focus of the entire changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@plugins/security-review/README.md`:
- Around line 33-35: The fenced code block containing the command `/plugin
marketplace add openshift-eng/edge-tooling security-review` lacks a language
tag; update that fenced block in README.md to use "bash" (i.e., change the
opening fence from ``` to ```bash) so markdownlint passes and CI markdown checks
recognize it.
- Around line 7-14: The markdown headings "### Bash hooks" and "### Write hooks"
currently have their lists directly under them and fail markdownlint MD022; fix
by adding a single blank line after each of those heading lines so the bullet
lists are separated (e.g., update the sections titled "Bash hooks" and "Write
hooks" to insert one empty line between the heading and the following "-" list
items), then run markdownlint to verify the MD022 warning is resolved.

In `@plugins/security-review/scripts/block-destructive.sh`:
- Around line 16-21: The block that inspects COMMAND currently only matches `rm
-rf /` when that path is the final token and misses variants like `sudo rm -rf
/` or `rm -rf -- /`; update the two grep patterns that use COMMAND to allow an
optional `sudo` prefix and an optional `--` separator and to match the dangerous
path as a token anywhere in the command (not just at end). Specifically adjust
the regexes used against the COMMAND variable (the existing grep -qE patterns)
to accept an optional `(^|\\s)sudo\\s+` before `rm`, an optional `\\s+--\\s+`
between options and the path, and to detect `/`, `~`, or `$HOME` as a standalone
token (use word boundaries or token separators) so variants like `sudo rm -rf --
/` or `rm -rf / extra` are also blocked.
- Around line 1-3: The script block-destructive.sh uses the non-standard
shebang; update the shebang line to use the repo-standard "#!/usr/bin/bash",
keep the existing "set -euo pipefail" line, and audit the script to quote all
variable expansions (e.g., "$var") and any command substitutions to satisfy
shellcheck; run shellcheck, fix reported issues, and commit the updated
block-destructive.sh with the corrected shebang and quoting changes.

In `@plugins/security-review/scripts/check-file-secrets.sh`:
- Around line 31-48: The credential regexes in check-file-secrets.sh (the checks
that append to FOUND for "Hardcoded password/token/secret/API key detected")
only match quoted values and miss common unquoted forms like PASSWORD=secret or
export TOKEN=abc; update each grep -qiE pattern (the ones checking 'password',
'token', 'secret', 'api_key') to also allow unquoted values and optional
"export" and separators (colon or equals) by accepting optional leading
"export\s*", optional quoting, and matching non-whitespace/non-# sequences after
the separator so both quoted and unquoted forms are caught; keep the existing
FOUND appends but change only the regex expressions so behavior and message
strings remain the same.
- Around line 1-3: The script
plugins/security-review/scripts/check-file-secrets.sh uses the non-standard
shebang; change the first line to the repo-standard "#!/usr/bin/bash" (replace
"#!/bin/bash"), then run shellcheck and ensure all variables in
check-file-secrets.sh are properly quoted and the script still keeps set -euo
pipefail so it conforms to hook-script guidelines.

In `@plugins/security-review/scripts/check-secrets.sh`:
- Around line 1-3: Update the script header in check-secrets.sh to use the
repo-standard shebang by replacing "#!/bin/bash" with "#!/usr/bin/bash"; keep
"set -euo pipefail" as-is, audit the script for unquoted variable usages and
quote them (e.g., variables in conditionals, expansions, and command args), and
run/fix any issues reported by shellcheck to ensure hook-script compliance.
- Around line 17-25: The script currently uses git diff --cached (STAGED) to
detect secrets but COMMAND can be "git add", which modifies the working tree and
not the index yet, so new files added (git add secret.env) are missed; update
the hook to either (a) when COMMAND contains "git add" parse the pathspecs from
COMMAND and scan those working-tree files directly for secrets (instead of
relying on STAGED/git diff --cached), or (b) restrict the hook to only run for
"git commit" and leave the cached-diff logic for that case; reference the
COMMAND variable and STAGED handling in the script and ensure the new branch
scans working-tree files named in COMMAND when handling git add.
- Around line 5-10: The script currently treats missing jq as non-fatal (command
-v jq ... exit 0) which disables security checks; change the behavior to fail
closed and/or fall back to a bundled parser: remove the exit 0 path and
implement a fallback that parses INPUT into COMMAND using a safe builtin (e.g.,
python -c 'import
sys,json;print(json.load(sys.stdin).get("toolInput",{}).get("command",""))')
when jq is not available, and if neither jq nor Python are present, print an
error and exit non-zero; apply the same change to the other affected scripts
(plugins/security-review/scripts/block-destructive.sh and
plugins/security-review/scripts/check-file-secrets.sh), updating the jq usage
that reads INPUT -> COMMAND to use the new fallback and ensuring scripts exit
with a non-zero code on missing parsers.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 232e4727-6fde-42ae-b611-c043cee36a44

📥 Commits

Reviewing files that changed from the base of the PR and between c994b2d and 5c91e0a.

📒 Files selected for processing (6)
  • plugins/security-review/.claude-plugin/plugin.json
  • plugins/security-review/README.md
  • plugins/security-review/hooks/hooks.json
  • plugins/security-review/scripts/block-destructive.sh
  • plugins/security-review/scripts/check-file-secrets.sh
  • plugins/security-review/scripts/check-secrets.sh

Comment thread plugins/security-review/README.md
Comment thread plugins/security-review/README.md Outdated
Comment thread plugins/security-review/scripts/block-destructive.sh Outdated
Comment thread plugins/security-review/scripts/block-destructive.sh Outdated
Comment thread plugins/security-review/scripts/check-file-secrets.sh Outdated
Comment thread plugins/security-review/scripts/check-file-secrets.sh
Comment thread plugins/security-review/scripts/check-secrets.sh Outdated
Comment thread plugins/security-review/scripts/check-secrets.sh Outdated
Comment thread plugins/security-review/scripts/check-secrets.sh Outdated
- Use #!/usr/bin/bash shebang across all scripts
- Add jq→python3 fallback, fail closed if neither available
- Broaden credential regexes to catch unquoted values and export prefix
- Improve rm -rf detection to handle sudo and -- separator
- Restrict check-secrets to git commit only (git add covered by Write hook)
- Fix README markdown: blank lines after headings, bash language tag

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@plugins/security-review/scripts/block-destructive.sh`:
- Around line 27-30: The current guard lets commands that mix dangerous targets
with relative paths slip through because the grep pattern excludes matches
containing "./"; update the check on the COMMAND variable (the if using echo
"$COMMAND" | grep -qE ...) so it no longer exempts "./" — instead detect any
invocation of rm with recursive/force flags followed by a dangerous target (/,
~, or $HOME) regardless of other paths present; e.g., replace the regex with one
that simply looks for rm\s+-[a-zA-Z]*r[a-zA-Z]*f[a-zA-Z]*\s+(?:/|~|\$HOME)\b or
otherwise remove the "./" exclusion so mixed-target commands are blocked.
- Around line 24-25: The grep ERE patterns in the block-destructive.sh script
incorrectly use the Perl-style escape \s (which is literal in POSIX ERE); update
every grep -qE pattern that uses \s to use POSIX character class [[:space:]]
(e.g. replace occurrences like \s+ with [[:space:]]+) so the whitespace matching
works; apply this change for the patterns that reference the COMMAND variable
and are invoked with grep -qE (lines noted in the review) so commands like "git
reset --hard" and the rm/rm -rf checks correctly match.

In `@plugins/security-review/scripts/check-file-secrets.sh`:
- Around line 44-66: The current grep checks on CONTENT (which append to FOUND)
are too broad and match templated/env-var references like password=$VAR or
password=${VAR}; update each pattern (the password, token, secret, api_key
checks and the DB connection-string check) to only flag literal values by
rejecting values that start with $ or contain { or {{; e.g. change the regex in
the password/token/secret/api_key checks to use a negative lookahead for $, { or
`{{` such as '(export\s+)?password\s*[:=]\s*(?![${{])\S' (apply similarly for
token/secret/api_key) and tighten the DB regex so the password portion between :
and @ does not start with $ or contain { (e.g.
'(mysql|postgres|postgresql|mongodb|redis)://[^:]+:(?![${])[^\s@]+@'); keep
using CONTENT and FOUND as in the diff.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0bfa6bac-f8fa-48d4-ac2a-2e176d258ae3

📥 Commits

Reviewing files that changed from the base of the PR and between 5c91e0a and d85701b.

📒 Files selected for processing (4)
  • plugins/security-review/README.md
  • plugins/security-review/scripts/block-destructive.sh
  • plugins/security-review/scripts/check-file-secrets.sh
  • plugins/security-review/scripts/check-secrets.sh
✅ Files skipped from review due to trivial changes (1)
  • plugins/security-review/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • plugins/security-review/scripts/check-secrets.sh

Comment thread plugins/security-review/scripts/block-destructive.sh Outdated
Comment thread plugins/security-review/scripts/block-destructive.sh Outdated
Comment thread plugins/security-review/scripts/check-file-secrets.sh Outdated
jeff-roche and others added 2 commits April 22, 2026 13:42
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
block-destructive.sh:
- Remove ./ exclusion that allowed bypassing rm -rf / detection
- Replace \s with POSIX [[:space:]] for portability

check-file-secrets.sh:
- Skip env var refs ($VAR, ${VAR}) and templates ({{ }}) in credential checks
- Tighten DB connection string regex to skip variable passwords

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
plugins/security-review/scripts/check-file-secrets.sh (1)

45-66: ⚠️ Potential issue | 🟠 Major

Quoted env-var/template values are still being flagged as hardcoded secrets.

The updated regexes still match safe patterns like password="${DB_PASSWORD}" and postgres://user:"${DB_PASSWORD}"@host/db because quoted values pass the current first-character exclusion. That reintroduces false positives for env-var-based secrets handling.

Proposed fix (tighten literal-only detection and satisfy shellcheck style)
-# Password assignments — skip env var refs ($VAR, ${VAR}) and templates ({{ }})
-if echo "$CONTENT" | grep -qiE '(export[[:space:]]+)?password[[:space:]]*[:=][[:space:]]*[^${[:space:]]'; then
+# Password assignments — skip env var refs ($VAR, ${VAR}) and templates ({{ }})
+if echo "$CONTENT" | grep -qiE "(export[[:space:]]+)?password[[:space:]]*[:=][[:space:]]*[\"']?[^\\$\\{[:space:]\"']"; then
     FOUND="${FOUND}\n- Hardcoded password detected"
 fi
 
 # Token assignments
-if echo "$CONTENT" | grep -qiE '(export[[:space:]]+)?token[[:space:]]*[:=][[:space:]]*[^${[:space:]]'; then
+if echo "$CONTENT" | grep -qiE "(export[[:space:]]+)?token[[:space:]]*[:=][[:space:]]*[\"']?[^\\$\\{[:space:]\"']"; then
     FOUND="${FOUND}\n- Hardcoded token detected"
 fi
 
 # Secret assignments
-if echo "$CONTENT" | grep -qiE '(export[[:space:]]+)?secret[[:space:]]*[:=][[:space:]]*[^${[:space:]]'; then
+if echo "$CONTENT" | grep -qiE "(export[[:space:]]+)?secret[[:space:]]*[:=][[:space:]]*[\"']?[^\\$\\{[:space:]\"']"; then
     FOUND="${FOUND}\n- Hardcoded secret detected"
 fi
 
 # API key assignments
-if echo "$CONTENT" | grep -qiE '(export[[:space:]]+)?api_key[[:space:]]*[:=][[:space:]]*[^${[:space:]]'; then
+if echo "$CONTENT" | grep -qiE "(export[[:space:]]+)?api_key[[:space:]]*[:=][[:space:]]*[\"']?[^\\$\\{[:space:]\"']"; then
     FOUND="${FOUND}\n- Hardcoded API key detected"
 fi
 
 # Database connection strings with embedded passwords (skip variable refs in password position)
-if echo "$CONTENT" | grep -qiE '(mysql|postgres|postgresql|mongodb|redis)://[^:]+:[^${[:space:]@][^@]*@'; then
+if echo "$CONTENT" | grep -qiE "(mysql|postgres|postgresql|mongodb|redis)://[^:]+:[\"']?[^\\$\\{[:space:]@\"'][^@\\{[:space:]]*@"; then
     FOUND="${FOUND}\n- Database connection string with embedded password detected"
 fi
#!/usr/bin/bash
set -euo pipefail

assign='(export[[:space:]]+)?password[[:space:]]*[:=][[:space:]]*[^${[:space:]]'
db='(mysql|postgres|postgresql|mongodb|redis)://[^:]+:[^${[:space:]@][^@]*@'

echo "Expect NO match for env/template refs:"
for s in \
  'password="${DB_PASSWORD}"' \
  "password='\${DB_PASSWORD}'" \
  'password={{ secrets.DB_PASSWORD }}' \
  'postgres://user:"${DB_PASSWORD}"@host/db'
do
  printf '  %s\n' "$s"
  printf '%s\n' "$s" | grep -qiE "$assign" && echo "    -> ASSIGN matched (unexpected)"
  printf '%s\n' "$s" | grep -qiE "$db" && echo "    -> DB matched (unexpected)"
done

echo "Expect match for literal secrets:"
for s in \
  'password="supersecret"' \
  'postgres://user:literalPass@host/db'
do
  printf '  %s\n' "$s"
  printf '%s\n' "$s" | grep -qiE "$assign" && echo "    -> ASSIGN matched (expected for first case)"
  printf '%s\n' "$s" | grep -qiE "$db" && echo "    -> DB matched (expected for second case)"
done

As per coding guidelines, "General guideline: avoid hardcoded credentials (use environment variables)." and "Shell scripts must ... pass shellcheck."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/security-review/scripts/check-file-secrets.sh` around lines 45 - 66,
The regexes in check-file-secrets.sh (the assignment patterns used for
password/token/secret/api_key and the db connection pattern) are too permissive
and still match quoted/template/env references like password="${DB_PASSWORD}";
update those regexes so the character after the separator ([:=] or the ':' in
connection strings) explicitly forbids quotes, $, { and template braces (e.g.
exclude ['"$\{])—i.e., change the [^${[:space:]] or [^${[:space:]@] character
classes to include quotes and braces as disallowed characters—and apply the same
tightening to the db pattern (the db regex that matches
(mysql|postgres...)://...:...@). Modify the four assignment regexes and the db
regex in check-file-secrets.sh (the variables/patterns shown in this diff) so
literal-only secrets are detected but env/template references are skipped; keep
the regex style compatible with shellcheck.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@plugins/security-review/scripts/block-destructive.sh`:
- Around line 46-49: The current check that inspects COMMAND for "git clean"
only matches "-fd" order; update the matcher so when COMMAND contains "git
clean" it detects any combination or spacing of the "f" and "d" flags (e.g.,
-fd, -df, -f -d, -xdf), by ensuring both flags are present in the clean flags
portion (for example test for "git[[:space:]]+clean" then separately assert
presence of "-f" and "-d" in the flag tokens or use a regex that requires both
letters in any order); keep the existing behavior of printing the same echo
message and exiting when both destructive flags are found, referencing the
COMMAND variable and the existing git clean check.
- Around line 23-27: The current checks on COMMAND allow bypasses because the
first grep requires r and f to appear in the same option token and the second
grep only matches bare ~ or $HOME; change the logic to (1) detect rm invocation
(COMMAND contains "rm") and separately assert both -r and -f appear anywhere in
the arguments (so `-r -f`, `-rf`, `-fr`, etc. are all caught) and (2) broaden
the target-path regex to match ~/ and $HOME/ as well as bare ~ and $HOME (e.g.
accept ~ followed by / or end-of-token and $HOME followed by / or end-of-token);
update the two grep checks that reference COMMAND to implement these rules.

In `@plugins/security-review/scripts/check-file-secrets.sh`:
- Around line 8-10: The jq invocation in parse_json_field uses jq -r "$field"
which prints the literal "null" for null/missing values; change the jq
expression to coerce null/missing to empty (e.g. append // empty to the jq
expression) so that FILE_PATH and CONTENT receive empty output instead of the
string "null" and the existing empty-content early exit works correctly. Update
the jq call in parse_json_field used to populate FILE_PATH and CONTENT
accordingly.

---

Duplicate comments:
In `@plugins/security-review/scripts/check-file-secrets.sh`:
- Around line 45-66: The regexes in check-file-secrets.sh (the assignment
patterns used for password/token/secret/api_key and the db connection pattern)
are too permissive and still match quoted/template/env references like
password="${DB_PASSWORD}"; update those regexes so the character after the
separator ([:=] or the ':' in connection strings) explicitly forbids quotes, $,
{ and template braces (e.g. exclude ['"$\{])—i.e., change the [^${[:space:]] or
[^${[:space:]@] character classes to include quotes and braces as disallowed
characters—and apply the same tightening to the db pattern (the db regex that
matches (mysql|postgres...)://...:...@). Modify the four assignment regexes and
the db regex in check-file-secrets.sh (the variables/patterns shown in this
diff) so literal-only secrets are detected but env/template references are
skipped; keep the regex style compatible with shellcheck.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 99a6dc8b-884f-4b60-95bd-0abc72597e2b

📥 Commits

Reviewing files that changed from the base of the PR and between c5a0849 and 051be4b.

📒 Files selected for processing (2)
  • plugins/security-review/scripts/block-destructive.sh
  • plugins/security-review/scripts/check-file-secrets.sh

Comment thread plugins/security-review/scripts/block-destructive.sh Outdated
Comment thread plugins/security-review/scripts/block-destructive.sh Outdated
Comment thread plugins/security-review/scripts/check-file-secrets.sh
block-destructive.sh:
- Detect -r and -f flags independently (catches rm -r -f /, rm -fr, etc.)
- Catch ~/ and $HOME/ as dangerous targets (bare home deletion)
- Detect git clean -df, -f -d, -xdf (any flag order/combination)

check-file-secrets.sh:
- Add jq // empty to prevent null → "null" string on missing fields
- Exclude quotes from credential char class (skips password="${VAR}")
- Extract shared CRED_REJECT variable for consistent exclusion pattern
- Also tighten DB connection string regex

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
plugins/security-review/scripts/block-destructive.sh (1)

84-88: Consider detecting --recursive in addition to -R for chmod.

The current pattern only matches -R but chmod --recursive 777 /path would bypass the check. This is a minor gap since -R is the more common form.

💡 Optional enhancement
 # chmod -R 777
-if echo "$COMMAND" | grep -qE 'chmod[[:space:]]+(-R[[:space:]]+777|777[[:space:]]+-R)'; then
+if echo "$COMMAND" | grep -qE 'chmod[[:space:]]+((-R|--recursive)[[:space:]]+777|777[[:space:]]+(-R|--recursive))'; then
     echo "Blocked: \`chmod -R 777\` sets insecure permissions. Use more restrictive permissions (e.g., 755 for directories, 644 for files)." >&2
     exit 1
 fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@plugins/security-review/scripts/block-destructive.sh` around lines 84 - 88,
Update the regex that blocks recursive chmod to also detect the long option form
--recursive: change the grep pattern that currently looks for '-R' to include an
alternation for '--recursive' (e.g., use '(-R|--recursive)' or similar) so the
check on COMMAND catches both "chmod -R 777 ..." and "chmod --recursive 777
..."; keep the existing checks for 777 and spacing around COMMAND so the error
and exit behavior (echo to stderr and exit 1) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@plugins/security-review/scripts/block-destructive.sh`:
- Around line 84-88: Update the regex that blocks recursive chmod to also detect
the long option form --recursive: change the grep pattern that currently looks
for '-R' to include an alternation for '--recursive' (e.g., use
'(-R|--recursive)' or similar) so the check on COMMAND catches both "chmod -R
777 ..." and "chmod --recursive 777 ..."; keep the existing checks for 777 and
spacing around COMMAND so the error and exit behavior (echo to stderr and exit
1) remain unchanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f049f6e5-ae84-460c-be29-b4828e22fecd

📥 Commits

Reviewing files that changed from the base of the PR and between 051be4b and 56af75a.

📒 Files selected for processing (2)
  • plugins/security-review/scripts/block-destructive.sh
  • plugins/security-review/scripts/check-file-secrets.sh

@brandisher
Copy link
Copy Markdown
Contributor

/assign

Copy link
Copy Markdown
Contributor

@brandisher brandisher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR has similar highlights to #75

The plugin is security-review but its not reviewing security and generally that has a defined context that's not captured in these changes; I suspect that's also not really the target for this. These also aren't protections that we'd want to optionally install -- a plugin isn't a good use case for this.

Given that this is security related and on the proactive side, each script sits in a different place in the lifecycle. block-desctructive would make sense as a repo level hook. check-file-secrets/check-secrets are pre-commit hooks which could be leveraged by the work done in #71. Both check-secrets and check-file-secrets need tests; there's nothing here validating the regex correctness.

/hold

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2026
openshift-merge-bot Bot pushed a commit that referenced this pull request Apr 23, 2026
…ew process (#77)

* docs: add architectural guidelines from PR #74 and #75 reviews

Codify recurring review feedback into CONTRIBUTING.md so contributors
can self-evaluate before submitting: naming accuracy, hooks vs plugins
for mandatory checks, lifecycle placement, reuse, and test coverage
for validation logic.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: remove ai-helpers references from CONTRIBUTING.md

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
@@ -0,0 +1,37 @@
# security-review
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: does this needs to be a Claude plugin? sounds more like a git pre-commit, it could be more efficient

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that's fair (also echo's what @brandisher mentioned)

I think there are arguments that it could be both so you can apply it across the repos you work in but we enforce it within this repo via githooks/claude hooks

@jeff-roche jeff-roche marked this pull request as draft April 27, 2026 12:27
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 27, 2026
@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 1, 2026

@jeff-roche: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/shellcheck 56af75a link true /test shellcheck

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants