Skip to content

fix(sandbox): use custom timeoutSeconds from SandboxConfig#3198

Open
jlaportebot wants to merge 1 commit into
microsoft:mainfrom
jlaportebot:fix/timeout-seconds-sandbox
Open

fix(sandbox): use custom timeoutSeconds from SandboxConfig#3198
jlaportebot wants to merge 1 commit into
microsoft:mainfrom
jlaportebot:fix/timeout-seconds-sandbox

Conversation

@jlaportebot

Copy link
Copy Markdown
Contributor

Summary

Fixes issue #3118: TypeScript Docker sandbox ignores custom timeoutSeconds.

Changes

  • Store SandboxConfig per session in using a new Map
  • Use in instead of hardcoded 60_000ms
  • Add to ensure timeout cannot be bypassed by sandboxed code catching SIGTERM
  • Add as a hard backstop for cleanup
  • Validate to prevent invalid timeouts
  • Handle both exit code 124 (SIGTERM timeout) and 137 (SIGKILL) as timeout
  • Clean up configs Map in

Test Plan

  1. Create sandbox session with
  2. Execute code that sleeps for 5 seconds
  3. Verify execution is killed at ~2 seconds (not 60 seconds)
  4. Verify result has and

This is a security fix — SIGTERM can be caught/ignored by sandboxed code via , but SIGKILL cannot be caught.

- Store SandboxConfig per session in createSession()
- Use timeoutSeconds in executeCode() with SIGKILL enforcement
- Add --kill-after=5s backstop and validate timeoutSeconds >= 1
- Handle both SIGTERM (124) and SIGKILL (137) exit codes as timeout
- Clean up configs map in destroySession()
@github-actions

Copy link
Copy Markdown
🤖 AI Agent: security-scanner — Security Review

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Security Review

Severity Finding Fix
High Sandbox escape risk: The use of execFile with user-provided code and arguments could allow an attacker to execute arbitrary commands if input validation is insufficient. Validate and sanitize all user inputs thoroughly before passing them to execFile. Consider using a more restrictive execution mechanism.
High Trust chain weakness: The timeout command is used to enforce execution limits, but its behavior depends on the host environment and could be overridden or manipulated. Use a more robust mechanism for enforcing timeouts, such as directly managing process lifetimes within the application.
Medium Potential race condition: The cleanup of configs and containers maps in the finally block may not handle concurrent access properly. Implement synchronization mechanisms to ensure thread-safe access to shared resources.
Medium Policy engine circumvention: The timeoutSeconds value from SandboxConfig is used directly without sufficient validation, which could allow a malicious user to set excessively high or low values. Add stricter validation for timeoutSeconds to enforce reasonable bounds.

@github-actions github-actions Bot added the size/S Small PR (< 50 lines) label Jun 27, 2026
@github-actions

Copy link
Copy Markdown
🤖 AI Agent: breaking-change-detector — API Compatibility

AI-generated review output. Treat it as untrusted analysis and verify before acting.

API Compatibility

Severity Change Impact
High Added a new configs Map to the DockerSandboxProvider class to store SandboxConfig per session. Existing consumers of DockerSandboxProvider that rely on its internal state may encounter issues if they are not updated to account for the new configs map.
High Changed the execFile function call to use a dynamic timeout value (timeoutMs) derived from SandboxConfig instead of a hardcoded value. Existing consumers relying on the previous hardcoded timeout behavior may experience unexpected changes in execution behavior.
High Modified the killReason property in the SandboxResult object to include a new timedOut condition and updated its logic. Consumers parsing or relying on the killReason property may need to update their logic to handle the new timedOut condition.
High Added cleanup of the configs map in the destroySession method. Existing consumers relying on the configs map persisting beyond a session's lifecycle may encounter issues.

@github-actions

Copy link
Copy Markdown
🤖 AI Agent: docs-sync-checker — Docs Sync

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Docs Sync

  • DockerSandboxProvider in agent-governance-typescript/src/sandbox.ts -- missing docstring for the new configs property.
  • README.md -- no updates found related to the new timeoutSeconds feature.
  • CHANGELOG -- missing entry for the behavioral changes introduced by the custom timeoutSeconds implementation.

@github-actions

Copy link
Copy Markdown
🤖 AI Agent: test-generator — `agent-governance-typescript/src/sandbox.ts`

AI-generated review output. Treat it as untrusted analysis and verify before acting.

agent-governance-typescript/src/sandbox.ts

  • test_custom_timeout_applied -- Verify that the custom timeoutSeconds from SandboxConfig is correctly applied during code execution.
  • test_invalid_timeout_handling -- Validate that invalid timeoutSeconds values in SandboxConfig are properly handled and do not cause unexpected behavior.
  • test_sigkill_handling -- Ensure that the sandbox correctly handles SIGKILL signals and marks the execution as killed with the appropriate reason.
  • test_config_cleanup -- Confirm that the configs Map is properly cleaned up after a session ends.

@github-actions

Copy link
Copy Markdown
🤖 AI Agent: code-reviewer — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

TL;DR: 1 blocker, 1 warning. The PR addresses a critical security issue but has a potential validation gap and a minor improvement opportunity.

# Sev Issue Where
1 Blocker timeoutSeconds validation is insufficient to prevent invalid values. sandbox.ts
2 Warning Hardcoded --kill-after=5s may not align with user-configured timeouts. sandbox.ts

Action items:

  1. Ensure timeoutSeconds is validated to be a positive integer and within a reasonable range to prevent potential misuse or unexpected behavior.

Warnings (fine as follow-up PRs):

# Issue Where
1 Consider making --kill-after=5s configurable to align with timeoutSeconds. sandbox.ts

@github-actions

Copy link
Copy Markdown

PR Review Summary

Check Status Details
🔍 Code Review ⚠️ Missing No current-run comment
🛡️ Security Scan ⚠️ Missing No current-run comment
🔄 Breaking Changes ⚠️ Missing No current-run comment
📝 Docs Sync ⚠️ Missing No current-run comment
🧪 Test Coverage ⚠️ Missing No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

@github-actions

Copy link
Copy Markdown

🔴 Contributor Check: HIGH

Check Result
Profile HIGH
Credential LOW
Overall HIGH

Automated check by AGT Contributor Check.

@liamcrumm

Copy link
Copy Markdown
Contributor

Please fix failing checks, also would prefer if you club #3198, #3201, and #3203 into one PR if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review:HIGH Contributor reputation check flagged HIGH risk size/S Small PR (< 50 lines)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants