From 16985d674a90df373a291a7366c4fbbd762d3b93 Mon Sep 17 00:00:00 2001 From: zuhairhalawa <71436209+zuhairhalawa@users.noreply.github.com> Date: Fri, 6 Nov 2026 00:28:14 +0400 Subject: [PATCH] feat: flag left-out packages, known-range conversions and version spelling Three shapes from real advisory edits were missed or described poorly. - sibling-package-unchanged: version ranges changed for one package but not for another in the same advisory that had the same version data. Needs review. - known-range-converted: last_known_affected_version_range replaced by a range's one new end. This was an added event plus a removed field that suggested data loss. The finding says whether the two name the same end, comparing strings only. - version-spelling-changed: a range value that changes only in leading zeros. validate also warns with version-leading-zero for npm packages and SEMVER ranges. Allow lists naming the old codes no longer cover these cases. The version is not bumped; the changelog entry is under Unreleased. --- CHANGELOG.md | 16 ++ README.md | 12 +- src/core/diff/affected.ts | 197 ++++++++++++++++-- src/core/validate.ts | 12 +- src/core/versionSpelling.ts | 29 +++ test/real-edits.test.ts | 402 ++++++++++++++++++++++++++++++++++++ test/real.test.ts | 8 +- 7 files changed, 658 insertions(+), 18 deletions(-) create mode 100644 src/core/versionSpelling.ts create mode 100644 test/real-edits.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index d9f49c0..ab2b0cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## Unreleased + +This changes finding codes and allow-list coverage. The cases come from running the tool on real advisory edits. + +- New `sibling-package-unchanged` finding: version ranges changed for one package but not for another in the same + advisory that had the same version data. It needs review, because packages often share a fix but can really + differ. A `--allow` list that used to pass such an edit now also has to name this code. +- New `known-range-converted` finding: `last_known_affected_version_range` replaced by a range's one new end. This + was reported as an added event and a removed field, and the second suggested the value may have been dropped by + accident. The finding says whether the two name the same end, comparing strings only. A `--allow` list naming + `range-event-added,field-removed` no longer covers this case. +- New `version-spelling-changed` finding: a range value that changes only in leading zeros, such as `2025.08.14` to + `2025.8.14`. `--allow range-event-changed` no longer covers this case. +- New `version-leading-zero` warning from `validate`, for range events in `npm` packages and `SEMVER` ranges. The + file is still valid OSV, so `validate` still exits 0. + ## 0.2.0 This release changes finding codes, allow-list coverage and some exit codes. See the diff --git a/README.md b/README.md index 7f1d4c7..88a68b4 100644 --- a/README.md +++ b/README.md @@ -197,7 +197,7 @@ On top of the schema it adds a few checks, each reported with its own code: - GitHub REST/GraphQL advisory objects (not OSV) are recognised and rejected with a pointer to the OSV format. Problems are `error` (invalid, exit 2) or `warning` (valid but unusual: missing severity, duplicate -references, scheme-less URLs, non-semver versions in a SEMVER range, ...). JSON Schema +references, scheme-less URLs, non-semver versions in a SEMVER range, leading zeros in an npm or SEMVER version, ...). JSON Schema `format` keywords (`uri`, `date-time`) are treated as annotations, as in the specification's default mode, so a genuine GitHub record with a scheme-less reference URL is valid with a warning. @@ -209,7 +209,7 @@ the indexes in paths: | --- | --- | | `severity[]` (also `affected[].severity`) | Matched by type, so CVSS v3 and v4 (and several entries) stay distinct. Removed = review, changed = review (with metric delta and v3 base score), added = info. | | `affected[]` | Matched by ecosystem+name; a lone unmatched package in the same ecosystem is treated as a **rename** so its ranges/severity are still compared. Removed/added packages = review. | -| ranges / events | `introduced`, `fixed`, `last_affected`, `limit` added/removed/changed = review, with an explanation (e.g. removing the only `fixed` leaves the range open-ended). Replacing `last_affected` with `fixed` (or the reverse) in a range, with the same number of values on both sides, = one review finding (`range-bound-converted`), because `last_affected` is an inclusive end and `fixed` an exclusive one, so affected status may change and needs checking against the package's release history (the repository's history for a GIT range). Several values of one kind in a range are listed without being paired. Explicit `versions[]` additions/removals = review. | +| ranges / events | `introduced`, `fixed`, `last_affected`, `limit` added/removed/changed = review, with an explanation (e.g. removing the only `fixed` leaves the range open-ended). Replacing `last_affected` with `fixed` (or the reverse) in a range, with the same number of values on both sides, = one review finding (`range-bound-converted`), because `last_affected` is an inclusive end and `fixed` an exclusive one, so affected status may change and needs checking against the package's release history (the repository's history for a GIT range). Several values of one kind in a range are listed without being paired. Replacing `database_specific.last_known_affected_version_range` with a range's one new end = one review finding (`known-range-converted`), which says whether the two name the same end. A value that changes only in leading zeros = review (`version-spelling-changed`). Version ranges changing for one package but not for another that had the same version data = review (`sibling-package-unchanged`). Explicit `versions[]` additions/removals = review. | | references, aliases, related, upstream | Removal = review (lost provenance / cross-references); addition = info. URLs compared ignoring trailing slash and host case. | | CWE (`database_specific.cwe_ids`), GitHub severity label | Removal/change = review; addition = info. | | `id`, `published`, `withdrawn` | Changes = review (withdrawing is flagged). Two exceptions = info: setting a missing `published`, and changing the date of an existing `withdrawn`. | @@ -262,6 +262,14 @@ and every such finding says why. of events, severity entries and references have only the per-list limit. None of this is a run-time guarantee: a crafted custom list of about 1.5 MB took over a minute in one local measurement, and a file near the 25 MB size limit can take far longer. +- `sibling-package-unchanged` compares each package's version data as written, so it can only ask whether a + package was left out. Packages can really differ, and it does not notice packages whose ranges already differed + before the edit. Allow the code when the difference is intended. +- `known-range-converted` compares strings only. It reads `< Y` and `<= Y`; any other form is reported as not + naming the same end, even if it does. +- The leading-zero checks read spelling only, and cover range events in `npm` packages and `SEMVER` ranges. They + cannot tell whether a version was published. Other ecosystems can allow these spellings, so `validate` says + nothing about them there. - The tool does not compare versions, so a `range-bound-converted` finding cannot say whether any release changes status. Check the package's release history. If a value stays the same on both sides, the finding says that this end now includes or excludes it. Several values of one kind in a range are listed without diff --git a/src/core/diff/affected.ts b/src/core/diff/affected.ts index 3937534..6e902ec 100644 --- a/src/core/diff/affected.ts +++ b/src/core/diff/affected.ts @@ -1,7 +1,8 @@ import { MAX_MATCHING_WORK, PAIRING_LIMIT_WHY, overlap, pairItems, setMinus } from "../pairing.js"; import type { WorkBudget } from "../pairing.js"; import type { Finding, JsonObject, JsonValue } from "../types.js"; -import { asArray, asObjectArray, canonical, clone, getOwn, isObject, isString, shorten } from "../util.js"; +import { asArray, asObjectArray, canonical, clone, getOwn, hasOwn, isObject, isString, shorten } from "../util.js"; +import { differsOnlyInLeadingZeros, hasLeadingZero, rejectsLeadingZeros } from "../versionSpelling.js"; import { diffGeneric, diffOtherKeys } from "./generic.js"; import { diffSeverity } from "./severity.js"; @@ -152,6 +153,88 @@ function boundConvertedWhy(git: boolean, valueKept: boolean): string { : `${ends} Replacing one with the other may change which releases are affected, or may change nothing, depending on what the package has released. AdvisoryGuard does not compare versions, so check the package's release history.`; } +const KNOWN_RANGE = "last_known_affected_version_range"; + +function knownRangeOf(entry: JsonObject): JsonValue | undefined { + const specific = getOwn(entry, "database_specific"); + return isObject(specific) ? getOwn(specific, KNOWN_RANGE) : undefined; +} + +/** The known range an edit removed from this entry, if it was a string. */ +function knownRangeRemoved(b: JsonObject, a: JsonObject): string | undefined { + const known = knownRangeOf(b); + if (!isString(known)) return undefined; + const specific = getOwn(a, "database_specific"); + return isObject(specific) && hasOwn(specific, KNOWN_RANGE) ? undefined : known; +} + +function withoutKnownRange(entry: JsonValue): JsonValue { + if (!isObject(entry)) return entry; + const specific = getOwn(entry, "database_specific"); + if (isObject(specific)) { + delete specific["last_known_affected_version_range"]; + if (Object.keys(specific).length === 0) delete entry["database_specific"]; + } + return entry; +} + +interface AddedEnd { + kind: "fixed" | "last_affected"; + value: string; +} + +/** The one end a range gained, when that is the only change to its events. */ +function soleAddedEnd(gone: Record, fresh: Record): AddedEnd | undefined { + if (EVENT_KINDS.some((k) => gone[k].length > 0) || fresh.introduced.length > 0 || fresh.limit.length > 0) return undefined; + const ends: AddedEnd[] = [ + ...fresh.fixed.map((value) => ({ kind: "fixed" as const, value })), + ...fresh.last_affected.map((value) => ({ kind: "last_affected" as const, value })), + ]; + return ends.length === 1 ? ends[0] : undefined; +} + +/** Whether the text names exactly this end. This compares strings only, not versions. */ +function knownRangeNames(known: string, end: AddedEnd): boolean { + const m = /^\s*(<=|<)\s*(\S+)\s*$/.exec(known); + if (!m) return false; + return m[2] === end.value && m[1] === (end.kind === "fixed" ? "<" : "<="); +} + +function knownRangeConverted(known: string, end: AddedEnd, where: string, path: string): Finding { + const same = knownRangeNames(known, end); + const seen = "Tools that read only the range events now see this end, which they did not before, so versions past it may stop being flagged."; + return { + code: "known-range-converted", + level: "review", + path, + message: `${KNOWN_RANGE} "${shorten(known, 80)}" was replaced by "${end.kind}" ${end.value} in ${where}: ${same ? "both name the same end" : "the two do not name the same end"}.`, + why: same + ? `${seen} Confirm the value is right.` + : `${seen} AdvisoryGuard does not compare versions, so check the package's release history to confirm the new end is right.`, + before: known, + after: end.value, + }; +} + +function spellingChanged(kind: EventKind, from: string, to: string, where: string, path: string, strict: boolean): Finding { + const fromZero = hasLeadingZero(from); + const toZero = hasLeadingZero(to); + const change = fromZero && !toZero ? ", with leading zeros removed" : !fromZero && toZero ? ", with leading zeros added" : ""; + return { + code: "version-spelling-changed", + level: "review", + path, + message: change + ? `"${kind}" value changed spelling in ${where}: ${from} -> ${to}${change}. The numbers are the same.` + : `"${kind}" values in ${where} differ only in leading zeros: ${from} -> ${to}.`, + why: strict + ? "Semantic versions cannot have leading zeros, so a value with them may not match any published release, and tools may read the two spellings differently. Confirm the spelling that was published." + : "Some version schemes treat these spellings as the same version and others do not, so tools may read them differently. Confirm the spelling that was published.", + before: from, + after: to, + }; +} + function reportBoundConversions( gone: Record, fresh: Record, @@ -202,7 +285,20 @@ function reportBoundConversions( } } -function diffRanges(b: JsonObject, a: JsonObject, label: string, path: string, out: Finding[], ctx: MatchContext): void { +/** + * Compare the ranges of a pair of entries. `known` is a known range the edit removed; returns whether a + * range's new end was reported as replacing it, in which case the caller should not report it as removed. + */ +function diffRanges( + b: JsonObject, + a: JsonObject, + label: string, + path: string, + ecosystem: string | undefined, + known: string | undefined, + out: Finding[], + ctx: MatchContext, +): boolean { const bs = asObjectArray(getOwn(b, "ranges")); const as = asObjectArray(getOwn(a, "ranges")); const rangeId = (r: JsonObject): string => `${textOf(getOwn(r, "type"), "?")}|${textOf(getOwn(r, "repo"), "")}`; @@ -220,32 +316,58 @@ function diffRanges(b: JsonObject, a: JsonObject, label: string, path: string, o if (restB.some((bi) => idsA.has(rangeId(bs[bi] as JsonObject)))) ctx.rangesSkipped = true; } - for (const { bi, ai, exact } of pairs) { + const valuesOf = (r: JsonObject, kind: EventKind): string[] => + asObjectArray(getOwn(r, "events")).map((e) => getOwn(e, kind)).filter(isString); + const eventDelta = (x: JsonObject, y: JsonObject): Record => ({ + introduced: setMinus(valuesOf(x, "introduced"), valuesOf(y, "introduced")).sort(), + fixed: setMinus(valuesOf(x, "fixed"), valuesOf(y, "fixed")).sort(), + last_affected: setMinus(valuesOf(x, "last_affected"), valuesOf(y, "last_affected")).sort(), + limit: setMinus(valuesOf(x, "limit"), valuesOf(y, "limit")).sort(), + }); + + // If several ranges each gained an end, which one replaced the known range cannot be known, and + // array order must not decide. They are then reported as ordinary additions. + const candidates = known === undefined + ? [] + : pairs.filter(({ bi, ai, exact }) => { + const rb = bs[bi]; + const ra = as[ai]; + return !exact && rb && ra && soleAddedEnd(eventDelta(rb, ra), eventDelta(ra, rb)) !== undefined; + }); + const replacing = candidates.length === 1 ? candidates[0] : undefined; + + for (const pair of pairs) { + const { bi, ai, exact } = pair; if (exact) continue; const rb = bs[bi]; const ra = as[ai]; if (!rb || !ra) continue; const rtype = textOf(getOwn(rb, "type"), "range"); const where = `${rtype} range of ${label}`; - const valuesOf = (r: JsonObject, kind: EventKind): string[] => - asObjectArray(getOwn(r, "events")).map((e) => getOwn(e, kind)).filter(isString); - const eventDelta = (x: JsonObject, y: JsonObject): Record => ({ - introduced: setMinus(valuesOf(x, "introduced"), valuesOf(y, "introduced")).sort(), - fixed: setMinus(valuesOf(x, "fixed"), valuesOf(y, "fixed")).sort(), - last_affected: setMinus(valuesOf(x, "last_affected"), valuesOf(y, "last_affected")).sort(), - limit: setMinus(valuesOf(x, "limit"), valuesOf(y, "limit")).sort(), - }); const goneByKind = eventDelta(rb, ra); const freshByKind = eventDelta(ra, rb); const afterNamesFixed = valuesOf(ra, "fixed").length > 0; const afterHasEnd = afterNamesFixed || valuesOf(ra, "last_affected").length > 0; reportBoundConversions(goneByKind, freshByKind, where, path, rtype === "GIT", afterNamesFixed, out); + if (pair === replacing && known !== undefined) { + const end = soleAddedEnd(goneByKind, freshByKind); + if (end) { + out.push(knownRangeConverted(known, end, where, path)); + freshByKind[end.kind] = []; + } + } for (const kind of EVENT_KINDS) { const gone = goneByKind[kind]; const fresh = freshByKind[kind]; // Which of several values replaced which cannot be known without comparing versions. const n = gone.length === 1 && fresh.length === 1 ? 1 : 0; for (let k = 0; k < n; k++) { + const from = gone[k] as string; + const to = fresh[k] as string; + if (rtype !== "GIT" && differsOnlyInLeadingZeros(from, to)) { + out.push(spellingChanged(kind, from, to, where, path, rejectsLeadingZeros(ecosystem, rtype))); + continue; + } out.push({ code: "range-event-changed", level: "review", @@ -316,6 +438,7 @@ function diffRanges(b: JsonObject, a: JsonObject, label: string, path: string, o after: canonical(r), }); } + return replacing !== undefined; } function diffVersions(b: JsonObject, a: JsonObject, label: string, path: string, out: Finding[]): void { @@ -423,9 +546,56 @@ function diffPair(b: JsonObject, a: JsonObject, ai: number, out: Finding[], ctx: } diffSeverity(getOwn(b, "severity"), getOwn(a, "severity"), `${path}.severity`, ` (affected ${label})`, out); - diffRanges(b, a, label, path, out, ctx); + const replaced = diffRanges(b, a, label, path, pa?.ecosystem ?? pb?.ecosystem, knownRangeRemoved(b, a), out, ctx); diffVersions(b, a, label, path, out); - diffGeneric(remainder(b), remainder(a), path, out); + // Every other key of database_specific is still compared, so a value dropped alongside it is still reported. + const rest = (entry: JsonObject): JsonValue => (replaced ? withoutKnownRange(remainder(entry)) : remainder(entry)); + diffGeneric(rest(b), rest(a), path, out); +} + +/** + * Packages in one advisory often share a fix. Report any that had the same version data as a package + * this edit changed, but did not change themselves. One finding per group keeps long lists readable. + */ +function reportLeftOutPackages(bs: JsonObject[], as: JsonObject[], matched: { bi: number; ai: number }[], out: Finding[]): void { + const dataKey = (entry: JsonObject): string | null => { + const ranges = asArray(getOwn(entry, "ranges")); + const known = knownRangeOf(entry); + return ranges.length === 0 && known === undefined ? null : JSON.stringify([canonical(ranges), canonical(known)]); + }; + const groups = new Map; same: Set }>(); + for (const { bi, ai } of matched) { + const b = bs[bi]; + const a = as[ai]; + if (!b || !a) continue; + const p = pkgOf(a); + if (!p || (!p.name && !p.ecosystem)) continue; + const key = dataKey(b); + if (key === null) continue; + let group = groups.get(key); + if (!group) { + group = { changed: new Set(), same: new Set() }; + groups.set(key, group); + } + // Only the ranges count as a change: a known range note can go on its own without changing what the advisory covers. + const rangesChanged = canonical(asArray(getOwn(a, "ranges"))) !== canonical(asArray(getOwn(b, "ranges"))); + (rangesChanged ? group.changed : group.same).add(labelOf(a, ai)); + } + for (const key of [...groups.keys()].sort()) { + const group = groups.get(key); + if (!group || group.changed.size === 0) continue; + // Two entries for one package can differ on purpose, so that is not a package left out. + const leftOut = [...group.same].filter((l) => !group.changed.has(l)).sort(); + if (leftOut.length === 0) continue; + out.push({ + code: "sibling-package-unchanged", + level: "review", + path: `affected[${leftOut[0] as string}]`, + message: `Version ranges changed for ${previewList([...group.changed].sort())} but not for ${previewList(leftOut)}, which had the same version data before this edit.`, + why: "Packages in one advisory often share a fix. If these do, they need the same change. If they really differ, this can be allowed.", + after: leftOut, + }); + } } /** @@ -546,6 +716,7 @@ export function diffAffected( for (const { bi } of changed) { for (const f of reports.get(bi) ?? []) out.push(f); } + reportLeftOutPackages(bs, as, matched, out); if (ctx.rangesSkipped) { out.push({ diff --git a/src/core/validate.ts b/src/core/validate.ts index 6bc0de5..7311bda 100644 --- a/src/core/validate.ts +++ b/src/core/validate.ts @@ -5,6 +5,7 @@ import type { DuplicateKey } from "./load.js"; import { LATEST_KNOWN_SCHEMA_MINOR, SUPPORTED_SCHEMA_MAJOR, loadOsvSchema } from "./osvSchema.js"; import type { Finding, JsonObject, JsonValue, ValidationResult } from "./types.js"; import { asArray, asObjectArray, getOwn, isObject, isString, shorten } from "./util.js"; +import { hasLeadingZero, rejectsLeadingZeros, withoutLeadingZeros } from "./versionSpelling.js"; let compiled: ValidateFunction | undefined; @@ -294,14 +295,14 @@ function checkAffected(affected: JsonValue | undefined, out: Finding[]): void { } if (Array.isArray(ranges)) { ranges.forEach((range, j) => { - if (isObject(range)) checkRange(range, `${base}.ranges[${j}]`, out); + if (isObject(range)) checkRange(range, `${base}.ranges[${j}]`, isObject(pkg) ? getOwn(pkg, "ecosystem") : undefined, out); }); } checkSeverityList(getOwn(entry, "severity"), `${base}.severity`, out); }); } -function checkRange(range: JsonObject, path: string, out: Finding[]): void { +function checkRange(range: JsonObject, path: string, ecosystem: JsonValue | undefined, out: Finding[]): void { const type = getOwn(range, "type"); const events = getOwn(range, "events"); if (!Array.isArray(events)) return; @@ -320,6 +321,13 @@ function checkRange(range: JsonObject, path: string, out: Finding[]): void { path: evPath, message: `"${shorten(value, 60)}" is not a valid semantic version, but the range type is SEMVER.`, }); + } else if (rejectsLeadingZeros(ecosystem, type) && hasLeadingZero(value)) { + out.push({ + code: "version-leading-zero", + level: "warning", + path: evPath, + message: `"${shorten(value, 60)}" has a leading zero, which semantic versions do not allow, so it may not match any published release. The likely spelling is ${shorten(withoutLeadingZeros(value), 60)}.`, + }); } } }); diff --git a/src/core/versionSpelling.ts b/src/core/versionSpelling.ts new file mode 100644 index 0000000..1d7185b --- /dev/null +++ b/src/core/versionSpelling.ts @@ -0,0 +1,29 @@ +/** + * Leading zeros in a version's numeric parts. This only reads spelling: AdvisoryGuard still does not + * compare versions. A `v` prefix, pre-release and build are kept as written. + */ +const VERSION = /^(v?)([0-9]+(?:[.][0-9]+)*)([-+].*)?$/s; + +export function withoutLeadingZeros(value: string): string { + const m = VERSION.exec(value); + if (!m) return value; + const core = (m[2] as string) + .split(".") + .map((part) => part.replace(/^0+(?=[0-9])/, "")) + .join("."); + return `${m[1] ?? ""}${core}${m[3] ?? ""}`; +} + +export function hasLeadingZero(value: string): boolean { + return withoutLeadingZeros(value) !== value; +} + +export function differsOnlyInLeadingZeros(a: string, b: string): boolean { + return a !== b && withoutLeadingZeros(a) === withoutLeadingZeros(b); +} + +/** npm versions are semantic versions, and a SEMVER range declares that it holds them. */ +export function rejectsLeadingZeros(ecosystem: unknown, rangeType: unknown): boolean { + if (rangeType === "GIT") return false; + return rangeType === "SEMVER" || ecosystem === "npm"; +} diff --git a/test/real-edits.test.ts b/test/real-edits.test.ts new file mode 100644 index 0000000..5e44716 --- /dev/null +++ b/test/real-edits.test.ts @@ -0,0 +1,402 @@ +// Checks for three edit shapes seen in real advisory pull requests. The records are synthetic. +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, describe, expect, it } from "vitest"; +import { run } from "../src/cli/main.js"; +import type { CliIo } from "../src/cli/main.js"; +import type { JsonObject } from "../src/core/types.js"; +import { validateText } from "../src/index.js"; +import { renderDiff } from "../src/report/text.js"; +import { createStyle } from "../src/report/style.js"; +import { codes, diff, reviewCodes } from "./helpers.js"; + +const tmp = mkdtempSync(join(tmpdir(), "advisoryguard-real-edits-")); +afterAll(() => rmSync(tmp, { recursive: true, force: true })); + +const KNOWN = "last_known_affected_version_range"; + +interface EntryOptions { + known?: unknown; + ecosystem?: string; + type?: string; + extra?: Record; +} + +function entry(name: string, events: JsonObject[], opts: EntryOptions = {}): JsonObject { + const { known, ecosystem = "Maven", type = "ECOSYSTEM", extra = {} } = opts; + return { + package: { ecosystem, name }, + ranges: [{ type, events }], + ...(known === undefined ? {} : { database_specific: { [KNOWN]: known } }), + ...extra, + } as JsonObject; +} + +function record(...affected: JsonObject[]): JsonObject { + return { + schema_version: "1.4.0", + id: "GHSA-2222-3333-4444", + modified: "2026-01-01T00:00:00Z", + published: "2026-01-01T00:00:00Z", + summary: "Synthetic advisory", + details: "Synthetic advisory used for local regression tests.", + severity: [{ type: "CVSS_V3", score: "CVSS:3.1/AV:N/AC:L/PR:L/UI:N/S:U/C:L/I:N/A:N" }], + affected, + references: [{ type: "WEB", url: "https://example.test/advisory" }], + } as JsonObject; +} + +function cli(args: string[]): { code: number; out: string } { + let out = ""; + const io: CliIo = { stdout: { write: (s: string) => ((out += s), true) }, stderr: { write: () => true }, env: {} }; + return { code: run(args, io), out }; +} + +function cliDiff(before: JsonObject, after: JsonObject, extra: string[] = []): { code: number; out: string } { + const b = join(tmp, "before.json"); + const a = join(tmp, "after.json"); + writeFileSync(b, JSON.stringify(before)); + writeFileSync(a, JSON.stringify(after)); + return cli(["diff", b, a, ...extra]); +} + +const OPEN: JsonObject[] = [{ introduced: "0" }]; +const fixedAt = (v: string): JsonObject[] => [{ introduced: "0" }, { fixed: v }]; +const lastAt = (v: string): JsonObject[] => [{ introduced: "0" }, { last_affected: v }]; + +describe("a known range replaced by an end event", () => { + const before = record(entry("a:lib", OPEN, { known: "< 2.1.0" })); + const after = record(entry("a:lib", fixedAt("2.1.0"))); + + it("is one review finding, not an added event and a removed field", () => { + const r = diff(before, after); + expect(reviewCodes(r)).toEqual(["known-range-converted"]); + expect(codes(r)).not.toContain("field-removed"); + expect(codes(r)).not.toContain("range-event-added"); + const f = r.findings[0]; + expect(f?.before).toBe("< 2.1.0"); + expect(f?.after).toBe("2.1.0"); + expect(f?.message).toContain("< 2.1.0"); + expect(f?.message).toContain("same"); + expect(f?.why).toMatch(/flagged/); + }); + + it("treats <= and last_affected as the same end", () => { + const r = diff(record(entry("a:lib", OPEN, { known: "<= 2.0.9" })), record(entry("a:lib", lastAt("2.0.9")))); + expect(reviewCodes(r)).toEqual(["known-range-converted"]); + expect(r.findings[0]?.message).toContain("same"); + }); + + it.each([ + ["a different value", "< 2.1.0", fixedAt("3.0.0")], + ["<= with fixed at that value", "<= 2.1.0", fixedAt("2.1.0")], + ["< with last_affected at that value", "< 2.1.0", lastAt("2.1.0")], + ["a form it cannot read", ">= 1.0, < 2.1.0", fixedAt("2.1.0")], + ])("says so when the two do not name the same end: %s", (_name, known, events) => { + const r = diff(record(entry("a:lib", OPEN, { known })), record(entry("a:lib", events))); + expect(reviewCodes(r)).toEqual(["known-range-converted"]); + const f = r.findings[0]; + expect(f?.message).not.toContain("both name"); + expect(f?.message).toContain("do not name"); + expect(f?.why).toContain("does not compare versions"); + }); + + it("needs its own code in an allow list", () => { + expect(diff(before, after, ["known-range-converted"]).status).toBe("clean"); + expect(diff(before, after, ["range-event-added", "field-removed"]).status).toBe("review"); + expect(cliDiff(before, after).code).toBe(1); + expect(cliDiff(before, after, ["--allow", "known-range-converted"]).code).toBe(0); + }); + + it("still reports another key dropped from database_specific", () => { + const b = record(entry("a:lib", OPEN, { extra: { database_specific: { [KNOWN]: "< 2.1.0", note: "kept" } } })); + const r = diff(b, record(entry("a:lib", fixedAt("2.1.0")))); + expect(reviewCodes(r).sort()).toEqual(["field-removed", "known-range-converted"]); + const removed = r.findings.find((f) => f.code === "field-removed"); + expect(removed?.before).toEqual({ note: "kept" }); + expect(removed?.message).not.toContain(KNOWN); + }); + + it("does not report database_specific when only the known range left it", () => { + const b = record(entry("a:lib", OPEN, { extra: { database_specific: { [KNOWN]: "< 2.1.0", note: "kept" } } })); + const a = record(entry("a:lib", fixedAt("2.1.0"), { extra: { database_specific: { note: "kept" } } })); + expect(reviewCodes(diff(b, a))).toEqual(["known-range-converted"]); + const emptied = record(entry("a:lib", fixedAt("2.1.0"), { extra: { database_specific: {} } })); + expect(reviewCodes(diff(before, emptied))).toEqual(["known-range-converted"]); + }); + + it("does not apply when no end was added", () => { + const r = diff(before, record(entry("a:lib", OPEN))); + expect(reviewCodes(r)).toEqual(["field-removed"]); + }); + + it("does not apply when the known range stays", () => { + const r = diff(before, record(entry("a:lib", fixedAt("2.1.0"), { known: "< 2.1.0" }))); + expect(reviewCodes(r)).toEqual(["range-event-added"]); + }); + + it("does not apply when an end was also removed", () => { + const r = diff(record(entry("a:lib", lastAt("2.0.0"), { known: "<= 2.0.0" })), record(entry("a:lib", fixedAt("2.1.0")))); + expect(reviewCodes(r).sort()).toEqual(["field-removed", "range-bound-converted"]); + }); + + it("does not apply when the known range is not a string", () => { + const r = diff(record(entry("a:lib", OPEN, { known: 5 })), record(entry("a:lib", fixedAt("2.1.0")))); + expect(reviewCodes(r).sort()).toEqual(["field-removed", "range-event-added"]); + }); + + it("does not guess between two ranges that each gain an end, in either order", () => { + const rangeA = { type: "ECOSYSTEM", events: [{ introduced: "1.0.0" }] }; + const rangeB = { type: "ECOSYSTEM", events: [{ introduced: "5.0.0" }] }; + const endA = { type: "ECOSYSTEM", events: [{ introduced: "1.0.0" }, { fixed: "2.0.0" }] }; + const endB = { type: "ECOSYSTEM", events: [{ introduced: "5.0.0" }, { fixed: "6.0.0" }] }; + const mk = (ranges: unknown[], known?: string): JsonObject => + record({ package: { ecosystem: "Maven", name: "a:lib" }, ranges, ...(known ? { database_specific: { [KNOWN]: known } } : {}) } as JsonObject); + const r1 = diff(mk([rangeA, rangeB], "< 6.0.0"), mk([endA, endB])); + const r2 = diff(mk([rangeB, rangeA], "< 6.0.0"), mk([endB, endA])); + expect(reviewCodes(r1).sort()).toEqual(["field-removed", "range-event-added", "range-event-added"]); + expect(reviewCodes(r2).sort()).toEqual(reviewCodes(r1).sort()); + }); +}); + +describe("packages left out of a version change", () => { + it("names the package that kept the old range", () => { + const r = diff( + record(entry("a:api", OPEN, { known: "< 2.1.0" }), entry("a:server", OPEN, { known: "< 2.1.0" })), + record(entry("a:api", fixedAt("2.1.0")), entry("a:server", OPEN, { known: "< 2.1.0" })), + ); + expect(reviewCodes(r).sort()).toEqual(["known-range-converted", "sibling-package-unchanged"]); + const f = r.findings.find((x) => x.code === "sibling-package-unchanged"); + expect(f?.message).toContain("Maven/a:api"); + expect(f?.message).toContain("Maven/a:server"); + expect(f?.after).toEqual(["Maven/a:server"]); + expect(f?.why).toMatch(/fix/); + }); + + it("gives one finding listing every package left out", () => { + const r = diff( + record(entry("a:one", OPEN, { known: "< 2" }), entry("a:two", OPEN, { known: "< 2" }), entry("a:three", OPEN, { known: "< 2" })), + record(entry("a:one", fixedAt("2")), entry("a:two", OPEN, { known: "< 2" }), entry("a:three", OPEN, { known: "< 2" })), + ); + const found = r.findings.filter((x) => x.code === "sibling-package-unchanged"); + expect(found).toHaveLength(1); + expect(found[0]?.after).toEqual(["Maven/a:three", "Maven/a:two"]); + }); + + it("applies to ordinary ranges too", () => { + const r = diff( + record(entry("a:one", lastAt("1.0.0")), entry("a:two", lastAt("1.0.0"))), + record(entry("a:one", fixedAt("1.0.1")), entry("a:two", lastAt("1.0.0"))), + ); + expect(reviewCodes(r).sort()).toEqual(["range-bound-converted", "sibling-package-unchanged"]); + }); + + it("does not depend on array order", () => { + const b = [entry("a:one", OPEN, { known: "< 2" }), entry("a:two", OPEN, { known: "< 2" }), entry("a:three", OPEN, { known: "< 2" })]; + const a = [entry("a:one", fixedAt("2")), entry("a:two", fixedAt("2")), entry("a:three", OPEN, { known: "< 2" })]; + const pick = (x: JsonObject[], y: JsonObject[]) => + diff(record(...x), record(...y)).findings.filter((f) => f.code === "sibling-package-unchanged").map((f) => [f.message, f.after, f.path]); + expect(pick([...b].reverse(), a)).toEqual(pick(b, a)); + expect(pick(b, [...a].reverse())).toEqual(pick(b, a)); + }); + + it("can be allowed, and that does not clear the change itself", () => { + const b = record(entry("a:one", OPEN, { known: "< 2" }), entry("a:two", OPEN, { known: "< 2" })); + const a = record(entry("a:one", fixedAt("2")), entry("a:two", OPEN, { known: "< 2" })); + expect(diff(b, a, ["sibling-package-unchanged"]).status).toBe("review"); + expect(diff(b, a, ["sibling-package-unchanged", "known-range-converted"]).status).toBe("clean"); + }); + + it.each([ + ["every package changed", [entry("a:one", lastAt("1")), entry("a:two", lastAt("1"))], [entry("a:one", fixedAt("2")), entry("a:two", fixedAt("2"))]], + ["the two changed differently", [entry("a:one", lastAt("1")), entry("a:two", lastAt("1"))], [entry("a:one", fixedAt("2")), entry("a:two", fixedAt("3"))]], + ["the ranges differed before", [entry("a:one", lastAt("1")), entry("a:two", lastAt("2"))], [entry("a:one", fixedAt("2")), entry("a:two", lastAt("2"))]], + ["the known ranges differed before", [entry("a:one", OPEN, { known: "< 1" }), entry("a:two", OPEN, { known: "< 2" })], [entry("a:one", fixedAt("1")), entry("a:two", OPEN, { known: "< 2" })]], + ["a single package", [entry("a:one", lastAt("1"))], [entry("a:one", fixedAt("2"))]], + ["nothing changed", [entry("a:one", lastAt("1")), entry("a:two", lastAt("1"))], [entry("a:one", lastAt("1")), entry("a:two", lastAt("1"))]], + ["only a reference changed", [entry("a:one", lastAt("1"), { extra: { note: "x" } }), entry("a:two", lastAt("1"))], [entry("a:one", lastAt("1"), { extra: { note: "y" } }), entry("a:two", lastAt("1"))]], + ["a package was only added", [entry("a:one", lastAt("1"))], [entry("a:one", lastAt("1")), entry("a:new", lastAt("1"), { ecosystem: "npm" })]], + ["two entries for one package", [entry("a:one", lastAt("1")), entry("a:one", lastAt("1"))], [entry("a:one", fixedAt("2")), entry("a:one", lastAt("1"))]], + ])("stays quiet when %s", (_name, b, a) => { + expect(codes(diff(record(...b), record(...a)))).not.toContain("sibling-package-unchanged"); + }); + + it("stays quiet when only a known range note left one package and its ranges did not change", () => { + const r = diff( + record(entry("a:one", fixedAt("2"), { known: "<= 1" }), entry("a:two", fixedAt("2"), { known: "<= 1" })), + record(entry("a:one", fixedAt("2")), entry("a:two", fixedAt("2"), { known: "<= 1" })), + ); + expect(codes(r)).not.toContain("sibling-package-unchanged"); + expect(reviewCodes(r)).toEqual(["field-removed"]); + }); + + it("counts a package whose note changed but whose ranges did not as left out", () => { + const r = diff( + record(entry("a:one", OPEN, { known: "< 2" }), entry("a:two", OPEN, { known: "< 2" })), + record(entry("a:one", fixedAt("2")), entry("a:two", OPEN)), + ); + const f = r.findings.find((x) => x.code === "sibling-package-unchanged"); + expect(f?.after).toEqual(["Maven/a:two"]); + }); + + it("stays quiet when the packages have no version data to compare", () => { + const bare = (name: string): JsonObject => ({ package: { ecosystem: "Maven", name }, versions: ["1.0.0"] }) as JsonObject; + const r = diff(record(bare("a:one"), bare("a:two")), record(entry("a:one", lastAt("1")), bare("a:two"))); + expect(codes(r)).not.toContain("sibling-package-unchanged"); + }); + + it("stays quiet about an entry with no package", () => { + const c1 = "1".repeat(40); + const c2 = "2".repeat(40); + const ranges = (events: JsonObject[]): JsonObject[] => [{ type: "GIT", repo: "https://example.test/r", events }] as unknown as JsonObject[]; + const bare = (events: JsonObject[]): JsonObject => ({ ranges: ranges(events) }) as JsonObject; + const named = (events: JsonObject[]): JsonObject => entry("a:two", events, { extra: { ranges: ranges(events) } }); + // The entries have identical ranges, so only the missing package keeps the bare one from being named. + const r = diff(record(bare(lastAt(c1)), named(lastAt(c1))), record(bare(lastAt(c1)), named(fixedAt(c2)))); + expect(reviewCodes(r)).toContain("range-bound-converted"); + expect(codes(r)).not.toContain("sibling-package-unchanged"); + }); + + it("stays small for a very long list of packages", () => { + const names = Array.from({ length: 3000 }, (_, i) => `g:p${i}`); + const r = diff( + record(...names.map((n) => entry(n, lastAt("1")))), + record(...names.map((n, i) => entry(n, i === 0 ? fixedAt("2") : lastAt("1")))), + ); + const found = r.findings.filter((x) => x.code === "sibling-package-unchanged"); + expect(found).toHaveLength(1); + expect(found[0]?.message.length).toBeLessThan(600); + expect(found[0]?.after).toHaveLength(2999); + }); +}); + +describe("version spelling in validate", () => { + const warned = (events: JsonObject[], opts: EntryOptions = {}): string[] => { + const v = validateText(JSON.stringify(record(entry("pkg", events, opts)))); + expect(v.valid).toBe(true); + return v.findings.filter((f) => f.code === "version-leading-zero").map((f) => f.message); + }; + + it.each([ + ["fixed", [{ introduced: "0" }, { fixed: "2025.08.14" }], "2025.8.14"], + ["last_affected", [{ introduced: "0" }, { last_affected: "20260509.0340.15" }], "20260509.340.15"], + ["introduced", [{ introduced: "01.2.3" }], "1.2.3"], + ["a v prefix", [{ introduced: "0" }, { fixed: "v1.02.3" }], "v1.2.3"], + ["a pre-release", [{ introduced: "0" }, { fixed: "1.02.3-rc.1" }], "1.2.3-rc.1"], + ])("warns on npm: %s", (_name, events, suggestion) => { + const m = warned(events as unknown as JsonObject[], { ecosystem: "npm" }); + expect(m).toHaveLength(1); + expect(m[0]).toContain(suggestion); + }); + + it("warns for a SEMVER range in any ecosystem", () => { + expect(warned(fixedAt("1.02.3"), { ecosystem: "PyPI", type: "SEMVER" })).toHaveLength(1); + }); + + it.each([ + ["a clean npm version", fixedAt("2025.8.14"), { ecosystem: "npm" }], + ["zero parts", [{ introduced: "0" }, { fixed: "0.0.0" }, { introduced: "1.0.0" }, { fixed: "1.10.0" }], { ecosystem: "npm" }], + ["a pre-release with leading zeros of its own", fixedAt("1.2.3-01"), { ecosystem: "npm" }], + ["an ecosystem that allows them", fixedAt("1.02.3"), { ecosystem: "PyPI" }], + ["a date-like Go pseudo-version part", fixedAt("0.0.0-20230102150405-0123456789ab"), { ecosystem: "Go" }], + ])("stays quiet on %s", (_name, events, opts) => { + expect(warned(events as JsonObject[], opts as EntryOptions)).toEqual([]); + }); + + it("stays quiet for a GIT range on an npm package", () => { + // All digits with a leading zero, so only the range type keeps it from looking like a misspelled version. + const commit = "0123456789012345678901234567890123456789"; + const ranges = [{ type: "GIT", repo: "https://example.test/r", events: fixedAt(commit) }]; + expect(warned(fixedAt(commit), { ecosystem: "npm", extra: { ranges } })).toEqual([]); + }); + + it("is a warning, so validate still exits 0", () => { + const p = join(tmp, "spelling.json"); + writeFileSync(p, JSON.stringify(record(entry("pkg", fixedAt("2025.08.14"), { ecosystem: "npm" })))); + const r = cli(["validate", p]); + expect(r.code).toBe(0); + expect(r.out).toContain("version-leading-zero"); + }); +}); + +describe("version spelling in diff", () => { + const npm = (v: string): JsonObject => record(entry("pkg", fixedAt(v), { ecosystem: "npm" })); + + it("reports removing leading zeros as a spelling change", () => { + const r = diff(npm("2025.08.14"), npm("2025.8.14")); + expect(reviewCodes(r)).toEqual(["version-spelling-changed"]); + const f = r.findings[0]; + expect(f?.before).toBe("2025.08.14"); + expect(f?.after).toBe("2025.8.14"); + expect(f?.message).toContain("leading zeros removed"); + expect(f?.why).toContain("cannot have leading zeros"); + expect(f?.why).not.toContain("between"); + }); + + it("says so when a leading zero is introduced", () => { + const r = diff(npm("2025.8.14"), npm("2025.08.14")); + expect(reviewCodes(r)).toEqual(["version-spelling-changed"]); + expect(r.findings[0]?.message).toContain("leading zeros added"); + expect(r.after.findings.map((f) => f.code)).toContain("version-leading-zero"); + }); + + it("does not claim a direction when zeros moved", () => { + const r = diff(npm("1.02.3"), npm("1.2.03")); + expect(reviewCodes(r)).toEqual(["version-spelling-changed"]); + expect(r.findings[0]?.message).toContain("differ only in leading zeros"); + }); + + it("uses softer wording where the version scheme allows them", () => { + const py = (v: string): JsonObject => record(entry("pkg", fixedAt(v), { ecosystem: "PyPI" })); + const r = diff(py("1.02.3"), py("1.2.3")); + expect(reviewCodes(r)).toEqual(["version-spelling-changed"]); + expect(r.findings[0]?.why).toContain("Some version schemes"); + }); + + it("applies to last_affected and introduced", () => { + const m = (e: JsonObject[]): JsonObject => record(entry("pkg", e, { ecosystem: "npm" })); + expect(reviewCodes(diff(m(lastAt("1.02.0")), m(lastAt("1.2.0"))))).toEqual(["version-spelling-changed"]); + expect(reviewCodes(diff(m([{ introduced: "01.0.0" }]), m([{ introduced: "1.0.0" }])))).toEqual(["version-spelling-changed"]); + }); + + it.each([ + ["a real version change", "1.2.0", "1.3.0"], + ["a different number of parts", "1.2", "1.2.0"], + ["a v prefix", "v1.2.3", "1.2.3"], + ["a pre-release difference", "1.2.3-01", "1.2.3-1"], + ["a build difference", "1.02.3+a", "1.2.3+b"], + ["removing a zero that changes the number", "1.20.0", "1.2.0"], + ])("leaves %s as an ordinary change", (_name, x, y) => { + expect(reviewCodes(diff(npm(x), npm(y)))).toEqual(["range-event-changed"]); + }); + + it("leaves GIT commits alone", () => { + const git = (c: string): JsonObject => record({ ranges: [{ type: "GIT", repo: "https://example.test/r", events: [{ introduced: "0" }, { fixed: c }] }] } as JsonObject); + expect(reviewCodes(diff(git("0123"), git("123")))).toEqual(["range-event-changed"]); + }); + + it("needs its own code in an allow list", () => { + expect(diff(npm("2025.08.14"), npm("2025.8.14"), ["version-spelling-changed"]).status).toBe("clean"); + expect(diff(npm("2025.08.14"), npm("2025.8.14"), ["range-event-changed"]).status).toBe("review"); + }); +}); + +describe("the shape of a partly converted multi-package advisory", () => { + const before = record(entry("dev:api", OPEN, { known: "< 2.1.0" }), entry("dev:server", OPEN, { known: "< 2.1.0" })); + const after = record(entry("dev:api", fixedAt("2.1.0")), entry("dev:server", OPEN, { known: "< 2.1.0" })); + + it("prints a report that names the left-out package", () => { + const r = diff(before, after); + const text = renderDiff("before.json", "after.json", r, createStyle(false)); + expect(text).toContain("sibling-package-unchanged"); + expect(text).toContain("Maven/dev:server"); + expect(cliDiff(before, after).code).toBe(1); + }); + + it("has no such finding once every package is converted", () => { + const all = record(entry("dev:api", fixedAt("2.1.0")), entry("dev:server", fixedAt("2.1.0"))); + const r = diff(before, all); + expect(reviewCodes(r)).toEqual(["known-range-converted", "known-range-converted"]); + }); +}); diff --git a/test/real.test.ts b/test/real.test.ts index 5aa341f..5a6348d 100644 --- a/test/real.test.ts +++ b/test/real.test.ts @@ -197,7 +197,13 @@ describe("genuine GitHub advisories: no false negatives (injected mistakes are c const ev = d.affected[idx].ranges.flatMap((r: any) => r.events).find((e: any) => e.fixed !== undefined); ev.fixed = ev.fixed + ".1"; const codes = run(orig, d).findings.map((x) => x.code); - expect(codes).toEqual(["range-event-changed"]); + expect(codes.filter((c) => c !== "sibling-package-unchanged")).toEqual(["range-event-changed"]); + // Editing one package also names any other package that had identical version data. + const versionData = (a: any): string => JSON.stringify([a.ranges, a.database_specific?.last_known_affected_version_range]); + const shared = affected.some( + (a, i) => i !== idx && a.package?.name !== affected[idx].package?.name && versionData(a) === versionData(affected[idx]), + ); + expect(codes.includes("sibling-package-unchanged")).toBe(shared); }); it.each(files)("%s: corrupting the record makes it invalid with a specific, located error", (f) => {