Write mode-constrained inbound files directly to their target (#12320)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Adapter utilities transfer files between the host and an agent environment > - A sandbox target already provides the security boundary for inbound files > - The generic fallback adds a temporary file and a rename that do not add protection inside that boundary > - This pull request writes a mode-constrained inbound file directly to its target and applies the mode after the write > - The benefit is a simpler transfer path while host targets keep the strict pre-write mode rule ## Linked Issues or Issue Description **What existing behavior does this improve?** The inbound file-sync fallback for a mode-constrained file stages the file under a temporary name, applies the mode, and renames the file into place. **Subsystem affected** `packages/adapter-utils` and `packages/plugins`. **Current behavior** A sandbox target uses a temporary path before it receives the file. The host then changes the mode and renames the file to the target path. **Proposed behavior** A sandbox target receives the file at its target path. The host applies the mode after the write. A host target still applies the mode before the first byte. **Reason and benefit** The sandbox boundary already protects the target. The direct write removes an unnecessary staging path and rename. **Breaking changes** None. The directory path and outbound transfer path keep their existing behavior. ## What Changed - Write a mode-constrained single-file inbound transfer directly to the sandbox target. - Apply the mode after the direct write and keep the confinement check before post-upload commands. - Scope the protocol comment by transfer direction and preserve the strict host-target rule. - Keep directory inbound transfers and outbound transfers unchanged. ## Verification - Run the targeted unit suite for the changed package. - Verify the suite covers direct target writes, post-write mode application, and confinement rejection. - Run `tsc --noEmit` for both changed packages. - Review the full GitHub Actions check set after the PR opens. ## Risks - A sandbox provider that assumes a temporary inbound path could expose a behavior mismatch. - The confinement check remains before post-upload commands, which limits escape risk. - Host targets keep the pre-write mode rule, so host permission behavior does not change. ## Model Used OpenAI GPT-5. This model assisted with Git operations, PR preparation, review coordination, and tool use. Context window size and reasoning mode are not exposed by the runtime. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Nicky Leach
GitHub
parent
67f9867bc6
commit
b06034d762
|
|
@ -748,7 +748,7 @@ describe("command managed runtime", () => {
|
|||
expect(execTimeouts).toEqual([runTimeoutMs, syncClientTimeoutMs]);
|
||||
});
|
||||
|
||||
it("fallback syncIn stages mode-constrained files before chmod and rename", async () => {
|
||||
it("fallback syncIn writes a mode-constrained file directly to its target and then applies the mode", async () => {
|
||||
const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-syncin-mode-"));
|
||||
cleanupDirs.push(rootDir);
|
||||
const sourceFile = path.join(rootDir, "source.txt");
|
||||
|
|
@ -766,67 +766,19 @@ describe("command managed runtime", () => {
|
|||
]);
|
||||
|
||||
expect(await readFile(targetFile, "utf8")).toBe("payload\n");
|
||||
// The write goes straight to the target path. No staging name and no
|
||||
// rename step exist between the write and the chmod.
|
||||
const scripts = calls.map((call) => (call.args ?? []).join(" "));
|
||||
expect(scripts).toHaveLength(5);
|
||||
expect(scripts[0]).toContain(targetFile + ".paperclip-syncin.");
|
||||
expect(scripts[0]).toContain(".paperclip-upload.");
|
||||
expect(scripts[1]).toContain("rm -rf");
|
||||
expect(scripts[1]).toContain(".paperclip-upload.");
|
||||
expect(scripts[2]).toContain("chmod 640");
|
||||
expect(scripts[2]).toContain(targetFile + ".paperclip-syncin.");
|
||||
expect(scripts[3]).toContain("mv -f");
|
||||
expect(scripts[3]).toContain(targetFile + ".paperclip-syncin.");
|
||||
expect(scripts[3]).toContain(targetFile);
|
||||
expect(scripts[4]).toContain("rm -rf");
|
||||
expect(scripts[4]).toContain(targetFile + ".paperclip-syncin.");
|
||||
expect(scripts.some((script) => script.includes(".paperclip-syncin."))).toBe(false);
|
||||
expect(scripts.some((script) => script.includes("mv -f") && script.includes(".paperclip-syncin."))).toBe(
|
||||
false,
|
||||
);
|
||||
const chmodScript = scripts.find((script) => script.includes("chmod 640"));
|
||||
expect(chmodScript).toBeDefined();
|
||||
expect(chmodScript).toContain(targetFile);
|
||||
});
|
||||
|
||||
it("fallback syncIn cleans up a staged file when chmod fails before rename", async () => {
|
||||
const rootDir = await mkdtemp(path.join(os.tmpdir(), "paperclip-syncin-cleanup-"));
|
||||
cleanupDirs.push(rootDir);
|
||||
const sourceFile = path.join(rootDir, "source.txt");
|
||||
const targetFile = path.join(rootDir, "target.txt");
|
||||
await writeFile(sourceFile, "payload\n", "utf8");
|
||||
|
||||
const { runner, calls } = makeSpawnRunner({ supportsSingleStreamStdinProgress: true });
|
||||
const delegatedExecute = runner.execute.bind(runner);
|
||||
runner.execute = async (input) => {
|
||||
const script = (input.args ?? []).join(" ");
|
||||
if (script.includes("chmod 600")) {
|
||||
calls.push({ command: input.command, args: input.args, cwd: input.cwd, stdin: input.stdin });
|
||||
return {
|
||||
exitCode: 1,
|
||||
signal: null,
|
||||
timedOut: false,
|
||||
stdout: "",
|
||||
stderr: "chmod failed",
|
||||
pid: null,
|
||||
startedAt: new Date().toISOString(),
|
||||
};
|
||||
}
|
||||
return await delegatedExecute(input);
|
||||
};
|
||||
const client = createCommandManagedRuntimeClient({ runner, commandCwd: "/", timeoutMs: 30_000 });
|
||||
|
||||
await expect(
|
||||
client.syncIn!([
|
||||
{
|
||||
operationId: "op-cleanup",
|
||||
files: [{ sourcePath: sourceFile, targetPath: targetFile, kind: "file", mode: 0o600 }],
|
||||
},
|
||||
]),
|
||||
).rejects.toThrow(/chmod failed/);
|
||||
|
||||
const chmodCall = calls.find((call) => (call.args ?? []).join(" ").includes("chmod 600"));
|
||||
expect(chmodCall).toBeDefined();
|
||||
const stagedPath = (chmodCall?.args ?? []).join(" ").match(/chmod 600 '([^']+)'/)?.[1];
|
||||
expect(stagedPath).toBeDefined();
|
||||
await expect(readFile(stagedPath!, "utf8")).rejects.toMatchObject({ code: "ENOENT" });
|
||||
expect(calls.some((call) => (call.args ?? []).join(" ").includes(`rm -rf '${stagedPath}'`))).toBe(true);
|
||||
await expect(readFile(targetFile, "utf8")).rejects.toMatchObject({ code: "ENOENT" });
|
||||
});
|
||||
|
||||
it("test_post_upload_commands_execute_verbatim_not_rewritten (C1 opaque)", async () => {
|
||||
it("post-upload commands execute verbatim and are never rewritten", async () => {
|
||||
// The provider/client treats each command as opaque: it is executed VERBATIM,
|
||||
// never concatenated with asset keys / paths or otherwise rewritten.
|
||||
const executed: string[] = [];
|
||||
|
|
@ -850,7 +802,7 @@ describe("command managed runtime", () => {
|
|||
expect(executed).toContain(verbatim);
|
||||
});
|
||||
|
||||
it("test_post_upload_command_cwd_escaping_target_root_is_rejected (C2)", async () => {
|
||||
it("post-upload command cwd that escapes the target root is rejected", async () => {
|
||||
// A `cwd` that escapes the operation's target root — via `..` or an absolute
|
||||
// path outside the target — is rejected BEFORE any handoff (no execute).
|
||||
let executeCalls = 0;
|
||||
|
|
@ -904,7 +856,7 @@ describe("command managed runtime", () => {
|
|||
expect(executeCalls).toBe(0);
|
||||
});
|
||||
|
||||
it("test_fallback_syncIn_aborts_and_rejects_on_first_nonzero_exit (C4 fail-fast)", async () => {
|
||||
it("fallback syncIn aborts on the first non-zero exit", async () => {
|
||||
// The first non-zero post-upload command aborts the operation: syncIn rejects,
|
||||
// the remaining commands do NOT run, and there is no silent partial fallback.
|
||||
const executed: string[] = [];
|
||||
|
|
|
|||
|
|
@ -208,10 +208,6 @@ function buildSyncInExtractDirectoryCommand(input: { remoteTarPath: string; targ
|
|||
function buildSyncInChmodCommand(input: { mode: number; targetPath: string }): string {
|
||||
return `chmod ${(input.mode & 0o7777).toString(8)} ${shellQuote(input.targetPath)}`;
|
||||
}
|
||||
function buildSyncInRenameCommand(input: { sourcePath: string; targetPath: string }): string {
|
||||
return "mv -f " + shellQuote(input.sourcePath) + " " + shellQuote(input.targetPath);
|
||||
}
|
||||
|
||||
function buildUniqueStagingPath(input: { targetPath: string; suffix: string }): string {
|
||||
return `${input.targetPath}${input.suffix}.${randomUUID()}`;
|
||||
}
|
||||
|
|
@ -444,18 +440,10 @@ export function createCommandManagedRuntimeClient(input: {
|
|||
bytesTransferred += tarBytes.byteLength;
|
||||
} else {
|
||||
const fileBytes = await fs.readFile(mapping.sourcePath);
|
||||
const targetPathForWrite = mapping.mode != null
|
||||
? buildUniqueStagingPath({ targetPath: mapping.targetPath, suffix: ".paperclip-syncin" })
|
||||
: mapping.targetPath;
|
||||
if (mapping.mode != null) cleanupPaths.push(targetPathForWrite);
|
||||
await client.writeFile(targetPathForWrite, bufferToArrayBuffer(fileBytes));
|
||||
await client.writeFile(mapping.targetPath, bufferToArrayBuffer(fileBytes));
|
||||
if (mapping.mode != null) {
|
||||
await client.run(
|
||||
buildSyncInChmodCommand({ mode: mapping.mode, targetPath: targetPathForWrite }),
|
||||
{ timeoutMs: input.timeoutMs },
|
||||
);
|
||||
await client.run(
|
||||
buildSyncInRenameCommand({ sourcePath: targetPathForWrite, targetPath: mapping.targetPath }),
|
||||
buildSyncInChmodCommand({ mode: mapping.mode, targetPath: mapping.targetPath }),
|
||||
{ timeoutMs: input.timeoutMs },
|
||||
);
|
||||
}
|
||||
|
|
@ -468,9 +456,9 @@ export function createCommandManagedRuntimeClient(input: {
|
|||
}
|
||||
filesTransferred += 1;
|
||||
}
|
||||
// Ordered, fail-fast post-upload commands (C1 opaque / C4 fail-loud). Each
|
||||
// command string is executed VERBATIM — never rewritten, concatenated, or
|
||||
// appended to. First non-zero exit or timeout throws and stops the rest.
|
||||
// Ordered, fail-fast post-upload commands. Each command string is
|
||||
// executed VERBATIM — never rewritten, concatenated, or appended to.
|
||||
// The first non-zero exit or timeout throws and stops the rest.
|
||||
for (const command of operation.postUploadCommands ?? []) {
|
||||
const result = await input.runner.execute({
|
||||
command: shellCommand,
|
||||
|
|
|
|||
|
|
@ -725,8 +725,17 @@ export interface PluginSyncFileMapping {
|
|||
kind: "file" | "directory";
|
||||
/**
|
||||
* POSIX file mode to apply at the target (e.g. `0o600` for secret material).
|
||||
* When set, providers MUST create the target with this mode with no
|
||||
* world-readable window (create-with-mode or chmod-before-bytes, never after).
|
||||
* The target MUST carry this mode when the transfer completes.
|
||||
*
|
||||
* For a transfer to a host target, providers MUST apply the mode with no
|
||||
* world-readable window: create the target with the mode, or apply the mode
|
||||
* before the bytes arrive at the target path. A host file sits outside the
|
||||
* sandbox boundary, so an open window shows the bytes to other host
|
||||
* processes.
|
||||
*
|
||||
* For a transfer to a sandbox target, providers MAY apply the mode after
|
||||
* they write the bytes. The sandbox is the trust boundary, so a short window
|
||||
* shows the bytes only to code that already runs in that sandbox.
|
||||
*/
|
||||
mode?: number;
|
||||
/** Glob patterns to exclude when `kind` is `"directory"`. */
|
||||
|
|
|
|||
Loading…
Reference in New Issue