Skip to content

Commit 44c0d5f

Browse files
hyperpolymathclaude
andcommitted
feat(bridge): split phantom into phantom-declared vs phantom-transitive
The Track E sweep across 29 Cargo CVE issues revealed that the previous 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 dependency 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 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, Cohort E-2 4/5). Changes ------- - `ReachabilityStatus::Phantom` split into `PhantomDeclared` / `PhantomTransitive`. Serde rename_all = kebab-case so the JSON wire values are `phantom-declared` / `phantom-transitive`. - `ReachabilityEvidence` gains an optional `parent_dep` (set only on `PhantomTransitive` when identifiable via Cargo.lock). - `reachability::check_reachability_with_manifest` is the new entry point — takes the declared-deps set and a parent map, returns the correct variant. The old `check_reachability` is `#[cfg(test)]` only. - `lockfile::collect_cargo_parents` parses `Cargo.lock`'s `dependencies` arrays and BFS-walks from each direct dep, attributing every transitive to a parent (first match wins). - `classify::classify` no longer takes `is_direct` — the variant on the evidence already carries that information. Three-way classifier output (Mitigable / Unmitigable / Informational) is unchanged. The rationale and fix-action diverge per variant: - PhantomDeclared: "Strip from Cargo.toml — run `cargo machete --fix`" - PhantomTransitive: "Pulled in transitively by `<parent>` — fix requires bumping the parent dependency past the affected version" - `BridgeReport::schema_version` bumped 0.1.0 → 0.2.0 to flag the reach-field semantics change. Tests ----- Added 8 regression tests covering: - `reachability::phantom_declared_when_crate_in_cargo_toml_but_no_use` - `reachability::phantom_transitive_when_crate_not_in_cargo_toml` (with parent identified) - `reachability::reachable_when_declared_and_used` - `reachability::phantom_transitive_with_unknown_parent_is_still_classified` - `reachability::phantom_classification_normalises_underscore_to_hyphen` - `reachability::phantom_declared_resolves_workspace_member_dep` - `lockfile::parent_map_identifies_direct_to_transitive_path` - `lockfile::parent_map_normalises_underscore_to_hyphen` - `lockfile::parent_map_returns_empty_when_no_lockfile` - `classify::test_phantom_declared_recommends_machete_strip` - `classify::test_phantom_transitive_recommends_parent_bump` - `classify::test_phantom_transitive_unknown_parent_falls_back_gracefully` - `classify::test_phantom_variants_both_classify_informational` Existing tests rewritten to use the new evidence shape; all 38 bridge tests pass + full suite (327 lib + integration) is green. Refs: panic-attack#74 (vendored-pin allowlist), panic-attack#75 (Dioxus/GTK informational), file-soup#50 (true phantom-declared). Closes #32 (Track C/E sweep parent). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 7935204 commit 44c0d5f

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)