Skip to content

Compare path elements when checking that a served file is below the base directory - #1218

Merged
pjfanning merged 2 commits into
apache:mainfrom
pjfanning:path-safety
Sep 1, 2026
Merged

Compare path elements when checking that a served file is below the base directory#1218
pjfanning merged 2 commits into
apache:mainfrom
pjfanning:path-safety

Conversation

@pjfanning

@pjfanning pjfanning commented Aug 22, 2026

Copy link
Copy Markdown
Member

Motivation

checkIsSafeDescendant in FileAndResourceDirectives decides whether a requested file is really below the served directory by comparing the two canonical paths as plain strings:

if (!canonicalFinalPath.startsWith(baseFile.getCanonicalPath))

A string prefix is not a path prefix. With /var/www served, the location /var/www-private/secret satisfies startsWith without being contained in the served directory. The segment filter in safeJoinPaths does not catch this, since no path segment is suspicious — the path only moves out of the directory when getCanonicalPath resolves a symbolic link that points at such a sibling. The file is then served.

Modification

Compare the canonical paths element by element through java.nio.file.Path rather than as strings. The base directory itself still compares as contained, which the directory listing in getFromBrowseableDirectory relies on (its root listing resolves to exactly the base path).

Result

Only files that are genuinely below the served directory are served. A symbolic link into a sibling directory is rejected with the warning that was already there, whatever the sibling is called. Requests that were previously served are unaffected, and the existing symlink and traversal tests still pass.

Tests

  • sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSymlinkSpec" - pass; new test serves through a symlink pointing at a sibling directory named after the served one, and asserts the request is rejected. Verified that it fails without the change (the file is served, handled is true).
  • sbt http-tests/test - pass (1484 tests)
  • sbt http/mimaReportBinaryIssues - pass
  • sbt http/scalafmt http-tests/Test/scalafmt - clean

References

None - tightens the containment check for file and resource directives

pjfanning added a commit to potiuk/pekko-http that referenced this pull request Aug 31, 2026
@pjfanning
Correct two §5/§9 claims against the source and answer Q3
17cbbf6 
Motivation:
Maintainer review found two claims that do not hold on main and one
internal inconsistency:
- §5 claimed Pekko HTTP writes no files, but fileUploadAll creates temp
  files and storeUploadedFile(s) writes entity bytes to an
  application-chosen destination (FileUploadDirectives.scala:178).
- §9 and §14 Q3 described safeDirectoryChildPath as containing
  traversal, but its canonical-path check compares strings, so a
  symlink resolving into a sibling directory that shares the served
  root as a string prefix escapes it (fix in flight in apache#1218).
- The status line said both "Q1-Q8 answered" and "all ten answered"
  while Q3 still ended in an open question, and the pinned commit was
  the PR's own first commit rather than the main commit reviewed.

Modification:
Restate the §5 file-system claim with the upload-directive carve-out
and correct Q9 accordingly. Answer Q3 as a correction: the escape is
VALID under §5b.4 and fixed by apache#1218; update §9 and the §15 back-map to
match. Note in §12 that apache#1217 would invalidate the shutdown-hook claim
on merge. Fix the status line, the tag tally, and the commit pin
(444d939 -> 85d7243, the main commit the branch is based on).

Result:
Every §5 negative claim matches the source at the pinned commit, Q3 is
answered consistently with the "all ten answered" status, and the two
in-flight PRs that touch the model's claims (apache#1217, apache#1218) are
cross-linked.

Tests:
Not run - docs only

References:
Refs apache#1218, Refs apache#1217
@pjfanning pjfanning mentioned this pull request Aug 31, 2026
Open
@pjfanning

pjfanning commented Aug 31, 2026

Copy link
Copy Markdown
Member Author

Updated after review: rebased onto main and pushed 1144ef3 addressing the findings.

  • checkIsSafeDescendant now catches InvalidPathException/IOException and treats such paths as not contained — previously a Windows segment like a<b (or %00 on any platform, pre-existing) escaped as a 500 instead of the clean rejection, skipping the traversal warning. New directional test uses a %00 segment; verified failing with the fix stashed.
  • The scaladoc above safeDirectoryChildPath now states the element-wise containment contract instead of the string-prefix semantics the fix removed, and notes that Windows canonicalization does not resolve NTFS symlinks/junctions — that link-escape class remains open there (pre-existing, Path.toRealPath would be the principled follow-up).
  • Symlink spec fixture no longer writes content nothing asserts on.

pjfanning added a commit to potiuk/pekko-http that referenced this pull request Aug 31, 2026
@pjfanning
Fold the reworked apache#1217 and apache#1218 into §5, §9 and Q3
12c07b5 
Motivation:
Both in-flight PRs the model references changed shape after review.
apache#1218 gained a documented platform caveat - File.getCanonicalPath does
not resolve NTFS symbolic links or junctions on Windows, so the
link-escape class stays open there - and now rejects path segments that
no file-system path may contain instead of erroring. apache#1217 replaced the
per-file deleteOnExit with one temp directory per actor system removed
by a CoordinatedShutdown task. Q3's answer claimed symlink escapes are
rejected "whatever its target is named", which overclaims on Windows.

Modification:
Scope the Q3 and §9 symlink-rejection claims to platforms where
canonicalization resolves links, record the Windows residual and the
toRealPath follow-up, note the invalid-segment hardening, and update
the §5 upload-directive note to describe apache#1217's per-system directory
and CoordinatedShutdown cleanup.

Result:
The model's containment and file-writing claims match what apache#1217 and
apache#1218 actually implement, on every platform they address.

Tests:
Not run - docs only

References:
Refs apache#1217, Refs apache#1218
@pjfanning
compare path elements when checking that a file is below the served d…
2b8889a 
…irectory

Motivation:
`checkIsSafeDescendant` compared the canonical location of the requested
file with the canonical path of the served directory as a plain string
prefix. A path such as `/var/www-private/secret` has `/var/www` as a string
prefix without being contained in it, so a symbolic link inside the served
directory that resolves to such a sibling directory passed the check and the
file was served. The segment filter in `safeJoinPaths` does not catch this,
because no path segment is suspicious; only canonicalization moves the
location out of the directory.

Modification:
Compare the two canonical paths element by element via `java.nio.file.Path`
instead of as strings. That keeps the base directory itself accepted, which
the directory listing of `getFromBrowseableDirectory` relies on.

Result:
Only files that are really below the served directory are served, and a
symbolic link to a sibling directory is rejected with the existing warning
regardless of how the sibling is named.

Tests:
- sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSymlinkSpec" - pass, 1 new test that serves a symlink to a sibling directory named after the served one; it fails without the change
- sbt http-tests/test - pass
- sbt http/mimaReportBinaryIssues - pass
- sbt http/scalafmt http-tests/Test/scalafmt - clean

References:
None - tightens the containment check for file and resource directives
@pjfanning
reject paths that cannot exist instead of erroring, document the elem…
7c6f5c3 
…ent-wise containment

Motivation:
Review of the containment change found that Paths.get can throw an
unchecked InvalidPathException where the previous String.startsWith
never threw: on Windows a decoded segment such as `a<b` passes
safeJoinPaths and File.getCanonicalPath but not the stricter
WindowsPathParser, turning a previously clean rejection into a 500
that skips the traversal warning and breaks rejection-based route
chaining. A NUL byte in a segment (`%00`) had the same effect on every
platform even before this PR, via the IOException that
File.getCanonicalPath throws for it. The scaladoc above
safeDirectoryChildPath also still described the string-prefix
containment semantics that the previous commit removed as insecure,
and the symlink spec wrote file content that no assertion reads.

Modification:
Catch InvalidPathException and IOException in checkIsSafeDescendant
and treat such paths as not contained, producing the existing warning
and rejection; no file with such a name can exist, so nothing
servable is lost. Restate the scaladoc containment contract as
element-wise comparison and note that Windows canonicalization does
not resolve NTFS symbolic links or junctions. Create the sibling
fixture file empty in the symlink spec. Rebased onto main.

Result:
A request segment that no file-system path may contain is rejected
with the traversal warning instead of escaping to the exception
handler as a 500, on all platforms; the documented contract matches
the implementation.

Tests:
- sbt "http-tests/testOnly org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSpec org.apache.pekko.http.scaladsl.server.directives.FileAndResourceDirectivesSymlinkSpec" - pass (52 tests); new test rejects a %00 segment and asserts the traversal warning; verified it fails with the fix stashed (unhandled IOException)
- sbt http/mimaReportBinaryIssues - pass
- native scalafmt run on the three changed files - clean

References:
None - hardens the containment check follow-up from review
@pjfanning
pjfanning merged commit 2bb837f into apache:main Sep 1, 2026
6 checks passed
@pjfanning pjfanning added this to the 2.0.0-M2 milestone Sep 1, 2026
@pjfanning pjfanning mentioned this pull request Sep 3, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@He-Pin He-Pin He-Pin approved these changes
@jrudolph jrudolph Awaiting requested review from jrudolph
@raboof raboof Awaiting requested review from raboof
@nvollmar nvollmar Awaiting requested review from nvollmar
@Philippus Philippus Awaiting requested review from Philippus
@samueleresca samueleresca Awaiting requested review from samueleresca

Assignees

No one assigned

Labels

backport

Projects

None yet

Milestone

2.0.0-M2

Development

Successfully merging this pull request may close these issues.

2 participants

@pjfanning @He-Pin