Harden package publishing and registry credentials - #631
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (12)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Hardens wflpkg registry auth and package publishing by binding credentials to a canonical HTTPS origin, producing archives from a private immutable snapshot, and enforcing ignore/FS/resource safety constraints throughout publish + verification.
Changes:
- Add registry-origin–scoped credentials storage (atomic/private writes) and
login [registry]support with safer logout recovery. - Create publish archives as private, create-new snapshots outside the project and derive a versioned
wflhash:v2:checksum from the completed archive. - Honor root + nested Git-compatible
.gitignorerules, reject unsafe patterns/filesystem objects, and add request/response size ceilings with streaming uploads.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Update CLI help + pass optional registry argument to wfl login. |
| fuzz/Cargo.lock | Lockfile updates for new deps used by hardened publishing/auth paths. |
| crates/wflpkg/src/registry/auth.rs | Registry-scoped credentials, bounded/no-follow reads, atomic/private writes, logout recovery helpers. |
| crates/wflpkg/src/registry/api.rs | Stream publish uploads with size ceilings; bound registry response bodies. |
| crates/wflpkg/src/package_files.rs | New .gitignore stack supporting nested rules with safety budgets and fail-closed parsing. |
| crates/wflpkg/src/main.rs | Add login [registry] CLI parsing + tests for the helper. |
| crates/wflpkg/src/lib.rs | Wire in new package_files module and exclusion list used by archive/checksum. |
| crates/wflpkg/src/commands/share.rs | Publish from private temp archive, validate entry/manifest safely, bind token origin to registry origin, checksum-from-archive. |
| crates/wflpkg/src/commands/login.rs | Enforce canonical registry origin for login; add logout behavior that can recover malformed auth. |
| crates/wflpkg/src/checksum.rs | Introduce wflhash:v2: transcript; checksum-from-archive; verify hashes all regular files; portable paths. |
| crates/wflpkg/src/archive.rs | Archive creation: create-new private outputs, ignore rules, traversal budgets, special-file rejection, and race-resistant reads. |
| crates/wflpkg/Cargo.toml | Add deps needed for ignore handling, tempfile usage, and streaming uploads. |
| CHANGELOG.md | Document security hardening behaviors and new CLI semantics. |
| Cargo.lock | Workspace lock updates for new deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for line in content.lines() { | ||
| reject_unsupported_git_classes(line)?; | ||
| builder | ||
| .add_line(Some(ignore_path.clone()), line) | ||
| .map_err(ignore_pattern_error)?; | ||
| } |
| let file = tokio::fs::File::open(archive_path) | ||
| .await | ||
| .map_err(PackageError::Io)?; |
| pub fn clear_token(&self) -> Result<(), PackageError> { | ||
| if self.auth_file.exists() { | ||
| std::fs::remove_file(&self.auth_file)?; | ||
| match std::fs::symlink_metadata(&self.auth_file) { | ||
| Ok(_) => std::fs::remove_file(&self.auth_file)?, | ||
| Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} | ||
| Err(error) => return Err(PackageError::Io(error)), |
| // Skip symlinks to avoid following links outside the project | ||
| if ft.is_symlink() { | ||
| continue; | ||
| } |
| base: &Path, | ||
| path: &Path, | ||
| hasher: &mut Sha256, | ||
| ) -> Result<(), PackageError> { | ||
| for entry in sorted_entries(path)? { | ||
| let entry_path = entry.path(); | ||
| let file_type = entry.file_type()?; | ||
| if file_type.is_symlink() || (!file_type.is_dir() && !file_type.is_file()) { | ||
| return Err(unsupported_object(&entry_path)); | ||
| } | ||
| if file_type.is_dir() { | ||
| hash_verified_directory(base, &entry_path, hasher)?; | ||
| } else { | ||
| let content = std::fs::read(&entry_path)?; | ||
| let relative = entry_path.strip_prefix(base).unwrap_or(&entry_path); | ||
| let rel_bytes = relative.to_string_lossy(); | ||
| hasher.update((rel_bytes.len() as u64).to_le_bytes()); | ||
| hasher.update(rel_bytes.as_bytes()); | ||
| hasher.update(&content); | ||
| hash_file(base, &entry_path, hasher)?; | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🔍 verify_checksum recursion is unbounded (no depth/entry budget)
hash_verified_directory (crates/wflpkg/src/checksum.rs:152-169) and verify_checksum recurse without the MAX_PACKAGE_DEPTH/traversal budget that archive creation enforces (crates/wflpkg/src/archive.rs:68-70). Archive extraction (extract_archive) also imposes no depth limit, so a maliciously crafted .wflpkg with very deep nesting could be extracted and then overflow the stack during verification. Currently verify_checksum is only referenced from tests (no production install path calls it yet), so this is not presently reachable — but when the install/verify path in the complementary PR (#630) starts calling it on downloaded packages, an attacker-controlled archive could trigger a stack-overflow DoS. Recommend adding a depth bound to verification/extraction before wiring it into the download path.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
wfl login [registry], recover malformed auth via logout, and write credentials atomically with private permissions.gitignorerules while failing closed on unsupported patterns, symlinks, special files, unsafe manifests/entry points, and traversal budgetswflhash:v2:checksum from the completed immutable archive and make installed-package verification hash every regular fileSecurity impact
A project-controlled registry setting could send the user's saved bearer token to another host. Publishing also created its archive inside the untrusted project, followed an existing output path, ignored
.gitignore(which could upload.env, logs, and debug reports), and checksummed a separately reread live tree with an ambiguous transcript. These paths now bind credentials to the authenticated origin, package an immutable private snapshot without following symlinks, apply Git-compatible exclusions and resource ceilings, and verify a versioned checksum over the exact archive contents.Validation
cargo test -p wflpkg: 238 tests passedcargo fmt --all -- --checkpassedgit diff --checkpassedwflpkgtargets; the remainingfield_reassign_with_defaultwarning is pre-existing inversion_and_lockfile_tests.rsComplementary package removal/cache/extraction confinement is isolated in #630. Part of the Rust-source production-readiness work tracked in #610.