-
-
Notifications
You must be signed in to change notification settings - Fork 986
fix(security): disable filesystem edits by default for clink CLI #418
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
14f0bd7
60da719
fa5dd9b
71bbd97
7c10f1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,19 +6,36 @@ | |
| from clink.parsers.base import ParserError | ||
|
|
||
| from .base import AgentOutput, BaseCLIAgent | ||
|
|
||
| from collections.abc import Sequence | ||
|
|
||
| class ClaudeAgent(BaseCLIAgent): | ||
| """Claude CLI agent with system-prompt injection support.""" | ||
|
|
||
| def _build_command(self, *, role: ResolvedCLIRole, system_prompt: str | None) -> list[str]: | ||
| def _build_command( | ||
| self, | ||
| *, | ||
| role: ResolvedCLIRole, | ||
| system_prompt: str | None, | ||
| allow_edits: bool = False, | ||
| editable_paths: Sequence[str] = (), | ||
| ) -> list[str]: | ||
| command = list(self.client.executable) | ||
| command.extend(self.client.internal_args) | ||
| command.extend(self.client.config_args) | ||
|
|
||
| if system_prompt and "--append-system-prompt" not in self.client.config_args: | ||
| config_args = self._sanitize_permission_args( | ||
| self.client.config_args, | ||
| allow_edits=allow_edits, | ||
| ) | ||
| command.extend(config_args) | ||
|
|
||
| if system_prompt and "--append-system-prompt" not in config_args: | ||
| command.extend(["--append-system-prompt", system_prompt]) | ||
|
|
||
| if allow_edits and editable_paths: | ||
| for path in editable_paths: | ||
| command.extend(["--allowedTools", f"Edit({path})"]) | ||
| command.extend(["--allowedTools", f"Write({path})"]) | ||
|
|
||
| command.extend(role.role_args) | ||
| return command | ||
|
|
||
|
|
@@ -47,3 +64,33 @@ def _recover_from_error( | |
| parser_name=self._parser.name, | ||
| output_file_content=output_file_content, | ||
| ) | ||
|
|
||
| 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]) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation of I suggest refactoring this method to use an iterator. This approach is more Pythonic and correctly handles all cases, including a dangling 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 |
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -50,6 +50,14 @@ class CLinkRequest(BaseModel): | |||||
| default=None, | ||||||
| description=COMMON_FIELD_DESCRIPTIONS["continuation_id"], | ||||||
| ) | ||||||
| allow_edits: bool = Field( | ||||||
| default=False, | ||||||
| description="Explicitly allow filesystem edits by the CLI. Disabled by default for security.", | ||||||
| ) | ||||||
| editable_paths: list[str] = Field( | ||||||
| default_factory=list, | ||||||
| description="Optional allow-list of absolute paths that may be edited when allow_edits=true.", | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| class CLinkTool(SimpleTool): | ||||||
|
|
@@ -140,6 +148,15 @@ def get_input_schema(self) -> dict[str, Any]: | |||||
| "enum": self._all_roles or ["default"], | ||||||
| "description": role_description, | ||||||
| }, | ||||||
| "allow_edits": { | ||||||
| "type": "boolean", | ||||||
| "description": "Allow filesystem edits by the CLI. Defaults to false.", | ||||||
| }, | ||||||
| "editable_paths": { | ||||||
| "type": "array", | ||||||
| "items": {"type": "string"}, | ||||||
| "description": "Absolute paths allowed for editing when allow_edits=true.", | ||||||
| }, | ||||||
| "absolute_file_paths": SchemaBuilder.SIMPLE_FIELD_SCHEMAS["absolute_file_paths"], | ||||||
| "images": SchemaBuilder.COMMON_FIELD_SCHEMAS["images"], | ||||||
| "continuation_id": SchemaBuilder.COMMON_FIELD_SCHEMAS["continuation_id"], | ||||||
|
|
@@ -165,6 +182,13 @@ async def execute(self, arguments: dict[str, Any]) -> list[TextContent]: | |||||
| self._current_arguments = arguments | ||||||
| request = self.get_request_model()(**arguments) | ||||||
|
|
||||||
| if request.editable_paths and not request.allow_edits: | ||||||
| self._raise_tool_error("editable_paths requires allow_edits=true.") | ||||||
|
|
||||||
| editable_path_error = self._validate_editable_paths(request) | ||||||
| if editable_path_error: | ||||||
| self._raise_tool_error(editable_path_error) | ||||||
|
|
||||||
| path_error = self._validate_file_paths(request) | ||||||
| if path_error: | ||||||
| self._raise_tool_error(path_error) | ||||||
|
|
@@ -211,6 +235,8 @@ async def execute(self, arguments: dict[str, Any]) -> list[TextContent]: | |||||
| system_prompt=system_prompt_text if system_prompt_text.strip() else None, | ||||||
| files=absolute_file_paths, | ||||||
| images=images, | ||||||
| allow_edits=request.allow_edits, | ||||||
| editable_paths=request.editable_paths, | ||||||
| ) | ||||||
| except CLIAgentError as exc: | ||||||
| metadata = self._build_error_metadata(client_config, exc) | ||||||
|
|
@@ -290,7 +316,12 @@ async def _prepare_prompt_for_role( | |||||
| if include_system_prompt and active_prompt: | ||||||
| sections.append(active_prompt) | ||||||
| sections.append(guidance) | ||||||
| sections.append("=== USER REQUEST ===\n" + user_content) | ||||||
| sections.append("=== UNTRUSTED USER REQUEST ===\n" + user_content) | ||||||
| if not request.allow_edits: | ||||||
| sections.append( | ||||||
| "=== EXECUTION POLICY ===\n" | ||||||
| "You must NOT perform any filesystem modifications or apply edits." | ||||||
| ) | ||||||
| if file_section: | ||||||
| sections.append("=== FILE REFERENCES ===\n" + file_section) | ||||||
| sections.append("Provide your response below using your own CLI tools as needed:") | ||||||
|
|
@@ -461,3 +492,15 @@ def _format_file_references(self, files: list[str]) -> str: | |||||
| except OSError: | ||||||
| references.append(f"- {file_path} (unavailable)") | ||||||
| return "\n".join(references) | ||||||
|
|
||||||
| def _validate_editable_paths(self, request: CLinkRequest) -> str | None: | ||||||
| for raw_path in request.editable_paths: | ||||||
| try: | ||||||
| path = Path(raw_path) | ||||||
| except Exception: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Catching a broad
Suggested change
|
||||||
| return f"Invalid editable path: {raw_path}" | ||||||
|
|
||||||
| if not path.is_absolute(): | ||||||
| return f"editable_paths must be absolute paths: {raw_path}" | ||||||
|
|
||||||
| return None | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new
allow_edits/editable_pathscontrols are not enforced inBaseCLIAgent:_build_commandstill unconditionally forwardsself.client.config_args. I checkedconf/cli_clients/gemini.jsonandconf/cli_clients/codex.json, which include--yoloand--dangerously-bypass-approvals-and-sandbox; those write-capable flags will still be active even whenallow_edits=false, so the “disabled by default” guarantee does not hold for these runners.Useful? React with 👍 / 👎.