fix: block unsafe root-like delete_file paths#2859
Conversation
|
@BugBot run |
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 focuses on improving the stability and user control within the local agent. It addresses a critical security concern by implementing comprehensive validation and runtime checks for the Highlights
Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
wwwillchen
added
the
needs-human:review-issue
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a crucial security fix to prevent the delete_file tool from targeting project root-equivalent paths. The implementation is robust, adding validation at both the schema and runtime levels, and is well-supported by new unit tests. I've suggested a minor simplification to the path validation logic to enhance code clarity.
Additionally, a new step-limit feature has been added to pause the agent after a configurable number of tool calls, which is a great enhancement for controlling agent execution. The implementation looks solid, though I've identified an unused limit property being passed through components and have recommended its removal to keep the code clean. Overall, these are valuable improvements to the agent's safety and control.
| normalizedPath === "." || | ||
| normalizedPath === "./" || | ||
| normalizedPath === "" |
There was a problem hiding this comment.
The conditions normalizedPath === "./" and normalizedPath === "" are redundant because path.posix.normalize() converts an empty string or './' into '.'. Simplifying this security-related check to just normalizedPath === "." will make the code cleaner and easier to maintain without altering its behavior.
normalizedPath === "."| } | ||
|
|
||
| export function DyadStepLimit({ node, children }: DyadStepLimitProps) { | ||
| const { steps = "50", limit: _limit = "50", state } = node.properties; |
There was a problem hiding this comment.
The limit property is destructured from node.properties and aliased to _limit, but it's never used within the component. This variable should be removed to improve code clarity. The limit prop is passed from DyadMarkdownParser.tsx and should be removed there as well.
| const { steps = "50", limit: _limit = "50", state } = node.properties; | |
| const { steps = "50", state } = node.properties; |
| node={{ | ||
| properties: { | ||
| steps: attributes.steps, | ||
| limit: attributes.limit, |
There was a problem hiding this comment.
The limit prop is passed to the DyadStepLimit component but is not used there. It should be removed to avoid passing an unused prop and to keep the component interface clean.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 474de522ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| // Check if we hit the step limit and append a notice to the response | ||
| if (totalStepsExecuted >= MAX_TOOL_CALL_STEPS) { |
There was a problem hiding this comment.
This appends <dyad-step-limit> whenever totalStepsExecuted >= MAX_TOOL_CALL_STEPS, but totalStepsExecuted is cumulative across outer passes (including todo follow-up passes), so the notice can be added even when no pass was actually cut off by stepCountIs(...) and the agent already finished normally. In a multi-pass run (e.g., ~30 steps in pass 1 and ~20 in pass 2), users will still see “Paused after 50 tool calls” and an unnecessary Continue CTA, which misrepresents completion state and can trigger redundant follow-up turns.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| if ( | ||
| normalizedPath === "." || | ||
| normalizedPath === "./" || | ||
| normalizedPath === "" |
There was a problem hiding this comment.
Dead conditions after path.posix.normalize call
Low Severity
The conditions normalizedPath === "./" and normalizedPath === "" are unreachable dead code. path.posix.normalize never returns "./" (it strips trailing slashes, returning "." instead) and never returns "" (it converts empty strings to "."). Both inputs of concern already collapse to ".", which is correctly caught by the preceding normalizedPath === "." check. The dead branches create a false impression that these cases require separate handling.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files
Confidence score: 4/5
- This PR looks safe to merge with minimal risk: the reported issue is moderate-low severity (4/10) and appears limited to pause-state messaging behavior rather than a hard failure.
- In
src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts, using total steps across all passes for the limit check can show a "paused" message even when no individual pass actually hitMAX_TOOL_CALL_STEPS, which may confuse users and trigger incorrect status reporting. - Pay close attention to
src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts- ensure the limit condition is based on a per-pass hit (steps.length >= MAX_TOOL_CALL_STEPS) instead of cross-pass totals.
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="src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts">
<violation number="1" location="src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts:1147">
P2: The step-limit check uses the total steps across all passes, which can trigger the "paused" message even when no pass hit the per-pass limit. Track a per-pass limit hit (e.g., `steps.length >= MAX_TOOL_CALL_STEPS`) and only emit the notice when the limit was actually reached.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
|
|
||
| // Check if we hit the step limit and append a notice to the response | ||
| if (totalStepsExecuted >= MAX_TOOL_CALL_STEPS) { |
There was a problem hiding this comment.
P2: The step-limit check uses the total steps across all passes, which can trigger the "paused" message even when no pass hit the per-pass limit. Track a per-pass limit hit (e.g., steps.length >= MAX_TOOL_CALL_STEPS) and only emit the notice when the limit was actually reached.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/pro/main/ipc/handlers/local_agent/local_agent_handler.ts, line 1147:
<comment>The step-limit check uses the total steps across all passes, which can trigger the "paused" message even when no pass hit the per-pass limit. Track a per-pass limit hit (e.g., `steps.length >= MAX_TOOL_CALL_STEPS`) and only emit the notice when the limit was actually reached.</comment>
<file context>
@@ -1133,6 +1143,24 @@ export async function handleLocalAgentStream(
}
+ // Check if we hit the step limit and append a notice to the response
+ if (totalStepsExecuted >= MAX_TOOL_CALL_STEPS) {
+ logger.info(
+ `Chat ${req.chatId} hit step limit of ${MAX_TOOL_CALL_STEPS} steps`,
</file context>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| normalizedPath === "." || | ||
| normalizedPath === "./" || | ||
| normalizedPath === "" | ||
| ) { |
There was a problem hiding this comment.
🟡 MEDIUM | security
Parent-traversal path .. bypasses the root-deletion guard
path.posix.normalize('..') returns '..', not '.', so a bare .. path passes all three conditions ('.', './', '') and falls through to safeJoin. While safeJoin will ultimately throw for .. (preventing actual deletion), the guard's purpose is to provide a clear "Refusing to delete project root" error. The current code gives a different, less descriptive error from safeJoin instead.
Also: path.posix.normalize never returns './' or '' — it normalizes both to '.'. These two branches are unreachable dead code.
💡 Suggestion: Simplify and strengthen the guard:
| ) { | |
| if ( | |
| normalizedPath === "." || | |
| normalizedPath === ".." || | |
| normalizedPath.startsWith("../") | |
| ) { |
| fullResponse += stepLimitMessage; | ||
| await updateResponseInDb(placeholderMessageId, fullResponse); | ||
| sendResponseChunk( | ||
| event, |
There was a problem hiding this comment.
🟡 MEDIUM | logic
Step limit detection uses cross-pass accumulation that can false-positive
totalStepsExecuted accumulates steps.length from every pass through the outer while loop (main pass + todo follow-up passes). stepCountIs(MAX_TOOL_CALL_STEPS) resets per-pass, so individual passes are capped at 50. But if pass 1 uses 30 steps (stopped for another reason, e.g. addIntegrationTool) and pass 2 uses 25 steps, totalStepsExecuted = 55 >= 50 triggers the notification even though neither pass was stopped by the step limit.
💡 Suggestion: Track whether any pass was actually stopped by the step limit rather than summing:
const hitStepLimit = steps.length >= MAX_TOOL_CALL_STEPS;Then use if (hitStepLimit) for the notification.
| onClick={handleContinue} | ||
| className="ml-auto hover:cursor-pointer" | ||
| > | ||
| {isLoading ? ( |
There was a problem hiding this comment.
🟡 MEDIUM | interaction
Continue button not disabled while a stream is already active
The button is only disabled by the local isLoading state. If the chat is already streaming (e.g., the user triggered another prompt), the button remains enabled. useStreamChat() likely silently no-ops via pendingStreamChatIds, but the user gets no feedback.
Other similar action cards (e.g., DyadAddIntegration) solve this by consuming isStreaming from useStreamChat().
💡 Suggestion: Destructure isStreaming from useStreamChat() and set disabled={isLoading || isStreaming}.
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (2 items)
🚫 Dropped False Positives (4 items)
Generated by Dyadbot multi-agent code review |
Greptile SummaryThis PR hardens the Key points:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[delete_file called with path] --> B{Schema: path not blank?}
B -- No --> C[Zod error: Path cannot be empty]
B -- Yes --> D[Normalize with path.posix.normalize]
D --> E{normalizedPath equals dot?}
E -- Yes --> F[throw: Refusing to delete project root]
E -- No --> G[safeJoin appPath + args.path]
G --> H{Path escapes base dir?}
H -- Yes --> I[throw: Unsafe path]
H -- No --> J{fs.existsSync?}
J -- No --> K[warn: file does not exist]
J -- Yes --> L{isDirectory?}
L -- Yes --> M[rmdirSync recursive]
L -- No --> N[unlinkSync]
M --> O[gitRemove]
N --> O
O --> P{supabaseProjectId and isServerFunction?}
P -- Yes --> Q[deleteSupabaseFunction]
P -- No --> R[return: Successfully deleted]
Q --> R
K --> R
Last reviewed commit: 5dcdb32 |
Prevent delete_file from accepting blank or project-root-equivalent paths so accidental root deletion cannot occur, and add regression coverage for dangerous inputs. Also document a GitHub label-removal edge case involving colon characters in gh api DELETE paths. Made-with: Cursor
wwwillchen
force-pushed
the
agent-2754-1772236131
branch
from
474de52 to
bc5e06c
Compare
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (1 item)
🚫 Dropped False Positives (5 items)
Note: 3 issues already flagged by other reviewers were confirmed valid and deduplicated:
Generated by Dyadbot multi-agent code review |
| execute: async (args, ctx: AgentContext) => { | ||
| const normalizedPath = path.posix.normalize( | ||
| args.path.replace(/\\/g, "/"), | ||
| ); | ||
| if ( | ||
| normalizedPath === "." || | ||
| normalizedPath === "./" || | ||
| normalizedPath === "" | ||
| ) { | ||
| throw new Error( | ||
| `Refusing to delete project root for path: "${args.path}"`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | correctness
Guard and safeJoin use different normalization strategies
The guard converts backslashes to / and uses path.posix.normalize, but safeJoin (called on line 58 with the original args.path) uses OS-native path.resolve. These two layers reason about paths using different normalization semantics, which makes the guard fragile and hard to audit.
A simpler, more robust approach would resolve the path once against ctx.appPath and compare:
| execute: async (args, ctx: AgentContext) => { | |
| const normalizedPath = path.posix.normalize( | |
| args.path.replace(/\\/g, "/"), | |
| ); | |
| if ( | |
| normalizedPath === "." || | |
| normalizedPath === "./" || | |
| normalizedPath === "" | |
| ) { | |
| throw new Error( | |
| `Refusing to delete project root for path: "${args.path}"`, | |
| ); | |
| } | |
| const resolvedPath = path.resolve(ctx.appPath, args.path); | |
| if (resolvedPath === path.resolve(ctx.appPath)) { | |
| throw new Error( | |
| `Refusing to delete project root for path: "${args.path}"`, | |
| ); | |
| } |
This uses the same native resolution that safeJoin relies on, making the guard's semantics consistent with the actual deletion path.
🎭 Playwright Test Results❌ Some tests failed
Summary: 237 passed, 2 failed, 7 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/select_component.spec.ts \
e2e-tests/template-create-nextjs.spec.ts
|
Removed section about removing labels with special characters due to security restrictions on DELETE calls.
|
@BugBot run |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if ( | ||
| normalizedPath === "." || | ||
| normalizedPath === "./" || | ||
| normalizedPath === "" | ||
| ) { | ||
| throw new Error( | ||
| `Refusing to delete project root for path: "${args.path}"`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Dead conditions after path.posix.normalize
path.posix.normalize never returns "./" or "" — it always collapses those to ".". This means the second and third conditions in the guard are unreachable dead code:
normalizedPath === "./"—normalize("./")returns".", not"./".normalizedPath === ""—normalize("")also returns".", not"".
The first condition (=== ".") is the only one that actually fires, and it already covers both of those inputs. The tests pass because they're all caught by the "." branch before reaching the dead branches.
| if ( | |
| normalizedPath === "." || | |
| normalizedPath === "./" || | |
| normalizedPath === "" | |
| ) { | |
| throw new Error( | |
| `Refusing to delete project root for path: "${args.path}"`, | |
| ); | |
| } | |
| if (normalizedPath === ".") { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/pro/main/ipc/handlers/local_agent/tools/delete_file.ts
Line: 48-56
Comment:
**Dead conditions after `path.posix.normalize`**
`path.posix.normalize` never returns `"./"` or `""` — it always collapses those to `"."`. This means the second and third conditions in the guard are unreachable dead code:
- `normalizedPath === "./"` — `normalize("./")` returns `"."`, not `"./"`.
- `normalizedPath === ""` — `normalize("")` also returns `"."`, not `""`.
The first condition (`=== "."`) is the only one that actually fires, and it already covers both of those inputs. The tests pass because they're all caught by the `"."` branch before reaching the dead branches.
```suggestion
if (normalizedPath === ".") {
```
How can I resolve this? If you propose a fix, please make it concise.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (2 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
|
|
||
| expect(fs.existsSync).not.toHaveBeenCalled(); | ||
| expect(fs.unlinkSync).not.toHaveBeenCalled(); | ||
| expect(fs.rmdirSync).not.toHaveBeenCalled(); | ||
| expect(gitRemove).not.toHaveBeenCalled(); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🟡 MEDIUM | test-coverage
Test suite missing .. (bare parent traversal) case
The it.each array covers ".", "./", ".\\", "foo/..", and "foo\\.." but omits a bare "..". Since path.posix.normalize("..") returns ".." (not "."), this input actually bypasses the root-path guard in the implementation — a gap already flagged by another reviewer on the implementation side.
Adding ".." to this test list would make the gap visible in CI: the test would fail, documenting that the guard needs to handle parent-traversal paths too.
💡 Suggestion: Add ".." to the it.each array so the test serves as an executable specification of the guard's contract.
1 participant
Summary
delete_filepaths at schema level..,./,.\\, or normalizedfoo/..).Test plan
npm test -- src/pro/main/ipc/handlers/local_agent/tools/delete_file.spec.tsnpm run fmt && npm run lint:fix && npm run tsMade with Cursor