diff --git a/src/icebergsca/core/triage.py b/src/icebergsca/core/triage.py index 7764a15..01d9d58 100644 --- a/src/icebergsca/core/triage.py +++ b/src/icebergsca/core/triage.py @@ -27,7 +27,7 @@ import tomllib from dataclasses import dataclass, replace -from datetime import date, timedelta +from datetime import date, datetime, timedelta from typing import Any from icebergsca.core.errors import ConfigError @@ -204,6 +204,11 @@ def _expires(path: str, where: str, value: Any, today: date) -> date: """ if value is None: return today + timedelta(days=DEFAULT_EXPIRY_DAYS) + if isinstance(value, datetime): + # TOML also has datetime types, and ``datetime`` *is a* ``date`` — but one + # that ``today > expires`` cannot compare against, which would turn a legal + # ``expires = 2026-10-27 00:00:00`` into a crash at matching time. + return value.date() if isinstance(value, date): return value if isinstance(value, str): diff --git a/src/icebergsca/core/versions.py b/src/icebergsca/core/versions.py index bbf3649..1dd1cc3 100644 --- a/src/icebergsca/core/versions.py +++ b/src/icebergsca/core/versions.py @@ -30,6 +30,16 @@ #: an inclusive upper bound of ``2.0`` would exclude ``2.0.0``. _RELEASE_WIDTH = 4 +#: PEP 440 pre-release letters, already normalised by ``packaging`` (``alpha`` → ``a``). +_PRE_RANK = {"a": 0, "b": 1, "rc": 2} +#: Rank for a release with no pre-release segment — above every pre-release. +_FINAL_RANK = 3 +#: Rank for a bare dev release (``1.0.dev1``), which PEP 440 sorts below any alpha. +_DEV_RANK = -1 + +#: The first element carries the epoch (always 0 outside PyPI) followed by the padded +#: release; the second is 0 for a pre-release and 1 otherwise; the third orders the +#: pre/post/dev suffix. VersionKey = tuple[tuple[int, ...], int, tuple[tuple[int, str], ...]] @@ -54,14 +64,40 @@ def parse(ecosystem: EcosystemId, version: str) -> VersionKey | None: except InvalidVersion: return _loose(version) return ( - _pad(parsed.release), + (parsed.epoch, *_pad(parsed.release)), 0 if parsed.is_prerelease else 1, - ((0, str(parsed.pre or parsed.dev or "")),), + _pep440_suffix(parsed), ) return _loose(version) +def _pep440_suffix(parsed: Version) -> tuple[tuple[int, str], ...]: + """Order the pre/post/dev segments the way PEP 440 does. + + dev < a < b < rc < final < post, with the numeric parts compared as numbers — a + stringified ``parsed.pre`` would place ``rc10`` below ``rc2``. Elements are + ``(int, str)`` pairs so the shape stays comparable with what :func:`_loose` + yields when an unparseable version in the same package fell back to it. + """ + if parsed.pre is not None: + letter, number = parsed.pre + pre_rank, pre_number = _PRE_RANK.get(letter, _FINAL_RANK - 1), number + elif parsed.dev is not None and parsed.post is None: + pre_rank, pre_number = _DEV_RANK, 0 + else: + pre_rank, pre_number = _FINAL_RANK, 0 + return ( + (pre_rank, ""), + (pre_number, ""), + # An absent post-release sorts below ``.post0``, so ``1.0 < 1.0.post0``. + (-1 if parsed.post is None else parsed.post, ""), + # A dev marker sorts below its own release: ``1.0a1.dev1 < 1.0a1``. + (1 if parsed.dev is None else 0, ""), + (parsed.dev or 0, ""), + ) + + def _loose(version: str) -> VersionKey | None: """Generic dotted ordering with pre-release handling. @@ -85,7 +121,9 @@ def _loose(version: str) -> VersionKey | None: ) if not release: return None - release = _pad(release) + # The leading 0 is the epoch slot, so a loose key stays comparable with a PEP 440 + # one when both appear for the same PyPI package. + release = (0, *_pad(release)) prerelease: tuple[tuple[int, str], ...] = tuple( (int(segment), "") if segment.isdigit() else (_ALPHA_RANK, segment.lower()) diff --git a/src/icebergsca/ecosystems/dotnet.py b/src/icebergsca/ecosystems/dotnet.py index faeabef..6f1b603 100644 --- a/src/icebergsca/ecosystems/dotnet.py +++ b/src/icebergsca/ecosystems/dotnet.py @@ -30,7 +30,9 @@ from icebergsca.ecosystems.base import EcosystemSpec, FileSpec, build_dependencies #: NuGet version ranges use interval notation: ``[1.0,2.0)``, ``[1.0]``, ``(1.0,)``. -#: A bare version means "this or newer", so only bracketed equality is a true pin. +#: A bare version formally means "this or newer", but NuGet restores the *lowest* +#: version that satisfies a range, which is the declared version itself whenever it +#: exists — so a bare version is treated as a pin, and bracketed forms as ranges. _RANGE_CHARS = "[]()" #: ``PackageReference`` items whose asset flags exclude them from the compiled diff --git a/src/icebergsca/ecosystems/maven/resolver.py b/src/icebergsca/ecosystems/maven/resolver.py index 6eea7c1..43ca74f 100644 --- a/src/icebergsca/ecosystems/maven/resolver.py +++ b/src/icebergsca/ecosystems/maven/resolver.py @@ -473,8 +473,16 @@ def _children( # -- effective POMs ---------------------------------------------------- - async def _effective(self, coordinate: Coordinate) -> EffectivePom | None: - """Merge a POM with its parent chain and any BOMs it imports.""" + async def _effective( + self, coordinate: Coordinate, visiting: frozenset[str] = frozenset() + ) -> EffectivePom | None: + """Merge a POM with its parent chain and any BOMs it imports. + + ``visiting`` carries the BOM-import path that led here, so a cycle of + ``import``-scoped BOMs is cut rather than recursed into. Nothing on Central + should contain one, but "should" is not a stack-depth guarantee, and the + backfill path has no ``gather`` around it to absorb a RecursionError. + """ pom = await self._fetch_pom(coordinate) if pom is None: return None @@ -506,7 +514,7 @@ async def _effective(self, coordinate: Coordinate) -> EffectivePom | None: for entry in reversed(chain): await self._apply_management( - entry, properties, managed, managed_scopes, managed_exclusions + entry, properties, managed, managed_scopes, managed_exclusions, visiting ) return EffectivePom( @@ -525,6 +533,7 @@ async def _apply_management( managed: dict[str, str], managed_scopes: dict[str, str], managed_exclusions: dict[str, frozenset[str]], + visiting: frozenset[str] = frozenset(), ) -> None: """Fold one POM's dependencyManagement in, expanding imported BOMs first. @@ -537,7 +546,14 @@ async def _apply_management( version = interpolate(entry.version, properties) if entry.is_import and version and not has_unresolved_property(version): - bom = await self._effective(Coordinate(group, artifact, version)) + coordinate = Coordinate(group, artifact, version) + if str(coordinate) in visiting: + # A BOM that imports itself, however indirectly. Everything it + # manages was already folded in on the way down; recursing again + # would never terminate. + logger.debug("BOM import cycle at %s; not descending", coordinate) + continue + bom = await self._effective(coordinate, visiting | {str(coordinate)}) if bom is not None: properties.update( {k: v for k, v in bom.properties.items() if k not in properties} diff --git a/src/icebergsca/ecosystems/npm.py b/src/icebergsca/ecosystems/npm.py index 5d3e2ab..0515bbe 100644 --- a/src/icebergsca/ecosystems/npm.py +++ b/src/icebergsca/ecosystems/npm.py @@ -167,9 +167,13 @@ def _find_line(content: str, name: str) -> int | None: def _entry_scope(entry: dict[str, Any]) -> Scope | None: """Read npm's precomputed scope flags, or ``None`` if it recorded none.""" - if entry.get("dev") or entry.get("devOptional"): + if entry.get("dev"): return Scope.DEV - if entry.get("optional"): + if entry.get("optional") or entry.get("devOptional"): + # ``devOptional`` marks a package that is in the dev tree *and* an optional + # dependency of something that ships, so a production install still gets it. + # Optional is scanned by default and dev is not — calling it dev would hide + # a shipping package, which is the one direction this tool must not fail in. return Scope.OPTIONAL return None @@ -500,7 +504,10 @@ def _link_yarn_children( _YARN_HEADER = re.compile(r"^(?P\S.*?):\s*$") -_YARN_FIELD = re.compile(r'^\s+(?P[\w-]+)\s+"?(?P[^"]*)"?\s*$') +#: A field or dependency line. The key may be quoted — yarn writes scoped packages +#: as ``"@babel/core" "^7.0.0"`` — and a key pattern that cannot match the quotes +#: silently drops every ``@scope/`` edge in the file. +_YARN_FIELD = re.compile(r'^\s+"?(?P[^\s"]+)"?\s+"?(?P[^"]*)"?\s*$') def _parse_yarn_v1(path: Path, content: str) -> list[Dependency]: @@ -544,9 +551,7 @@ def _parse_yarn_v1(path: Path, content: str) -> list[Dependency]: if field is None: continue if section in ("dependencies", "optionalDependencies"): - current["dependencies"][field.group("key").strip('"')] = field.group( - "value" - ) + current["dependencies"][field.group("key")] = field.group("value") elif field.group("key") == "version": current["version"] = field.group("value") section = None diff --git a/src/icebergsca/ecosystems/python.py b/src/icebergsca/ecosystems/python.py index c586c01..a92613e 100644 --- a/src/icebergsca/ecosystems/python.py +++ b/src/icebergsca/ecosystems/python.py @@ -15,7 +15,7 @@ from typing import Any from packaging.requirements import InvalidRequirement, Requirement -from packaging.specifiers import SpecifierSet +from packaging.specifiers import InvalidSpecifier, SpecifierSet from icebergsca.core.errors import ParseError from icebergsca.core.graph import Node, most_included, resolve @@ -82,6 +82,22 @@ def _pin_from_specifier(spec: SpecifierSet) -> tuple[str | None, Pin]: return None, Pin.UNRESOLVED +def _pin_from_constraint(constraint: str | None) -> tuple[str | None, Pin]: + """The same pin rules, for a constraint that is still a raw string. + + Slicing ``==`` off the front by hand would read ``==1.*`` as the version ``1.*`` + — a junk version that OSV matches nothing against — and ``===1.2.3`` as + ``=1.2.3``. Going through the specifier parser keeps one definition of "pinned". + """ + if not constraint: + return None, Pin.UNRESOLVED + try: + spec = SpecifierSet(constraint) + except InvalidSpecifier: + return None, Pin.UNRESOLVED + return _pin_from_specifier(spec) + + def _find_line(content: str, needle: str) -> int | None: """Best-effort line number for a name inside a structured file. @@ -395,16 +411,14 @@ def _parse_pipfile(path: Path, content: str) -> list[Dependency]: # Pipfile writes "*" for "any version", which is a constraint carrying # no information rather than a version. constraint = None if constraint in ("*", "") else constraint - version = ( - constraint[2:] if constraint and constraint.startswith("==") else None - ) + version, pin = _pin_from_constraint(constraint) dependencies.append( Dependency( ref=PackageRef(EcosystemId.PYPI, _normalise(name), version), scope=scope, direct=True, source=SourceLocation(path, _find_line(content, name)), - pin=Pin.PINNED if version else Pin.UNRESOLVED, + pin=pin, constraint=constraint, ) ) @@ -507,14 +521,20 @@ def _parse_uv_lock(path: Path, content: str) -> list[Dependency]: def _collect_uv_seeds(root: dict[str, Any], seeds: dict[str, Scope]) -> None: - """Record what the project asked for directly, and under which scope.""" + """Record what the project asked for directly, and under which scope. + + A package declared in several groups is merged to the most-included scope, not + to whichever group happened to be read first: an extra listed under both + ``docs`` and ``gui`` installs whenever either is asked for, and first-wins + would let the dev-flavoured group hide it from the default scan. + """ for name in _uv_names(root.get("dependencies")): - seeds[name] = Scope.RUNTIME + _seed(seeds, name, Scope.RUNTIME) for group, entries in (root.get("optional-dependencies") or {}).items(): scope = _scope_for_group(group) for name in _uv_names(entries): - seeds.setdefault(name, scope) + _seed(seeds, name, scope) # PEP 735 groups live under metadata and are development tooling by definition. metadata = root.get("metadata") @@ -523,7 +543,12 @@ def _collect_uv_seeds(root: dict[str, Any], seeds: dict[str, Scope]) -> None: for group, entries in requires_dev.items(): scope = _scope_for_group(group) if group else Scope.DEV for name in _uv_names(entries): - seeds.setdefault(name, scope) + _seed(seeds, name, scope) + + +def _seed(seeds: dict[str, Scope], name: str, scope: Scope) -> None: + current = seeds.get(name) + seeds[name] = scope if current is None else most_included((scope, current)) # --------------------------------------------------------------------------- @@ -613,7 +638,8 @@ def _parse_pipfile_lock(path: Path, content: str) -> list[Dependency]: continue for name, entry in table.items(): raw = entry.get("version") if isinstance(entry, dict) else None - version = raw[2:] if isinstance(raw, str) and raw.startswith("==") else None + constraint = raw if isinstance(raw, str) else None + version, pin = _pin_from_constraint(constraint) dependencies.append( Dependency( ref=PackageRef(EcosystemId.PYPI, _normalise(name), version), @@ -623,8 +649,8 @@ def _parse_pipfile_lock(path: Path, content: str) -> list[Dependency]: # merges that in; assuming direct here would be a guess. direct=False, source=source, - pin=Pin.PINNED if version else Pin.UNRESOLVED, - constraint=raw if isinstance(raw, str) else None, + pin=pin, + constraint=constraint, ) ) diff --git a/src/icebergsca/osv/client.py b/src/icebergsca/osv/client.py index ffe0324..4c1b157 100644 --- a/src/icebergsca/osv/client.py +++ b/src/icebergsca/osv/client.py @@ -109,8 +109,11 @@ async def scan(self, refs: Sequence[PackageRef]) -> OSVResult: advisory=_advisory(record), fixed_version=severity_module.fixed_version(record, ref), ) - for entry in entries + # querybatch already said this advisory affects the package; a failed + # detail fetch costs the severity and fix version, never the finding. if (record := details.get(_detail_key(entry))) is not None + else VulnHit(advisory=_skeleton(entry)) + for entry in entries ] if hits: merged = merge_aliases(hits) @@ -463,6 +466,16 @@ def _detail_key(entry: dict[str, str]) -> str: return f"{entry['id']}@{entry.get('modified', '')}" +def _skeleton(entry: dict[str, str]) -> Advisory: + """An advisory built from querybatch data alone, for when detail never arrived. + + Carries only the ID and modified timestamp, so it renders with an unknown + severity and no fix version — under-informed, but reported. Dropping the hit + instead would present a package OSV said is affected as if it were clean. + """ + return Advisory(id=entry["id"], modified=entry.get("modified", "")) + + def _advisory(record: dict[str, Any]) -> Advisory: references = tuple( reference["url"] diff --git a/tests/fixtures/npm/yarn-v1/yarn.lock b/tests/fixtures/npm/yarn-v1/yarn.lock index 8c337ee..7f93803 100644 --- a/tests/fixtures/npm/yarn-v1/yarn.lock +++ b/tests/fixtures/npm/yarn-v1/yarn.lock @@ -32,3 +32,10 @@ ms@^2.1.3: version "2.1.3" resolved "https://registry.yarnpkg.com/ms/-/ms-2.1.3.tgz#mno" integrity sha512-eeee + +app-ui@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/app-ui/-/app-ui-1.0.0.tgz#pqr" + integrity sha512-ffff + dependencies: + "@scope/widget" "^2.0.0" diff --git a/tests/test_lockfiles.py b/tests/test_lockfiles.py index b6fbceb..a559d35 100644 --- a/tests/test_lockfiles.py +++ b/tests/test_lockfiles.py @@ -136,6 +136,31 @@ def test_uv_lock_records_parents() -> None: assert [ref.name for ref in deps["idna"].parents] == ["anyio"] +def test_uv_lock_merges_scopes_across_groups_by_inclusion() -> None: + """A package in both a dev-flavoured extra and an ordinary one installs whenever + either is asked for, so the most-included scope must win — first-wins would let + the ``docs`` group hide it from the default scan.""" + content = """\ +version = 1 + +[[package]] +name = "example-app" +version = "0.1.0" +source = { editable = "." } + +[package.optional-dependencies] +docs = [{ name = "sphinx" }] +gui = [{ name = "sphinx" }] + +[[package]] +name = "sphinx" +version = "7.0.0" +source = { registry = "https://pypi.org/simple" } +""" + deps = {d.ref.name: d for d in python.parse_lockfile(Path("uv.lock"), content)} + assert deps["sphinx"].scope is Scope.OPTIONAL + + def test_uv_lock_rejects_a_file_with_no_packages() -> None: with pytest.raises(ParseError, match="no \\[\\[package\\]\\] entries"): python.parse_lockfile(Path("uv.lock"), "version = 1\n") @@ -200,6 +225,21 @@ def test_pipfile_manifest_reads_both_sections() -> None: assert deps["flask"].constraint is None +def test_pipfile_wildcard_equality_is_a_range_not_a_pin() -> None: + """``==2.*`` sliced to the "version" ``2.*`` would be sent to OSV, match no + advisory range, and read as clean. ``===2.0.0`` is the opposite case: a real + pin that the naive slice mangled into ``=2.0.0``.""" + content = '[packages]\nrequests = "==2.*"\nflask = "===2.0.0"\n' + deps = {d.ref.name: d for d in python.parse_manifest(Path("Pipfile"), content)} + + assert deps["requests"].ref.version is None + assert deps["requests"].pin is Pin.UNRESOLVED + assert deps["requests"].constraint == "==2.*" + + assert deps["flask"].ref.version == "2.0.0" + assert deps["flask"].pin is Pin.PINNED + + # --------------------------------------------------------------------------- # package.json # --------------------------------------------------------------------------- @@ -262,6 +302,27 @@ def test_lock_v3_uses_npms_own_scope_flags() -> None: assert deps["fsevents"].scope is Scope.OPTIONAL +def test_lock_v3_dev_optional_still_ships() -> None: + """npm's devOptional flag means "dev tree *and* optional dep of something that + ships" — a production install still gets it, so it must not be classed as dev, + which the default scan excludes.""" + content = """{ + "lockfileVersion": 3, + "packages": { + "": {"dependencies": {"sharp": "^0.33.0"}}, + "node_modules/sharp": { + "version": "0.33.0", + "dependencies": {"detect-libc": "^2.0.0"} + }, + "node_modules/detect-libc": {"version": "2.0.3", "devOptional": true} + } + }""" + deps = { + d.ref.name: d for d in npm.parse_lockfile(Path("package-lock.json"), content) + } + assert deps["detect-libc"].scope is Scope.OPTIONAL + + def test_lock_v3_marks_root_dependencies_direct() -> None: deps = load("npm", "lock-v3", "package-lock.json") assert deps["express"].direct is True @@ -409,6 +470,13 @@ def test_yarn_v1_resolves_edges_through_the_descriptor_index() -> None: assert [ref.name for ref in nested.parents] == ["debug"] +def test_yarn_v1_reads_quoted_scoped_dependency_edges() -> None: + """yarn quotes scoped packages in dependency lists — ``"@scope/widget" "^2.0.0"`` + — and a key pattern that cannot match the quotes drops every such edge.""" + deps = load("npm", "yarn-v1", "yarn.lock") + assert [ref.name for ref in deps["@scope/widget"].parents] == ["app-ui"] + + def test_yarn_v1_keeps_both_resolved_versions() -> None: parsed = npm.parse_lockfile( Path("yarn.lock"), (FIXTURES / "npm" / "yarn-v1" / "yarn.lock").read_text() diff --git a/tests/test_maven.py b/tests/test_maven.py index 895983f..9bd8c30 100644 --- a/tests/test_maven.py +++ b/tests/test_maven.py @@ -352,6 +352,40 @@ async def test_imported_boms_supply_versions() -> None: assert entry.ref.version == "3.3" +async def test_mutually_importing_boms_terminate() -> None: + """A cycle of import-scoped BOMs is cut, not recursed into until the stack dies. + + Central should never serve one, but the resolver must not bet its stack on + that — and the versions gathered on the way down still apply. + """ + import_bom = ( + "g{a}" + "1.0pomimport" + ) + responses = { + f"GET:{pom_url('g', 'a', '1.0')}": pom_xml( + "g", + "a", + "1.0", + management=import_bom.format(a="bom-x"), + dependencies=dep_xml("g", "from-bom", ""), + ), + f"GET:{pom_url('g', 'bom-x', '1.0')}": pom_xml( + "g", + "bom-x", + "1.0", + management=import_bom.format(a="bom-y") + dep_xml("g", "from-bom", "3.3"), + ), + f"GET:{pom_url('g', 'bom-y', '1.0')}": pom_xml( + "g", "bom-y", "1.0", management=import_bom.format(a="bom-x") + ), + f"GET:{pom_url('g', 'from-bom', '3.3')}": pom_xml("g", "from-bom", "3.3"), + } + result = await resolver(responses).expand((direct("g:a", "1.0"),)) + entry = next(d for d in result.dependencies if d.ref.name == "g:from-bom") + assert entry.ref.version == "3.3" + + async def test_unreachable_central_degrades_to_direct_dependencies() -> None: """Losing the network costs depth, never correctness of what we already had.""" result = await resolver({}).expand((direct("g:a", "1.0"),)) diff --git a/tests/test_osv.py b/tests/test_osv.py index f1c69ca..9a1f449 100644 --- a/tests/test_osv.py +++ b/tests/test_osv.py @@ -315,6 +315,30 @@ async def test_stale_cache_is_used_when_osv_is_down_and_says_so() -> None: assert any("expired cache" in w for w in result.warnings) +async def test_failed_detail_fetch_keeps_the_finding() -> None: + """querybatch said the package is affected; losing the detail must not lose that. + + The hit degrades to an ID with an unknown severity and no fix version — but a + package OSV says is affected must never render as clean because a second request + failed. + """ + client, _, _ = make_client( + { + f"POST:{QUERYBATCH_URL}": batch( + [{"id": GHSA, "modified": ADVISORY["modified"]}] + ) + # No GET stub: the detail request 404s. + } + ) + result = await client.scan([REQUESTS]) + + (hit,) = result.advisories[REQUESTS] + assert hit.advisory.id == GHSA + assert hit.advisory.level is SeverityLevel.UNKNOWN + assert hit.fixed_version is None + assert any("could not fetch details" in w for w in result.warnings) + + async def test_offline_mode_makes_no_requests() -> None: client, transport, _ = make_client({}, offline=True) result = await client.scan([REQUESTS]) diff --git a/tests/test_resolve.py b/tests/test_resolve.py index a492b00..1f78362 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -34,6 +34,14 @@ (NPM, "1.0.0-alpha", "1.0.0-beta"), (PYPI, "1.0.0rc1", "1.0.0"), (PYPI, "2.0", "10.0"), + # Numeric pre-release comparison: stringified, rc10 sorts below rc2. + (PYPI, "1.0.0rc2", "1.0.0rc10"), + (PYPI, "1.0.0a2", "1.0.0b1"), + # PEP 440: dev releases sort below any alpha, post releases above the final. + (PYPI, "1.0.0.dev1", "1.0.0a1"), + (PYPI, "1.0.0", "1.0.0.post1"), + # An epoch outranks any release number. + (PYPI, "2.0.0", "1!1.0.0"), (GO, "v1.2.3", "v1.3.0"), (GEM, "1.2.3", "1.2.10"), ], diff --git a/tests/test_triage.py b/tests/test_triage.py index 71a526c..f5d9573 100644 --- a/tests/test_triage.py +++ b/tests/test_triage.py @@ -92,6 +92,15 @@ def test_a_native_toml_date_is_accepted_as_well_as_a_quoted_one() -> None: assert rule.expires == date(2026, 12, 25) +def test_a_native_toml_datetime_becomes_a_date() -> None: + """``datetime`` is a ``date`` to isinstance but not to comparison: left as-is, + ``today > expires`` raises at matching time and takes the scan down with it.""" + (rule,) = rules(entry() + "expires = 2026-12-25 09:30:00\n") + assert rule.expires == date(2026, 12, 25) + assert rule.is_expired(date(2026, 12, 26)) is True + assert rule.is_expired(date(2026, 12, 25)) is False + + @pytest.mark.parametrize("missing", ["advisory", "package", "reason"]) def test_a_missing_required_field_is_an_error(missing: str) -> None: fields = {