Add compression tests for cargo and git outputs#7
Conversation
|
Warning Review limit reached
More reviews will be available in 48 minutes and 10 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded fixture-backed golden tests for the built-in compressor, updated carriage-return handling in compression output, and made Linux sandbox seccomp network blocking conditional on Landlock network restriction. ChangesGolden compression tests
Linux network sandbox policy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@crates/skarn-compress/tests/compress_golden.rs`:
- Around line 6-13: The helper functions in compress_golden.rs are failing the
formatting check, so run rustfmt and keep the affected helpers aligned with the
standard chain layout and assert_eq! formatting. Update the formatting around
fixture_path, read_fixture, and the related test assertions so the file matches
cargo fmt --all expectations.
🪄 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: 37329c13-1614-4dd3-939b-70e5f1286e72
📒 Files selected for processing (5)
crates/skarn-compress/tests/compress_golden.rscrates/skarn-compress/tests/fixtures/cargo_expected.txtcrates/skarn-compress/tests/fixtures/cargo_fixture.txtcrates/skarn-compress/tests/fixtures/git_expected.txtcrates/skarn-compress/tests/fixtures/git_fixture.txt
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 `@crates/skarn-sandbox/src/linux.rs`:
- Line 140: `install_seccomp` and its callers are using a single `restrict_net`
flag, which lets `NetPolicy::AllowOutbound` still permit `bind`/listener
sockets. Split the network restriction into separate outbound-connect and
inbound-bind controls in `linux.rs`, and thread the new policy through the
affected `install_seccomp` call sites so `AllowOutbound` blocks listeners while
still allowing outbound connections.
- Around line 178-181: The DenyAll network policy in linux.rs is still allowing
UDP egress because the deny list in the network-blocking branch only covers
connect and bind; update the NetPolicy::DenyAll handling to also block datagram
transmission by denying SYS_sendto, SYS_sendmsg, and SYS_sendmmsg, or by
blocking SYS_socket for AF_INET/AF_INET6 in the same sandbox rule setup. Make
the change in the code path that builds the syscall rules for block_net, and add
a regression test that opens an unconnected UDP socket and verifies sendto is
rejected.
🪄 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: bdd748c5-baa1-41d4-b599-4ec5c8251fa2
📒 Files selected for processing (2)
crates/skarn-compress/tests/compress_golden.rscrates/skarn-sandbox/src/linux.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/skarn-compress/tests/compress_golden.rs
|
|
||
| // Apply seccomp on top (best-effort: if it fails we still have Landlock). | ||
| match install_seccomp() { | ||
| match install_seccomp(restrict_net) { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Keep AllowOutbound from permitting listeners.
Passing a single restrict_net boolean means NetPolicy::AllowOutbound does not block bind, so it behaves like AllowAll for inbound/listening sockets despite the upstream “Allow outbound connections” contract.
Suggested split between connect and bind blocking
- match install_seccomp(restrict_net) {
+ let block_connect = matches!(policy.net, NetPolicy::DenyAll);
+ let block_bind = matches!(policy.net, NetPolicy::DenyAll | NetPolicy::AllowOutbound);
+ match install_seccomp(block_connect, block_bind) {
Ok(()) => notes.push("seccomp-bpf denylist applied".to_string()),
Err(e) => notes.push(format!("seccomp-bpf not applied: {e}")),
}-fn install_seccomp(block_net: bool) -> std::result::Result<(), String> {
+fn install_seccomp(block_connect: bool, block_bind: bool) -> std::result::Result<(), String> {
@@
- if block_net {
+ if block_connect {
rules.insert(libc::SYS_connect, Vec::new());
+ }
+ if block_bind {
rules.insert(libc::SYS_bind, Vec::new());
}Also applies to: 178-181
🤖 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 `@crates/skarn-sandbox/src/linux.rs` at line 140, `install_seccomp` and its
callers are using a single `restrict_net` flag, which lets
`NetPolicy::AllowOutbound` still permit `bind`/listener sockets. Split the
network restriction into separate outbound-connect and inbound-bind controls in
`linux.rs`, and thread the new policy through the affected `install_seccomp`
call sites so `AllowOutbound` blocks listeners while still allowing outbound
connections.
| if block_net { | ||
| rules.insert(libc::SYS_connect, Vec::new()); | ||
| rules.insert(libc::SYS_bind, Vec::new()); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect current sandbox network coverage for DenyAll UDP/datagram and outbound-only bind cases.
set -euo pipefail
rg -n -C2 \
'UdpSocket|sendto|sendmsg|sendmmsg|AF_INET|AF_INET6|AllowOutbound|DenyAll|SYS_socket|SYS_connect|SYS_bind' \
crates/skarn-sandbox crates/skarnRepository: Rani367/Skarn
Length of output: 6768
Close the DenyAll datagram bypass
The current DenyAll implementation on Linux only blocks connect and bind. This leaves sendto/sendmsg on unconnected UDP sockets open, allowing network egress without violating the blocked syscalls. A process can create an AF_INET/AF_INET6 datagram socket and send data directly.
Required Fix:
When NetPolicy::DenyAll is active, extend the deny rules to include:
SYS_socket(preferably filtered forAF_INET/AF_INET6families) to prevent network socket creation, ORSYS_sendto,SYS_sendmsg, andSYS_sendmmsgto block datagram transmission explicitly.
Ensure a regression test is added for UDP sendto on an unconnected socket to verify the block.
Code gap
// crates/skarn-sandbox/src/linux.rs:178-181
// Currently only blocks connect/bind, allowing:
// socket(AF_INET, SOCK_DGRAM, 0) -> sendto(...)
if block_net {
rules.insert(libc::SYS_connect, Vec::new());
rules.insert(libc::SYS_bind, Vec::new());
}🤖 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 `@crates/skarn-sandbox/src/linux.rs` around lines 178 - 181, The DenyAll
network policy in linux.rs is still allowing UDP egress because the deny list in
the network-blocking branch only covers connect and bind; update the
NetPolicy::DenyAll handling to also block datagram transmission by denying
SYS_sendto, SYS_sendmsg, and SYS_sendmmsg, or by blocking SYS_socket for
AF_INET/AF_INET6 in the same sandbox rule setup. Make the change in the code
path that builds the syscall rules for block_net, and add a regression test that
opens an unconnected UDP socket and verifies sendto is rejected.
1 participant
This PR solves(#4). Adds two golden-file integration tests under crates/skarn-compress/tests/ that lock in the compression behavior of the cargo and git profiles, preventing regressions when editing default_rules.yaml.
Summary by CodeRabbit
\rfrom producing empty segments when carriage-collapsing is enabled.