A1: VersionIntent and constraint-aware package threat assessment#151
Open
sheeki03 wants to merge 16 commits into
Open
A1: VersionIntent and constraint-aware package threat assessment#151sheeki03 wants to merge 16 commits into
sheeki03 wants to merge 16 commits into
Conversation
…assessment (A1)
Replace the lossy `version: Option<String>` on package references with a
`VersionIntent` enum (Unspecified / Exact / Constraint{raw, parsed} / Resolved)
at both sites (the command-path `PackageRef` and the ecosystem dependency
model), so an unpinned or constrained install is no longer flattened to "no
version" and silently treated as unpinned-clean. An unparsed or only partially
understood constraint is preserved verbatim and treated as unresolved, never as
an exact pin.
Add a constraint-aware, serializable `assess_package` to the threat DB returning
a tagged `PackageThreatAssessment` (NoRecord / ExactMatch / ConstraintIntersects
Affected / ConstraintExcludesAffected / Unresolved) built from wire strings
(`ThreatMatchSummary`), since neither `ThreatMatch` nor `ThreatSource` derive
Serialize. `check_package` is kept as a shim. An all-versions record hard-matches
even when Unspecified; `ConstraintExcludesAffected` is returned only on proven
exclusion (whole requirement parsed, every affected version parsed, all evaluate
false), and any unsupported PEP 440 form (marker, epoch, local, `~=`, `===`,
wildcard, parse failure) becomes Unresolved. Supplemental precedence: exact wins,
else unresolved with deduped affected versions, else intersect, else exclude,
else NoRecord, so an overlay can never silence a primary hit.
Add a new Medium/Warn RuleId `ThreatUnresolvedMaliciousPackage`, fired for the
Unresolved and ConstraintIntersectsAffected paths on both the command path
(`rules::threatintel::check`) and the manifest/installed path (`ecosystem_scan`).
It does not short-circuit other signals: a co-present typosquat still fires; only
an ExactMatch suppresses weaker name-similarity findings.
Introduce `escalation::finalize_static_verdict`, applying severity overrides,
action derivation, action overrides, then paranoia filtering without downgrading
an override-forced Block, and use it in `ecosystem_scan` (which previously built
its verdict via `Verdict::from_findings` and so ignored `action_overrides`).
`confidence_to_severity` is now `pub(crate)` so the scan path maps an ExactMatch
to the same severity as the command path. `DependencyAssessment` carries an
`Option<PackageThreatAssessment>` summary (skip_serializing_if), and
`DeclaredDependency.version` serializes as the original version string so the
JSON shape is unchanged.
A conservative PEP 440 release-segment evaluator lives in the new
`version_intent` module; a full solver is intentionally out of scope. Includes
assess_package unit cases per VersionIntent (including unparsed and `===` giving
Unresolved, excluding vs intersecting, all-versions matching Unspecified,
supplemental dedup), a `check_package` shim regression, threatintel.toml fixtures
(unpinned and range malicious warn; co-present typosquat still fires), an
ecosystem_scan installed-METADATA exact-match test, and finalize_static_verdict
tests for the action-override-honored and paranoia-no-downgrade behavior.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a ChangesConstraint-Aware Threat Detection
Sequence Diagram(s)sequenceDiagram
participant Parser as "Command/Manifest Parser"
participant ThreatPath as "rules::threatintel / ecosystem_scan"
participant ThreatDb as "ThreatDb"
participant Escalation as "escalation::finalize_static_verdict"
Parser->>ThreatPath: PackageRef / DeclaredDependency<br/>(version: VersionIntent)
ThreatPath->>ThreatDb: assess_package(eco, name, &VersionIntent)
ThreatDb-->>ThreatPath: PackageThreatAssessment
alt ExactMatch
ThreatPath->>ThreatPath: emit ThreatMaliciousPackage (High)<br/>short-circuit, skip typosquat
else ConstraintIntersectsAffected or Unresolved
ThreatPath->>ThreatPath: emit ThreatUnresolvedMaliciousPackage (Medium)<br/>continue with typosquat/similarity checks
else NoRecord / ConstraintExcludesAffected
ThreatPath->>ThreatPath: no package finding<br/>allow normal checks to proceed
end
ThreatPath->>Escalation: findings + policy + tier
Escalation->>Escalation: apply severity_override<br/>apply action_overrides (snapshot block upgrade)<br/>paranoia filter (restore block if downgraded)
Escalation-->>ThreatPath: Verdict
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…A1 review) T1.9: assess_package_self compared the requested version to affected versions by literal string, so an exact pin like ==1.4 missed a record listing 1.4.0. It now also matches on numeric ReleaseVersion equality when both parse. A lone ==<ver> in from_pep440_specifier is Exact regardless of parse success, so a prerelease pin (==1.0.0rc1) blocks instead of degrading to a constraint warn. T3.4: cap ReleaseVersion::parse at 16 dot-separated segments to bound the segment allocation on attacker-influenceable version strings.
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tirith-core/src/ecosystem_scan.rs (1)
423-428:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve written constraints before emitting
Unspecified.These parsers now drop constraints/pins and set
VersionIntent::Unspecified; withassess_package, a safe pin such asmalicious-pkg==9.9.9orgem "x", "9.9.9"warns as unresolved against any version-specific malicious record. Preserve the raw spec intoVersionIntent(PEP 440 parsing for PyPI; raw unsupported constraints for Gemfile) so users can clear warnings by pinning a non-affected version.Also applies to: 463-477, 710-716
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/ecosystem_scan.rs` around lines 423 - 428, The python_requirement_name function is only extracting the package name from requirements.txt lines and discarding the version constraint information, causing all dependencies to be marked with VersionIntent::Unspecified. Instead of hardcoding VersionIntent::Unspecified in the DeclaredDependency creation, extract the version constraint from the same line using PEP 440 parsing (or a helper function) and populate the version field with the parsed constraint. This allows the assess_package function to properly handle pinned versions like malicious-pkg==9.9.9 and prevent false warnings when users pin non-affected versions. Apply the same fix to the other locations mentioned (around lines 463-477 and 710-716) where similar parsing occurs for different package managers.
🧹 Nitpick comments (2)
tests/fixtures/threatintel.toml (1)
72-105: ⚡ Quick winAdd a fixture that guards the non-semver exact-token path.
These new fixtures cover unresolved npm intent well, but they don’t lock the Docker/Go literal-token regression class (
sha256:...,latest, prerelease-like literals) where exact malicious hits can be downgraded to unresolved. Add at least one fixture that expectsthreat_malicious_package(and forbids unresolved) for a version-keyed malicious record using a literal non-semver token.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fixtures/threatintel.toml` around lines 72 - 105, Add a new fixture block after the existing fixtures that tests a literal non-semver token (such as a Docker sha256 hash, "latest", or prerelease-like literal) with a malicious package record. The fixture should use an input command that includes this non-semver literal token, set expected_action to "block", include "threat_malicious_package" in the expected_rules array, and add "threat_unresolved_malicious_package" to the forbidden_rules array to ensure the exact malicious match is confirmed rather than being downgraded to unresolved. This guards against regression where literal token paths incorrectly downgrade exact malicious hits to unresolved status.crates/tirith-core/src/scoring.rs (1)
83-83: ⚡ Quick winAdd a regression assertion for the new threat-intel rule classification.
The new classification is correct, but the nearby unit test should assert
ThreatUnresolvedMaliciousPackageexplicitly to lock this behavior.Suggested test update
fn is_threat_intel_rule_classifies_threat_family() { assert!(is_threat_intel_rule(RuleId::ThreatMaliciousPackage)); + assert!(is_threat_intel_rule( + RuleId::ThreatUnresolvedMaliciousPackage + )); assert!(is_threat_intel_rule(RuleId::ThreatCisaKev)); assert!(is_threat_intel_rule(RuleId::ThreatSafeBrowsing)); assert!(!is_threat_intel_rule(RuleId::CurlPipeShell));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/scoring.rs` at line 83, Add a regression assertion in the unit test near the RuleId::ThreatUnresolvedMaliciousPackage classification to explicitly verify that this threat-intel rule is correctly classified. This assertion should test the behavior of the new ThreatUnresolvedMaliciousPackage rule to ensure its classification does not regress in future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tirith-core/src/ecosystem_scan.rs`:
- Around line 283-288: The from_explicit_version call is incorrectly classifying
plain version tokens (like "1.2.3", "1", "1.2") as exact version pins when they
should be treated as version ranges per ecosystem semantics (Cargo caret
requirements, npm X-Ranges). Replace the call to
VersionIntent::from_explicit_version(v) with a more conservative approach using
Constraint { raw, parsed: None } to preserve these declarations as unresolved
ranges, allowing threat assessment to properly match against versions that fall
within the declared range. Apply this same fix in two locations: around line 283
in the current snippet and also around lines 587-591 for Cargo dependencies.
In `@crates/tirith-core/src/escalation.rs`:
- Around line 327-332: The finalize_static_verdict function restores an
override-forced Block verdict after paranoia filtering, but it does not preserve
the findings that caused the override, leaving callers with a Block verdict that
lacks explanation. Modify finalize_static_verdict to retain the rule IDs
returned by apply_action_overrides, and after the call to
crate::engine::filter_findings_by_paranoia, re-add any findings whose rule IDs
match those override-causing rule IDs back into the verdict's findings. Mirror
the approach used in post_process_verdict to ensure consistency. Additionally,
add a regression test that verifies an overridden Low or Info severity finding
is preserved in the verdict even when default paranoia filtering would normally
remove it.
In `@crates/tirith-core/src/install_txn.rs`:
- Around line 645-653: The PackageRef construction for Docker ecosystem at the
version_token assignment and similar locations for Go ecosystem (around lines
708-710 and 730) route non-semver selectors like sha256 digests and "latest"
through VersionIntent::from_explicit_version, which classifies them as
unsupported constraints and causes true version-keyed malicious hits to be
downgraded to unresolved warnings instead of exact malicious blocks. Add a
verbatim exact-compare fallback in the assessment layer before returning
unsupported or unresolved intent, so that literal selectors for Docker and Go
ecosystems can still be matched exactly against malicious package versions even
when they don't conform to semver formatting.
In `@crates/tirith-core/src/rules/threatintel.rs`:
- Around line 751-812: The assess_package method needs to add a raw-version
fallback for non-semver version tokens before returning Unresolved. When a
version constraint cannot be parsed (parsed: None), and before returning
PackageThreatAssessment::Unresolved, explicitly check if the raw unparsed
version token matches any exact affected-version records in the threat
intelligence database. If a raw version match is found, return
PackageThreatAssessment::ExactMatch instead of downgrading to Unresolved. This
ensures that malicious packages with non-standard version formats maintain their
high-severity ThreatMaliciousPackage classification instead of being incorrectly
downgraded to a medium-severity unresolved warning.
- Around line 178-181: The `from_pep440_specifier` helper is incorrectly parsing
unsupported PEP 440 specifier forms (such as `===`, markers like `python_version
>= '3.11'`, epochs, and locals) as exact versions instead of rejecting them as
unresolved. Before passing the `rest` variable to `from_pep440_specifier`, add
validation logic to detect and reject these unsupported forms (check for the
presence of markers, triple-equals `===`, epochs using colons, and local version
segments using `+`), and return an unresolved intent with an appropriate warning
for these cases instead of allowing them to reach the exact version parsing fast
path. Additionally, add parser tests that verify marker requirements and
arbitrary-equality specifications are properly handled as unresolved rather than
being incorrectly converted to exact versions.
In `@crates/tirith-core/src/threatdb.rs`:
- Around line 1095-1101: When handling VersionIntent::Constraint with parsed:
None, do not immediately return Unresolved. Before returning Unresolved, check
if the raw value exactly matches any of the affected versions in the affected
collection. If an exact match is found, return an ExactMatch
PackageThreatAssessment instead. Only return Unresolved if the raw value does
not match any affected versions. Additionally, add a regression test that
verifies when the DB affected version and the request raw token are identical
unparsed values, the result is ExactMatch.
In `@crates/tirith-core/src/version_intent.rs`:
- Around line 97-101: The exact version parsing block that checks for the "=="
prefix is not rejecting environment markers (indicated by semicolons). Modify
the condition in the exact version check (the one that currently validates
`!ver.is_empty() && !ver.contains(',') && !ver.contains('*')`) to also reject
versions containing semicolons by adding `&& !ver.contains(';')`. This ensures
that marker-qualified exact pins like `==1.0.0;python_version<"3.9"` fall
through to the Constraint parsing path instead of creating a malformed Exact
version intent.
---
Outside diff comments:
In `@crates/tirith-core/src/ecosystem_scan.rs`:
- Around line 423-428: The python_requirement_name function is only extracting
the package name from requirements.txt lines and discarding the version
constraint information, causing all dependencies to be marked with
VersionIntent::Unspecified. Instead of hardcoding VersionIntent::Unspecified in
the DeclaredDependency creation, extract the version constraint from the same
line using PEP 440 parsing (or a helper function) and populate the version field
with the parsed constraint. This allows the assess_package function to properly
handle pinned versions like malicious-pkg==9.9.9 and prevent false warnings when
users pin non-affected versions. Apply the same fix to the other locations
mentioned (around lines 463-477 and 710-716) where similar parsing occurs for
different package managers.
---
Nitpick comments:
In `@crates/tirith-core/src/scoring.rs`:
- Line 83: Add a regression assertion in the unit test near the
RuleId::ThreatUnresolvedMaliciousPackage classification to explicitly verify
that this threat-intel rule is correctly classified. This assertion should test
the behavior of the new ThreatUnresolvedMaliciousPackage rule to ensure its
classification does not regress in future changes.
In `@tests/fixtures/threatintel.toml`:
- Around line 72-105: Add a new fixture block after the existing fixtures that
tests a literal non-semver token (such as a Docker sha256 hash, "latest", or
prerelease-like literal) with a malicious package record. The fixture should use
an input command that includes this non-semver literal token, set
expected_action to "block", include "threat_malicious_package" in the
expected_rules array, and add "threat_unresolved_malicious_package" to the
forbidden_rules array to ensure the exact malicious match is confirmed rather
than being downgraded to unresolved. This guards against regression where
literal token paths incorrectly downgrade exact malicious hits to unresolved
status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: caca0c21-5f92-4762-8f37-a262904b5e69
📒 Files selected for processing (18)
crates/tirith-core/assets/data/rule_explanations.tomlcrates/tirith-core/build.rscrates/tirith-core/src/ecosystem_scan.rscrates/tirith-core/src/engine.rscrates/tirith-core/src/escalation.rscrates/tirith-core/src/install_txn.rscrates/tirith-core/src/lib.rscrates/tirith-core/src/mcp/output_filter.rscrates/tirith-core/src/rules/threatintel.rscrates/tirith-core/src/scoring.rscrates/tirith-core/src/threatdb.rscrates/tirith-core/src/threatdb_api.rscrates/tirith-core/src/verdict.rscrates/tirith-core/src/version_intent.rscrates/tirith-core/tests/golden_fixtures.rscrates/tirith/src/cli/install.rstests/fixtures/ecosystem.tomltests/fixtures/threatintel.toml
…ent (A1 review)
- from_pep440_specifier: gate the `==` exact fast path on looks_like_plain_version
so arbitrary-equality (`===`), environment markers, epochs, and wildcards fall
through to an unresolved Constraint instead of a bogus Exact string.
- assess_package_self: raw-token exact fallback. A non-semver token (a Docker
digest, a `latest` tag) preserved as Constraint{parsed:None} that literally
equals an affected version is now an ExactMatch (malicious block), not a
downgraded Unresolved warning.
- finalize_static_verdict: re-expose the override-causal finding after paranoia
filtering, so a restored override-forced Block is never left without a cause
(mirrors post_process_verdict).
- ecosystem_scan: capture the PEP 508 specifier for requirements.txt and PEP 621
dependencies so a real pin is assessed instead of a spurious unresolved warning;
classify npm partial versions (`1`, `1.2`) as X-ranges, not too-narrow exact pins.
Addresses CodeRabbit (version_intent/threatdb/threatintel/install_txn/escalation/
ecosystem_scan) and Greptile (pinned-dependency false positive) review findings.
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tirith-core/src/rules/threatintel.rs (1)
307-370:⚠️ Potential issue | 🟠 MajorTreat
cargo add name@versionas a Cargo requirement constraint, not an exact pin.
cargo add serde@1.0.193adds a caret requirement (equivalent to^1.0.193), not an exact version. The current code uses the sharedsplit_name_versionpath for bothcargo installandcargo add, incorrectly modeling@versionasExactfor both commands. This causes compatible affected versions to returnNoRecordinstead of unresolved findings. Keepcargo install --versionexact, but modelcargo addversions as unresolved constraints.Proposed fix
+fn cargo_requirement_intent(ver: &str) -> VersionIntent { + let raw = ver.trim(); + if raw.is_empty() { + VersionIntent::Unspecified + } else { + VersionIntent::Constraint { + raw: raw.to_string(), + parsed: None, + } + } +} + fn extract_cargo_packages(args: &[String], packages: &mut Vec<PackageRef>) { let mut iter = args.iter(); let mut found_subcmd = false; + let mut cargo_add = false; @@ if !found_subcmd { if matches!(lower.as_str(), "install" | "add") { + cargo_add = lower == "add"; found_subcmd = true; } continue; @@ - last.version = VersionIntent::from_explicit_version(ver); + last.version = if cargo_add { + cargo_requirement_intent(ver) + } else { + VersionIntent::from_explicit_version(ver) + }; } } } @@ - let (name, version) = split_name_version(arg, '@'); + let (name, version) = if cargo_add { + match arg.split_once('@') { + Some((name, ver)) => (name, cargo_requirement_intent(ver)), + None => (arg.as_str(), VersionIntent::Unspecified), + } + } else { + split_name_version(arg, '@') + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/rules/threatintel.rs` around lines 307 - 370, The extract_cargo_packages function incorrectly treats `cargo add name@version` as an exact version constraint when it should be treated as an unresolved caret requirement. Track whether the current subcommand being processed is "add" or "install" by maintaining the found_subcmd flag value separately. When creating a PackageRef for a cargo add command, check if a version was extracted via split_name_version and if so, set the version to an unresolved constraint (VersionIntent::Unresolved) instead of using the explicit version from split_name_version. Keep the existing logic for handling --version flags in cargo install as exact versions. This distinction between the two commands ensures that cargo add versions are properly modeled as caret constraints rather than exact pins.
🧹 Nitpick comments (2)
crates/tirith-core/src/scoring.rs (1)
83-83: ⚡ Quick winAdd an explicit classification assertion for the new threat-intel RuleId.
is_threat_intel_rule_classifies_threat_familyshould includeRuleId::ThreatUnresolvedMaliciousPackageto lock this behavior and prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/scoring.rs` at line 83, The new RuleId::ThreatUnresolvedMaliciousPackage needs to be explicitly handled in the is_threat_intel_rule_classifies_threat_family function to ensure proper threat classification behavior and prevent future regressions. Add a match arm for RuleId::ThreatUnresolvedMaliciousPackage in the is_threat_intel_rule_classifies_threat_family function that returns true, alongside any other threat-intel rules that classify threat families.tests/fixtures/threatintel.toml (1)
72-105: ⚡ Quick winAdd a fixture for non-semver explicit versions to guard the hot-path regression.
Please add a case like
npm install evil-package@latest(or another non-PEP440/non-semver token) that must still produce the confirmed malicious-package rule when the threat DB contains the same raw affected version string. Current additions don’t lock this regression path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fixtures/threatintel.toml` around lines 72 - 105, Add a new fixture to the threatintel.toml file that tests the non-semver explicit version regression path. Create a fixture with a name like npm_install_explicit_nonsemantics_malicious_matches that uses input like "npm install evil-package@latest" or another non-semver token, sets min_milestone to 1, context to "exec", expected_action to "block", and includes threat_malicious_package in the expected_rules (not just threat_unresolved_malicious_package). This ensures that when the threat database contains the same raw affected version string, the confirmed exact-match malicious package rule fires correctly for non-semver version specifiers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tirith-core/src/ecosystem_scan.rs`:
- Around line 324-330: The code performs unchecked arithmetic operations with
`major + 1` and `minor + 1` in the match arms for version range parsing, which
can overflow when these values are u64::MAX, causing panics in checked builds or
incorrect behavior in release builds. For each match arm ([major] and [major,
minor]), add overflow checks before performing the increment operations using
checked_add or by comparing against u64::MAX, and return None to leave the spec
unresolved if an overflow would occur, otherwise proceed with the range
generation.
- Around line 490-508: The `python_requirement_spec` function is stripping the
environment marker (the part after `;`) before extracting the version specifier,
which causes marker-qualified requirements like `evil==1.0; python_version <
"3.9"` to lose their markers and be treated as unconditional exact pins. To fix
this, remove the line that strips the marker (the split on `;`) and instead
apply the specifier extraction logic directly to the full requirement line. This
will preserve the environment marker in the returned string, allowing
`from_pep440_specifier` to detect it via `looks_like_plain_version` and properly
treat marker-qualified pins as unresolved constraints rather than exact pins.
In `@crates/tirith-core/src/rules/threatintel.rs`:
- Around line 689-694: The UnresolvedKind::Unresolved branch is ignoring the
reason field, resulting in generic messages that don't accurately reflect
whether the issue is with the user's constraint or the threat feed's
affected-version strings. Instead of using static messages like "The constraint
could not be fully resolved", extract and use the UnresolvedReason to generate
more specific error messages that accurately convey the actual root cause of the
resolution failure. Apply this change to both the UnresolvedKind::Unresolved
match block and the similar code handling the other unresolved kind variants
mentioned in the also applies to reference.
In `@crates/tirith-core/src/threatdb.rs`:
- Around line 1113-1133: The issue is that when rec.versions is empty, the code
creates an empty parsed_affected vector, causing intersecting.is_empty() to be
true, which returns ConstraintExcludesAffected. This incorrectly treats missing
version metadata as proof that the constraint excludes all affected versions,
when it should instead indicate that there is insufficient data to determine
exclusion. Fix this by adding a check after the version parsing loop in the
section with parsed_affected: if parsed_affected is empty and rec.versions was
empty, return PackageThreatAssessment::Unresolved with an appropriate reason
(like AffectedVersionUnparsed or a new reason indicating missing data), before
reaching the ConstraintExcludesAffected return. Only return
ConstraintExcludesAffected if parsed_affected contains entries but none matched
the constraint. Additionally, add a regression test that builds a
version-specific malicious record with empty versions array and verifies it
returns Unresolved rather than ConstraintExcludesAffected.
In `@crates/tirith-core/src/version_intent.rs`:
- Around line 97-106: The `looks_like_plain_version` function currently accepts
the `+` character, which represents PEP 440 local versions (e.g.,
`==1.0+ubuntu1`). This causes the condition within the `strip_prefix("==")`
block to treat such versions as Exact, but the threat-DB literal/numeric
comparison cannot properly match base records, leading to silent failures
instead of the expected UNRESOLVED behavior. Modify `looks_like_plain_version`
to reject the `+` character alongside the other already-rejected special
characters (`;`, `!`, `*`, etc.), so that local-version pins fall through to the
Constraint path and correctly result in UNRESOLVED instead of creating bogus
Exact strings.
- Around line 151-161: The looks_like_plain_version function incorrectly accepts
malformed versions with empty segments like `1.`, `1..2`, and `1.+` as exact
pins. The current segment validation only rejects wildcard segments (x/X) but
does not check for empty segments. Change the validation logic from rejecting
segments that match x or X to accepting only non-empty segments that do not
match x or X by replacing the any() check with an all() check that ensures each
segment produced by splitting on ['.', '-', '+'] is both non-empty and not equal
to x or X.
---
Outside diff comments:
In `@crates/tirith-core/src/rules/threatintel.rs`:
- Around line 307-370: The extract_cargo_packages function incorrectly treats
`cargo add name@version` as an exact version constraint when it should be
treated as an unresolved caret requirement. Track whether the current subcommand
being processed is "add" or "install" by maintaining the found_subcmd flag value
separately. When creating a PackageRef for a cargo add command, check if a
version was extracted via split_name_version and if so, set the version to an
unresolved constraint (VersionIntent::Unresolved) instead of using the explicit
version from split_name_version. Keep the existing logic for handling --version
flags in cargo install as exact versions. This distinction between the two
commands ensures that cargo add versions are properly modeled as caret
constraints rather than exact pins.
---
Nitpick comments:
In `@crates/tirith-core/src/scoring.rs`:
- Line 83: The new RuleId::ThreatUnresolvedMaliciousPackage needs to be
explicitly handled in the is_threat_intel_rule_classifies_threat_family function
to ensure proper threat classification behavior and prevent future regressions.
Add a match arm for RuleId::ThreatUnresolvedMaliciousPackage in the
is_threat_intel_rule_classifies_threat_family function that returns true,
alongside any other threat-intel rules that classify threat families.
In `@tests/fixtures/threatintel.toml`:
- Around line 72-105: Add a new fixture to the threatintel.toml file that tests
the non-semver explicit version regression path. Create a fixture with a name
like npm_install_explicit_nonsemantics_malicious_matches that uses input like
"npm install evil-package@latest" or another non-semver token, sets
min_milestone to 1, context to "exec", expected_action to "block", and includes
threat_malicious_package in the expected_rules (not just
threat_unresolved_malicious_package). This ensures that when the threat database
contains the same raw affected version string, the confirmed exact-match
malicious package rule fires correctly for non-semver version specifiers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9d1a759b-3e09-473e-8a37-8f5961fd010b
📒 Files selected for processing (18)
crates/tirith-core/assets/data/rule_explanations.tomlcrates/tirith-core/build.rscrates/tirith-core/src/ecosystem_scan.rscrates/tirith-core/src/engine.rscrates/tirith-core/src/escalation.rscrates/tirith-core/src/install_txn.rscrates/tirith-core/src/lib.rscrates/tirith-core/src/mcp/output_filter.rscrates/tirith-core/src/rules/threatintel.rscrates/tirith-core/src/scoring.rscrates/tirith-core/src/threatdb.rscrates/tirith-core/src/threatdb_api.rscrates/tirith-core/src/verdict.rscrates/tirith-core/src/version_intent.rscrates/tirith-core/tests/golden_fixtures.rscrates/tirith/src/cli/install.rstests/fixtures/ecosystem.tomltests/fixtures/threatintel.toml
…overflow (A1 re-review) A pathological npm version segment (18446744073709551615) would overflow major+1 / minor+1 in npm_partial_xrange: a panic in debug, or a bogus >=MAX,<0 range that never matches in release. checked_add returns None on overflow, so the spec stays an exact pin (Greptile).
…, don't exclude on empty affected (A1 re-review) - from_pep440_specifier: a PEP 440 local version (==1.0+ubuntu1) is outside the comparable subset and now stays an unresolved Constraint instead of a false Exact (CodeRabbit, Critical). - looks_like_plain_version: reject empty version segments (1., 1..2, 1.+ dynamic selector) alongside x/X wildcards, so malformed/dynamic versions are not exact pins (CodeRabbit). - python_requirement_spec: keep the ; environment marker so a marker-qualified requirement degrades to an unresolved Constraint, not an unconditional exact pin (CodeRabbit). - assess_package_self: a version-specific malicious record with NO affected versions is Unresolved, not a proven ConstraintExcludesAffected exclusion (CodeRabbit).
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/tirith-core/src/rules/threatintel.rs (1)
159-181:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSplit pip environment markers before marker operators corrupt the package name.
For
pip install 'evil; python_version<"3.9"', the current.find('<')sees the marker comparator and makesname_partincludeevil; python_version, so the threat DB lookup missesevilentirely. Treat;as a name/spec boundary when it appears before any version operator, while still preserving the marker inrestso it becomes unresolved.Proposed fix
let split_pos = pkg_str .find("==") .or_else(|| pkg_str.find(">=")) .or_else(|| pkg_str.find("<=")) .or_else(|| pkg_str.find("~=")) .or_else(|| pkg_str.find("!=")) .or_else(|| pkg_str.find('>')) .or_else(|| pkg_str.find('<')); - if let Some(pos) = split_pos { - (&pkg_str[..pos], &pkg_str[pos..]) - } else { - (pkg_str, "") + match (split_pos, pkg_str.find(';')) { + (Some(spec_pos), Some(marker_pos)) if marker_pos < spec_pos => { + (&pkg_str[..marker_pos], &pkg_str[marker_pos..]) + } + (None, Some(marker_pos)) => (&pkg_str[..marker_pos], &pkg_str[marker_pos..]), + (Some(pos), _) => (&pkg_str[..pos], &pkg_str[pos..]), + (None, None) => (pkg_str, ""), } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/rules/threatintel.rs` around lines 159 - 181, The code currently searches for version operators (==, >=, <=, etc.) in the entire pkg_str without first removing environment markers, which causes the find() chain to incorrectly match operators within marker conditions like python_version<"3.9". Before applying the version operator search logic, first split pkg_str at the semicolon (;) to separate the package specification from any environment markers, then apply the existing find() chain only to the package spec portion, and finally append the marker portion back to rest to preserve it for proper handling downstream. Update the split_pos logic and the name_part/rest tuple construction to work with this marker-aware parsing.
♻️ Duplicate comments (1)
crates/tirith-core/src/ecosystem_scan.rs (1)
661-665:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat Cargo manifest versions as requirements, not exact pins.
Cargo.tomlversion = "1.0"/{ version = "1" }are dependency requirements, but line 664 turns them intoExactviafrom_explicit_version.Exactonly checks the literal/numeric declared token, so a malicious affected version inside the allowed Cargo range can be missed instead of producing the unresolved malicious-package warning. Preserve Cargo manifest specs as unresolved constraints until Cargo semver is modeled. This was previously called out for the Cargo path and still appears here.Cargo reference specifying dependencies: does `version = "1.2.3"` mean a caret version requirement rather than an exact pin?Proposed fix
- version: version - .map(|v| VersionIntent::from_explicit_version(&v)) - .unwrap_or(VersionIntent::Unspecified), + version: version + .map(|v| { + let v = v.trim(); + if v.is_empty() { + VersionIntent::Unspecified + } else { + VersionIntent::Constraint { + raw: v.to_string(), + parsed: None, + } + } + }) + .unwrap_or(VersionIntent::Unspecified),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/ecosystem_scan.rs` around lines 661 - 665, The code at line 664 is treating Cargo manifest versions as exact pins by calling from_explicit_version, but Cargo manifest version requirements (like "1.0") are not exact pins and should be preserved as unresolved constraints to avoid missing malicious versions within allowed ranges. Replace the mapping that calls from_explicit_version with a direct assignment of VersionIntent::Unresolved to ensure Cargo manifest dependencies are treated as unresolved requirements rather than exact versions until Cargo semver is properly modeled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tirith-core/assets/data/rule_explanations.toml`:
- Around line 1337-1338: Remove the duplicate `[[rule]]` declaration in the
rule_explanations.toml file. The threat_unresolved_malicious_package rule block
has `[[rule]]` appearing twice consecutively, which creates an empty TOML table
entry before the actual rule definition. Keep only one `[[rule]]` declaration
immediately before the `id = "threat_unresolved_malicious_package"` line and
delete the duplicate preceding it.
In `@crates/tirith-core/src/rules/threatintel.rs`:
- Line 303: The version intent parsing at the assignment for version using
intent_from_opt_token does not properly handle npm partial version specifiers
like "4" and "1.2" which npm treats as ranges (4.x, 1.2.x) but are currently
classified as exact pins. Extract or reuse the npm-specific version parsing
logic from ecosystem_scan.rs (specifically the npm_manifest_intent function or
npm_partial_xrange approach) and apply it to the version parameter in this
location to ensure partial versions are correctly interpreted as ranges instead
of exact version matches, aligning the CLI parsing behavior with the manifest
parsing behavior.
In `@crates/tirith-core/src/threatdb.rs`:
- Around line 1061-1088: The code currently allows records with
all_versions_malicious=false and empty rec.versions to fall through to NoRecord
for Exact and Resolved version intents, which misses reporting malicious
packages with incomplete metadata. Add a guard check immediately after the
all_versions_malicious check to handle the case where rec.versions.is_empty() is
true, returning PackageThreatAssessment::Unresolved instead of allowing the
match intent logic to proceed, ensuring that records with missing
affected-version data are treated as unresolved rather than as a non-match.
In `@tests/fixtures/threatintel.toml`:
- Around line 72-105: The test fixture suite is missing regression tests for two
known P1 issues: npm partial-xrange overflow handling (the value
18446744073709551615) and non-semver exact-string matching fallback behavior.
After the existing fixtures that validate unresolved malicious package warnings
(like npm_install_unpinned_malicious_warns_unresolved,
npm_install_range_malicious_warns_unresolved, and
npm_install_unresolved_plus_typosquat_both_fire), add two additional fixture
entries using the same structure shown in the diff. Create one fixture that
tests the npm partial-xrange overflow scenario and another that validates
non-semver exact-string matching fallback behavior, ensuring each has
appropriate input, expected_action, expected_rules, and forbidden_rules to
prevent silent regressions of these known issues.
---
Outside diff comments:
In `@crates/tirith-core/src/rules/threatintel.rs`:
- Around line 159-181: The code currently searches for version operators (==,
>=, <=, etc.) in the entire pkg_str without first removing environment markers,
which causes the find() chain to incorrectly match operators within marker
conditions like python_version<"3.9". Before applying the version operator
search logic, first split pkg_str at the semicolon (;) to separate the package
specification from any environment markers, then apply the existing find() chain
only to the package spec portion, and finally append the marker portion back to
rest to preserve it for proper handling downstream. Update the split_pos logic
and the name_part/rest tuple construction to work with this marker-aware
parsing.
---
Duplicate comments:
In `@crates/tirith-core/src/ecosystem_scan.rs`:
- Around line 661-665: The code at line 664 is treating Cargo manifest versions
as exact pins by calling from_explicit_version, but Cargo manifest version
requirements (like "1.0") are not exact pins and should be preserved as
unresolved constraints to avoid missing malicious versions within allowed
ranges. Replace the mapping that calls from_explicit_version with a direct
assignment of VersionIntent::Unresolved to ensure Cargo manifest dependencies
are treated as unresolved requirements rather than exact versions until Cargo
semver is properly modeled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 482f9f41-136f-4102-8234-d2fdb3d5b560
📒 Files selected for processing (18)
crates/tirith-core/assets/data/rule_explanations.tomlcrates/tirith-core/build.rscrates/tirith-core/src/ecosystem_scan.rscrates/tirith-core/src/engine.rscrates/tirith-core/src/escalation.rscrates/tirith-core/src/install_txn.rscrates/tirith-core/src/lib.rscrates/tirith-core/src/mcp/output_filter.rscrates/tirith-core/src/rules/threatintel.rscrates/tirith-core/src/scoring.rscrates/tirith-core/src/threatdb.rscrates/tirith-core/src/threatdb_api.rscrates/tirith-core/src/verdict.rscrates/tirith-core/src/version_intent.rscrates/tirith-core/tests/golden_fixtures.rscrates/tirith/src/cli/install.rstests/fixtures/ecosystem.tomltests/fixtures/threatintel.toml
…, unresolved on empty affected, npm-aware CLI specs (A1 re-review r2)
- assess_package_self: a PEP 440 LOCAL version (1.0+ubuntu1) stays Exact and now
matches both an exact-local DB record (literal) AND a malicious BASE record (1.0),
so reverting the prior over-broad `+` guard no longer downgrades a real exact-local
block to Unresolved (Greptile P0) while a malicious base is still caught (CodeRabbit).
- the empty-affected-versions guard moved BEFORE the intent match, so an Exact/Resolved
request against a versionless malicious record is Unresolved, not a silent NoRecord
(CodeRabbit).
- npm CLI specs (npm install lodash@4) now use npm_manifest_intent so a bare partial is
an X-range, matching the manifest path, instead of a bogus Exact("4") that misses a
record for the resolved 4.17.21 (CodeRabbit).
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
crates/tirith-core/assets/data/rule_explanations.toml (1)
1337-1338:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the extra
[[rule]]table header.Line 1337 is duplicated consecutively, which creates an unintended empty rule entry before
threat_unresolved_malicious_package.🛠️ Proposed fix
-[[rule]] [[rule]] id = "threat_unresolved_malicious_package"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/assets/data/rule_explanations.toml` around lines 1337 - 1338, The rule_explanations.toml file has a duplicate [[rule]] table header that creates an empty rule entry before the threat_unresolved_malicious_package rule definition. Locate the duplicate [[rule]] header that appears on line 1337 (the one immediately before id = "threat_unresolved_malicious_package") and remove it, keeping only one [[rule]] header for this rule entry.crates/tirith-core/src/ecosystem_scan.rs (1)
661-665:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep Cargo manifest requirements unresolved rather than exact.
Cargo.tomldependency specs are requirements, not lockfile-resolved versions. Passing"1.2.3"throughfrom_explicit_versionmakes itExact("1.2.3"), so threat assessment can miss a malicious affected version inside the allowed Cargo range. Since Cargo constraints are not modeled here, preserve the raw spec as an unparsedConstraint. This is the Cargo half of the previously flagged manifest-semver issue.Cargo dependency version requirement "1.2.3" caret requirement exact pin Cargo referenceSuggested conservative fix
- // Cargo semver requirement: not parsed; plain is exact, - // a range stays unresolved. + // Cargo semver requirement: not parsed here; keep it + // unresolved rather than treating manifest specs as + // lockfile-resolved exact versions. version: version - .map(|v| VersionIntent::from_explicit_version(&v)) + .map(|v| VersionIntent::Constraint { + raw: v, + parsed: None, + }) .unwrap_or(VersionIntent::Unspecified),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/ecosystem_scan.rs` around lines 661 - 665, The issue is that Cargo manifest dependency version specs (like "1.2.3") are being converted to exact versions via from_explicit_version, but these are requirements/constraints, not resolved lockfile versions. Replace the call to from_explicit_version with a mechanism that preserves the raw Cargo spec as an unparsed constraint instead of converting it to an Exact version intent. When a version string is present, represent it as an unresolved constraint rather than an exact match, and only use VersionIntent::Unspecified when no version is specified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/tirith-core/src/ecosystem_scan.rs`:
- Around line 497-519: The python_requirement_spec function is incorrectly
identifying comparison operators in environment markers as version specifiers.
For a requirement like django ; python_version < "3.9", the function finds the <
inside the marker and returns it as a version constraint when none exists.
Modify the function to search for version operators only in the substring before
the semicolon (which marks the start of the environment marker), while still
preserving the marker suffix when a real version operator appears before the
semicolon.
- Around line 594-598: The loop over deps.keys() discards the version
specification values entirely, causing all Poetry dependencies to be marked as
VersionIntent::Unspecified regardless of their actual pinned versions. Instead
of only iterating over keys, also extract the corresponding value from deps,
which can be either a string (like "==2.28.1") or an inline table (like {
version = "^2.28" }). Create a helper to parse this value and extract the
version string, then pass the result to VersionIntent::from_pep440_specifier()
in the push() call instead of using VersionIntent::Unspecified. Unsupported
syntax like ^2.0 will naturally degrade to Constraint rather than Unspecified.
In `@crates/tirith-core/src/scoring.rs`:
- Line 83: The new RuleId::ThreatUnresolvedMaliciousPackage rule has been added
to the threat-intel classification set but lacks a corresponding test assertion.
Add a direct positive assertion in the classification test to verify that
RuleId::ThreatUnresolvedMaliciousPackage is correctly mapped to the threat-intel
category. This ensures the classification mapping cannot regress without being
caught by tests.
In `@crates/tirith-core/src/threatdb.rs`:
- Around line 1115-1159: The parsed constraint evaluation block in
VersionIntent::Constraint should only apply PEP 440 semantics for PyPI
ecosystem. Currently, non-PyPI ecosystems can incorrectly return
ConstraintExcludesAffected instead of Unresolved. After successfully unpacking
the parsed constraint (the `Some(constraint)` case), add an ecosystem check to
verify this is PyPI before proceeding with the constraint matching logic using
constraint.matches(rv). If the ecosystem is not PyPI, return
PackageThreatAssessment::Unresolved with UnresolvedReason::ConstraintUnsupported
immediately. Additionally, add a regression test case that verifies a non-PyPI
package record with a parsed constraint (such as npm at 1.0.0 with >=2.0.0)
returns Unresolved, not ConstraintExcludesAffected.
In `@crates/tirith-core/src/version_intent.rs`:
- Around line 11-18: The module documentation starting at line 11 states that
prerelease and local version forms deliberately fail to parse and fall back to
UNRESOLVED, but the implementation and tests show that exact-pin specifiers like
`==1.0.0rc1` and `==1.0+ubuntu1` are kept as Exact constraints instead. Update
the documentation comments in the module preamble (around lines 11-18) and
anywhere else the contract is described (also at lines 97-108 and 374-427) to
clarify that prerelease and local version components are accepted and handled as
Exact constraints when using the equality operator, so the public contract
matches the actual tested behavior.
---
Duplicate comments:
In `@crates/tirith-core/assets/data/rule_explanations.toml`:
- Around line 1337-1338: The rule_explanations.toml file has a duplicate
[[rule]] table header that creates an empty rule entry before the
threat_unresolved_malicious_package rule definition. Locate the duplicate
[[rule]] header that appears on line 1337 (the one immediately before id =
"threat_unresolved_malicious_package") and remove it, keeping only one [[rule]]
header for this rule entry.
In `@crates/tirith-core/src/ecosystem_scan.rs`:
- Around line 661-665: The issue is that Cargo manifest dependency version specs
(like "1.2.3") are being converted to exact versions via from_explicit_version,
but these are requirements/constraints, not resolved lockfile versions. Replace
the call to from_explicit_version with a mechanism that preserves the raw Cargo
spec as an unparsed constraint instead of converting it to an Exact version
intent. When a version string is present, represent it as an unresolved
constraint rather than an exact match, and only use VersionIntent::Unspecified
when no version is specified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a04a9f64-9be6-4526-be8b-1dea93acdead
📒 Files selected for processing (18)
crates/tirith-core/assets/data/rule_explanations.tomlcrates/tirith-core/build.rscrates/tirith-core/src/ecosystem_scan.rscrates/tirith-core/src/engine.rscrates/tirith-core/src/escalation.rscrates/tirith-core/src/install_txn.rscrates/tirith-core/src/lib.rscrates/tirith-core/src/mcp/output_filter.rscrates/tirith-core/src/rules/threatintel.rscrates/tirith-core/src/scoring.rscrates/tirith-core/src/threatdb.rscrates/tirith-core/src/threatdb_api.rscrates/tirith-core/src/verdict.rscrates/tirith-core/src/version_intent.rscrates/tirith-core/tests/golden_fixtures.rscrates/tirith/src/cli/install.rstests/fixtures/ecosystem.tomltests/fixtures/threatintel.toml
…ncy version (A1 re-review r2) For `django ; python_version < "3.9"` (a marker-only requirement with no version spec), python_requirement_spec found the `<` INSIDE the marker and returned `< "3.9"` as django's version. Search for the version operator only in the part before the `;`, so a marker-only dep is Unspecified; a real operator before the marker still keeps the whole tail (marker included) so from_pep440_specifier rejects it (CodeRabbit).
…t new threat-intel rule classification (A1 re-review r2) - the module contract doc listed "a local version" among specs that fail to parse to UNRESOLVED, but an EXACT local pin (1.0+ubuntu1) is kept as Exact and matched literally + by base release; only a local inside a RANGE stays unresolved (CodeRabbit). - is_threat_intel_rule_classifies_threat_family now asserts ThreatUnresolvedMaliciousPackage so the new rule's classification cannot regress silently (CodeRabbit).
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/tirith-core/src/threatdb.rs (1)
1146-1170:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturn proven intersections before falling back to
AffectedVersionUnparsed.This still returns
Unresolvedon the first unparseable affected version, even when a later parseable affected version satisfies the constraint. Collect matching parseable versions first; only returnAffectedVersionUnparsedif no intersection was proven.🐛 Proposed fix
- // Every affected version must parse for a proven exclusion; - // otherwise we cannot claim the constraint excludes them. - let mut parsed_affected: Vec<(String, ReleaseVersion)> = - Vec::with_capacity(rec.versions.len()); + // A single parsed affected version that matches proves intersection. + // Every affected version must parse only for a proven exclusion. + let mut intersecting = Vec::new(); + let mut saw_unparsed_affected = false; for v in &rec.versions { match ReleaseVersion::parse(v) { - Some(rv) => parsed_affected.push((v.to_string(), rv)), + Some(rv) if constraint.matches(&rv) => intersecting.push(v.to_string()), + Some(_) => {} None => { - return PackageThreatAssessment::Unresolved { - summary, - reason: UnresolvedReason::AffectedVersionUnparsed, - affected_versions: affected, - }; + saw_unparsed_affected = true; } } } - let intersecting: Vec<String> = parsed_affected - .iter() - .filter(|(_, rv)| constraint.matches(rv)) - .map(|(s, _)| s.clone()) - .collect(); - if intersecting.is_empty() { - PackageThreatAssessment::ConstraintExcludesAffected - } else { + if !intersecting.is_empty() { PackageThreatAssessment::ConstraintIntersectsAffected { summary, affected_versions: intersecting, } + } else if saw_unparsed_affected { + PackageThreatAssessment::Unresolved { + summary, + reason: UnresolvedReason::AffectedVersionUnparsed, + affected_versions: affected, + } + } else { + PackageThreatAssessment::ConstraintExcludesAffected }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/threatdb.rs` around lines 1146 - 1170, The code currently returns PackageThreatAssessment::Unresolved with AffectedVersionUnparsed as soon as it encounters a single version that cannot be parsed, rather than continuing to check other versions. Refactor the loop to continue processing all versions regardless of parse failures, collecting both the parseable versions and tracking whether any parse failures occurred. After the loop completes, only return the AffectedVersionUnparsed error if the intersecting vector is empty AND there were versions that failed to parse during iteration. This ensures that proven intersections with parseable affected versions are returned before falling back to the AffectedVersionUnparsed unresolved state.
🧹 Nitpick comments (1)
crates/tirith-core/src/threatdb.rs (1)
1553-1559: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAlign the merge doc with the provenance-preserving behavior.
The doc still says affected versions are deduplicated from every layer, but the implementation intentionally returns affected versions only from the layer whose summary is reported. Please update this comment so future changes do not reintroduce cross-source misattribution.
♻️ Proposed doc fix
-/// either layer is unresolved the result is `Unresolved` with deduplicated -/// affected versions drawn from every layer that names any (so the warning -/// lists them all); else a concrete intersection (deduped); else a proven -/// exclusion; else `NoRecord`. Crucially, `ConstraintExcludesAffected` (which +/// either layer is unresolved the result is `Unresolved` with the summary, +/// reason, and affected versions from the reported layer; else a concrete +/// intersection with affected versions from the reported layer; else a proven +/// exclusion; else `NoRecord`. Crucially, `ConstraintExcludesAffected` (which🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/src/threatdb.rs` around lines 1553 - 1559, The documentation comment describing the merge precedence behavior incorrectly states that affected versions are deduplicated from every layer that names any, but the actual implementation returns affected versions only from the layer whose summary is reported. Update the Precedence documentation comment to accurately reflect that affected versions are sourced only from the layer whose summary is being returned, not from all layers, to ensure the documentation aligns with the actual provenance-preserving behavior and prevents future regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/tirith-core/src/threatdb.rs`:
- Around line 1146-1170: The code currently returns
PackageThreatAssessment::Unresolved with AffectedVersionUnparsed as soon as it
encounters a single version that cannot be parsed, rather than continuing to
check other versions. Refactor the loop to continue processing all versions
regardless of parse failures, collecting both the parseable versions and
tracking whether any parse failures occurred. After the loop completes, only
return the AffectedVersionUnparsed error if the intersecting vector is empty AND
there were versions that failed to parse during iteration. This ensures that
proven intersections with parseable affected versions are returned before
falling back to the AffectedVersionUnparsed unresolved state.
---
Nitpick comments:
In `@crates/tirith-core/src/threatdb.rs`:
- Around line 1553-1559: The documentation comment describing the merge
precedence behavior incorrectly states that affected versions are deduplicated
from every layer that names any, but the actual implementation returns affected
versions only from the layer whose summary is reported. Update the Precedence
documentation comment to accurately reflect that affected versions are sourced
only from the layer whose summary is being returned, not from all layers, to
ensure the documentation aligns with the actual provenance-preserving behavior
and prevents future regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d06fad52-6bfa-4260-bb30-da3d9e039093
📒 Files selected for processing (18)
crates/tirith-core/assets/data/rule_explanations.tomlcrates/tirith-core/build.rscrates/tirith-core/src/ecosystem_scan.rscrates/tirith-core/src/engine.rscrates/tirith-core/src/escalation.rscrates/tirith-core/src/install_txn.rscrates/tirith-core/src/lib.rscrates/tirith-core/src/mcp/output_filter.rscrates/tirith-core/src/rules/threatintel.rscrates/tirith-core/src/scoring.rscrates/tirith-core/src/threatdb.rscrates/tirith-core/src/threatdb_api.rscrates/tirith-core/src/verdict.rscrates/tirith-core/src/version_intent.rscrates/tirith-core/tests/golden_fixtures.rscrates/tirith/src/cli/install.rstests/fixtures/ecosystem.tomltests/fixtures/threatintel.toml
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
…sion spec (A1 re-review)
python_requirement_spec received the raw line, so `malware==1.0.0 # pinned safe version` carried
the comment into the version token. from_pep440_specifier then saw `==1.0.0 # ...`, failed
looks_like_plain_version (the space), and produced Constraint{parsed:None} -> Unresolved. Net
effect: an EXACT pin of the affected version was silently downgraded from ThreatMaliciousPackage
(Critical/Block) to ThreatUnresolvedMaliciousPackage (Medium/Warn), and a safe pin got a spurious
warn. Drop the pip-style ` #` / `\t#` inline comment before the operator search (Greptile P1).
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
Owner
Author
|
@coderabbitai full review |
Owner
Author
|
@greptileai review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/tirith-core/assets/data/rule_explanations.toml (1)
1337-1338:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove the duplicated
[[rule]]header at Line 1337.There are two consecutive
[[rule]]entries before the new rule body. This introduces an empty table element and can break rule explanation parsing.Proposed fix
-[[rule]] [[rule]] id = "threat_unresolved_malicious_package"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/tirith-core/assets/data/rule_explanations.toml` around lines 1337 - 1338, Remove the duplicate [[rule]] header that appears consecutively before the threat_unresolved_malicious_package rule definition. Keep only one [[rule]] header before the id = "threat_unresolved_malicious_package" line. The duplicate entry creates an empty table element that breaks the rule explanation parsing in the TOML file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@crates/tirith-core/assets/data/rule_explanations.toml`:
- Around line 1337-1338: Remove the duplicate [[rule]] header that appears
consecutively before the threat_unresolved_malicious_package rule definition.
Keep only one [[rule]] header before the id =
"threat_unresolved_malicious_package" line. The duplicate entry creates an empty
table element that breaks the rule explanation parsing in the TOML file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 47966446-48da-437d-8c2c-f03e8ca4ca2f
📒 Files selected for processing (18)
crates/tirith-core/assets/data/rule_explanations.tomlcrates/tirith-core/build.rscrates/tirith-core/src/ecosystem_scan.rscrates/tirith-core/src/engine.rscrates/tirith-core/src/escalation.rscrates/tirith-core/src/install_txn.rscrates/tirith-core/src/lib.rscrates/tirith-core/src/mcp/output_filter.rscrates/tirith-core/src/rules/threatintel.rscrates/tirith-core/src/scoring.rscrates/tirith-core/src/threatdb.rscrates/tirith-core/src/threatdb_api.rscrates/tirith-core/src/verdict.rscrates/tirith-core/src/version_intent.rscrates/tirith-core/tests/golden_fixtures.rscrates/tirith/src/cli/install.rstests/fixtures/ecosystem.tomltests/fixtures/threatintel.toml
| if last.ecosystem == Ecosystem::RubyGems | ||
| && matches!(last.version, VersionIntent::Unspecified) | ||
| { | ||
| last.version = VersionIntent::from_explicit_version(ver); |
There was a problem hiding this comment.
from_explicit_version does not strip the leading = that Ruby Gems CLI passes as part of a version constraint, so gem install evil-pkg --version "= 1.0.0" produces Constraint { raw: "= 1.0.0" } → Unresolved (Medium/Warn) instead of Exact("1.0.0") → ExactMatch (Critical/Block). from_gem_version already handles this case by stripping the leading = before delegating to from_explicit_version, and is the correct helper to use here — it is used consistently in gemfile_gem_spec for the same ecosystem.
Suggested change
| last.version = VersionIntent::from_explicit_version(ver); | |
| last.version = VersionIntent::from_gem_version(ver); |
Owner
Author
|
@greptileai review |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Owner
Author
|
@greptileai review |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
Owner
Author
|
@greptileai review |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
…es so an attacker-cased dist excludes itself from threat-DB lookup (A1 re-review)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the artifact-firewall hardening train (Stack A, PR 1 of 4). Bases on
main. The rest of Stack A (A2 to A4) and Stack B will stack on this branch.What
Replace the lossy
version: Option<String>on package references with aVersionIntentenum (Unspecified / Exact / Constraint{raw, parsed} / Resolved) at both the command-pathPackageRefand the ecosystem dependency model, so an unpinned or constrained install is no longer flattened to "no version" and silently treated as unpinned-clean. An unparsed or only partially understood constraint is preserved verbatim and treated as unresolved, never an exact pin.Add a constraint-aware, serializable
assess_packageto the threat DB returning a taggedPackageThreatAssessment(NoRecord / ExactMatch / ConstraintIntersectsAffected / ConstraintExcludesAffected / Unresolved), built from wire strings (ThreatMatchSummary) since neitherThreatMatchnorThreatSourcederive Serialize.check_packagestays as a shim. An all-versions record hard-matches even when Unspecified;ConstraintExcludesAffectedis returned only on proven exclusion (whole requirement parsed, every affected version parsed, all evaluate false); any unsupported PEP 440 form (marker, epoch, local,~=,===, wildcard, parse failure) becomes Unresolved. PEP 440 parsing is PyPI only for now; other ecosystems stay unresolved.New Medium / Warn RuleId
ThreatUnresolvedMaliciousPackage, fired for the Unresolved and ConstraintIntersectsAffected paths on both the command path and the manifest/installed path. It does not short-circuit other signals: a co-present typosquat still fires, and only an ExactMatch suppresses weaker name-similarity findings.Introduce
escalation::finalize_static_verdictand use it inecosystem_scan, which previously built its verdict viaVerdict::from_findingsand so ignoredaction_overrides. It applies severity overrides, action derivation, action overrides, then paranoia filtering without downgrading an override-forced Block.Notes
test-threatdb.dat, because the generator also rotates the production Ed25519 verify key.test-threatdb.datcontent (syntheticevil-package/reacct), not real package names.Gate
fmt clean, clippy
-D warningsclean, full workspace builds, 2961 tirith-core tests pass. The known parallel-env-race flakes (checkpoint-restore, clipboard-sidecar, url_validate documentation-range) pass single-threaded and touch no A1 code.Stack
A2 (typed scan outcomes, coverage, file classification) will base on this branch.
Summary by CodeRabbit
Release Notes
New Features
threat_unresolved_malicious_packagewhen a malicious package name is known but the requested version is unpinned or overlaps affected versions, with remediation guidance.Bug Fixes
Refactor
Tests
Greptile Summary
This PR replaces the lossy
version: Option<String>on package references with aVersionIntentenum (Unspecified / Exact / Constraint / Resolved), adds a constraint-awareassess_packageto the threat DB returning a taggedPackageThreatAssessment, and introducesfinalize_static_verdictto ensureecosystem_scanhonoursaction_overridesand paranoia policy levers that the bareVerdict::from_findingsconstructor ignored.VersionIntent+ PEP 440 constraint evaluator (version_intent.rs): lossless representation of pip specifiers, Cargo caret requirements, npm X-ranges, and Gem version strings; conservatively rejects unsupported forms (markers, epochs,~=, wildcards) asUnresolved.assess_package/merge_assessments(threatdb.rs): constraint-aware assessment returningNoRecord / ExactMatch / ConstraintIntersectsAffected / ConstraintExcludesAffected / Unresolved; raw-token exact fallback handles non-semver identifiers (Docker digests, dist-tags);ConstraintExcludesAffectedrequires proven exclusion of every affected version.finalize_static_verdict(escalation.rs) + manifest parsers (ecosystem_scan.rs): all manifest paths now carry version intent; Pythonpython_requirement_specstrips extras brackets and inline comments;npm_partial_xrangeuseschecked_addto prevent overflow;ecosystem_scannow applies action and severity overrides via the new finalizer.Confidence Score: 4/5
Safe to merge with one known cosmetic gap: the ecosystem-scan path's warning for a constraint that provably overlaps malicious versions uses the same imprecise 'could not be resolved' message as a fully-unresolvable constraint, where the command path correctly says 'overlaps the affected versions'. No security bypasses or missed detections were found.
The new
assess_packagelogic, theVersionIntentconstructors,python_requirement_specinline-comment stripping,npm_partial_xrangeoverflow guard, andfinalize_static_verdictare all correct and well-tested. The one remaining gap —unresolved_dependency_findingemitting a generic message forConstraintIntersectsAffectedwhile the command path generates a more specific one — is a UX inconsistency, not a missed detection or false negative. Prior-cycle findings (gem CLIfrom_explicit_versioninstead offrom_gem_version, Gemfile multi-constraint second-spec dropped, Poetry version specs as Unspecified) are acknowledged deferred work and stay conservative (warn rather than silently pass).crates/tirith-core/src/ecosystem_scan.rs— theunresolved_dependency_findingfunction and thefindings_forblock that calls it for bothConstraintIntersectsAffectedandUnresolved.Important Files Changed
VersionIntentenum and PEP 440 constraint parser; handles edge cases (local versions, markers, wildcards, overflow) conservatively and is well-tested.assess_package/assess_package_selfwith constraint-aware assessment andmerge_assessmentsfor supplemental overlay; all precedence rules and edge cases (empty-versions, local-version base-match, raw-token fallback) are well-tested.VersionIntentthrough every ecosystem;python_requirement_speccorrectly strips extras and inline comments;npm_partial_xrangeuseschecked_addfor overflow safety. Minor:unresolved_dependency_findinguses a generic 'could not be resolved' message forConstraintIntersectsAffectedwhere a more specific description is warranted.assess_package; correct ecosystem-specific helpers used (e.g.,from_cargo_version,from_pep440_specifier). Gem CLI--versionflag still callsfrom_explicit_versionrather thanfrom_gem_version, which was flagged in a previous review cycle.finalize_static_verdictcloses the gap whereecosystem_scanignoredaction_overrides; paranoia never downgrades an override-forced Block; causal findings are re-exposed post-paranoia so a restored Block still has an explanation. Four dedicated tests cover the critical code paths.VersionIntent; OSV enrichment correctly restrictseffective_versiontoexact_version()only, preventing range text from being sent as a literal version to the OSV API.exact_version()instead ofversion.clone()for the OSV lookup, correctly preventing constraint text from being sent as a literal version string.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["Package reference\n(name + raw version string)"] --> B["VersionIntent\nconstructor"] B --> |"pip ==1.2.3"| C["Exact('1.2.3')"] B --> |"pip >=1.0,<2.0\n(parsed PEP 440)"| D["Constraint{parsed:Some}"] B --> |"cargo ^1.0\n(unparsed)"| E["Constraint{parsed:None}"] B --> |"pip install foo\n(no version)"| F["Unspecified"] B --> |"lockfile pin"| G["Resolved('1.2.3')"] C --> H["assess_package_self"] D --> H E --> H F --> H G --> H H --> |"all_versions_malicious"| I["ExactMatch → Critical/Block"] H --> |"Exact/Resolved matches DB"| I H --> |"Exact/Resolved misses DB"| J["NoRecord → no finding"] H --> |"Unspecified"| K["Unresolved → Medium/Warn"] H --> |"Constraint parsed,\noverlaps affected"| L["ConstraintIntersects → Medium/Warn"] H --> |"Constraint parsed,\nexcludes all affected"| M["ConstraintExcludes → no finding"] H --> |"Constraint unparsed,\nraw token literal match"| I H --> |"Constraint unparsed,\nno literal match"| K I --> N["finalize_static_verdict\n(overrides + paranoia)"] K --> N L --> N N --> O["EcosystemScanReport / InstallVerdict"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A["Package reference\n(name + raw version string)"] --> B["VersionIntent\nconstructor"] B --> |"pip ==1.2.3"| C["Exact('1.2.3')"] B --> |"pip >=1.0,<2.0\n(parsed PEP 440)"| D["Constraint{parsed:Some}"] B --> |"cargo ^1.0\n(unparsed)"| E["Constraint{parsed:None}"] B --> |"pip install foo\n(no version)"| F["Unspecified"] B --> |"lockfile pin"| G["Resolved('1.2.3')"] C --> H["assess_package_self"] D --> H E --> H F --> H G --> H H --> |"all_versions_malicious"| I["ExactMatch → Critical/Block"] H --> |"Exact/Resolved matches DB"| I H --> |"Exact/Resolved misses DB"| J["NoRecord → no finding"] H --> |"Unspecified"| K["Unresolved → Medium/Warn"] H --> |"Constraint parsed,\noverlaps affected"| L["ConstraintIntersects → Medium/Warn"] H --> |"Constraint parsed,\nexcludes all affected"| M["ConstraintExcludes → no finding"] H --> |"Constraint unparsed,\nraw token literal match"| I H --> |"Constraint unparsed,\nno literal match"| K I --> N["finalize_static_verdict\n(overrides + paranoia)"] K --> N L --> N N --> O["EcosystemScanReport / InstallVerdict"]Comments Outside Diff (3)
crates/tirith-core/src/threatdb.rs, line 853-860 (link)The command path now calls
assess_packageinstead ofcheck_package. For version strings thatlooks_like_plain_versionrejects — Docker digests (sha256:abcdef…), the implicit-"latest"token fromparse_docker_specs/parse_go_specs, and any other non-semver specifier —from_explicit_versionproducesConstraint { raw, parsed: None }. This arm returnsUnresolved { ConstraintUnsupported }without attempting a verbatim string comparison againstrec.versions.The old
check_packagepath usedrv == versiondirectly. For a threat DB record that is notall_versions_maliciousand whose affected version strings are non-semver (e.g., a specific Docker tag"3.15.0-alpine"or a digest), the assessment silently changes fromExactMatch→ThreatMaliciousPackage(Block/Critical) toUnresolved→ThreatUnresolvedMaliciousPackage(Warn/Medium).A minimal fix is to do a verbatim fallback before the
ConstraintUnsupportedbail-out:The existing
check_package_shim_still_matchestest validates the shim, not the newassess_packagepath, so a regression here would not be caught by the current test suite.crates/tirith-core/src/version_intent.rs, line 1409-1424 (link)from_explicit_versionaccepts pre-release tails as Exact butfrom_pep440_specifier("==...")does notlooks_like_plain_versionallows-/+tails, sofrom_explicit_version("1.2.3-beta.1")→Exact("1.2.3-beta.1")(string-matched against the DB). Butfrom_pep440_specifier("==1.2.3-beta.1")failsReleaseVersion::parseon the hyphen and falls through toConstraint { parsed: None }→Unresolvedinassess_package_self, meaning the exact string"1.2.3-beta.1"is never compared against DB records.If a malicious PyPI package's affected version is stored as
"1.2.3-beta.1"(a normalized PEP 440 pre-release would be1.2.3b1, but threat feeds occasionally store the pip-style form), a user runningpip install pkg==1.2.3-beta.1would getUnresolved(false-positive warn) when the version is not in the DB, orUnresolvedinstead ofExactMatchwhen it is — either way the string comparison is skipped. The verbatim fallback suggested onassess_package_selfwould close this gap at the same time.crates/tirith-core/src/ecosystem_scan.rs, line 104-118 (link)npm_partial_xrange— security bypass in release mode / panic in debug modemajor + 1andminor + 1are plainu64arithmetic. If apackage.jsoncontains a dependency version like"18446744073709551615"(a string that parses cleanly as a single numeric segment),npm_partial_xrangeoverflows:">=18446744073709551615.0.0,<0.0.0".<0.0.0never matches any real version, soVersionConstraint::matchesreturnsfalsefor every affected version,intersecting.is_empty()istrue, andassess_package_selfreturnsConstraintExcludesAffected— a proven-safe verdict — for a package that is actually malicious. The threat warning is completely suppressed.Use
checked_add(1)and returnNoneon overflow so the version falls back to anExactpin (which performs a correct literal/numeric string compare against the DB) rather than generating a malformed exclusion constraint.Reviews (29): Last reviewed commit: "fix(ecosystem): strip inline `#` comment..." | Re-trigger Greptile