feat: data plane (quinn) + relay + trust + sync (v0.2.0)#2
Conversation
Separate ticket issuer and subject identities so an operator-controlled relay can sign access for a different peer instead of assuming self-issued tickets only.\n\nAlso adds CI and a contribution guide so the repository can evolve through short-lived feature branches and PRs instead of direct main-only changes.
Add a compiled Quinn-based transport profile layer with low-latency and relay-oriented presets, plus an ARCHITECTURE.md that documents the control-plane/data-plane split and transport strategy with Mermaid diagrams.
Add a CLI surface for emitting named Quinn transport profiles so the QUIC foundation is directly inspectable and reusable without embedding library code.
- Replace src/quic.rs with src/quic/ directory (endpoint.rs + mod.rs) - QuicTransportProfile with low_latency_interactive + relay_backhaul presets - Ticket-gated session handshake (client_handshake, server_handshake) - TrustStore with ~/.config/snappipe/trust.toml persistence - Relay with ByteStream trait for testability - NonceStore (TTL 60s, in-memory + hex persistence) - RateLimiter (token bucket per NodeId, default 100 req/min) - Directory walk + mtime preservation in sync.rs - 4 E2E tests with real QUIC endpoints on loopback Repository URL corrected: louzt/SnapPipe -> LOUST-PRO/SnapPipe All 52 tests pass (48 unit + 4 integration), cargo clippy clean.
|
Warning Review limit reached
More reviews will be available in 45 minutes and 27 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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 Plus Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThe PR adds explicit issuer/subject ticket claims, QUIC transport profiles and endpoint support, persistent trust/rate/nonce state, ticket-gated session and relay flows, sync-plane directory walking, and repository docs, CI, and contribution guidance. ChangesSnapPipe foundation
Sequence Diagram(s)sequenceDiagram
participant client_handshake
participant server_handshake
participant TrustStore
participant Relay
participant RateLimiter
client_handshake->>server_handshake: send length-prefixed SignedTicket
server_handshake->>TrustStore: is_trusted(issuer)
server_handshake-->>client_handshake: HandshakeResponse::Ok or HandshakeResponse::Err
client_handshake->>Relay: open QUIC connection after handshake
Relay->>RateLimiter: sync_node_limit(peer_node)
Relay->>Relay: handle_connection byte pump and record ConnectionLog
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (1)
src/lib.rs (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the Clippy suppressions to the frozen module.
These crate-level allows also suppress future issues in the new data-plane modules. If the warnings are only from
session.rs, attach the attributes to that module declaration instead.Suggested narrowing
-// Phase 2c `session.rs` is frozen; suppress pre-existing clippy nits that -// would otherwise block `-D warnings` CI on this crate. -#![allow(clippy::clone_on_copy)] -#![allow(clippy::let_underscore_future)] - pub mod quic; +#[allow(clippy::clone_on_copy)] +#[allow(clippy::let_underscore_future)] pub mod session;🤖 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 `@src/lib.rs` around lines 1 - 4, The Clippy suppressions are currently applied at the crate level in lib.rs, which silences warnings in new modules as well. Move the allow attributes to the frozen session module declaration instead, so only session.rs is exempt while the rest of the crate still gets Clippy coverage. Use the session module name in lib.rs to scope the attributes narrowly and keep the new data-plane modules checked.
🤖 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 @.github/workflows/ci.yml:
- Around line 16-22: The workflow uses mutable action refs and leaves checkout
credentials persisted, so update the Checkout step in the CI workflow to disable
persisted credentials and pin both actions to immutable commit SHAs.
Specifically, adjust the actions/checkout invocation to include
persist-credentials set to false, and replace the dtolnay/rust-toolchain@stable
reference with a pinned SHA while keeping the rustfmt component configuration
intact.
In `@README.md`:
- Around line 150-157: The QUIC documentation path is stale and points readers
to a file that no longer exists. Update the README’s QUIC notes to reference the
current module location under src/quic/ instead of src/quic.rs, using the
existing QUIC transport symbols such as the quic module and endpoint.rs so
readers can find the actual implementation.
In `@src/lib.rs`:
- Line 56: `issuer` is being trusted as the identity for verification, but
`verify_ticket` can still accept a ticket whose claims say one issuer while the
`verifying_key` belongs to another. Update `verify_ticket` in `src/lib.rs` to
explicitly compare the claims `issuer` against the identity implied by
`verifying_key`, and reject the ticket when they differ before returning
verified claims. Use the existing `issuer` field and `verify_ticket` flow as the
place to enforce this trust-binding check.
In `@src/quic/endpoint.rs`:
- Around line 168-179: The server endpoint builder currently hides the
self-signed certificate it generates inside build_server_endpoint, so callers
cannot obtain or trust the cert. Update build_server_endpoint to either accept a
caller-provided DevCert or return the generated certificate together with the
Endpoint, and adjust the default_server_config/self_signed_dev_cert flow so the
certificate is available to the caller for trust setup.
- Around line 124-146: The rustls config builders are hard-coding the ALPN value
instead of using the profile setting, so update the `rustls_server_config` and
`rustls_client_config` logic to take the ALPN from
`QuicEndpointConfig.profile.alpn` (or the relevant profile object) and assign
that value to `alpn_protocols` in both places. Make sure the server and client
configs both use the same profile-derived ALPN so the CLI profile command and
TLS negotiation stay aligned.
- Around line 99-165: The rustls configuration block in rustls_server_config and
rustls_client_config has formatting drift that rustfmt will reject. Reformat the
affected code in src/quic/endpoint.rs by running cargo fmt so the builder
chains, line wrapping, and indentation match the project style and CI formatting
checks pass.
In `@src/quic/mod.rs`:
- Around line 1-117: The QUIC module needs formatting cleanup to satisfy the
rustfmt-only CI failure. Run rustfmt on the code around QuicTransportProfile and
build_transport_config, especially the imports and chained TransportConfig
setup, so the formatting matches the repository style and unblocks CI.
In `@src/rate_limit.rs`:
- Around line 89-98: Clamp zero overrides in the rate limit logic so they behave
like the constructor default: update set_limit and any trust-entry handling that
stores per-node limits to treat 0 as DEFAULT_RATE_PER_MIN instead of persisting
a zero-capacity bucket. Use the same fallback behavior already implemented in
new and ensure the limit applied to trusted nodes cannot be permanently set to
0.
In `@src/relay.rs`:
- Around line 136-227: `handle_connection` only relays bytes from `incoming` to
`outgoing`, so it remains half-duplex instead of bidirectional. Update
`Relay::handle_connection` to forward data in both directions concurrently,
using the existing `ByteStream` inputs and the same logging/rate-limit tracking,
and ensure `ConnectionLog` still records the final outcome from either
direction.
- Around line 185-213: Propagate EOF to the downstream stream in the relay loop:
in `src/relay.rs` inside the `loop` that reads from `incoming` and writes to
`outgoing`, make sure the `Ok(0)` path performs `outgoing.finish().await` before
breaking, so the peer sees FIN instead of waiting indefinitely. Keep the fix
localized to the relay forwarding logic around `incoming.read`,
`outgoing.write_all`, and the `ConnectionOutcome::Closed` handling.
In `@src/session.rs`:
- Around line 204-237: server_handshake is hard-coded to verify every ticket
against a single issuer_verifying_key, which prevents trusted tickets from other
issuers from ever reaching TrustCheck; update the handshake flow to support
selecting the correct issuer verification key from the ticket claims or trust
store before calling verify_ticket, and adjust server_handshake so it validates
signatures against the matching issuer rather than one caller-provided key.
- Around line 177-181: Bound the peer-controlled handshake frame length before
allocating in the session receive path, since `recv.read_exact`,
`u64::from_be_bytes`, and the subsequent `Vec<u8>` allocation currently trust
the remote size. Update the handshake parsing logic in the relevant receive
helper used by `src/session.rs` (and the matching send/receive code around the
other referenced block) to validate the decoded length against a reasonable
maximum frame size, reject oversized values with an error, and only allocate the
body buffer after that check passes.
In `@src/sync.rs`:
- Around line 1-350: The change in sync.rs needs formatting normalization
because rustfmt is failing in CI. Run cargo fmt and ensure the resulting
formatting is applied consistently across the walk_dir_with, build_entry,
diff_entries, and test sections so cargo fmt --check passes before merging.
- Around line 95-110: The WalkDir traversal in sync logic is dropping
`walkdir::Error` values and still descending into directories that should be
excluded. Update the `WalkDir::new(root)` pipeline in `sync.rs` to use
`filter_entry(...)` for pruning based on `predicate.keep(...)` instead of a
trailing `.filter(...)`, and preserve iterator errors by mapping any
`walkdir::Error` into `SyncError::Walk`. Keep the existing path normalization
behavior around `entry.path().strip_prefix(root)` and use the same
`predicate.keep` call site to locate the fix.
- Around line 26-31: The FileEntry timestamp handling only keeps whole seconds,
so subsecond mtime precision is lost across sync and comparisons. Update
FileEntry and the related sync flow in build_entry, apply_mtime, and
diff_entries to store the full timestamp value instead of truncating with
as_secs() and zero nanos; then compare and restore the exact mtime so same-size
edits within one second are detected and replayed accurately.
- Around line 113-130: The sync walk in `entries.par_iter().try_for_each` still
processes symlink entries because `metadata()` follows links, which can let
out-of-tree targets into the manifest. Update the `Sync` logic to reject
`entry.file_type().is_symlink()` before calling `std::fs::metadata(path)` in
both the file and directory handling paths, or switch to `symlink_metadata` and
require only real files; keep the check near the existing
`file_type().is_dir()`, `strip_prefix`, and `build_entry` flow.
In `@src/trust.rs`:
- Around line 84-89: The load_or_default path in trust::load_or_default is
masking real load errors by falling back to an empty Trust store on any failure.
Update the Self::load_from_path handling so only the absence of a default path
or an explicit “no file” case returns a fresh empty store, while malformed files
and I/O/permission errors are propagated or otherwise surfaced instead of
creating an empty in-memory registry. Keep the fix localized to load_or_default
and the related Self::load_from_path flow so save() cannot silently overwrite a
broken trust file.
- Around line 130-140: The trust store persistence in `save()` is lossy and
cannot round-trip `TrustEntry`: it only serializes `display_name` and
`rate_limit_per_min`, while `parse()` reconstructs `added_at_unix` as 0 and will
misread unescaped names containing `:`, `"` or newlines. Update the
`save()`/`parse()` pair in `TrustStore` to serialize all `TrustEntry` fields
needed for lossless restore and to escape or encode string values safely, then
make the parser decode that format back into the original entry.
In `@tests/quic_e2e.rs`:
- Around line 206-226: The negative QUIC handshake test is using the trusted
issuer key in the server path, so it fails on signature validation before
reaching the trust check. In the server task that calls server_handshake, switch
to the untrusted issuer’s verifying key so the ticket can validate and the
TrustCheck branch is exercised, then update the assertions in quic_e2e.rs to
expect the trust-specific rejection rather than a generic signature failure.
---
Nitpick comments:
In `@src/lib.rs`:
- Around line 1-4: The Clippy suppressions are currently applied at the crate
level in lib.rs, which silences warnings in new modules as well. Move the allow
attributes to the frozen session module declaration instead, so only session.rs
is exempt while the rest of the crate still gets Clippy coverage. Use the
session module name in lib.rs to scope the attributes narrowly and keep the new
data-plane modules checked.
🪄 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: 1195df05-9ef9-42bf-8452-930a85b045c2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/ci.ymlARCHITECTURE.mdCONTRIBUTING.mdCargo.tomlREADME.mdsrc/lib.rssrc/main.rssrc/nonce_store.rssrc/quic/endpoint.rssrc/quic/mod.rssrc/rate_limit.rssrc/relay.rssrc/session.rssrc/sync.rssrc/trust.rstests/quic_e2e.rs
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| components: rustfmt |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
sed -n '1,120p' .github/workflows/ci.ymlRepository: LOUST-PRO/SnapPipe
Length of output: 608
🏁 Script executed:
sed -n '1,120p' .github/workflows/ci.ymlRepository: LOUST-PRO/SnapPipe
Length of output: 608
🏁 Script executed:
sed -n '1,120p' .github/workflows/ci.ymlRepository: LOUST-PRO/SnapPipe
Length of output: 608
Pin the workflow actions and disable persisted checkout credentials.
actions/checkout@v4 and dtolnay/rust-toolchain@stable are mutable refs. Pin both to commit SHAs, and set persist-credentials: false on checkout so the GitHub token is not left in git config.
Suggested fix
- name: Checkout
- uses: actions/checkout@v4
+ uses: actions/checkout@<sha>
+ with:
+ persist-credentials: false
- name: Install Rust toolchain
- uses: dtolnay/rust-toolchain@stable
+ uses: dtolnay/rust-toolchain@<sha>🧰 Tools
🪛 zizmor (1.26.1)
[warning] 16-17: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 17-17: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 20-20: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 16 - 22, The workflow uses mutable
action refs and leaves checkout credentials persisted, so update the Checkout
step in the CI workflow to disable persisted credentials and pin both actions to
immutable commit SHAs. Specifically, adjust the actions/checkout invocation to
include persist-credentials set to false, and replace the
dtolnay/rust-toolchain@stable reference with a pinned SHA while keeping the
rustfmt component configuration intact.
Source: Linters/SAST tools
| ## QUIC notes | ||
|
|
||
| The repository now includes Quinn-based transport profiles in `src/quic.rs` for: | ||
|
|
||
| - low-latency interactive sessions | ||
| - relay/backhaul-oriented sessions | ||
|
|
||
| This is a transport-configuration foundation, not a claim that the full runtime/session orchestrator is finished. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the documented QUIC path.
src/quic.rs is stale; the QUIC transport code now lives under src/quic/ (src/quic/mod.rs plus endpoint.rs). Keeping the old path here will send readers to a file that no longer exists.
Suggested fix
-The repository now includes Quinn-based transport profiles in `src/quic.rs` for:
+The repository now includes Quinn-based transport profiles in `src/quic/` (`src/quic/mod.rs`) for:📝 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.
| ## QUIC notes | |
| The repository now includes Quinn-based transport profiles in `src/quic.rs` for: | |
| - low-latency interactive sessions | |
| - relay/backhaul-oriented sessions | |
| This is a transport-configuration foundation, not a claim that the full runtime/session orchestrator is finished. | |
| ## QUIC notes | |
| The repository now includes Quinn-based transport profiles in `src/quic/` (`src/quic/mod.rs`) for: | |
| - low-latency interactive sessions | |
| - relay/backhaul-oriented sessions | |
| This is a transport-configuration foundation, not a claim that the full runtime/session orchestrator is finished. |
🤖 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 `@README.md` around lines 150 - 157, The QUIC documentation path is stale and
points readers to a file that no longer exists. Update the README’s QUIC notes
to reference the current module location under src/quic/ instead of src/quic.rs,
using the existing QUIC transport symbols such as the quic module and
endpoint.rs so readers can find the actual implementation.
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct TicketClaims { | ||
| pub version: u8, | ||
| pub issuer: NodeId, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bind issuer to the key used for verification.
issuer is now used as trust identity, but verify_ticket can return claims whose issuer does not match the verifying_key if a ticket is constructed manually. Reject that mismatch during verification so callers cannot accidentally trust a self-asserted issuer.
Suggested verification guard
verifying_key
.verify(payload.as_bytes(), &signature)
.map_err(|_| TicketError::InvalidSignature)?;
+
+ if ticket.claims.issuer != NodeId::from_verifying_key(verifying_key) {
+ return Err(TicketError::InvalidSignature);
+ }
Ok(ticket.claims.clone())🤖 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 `@src/lib.rs` at line 56, `issuer` is being trusted as the identity for
verification, but `verify_ticket` can still accept a ticket whose claims say one
issuer while the `verifying_key` belongs to another. Update `verify_ticket` in
`src/lib.rs` to explicitly compare the claims `issuer` against the identity
implied by `verifying_key`, and reject the ticket when they differ before
returning verified claims. Use the existing `issuer` field and `verify_ticket`
flow as the place to enforce this trust-binding check.
| server_crypto.alpn_protocols = vec![b"/snappipe/0".to_vec()]; | ||
| Ok(Arc::new(server_crypto)) | ||
| } | ||
|
|
||
| /// Build a rustls-backed `ClientConfig` that trusts the given dev cert as a root. | ||
| fn rustls_client_config(dev_cert: &DevCert) -> Result<Arc<rustls::ClientConfig>, QuicEndpointError> { | ||
| let mut roots = rustls::RootCertStore::empty(); | ||
| let cert = dev_cert | ||
| .cert_chain | ||
| .first() | ||
| .ok_or_else(|| QuicEndpointError::Rustls("empty cert chain".into()))?; | ||
| roots | ||
| .add(cert.clone()) | ||
| .map_err(|err| QuicEndpointError::Rustls(err.to_string()))?; | ||
|
|
||
| let provider = rustls::crypto::ring::default_provider(); | ||
| let provider_arc = Arc::new(provider); | ||
| let mut client_crypto = rustls::ClientConfig::builder_with_provider(provider_arc) | ||
| .with_safe_default_protocol_versions() | ||
| .map_err(|err| QuicEndpointError::Rustls(err.to_string()))? | ||
| .with_root_certificates(roots) | ||
| .with_no_client_auth(); | ||
| client_crypto.alpn_protocols = vec![b"/snappipe/0".to_vec()]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Thread the profile ALPN into rustls configs.
These configs hard-code /snappipe/0, while QuicEndpointConfig.profile.alpn and the CLI profile command allow custom ALPN values. That mismatch can make generated profiles disagree with TLS negotiation.
🤖 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 `@src/quic/endpoint.rs` around lines 124 - 146, The rustls config builders are
hard-coding the ALPN value instead of using the profile setting, so update the
`rustls_server_config` and `rustls_client_config` logic to take the ALPN from
`QuicEndpointConfig.profile.alpn` (or the relevant profile object) and assign
that value to `alpn_protocols` in both places. Make sure the server and client
configs both use the same profile-derived ALPN so the CLI profile command and
TLS negotiation stay aligned.
| let iter = WalkDir::new(root).follow_links(false).into_iter(); | ||
| let entries: Vec<_> = iter | ||
| .filter_map(|entry| entry.ok()) | ||
| .filter(|entry| { | ||
| let rel = match entry.path().strip_prefix(root) { | ||
| Ok(p) => p.to_string_lossy().replace('\\', "/"), | ||
| Err(_) => return false, | ||
| }; | ||
| let rel_for_predicate = if rel.is_empty() { | ||
| String::new() | ||
| } else { | ||
| rel.clone() | ||
| }; | ||
| predicate.keep(&rel_for_predicate, entry.file_type().is_dir()) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files src/sync.rs
printf '\nLine count:\n'
wc -l src/sync.rs
printf '\nOutline:\n'
ast-grep outline src/sync.rs --view expanded || true
printf '\nRelevant slice:\n'
sed -n '1,220p' src/sync.rs | cat -nRepository: LOUST-PRO/SnapPipe
Length of output: 10560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Search for walkdir usage and SyncError:\n'
rg -n "WalkDir|filter_entry|walkdir::Error|SyncError|metadata\\(" src/sync.rs src -g '!target'
printf '\nRelevant dependency declarations:\n'
rg -n 'walkdir|filetime' Cargo.toml Cargo.lockRepository: LOUST-PRO/SnapPipe
Length of output: 2691
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files src/sync.rs
printf '\nLine count:\n'
wc -l src/sync.rs
printf '\nOutline:\n'
ast-grep outline src/sync.rs --view expanded || true
printf '\nRelevant slice:\n'
sed -n '1,220p' src/sync.rs | cat -n
printf '\nSearch for walkdir usage and SyncError:\n'
rg -n "WalkDir|filter_entry|walkdir::Error|SyncError|metadata\\(" src/sync.rs src -g '!target'
printf '\nRelevant dependency declarations:\n'
rg -n 'walkdir|filetime' Cargo.toml Cargo.lockRepository: LOUST-PRO/SnapPipe
Length of output: 13096
Preserve walk errors and prune rejected subtrees with filter_entry.
filter_map(|entry| entry.ok()) drops walk errors, and the trailing .filter(...) does not prevent WalkDir from descending into rejected directories. That can hide I/O failures and still traverse paths callers meant to exclude. Use filter_entry(...) and surface walkdir::Error as SyncError::Walk.
🤖 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 `@src/sync.rs` around lines 95 - 110, The WalkDir traversal in sync logic is
dropping `walkdir::Error` values and still descending into directories that
should be excluded. Update the `WalkDir::new(root)` pipeline in `sync.rs` to use
`filter_entry(...)` for pruning based on `predicate.keep(...)` instead of a
trailing `.filter(...)`, and preserve iterator errors by mapping any
`walkdir::Error` into `SyncError::Walk`. Keep the existing path normalization
behavior around `entry.path().strip_prefix(root)` and use the same
`predicate.keep` call site to locate the fix.
| entries.par_iter().try_for_each(|entry| -> Result<(), SyncError> { | ||
| if entry.file_type().is_dir() { | ||
| return Ok(()); | ||
| } | ||
| let path = entry.path(); | ||
| let rel = path | ||
| .strip_prefix(root) | ||
| .map_err(|_| SyncError::PathEscape(path.to_string_lossy().to_string()))? | ||
| .to_string_lossy() | ||
| .replace('\\', "/"); | ||
| if rel.is_empty() { | ||
| return Ok(()); | ||
| } | ||
| let entry_meta = std::fs::metadata(path).map_err(|err| SyncError::Walk { | ||
| path: path.to_path_buf(), | ||
| message: err.to_string(), | ||
| })?; | ||
| let record = build_entry(&rel, &entry_meta); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' src/sync.rs
printf '\n--- SEARCH ---\n'
rg -n "follow_links|symlink_metadata|is_symlink|metadata\\(" srcRepository: LOUST-PRO/SnapPipe
Length of output: 8048
🏁 Script executed:
sed -n '1,220p' src/sync.rs
printf '\n--- SEARCH ---\n'
rg -n "follow_links|symlink_metadata|is_symlink|metadata\\(" srcRepository: LOUST-PRO/SnapPipe
Length of output: 8048
🌐 Web query:
WalkDir follow_links(false) symlink entries metadata follows symlink docs
💡 Result:
When using the Rust walkdir crate with follow_links(false) (which is the default behavior), the iterator treats symbolic links as files themselves rather than following them to their targets [1][2]. In this configuration, the following rules apply to metadata and directory entries: 1. Metadata Access: The DirEntry metadata operations (such as file_type or metadata) will report information about the symbolic link itself by calling std::fs::symlink_metadata, not the target of the link [3][4]. 2. Path Reporting: The path method on the yielded DirEntry returns the path to the symbolic link itself [3][4]. 3. Identifying Symlinks: You can reliably determine if an entry is a symbolic link by using the path_is_symlink method, which is unaffected by the follow_links setting and always returns true if the entry was created from a symbolic link [3][4]. If you need to access the metadata of the target when follow_links is false, you must manually resolve the link using std::fs::read_link(entry.path) or similar methods [3][4]. When follow_links is set to true, the behavior changes such that DirEntry operations (except path) transparently operate on the link target instead of the link itself [3][4].
Citations:
- 1: https://docs.rs/walkdir/latest/walkdir/struct.WalkDir.html
- 2: https://docs.rs/walkdir/latest/src/walkdir/lib.rs.html
- 3: https://tikv.github.io/doc/walkdir/struct.DirEntry.html
- 4: https://docs.rs/walkdir/2.3.0/walkdir/struct.DirEntry.html
🌐 Web query:
WalkDir follow_links(false) symlink entries metadata follows symlink docs
💡 Result:
In the Rust walkdir crate, when follow_links(false) is configured (which is the default behavior), the library does not follow symbolic links [1][2]. Under this setting, metadata for symbolic link entries is retrieved using std::fs::symlink_metadata, meaning it describes the symbolic link itself rather than the target it points to [3]. However, there is a known time-of-check/time-of-use (TOCTOU) race condition in the crate (as of late 2025) where, if an attacker replaces a directory with a symlink between the library's initial check and the subsequent directory traversal, walkdir may inadvertently traverse into the symlink despite follow_links(false) [4]. Key details regarding behavior: - When follow_links is false, DirEntry operations (excluding path()) generally act upon the symbolic link itself [3]. - The path_is_symlink() method on a DirEntry provides an accurate check of whether the entry is a symbolic link, and this method is independent of the follow_links configuration [3]. - To resolve the target of a symlink when follow_links is disabled, you must use std::fs::read_link(entry.path()) [3]. - If you require the metadata of the target for a symlink entry when follow_links is false, you must manually call std::fs::metadata() on the target path resolved via read_link() [3].
Citations:
- 1: https://docs.rs/walkdir/latest/walkdir/struct.WalkDir.html
- 2: https://docs.rs/walkdir/latest/walkdir/
- 3: https://docs.rs/walkdir/2.3.0/walkdir/struct.DirEntry.html
- 4: TOCTOU race allows symlink traversal even when follow_links is disabled BurntSushi/walkdir#209
Skip symlink entries before metadata()
follow_links(false) still yields symlink entries, and std::fs::metadata(path) resolves them to the target. A symlink under the root can pull out-of-tree files into the manifest. Reject entry.file_type().is_symlink() (or use symlink_metadata and require a real file) in both branches before building FileEntry.
🤖 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 `@src/sync.rs` around lines 113 - 130, The sync walk in
`entries.par_iter().try_for_each` still processes symlink entries because
`metadata()` follows links, which can let out-of-tree targets into the manifest.
Update the `Sync` logic to reject `entry.file_type().is_symlink()` before
calling `std::fs::metadata(path)` in both the file and directory handling paths,
or switch to `symlink_metadata` and require only real files; keep the check near
the existing `file_type().is_dir()`, `strip_prefix`, and `build_entry` flow.
| pub fn load_or_default() -> Self { | ||
| match Self::default_path() { | ||
| Some(path) => Self::load_from_path(&path).unwrap_or_else(|_| Self { | ||
| inner: Arc::new(RwLock::new(HashMap::new())), | ||
| path: Some(path), | ||
| }), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't downgrade load failures to an empty trust store.
This path treats malformed files and real I/O failures the same as “no trust entries”. That turns a bad parse/permission issue into an empty in-memory registry, and the next save() will overwrite the on-disk file with empty contents.
🤖 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 `@src/trust.rs` around lines 84 - 89, The load_or_default path in
trust::load_or_default is masking real load errors by falling back to an empty
Trust store on any failure. Update the Self::load_from_path handling so only the
absence of a default path or an explicit “no file” case returns a fresh empty
store, while malformed files and I/O/permission errors are propagated or
otherwise surfaced instead of creating an empty in-memory registry. Keep the fix
localized to load_or_default and the related Self::load_from_path flow so save()
cannot silently overwrite a broken trust file.
| body.push_str("# SnapPipe trust store: one issuer per line as `node_id = \"name:rate\"`.\n"); | ||
| body.push_str("# Lines beginning with `#` and blank lines are ignored.\n"); | ||
| let mut sorted = entries; | ||
| sorted.sort_by(|a, b| a.0.as_str().cmp(b.0.as_str())); | ||
| for (node, entry) in &sorted { | ||
| body.push_str(node.as_str()); | ||
| body.push_str(" = \""); | ||
| body.push_str(&entry.display_name); | ||
| body.push(':'); | ||
| body.push_str(&entry.rate_limit_per_min.to_string()); | ||
| body.push_str("\"\n"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The persistence format cannot round-trip TrustEntry.
save() only writes "name:rate", while parse() reconstructs entries with added_at_unix = 0. The raw display_name is also written unescaped, so values containing :, " or newlines become unreadable or change meaning on reload. The persistent store is lossy today.
Also applies to: 225-247
🤖 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 `@src/trust.rs` around lines 130 - 140, The trust store persistence in `save()`
is lossy and cannot round-trip `TrustEntry`: it only serializes `display_name`
and `rate_limit_per_min`, while `parse()` reconstructs `added_at_unix` as 0 and
will misread unescaped names containing `:`, `"` or newlines. Update the
`save()`/`parse()` pair in `TrustStore` to serialize all `TrustEntry` fields
needed for lossless restore and to escape or encode string values safely, then
make the parser decode that format back into the original entry.
| let issuer_vk = trusted_issuer.verifying_key(); | ||
| let subject_vk = subject_key.verifying_key(); | ||
| let _server_task = tokio::spawn(async move { | ||
| let incoming = server.accept().await.expect("accept"); | ||
| let conn = incoming.await.expect("incoming await"); | ||
| let (send, recv) = conn.accept_bi().await.expect("accept_bi"); | ||
| server_handshake(send, recv, &issuer_vk, &subject_vk, trust, now + 300).await | ||
| }); | ||
|
|
||
| let connecting = client | ||
| .connect(server_addr, "localhost") | ||
| .expect("connect attempt"); | ||
| let client_conn = timeout(Duration::from_secs(5), connecting) | ||
| .await | ||
| .expect("connect timeout") | ||
| .expect("client connect"); | ||
| let result = client_handshake(&client_conn, &ticket).await; | ||
|
|
||
| let server_result = _server_task.await.expect("server join"); | ||
| assert!(result.is_err(), "client must fail when issuer is untrusted"); | ||
| assert!(server_result.is_err(), "server must reject untrusted issuer"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
This negative test never reaches the trust gate.
The server verifies with trusted_issuer.verifying_key(), but the ticket is signed by untrusted_issuer, so this fails as InvalidSignature before TrustCheck is consulted. Use the untrusted issuer's verifying key here and assert the trust-specific failure so this test covers the intended branch.
🤖 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 `@tests/quic_e2e.rs` around lines 206 - 226, The negative QUIC handshake test
is using the trusted issuer key in the server path, so it fails on signature
validation before reaching the trust check. In the server task that calls
server_handshake, switch to the untrusted issuer’s verifying key so the ticket
can validate and the TrustCheck branch is exercised, then update the assertions
in quic_e2e.rs to expect the trust-specific rejection rather than a generic
signature failure.
…ickets # Conflicts: # Cargo.toml
1 participant
Summary
src/quic.rswithsrc/quic/directory (endpoint.rs + mod.rs)QuicTransportProfilewith low_latency_interactive + relay_backhaul presetsTrustStorewith~/.config/snappipe/trust.tomlpersistenceRelaywithByteStreamtrait for testabilityNonceStore(TTL 60s, in-memory + hex persistence)RateLimiter(token bucket per NodeId, default 100 req/min)Repository URL corrected
repositoryfield in Cargo.toml:louzt/SnapPipe→LOUST-PRO/SnapPipeTests
Commits
0f5359ffeat(core): bootstrap identity and signed ticket foundationd9c6338feat(ticket): support issuer and subject identities93e9faafeat(quic): add Quinn transport profile foundation9a2d14efeat(cli): expose quic transport presets90ef3eefeat: data plane (quinn) + relay + trust + sync (v0.2.0)Scope
This is NOT in this PR:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests