reject dot-dot segments in grape coordinate validation#2584
Open
netliomax25-code wants to merge 1 commit into
Open
reject dot-dot segments in grape coordinate validation#2584netliomax25-code wants to merge 1 commit into
netliomax25-code wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens Grape coordinate validation to prevent .. path traversal via dependency coordinate components that are later used in Ivy/Maven cache path construction.
Changes:
- Reject coordinate values containing
..in bothGrapeIvy.createGrabRecordandGrapeMaven.createGrabRecord. - Add regression tests in
GrapeIvyTestfor..inversionandgroup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| subprojects/groovy-grape-maven/src/main/groovy/groovy/grape/maven/GrapeMaven.groovy | Adds a .. rejection check during Maven grab record validation. |
| subprojects/groovy-grape-ivy/src/main/groovy/groovy/grape/ivy/GrapeIvy.groovy | Adds a .. rejection check during Ivy grab record validation. |
| subprojects/groovy-grape-ivy/src/test/groovy/groovy/grape/ivy/GrapeIvyTest.groovy | Adds regression tests covering the new validation behavior for Ivy. |
Comment on lines
+429
to
+445
| @Test | ||
| void testInvalidVersionDotDot() { | ||
| def ex = shouldFail ''' | ||
| groovy.grape.Grape.grab(group: 'org.ejml', module: 'ejml-simple', version: '..') | ||
| ''' | ||
| assert ex.message.contains('for version') | ||
| assert ex.message.contains("should not contain '..'") | ||
| } | ||
|
|
||
| @Test | ||
| void testInvalidGroupDotDot() { | ||
| def ex = shouldFail ''' | ||
| groovy.grape.Grape.grab(group: '..', module: 'ejml-simple', version: '0.41') | ||
| ''' | ||
| assert ex.message.contains('for group') | ||
| assert ex.message.contains("should not contain '..'") | ||
| } |
Comment on lines
+589
to
+591
| if (v.toString().contains('..')) { | ||
| throw new RuntimeException("Grab: invalid value of '$v' for $k: should not contain '..'") | ||
| } |
✅ All tests passed ✅🏷️ Commit: 3168577 Learn more about TestLens at testlens.app. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
createGrabRecord rejects path separators and shell metacharacters in coordinate values, but a value made only of dot segments still passes both the version blacklist and the group/module whitelist, so a version or group of '..' survives and is later interpolated into the ivy/maven cache file paths as a parent-directory hop. This adds a contains('..') guard next to the existing checks, after the backslash fix, and applies it to GrapeMaven too since it shares the same validation.