feat(gatekeeper): support safe fd redirects in chain parser#95
feat(gatekeeper): support safe fd redirects in chain parser#95
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the gatekeeper's command chain parser by introducing more granular handling of shell redirects. It specifically allows safe file descriptor (FD) to FD redirects and /dev/null redirects, which are common and harmless, while continuing to block potentially dangerous arbitrary file redirects. This refinement reduces the number of legitimate commands incorrectly flagged for AI review, streamlining the approval process and improving the overall efficiency of command execution without compromising security. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refines the chain-parser logic to allow specific safe shell redirect syntaxes, such as file descriptor redirects (e.g., 2>&1, >&-) and redirects to /dev/null, which were previously marked as unparseable. It introduces an isDigit helper function and updates the parsing mechanism to differentiate these safe operations from potentially dangerous arbitrary file redirects. New test cases have been added to validate these changes. A review comment points out a potential out-of-bounds read in the whitespace skipping loop within the parseChainedCommand function, suggesting a boundary check for robustness.
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 3/5
- There is a concrete parser correctness risk in
plugins/gatekeeper/src/chain-parser.ts: matching the/dev/nullprefix without a word-boundary/next-character check can incorrectly approve paths like/dev/nullicious. - Because this issue is severity 7/10 with high confidence (9/10) and can affect redirect validation behavior, this carries real user-impacting regression risk before merge.
- The change still appears narrowly scoped to parser logic, so risk is manageable once the boundary check is added and validated.
- Pay close attention to
plugins/gatekeeper/src/chain-parser.ts- ensure/dev/nullmatching enforces a proper boundary so only the exact target is accepted.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/gatekeeper/src/chain-parser.ts">
<violation number="1" location="plugins/gatekeeper/src/chain-parser.ts:121">
P1: Missing word-boundary check after `/dev/null` match allows the parser to approve redirects to files like `/dev/nullicious`. After confirming the 9-character prefix matches `/dev/null`, verify the next character is a word boundary (end-of-string, whitespace, `;`, `&`) — otherwise fall through to the "unsafe" path.</violation>
</file>
Architecture diagram
sequenceDiagram
participant User as User/System
participant GK as Gatekeeper Service
participant CP as Chain Parser
participant AI as AI Review Layer
participant Exec as Execution Engine
Note over User,Exec: Command Submission Flow
User->>GK: Request command execution (e.g., 'cd /app && bun test 2>&1')
GK->>CP: parseChainedCommand(cmd)
rect rgb(240, 240, 240)
Note over CP: Tokenization & Safety Check
CP->>CP: Identify chain operators (&&, ;)
alt NEW: Safe Redirect Detected
Note right of CP: Matches 2>&1, >&2, <&3, >&-, <&- or /dev/null
CP->>CP: Include redirect in command segment
else CHANGED: Unsafe Redirect Detected
Note right of CP: Matches > file.txt, < file.txt, >> file.txt
CP-->>GK: Return { kind: 'unparseable' }
else Pipe or Subshell
CP-->>GK: Return { kind: 'unparseable' }
end
end
alt Command is Parseable
CP-->>GK: Return list of commands
GK->>GK: Validate commands against allow-list
alt All commands approved
GK->>Exec: Run commands sequentially
Exec-->>User: Command Output
else Safety violation
GK->>AI: Request manual/AI review
AI-->>User: Review results
end
else Command is Unparseable
GK->>AI: Request manual/AI review
AI-->>User: Review results
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="plugins/gatekeeper/src/pre-tool-use.test.ts">
<violation number="1" location="plugins/gatekeeper/src/pre-tool-use.test.ts:100">
P2: This assertion is ambiguous: `splitChainedCommands(...).toBeNull()` passes for both safe-single and unparseable, so it does not actually verify `/dev/nullicious` is rejected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Allow fd-to-fd redirects (2>&1, >&2, <&3, >&-) and /dev/null redirects (>/dev/null, 2>/dev/null, >>/dev/null) in the chain parser instead of immediately classifying them as unparseable. Arbitrary file redirects (> file.txt, >> file.txt, < file.txt) remain blocked.
- Add bounds check to whitespace-skipping while loop in /dev/null redirect parser (gemini-code-assist) - Add word-boundary check after /dev/null match to prevent approving paths like /dev/nullicious (cubic-dev-ai P1) - Add regression test for /dev/nullicious boundary case
Replace ambiguous splitChainedCommands assertion for /dev/nullicious with evaluate(bash(...)) to explicitly verify the unsafe redirect is passed through
c559b45 to
a9a7e1d
Compare
Summary
2>&1,>&2,<&3,>&-,<&-) in the chain parser instead of marking them asunparseable/dev/nullredirects (>/dev/null,2>/dev/null,>>/dev/null) with optional whitespace> file.txt,>> file.txt,< file.txt) remain blocked asunparseablecd /path && bun test 2>&1that were previously sent to AI reviewTest plan
bun test)2>&1,>&2,2>&-,<&3,<&-) parsed as single commands/dev/nullredirects with/without whitespace parsed as single commandscmd1 && cmd2 2>&1)> file.txt,>> file.txt,< file.txt) stillunparseablels 2>&1 | grep foo) stillunparseable(passthrough)bun run buildproduces updated distSummary by cubic
Add support for safe fd redirects and /dev/null redirects in the Gatekeeper chain parser while still blocking file redirects. Hardened /dev/null parsing and auto-approve chains like cd /path && bun test 2>&1.
New Features
Bug Fixes
Written for commit 98c6860. Summary will update on new commits.