phaneendra-injarapu opened a new pull request, #123:
URL: https://github.com/apache/maven-shared-io/pull/123
Fixes #97 (originally reported as bug #15).
### Problem
`FileLocatorStrategy.resolve` passed the location specification straight to
`new File(locationSpecification)` with no normalization and no boundary
check.
Two consequences:
1. `..` and `.` elements survived into the resolved `File`, so the `File`
handed
back to callers (and the path printed into the `MessageHolder`) did not
match
the file actually being addressed.
2. The class has no notion of a root directory, so a caller that accepts a
specification from an untrusted source had no way to keep resolution
inside an
intended directory — `../../etc/passwd` resolved happily.
As the issue notes, this is low priority in the normal Maven plugin case,
where
the POM supplying the specification is trusted. The fix is therefore
scoped to
close the gap without changing behaviour for existing callers.
### How
**1. Normalization, always on.** The specification is parsed as a `Path`
and
`normalize()`d before it becomes a `File`, so `..` and `.` elements are
collapsed
lexically. A specification that is not a valid path (for example one
containing a
NUL byte) now adds a message and returns `null`, which is how this class
already
reports every other failure, instead of throwing `InvalidPathException` at
the
caller.
**2. Optional confinement, opt-in.** Normalization alone cannot stop
traversal
here: with no root to compare against, an absolute or upward-pointing path
is a
perfectly legitimate request for this strategy, and rejecting it would
break
existing users. So the boundary is introduced as a new constructor:
```java
new FileLocatorStrategy(baseDirectory)
With a base directory set, a relative specification resolves against that
directory rather than the process working directory, and any specification
that
resolves outside it is refused with a message and a null Location. The
containment test compares canonical paths, so a symbolic link that sits
inside
the base directory but points outside it is also refused. If the canonical
path
is unavailable (IOException), the check falls back to comparing normalized
absolute paths rather than failing open.
The out-of-base check runs before the existence check, so a refused path
does
not reveal whether the target file exists.
Compatibility
- The no-arg constructor keeps the previous semantics: relative
specifications
resolve against the working directory and any file is resolvable. Only the
../. collapsing is new, and it resolves to the same file contents.
- Location.getSpecification() still returns the original, unmodified
string;
only the resolved File is normalized.
- Source- and binary-compatible: one added constructor, no signature
changes.
- A null specification now throws a named NullPointerException
(Objects.requireNonNull) instead of an unlabeled one from new File(null) —
same exception type, better message.
Tests
Six new cases in FileLocatorStrategyTest:
┌──────────────────────────────────────────────────────────────┬────────────────────────────────────────────────────────────────────────────────┐
│ Test │
Covers │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldNormalizeTraversalSequencesInTheSpecification │ .. is
collapsed in the resolved File; the specification is preserved │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldResolveRelativeSpecificationAgainstTheBaseDirectory │
base-relative resolution │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldRefuseRelativeSpecificationThatEscapesTheBaseDirectory │ ../
escape refused with a message │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldRefuseAbsoluteSpecificationOutsideTheBaseDirectory │ absolute
escape refused │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldRefuseSymbolicLinkThatPointsOutsideTheBaseDirectory │ symlink
escape refused (skipped via assumeTrue where symlinks are unsupported) │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldRejectNullSpecification │ null
specification contract │
└──────────────────────────────────────────────────────────────┴────────────────────────────────────────────────────────────────────────────────┘
```
The two pre-existing tests are unchanged and still pass.
## Contribution Checklist
- [x] Your pull request should address just one issue, without pulling in
other changes.
- [x] Write a pull request description that is detailed enough to
understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject
line and body.
- [x] Write unit tests that match behavioral changes, where the tests fail
if the changes to the runtime are not applied.
(6 new tests in `FileLocatorStrategyTest`.
`shouldNormalizeTraversalSequencesInTheSpecification` fails against
the previous
implementation, which returned an un-normalized `File`. The four
base-directory tests
exercise the `FileLocatorStrategy(File)` constructor this PR
introduces, so they cannot
compile against the previous implementation at all.
`shouldRejectNullSpecification` pins
down behaviour that was previously incidental — the old code also
threw NPE, from
`new File(null)`.)
- [x] Run `mvn verify` to make sure basic checks pass.
(Test suite verified: 0 failures. Main sources compile at `--release
8`.
`mvn checkstyle:check` reports 0 Checkstyle violations, and spotless
and apache-rat are
clean. A full `mvn verify` could not be completed locally because
the environment has no
access to Maven Central to resolve the surefire plugin's
dependencies; relying on CI for
the complete run.)
- [x] I hereby declare this contribution to be licenced under the Apache
License Version 2.0, January 2004
--
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]
