From 1c8ba3f5cd1ce456b9b37eebf16bc215c848b8c0 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 5 Aug 2026 11:59:33 -0400 Subject: [PATCH 1/2] fix(scan,get): rank patches by merged, then severity, then patch recency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a package has more than one available patch, `select_patches` never consulted severity. For an authorized user (`can_access_paid == true`) the pick was "newest paid patch, else newest patch": group.sort_by(|a, b| b.published_at.cmp(&a.published_at)); let choice = group.iter().find(|p| p.tier == "paid").or_else(|| group.first()) so a package whose newest patch fixed a `low` got that one and its `critical` patch was silently dropped. The date sort was broken outright besides. `publishedAt` is RFC 2822 / HTTP-date on the wire (`Fri, 27 Mar 2026 19:12:42 GMT`, verified live across npm, PyPI, cargo and gem), so a raw `String::cmp` orders by *day-of-week name* — `Fri` < `Mon` < `Sat` < `Sun` < `Thu` < `Tue` < `Wed`. Every fixture in this repo uses ISO-8601, which is why it was never caught. A second, independent chooser — `detect_updates` — took `.first()` off the raw batch response with no ordering at all, so `updates[].newUuid` could name a different patch than `--apply` installed. The new order, best first: 1. merged patches 2. severity: critical > high > medium/moderate > low > unknown 3. patch publish date, most recent first 4. tier (paid), then uuid — tiebreaks only, so the order is total and output is reproducible `tier` is now an access filter, not a ranking key: a free `critical` outranks a paid `low`. This reverses `select_paid_user_prefers_paid_over_free_same_purl`, which now holds only when everything above tier ties. Rank 3 is the date *the patch* was published, never the upstream package's release date. The two are unrelated — axios@1.6.0 shipped 2023-10-26 and carries patches published 2026-03-27 and 2026-08-03. Implementation notes: * `api::ranking` is the one comparator; ordering is normalized at the API client boundary so the table, `--json` arrays, `get`'s listing, the interactive prompt and `detect_updates` all inherit it instead of each re-deriving it. * `utils::date` parses RFC 2822, RFC 3339 and bare dates to epoch seconds with no new dependency (reuses the Hinnant calendar algorithm already in `vex::time`). * The three duplicated severity ladders now delegate to one. * `merged` is not emitted by any endpoint yet. It deserializes defensively (`de_truthy_flag` + aliases) so a `mergedAt` *string* payload cannot hard-error an entire patch-list response. Confirm the real JSON key and prune the aliases before this ships. * Free/unauthorized callers keep the interactive picker and the `selection_required` JSON error; only the presented order changed — which is what fixes `scan` for them, since `select_one` auto-selects index 0 in non-TTY and defaults to it in a TTY. Known gap, documented in CLI_CONTRACT.md: the batch endpoint omits `publishedAt` while by-package carries it, so scan's *listing* and apply's *selection* can differ when top candidates tie on merged AND severity. Only the reported order is affected, never what lands on disk. `BatchPatchInfo.published_at` is already wired, so this closes server-side. Tests: the date rung is mutation-checked — stubbing it out fails 8 tests across core, cli and the wiremock e2e. Adds a live production canary (`canary_published_at_is_a_patch_date_not_a_package_date`) that fails if the API ever switches `publishedAt` to a package-level date, which would silently disable recency ranking with no error anywhere. Co-Authored-By: Claude Opus 5 (1M context) --- crates/socket-patch-cli/CLI_CONTRACT.md | 65 +- crates/socket-patch-cli/src/commands/get.rs | 232 ++++++- .../src/commands/scan/discovery.rs | 129 +++- .../socket-patch-cli/src/commands/scan/mod.rs | 7 + .../tests/e2e_hosted_production.rs | 132 ++++ .../tests/in_process_get_update_count.rs | 1 + .../socket-patch-cli/tests/in_process_scan.rs | 271 ++++++++ .../socket-patch-cli/tests/scan_invariants.rs | 88 +++ crates/socket-patch-core/src/api/client.rs | 81 ++- crates/socket-patch-core/src/api/mod.rs | 1 + crates/socket-patch-core/src/api/ranking.rs | 612 ++++++++++++++++++ crates/socket-patch-core/src/api/types.rs | 138 ++++ crates/socket-patch-core/src/utils/date.rs | 448 +++++++++++++ crates/socket-patch-core/src/utils/mod.rs | 3 +- crates/socket-patch-core/src/utils/serde.rs | 107 ++- 15 files changed, 2230 insertions(+), 85 deletions(-) create mode 100644 crates/socket-patch-core/src/api/ranking.rs create mode 100644 crates/socket-patch-core/src/utils/date.rs diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index b2e8a324..e048db77 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -938,7 +938,7 @@ rely on these keys. "description": "Fixes prototype pollution in minimist", "license": "MIT", "tier": "free" | "paid", - "exportedAt": "2024-01-01T00:00:00Z", // publishedAt from API + "exportedAt": "2024-01-01T00:00:00Z", // publishedAt from API — when the PATCH was published "severity": "critical" | "high" | "medium" | "low", // max across all vulnerabilities; omitted when no vulns "vulnerabilities": [ { @@ -966,6 +966,69 @@ added. It's also omitted on `failed`. test snapshots are stable. `severity` at the top level is the max across the array using the ordering `critical > high > medium = moderate > low > (unknown)`. +`exportedAt` is the API's `publishedAt` **verbatim**: the date **the +patch** was published, *not* the date the upstream package version was +released. The two are unrelated — a package from 2020 routinely carries +a patch published last week, and two patches for one package version +carry two different dates. Note the wire format is RFC 2822 / HTTP-date +(`Fri, 27 Mar 2026 19:12:42 GMT`), not ISO 8601 — do not compare these +as raw strings, they sort by weekday name. + +### Which patch gets selected + +A package can have several available patches; the manifest holds one +record per PURL, so exactly one is chosen. Both `get` and every `scan` +mode rank candidates identically (`socket_patch_core::api::ranking`), +best first: + +1. **Merged** patches — the fix has landed upstream. +2. **Severity** — `critical > high > medium = moderate > low > (unknown)`, + taken as the worst severity across everything the patch fixes. +3. **Patch publish date**, most recent first — when the *patch* was + published, never the upstream package's release date. Unparseable or + absent dates sort last. +4. `tier` (paid first), then `uuid` — tiebreaks only, present so the + order is total and therefore reproducible across runs. + +`tier` is an **access filter, not a ranking signal**: a free `critical` +patch outranks a paid `low` one. Paid patches are excluded outright for +callers whose `canAccessPaidPatches` is false. + +This ordering is also the presentation order everywhere patches are +listed — `scan --json`'s `packages[].patches[]`, `get`'s "Found +patches:" listing, and the `selection_required` `options[]` array — so +`patches[0]` for a package is the patch that would be applied, and +`updates[].newUuid` names that same patch. + +Free/unauthorized callers with more than one candidate for a PURL still +get the interactive picker (or `selection_required` in `--json`); the +ranking decides the presented order and hence the highlighted default, +not the outcome. + +Two additive keys may appear on `scan --json`'s `packages[].patches[]` +entries, both omitted when absent: `publishedAt` (present whenever the +server supplies it; the public-proxy fallback path fills it in from the +per-package results) and `merged` (only ever present as `true`). + +> **Known gap — batch responses without `publishedAt`.** `scan`'s +> discovery (`packages[]`, the table, `updates[]`) is built from the +> **batch** endpoint, whose response shape currently omits `publishedAt`; +> the selection that `--apply` performs is built from the **by-package** +> endpoint, which carries it. Ranks 1, 2, 4 and 5 agree across both, so +> the two only diverge for a package whose top candidates tie on merge +> status *and* severity — there the batch side falls through to the UUID +> tiebreak while apply correctly uses the date. +> +> Live example: `pkg:npm/axios@1.6.0` has two free `HIGH` patches; +> `packages[0].patches[0]` reports `0bc312a6…` (2026-03-27) while +> `--apply` installs the newer `83f5a654…` (2026-08-03), which is the +> correct choice. Only the reported ordering is affected — never which +> patch lands on disk. +> +> The client already deserializes `publishedAt` on the batch shape +> (`#[serde(default)]`), so this closes with no client change the moment +> the batch endpoint emits it. + ### `jq` recipes for PR-comment bots Applied + updated patches (envelope shape): diff --git a/crates/socket-patch-cli/src/commands/get.rs b/crates/socket-patch-cli/src/commands/get.rs index e22eb5d1..c15f8ce8 100644 --- a/crates/socket-patch-cli/src/commands/get.rs +++ b/crates/socket-patch-cli/src/commands/get.rs @@ -3,6 +3,7 @@ use regex::Regex; use socket_patch_core::api::client::{ build_proxy_fallback_client, get_api_client_with_overrides, is_fallback_candidate, }; +use socket_patch_core::api::ranking::{cmp_search_results, severity_order}; use socket_patch_core::api::types::{ PatchResponse, PatchSearchResult, SearchResponse, VulnerabilityResponse, }; @@ -82,18 +83,14 @@ pub(crate) fn decide_patch_action( } } -/// Ordinal rank for severity strings. Higher = worse. Unknown labels -/// (including GHSA's `moderate` which maps to `medium`) get sensible -/// defaults so the max-severity selector still works. +/// Ordinal rank for severity strings. Higher = worse — the inverse of +/// core's [`severity_order`], which this derives from so the two ladders +/// cannot drift. Unknown labels (including GHSA's `moderate`, which maps to +/// `medium`) get sensible defaults so the max-severity selector still works. fn severity_rank(severity: &str) -> u8 { - match severity.to_ascii_lowercase().as_str() { - "critical" => 4, - "high" => 3, - // GHSA emits `moderate`; treat it as the medium-tier signal. - "moderate" | "medium" => 2, - "low" => 1, - _ => 0, - } + // severity_order: 0 = critical … 4 = unknown. Flip it so 4 = critical + // and unknown lands at 0, which callers below treat as "no signal". + 4 - severity_order(Some(severity)) } /// Return the highest-severity label from a vulnerabilities map. @@ -489,11 +486,23 @@ fn detect_identifier_type(identifier: &str) -> Option { /// Select one patch per PURL from available patches. /// -/// - Paid users: auto-select the most recent paid patch per PURL. +/// Within a PURL, candidates are ranked by [`cmp_search_results`]: merged +/// patches first, then by severity (critical → low), then most recently +/// published. `tier` is an access filter here, not a ranking signal — a +/// free critical patch outranks a paid low one. +/// +/// - Users with paid access: auto-select the top-ranked patch per PURL. /// - Free users with one patch: auto-select it. -/// - Free users with multiple patches: interactive selection via dialoguer. +/// - Free users with multiple patches: interactive selection via dialoguer, +/// with the options presented in ranked order so the best patch is both +/// the highlighted default and what a non-TTY run auto-picks. /// - JSON mode with multiple free patches: returns an error with options list. /// +/// The returned vec is sorted by PURL. It is assembled from a `HashMap`, +/// whose iteration order is randomized per process; without the sort the +/// download order — and every `--json` array derived from it — would differ +/// run to run. +/// /// Returns `Ok(selected_patches)` or `Err(exit_code)` if selection fails. pub(crate) fn select_patches( patches: &[PatchSearchResult], @@ -510,18 +519,23 @@ pub(crate) fn select_patches( let mut selected = Vec::new(); - for (purl, mut group) in by_purl { - // Sort by published_at descending (most recent first) - group.sort_by(|a, b| b.published_at.cmp(&a.published_at)); + // Iterate PURLs in a fixed order too: the interactive prompts below are + // presented to a human one after another, and a randomized sequence + // would be disorienting across otherwise identical runs. + let mut groups: Vec<(String, Vec<&PatchSearchResult>)> = by_purl.into_iter().collect(); + groups.sort_by(|a, b| a.0.cmp(&b.0)); + + for (purl, mut group) in groups { + // Canonical best-first order (see `api::ranking`). The API client + // already sorts each response, but this call site merges results + // across several queries, so re-sort the assembled group. + group.sort_by(|a, b| cmp_search_results(a, b)); if can_access_paid { - // Paid user: prefer most recent paid patch, fallback to most recent free - let choice = group - .iter() - .find(|p| p.tier == "paid") - .or_else(|| group.first()) - .unwrap(); - selected.push((*choice).clone()); + // Take the top-ranked patch. Note this is NOT "prefer paid": + // tier only breaks ties once merge status, severity and recency + // have all tied. + selected.push(group[0].clone()); } else if group.len() == 1 { selected.push(group[0].clone()); } else { @@ -603,6 +617,8 @@ pub(crate) fn select_patches( } } + // PURL-sorted by construction: `groups` was sorted above and this loop + // pushes at most one entry per group. Ok(selected) } @@ -1707,9 +1723,17 @@ pub async fn run(args: GetArgs) -> i32 { code } +/// Print the patches a search turned up, grouped by PURL and best-first +/// within each PURL — the same order [`select_patches`] resolves in, so the +/// listing's first entry for a package is the one that will be applied. +/// A `by-cve` / `by-ghsa` search can span several packages, hence the PURL +/// grouping. fn display_search_results(patches: &[PatchSearchResult], can_access_paid: bool) { println!("\nFound patches:\n"); + let mut patches: Vec<&PatchSearchResult> = patches.iter().collect(); + patches.sort_by(|a, b| a.purl.cmp(&b.purl).then_with(|| cmp_search_results(a, b))); + for (i, patch) in patches.iter().enumerate() { let tier_label = if patch.tier == "paid" { " [PAID]" @@ -2065,9 +2089,32 @@ mod tests { license: "MIT".into(), tier: tier.into(), vulnerabilities: HashMap::::new(), + merged: false, } } + /// `mk_patch` with a single vulnerability at the given severity, so the + /// severity rung of the ranking is exercised. + fn mk_patch_sev( + uuid: &str, + purl: &str, + tier: &str, + published_at: &str, + severity: &str, + ) -> PatchSearchResult { + let mut p = mk_patch(uuid, purl, tier, published_at); + p.vulnerabilities.insert( + format!("GHSA-{uuid}"), + VulnerabilityResponse { + cves: vec![], + summary: String::new(), + severity: severity.into(), + description: String::new(), + }, + ); + p + } + #[test] fn select_free_user_one_free_patch_returns_it() { let patches = vec![mk_patch("u1", "pkg:npm/foo@1.0", "free", "2024-01-01")]; @@ -2077,14 +2124,143 @@ mod tests { } #[test] - fn select_paid_user_prefers_paid_over_free_same_purl() { + fn select_paid_user_picks_highest_severity_not_most_recent() { + // The reported bug. An authorized user's package has a fresh `low` + // patch and an older `critical` one; the old selector took the + // newest and silently left the critical unfixed. + let patches = vec![ + mk_patch_sev("new_low", "pkg:npm/foo@1.0", "paid", "2026-06-01", "low"), + mk_patch_sev( + "old_crit", + "pkg:npm/foo@1.0", + "paid", + "2024-01-01", + "critical", + ), + ]; + let out = select_patches(&patches, true, false).expect("ok"); + assert_eq!(out.len(), 1); + assert_eq!(out[0].uuid, "old_crit"); + } + + #[test] + fn select_paid_user_picks_free_critical_over_paid_low() { + // Severity outranks tier: `tier` gates *access*, it does not rank. + // A paid subscriber must not be handed a low-severity paid patch + // when a critical free one exists for the same package. + let patches = vec![ + mk_patch_sev("paid_low", "pkg:npm/foo@1.0", "paid", "2026-06-01", "low"), + mk_patch_sev( + "free_crit", + "pkg:npm/foo@1.0", + "free", + "2024-01-01", + "critical", + ), + ]; + let out = select_patches(&patches, true, false).expect("ok"); + assert_eq!(out.len(), 1); + assert_eq!(out[0].uuid, "free_crit"); + assert_eq!(out[0].tier, "free"); + } + + #[test] + fn select_prefers_merged_patch_over_higher_severity() { + // Rule 1 beats rule 2: a merged patch is the fix the ecosystem has + // converged on, so it leads even a critical non-merged patch. + let mut merged = mk_patch_sev("merged", "pkg:npm/foo@1.0", "free", "2020-01-01", "low"); + merged.merged = true; + let patches = vec![ + mk_patch_sev("crit", "pkg:npm/foo@1.0", "paid", "2026-06-01", "critical"), + merged, + ]; + let out = select_patches(&patches, true, false).expect("ok"); + assert_eq!(out.len(), 1); + assert_eq!(out[0].uuid, "merged"); + } + + #[test] + fn select_recency_is_chronological_not_lexicographic() { + // `publishedAt` is RFC 2822 on the wire, so the old raw-string + // compare ordered by weekday name. With equal severities the newer + // patch must win regardless of which weekday it fell on. + let older = "Wed, 01 Jan 2025 00:00:00 GMT"; + let newer = "Fri, 01 Aug 2026 00:00:00 GMT"; + assert!(older > newer, "precondition: raw strings sort backwards"); + // Adversarial UUIDs: `a_older` sorts first, so the final uuid + // tiebreak points at the wrong patch and cannot rescue this test if + // the date rung breaks. + let patches = vec![ + mk_patch_sev("a_older", "pkg:npm/foo@1.0", "paid", older, "high"), + mk_patch_sev("z_newer", "pkg:npm/foo@1.0", "paid", newer, "high"), + ]; + let out = select_patches(&patches, true, false).expect("ok"); + assert_eq!(out.len(), 1); + assert_eq!(out[0].uuid, "z_newer"); + } + + #[test] + fn select_recency_uses_the_patch_date_not_the_package_release_date() { + // Real production pair: both patches are for `axios@1.6.0` — one + // package version, one upstream release date (2023-10-26) — yet + // they carry different publish dates because the field describes + // the PATCH. Severities tie, so the date is the deciding rung. + // + // Non-vacuity: `0bc312a6` < `83f5a654`, so if the ranking ever fell + // back to the UUID tiebreak (which is what a package-level date + // would cause, both keys being equal) this would select the OLDER + // patch and fail. + let patches = vec![ + mk_patch_sev( + "0bc312a6", + "pkg:npm/axios@1.6.0", + "free", + "Fri, 27 Mar 2026 19:12:42 GMT", + "HIGH", + ), + mk_patch_sev( + "83f5a654", + "pkg:npm/axios@1.6.0", + "free", + "Mon, 03 Aug 2026 20:23:06 GMT", + "HIGH", + ), + ]; + let out = select_patches(&patches, true, false).expect("ok"); + assert_eq!(out.len(), 1, "one patch per PURL"); + assert_eq!(out[0].uuid, "83f5a654"); + } + + #[test] + fn select_returns_purl_sorted_output() { + // The grouping map has randomized iteration order; without an + // explicit sort the download sequence (and every JSON array derived + // from it) would differ run to run. + let patches = vec![ + mk_patch("c", "pkg:npm/ccc@1.0", "paid", "2024-01-01"), + mk_patch("a", "pkg:npm/aaa@1.0", "paid", "2024-01-01"), + mk_patch("b", "pkg:npm/bbb@1.0", "paid", "2024-01-01"), + ]; + for _ in 0..8 { + let out = select_patches(&patches, true, false).expect("ok"); + let purls: Vec<&str> = out.iter().map(|p| p.purl.as_str()).collect(); + assert_eq!( + purls, + ["pkg:npm/aaa@1.0", "pkg:npm/bbb@1.0", "pkg:npm/ccc@1.0"] + ); + } + } + + #[test] + fn select_paid_user_prefers_paid_when_everything_else_ties() { + // Tier survives only as a late tiebreak: same merge status, same + // (absent) severity, same publish date → paid wins. let patches = vec![ - mk_patch("free1", "pkg:npm/foo@1.0", "free", "2024-06-01"), + mk_patch("free1", "pkg:npm/foo@1.0", "free", "2024-01-01"), mk_patch("paid1", "pkg:npm/foo@1.0", "paid", "2024-01-01"), ]; let out = select_patches(&patches, true, false).expect("ok"); assert_eq!(out.len(), 1); - // Paid wins even if free is more recent. assert_eq!(out[0].uuid, "paid1"); assert_eq!(out[0].tier, "paid"); } @@ -2345,6 +2521,7 @@ mod tests { description: "desc".into(), license: "MIT".into(), tier: "free".into(), + merged: false, }; let meta = patch_event_metadata(&patch); assert!(meta.as_object().unwrap().get("severity").is_none()); @@ -2375,6 +2552,7 @@ mod tests { description: "Fixes prototype pollution in minimist".into(), license: "MIT".into(), tier: "free".into(), + merged: false, }; let meta = patch_event_metadata(&patch); assert_eq!(meta["description"], "Fixes prototype pollution in minimist"); @@ -2416,6 +2594,7 @@ mod tests { description: String::new(), license: String::new(), tier: String::new(), + merged: false, }; let meta = patch_event_metadata(&patch); let ids: Vec<&str> = meta["vulnerabilities"] @@ -2438,6 +2617,7 @@ mod tests { description: "desc".into(), license: "MIT".into(), tier: "free".into(), + merged: false, }; let meta = patch_event_metadata(&patch); // `severity` is intentionally omitted (not null) when there diff --git a/crates/socket-patch-cli/src/commands/scan/discovery.rs b/crates/socket-patch-cli/src/commands/scan/discovery.rs index 07bcd03f..432a8690 100644 --- a/crates/socket-patch-cli/src/commands/scan/discovery.rs +++ b/crates/socket-patch-cli/src/commands/scan/discovery.rs @@ -2,6 +2,7 @@ //! supplements, update detection against the existing manifest, vendor //! baseline pre-verification, and the table's vuln-ID / severity helpers. +use socket_patch_core::api::ranking::cmp_batch_infos; use socket_patch_core::api::types::{BatchPackagePatches, PatchSearchResult}; use socket_patch_core::manifest::schema::PatchManifest; use socket_patch_core::utils::purl::{normalize_purl, strip_purl_qualifiers}; @@ -192,10 +193,26 @@ pub(super) fn detect_updates( let Some(existing) = manifest.patches.get(&pkg.purl) else { continue; }; - // Treat the first patch in the batch as the candidate the apply path - // would resolve to (mirrors `select_patches` ordering — newest-first - // for paid users, single-patch auto-select for free). - let Some(candidate) = pkg.patches.first() else { + // The candidate is the top-ranked patch — the one the apply path + // resolves to. Both sides rank with `api::ranking`, so the + // `[UPDATE]` marker and the JSON `updates` array track what + // `--apply` installs. + // + // Caveat, and the one place the two can still disagree: we rank + // BATCH-shaped patches here, while apply ranks the richer + // by-package shape. The batch response currently omits + // `publishedAt`, so when a package's top candidates tie on merge + // status AND severity, this falls through to the UUID tiebreak + // while apply correctly uses the date. `BatchPatchInfo` already + // deserializes `publishedAt` when present, so the divergence + // disappears the moment the endpoint emits it — no client change. + // (Verified live on pkg:npm/axios@1.6.0, two free HIGH patches.) + // + // `ApiClient` already returns each package's patches best-first, so + // `min_by` here is a cheap guard rather than a correction — but it + // is load-bearing for callers that build a `BatchPackagePatches` + // themselves rather than getting one from the client. + let Some(candidate) = pkg.patches.iter().min_by(|a, b| cmp_batch_infos(a, b)) else { continue; }; if candidate.uuid != existing.uuid { @@ -232,15 +249,11 @@ pub(super) fn collect_vuln_ids(pkg: &BatchPackagePatches) -> Vec { cves.into_iter().chain(ghsas).collect() } +/// Severity ordering for the scan table's SEVERITY column: lower = worse. +/// Delegates to the workspace-wide ladder so the table, the selector and +/// the API client can never disagree about what `moderate` means. pub(super) fn severity_order(s: &str) -> u8 { - match s.to_lowercase().as_str() { - "critical" => 0, - "high" => 1, - // GHSA emits `moderate`; same tier as medium (see get.rs severity_rank). - "medium" | "moderate" => 2, - "low" => 3, - _ => 4, - } + socket_patch_core::api::ranking::severity_order(Some(s)) } #[cfg(test)] @@ -306,6 +319,31 @@ mod tests { ghsa_ids: Vec::new(), severity: None, title: String::new(), + published_at: None, + merged: false, + }) + .collect(), + } + } + + /// `batch_with`, but each patch carries an explicit severity and + /// publish date so the ranking rungs above the uuid tiebreak are + /// actually exercised. + fn batch_ranked(purl: &str, patches: &[(&str, &str, &str)]) -> BatchPackagePatches { + BatchPackagePatches { + purl: purl.to_string(), + patches: patches + .iter() + .map(|(uuid, severity, published)| BatchPatchInfo { + uuid: (*uuid).to_string(), + purl: purl.to_string(), + tier: "free".to_string(), + cve_ids: Vec::new(), + ghsa_ids: Vec::new(), + severity: Some((*severity).to_string()), + title: String::new(), + published_at: Some((*published).to_string()), + merged: false, }) .collect(), } @@ -370,34 +408,65 @@ mod tests { } #[test] - fn detect_updates_uses_first_patch_as_candidate() { - // `detect_updates` mirrors `select_patches` by picking the first - // patch in the batch as the candidate UUID. Locking this in so a - // future select_patches refactor doesn't silently drift the two. + fn detect_updates_uses_the_highest_ranked_patch_as_candidate() { + // `detect_updates` must name the UUID the apply path will actually + // install, which is the top-ranked patch (`api::ranking`), NOT + // whatever the server happened to list first. Here the critical + // patch is listed last and is the older of the two. let m = manifest_with(&[("pkg:npm/foo@1.0", "uuid-a")]); - let pkgs = vec![batch_with("pkg:npm/foo@1.0", &["uuid-b", "uuid-c"])]; + let pkgs = vec![batch_ranked( + "pkg:npm/foo@1.0", + &[ + ("uuid-low-new", "low", "2026-06-01T00:00:00Z"), + ("uuid-crit-old", "critical", "2024-01-01T00:00:00Z"), + ], + )]; let updates = detect_updates(Some(&m), &pkgs); assert_eq!(updates.len(), 1); - assert_eq!(updates[0].new_uuid, "uuid-b"); + assert_eq!(updates[0].new_uuid, "uuid-crit-old"); + } + + #[test] + fn detect_updates_candidate_ordering_ignores_incoming_list_order() { + // Same input, reversed. A positional `.first()` would flip its + // answer; a ranked candidate must not. + let m = manifest_with(&[("pkg:npm/foo@1.0", "uuid-a")]); + let forward = batch_ranked( + "pkg:npm/foo@1.0", + &[ + ("uuid-crit", "critical", "2024-01-01T00:00:00Z"), + ("uuid-high", "high", "2026-06-01T00:00:00Z"), + ], + ); + let mut reversed = forward.clone(); + reversed.patches.reverse(); + assert_eq!( + detect_updates(Some(&m), &[forward])[0].new_uuid, + detect_updates(Some(&m), &[reversed])[0].new_uuid, + ); } #[test] fn detect_updates_no_update_when_manifest_holds_candidate_despite_other_patches() { // Regression: the human-readable table once flagged `[UPDATE]` (and // bumped `updates_available`) whenever *any* batch patch differed from - // the manifest UUID. But the apply path resolves to the FIRST patch, - // so a manifest already holding that candidate is up to date even when - // the batch also lists older patches. The table and the JSON `updates` - // array must agree; both derive from this function, which compares the - // candidate (first) patch only. - let m = manifest_with(&[("pkg:npm/foo@1.0", "uuid-newest")]); - let pkgs = vec![batch_with( + // the manifest UUID. But the apply path resolves to the top-ranked + // patch, so a manifest already holding that candidate is up to date + // even when the batch also lists lesser patches. The table and the + // JSON `updates` array must agree; both derive from this function, + // which compares the ranked candidate only. + let m = manifest_with(&[("pkg:npm/foo@1.0", "uuid-critical")]); + let pkgs = vec![batch_ranked( "pkg:npm/foo@1.0", - &["uuid-newest", "uuid-older", "uuid-oldest"], + &[ + ("uuid-low", "low", "2026-08-01T00:00:00Z"), + ("uuid-critical", "critical", "2024-01-01T00:00:00Z"), + ("uuid-medium", "medium", "2026-07-01T00:00:00Z"), + ], )]; assert!( detect_updates(Some(&m), &pkgs).is_empty(), - "manifest already holds the candidate (first) patch — no update" + "manifest already holds the ranked candidate — no update" ); } @@ -416,6 +485,8 @@ mod tests { ghsa_ids: ghsas.iter().map(|s| (*s).to_string()).collect(), severity: None, title: String::new(), + published_at: None, + merged: false, }], } } @@ -461,6 +532,8 @@ mod tests { ghsa_ids: vec![], severity: None, title: String::new(), + published_at: None, + merged: false, }, BatchPatchInfo { uuid: "u2".to_string(), @@ -470,6 +543,8 @@ mod tests { ghsa_ids: vec!["GHSA-aaaa-aaaa-aaaa".to_string()], severity: None, title: String::new(), + published_at: None, + merged: false, }, ], }; diff --git a/crates/socket-patch-cli/src/commands/scan/mod.rs b/crates/socket-patch-cli/src/commands/scan/mod.rs index 66378994..9643f7fa 100644 --- a/crates/socket-patch-cli/src/commands/scan/mod.rs +++ b/crates/socket-patch-cli/src/commands/scan/mod.rs @@ -729,6 +729,13 @@ pub async fn run(mut args: ScanArgs) -> i32 { } } + // The client returns each batch's packages PURL-sorted, but the batches + // themselves are concatenated in chunk order, so the assembled list is + // only sorted *within* each chunk. Sort globally: this list drives the + // human table, the `--json` `packages` array, and the apply order, all + // of which operators diff across runs. + all_packages_with_patches.sort_by(|a, b| a.purl.cmp(&b.purl)); + // If every batch errored, surface this as a full scan failure rather // than silently reporting zero patches (which historically looked // identical to "no patches for these packages"). diff --git a/crates/socket-patch-cli/tests/e2e_hosted_production.rs b/crates/socket-patch-cli/tests/e2e_hosted_production.rs index 47ce23c6..8cb1753c 100644 --- a/crates/socket-patch-cli/tests/e2e_hosted_production.rs +++ b/crates/socket-patch-cli/tests/e2e_hosted_production.rs @@ -480,6 +480,42 @@ async fn published_uuids(purl: &str) -> Result, String> { .unwrap_or_default()) } +/// `GET /patch/by-package/` returning `(uuid, publishedAt)` pairs. +/// Sibling of [`published_uuids`] for tests that care about patch metadata +/// rather than just which UUIDs exist. +async fn published_patch_dates(purl: &str) -> Result, String> { + let url = format!("{PROXY}/patch/by-package/{}", urlencode(purl)); + let resp = reqwest::Client::new() + .get(&url) + .header("Accept", "application/json") + .send() + .await + .map_err(|e| format!("GET {url}: {e}"))?; + let status = resp.status(); + let body = resp + .text() + .await + .map_err(|e| format!("GET {url}: reading body: {e}"))?; + if !status.is_success() { + return Err(format!("GET {url}: HTTP {status}\n{body}")); + } + let v: serde_json::Value = + serde_json::from_str(&body).map_err(|e| format!("GET {url}: bad JSON ({e}):\n{body}"))?; + Ok(v["patches"] + .as_array() + .map(|a| { + a.iter() + .filter_map(|p| { + Some(( + p["uuid"].as_str()?.to_string(), + p["publishedAt"].as_str()?.to_string(), + )) + }) + .collect() + }) + .unwrap_or_default()) +} + /// Percent-encode a PURL for use as a single path segment. `reqwest` will not /// do this for us — a raw `pkg:npm/...` would be split into path segments and /// 404. @@ -545,6 +581,102 @@ async fn preflight_required_patches_are_published() { ); } +/// Canary: production's `publishedAt` must stay a **per-patch** date. +/// +/// Patch selection ranks by recency (`socket_patch_core::api::ranking`), and +/// that rung is only meaningful if `publishedAt` describes the patch rather +/// than the upstream package release. If the server ever started emitting the +/// package's release date, every patch for a given PURL would collapse to one +/// value, recency would silently stop discriminating, and selection would +/// quietly fall through to the UUID tiebreak — a wrong answer with no error +/// anywhere. Nothing else in the suite would catch that. +/// +/// `PYPI_PURL` is the probe because production publishes three patches for +/// it (see [`PYPI_UUIDS`]). Two assertions: +/// +/// 1. the dates are not all identical — impossible for a package-level date; +/// 2. no patch date equals the package's own upload time on PyPI. +/// +/// (2) is skipped, with a note, if pypi.org is unreachable — a PyPI outage is +/// not a socket-patch regression. (1) is unconditional. +#[tokio::test(flavor = "multi_thread")] +#[ignore = "live production API: contacts patches-api.socket.dev + pypi.org. Run with --ignored."] +async fn canary_published_at_is_a_patch_date_not_a_package_date() { + let patches = published_patch_dates(PYPI_PURL) + .await + .unwrap_or_else(|e| panic!("production probe failed for {PYPI_PURL}: {e}")); + + assert!( + patches.len() >= 2, + "{PYPI_PURL} must publish >=2 patches for this canary to have teeth; \ + production returned {}. Re-pick a multi-patch PURL and update this test.", + patches.len() + ); + + let distinct: std::collections::HashSet<&str> = + patches.iter().map(|(_, d)| d.as_str()).collect(); + assert!( + distinct.len() > 1, + "all {} patches for {PYPI_PURL} share one publishedAt ({:?}). That is the \ + signature of a PACKAGE-level date: recency ranking has stopped \ + discriminating and selection is falling through to the UUID tiebreak.\n\ + patches: {patches:#?}", + patches.len(), + distinct + ); + + // (2) Cross-check against the real upstream release date. + let pypi_url = format!("https://pypi.org/pypi/{PYPI_NAME}/json"); + let Ok(resp) = reqwest::Client::new().get(&pypi_url).send().await else { + eprintln!("[skip] pypi.org unreachable; distinct-dates assertion still enforced"); + return; + }; + let Ok(body) = resp.text().await else { + eprintln!("[skip] pypi.org body unreadable; distinct-dates assertion still enforced"); + return; + }; + let Ok(v) = serde_json::from_str::(&body) else { + eprintln!("[skip] pypi.org returned non-JSON; distinct-dates assertion still enforced"); + return; + }; + let uploads: Vec = v["releases"][PYPI_VERSION] + .as_array() + .map(|files| { + files + .iter() + .filter_map(|f| f["upload_time_iso_8601"].as_str().map(str::to_string)) + .collect() + }) + .unwrap_or_default(); + if uploads.is_empty() { + eprintln!("[skip] pypi.org listed no upload times for {PYPI_NAME} {PYPI_VERSION}"); + return; + } + // PyPI stamps ISO-8601; the patch API stamps RFC 2822. They cannot be + // compared as strings, so compare the calendar DATE via the same parser + // the ranking uses. + use socket_patch_core::utils::date::parse_timestamp_secs; + let upload_days: std::collections::HashSet = uploads + .iter() + .filter_map(|u| parse_timestamp_secs(u)) + .map(|s| s / 86_400) + .collect(); + for (uuid, published) in &patches { + let Some(secs) = parse_timestamp_secs(published) else { + panic!( + "production publishedAt {published:?} (patch {uuid}) does not parse — \ + utils::date must handle every format the API emits" + ); + }; + assert!( + !upload_days.contains(&(secs / 86_400)), + "patch {uuid} reports publishedAt {published:?}, which falls on the same day \ + {PYPI_NAME} {PYPI_VERSION} was uploaded to PyPI ({uploads:?}). That strongly \ + suggests the field switched to the PACKAGE release date." + ); + } +} + // =========================================================================== // npm ecosystem — five package managers, five lockfile flavors // =========================================================================== diff --git a/crates/socket-patch-cli/tests/in_process_get_update_count.rs b/crates/socket-patch-cli/tests/in_process_get_update_count.rs index 04bb8ccf..c2798e20 100644 --- a/crates/socket-patch-cli/tests/in_process_get_update_count.rs +++ b/crates/socket-patch-cli/tests/in_process_get_update_count.rs @@ -51,6 +51,7 @@ fn search_result(uuid: &str, purl: &str) -> PatchSearchResult { license: "MIT".into(), tier: "free".into(), vulnerabilities: HashMap::new(), + merged: false, } } diff --git a/crates/socket-patch-cli/tests/in_process_scan.rs b/crates/socket-patch-cli/tests/in_process_scan.rs index 6b9dcb81..21f9f558 100644 --- a/crates/socket-patch-cli/tests/in_process_scan.rs +++ b/crates/socket-patch-cli/tests/in_process_scan.rs @@ -344,6 +344,277 @@ async fn scan_apply_wet_writes_manifest_and_blob() { ); } +// --------------------------------------------------------------------------- +// Multi-patch packages — which patch scan resolves to +// --------------------------------------------------------------------------- + +/// Second patch UUID for the multi-patch fixtures. Deliberately sorts +/// *after* `UUID` lexicographically, so a test that passes because of the +/// uuid tiebreak rather than the severity ranking would still name `UUID`. +const UUID_LOW: &str = "22222222-2222-4222-8222-222222222222"; + +/// A package with two available patches: a freshly-published `low` and an +/// older `critical`. `paid` toggles `canAccessPaidPatches`, which selects +/// between `select_patches`' auto-select branch and its interactive one. +/// +/// This is the exact shape of the reported bug — the old selector took the +/// most recent patch and left the critical unfixed. +async fn mock_two_patches(server: &MockServer, paid: bool) { + let low = serde_json::json!({ + "uuid": UUID_LOW, "purl": PURL, "tier": "free", + // Uppercase severity + RFC 2822 date, exactly as production emits + // them (verified against patches-api.socket.dev). + "cveIds": [], "ghsaIds": [], "severity": "LOW", "title": "low sev", + "publishedAt": "Mon, 03 Aug 2026 20:23:06 GMT", + }); + let critical = serde_json::json!({ + "uuid": UUID, "purl": PURL, "tier": "free", + "cveIds": [], "ghsaIds": [], "severity": "CRITICAL", "title": "critical sev", + "publishedAt": "Wed, 01 Jan 2025 00:00:00 GMT", + }); + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/batch"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + // Listed newest-first, i.e. the order the old `.first()` / + // date-sort logic would have taken the WRONG patch from. + "packages": [{ "purl": PURL, "patches": [low, critical] }], + "canAccessPaidPatches": paid, + }))) + .mount(server) + .await; + + Mock::given(method("GET")) + .and(path_regex(format!( + "^/v0/orgs/{ORG}/patches/by-package/.+$" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "patches": [ + { + "uuid": UUID_LOW, "purl": PURL, + "publishedAt": "Mon, 03 Aug 2026 20:23:06 GMT", + "description": "low", "license": "MIT", "tier": "free", + "vulnerabilities": { "GHSA-low0-low0-low0": { + "cves": [], "summary": "s", "severity": "LOW", "description": "d" + }} + }, + { + "uuid": UUID, "purl": PURL, + "publishedAt": "Wed, 01 Jan 2025 00:00:00 GMT", + "description": "critical", "license": "MIT", "tier": "free", + "vulnerabilities": { "GHSA-crit-crit-crit": { + "cves": [], "summary": "s", "severity": "CRITICAL", "description": "d" + }} + } + ], + "canAccessPaidPatches": paid, + }))) + .mount(server) + .await; +} + +/// The apply path must fetch the CRITICAL patch's view, not the low one. +/// +/// Note both severities are spelled uppercase and both dates are RFC 2822, +/// exactly as production emits them — so this also covers the case-folding +/// and the date parse. Applying itself partial-fails (the handcrafted +/// `node_modules` file can't match the fixture's beforeHash), which is +/// beside the point: the assertion is about *which patch was chosen*. +#[tokio::test] +#[serial] +async fn scan_apply_picks_critical_over_more_recent_low_for_paid_user() { + let server = MockServer::start().await; + mock_two_patches(&server, true).await; + mock_view_with_blob(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "in-proc-scan", "1.0.0"); + let mut args = default_args(tmp.path()); + args.common.api_url = Some(server.uri()); + args.apply = true; + + run_scrubbed(args).await; + + let reqs = recorded(&server).await; + assert_eq!( + view_gets(&reqs, UUID), + 1, + "must fetch the CRITICAL patch's view exactly once" + ); + assert_eq!( + view_gets(&reqs, UUID_LOW), + 0, + "must not fetch the low-severity patch — it lost the ranking" + ); + + let manifest_path = tmp.path().join(".socket/manifest.json"); + let manifest: serde_json::Value = + serde_json::from_str(&std::fs::read_to_string(&manifest_path).unwrap()).unwrap(); + assert_eq!( + manifest["patches"][PURL]["uuid"], UUID, + "manifest must record the critical patch; got {manifest}" + ); +} + +/// Same package, but the user has no paid access, so `select_patches` +/// takes the interactive branch. Tests run headless, so `select_one` +/// auto-selects option 0 — which means the *presented order* is what +/// decides, and it must be the ranked order. +#[tokio::test] +#[serial] +async fn scan_apply_picks_critical_for_free_user_via_ranked_prompt_order() { + let server = MockServer::start().await; + mock_two_patches(&server, false).await; + mock_view_with_blob(&server).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "in-proc-scan", "1.0.0"); + let mut args = default_args(tmp.path()); + args.common.api_url = Some(server.uri()); + args.apply = true; + + run_scrubbed(args).await; + + let reqs = recorded(&server).await; + assert_eq!( + view_gets(&reqs, UUID), + 1, + "non-TTY auto-select takes option 0, which must be the critical patch" + ); + assert_eq!(view_gets(&reqs, UUID_LOW), 0); +} + +/// Mock `view/` for an explicit uuid, echoing back its own +/// `publishedAt`. Both patches in the date-tiebreak fixture get one, so a +/// wrong selection produces a *wrong* answer rather than a 404 — the test +/// then discriminates on selection alone, not on which mock happens to +/// exist. +async fn mock_view_for(server: &MockServer, uuid: &str, published_at: &str) { + Mock::given(method("GET")) + .and(path(format!("/v0/orgs/{ORG}/patches/view/{uuid}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "uuid": uuid, + "purl": PURL, + "publishedAt": published_at, + "files": { + "package/index.js": { + "beforeHash": "0000000000000000000000000000000000000000000000000000000000000000", + "afterHash": "1111111111111111111111111111111111111111111111111111111111111111", + "blobContent": "cGF0Y2hlZAo=", + } + }, + "vulnerabilities": {}, + "description": "x", "license": "MIT", "tier": "free", + }))) + .mount(server) + .await; +} + +/// Severity ties, so the PATCH PUBLISH DATE is the only thing left to +/// decide — and it must decide correctly. +/// +/// This is the end-to-end guard for "recency means the date the patch was +/// published, not the date the package was released". Both patches are for +/// the same `PURL` (one package version, one upstream release date) and both +/// are `HIGH`; they differ only in `publishedAt`. The fixture uses the real +/// production values from `pkg:npm/axios@1.6.0`. +/// +/// Non-vacuity, two ways: the older patch is listed FIRST in the response +/// (so a positional `.first()` picks it) and its UUID sorts first (so the +/// UUID tiebreak — which is exactly where a package-level date would land +/// us, both keys being equal — also picks it). Only a genuine per-patch date +/// yields `UUID_NEWER`. +#[tokio::test] +#[serial] +async fn scan_apply_picks_the_more_recently_published_patch_when_severity_ties() { + const UUID_OLDER: &str = "0bc312a6-1b43-46bb-ba83-95b53867deb3"; + const UUID_NEWER: &str = "83f5a654-db80-4086-aa3d-593036fe7c7d"; + const PUBLISHED_OLDER: &str = "Fri, 27 Mar 2026 19:12:42 GMT"; + const PUBLISHED_NEWER: &str = "Mon, 03 Aug 2026 20:23:06 GMT"; + assert!( + UUID_OLDER < UUID_NEWER, + "uuid tiebreak favors the older patch" + ); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG}/patches/batch"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "packages": [{ "purl": PURL, "patches": [ + { "uuid": UUID_OLDER, "purl": PURL, "tier": "free", "cveIds": [], "ghsaIds": [], + "severity": "HIGH", "title": "older", "publishedAt": PUBLISHED_OLDER }, + { "uuid": UUID_NEWER, "purl": PURL, "tier": "free", "cveIds": [], "ghsaIds": [], + "severity": "HIGH", "title": "newer", "publishedAt": PUBLISHED_NEWER }, + ]}], + "canAccessPaidPatches": true, + }))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path_regex(format!( + "^/v0/orgs/{ORG}/patches/by-package/.+$" + ))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "patches": [ + { "uuid": UUID_OLDER, "purl": PURL, "publishedAt": PUBLISHED_OLDER, + "description": "older", "license": "MIT", "tier": "free", + "vulnerabilities": { "GHSA-4hjh-wcwx-xvwj": { + "cves": ["CVE-2025-58754"], "summary": "s", + "severity": "HIGH", "description": "d" }}}, + { "uuid": UUID_NEWER, "purl": PURL, "publishedAt": PUBLISHED_NEWER, + "description": "newer", "license": "MIT", "tier": "free", + "vulnerabilities": { "GHSA-jr5f-v2jv-69x6": { + "cves": ["CVE-2025-27152"], "summary": "s", + "severity": "HIGH", "description": "d" }}}, + ], + "canAccessPaidPatches": true, + }))) + .mount(&server) + .await; + mock_view_for(&server, UUID_OLDER, PUBLISHED_OLDER).await; + mock_view_for(&server, UUID_NEWER, PUBLISHED_NEWER).await; + + let tmp = tempfile::tempdir().unwrap(); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "in-proc-scan", "1.0.0"); + let mut args = default_args(tmp.path()); + args.common.api_url = Some(server.uri()); + args.apply = true; + + run_scrubbed(args).await; + + let reqs = recorded(&server).await; + assert_eq!( + view_gets(&reqs, UUID_NEWER), + 1, + "the more recently published patch must be the one fetched" + ); + assert_eq!( + view_gets(&reqs, UUID_OLDER), + 0, + "the older patch must not be fetched" + ); + + // Provenance: the manifest's `exportedAt` must be the SELECTED patch's + // own publish date. A wrong value here would mean the record and the + // blob came from different patches. + let manifest: serde_json::Value = serde_json::from_str( + &std::fs::read_to_string(tmp.path().join(".socket/manifest.json")).unwrap(), + ) + .unwrap(); + assert_eq!(manifest["patches"][PURL]["uuid"], UUID_NEWER); + assert_eq!( + manifest["patches"][PURL]["exportedAt"], PUBLISHED_NEWER, + "exportedAt must carry the selected patch's own publishedAt; got {manifest}" + ); +} + +// The JSON `updates[]` counterpart to these two — that the candidate UUID +// scan *reports* is the one apply *installs* — needs stdout, so it lives in +// the subprocess suite as +// `scan_invariants::scan_update_candidate_is_the_highest_ranked_patch`. + // --------------------------------------------------------------------------- // --prune (without --apply) // --------------------------------------------------------------------------- diff --git a/crates/socket-patch-cli/tests/scan_invariants.rs b/crates/socket-patch-cli/tests/scan_invariants.rs index 8b3045e0..ad1b1bf5 100644 --- a/crates/socket-patch-cli/tests/scan_invariants.rs +++ b/crates/socket-patch-cli/tests/scan_invariants.rs @@ -295,6 +295,94 @@ async fn scan_emits_updates_entry_when_newer_uuid_available() { assert_single_batch_carries_purl(&reqs, purl); } +#[tokio::test] +async fn scan_update_candidate_is_the_highest_ranked_patch() { + // `updates[].newUuid` must name the patch `--apply` would install — + // the highest-ranked one (merged → severity → recency), NOT whatever + // the server listed first. The two are computed by different code over + // different API shapes (`detect_updates` over the batch response, + // `select_patches` over by-package), so they can drift. + // + // The fixture is the reported bug in miniature: the low-severity patch + // is listed first AND is the more recently published, but the critical + // one must win. Severities are uppercase and dates are RFC 2822, as + // production emits them. + let mock = MockServer::start().await; + let purl = "pkg:npm/minimist@1.2.2"; + let manifest_uuid = "11111111-1111-4111-8111-111111111111"; + let low_uuid = "22222222-2222-4222-8222-222222222222"; + let critical_uuid = "99999999-9999-4999-8999-999999999999"; + Mock::given(method("POST")) + .and(path(format!("/v0/orgs/{ORG_SLUG}/patches/batch"))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "packages": [{ + "purl": purl, + "patches": [ + { + "uuid": low_uuid, "purl": purl, "tier": "free", + "cveIds": [], "ghsaIds": [], + "severity": "LOW", "title": "Low, but newest", + "publishedAt": "Mon, 03 Aug 2026 20:23:06 GMT", + }, + { + "uuid": critical_uuid, "purl": purl, "tier": "free", + "cveIds": [], "ghsaIds": [], + "severity": "CRITICAL", "title": "Critical, but older", + "publishedAt": "Wed, 01 Jan 2025 00:00:00 GMT", + } + ] + }], + "canAccessPaidPatches": true, + }))) + .mount(&mock) + .await; + + let tmp = tempfile::tempdir().expect("tempdir"); + write_root_package_json(tmp.path()); + write_npm_package(tmp.path(), "minimist", "1.2.2"); + let socket = tmp.path().join(".socket"); + std::fs::create_dir_all(&socket).unwrap(); + std::fs::write( + socket.join("manifest.json"), + format!( + r#"{{ + "patches": {{ + "{purl}": {{ + "uuid": "{manifest_uuid}", + "exportedAt": "2024-01-01T00:00:00Z", + "files": {{}}, + "vulnerabilities": {{}}, + "description": "old", + "license": "MIT", + "tier": "free" + }} + }} +}}"# + ), + ) + .unwrap(); + + let (code, stdout, _) = run_scan(tmp.path(), &mock.uri(), &[]); + assert_eq!(code, 0); + let v: serde_json::Value = serde_json::from_str(stdout.trim()).expect("valid JSON"); + let updates = v["updates"].as_array().expect("updates array"); + assert_eq!(updates.len(), 1, "one PURL changed UUID; got {v}"); + assert_eq!( + updates[0]["newUuid"], critical_uuid, + "the update candidate must be the critical patch, not the newer low one; got {v}" + ); + + // The `packages[].patches` array the operator reads is ordered the same + // way, so the listing and the decision agree. + let listed = v["packages"][0]["patches"] + .as_array() + .expect("patches array"); + assert_eq!( + listed[0]["uuid"], critical_uuid, + "listed patches must be best-first; got {v}" + ); +} + // --------------------------------------------------------------------------- // Discovery — no manifest, no `updates` field (nothing to diff against) // --------------------------------------------------------------------------- diff --git a/crates/socket-patch-core/src/api/client.rs b/crates/socket-patch-core/src/api/client.rs index dceda68b..5deef385 100644 --- a/crates/socket-patch-core/src/api/client.rs +++ b/crates/socket-patch-core/src/api/client.rs @@ -4,6 +4,11 @@ use reqwest::header::{self, HeaderMap, HeaderValue}; use reqwest::StatusCode; use serde::Serialize; +// Severity order for sorting (most severe = lowest number). This file used +// to carry its own copy of the ladder, which is how three slightly +// different ones came to exist; there is now exactly one, in `api::ranking`. +use crate::api::ranking::severity_order as get_severity_order; +use crate::api::ranking::{cmp_batch_infos, cmp_search_results}; use crate::api::types::*; use crate::constants::USER_AGENT as USER_AGENT_VALUE; use crate::utils::env_compat::{is_debug_enabled, proxy_url_from_env}; @@ -16,18 +21,6 @@ fn debug_log(message: &str) { } } -/// Severity order for sorting (most severe = lowest number). -fn get_severity_order(severity: Option<&str>) -> u8 { - match severity.map(|s| s.to_lowercase()).as_deref() { - Some("critical") => 0, - Some("high") => 1, - // GHSA emits `moderate` for the medium tier. - Some("medium") | Some("moderate") => 2, - Some("low") => 3, - _ => 4, - } -} - /// Options for constructing an [`ApiClient`]. #[derive(Debug, Clone)] pub struct ApiClientOptions { @@ -226,11 +219,15 @@ impl ApiClient { let slug = org_slug.or(self.org_slug.as_deref()).unwrap_or("default"); format!("/v0/orgs/{slug}/patches/{route}/{encoded}") }; - let result = self.get_json::(&path).await?; - Ok(result.unwrap_or_else(|| SearchResponse { - patches: Vec::new(), - can_access_paid_patches: false, - })) + let mut result = self + .get_json::(&path) + .await? + .unwrap_or_else(|| SearchResponse { + patches: Vec::new(), + can_access_paid_patches: false, + }); + result.patches.sort_by(cmp_search_results); + Ok(result) } /// Search patches by CVE ID. @@ -275,6 +272,9 @@ impl ApiClient { /// when the deployed proxy predates the batch endpoint. /// /// Maximum 500 PURLs per request. + /// + /// Every return path is normalized through [`sort_batch_response`], so + /// callers may rely on each package's `patches` being best-first. pub async fn search_patches_batch( &self, org_slug: Option<&str>, @@ -287,23 +287,27 @@ impl ApiClient { let result = self .post_json::(&path, &body) .await?; - return Ok(result.unwrap_or_else(|| BatchSearchResponse { + let mut result = result.unwrap_or_else(|| BatchSearchResponse { packages: Vec::new(), can_access_paid_patches: false, - })); + }); + sort_batch_response(&mut result); + return Ok(result); } // Public proxy: prefer the POST /patch/batch endpoint; degrade to // individual per-package GET requests when the deployed proxy // predates it or when batch validation rejects the chunk (see // `proxy_batch_post` for the decision table). - match self.proxy_batch_post(purls).await? { - Some(response) => Ok(response), + let mut response = match self.proxy_batch_post(purls).await? { + Some(response) => response, None => { self.search_patches_batch_via_individual_queries(purls) - .await + .await? } - } + }; + sort_batch_response(&mut response); + Ok(response) } /// Resolve hosted-patch references for a set of published-patch UUIDs @@ -877,12 +881,8 @@ impl ApiClient { ))); } } - match crate::utils::http::read_capped( - resp, - MAX_VENDOR_PACKAGE_BYTES, - "vendor package", - ) - .await + match crate::utils::http::read_capped(resp, MAX_VENDOR_PACKAGE_BYTES, "vendor package") + .await { Ok(bytes) => ServeDownload::Ok(bytes), Err(e) => ServeDownload::Failed(ApiError::Network(e)), @@ -1476,9 +1476,29 @@ fn convert_search_result_to_batch_info(patch: PatchSearchResult) -> BatchPatchIn ghsa_ids, severity: highest_severity, title, + // Carry the timestamp through. The batch shape does not require it, + // but dropping it here would cost this path the recency tiebreak in + // `ranking` — and it is the one path where we definitely have it. + published_at: Some(patch.published_at), + merged: patch.merged, } } +/// Put every package's patch list into canonical best-first order, and the +/// packages themselves into PURL order. +/// +/// Applied to every [`BatchSearchResponse`] the client returns, so server +/// ordering — or, on the fallback path, `JoinSet` completion order — never +/// reaches a caller. `scan` renders `packages[].patches` straight to the +/// operator and treats the leading entry as the patch apply will install; +/// both only hold because of this. +fn sort_batch_response(response: &mut BatchSearchResponse) { + for pkg in &mut response.packages { + pkg.patches.sort_by(cmp_batch_infos); + } + response.packages.sort_by(|a, b| a.purl.cmp(&b.purl)); +} + /// Assemble a [`BatchSearchResponse`] from the per-PURL [`SearchResponse`]s /// gathered by the public-proxy fallback (one GET per package). /// @@ -1671,6 +1691,7 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: vulns, + merged: false, }; let info = convert_search_result_to_batch_info(patch); @@ -1743,6 +1764,7 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: vulns, + merged: false, } } @@ -2444,6 +2466,7 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: HashMap::new(), + merged: false, }) .collect(), can_access_paid_patches, diff --git a/crates/socket-patch-core/src/api/mod.rs b/crates/socket-patch-core/src/api/mod.rs index f81f6c4e..b27a33bc 100644 --- a/crates/socket-patch-core/src/api/mod.rs +++ b/crates/socket-patch-core/src/api/mod.rs @@ -1,3 +1,4 @@ pub mod blob_fetcher; pub mod client; +pub mod ranking; pub mod types; diff --git a/crates/socket-patch-core/src/api/ranking.rs b/crates/socket-patch-core/src/api/ranking.rs new file mode 100644 index 00000000..f7734a34 --- /dev/null +++ b/crates/socket-patch-core/src/api/ranking.rs @@ -0,0 +1,612 @@ +//! Canonical ordering for patches available on a single package. +//! +//! When a package has more than one available patch, exactly one gets +//! applied (the manifest holds one patch record per PURL). This module is +//! the single place that decides which, and the single place that decides +//! how patch lists are presented. Every listing the CLI prints, every JSON +//! array it emits, and the actual apply-time selection all derive from the +//! comparators here, so the user can never be shown one ordering and handed +//! a different patch. +//! +//! **The order, best first:** +//! +//! 1. **Merged patches** — the fix has landed upstream, so it is the one +//! the ecosystem is converging on. +//! 2. **Severity** — critical > high > medium/moderate > low > unknown, +//! taken as the worst severity across everything the patch fixes. +//! 3. **Patch publish date**, most recent first. This is the date *the +//! patch* was published, never the date the upstream package version +//! was released — a 2020 package routinely carries a patch published +//! last week, and two patches for one package have two different dates. +//! See [`crate::api::types::PatchResponse::published_at`]. +//! 4. Paid tier, then UUID — pure tiebreaks, present only so the order is +//! total and therefore reproducible run to run. +//! +//! Note what is *not* in the list: `tier` is an access filter, not a +//! ranking signal. A free critical patch outranks a paid low one. + +use std::cmp::{Ordering, Reverse}; + +use crate::api::types::{BatchPatchInfo, PatchSearchResult}; +use crate::utils::date::parse_timestamp_secs; + +/// Severity ordering for sorting: **most severe = lowest number**. +/// +/// The single severity ladder for the whole workspace. GHSA emits +/// `moderate` where the Socket API emits `medium`; they are the same tier. +/// Live payloads are uppercase (`"CRITICAL"`), so matching is +/// case-insensitive. Anything unrecognized — including `None` — ranks below +/// `low`, so a patch with no severity information never outranks one that +/// has some. +pub fn severity_order(severity: Option<&str>) -> u8 { + match severity.map(|s| s.to_ascii_lowercase()).as_deref() { + Some("critical") => 0, + Some("high") => 1, + Some("medium") | Some("moderate") => 2, + Some("low") => 3, + _ => 4, + } +} + +/// Worst (lowest-numbered) severity across an iterator of severity labels. +/// An empty iterator yields the unknown rank, matching `severity_order(None)`. +pub fn max_severity_order<'a>(severities: impl Iterator) -> u8 { + severities + .map(|s| severity_order(Some(s))) + .min() + .unwrap_or_else(|| severity_order(None)) +} + +/// The comparable ranking key. Sorting ascending puts the best patch first. +/// +/// Kept as an explicit tuple-shaped struct rather than an ad-hoc tuple so +/// the two entry points below cannot drift in field order, and so the +/// meaning of each position is documented in one place. +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord)] +struct RankKey<'a> { + /// `false` sorts first, so this is negated: merged patches lead. + not_merged: bool, + /// 0 = critical … 4 = unknown. + severity: u8, + /// Newest **patch** first — the patch's own publication date, not the + /// package's release date. Unparseable or absent timestamps collapse + /// to 0 and therefore sort last: the right treatment for a date we + /// cannot trust, and the reason this is epoch seconds rather than the + /// raw string (see [`crate::utils::date`]). + patch_published: Reverse, + /// `false` sorts first, so paid leads. A tiebreak only: it can never + /// override severity or recency. + not_paid: bool, + /// Total-order backstop. Without it, two patches identical in every + /// ranked dimension would keep their incoming (server / HashMap) order + /// and the CLI's output would not be reproducible. + uuid: &'a str, +} + +fn rank_search_result(p: &PatchSearchResult) -> RankKey<'_> { + RankKey { + not_merged: !p.merged, + severity: max_severity_order(p.vulnerabilities.values().map(|v| v.severity.as_str())), + patch_published: Reverse(parse_timestamp_secs(&p.published_at).unwrap_or(0)), + not_paid: p.tier != "paid", + uuid: &p.uuid, + } +} + +fn rank_batch_info(p: &BatchPatchInfo) -> RankKey<'_> { + RankKey { + not_merged: !p.merged, + severity: severity_order(p.severity.as_deref()), + patch_published: Reverse( + p.published_at + .as_deref() + .and_then(parse_timestamp_secs) + .unwrap_or(0), + ), + not_paid: p.tier != "paid", + uuid: &p.uuid, + } +} + +/// Compare two search results best-first. Pass straight to `sort_by`. +pub fn cmp_search_results(a: &PatchSearchResult, b: &PatchSearchResult) -> Ordering { + rank_search_result(a).cmp(&rank_search_result(b)) +} + +/// Compare two batch-shaped patches best-first. Pass straight to `sort_by`. +/// +/// Ranks on the same key as [`cmp_search_results`], but the batch shape +/// carries a server-computed max `severity` instead of a vulnerability map, +/// and may omit `publishedAt` entirely. +pub fn cmp_batch_infos(a: &BatchPatchInfo, b: &BatchPatchInfo) -> Ordering { + rank_batch_info(a).cmp(&rank_batch_info(b)) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::api::types::VulnerabilityResponse; + use std::collections::HashMap; + + fn vulns(entries: &[(&str, &str)]) -> HashMap { + entries + .iter() + .map(|(id, sev)| { + ( + (*id).to_string(), + VulnerabilityResponse { + cves: Vec::new(), + summary: String::new(), + severity: (*sev).to_string(), + description: String::new(), + }, + ) + }) + .collect() + } + + fn search( + uuid: &str, + tier: &str, + published: &str, + severity: &str, + merged: bool, + ) -> PatchSearchResult { + PatchSearchResult { + uuid: uuid.to_string(), + purl: "pkg:npm/foo@1.0.0".to_string(), + published_at: published.to_string(), + description: String::new(), + license: "MIT".to_string(), + tier: tier.to_string(), + vulnerabilities: vulns(&[("GHSA-aaaa-aaaa-aaaa", severity)]), + merged, + } + } + + fn batch( + uuid: &str, + tier: &str, + published: Option<&str>, + severity: Option<&str>, + merged: bool, + ) -> BatchPatchInfo { + BatchPatchInfo { + uuid: uuid.to_string(), + purl: "pkg:npm/foo@1.0.0".to_string(), + tier: tier.to_string(), + cve_ids: Vec::new(), + ghsa_ids: Vec::new(), + severity: severity.map(str::to_string), + title: String::new(), + published_at: published.map(str::to_string), + merged, + } + } + + /// Sort and return the winning uuid. + fn best_search(mut patches: Vec) -> String { + patches.sort_by(cmp_search_results); + patches[0].uuid.clone() + } + + fn best_batch(mut patches: Vec) -> String { + patches.sort_by(cmp_batch_infos); + patches[0].uuid.clone() + } + + // ── severity_order ──────────────────────────────────────────────── + + #[test] + fn severity_ladder_is_ordered_worst_first() { + assert!(severity_order(Some("critical")) < severity_order(Some("high"))); + assert!(severity_order(Some("high")) < severity_order(Some("medium"))); + assert!(severity_order(Some("medium")) < severity_order(Some("low"))); + assert!(severity_order(Some("low")) < severity_order(None)); + assert_eq!(severity_order(Some("unknown")), severity_order(None)); + } + + #[test] + fn severity_ladder_is_case_insensitive() { + // Live API payloads are uppercase: `"severity": "HIGH"`. + for s in ["CRITICAL", "Critical", "critical"] { + assert_eq!(severity_order(Some(s)), 0, "input={s}"); + } + assert_eq!(severity_order(Some("HIGH")), severity_order(Some("high"))); + } + + #[test] + fn moderate_is_the_medium_tier() { + assert_eq!( + severity_order(Some("moderate")), + severity_order(Some("medium")) + ); + assert!(severity_order(Some("MODERATE")) < severity_order(Some("low"))); + } + + #[test] + fn max_severity_order_takes_the_worst() { + assert_eq!( + max_severity_order(["low", "critical", "high"].into_iter()), + 0 + ); + assert_eq!(max_severity_order(["low", "medium"].into_iter()), 2); + assert_eq!(max_severity_order([].into_iter()), severity_order(None)); + } + + // ── Rank key precedence ─────────────────────────────────────────── + + #[test] + fn merged_outranks_a_more_severe_unmerged_patch() { + // Rule 1 beats rule 2: a merged low patch leads a critical one. + assert_eq!( + best_search(vec![ + search("crit", "free", "2026-01-01T00:00:00Z", "critical", false), + search("merged", "free", "2020-01-01T00:00:00Z", "low", true), + ]), + "merged" + ); + } + + #[test] + fn severity_outranks_recency() { + // The reported bug: the newest patch fixes a `low`, an older one + // fixes a `critical`. Critical must win. + assert_eq!( + best_search(vec![ + search("newest_low", "free", "2026-08-01T00:00:00Z", "low", false), + search( + "older_crit", + "free", + "2020-01-01T00:00:00Z", + "critical", + false + ), + ]), + "older_crit" + ); + } + + #[test] + fn severity_outranks_tier() { + // A free critical must beat a paid low. `tier` gates access, it + // does not rank. + assert_eq!( + best_search(vec![ + search("paid_low", "paid", "2026-08-01T00:00:00Z", "low", false), + search( + "free_crit", + "free", + "2020-01-01T00:00:00Z", + "critical", + false + ), + ]), + "free_crit" + ); + } + + #[test] + fn recency_breaks_severity_ties() { + // UUIDs are deliberately adversarial: `a_old` sorts first, so the + // final uuid tiebreak would pick the WRONG patch. Only a working + // date rung yields `z_new`. (Mutation-checked: stubbing the date + // out fails this test.) + assert_eq!( + best_search(vec![ + search("a_old", "free", "2024-01-01T00:00:00Z", "high", false), + search("z_new", "free", "2026-01-01T00:00:00Z", "high", false), + ]), + "z_new" + ); + } + + #[test] + fn recency_uses_the_patch_date_not_the_package_release_date() { + // Both patches are for the SAME package version (one `purl`, one + // upstream release date), yet they must still be ordered — which is + // only possible because each carries its OWN publication date. + // + // Verbatim live data: `pkg:npm/axios@1.6.0` shipped to npm on + // 2023-10-26, and has two patches published 2026-03-27 and + // 2026-08-03. If the ranking ever keyed off a package-level date, + // both keys would be equal here and the ordering would collapse to + // the UUID tiebreak — which would pick `0bc312a6` (the OLDER + // patch), not `83f5a654`. + let older = search( + "0bc312a6", + "free", + "Fri, 27 Mar 2026 19:12:42 GMT", + "high", + false, + ); + let newer = search( + "83f5a654", + "free", + "Mon, 03 Aug 2026 20:23:06 GMT", + "high", + false, + ); + assert_eq!(older.purl, newer.purl, "same package version"); + assert!( + older.uuid < newer.uuid, + "uuid tiebreak would favor the older patch, so this test is \ + non-vacuous: only a real per-patch date can produce `83f5a654`" + ); + assert_eq!(best_search(vec![older, newer]), "83f5a654"); + } + + #[test] + fn recency_is_chronological_not_lexicographic() { + // Regression: `publishedAt` is RFC 2822 on the wire, so a raw + // string compare orders by weekday name. `Wed` sorts after `Fri` + // lexicographically, so the OLDER patch used to win here. + let older = "Wed, 01 Jan 2025 00:00:00 GMT"; + let newer = "Fri, 01 Aug 2026 00:00:00 GMT"; + assert!(older > newer, "precondition: raw strings sort backwards"); + // Adversarial UUIDs so the uuid tiebreak cannot supply the right + // answer by accident. + assert_eq!( + best_search(vec![ + search("a_older", "free", older, "high", false), + search("z_newer", "free", newer, "high", false), + ]), + "z_newer" + ); + } + + #[test] + fn unparseable_dates_sort_last_without_disturbing_severity() { + // A garbage timestamp must not promote a patch, but it also must + // not demote it below a less severe one. + assert_eq!( + best_search(vec![ + search("dated_high", "free", "2026-01-01T00:00:00Z", "high", false), + search("undated_crit", "free", "not a date", "critical", false), + ]), + "undated_crit" + ); + assert_eq!( + best_search(vec![ + search("undated", "free", "", "high", false), + search("dated", "free", "2020-01-01T00:00:00Z", "high", false), + ]), + "dated" + ); + } + + #[test] + fn date_outranks_the_tier_and_uuid_tiebreaks() { + // Pins the RUNG ORDER below severity: a newer FREE patch with a + // late-sorting uuid must still beat an older PAID one with an + // early-sorting uuid. Both lower tiebreaks point the wrong way, so + // this fails the moment the date rung stops working or is demoted. + assert_eq!( + best_search(vec![ + search("a_old_paid", "paid", "2024-01-01T00:00:00Z", "high", false), + search("z_new_free", "free", "2026-01-01T00:00:00Z", "high", false), + ]), + "z_new_free" + ); + } + + #[test] + fn tier_breaks_ties_after_date() { + assert_eq!( + best_search(vec![ + search("free", "free", "2026-01-01T00:00:00Z", "high", false), + search("paid", "paid", "2026-01-01T00:00:00Z", "high", false), + ]), + "paid" + ); + } + + #[test] + fn uuid_is_a_deterministic_final_tiebreak() { + // Two patches identical in every ranked dimension must still land + // in a fixed order — otherwise `scan --json` is not reproducible. + let a = search("aaaa", "free", "2026-01-01T00:00:00Z", "high", false); + let z = search("zzzz", "free", "2026-01-01T00:00:00Z", "high", false); + assert_eq!(best_search(vec![z.clone(), a.clone()]), "aaaa"); + assert_eq!(best_search(vec![a, z]), "aaaa"); + } + + #[test] + fn full_precedence_chain_in_one_sort() { + let mut patches = [ + search("d_low_new", "paid", "2026-08-01T00:00:00Z", "low", false), + search( + "b_crit_old", + "free", + "2020-01-01T00:00:00Z", + "critical", + false, + ), + search("a_merged", "free", "2019-01-01T00:00:00Z", "low", true), + search("c_high_new", "free", "2026-01-01T00:00:00Z", "high", false), + ]; + patches.sort_by(cmp_search_results); + let order: Vec<&str> = patches.iter().map(|p| p.uuid.as_str()).collect(); + assert_eq!(order, ["a_merged", "b_crit_old", "c_high_new", "d_low_new"]); + } + + #[test] + fn worst_vulnerability_in_the_map_drives_severity() { + let mixed = PatchSearchResult { + vulnerabilities: vulns(&[("GHSA-a", "low"), ("GHSA-b", "critical")]), + ..search("mixed", "free", "2020-01-01T00:00:00Z", "low", false) + }; + let high = search("high_only", "free", "2026-01-01T00:00:00Z", "high", false); + // `mixed` is older but carries a critical — it must win. + assert_eq!(best_search(vec![high, mixed]), "mixed"); + } + + #[test] + fn patch_with_no_vulnerabilities_ranks_below_one_with_a_low() { + let none = PatchSearchResult { + vulnerabilities: HashMap::new(), + ..search("no_vulns", "free", "2026-08-01T00:00:00Z", "low", false) + }; + let low = search("has_low", "free", "2020-01-01T00:00:00Z", "low", false); + assert_eq!(best_search(vec![none, low]), "has_low"); + } + + // ── Batch shape parity ──────────────────────────────────────────── + + #[test] + fn batch_ranking_matches_search_ranking() { + assert_eq!( + best_batch(vec![ + batch( + "newest_low", + "free", + Some("2026-08-01T00:00:00Z"), + Some("low"), + false + ), + batch( + "older_crit", + "free", + Some("2020-01-01T00:00:00Z"), + Some("critical"), + false + ), + ]), + "older_crit" + ); + assert_eq!( + best_batch(vec![ + batch( + "crit", + "free", + Some("2026-01-01T00:00:00Z"), + Some("critical"), + false + ), + batch( + "merged", + "free", + Some("2020-01-01T00:00:00Z"), + Some("low"), + true + ), + ]), + "merged" + ); + } + + #[test] + fn batch_recency_uses_the_patch_date_not_the_package_release_date() { + // Batch-shape twin of + // `recency_uses_the_patch_date_not_the_package_release_date`: one + // package version, two patches, ordered by their own publish + // dates. `u_aaa` sorts first by UUID, so only a real per-patch + // date can produce `u_zzz`. + assert_eq!( + best_batch(vec![ + batch( + "u_aaa", + "free", + Some("Fri, 27 Mar 2026 19:12:42 GMT"), + Some("HIGH"), + false + ), + batch( + "u_zzz", + "free", + Some("Mon, 03 Aug 2026 20:23:06 GMT"), + Some("HIGH"), + false + ), + ]), + "u_zzz" + ); + } + + #[test] + fn same_patch_dates_across_different_packages_do_not_interact() { + // Ranking is computed per patch and is blind to the package: two + // patches sharing a publish date rank identically regardless of + // which purl they belong to. Guards against anyone "optimizing" + // the key to be derived from package-level state. + let mut a = search("u1", "free", "2026-01-01T00:00:00Z", "high", false); + let mut b = search("u2", "free", "2026-01-01T00:00:00Z", "high", false); + let same_purl = cmp_search_results(&a, &b); + a.purl = "pkg:npm/alpha@1.0.0".to_string(); + b.purl = "pkg:npm/omega@9.9.9".to_string(); + assert_eq!( + same_purl, + cmp_search_results(&a, &b), + "changing the package must not change the relative rank" + ); + } + + #[test] + fn batch_without_published_at_still_ranks_by_severity() { + // The batch endpoint historically omits `publishedAt`; losing the + // recency tiebreak must not cost us the severity ordering. + assert_eq!( + best_batch(vec![ + batch("low", "free", None, Some("low"), false), + batch("crit", "free", None, Some("critical"), false), + ]), + "crit" + ); + } + + #[test] + fn batch_missing_severity_ranks_last() { + assert_eq!( + best_batch(vec![ + batch("unknown", "free", Some("2026-08-01T00:00:00Z"), None, false), + batch( + "low", + "free", + Some("2020-01-01T00:00:00Z"), + Some("low"), + false + ), + ]), + "low" + ); + } + + #[test] + fn batch_ordering_is_total_and_deterministic() { + let all = || { + vec![ + batch("u3", "free", None, None, false), + batch( + "u1", + "paid", + Some("2026-01-01T00:00:00Z"), + Some("high"), + false, + ), + batch( + "u2", + "free", + Some("2026-01-01T00:00:00Z"), + Some("high"), + false, + ), + batch( + "u0", + "free", + Some("2020-01-01T00:00:00Z"), + Some("critical"), + true, + ), + ] + }; + let mut first = all(); + first.sort_by(cmp_batch_infos); + let mut second = all(); + second.reverse(); + second.sort_by(cmp_batch_infos); + let ids = + |v: &[BatchPatchInfo]| -> Vec { v.iter().map(|p| p.uuid.clone()).collect() }; + assert_eq!(ids(&first), ids(&second)); + assert_eq!(ids(&first), ["u0", "u1", "u2", "u3"]); + } +} diff --git a/crates/socket-patch-core/src/api/types.rs b/crates/socket-patch-core/src/api/types.rs index 38fc0aa0..999e4f1c 100644 --- a/crates/socket-patch-core/src/api/types.rs +++ b/crates/socket-patch-core/src/api/types.rs @@ -25,12 +25,43 @@ pub struct OrganizationsResponse { pub struct PatchResponse { pub uuid: String, pub purl: String, + /// When **this patch** was published by Socket — NOT when the upstream + /// package version was released. The distinction matters because patch + /// selection ranks by recency: a 2020 package can carry a patch + /// published last week. + /// + /// Confirmed per-patch against the live API: `pkg:npm/axios@1.6.0` has + /// two patches dated 2026-03-27 and 2026-08-03 while the package itself + /// shipped 2023-10-26, and `pkg:pypi/urllib3@1.26.18` carries three + /// patches with three distinct dates. + /// + /// **RFC 2822 / HTTP-date on the wire** — the live API emits + /// `Fri, 27 Mar 2026 19:12:42 GMT` (verified across npm, PyPI, cargo + /// and gem), while this repo's fixtures use RFC 3339. Never compare + /// these as raw strings; route through + /// [`crate::utils::date::parse_timestamp_secs`], which handles both. pub published_at: String, pub files: HashMap, pub vulnerabilities: HashMap, pub description: String, pub license: String, pub tier: String, + /// Upstream-merge marker: this patch's fix has landed upstream. + /// Merged patches outrank everything else in patch selection (see + /// [`crate::api::ranking`]). + /// + /// Not yet emitted by any endpoint, so it defaults to `false` rather + /// than being required, and the deserializer tolerates whichever + /// spelling and type the server settles on. + #[serde( + default, + alias = "isMerged", + alias = "mergedAt", + alias = "upstreamMerged", + deserialize_with = "crate::utils::serde::de_truthy_flag", + skip_serializing_if = "crate::utils::serde::is_false" + )] + pub merged: bool, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -57,11 +88,24 @@ pub struct VulnerabilityResponse { pub struct PatchSearchResult { pub uuid: String, pub purl: String, + /// When **this patch** was published — not the package's release date. + /// See [`PatchResponse::published_at`] for the full semantics and the + /// wire-format caveat. pub published_at: String, pub description: String, pub license: String, pub tier: String, pub vulnerabilities: HashMap, + /// Upstream-merge marker — see [`PatchResponse::merged`]. + #[serde( + default, + alias = "isMerged", + alias = "mergedAt", + alias = "upstreamMerged", + deserialize_with = "crate::utils::serde::de_truthy_flag", + skip_serializing_if = "crate::utils::serde::is_false" + )] + pub merged: bool, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -82,6 +126,25 @@ pub struct BatchPatchInfo { pub ghsa_ids: Vec, pub severity: Option, pub title: String, + /// When **this patch** was published (see + /// [`PatchResponse::published_at`]), if the server supplies it. The + /// batch shape historically omits it, which is why it is optional — a + /// `None` here only weakens the recency tiebreak in + /// [`crate::api::ranking`], it never changes the merged/severity + /// ordering. The public-proxy fallback path fills it in from the + /// per-package search results. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub published_at: Option, + /// Upstream-merge marker — see [`PatchResponse::merged`]. + #[serde( + default, + alias = "isMerged", + alias = "mergedAt", + alias = "upstreamMerged", + deserialize_with = "crate::utils::serde::de_truthy_flag", + skip_serializing_if = "crate::utils::serde::is_false" + )] + pub merged: bool, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -171,6 +234,7 @@ mod tests { description: "desc".into(), license: "MIT".into(), tier: "free".into(), + merged: false, }; let json = serde_json::to_string(&pr).unwrap(); assert!(json.contains("publishedAt")); @@ -242,6 +306,8 @@ mod tests { ghsa_ids: vec!["GHSA-1111-2222-3333".into()], severity: Some("high".into()), title: "Test".into(), + published_at: None, + merged: false, }], }], can_access_paid_patches: false, @@ -263,6 +329,8 @@ mod tests { ghsa_ids: vec!["GHSA-1111-2222-3333".into()], severity: Some("high".into()), title: "Test".into(), + published_at: None, + merged: false, }; let json = serde_json::to_string(&bpi).unwrap(); assert!(json.contains("cveIds")); @@ -298,6 +366,7 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: HashMap::new(), + merged: false, }; let json = serde_json::to_string(&psr).unwrap(); let back: PatchSearchResult = serde_json::from_str(&json).unwrap(); @@ -532,6 +601,7 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: HashMap::new(), + merged: false, }], can_access_paid_patches: true, }; @@ -563,4 +633,72 @@ mod tests { assert!(!sr.can_access_paid_patches); assert_eq!(sr.patches[0].published_at, "2024-01-01T00:00:00Z"); } + + #[test] + fn published_at_is_per_patch_not_per_package() { + // Verbatim production payload for + // `GET /patch/by-package/pkg%3Anpm%2Faxios%401.6.0`, captured + // 2026-08-04 (description/summary bodies elided). + // + // The point: ONE package version, TWO patches, TWO different + // `publishedAt` values. That is only possible if the field + // describes the patch. If it ever described the package release + // (axios@1.6.0 shipped 2023-10-26) both entries would carry the + // same date, and `api::ranking`'s recency rung would silently + // collapse into the UUID tiebreak. + let json = r#"{ + "canAccessPaidPatches": false, + "patches": [ + { + "uuid": "0bc312a6-1b43-46bb-ba83-95b53867deb3", + "purl": "pkg:npm/axios@1.6.0", + "publishedAt": "Fri, 27 Mar 2026 19:12:42 GMT", + "description": "", "license": "MIT", "tier": "free", + "vulnerabilities": { + "GHSA-4hjh-wcwx-xvwj": { + "cves": ["CVE-2025-58754"], + "summary": "DoS through lack of data size check", + "severity": "HIGH", + "description": "" + } + } + }, + { + "uuid": "83f5a654-db80-4086-aa3d-593036fe7c7d", + "purl": "pkg:npm/axios@1.6.0", + "publishedAt": "Mon, 03 Aug 2026 20:23:06 GMT", + "description": "", "license": "", "tier": "free", + "vulnerabilities": { + "GHSA-jr5f-v2jv-69x6": { + "cves": ["CVE-2025-27152"], + "summary": "Possible SSRF via absolute URL", + "severity": "HIGH", + "description": "" + } + } + } + ] + }"#; + let sr: SearchResponse = serde_json::from_str(json).unwrap(); + assert_eq!(sr.patches.len(), 2); + assert_eq!( + sr.patches[0].purl, sr.patches[1].purl, + "fixture must be two patches for the SAME package version" + ); + assert_ne!( + sr.patches[0].published_at, sr.patches[1].published_at, + "publishedAt must vary per patch, not per package" + ); + assert_eq!(sr.patches[0].published_at, "Fri, 27 Mar 2026 19:12:42 GMT"); + assert_eq!(sr.patches[1].published_at, "Mon, 03 Aug 2026 20:23:06 GMT"); + // Production spells severity in uppercase; every ladder in the + // workspace lowercases before matching. + assert_eq!( + sr.patches[0].vulnerabilities["GHSA-4hjh-wcwx-xvwj"].severity, + "HIGH" + ); + // No `merged` key on the wire today -> false, not a parse error. + assert!(!sr.patches[0].merged); + assert!(!sr.patches[1].merged); + } } diff --git a/crates/socket-patch-core/src/utils/date.rs b/crates/socket-patch-core/src/utils/date.rs new file mode 100644 index 00000000..19f6c6c4 --- /dev/null +++ b/crates/socket-patch-core/src/utils/date.rs @@ -0,0 +1,448 @@ +//! Minimal timestamp parser for the `publishedAt` field on patch records. +//! +//! The Socket patch API serves `publishedAt` as an **RFC 2822 / HTTP-date** +//! string — `Fri, 27 Mar 2026 19:12:42 GMT` — verified live across npm, +//! PyPI, cargo and gem. Test fixtures throughout this repo use RFC 3339 +//! (`2026-03-27T19:12:42Z`) instead, so both spellings must parse. +//! +//! This matters because these strings are *ordered*: patch selection ranks +//! by publish date, and comparing the RFC 2822 form as a raw string sorts +//! by day-of-week name (`Fri` < `Mon` < `Sat` < `Sun` < `Thu` < `Tue` < +//! `Wed`), not chronologically. Converting to epoch seconds first is the +//! only way to get a correct order. +//! +//! Doing this by hand avoids a chrono/jiff dependency, matching the +//! existing hand-rolled formatter in [`crate::vex::time`]. + +/// Parse a patch `publishedAt` timestamp into UNIX epoch seconds (UTC). +/// +/// Accepts, in the order tried: +/// +/// - RFC 2822 / HTTP-date: `Fri, 27 Mar 2026 19:12:42 GMT` (the format +/// production actually emits). The leading day-of-week is optional and +/// never validated — it is redundant with the date and servers get it +/// wrong often enough that rejecting on it would be worse than ignoring +/// it. A trailing zone of `GMT` / `UTC` / `Z` / `+0000` / `-0000` is +/// accepted; any other numeric offset is applied. +/// - RFC 3339 / ISO 8601: `2026-03-27T19:12:42Z`, with optional fractional +/// seconds and an optional `±HH:MM` offset. +/// - A bare civil date: `2026-03-27` (midnight UTC). +/// +/// Returns `None` for anything else, including pre-1970 instants — callers +/// rank `None` last, which is the right treatment for a timestamp we cannot +/// trust. Never panics on malformed input. +pub fn parse_timestamp_secs(s: &str) -> Option { + let s = s.trim(); + if s.is_empty() { + return None; + } + parse_rfc2822(s).or_else(|| parse_rfc3339(s)) +} + +/// Days since 1970-01-01 for a civil (proleptic Gregorian) date. +/// +/// Howard Hinnant's `days_from_civil` (public domain): +/// . +/// This is the exact inverse of the `civil_from_days` half of +/// [`crate::vex::time::unix_to_ymdhms`]; the round-trip is pinned by +/// `days_from_civil_inverts_unix_to_ymdhms` below. +fn days_from_civil(year: i64, month: u32, day: u32) -> i64 { + let y = if month <= 2 { year - 1 } else { year }; + let era = if y >= 0 { y } else { y - 399 } / 400; + let yoe = y - era * 400; // [0, 399] + let mp = if month > 2 { month - 3 } else { month + 9 } as i64; // Mar = 0 + let doy = (153 * mp + 2) / 5 + day as i64 - 1; // [0, 365] + let doe = yoe * 365 + yoe / 4 - yoe / 100 + doy; // [0, 146096] + era * 146_097 + doe - 719_468 +} + +/// Assemble a UTC (Y, M, D, h, m, s) tuple into epoch seconds, rejecting +/// out-of-range fields and pre-1970 instants. +fn to_epoch_secs(year: i64, month: u32, day: u32, hour: u32, min: u32, sec: u32) -> Option { + if !(1..=12).contains(&month) || !(1..=31).contains(&day) { + return None; + } + // Leap seconds arrive as `:60`; clamping beats rejecting the record. + if hour > 23 || min > 59 || sec > 60 { + return None; + } + let days = days_from_civil(year, month, day); + let secs = days + .checked_mul(86_400)? + .checked_add((hour * 3600 + min * 60 + sec.min(59)) as i64)?; + u64::try_from(secs).ok() +} + +/// Month index (1-12) for an RFC 2822 three-letter month abbreviation. +fn month_from_abbrev(abbrev: &str) -> Option { + const MONTHS: [&str; 12] = [ + "jan", "feb", "mar", "apr", "may", "jun", "jul", "aug", "sep", "oct", "nov", "dec", + ]; + let lower = abbrev.to_ascii_lowercase(); + MONTHS + .iter() + .position(|m| *m == lower) + .map(|i| i as u32 + 1) +} + +/// Parse `[Day, ]DD Mon YYYY HH:MM[:SS] [zone]`. +/// +/// The zone is optional (absent means UTC, matching HTTP-date practice for +/// the malformed-but-common no-zone spelling). +fn parse_rfc2822(s: &str) -> Option { + // Drop the optional `Fri,` day-of-week prefix. + let rest = match s.split_once(',') { + Some((_dow, rest)) => rest, + None => s, + }; + let mut parts = rest.split_ascii_whitespace(); + + let day: u32 = parts.next()?.parse().ok()?; + let month = month_from_abbrev(parts.next()?)?; + let year: i64 = parts.next()?.parse().ok()?; + + let (hour, min, sec) = match parts.next() { + Some(time) => parse_hms(time)?, + // A bare `27 Mar 2026` is a legal enough date; treat it as midnight. + None => (0, 0, 0), + }; + + let base = to_epoch_secs(year, month, day, hour, min, sec)?; + match parts.next() { + None => Some(base), + Some(zone) => apply_zone(base, zone), + } +} + +/// Shift `base` (which was parsed as if UTC) by an RFC 2822 zone token. +/// +/// Named zones other than the UTC aliases are the obsolete RFC 822 forms; +/// per RFC 2822 §4.3 they are to be treated as `-0000`, i.e. UTC. +fn apply_zone(base: u64, zone: &str) -> Option { + let offset_secs = match zone { + "GMT" | "UTC" | "UT" | "Z" | "+0000" | "-0000" => 0, + // An unrecognized token is an obsolete RFC 822 named zone, which + // RFC 2822 §4.3 says to read as `-0000` — i.e. no shift. + _ => parse_numeric_offset(zone).unwrap_or_default(), + }; + // The parsed fields were wall-clock in `zone`; UTC is that minus the + // offset. + let shifted = (base as i64).checked_sub(offset_secs)?; + u64::try_from(shifted).ok() +} + +/// Parse `±HHMM` or `±HH:MM` into signed seconds. +fn parse_numeric_offset(zone: &str) -> Option { + let (sign, digits) = match zone.as_bytes().first()? { + b'+' => (1i64, &zone[1..]), + b'-' => (-1i64, &zone[1..]), + _ => return None, + }; + let digits: String = digits.chars().filter(|c| *c != ':').collect(); + if digits.len() != 4 || !digits.bytes().all(|b| b.is_ascii_digit()) { + return None; + } + let hours: i64 = digits[..2].parse().ok()?; + let mins: i64 = digits[2..].parse().ok()?; + Some(sign * (hours * 3600 + mins * 60)) +} + +/// Parse `HH:MM[:SS]`. +fn parse_hms(time: &str) -> Option<(u32, u32, u32)> { + let mut it = time.split(':'); + let hour: u32 = it.next()?.parse().ok()?; + let min: u32 = it.next()?.parse().ok()?; + let sec: u32 = match it.next() { + Some(s) => s.parse().ok()?, + None => 0, + }; + if it.next().is_some() { + return None; + } + Some((hour, min, sec)) +} + +/// Parse `YYYY-MM-DD[(T| )HH:MM[:SS][.fff]][Z|±HH:MM]`. +fn parse_rfc3339(s: &str) -> Option { + let (date, time) = match s.find(['T', 't', ' ']) { + Some(i) => (&s[..i], Some(&s[i + 1..])), + None => (s, None), + }; + + let mut d = date.split('-'); + let year: i64 = d.next()?.parse().ok()?; + let month: u32 = d.next()?.parse().ok()?; + let day: u32 = d.next()?.parse().ok()?; + if d.next().is_some() { + return None; + } + + let Some(time) = time else { + return to_epoch_secs(year, month, day, 0, 0, 0); + }; + + // Split the zone suffix off the clock time. + let (clock, zone) = match time.rfind(['Z', 'z', '+']) { + Some(i) => (&time[..i], Some(&time[i..])), + // A `-` can only be a zone sign here — the date half is already gone. + None => match time.rfind('-') { + Some(i) => (&time[..i], Some(&time[i..])), + None => (time, None), + }, + }; + // Fractional seconds carry no ranking signal at this granularity. + let clock = clock.split('.').next()?; + let (hour, min, sec) = parse_hms(clock)?; + + let base = to_epoch_secs(year, month, day, hour, min, sec)?; + match zone { + None | Some("Z") | Some("z") => Some(base), + Some(z) => { + let offset = parse_numeric_offset(z)?; + u64::try_from((base as i64).checked_sub(offset)?).ok() + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::vex::time::unix_to_ymdhms; + + // ── RFC 2822 / HTTP-date: the format production actually emits ── + + /// Verbatim payloads captured from + /// `GET https://patches-api.socket.dev/patch/by-package/` on + /// 2026-08-04. If this shape ever stops parsing, patch ranking + /// silently degrades to "unknown date, sorts last" for every patch. + #[test] + fn parses_live_production_published_at_strings() { + let cases = [ + ("Fri, 27 Mar 2026 19:12:42 GMT", (2026, 3, 27, 19, 12, 42)), + ("Mon, 03 Aug 2026 20:23:06 GMT", (2026, 8, 3, 20, 23, 6)), + ("Wed, 29 Jul 2026 19:39:44 GMT", (2026, 7, 29, 19, 39, 44)), + ("Thu, 19 Mar 2026 14:53:13 GMT", (2026, 3, 19, 14, 53, 13)), + ]; + for (input, expected) in cases { + let secs = parse_timestamp_secs(input).unwrap_or_else(|| panic!("failed: {input}")); + assert_eq!(unix_to_ymdhms(secs), expected, "input={input}"); + } + } + + /// The whole reason this module exists: lexicographic comparison of + /// RFC 2822 strings orders by weekday name, so an older `Wed` sorts + /// ahead of a newer `Fri`. Parsed epoch seconds must not. + #[test] + fn weekday_prefix_does_not_dominate_ordering() { + let older = "Wed, 01 Jan 2025 00:00:00 GMT"; + let newer = "Fri, 01 Aug 2026 00:00:00 GMT"; + assert!(older > newer, "precondition: raw strings sort backwards"); + assert!( + parse_timestamp_secs(older).unwrap() < parse_timestamp_secs(newer).unwrap(), + "parsed order must be chronological" + ); + } + + #[test] + fn parses_every_month_abbreviation() { + for (i, mon) in [ + "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", + ] + .iter() + .enumerate() + { + let s = format!("Mon, 15 {mon} 2026 00:00:00 GMT"); + let secs = parse_timestamp_secs(&s).unwrap_or_else(|| panic!("failed: {s}")); + let (y, m, d, ..) = unix_to_ymdhms(secs); + assert_eq!((y, m, d), (2026, i as u32 + 1, 15), "input={s}"); + } + } + + #[test] + fn month_abbreviation_is_case_insensitive() { + let a = parse_timestamp_secs("Fri, 27 MAR 2026 19:12:42 GMT").unwrap(); + let b = parse_timestamp_secs("Fri, 27 mar 2026 19:12:42 GMT").unwrap(); + let c = parse_timestamp_secs("Fri, 27 Mar 2026 19:12:42 GMT").unwrap(); + assert_eq!(a, b); + assert_eq!(b, c); + } + + #[test] + fn utc_zone_aliases_are_equivalent() { + let base = parse_timestamp_secs("Fri, 27 Mar 2026 19:12:42 GMT").unwrap(); + for zone in ["GMT", "UTC", "UT", "Z", "+0000", "-0000"] { + assert_eq!( + parse_timestamp_secs(&format!("Fri, 27 Mar 2026 19:12:42 {zone}")).unwrap(), + base, + "zone={zone}" + ); + } + } + + #[test] + fn numeric_offsets_shift_to_utc() { + let utc = parse_timestamp_secs("Fri, 27 Mar 2026 19:12:42 GMT").unwrap(); + // 19:12:42 +0200 is 17:12:42 UTC — two hours EARLIER in absolute time. + assert_eq!( + parse_timestamp_secs("Fri, 27 Mar 2026 19:12:42 +0200").unwrap(), + utc - 7200 + ); + assert_eq!( + parse_timestamp_secs("Fri, 27 Mar 2026 19:12:42 -0530").unwrap(), + utc + 5 * 3600 + 1800 + ); + } + + #[test] + fn day_of_week_prefix_is_optional_and_unvalidated() { + let with = parse_timestamp_secs("Fri, 27 Mar 2026 19:12:42 GMT").unwrap(); + assert_eq!(parse_timestamp_secs("27 Mar 2026 19:12:42 GMT"), Some(with)); + // A wrong weekday is ignored, not rejected — servers get it wrong. + assert_eq!( + parse_timestamp_secs("Tue, 27 Mar 2026 19:12:42 GMT"), + Some(with) + ); + } + + #[test] + fn rfc2822_seconds_are_optional() { + let secs = parse_timestamp_secs("Fri, 27 Mar 2026 19:12 GMT").unwrap(); + assert_eq!(unix_to_ymdhms(secs), (2026, 3, 27, 19, 12, 0)); + } + + // ── RFC 3339 / ISO 8601: the format every in-repo fixture uses ── + + #[test] + fn parses_rfc3339_fixture_format() { + let secs = parse_timestamp_secs("2024-01-01T00:00:00Z").unwrap(); + assert_eq!(unix_to_ymdhms(secs), (2024, 1, 1, 0, 0, 0)); + assert_eq!(secs, 1_704_067_200); + } + + #[test] + fn parses_rfc3339_variants() { + let base = parse_timestamp_secs("2024-05-24T12:14:56Z").unwrap(); + assert_eq!(base, 1_716_552_896); + // Lowercase separators, space separator, missing zone, fractional + // seconds — all the same instant. + for s in [ + "2024-05-24t12:14:56z", + "2024-05-24 12:14:56Z", + "2024-05-24T12:14:56", + "2024-05-24T12:14:56.123Z", + ] { + assert_eq!(parse_timestamp_secs(s), Some(base), "input={s}"); + } + // Offsets shift to UTC. + assert_eq!( + parse_timestamp_secs("2024-05-24T14:14:56+02:00"), + Some(base) + ); + assert_eq!( + parse_timestamp_secs("2024-05-24T10:14:56-02:00"), + Some(base) + ); + } + + #[test] + fn parses_bare_civil_date_as_midnight() { + assert_eq!(parse_timestamp_secs("2024-01-01"), Some(1_704_067_200)); + } + + #[test] + fn parses_leap_day() { + let secs = parse_timestamp_secs("2024-02-29T00:00:00Z").unwrap(); + assert_eq!(unix_to_ymdhms(secs), (2024, 2, 29, 0, 0, 0)); + } + + // ── Rejection ───────────────────────────────────────────────────── + + #[test] + fn rejects_unparseable_input() { + for s in [ + "", + " ", + "not a date", + "Fri, 27 Xyz 2026 19:12:42 GMT", // bad month + "Fri, 99 Mar 2026 19:12:42 GMT", // day out of range + "2024-13-01T00:00:00Z", // month out of range + "2024-01-01T25:00:00Z", // hour out of range + "2024-01-01T00:99:00Z", // minute out of range + "1969-12-31T23:59:59Z", // pre-epoch + "2024-01-01T00:00:00:00Z", // too many clock fields + "2024-01-01-01", // too many date fields + ] { + assert_eq!(parse_timestamp_secs(s), None, "should reject: {s:?}"); + } + } + + #[test] + fn surrounding_whitespace_is_tolerated() { + assert_eq!( + parse_timestamp_secs(" 2024-01-01T00:00:00Z "), + Some(1_704_067_200) + ); + } + + #[test] + fn does_not_panic_on_adversarial_input() { + // Multi-byte codepoints at every index the parsers slice on: a + // byte-index slice landing mid-codepoint would panic. + for s in [ + "日本語", + "Fri,日 27 Mar 2026", + "2024-01-01T日", + "2024-01-01日00:00:00Z", + "+", + "-", + "T", + ":::::", + "2024--01-01", + ] { + let _ = parse_timestamp_secs(s); + } + } + + // ── Cross-checks against the existing formatter ─────────────────── + + /// `days_from_civil` must invert the `civil_from_days` half of + /// `vex::time::unix_to_ymdhms` exactly. Swept across ~1265 years so + /// every leap rule and century boundary is covered. + #[test] + fn days_from_civil_inverts_unix_to_ymdhms() { + for days in 0..462_000i64 { + let (y, m, d, ..) = unix_to_ymdhms(days as u64 * 86_400); + assert_eq!( + days_from_civil(y as i64, m, d), + days, + "mismatch at day {days} ({y}-{m}-{d})" + ); + } + } + + /// Parsing must be monotonic: a later instant always yields a larger + /// epoch value, in both wire formats. Oracle-free guard against a + /// scrambled field or a dropped carry. + #[test] + fn parsed_order_is_chronological_in_both_formats() { + const MONTHS: [&str; 12] = [ + "Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec", + ]; + const STRIDE: u64 = 147_853; // ~1.71 days + let mut secs = 0u64; + let mut prev_2822 = 0u64; + while secs < 1_900_000_000 { + let (y, m, d, h, mi, s) = unix_to_ymdhms(secs); + let iso = format!("{y:04}-{m:02}-{d:02}T{h:02}:{mi:02}:{s:02}Z"); + let rfc = format!( + "Mon, {d:02} {} {y:04} {h:02}:{mi:02}:{s:02} GMT", + MONTHS[m as usize - 1] + ); + assert_eq!(parse_timestamp_secs(&iso), Some(secs), "iso={iso}"); + assert_eq!(parse_timestamp_secs(&rfc), Some(secs), "rfc={rfc}"); + assert!(secs > prev_2822 || secs == 0); + prev_2822 = secs; + secs += STRIDE; + } + } +} diff --git a/crates/socket-patch-core/src/utils/mod.rs b/crates/socket-patch-core/src/utils/mod.rs index fc28d23e..fc52ea80 100644 --- a/crates/socket-patch-core/src/utils/mod.rs +++ b/crates/socket-patch-core/src/utils/mod.rs @@ -1,8 +1,9 @@ pub mod cleanup_blobs; +pub mod date; pub mod env_compat; pub mod fs; -pub(crate) mod http; pub mod fuzzy_match; +pub(crate) mod http; pub mod process; pub mod purl; pub(crate) mod serde; diff --git a/crates/socket-patch-core/src/utils/serde.rs b/crates/socket-patch-core/src/utils/serde.rs index 7f5f288c..ce28f934 100644 --- a/crates/socket-patch-core/src/utils/serde.rs +++ b/crates/socket-patch-core/src/utils/serde.rs @@ -1,6 +1,7 @@ //! Shared serde helpers. -use serde::{Serialize, Serializer}; +use serde::de::{self, Deserializer, Unexpected}; +use serde::{Deserialize, Serialize, Serializer}; use std::collections::{BTreeMap, HashMap}; /// Serialize a `HashMap` with its keys in sorted order so the emitted JSON @@ -19,3 +20,107 @@ where { map.iter().collect::>().serialize(serializer) } + +/// `skip_serializing_if` companion for `bool` fields that default to false — +/// keeps them out of the emitted JSON entirely rather than writing +/// `"merged": false` on every record. +pub fn is_false(b: &bool) -> bool { + !*b +} + +/// Deserialize a marker flag whose on-the-wire *type* is not pinned. +/// +/// The patch API's upstream-merge marker is expected to arrive as a plain +/// `true`/`false`, but the same signal is equally likely to ship as a +/// nullable timestamp (`"mergedAt": "Fri, 27 Mar 2026 …"`). Typing the +/// field as `bool` alone would make a string payload a hard deserialize +/// error, which would take down the *entire* patch-list response — a +/// server-side field-type choice must never be able to break the client +/// that way. +/// +/// So: `true` / a non-empty string / a non-zero number all mean "set"; +/// `false` / `null` / an empty string / `0` / an absent key all mean +/// "unset". Anything structurally unexpected (an array, an object) is a +/// genuine contract violation and still errors. +pub fn de_truthy_flag<'de, D>(deserializer: D) -> Result +where + D: Deserializer<'de>, +{ + match Option::::deserialize(deserializer)? { + None | Some(serde_json::Value::Null) => Ok(false), + Some(serde_json::Value::Bool(b)) => Ok(b), + Some(serde_json::Value::String(s)) => Ok(!s.is_empty()), + Some(serde_json::Value::Number(n)) => Ok(n.as_f64().map(|f| f != 0.0).unwrap_or(true)), + Some(other) => Err(de::Error::invalid_type( + match &other { + serde_json::Value::Array(_) => Unexpected::Seq, + _ => Unexpected::Map, + }, + &"a boolean, string, number, or null", + )), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[derive(Debug, Deserialize)] + struct Holder { + #[serde(default, deserialize_with = "de_truthy_flag")] + merged: bool, + } + + fn parse(json: &str) -> bool { + serde_json::from_str::(json) + .expect("deserialize") + .merged + } + + #[test] + fn absent_key_is_false() { + // The state of the world today: no server emits this field yet. + assert!(!parse("{}")); + } + + #[test] + fn booleans_pass_through() { + assert!(parse(r#"{"merged":true}"#)); + assert!(!parse(r#"{"merged":false}"#)); + } + + #[test] + fn null_is_false() { + assert!(!parse(r#"{"merged":null}"#)); + } + + #[test] + fn timestamp_string_is_truthy() { + // The `mergedAt`-shaped payload: a string means "merged at that + // time", an empty string means nothing. + assert!(parse(r#"{"merged":"Fri, 27 Mar 2026 19:12:42 GMT"}"#)); + assert!(parse(r#"{"merged":"2026-03-27T19:12:42Z"}"#)); + assert!(!parse(r#"{"merged":""}"#)); + } + + #[test] + fn numbers_follow_zero_is_false() { + assert!(parse(r#"{"merged":1}"#)); + assert!(parse(r#"{"merged":1743102762}"#)); + assert!(!parse(r#"{"merged":0}"#)); + } + + #[test] + fn structural_mismatches_still_error() { + // A tolerant type coercion must not become "accept anything" — + // an array or object here means the contract genuinely drifted. + assert!(serde_json::from_str::(r#"{"merged":[]}"#).is_err()); + assert!(serde_json::from_str::(r#"{"merged":{}}"#).is_err()); + } + + #[test] + fn is_false_gates_serialization() { + assert!(is_false(&false)); + assert!(!is_false(&true)); + } +} From 77d05f95b512a471f162bcf758b77fd40c0f5aff Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Wed, 5 Aug 2026 13:03:59 -0400 Subject: [PATCH 2/2] fix(ranking): infer merge state from advisory coverage; severity outranks it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the speculative `merged` API field with an inference, and reorders the ranking so a merged patch can never shadow a higher-severity fix. ## Merge state is inferable today The previous commit added a `merged: bool` deserialized from a guessed key (`merged`/`isMerged`/`mergedAt`/`upstreamMerged`) behind a type-tolerant deserializer, because no endpoint emits one. That guesswork is unnecessary: a merged patch is by definition one that folds several fixes into a single blob, so it NAMES several advisories — which every endpoint already returns. Merge state is now the count of distinct advisories a patch remediates: `vulnerabilities` map keys on by-package/view, `ghsaIds` on batch (falling back to `cveIds` only when no GHSA is named). Advisories are counted, not CVE ids: one advisory routinely carries several CVE aliases, and counting those would inflate a single-fix patch into a phantom merged one. Removes `merged`, `de_truthy_flag` and `is_false` — no speculative API surface, nothing to confirm before merge, and no server change needed. Surveyed production 2026-08-05: all 28 patches sampled across npm/PyPI/gem/cargo cover exactly one advisory, so the rung is inert today and ranking falls through to recency. Also confirmed the patches for a package are built against a shared pristine baseline (identical `beforeHash`) and conflict rather than chain — there is no merged patch to find yet, not a detection failure. ## Severity now outranks merge state Order was merged -> severity -> recency; it is now: 1. severity 2. merge state 3. patch recency 4. tier, uuid The merged patch stays the general preference — breadth is what an operator wants when only one patch per PURL can be applied — but it must not shadow a worse vulnerability. Because a patch's severity is the worst advisory it fixes, putting severity on top expresses that exactly: | merged | rival | winner | why | | high | critical | rival | higher severity available | | critical | high | merged | merged covers the worst | | high | high | merged | tie -> breadth decides | ## Testing Both rungs are mutation-checked: * coverage rung stubbed to a constant -> 9 tests fail across core, cli and the wiremock e2e; * coverage promoted above severity (losing the "unless higher severity" rule) -> 3 tests fail, exactly the ones expressing the exception. An earlier mutation run also exposed two recency tests passing vacuously (their UUIDs agreed with the correct answer); they now use adversarial UUIDs where the tiebreak points the wrong way. Adds a live production canary, `canary_patches_name_advisories_so_merge_state_is_inferable`, guarding the inference signal itself: if production ever stopped populating `vulnerabilities`, coverage would collapse to 0, the merge rung would go permanently inert, and selection would fall through to recency with no error anywhere. It asserts only that the signal exists (>= 1 advisory per patch), never a count, so publishing a genuinely merged patch does not fail it — that case is reported informationally instead. Co-Authored-By: Claude Opus 5 (1M context) --- crates/socket-patch-cli/CLI_CONTRACT.md | 55 +- crates/socket-patch-cli/src/commands/get.rs | 79 ++- .../src/commands/scan/discovery.rs | 5 - .../tests/e2e_hosted_production.rs | 92 ++++ .../tests/in_process_get_update_count.rs | 1 - crates/socket-patch-core/src/api/client.rs | 4 - crates/socket-patch-core/src/api/ranking.rs | 481 +++++++++++++----- crates/socket-patch-core/src/api/types.rs | 48 +- crates/socket-patch-core/src/utils/serde.rs | 107 +--- 9 files changed, 560 insertions(+), 312 deletions(-) diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index e048db77..41277b52 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -981,9 +981,10 @@ record per PURL, so exactly one is chosen. Both `get` and every `scan` mode rank candidates identically (`socket_patch_core::api::ranking`), best first: -1. **Merged** patches — the fix has landed upstream. -2. **Severity** — `critical > high > medium = moderate > low > (unknown)`, +1. **Severity** — `critical > high > medium = moderate > low > (unknown)`, taken as the worst severity across everything the patch fixes. +2. **Merge state** — a patch that remediates *more* advisories in one blob + leads. Inferred, not flagged: see below. 3. **Patch publish date**, most recent first — when the *patch* was published, never the upstream package's release date. Unparseable or absent dates sort last. @@ -994,6 +995,42 @@ best first: patch outranks a paid `low` one. Paid patches are excluded outright for callers whose `canAccessPaidPatches` is false. +#### Merge state is inferred, not reported + +There is no `merged` field on the wire and none is required. A merged +patch is by definition one that folds several fixes into a single blob, +so it **names several advisories** — which every endpoint already tells +us. Merge state is therefore the count of distinct advisories a patch +remediates: `vulnerabilities` map keys on `by-package` / `view`, +`ghsaIds` on `batch` (falling back to `cveIds` only when no GHSA is +named). `1` is an ordinary patch, `>= 2` is merged. + +Advisories are counted, **not** CVE ids: one advisory routinely carries +several CVE aliases, and counting those would inflate a single-fix patch +into a phantom merged one. + +As of 2026-08-05 production publishes no merged patches — all 28 patches +sampled across npm/PyPI/gem/cargo covered exactly one advisory each — so +this rung is currently inert and ranking falls through to recency. The +moment a consolidated patch is published it is preferred automatically, +with no client *or* server change. + +#### Why severity sits above merge state + +The merged patch is the general preference: it fixes the most in one +shot, and only one patch per PURL can be applied, so breadth is what an +operator wants. But it must never shadow a *worse* vulnerability. If a +patch addresses a higher-severity advisory than anything the merged patch +covers, that one wins — you do not leave a critical unfixed to pick up +two extra mediums. Severity on the top rung expresses exactly that, +because a patch's severity is the worst advisory it fixes: + +| merged patch | rival patch | winner | why | +|---|---|---|---| +| high | critical | rival | higher severity available | +| critical | high | merged | merged already covers the worst | +| high | high | merged | severities tie → breadth decides | + This ordering is also the presentation order everywhere patches are listed — `scan --json`'s `packages[].patches[]`, `get`'s "Found patches:" listing, and the `selection_required` `options[]` array — so @@ -1005,18 +1042,18 @@ get the interactive picker (or `selection_required` in `--json`); the ranking decides the presented order and hence the highlighted default, not the outcome. -Two additive keys may appear on `scan --json`'s `packages[].patches[]` -entries, both omitted when absent: `publishedAt` (present whenever the -server supplies it; the public-proxy fallback path fills it in from the -per-package results) and `merged` (only ever present as `true`). +One additive key may appear on `scan --json`'s `packages[].patches[]` +entries, omitted when absent: `publishedAt`, present whenever the server +supplies it (the public-proxy fallback path fills it in from the +per-package results). > **Known gap — batch responses without `publishedAt`.** `scan`'s > discovery (`packages[]`, the table, `updates[]`) is built from the > **batch** endpoint, whose response shape currently omits `publishedAt`; > the selection that `--apply` performs is built from the **by-package** -> endpoint, which carries it. Ranks 1, 2, 4 and 5 agree across both, so -> the two only diverge for a package whose top candidates tie on merge -> status *and* severity — there the batch side falls through to the UUID +> endpoint, which carries it. Ranks 1, 2 and 4 agree across both, so the +> two only diverge for a package whose top candidates tie on severity +> *and* merge state — there the batch side falls through to the UUID > tiebreak while apply correctly uses the date. > > Live example: `pkg:npm/axios@1.6.0` has two free `HIGH` patches; diff --git a/crates/socket-patch-cli/src/commands/get.rs b/crates/socket-patch-cli/src/commands/get.rs index c15f8ce8..51fb82a1 100644 --- a/crates/socket-patch-cli/src/commands/get.rs +++ b/crates/socket-patch-cli/src/commands/get.rs @@ -2089,7 +2089,6 @@ mod tests { license: "MIT".into(), tier: tier.into(), vulnerabilities: HashMap::::new(), - merged: false, } } @@ -2164,19 +2163,77 @@ mod tests { assert_eq!(out[0].tier, "free"); } + /// `mk_patch_sev` with one advisory per severity — two or more makes it + /// a *merged* patch (see `api::ranking::merged_coverage`), which is + /// inferred from the advisory count, not from any API flag. + fn mk_patch_multi( + uuid: &str, + purl: &str, + tier: &str, + published_at: &str, + severities: &[&str], + ) -> PatchSearchResult { + let mut p = mk_patch(uuid, purl, tier, published_at); + for (i, sev) in severities.iter().enumerate() { + p.vulnerabilities.insert( + format!("GHSA-{uuid}-{i}"), + VulnerabilityResponse { + cves: vec![], + summary: String::new(), + severity: (*sev).into(), + description: String::new(), + }, + ); + } + p + } + #[test] - fn select_prefers_merged_patch_over_higher_severity() { - // Rule 1 beats rule 2: a merged patch is the fix the ecosystem has - // converged on, so it leads even a critical non-merged patch. - let mut merged = mk_patch_sev("merged", "pkg:npm/foo@1.0", "free", "2020-01-01", "low"); - merged.merged = true; + fn select_prefers_merged_patch_when_severities_tie() { + // The general preference: `z_merged` remediates two HIGH advisories + // in one blob, `a_single` only one. Severities tie, so breadth + // decides. `a_single` is both newer AND earlier by uuid, so only + // the coverage rung can produce this result. let patches = vec![ - mk_patch_sev("crit", "pkg:npm/foo@1.0", "paid", "2026-06-01", "critical"), - merged, + mk_patch_sev("a_single", "pkg:npm/foo@1.0", "paid", "2026-06-01", "high"), + mk_patch_multi( + "z_merged", + "pkg:npm/foo@1.0", + "free", + "2020-01-01", + &["high", "high"], + ), + ]; + let out = select_patches(&patches, true, false).expect("ok"); + assert_eq!(out.len(), 1); + assert_eq!(out[0].uuid, "z_merged"); + } + + #[test] + fn select_prefers_a_higher_severity_patch_over_the_merged_one() { + // The exception. A merged patch must not shadow a worse + // vulnerability: `z_critical` addresses a CRITICAL the merged patch + // does not cover, so it wins despite being older, single-advisory, + // and last by uuid. + let patches = vec![ + mk_patch_multi( + "a_merged", + "pkg:npm/foo@1.0", + "free", + "2026-06-01", + &["high", "high"], + ), + mk_patch_sev( + "z_critical", + "pkg:npm/foo@1.0", + "free", + "2020-01-01", + "critical", + ), ]; let out = select_patches(&patches, true, false).expect("ok"); assert_eq!(out.len(), 1); - assert_eq!(out[0].uuid, "merged"); + assert_eq!(out[0].uuid, "z_critical"); } #[test] @@ -2521,7 +2578,6 @@ mod tests { description: "desc".into(), license: "MIT".into(), tier: "free".into(), - merged: false, }; let meta = patch_event_metadata(&patch); assert!(meta.as_object().unwrap().get("severity").is_none()); @@ -2552,7 +2608,6 @@ mod tests { description: "Fixes prototype pollution in minimist".into(), license: "MIT".into(), tier: "free".into(), - merged: false, }; let meta = patch_event_metadata(&patch); assert_eq!(meta["description"], "Fixes prototype pollution in minimist"); @@ -2594,7 +2649,6 @@ mod tests { description: String::new(), license: String::new(), tier: String::new(), - merged: false, }; let meta = patch_event_metadata(&patch); let ids: Vec<&str> = meta["vulnerabilities"] @@ -2617,7 +2671,6 @@ mod tests { description: "desc".into(), license: "MIT".into(), tier: "free".into(), - merged: false, }; let meta = patch_event_metadata(&patch); // `severity` is intentionally omitted (not null) when there diff --git a/crates/socket-patch-cli/src/commands/scan/discovery.rs b/crates/socket-patch-cli/src/commands/scan/discovery.rs index 432a8690..5a47bfdc 100644 --- a/crates/socket-patch-cli/src/commands/scan/discovery.rs +++ b/crates/socket-patch-cli/src/commands/scan/discovery.rs @@ -320,7 +320,6 @@ mod tests { severity: None, title: String::new(), published_at: None, - merged: false, }) .collect(), } @@ -343,7 +342,6 @@ mod tests { severity: Some((*severity).to_string()), title: String::new(), published_at: Some((*published).to_string()), - merged: false, }) .collect(), } @@ -486,7 +484,6 @@ mod tests { severity: None, title: String::new(), published_at: None, - merged: false, }], } } @@ -533,7 +530,6 @@ mod tests { severity: None, title: String::new(), published_at: None, - merged: false, }, BatchPatchInfo { uuid: "u2".to_string(), @@ -544,7 +540,6 @@ mod tests { severity: None, title: String::new(), published_at: None, - merged: false, }, ], }; diff --git a/crates/socket-patch-cli/tests/e2e_hosted_production.rs b/crates/socket-patch-cli/tests/e2e_hosted_production.rs index 8cb1753c..3ca5edb3 100644 --- a/crates/socket-patch-cli/tests/e2e_hosted_production.rs +++ b/crates/socket-patch-cli/tests/e2e_hosted_production.rs @@ -480,6 +480,37 @@ async fn published_uuids(purl: &str) -> Result, String> { .unwrap_or_default()) } +/// `GET /patch/by-package/` returning, per patch, the `(uuid, +/// advisory_count)` pair that drives merge-state inference. +async fn published_patch_advisory_counts(purl: &str) -> Result, String> { + let url = format!("{PROXY}/patch/by-package/{}", urlencode(purl)); + let resp = reqwest::Client::new() + .get(&url) + .header("Accept", "application/json") + .send() + .await + .map_err(|e| format!("GET {url}: {e}"))?; + let body = resp + .text() + .await + .map_err(|e| format!("GET {url}: reading body: {e}"))?; + let v: serde_json::Value = + serde_json::from_str(&body).map_err(|e| format!("GET {url}: bad JSON ({e}):\n{body}"))?; + Ok(v["patches"] + .as_array() + .map(|a| { + a.iter() + .filter_map(|p| { + Some(( + p["uuid"].as_str()?.to_string(), + p["vulnerabilities"].as_object()?.len(), + )) + }) + .collect() + }) + .unwrap_or_default()) +} + /// `GET /patch/by-package/` returning `(uuid, publishedAt)` pairs. /// Sibling of [`published_uuids`] for tests that care about patch metadata /// rather than just which UUIDs exist. @@ -581,6 +612,67 @@ async fn preflight_required_patches_are_published() { ); } +/// Canary: production must keep naming advisories, because merge state is +/// **inferred** from the advisory count rather than read off a flag. +/// +/// `api::ranking` ranks a patch that remediates several advisories above one +/// that remediates a single advisory. The whole signal is the size of the +/// `vulnerabilities` map. If production ever stopped populating it — shipping +/// patches with an empty map, or moving advisory ids somewhere else — every +/// patch would collapse to coverage 0, the merge rung would go permanently +/// inert, and selection would silently fall through to recency with no error +/// anywhere. +/// +/// This asserts only that the signal EXISTS (every patch names >= 1 +/// advisory), never how many. Production publishes no merged patches today — +/// all patches sampled cover exactly one advisory — and the day that changes +/// is not a regression, so a count of >= 2 must not fail this test. +#[tokio::test(flavor = "multi_thread")] +#[ignore = "live production API: contacts patches-api.socket.dev. Run with --ignored."] +async fn canary_patches_name_advisories_so_merge_state_is_inferable() { + let mut failures: Vec = Vec::new(); + let mut coverage_seen: Vec<(String, String, usize)> = Vec::new(); + + for purl in [NPM_PURL, PYPI_PURL, CARGO_PURL, GEM_PURL] { + match published_patch_advisory_counts(purl).await { + Err(e) => failures.push(format!("{purl}: production probe failed: {e}")), + Ok(patches) if patches.is_empty() => { + failures.push(format!("{purl}: production publishes no patches")) + } + Ok(patches) => { + for (uuid, count) in patches { + if count == 0 { + failures.push(format!( + "{purl}: patch {uuid} names ZERO advisories — merge-state \ + inference has no signal to work with, so the merge rung in \ + api::ranking is dead for this patch" + )); + } + coverage_seen.push((purl.to_string(), uuid, count)); + } + } + } + } + + assert!( + failures.is_empty(), + "merge-state inference signal is missing from production:\n - {}", + failures.join("\n - ") + ); + + // Informational: surfaces the day production starts publishing merged + // patches, without failing when it does. + let merged: Vec<_> = coverage_seen.iter().filter(|(_, _, c)| *c >= 2).collect(); + if merged.is_empty() { + eprintln!( + "[info] production publishes no merged patches yet ({} patches, all single-advisory)", + coverage_seen.len() + ); + } else { + eprintln!("[info] production now publishes merged patches: {merged:?}"); + } +} + /// Canary: production's `publishedAt` must stay a **per-patch** date. /// /// Patch selection ranks by recency (`socket_patch_core::api::ranking`), and diff --git a/crates/socket-patch-cli/tests/in_process_get_update_count.rs b/crates/socket-patch-cli/tests/in_process_get_update_count.rs index c2798e20..04bb8ccf 100644 --- a/crates/socket-patch-cli/tests/in_process_get_update_count.rs +++ b/crates/socket-patch-cli/tests/in_process_get_update_count.rs @@ -51,7 +51,6 @@ fn search_result(uuid: &str, purl: &str) -> PatchSearchResult { license: "MIT".into(), tier: "free".into(), vulnerabilities: HashMap::new(), - merged: false, } } diff --git a/crates/socket-patch-core/src/api/client.rs b/crates/socket-patch-core/src/api/client.rs index 5deef385..cef4c8cd 100644 --- a/crates/socket-patch-core/src/api/client.rs +++ b/crates/socket-patch-core/src/api/client.rs @@ -1480,7 +1480,6 @@ fn convert_search_result_to_batch_info(patch: PatchSearchResult) -> BatchPatchIn // but dropping it here would cost this path the recency tiebreak in // `ranking` — and it is the one path where we definitely have it. published_at: Some(patch.published_at), - merged: patch.merged, } } @@ -1691,7 +1690,6 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: vulns, - merged: false, }; let info = convert_search_result_to_batch_info(patch); @@ -1764,7 +1762,6 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: vulns, - merged: false, } } @@ -2466,7 +2463,6 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: HashMap::new(), - merged: false, }) .collect(), can_access_paid_patches, diff --git a/crates/socket-patch-core/src/api/ranking.rs b/crates/socket-patch-core/src/api/ranking.rs index f7734a34..cba276d9 100644 --- a/crates/socket-patch-core/src/api/ranking.rs +++ b/crates/socket-patch-core/src/api/ranking.rs @@ -10,10 +10,10 @@ //! //! **The order, best first:** //! -//! 1. **Merged patches** — the fix has landed upstream, so it is the one -//! the ecosystem is converging on. -//! 2. **Severity** — critical > high > medium/moderate > low > unknown, +//! 1. **Severity** — critical > high > medium/moderate > low > unknown, //! taken as the worst severity across everything the patch fixes. +//! 2. **Merge state** — a patch that remediates *more* advisories in one +//! blob leads. See [`merged_coverage`] for how this is inferred. //! 3. **Patch publish date**, most recent first. This is the date *the //! patch* was published, never the date the upstream package version //! was released — a 2020 package routinely carries a patch published @@ -22,8 +22,26 @@ //! 4. Paid tier, then UUID — pure tiebreaks, present only so the order is //! total and therefore reproducible run to run. //! -//! Note what is *not* in the list: `tier` is an access filter, not a -//! ranking signal. A free critical patch outranks a paid low one. +//! # Why severity sits above merge state +//! +//! The merged patch is the general preference: it fixes the most in one +//! shot, and the manifest only holds one patch per PURL, so breadth is +//! what an operator actually wants. But it must not shadow a *worse* +//! vulnerability. If a newly published patch addresses a higher-severity +//! advisory than anything the merged patch covers, that one wins — you do +//! not leave a critical unfixed to pick up two extra mediums. +//! +//! Putting severity on the top rung expresses exactly that, because the +//! severity of a patch is the *worst* advisory it fixes: +//! +//! | merged patch | rival patch | winner | why | +//! |---|---|---|---| +//! | high | critical | rival | higher severity available | +//! | critical | high | merged | merged already covers the worst | +//! | high | high | merged | severities tie → breadth decides | +//! +//! Note what is *not* a ranking signal: `tier` is an access filter. A free +//! critical patch outranks a paid low one. use std::cmp::{Ordering, Reverse}; @@ -57,6 +75,29 @@ pub fn max_severity_order<'a>(severities: impl Iterator) -> u8 { .unwrap_or_else(|| severity_order(None)) } +/// How many distinct advisories a patch remediates — the **inferred merge +/// state**, derived entirely from data the API already returns. +/// +/// There is no `merged` flag on the wire, and none is needed: a merged +/// patch is by definition one that folds several fixes into a single blob, +/// so it names several advisories. `1` is an ordinary single-advisory +/// patch; `>= 2` is a merged one; `0` means the patch names no advisory at +/// all and cannot be preferred on this axis. +/// +/// Counting **advisories** (GHSA ids) rather than CVE ids is deliberate: +/// one advisory routinely carries several CVE aliases, and counting those +/// would inflate a single-fix patch into a phantom merged one. +/// +/// Empirically, production publishes no merged patches yet — all 28 +/// patches sampled across npm/PyPI/gem/cargo on 2026-08-05 covered exactly +/// one advisory each, so this returns `1` for every patch live today. That +/// is the correct answer, not a degenerate one: the ranking simply falls +/// through to recency, and the moment Socket publishes a consolidated +/// patch it is preferred automatically, with no client or server change. +pub fn merged_coverage(advisory_count: usize) -> usize { + advisory_count +} + /// The comparable ranking key. Sorting ascending puts the best patch first. /// /// Kept as an explicit tuple-shaped struct rather than an ad-hoc tuple so @@ -64,10 +105,14 @@ pub fn max_severity_order<'a>(severities: impl Iterator) -> u8 { /// meaning of each position is documented in one place. #[derive(Debug, PartialEq, Eq, PartialOrd, Ord)] struct RankKey<'a> { - /// `false` sorts first, so this is negated: merged patches lead. - not_merged: bool, - /// 0 = critical … 4 = unknown. + /// 0 = critical … 4 = unknown. Top rung — see the module docs for why + /// this outranks merge state. severity: u8, + /// Advisory count, most first (hence `Reverse`): the inferred merge + /// state from [`merged_coverage`]. Below severity so a merged patch + /// can never shadow a higher-severity fix; above recency so breadth + /// beats freshness when the severities tie. + coverage: Reverse, /// Newest **patch** first — the patch's own publication date, not the /// package's release date. Unparseable or absent timestamps collapse /// to 0 and therefore sort last: the right treatment for a date we @@ -85,8 +130,10 @@ struct RankKey<'a> { fn rank_search_result(p: &PatchSearchResult) -> RankKey<'_> { RankKey { - not_merged: !p.merged, severity: max_severity_order(p.vulnerabilities.values().map(|v| v.severity.as_str())), + // The map is keyed by advisory id, so its length IS the advisory + // count — no CVE-alias inflation. + coverage: Reverse(merged_coverage(p.vulnerabilities.len())), patch_published: Reverse(parse_timestamp_secs(&p.published_at).unwrap_or(0)), not_paid: p.tier != "paid", uuid: &p.uuid, @@ -94,9 +141,18 @@ fn rank_search_result(p: &PatchSearchResult) -> RankKey<'_> { } fn rank_batch_info(p: &BatchPatchInfo) -> RankKey<'_> { + // `ghsa_ids` is the batch shape's mirror of the `vulnerabilities` map + // keys, so it is the advisory count. Fall back to `cve_ids` only when + // the server named no GHSA at all — otherwise a single advisory with + // two CVE aliases would read as a merged patch. + let advisories = if p.ghsa_ids.is_empty() { + p.cve_ids.len() + } else { + p.ghsa_ids.len() + }; RankKey { - not_merged: !p.merged, severity: severity_order(p.severity.as_deref()), + coverage: Reverse(merged_coverage(advisories)), patch_published: Reverse( p.published_at .as_deref() @@ -145,13 +201,25 @@ mod tests { .collect() } - fn search( + /// A single-advisory patch — the only shape production publishes today. + fn search(uuid: &str, tier: &str, published: &str, severity: &str) -> PatchSearchResult { + search_multi(uuid, tier, published, &[severity]) + } + + /// A patch fixing one advisory per entry in `severities`. Two or more + /// makes it a *merged* patch under [`merged_coverage`]. + fn search_multi( uuid: &str, tier: &str, published: &str, - severity: &str, - merged: bool, + severities: &[&str], ) -> PatchSearchResult { + let entries: Vec<(String, &str)> = severities + .iter() + .enumerate() + .map(|(i, s)| (format!("GHSA-{uuid}-{i}"), *s)) + .collect(); + let refs: Vec<(&str, &str)> = entries.iter().map(|(k, v)| (k.as_str(), *v)).collect(); PatchSearchResult { uuid: uuid.to_string(), purl: "pkg:npm/foo@1.0.0".to_string(), @@ -159,8 +227,7 @@ mod tests { description: String::new(), license: "MIT".to_string(), tier: tier.to_string(), - vulnerabilities: vulns(&[("GHSA-aaaa-aaaa-aaaa", severity)]), - merged, + vulnerabilities: vulns(&refs), } } @@ -169,18 +236,30 @@ mod tests { tier: &str, published: Option<&str>, severity: Option<&str>, - merged: bool, + ) -> BatchPatchInfo { + batch_multi(uuid, tier, published, severity, 1) + } + + /// Batch-shaped patch naming `advisories` GHSA ids — the batch mirror + /// of `search_multi`. + fn batch_multi( + uuid: &str, + tier: &str, + published: Option<&str>, + severity: Option<&str>, + advisories: usize, ) -> BatchPatchInfo { BatchPatchInfo { uuid: uuid.to_string(), purl: "pkg:npm/foo@1.0.0".to_string(), tier: tier.to_string(), cve_ids: Vec::new(), - ghsa_ids: Vec::new(), + ghsa_ids: (0..advisories) + .map(|i| format!("GHSA-{uuid}-{i}")) + .collect(), severity: severity.map(str::to_string), title: String::new(), published_at: published.map(str::to_string), - merged, } } @@ -237,32 +316,135 @@ mod tests { // ── Rank key precedence ─────────────────────────────────────────── #[test] - fn merged_outranks_a_more_severe_unmerged_patch() { - // Rule 1 beats rule 2: a merged low patch leads a critical one. + fn merged_patch_wins_when_severities_tie() { + // The general preference. `z_merged` fixes two HIGH advisories, + // `a_single` fixes one; severities tie, so breadth decides. The + // uuid tiebreak points at `a_single`, and `a_single` is also the + // more recent patch — so only the coverage rung can produce this. assert_eq!( best_search(vec![ - search("crit", "free", "2026-01-01T00:00:00Z", "critical", false), - search("merged", "free", "2020-01-01T00:00:00Z", "low", true), + search("a_single", "free", "2026-08-01T00:00:00Z", "high"), + search_multi( + "z_merged", + "free", + "2020-01-01T00:00:00Z", + &["high", "high"] + ), ]), - "merged" + "z_merged" ); } #[test] - fn severity_outranks_recency() { - // The reported bug: the newest patch fixes a `low`, an older one - // fixes a `critical`. Critical must win. + fn a_higher_severity_patch_beats_the_merged_one() { + // The exception. The merged patch consolidates two HIGHs, but a + // rival addresses a CRITICAL it does not cover. Taking breadth here + // would leave the worst vulnerability unfixed, so the CRITICAL + // wins — even though it is older, single-advisory, and its uuid + // sorts last. + assert_eq!( + best_search(vec![ + search_multi( + "a_merged", + "free", + "2026-08-01T00:00:00Z", + &["high", "high"] + ), + search("z_critical", "free", "2020-01-01T00:00:00Z", "critical"), + ]), + "z_critical" + ); + } + + #[test] + fn merged_patch_wins_when_it_already_covers_the_worst_advisory() { + // Third row of the table in the module docs: the merged patch's max + // severity already matches the rival's, so there is no + // higher-severity fix being shadowed and breadth decides again. assert_eq!( best_search(vec![ - search("newest_low", "free", "2026-08-01T00:00:00Z", "low", false), search( - "older_crit", + "a_critical_only", + "free", + "2026-08-01T00:00:00Z", + "critical" + ), + search_multi( + "z_merged_crit", + "free", + "2020-01-01T00:00:00Z", + &["critical", "low"], + ), + ]), + "z_merged_crit" + ); + } + + #[test] + fn coverage_counts_advisories_not_cve_aliases() { + // One advisory carrying several CVE aliases is NOT a merged patch. + // The search shape counts `vulnerabilities` map keys, so aliases in + // `cves` cannot inflate it; pin that a single-advisory patch stays + // at coverage 1 no matter how many CVEs hang off it. + let mut aliased = search("a_aliased", "free", "2026-08-01T00:00:00Z", "high"); + aliased + .vulnerabilities + .values_mut() + .next() + .unwrap() + .cves + .extend(["CVE-1".into(), "CVE-2".into(), "CVE-3".into()]); + assert_eq!(aliased.vulnerabilities.len(), 1, "still one advisory"); + // A genuine 2-advisory patch must still outrank it despite being + // older and later-sorting by uuid. + assert_eq!( + best_search(vec![ + aliased, + search_multi( + "z_merged", "free", "2020-01-01T00:00:00Z", - "critical", - false + &["high", "high"] ), ]), + "z_merged" + ); + } + + #[test] + fn merged_coverage_is_the_advisory_count() { + assert_eq!(merged_coverage(0), 0); + assert_eq!(merged_coverage(1), 1, "ordinary single-advisory patch"); + assert!(merged_coverage(2) > merged_coverage(1), "merged leads"); + assert!(merged_coverage(5) > merged_coverage(2)); + } + + #[test] + fn patch_naming_no_advisory_ranks_below_a_single_advisory_patch() { + // Coverage 0: nothing to prefer it for. It is also newer and + // earlier by uuid, so only the coverage rung demotes it. + let none = PatchSearchResult { + vulnerabilities: HashMap::new(), + ..search("a_none", "free", "2026-08-01T00:00:00Z", "high") + }; + // Give both the same (unknown) severity so coverage is the decider: + // an empty vulnerabilities map ranks `severity_order(None)`. + let one = PatchSearchResult { + vulnerabilities: vulns(&[("GHSA-x", "not-a-severity")]), + ..search("z_one", "free", "2020-01-01T00:00:00Z", "high") + }; + assert_eq!(best_search(vec![none, one]), "z_one"); + } + + #[test] + fn severity_outranks_recency() { + // The reported bug: the newest patch fixes a `low`, an older one + // fixes a `critical`. Critical must win. + assert_eq!( + best_search(vec![ + search("newest_low", "free", "2026-08-01T00:00:00Z", "low"), + search("older_crit", "free", "2020-01-01T00:00:00Z", "critical"), + ]), "older_crit" ); } @@ -273,14 +455,8 @@ mod tests { // does not rank. assert_eq!( best_search(vec![ - search("paid_low", "paid", "2026-08-01T00:00:00Z", "low", false), - search( - "free_crit", - "free", - "2020-01-01T00:00:00Z", - "critical", - false - ), + search("paid_low", "paid", "2026-08-01T00:00:00Z", "low"), + search("free_crit", "free", "2020-01-01T00:00:00Z", "critical"), ]), "free_crit" ); @@ -294,8 +470,8 @@ mod tests { // out fails this test.) assert_eq!( best_search(vec![ - search("a_old", "free", "2024-01-01T00:00:00Z", "high", false), - search("z_new", "free", "2026-01-01T00:00:00Z", "high", false), + search("a_old", "free", "2024-01-01T00:00:00Z", "high"), + search("z_new", "free", "2026-01-01T00:00:00Z", "high"), ]), "z_new" ); @@ -313,20 +489,8 @@ mod tests { // both keys would be equal here and the ordering would collapse to // the UUID tiebreak — which would pick `0bc312a6` (the OLDER // patch), not `83f5a654`. - let older = search( - "0bc312a6", - "free", - "Fri, 27 Mar 2026 19:12:42 GMT", - "high", - false, - ); - let newer = search( - "83f5a654", - "free", - "Mon, 03 Aug 2026 20:23:06 GMT", - "high", - false, - ); + let older = search("0bc312a6", "free", "Fri, 27 Mar 2026 19:12:42 GMT", "high"); + let newer = search("83f5a654", "free", "Mon, 03 Aug 2026 20:23:06 GMT", "high"); assert_eq!(older.purl, newer.purl, "same package version"); assert!( older.uuid < newer.uuid, @@ -348,8 +512,8 @@ mod tests { // answer by accident. assert_eq!( best_search(vec![ - search("a_older", "free", older, "high", false), - search("z_newer", "free", newer, "high", false), + search("a_older", "free", older, "high"), + search("z_newer", "free", newer, "high"), ]), "z_newer" ); @@ -361,15 +525,15 @@ mod tests { // not demote it below a less severe one. assert_eq!( best_search(vec![ - search("dated_high", "free", "2026-01-01T00:00:00Z", "high", false), - search("undated_crit", "free", "not a date", "critical", false), + search("dated_high", "free", "2026-01-01T00:00:00Z", "high"), + search("undated_crit", "free", "not a date", "critical"), ]), "undated_crit" ); assert_eq!( best_search(vec![ - search("undated", "free", "", "high", false), - search("dated", "free", "2020-01-01T00:00:00Z", "high", false), + search("undated", "free", "", "high"), + search("dated", "free", "2020-01-01T00:00:00Z", "high"), ]), "dated" ); @@ -383,8 +547,8 @@ mod tests { // this fails the moment the date rung stops working or is demoted. assert_eq!( best_search(vec![ - search("a_old_paid", "paid", "2024-01-01T00:00:00Z", "high", false), - search("z_new_free", "free", "2026-01-01T00:00:00Z", "high", false), + search("a_old_paid", "paid", "2024-01-01T00:00:00Z", "high"), + search("z_new_free", "free", "2026-01-01T00:00:00Z", "high"), ]), "z_new_free" ); @@ -394,8 +558,8 @@ mod tests { fn tier_breaks_ties_after_date() { assert_eq!( best_search(vec![ - search("free", "free", "2026-01-01T00:00:00Z", "high", false), - search("paid", "paid", "2026-01-01T00:00:00Z", "high", false), + search("free", "free", "2026-01-01T00:00:00Z", "high"), + search("paid", "paid", "2026-01-01T00:00:00Z", "high"), ]), "paid" ); @@ -405,38 +569,55 @@ mod tests { fn uuid_is_a_deterministic_final_tiebreak() { // Two patches identical in every ranked dimension must still land // in a fixed order — otherwise `scan --json` is not reproducible. - let a = search("aaaa", "free", "2026-01-01T00:00:00Z", "high", false); - let z = search("zzzz", "free", "2026-01-01T00:00:00Z", "high", false); + let a = search("aaaa", "free", "2026-01-01T00:00:00Z", "high"); + let z = search("zzzz", "free", "2026-01-01T00:00:00Z", "high"); assert_eq!(best_search(vec![z.clone(), a.clone()]), "aaaa"); assert_eq!(best_search(vec![a, z]), "aaaa"); } #[test] fn full_precedence_chain_in_one_sort() { + // Exercises all four rungs at once. UUIDs are lettered in reverse + // of the expected order so the uuid tiebreak cannot reproduce the + // answer on its own. let mut patches = [ - search("d_low_new", "paid", "2026-08-01T00:00:00Z", "low", false), - search( - "b_crit_old", + // rung 3: loses to `d` on recency (same severity, same coverage) + search("e_high_old", "free", "2019-01-01T00:00:00Z", "high"), + // rung 1: worst severity of the lot + search("d_high_new", "free", "2026-01-01T00:00:00Z", "high"), + // rung 1: critical, but single-advisory + search("c_crit_single", "paid", "2026-08-01T00:00:00Z", "critical"), + // rung 2: critical AND merged -> the winner + search_multi( + "b_crit_merged", "free", "2020-01-01T00:00:00Z", - "critical", - false, + &["critical", "low"], ), - search("a_merged", "free", "2019-01-01T00:00:00Z", "low", true), - search("c_high_new", "free", "2026-01-01T00:00:00Z", "high", false), + // rung 1: lowest severity, so last despite being newest + search("a_low_newest", "paid", "2026-12-01T00:00:00Z", "low"), ]; patches.sort_by(cmp_search_results); let order: Vec<&str> = patches.iter().map(|p| p.uuid.as_str()).collect(); - assert_eq!(order, ["a_merged", "b_crit_old", "c_high_new", "d_low_new"]); + assert_eq!( + order, + [ + "b_crit_merged", + "c_crit_single", + "d_high_new", + "e_high_old", + "a_low_newest" + ] + ); } #[test] fn worst_vulnerability_in_the_map_drives_severity() { let mixed = PatchSearchResult { vulnerabilities: vulns(&[("GHSA-a", "low"), ("GHSA-b", "critical")]), - ..search("mixed", "free", "2020-01-01T00:00:00Z", "low", false) + ..search("mixed", "free", "2020-01-01T00:00:00Z", "low") }; - let high = search("high_only", "free", "2026-01-01T00:00:00Z", "high", false); + let high = search("high_only", "free", "2026-01-01T00:00:00Z", "high"); // `mixed` is older but carries a critical — it must win. assert_eq!(best_search(vec![high, mixed]), "mixed"); } @@ -445,9 +626,9 @@ mod tests { fn patch_with_no_vulnerabilities_ranks_below_one_with_a_low() { let none = PatchSearchResult { vulnerabilities: HashMap::new(), - ..search("no_vulns", "free", "2026-08-01T00:00:00Z", "low", false) + ..search("no_vulns", "free", "2026-08-01T00:00:00Z", "low") }; - let low = search("has_low", "free", "2020-01-01T00:00:00Z", "low", false); + let low = search("has_low", "free", "2020-01-01T00:00:00Z", "low"); assert_eq!(best_search(vec![none, low]), "has_low"); } @@ -455,46 +636,116 @@ mod tests { #[test] fn batch_ranking_matches_search_ranking() { + // Severity outranks recency, same as the search shape. assert_eq!( best_batch(vec![ batch( "newest_low", "free", Some("2026-08-01T00:00:00Z"), - Some("low"), - false + Some("low") ), batch( "older_crit", "free", Some("2020-01-01T00:00:00Z"), - Some("critical"), - false + Some("critical") ), ]), "older_crit" ); + // Coverage decides once severities tie — the batch shape infers it + // from `ghsaIds` rather than a vulnerabilities map. assert_eq!( best_batch(vec![ batch( - "crit", + "a_single", + "free", + Some("2026-08-01T00:00:00Z"), + Some("high") + ), + batch_multi( + "z_merged", + "free", + Some("2020-01-01T00:00:00Z"), + Some("high"), + 2 + ), + ]), + "z_merged" + ); + // ...and a higher-severity rival still beats the merged patch. + assert_eq!( + best_batch(vec![ + batch_multi( + "a_merged", "free", - Some("2026-01-01T00:00:00Z"), - Some("critical"), - false + Some("2026-08-01T00:00:00Z"), + Some("high"), + 2 ), batch( - "merged", + "z_crit", "free", Some("2020-01-01T00:00:00Z"), - Some("low"), - true + Some("critical") ), ]), - "merged" + "z_crit" ); } + #[test] + fn batch_coverage_counts_ghsa_ids_not_cve_aliases() { + // A single advisory with three CVE aliases must stay coverage 1. + // `ghsa_ids` is non-empty, so `cve_ids` is ignored entirely. + let mut aliased = batch( + "a_aliased", + "free", + Some("2026-08-01T00:00:00Z"), + Some("high"), + ); + aliased.cve_ids = vec!["CVE-1".into(), "CVE-2".into(), "CVE-3".into()]; + assert_eq!(aliased.ghsa_ids.len(), 1, "still one advisory"); + assert_eq!( + best_batch(vec![ + aliased, + batch_multi( + "z_merged", + "free", + Some("2020-01-01T00:00:00Z"), + Some("high"), + 2 + ), + ]), + "z_merged" + ); + } + + #[test] + fn batch_falls_back_to_cve_ids_when_no_ghsa_is_named() { + // Some patches may name only CVEs. With `ghsa_ids` empty the + // advisory count comes from `cve_ids` instead, so a + // two-CVE-no-GHSA patch still reads as merged. + let mut single = batch( + "a_single", + "free", + Some("2026-08-01T00:00:00Z"), + Some("high"), + ); + single.ghsa_ids.clear(); + single.cve_ids = vec!["CVE-1".into()]; + let mut merged = batch( + "z_merged", + "free", + Some("2020-01-01T00:00:00Z"), + Some("high"), + ); + merged.ghsa_ids.clear(); + merged.cve_ids = vec!["CVE-2".into(), "CVE-3".into()]; + assert_eq!(best_batch(vec![single, merged]), "z_merged"); + } + #[test] fn batch_recency_uses_the_patch_date_not_the_package_release_date() { // Batch-shape twin of @@ -508,15 +759,13 @@ mod tests { "u_aaa", "free", Some("Fri, 27 Mar 2026 19:12:42 GMT"), - Some("HIGH"), - false + Some("HIGH") ), batch( "u_zzz", "free", Some("Mon, 03 Aug 2026 20:23:06 GMT"), - Some("HIGH"), - false + Some("HIGH") ), ]), "u_zzz" @@ -529,8 +778,8 @@ mod tests { // patches sharing a publish date rank identically regardless of // which purl they belong to. Guards against anyone "optimizing" // the key to be derived from package-level state. - let mut a = search("u1", "free", "2026-01-01T00:00:00Z", "high", false); - let mut b = search("u2", "free", "2026-01-01T00:00:00Z", "high", false); + let mut a = search("u1", "free", "2026-01-01T00:00:00Z", "high"); + let mut b = search("u2", "free", "2026-01-01T00:00:00Z", "high"); let same_purl = cmp_search_results(&a, &b); a.purl = "pkg:npm/alpha@1.0.0".to_string(); b.purl = "pkg:npm/omega@9.9.9".to_string(); @@ -547,8 +796,8 @@ mod tests { // recency tiebreak must not cost us the severity ordering. assert_eq!( best_batch(vec![ - batch("low", "free", None, Some("low"), false), - batch("crit", "free", None, Some("critical"), false), + batch("low", "free", None, Some("low")), + batch("crit", "free", None, Some("critical")), ]), "crit" ); @@ -558,14 +807,8 @@ mod tests { fn batch_missing_severity_ranks_last() { assert_eq!( best_batch(vec![ - batch("unknown", "free", Some("2026-08-01T00:00:00Z"), None, false), - batch( - "low", - "free", - Some("2020-01-01T00:00:00Z"), - Some("low"), - false - ), + batch("unknown", "free", Some("2026-08-01T00:00:00Z"), None), + batch("low", "free", Some("2020-01-01T00:00:00Z"), Some("low")), ]), "low" ); @@ -575,28 +818,10 @@ mod tests { fn batch_ordering_is_total_and_deterministic() { let all = || { vec![ - batch("u3", "free", None, None, false), - batch( - "u1", - "paid", - Some("2026-01-01T00:00:00Z"), - Some("high"), - false, - ), - batch( - "u2", - "free", - Some("2026-01-01T00:00:00Z"), - Some("high"), - false, - ), - batch( - "u0", - "free", - Some("2020-01-01T00:00:00Z"), - Some("critical"), - true, - ), + batch("u3", "free", None, None), + batch("u1", "paid", Some("2026-01-01T00:00:00Z"), Some("high")), + batch("u2", "free", Some("2026-01-01T00:00:00Z"), Some("high")), + batch("u0", "free", Some("2020-01-01T00:00:00Z"), Some("critical")), ] }; let mut first = all(); diff --git a/crates/socket-patch-core/src/api/types.rs b/crates/socket-patch-core/src/api/types.rs index 999e4f1c..2cff464e 100644 --- a/crates/socket-patch-core/src/api/types.rs +++ b/crates/socket-patch-core/src/api/types.rs @@ -46,22 +46,6 @@ pub struct PatchResponse { pub description: String, pub license: String, pub tier: String, - /// Upstream-merge marker: this patch's fix has landed upstream. - /// Merged patches outrank everything else in patch selection (see - /// [`crate::api::ranking`]). - /// - /// Not yet emitted by any endpoint, so it defaults to `false` rather - /// than being required, and the deserializer tolerates whichever - /// spelling and type the server settles on. - #[serde( - default, - alias = "isMerged", - alias = "mergedAt", - alias = "upstreamMerged", - deserialize_with = "crate::utils::serde::de_truthy_flag", - skip_serializing_if = "crate::utils::serde::is_false" - )] - pub merged: bool, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -96,16 +80,6 @@ pub struct PatchSearchResult { pub license: String, pub tier: String, pub vulnerabilities: HashMap, - /// Upstream-merge marker — see [`PatchResponse::merged`]. - #[serde( - default, - alias = "isMerged", - alias = "mergedAt", - alias = "upstreamMerged", - deserialize_with = "crate::utils::serde::de_truthy_flag", - skip_serializing_if = "crate::utils::serde::is_false" - )] - pub merged: bool, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -130,21 +104,11 @@ pub struct BatchPatchInfo { /// [`PatchResponse::published_at`]), if the server supplies it. The /// batch shape historically omits it, which is why it is optional — a /// `None` here only weakens the recency tiebreak in - /// [`crate::api::ranking`], it never changes the merged/severity - /// ordering. The public-proxy fallback path fills it in from the + /// [`crate::api::ranking`], it never changes the severity or + /// merge-state ordering. The public-proxy fallback path fills it in from the /// per-package search results. #[serde(default, skip_serializing_if = "Option::is_none")] pub published_at: Option, - /// Upstream-merge marker — see [`PatchResponse::merged`]. - #[serde( - default, - alias = "isMerged", - alias = "mergedAt", - alias = "upstreamMerged", - deserialize_with = "crate::utils::serde::de_truthy_flag", - skip_serializing_if = "crate::utils::serde::is_false" - )] - pub merged: bool, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -234,7 +198,6 @@ mod tests { description: "desc".into(), license: "MIT".into(), tier: "free".into(), - merged: false, }; let json = serde_json::to_string(&pr).unwrap(); assert!(json.contains("publishedAt")); @@ -307,7 +270,6 @@ mod tests { severity: Some("high".into()), title: "Test".into(), published_at: None, - merged: false, }], }], can_access_paid_patches: false, @@ -330,7 +292,6 @@ mod tests { severity: Some("high".into()), title: "Test".into(), published_at: None, - merged: false, }; let json = serde_json::to_string(&bpi).unwrap(); assert!(json.contains("cveIds")); @@ -366,7 +327,6 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: HashMap::new(), - merged: false, }; let json = serde_json::to_string(&psr).unwrap(); let back: PatchSearchResult = serde_json::from_str(&json).unwrap(); @@ -601,7 +561,6 @@ mod tests { license: "MIT".into(), tier: "free".into(), vulnerabilities: HashMap::new(), - merged: false, }], can_access_paid_patches: true, }; @@ -697,8 +656,5 @@ mod tests { sr.patches[0].vulnerabilities["GHSA-4hjh-wcwx-xvwj"].severity, "HIGH" ); - // No `merged` key on the wire today -> false, not a parse error. - assert!(!sr.patches[0].merged); - assert!(!sr.patches[1].merged); } } diff --git a/crates/socket-patch-core/src/utils/serde.rs b/crates/socket-patch-core/src/utils/serde.rs index ce28f934..7f5f288c 100644 --- a/crates/socket-patch-core/src/utils/serde.rs +++ b/crates/socket-patch-core/src/utils/serde.rs @@ -1,7 +1,6 @@ //! Shared serde helpers. -use serde::de::{self, Deserializer, Unexpected}; -use serde::{Deserialize, Serialize, Serializer}; +use serde::{Serialize, Serializer}; use std::collections::{BTreeMap, HashMap}; /// Serialize a `HashMap` with its keys in sorted order so the emitted JSON @@ -20,107 +19,3 @@ where { map.iter().collect::>().serialize(serializer) } - -/// `skip_serializing_if` companion for `bool` fields that default to false — -/// keeps them out of the emitted JSON entirely rather than writing -/// `"merged": false` on every record. -pub fn is_false(b: &bool) -> bool { - !*b -} - -/// Deserialize a marker flag whose on-the-wire *type* is not pinned. -/// -/// The patch API's upstream-merge marker is expected to arrive as a plain -/// `true`/`false`, but the same signal is equally likely to ship as a -/// nullable timestamp (`"mergedAt": "Fri, 27 Mar 2026 …"`). Typing the -/// field as `bool` alone would make a string payload a hard deserialize -/// error, which would take down the *entire* patch-list response — a -/// server-side field-type choice must never be able to break the client -/// that way. -/// -/// So: `true` / a non-empty string / a non-zero number all mean "set"; -/// `false` / `null` / an empty string / `0` / an absent key all mean -/// "unset". Anything structurally unexpected (an array, an object) is a -/// genuine contract violation and still errors. -pub fn de_truthy_flag<'de, D>(deserializer: D) -> Result -where - D: Deserializer<'de>, -{ - match Option::::deserialize(deserializer)? { - None | Some(serde_json::Value::Null) => Ok(false), - Some(serde_json::Value::Bool(b)) => Ok(b), - Some(serde_json::Value::String(s)) => Ok(!s.is_empty()), - Some(serde_json::Value::Number(n)) => Ok(n.as_f64().map(|f| f != 0.0).unwrap_or(true)), - Some(other) => Err(de::Error::invalid_type( - match &other { - serde_json::Value::Array(_) => Unexpected::Seq, - _ => Unexpected::Map, - }, - &"a boolean, string, number, or null", - )), - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[derive(Debug, Deserialize)] - struct Holder { - #[serde(default, deserialize_with = "de_truthy_flag")] - merged: bool, - } - - fn parse(json: &str) -> bool { - serde_json::from_str::(json) - .expect("deserialize") - .merged - } - - #[test] - fn absent_key_is_false() { - // The state of the world today: no server emits this field yet. - assert!(!parse("{}")); - } - - #[test] - fn booleans_pass_through() { - assert!(parse(r#"{"merged":true}"#)); - assert!(!parse(r#"{"merged":false}"#)); - } - - #[test] - fn null_is_false() { - assert!(!parse(r#"{"merged":null}"#)); - } - - #[test] - fn timestamp_string_is_truthy() { - // The `mergedAt`-shaped payload: a string means "merged at that - // time", an empty string means nothing. - assert!(parse(r#"{"merged":"Fri, 27 Mar 2026 19:12:42 GMT"}"#)); - assert!(parse(r#"{"merged":"2026-03-27T19:12:42Z"}"#)); - assert!(!parse(r#"{"merged":""}"#)); - } - - #[test] - fn numbers_follow_zero_is_false() { - assert!(parse(r#"{"merged":1}"#)); - assert!(parse(r#"{"merged":1743102762}"#)); - assert!(!parse(r#"{"merged":0}"#)); - } - - #[test] - fn structural_mismatches_still_error() { - // A tolerant type coercion must not become "accept anything" — - // an array or object here means the contract genuinely drifted. - assert!(serde_json::from_str::(r#"{"merged":[]}"#).is_err()); - assert!(serde_json::from_str::(r#"{"merged":{}}"#).is_err()); - } - - #[test] - fn is_false_gates_serialization() { - assert!(is_false(&false)); - assert!(!is_false(&true)); - } -}