[UNOMI-973] - Confine recurrent import/export file endpoints to configurable base directories - #849
Open
jayblanc wants to merge 5 commits into
Open
[UNOMI-973] - Confine recurrent import/export file endpoints to configurable base directories#849jayblanc wants to merge 5 commits into
jayblanc wants to merge 5 commits into
Conversation
A recurrent import configuration names a source, and a recurrent export configuration names a destination; both are used as Camel endpoint URIs. When the scheme is file, the URI must resolve inside a base directory the deployment permits -- the directory it names, and every path-bearing option it carries (fileName, move, moveFailed, preMove, doneFileName, include). FileEndpointContainmentTest covers route construction for both directions: in-bounds sources and destinations keep building routes, at any depth under the base directory and whether or not the directory exists yet; out-of-bounds ones build none. Remote schemes carry no local path and stay unaffected. Carry the permitted base directories into the route builders, separately for each direction, so a route builder cannot be wired with the wrong list. They are not read yet, so the containment tests fail. Two more fail on their own ground: a source without a scheme, and a blank destination, each raise StringIndexOutOfBoundsException out of configure() and cost the deployment the other routes of the same batch.
…ectories EndpointValidator decides whether a configured endpoint URI may be used: its scheme must belong to the allow-list, and a file endpoint must resolve inside one of the base directories the deployment permits. Containment covers the directory the URI names and every path-bearing option it carries, and is decided on canonical paths -- percent-encoding decoded, RAW() unwrapped, parent segments resolved, symbolic links followed -- compared component by component, so a sibling sharing a textual prefix is not taken for a child. It is recursive and does not require the directory to exist, since an export destination is created on first write. Two new settings carry the base directories, one per direction, defaulting under karaf.data: an export cannot then write into a directory an import route polls. Route builders receive them through direction-specific setters, so neither can be wired with the other's list. The scheme test now matches whole schemes rather than searching the raw setting, and an endpoint with no scheme, or a blank destination, is reported and skipped instead of raising StringIndexOutOfBoundsException out of configure() -- which cost the deployment every other route of the batch. The oneshot upload route builds its own endpoint and is unaffected. Remote schemes carry no local path and stay governed by the scheme allow-list alone. Recurrent file configurations resolving outside the permitted directories stop building routes, which is a behaviour change for existing deployments.
…hen it is saved The route carrying an import or export configuration is built asynchronously, long after the REST call has answered. A configuration whose endpoint is refused there was still stored and still answered 200, leaving one log line as the only trace -- the caller could not tell it from one that works. Both configuration endpoints now validate the endpoint URI of a recurrent configuration before storing it, and answer 400 Bad Request carrying the reason, so the caller can correct it. A oneshot import names no endpoint -- its file is uploaded separately -- and is unaffected. RouterCamelContext publishes the scheme allow-list and the permitted base directories through ConfigSharingService, the way it already publishes the oneshot upload directory, since router-rest cannot see the configuration router-core is wired with.
…ured The permitted directories are an operational setting and the configurations are user data, so the two drift apart: a configuration that was legitimate when it was created is refused once the deployment is reconfigured. Refusing it silently left its owner with a configuration that looks fine and does nothing, the only trace being a log line. A configuration whose endpoint is refused while its route is built is now marked INVALID_ENDPOINT and saved, so it can be seen and dealt with. Nothing is deleted and no restart is blocked: correcting or removing it belongs to whoever owns it. Restoring the permitted directories clears the mark on its own at the next rebuild, so operations can undo a change without anyone touching the configurations. The status is its own value rather than one of the execution statuses: those report on a run that happened, this one says no run can. Keeping them apart is what makes the mark safe to clear -- the record of a run that genuinely failed is left alone. Saving is done without asking for a route refresh, which would rebuild the route, refuse it again and save it again, without end.
The unit tests decide the containment rules; what only a running Unomi can show is that the settings reach the two places that read them, through paths nothing else exercises: the REST layer, which refuses a configuration as it is saved, and the route builders, which refuse one that is already stored. ProfileImportExportContainmentIT covers both. A configuration outside the permitted directories is answered 400 with a reason in the body and is not stored; one inside is accepted. One stored while bypassing the REST layer -- the configuration that was already there when the deployment was reconfigured -- is marked INVALID_ENDPOINT, consumes no file and writes none, and recovers on its own once its endpoint is acceptable again. The existing recurrent tests move to the permitted directories, which is what their configurations now have to name. ProfileImportBasicIT is left untouched on purpose: the oneshot upload builds its own endpoint, and its passing unchanged is what shows it stayed out of this.
Contributor
There was a problem hiding this comment.
Pull request overview
Confines recurrent file imports and exports to configurable directories and reports invalid endpoints clearly.
Changes:
- Adds shared endpoint validation and directional base-directory settings.
- Returns HTTP 400 for refused configurations and records
INVALID_ENDPOINTduring route rebuilding. - Adds unit and integration coverage for containment and recovery.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
itests/src/test/resources/org.apache.unomi.router.cfg |
Configures permitted test directories. |
itests/src/test/java/org/apache/unomi/itests/ProfileImportExportContainmentIT.java |
Tests runtime containment behavior. |
itests/src/test/java/org/apache/unomi/itests/ProfileExportIT.java |
Uses the permitted export directory. |
itests/src/test/java/org/apache/unomi/itests/AllITs.java |
Registers the new integration test. |
extensions/router/router-rest/src/test/java/org/apache/unomi/router/rest/ConfigurationEndpointValidationTest.java |
Tests REST validation and refusal. |
extensions/router/router-rest/src/main/java/org/apache/unomi/router/rest/ImportConfigurationServiceEndPoint.java |
Validates recurrent import sources. |
extensions/router/router-rest/src/main/java/org/apache/unomi/router/rest/ExportConfigurationServiceEndPoint.java |
Validates recurrent export destinations. |
extensions/router/router-rest/src/main/java/org/apache/unomi/router/rest/AbstractConfigurationServiceEndpoint.java |
Provides shared HTTP refusal handling. |
extensions/router/router-rest/pom.xml |
Adds JUnit test support. |
extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/RefusedConfigurationStatusTest.java |
Tests invalid status lifecycle. |
extensions/router/router-core/src/test/java/org/apache/unomi/router/core/route/FileEndpointContainmentTest.java |
Tests route-level containment rules. |
extensions/router/router-core/src/main/resources/OSGI-INF/blueprint/blueprint.xml |
Wires base-directory settings. |
extensions/router/router-core/src/main/resources/org.apache.unomi.router.cfg |
Defines production defaults. |
extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/RouterAbstractRouteBuilder.java |
Persists endpoint validation outcomes. |
extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileImportFromSourceRouteBuilder.java |
Enforces import containment. |
extensions/router/router-core/src/main/java/org/apache/unomi/router/core/route/ProfileExportCollectRouteBuilder.java |
Enforces export containment. |
extensions/router/router-core/src/main/java/org/apache/unomi/router/core/context/RouterCamelContext.java |
Propagates settings to REST and routes. |
extensions/router/router-core/pom.xml |
Adds JUnit test support. |
extensions/router/router-api/src/main/java/org/apache/unomi/router/api/RouterConstants.java |
Adds shared settings and status constants. |
extensions/router/router-api/src/main/java/org/apache/unomi/router/api/EndpointValidator.java |
Implements endpoint and path validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+113
to
+124
| for (String[] parameter : parseQuery(query)) { | ||
| if (!PATH_BEARING_OPTIONS.contains(parameter[0].toLowerCase(Locale.ROOT))) { | ||
| continue; | ||
| } | ||
| String value = stripRaw(decode(parameter[1])); | ||
| if (value.isEmpty()) { | ||
| continue; | ||
| } | ||
| if (!isContained(directory.resolve(value), baseDirs)) { | ||
| return "option '" + parameter[0] + "' points outside the permitted directories"; | ||
| } | ||
| } |
Comment on lines
+117
to
+121
| String value = stripRaw(decode(parameter[1])); | ||
| if (value.isEmpty()) { | ||
| continue; | ||
| } | ||
| if (!isContained(directory.resolve(value), baseDirs)) { |
| return null; | ||
| } | ||
|
|
||
| return validateContainment(endpointUri, permittedBaseDirs); |
Comment on lines
+206
to
+220
| ByteArrayOutputStream decoded = new ByteArrayOutputStream(value.length()); | ||
| for (int i = 0; i < value.length(); i++) { | ||
| char character = value.charAt(i); | ||
| if (character == '%' && i + 2 < value.length()) { | ||
| int high = Character.digit(value.charAt(i + 1), 16); | ||
| int low = Character.digit(value.charAt(i + 2), 16); | ||
| if (high >= 0 && low >= 0) { | ||
| decoded.write((high << 4) + low); | ||
| i += 2; | ||
| continue; | ||
| } | ||
| } | ||
| decoded.write(character); | ||
| } | ||
| return new String(decoded.toByteArray(), StandardCharsets.UTF_8); |
Comment on lines
+163
to
+166
| try { | ||
| return existing.toRealPath().resolve(existing.relativize(normalized)); | ||
| } catch (IOException e) { | ||
| return normalized; |
Comment on lines
+57
to
+59
| private static final Set<String> PATH_BEARING_OPTIONS = Collections.unmodifiableSet(new HashSet<>(Arrays.asList( | ||
| "filename", "tempfilename", "move", "movefailed", "premove", "donefilename", | ||
| "include", "antinclude", "antfilter"))); |
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.
A recurrent import configuration names a
source, and a recurrent export configuration names adestination. Both are used as Apache Camel endpoint URIs, and the only thing that governs them is ascheme allow-list,
org.apache.unomi.router.config.allowedEndpoints, whose shipped default isfile,ftp,sftp,ftps.With
filein that list, any absolute path on the Unomi host is accepted — including Unomi's owndeploy/,etc/anddata/directories. A deployment has no way to say where profile import andexport files are supposed to live, and an administrator has no way to know they have typed a path that
will disrupt the installation.
Two smaller problems sit beside it:
built for it, and the only trace is one
ERRORline in the log. The caller cannot tell a workingconfiguration from one that will never run.
StringIndexOutOfBoundsExceptionout ofconfigure(), because the scheme is read withsubstring(0, uri.indexOf(':'))without checking theindex. The exception aborts
addRoutes, so one malformed configuration costs the deployment everyother route of the same batch, silently, at startup.
What this changes
Two new settings let a deployment declare where
fileendpoints may resolve, one per direction, eachaccepting a comma-separated list:
They are kept apart on purpose: with a single shared directory, an export writing a
.csvcould bepicked up by an import route polling the same place. A deployment that wants one directory can point
both settings at it.
EndpointValidator(router-api, so both router-core and router-rest can use it) decides whether anendpoint may be used:
require the directory to exist, since an export destination is created on first write;
fileName,tempFileName,move,moveFailed,preMove,doneFileName,include,antInclude,antFilter), since validating only the directory would let an option resolve elsewhere and quietlydefeat the setting;
RAW(...)unwrapped, parent segmentsnormalized, symbolic links followed — compared component by component, so a sibling directory that
merely shares a textual prefix with a permitted one is not taken for one of its children;
move=.doneresolves under the endpoint's own directory, whichis how the feature is normally used, and the router appends
moveFailed=.erroritself;ftp,sftpandftpscarry no local path and are unaffected;import.oneshot.uploadDirand isuntouched.
Reporting the refusal
saveConfigurationvalidates the endpoint of a recurrent configuration before storing it, and answers400 Bad Requestwith the reason in the body astext/plain. The configuration is not stored, sothe caller gets a synchronous, actionable answer instead of a
200followed by silence.The permitted directories are an operational setting while the configurations are user data, so the two
drift apart: a configuration that was legitimate when it was created is refused once the deployment is
reconfigured. To keep that visible rather than silent, a configuration whose route cannot be built is
marked with a new status,
INVALID_ENDPOINT, and saved. Nothing is deleted and no startup is blocked —correcting or removing it belongs to whoever owns it. Restoring the permitted directories clears the
mark on its own at the next rebuild, so an operational change can be undone without touching any
configuration.
INVALID_ENDPOINTis deliberately not one of the existing execution statuses: those report on a runthat happened, this one says no run can. Keeping them apart is what makes the mark safe to clear
automatically — the record of a run that genuinely failed is left alone.
Robustness
setting for a substring.
configure(); the other configurations of the batch keep their routes.Compatibility
Breaking. A recurrent import or export configuration using
fileand resolving outside thepermitted base directories stops building its route, and is marked
INVALID_ENDPOINT. A deploymentthat relies on another location must set
config.import.baseDir/config.export.baseDiraccordingly. This needs a release note.
Tests
Unit tests, in the modules that hold the behaviour:
FileEndpointContainmentTest(router-core) — route construction for both directions: in-boundssources and destinations keep building routes, at any depth and whether or not the directory exists;
out-of-bounds ones build none, including through each path-bearing option, encoded parent segments,
RAW(), symbolic links, and prefix-sharing siblings.RefusedConfigurationStatusTest(router-core) — the status is set on refusal, cleared on recovery, agenuinely failed run keeps its own status, and the save does not schedule a route refresh, which
would rebuild, refuse and save again without end.
ConfigurationEndpointValidationTest(router-rest) — refusal answers 400 and stores nothing; aoneshot import that names no endpoint is still stored.
Integration tests, for what only a running Unomi can show — that the settings reach the REST layer and
the route builders, which read them through different paths:
ProfileImportExportContainmentIT(new) — refusal at save time answered 400 with a reason in thebody and nothing stored; a configuration stored while bypassing the REST layer is marked
INVALID_ENDPOINT, consumes no file and writes none, and recovers on its own.ProfileExportIT,ProfileImportSurfersIT,ProfileImportActorsIT,ProfileImportRankingITnowname the permitted directories.
ProfileImportBasicITis untouched on purpose: its passing unchangedis what shows the oneshot upload stayed out of this.
[UNOMI-XXX] - Title of the pull requestmvn clean install -P integration-teststo make sure basic checks passOn that last box, to be accurate rather than reassuring: the full integration-test suite has not
been run locally. What was run is every unit test of
router-api,router-coreandrouter-rest, andthe six integration-test classes this change touches or adds —
ProfileImportExportContainmentIT,ProfileExportIT,ProfileImportSurfersIT,ProfileImportActorsIT,ProfileImportRankingITandProfileImportBasicIT, all passing. The rest of the suite is left to CI.