refactor(fix): decline a wildcard only where a bare version is not a caret range - #106
Open
justin13888 wants to merge 5 commits into
Open
refactor(fix): decline a wildcard only where a bare version is not a caret range#106justin13888 wants to merge 5 commits into
justin13888 wants to merge 5 commits into
Conversation
A rewriter that replaces a constraint's version span writes the new version back bare, so whether that is safe turns on what a bare version means to the resolver. Cargo reads it as a caret range, npm as one exact release, NuGet as an inclusive minimum — three different answers that no amount of looking at the constraint string can tell apart. `Ecosystem::bare_version` records the reading per variant, decided from each resolver's own documentation and evidenced in the doc comment, with `bare_version_is_exact` as the shorthand for the sharpest case.
…caret `rewrite_constraint` declined every wildcard, because it saw only the raw constraint string and a blanket refusal was the safe call. The ecosystem was never actually out of reach: `plan` holds the manifest path, and `ManifestKind::detect` names it. With the reading threaded through, the refusal narrows to the cases that need it. Cargo's `serde = "1.*"` becomes `serde = "1.0.219"` again — a caret range with a raised floor, which is what the wildcard already was — while npm's `"1.x"`, NuGet's `1.*`, and Gradle's `1.+` stay untouched, the first because a bare version there is one release and the last two because a bare version there has no upper bound at all. The rewrite is allowed only for the wildcard shape a caret reproduces: no operator prefix, and the wildcard in the minor position. `*` admits every major and `1.2.*` stops at `1.3.0`, so both stay declined even in Cargo, and an unrecognized manifest kind is read as the most restrictive answer rather than as permission.
npm reads a version with fewer components than a full one as an X-range: `"react": "16"` is `16.x` and `"1.0"` is `1.0.x`. There is no `*` anywhere in those, so the wildcard guard saw nothing to object to and `fix` wrote `"16.14.0"` into a dependency that had been tracking a line of releases — the same harm the wildcard guard exists to prevent, reached without a wildcard. Nothing in the constraint string separates it from Cargo's `"1.0"`, where the identical rewrite is correct, so the guard is the ecosystem's reading of a bare version: declined wherever that reading is exact, kept wherever it is a caret or a minimum and raising the floor is all the rewrite does. Composer normalizes a partial to a full version and Hex and pub reject one, so declining there gives up a rewrite that would have been harmless. That is the cheaper side of the trade: the cost is one unfixed constraint that already pinned, against an author losing their range.
…e-wildcard This branch was cut before ten commits landed on its base, nine of them repairs for defects an adversarial review confirmed. Bring them forward so the ecosystem-aware wildcard work sits on the repaired base rather than the one that silently downgrades a security pin. `crates/dependable/src/fix.rs` was the only conflict, and both hunks were additive: - The imports. The base added `DependencyKind` for the override guard; this branch added `BareVersion`, `Ecosystem`, and `ManifestKind` for the ecosystem reading. Both are kept. - The test module preamble. The base opened it with `fix_all_leaves_an_override_alone`; this branch opened it with the `EVERY_ECOSYSTEM` constant. Both are kept, and the override test's `plan_fixes` call gains the `ecosystem` argument this branch added to the signature — `Some(ManifestKind::PackageJson.ecosystem())`, the reading its own fixture is written in. The override guard itself merged without conflict and is intact: `plan_fixes` still skips `DependencyKind::Override` before it reaches the target selection, so `fix --all` leaves a pnpm security pin alone.
…e-wildcard Brings the second repair round forward: the `ErrorOrigin` gate, the Go proxy 410, the override key range split, the bare PEP 440 `*`, and the undetermined/unresolved stderr notes. Conflict in `crates/dependable-fetch/src/lib.rs`: both sides widened the same `dependable_core` re-export list. Resolved as the union, keeping `BareVersion` and `ErrorOrigin`.
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
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.
Closes #92.
rewrite_constraintdeclined every wildcard constraint (from #87 / PR #89). Thecomment on that guard claimed the layer could not see the ecosystem. It could:
planalready takesmanifest: &Path, andManifestKind::detect(path).ecosystem()is two lines away. The blanket decline was conservatism, not impossibility.
What a bare version means, per ecosystem
The rewrite keeps a constraint's operator prefix and substitutes the version, so
a constraint written with no operator is written back as a bare version.
Whether that is safe is therefore a question about the ecosystem's resolver, not
about the string.
Ecosystem::bare_versionrecords the answer, withbare_version_is_exactas the shorthand for the sharpest case.Three answers turned out to be needed, not two.
bare_version_is_exactalonecannot decide this: a bare NuGet version is not exact, and NuGet must still
decline — an inclusive minimum has no upper bound, so
1.*→1.9.0widenswhere npm's
1.x→1.9.0narrows. Both are wrong; only a bare version readas a caret preserves the range.
bare_version()bare_version_is_exact()RustCaretfalseserde = "1.0"is^1.0.1.*only)GoMinimumfalserequireline is the lowest version the module needs; minimal version selection then builds with the highest such requirement in the graph.NpmExacttrue=. A partial version is an X-range instead.PythonExacttruePhpExacttrue1.0.*).DartExacttrue1.2.3as "Only the given version", and the docs steer authors to^1.2.3precisely because the bare form is that restrictive.CSharpMinimumfalse1.0isx ≥ 1.0, "minimum version, inclusive".[1.0]is how an exact match is written.ElixirExacttrueVersion.match?("2.0.1", "2.0.0")is false. Floating needs~>.JvmMinimumfalsestrictlyis the pinning form. Maven's plain<version>is likewise a soft requirement mediation may override.Two variants that disagree with the issue
The issue expected Go and Dart to join Cargo in the "safe to substitute"
bucket. Both were checked against the resolver's own documentation instead:
Exact, not a caret. pub's own version-constraint table reads abare
1.2.3as "only the given version" — that is exactly why the docs push^1.2.3. Substituting would have collapsed a pubspec range to one release.What reverses it: pub changing the traditional-syntax reading of a bare
version, which would be a breaking change to every existing pubspec.
Minimum, and unreachable either way.go.modhas no wildcardsyntax at all, so no
go.modcan reach this guard. The answer is recorded forcorrectness rather than for effect.
The rule, and why it is narrower than "not exact"
A wildcard is rewritten only when all of these hold:
Rustalone today);back is not a bare version, so the caret reading that justifies the rewrite
does not apply (
^1.x,=1.*, Python's==1.*);*,x, orX.Point 3 is not decoration. Two shapes stay declined even in Cargo, and
substituting either would have been a new bug:
*is every version; any concrete release confines it to one major — anarrowing.
1.2.*is>=1.2.0, <1.3.0, while a caret over any1.2.zreaches to<2.0.0— a widening past the bound the author wrote.1.+is a prefix range with its own resolution rules; nocaret-reading ecosystem accepts
+as a wildcard, so it is not treated as one.What survives is exactly the issue's case:
serde = "1.*"(>=1.0.0, <2.0.0)becomes
serde = "1.0.219"(>=1.0.219, <2.0.0). The floor rises, which is whatfixdoes to every constraint; the upper bound holds.ManifestKind::detect(path) == NoneDeclined.
planpassesOption<Ecosystem>, andrewrite_constraintreadsNoneasBareVersion::Exact— the reading that declines strictly more thaneither other one. An unrecognized manifest is therefore never rewritten into
something a recognized manifest would have refused. It is not an error: the
run still proceeds, and every ecosystem-independent form (
^1.0,1.0.0) isstill rewritten. Asserted by
an_unrecognized_manifest_kind_declines_every_ecosystem_dependent_form.The npm partial-version gap (the issue's "Related")
npm reads a partial version as an X-range:
"react": "16"is16.x,"1.0"is1.0.x. There is no*anywhere, sois_wildcardsaw nothing andfixwrote"16.14.0"— a pin. Nothing in the string separates it from Cargo's"1.0",where the same rewrite is correct.
Guarded now for every ecosystem whose bare version is exact, and only for
operator-free constraints —
^16is a caret range in npm too and is stillrewritten to
^16.14.0. Cargo (1.0=^1.0) and NuGet (1.0=>= 1.0) keepthe rewrite, because there it only raises a floor.
Judgement call: Composer normalizes a partial to a full version, and Hex and
pub reject one outright, so declining there gives up a rewrite that would have
been harmless. "Harmless" is the whole claim being made, though, and the trade is
one unfixed constraint that already pinned against an author silently losing
their range. What reverses it: a per-ecosystem "partial is a range" fact, if
someone wants Composer's partials fixed again.
Tests
Nothing was deleted or weakened.
rewrite_constraintgrew a third argument at47 call sites; the assertions moved, none changed meaning.
Re-expressed, not removed:
rewrite_leaves_every_concrete_version_form_rewritable— the regression net,still green, all 12 shapes intact. Each is now asserted against the ecosystem
that actually writes it (Go pseudo-versions and
+incompatibleagainstGo,NuGet's
1.0.0.4and[1.0,2.0)againstCSharp, Python's1!2.0and~=1.4against
Python, Hex's~> 1.0againstElixir), which is a stronger claimthan the single-ecosystem original: a partial-version or wildcard guard firing
on the wrong reading would take one of these with it. Python's
==1.*stillstays declined, now doubly so (exact reading and an operator prefix).
rewrite_never_narrows_a_wildcard_to_a_pin— split into two halves. Everyoriginal assertion survives: all seven shapes are asserted for every
non-caret ecosystem, and
1.+,^1.x,=1.*,1.2.x,*are asserteddeclined for Cargo too.
rewrite_preserves_operator_prefix,rewrite_skips_dist_tags,rewrite_declines_a_wildcard_wearing_a_stability_flag,rewrite_declines_a_stability_flag_on_a_plain_version,rewrite_skips_multi_constraint,rewrite_skips_space_and_pipe_compound_constraints— these guards run beforethe ecosystem is consulted (or apply to forms that are not bare versions), so
each now loops over all nine ecosystems rather than asserting against one.
That turns "ecosystem-independent" from an assumption into an assertion.
a_wildcard_dependency_is_left_untouched_by_fix(npm1.x) anda_wildcard_dependency_is_left_untouched_by_fix_all(Composer2.8.*@dev) —unchanged verdicts; both ecosystems still decline.
New:
every_ecosystem_states_how_it_reads_a_bare_version— the table above as atest, spelled out per variant so a new
Ecosystemforces the decision.the_exactness_shorthand_agrees_with_the_full_reading.rewrite_updates_a_minor_wildcard_where_a_bare_version_is_a_caret— the newbehaviour, plus the same input declined for the other eight and for
None.rewrite_declines_a_partial_version_where_a_bare_version_is_exact.a_cargo_minor_wildcard_is_updated_by_fix— end to end:serde = "1.*"isrewritten while its
clap = "1.2.*"neighbour is not, in the same file on thesame run.
the_same_wildcard_is_still_declined_for_npm— the same manifest shape, oneecosystem apart, opposite verdict.
an_npm_partial_version_is_left_untouched_by_fix—"react": "16"untouchedwhile
"vue": "^3.0.0"is rewritten beside it.an_unrecognized_manifest_kind_declines_every_ecosystem_dependent_form.API
BareVersionis new and public, re-exported fromdependable-coreanddependable-fetch. An enum rather than a second boolean besidebare_version_is_exact: two booleans that must stay consistent is the shape thatinvites exactly the silent-corruption bug this guard exists to prevent, and one
matchwith nine deliberate arms is what makes the table above reviewable.Validation
FORCE_COLORandCOLORTERMare unset for the first two because this shellexports them and three
treetests are colour-sensitive; that is a separateissue, fixed in PR #101, and is unrelated to this change.
Restacked on the repaired base
This branch was cut from
fix/stabilization-passbefore ten commits landed on it,nine of them repairs for two CRITICAL and two HIGH defects.
origin/fix/stabilization-pass(
34a5b92) is merged in ata66e581— a merge, not a rebase, so the pushedhistory is unchanged.
Conflicts
crates/dependable/src/fix.rswas the only conflicted file. Both hunks wereadditive and both sides were kept:
DependencyKindfor the override guard; this branch addedBareVersion,Ecosystem,ManifestKindfor the ecosystem reading. Kept as twouselines carrying all of them.fix_all_leaves_an_override_alone, this branch with theEVERY_ECOSYSTEMconstant. Both kept; the override test'splan_fixescall gains theecosystemargument this branch added to the signature, asSome(ManifestKind::PackageJson.ecosystem())— the reading its ownpackage.jsonfixture is written in.The override guard in
plan_fixesmerged without conflict and is intact: anitem of kind
Overrideis skipped before target selection, sofix --alldoesnot rewrite a security pin.
Gates
Both the base (925) and this branch's pre-merge count are exceeded, so no test
was lost in the resolution.
Repairs verified end to end
Run against the built binary, not inspected:
fix --allon{"pnpm":{"overrides":{"foo@2>bar":"3.0.0"}}}leaves the overrideuntouched. Worth noting the registry resolves
bar— the last arrow segment — toan unrelated package at
0.1.2, which is exactly the downgrade the guard prevents.errorand exits 0 under--fail-on vulnerable, withnote: 1 dependency was not found in its registry, so it is not gated on.version = "[4.0,4.9"reports undetermined, notup to date."overrides": {"semver": "$semver"}resolves as a reference to thesemverconstraint and exits 0.Restacked on the second repair round
fix/stabilization-passreceived ten further commits after this branch was last brought forward — repairs for four defects that its own first round of repairs had introduced. This branch now contains them: Go's410 Gonecounted as not-found alongside404; anErrorOrigin(NotFound/Unanswered/Local) carried onCheckResultfrom the typed fetch error, splittingScanIntegrityintounresolved(exempt from the gate, reported) andunevaluated(unanswerable); an override key carrying a version range no longer split on the>inside that range; a bare*in PEP 440 and Poetry translating to*rather than reading as a failed translation; a stderr note for undetermined dependencies;-qhonoured on both notes;PackageSource::Unresolvedgaining its own"unresolved"list token; andcrates/dependable/tests/cli_gate.rs, a stub HTTP registry that can answer with a chosen status code and content type.The merge was clean — no conflicted files.
Everything load-bearing across the stack survived, verified in the merged tree rather than assumed: the
DependencyKind::Overrideexclusion inplan_fixes(which deliberately records no decline note, since an override is not a refused rewrite but a version the author forced), all threehas_update()call sites,report_declined_fixesand theErrorOrigin-based gate coexisting inrunner.rs,Ecosystem::bare_version(), and translation-failure detection reachingUndetermined.Validation
The behaviours the stack must not lose are each covered by a test that ran green in the merged tree, rather than by a claim: