Jerome Blanchard created UNOMI-973:
--------------------------------------
Summary: Confine recurrent import/export file endpoints to
configurable base directories
Key: UNOMI-973
URL: https://issues.apache.org/jira/browse/UNOMI-973
Project: Apache Unomi
Issue Type: Improvement
Components: unomi(-core)
Affects Versions: unomi-3.0.1
Reporter: Jerome Blanchard
### Today
A recurrent import configuration names a `source`, and a recurrent export
configuration names a
`destination`. Both are used as Apache Camel endpoint URIs, and the only thing
that governs them is a
scheme allow-list, `org.apache.unomi.router.config.allowedEndpoints`, whose
shipped default is
`file,ftp,sftp,ftps`.
With `file` in that list, **any absolute path on the Unomi host is accepted** —
including Unomi's own
`deploy/`, `etc/` and `data/` directories, and anything else the process can
reach. A deployment has no
way to say where profile import and export 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:
* A configuration whose endpoint is refused is answered **HTTP 200**. It is
stored, no route is ever
built for it, and the only trace is one `ERROR` line in the log. The caller
cannot tell a working
configuration from one that will never run.
* An endpoint with no scheme — or a blank destination — raises
`StringIndexOutOfBoundsException` out of
`configure()`, because the scheme is read with `substring(0,
uri.indexOf(':'))` without checking the
index. The exception aborts `addRoutes`, so **one malformed configuration
costs the deployment every
other route of the same batch**, silently, at startup.
### Proposal
Let a deployment declare where `file` endpoints may resolve, and refuse the
ones that resolve elsewhere.
Two new settings, one per direction, each accepting a comma-separated list:
```
config.import.baseDir=${org.apache.unomi.router.config.import.baseDir:-${karaf.data}/router/import/}
config.export.baseDir=${org.apache.unomi.router.config.export.baseDir:-${karaf.data}/router/export/}
```
They are kept apart on purpose: with a single shared directory, an export
writing a `.csv` could be
picked up by an import route polling the same place. A deployment that wants
one directory can point
both settings at it.
### Behaviour
* Containment is **recursive** — any depth under a permitted base directory is
accepted — and does not
require the directory to exist, since an export destination is created on
first write.
* It covers the directory the URI names **and every path-bearing option it
carries** (`fileName`,
`tempFileName`, `move`, `moveFailed`, `preMove`, `doneFileName`, `include`,
`antInclude`,
`antFilter`). Validating only the directory would let an option resolve
elsewhere and quietly defeat
the setting.
* It is decided on canonical paths: percent-encoding decoded, `RAW(...)`
unwrapped, parent segments
normalized, symbolic links followed. Paths are 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.
* Relative option values keep working — `move=.done` resolves under the
endpoint's own directory, which
is how the feature is normally used, and the router appends
`moveFailed=.error` itself.
* `ftp`, `sftp` and `ftps` carry no local path and are unaffected; the scheme
allow-list keeps governing
them.
* The **oneshot** import route builds its own endpoint from
`import.oneshot.uploadDir` and is
unaffected. Oneshot configurations name no endpoint and are not validated.
### Reporting the refusal
`saveConfiguration` now validates the endpoint of a recurrent configuration
**before storing it**, and
answers **`400 Bad Request`** with the reason in the body as `text/plain`. The
configuration is not
stored, so the caller gets a synchronous, actionable answer instead of a `200`
followed 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_ENDPOINT` is deliberately not one of the existing 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
automatically — the record of a run that genuinely failed is left alone.
### Robustness
* The scheme is matched against the allow-list as a **set of whole schemes**,
rather than searching the
raw setting for a substring.
* An endpoint with no scheme, or a blank destination, is reported and skipped
instead of raising out of
`configure()`; the other configurations of the batch keep their routes.
### Compatibility
**Breaking.** A recurrent import or export configuration using `file` and
resolving outside the
permitted base directories stops building its route, and is marked
`INVALID_ENDPOINT`. A deployment that
relies on another location must set `config.import.baseDir` /
`config.export.baseDir` accordingly. This
needs a release note.
### Tests
* `FileEndpointContainmentTest` (router-core) — route construction for both
directions: in-bounds
sources 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,
a genuinely 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; a
oneshot import that names no endpoint is still stored.
* Existing integration tests updated to the permitted directories:
`ProfileExportIT`,
`ProfileImportSurfersIT`, `ProfileImportActorsIT`, `ProfileImportRankingIT`.
`ProfileImportBasicIT`
(oneshot) is unchanged, which is the point.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
