Conversation
The containment check compared canonical paths as string prefixes, so a sibling directory sharing the configured directory's name prefix (e.g. ui/admin/default-old next to ui/admin/default) was reachable via "..". Resolve requests with java.nio.file.Path and check containment on path components, resolving symlinks on both sides. Serve files through Files/Path instead of file: URLs. Resolves CodeQL alerts #2, #3, OpenIdentityPlatform#8, OpenIdentityPlatform#9 (java/partial-path-traversal) and OpenIdentityPlatform#5, OpenIdentityPlatform#6, OpenIdentityPlatform#7 (java/ssrf) in openidm-servlet.
5 tasks
…let path The bare-context-root redirect built its target from req.getServletPath(); use the configured urlContextRoot, which is the same value for the Pax Web alias registration but does not depend on anything in the request. Resolves CodeQL alert OpenIdentityPlatform#4 (java/unvalidated-url-redirection).
2 tasks
Resolves CodeQL alert #437 (java/inefficient-empty-string-test).
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.
Summary
Fixes the seven critical CodeQL Code scanning alerts, all in
ResourceServlet, plus the medium and the note-level ones in the same file (second and third commits):java/partial-path-traversal(-from-remote): the containment check comparedgetCanonicalPath()results as string prefixes, so a sibling directory sharing the configured directory's name prefix (e.g.ui/admin/default-oldnext toui/admin/default) was reachable via/../default-old/.... Practical impact is limited (needs such a sibling to exist; Jetty 12 normalises..beforegetPathInfo()), but the check itself was wrong.java/ssrf: false positives — theURLwas always afile:URL produced byFile.toURI(). Removing theURLdetour eliminates the alerts rather than dismissing them.java/unvalidated-url-redirection(second commit): the bare-context-root redirect (/admin→/admin/) built its target fromreq.getServletPath(). It now uses the configuredurlContextRoot— the same value for the Pax Web alias registration, but nothing taken from the request. In practice this was a false positive (Jetty 12 rejects//as an ambiguous path segment); the replacement is simply cleaner than a dismissal.java/inefficient-empty-string-test(third commit):"".equals(target)→target.isEmpty()in the samedoGetguard.Changes
ResourceServlet: newlocate(dir, target)resolves the request against the configured directory withjava.nio.file.Path—normalize(), containment viaPath.startsWith(component-wise, not string prefix), thentoRealPath()on both sides so symlinks are resolved exactly asgetCanonicalPath()did before. Invalid paths and files that vanish between the check and the resolve yield 404 instead of 500.handleIndexHtml/handle/getLastModified/copyResourcetake aPathand useFiles;File,URL,URLConnectionare gone from the class.Content-Lengthis now set before the body is written (previously after, which was ignored once the response buffer had flushed).ResourceServletTest(TestNG + Mockito, module previously had no tests): drives the realdoGetagainst a temp directory tree — normal lookups (default/extension precedence,index.htmlinjection, missing extension dir, 304), rejections (directory, unknown file, prefix-sibling traversal for both dirs, climbing above the UI root), and symlink policy (link escaping the dir → 404; the dir itself being a symlink → served).openidm-servlet/pom.xml: test-scopedtestng,mockito-all,slf4j-simple.Test plan
ResourceServletTest— 16/16 pass; the three prefix-traversal cases and the two redirect cases fail onmasterand pass heremvn -pl openidm-servlet -am package— bundle builds, manifest unchanged apart from dropped imports