SOLR-18618: Upload configsets from a backup repository by HoustonPutman · Pull Request #5287 · apache/solr

HoustonPutman · 2026-11-03T22:42:44Z

https://issues.apache.org/jira/browse/SOLR-18618

The new API adds the features as params, instead of a body. The body would have been a good option, but it kind of contrasts with the existing query param options, and in the file API currently files being uploaded with a JSON content type would have a weird behavior going forward.

Unfortunately there's not a good way of gating the way that the API is used (body or repository & location). We could use a separate API path, but I couldn't think of anything acceptable for that. Open to suggestions though.

…itory

epugh · 2026-11-03T22:49:20Z

what about:

PUT /configsets/{configSetName}/zip
PUT /configsets/{configSetName}/repo

Or, maybe you just keep PUT /configsets/{configSetName} and move everything into the body? The V2 API's are experimental, so you CAN change how we handle the zip use case...

Would distingushing on the accept type help? One takes a binary file and the other takes json?

HoustonPutman · 2026-11-03T23:24:37Z

Would distingushing on the accept type help?

Yeah, and that would work great for full configSet upload. For a single file it gets iffy in case the file is of type json. Basically we can make it that if the file matches the schema ({location:"...", "repository": "...."}), it is a repo upload, otherwise its a file upload. The problem is that if the user has a small error, it will be uploaded as a file to the configSet instead of getting a 400 error back.

nick-boss-tech · 2026-11-05T08:18:53Z

🤖 AI text below 🤖 (reviewed by Nick Shanin)

I prepared a follow-up covering V1/V2 body exclusivity, symlink rejection for local repository imports, and dot-prefixed directory imports.

The comparison has eight files: the original seven plus a test-policy update that lets the symlink tests create links under Solr's test security manager. The tidy and test setup fixes are within existing files.

Verification

At d0cf2471f37ae973ca06390ec195cf51f8fccbb3, focused runs passed for LocalFS (12/12), the two core API suites (12/12 and 5/5), and GCS (9/9). Fail-before runs detected the original body, symlink, and directory-import behavior. Tidy and Error Prone passed. ./gradlew check -x test passed on rerun after the download relay returned a checksum-mismatched UI archive; the rerun used a checksum-verified copy. The S3 suite skipped all 10 tests because Docker was unavailable, so S3 remains unverified.

The patch is a fast-forward from this PR's current head, 19f7c83812a4c81227601ff1abadca84beb05cb9. Pinned diff.

From a checkout at the current PR head, the changes can be taken with:

git fetch https://github.com/nick-boss-tech/solr.git pr-5287-configset-source-validation
git merge --ff-only d0cf2471f37ae973ca06390ec195cf51f8fccbb3

gerlowskija · 2026-11-09T11:59:01Z

FWIW I kindof like the params-not-request-body approach here. There's less ambiguity, for one thing.

For another you could also argue it's more REST-ful. The request-body of a POST/PUT is intended to represent the "resource" being created/updated. "repository" and "location" aren't really the configset resource itself, they're something else. (coordinates? metadata? not sure what it makes sense to call them).

Anyway, I'm fine w/ either approach, but I'd opt for the query params personnally.

gerlowskija

Left a few comments inline. I think the most important one(s) are around the security implications. Not sure whether I'm just being overly paranoid there, or whether those actually need addressed but would love some other eyes on those.

gerlowskija · 2026-11-09T12:05:51Z

-            .isEmpty()) { // Uploading a whole configset
+        final var repository = req.getParams().get(CoreAdminParams.BACKUP_REPOSITORY);
+        final var location = req.getParams().get(CoreAdminParams.BACKUP_LOCATION);
+        if ((repository == null) != (location == null)) {


[0] This validation should probably live in the v2 API implementation class(es). Or if you have added it there elsewhere in this diff, then it probably isn't needed here.

gerlowskija · 2026-11-09T12:37:32Z

+      if (!repository.exists(source)) {
+        throw new SolrException(
+            SolrException.ErrorCode.BAD_REQUEST,
+            "The location " + source + " does not exist in the backup repository");


[Q] This line got me thinking: "Oh I wonder if users could use this to sniff out files that exist up in the repository", and that got me thinking about the security implications of this change more broadly.

With these params any users with upload-configset perms now have the ability to essentially read arbitrary content out of whatever repository the admins have set up.

A few disjointed thoughts along these lines:

I guess, technically, arbitrary-read is a risk with the existing backup-restore code, but that seems significantly mitigated by the fact that backups have a lot of structure and the API is validating that "location" values point to a valid backup before its doing any file copies, etc. Configsets don't have that structure necessarily, so we can't do much similar validation here.

Many repositories take a default or "root" location. The risk would be much reduced if S3BackupRepository and friends were coded to prevent path-escape from that root location. But at a glance that doesn't appear to the case?

The community has generally agreed that "configset-upload" perms are pretty dangerous and should only be given out to very trusted users. So maybe this isn't a huge deal in practice, and could just be solved with documentation saying that "upload-configset allows users to read out of backupRepositories"

If it's not just me and folks feel like there are some valid security concerns here, maybe a solr.xml setting or sysprop to gate the use of these parameters would help? i.e. <allowBackupRepoConfigsetUpload>true</allowBackupRepoConfigsetUpload>

If it's not just me and folks feel like there are some valid security concerns here, maybe a solr.xml setting or sysprop to gate the use of these parameters would help? i.e. <allowBackupRepoConfigsetUpload>true</allowBackupRepoConfigsetUpload>

I like this idea, but maybe as an option for the backup repo itself. <allowConfigSetUpload>

gerlowskija · 2026-11-09T12:43:44Z

+      }
+
+      // Read everything before writing so a failure doesn't leave a half-uploaded configset.
+      final Map<String, byte[]> files = new LinkedHashMap<>();


[0] Could this be an OOM risk? if a user accidentally provides a truncated location value it seems like nothing would stop Solr from copying a (potentially large) file tree and keeping each byte on-heap.

gerlowskija · 2026-11-09T14:29:33Z

+          SolrException.ErrorCode.BAD_REQUEST,
+          "'location' is not specified as a parameter or as a default repository property");
+    }
+    if ("file".equals(repository.createURI("x").getScheme())) {


[0] Creating a URI for a non-existent file seems a bit hacky. Could we just use the location string itself, or is there a reason to avoid that?

if ("file".equals(locationUri.getScheme())) { core.getCoreContainer().assertPathAllowed(Paths.get(locationUri)); }

gerlowskija · 2026-11-09T14:32:10Z

+
+  private static final String SOLR_XML =
+      AbstractInstallShardTest.defaultSolrXmlTextWithBackupRepository(
+          "<str name=\"allowPaths\">${solr.security.allow.paths:}</str>\n"


Suggested change

"<str name=\"allowPaths\">${solr.security.allow.paths:}</str>\n"

"<str name=\"allowPaths\">*</str>\n"

If we're always going to set this property to *, we don't need the placeholder variable here...

gerlowskija · 2026-11-09T14:32:47Z

+
+  @BeforeClass
+  public static void setupClass() throws Exception {
+    System.setProperty("solr.security.allow.paths", "*");


Suggested change

System.setProperty("solr.security.allow.paths", "*");

(Only accept this if you take the proposed change on L40 above)

gerlowskija · 2026-11-09T14:36:15Z

+The `overwrite` and `cleanup` parameters work the same way as for zip uploads.
+
+[source,bash]
+----


[0] Love the docs. I'll push a change to reformat the v1/v2 examples so they use the "tabbed" formatting we've been adopting more and more for "paired" examples (e.g. v1/v2, Windows/Linux, etc.)

gerlowskija · 2026-11-09T14:37:36Z

+  }
+
+  private ConfigsetsApi.UploadConfigSet uploadRequest(String configSetName, URI source) {
+    final var request = new ConfigsetsApi.UploadConfigSet(configSetName, emptyBody());


[+1] Love that you're using the generated client objects here, great to get test coverage on these!

gerlowskija · 2026-11-09T14:55:55Z

[-0] This zip file example now appears in the "Upload from Backup Repo" section, which probably isn't what you intended?

I already offered to handle the v1/v2 example tabbing, so I'm going to take a crack at fixing this as well.

nick-boss-tech · 2026-11-09T16:36:52Z

🤖 AI text 🤖

There is also a path-boundary issue in the new local repository import. The allowed-path check is applied to the requested location, but traversal follows symbolic links inside that directory. A ConfigSet editor can therefore cause the import to read outside the intended allowed paths when the local repository contains such a link. The single-file import has the same concern. This depends on the local repository contents and the caller's ConfigSet permissions.

The earlier offered patch includes path checks and symlink regression tests.

SOLR-18618: Upload configsets and configset files from a backup repos…

19f7c83

…itory

HoustonPutman requested a review from gerlowskija November 3, 2026 22:42

github-actions Bot added documentation Improvements or additions to documentation test-framework module:s3-repository module:gcs-repository tests cat:cloud cat:api labels Nov 3, 2026

gerlowskija reviewed Nov 9, 2026

View reviewed changes

Minor doc tweak: v1/v2 example tabbing

7aeb2b6

	"<str name=\"allowPaths\">${solr.security.allow.paths:}</str>\n"
	"<str name=\"allowPaths\">*</str>\n"

Conversation

HoustonPutman commented Nov 3, 2026

Uh oh!

epugh commented Nov 3, 2026

Uh oh!

HoustonPutman commented Nov 3, 2026

Uh oh!

nick-boss-tech commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gerlowskija commented Nov 9, 2026

Uh oh!

gerlowskija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nick-boss-tech commented Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nick-boss-tech commented Nov 5, 2026 •

edited

Loading

nick-boss-tech commented Nov 9, 2026 •

edited

Loading