jayblanc opened a new pull request, #849:
URL: https://github.com/apache/unomi/pull/849
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. 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.
## What this changes
Two new settings let a deployment declare where `file` endpoints may
resolve, 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.
`EndpointValidator` (router-api, so both router-core and router-rest can use
it) decides whether an
endpoint may be used:
- 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`), since 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 — 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 **oneshot** import route builds its own endpoint from
`import.oneshot.uploadDir` and is
untouched.
## Reporting the refusal
`saveConfiguration` 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
Unit tests, in the modules that hold the behaviour:
- `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.
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 the
body 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`,
`ProfileImportRankingIT` now
name the permitted directories. `ProfileImportBasicIT` is untouched on
purpose: its passing unchanged
is what shows the oneshot upload stayed out of this.
---
- [x] Make sure there is a [JIRA
issue](https://issues.apache.org/jira/browse/UNOMI-973) filed for the change —
UNOMI-973
- [x] Format the pull request title like `[UNOMI-XXX] - Title of the pull
request`
- [x] Provide integration tests for your changes
- [x] Write a pull request description that is detailed enough to
understand what the pull request does, how, and why
- [ ] Run `mvn clean install -P integration-tests` to make sure basic
checks pass
On 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-core` and `router-rest`, and
the six integration-test classes this change touches or adds —
`ProfileImportExportContainmentIT`,
`ProfileExportIT`, `ProfileImportSurfersIT`, `ProfileImportActorsIT`,
`ProfileImportRankingIT` and
`ProfileImportBasicIT`, all passing. The rest of the suite is left to CI.
- [ ] I hereby declare this contribution to be licenced under the [Apache
License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
