SOLR-18178: Fix configset archive path handling - #4968
Open
nick-boss-tech wants to merge 9 commits into
Open
nick-boss-tech wants to merge 9 commits into
nick-boss-tech wants to merge 9 commits into
Conversation
UploadConfigSet now normalizes backslashes before handing entry names to the config-set backends, which turns Windows-style traversal entries into live '../' syntax on every platform. Pin that both '../evil.txt' and '..\evil2.txt' are contained: neither lands in the configset nor escapes next to it on disk, while legitimate entries still upload.
…spatch UploadConfigSet normalizes backslashes in zip entry names and hands the result to whichever ConfigSetService backend is configured. The filesystem backend contains traversal entries on its own, but the ZooKeeper backend builds a znode path from the entry name, so a single '../' entry aborts the whole upload in SolrCloud instead of being contained. Skip entries whose normalized path is absolute or contains '.' or '..' segments at dispatch, before either backend sees them. Adds a backend-agnostic test that pins exactly which entry names reach a ConfigSetService: only the legitimate entry is dispatched. The test fails against the unpatched dispatch.
isSafeZipEntryPath accepted a normalized entry such as C:/outside/evil.txt: it does not start with a slash and has no dot segments, yet it is absolute on Windows and reached the config-set backend dispatch, outside the guard this change added. Empty entry names passed the same check. The guard now rejects both on every platform (a colon cannot appear in a Windows file name, so a leading drive designator is never a legitimate entry name). Tests pin the guard directly and cover both spellings through the filesystem backend and a mock backend dispatch.
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
1 participant
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.
🤖 AI text below 🤖 (posted on behalf of Nick Shanin)
https://issues.apache.org/jira/browse/SOLR-18178
What happens today
Configset archive handling built ZIP entry names with OS-native path separators, so an archive produced on Windows carried backslash entry names that broke on extraction in
UploadConfigSet. Normalizing those names creates a second problem: a Windows-style traversal name such as..\evil.txt, inert as a literal filename on Linux, becomes live../syntax on every platform once backslashes become slashes. The ZooKeeper configset backend had no traversal guard of its own, so an escaping entry name went straight into the znode path and aborted the upload partway through the archive.What this change does
Archive entry names are normalized to forward slashes everywhere: a new
DownloadConfigSet.toZipEntryNamebuilds ZIP-legal names (/separators, no leading slash, directories get a trailing slash), andUploadConfigSetextraction plusFileSystemConfigSetServicedirectory listing use the same normalization. Before any entry is dispatched to a backend,UploadConfigSetnow skips unsafe paths: after normalization, an entry with a leading/, a.or..path segment, a drive-qualified Windows path (C:/..., either slash spelling), or an empty name is logged and skipped, and the upload of the remaining entries proceeds. The filesystem backend's child-of-root check stays in place as a second layer.Proof
Verified at head 61b8f76 on 2026-10-04 (tidy clean, Error Prone compile clean,
:solr:core:check -x testgreen).UploadConfigSetAPITest15/15, includingtestSafeZipEntryPathRules,testZipUploadRejectsPathTraversalEntries,testZipUploadSkipsUnsafeEntryPathsBeforeBackendDispatch(a ZIP mixing unsafe entries withconf/good.txtdispatches only the good entry), andtestZipUploadRejectsDriveQualifiedEntries;DownloadConfigSetAPITest5/5 covers archives containing subdirectories.TestFileSystemConfigSetService3/3 andTestSchemaDesignerConfigSetHelper8/8 pass, covering the export-side directory listing and the schema-designer ZIP entry assertions.DownloadConfigSetAPITest.testZipConfigSetUsesForwardSlashEntryNames(lang\extra0) andTestSchemaDesignerConfigSetHelper.testDownloadAndZip(lang\contractions_ca.txt). The test files for those two classes are unchanged since that run.TestFileSystemConfigSetServicepasses 3/3 at head. Against the base code with the branch's test file overlaid,testGetAllConfigFilesUsesForwardSlashesForDirectoriesfails (3 tests, 1 failure):expected:<[lang/, lang/nested/, lang/nested/deep.txt, lang/stopwords_en.txt]> but was:<[lang/, lang/nested/deep.txt, lang/stopwords_en.txt, lang\nested/]>.A choice to check
An archive containing an unsafe entry could instead fail as a whole. This change skips the unsafe entries, logs each one, and uploads the rest, so one bad entry cannot block a configset whose other entries are fine. Was skipping the right call, or should the upload fail closed?
Limits
Found while testing and not fixed here:
uploadConfigSet(..., cleanup=true)can delete newly uploaded files. The deletion set is computed before the upload and directories in it are deleted recursively afterwards, so a ZIP that adds files under a pre-existing directory without an explicit directory entry loses those files to the cleanup. Pre-existing behavior; a separate ticket follows.The export-side separator tests were first run on the Linux fork runner, where
Path.toStringalready uses forward slashes, so they pass on base and head alike and cannot discriminate there. The Windows run for the ZIP export tests is now done (see Proof). The directory-listing test inTestFileSystemConfigSetServicepassed on base even on Windows as originally written, because it built only a one-level directory, whose name contains no separator. The test now also covers a nested directory path, and the Windows comparison for that nested case is done (2026-10-04, at 61b8f76): the strengthened test passes at head and fails against the base code with the branch's test file overlaid, where the listing returnslang\nested/instead oflang/nested/.The backslash replacement is unconditional on both sides: on Linux, a file legitimately named with a backslash (for example
a\b.txt) is exported asa/b.txt, and an upload entry of that name lands in a subdirectory, rather than keeping the literal filename.Changelog:
changelog/unreleased/SOLR-18178.yml(fixed)AI assistance
AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution.