Fix package boundary edge cases#855
Conversation
Strix Security ReviewNo security issues found. Updated for Reviewed by Strix |
📝 WalkthroughWalkthroughThis PR fixes six security/robustness issues: FFI source path escape via ChangesFFI Source Path Escape Prevention
External Package Import Path Escape & Alias Fixes
Process Argv NUL-Byte Validation
Windows Reserved Package Name Rejection
Version and Changelog Updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ProcessRun as process.run
participant ValidateArgs as validate_args
participant Runtime as raven_process_run
Caller->>ProcessRun: run(program, args)
ProcessRun->>ValidateArgs: validate_args(args)
ValidateArgs-->>ProcessRun: Err if NUL byte found
ProcessRun-->>Caller: Err(process error)
ValidateArgs-->>ProcessRun: Ok(true)
ProcessRun->>Runtime: raven_process_run(program, args)
Runtime-->>Caller: process output
sequenceDiagram
participant Builder as rvpm build
participant Ops as collect_ffi
participant Checker as checked_ffi_source
participant FS as filesystem
Builder->>Ops: gather_native_link([ffi].sources)
Ops->>Checker: checked_ffi_source(base, source)
Checker->>Checker: reject absolute/.. /root paths
Checker->>FS: canonicalize(candidate)
FS-->>Checker: canonical path
Checker->>Checker: verify path within canonical base
Checker-->>Ops: PathBuf or UnsafeFfiSource error
Ops-->>Builder: validated sources or build failure
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
src/manifest/mod.rs (1)
384-388: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for
com0/lpt0and the other single-word reserved names.The new tests only cover
con,COM1,lpt9,com10, andconsole. Given the COM0/LPT0 ambiguity noted above, and thatprn/aux/nularen't exercised at all, expanding coverage would catch regressions here.🤖 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/manifest/mod.rs` around lines 384 - 388, Expand the `is_valid_package_name` test coverage in the manifest module to include the missing reserved-name cases. Add assertions for `com0` and `lpt0` to cover the COM0/LPT0 ambiguity, and also exercise the other single-word reserved names like `prn`, `aux`, and `nul` alongside the existing `con`, `COM1`, `lpt9`, `com10`, and `console` checks.
🤖 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 `@src/manifest/mod.rs`:
- Around line 42-49: The `is_windows_reserved_package_name` helper still allows
`com0` and `lpt0`, so update that reserved-name check to reject those cases as
well. In `src/manifest/mod.rs`, adjust the logic in
`is_windows_reserved_package_name` so the `com`/`lpt` branch treats `0` as
reserved instead of excluding it, while keeping the existing checks for `con`,
`prn`, `aux`, and `nul`.
In `@src/ops/mod.rs`:
- Around line 320-341: The checked_ffi_source logic validates candidate_canon
against the package root but still returns the non-canonical candidate, which
lets later callers like collect_ffi and compile_c_sources use a path that can be
swapped via symlink. Update checked_ffi_source in ops::mod to return the
canonicalized candidate_canon after the safety check, matching the behavior of
checked_package_path in resolve::stdlib, and keep the existing UnsafeFfiSource
and Io error handling intact.
In `@src/resolve/stdlib.rs`:
- Around line 193-207: The checked_package_path helper returns the raw candidate
path when candidate.exists() is false, which can race later reads in the stdlib
resolution flow. Update checked_package_path to avoid handing back an unchecked
path: either canonicalize and verify the path before returning it, or defer the
root validation to the actual read path that calls read_to_string so the final
path is always confirmed under package_root.
---
Nitpick comments:
In `@src/manifest/mod.rs`:
- Around line 384-388: Expand the `is_valid_package_name` test coverage in the
manifest module to include the missing reserved-name cases. Add assertions for
`com0` and `lpt0` to cover the COM0/LPT0 ambiguity, and also exercise the other
single-word reserved names like `prn`, `aux`, and `nul` alongside the existing
`con`, `COM1`, `lpt9`, `com10`, and `console` checks.
🪄 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: 0286aba7-4d03-47a1-bde6-26eae3d63fe5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
CHANGELOG.mdCargo.tomlexamples/v2/proc_nul_arg_parent.rvexamples/v2/stdlib_robustness.rvsrc/bin/rvpm.rssrc/manifest/init.rssrc/manifest/mod.rssrc/ops/mod.rssrc/resolve/stdlib.rsstdlib/std/process.rvtests/codegen_smoke.rs
| fn is_windows_reserved_package_name(name: &str) -> bool { | ||
| let lower = name.to_ascii_lowercase(); | ||
| matches!(lower.as_str(), "con" | "prn" | "aux" | "nul") | ||
| || (lower.len() == 4 | ||
| && (lower.starts_with("com") || lower.starts_with("lpt")) | ||
| && lower.as_bytes()[3].is_ascii_digit() | ||
| && lower.as_bytes()[3] != b'0') | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- src/manifest/mod.rs around target lines ---\n'
sed -n '1,120p' src/manifest/mod.rs
printf '\n--- search for reserved package name logic and related tests ---\n'
rg -n "is_windows_reserved_package_name|COM0|LPT0|reserved crate|reserved package|issue `#851`|windows reserved" -S .Repository: martian56/raven
Length of output: 244
🌐 Web query:
Windows reserved device names COM0 LPT0 reserved Microsoft docs
💡 Result:
In Windows, COM0 and LPT0 are considered reserved device names and are generally treated as invalid file names [1][2]. While older or incomplete documentation sometimes omitted them, these names are restricted by the system in the same manner as other legacy device names like COM1–COM9 and LPT1–LPT9 [3][4][1]. Attempting to create a file or directory with these names will typically result in an error (e.g., "The specified device name is invalid") because the Windows API reserves them globally for device communication [2]. This reservation is a legacy holdover intended to prevent name collisions with system devices [1]. Consequently, developers are strongly advised against using these names for files or directories [3][4]. Note that this restriction applies to the Win32 namespace [3][4]. Accessing these devices or bypassing the restriction is possible using specialized techniques, such as prepending the path with \.\ (the device namespace) or using native NT APIs, which do not enforce these legacy DOS-era naming conventions [4][5][6].
Citations:
- 1: https://www.parasymmetric.com/2025/05/25/windows-reserved-file-names/
- 2: windows: COM0 and LPT0 are also invalid on Windows 7? gohugoio/hugo#2883
- 3: https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file
- 4: https://learn.microsoft.com/en-gb/windows/win32/fileio/naming-a-file
- 5: https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/introduction-to-ms-dos-device-names
- 6: https://superuser.com/questions/1929654/why-can-i-create-a-file-with-a-name-containing-a-reserved-device-name-in-windows
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/manifest/mod.rs around target lines ---'
sed -n '1,120p' src/manifest/mod.rs
echo
echo '--- search for reserved package name logic and related tests ---'
rg -n "is_windows_reserved_package_name|COM0|LPT0|reserved crate|reserved package|issue `#851`|windows reserved" -S .Repository: martian56/raven
Length of output: 4902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- locate src/manifest/mod.rs ---'
git ls-files | rg '^src/manifest/mod\.rs$'
echo
echo '--- show file length ---'
wc -l src/manifest/mod.rs
echo
echo '--- targeted excerpt ---'
sed -n '30,70p' src/manifest/mod.rs
echo
echo '--- search references ---'
rg -n "is_windows_reserved_package_name|COM0|LPT0|reserved crate|reserved package|issue `#851`|windows reserved" -S src tests .Repository: martian56/raven
Length of output: 2294
Reject com0/lpt0 here is_windows_reserved_package_name still permits those Windows device names; remove the != b'0' exception so package names that map to reserved binaries are rejected too.
🤖 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/manifest/mod.rs` around lines 42 - 49, The
`is_windows_reserved_package_name` helper still allows `com0` and `lpt0`, so
update that reserved-name check to reject those cases as well. In
`src/manifest/mod.rs`, adjust the logic in `is_windows_reserved_package_name` so
the `com`/`lpt` branch treats `0` as reserved instead of excluding it, while
keeping the existing checks for `con`, `prn`, `aux`, and `nul`.
| let candidate = base.join(rel); | ||
| if candidate.exists() { | ||
| let base_canon = base.canonicalize().map_err(|source| OpError::Io { | ||
| action: "canonicalize package root".to_string(), | ||
| path: base.to_path_buf(), | ||
| source, | ||
| })?; | ||
| let candidate_canon = candidate.canonicalize().map_err(|source| OpError::Io { | ||
| action: "canonicalize ffi source".to_string(), | ||
| path: candidate.clone(), | ||
| source, | ||
| })?; | ||
| if !candidate_canon.starts_with(&base_canon) { | ||
| return Err(OpError::UnsafeFfiSource { | ||
| base: base.to_path_buf(), | ||
| source: source.to_string(), | ||
| }); | ||
| } | ||
| return Ok(candidate); | ||
| } | ||
|
|
||
| Ok(candidate) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant implementation and nearby context.
sed -n '280,380p' src/ops/mod.rs
printf '\n==== stdlib resolver ====\n'
sed -n '1,220p' src/resolve/stdlib.rs
printf '\n==== search for compile_c_sources and path usage ====\n'
rg -n "compile_c_sources|checked_package_path|UnsafeFfiSource|canonicalize ffi source|ends_with\\(\"c/native.c\"\\)" src testsRepository: martian56/raven
Length of output: 15112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '==== linker::compile_c_sources ====\n'
sed -n '150,260p' src/codegen/linker.rs
printf '\n==== stdlib resolver callers ====\n'
sed -n '220,270p' src/resolve/stdlib.rs
printf '\n==== ops tests around checked_ffi_source ====\n'
sed -n '1100,1165p' src/ops/mod.rsRepository: martian56/raven
Length of output: 9390
Return the canonicalized FFI source path. checked_ffi_source validates candidate_canon but returns candidate; collect_ffi then passes that path to compile_c_sources, so a symlink swap can still redirect the compiler outside base. Return candidate_canon here, like checked_package_path does in src/resolve/stdlib.rs.
🤖 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/ops/mod.rs` around lines 320 - 341, The checked_ffi_source logic
validates candidate_canon against the package root but still returns the
non-canonical candidate, which lets later callers like collect_ffi and
compile_c_sources use a path that can be swapped via symlink. Update
checked_ffi_source in ops::mod to return the canonicalized candidate_canon after
the safety check, matching the behavior of checked_package_path in
resolve::stdlib, and keep the existing UnsafeFfiSource and Io error handling
intact.
| fn checked_package_path(&self, source: &str, candidate: PathBuf) -> Option<PathBuf> { | ||
| if !candidate.exists() { | ||
| return Some(candidate); | ||
| } | ||
|
|
||
| let package_dir = self.package_dir(source)?; | ||
| let cache_root = self.cache_root.canonicalize().ok()?; | ||
| let package_root = package_dir.canonicalize().ok()?; | ||
| if !package_root.starts_with(&cache_root) { | ||
| return None; | ||
| } | ||
|
|
||
| let resolved = candidate.canonicalize().ok()?; | ||
| resolved.starts_with(&package_root).then_some(resolved) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers of checked_package_path-backed resolvers and how their results are consumed.
rg -nP -C3 '\b(external_source_path|external_local_source_path|package_manifest_path)\s*\(' src/
# Confirm reads go through the returned path only.
rg -nP -C2 'read_to_string' src/resolve/stdlib.rsRepository: martian56/raven
Length of output: 14783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the resolver functions and the two call sites that consume their outputs.
sed -n '190,260p' src/resolve/stdlib.rs
printf '\n---\n'
sed -n '1030,1185p' src/resolve/stdlib.rs
# Inspect the package-boundary tests in this area.
printf '\n=== tests ===\n'
sed -n '2160,2265p' src/resolve/stdlib.rsRepository: martian56/raven
Length of output: 13800
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any direct use of the returned paths outside stdlib.rs.
rg -n 'external_source_path\(|external_local_source_path\(|package_manifest_path\(' srcRepository: martian56/raven
Length of output: 3271
Avoid returning the raw missing path here src/resolve/stdlib.rs:193-207
candidate.exists() races the later read_to_string call sites. If a file or symlink is created after this check, the unchecked candidate can be read outside the package root. Re-check at read time or canonicalize before returning.
🤖 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/resolve/stdlib.rs` around lines 193 - 207, The checked_package_path
helper returns the raw candidate path when candidate.exists() is false, which
can race later reads in the stdlib resolution flow. Update checked_package_path
to avoid handing back an unchecked path: either canonicalize and verify the path
before returning it, or defer the root validation to the actual read path that
calls read_to_string so the final path is always confirmed under package_root.
1 participant
Summary
[ffi].sourcesand cached external package source reads.std/processargv before calling the runtime, and reject Windows reserved package names.Fixed Issues
Closes #848
Closes #849
Closes #850
Closes #851
Closes #852
Closes #853
Closes #854
Excluded by request: #214, #215, #224.
Validation
cargo test -p raven --libcargo test -p raven --test codegen_smoke proc_argv_rejects_nul_argcargo test -p raven --test rvpm_buildcargo test --workspaceSummary by CodeRabbit
[ffi].sourcesso paths must stay within the package root.