test(e2e): add sandbox security & durability e2e cases - #1069
Open
G4614 wants to merge 5 commits into
Open
Conversation
Cover four platform P0s that the suite was missing: outbound allow-list enforcement, secret/token non-leak into the guest, cross-box filesystem isolation, and on-box data (file + SQLite) survival across stop/start.
📦 BoxLite review — looks good ·
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds five async E2E test modules covering cross-box filesystem isolation, TCP and UDP egress filtering, filesystem and SQLite persistence across restart, guest secret/API-token visibility, and export-import data durability. ChangesSandbox guarantees
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test/e2e/cases/test_cross_box_isolation.py`:
- Around line 56-62: Update the assertions in the _run call within the cross-box
isolation test to require output.strip() to equal exactly "DONE", ensuring grep
returned no matching paths while preserving the existing isolation-breach
diagnostic context.
In `@scripts/test/e2e/cases/test_restart_persistence.py`:
- Around line 26-35: Update the `_WRITE` subprocess script to keep the SQLite
connection open or exit via `NO_CKPT_ON_CLOSE`/`_exit` after printing `WROTE`,
preventing WAL checkpointing and deletion before `stop()`. Add an assertion that
the WAL file exists before restarting, so the test specifically validates
WAL-only persistence.
In `@scripts/test/e2e/cases/test_secret_no_leak.py`:
- Around line 41-47: Update the generated secret-scanning script in
test_secret_no_leak.py to recursively traverse directory paths under
/etc/environment, /root/.boxlite, and /run/secrets, including files such as
/run/secrets/e2e_probe in the collected buffer. Preserve the existing
file-reading and exception-tolerant behavior while ensuring directory contents
are inspected rather than opened only as directory entries.
🪄 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 Plus
Run ID: f68b0b46-b62f-48af-8dbe-e010c5b682a6
📒 Files selected for processing (4)
scripts/test/e2e/cases/test_cross_box_isolation.pyscripts/test/e2e/cases/test_egress_allowlist.pyscripts/test/e2e/cases/test_restart_persistence.pyscripts/test/e2e/cases/test_secret_no_leak.py
…ssert, recurse secret dirs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test/e2e/cases/test_export_import_roundtrip.py`:
- Around line 65-67: Update the cleanup logic around the source and destination
removals so rt.remove(dst.id, force=True) is attempted even when removing src.id
raises an exception. Use nested finally blocks or independent cleanup handling,
while preserving the conditional check that skips destination cleanup when dst
is None.
- Around line 61-63: Update the assertion in the export/import round-trip test
to compare the captured output directly with SENTINEL, removing the strip() call
so leading and trailing whitespace changes are detected while preserving the
existing failure message.
🪄 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 Plus
Run ID: f8871f53-fc8e-4159-9e84-579aa6b7a664
📒 Files selected for processing (1)
scripts/test/e2e/cases/test_export_import_roundtrip.py
…endent cleanup, bound grep
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/test/e2e/cases/test_cross_box_isolation.py`:
- Around line 62-64: Update the command constructed in the cross-box isolation
test to preserve and inspect the grep/timeout exit status instead of
unconditionally echoing DONE. Treat only grep’s no-match status as successful,
while propagating timeout, unavailable-command, and other scan failures; retain
the existing marker detection behavior and avoid hiding actionable errors.
🪄 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 Plus
Run ID: 30139c76-7ccf-43bb-a0fe-0db10b0080eb
📒 Files selected for processing (2)
scripts/test/e2e/cases/test_cross_box_isolation.pyscripts/test/e2e/cases/test_export_import_roundtrip.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/test/e2e/cases/test_egress_allowlist.py (2)
77-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProve blocked-target reachability before asserting filtering.
Both tests can pass because
BLOCKED_IPis unavailable, not because egress filtering works.
scripts/test/e2e/cases/test_egress_allowlist.py#L77-L100: preflightBLOCKED_IPover unrestricted TCP or use a controlled responder.scripts/test/e2e/cases/test_egress_allowlist.py#L185-L192: preflightBLOCKED_IPover unrestricted UDP or use a controlled responder.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test/e2e/cases/test_egress_allowlist.py` around lines 77 - 100, Ensure BLOCKED_IP reachability is established before validating filtering: in scripts/test/e2e/cases/test_egress_allowlist.py lines 77-100, preflight the target over unrestricted TCP or use a controlled responder before asserting test_allowlist_permits_only_listed_host; apply the corresponding unrestricted UDP preflight or controlled responder in lines 185-192. Keep the existing assertions and cleanup behavior, but make both tests fail setup when BLOCKED_IP is inherently unavailable rather than treating that as filtering evidence.
44-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound process waiting and stream draining together.
Both helpers drain output before starting the process timeout, so a stalled stream can hang the E2E suite indefinitely.
scripts/test/e2e/cases/test_egress_allowlist.py#L44-L55: rundrain(ex)concurrently with the boundedex.wait().scripts/test/e2e/cases/test_egress_allowlist.py#L140-L141: apply the same timeout-safe sequencing to the UDP helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test/e2e/cases/test_egress_allowlist.py` around lines 44 - 55, Update _probe and the UDP helper at scripts/test/e2e/cases/test_egress_allowlist.py lines 44-55 and 140-141 so drain(ex) runs concurrently with the bounded ex.wait() call, ensuring both stream consumption and process completion are covered by the timeout; preserve the existing output, exit-code, and result validation behavior at both sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/test/e2e/cases/test_egress_allowlist.py`:
- Around line 77-100: Ensure BLOCKED_IP reachability is established before
validating filtering: in scripts/test/e2e/cases/test_egress_allowlist.py lines
77-100, preflight the target over unrestricted TCP or use a controlled responder
before asserting test_allowlist_permits_only_listed_host; apply the
corresponding unrestricted UDP preflight or controlled responder in lines
185-192. Keep the existing assertions and cleanup behavior, but make both tests
fail setup when BLOCKED_IP is inherently unavailable rather than treating that
as filtering evidence.
- Around line 44-55: Update _probe and the UDP helper at
scripts/test/e2e/cases/test_egress_allowlist.py lines 44-55 and 140-141 so
drain(ex) runs concurrently with the bounded ex.wait() call, ensuring both
stream consumption and process completion are covered by the timeout; preserve
the existing output, exit-code, and result validation behavior at both sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b56b533-a945-44bd-87e5-5cf7cae5e6d1
📒 Files selected for processing (1)
scripts/test/e2e/cases/test_egress_allowlist.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add four sandbox platform e2e cases covering security & durability P0s the suite was missing: outbound allow-list enforcement, secret/access-token non-leak into the guest, cross-box filesystem isolation, and on-box data (file + WAL SQLite) survival across stop/start.
Test plan:
ruff format --check+ruff checkon the four new cases (clean)python -m py_compileon all four (syntax OK)NetworkSpec/Secret/BoxOptions,box.start()/stop())make test:e2e— requires the gated self-hosted e2e runner; not run here. Two assertions may legitimately surface real gaps rather than test bugs: per-IP allow-list enforcement (test_egress_allowlist) and rootfs survival across the managed runner's stop/start (test_restart_persistence).Summary by CodeRabbit