RTECO-1400 - Add path validation to prevent traversal attacks in artifact handling

[fluxxBot]

• 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.

---

[agrasth]

CI Status Summary

All checks are passing. No failures found.

Check	Result
CLAssistant	✅ Pass
Check PR Labels	✅ Pass
Go-Sec (ubuntu-latest)	✅ Pass
Lint (macOS)	✅ Pass
Lint (ubuntu)	✅ Pass
Lint (windows)	✅ Pass
Static Check (ubuntu-latest)	✅ Pass
macOS unit tests	✅ Pass
ubuntu unit tests	✅ Pass
windows unit tests	✅ Pass
scan-pull-request	✅ Pass

All 19 check runs across both workflow trigger runs completed successfully. This PR is green and ready for review.

[agrasth]

Ignore.

[agrasth]

Ignore.

[agrasth]

Ignore.

[agrasth]

Code Review: RTECO-1400 — Path Traversal Prevention in Maven Artifact Handling

Files reviewed: 1 production file (artifactory/commands/flexpack/maven.go) — 0 test files

Executive Summary

This PR correctly identifies and mitigates a real path traversal risk: Maven coordinates and packaging type extracted from a user-controlled pom.xml were flowing directly into filepath.Join calls without sanitization. The defense-in-depth approach (validating at extraction time and at composition time) is good instinct. Two issues need attention before merge: the new security logic ships with zero test coverage, and the validation uses a denylist rather than an allowlist — the weaker of the two strategies when the allowed character set is well-defined.

---

Findings

Testing

Major — artifactory/commands/flexpack/maven.go (validateMavenCoordinate) — No tests for new security validation logic

The entire validateMavenCoordinate function and all its call-sites have no test coverage. This is the most impactful gap: security controls without tests are silently broken by future refactors. At minimum, tests should cover:

• Valid coordinates pass (groupId with dots, version with dashes/dots, SNAPSHOT suffix)
• .. in any position is rejected
• / and \ are rejected
• Null byte (\x00) is rejected
• Empty string behavior (currently passes — see minor finding below)

Security

Minor — artifactory/commands/flexpack/maven.go:513 — Denylist is weaker than an allowlist for Maven coordinate validation

validateMavenCoordinate blocks .., /, \, and null bytes, but Maven coordinates have a narrow, well-defined character set. An allowlist rejects all future unknown bypass vectors (newlines, semicolons, ANSI codes, encoded characters) without needing to enumerate them:

var mavenCoordinateRe = regexp.MustCompile(`^[a-zA-Z0-9._\-]+$`)

func validateMavenCoordinate(value string) error {
    if value == "" {
        return fmt.Errorf("value is empty")
    }
    if !mavenCoordinateRe.MatchString(value) {
        return fmt.Errorf("value %q contains characters not permitted in Maven coordinates", value)
    }
    return nil
}

This subsumes the existing three checks and adds an empty-string guard.

Functionality

Minor — artifactory/commands/flexpack/maven.go:553 — Defense-in-depth check omits null byte (inconsistent with validateMavenCoordinate)

The secondary check on mainArtifactName tests .. and /\\ but not null bytes, making it a weaker subset of validateMavenCoordinate. To stay consistent and avoid maintaining two parallel denylist snippets that can drift:

// Before:
if strings.Contains(mainArtifactName, "..") || strings.ContainsAny(mainArtifactName, "/\\") {
    return fmt.Errorf("invalid artifact name %q", mainArtifactName)
}

// After:
if err := validateMavenCoordinate(mainArtifactName); err != nil {
    return fmt.Errorf("invalid artifact name: %w", err)
}

Note: mainArtifactName includes - and . separators, both of which are already permitted by the allowlist regex suggested above.

---

Positive Highlights

• Null byte check (strings.ContainsRune(value, 0)) is a non-obvious but correct inclusion; many path-traversal validators miss this vector.
• %w wrapping in error returns preserves error chain semantics for callers using errors.Is / errors.As.
• Graceful fallback to "jar" for invalid packaging type keeps the tool functional against a malicious pom.xml without crashing.
• Validating at both extraction time (getMavenArtifactCoordinates) and composition time (addDeployedArtifactsToBuildInfo) is the right defense-in-depth instinct.

Verdict

REQUEST CHANGES

Add unit tests for validateMavenCoordinate before merging — this is security-critical code and must be verifiable.

[fluxxBot]

The validation only runs on coordinate fields — groupId, artifactId, version, packaging (and the Parent fallbacks). These are Maven coordinates, not filesystem paths. By spec/convention they're restricted to [A-Za-z0-9_.-]:

groupId: dot-separated package-style, e.g. com.example — single dots as separators, never .. or /.
artifactId: e.g. my-app — no separators.
version: e.g. 1.0.0-SNAPSHOT — dots separate segments, never two consecutive (1..0 is invalid).
packaging: jar/war/pom/ear/bundle etc.
None of these can legitimately contain .., /, or .

[github-actions]

---

🐸 JFrog Frogbot