jsinovassin opened a new pull request, #857: URL: https://github.com/apache/unomi/pull/857
This targets `UNOMI-973-file-endpoint-containment` rather than `unomi-3.0.x`, so merging it folds these changes into [unomi#849](https://github.com/apache/unomi/pull/849) and leaves that PR as the single unit of review. Take what you agree with and drop the rest — each commit stands on its own. They come out of a careful pass over the diff of [unomi#849](https://github.com/apache/unomi/pull/849). The design there holds up; what follows are the edges it leaves open, in decreasing order of how much they matter. ## What each commit changes **Resolving a path one component at a time.** `canonicalize` normalized the whole path before following its symbolic links. Collapsing parent segments first erases the component they cancel, symbolic link included: `/base/link/..` read as `/base`, while the file system expands the link and applies the segment to its target, landing wherever that link points. A source written that way was accepted and its route was built. The path is now walked from the root: a link is expanded where it stands, and a parent segment applies to what the walk has resolved so far. What does not exist yet is kept as written, so an export destination is still decided on before it is created, and an existing part that cannot be resolved is still a refusal. **Confining `localWorkDirectory` whatever the scheme.** A scheme other than `file` was answered valid without its options being looked at, on the grounds that a remote endpoint carries no local path. `localWorkDirectory` is the exception. The FTP and SFTP components stage the content they download into files there: `FtpOperations.retrieveFileToFileInLocalWorkDirectory` writes `new File(localWorkDirectory, <the name the remote server announces>)` and creates the directories on the way. So `ftp://host/x?localWorkDirectory=<karaf.home>/deploy` was accepted, and `ftp` is in the shipped allow-list. The option now answers to the permitted base directories whatever the scheme. It stays out of `PATH_BEARING_OPTIONS` because Camel resolves it on its own rather than against the directory the endpoint names, and it carries no File Language expression to account for. **Appending `moveFailed` as an option rather than to the directory name.** The router appends its own `moveFailed` to the configured source with an unconditional `&`. A source that carries no query has no separator to append to, so the option became part of the directory the endpoint names: `file:///base` turned into `file:///base&moveFailed=.error`, a directory called `base&moveFailed=.error`. That was a malformed endpoint before this validation existed; with it, the two layers disagree — `saveConfiguration` validates the source as configured and answers 200, then the route builder validates the source with the option appended, refuses it and marks the configuration `INVALID_ENDPOINT`. Stored, answered 200, never run, which is what refusing at save time exists to prevent. **A system property and a manual entry for the two new settings.** `org.apache.unomi.router.cfg` reads `config.import.baseDir` and `config.export.baseDir` from system properties, but neither was declared in `custom.system.properties`, so unlike every other router setting they had no env-var override. A deployment that has to relocate its import directory — which this change forces on anyone whose configurations resolve elsewhere — could only do it by editing `etc/org.apache.unomi.router.cfg` inside the image. The manual's list of router properties now names them too. **Not reading a setting the router has not published yet as a refusal.** The REST endpoint validates against the settings the router's Camel context publishes on start-up, but it answers as soon as its own component is satisfied, which can be first. `getProperty` then returns `null`, an empty scheme allow-list follows from it, and a good configuration is answered `400 endpoint scheme 'file' is not allowed` — pointing its author at a scheme that was never the problem. An absent setting now answers 503 with what is actually the matter. **Keeping a failure to record the refusal from costing the batch its routes.** `recordEndpointOutcome` writes to the store from inside `configure()`, which builds the routes of every configuration of the batch. With the store unreachable — Elasticsearch not up yet at start-up, say — the exception leaves `addRoutes` and every other configuration loses its route, silently. That is the failure mode this validation exists to remove, reached through the report of a refusal rather than the refusal itself. The write is now logged and swallowed; the route is built or skipped exactly as decided, and only the record of it can go missing. ## On the `localWorkDirectory` one, since the claim it contradicts is explicit It was measured rather than argued. A harness — a minimal FTP server plus a Camel route built from the URI, no running Unomi — samples the target directory every 2 ms, because Camel deletes its local work file when the exchange completes (`GenericFileProcessStrategySupport.deleteLocalWorkFile`, called from `commit`, `rollback` and `abort`). Three runs, before the fix: - nominal transfer with a trivial route: `evil.jar.inprogress` then `evil.jar` appear and are cleaned up within about 7 ms; - the same with the exchange lasting 3 s, which is what a real CSV split and profile import costs: `evil.jar` — the attacker's name, the attacker's bytes — stands in the target directory from 501 ms to 3513 ms, the whole duration of the exchange, which the deploy watcher polls; - the remote server dropping the data connection mid-transfer: `evil.jar.inprogress`, 100 012 bytes of remote content, **is still there when the route stops**. `deleteLocalWorkFile` removes the final path, never the `.inprogress` temp file. So the deterministic part is a permanent write of remote content at an arbitrary absolute path, with a `.inprogress` suffix and therefore not directly deployable; the racy part is the exact chosen name standing for the duration of the exchange. Either way the invariant this PR establishes — that a recurrent import touches nothing outside the declared base directories — does not hold. Happy to attach the harness if it is useful. ## Tests Each of the five behavioural commits carries a test that fails without it, checked by reverting the fix and watching exactly the expected tests go red. The seventh commit covers what those changes touched and nothing yet observed: a path-bearing option leaving through a link and a parent segment; a link that stays inside the base directory still building its route; a base directory that is itself a link still matching the directory it points at; more parent segments than a path has components being a refusal rather than an exception; the export direction and the REST layer for `localWorkDirectory`, and the option matched on its name rather than on how it is spelt; the endpoint URI the route is actually built on; and the export endpoint and a partly published configuration for the 503. 71 tests over `router-api`, `router-core` and `router-rest`, all green. No integration test was added — `ProfileImportExportContainmentIT` already covers the shape of this, and none of these changes alters what it asserts. ## Two things for you to decide The 503 was a judgement call. Answering "not ready" rather than deferring validation is the conservative reading — nothing is stored, the caller retries — but deferring is defensible too, and it is your call. The tests are in a seventh commit rather than folded into the six, because hunks from several of them share the same test files and splitting that cleanly is worth less than it costs. Say the word and I will rebase them into their commits. -- 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]
