[
https://issues.apache.org/jira/browse/UNOMI-973?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Serge Huber updated UNOMI-973:
------------------------------
Fix Version/s: unomi-3.1.0
unomi-3.0.2
> 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
> Fix For: unomi-3.1.0, unomi-3.0.2
>
> Time Spent: 20m
> Remaining Estimate: 0h
>
> *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)
