fix(security): disable filesystem edits by default for clink CLI#418
fix(security): disable filesystem edits by default for clink CLI#418lilmingwa13 wants to merge 5 commits intoBeehiveInnovations:mainfrom
Conversation
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 significantly enhances the security of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 granular control over filesystem editing permissions for CLI agents, specifically for the Claude agent. It adds allow_edits and editable_paths parameters to the agent's run method and the CLinkRequest model, enabling explicit control over which paths an agent can modify. The changes include updating method signatures, implementing validation for editable paths (ensuring they are absolute and allow_edits is enabled), and modifying the command construction logic to pass these permissions to the underlying CLI. The default permission mode for the Claude client is also updated to 'default' (no edits). Additionally, the prompt now includes an explicit execution policy regarding filesystem modifications. Feedback indicates a bug in the _sanitize_permission_args method in clink/agents/claude.py where --permission-mode might be malformed if it's the last argument, and suggests simplifying its loop logic. There's also a recommendation to narrow the except Exception clause in _validate_editable_paths in tools/clink.py to more specific exceptions like TypeError or ValueError for improved error handling.
| def _sanitize_permission_args(self, args: list[str], *, allow_edits: bool) -> list[str]: | ||
| sanitized: list[str] = [] | ||
| i = 0 | ||
|
|
||
| while i < len(args): | ||
| arg = args[i] | ||
|
|
||
| if arg == "--permission-mode": | ||
| if i + 1 < len(args): | ||
| mode = args[i + 1] | ||
|
|
||
| if allow_edits: | ||
| sanitized.extend([arg, mode]) | ||
| else: | ||
| sanitized.extend([arg, "default"]) | ||
|
|
||
| i += 2 | ||
| continue | ||
|
|
||
| sanitized.append(arg) | ||
| i += 1 | ||
|
|
||
| if "--permission-mode" not in sanitized: | ||
| sanitized.extend([ | ||
| "--permission-mode", | ||
| "acceptEdits" if allow_edits else "default", | ||
| ]) | ||
|
|
||
| return sanitized No newline at end of file |
There was a problem hiding this comment.
The current implementation of _sanitize_permission_args has a bug when "--permission-mode" is the last argument in the list. It will append the flag without its value, leading to a malformed command. Additionally, the while loop is complex and can be simplified using an iterator, which makes the logic easier to follow and less prone to off-by-one errors.
I suggest refactoring this method to use an iterator. This approach is more Pythonic and correctly handles all cases, including a dangling "--permission-mode" flag.
def _sanitize_permission_args(self, args: list[str], *, allow_edits: bool) -> list[str]:
sanitized: list[str] = []
found_perm_mode = False
args_iter = iter(args)
for arg in args_iter:
if arg == "--permission-mode":
found_perm_mode = True
sanitized.append(arg)
try:
mode = next(args_iter)
if allow_edits:
sanitized.append(mode)
else:
sanitized.append("default")
except StopIteration:
# Dangling flag, append a default value.
sanitized.append("acceptEdits" if allow_edits else "default")
else:
sanitized.append(arg)
if not found_perm_mode:
sanitized.extend([
"--permission-mode",
"acceptEdits" if allow_edits else "default",
])
return sanitized| for raw_path in request.editable_paths: | ||
| try: | ||
| path = Path(raw_path) | ||
| except Exception: |
There was a problem hiding this comment.
Catching a broad Exception can hide unexpected errors. It's better to catch more specific exceptions. pathlib.Path can raise TypeError if the input is not a string or bytes, or ValueError for certain invalid paths (e.g., containing null characters). Catching these specific exceptions makes the error handling more precise.
| except Exception: | |
| except (TypeError, ValueError): |
![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: 60da719c6f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if allow_edits: | ||
| sanitized.extend([arg, mode]) |
There was a problem hiding this comment.
When allow_edits=true, _sanitize_permission_args keeps whatever --permission-mode value is already configured instead of switching to an edit-capable mode. Because this commit also changed conf/cli_clients/claude.json to --permission-mode default, an opt-in edit request still runs Claude in default mode, so write workflows that explicitly set allow_edits will not get the intended behavior. Override existing --permission-mode to acceptEdits whenever allow_edits is true.
Useful? React with 👍 / 👎.
| allow_edits: bool = False, | ||
| editable_paths: Sequence[str] = (), |
There was a problem hiding this comment.
The new allow_edits/editable_paths controls are not enforced in BaseCLIAgent: _build_command still unconditionally forwards self.client.config_args. I checked conf/cli_clients/gemini.json and conf/cli_clients/codex.json, which include --yolo and --dangerously-bypass-approvals-and-sandbox; those write-capable flags will still be active even when allow_edits=false, so the “disabled by default” guarantee does not hold for these runners.
Useful? React with 👍 / 👎.
lilmingwa13
changed the title
1 participant
Summary
This PR improves the security posture of the
clinktool by enforcing safe-by-default behavior when executing external AI CLIs.Previously, untrusted prompt input could be forwarded directly to a CLI configured with automatic file edit permissions (e.g.
--permission-mode acceptEdits), allowing unintended filesystem modifications.Changes
allow_edits) for write-capable CLI executioneditable_paths)Security Impact
This change prevents untrusted remote input from triggering filesystem modifications without explicit authorization, enforcing a clear trust boundary between:
Backward Compatibility
allow_edits=trueRelated Issue
Fixes #417
Disclosure
This issue has been submitted to MITRE for CVE consideration.
Full PoC and reproduction steps can be shared privately upon request.