Skip to content

fix(blueprint): chown copied files to sandbox user after restore (#1229) #1667

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.

Sign up for GitHub

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

Merged
ericksoa merged 1 commit into from
Apr 15, 2026
Merged

fix(blueprint): chown copied files to sandbox user after restore (#1229) #1667

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion nemoclaw/src/blueprint/snapshot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,36 @@ export async function restoreIntoSandbox(
["sandbox", "cp", source, `${sandboxName}:/sandbox/.openclaw`],
{ reject: false },
);
return result.exitCode === 0;
if (result.exitCode !== 0) {
return false;
}

// Files copied via `openshell sandbox cp` land as root:root because
// the helper runs as root inside the pod. /sandbox/.openclaw is the
// immutable gateway-config layer, but the symlinks under it point at
// /sandbox/.openclaw-data (the writable side), so the copied agent
// workspace and per-agent runtime dirs end up unwritable by the
// sandbox user. That broke writes to models.json, agent state, and
// workspace markdown files. Fix it with a best-effort recursive
// chown after the cp succeeds. We deliberately keep this best-effort
// (don't fail the restore if the chown fails) so a future runtime
// that already gets ownership right doesn't trip on a missing
// chown binary or a tightened exec policy. See #1229.
await execa(
"openshell",
[
"sandbox",
"exec",
sandboxName,
"--",
"chown",
"-R",
"sandbox:sandbox",
"/sandbox/.openclaw-data",
],
{ reject: false },
);
Comment on lines +110 to +123
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add regression tests for the new best-effort chown branch.

Line 110-Line 123 introduces a security-sensitive ownership correction, but there’s no co-located test asserting (1) the second execa call is made and (2) a failing chown still returns true.

Suggested test additions 
diff --git a/nemoclaw/src/blueprint/snapshot.test.ts b/nemoclaw/src/blueprint/snapshot.test.ts
@@
   it("calls openshell sandbox cp and returns true on success", async () => {
     addDir(`${SNAP}/openclaw`);
     mockExeca.mockResolvedValue({ exitCode: 0 });

     expect(await restoreIntoSandbox(SNAP, "mybox")).toBe(true);
@@
   });

+  it("runs best-effort chown after successful copy", async () => {
+    addDir(`${SNAP}/openclaw`);
+    mockExeca
+      .mockResolvedValueOnce({ exitCode: 0 }) // cp
+      .mockResolvedValueOnce({ exitCode: 1 }); // chown (ignored)
+
+    expect(await restoreIntoSandbox(SNAP, "mybox")).toBe(true);
+    expect(mockExeca).toHaveBeenNthCalledWith(
+      2,
+      "openshell",
+      [
+        "sandbox",
+        "exec",
+        "mybox",
+        "--",
+        "chown",
+        "-R",
+        "sandbox:sandbox",
+        "/sandbox/.openclaw-data",
+      ],
+      { reject: false },
+    );
+  });

As per coding guidelines {nemoclaw/src/blueprint,bin/lib}/**/*.{js,ts}: Security-sensitive code paths in isolation/sandbox features must have extra test coverage to prevent credential leaks and sandbox escapes.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await execa(
"openshell",
[
"sandbox",
"exec",
sandboxName,
"--",
"chown",
"-R",
"sandbox:sandbox",
"/sandbox/.openclaw-data",
],
{ reject: false },
);
it("calls openshell sandbox cp and returns true on success", async () => {
addDir(`${SNAP}/openclaw`);
mockExeca.mockResolvedValue({ exitCode: 0 });
expect(await restoreIntoSandbox(SNAP, "mybox")).toBe(true);
});
it("runs best-effort chown after successful copy", async () => {
addDir(`${SNAP}/openclaw`);
mockExeca
.mockResolvedValueOnce({ exitCode: 0 }) // cp
.mockResolvedValueOnce({ exitCode: 1 }); // chown (ignored)
expect(await restoreIntoSandbox(SNAP, "mybox")).toBe(true);
expect(mockExeca).toHaveBeenNthCalledWith(
2,
"openshell",
[
"sandbox",
"exec",
"mybox",
"--",
"chown",
"-R",
"sandbox:sandbox",
"/sandbox/.openclaw-data",
],
{ reject: false },
);
});
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@nemoclaw/src/blueprint/snapshot.ts` around lines 110 - 123, Add regression
tests that cover the new best-effort chown branch: in tests for the code that
calls execa("openshell", ["sandbox","exec", sandboxName, "--", "chown", "-R",
"sandbox:sandbox", "/sandbox/.openclaw-data"]), assert that the second execa
invocation is executed (mock/stub execa and verify it was called with the exact
args including sandboxName and "/sandbox/.openclaw-data") and assert that when
that chown invocation fails (simulate non-zero exit via reject:false behavior or
a thrown error) the surrounding function still returns true; add tests
co-located with snapshot.ts (and follow the project test pattern for
{nemoclaw/src/blueprint,bin/lib}/*) so the security-sensitive ownership
correction path is covered.

return true;
}

export function cutoverHost(): boolean {
Expand Down