fix(templates): reject path traversal, absolute paths, and symlinks during archive extraction - #766
Merged
Nanle-code merged 11 commits intoAug 31, 2026
Conversation
|
@TheWeirdDee Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Collaborator
|
@TheWeirdDee Please all failing CIs |
…uring archive extraction extract_zip_archive() (used by `registry install` and local `.zip` template sources) silently skipped entries with an absolute path or `..` parent traversal instead of rejecting the archive, and never checked for symlink entries at all. Treat any of these as reason to reject the whole archive rather than partially extracting an otherwise-malicious/corrupted package: - Absolute paths and parent-traversal components (already detected via zip::ZipFile::enclosed_name()) now produce a clear error instead of being silently dropped. - Symlink entries are now detected via the entry's Unix mode bits and rejected explicitly. - The existing zip-slip (resolves-outside-destination) check is kept as a final defense-in-depth layer. Adds tests for parent-traversal rejection, absolute-path rejection, a mixed archive that stops at the first malicious entry, and the symlink-mode classification helper, alongside the existing happy-path extraction test. Documents the behavior in REMOTE_REGISTRY_IMPLEMENTATION.md's Security section.
master currently fails cargo test/cargo clippy --all-targets for reasons entirely unrelated to this PR's actual change (archive extraction safety) — a grab-bag of struct-shape drift and one real missing feature, evidently left behind by several bad merges: - bindings.rs: read_spec_entries was defined twice (a #[cfg(test)] pub copy duplicating the real, unconditional private fn). Removed the duplicate; the original is already used by production code. - commands/audit.rs: `ci_passed` was specified twice in one AuditResult test literal. - plugins/manifest.rs: two PluginManifest test literals predated the required_capabilities field. - utils/compliance.rs: ComplianceSeverity was compared with `==` in a test but never derived PartialEq. - utils/ai.rs: a test called `cb.is_available()` (&mut self) on a non-mut binding. - utils/templates.rs, template_analytics.rs, template_recommender.rs: several TemplateEntry test literals predated the categories/ featured/repository_url/changelog/repository/security_review fields added since; also two spots comparing/constructing an Option<Vec<ChangelogEntry>> as a bare Vec. - plugins/registry.rs: this one wasn't just a stale test fixture. install_plugin() already took a `description: &str` parameter (per its own doc comment) but never stored it — InstalledPlugin had no description field at all, and the two helpers the tests expected (resolve_plugin_description, plugin_list_entries) didn't exist. Meanwhile `starforge plugin list`'s human-readable table hardcoded an empty string in the Description column. Added the field (with #[serde(default)] for old registry.json compatibility), wired install_plugin to store it, implemented both helpers (prefer the explicit description, fall back to the first command's), and switched both the JSON and table output in commands/plugin.rs to use them — fixing the always-blank Description column along the way. cargo test --lib --all-features --no-run and cargo clippy --all-features --locked -- -D warnings both now pass clean.
The rebase onto master dropped the now-redundant thiserror Cargo.toml addition (master already fixed the Migration trait signature this PR originally needed thiserror for, independently). Cargo.lock needs to match or --locked builds fail.
TheWeirdDee
force-pushed
the
fix/684-archive-path-traversal
branch
from
August 27, 2026 08:12
9b3a8a2 to
b1813f7
Compare
Same batch of pre-existing bugs already fixed on fix/651-scoped-lints
(cherry-picked from there) - these are all inherited from upstream
master and equally affect this branch:
- tests/ai_test_assistant.rs: two of the three generator helpers it
exercises (generate_edge_case_descriptions, generate_security_checks,
generate_warnings) had been relocated from utils::ai_test_assistant
into commands::ai_test as private fns, breaking the integration test.
Moved them back to utils::ai_test_assistant as pub fns and pointed
commands::ai_test at them instead of duplicating the logic. Also
added a missing .unwrap() after analyze_contract_for_testing started
returning a Result.
- tests/template_recommendation.rs: make_entry() predated the
categories/featured/repository_url fields on TemplateEntry.
- cargo fmt --all: fixed formatting drift.
- deny.toml: removed a stale RUSTSEC-2026-0190 ignore that no longer
matches any crate in the dependency tree.
- fuzz/Cargo.lock: regenerated; had drifted out of sync with
fuzz/Cargo.toml.
- .github/workflows/benchmark-latency.yml: cargo bench passed three
positional filters but Criterion's harness only accepts one -
combined into a regex alternation. Also wrapped the PR-comment step
in try/catch since fork PRs get a read-only GITHUB_TOKEN.
- templates/registry.json: SecurityReview.findings is Option<String>
but the bundled seed data has always shipped numeric values, so the
CLI's offline fallback path could never actually parse its own
bundled registry. Converted to match the type; added a regression
test.
- src/utils/database.rs: initialize() used
`get_meta("schema_version").is_ok()` to decide whether a database
was fresh, but Ok(None) is still Ok - broke every fresh install.
Changed to `.is_some()`.
- src/utils/test_optimizer.rs: batch_tests_by_profile computed the
non-IO-bound tests and then discarded them, batching an always-empty
placeholder instead - CPU/memory-bound/general tests were silently
dropped from every batch. save_state() didn't create config_dir
before writing into it.
- tests/contract_property_tests.rs: two tests with incorrect premises
(a byte-range that could never produce "short" input, and an
auth-recording assertion checked before anything was recorded).
…CI verification - tests/multisig_builder_ui.rs: update calls to match the current multisig_builder API (proposal_from_template now takes a network argument; render_progress_bar takes a &SignatureProgress computed via calculate_progress and returns a single formatted string, not a tuple). - tests/test_optimizer_integration.rs: TestOptimizer::history is a HashMap<String, TestHistory>, but call sites were passing the make_history() (String, TestHistory) tuple directly to insert(), which takes two separate arguments. Switched to extend([...]) which accepts (K, V) tuples directly. - src/utils/test_optimizer.rs: TestOptimizer::config_dir and ::cache were private, which the same integration test needs to construct the struct directly from outside the crate. Made them pub, consistent with the already-public history field. - tests/template_recommendation.rs: same findings: Some(0) -> None fix already applied on fix/666-hardware-wallet-ci. Same fixes as fix/666-hardware-wallet-ci, each verified there with isolated `cargo test --test <name> --no-run` compile checks.
Collaborator
|
@TheWeirdDee Please resolve conflicts!!!! |
8db592a merged an even newer upstream/master into this branch and reintroduced the exact same class of unresolved-duplicate-content bugs fixed in the previous commit on this branch (and in Nanle-code#759): every time this branch re-merges master, git auto-resolves overlapping-but-not- conflicting insertions by keeping both sides' content instead of one, with no conflict markers, so it silently breaks the build each time. Fixed, same root causes as before: - commands/plugin.rs: `source`/`description` fields specified twice in a PluginSummary struct literal, and plugin_list_entries() called twice into an immediately-shadowed, unused binding. - plugins/registry.rs: InstalledPlugin had `description` declared twice, and a stale duplicate resolve_plugin_description/plugin_list_entries pair shadowed the real implementation later in the file. - utils/ai_test_assistant.rs: generate_edge_case_descriptions, generate_security_checks, and generate_warnings were each defined twice (byte-identical bodies); kept one copy of each. - utils/templates.rs, utils/template_analytics.rs, utils/template_recommender.rs, tests/template_recommendation.rs: TemplateEntry test literals had changelog/repository/security_review/ categories/featured/repository_url/findings fields duplicated. - src/commands/audit.rs: AuditResult literal was missing `ci_passed`. - tests/bindings_tests.rs: all four test functions had both merge sides' bodies concatenated with a missing closing brace (this is what made Rustfmt fail outright before it could check anything else). - tests/multisig_builder_ui.rs: a `use` block had its import list duplicated with one item renamed between the two copies instead of merged. - .github/workflows/benchmark-latency.yml: two `catch` blocks chained onto one `try`, which is not valid JavaScript (failed Latency Budget Check specifically). New this time, a real bug rather than a Rust-source merge artifact: templates/registry.json (the bundled default template registry shipped in the binary) had "findings": null immediately followed by "findings": "<value>" in 9 of its SecurityReview objects — a duplicate JSON key from the same recurring merge pattern, but in data, not code. serde_json rejects it outright ("duplicate field `findings`"), which made default_registry_parses and load_registry_falls_back_to_bundled_default_when_remote_unreachable fail for real: any offline install or registry-fetch failure would have hit this and been unable to fall back to the bundled registry at all. Removed the stale `null` line, keeping the real value in each case. This PR's own change (extract_zip_archive's path-traversal/absolute- path/symlink rejection in utils/templates.rs, and its four tests) was untouched by any of this and continues to pass. Verified locally: cargo build, cargo check --all-features --tests, cargo fmt --all --check, and cargo clippy --all-features --locked -- -D warnings (CI's exact invocation) all pass. cargo test --lib templates:: passes 72/73 (this PR's four new tests plus extract_zip_archive_and_validate all included); the one local-only failure (test_publish_template_versioned_stores_by_version) is a Windows-only dirs::home_dir()-ignores-HOME artifact that does not reproduce on Linux CI. cargo test --test bindings_tests and --test multisig_builder_ui both pass in full (8/8, 4/4).
The same recurring merge-fallout pattern found in the Rust source (see
previous commit) also hit this workflow file: the `on:` trigger block
had two separate `schedule:` mapping keys (one added by each side of a
past master merge, never reconciled). Plain YAML parsers silently keep
the last one, but GitHub Actions' own workflow validator rejects the
duplicate key outright and fails the whole run in 0 seconds with no
job ever created ("This run likely failed because of a workflow file
issue") — this is why .github/workflows/fuzzing.yml showed as an
instant failure on this PR, separately from the Rust build/test/lint
checks fixed in the previous commit.
Merged both cron entries into a single `schedule:` list, which is
exactly what multiple schedules under one trigger looks like in
GitHub Actions syntax.
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
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.
Closes #684.
Objective
Reject absolute paths, parent traversal, links, and files outside the destination root when extracting a downloaded/local
.ziptemplate package.What was there before
templates::extract_zip_archive(src/utils/templates.rs) — used by bothregistry install(downloading a template archive from a remote registry) and local.ziptemplate sources — already had a partial zip-slip guard:enclosed_name()(from thezipcrate) already returnsNonefor entries with an absolute path or a..component that would escape the archive root — so those were being caught. But two things were missing:What changed
enclosed_name() == None) now make the whole extraction fail with a clear error naming the offending entry, instead of silently continuing to the next entry.entry.unix_mode()(masking forS_IFLNK), which also rejects the whole extraction with a clear error.out_pathresolves outsidedest) check as a final defense-in-depth layer, sinceenclosed_name()already guarantees no../absolute components make it that far — this check just makes the guarantee explicit rather than implicit.normalize_template_root→validate_template_structure) still passes unmodified.Tests added
extract_zip_archive_rejects_parent_traversal— an entry named../escaped.txtfails extraction with a clear error, and nothing is written outside the destination.extract_zip_archive_rejects_absolute_path— an entry named/etc/passwd-clonefails extraction with a clear error.extract_zip_archive_stops_at_first_malicious_entry— a mixed archive (one legitimate file, one traversal entry) fails as a whole rather than partially extracting the legitimate file (boundary case: not a single-bad-entry archive).is_symlink_mode_detects_symlink_and_ignores_other_types— unit test for the Unix-mode classification helper (regular file / directory / symlink bit patterns). I couldn't construct an actual symlink zip entry through the safezip::write::ZipWriterAPI for an end-to-end test —FileOptions::unix_permissions()masks off the file-type bits by design (zip0.6.6 has its own test,unix_permissions_bitmask, confirming this), andstart_file()always forces theS_IFREGbit. So the classification logic is unit-tested directly against synthetic mode values instead, which is what's actually security-relevant here (the bit-masking check), independent of how a real symlink entry would reach it.Documentation
Added an "Archive Extraction (Client-Side)" subsection to
REMOTE_REGISTRY_IMPLEMENTATION.md's existing## Securitysection, describing exactly what gets rejected and why (compatibility/security note per the issue's acceptance criteria).Testing performed
cargo test --lib templates::currently cannot complete onmasterin this repo — building the full test binary hits ~20+ pre-existing, unrelated compile errors elsewhere in the tree (missing struct fields onTemplateEntryintemplate_recommender.rs/template_analytics.rs/templates.rs's own older fixtures, a type mismatch onchangelog, a mutable-borrow issue inai.rs, etc. — see #759 for the same issue surfacing on a different feature). None of that is caused by or related to this change, and I did not touch any of those files.To validate this specific fix independent of that, I copied
extract_zip_archive+is_symlink_modeand the new tests into an isolated scratch crate (dependencies:anyhow,zip = "0.6",tempfile, matching what's pinned here) and ran them there:cargo fmt --checkon the touched file is clean.