Copilot commented on code in PR #255:
URL: https://github.com/apache/skywalking-eyes/pull/255#discussion_r2636590290
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +416,193 @@ func runtimeDepsFromGemspecs(dir string) ([]string,
error) {
return res, nil
}
+func parseGemspecDependencies(path string) ([]string, error) {
+ f, err := os.Open(path)
+ if err != nil {
+ return nil, err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ var deps []string
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ deps = append(deps, m[1])
+ }
+ }
+ return deps, scanner.Err()
+}
+
+func findInstalledGemspec(name, version string) (string, error) {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ if version != "" && rubyVersionRe.MatchString(version) {
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, err := os.Stat(path); err == nil {
+ return path, nil
+ }
+ } else {
+ entries, err := os.ReadDir(specsDir)
+ if err != nil {
+ continue
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasPrefix(e.Name(),
name+"-") || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ // Ensure that the character immediately after
the "name-" prefix
+ // is a digit, so we only consider filenames
where the suffix is
+ // a version component (e.g.,
"foo-1.0.0.gemspec") and avoid
+ // similar names like "foo-bar-1.0.0.gemspec"
when searching for "foo".
+ if len(stem) <= len(name)+1 {
+ continue
+ }
+ versionStart := stem[len(name)+1]
+ if versionStart < '0' || versionStart > '9' {
+ continue
+ }
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem {
+ continue
+ }
+ path := filepath.Join(specsDir, e.Name())
+ if specName, _, err := parseGemspecInfo(path);
err == nil && specName == name {
+ return path, nil
+ }
+ }
+ }
+ }
+ return "", os.ErrNotExist
+}
+
+func fetchLocalLicense(dir, targetName string) (string, error) {
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ return "", err
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ path := filepath.Join(dir, e.Name())
+ specName, license, err := parseGemspecInfo(path)
+ if err == nil && specName == targetName && license != "" {
+ return license, nil
+ }
+ }
+ return "", nil
+}
+
+func fetchInstalledLicense(name, version string) string {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ // If version is specific
+ if version != "" && rubyVersionRe.MatchString(version) {
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, license, err := parseGemspecInfo(path); err ==
nil && license != "" {
+ return license
+ }
+ } else {
+ // Scan for any version
+ entries, err := os.ReadDir(specsDir)
+ if err != nil {
+ continue
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasPrefix(e.Name(),
name+"-") || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem { // didn't have prefix
+ continue
+ }
Review Comment:
The condition `if ver == stem` on line 525 will never be true because the
filename has already been validated to have the prefix `name+"-"` on line 520.
This redundant check can be removed for clarity. The subsequent check on line
529 already handles the case where the version doesn't start with a digit,
which is the actual validation needed.
```suggestion
```
##########
pkg/deps/ruby.go:
##########
@@ -116,12 +143,109 @@ func (r *GemfileLockResolver) Resolve(lockfile string,
config *ConfigDeps, repor
return nil
}
+// GemspecResolver resolves dependencies from a .gemspec file.
+// It extracts runtime dependencies defined in the gemspec and recursively
resolves
+// their transitive dependencies by looking up installed gems in the local
environment.
+type GemspecResolver struct {
+ Resolver
+}
+
+// CanResolve checks if the given file is a .gemspec file.
+func (r *GemspecResolver) CanResolve(file string) bool {
+ return strings.HasSuffix(file, ".gemspec")
+}
+
+// Resolve parses the gemspec file, identifies runtime dependencies, and
resolves
+// them along with their transitive dependencies. It reports the found
dependencies
+// and their licenses.
+func (r *GemspecResolver) Resolve(file string, config *ConfigDeps, report
*Report) error {
+ f, err := os.Open(file)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ deps := make(map[string]string) // name -> version constraint
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ // NOTE: Version constraints are currently ignored. We
resolve to the first found installed version of the gem.
+ // This may lead to incorrect resolution if multiple
versions are installed and the first one doesn't satisfy the constraint.
+ deps[m[1]] = ""
+ }
+ }
+ if err := scanner.Err(); err != nil {
+ return err
+ }
+
+ // Recursive resolution
+ queue := make([]string, 0, len(deps))
+ visited := make(map[string]struct{}, len(deps))
+ for name := range deps {
+ queue = append(queue, name)
+ visited[name] = struct{}{}
+ }
+
+ for i := 0; i < len(queue); i++ {
+ name := queue[i]
+ // Find installed gemspec for 'name'
+ path, err := findInstalledGemspec(name, "")
+ if err == nil && path != "" {
+ // Parse dependencies of this gemspec
+ newDeps, err := parseGemspecDependencies(path)
+ if err == nil {
+ for _, dep := range newDeps {
+ if _, ok := visited[dep]; !ok {
+ if len(queue) > 10000 {
Review Comment:
The recursive dependency resolution uses a simple length check to prevent
infinite loops, but checking `len(queue) > 10000` after adding to the queue
means the queue could reach 10001 elements. The check should be performed
before adding to prevent exceeding the limit, or the error message should
reflect that the limit may be slightly exceeded.
```suggestion
if len(queue) >= 10000 {
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +416,193 @@ func runtimeDepsFromGemspecs(dir string) ([]string,
error) {
return res, nil
}
+func parseGemspecDependencies(path string) ([]string, error) {
+ f, err := os.Open(path)
+ if err != nil {
+ return nil, err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ var deps []string
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ deps = append(deps, m[1])
+ }
+ }
+ return deps, scanner.Err()
+}
+
+func findInstalledGemspec(name, version string) (string, error) {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ if version != "" && rubyVersionRe.MatchString(version) {
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, err := os.Stat(path); err == nil {
+ return path, nil
+ }
+ } else {
+ entries, err := os.ReadDir(specsDir)
+ if err != nil {
+ continue
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasPrefix(e.Name(),
name+"-") || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ // Ensure that the character immediately after
the "name-" prefix
+ // is a digit, so we only consider filenames
where the suffix is
+ // a version component (e.g.,
"foo-1.0.0.gemspec") and avoid
+ // similar names like "foo-bar-1.0.0.gemspec"
when searching for "foo".
+ if len(stem) <= len(name)+1 {
+ continue
+ }
+ versionStart := stem[len(name)+1]
+ if versionStart < '0' || versionStart > '9' {
+ continue
+ }
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem {
+ continue
+ }
Review Comment:
The condition `if ver == stem` on line 472 will never be true because the
function already verified that the filename has the prefix `name+"-"` earlier
(line 456), and the subsequent checks ensure the prefix is valid. This is dead
code that can be removed to improve code clarity.
```suggestion
```
##########
pkg/deps/ruby.go:
##########
@@ -116,12 +143,109 @@ func (r *GemfileLockResolver) Resolve(lockfile string,
config *ConfigDeps, repor
return nil
}
+// GemspecResolver resolves dependencies from a .gemspec file.
+// It extracts runtime dependencies defined in the gemspec and recursively
resolves
+// their transitive dependencies by looking up installed gems in the local
environment.
+type GemspecResolver struct {
+ Resolver
+}
+
+// CanResolve checks if the given file is a .gemspec file.
+func (r *GemspecResolver) CanResolve(file string) bool {
+ return strings.HasSuffix(file, ".gemspec")
+}
+
+// Resolve parses the gemspec file, identifies runtime dependencies, and
resolves
+// them along with their transitive dependencies. It reports the found
dependencies
+// and their licenses.
+func (r *GemspecResolver) Resolve(file string, config *ConfigDeps, report
*Report) error {
+ f, err := os.Open(file)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ deps := make(map[string]string) // name -> version constraint
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ // NOTE: Version constraints are currently ignored. We
resolve to the first found installed version of the gem.
+ // This may lead to incorrect resolution if multiple
versions are installed and the first one doesn't satisfy the constraint.
+ deps[m[1]] = ""
+ }
+ }
+ if err := scanner.Err(); err != nil {
+ return err
+ }
+
+ // Recursive resolution
+ queue := make([]string, 0, len(deps))
+ visited := make(map[string]struct{}, len(deps))
+ for name := range deps {
+ queue = append(queue, name)
+ visited[name] = struct{}{}
+ }
+
+ for i := 0; i < len(queue); i++ {
+ name := queue[i]
+ // Find installed gemspec for 'name'
+ path, err := findInstalledGemspec(name, "")
+ if err == nil && path != "" {
+ // Parse dependencies of this gemspec
+ newDeps, err := parseGemspecDependencies(path)
+ if err == nil {
+ for _, dep := range newDeps {
+ if _, ok := visited[dep]; !ok {
+ if len(queue) > 10000 {
+ return
fmt.Errorf("dependency graph too large")
Review Comment:
The error message "dependency graph too large" is vague and doesn't provide
actionable information to the user. Consider enhancing the message to include
the current queue size and suggest potential causes, such as: "dependency graph
exceeded maximum size of 10000 nodes (current: %d). This may indicate a
circular dependency or an unusually large dependency tree."
```suggestion
return
fmt.Errorf("dependency graph exceeded maximum size of 10000 nodes (current:
%d). This may indicate a circular dependency or an unusually large dependency
tree", len(queue))
```
##########
pkg/deps/ruby.go:
##########
@@ -116,12 +143,109 @@ func (r *GemfileLockResolver) Resolve(lockfile string,
config *ConfigDeps, repor
return nil
}
+// GemspecResolver resolves dependencies from a .gemspec file.
+// It extracts runtime dependencies defined in the gemspec and recursively
resolves
+// their transitive dependencies by looking up installed gems in the local
environment.
+type GemspecResolver struct {
+ Resolver
+}
+
+// CanResolve checks if the given file is a .gemspec file.
+func (r *GemspecResolver) CanResolve(file string) bool {
+ return strings.HasSuffix(file, ".gemspec")
+}
+
+// Resolve parses the gemspec file, identifies runtime dependencies, and
resolves
+// them along with their transitive dependencies. It reports the found
dependencies
+// and their licenses.
+func (r *GemspecResolver) Resolve(file string, config *ConfigDeps, report
*Report) error {
+ f, err := os.Open(file)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ deps := make(map[string]string) // name -> version constraint
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ // NOTE: Version constraints are currently ignored. We
resolve to the first found installed version of the gem.
+ // This may lead to incorrect resolution if multiple
versions are installed and the first one doesn't satisfy the constraint.
+ deps[m[1]] = ""
+ }
+ }
+ if err := scanner.Err(); err != nil {
+ return err
+ }
+
+ // Recursive resolution
+ queue := make([]string, 0, len(deps))
+ visited := make(map[string]struct{}, len(deps))
+ for name := range deps {
+ queue = append(queue, name)
+ visited[name] = struct{}{}
+ }
+
+ for i := 0; i < len(queue); i++ {
+ name := queue[i]
+ // Find installed gemspec for 'name'
+ path, err := findInstalledGemspec(name, "")
+ if err == nil && path != "" {
+ // Parse dependencies of this gemspec
+ newDeps, err := parseGemspecDependencies(path)
+ if err == nil {
+ for _, dep := range newDeps {
+ if _, ok := visited[dep]; !ok {
+ if len(queue) > 10000 {
+ return
fmt.Errorf("dependency graph too large")
+ }
+ visited[dep] = struct{}{}
+ queue = append(queue, dep)
+ if _, ok := deps[dep]; !ok {
+ deps[dep] = ""
+ }
+ }
+ }
Review Comment:
In the BFS loop for recursive dependency resolution, when
`findInstalledGemspec` or `parseGemspecDependencies` returns an error, the
error is silently ignored (lines 197-215). This means if there's an issue
reading a gemspec file (e.g., permission denied, corrupted file), the
transitive dependencies from that gem will be silently skipped without any
logging or warning. Consider adding debug logging when these operations fail to
help with troubleshooting incomplete dependency graphs.
```suggestion
if err != nil {
logrus.WithError(err).Debugf("failed to find installed
gemspec for gem %q", name)
continue
}
if path == "" {
logrus.Debugf("no installed gemspec found for gem %q",
name)
continue
}
// Parse dependencies of this gemspec
newDeps, err := parseGemspecDependencies(path)
if err != nil {
logrus.WithError(err).Debugf("failed to parse gemspec
dependencies from %q for gem %q", path, name)
continue
}
for _, dep := range newDeps {
if _, ok := visited[dep]; !ok {
if len(queue) > 10000 {
return fmt.Errorf("dependency graph too
large")
}
visited[dep] = struct{}{}
queue = append(queue, dep)
if _, ok := deps[dep]; !ok {
deps[dep] = ""
}
```
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +416,193 @@ func runtimeDepsFromGemspecs(dir string) ([]string,
error) {
return res, nil
}
+func parseGemspecDependencies(path string) ([]string, error) {
+ f, err := os.Open(path)
+ if err != nil {
+ return nil, err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ var deps []string
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ deps = append(deps, m[1])
+ }
+ }
+ return deps, scanner.Err()
+}
+
+func findInstalledGemspec(name, version string) (string, error) {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ if version != "" && rubyVersionRe.MatchString(version) {
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, err := os.Stat(path); err == nil {
+ return path, nil
+ }
+ } else {
+ entries, err := os.ReadDir(specsDir)
+ if err != nil {
+ continue
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasPrefix(e.Name(),
name+"-") || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ // Ensure that the character immediately after
the "name-" prefix
+ // is a digit, so we only consider filenames
where the suffix is
+ // a version component (e.g.,
"foo-1.0.0.gemspec") and avoid
+ // similar names like "foo-bar-1.0.0.gemspec"
when searching for "foo".
+ if len(stem) <= len(name)+1 {
+ continue
+ }
+ versionStart := stem[len(name)+1]
+ if versionStart < '0' || versionStart > '9' {
+ continue
+ }
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem {
+ continue
+ }
+ path := filepath.Join(specsDir, e.Name())
+ if specName, _, err := parseGemspecInfo(path);
err == nil && specName == name {
+ return path, nil
+ }
+ }
+ }
+ }
+ return "", os.ErrNotExist
+}
+
+func fetchLocalLicense(dir, targetName string) (string, error) {
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ return "", err
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ path := filepath.Join(dir, e.Name())
+ specName, license, err := parseGemspecInfo(path)
+ if err == nil && specName == targetName && license != "" {
+ return license, nil
+ }
+ }
+ return "", nil
+}
+
+func fetchInstalledLicense(name, version string) string {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ // If version is specific
+ if version != "" && rubyVersionRe.MatchString(version) {
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, license, err := parseGemspecInfo(path); err ==
nil && license != "" {
+ return license
+ }
+ } else {
+ // Scan for any version
+ entries, err := os.ReadDir(specsDir)
+ if err != nil {
+ continue
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasPrefix(e.Name(),
name+"-") || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem { // didn't have prefix
+ continue
+ }
+ // Ensure the character after the gem name
corresponds to the start of a version
+ if ver == "" || ver[0] < '0' || ver[0] > '9' {
+ continue
+ }
+ path := filepath.Join(specsDir, e.Name())
+ if specName, license, err :=
parseGemspecInfo(path); err == nil && specName == name && license != "" {
+ return license
+ }
+ }
Review Comment:
The `fetchInstalledLicense` function performs multiple directory scans when
a version is not specified. For each gem path in `GEM_PATH`, it reads all
entries in the specifications directory and iterates through them (lines
515-536). If `GEM_PATH` contains multiple directories with many gems, this
could lead to significant I/O overhead. Consider caching the results or using a
more efficient lookup strategy, such as building an index of available gems on
first access.
##########
pkg/deps/ruby.go:
##########
@@ -253,6 +416,193 @@ func runtimeDepsFromGemspecs(dir string) ([]string,
error) {
return res, nil
}
+func parseGemspecDependencies(path string) ([]string, error) {
+ f, err := os.Open(path)
+ if err != nil {
+ return nil, err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ var deps []string
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ deps = append(deps, m[1])
+ }
+ }
+ return deps, scanner.Err()
+}
+
+func findInstalledGemspec(name, version string) (string, error) {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ if version != "" && rubyVersionRe.MatchString(version) {
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, err := os.Stat(path); err == nil {
+ return path, nil
+ }
+ } else {
+ entries, err := os.ReadDir(specsDir)
+ if err != nil {
+ continue
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasPrefix(e.Name(),
name+"-") || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ // Ensure that the character immediately after
the "name-" prefix
+ // is a digit, so we only consider filenames
where the suffix is
+ // a version component (e.g.,
"foo-1.0.0.gemspec") and avoid
+ // similar names like "foo-bar-1.0.0.gemspec"
when searching for "foo".
+ if len(stem) <= len(name)+1 {
+ continue
+ }
+ versionStart := stem[len(name)+1]
+ if versionStart < '0' || versionStart > '9' {
+ continue
+ }
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem {
+ continue
+ }
+ path := filepath.Join(specsDir, e.Name())
+ if specName, _, err := parseGemspecInfo(path);
err == nil && specName == name {
+ return path, nil
+ }
+ }
+ }
+ }
+ return "", os.ErrNotExist
+}
+
+func fetchLocalLicense(dir, targetName string) (string, error) {
+ entries, err := os.ReadDir(dir)
+ if err != nil {
+ return "", err
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ path := filepath.Join(dir, e.Name())
+ specName, license, err := parseGemspecInfo(path)
+ if err == nil && specName == targetName && license != "" {
+ return license, nil
+ }
+ }
+ return "", nil
+}
+
+func fetchInstalledLicense(name, version string) string {
+ gemPaths := getGemPaths()
+ for _, dir := range gemPaths {
+ specsDir := filepath.Join(dir, "specifications")
+ // If version is specific
+ if version != "" && rubyVersionRe.MatchString(version) {
+ path := filepath.Join(specsDir,
name+"-"+version+".gemspec")
+ if _, license, err := parseGemspecInfo(path); err ==
nil && license != "" {
+ return license
+ }
+ } else {
+ // Scan for any version
+ entries, err := os.ReadDir(specsDir)
+ if err != nil {
+ continue
+ }
+ for _, e := range entries {
+ if e.IsDir() || !strings.HasPrefix(e.Name(),
name+"-") || !strings.HasSuffix(e.Name(), ".gemspec") {
+ continue
+ }
+ stem := strings.TrimSuffix(e.Name(), ".gemspec")
+ ver := strings.TrimPrefix(stem, name+"-")
+ if ver == stem { // didn't have prefix
+ continue
+ }
+ // Ensure the character after the gem name
corresponds to the start of a version
+ if ver == "" || ver[0] < '0' || ver[0] > '9' {
+ continue
+ }
+ path := filepath.Join(specsDir, e.Name())
+ if specName, license, err :=
parseGemspecInfo(path); err == nil && specName == name && license != "" {
+ return license
+ }
+ }
+ }
+ }
+ return ""
+}
Review Comment:
The new `fetchInstalledLicense` function is called in both resolvers but is
not directly tested in unit tests. While it's exercised through integration
tests, adding specific test cases for this function would improve coverage and
make it easier to verify edge cases like:
- Multiple versions of the same gem installed
- Invalid version strings
- Missing gemspec files
- Gems with similar names (e.g., "foo" vs "foo-bar")
##########
pkg/deps/ruby.go:
##########
@@ -116,12 +143,109 @@ func (r *GemfileLockResolver) Resolve(lockfile string,
config *ConfigDeps, repor
return nil
}
+// GemspecResolver resolves dependencies from a .gemspec file.
+// It extracts runtime dependencies defined in the gemspec and recursively
resolves
+// their transitive dependencies by looking up installed gems in the local
environment.
+type GemspecResolver struct {
+ Resolver
+}
+
+// CanResolve checks if the given file is a .gemspec file.
+func (r *GemspecResolver) CanResolve(file string) bool {
+ return strings.HasSuffix(file, ".gemspec")
+}
+
+// Resolve parses the gemspec file, identifies runtime dependencies, and
resolves
+// them along with their transitive dependencies. It reports the found
dependencies
+// and their licenses.
+func (r *GemspecResolver) Resolve(file string, config *ConfigDeps, report
*Report) error {
+ f, err := os.Open(file)
+ if err != nil {
+ return err
+ }
+ defer f.Close()
+
+ scanner := bufio.NewScanner(f)
+ deps := make(map[string]string) // name -> version constraint
+ for scanner.Scan() {
+ line := scanner.Text()
+ trimLeft := strings.TrimLeft(line, " \t")
+ if strings.HasPrefix(trimLeft, "#") {
+ continue
+ }
+ if m := gemspecRuntimeRe.FindStringSubmatch(line); len(m) == 2 {
+ // NOTE: Version constraints are currently ignored. We
resolve to the first found installed version of the gem.
+ // This may lead to incorrect resolution if multiple
versions are installed and the first one doesn't satisfy the constraint.
+ deps[m[1]] = ""
+ }
+ }
+ if err := scanner.Err(); err != nil {
+ return err
+ }
+
+ // Recursive resolution
+ queue := make([]string, 0, len(deps))
+ visited := make(map[string]struct{}, len(deps))
+ for name := range deps {
+ queue = append(queue, name)
+ visited[name] = struct{}{}
+ }
+
+ for i := 0; i < len(queue); i++ {
+ name := queue[i]
+ // Find installed gemspec for 'name'
+ path, err := findInstalledGemspec(name, "")
+ if err == nil && path != "" {
+ // Parse dependencies of this gemspec
+ newDeps, err := parseGemspecDependencies(path)
+ if err == nil {
+ for _, dep := range newDeps {
+ if _, ok := visited[dep]; !ok {
+ if len(queue) > 10000 {
+ return
fmt.Errorf("dependency graph too large")
+ }
Review Comment:
The BFS dependency graph size limit check (line 204) that prevents infinite
loops is not tested. Adding a test case with a circular dependency or a very
deep dependency tree would verify that the limit works correctly and produces
an appropriate error message.
##########
pkg/deps/ruby.go:
##########
@@ -104,7 +108,30 @@ func (r *GemfileLockResolver) Resolve(lockfile string,
config *ConfigDeps, repor
continue
}
- licenseID, err := fetchRubyGemsLicense(name, version)
+ if localPath != "" {
+ baseDir, err := filepath.Abs(dir)
+ if err != nil {
+ logrus.WithError(err).Warn("failed to resolve
base directory for local gem path")
+ } else {
+ candidatePath :=
filepath.Clean(filepath.Join(baseDir, localPath))
+ if candidatePath == baseDir ||
strings.HasPrefix(candidatePath, baseDir+string(os.PathSeparator)) {
+ fullPath := candidatePath
+ license, err :=
fetchLocalLicense(fullPath, name)
+ if err == nil && license != "" {
+
report.Resolve(&Result{Dependency: name, LicenseSpdxID: license, Version:
version})
+ continue
+ }
+ } else {
+ logrus.WithField("path",
localPath).Warn("ignoring potentially unsafe local gem path outside project
directory")
+ }
+ }
Review Comment:
The path traversal protection logic (lines 112-127) is not covered by tests.
While the local dependency test case exercises local path resolution, it
doesn't test edge cases like:
- Path traversal attempts (e.g., `path: '../../../etc'`)
- Symbolic link handling
- Absolute paths in the `remote:` field
- Paths with special characters or spaces
Adding test cases for these security-sensitive scenarios would ensure the
protection logic works correctly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
