fix(sandbox): correct sandlock integration semantics and fail loud#1367
Conversation
📝 WalkthroughWalkthroughThe changes enhance the Sandlock sandbox implementation with runtime Landlock availability validation, extended filesystem allowlisting via Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the SandlockSandbox by enforcing Landlock support at instantiation and removing the silent fallback to SubprocessSandbox. Key improvements include refined network policy logic, better timeout detection using sandlock's exit code sentinels, and a more robust execute_file implementation that passes script paths directly to interpreters. Feedback focuses on tightening the filesystem security policy: specifically, ensuring that the sandbox's temporary and working directories are explicitly granted read access (as required by Landlock) and narrowing the scope of extra_readable paths to individual files rather than entire parent directories to prevent unintended data exposure.
| limits=limits, | ||
| env=env, | ||
| working_dir=self._temp_dir, | ||
| extra_readable=[os.path.dirname(abs_path)], |
There was a problem hiding this comment.
Allowlisting the entire parent directory of the script is overly permissive and could expose sensitive files on the host if the script is located outside the sandbox's temporary directory. It is safer to only allowlist the script file itself. Note that this requires updating _create_policy to support files in extra_readable (by using os.path.exists instead of os.path.isdir).
| extra_readable=[os.path.dirname(abs_path)], | |
| extra_readable=[abs_path], |
| allowed_read_paths = [p for p in _candidate_read_paths if os.path.isdir(p)] | ||
| if extra_readable: | ||
| allowed_read_paths.extend( | ||
| p for p in extra_readable if os.path.isdir(p) | ||
| ) |
There was a problem hiding this comment.
The current implementation of _create_policy has two issues regarding the read allowlist:
- It only allows directories in
extra_readabledue to theos.path.isdircheck. This prevents allowlisting individual files, which is necessary for theexecute_filemethod to work securely. - The sandbox's temporary directory (
self._temp_dir) and theworking_dirare added to the writable allowlist but not the readable one. In Landlock, write access does not implicitly grant read access. The sandboxed process will likely fail to read its own working directory or files written to the sandbox unless they are explicitly added tofs_readable.
| allowed_read_paths = [p for p in _candidate_read_paths if os.path.isdir(p)] | |
| if extra_readable: | |
| allowed_read_paths.extend( | |
| p for p in extra_readable if os.path.isdir(p) | |
| ) | |
| allowed_read_paths = [p for p in _candidate_read_paths if os.path.isdir(p)] | |
| if extra_readable: | |
| allowed_read_paths.extend(p for p in extra_readable if os.path.exists(p)) | |
| for p in [self._temp_dir, working_dir]: | |
| if p and os.path.isdir(p) and p not in allowed_read_paths: | |
| allowed_read_paths.append(p) |
Greptile SummaryThis PR corrects the
Confidence Score: 4/5Safe to merge with a small follow-up: the integration test exception guard is incomplete and can hard-fail in certain sandlock version/kernel combinations instead of skipping cleanly. The core implementation changes look correct and well-motivated. The one gap is in the integration test: the The exception handler at the bottom of Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| raise RuntimeError( | ||
| "SandlockSandbox requires Landlock support (Linux kernel " | ||
| ">= 6.12 with CONFIG_SECURITY_LANDLOCK=y). This kernel " | ||
| f"reports Landlock ABI version {abi}. Use SubprocessSandbox " | ||
| "explicitly if weaker isolation is acceptable." | ||
| ) |
There was a problem hiding this comment.
The error message states Linux kernel >= 6.12, but Landlock ABI version 1 was introduced in Linux 5.13 (not 6.12). The PR description itself mentions "running on a kernel <5.13" in the breaking-change section, so the discrepancy is likely a typo. Users on kernels between 5.13 and 6.12 who encounter this error will be told their kernel is too old when it is actually sufficient.
| raise RuntimeError( | |
| "SandlockSandbox requires Landlock support (Linux kernel " | |
| ">= 6.12 with CONFIG_SECURITY_LANDLOCK=y). This kernel " | |
| f"reports Landlock ABI version {abi}. Use SubprocessSandbox " | |
| "explicitly if weaker isolation is acceptable." | |
| ) | |
| if abi < 1: | |
| raise RuntimeError( | |
| "SandlockSandbox requires Landlock support (Linux kernel " | |
| ">= 5.13 with CONFIG_SECURITY_LANDLOCK=y). This kernel " | |
| f"reports Landlock ABI version {abi}. Use SubprocessSandbox " | |
| "explicitly if weaker isolation is acceptable." | |
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/praisonai/tests/unit/sandbox/test_sandlock_sandbox.py (1)
104-106: Mock stdout/stderr types don't match real sandlock behavior.The mock uses
strforstdout/stderr, but real sandlock returnsbytes. While_decode()handles both types, this test doesn't exercise the byte decoding path. For consistency with the timeout/failure tests (which useb""), consider using bytes here too.📝 Suggested fix
mock_result = Mock() mock_result.exit_code = 0 - mock_result.stdout = "Hello, World!" - mock_result.stderr = "" + mock_result.stdout = b"Hello, World!" + mock_result.stderr = b"" + mock_result.success = TrueNote: You'll also need to add
mock_result.success = Truesince the implementation checksresult.successfirst.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/tests/unit/sandbox/test_sandlock_sandbox.py` around lines 104 - 106, Update the test's mock_result to mirror real sandlock behavior by setting mock_result.stdout and mock_result.stderr to bytes (e.g., b"Hello, World!" and b"") instead of str, and also set mock_result.success = True so the code path that checks result.success is exercised; this ensures the _decode() byte-handling branch and success branch are covered when tests run.src/praisonai/praisonai/sandbox/sandlock.py (1)
275-286: Broad exception catch is acceptable here but could be narrowed.The static analysis tool flags
except Exception(BLE001). While catching broad exceptions is generally discouraged, this usage is reasonable for wrapping sandlock execution where multiple exception types could occur. The exception details are preserved in the error field rather than silently swallowed.Consider narrowing to specific expected exceptions if sandlock documents them, but the current approach is pragmatic for resilience.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/sandbox/sandlock.py` around lines 275 - 286, The catch-all "except Exception" in the try block that awaits asyncio.get_running_loop().run_in_executor(None, _run) is triggering BLE001; either narrow it to the specific exceptions sandlock can raise (e.g., TimeoutError, RuntimeError, OSError) by replacing "except Exception as e:" with "except (TimeoutError, RuntimeError, OSError) as e:" in the surrounding function, or if you intentionally want the broad catch for sandbox resilience, keep it but add logging.exception(...) to record the traceback and add a concise lint justification comment (e.g., "# broad except to ensure sandbox resilience (BLE001)"), or alternatively suppress the linter for that line with a targeted noqa, while still returning the SandboxResult using SandboxResult, SandboxStatus, execution_id and started_at as currently done.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/praisonai/praisonai/sandbox/sandlock.py`:
- Around line 378-391: The code unconditionally adds os.path.dirname(abs_path)
to extra_readable which can be "/" for files at filesystem root and would expose
the entire root; change the logic before calling _run_sandlocked to detect this
edge case (abs_path from os.path.realpath) and if os.path.dirname(abs_path) ==
os.path.sep, pass the file path itself (abs_path) or a more restrictive path
list instead of "/" as the extra_readable entry; update the call site that
constructs extra_readable (in the method that builds cmd and calls
self._run_sandlocked) to use this conditional selection so only the file (or a
non-root directory) is added.
- Around line 85-91: The RuntimeError message raised in SandlockSandbox when abi
< 1 contains an incorrect kernel version; update the text in the raise
RuntimeError (in sandlock.py around the SandlockSandbox check using the variable
abi) to state the correct minimum kernel that introduced Landlock ABI v1 (Linux
kernel >= 5.13) while preserving the rest of the guidance
(CONFIG_SECURITY_LANDLOCK=y and suggestion to use SubprocessSandbox).
---
Nitpick comments:
In `@src/praisonai/praisonai/sandbox/sandlock.py`:
- Around line 275-286: The catch-all "except Exception" in the try block that
awaits asyncio.get_running_loop().run_in_executor(None, _run) is triggering
BLE001; either narrow it to the specific exceptions sandlock can raise (e.g.,
TimeoutError, RuntimeError, OSError) by replacing "except Exception as e:" with
"except (TimeoutError, RuntimeError, OSError) as e:" in the surrounding
function, or if you intentionally want the broad catch for sandbox resilience,
keep it but add logging.exception(...) to record the traceback and add a concise
lint justification comment (e.g., "# broad except to ensure sandbox resilience
(BLE001)"), or alternatively suppress the linter for that line with a targeted
noqa, while still returning the SandboxResult using SandboxResult,
SandboxStatus, execution_id and started_at as currently done.
In `@src/praisonai/tests/unit/sandbox/test_sandlock_sandbox.py`:
- Around line 104-106: Update the test's mock_result to mirror real sandlock
behavior by setting mock_result.stdout and mock_result.stderr to bytes (e.g.,
b"Hello, World!" and b"") instead of str, and also set mock_result.success =
True so the code path that checks result.success is exercised; this ensures the
_decode() byte-handling branch and success branch are covered when tests run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ced78a2-08ac-444c-aaf3-c98f7e9529d1
📒 Files selected for processing (2)
src/praisonai/praisonai/sandbox/sandlock.pysrc/praisonai/tests/unit/sandbox/test_sandlock_sandbox.py
| if abi < 1: | ||
| raise RuntimeError( | ||
| "SandlockSandbox requires Landlock support (Linux kernel " | ||
| ">= 6.12 with CONFIG_SECURITY_LANDLOCK=y). This kernel " | ||
| f"reports Landlock ABI version {abi}. Use SubprocessSandbox " | ||
| "explicitly if weaker isolation is acceptable." | ||
| ) |
There was a problem hiding this comment.
Incorrect kernel version in error message.
The error message states Landlock requires "Linux kernel >= 6.12", but Landlock ABI v1 was introduced in Linux kernel 5.13 (released 2021). Kernel 6.12 is not accurate and may mislead users into thinking they need a much newer kernel than actually required.
📝 Suggested fix
if abi < 1:
raise RuntimeError(
"SandlockSandbox requires Landlock support (Linux kernel "
- ">= 6.12 with CONFIG_SECURITY_LANDLOCK=y). This kernel "
+ ">= 5.13 with CONFIG_SECURITY_LANDLOCK=y). This kernel "
f"reports Landlock ABI version {abi}. Use SubprocessSandbox "
"explicitly if weaker isolation is acceptable."
)📝 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.
| if abi < 1: | |
| raise RuntimeError( | |
| "SandlockSandbox requires Landlock support (Linux kernel " | |
| ">= 6.12 with CONFIG_SECURITY_LANDLOCK=y). This kernel " | |
| f"reports Landlock ABI version {abi}. Use SubprocessSandbox " | |
| "explicitly if weaker isolation is acceptable." | |
| ) | |
| if abi < 1: | |
| raise RuntimeError( | |
| "SandlockSandbox requires Landlock support (Linux kernel " | |
| ">= 5.13 with CONFIG_SECURITY_LANDLOCK=y). This kernel " | |
| f"reports Landlock ABI version {abi}. Use SubprocessSandbox " | |
| "explicitly if weaker isolation is acceptable." | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/sandbox/sandlock.py` around lines 85 - 91, The
RuntimeError message raised in SandlockSandbox when abi < 1 contains an
incorrect kernel version; update the text in the raise RuntimeError (in
sandlock.py around the SandlockSandbox check using the variable abi) to state
the correct minimum kernel that introduced Landlock ABI v1 (Linux kernel >=
5.13) while preserving the rest of the guidance (CONFIG_SECURITY_LANDLOCK=y and
suggestion to use SubprocessSandbox).
| abs_path = os.path.realpath(file_path) | ||
| interp = "bash" if file_path.endswith((".sh", ".bash")) else "python3" | ||
| cmd: List[str] = [interp, abs_path] | ||
| if args: | ||
| cmd.extend(args) | ||
|
|
||
| return await self._run_sandlocked( | ||
| cmd, | ||
| execution_id=execution_id, | ||
| limits=limits, | ||
| env=env, | ||
| working_dir=self._temp_dir, | ||
| extra_readable=[os.path.dirname(abs_path)], | ||
| ) |
There was a problem hiding this comment.
Consider edge case when script is at filesystem root.
If abs_path resolves to a file directly under / (e.g., /script.py), then os.path.dirname(abs_path) returns "/", which would add the entire root filesystem to the read allowlist. While this is an unlikely edge case, it could weaken isolation.
🛡️ Suggested guard
abs_path = os.path.realpath(file_path)
+ script_dir = os.path.dirname(abs_path)
+ extra_read = [script_dir] if script_dir and script_dir != "/" else []
interp = "bash" if file_path.endswith((".sh", ".bash")) else "python3"
cmd: List[str] = [interp, abs_path]
if args:
cmd.extend(args)
return await self._run_sandlocked(
cmd,
execution_id=execution_id,
limits=limits,
env=env,
working_dir=self._temp_dir,
- extra_readable=[os.path.dirname(abs_path)],
+ extra_readable=extra_read,
)📝 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.
| abs_path = os.path.realpath(file_path) | |
| interp = "bash" if file_path.endswith((".sh", ".bash")) else "python3" | |
| cmd: List[str] = [interp, abs_path] | |
| if args: | |
| cmd.extend(args) | |
| return await self._run_sandlocked( | |
| cmd, | |
| execution_id=execution_id, | |
| limits=limits, | |
| env=env, | |
| working_dir=self._temp_dir, | |
| extra_readable=[os.path.dirname(abs_path)], | |
| ) | |
| abs_path = os.path.realpath(file_path) | |
| script_dir = os.path.dirname(abs_path) | |
| extra_read = [script_dir] if script_dir and script_dir != "/" else [] | |
| interp = "bash" if file_path.endswith((".sh", ".bash")) else "python3" | |
| cmd: List[str] = [interp, abs_path] | |
| if args: | |
| cmd.extend(args) | |
| return await self._run_sandlocked( | |
| cmd, | |
| execution_id=execution_id, | |
| limits=limits, | |
| env=env, | |
| working_dir=self._temp_dir, | |
| extra_readable=extra_read, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/praisonai/praisonai/sandbox/sandlock.py` around lines 378 - 391, The code
unconditionally adds os.path.dirname(abs_path) to extra_readable which can be
"/" for files at filesystem root and would expose the entire root; change the
logic before calling _run_sandlocked to detect this edge case (abs_path from
os.path.realpath) and if os.path.dirname(abs_path) == os.path.sep, pass the file
path itself (abs_path) or a more restrictive path list instead of "/" as the
extra_readable entry; update the call site that constructs extra_readable (in
the method that builds cmd and calls self._run_sandlocked) to use this
conditional selection so only the file (or a non-root directory) is added.
|
@claude You are the FINAL architecture reviewer. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Architecture Review and AnalysisCRITICAL LIMITATION: This is a PR from a FORK - I cannot push fixes directly to this branch. I can only provide review feedback. Todo List:
Phase 1: Architecture Review per AGENTS.md✅ Protocol-driven Design: This change is appropriately placed in the wrapper ( ✅ Backward Compatible: The changes don't break public APIs. The breaking change (removing silent fallback) is intentional and documented. ✅ Performance: No hot-path regressions - sandlock is lazy-imported and only affects those explicitly choosing Phase 2: Valid Issues Found by ReviewersI've analyzed all reviewer feedback and identified 2 CRITICAL BUGS that need immediate attention: 1. CRITICAL: Incorrect Kernel Version (
|
|
@congwang-mk just a gentle reminder to fix this issue. Thank you |
congwang-mk
force-pushed
the
fix/sandlock-integration
branch
from
8b959b3 to
c0c4eee
Compare
Code-review fixes from PR MervinPraison#1367: * execute_file no longer allowlists the script's entire parent directory for Landlock read. Only the script file itself is granted read access, so sibling files on the host are never exposed (gemini flagged this as high severity). Verified end-to-end against real sandlock: a script that tries to read a sibling file is denied. * _build_sandbox_kwargs filters extra_readable with os.path.exists instead of os.path.isdir, so individual files (not just directories) can be allowlisted — required for the file-only execute_file path. * Corrected the Landlock kernel version in the fail-loud error message. The wrapper now requires ABI >= min_landlock_abi() (currently v6), which shipped in Linux 6.12 — not 6.7. The message is phrased around the ABI version so it stays correct as the SDK's minimum changes. * test_sandlock_execution_success now uses bytes for stdout/stderr (to exercise the _decode() byte path, matching real sandlock) and sets result.success = True explicitly. Note: the suggestion to also add temp_dir/working_dir to fs_readable was not applied — sandlock's fs_writable already grants read access ("fs_readable ... in addition to writable paths"), confirmed by test.
|
@congwang-mk just a gentle reminder to fix this issue. Thank you |
|
Merge review — blocked (fork PR, stale CI)
Fix list:
Wrapper/sandbox layer ✅ | unit tests present ✅ |
|
Claude finished @MervinPraison's task —— View job PR Merge Review - Sandlock Integration ❌CRITICAL FINDING: Multiple Unresolved Issues from April Review
RECOMMENDATION: DO NOT MERGECritical Issues (BLOCKING)1. 🚨 INCORRECT KERNEL VERSION (
|
|
@claude Stale CI — required workflows missing on HEAD (2026-06-12 audit) Status: Fix:
Do not merge until standard PR checks have run and passed on the latest commit. |
|
Claude finished @MervinPraison's task —— View job PR Status Assessment - Sandlock Integration ❌CRITICAL LIMITATION: This is a PR from a FORK - I cannot push fixes directly to this branch. I can only provide review feedback. Todo List:
RECOMMENDATION: DO NOT MERGECritical Issues (BLOCKING)1. 🚨 INCORRECT KERNEL VERSION (
|
congwang-mk
force-pushed
the
fix/sandlock-integration
branch
from
ec8b347 to
3cece7f
Compare
Code-review fixes from PR MervinPraison#1367: * execute_file no longer allowlists the script's entire parent directory for Landlock read. Only the script file itself is granted read access, so sibling files on the host are never exposed (gemini flagged this as high severity). Verified end-to-end against real sandlock: a script that tries to read a sibling file is denied. * _build_sandbox_kwargs filters extra_readable with os.path.exists instead of os.path.isdir, so individual files (not just directories) can be allowlisted — required for the file-only execute_file path. * Corrected the Landlock kernel version in the fail-loud error message. The wrapper now requires ABI >= min_landlock_abi() (currently v6), which shipped in Linux 6.12 — not 6.7. The message is phrased around the ABI version so it stays correct as the SDK's minimum changes. * test_sandlock_execution_success now uses bytes for stdout/stderr (to exercise the _decode() byte path, matching real sandlock) and sets result.success = True explicitly. Note: the suggestion to also add temp_dir/working_dir to fs_readable was not applied — sandlock's fs_writable already grants read access ("fs_readable ... in addition to writable paths"), confirmed by test.
|
@MervinPraison — rebased onto latest Fixed
Not changed — and why
Verification
|
Merge conflict resolution (automated analysis vs
|
|
Claude finished @MervinPraison's task —— View job PR Analysis: Sandlock Integration FixesTodo List:
SummaryThis PR significantly improves the Key Improvements in This PR✅ What Was Fixed
Critical Issues Still Present🔴 BLOCKING Issues
Conflict ResolutionThe PR has a merge conflict in Recommended Resolution:
Architecture Assessment ✅The changes comply with PraisonAI architecture:
RecommendationMERGE THIS PR after rebasing to resolve conflicts. The security improvements are critical and the implementation is sound. The previous reviewers' concerns about:
@congwang-mk Your latest changes from June properly addressed the legitimate issues. The PR is ready to merge once rebased. |
The SandlockSandbox wrapper had several latent correctness issues that
could cause it to silently run with weaker isolation than intended, or
drop resource limits on the floor. Written against the sandlock 0.8.1
API, which replaces the standalone Policy object with direct
Sandbox(**kwargs) configuration and collapses the network knobs into a
single net_allow endpoint allowlist.
Fixes:
* Configuration uses the current sandlock API. sandlock 0.8.1 dropped
the Policy object; config is passed straight to Sandbox(**kwargs).
_build_sandbox_kwargs() assembles that dict.
* Network policy intent is now explicit via net_allow. An empty
allowlist denies all outbound (the default); network-enabled=True
opens all TCP plus UDP DNS (["*:*", "udp://*:53"]) so the sandboxed
process can actually resolve hostnames and connect.
* stdout/stderr are now str, not bytes. sandlock returns bytes from
Sandbox.run(); PraisonAI's SandboxResult is typed as str. Added a
_decode() helper with errors="replace" so downstream consumers never
see binary artefacts or crash on .lower() / .split().
* max_cpu is now actually passed to the sandbox. Previously
limits.cpu_percent was silently ignored.
* env variables are injected via the Sandbox env field (set/overridden
in the child), which the current API supports natively.
* execute_file() passes the script by path, not via `python3 -c <code>`.
Large scripts no longer hit ARG_MAX, and the script's parent
directory is added to the Landlock read allowlist via the new
extra_readable parameter on _build_sandbox_kwargs.
* Timeout detection is authoritative: we inspect result.error for
"timed out" rather than heuristically comparing wall-clock duration
against limits.timeout_seconds. (Refined in the following commit to
use sandlock's exit_code == -1 sentinel.)
* Sandbox handles are now managed via `with ... as sb:` so cleanup
runs on exception.
* fs_readable is filtered to paths that actually exist. Landlock
fails at spawn time if any allowlisted path is missing, so the
hardcoded list (which included /usr/local/lib/python3) caused
sandbox creation failures on most hosts. Now we filter with
os.path.isdir before constructing the sandbox.
Breaking change — silent fallback removed:
SandlockSandbox used to fall back to SubprocessSandbox whenever
Landlock support was too low, logging only a warning. This violates
the caller's explicit choice of kernel-level isolation: a
SandlockSandbox that isn't actually using Landlock is a security
footgun, and a warning in the logs is not a consent mechanism.
__init__ now raises RuntimeError if Landlock support is missing —
i.e. landlock_abi_version() < min_landlock_abi() (currently ABI 6,
Linux 6.7+). Callers who want graceful degradation should catch
ImportError / RuntimeError and construct SubprocessSandbox
explicitly, e.g.:
try:
sb = SandlockSandbox(cfg)
except (ImportError, RuntimeError):
sb = SubprocessSandbox(cfg)
The equivalent fallback branches in execute(), run_command(), and
execute_file() are removed.
Tests updated:
- test_raises_when_landlock_unavailable replaces the two fallback
tests and asserts RuntimeError is raised at construction time.
- test_sandbox_kwargs_with_minimal_limits / _network_enabled check
max_cpu and the net_allow deny-all / allow-TCP+DNS semantics.
- test_sandlock_execution_timeout mocks result.error to assert the
timeout path.
- test_sandlock_execution_failure sets result.error=None explicitly.
All unit tests pass, including the real-sandlock integration test.
Sandbox.run() does not populate result.error on timeout — only the Pipeline path does. String-matching on result.error was therefore unreliable for the common single-sandbox case: a real timeout from Sandbox.run() returns success=False, exit_code=-1, empty stderr, and error=None, which my previous logic mis-classified as FAILED. Switch to the structural signal: sandlock's ExitStatus::Timeout is exposed as exit_code == -1 (see sandlock's _sdk.py around line 1475). This matches how sandlock itself detects pipeline timeouts and works uniformly across Sandbox.run() and any future execution paths. Verified end-to-end with a real forced timeout against real sandlock: status: SandboxStatus.TIMEOUT exit: -1 error: Execution timed out after 1s Test updated to match: mock_timeout_result.exit_code = -1 and error = None (reflecting actual Sandbox.run() behavior).
Code-review fixes from PR MervinPraison#1367: * execute_file no longer allowlists the script's entire parent directory for Landlock read. Only the script file itself is granted read access, so sibling files on the host are never exposed (gemini flagged this as high severity). Verified end-to-end against real sandlock: a script that tries to read a sibling file is denied. * _build_sandbox_kwargs filters extra_readable with os.path.exists instead of os.path.isdir, so individual files (not just directories) can be allowlisted — required for the file-only execute_file path. * Corrected the Landlock kernel version in the fail-loud error message. The wrapper now requires ABI >= min_landlock_abi() (currently v6), which shipped in Linux 6.12 — not 6.7. The message is phrased around the ABI version so it stays correct as the SDK's minimum changes. * test_sandlock_execution_success now uses bytes for stdout/stderr (to exercise the _decode() byte path, matching real sandlock) and sets result.success = True explicitly. Note: the suggestion to also add temp_dir/working_dir to fs_readable was not applied — sandlock's fs_writable already grants read access ("fs_readable ... in addition to writable paths"), confirmed by test.
… skip, and error logging - Mirror writable dirs into the Landlock read allowlist. Landlock does not grant read from write, so the sandboxed process could not read its own working/temp directory (Gemini MEDIUM). - Fix test_real_sandlock_integration to skip below sandlock.min_landlock_abi() instead of ABI < 1, so it cleanly skips on intermediate-ABI kernels rather than hard-failing in the constructor (Greptile). - Log the full traceback in the broad except around sandlock execution so the underlying cause is not lost (CodeRabbit BLE001).
congwang-mk
force-pushed
the
fix/sandlock-integration
branch
from
3cece7f to
4a1f94e
Compare
| @@ -320,10 +377,10 @@ async def execute( | |||
| self._used_subprocess_fallback = True | |||
| result = await fallback.execute(code, language, limits, env, working_dir) | |||
| return self._tag_subprocess_fallback(result) | |||
There was a problem hiding this comment.
execute() and run_command() (lines 367–379 and 449–461) still contain the full subprocess-fallback blocks the PR description says were removed. Since __init__ now raises RuntimeError before the object can be constructed without Landlock, these branches are dead code in production — but they create an inconsistency with execute_file(), which was refactored to have no fallback at all. If a future caller or test reaches execute() in a state where is_available returns False (e.g. mocked incorrectly, or AttributeError swallowed by the is_available exception handler), execute() and run_command() silently degrade to subprocess isolation while execute_file() would propagate the error. The three execution methods should behave uniformly.
execute() and run_command() still carried subprocess-fallback branches (inherited from main during the rebase) while execute_file() had none. Since __init__ now raises RuntimeError when Landlock is unavailable, the object can't be constructed without kernel isolation, making those branches dead code — and an inconsistency: if is_available ever returned False, execute()/run_command() would silently degrade to subprocess isolation while execute_file() propagated the error. Remove the fallback blocks and the now-orphaned _tag_subprocess_fallback helper so all three execution methods behave uniformly and fail loud. The _used_subprocess_fallback / metadata fields are retained: they now report a constant "not fallen back / kernel-isolated", keeping main's metadata schema intact and truthful. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| if env: | ||
| sandbox_kwargs["env"] = dict(env) |
There was a problem hiding this comment.
if env: treats an explicitly-passed empty dict (env={}) as falsy and skips injecting the env key into sandbox_kwargs. This means a caller who passes env={} intending to run the sandboxed process with a clean, secrets-free environment will instead inherit the full parent process environment — silently leaking any SECRET_KEY, DATABASE_URL, or similar vars present in the parent. The correct guard is if env is not None:; the empty-dict case should still set sandbox_kwargs["env"] = {} to honour the caller's intent.
| if env: | |
| sandbox_kwargs["env"] = dict(env) | |
| if env is not None: | |
| sandbox_kwargs["env"] = dict(env) |
…=True)
The sandlock child inherited the parent process's FULL environment on
every run: _build_sandbox_kwargs never set clean_env, so sandlock used
its default (clean_env=False = inherit everything), leaking host secrets
(SECRET_KEY, DATABASE_URL, ...) into untrusted sandboxed code. This also
diverged from SubprocessSandbox, which deliberately builds a minimal,
policy-driven env instead of copying the host's.
The reviewer-reported symptom (env={} skipped by `if env:`) was real but
its proposed fix was insufficient: with clean_env=False, setting env={}
overrides nothing and the full host environment still leaks. The actual
lever is clean_env.
- Set clean_env=True so the child starts from a minimal baseline
(PATH/HOME/USER/TERM/LANG); per-call env is layered on top.
- Change the guard to `if env is not None:` so an explicit empty dict is
honoured rather than skipped.
- Add a regression test asserting clean_env=True and that env={} /
populated / None are all handled without host-env inheritance.
Verified behaviorally: with SECRET_KEY set in the parent, the sandboxed
process sees it as absent for env=None and env={}, while an explicit
override still applies.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Automated merge readiness report (local git rebase origin/main scan, 2026-06-22 22:03 UTC) GitHub status
CI (key jobs: test-core, smoke, test-windows, main (3.11), review)
Guidance
Final verdict after fix: reply merge-ready (CLEAN + green CI) or list remaining blockers. |
|
Claude finished @MervinPraison's task —— View job PR Merge Readiness Assessment - Sandlock IntegrationTodo List:
SummaryThis PR significantly improves the Architecture Compliance ✅The changes comply with PraisonAI architecture principles per AGENTS.md:
Key Improvements ✅
Critical Issues AssessmentPreviously Flagged Issues - Status:
Remaining Minor Issues
CI Status
|
|
@claude if any gaps or issues, create a new branch, implement the fix and then create pr |
|
Claude finished @MervinPraison's task —— View job • Sandlock integration — remaining gap cleanup ✅PR #1367 is already merged and resolved most reviewer concerns. I audited the current merged code for the gaps reviewers left open, fixed the two valid ones, and confirmed the rest were already addressed.
Fixed (2 valid Greptile P1 findings)
Confirmed NOT bugs (already correct in merged code)
Verification--- • Branch: |
3 participants
Summary
The
SandlockSandboxwrapper insrc/praisonai/praisonai/sandbox/sandlock.pyhas several latent correctness and security issues. A caller who asks for kernel-level isolation could silently get weaker isolation, resource limits get dropped on the floor, and the timeout classifier never actually fires for real timeouts.This PR fixes the integration to match sandlock's actual API, removes the silent fallback that violated the caller's explicit security choice, and strengthens the test suite accordingly.
What's fixed
Network policy intent is now explicit. sandlock uses tri-state semantics for
net_allow_hosts(None= unrestricted,[]= deny all,[...]= allowlist). The previous code passedNoneto aSequence[str]field when network was enabled and[]when disabled — semantically backwards and type-invalid. Rewritten to passnet_connect=[\"0-65535\"]when enabled ornet_allow_hosts=[]when disabled. TCP-level deny-all defaults handle the rest.stdout/stderrare nowstr, notbytes. sandlock returnsbytesfromSandbox.run(); PraisonAI'sSandboxResultis typedstr. Added a_decode()helper witherrors=\"replace\"so downstream consumers never see binary artefacts or crash on.lower()/.split().max_cpuis now actually passed.limits.cpu_percentwas silently ignored. Addedmax_cpu=limits.cpu_percentto the Policy construction.execute_file()passes the script by path. The old implementation slurped the file intopython3 -c <code>which is subject toARG_MAX. Now it invokes[interp, abs_path, *args]directly and adds the script's parent directory to the Landlock read allowlist via a newextra_readableparameter on_create_policy.Timeout detection uses sandlock's structural sentinel. sandlock exposes
ExitStatus::Timeoutasexit_code == -1(seesandlock/_sdk.pyaround line 1475).Sandbox.run()does not populateresult.erroron timeout — only pipelines do — so string-matching is unreliable. Switching toexit_code == -1works uniformly and matches how sandlock itself detects timeouts.Context manager around
Sandbox.with self._sandlock.Sandbox(policy) as sb:guarantees cleanup even if.run()raises mid-flight.fs_readableis filtered to paths that exist. Landlock fails at spawn time if any allowlisted path is missing, so the hardcoded list — which included/usr/local/lib/python3— causedsandlock_spawn failedon most hosts. Thetest_real_sandlock_integrationtest was silently failing on baseline for this reason; it now passes.Breaking change — silent fallback removed
SandlockSandbox.execute/run_command/execute_fileused to fall back toSubprocessSandboxwheneverlandlock_abi_version() < 1, logging only a warning. This violates the caller's explicit choice of kernel-level isolation: aSandlockSandboxthat isn't actually using Landlock is a security footgun, and a warning in the logs is not a consent mechanism.__init__now raisesRuntimeErrorif Landlock support is missing. The three fallback branches inexecute(),run_command(), andexecute_file()are removed.Callers who want graceful degradation should catch
ImportError/RuntimeErrorand constructSubprocessSandboxexplicitly:```python
try:
sb = SandlockSandbox(cfg)
except (ImportError, RuntimeError):
sb = SubprocessSandbox(cfg)
```
This is a deliberate fail-loud change — the previous behavior would silently weaken the security guarantees of any caller running on a kernel <5.13 or in a container with seccomp stripped.
Test plan
test_raises_when_landlock_unavailablereplaces the two old fallback tests and assertsRuntimeErrorat construction time.test_sandlock_execution_timeoutmocksexit_code = -1(the sentinel) instead of patchingtime.time.test_sandlock_execution_failuresetsresult.error = Noneexplicitly (reflects realSandbox.run()behavior).test_policy_creation_with_minimal_limitsstrengthened to verifymax_cpu,net_allow_hosts=[]deny-all, and thatnet_connectis left unset so TCP deny-all defaults apply.test_real_sandlock_integrationnow actually passes (previously failed on baseline due to the/usr/local/lib/python3hardcoded path).execute_filewith args, and a forced real timeout all behave correctly.Files changed
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements