Skip to content

Conversation

@agrasth
Copy link
Collaborator

@agrasth agrasth commented Oct 14, 2025

  • All tests passed. If this feature is not already covered by the tests, I added new tests.
  • All static analysis checks passed.
  • Appropriate label is added to auto generate release notes.
  • I used gofmt for formatting the code before submitting the pull request.
  • PR description is clear and concise, and it includes the proposed solution/fix.

Maven FlexPack: Fix Critical XML Parsing and Artifact Collection Issues

Changes

1. Added Proper XML Parsing

  • Added structs: PomProject, PomParent, DistributionManagement, Repository
  • Replaced strings.Index() XML parsing with xml.Unmarshal()
  • Imports: Added encoding/xml

Issue: String-based parsing breaks with XML comments, whitespace, namespaces, and attribute reordering.

2. Fixed Deploy Goal Detection

  • Updated wasDeployCommand() to match both deploy and *:deploy patterns

Issue: Maven plugin notation maven-deploy-plugin:deploy was not detected, causing artifact collection to be skipped.

3. Fixed Artifact Collection

  • Replaced hardcoded .jar lookup with directory scan
  • Now collects all artifacts: .jar, .war, .ear, -sources.jar, -javadoc.jar, -tests.jar
  • Added getPackagingType() helper

Issue: Only main JAR was collected, missing sources, javadoc, tests, and other packaging types (war/ear).

4. Fixed Repository Detection

  • getMavenDeployRepository() now uses XML parsing
  • Handles both <repository> and <snapshotRepository>
  • Supports two URL formats: standard and /api/maven/ patterns

Issue: String parsing failed on real-world POM files, and snapshots weren't handled correctly.

5. Added Settings.xml Support for Deployment Repository

Problem: Users who define deployment repositories in settings.xml via altDeploymentRepository weren't supported - only pom.xml was checked.
Fix:

  • Added SettingsXml struct parsing with active profile support
  • Finds settings.xml via -s flag or ~/.m2/settings.xml
  • Extracts altDeploymentRepository from active profiles (format: id::layout::url)
  • Falls back to settings.xml only if pom.xml has no

Why These Were Missed

These issues weren't caught because:

  • Initial implementation was tested with minimal/simple POM files
  • Tests didn't cover plugin notation, multi-artifact projects, or snapshot deployments
  • String-based XML parsing worked for the happy path but failed on edge cases

@agrasth agrasth added the bug Something isn't working label Oct 14, 2025
defer searchReader.Close()

// Filter to only artifacts modified in the last 2 minutes (just deployed)
cutoffTime := time.Now().Add(-2 * time.Minute)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this required? isnt the artifact name enough to filter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason why I added this was, if we deploy multiple times on a specific repo for same project it accumulates the artifacts, not overwrite. And wildcard matches ALL snapshots ever deployed.
With time filtering, we only tag artifacts modified in the last 2 minutes.
We can improve this, let me know if you have any ideas on this.

// Only include artifacts modified after cutoff
if modTime.After(cutoffTime) {
recentArtifacts = append(recentArtifacts, *item)
log.Debug(fmt.Sprintf("Including recently deployed artifact: %s (modified: %s)", item.Name, item.Modified))
Copy link
Collaborator

Choose a reason for hiding this comment

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

seeing too many debug logs for build info construction. Would you like to have verbose level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok improving it


// setMavenBuildPropertiesOnArtifacts sets build properties on deployed Maven artifacts
// Following the pattern from twine.go
func setMavenBuildPropertiesOnArtifacts(workingDir, buildName, buildNumber string, buildArgs *buildUtils.BuildConfiguration) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please consider refactoring this function, it is huge.

// Read pom.xml to get project information
// getMavenDeployRepository determines where Maven deployed artifacts
// by parsing pom.xml distributionManagement
func getMavenDeployRepository(workingDir string) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if the the deployment repository is mentioned in settings.xml? is it handled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it's NOT handled currently, let me check how can I implement this.

// Handle different URL patterns
if strings.Contains(repoUrl, "/api/maven/") {
// Format: http://host/artifactory/api/maven/REPO-KEY
parts := strings.Split(repoUrl, "/api/maven/")
Copy link
Collaborator

Choose a reason for hiding this comment

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

using net/url gives better control for extracting repokey

`rawURL := "https://example.com/api/v1/users/123/profile"

// Parse the URL
u, err := url.Parse(rawURL)
if err != nil {
	panic(err)
}

// Split path into segments
segments := strings.Split(strings.Trim(u.Path, "/"), "/")

// Example: extract the 4th segment (index 3)
if len(segments) > 3 {
	return segments[3], nil
} else {
	return "", fmt.Errof("unable to extract repo key check repotitory URL")
}`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks, updated this.

@github-actions
Copy link
Contributor

🚨 Frogbot scanned this pull request and found the below:

📗 Scan Summary

  • Frogbot scanned for vulnerabilities and found 1 issues
Scan Category Status Security Issues
Software Composition Analysis ℹ️ Not Scanned -
Contextual Analysis ℹ️ Not Scanned -
Static Application Security Testing (SAST) ✅ Done
1 Issues Found 1 Medium
Secrets ✅ Done -
Infrastructure as Code (IaC) ✅ Done Not Found


// parseSettingsXml reads and parses Maven settings.xml
func parseSettingsXml(settingsPath string) (*SettingsXml, error) {
data, err := os.ReadFile(settingsPath)
Copy link
Contributor

Choose a reason for hiding this comment

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

🎯 Static Application Security Testing (SAST) Vulnerability

Severity Finding
medium
Medium
Untrusted input used in file paths, allowing access to unintended files.
Full description

Vulnerability Details

Rule ID: go-path-traversal

Overview

Path Traversal is a type of vulnerability that allows an attacker to access
files or directories outside the intended directory structure. This can lead
to unauthorized access to sensitive files, potentially compromising the
security and confidentiality of the application or system.

Vulnerable example

func serveFile(w http.ResponseWriter, r *http.Request) {
    filePath := r.URL.Query().Get("file")
    http.ServeFile(w, r, filePath)
}

In this example, the serveFile function serves a file based on the
file query parameter provided by the user. However, there is no validation
on the file path, allowing an attacker to traverse directories and access
files outside the intended directory structure.

Remediation

To mitigate path traversal vulnerabilities, it is essential to validate
and sanitize user input. In this example, we check if the file path starts
with the allowed directory path before serving the file.

func serveFile(w http.ResponseWriter, r *http.Request) {
    filePath := r.URL.Query().Get("file")
+    if !strings.HasPrefix(filePath, "/path/to/allowed/directory/") {
+        http.Error(w, "Forbidden", http.StatusForbidden)
+        return
+    }
    http.ServeFile(w, r, filePath)
}
Code Flows
Vulnerable data flow analysis result

↘️ os.Args (at artifactory/commands/flexpack/maven.go line 265)

↘️ args (at artifactory/commands/flexpack/maven.go line 268)

↘️ args[i+1] (at artifactory/commands/flexpack/maven.go line 268)

↘️ return args[i+1] (at artifactory/commands/flexpack/maven.go line 268)

↘️ getSettingsXmlPath() (at artifactory/commands/flexpack/maven.go line 297)

↘️ settingsPath (at artifactory/commands/flexpack/maven.go line 302)

↘️ settingsPath (at artifactory/commands/flexpack/maven.go line 281)

↘️ settingsPath (at artifactory/commands/flexpack/maven.go line 282)




@agrasth agrasth merged commit e542210 into jfrog:main Oct 29, 2025
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants