Release v2.0.0: security hardening, test suite, CI pipeline#43
Merged
AnExiledDev merged 7 commits intomainfrom Mar 2, 2026
Merged
Release v2.0.0: security hardening, test suite, CI pipeline#43AnExiledDev merged 7 commits intomainfrom
AnExiledDev merged 7 commits intomainfrom
Conversation
Document the staging branch workflow: feature branches target staging for PRs, and staging is PRed to main for releases.
- Remove env var injection in agent redirect log path (S2-01) - Protected files guards fail closed on unexpected errors (S2-04) - Harden dangerous command regexes: prefix stripping, symbolic chmod, setuid, docker system/volume, git filter-branch, plus-refspec (S2-03) - Unify write-target patterns across guards (5 → 18 patterns) (C1-02) - Fix greedy alternation in redirect regex (>>|> order) (Q3-01) - Block bare git stash in read-only mode (Q3-04) - Narrow configApply allowed destinations to /usr/local/share (S2-09) - Add pytest to CI pipeline (Q3-08) - Add 36 new test cases covering all fixes (241 → 277 tests)
- Version 1.14.2 → 2.0.0 - prepublishOnly now runs npm run test:all (Node + Python tests) - Sync README version to match package.json
The test hardcoded /home/vscode but CI runs as /home/runner. Use the module's _home variable to match the actual environment.
Fix critical security findings from codebase review
📝 WalkthroughWalkthroughThis PR hardens guard scripts (fail-closed on errors), expands dangerous-command detection and write-target extraction (multi-targets), refines git-stash readonly logic, adds extensive plugin tests, updates CI to run plugin tests on staging, restricts deployment paths, and updates changelog/documentation and logging behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py (1)
44-47: Dead code:if not LOG_FILE:check is now unreachable.Since
LOG_FILEis now a hardcoded non-empty string, this guard will never trigger. Consider removing it or, if you want to retain the ability to disable logging, use a separate constant (e.g.,LOGGING_ENABLED = True).♻️ Option A: Remove the dead guard
def log(message: str) -> None: """Append a timestamped log entry if logging is enabled.""" - if not LOG_FILE: - return try: with open(LOG_FILE, "a") as f:♻️ Option B: Explicit toggle for disabling logs
LOG_FILE = "/tmp/agent-redirect.log" +LOGGING_ENABLED = True def log(message: str) -> None: - """Append a timestamped log entry if logging is enabled.""" - if not LOG_FILE: + """Append a timestamped log entry.""" + if not LOGGING_ENABLED: return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py around lines 44 - 47, The guard in the log function is dead because LOG_FILE is now a hardcoded non-empty string; remove the unreachable check or replace it with a proper toggle. Update the log(message: str) function to either (a) drop the "if not LOG_FILE: return" branch entirely and always append to LOG_FILE, or (b) introduce a separate boolean constant like LOGGING_ENABLED and check that instead, using the existing LOG_FILE as the target; ensure you modify references in log and any initialization that mentions LOG_FILE so behavior is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/CHANGELOG.md:
- Around line 12-13: Update the stale release-note counts in CHANGELOG.md: under
the "Plugin test suite" block update the total tests count from "241" to "277",
adjust the block-dangerous.py entry if its test count or pattern count changed
(e.g., update "46 tests" or "22 dangerous command patterns" to reflect the PR),
and change the referenced expansion from "18" patterns to "20" write-target
patterns so the summary aligns with the PR description; locate the "Plugin test
suite" header and the "block-dangerous.py" list item and edit those numeric
literals accordingly.
- Line 133: The changelog in the `## [Unreleased]` block contains repeated
top-level headings (`### Fixed`, `### Added`) which triggers MD024; consolidate
duplicated headings by merging all entries under a single `### Fixed` and a
single `### Added` section respectively so each heading appears only once per
release block (remove or move the duplicate `### Fixed`/`### Added` occurrences
and combine their list items), preserving entry order and formatting so the `##
[Unreleased]` block has one instance of each allowed top-level heading.
- Around line 5-7: The changelog uses a disallowed top-level heading "###
Security" in the Unreleased section; change that heading to an area subheading
(e.g., "#### Security") and place those two bullet lines under one of the
permitted top-level headings such as "### Changed" or "### Fixed" in the
Unreleased section so the structure follows the required groups (use "###
Added", "### Changed", "### Fixed", or "### Removed" as the parent and replace
"### Security" with "#### Security").
In
@.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/guard-readonly-bash.py:
- Around line 551-556: The current check for the "stash" subcommand incorrectly
assumes the subcommand is at words[2]; instead use the actual index where
"stash" was found (the index tracked during the global-flag skipping logic) —
e.g., use stash_index (or the same variable you used to set sub) and check the
token at stash_index+1 exists and is one of ("list","show"); if there is no next
token treat it as a bare "git stash" and block it, and if the next token is not
in ("list","show") block it and include that token in the error message.
In
@.devcontainer/plugins/devs-marketplace/plugins/protected-files-guard/scripts/guard-protected-bash.py:
- Around line 71-85: The regex list currently extracts only one target per
match, allowing multi-target commands to bypass protection; update the command
parsing in the script that iterates these regexes so that for multi-target
commands (e.g., patterns for "rm", "touch", "mkdir", "chmod", "chown", "ln",
"install", "rsync") you tokenize the command string with a shell-aware splitter
(e.g., shlex.split), collect all non-option operands as candidate paths, and
call check_path() on each operand instead of a single captured group. Ensure the
code paths that currently use the single-capture regex output (the regex list
and the caller that invokes check_path()) are adjusted to handle multiple
targets, and add unit tests for cases like "rm safe.txt .env", "touch a.txt
.secrets", and "chmod 600 safe.txt id_rsa" to verify all targets are checked.
---
Nitpick comments:
In
@.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py:
- Around line 44-47: The guard in the log function is dead because LOG_FILE is
now a hardcoded non-empty string; remove the unreachable check or replace it
with a proper toggle. Update the log(message: str) function to either (a) drop
the "if not LOG_FILE: return" branch entirely and always append to LOG_FILE, or
(b) introduce a separate boolean constant like LOGGING_ENABLED and check that
instead, using the existing LOG_FILE as the target; ensure you modify references
in log and any initialization that mentions LOG_FILE so behavior is consistent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.devcontainer/CHANGELOG.md.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/guard-readonly-bash.py.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py.devcontainer/plugins/devs-marketplace/plugins/dangerous-command-blocker/scripts/block-dangerous.py.devcontainer/plugins/devs-marketplace/plugins/protected-files-guard/scripts/guard-protected-bash.py.devcontainer/plugins/devs-marketplace/plugins/protected-files-guard/scripts/guard-protected.py.devcontainer/plugins/devs-marketplace/plugins/workspace-scope-guard/scripts/guard-workspace-scope.py.github/workflows/ci.ymlCLAUDE.mdREADME.mdpackage.jsonsetup.jstests/plugins/test_block_dangerous.pytests/plugins/test_guard_protected.pytests/plugins/test_guard_protected_bash.pytests/plugins/test_guard_readonly_bash.pytests/plugins/test_guard_workspace_scope.py
.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/guard-readonly-bash.py
Outdated
Show resolved
Hide resolved
...ainer/plugins/devs-marketplace/plugins/protected-files-guard/scripts/guard-protected-bash.py
Show resolved
Hide resolved
- Remove dead `if not LOG_FILE` guard in redirect-builtin-agents.py - Fix git global flag handling in stash/config index resolution - Add multi-target extraction for rm/touch/mkdir/chmod/chown commands - Switch CLAUDE.md changelog guidelines to domain headings - Restructure [Unreleased] changelog to domain headings, fix test counts - Add 12 new tests (289 total): global-flag stash tests, multi-target tests
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py (1)
41-50:⚠️ Potential issue | 🟠 MajorAvoid fixed
/tmplog path; it enables log-file hijacking via predictable temp location.At Line 41, writing to a constant
/tmp/agent-redirect.logwith plain append open (Line 47) can be abused via pre-created files/symlinks in shared environments. This weakens the security posture despite removing env-var injection.Note:
AGENT-REDIRECTION.md(Line 219) explicitly documents checking/tmp/agent-redirect.logfor testing. Any path migration must update this documentation reference.🔒 Suggested hardening patch
+import os import json import sys from datetime import datetime, timezone +from pathlib import Path @@ -LOG_FILE = "/tmp/agent-redirect.log" +LOG_DIR = Path.home() / ".cache" / "agent-system" +LOG_FILE = LOG_DIR / "agent-redirect.log" @@ def log(message: str) -> None: """Append a timestamped log entry.""" try: - with open(LOG_FILE, "a") as f: + LOG_DIR.mkdir(mode=0o700, parents=True, exist_ok=True) + flags = os.O_APPEND | os.O_CREAT | os.O_WRONLY + if hasattr(os, "O_NOFOLLOW"): + flags |= os.O_NOFOLLOW + fd = os.open(LOG_FILE, flags, 0o600) + with os.fdopen(fd, "a", encoding="utf-8") as f: ts = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%S") f.write(f"[{ts}] {message}\n") except OSError: pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py around lines 41 - 50, The LOG_FILE constant and log() function currently write to a predictable /tmp/agent-redirect.log which is vulnerable to symlink/hijack attacks; change LOG_FILE to use a non-predictable, per-process/per-user path (e.g. incorporate PID, UID or use a secure runtime dir like XDG_RUNTIME_DIR) and modify log() to open the file safely (create-only semantics or O_EXCL, or use tempfile.NamedTemporaryFile in a secure directory) instead of blindly appending to a fixed /tmp path; update the AGENT-REDIRECTION.md reference to the new location so tests/docs still point to the correct file.
♻️ Duplicate comments (2)
.devcontainer/CHANGELOG.md (2)
11-13:⚠️ Potential issue | 🟡 MinorUpdate stale test-count release notes.
Line 11 states 289 pytest tests, but this PR’s objective summary says 277/277. Please align the number to avoid release-note drift.
Based on learnings "Every change MUST have a corresponding entry in
.devcontainer/CHANGELOG.md", and that entry must accurately reflect the change set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/CHANGELOG.md around lines 11 - 13, The release note under "Plugin test suite" contains a stale test count (reads "289 pytest tests") that doesn't match the PR summary ("277/277"); update the numeric value in that changelog entry to 277 so the `.devcontainer/CHANGELOG.md` line for "Plugin test suite" accurately reflects the current test count and avoid release-note drift.
5-44:⚠️ Potential issue | 🟡 MinorRestructure
Unreleasedheadings to the required changelog format.Line 5 and the other top-level headings in this block should not be domain headings. Use one set of allowed top-level groups (
### Added,### Changed,### Fixed,### Removed) and nest area headings as#### Security,#### Testing, etc.Proposed structure
-### Security -- Removed environment variable injection vector in agent redirect log path (S2-01) +### Fixed +#### Security +- Removed environment variable injection vector in agent redirect log path (S2-01) ... -### Testing +### Added +#### Testing ... -### Dangerous Command Blocker +### Changed +#### Dangerous Command BlockerAs per coding guidelines "Group changelog entries under appropriate headings:
### Added,### Changed,### Fixed, or### Removed" and "Use sub-headings (####) in CHANGELOG.md to organize entries by area (e.g., Workspace Scope Guard, Features, Configuration)".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/CHANGELOG.md around lines 5 - 44, The current top-level headings (e.g., "### Security", "### Testing", "### Dangerous Command Blocker", "### Guards", "### Documentation") must be converted into sub-headings under one of the allowed top-level groups; create the four required top-level headings "### Added", "### Changed", "### Fixed", and "### Removed" and move each of the existing sections under the most appropriate group as "#### Security", "#### Testing", "#### Dangerous Command Blocker", "#### Guards", and "#### Documentation" (for example: Testing and Documentation entries under "### Added"/"### Changed", Security/Guards/Dangerous Command Blocker items under "### Fixed" or "### Changed" as appropriate), ensuring no other top-level "###" headings remain and that each original "### ..." is replaced by a "#### ..." sub-heading beneath the chosen allowed top-level group.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/CHANGELOG.md:
- Around line 22-33: Rewrite the CHANGELOG bullets to be user-facing outcomes
rather than implementation details: replace lines like "was slipping through
regex" and "greedy alternation in write-target regex" with outcome statements
such as "force-with-lease is now blocked" and "append redirects are detected
before overwrite redirects"; convert "misidentifies subcommand" to "git stash
with global flags is correctly recognized" and similar for other items (e.g.,
change "Fixed README — error handling was documented as 'fails open' but code
actually fails closed" to "README updated to reflect that error handling fails
closed"). Update the Guards section entries (e.g., "Multi-target command
support", "Bare `git stash`", "Fixed git global flag handling") to describe the
user-visible behavior changes rather than the internal fixes.
---
Outside diff comments:
In
@.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py:
- Around line 41-50: The LOG_FILE constant and log() function currently write to
a predictable /tmp/agent-redirect.log which is vulnerable to symlink/hijack
attacks; change LOG_FILE to use a non-predictable, per-process/per-user path
(e.g. incorporate PID, UID or use a secure runtime dir like XDG_RUNTIME_DIR) and
modify log() to open the file safely (create-only semantics or O_EXCL, or use
tempfile.NamedTemporaryFile in a secure directory) instead of blindly appending
to a fixed /tmp path; update the AGENT-REDIRECTION.md reference to the new
location so tests/docs still point to the correct file.
---
Duplicate comments:
In @.devcontainer/CHANGELOG.md:
- Around line 11-13: The release note under "Plugin test suite" contains a stale
test count (reads "289 pytest tests") that doesn't match the PR summary
("277/277"); update the numeric value in that changelog entry to 277 so the
`.devcontainer/CHANGELOG.md` line for "Plugin test suite" accurately reflects
the current test count and avoid release-note drift.
- Around line 5-44: The current top-level headings (e.g., "### Security", "###
Testing", "### Dangerous Command Blocker", "### Guards", "### Documentation")
must be converted into sub-headings under one of the allowed top-level groups;
create the four required top-level headings "### Added", "### Changed", "###
Fixed", and "### Removed" and move each of the existing sections under the most
appropriate group as "#### Security", "#### Testing", "#### Dangerous Command
Blocker", "#### Guards", and "#### Documentation" (for example: Testing and
Documentation entries under "### Added"/"### Changed", Security/Guards/Dangerous
Command Blocker items under "### Fixed" or "### Changed" as appropriate),
ensuring no other top-level "###" headings remain and that each original "###
..." is replaced by a "#### ..." sub-heading beneath the chosen allowed
top-level group.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.devcontainer/CHANGELOG.md.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/guard-readonly-bash.py.devcontainer/plugins/devs-marketplace/plugins/agent-system/scripts/redirect-builtin-agents.py.devcontainer/plugins/devs-marketplace/plugins/protected-files-guard/scripts/guard-protected-bash.pyCLAUDE.mdtests/plugins/test_guard_protected.pytests/plugins/test_guard_protected_bash.pytests/plugins/test_guard_readonly_bash.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
Comment on lines
+22
to
+33
| - **Force push block now suggests `git merge` as workaround** — error message explains how to avoid diverged history instead of leaving the agent to improvise destructive workarounds | ||
| - **Block `--force-with-lease`** — was slipping through regex; all force push variants now blocked uniformly | ||
| - **Block remote branch deletion** — `git push origin --delete` and colon-refspec deletion (`git push origin :branch`) now blocked; deleting remote branches closes associated PRs | ||
| - **Fixed README** — error handling was documented as "fails open" but code actually fails closed; corrected to match behavior | ||
| - Dangerous command blocker handles prefix bypasses (`\rm`, `command rm`, `env rm`) and symbolic chmod (S2-03) | ||
|
|
||
| ### Guards | ||
| - Fixed greedy alternation in write-target regex — `>>` now matched before `>` (Q3-01) | ||
| - Unified write-target extraction patterns across guards — protected-files bash guard expanded from 5 to 20 patterns (C1-02) | ||
| - Multi-target command support — `rm`, `touch`, `mkdir`, `chmod`, `chown` with multiple file operands now check all targets | ||
| - Bare `git stash` (equivalent to push) now blocked in read-only mode (Q3-04) | ||
| - Fixed git global flag handling — `git -C /path stash list` no longer misidentifies the stash subcommand |
There was a problem hiding this comment.
Rewrite implementation-centric bullets from user-impact perspective.
Several bullets describe internal mechanics (e.g., “was slipping through regex”, “greedy alternation”, “misidentifies subcommand”). Prefer outcome language (“force-with-lease is now blocked”, “append redirects are correctly detected first”, etc.) so release notes stay user-focused.
As per coding guidelines "Write CHANGELOG entries from the user's perspective — what changed, not how it was implemented".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/CHANGELOG.md around lines 22 - 33, Rewrite the CHANGELOG
bullets to be user-facing outcomes rather than implementation details: replace
lines like "was slipping through regex" and "greedy alternation in write-target
regex" with outcome statements such as "force-with-lease is now blocked" and
"append redirects are detected before overwrite redirects"; convert
"misidentifies subcommand" to "git stash with global flags is correctly
recognized" and similar for other items (e.g., change "Fixed README — error
handling was documented as 'fails open' but code actually fails closed" to
"README updated to reflect that error handling fails closed"). Update the Guards
section entries (e.g., "Multi-target command support", "Bare `git stash`",
"Fixed git global flag handling") to describe the user-visible behavior changes
rather than the internal fixes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Release PR merging staging into main for v2.0.0 public release.
Security Hardening (PR #42)
\rm,command rm,env rm) with 11 new blocked patterns (S2-03)/usr/localto/usr/local/share(S2-09)>>now matched before>(Q3-01)git stashblocked in read-only mode (Q3-04)Test Suite
prepublishOnlynow runs full test suite (npm run test:all)CI Pipeline
test-pluginsjob (Python + pytest) to GitHub ActionsmainandstagingbranchesPre-release Cleanup (PR #41)
.env.examplesynced with missing flags--resetoption documentedTest plan
npm test— 18/18 passpytest tests/ -v— 277/277 passSummary by CodeRabbit
New Features
Bug Fixes
Documentation
CI/CD
Chores