Repository navigation
Add truthful sandbox constraint preflight and strict jail spawning - #28
Conversation
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Tiny Sweeper reviewThe change adds a truthful per-constraint preflight model to tinybox-core and wires it into the object-safe Sandbox trait and the tinybox-jail backends, downgrades several coarse resource-limit capability declarations, introduces strict spawn entry points for directory jails, and replaces lossy canonicalization in the jail registry with a failing-closed helper. CI now pins third-party actions to commit SHAs and adds a cross-platform jail test job. The prior critique and security lanes reported findings about registry canonicalization and test environment dependencies; the tests and description lanes judged those resolved. The evidence notes code retrieval and memory tooling were unavailable during review. State: Changes requested Review snapshot
Completeness: Complete What changedNo supported behavioral explanation was produced. Features
Tests
Findings
Resolved this pass
Before merge
How this fits togetherflowchart LR
n0["Error<br/>changed"]:::changed
n1["DockerSandbox<br/>changed<br/>1 finding"]:::blocking
n2["Result"]:::impacted
n3["Sandbox"]:::impacted
n4["JailBackend"]:::impacted
n5["io"]:::impacted
n6["spawn"]:::impacted
n1 -->|implements| n3
n2 -->|uses| n0
n4 -->|uses| n2
n4 -->|uses| n5
n5 -->|uses| n0
n6 -->|uses| n0
n6 -->|uses| n2
n6 -->|uses| n5
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to No demonstrated issue blocks merging. Run the planned platform CI checks before release. Security Architecture Review
Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 63 functions across 19 files. (5 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely summarizes the main changes: truthful sandbox constraint preflight and strict jail spawning.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 63 functions across 19 files. (5 skipped: 5 unsupported.)
- Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
-
-
-
-
-
-
-
-
📝 Generate docstrings 💡
-
-
-
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
🛠️ Fix failing CI checks 💡
-
-
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
I’m a rabbit checking paths before the leap,
I sort each promise, firm or incomplete.
The jail asks what its backend can enforce,
And turns away a request that lacks that force.
Across three platforms, tests now run,
Then I hop home when checks are done.
Comment @coderabbitai help to get the list of available commands.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0589 · 780,204 in / 43,607 out · 79,510 cached (10%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0322 · 393,369 in / 23,449 out · 48,200 cached (12%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0260 · 320,720 in / 13,214 out · 29,326 cached (9%) · gpt-5.6-luna
tests: $0.0003 · 33,191 in / 3,407 out · 1,856 cached (6%) · glm-5.3-flash
description: $0.0001 · 15,973 in / 948 out · 0 cached (0%) · glm-5.3-flash
tinysweeper
Bot
added
the
priority: p0
![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: e1d0d6c69b
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8e532bdf3
ℹ️ 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".
There was a problem hiding this comment.
Requesting changes: 1 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0501 · 621,200 in / 63,139 out · 118,871 cached (19%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0277 · 315,185 in / 32,633 out · 68,257 cached (22%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0197 · 208,464 in / 22,247 out · 50,614 cached (24%) · gpt-5.6-luna
tests: $0.0014 · 49,831 in / 3,952 out · 0 cached (0%) · glm-5.3-flash
description: $0.0006 · 23,116 in / 1,196 out · 0 cached (0%) · glm-5.3-flash
Co-authored-by: Medulla <medulla@tinyhumans.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e6e0be11a
ℹ️ 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".
| let mut index = reg.index.lock().unwrap(); | ||
| index.records.get_mut(&record.id).unwrap().dir = lexical.join(&record.id); |
There was a problem hiding this comment.
When the registry mutex is poisoned or the expected record is unexpectedly absent, these new tests panic through unwrap() instead of returning their existing io::Result; the same pattern appears in the other newly added registry migration tests. The repository explicitly forbids unwrap() in test targets and requires tests to return Result and use ?, so convert these lookups to propagated errors rather than relying on the crate's pre-existing test-only lint allowance.
AGENTS.md reference: AGENTS.md:L193-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0235 · 340,841 in / 27,971 out · 33,495 cached (10%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0132 · 168,250 in / 13,815 out · 22,065 cached (13%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0096 · 88,744 in / 10,750 out · 11,302 cached (13%) · gpt-5.6-luna
tests: $0.0002 · 27,124 in / 906 out · 0 cached (0%) · glm-5.3-flash
description: $0.0002 · 26,609 in / 640 out · 0 cached (0%) · glm-5.3-flash
| let mut jail = Jail::new(&record.dir, &record.label); | ||
| jail.canonicalize()?; | ||
| Ok(jail) | ||
| Ok(Jail::new(&resolved, &record.label)) |
There was a problem hiding this comment.
Make jail validation and spawn atomic
The jail path is canonicalized and checked for containment before a path-based Jail is returned, but the backend uses that path later. A process that can replace or rename the validated directory between canonicalize() and backend use can cause the backend to operate on a different target, defeating the check. Keep the validated directory identity open through spawn, or use an equivalent atomic directory-handle-based confinement mechanism rather than passing a re-resolved path.
[RULE] toctou-path-validation ·
| std::os::unix::fs::symlink(outside.path(), &link)?; | ||
| reg.index | ||
| .lock() | ||
| .unwrap() |
There was a problem hiding this comment.
Return test errors instead of unwrapping
The added tests use unwrap() despite the repository rule requiring fallible tests to return Result and use ?. An unexpected lock, lookup, or persistence failure will panic instead of reporting the actual test error, and this also violates the workspace-wide no-unwrap policy. Convert these tests to return io::Result<()> and propagate failures with ?.
[RULE] no-unwrap ·
| .base | ||
| .canonicalize() | ||
| .unwrap_or_else(|_| self.base.clone()); | ||
| let resolved = path::canonicalize_missing(&record.dir)?; |
There was a problem hiding this comment.
Make canonicalization and deletion atomic
The containment check resolves record.dir, but deletion later uses the original path. A local attacker who can modify entries under the registry can replace an intermediate component after this check with a symlink or otherwise redirect the path, causing remove_dir_all to operate outside the validated target. Perform the validation and removal using an OS-level directory handle or another operation that binds the checked object to the deleted object.
[RULE] path-check-use-race ·
| let jail = self.jail_for(id)?; | ||
| log::debug!("[cwd_jail] registry.spawn_in id={id}"); | ||
| spawn_with(default_backend().as_ref(), &jail, cmd) | ||
| default_backend().spawn(&jail, cmd) |
There was a problem hiding this comment.
Preserve the resource-limit capability declaration
This replaces the existing spawn_with path with a direct backend call. The wrapper is responsible for preserving the declared resource-limit capability contract; bypassing it lets registry spawning use a backend without enforcing or advertising that capability consistently. Route this through the capability-aware spawn path, or otherwise make the declaration and enforcement explicit before calling the backend.
Additional critique observation
Preserve the backend capability preflight
[RULE] capability-preflight
This replaces the previous spawn_with wrapper with a direct backend call. The wrapper is where the registry's declared jail constraints and backend capability checks are preserved; calling spawn directly allows a backend to run without that preflight, including when a requested resource limit or other constraint is unsupported. Keep the wrapper or perform the same capability validation before invoking the backend.
[RULE] capability-contract ·
| let record = reg.create("removed")?; | ||
| fs::remove_dir_all(&record.dir)?; | ||
| assert_eq!( | ||
| reg.spawn_in_with(&record.id, &super::super::NoopBackend, Command::new("true")) |
There was a problem hiding this comment.
Use an executable guaranteed to exist in the test environment
These new tests invoke the ambient true executable by name, making the test suite depend on the host PATH and on an executable that is not guaranteed by the test environment. Use a repository-controlled executable or a backend/test seam that does not require an external command.
[RULE] environment-dependent-test ·
| } | ||
|
|
||
| /// Locate `true` only in the documented Landlock baseline, never in raw PATH. | ||
| fn permitted_true() -> std::io::Result<std::path::PathBuf> { |
There was a problem hiding this comment.
Avoid requiring a host-provided coreutils executable
This test returns an error when neither /usr/bin/true nor /bin/true exists, so the Linux test suite fails on minimal or non-coreutils environments even though Landlock itself may be available. Use a test executable guaranteed by the test environment or skip this test when no suitable executable is present.
[RULE] test-environment-dependency ·
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is high.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.1134 · 1,325,238 in / 127,473 out · 198,003 cached (15%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0597 · 719,822 in / 67,526 out · 122,822 cached (17%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0530 · 520,815 in / 55,382 out · 71,725 cached (14%) · gpt-5.6-luna
tests: $0.0002 · 27,162 in / 1,476 out · 1,856 cached (7%) · glm-5.3-flash
description: $0.0002 · 26,707 in / 548 out · 1,472 cached (6%) · glm-5.3-flash
| } | ||
| // Spawning requires an existing root. Never make a synthetic missing | ||
| // path pass containment and then resolve it to a different target. | ||
| let resolved = match record.dir.canonicalize() { |
There was a problem hiding this comment.
Make jail validation and spawn atomic
The directory is canonicalized and checked against self.base, then the resulting path is returned in a Jail for a later backend spawn. An attacker can replace the directory or a path component after this check, so the backend may operate on a different target, including one outside the registry. Keep validation and backend setup/use in one operation, or enforce an invariant that prevents the root from being replaced during the gap.
[RULE] toctou-path-validation ·
| @@ -125,7 +125,6 @@ impl DockerSandbox { | |||
| pub const fn declared_capabilities() -> SandboxCapabilities { | |||
| SandboxCapabilities::new(IsolationLevel::Kernel, SnapshotSupport::Filesystem) | |||
| .with_fork() | |||
There was a problem hiding this comment.
Preserve the resource-limit capability declaration
The new constraint_support method declares CPU, memory, and PID constraints as enforced, but declared_capabilities() no longer advertises ResourceLimits. Callers that select or preflight backends using capabilities() will therefore treat this Docker backend as unable to handle resource-limited specs, despite the backend declaring support for those constraints. Restore the capability declaration until all callers have migrated away from this capability.
[RULE] capability-consistency ·
| @@ -127,7 +127,6 @@ impl MicroVmSandbox { | |||
| #[must_use] | |||
| pub const fn declared_capabilities() -> SandboxCapabilities { | |||
| SandboxCapabilities::new(IsolationLevel::Hardware, SnapshotSupport::None) | |||
There was a problem hiding this comment.
Preserve the resource-limit capability declaration
Removing .with_resource_limits() changes the public capability contract: callers that select or validate backends using the coarse resource-limit capability will now see this backend as unsupported, even though the VM still has fixed CPU and memory sizing. The new ConstraintSupport declaration does not restore that capability; it reports CPU and memory only as best effort. Keep the resource-limit capability declared alongside the hardware isolation capability.
[RULE] capability-declaration ·
| // The guest is given fixed vCPUs and fixed memory by the hypervisor and | ||
| // cannot see that any more exists, which is a stronger limit than a cgroup. | ||
| assert!(caps.supports(Capability::ResourceLimits)); | ||
| assert!(!caps.supports(Capability::ResourceLimits)); |
There was a problem hiding this comment.
Preserve the resource-limit capability declaration
This changes the contract test to require that the microVM does not support resource limits. The surrounding test says the hypervisor gives the guest fixed vCPUs and fixed memory, and the existing resource-configuration coverage exercises those limits; those are resource limits rather than an absence of the capability. Keeping this assertion false allows the backend to advertise no resource-limit support despite enforcing the requested CPU and memory bounds. Restore the positive capability assertion.
Additional security observation
Preserve the resource-limit capability declaration
[RULE] capability-contract-regression
This changes the microVM's declared contract to say that resource limits are unsupported, even though the hypervisor is configured with fixed vCPU and memory limits and the surrounding test documents those as enforced limits. Callers use this capability declaration to reject otherwise supported resource-constrained workloads, so the changed assertion either makes the test fail against the implementation or codifies an incorrect capability contract. Keep the resource-limit capability declared.
Suggested change for this observation (reference only)
assert!(caps.supports(Capability::ResourceLimits));
Suggested change for the opening observation
| assert!(!caps.supports(Capability::ResourceLimits)); | |
| assert!(caps.supports(Capability::ResourceLimits)); |
[RULE] capability-contract ·
| .base | ||
| .canonicalize() | ||
| .unwrap_or_else(|_| self.base.clone()); | ||
| let resolved = path::canonicalize_missing(&record.dir)?; |
There was a problem hiding this comment.
Make deletion atomic with path validation
canonicalize_missing verifies that the recorded path resolves under self.base, but the later deletion still uses the original path after that check. An attacker can rename the validated directory and replace it with a directory outside the registry before remove_dir_all runs, causing deletion of the replacement target. Perform validation and removal on an object whose identity is held across the operation, or otherwise prevent the directory from being replaced between the check and deletion.
[RULE] toctou-filesystem-race ·
| let mut jail = Jail::new(&record.dir, &record.label); | ||
| jail.canonicalize()?; | ||
| Ok(jail) | ||
| Ok(Jail::new(&resolved, &record.label)) |
There was a problem hiding this comment.
Make jail validation and spawn atomic
The resolved path is checked against the registry base before constructing the Jail, but the backend opens or otherwise uses that path later. A concurrent filesystem rename or replacement can change what the backend applies confinement to after the check. Bind the validated directory to the spawned process through an atomic or descriptor-based mechanism, or have the backend perform validation and use as one operation.
[RULE] time-of-check-to-time-of-use ·
| let record = reg.create("corrupt")?; | ||
| let link = root.path().join("outside"); | ||
| std::os::unix::fs::symlink(outside.path(), &link)?; | ||
| reg.index |
There was a problem hiding this comment.
Return test errors instead of unwrapping
This newly added test returns io::Result<()>, but it still unwraps the registry mutex and record lookup. The repository rule forbids unwrap() in tests; propagate or convert both failures so a test failure is reported as an error instead of panicking.
Additional tests observation
Return test errors instead of unwrapping in the new registry tests
[RULE] no-unwrap-in-tests
The repository rule forbids unwrap() anywhere, including tests, which must return Result and use ?. The new tests removed_jail_under_symlinked_base_reports_missing_not_outside, missing_jail_through_outside_symlink_remains_forbidden, old_records_inherit_only_normalized_trusted_base_components and normalized_trusted_base_does_not_adopt_corrupt_suffix_traversal reach into reg.index.lock().unwrap() and unwrap the record lookup. Make the tests return io::Result<()> and map the poisoned-lock and missing-record cases into errors, or add a small helper that does so.
Suggested change for this observation (reference only)
let mut index = reg.index.lock().map_err(|_| io::Error::other("poisoned index"))?;
let record_dir = index
.records
.get_mut(&record.id)
.ok_or_else(|| io::Error::other("record vanished"))?;
record_dir.dir = link.join("missing/nested");
[RULE] unchecked-error ·
| } | ||
|
|
||
| #[cfg(test)] | ||
| #[path = "registry_path_tests.rs"] |
There was a problem hiding this comment.
Place module-local tests in mod_tests.rs
The repository rule requires module-local unit tests to live in a sibling mod_tests.rs file. Rename registry_path_tests.rs to mod_tests.rs and update this path accordingly.
[RULE] test-module-naming ·
| } | ||
|
|
||
| /// Locate `true` only in the documented Landlock baseline, never in raw PATH. | ||
| fn permitted_true() -> std::io::Result<std::path::PathBuf> { |
There was a problem hiding this comment.
Use an executable guaranteed to exist in the test environment
This test helper still assumes that true is installed at one of two host-specific coreutils paths. Minimal Linux images, nonstandard distributions, and some CI environments do not provide either path, so strict_landlock_filesystem_only_spawn_still_works fails before exercising the backend. Avoid requiring a host-provided coreutils binary; use a repository-built test executable or skip the capability test when no suitable executable is available.
[RULE] environment-dependent-test ·
| reg.jail_for(&record.id)?.root, | ||
| base.canonicalize()?.join(&record.id) | ||
| ); | ||
| let mut command = Command::new(if cfg!(windows) { "cmd" } else { "true" }); |
There was a problem hiding this comment.
Avoid requiring a host-provided coreutils executable
This new test invokes true on Unix and therefore depends on a host-provided coreutils command being present and discoverable via PATH. The test should use an executable controlled by the test suite rather than an external system utility.
[RULE] host-executable-dependency ·
Hosts can now inspect each sandbox constraint before execution and require complete enforcement. Strict preflight refuses unsupported or best-effort constraints. This implements the initial preflight portion of #27 and supports tinyhumansai/openhuman#7328.
Landlock reports filesystem-only confinement and rejects unsupported network/subprocess denials before spawning. Seatbelt launcher presence alone establishes no enforcement guarantee: restrictions are best effort and isolation is None. Docker, namespaces and MicroVMs report individual resource support instead of claiming complete resource-limit enforcement.
The new three-OS lane also exposed missing-child canonical-path bugs in jail registry deletion/spawning. The fix canonicalizes the trusted registry base and preserves legacy aliases without accepting corrupted suffix traversal. Spawning checks an existing canonical jail root once and passes that exact root to the backend; missing-path resolution is only used for cleanup or diagnostics. Registry parents must remain host-owned: this path-based API does not promise descriptor-atomic safety against independently privileged host writers. Tests now handle native directory-write error kinds and verify env_clear with a real inherited sentinel. Fixed the Unix-only import preventing Windows compilation, lowercase refusal labels, and pinned CI actions.
Validation: full all-feature suite passed (684 tests before the final Seatbelt followups; one preexisting ignored doctest), latest 107 jail tests and focused core tests pass, stable 1.99 strict Clippy, formatting, all-target Windows compilation, warning-denied rustdoc and >=90% coverage in every measured source file pass. Landlock, registry alias, lowercase-error and Seatbelt declaration regressions failed against the corresponding unfixed code. Hosted Linux/macOS/Windows jail jobs and all build jobs pass on the latest head. Automated re-review is pending. External Docker/SSH/MicroVM live suites remain environment gated.
Related PRs: tinyhumansai/tinysecurity#2 and tinyhumansai/openhuman#7331. OpenHuman remains a draft until the required upstream release and pin update.
Later backend/resource/network implementations, AppContainer, events, bus expansion and policy extraction remain tracked by #27. This PR does not close the tracker. OpenHuman must adopt the released upstream change before updating its gitlink.