Skip to content

Rust: Make crate fallback logic more conservative in path resolution library #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anurag6569201 wants to merge 1 commit into
base: qa/agent-github-codeql/pr-01-22495/base
Choose a base branch
from
Open

Rust: Make crate fallback logic more conservative in path resolution library #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 7 additions & 27 deletions go/ql/lib/semmle/go/dependencies/SemVer.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ overlay[local?]
module;

import semmle.go.dependencies.Dependencies
private import codeql.util.SemVer

/**
* A SemVer-formatted version string in a dependency.
Expand All @@ -17,63 +18,42 @@ class DependencySemVer extends string {

DependencySemVer() {
this = dep.getDepVersion() and
normalized = normalizeSemver(this)
normalized = padSemVer(this)
}

/**
* Holds if this version may be before `last`.
*/
bindingset[last]
predicate maybeBefore(string last) { normalized < normalizeSemver(last) }
predicate maybeBefore(string last) { normalized < padSemVer(last) }

/**
* Holds if this version may be after `first`.
*/
bindingset[first]
predicate maybeAfter(string first) { normalizeSemver(first) < normalized }
predicate maybeAfter(string first) { padSemVer(first) < normalized }

/**
* Holds if this version may be between `first` (inclusive) and `last` (exclusive).
*/
bindingset[first, last]
predicate maybeBetween(string first, string last) {
normalizeSemver(first) <= normalized and
normalized < normalizeSemver(last)
padSemVer(first) <= normalized and
normalized < padSemVer(last)
}

/**
* Holds if this version is equivalent to `other`.
*/
bindingset[other]
predicate is(string other) { normalized = normalizeSemver(other) }
predicate is(string other) { normalized = padSemVer(other) }

/**
* Gets the dependency that uses this string.
*/
Dependency getDependency() { result = dep }
}

bindingset[str]
private string leftPad(string str) { result = ("000" + str).suffix(str.length()) }

/**
* Normalizes a SemVer string such that the lexicographical ordering
* of two normalized strings is consistent with the SemVer ordering.
*
* Pre-release information and build metadata is not yet supported.
*/
bindingset[orig]
private string normalizeSemver(string orig) {
exists(string pattern, string major, string minor, string patch |
pattern = "v?(\\d+)\\.(\\d+)\\.(\\d+)(\\D.*)?" and
major = orig.regexpCapture(pattern, 1) and
minor = orig.regexpCapture(pattern, 2) and
patch = orig.regexpCapture(pattern, 3)
|
result = leftPad(major) + "." + leftPad(minor) + "." + leftPad(patch)
)
}

/**
* A version string in a dependency that has a SemVer, but also contains a git commit SHA.
*
Expand Down
34 changes: 7 additions & 27 deletions javascript/ql/lib/semmle/javascript/dependencies/SemVer.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/

import semmle.javascript.dependencies.Dependencies
private import codeql.util.SemVer

/**
* A SemVer-formatted version string in a dependency.
Expand All @@ -15,59 +16,38 @@ class DependencySemVer extends string {

DependencySemVer() {
dep.info(_, this) and
normalized = normalizeSemver(this)
normalized = padSemVer(this)
}

/**
* Holds if this version may be before `last`.
*/
bindingset[last]
predicate maybeBefore(string last) { normalized < normalizeSemver(last) }
predicate maybeBefore(string last) { normalized < padSemVer(last) }

/**
* Holds if this version may be after `first`.
*/
bindingset[first]
predicate maybeAfter(string first) { normalizeSemver(first) < normalized }
predicate maybeAfter(string first) { padSemVer(first) < normalized }

/**
* Holds if this version may be between `first` (inclusive) and `last` (exclusive).
*/
bindingset[first, last]
predicate maybeBetween(string first, string last) {
normalizeSemver(first) <= normalized and
normalized < normalizeSemver(last)
padSemVer(first) <= normalized and
normalized < padSemVer(last)
}

/**
* Holds if this version is equivalent to `other`.
*/
bindingset[other]
predicate is(string other) { normalized = normalizeSemver(other) }
predicate is(string other) { normalized = padSemVer(other) }

/**
* Gets the dependency that uses this string.
*/
Dependency getDependency() { result = dep }
}

bindingset[str]
private string leftPad(string str) { result = ("000" + str).suffix(str.length()) }

/**
* Normalizes a SemVer string such that the lexicographical ordering
* of two normalized strings is consistent with the SemVer ordering.
*
* Pre-release information and build metadata is not yet supported.
*/
bindingset[orig]
private string normalizeSemver(string orig) {
exists(string pattern, string major, string minor, string patch |
pattern = "(\\d+)\\.(\\d+)\\.(\\d+)" and
major = orig.regexpCapture(pattern, 1) and
minor = orig.regexpCapture(pattern, 2) and
patch = orig.regexpCapture(pattern, 3)
|
result = leftPad(major) + "." + leftPad(minor) + "." + leftPad(patch)
)
}
50 changes: 6 additions & 44 deletions ruby/ql/lib/codeql/ruby/frameworks/Gemfile.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*/

private import codeql.ruby.AST
private import codeql.util.SemVer

/**
* Provides classes and predicates for Gemfiles, including version constraint logic.
Expand Down Expand Up @@ -138,7 +139,7 @@ module Gemfile {
exists(int thisMajor, int thisMinor, int otherMajor, int otherMinor |
thisMajor = this.getVersion().getMajor() and
thisMinor = this.getVersion().getMinor() and
exists(string maj, string mi | normalizeSemver(other, _, maj, mi, _) |
exists(string maj, string mi | exists(padSemVer(other, maj, mi, _)) |
otherMajor = maj.toInt() and otherMinor = mi.toInt()
)
|
Expand Down Expand Up @@ -171,26 +172,26 @@ module Gemfile {

Version() {
this = any(Gem c).getAVersionConstraint().getVersionString() and
normalized = normalizeSemver(this)
normalized = padSemVer(this)
}

/**
* Holds if this version is strictly before the version defined by `other`.
*/
bindingset[other]
predicate before(string other) { normalized < normalizeSemver(other) }
predicate before(string other) { normalized < padSemVer(other) }

/**
* Holds if this versino is equal to the version defined by `other`.
*/
bindingset[other]
predicate equal(string other) { normalized = normalizeSemver(other) }
predicate equal(string other) { normalized = padSemVer(other) }

/**
* Holds if this version is strictly after the version defined by `other`.
*/
bindingset[other]
predicate after(string other) { normalized > normalizeSemver(other) }
predicate after(string other) { normalized > padSemVer(other) }

/**
* Holds if this version defines a patch number.
Expand All @@ -212,43 +213,4 @@ module Gemfile {
*/
int getPatch() { result = getPatch(normalized).toInt() }
}

/**
* Normalizes a SemVer string such that the lexicographical ordering
* of two normalized strings is consistent with the SemVer ordering.
*
* Pre-release information and build metadata is not supported.
*/
bindingset[orig]
private predicate normalizeSemver(
string orig, string normalized, string major, string minor, string patch
) {
major = getMajor(orig) and
(
minor = getMinor(orig)
or
not exists(getMinor(orig)) and minor = "0"
) and
(
patch = getPatch(orig)
or
not exists(getPatch(orig)) and patch = "0"
) and
normalized = leftPad(major) + "." + leftPad(minor) + "." + leftPad(patch)
}

bindingset[orig]
private string normalizeSemver(string orig) { normalizeSemver(orig, result, _, _, _) }

bindingset[s]
private string getMajor(string s) { result = s.regexpCapture("(\\d+).*", 1) }

bindingset[s]
private string getMinor(string s) { result = s.regexpCapture("(\\d+)\\.(\\d+).*", 2) }

bindingset[s]
private string getPatch(string s) { result = s.regexpCapture("(\\d+)\\.(\\d+)\\.(\\d+).*", 3) }

bindingset[str]
private string leftPad(string str) { result = ("000" + str).suffix(str.length()) }
}
26 changes: 21 additions & 5 deletions rust/ql/lib/codeql/rust/internal/PathResolution.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ private import codeql.rust.elements.internal.CallExprImpl::Impl as CallExprImpl
private import codeql.rust.internal.CachedStages
private import codeql.rust.frameworks.stdlib.Builtins as Builtins
private import codeql.util.Option
private import codeql.util.SemVer

private newtype TNamespace =
TTypeNamespace() or
Expand Down Expand Up @@ -568,6 +569,16 @@ class CrateItemNode extends NamedItemNode instanceof Crate {
)
}

pragma[nomagic]
predicate isLatestVersion(string name) {
this =
max(CrateItemNode c, string ver |
name = c.getName() and ver = padSemVer(c.(Crate).getVersion())
|
c order by ver
)
}

override string getName() { result = Crate.super.getName() }

override Namespace getNamespace() {
Expand Down Expand Up @@ -1529,11 +1540,11 @@ private predicate crateDependencyEdge(SourceFileItemNode file, string name, Crat
crateDependency(file, name, dep)
or
// As a fallback, give all files access to crates that do not conflict with known dependencies
// and declarations. This is in order to workaround incomplete crate dependency information
// provided by the extractor, as well as `CrateItemNode.getASourceFile()` being unable to map
// a given file to its crate (for example, if the file is `mod` imported inside a macro that the
// extractor is unable to expand).
name = dep.getName() and
// and declarations, as long as those crates have a unique latest version.
// This is in order to workaround incomplete crate dependency information provided by the extractor,
// as well as `CrateItemNode.getASourceFile()` being unable to map a given file to its crate (for
// example, if the file is `mod` imported inside a macro that the extractor is unable to expand).
dep = unique(CrateItemNode dep0 | dep0.isLatestVersion(name)) and
not hasDeclOrDep(file, name)
}

Expand Down Expand Up @@ -2385,6 +2396,11 @@ private module Debug {
useImportEdge(use, name, item, kind)
}

predicate debugCrateDependencyEdge(SourceFileItemNode file, string name, CrateItemNode dep) {
file = getRelevantLocatable() and
crateDependencyEdge(file, name, dep)
}

ItemNode debugGetASuccessor(ItemNode i, string name, SuccessorKind kind) {
i = getRelevantLocatable() and
result = i.getASuccessor(name, kind, _)
Expand Down
57 changes: 57 additions & 0 deletions shared/util/codeql/util/SemVer.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/**
* Provides logic for working SemVer (Semantic Versioning).
*/
overlay[local?]
module;

bindingset[str]
private string leftPad(string str) { result = ("0000" + str).suffix(str.length()) }

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · CRITICAL

The shared leftPad changed from 3-digit to 4-digit padding.

Impact: The shared leftPad changed from 3-digit to 4-digit padding. This alters lexicographic ordering for version components with 4+ digits. For example, 1.2.1000 vs 1.2.999 now compares differently than the previous Go/Ruby implementations, producing incorrect maybeBefore/maybeAfter/maybeBetween and before/equal/after results.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The new shared 'leftPad' uses '("0000" + str).suffix(str.length())', which pads to 4 digits, while the removed Go/Ruby implementations used '("000" + str).suffix(str.length())', pa

Impact: The new shared 'leftPad' uses '("0000" + str).suffix(str.length())', which pads to 4 digits, while the removed Go/Ruby implementations used '("000" + str).suffix(str.length())', padding to 3 digits. This changes normalized version ordering for versions with 4+ digit components. For example, '1.2.1000' and '1.2.999' now compare differently than before, potentially producing incorrect 'maybeBefore'/'maybeAfter'/'maybe…

Suggested fix: Fix the review finding before release.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The new 'padSemVer' uses 'leftPad' with 4-digit padding, but the removed Ruby 'normalizeSemver' used 3-digit padding.

Impact: The new 'padSemVer' uses 'leftPad' with 4-digit padding, but the removed Ruby 'normalizeSemver' used 3-digit padding. This changes the normalized representation for Ruby Gemfile version comparisons, potentially altering 'before', 'equal', and 'after' results for versions with 4+ digit components.

Suggested fix: Fix the review finding before release.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The shared SemVer utility silently changes behavior across Go, JavaScript, and Ruby by mixing previously different regex strictness and padding widths.

Impact: The shared SemVer utility silently changes behavior across Go, JavaScript, and Ruby by mixing previously different regex strictness and padding widths. There is no comment or test evidence documenting that 4-digit padding and optional 'v' are intentional, making it hard for a newcomer to understand whether the behavioral changes are deliberate or accidental.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.


/**
* Gets the major number of a SemVer string.
*/
bindingset[s]
string getMajor(string s) { result = s.regexpCapture("v?(\\d+).*", 1) }

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · CRITICAL

The new padSemVer regexes use '.*' after numeric components, so getMajor can match strings that are not valid SemVer, such as '123abc'.

Impact: The new padSemVer regexes use '.*' after numeric components, so getMajor can match strings that are not valid SemVer, such as '123abc'. The removed Go and JavaScript implementations required a full SemVer pattern. This loosening can cause non-SemVer strings to be normalized and compared, producing incorrect version ordering results.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · HIGH

The new shared padSemVer accepts an optional leading 'v' in getMajor/getMinor/getPatch.

Impact: The new shared padSemVer accepts an optional leading 'v' in getMajor/getMinor/getPatch. The removed JavaScript and Ruby implementations did not accept a leading 'v'. JavaScript dependency versions such as 'v1.2.3' that previously did not normalize now normalize and participate in version comparisons, changing query results for JavaScript and Ruby dependencies.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · MEDIUM

The new 'padSemVer' regexes in 'getMajor', 'getMinor', and 'getPatch' accept an optional leading 'v' ('v?(\d+)...').

Impact: The new 'padSemVer' regexes in 'getMajor', 'getMinor', and 'getPatch' accept an optional leading 'v' ('v?(\d+)...'). The removed Go 'normalizeSemver' also accepted 'v?', but the removed JavaScript 'normalizeSemver' did not. JavaScript dependency versions such as 'v1.2.3' that previously did not normalize now normalize and participate in version comparisons, changing query results for JavaScript dependencies.

Suggested fix: Fix the review finding before release.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · MEDIUM

The new 'padSemVer' regexes use '.*' after the numeric component, so 'getMajor' can match a string that starts with digits but is not a valid SemVer (e.g., '123abc').

Impact: The new 'padSemVer' regexes use '.' after the numeric component, so 'getMajor' can match a string that starts with digits but is not a valid SemVer (e.g., '123abc'). The removed Go implementation required a full SemVer pattern 'v?(\d+)\.(\d+)\.(\d+)(\D.)?', and the removed JavaScript implementation required '(\d+)\.(\d+)\.(\d+)'. This loosening can cause non-SemVer strings to be normalized and compared,…

Suggested fix: Fix the review finding before release.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · MEDIUM

The new 'padSemVer' regexes accept an optional leading 'v', but the removed Ruby 'getMajor', 'getMinor', and 'getPatch' did not.

Impact: The new 'padSemVer' regexes accept an optional leading 'v', but the removed Ruby 'getMajor', 'getMinor', and 'getPatch' did not. Ruby Gemfile version strings such as 'v1.2.3' that previously did not normalize now normalize and participate in version comparisons, changing query results for Ruby dependencies.

Suggested fix: Fix the review finding before release.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · MEDIUM

The new 'padSemVer' regexes use '.*' after the numeric component, so 'getMajor' can match a string that starts with digits but is not a valid SemVer.

Impact: The new 'padSemVer' regexes use '.' after the numeric component, so 'getMajor' can match a string that starts with digits but is not a valid SemVer. The removed Go 'normalizeSemver' required a full SemVer pattern 'v?(\d+)\.(\d+)\.(\d+)(\D.)?'. This loosening can cause non-SemVer strings to be normalized and compared, producing incorrect version ordering results.

Suggested fix: Fix the review finding before release.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · MEDIUM

The new 'padSemVer' regexes accept an optional leading 'v', but the removed JavaScript 'normalizeSemver' did not.

Impact: The new 'padSemVer' regexes accept an optional leading 'v', but the removed JavaScript 'normalizeSemver' did not. JavaScript dependency versions such as 'v1.2.3' that previously did not normalize now normalize and participate in version comparisons, changing query results for JavaScript dependencies.

Suggested fix: Fix the review finding before release.

@shipwright-agent shipwright-agent Bot Sep 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shipwright · MEDIUM

The new 'padSemVer' regexes accept an optional leading 'v', but the removed Go 'normalizeSemver' also accepted 'v?', so this is not a new defect for Go.

Impact: The new 'padSemVer' regexes accept an optional leading 'v', but the removed Go 'normalizeSemver' also accepted 'v?', so this is not a new defect for Go. However, the shared implementation now applies this looser matching to JavaScript and Ruby, which previously did not accept a leading 'v'. This can cause non-SemVer strings to be normalized and compared, producing incorrect version ordering results.

Suggested fix: Fix the review finding before release.

/**
* Gets the minor number of a SemVer string.
*/
bindingset[s]
string getMinor(string s) { result = s.regexpCapture("v?(\\d+)\\.(\\d+).*", 2) }

/**
* Gets the patch number of a SemVer string.
*/
bindingset[s]
string getPatch(string s) { result = s.regexpCapture("v?(\\d+)\\.(\\d+)\\.(\\d+).*", 3) }

/**
* Normalizes a SemVer string such that the lexicographical ordering
* of two normalized strings is consistent with the SemVer ordering.
*
* Pre-release information and build metadata is not yet supported.
*/
bindingset[orig]
string padSemVer(string orig, string major, string minor, string patch) {
major = getMajor(orig) and
(
minor = getMinor(orig)
or
not exists(getMinor(orig)) and minor = "0"
) and
(
patch = getPatch(orig)
or
not exists(getPatch(orig)) and patch = "0"
) and
result = leftPad(major) + "." + leftPad(minor) + "." + leftPad(patch)
}

/**
* Normalizes a SemVer string such that the lexicographical ordering
* of two normalized strings is consistent with the SemVer ordering.
*
* Pre-release information and build metadata is not yet supported.
*/
bindingset[orig]
string padSemVer(string orig) { result = padSemVer(orig, _, _, _) }