Repository navigation
fix(linux): preserve shell literals in systemd scopes - #102
Merged
Merged
Conversation
…cope Adds a live test that runs a shell command referencing a dollar literal inside a cgroup-limited namespace sandbox and asserts the text is passed through unchanged. This guards against the scope setup expanding or mangling dollar references in the executed command. Auto-committed-on: dragonfly
The live systemd scope test now checks that a braced shell variable reference survives as a literal instead of the bare form. This widens coverage to the syntax that is more likely to be mangled by quoting. Auto-committed-on: dragonfly
…tinybox-linux/src/sandbox/mod_t Auto-committed-on: dragonfly
The `--expand-environment=no` argument is only understood by systemd v254 and newer, so passing it unconditionally broke sandbox limits on older releases. The flag is now added only when `systemd-run --help` reports support for it, cached in a `OnceLock` to avoid repeated probes. Auto-committed-on: dragonfly
Widen cgroup_prefix and systemd_supports_expand_environment to pub(super) so tests can call them directly. The scope test now asserts the flag against the detected capability instead of assuming support, and a new test checks that the flag is emitted only when systemd supports it. Auto-committed-on: dragonfly
Auto-committed-on: dragonfly
The expand-environment feature probe now returns a Result so that a failed systemd-run invocation surfaces as an unsupported-capability error instead of silently disabling resource limits. The help-output parsing was split into its own function so the legacy and failure paths can be tested directly. Auto-committed-on: dragonfly
The exec argument builder now takes the expand-environment capability as a parameter instead of probing systemd itself. This removes the cached probe and its unused imports from the module, leaving the pure argument builders free of process and global state. Auto-committed-on: dragonfly
The systemd-run feature probe now runs through the sandbox host abstraction instead of spawning a process directly, so the check is observable and testable. The probe result is passed into argument construction, and a failed probe still refuses to launch the workload. Auto-committed-on: dragonfly
Reformatted the argument parsing signature and several call sites and test assertions so they fit within the line width limit. No behaviour changed. Auto-committed-on: dragonfly
Auto-committed-on: dragonfly
The `--expand-environment` byte literal used when probing systemd help output is now a module-level constant so the flag name lives in one place. The detection logic itself is unchanged. Auto-committed-on: dragonfly
The systemd environment flag probe tests now compare full Ok and Err results rather than unwrapping, so a failure reports the actual value instead of panicking. The tests also return Result to propagate errors. Auto-committed-on: dragonfly
The systemd environment flag probe test no longer returns a Result since its assertions cannot fail with an error, so the return type and trailing Ok(()) were removed. Auto-committed-on: dragonfly
Tiny Sweeper reviewTiny Sweeper reviewed this change across 5 lane(s) and found 0 active actionable finding(s). Detailed lane evidence and any incomplete work are listed below. State: Incomplete Review snapshot
Completeness: Incomplete FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred.
FindingsNo active actionable findings. Could not review: crates/tinybox-linux/src/sandbox/args.rs, crates/tinybox-linux/src/sandbox/mod.rs, crates/tinybox-linux/src/sandbox/mod_tests.rs, crates/tinybox-linux/tests/live_namespaces.rs, tinysweeper/description, tinysweeper/tests Before merge
How this fits togetherflowchart LR
n0["NamespaceSandbox<br/>changed"]:::changed
n1["RecordingHost<br/>changed"]:::changed
n2["limits_wrap_the_sandbox_in_a_systemd_scope<br/>changed"]:::changed
n3["live_a_memory_limit_is_actually_enforced<br/>changed"]:::changed
n4["sandbox"]:::impacted
n5["spec"]:::impacted
n6["last"]:::impacted
n7["spec"]:::impacted
n8["enabled"]:::impacted
n9["sandbox"]:::impacted
n2 -->|calls| n5
n2 -->|tests| n5
n2 -->|calls| n6
n2 -->|tests| n6
n3 -->|calls| n7
n3 -->|calls| n8
n4 -->|uses| n0
n4 -->|uses| n1
n9 -->|uses| n0
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
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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
This review includes 4 billable files and costs up to $1.00. Or wait 7 minutes for your next included review. View limit details
Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
tinysweeper found nothing blocking, but could not review everything, so this is not an approval: crates/tinybox-linux/src/sandbox/args.rs, crates/tinybox-linux/src/sandbox/mod.rs, crates/tinybox-linux/src/sandbox/mod_tests.rs, crates/tinybox-linux/tests/live_namespaces.rs, tinysweeper/description, tinysweeper/tests.
$0.0001 · 0 in / 0 out · 416 embedded · ladder/vectors-oai3
tinysweeper
Bot
added
the
priority: p3
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.
When namespace limits are enabled,
systemd-run --scopecan expand${VAR}in the shell command argument before/bin/shevaluates its quoting. This changes literal shell input and can silently corrupt otherwise valid commands.The namespace backend now probes
systemd-run --helpthrough the sameHostused for launch, withLC_ALL=C. It adds--expand-environment=nowhen the option is advertised. A successful legacy help response without the option keeps the old argv; failed probes refuse resource-limited execution rather than risk expansion. Upstream tagged source shows the option first in v254; v249 lacks it, while scope expansion was backward-compatible/off by default through v257. The live regression was exercised on systemd 259.Validation:
${cgroup_path}payload produced only a newline (1 failed).${cgroup_path}byte-for-byte (1 passed).cargo fmt --all -- --checkcargo clippy --all-targets --all-features -- -D warningscargo build --all-targets --all-featurescargo test --all-featuresscripts/check-coverage.sh(90% per-file gate)This fixes the command-argument corruption slice of tinybox#27; it does not claim that the broader native resource-enforcement work is complete. Related OpenHuman epic: tinyhumansai/openhuman#7328.