diff --git a/node/maintenance.md b/node/maintenance.md index c80b170..2978aa6 100644 --- a/node/maintenance.md +++ b/node/maintenance.md @@ -81,10 +81,30 @@ If initial or final audit evidence is unavailable or incomplete, the run fails and skips publication. The report identifies the cause and retains the separate frozen-install and project-check results. Its summary and report artifact remain available after failure; passing project checks do not make the audit complete. -The report leads with proposed versions and validation results. Routine-only +The report leads with a summary and validation results. Confirmed security fixes +appear in a table; the full dependency version list is expandable. Routine-only headlines omit unrelated security counts. Existing blocked findings with unchanged audit evidence appear as neutral background, with severity and constraints in expandable details. New findings and evidence that needs review remain visible. +Security-fix rows link to one primary reported advisory, and count additional +advisories with a pointer to the full audit details. The full audit details +retain every advisory link, including the underlying advisory for an indirectly +affected package. When npm newly lists an unchanged +parent package through an existing advisory, and the affected package versions +and installed paths are unchanged, the report explains this in the details. +This also requires unchanged parent lock records and previously reported +advisory sources through its immediate dependencies. +It does not flag that entry as a newly introduced vulnerability. Other newly +reported findings remain visible for review; an audit difference alone does not +prove that the proposal introduced a vulnerability. + +Routine-upgrade proposals also show a table of selected direct upgrades that +remain in the final proposal. Current and proposed values are the declared +`package.json` requirements. Development and optional dependencies are marked. +Transitive changes stay in the expandable full version list. Release-note links +open the GitHub releases page identified by published package metadata. If that +metadata is unavailable or names another host, the row links to the proposed +version's npm package details instead. Reports retain unresolved findings, pin decisions, validation failures, changed locked version sets, and candidate dependency-file SHA-256 hashes. Grouped logs diff --git a/node/runtime/templates/dependency_audit.py b/node/runtime/templates/dependency_audit.py index 8397a49..8b1c563 100644 --- a/node/runtime/templates/dependency_audit.py +++ b/node/runtime/templates/dependency_audit.py @@ -11,6 +11,7 @@ from datetime import datetime, timedelta, timezone from pathlib import Path from tempfile import TemporaryDirectory +from urllib.parse import quote import yaml @@ -1546,7 +1547,13 @@ def _npm_dependency_node(packages, parent, dependency): def _npm_dependency_reaches( - lock, root_name, target_name, target_nodes, *, root_nodes=None + lock, + root_name, + target_name, + target_nodes, + *, + root_nodes=None, + include_peer_dependencies=False, ): """Follow npm edges from a direct package or exact installed nodes.""" packages = lock.get("packages", {}) @@ -1573,7 +1580,10 @@ def _npm_dependency_reaches( continue if node in target_nodes and node.rsplit("node_modules/", 1)[-1] == target_name: return True - for section in ("dependencies", "optionalDependencies"): + sections = ("dependencies", "optionalDependencies") + if include_peer_dependencies: + sections += ("peerDependencies",) + for section in sections: dependencies = package.get(section, {}) if not isinstance(dependencies, dict): continue @@ -2191,6 +2201,188 @@ def newly_reported_findings(before, after, source): return added +def npm_advisory_sources(data, name): + """Follow propagated npm findings to their direct advisory records.""" + if not isinstance(data, dict): + return {}, False + findings = data.get("vulnerabilities", {}) + pending, visited, sources = [name], set(), {} + complete = True + while pending: + package = pending.pop() + if package in visited: + continue + visited.add(package) + via = findings.get(package, {}).get("via", []) + if not via: + complete = False + for detail in via: + if isinstance(detail, str): + pending.append(detail) + elif ( + isinstance(detail, dict) + and isinstance(detail.get("url"), str) + and detail["url"] + ): + sources.setdefault(package, {})[detail["url"]] = detail + else: + complete = False + return sources, complete and bool(sources) + + +def _npm_vulnerable_nodes(data, lock, name): + vulnerability = data.get("vulnerabilities", {}).get(name, {}) + nodes = vulnerability.get("nodes") + if isinstance(nodes, list): + recorded = {node for node in nodes if isinstance(node, str) and node} + if recorded: + return recorded + return set(package_paths(lock, name, "npm")) + + +def _npm_reachable_nodes(lock, root, target_name, target_nodes): + return { + node + for node in target_nodes + if _npm_dependency_reaches( + lock, + None, + target_name, + {node}, + root_nodes=[root], + include_peer_dependencies=True, + ) + } + + +def reported_parent_findings(before, after, source, before_lock, after_lock): + """Identify new parent entries backed only by unchanged existing findings.""" + if source != "npm" or not isinstance(before, dict) or not isinstance(after, dict): + return {} + parents = {} + for name in newly_reported_findings(before, after, source): + if name in before.get("vulnerabilities", {}): + continue + via = after["vulnerabilities"][name].get("via", []) + if not via or not all(isinstance(detail, str) for detail in via): + continue + old_paths = package_paths(before_lock, name, source) + if not old_paths or old_paths != package_paths(after_lock, name, source): + continue + if any( + before_lock["packages"][node] != after_lock["packages"][node] + for node in old_paths + ): + continue + parent_after = set(package_paths(after_lock, name, source)) + recorded_parent_nodes = after["vulnerabilities"][name].get("nodes") + if isinstance(recorded_parent_nodes, list): + parent_roots = { + node for node in recorded_parent_nodes if isinstance(node, str) and node + } + if not parent_roots or not parent_roots <= parent_after: + continue + else: + parent_roots = parent_after + # A previously unrelated dependency can become vulnerable through an + # already installed advisory source. Retain the warning for that case. + existing_chain = True + for dependency in via: + previous_sources, previous_complete = npm_advisory_sources( + before, dependency + ) + current_sources, current_complete = npm_advisory_sources(after, dependency) + if ( + not previous_complete + or not current_complete + or any( + not details.keys() <= previous_sources.get(package, {}).keys() + for package, details in current_sources.items() + ) + ): + existing_chain = False + break + if not existing_chain: + continue + sources, complete = npm_advisory_sources(after, name) + if not complete: + continue + for package, advisories in sources.items(): + previous, previous_complete = npm_advisory_sources(before, package) + existing = previous.get(package, {}) + paths = package_paths(before_lock, package, source) + if ( + not previous_complete + or not advisories.keys() <= existing.keys() + or not paths + or paths != package_paths(after_lock, package, source) + or before["vulnerabilities"][package].get("nodes") + != after["vulnerabilities"][package].get("nodes") + ): + break + before_nodes = _npm_vulnerable_nodes(before, before_lock, package) + after_nodes = _npm_vulnerable_nodes(after, after_lock, package) + if not before_nodes or not after_nodes: + break + for root in parent_roots: + before_reachable = _npm_reachable_nodes( + before_lock, root, package, before_nodes + ) + after_reachable = _npm_reachable_nodes( + after_lock, root, package, after_nodes + ) + if ( + not before_reachable + or not after_reachable + or before_reachable != after_reachable + ): + break + else: + continue + break + else: + parents[name] = sorted(sources) + return parents + + +def _advisory_link_items(data, name, source, fixed_ids): + if source == "npm": + sources, _ = npm_advisory_sources(data, name) + urls = {url for details in sources.values() for url in details} + else: + urls = { + identity + for identity, package, _ in audit_entries(data, source) + if package == name and identity in fixed_ids + } + links = [] + for url in sorted(urls): + # Keep registry-supplied URLs inside one Markdown table cell and link. + if not re.fullmatch(r"https?://[^\s<>()\[\]|`]+", url): + continue + label = url.rstrip("/").rsplit("/", 1)[-1] + if not re.fullmatch(r"GHSA-[a-z0-9-]+", label): + label = "Advisory" + links.append(f"[{label}]({url})") + return links + + +def advisory_links(data, name, source, fixed_ids): + links = _advisory_link_items(data, name, source, fixed_ids) + return ", ".join(links) or f"Advisory URL not supplied by {source}." + + +def compact_advisory_links(data, name, source, fixed_ids): + links = _advisory_link_items(data, name, source, fixed_ids) + if not links: + return f"Advisory URL not supplied by {source}." + additional = len(links) - 1 + if additional: + unit = "advisory" if additional == 1 else "advisories" + return f"{links[0]} (+{additional} additional {unit}; see full audit details)" + return links[0] + + def audit_fix_reason(detail, source): status = detail.get("fixStatus", "") if "Blocked by retained override" in status: @@ -2259,10 +2451,16 @@ def format_blocked_findings(data, source): def format_audit_summary( - before: dict | str, after: dict | str, source: str, *, fixed_ids=None + before: dict | str, + after: dict | str, + source: str, + *, + fixed_ids=None, + fixes_only=False, + reported_parents=None, ) -> str: if isinstance(after, str): - return f"⚪ **Security:** {after}" + return "" if fixes_only else f"⚪ **Security:** {after}" def complete(data, rows): return isinstance(data, dict) and ( @@ -2282,7 +2480,12 @@ def complete(data, rows): unconfirmed_ids = ( (old_ids - current_ids) - resolved_ids if fixed_ids is not None else set() ) - new_ids = newly_reported_findings(before, after, source) if comparable else set() + reported_parents = reported_parents or {} + new_ids = ( + newly_reported_findings(before, after, source) - reported_parents.keys() + if comparable + else set() + ) old_groups: dict = {} groups: dict = {} for rows, target in ((old, old_groups), (current, groups)): @@ -2295,6 +2498,8 @@ def complete(data, rows): for name in old_groups.keys() | groups.keys(): previous, pending = old_groups.get(name, {}), groups.get(name, {}) fixed = len(resolved_ids.intersection(previous)) + if fixes_only and not fixed: + continue unconfirmed_disappeared = set(previous).intersection(unconfirmed_ids) unconfirmed_details = [ previous[identity] for identity in unconfirmed_disappeared @@ -2399,6 +2604,14 @@ def complete(data, rows): if source == "npm" and name in old_groups else f"{new_count} new {unit}. " ) + action + if name in reported_parents: + source_packages = ", ".join( + f"`{package}`" for package in reported_parents[name] + ) + action = ( + f"Newly reported parent of an existing finding in {source_packages}; " + "affected versions and installed paths are unchanged. " + ) + action counts = [] if fixed: counts.append(f"{fixed} fixed") @@ -2429,13 +2642,20 @@ def complete(data, rows): rows.append( ( (rank, severity, name), - f"| {label}`{name}` | {' · '.join(counts)} | {action.strip()} |", + f"| {label}`{name}` | {' · '.join(counts)} | {action.strip()} |" + + ( + f" {compact_advisory_links(before, name, source, resolved_ids)} |" + if fixes_only + else "" + ), ) ) def packages(number): return f"{number} package" + ("" if number == 1 else "s") + if fixes_only and not fixed_packages: + return "" if not comparable: headline = "⚪ **Fixes unconfirmed**" elif fixed_packages: @@ -2461,12 +2681,92 @@ def packages(number): if rows: unit = "Advisories" if source == "pnpm" else "Affected package entries" headline += ( - f"\n\n| Package | {unit} | Action / blocker |\n| --- | --- | --- |\n" + f"\n\n| Package | {unit} | Action / blocker |" + + (" Advisories |" if fixes_only else "") + + "\n| --- | --- | --- |" + + (" --- |" if fixes_only else "") + + "\n" ) headline += "\n".join(row for _, row in sorted(rows)) return headline +def format_change_report( + proposed_changes: str, security_fixes: str, routine_upgrades: str = "" +) -> str: + """Keep confirmed fixes visible and the complete version list expandable.""" + return ( + (f"## Security fixes\n\n{security_fixes}\n\n" if security_fixes else "") + + ( + f"## Direct dependency upgrades\n\n{routine_upgrades}\n\n" + if routine_upgrades + else "" + ) + + "
\nAll dependency changes\n\n" + + f"{proposed_changes}\n\n
\n\n" + ) + + +def routine_upgrade_rows(initial, security, final, targets): + """List intentionally selected direct upgrades retained in the final proposal.""" + rows = [] + for section, name in sorted(targets, key=lambda item: (item[1], item[0])): + current = initial[2].get(section, {}).get(name) + baseline = security[2].get(section, {}).get(name) + proposed = final[2].get(section, {}).get(name) + if proposed is not None and proposed != baseline and proposed != current: + rows.append((section, name, current, proposed)) + return rows + + +def release_notes_link(directory, name, version, snapshot, notes, deadline): + fallback = f"[Package details](https://www.npmjs.com/package/{quote(name, safe='@/')}/v/{quote(version, safe='.')})" + if stable_version(version) is None or time.monotonic() >= deadline: + return fallback + result = run( + directory, + "view", + f"{name}@{version}", + "repository.url", + "--json", + deadline=min(deadline, time.monotonic() + 10), + ) + if restore_invalid_state( + directory, snapshot, notes, f"Release notes lookup for {name}", frozen=True + ): + return fallback + try: + repository = json.loads(result.stdout) + except ValueError: + return fallback + if result.returncode or not isinstance(repository, str): + return fallback + match = re.fullmatch( + r"(?:git\+)?(?:https?|git)://github\.com/([A-Za-z0-9_.-]+)/([A-Za-z0-9_.-]+?)(?:\.git)?/?", + repository, + ) + if match and all(part not in {".", ".."} for part in match.groups()): + return f"[Releases](https://github.com/{match[1]}/{match[2]}/releases)" + return fallback + + +def format_routine_upgrades(rows, links): + if not rows: + return "" + labels = { + "dependencies": "", + "devDependencies": " (dev)", + "optionalDependencies": " (optional)", + } + return ( + "| Package | Current | Proposed | Release notes |\n| --- | --- | --- | --- |\n" + + "\n".join( + f"| `{name}`{labels[section]} | `{current}` | `{proposed}` | {links[(name, proposed)]} |" + for section, name, current, proposed in rows + ) + ) + + def within_upgrade_scope(before, after, scope): return after >= before and ( scope == "major" @@ -2683,6 +2983,7 @@ def prepare( set(before.get("vulnerabilities", {})) if isinstance(before, dict) else set() ) changes = [] + routine_targets = set() cache: dict = {} # Give compatible security fixes the first use of the shared time budget. @@ -2836,6 +3137,7 @@ def prepare( if tuple(map(int, latest.split("."))) > floor: manifest[section][name] = latest changes.append((name, f"- {name}: `{current}` → `{latest}`")) + routine_targets.add((section, name)) if changes: manifest_path.write_text(json.dumps(manifest, indent=2) + "\n") @@ -2877,6 +3179,7 @@ def prepare( "the manifest and lockfile from the initial security fix attempt." ) changes = [] + routine_targets.clear() else: stable_snapshot = valid_snapshot(directory) or security_snapshot last_valid_snapshot = stable_snapshot @@ -3080,6 +3383,15 @@ def published_patch_versions(specifier, *, all_versions=False): validation["passed"] = check_status == "Passed." validation["audit_errors"] = audit_errors validation["local_patches"] = patch_report + routine_rows = routine_upgrade_rows( + initial_snapshot, security_snapshot, after_snapshot, routine_targets + ) + release_links = {} + for _, name, _, proposed in routine_rows: + if (name, proposed) not in release_links: + release_links[(name, proposed)] = release_notes_link( + directory, name, proposed, after_snapshot, notes, deadline + ) fingerprints = "\n".join( f"- `{name}`: `{hashlib.sha256(content).hexdigest()}`" for name, content in { @@ -3190,7 +3502,13 @@ def published_patch_versions(specifier, *, all_versions=False): fixed_count = len(fixed_ids) initial_ids = finding_ids(before, package_manager) remaining_ids = finding_ids(after, package_manager) - new_ids = newly_reported_findings(before, after, package_manager) + reported_parents = reported_parent_findings( + before, after, package_manager, before_lock, after_lock + ) + new_ids = ( + newly_reported_findings(before, after, package_manager) + - reported_parents.keys() + ) remaining_count = len(remaining_ids or ()) remaining_unit = ( ("advisory" if remaining_count == 1 else "advisories") @@ -3260,8 +3578,6 @@ def published_patch_versions(specifier, *, all_versions=False): else "" ) + ( - f"## Proposed dependency changes\n\n" - f"{proposed_changes}\n\n" f"## Validation\n\n" f"- Frozen install: {install_status[0].lower() + install_status[1:]}\n" f"- Project checks: {check_status[0].lower() + check_status[1:]}\n\n" @@ -3288,15 +3604,27 @@ def published_patch_versions(specifier, *, all_versions=False): ) ) report += ( - f"🔴 **New security findings in {new_count} {unit}; review before merging.**\n\n" + f"🔴 **Newly reported security findings in {new_count} {unit}; review the advisory details before merging.**\n\n" if package_manager == "npm" else f"🔴 **{new_count} newly reported {unit}; review before merging.**\n\n" ) if not initial_audit_available: report += "Initial audit evidence was unavailable; retained security-fix changes are unconfirmed.\n\n" + report += format_change_report( + proposed_changes, + format_audit_summary( + before, + after, + package_manager, + fixed_ids=fixed_ids, + fixes_only=True, + reported_parents=reported_parents, + ), + format_routine_upgrades(routine_rows, release_links), + ) report += ( f"
\n{'Blocked findings and constraints' if blocked_findings else 'Security findings and blockers'}\n\n" - f"{blocked_findings or format_audit_summary(before, after, package_manager, fixed_ids=fixed_ids)}\n\n" + f"{blocked_findings or format_audit_summary(before, after, package_manager, fixed_ids=fixed_ids, reported_parents=reported_parents)}\n\n" "
\n\n" ) if change_sections: diff --git a/node/tests/test_dependency_audit.py b/node/tests/test_dependency_audit.py index d849742..90e3eba 100644 --- a/node/tests/test_dependency_audit.py +++ b/node/tests/test_dependency_audit.py @@ -306,9 +306,11 @@ def upgrade(command, **kwargs): monkeypatch.setattr(subprocess, "run", upgrade) report = prepare(npm_project) - visible = report.split("
", 1)[0] - assert visible.count("- renderer:") == 1 - assert "- renderer: `1.0.0` → `2.0.0`" in visible + visible, changes = report.split("
", 1) + changes = changes.split("
", 1)[0] + assert "- renderer:" not in visible + assert changes.count("- renderer:") == 1 + assert "- renderer: `1.0.0` → `2.0.0`" in changes details = report.split("## Security findings and fixes", 1)[1].split( "
", 1 )[0] diff --git a/node/tests/test_maintenance_options.py b/node/tests/test_maintenance_options.py index 2a3270e..361dc35 100644 --- a/node/tests/test_maintenance_options.py +++ b/node/tests/test_maintenance_options.py @@ -352,6 +352,173 @@ def test_clear_headline_reports_optional_updates_and_unfixed_security( assert report.startswith("**1 routine dependency update · Validation passed**") +def test_routine_report_shows_selected_direct_upgrades_and_release_notes( + site, monkeypatch +): + commands = fake_commands(monkeypatch) + invoke = helper["prepare"].__globals__["run"] + + def metadata(directory, *args, **kwargs): + if args[:3] == ("view", "renderer@2.0.0", "repository.url"): + commands.append(args) + return subprocess.CompletedProcess( + args, 0, '"git+https://github.com/example/renderer.git"', "" + ) + return invoke(directory, *args, **kwargs) + + monkeypatch.setitem(helper["prepare"].__globals__, "run", metadata) + report = helper["prepare"](site) + visible, collapsed = report.split("
", 1) + assert visible.index("## Validation") < visible.index( + "## Direct dependency upgrades" + ) + assert ( + "| `renderer` | `^1.2.3` | `2.0.0` | [Releases](https://github.com/example/renderer/releases) |" + in visible + ) + assert "All dependency changes" in collapsed + assert ( + sum( + args[:3] == ("view", "renderer@2.0.0", "repository.url") + for args in commands + ) + == 1 + ) + + +def test_security_only_report_has_no_routine_table_or_release_lookup(site, monkeypatch): + commands = fake_commands(monkeypatch) + report = helper["prepare"](site, maintenance_mode="security-only") + assert "## Direct dependency upgrades" not in report + assert not any("repository.url" in args for args in commands) + + +def test_failed_routine_resolution_has_no_direct_upgrade_table(site, monkeypatch): + fake_commands(monkeypatch) + invoke = helper["prepare"].__globals__["run"] + + def fail(directory, *args, **kwargs): + result = invoke(directory, *args, **kwargs) + if args[0] == "install" and "--lockfile-only" in args: + result.returncode = 1 + return result + + monkeypatch.setitem(helper["prepare"].__globals__, "run", fail) + report = helper["prepare"](site) + assert "## Direct dependency upgrades" not in report + assert "Stable upgrade resolution failed" in report + + +@pytest.mark.parametrize( + "repository", + [ + '"git+https://github.com/example/project.git"', + '"https://github.com/example/project"', + '"https://gitlab.com/example/project.git"', + "null", + "not-json", + ], +) +def test_release_note_lookup_uses_metadata_or_explicit_package_fallback( + site, monkeypatch, repository +): + snapshot = helper["valid_snapshot"](site) + monkeypatch.setitem( + helper["release_notes_link"].__globals__, + "run", + lambda *args, **kwargs: subprocess.CompletedProcess([], 0, repository, ""), + ) + link = helper["release_notes_link"]( + site, "@scope/renderer", "2.0.0", snapshot, [], float("inf") + ) + if "github.com" in repository: + assert link == "[Releases](https://github.com/example/project/releases)" + else: + assert ( + link + == "[Package details](https://www.npmjs.com/package/@scope/renderer/v/2.0.0)" + ) + + +def test_direct_upgrade_rows_exclude_transitive_security_and_rolled_back_changes(): + initial = ( + b"", + {}, + { + "dependencies": { + "routine": "1.0.0", + "security": "1.0.0", + "unchanged": "1.0.0", + }, + "devDependencies": {"dev": "1.0.0"}, + }, + {}, + ) + security = ( + b"", + {}, + { + "dependencies": { + "routine": "1.0.0", + "security": "1.1.0", + "unchanged": "1.0.0", + }, + "devDependencies": {"dev": "1.0.0"}, + }, + {}, + ) + final = ( + b"", + {}, + { + "dependencies": { + "routine": "2.0.0", + "security": "1.1.0", + "unchanged": "1.0.0", + }, + "devDependencies": {"dev": "2.0.0"}, + }, + {}, + ) + targets = { + ("dependencies", "routine"), + ("dependencies", "security"), + ("dependencies", "unchanged"), + ("devDependencies", "dev"), + } + rows = helper["routine_upgrade_rows"](initial, security, final, targets) + assert rows == [ + ("devDependencies", "dev", "1.0.0", "2.0.0"), + ("dependencies", "routine", "1.0.0", "2.0.0"), + ] + assert helper["routine_upgrade_rows"](initial, security, security, targets) == [] + assert helper["routine_upgrade_rows"](initial, security, final, set()) == [] + table = helper["format_routine_upgrades"]( + rows, {("dev", "2.0.0"): "details", ("routine", "2.0.0"): "details"} + ) + assert "`dev` (dev)" in table + assert "security" not in table and "unchanged" not in table + + +def test_release_notes_metadata_cannot_change_the_retained_proposal(site, monkeypatch): + snapshot = helper["valid_snapshot"](site) + notes = [] + + def mutate(directory, *args, **kwargs): + (directory / "package.json").write_text('{"scripts":{"build":"changed"}}') + return subprocess.CompletedProcess( + [], 0, '"https://github.com/example/project"', "" + ) + + monkeypatch.setitem(helper["release_notes_link"].__globals__, "run", mutate) + link = helper["release_notes_link"]( + site, "renderer", "2.0.0", snapshot, notes, float("inf") + ) + assert helper["valid_snapshot"](site) == snapshot + assert "Package details" in link and "Releases" not in link + assert any("Release notes lookup" in note for note in notes) + + def test_registry_finding_removal_does_not_turn_unrelated_churn_into_a_security_fix( site, monkeypatch ): @@ -2017,14 +2184,12 @@ def audit(*args, **kwargs): report = helper["prepare"](site) assert report.startswith("**1 routine dependency update · Validation passed**") visible, details = report.split("
", 1) - assert ( - visible.index("## Proposed dependency changes") - < visible.index("- renderer:") - < visible.index("## Validation") - < visible.index( - "## Existing security findings" if blocked else "## Security status" - ) + assert visible.index("## Validation") < visible.index( + "## Existing security findings" if blocked else "## Security status" ) + assert "- renderer:" not in visible + assert details.startswith("\nAll dependency changes") + assert "- renderer:" in details assert "- Frozen install: passed." in visible assert "- Project checks: passed." in visible assert "**Maintenance:**" not in visible diff --git a/node/tests/test_node_standard.py b/node/tests/test_node_standard.py index d4e453a..fb4b881 100644 --- a/node/tests/test_node_standard.py +++ b/node/tests/test_node_standard.py @@ -862,14 +862,13 @@ def test_summary_precedes_collapsed_full_audit(npm_project, monkeypatch): assert "**Maintenance:** 0 additional package updates (0 total)." in details assert "0 security fixes · 0 affected package entries remain" in summary assert "🟢 **No security findings**" in details - assert "| Package |" not in summary + assert "| Package | Current | Proposed | Release notes |" in summary + assert "## Security fixes" not in summary assert "- Project checks: passed." in summary - assert "renderer: `1.0.0` → `2.0.0`" in summary - assert ( - summary.index("## Proposed dependency changes") - < summary.index("## Validation") - < summary.index("## Security status") - ) + assert "renderer: `1.0.0` → `2.0.0`" not in summary + assert "renderer: `1.0.0` → `2.0.0`" in details + assert details.startswith("\nAll dependency changes") + assert summary.index("## Validation") < summary.index("## Security status") assert "Validation script:" not in summary assert "Run configuration and audit evidence" in details assert "No upgrades to affected dependencies" not in report @@ -992,7 +991,7 @@ def run(command, **kwargs): assert "routine dependency" not in visible assert "0 security fixes · 1 affected package entry remains" in visible assert ( - "🔴 **New security findings in 1 affected package entry; review before merging.**" + "🔴 **Newly reported security findings in 1 affected package entry; review the advisory details before merging.**" in visible ) assert "unchanged by this PR" not in visible @@ -2093,6 +2092,52 @@ def test_fully_resolved_findings_still_produce_one_table(security_findings): assert "remaining" not in result +def test_visible_security_fixes_exclude_unrelated_and_unconfirmed_findings( + security_findings, +): + before = security_findings( + [ + ("leaf", "fixed", "high", ""), + ("leaf", "pending", "high", "No patched version declared by the registry."), + ("withdrawn", "unconfirmed", "high", ""), + ( + "unrelated", + "blocked", + "high", + "No patched version declared by the registry.", + ), + ] + ) + after = security_findings( + [ + ("leaf", "pending", "high", "No patched version declared by the registry."), + ( + "unrelated", + "blocked", + "high", + "No patched version declared by the registry.", + ), + ] + ) + fixed_ids = {"https://github.com/advisories/fixed"} + table = audit_module["format_audit_summary"]( + before, after, "pnpm", fixed_ids=fixed_ids, fixes_only=True + ) + assert "| `leaf` | 1 fixed · 1 pending |" in table + assert "The registry supplies no patched version." in table + assert "unrelated" not in table and "withdrawn" not in table + report = audit_module["format_change_report"]("- leaf: `1.0.0` → `1.1.0`", table) + visible, collapsed = report.split("
", 1) + assert table in visible + assert "- leaf:" not in visible and "- leaf:" in collapsed + assert ( + audit_module["format_audit_summary"]( + before, after, "pnpm", fixed_ids=set(), fixes_only=True + ) + == "" + ) + + def test_new_advisories_for_existing_package_stay_visible(security_findings): old = [(f"package-{i}", str(i), "high", "") for i in range(7)] result = audit_module["format_audit_summary"]( @@ -2104,6 +2149,399 @@ def test_new_advisories_for_existing_package_stay_visible(security_findings): assert all(result.count(f"`package-{i}`") == 1 for i in range(7)) +@pytest.fixture +def propagated_npm_finding(): + advisory = { + "url": "https://github.com/advisories/GHSA-test-leaf", + "title": "Leaf advisory", + "severity": "low", + "range": "<2.0.0", + } + before = { + "metadata": {"vulnerabilities": {"low": 1}}, + "vulnerabilities": { + "leaf": { + "via": [advisory], + "severity": "low", + "nodes": ["node_modules/leaf"], + } + }, + } + after = json.loads(json.dumps(before)) + after["vulnerabilities"]["parent"] = { + "via": ["middle"], + "severity": "low", + "fixAvailable": False, + } + # The intermediate finding is already present; npm newly marks its parent. + for data in (before, after): + data["vulnerabilities"]["middle"] = { + "via": ["leaf"], + "severity": "low", + "nodes": ["node_modules/middle"], + } + lock = { + "packages": { + "node_modules/parent": { + "version": "1.0.0", + "dependencies": {"middle": "1.0.0"}, + }, + "node_modules/middle": { + "version": "1.0.0", + "dependencies": {"leaf": "1.0.0"}, + }, + "node_modules/leaf": {"version": "1.0.0"}, + } + } + return before, after, lock + + +@pytest.mark.parametrize( + "change", + [ + "none", + "new-advisory", + "new-root-path", + "root-version", + "parent-version", + "missing-evidence", + "missing-parent", + "direct-advisory", + "parent-edge", + "middle-edge", + "middle-version", + "new-via", + ], +) +def test_new_parent_is_background_only_with_unchanged_advisory_and_paths( + propagated_npm_finding, change +): + before, after, before_lock = propagated_npm_finding + after_lock = json.loads(json.dumps(before_lock)) + if change == "new-advisory": + after["vulnerabilities"]["leaf"]["via"][0]["url"] += "-new" + elif change == "new-root-path": + after_lock["packages"]["node_modules/parent/node_modules/leaf"] = { + "version": "1.0.0" + } + elif change == "root-version": + after_lock["packages"]["node_modules/leaf"]["version"] = "1.1.0" + elif change == "parent-version": + after_lock["packages"]["node_modules/parent"]["version"] = "1.1.0" + elif change == "missing-evidence": + after["vulnerabilities"]["parent"]["via"].append("unknown") + elif change == "missing-parent": + del before_lock["packages"]["node_modules/parent"] + elif change == "direct-advisory": + after["vulnerabilities"]["parent"]["via"].append( + dict(after["vulnerabilities"]["leaf"]["via"][0]) + ) + elif change == "parent-edge": + after_lock["packages"]["node_modules/parent"]["dependencies"] = { + "middle": "1.0.0", + "other": "1.0.0", + } + elif change == "middle-edge": + after_lock["packages"]["node_modules/middle"]["dependencies"] = {} + elif change == "middle-version": + after_lock["packages"]["node_modules/middle"]["version"] = "1.1.0" + elif change == "new-via": + del before["vulnerabilities"]["middle"] + parents = audit_module["reported_parent_findings"]( + before, after, "npm", before_lock, after_lock + ) + expected = {"parent": ["leaf"]} if change == "none" else {} + if change == "middle-version": + expected = {"parent": ["leaf"]} + if change == "new-via": + expected = {"middle": ["leaf"]} + assert parents == expected + report = audit_module["format_audit_summary"]( + before, after, "npm", fixed_ids=set(), reported_parents=parents + ) + if change in ("none", "middle-version"): + assert "🔴" not in report + assert "Newly reported parent of an existing finding in `leaf`" in report + assert "affected versions and installed paths are unchanged" in report + else: + assert "New findings" in report + + +def test_security_fix_links_follow_indirect_advisories(propagated_npm_finding): + _, before, _ = propagated_npm_finding + after = {"metadata": {"vulnerabilities": {}}, "vulnerabilities": {}} + table = audit_module["format_audit_summary"]( + before, after, "npm", fixed_ids={"parent"}, fixes_only=True + ) + assert ( + "| 🟢\u00a0`parent` | 1 fixed | Resolved. | [GHSA-test-leaf](https://github.com/advisories/GHSA-test-leaf) |" + in table + ) + # A cycle must terminate and retain any reachable direct advisory links. + before["vulnerabilities"]["leaf"]["via"].append("parent") + assert ( + audit_module["advisory_links"](before, "parent", "npm", set()).count( + "https://github.com/advisories/GHSA-test-leaf" + ) + == 1 + ) + + +def test_new_parent_warning_survives_new_intermediate_reachability( + propagated_npm_finding, +): + before, after, before_lock = propagated_npm_finding + before_lock["packages"]["node_modules/middle"]["dependencies"] = {} + before_lock["packages"]["node_modules/elsewhere/node_modules/middle"] = { + "version": "1.1.0", + "dependencies": {"leaf": "1.0.0"}, + } + before["vulnerabilities"]["middle"]["nodes"] = [ + "node_modules/elsewhere/node_modules/middle" + ] + after_lock = json.loads(json.dumps(before_lock)) + after_lock["packages"]["node_modules/middle"]["version"] = "1.1.0" + after_lock["packages"]["node_modules/middle"]["dependencies"] = {"leaf": "1.0.0"} + after["vulnerabilities"]["middle"]["nodes"] = ["node_modules/middle"] + + parents = audit_module["reported_parent_findings"]( + before, after, "npm", before_lock, after_lock + ) + assert parents == {} + report = audit_module["format_audit_summary"]( + before, after, "npm", fixed_ids=set(), reported_parents=parents + ) + assert "New findings in 1 package" in report + assert "`parent`" in report + + +def test_new_parent_warning_survives_new_route_in_one_parent_copy(): + advisory = { + "url": "https://github.com/advisories/GHSA-probe-leaf", + "title": "Existing leaf advisory", + "severity": "high", + "range": "<2.0.0", + } + before = { + "metadata": {"vulnerabilities": {"high": 2, "total": 2}}, + "vulnerabilities": { + "leaf": { + "fixAvailable": False, + "nodes": ["node_modules/leaf"], + "range": "*", + "severity": "high", + "via": [advisory], + }, + "middle": { + "fixAvailable": False, + "nodes": ["node_modules/a/node_modules/middle"], + "range": "*", + "severity": "high", + "via": ["leaf"], + }, + }, + } + after = json.loads(json.dumps(before)) + after["metadata"]["vulnerabilities"] = {"high": 3, "total": 3} + after["vulnerabilities"]["middle"]["nodes"] = [ + "node_modules/a/node_modules/middle", + "node_modules/b/node_modules/middle", + ] + after["vulnerabilities"]["parent"] = { + "fixAvailable": False, + "nodes": [ + "node_modules/a/node_modules/parent", + "node_modules/b/node_modules/parent", + ], + "range": "*", + "severity": "high", + "via": ["middle"], + } + before_lock = { + "packages": { + "node_modules/a/node_modules/middle": { + "dependencies": {"leaf": "*"}, + "version": "1.0.0", + }, + "node_modules/a/node_modules/parent": { + "dependencies": {"middle": "*"}, + "version": "1.0.0", + }, + "node_modules/b/node_modules/middle": { + "dependencies": {}, + "version": "1.0.0", + }, + "node_modules/b/node_modules/parent": { + "dependencies": {"middle": "*"}, + "version": "1.0.0", + }, + "node_modules/leaf": {"version": "1.0.0"}, + } + } + after_lock = json.loads(json.dumps(before_lock)) + after_lock["packages"]["node_modules/b/node_modules/middle"] = { + "dependencies": {"leaf": "*"}, + "version": "1.1.0", + } + + parents = audit_module["reported_parent_findings"]( + before, after, "npm", before_lock, after_lock + ) + assert parents == {} + assert audit_module["newly_reported_findings"]( + before, after, "npm" + ) - parents.keys() == {"parent"} + report = audit_module["format_audit_summary"]( + before, after, "npm", fixed_ids=set(), reported_parents=parents + ) + assert "New findings in 1 package" in report + assert "Newly reported parent of an existing finding" not in report + assert "affected versions and installed paths are unchanged" not in report + + +def test_new_parent_warning_survives_unproved_empty_reachability( + propagated_npm_finding, +): + before, _, before_lock = propagated_npm_finding + before_lock["packages"]["node_modules/middle"]["dependencies"] = {} + after = json.loads(json.dumps(before)) + after["metadata"]["vulnerabilities"] = {"low": 3, "total": 3} + after["vulnerabilities"]["parent"] = { + "fixAvailable": False, + "nodes": ["node_modules/parent"], + "range": "*", + "severity": "low", + "via": ["middle"], + } + after_lock = json.loads(json.dumps(before_lock)) + + parents = audit_module["reported_parent_findings"]( + before, after, "npm", before_lock, after_lock + ) + assert parents == {} + assert audit_module["newly_reported_findings"]( + before, after, "npm" + ) - parents.keys() == {"parent"} + report = audit_module["format_audit_summary"]( + before, after, "npm", fixed_ids=set(), reported_parents=parents + ) + assert "New findings in 1 package" in report + assert "Newly reported parent of an existing finding" not in report + + +def test_new_parent_background_allows_intermediate_version_change_with_same_path( + propagated_npm_finding, +): + before, after, before_lock = propagated_npm_finding + after_lock = json.loads(json.dumps(before_lock)) + after_lock["packages"]["node_modules/middle"]["version"] = "1.1.0" + parents = audit_module["reported_parent_findings"]( + before, after, "npm", before_lock, after_lock + ) + assert parents == {"parent": ["leaf"]} + + +def test_new_parent_background_follows_peer_intermediate_version_change(): + advisory = { + "url": "https://github.com/advisories/GHSA-test-katex", + "title": "KaTeX advisory", + "severity": "low", + "range": "<2.0.0", + } + before = { + "metadata": {"vulnerabilities": {"low": 1}}, + "vulnerabilities": { + "katex": { + "via": [advisory], + "severity": "low", + "nodes": ["node_modules/katex"], + }, + "mermaid": { + "via": ["katex"], + "severity": "low", + "nodes": ["node_modules/mermaid"], + }, + }, + } + after = json.loads(json.dumps(before)) + after["vulnerabilities"]["layout-elk"] = { + "via": ["mermaid"], + "severity": "low", + } + before_lock = { + "packages": { + "node_modules/layout-elk": { + "version": "1.0.0", + "peerDependencies": {"mermaid": "^1.0.0"}, + }, + "node_modules/mermaid": { + "version": "1.0.0", + "dependencies": {"katex": "1.0.0"}, + }, + "node_modules/katex": {"version": "1.0.0"}, + } + } + after_lock = json.loads(json.dumps(before_lock)) + after_lock["packages"]["node_modules/mermaid"]["version"] = "1.1.0" + parents = audit_module["reported_parent_findings"]( + before, after, "npm", before_lock, after_lock + ) + assert parents == {"layout-elk": ["katex"]} + + +def test_security_fix_links_only_include_confirmed_pnpm_advisories(security_findings): + before = security_findings( + [("leaf", "fixed", "high", ""), ("leaf", "pending", "high", "")] + ) + after = security_findings([("leaf", "pending", "high", "")]) + table = audit_module["format_audit_summary"]( + before, + after, + "pnpm", + fixed_ids={"https://github.com/advisories/fixed"}, + fixes_only=True, + ) + assert "[Advisory](https://github.com/advisories/fixed)" in table + assert "https://github.com/advisories/pending" not in table + assert "1 fixed · 1 pending" in table + + +def test_visible_security_fix_uses_one_primary_link_and_counts_additional_advisories(): + urls = [ + f"https://github.com/advisories/GHSA-test-{name}" + for name in ("one", "two", "three") + ] + before = { + "metadata": {"vulnerabilities": {"high": 3, "total": 3}}, + "package_manager": "pnpm", + "vulnerabilities": { + "leaf": { + "severity": "high", + "range": "<2.0.0", + "fixAvailable": True, + "via": [ + { + "title": f"Advisory {index}", + "url": url, + "severity": "high", + "range": "<2.0.0", + } + for index, url in enumerate(urls, start=1) + ], + } + }, + } + after = {"metadata": {"vulnerabilities": {}}, "vulnerabilities": {}} + table = audit_module["format_audit_summary"]( + before, after, "pnpm", fixed_ids=set(urls), fixes_only=True + ) + assert urls[0] in table + assert urls[1] not in table and urls[2] not in table + assert "+2 additional advisories; see full audit details" in table + full_audit = audit_module["format_audit"](before, {"packages": {}}) + assert all(url in full_audit for url in urls) + + def test_npm_outcomes_keep_package_entry_semantics(): before = { "metadata": {"vulnerabilities": {"high": 1}}, @@ -2245,8 +2683,8 @@ def update(command, **kwargs): else: assert "**Maintenance:** 1 additional package update (2 total)." in details assert "1 security fix" in visible - assert "| 🟢\u00a0`security` |" not in visible - assert "| 🟢\u00a0`security` |" in report.split("
", 1)[1] + assert "## Security fixes" in visible + assert "| 🟢\u00a0`security` | 1 fixed | Resolved. |" in visible def test_partial_package_is_orange_only_when_pending_work_needs_user_decision(