fix(sandbox): allow cron tool in sandbox mode (gateway-routed, not containerized)#53565
fix(sandbox): allow cron tool in sandbox mode (gateway-routed, not containerized)#53565lupuletic wants to merge 1 commit intoopenclaw:mainfrom
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
Vulnerabilities1. 🟠 Sandbox default policy exposes admin-scoped Gateway cron RPC via allowed
|
| Property | Value |
|---|---|
| Severity | High |
| CWE | CWE-284 |
| Location | src/agents/sandbox/constants.ts:13-34 |
Description
The sandbox tool policy now allows the cron tool by default. Although it is described as “gateway-routed via WebSocket RPC”, this still exposes Gateway cron RPC methods to sandboxed agents.
Why this is a security issue:
- The
crontool callscallGatewayTool(...)(WebSocket RPC) for actions likecron.add,cron.update,cron.remove, andcron.run. - These Gateway methods are classified as admin-scope operations (
operator.admin). - As a result, a compromised/malicious agent operating in a sandbox session can potentially:
- create/update/remove cron jobs (persistence)
- trigger jobs immediately
- schedule payloads that inject
systemEventinto the main session or runagentTurnin named/persistent sessions (depending on cron payload/sessionTarget validation)
Even if the Docker container is not executing the job, the sandbox boundary is still crossed because privileged actions are delegated to the Gateway.
Vulnerable change (policy exposure):
export const DEFAULT_TOOL_ALLOW = [
// ...
"cron",
// ...
] as const;
export const DEFAULT_TOOL_DENY = ["browser", "canvas", "nodes", "gateway", ...CHANNEL_IDS] as const;And the cron tool routes to admin-scoped RPC methods:
return jsonResult(await callGateway("cron.add", gatewayOpts, job));If the sandbox environment has access to a Gateway token (via config/env/default credentials resolution), this becomes a practical escalation path from “sandbox agent” to “Gateway admin cron control”.
Recommendation
Treat cron as a privileged Gateway-facing tool and do not allow it by default in sandbox unless the sandbox is explicitly trusted.
Recommended options:
- Revert default allow and require explicit opt-in per sandbox policy:
// DEFAULT_TOOL_ALLOW: remove "cron"
// DEFAULT_TOOL_DENY: include "cron" by default- If
cronmust be available, enforce least privilege for sandbox usage by restricting cron RPC capabilities server-side (preferred):
- Introduce separate Gateway methods for “sandbox cron” that only allow safe operations (e.g., list/status only), or
- Enforce an authorization check in the Gateway cron handlers so that tokens/scopes used by sandbox agents cannot call
cron.add/update/remove/run.
- Add hard validation to prevent persistence/cross-session effects from sandbox-created jobs:
- require
sessionTarget="isolated"for sandbox-created jobs - forbid
session:<custom-id>andmaintargets from sandbox - enforce short TTL / auto-deleteAfterRun for sandbox jobs
This ensures sandbox agents cannot convert cron into a persistence or privilege-escalation mechanism.
Analyzed PR: #53565 at commit 837ee7f
Last updated on: 2026-03-29T14:15:36Z
Latest run failed. Keeping previous successful results. Trace ID: 019d3d6159efe01fb503d1bd2c316cae.
Last updated on: 2026-03-30T09:40:18Z
AI-Assisted PR Checklist
|
Greptile SummaryThis PR correctly removes
Note: The Confidence Score: 5/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/tool-policy.test.ts
Line: 229-247
Comment:
**Duplicate assertions across the two new tests**
The second test (`#50303`) re-asserts `isToolAllowed(policy, "cron") === true` and rebuilds the same `resolved`/`policy` from `resolveSandboxToolPolicyForAgent(undefined, undefined)` that the first test already covers. The only net-new assertion is the `gateway` check. Consider either merging the two tests or removing the duplicate `cron` assertion from the second test to keep the suite DRY.
```suggestion
it("default sandbox policy allows cron (gateway-routed, not containerized)", () => {
const resolved = resolveSandboxToolPolicyForAgent(undefined, undefined);
const policy: SandboxToolPolicy = { allow: resolved.allow, deny: resolved.deny };
expect(isToolAllowed(policy, "cron")).toBe(true);
expect(DEFAULT_TOOL_ALLOW).toContain("cron");
expect(DEFAULT_TOOL_DENY).not.toContain("cron");
// browser requires container execution — should remain denied
expect(isToolAllowed(policy, "browser")).toBe(false);
// gateway exposes admin actions — should remain denied
expect(isToolAllowed(policy, "gateway")).toBe(false);
});
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(sandbox): allow cron tool in sandbox..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
lupuletic
force-pushed
the
fix/sandbox-cron-allow-50303
branch
from
edbf832 to
05218ee
Compare
![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: 05218eecb6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
fabianwilliams
left a comment
fabianwilliams
left a comment
There was a problem hiding this comment.
The code change is clean and the UX fix makes sense — sandboxed agents hallucinating fake cron calls is clearly wrong. CI is green, tests cover the policy change, and the ownerOnly guard stays intact.
That said, I want to flag the security tradeoff that @aisle-research-bot raised, because I think it deserves a core maintainer opinion before merging.
The tension: Sandbox mode is defense-in-depth. Even though ownerOnly protects cron at the authorization layer, having it denied at the sandbox layer was a second barrier. Removing it means a sandboxed agent with owner context can now:
- Create persistent scheduled jobs that outlive the session (
cron.add) - Send webhook deliveries to arbitrary URLs (
delivery.mode="webhook")
The author's reasoning is sound — cron is gateway-routed, not containerized, so the sandbox deny list is the wrong enforcement point. But the security bot's suggestion of allowing read-only cron operations (cron.list, cron.status) while keeping mutations denied in sandbox could be a nice middle ground.
@BradGroux — would appreciate your take on whether the ownerOnly guard is sufficient here or if sandbox should maintain its own deny for cron mutations as defense-in-depth.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0baf5c973c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "edit", | ||
| "apply_patch", | ||
| "image", | ||
| "cron", |
There was a problem hiding this comment.
Adding cron to the default sandbox allowlist lets a sandboxed non-main chat schedule an agentTurn job with sessionTarget:"session:main", which then runs outside the sandbox boundary. The cron runner forwards custom session targets directly into isolated runs (src/gateway/server-cron.ts:288-302), and sandbox mode non-main explicitly skips sandboxing for the main session key (src/agents/sandbox/runtime-status.ts:15-23), so this commit introduces a containment bypass whenever sandbox.mode is non-main.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Review: fix(sandbox) allow cron tool in sandbox mode (gateway-routed, not containerized)
Commits reviewed: both commits on branch
Context
This PR moves cron from DEFAULT_TOOL_DENY to DEFAULT_TOOL_ALLOW in sandbox constants, with the rationale that cron is gateway-routed (WebSocket RPC to the host) and never executes inside the sandbox container. The Aisle security bot flagged this as CWE-269 (Improper Privilege Management), and @fabianwilliams raised the defense-in-depth question in comments.
Security analysis
There are two reasonable perspectives on this change, and they diverge in a way worth discussing explicitly.
The case for allowing cron:
ownerOnlyis the correct enforcement layer for cron. It validates that the requesting user is the owner before any mutation.- Cron never touches the sandbox container. It is a WebSocket RPC call to the gateway, which runs on the host. The sandbox deny-list was never a real containment boundary for cron; it was just preventing the tool from being called at all.
- Denying cron in sandbox causes agents to hallucinate fake cron calls or work around the restriction in unpredictable ways, which is arguably worse than letting the tool work as designed.
- Other gateway-routed tools (
sessions_*) are already allowed in sandbox without similar concern.
The case for keeping cron denied:
- A sandboxed agent running with owner context can invoke
cron.addwithpayload.kind="agentTurn", which triggersrunCronIsolatedAgentTurnon the HOST runtime. This is a sandbox-to-host execution path. ownerOnlyprotects against non-owner users but does not constrain a compromised or hallucinating agent that is already running as the owner. The deny-list provided a second barrier.- Cron is uniquely high-impact among gateway-routed tools due to persistence (jobs survive session end), scheduling (delayed execution), and webhook delivery capabilities.
My assessment:
The ownerOnly guard is the right enforcement layer. The sandbox deny-list was defense-in-depth, but cron is fundamentally a host-side tool that happens to be callable from sandbox sessions. Blocking it does not improve security in a meaningful way when the agent already has owner credentials. The UX degradation from blocking (hallucinated workarounds, confusion) is a real cost.
That said, the persistence concern is valid. A compromised agent creating persistent cron jobs is a different threat profile than a one-shot tool call that ends with the session.
Suggestions
1. Consider read-only vs mutation split (Medium)
If there is appetite for a middle ground: allow cron.status, cron.list, and cron.runs in sandbox while blocking cron.add, cron.update, cron.remove, and cron.run. This gives agents visibility into scheduled work without granting mutation capability. However, this requires tool-level action filtering that may not exist in the current sandbox infrastructure, making it a larger change than this PR intends.
2. Prompt note is good (Low)
The added prompt note clarifying that cron is gateway-routed and subject to ownerOnly checks is a useful addition. Agents benefit from understanding why the tool works the way it does.
Test coverage
- ✅ Tests verify
cronis inDEFAULT_TOOL_ALLOWand not inDEFAULT_TOOL_DENY ⚠️ No integration test for sandbox session invokingcron.addwith ownership validation⚠️ No test for mutation-vs-read-only behavior distinction
Summary
This is a reasonable change. The cron tool is gateway-routed and was never actually contained by the sandbox. The ownerOnly guard is the real security boundary. The PR is small, clean, and the test updates match the intent. The defense-in-depth concern is worth noting but does not rise to blocker level given how cron routing actually works.
FYI - @fabianwilliams
|
@BradGroux Thanks for the thorough analysis — this is exactly the kind of assessment I was hoping for when I tagged you. I agree with your conclusion: Your read-only vs mutation split suggestion is interesting — No objections from me on merging as-is. The persistence concern is noted and worth revisiting if sandbox threat modeling evolves. |
|
The sandbox escape concern via sessionTarget has been evaluated by maintainers (see fabianwilliams and BradGroux discussion). The ownerOnly tool-level gate is the enforcement boundary, not the sandbox deny-list. Cron is gateway-routed (WebSocket RPC), never containerized, so the deny-list was incorrectly blocking it. |
lupuletic
force-pushed
the
fix/sandbox-cron-allow-50303
branch
from
0baf5c9 to
837ee7f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 837ee7f96e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…bSocket RPC, not containerized (openclaw#50303)
lupuletic
force-pushed
the
fix/sandbox-cron-allow-50303
branch
from
837ee7f to
edf7507
Compare
3 participants
The cron tool is in DEFAULT_TOOL_DENY for sandbox mode, so when sandbox.mode='all' it is stripped from the agent's tool list in conversation — causing the model to hallucinate fake tool calls instead of invoking the real cron tool, which is gateway-routed via WebSocket RPC and doesn't need sandbox containment.
Closes #50303
Changes:
Testing:
AI-assisted (Claude + Codex committee consensus, fully tested).