Validate path components#1959
Conversation
But make it reusable as well. Supersedes apache#1958
gnodet
left a comment
There was a problem hiding this comment.
AI-Assisted Review — PR #1959 (Validate path components)
The core contribution here — extracting path traversal validation into PathUtils for cross-project reuse and applying it at the DefaultLocalPathComposer boundary — is well-designed and correct. The equals("..") + contains("/") + contains("\\") approach is sound and more precise than PR #1958's contains(".."), with the reasoning properly documented in code comments ("version '1..' is valid").
Verified concern: GenericVersion constructor validation
The addition of validatePathComponent inside the GenericVersion constructor conflates filesystem-safety with version parsing. GenericVersion is a pure version-parsing/comparison class used for version ranges, dependency resolution ordering, constraint evaluation, and version comparison — none of which involve filesystem paths.
The IllegalArgumentException escapes through parseVersion() uncaught. The call chain:
GenericVersionScheme.parseVersion(String)declaresthrows InvalidVersionSpecificationException(checked)- It calls
new GenericVersion(version)→validatePathComponent()→ throwsIllegalArgumentException(unchecked) - Callers like
AbstractPatternDependencyFilter.isVersionIncludedInRange()catch onlyInvalidVersionSpecificationException— the unchecked exception propagates unhandled
This is a behavioral change to a widely-used public API. Any downstream code using parseVersion() with a version string containing / or \ (unusual but not currently invalid) would get an uncaught IllegalArgumentException instead of a successful Version object.
Suggestion
Consider keeping the PathUtils extraction and DefaultLocalPathComposer integration (which is the right enforcement point — where versions become filesystem path segments), but removing the GenericVersion constructor change. If version-level validation is desired as defense-in-depth, it could either:
- Throw
InvalidVersionSpecificationExceptioninstead (if path traversal is considered a version syntax violation), or - Be applied at a different layer (e.g., in Maven's
DefaultVersionResolver)
Other notes
- The metadata type exclusion from validation is correctly documented (
.meta/prefixes.txtcontains legitimate path separators) - This PR completely supersedes PR #1958 — if merged, #1958 should be closed
- Test coverage for the core path validation through
DefaultLocalPathComposerTestis solid; direct unit tests forPathUtils.validatePathComponentand theGenericVersionconstructor behavior would strengthen coverage
🤖 This review was generated with AI assistance (reviewer + independent verifier pattern). Findings were independently verified before posting.
|
Agreed with assessment: will remove |
|
Thanks for the quick update, @cstamas! Removing the |
|
@cstamas Please assign appropriate label to PR according to the type of change. |
| // OTOH: version "1.." is valid version string! | ||
| if (value.equals("..") || value.contains("/") || value.contains("\\")) { | ||
| throw new IllegalArgumentException( | ||
| "Invalid " + label + ": must not contain '..', '/' or '\\': " + value); |
There was a problem hiding this comment.
Should maybe be:
"Invalid " + label + ": must not be equal to '..' or contain '/' or '\': " + value);
But make it reusable as well. When 2.0.21 released, Maven codebase of
DefaultVersionResolverandDefaultVersionRangeResolvershould be updated too, and use this util method.Supersedes (and based on) #1958