-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(sandbox): use base64+stdin for edit() to avoid bash -lc escaping … #3084
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
b00209d
78df852
45cb3f0
a051736
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 |
|---|---|---|
|
|
@@ -255,30 +255,41 @@ public EditResult edit( | |
| Base64.getEncoder() | ||
| .encodeToString(payload.getBytes(java.nio.charset.StandardCharsets.UTF_8)); | ||
|
|
||
| String cmd = | ||
| "python3 -c \"import sys, os, base64, json\\n" | ||
| + "payload =" | ||
| + " json.loads(base64.b64decode(sys.stdin.read().strip()).decode('utf-8'))\\n" | ||
| + "path, old, new = payload['path'], payload['old'], payload['new']\\n" | ||
| + "replace_all = payload.get('replace_all', False)\\n" | ||
| + "if not os.path.isfile(path):\\n" | ||
| + " print(json.dumps({'error': 'file_not_found'}))\\n" | ||
| + " sys.exit(0)\\n" | ||
| + "with open(path, 'rb') as f: text = f.read().decode('utf-8')\\n" | ||
| + "count = text.count(old)\\n" | ||
| + "if count == 0:\\n" | ||
| + " print(json.dumps({'error': 'string_not_found'}))\\n" | ||
| + " sys.exit(0)\\n" | ||
| + "if count > 1 and not replace_all:\\n" | ||
| + " print(json.dumps({'error': 'multiple_occurrences', 'count': count}))\\n" | ||
| + " sys.exit(0)\\n" | ||
| // Edit script is assembled with real newlines, then base64-encoded and piped via stdin | ||
| // to `python3 -`; the payload is passed through argv[1]. | ||
| // | ||
| // Do NOT revert to `python3 -c "...\n..."` inline form: under `bash -lc`, \n inside | ||
| // double quotes is a literal backslash+n (not a newline), so the entire script collapses | ||
| // into one line and Python fails with: | ||
| // SyntaxError: unexpected character after line continuation character | ||
| // This makes edit_file 100% non-functional in sandbox environments (the model falls back | ||
| // to write_file, which refuses to overwrite existing files, so no file can be modified). | ||
| // Using stdin + argv avoids all quoting/escaping issues. | ||
| String script = | ||
| "import sys, os, base64, json\n" | ||
| + "payload = json.loads(base64.b64decode(sys.argv[1]).decode('utf-8'))\n" | ||
| + "path, old, new = payload['path'], payload['old'], payload['new']\n" | ||
| + "replace_all = payload.get('replace_all', False)\n" | ||
| + "if not os.path.isfile(path):\n" | ||
| + " print(json.dumps({'error': 'file_not_found'}))\n" | ||
| + " sys.exit(0)\n" | ||
| + "with open(path, 'rb') as f: text = f.read().decode('utf-8')\n" | ||
| + "count = text.count(old)\n" | ||
| + "if count == 0:\n" | ||
| + " print(json.dumps({'error': 'string_not_found'}))\n" | ||
| + " sys.exit(0)\n" | ||
| + "if count > 1 and not replace_all:\n" | ||
| + " print(json.dumps({'error': 'multiple_occurrences', 'count': count}))\n" | ||
| + " sys.exit(0)\n" | ||
| + "result = text.replace(old, new) if replace_all else text.replace(old, new," | ||
| + " 1)\\n" | ||
| + "with open(path, 'wb') as f: f.write(result.encode('utf-8'))\\n" | ||
| + "print(json.dumps({'count': count}))\\n" | ||
| + "\" 2>&1 <<'__EDIT_EOF__'\n" | ||
| + payloadB64 | ||
| + "\n__EDIT_EOF__\n"; | ||
| + " 1)\n" | ||
| + "with open(path, 'wb') as f: f.write(result.encode('utf-8'))\n" | ||
| + "print(json.dumps({'count': count}))\n"; | ||
| String scriptB64 = | ||
| Base64.getEncoder() | ||
| .encodeToString(script.getBytes(java.nio.charset.StandardCharsets.UTF_8)); | ||
|
|
||
| String cmd = "echo " + scriptB64 + " | base64 -d | python3 - " + payloadB64 + " 2>&1"; | ||
|
Collaborator
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. [Warning] The fix moves the payload from stdin to
Suggested shape that keeps the newline fix and the previous safety properties: keep the base64 script in argv (it is constant-size and contains no user data) and put only the payload back on stdin, e.g. String cmd = "echo " + scriptB64 + " | base64 -d | python3 - 2>&1 <<'__EDIT_EOF__'\n"
+ payloadB64 + "\n__EDIT_EOF__\n";(with |
||
|
|
||
|
Collaborator
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 base64 alphabet is newline-free, so the quoted heredoc delimiter can never be terminated early by the payload — that part is safe. What is not safe is the error path: if the temp file ever goes missing or |
||
| ExecuteResponse result = execute(runtimeContext, cmd, null); | ||
| String output = result.output() != null ? result.output().strip() : ""; | ||
|
|
||
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.
[Info]
echois only guaranteed to pass a single long argument through verbatim by bash's builtin; someechoimplementations/versions wrap very long output, and a wrapped base64 stream decodes to a truncated script (manifesting as a PythonSyntaxError, i.e. the same symptom this PR is fixing). This repo already hit the mirror-image problem on the read path — seeSandboxBackedFilesystem#L209("MIME decoder tolerates wrapped base64 output from GNUbase64").printf '%s\n'avoids the ambiguity, and it would be worth extendingedit_usesBase64PipedScriptwith a large-payload case asserting the emitted command is a single line.