[
https://issues.apache.org/jira/browse/UNOMI-973?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jerome Blanchard updated UNOMI-973:
-----------------------------------
Description:
*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.
was:
*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.
> 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
> Priority: Major
>
> *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)
