bamaer commented on PR #8306:
URL: https://github.com/apache/hop/pull/8306#issuecomment-5630549745
## Verdict
The approach is right. The RFC 3986 reasoning in the description is correct:
a query-only `ServiceHandler` genuinely cannot work, and a directory-shaped
path is the only thing that gives the iframe a document base. The layering is
good too — pure path logic in `ExplorerFileServing` with no RAP/SWT types so
`hop-ui` can test it, RAP-specific bits behind an interface, I/O inside
`uiSession.exec` so HopVfs session namespaces resolve correctly, and
origin-relative URLs so a TLS proxy doesn't mixed-content-block the iframe.
The extension allow-list defaults to deny rather than blocking a denylist,
the token is bound to *both* the RAP UI session and the HTTP session rather
than either alone, the desktop path degrades to `setText()` instead of throwing
when a VFS scheme isn't browser-fetchable, and both `web.xml` copies are kept
in sync as the file comment asks. The sandbox tests cover the cases that matter
(`%2e%2e`, `C:/`, nested `../../`, folder, unknown extension).
Four things I'd want addressed before merge.
### 1. No CSP — script in an explorer file runs in the Hop Web origin
`/explorer-file/...` is same-origin with `/ui`. Any `<script>` in an HTML
file the user opens, a directly-navigated `.svg`, or a `.js` under the explorer
root executes with the user's session cookie and can call `/hop/api/v1/*` and
`/hop/*` as that user.
`Browser.setText()` also rendered same-origin, so this isn't wholly new —
but the PR adds `js` and `svg` to the allow-list and makes the content directly
fetchable by URL, which is the moment to close it. The PR's Security section
lists nosniff, no-store, token, sandbox and size cap; CSP is the one missing
lever.
There's a real trade-off, so I'd rather name it than prescribe a header:
- **Strict** (`default-src 'none'; img-src 'self' data:; style-src 'self'
'unsafe-inline'; font-src 'self'`) kills script entirely and keeps everything
the PR is for. It also breaks `updateTitleFromPageTitle()` on Hop Web — that
calls `wBrowser.evaluate("return document.title;")`, which needs to inject
script into the iframe document.
- **Pragmatic** (`connect-src 'none'; form-action 'none'; base-uri 'none'`)
leaves in-page script working so the tab title survives, but blocks the
fetch-the-Hop-API escalation, which is the part that actually matters.
I'd take the pragmatic one unless losing the page-title feature on web is
acceptable. Separately: is `js` needed in the allow-list at all, or is it there
speculatively?
### 2. The lease keeps a stale HTTP session id
`ExplorerFileLease` makes `httpSessionId` final, and on the existing-lease
path `ExplorerFileRegistry.getOrCreate` only calls `lease.setRootVfsUri(...)` —
it never refreshes the session id. Containers rotate the HTTP session id on
authentication (session-fixation defence). After that rotation `sessionMatches`
fails permanently, and every explorer file 404s for the rest of the UI session
with nothing in the UI explaining why.
Cheapest fix: drop the stored id and derive it at check time from
`lease.getUiSession().getHttpSession().getId()`.
### 3. The path is URL-decoded two extra times
`HttpServletRequest.getPathInfo()` is decoded by the container per the
servlet spec. `sanitizeRelativePath` then decodes it again, and
`resolveUnderRoot` sanitizes a *third* time on an already-sanitized string from
`parsePathInfo`.
Not a traversal hole — the `..`, `:` and leading-`/` checks all run after
the decode, and the `%252e%252e` / `%252f` cases walk through to rejection. But
it breaks filenames containing `%`. Verified round trip through
`buildPublicPath` -> container decode -> `sanitizeRelativePath`:
```
file=100% done.html url=100%25%20done.html -> throws
IllegalArgumentException -> swallowed -> 404
file=a%20b.html url=a%2520b.html -> resolves to "a b.html"
(wrong file -> 404)
file=My File.html url=My%20File.html -> OK
```
Sanitize once, at the boundary, and treat the input as already decoded.
### 4. `sanitizeRelativePath` rejects any path containing `:`
`decoded.contains(":")` is meant to catch schemes, but it also rejects legal
filenames on Linux and macOS — `report:2026-09-11.html`. Those files get
`urlFor() == null` and silently fall back to `setText()`, so the exact bug this
PR fixes still bites them, with no diagnostic. Anchor the check to a scheme
prefix on the first segment instead of a substring test.
---
Reviewed from the diff; I did not check out the branch or run the build, so
the "89 tests pass" claim is unverified on my end.
--
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]
