Skip to content

fix #97: normalize file locator paths and add optional base-directory confinement - #123

Open
phaneendra-injarapu wants to merge 1 commit into
apache:masterfrom
phaneendra-injarapu:issue-97-FileLocatorStrategy
Open

fix #97: normalize file locator paths and add optional base-directory confinement#123
phaneendra-injarapu wants to merge 1 commit into
apache:masterfrom
phaneendra-injarapu:issue-97-FileLocatorStrategy

Conversation

@phaneendra-injarapu

Copy link
Copy Markdown
Contributor

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:

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:

┌──────────────────────────────────────────────────────────────┬────────────────────────────────────────────────────────────────────────────────┐
│                             TestCovers                                     │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldNormalizeTraversalSequencesInTheSpecification          │ .. is collapsed in the resolved File; the specification is preserved           │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldResolveRelativeSpecificationAgainstTheBaseDirectorybase-relative resolution                                                       │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldRefuseRelativeSpecificationThatEscapesTheBaseDirectory │ ../ escape refused with a message                                              │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldRefuseAbsoluteSpecificationOutsideTheBaseDirectoryabsolute escape refused                                                        │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldRefuseSymbolicLinkThatPointsOutsideTheBaseDirectorysymlink escape refused (skipped via assumeTrue where symlinks are unsupported) │
├──────────────────────────────────────────────────────────────┼────────────────────────────────────────────────────────────────────────────────┤
│ shouldRejectNullSpecificationnull specification contract                                                    │
└──────────────────────────────────────────────────────────────┴────────────────────────────────────────────────────────────────────────────────┘

The two pre-existing tests are unchanged and still pass.

Contribution Checklist

  • Your pull request should address just one issue, without pulling in other changes.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body.
  • 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).)
  • 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.)
  • I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004

…rectory confinement

  FileLocatorStrategy passed the location specification directly to
  into the MessageHolder output, and there was no way to keep resolution
  inside an intended directory when the specification came from an untrusted
  source.

  Normalize the specification through java.nio.file.Path before it becomes a
  File, and report an invalid path through the MessageHolder like every other
  failure in this class instead of throwing.

  Add a FileLocatorStrategy(File baseDirectory) constructor for callers that
  handle untrusted specifications. It resolves relative specifications against
  the base directory and refuses anything resolving outside it, comparing
  canonical paths so a symbolic link pointing out of the base directory is
  also refused. The out-of-base check runs before the existence check so a
  refused path does not reveal whether the file exists.

  The no-arg constructor keeps its previous semantics, so existing callers are
  unaffected.
/**
* file locator strategy.
*
* <p>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

write this by hand. This should describe the class, not the bug.

/**
* Create a strategy that only resolves files in the given base directory.
*
* @param baseDirectory the directory that contains the files to resolve; a relative specification is resolved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

not grammatically parallel

}

if (baseDirectory != null && !isInBaseDirectory(file)) {
messageHolder.addMessage(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not just throw an exception here?

} catch (InvalidPathException e) {
messageHolder.addMessage("File: " + locationSpecification + " is not a valid path.");

return null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This null return needs to be well documented, if it's done at all. Am=n exception feels more appropriate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FileLocatorStrategy: unsanitized path allows directory traversal

2 participants