Skip to content

Commit 03fdd9f

Browse files
feat(bridge): split phantom into phantom-declared vs phantom-transitive (#76)
## Summary The Track E sweep across 29 Cargo CVE issues revealed that the previous bridge classifier marked **any** crate without a direct `use <crate>` statement as `reach=phantom`, conflating two very different remediation paths: - **True phantom (`PhantomDeclared`)** — crate IS in a `Cargo.toml` `[dependencies]` / `[workspace.dependencies]` block but never imported. Fix: `cargo machete --fix` strips the dep and the whole subtree drops from `Cargo.lock`. **file-soup#50** is the canonical case. - **Misclassified (`PhantomTransitive`)** — crate is NOT in any `Cargo.toml`; pulled in transitively by a parent. Local strip is impossible; the fix requires bumping the parent past the affected version. Of 28 non-pilot Track E repos, **~26 fell here** (Batch A 7/7 misclassified, Batch B almost certainly similar, Cohort E-2 4/5 misclassified per #75). ## What changed - **Enum split**: `ReachabilityStatus::Phantom` → `PhantomDeclared` / `PhantomTransitive`. JSON wire values (serde `rename_all = kebab-case`) are now `phantom-declared` / `phantom-transitive`. - **Evidence shape**: `ReachabilityEvidence` gains optional `parent_dep` (populated only when transitive AND identifiable via Cargo.lock). - **Reachability scan**: new `check_reachability_with_manifest(project_dir, crate, declared_deps, parent_map)` returns the correct variant. Lookup is normalised (underscore↔hyphen, lowercase) so a CVE feed reporting `serde_json` matches a manifest line `serde-json`. - **Parent identification**: `lockfile::collect_cargo_parents` parses `Cargo.lock`'s `dependencies = [...]` arrays and BFS-walks from each direct dep, attributing every reachable transitive (first BFS to reach wins). Best-effort — returns empty map on missing/unparseable lockfile, and `classify` falls back to "an upstream parent dependency" phrasing. - **Classifier**: `classify::classify` no longer takes `is_direct` — the evidence's status variant already carries that info. Three-way output (Mitigable / Unmitigable / Informational) is unchanged. The rationale and fix-action diverge: - `PhantomDeclared`: *"Strip from Cargo.toml — run `cargo machete --fix` (or remove the dependency line manually) for `<crate>`."* - `PhantomTransitive`: *"Pulled in transitively by `<parent>` — fix requires bumping the parent dependency past the affected version of `<crate>`. No local strip is possible."* - **Schema bump**: `BridgeReport::schema_version` 0.1.0 → 0.2.0 to flag the reach-field semantics change. - **No new deps**: reuses the existing in-house TOML / Cargo.lock string parsers (consistent with `collect_direct_cargo_dependencies`). ## Test coverage 13 new tests across the three bridge modules: **`bridge::reachability`** (6 new): - `phantom_declared_when_crate_in_cargo_toml_but_no_use` - `phantom_transitive_when_crate_not_in_cargo_toml` (parent identified) - `reachable_when_declared_and_used` - `phantom_transitive_with_unknown_parent_is_still_classified` - `phantom_classification_normalises_underscore_to_hyphen` - `phantom_declared_resolves_workspace_member_dep` (workspace-member declared dep) **`bridge::lockfile`** (3 new): - `parent_map_identifies_direct_to_transitive_path` - `parent_map_returns_empty_when_no_lockfile` - `parent_map_normalises_underscore_to_hyphen` **`bridge::classify`** (4 new, rewritten old ones): - `test_phantom_declared_recommends_machete_strip` - `test_phantom_transitive_recommends_parent_bump` - `test_phantom_transitive_unknown_parent_falls_back_gracefully` - `test_phantom_variants_both_classify_informational` All 38 bridge-module tests pass; full suite (327 lib + integration) green; `cargo check` clean on default features (bridge is feature-gated behind `http`). ## Files touched | File | +/- | |---|---| | `src/bridge/mod.rs` | +35 / -14 | | `src/bridge/reachability.rs` | +213 / -18 | | `src/bridge/classify.rs` | +109 / -52 | | `src/bridge/lockfile.rs` | +244 / -7 | ## Related work - panic-attack#74 — vendored-pin allowlist (Cohort E-3). With this PR, the classifier no longer falsely recommends stripping vendored-TLS / build-script-only crates, because those would only be in scope when they're `PhantomDeclared` (i.e., genuinely in `Cargo.toml`). The allowlist still needs to ride on top to handle e.g. `openssl-sys = { features = ["vendored"] }`. - panic-attack#75 — Dioxus/GTK informational (Cohort E-2). With parent-dep identification, the genuine presswerk case will now be labelled with `parent_dep: "dioxus"` (or `wry`, whichever the BFS reaches first). - file-soup#50 — canonical `PhantomDeclared` case. Closes #32 (Track C/E sweep parent). ## Test plan - [x] `cargo check --features http` clean - [x] `cargo check` (default features) clean - [x] `cargo test --features http --lib bridge::` — 38 passed - [x] `cargo test --features http` — full suite green (327 + integration) - [ ] Re-run Track E sweep against ~5 sample repos and confirm previously-`phantom` findings now split correctly into declared/transitive cohorts (post-merge validation) Co-authored-by: hyperpolymath <hyperpolymath@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1811b7b commit 03fdd9f

4 files changed

Lines changed: 601 additions & 73 deletions

File tree

src/bridge/classify.rs

Lines changed: 104 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -16,52 +16,63 @@ use super::{Classification, ReachabilityEvidence, ReachabilityStatus, Vulnerabil
1616

1717
/// Classify a vulnerability given its reachability evidence.
1818
///
19-
/// `is_direct` indicates whether the vulnerable package appears as a key in
20-
/// the project's `Cargo.toml` dependency tables. Phantom + transitive is
21-
/// common (a vulnerable crate pulled in through the dep graph but never
22-
/// imported in this project's source) and warrants a different remediation
23-
/// path than phantom + direct (where the manifest entry is genuinely
24-
/// unused). See #47.
19+
/// The three-way output (Mitigable / Unmitigable / Informational) is
20+
/// unchanged from #47 — both phantom variants classify as `Informational` —
21+
/// but the rationale and suggested fix differ:
22+
///
23+
/// - [`ReachabilityStatus::PhantomDeclared`] — declared in Cargo.toml but
24+
/// no `use` site. Strip the dep via `cargo machete --fix`. Canonical
25+
/// case: file-soup#50.
26+
/// - [`ReachabilityStatus::PhantomTransitive`] — not declared anywhere in
27+
/// the workspace, pulled in by a parent dep. Local strip is impossible;
28+
/// the fix requires bumping the parent (named in `evidence.parent_dep`
29+
/// when identifiable). Track E found ~26 of 29 issues here.
2530
///
2631
/// Returns (classification, rationale, suggested_action).
2732
pub fn classify(
2833
vuln: &Vulnerability,
2934
evidence: &ReachabilityEvidence,
30-
is_direct: bool,
3135
) -> (Classification, String, String) {
3236
match evidence.status {
33-
// ─── Phantom dependency: declared but never imported ───
34-
ReachabilityStatus::Phantom if is_direct => (
37+
// ─── Phantom + declared: manifest entry is genuinely unused ───
38+
ReachabilityStatus::PhantomDeclared => (
3539
Classification::Informational,
3640
format!(
3741
"{} {} is declared in Cargo.toml but never imported in any .rs file. \
3842
The vulnerable code is compiled but unreachable. \
39-
Removing the dependency from Cargo.toml eliminates this CVE entirely.",
43+
Stripping the dependency eliminates this CVE entirely.",
4044
vuln.package, vuln.version
4145
),
4246
format!(
43-
"Remove unused dependency `{}` from Cargo.toml",
47+
"Strip from Cargo.toml — run `cargo machete --fix` (or remove the \
48+
dependency line manually) for `{}`.",
4449
vuln.package
4550
),
4651
),
4752

48-
// ─── Phantom + transitive: pulled in by an upstream, not declared here ───
49-
ReachabilityStatus::Phantom => (
50-
Classification::Informational,
51-
format!(
52-
"{} {} is a transitive dependency (not declared in this project's Cargo.toml) \
53-
and never imported in any .rs file. The vulnerable code is compiled but \
54-
unreachable from this project. The CVE rides along through the dependency \
55-
graph; remediation is upstream, not local.",
56-
vuln.package, vuln.version
57-
),
58-
format!(
59-
"Transitive — run `cargo update -p {}` to pull a non-vulnerable version if \
60-
one is published, or upgrade the upstream crate that pulls it in. \
61-
Otherwise informational: code unreachable from this project.",
62-
vuln.package
63-
),
64-
),
53+
// ─── Phantom + transitive: pulled in by an upstream parent ───
54+
ReachabilityStatus::PhantomTransitive => {
55+
let parent_clause = match evidence.parent_dep.as_deref() {
56+
Some(p) => format!("`{p}`"),
57+
None => "an upstream parent dependency".to_string(),
58+
};
59+
(
60+
Classification::Informational,
61+
format!(
62+
"{} {} is a transitive dependency (not declared in this project's \
63+
Cargo.toml) and never imported in any .rs file. Pulled in by \
64+
{parent_clause}. The vulnerable code is compiled but unreachable \
65+
from this project; remediation lives upstream.",
66+
vuln.package, vuln.version
67+
),
68+
format!(
69+
"Pulled in transitively by {parent_clause} — fix requires bumping \
70+
the parent dependency past the affected version of `{}`. No local \
71+
strip is possible.",
72+
vuln.package
73+
),
74+
)
75+
}
6576

6677
// ─── Unreachable: imported but no taint flow (Phase 2) ───
6778
ReachabilityStatus::Unreachable => (
@@ -183,11 +194,21 @@ mod tests {
183194
}
184195
}
185196

186-
fn phantom_evidence() -> ReachabilityEvidence {
197+
fn phantom_declared_evidence() -> ReachabilityEvidence {
198+
ReachabilityEvidence {
199+
is_imported: false,
200+
import_sites: vec![],
201+
status: ReachabilityStatus::PhantomDeclared,
202+
parent_dep: None,
203+
}
204+
}
205+
206+
fn phantom_transitive_evidence(parent: Option<&str>) -> ReachabilityEvidence {
187207
ReachabilityEvidence {
188208
is_imported: false,
189209
import_sites: vec![],
190-
status: ReachabilityStatus::Phantom,
210+
status: ReachabilityStatus::PhantomTransitive,
211+
parent_dep: parent.map(str::to_string),
191212
}
192213
}
193214

@@ -200,70 +221,100 @@ mod tests {
200221
statement: "use test_crate::Thing;".to_string(),
201222
}],
202223
status: ReachabilityStatus::Reachable,
224+
parent_dep: None,
203225
}
204226
}
205227

206228
#[test]
207-
fn test_phantom_direct_recommends_removal() {
208-
let (cls, _, action) = classify(&mock_vuln(false, false), &phantom_evidence(), true);
229+
fn test_phantom_declared_recommends_machete_strip() {
230+
// file-soup#50 shape: crate declared in Cargo.toml, no `use` site —
231+
// strip the manifest entry.
232+
let (cls, rationale, action) = classify(&mock_vuln(false, false), &phantom_declared_evidence());
209233
assert_eq!(cls, Classification::Informational);
210234
assert!(
211-
action.contains("Remove unused dependency"),
212-
"direct phantom should recommend removal, got: {action}"
235+
action.contains("cargo machete --fix") || action.contains("Strip from Cargo.toml"),
236+
"declared phantom should recommend strip, got: {action}"
237+
);
238+
assert!(
239+
rationale.contains("declared in Cargo.toml"),
240+
"rationale should explain declared status, got: {rationale}"
213241
);
214242
}
215243

216244
#[test]
217-
fn test_phantom_transitive_recommends_cargo_update() {
218-
// Regression for #47: phantom-classified transitive deps were
219-
// incorrectly told to "Remove unused dependency from Cargo.toml".
220-
let (cls, rationale, action) =
221-
classify(&mock_vuln(false, false), &phantom_evidence(), false);
245+
fn test_phantom_transitive_recommends_parent_bump() {
246+
// Track E shape: ~26 of 29 issues were misclassified as phantom-declared
247+
// when they're actually transitive. The fix is to bump the parent, NOT
248+
// to strip the local manifest.
249+
let (cls, rationale, action) = classify(
250+
&mock_vuln(false, false),
251+
&phantom_transitive_evidence(Some("reqwest")),
252+
);
222253
assert_eq!(cls, Classification::Informational);
223254
assert!(
224-
!action.contains("Remove unused dependency"),
225-
"transitive phantom must NOT recommend manifest removal, got: {action}"
255+
!action.contains("cargo machete --fix"),
256+
"transitive phantom must NOT recommend manifest strip, got: {action}"
226257
);
227258
assert!(
228-
action.contains("Transitive"),
259+
action.contains("Pulled in transitively"),
229260
"transitive phantom action should label itself transitive, got: {action}"
230261
);
231262
assert!(
232-
action.contains("cargo update"),
233-
"transitive phantom action should suggest cargo update, got: {action}"
263+
action.contains("`reqwest`"),
264+
"transitive phantom action should name the parent dep, got: {action}"
265+
);
266+
assert!(
267+
action.contains("bumping the parent"),
268+
"transitive phantom action should suggest parent bump, got: {action}"
234269
);
235270
assert!(
236271
rationale.contains("transitive dependency"),
237272
"rationale should explain transitive status, got: {rationale}"
238273
);
239274
}
240275

276+
#[test]
277+
fn test_phantom_transitive_unknown_parent_falls_back_gracefully() {
278+
// Best-effort parent identification: if Cargo.lock didn't reveal one,
279+
// we still produce useful output.
280+
let (cls, rationale, action) = classify(
281+
&mock_vuln(false, false),
282+
&phantom_transitive_evidence(None),
283+
);
284+
assert_eq!(cls, Classification::Informational);
285+
assert!(
286+
action.contains("an upstream parent dependency"),
287+
"unknown-parent transitive should fall back to generic phrasing, got: {action}"
288+
);
289+
assert!(rationale.contains("transitive dependency"));
290+
}
291+
241292
#[test]
242293
fn test_reachable_no_fix_is_unmitigable() {
243-
let (cls, _, _) = classify(&mock_vuln(false, false), &reachable_evidence(), true);
294+
let (cls, _, _) = classify(&mock_vuln(false, false), &reachable_evidence());
244295
assert_eq!(cls, Classification::Unmitigable);
245296
}
246297

247298
#[test]
248299
fn test_reachable_semver_fix_is_mitigable() {
249-
let (cls, _, action) = classify(&mock_vuln(true, true), &reachable_evidence(), true);
300+
let (cls, _, action) = classify(&mock_vuln(true, true), &reachable_evidence());
250301
assert_eq!(cls, Classification::Mitigable);
251302
assert!(action.contains("cargo update"));
252303
}
253304

254305
#[test]
255306
fn test_reachable_breaking_fix_is_mitigable() {
256-
let (cls, _, action) = classify(&mock_vuln(true, false), &reachable_evidence(), true);
307+
let (cls, _, action) = classify(&mock_vuln(true, false), &reachable_evidence());
257308
assert_eq!(cls, Classification::Mitigable);
258309
assert!(action.contains("breaking change"));
259310
}
260311

261312
#[test]
262-
fn test_reachable_classification_unaffected_by_is_direct() {
263-
// is_direct is only used for the Phantom arm — reachable findings
264-
// should classify identically regardless of manifest declaration.
265-
let (cls_a, _, _) = classify(&mock_vuln(true, true), &reachable_evidence(), true);
266-
let (cls_b, _, _) = classify(&mock_vuln(true, true), &reachable_evidence(), false);
267-
assert_eq!(cls_a, cls_b);
313+
fn test_phantom_variants_both_classify_informational() {
314+
// Three-way classifier output is unchanged from #47.
315+
let (cls_decl, _, _) = classify(&mock_vuln(false, false), &phantom_declared_evidence());
316+
let (cls_trans, _, _) = classify(&mock_vuln(false, false), &phantom_transitive_evidence(None));
317+
assert_eq!(cls_decl, Classification::Informational);
318+
assert_eq!(cls_trans, Classification::Informational);
268319
}
269320
}

0 commit comments

Comments
 (0)