jnioche commented on PR #9182:
URL: https://github.com/apache/storm/pull/9182#issuecomment-6359006949
Thanks for working on this. I reviewed the diff and found a few gaps that I
think should be addressed before merging.
### Correctness / security
1. **Deep search still follows symlinked port/topology directories**
(`LogviewerLogSearchHandler.deepSearchLogsForTopology`). It never calls
`isWorkerDirWithoutSymbolicLink` / `isServableWorkerFile`, and both
`logsForPort` and `openForReading(file, false)` only reject a link at the
*last* path element. With the same layout as
`testListLogFilesLeavesOutSymbolicLinks` (`topoA/2222 -> topoB/1111`), a deep
search on `topoA` lists `topoA/2222/worker.log`, which passes
`isRegularFileNotLink`, gets authorized as a topoA file, and returns topoB's
log content. The single-port branch has the same issue, since it only checks
`exists()`.
2. **`openForReading` doesn't check that the opened file is a regular file**
(`LogFilePaths`). Only the symlink TOCTOU is closed. In runAsUser mode the
worker owns its port directory, so it can replace `worker.log` with a FIFO
after the check passes. `Files.newByteChannel(READ, NOFOLLOW_LINKS)` on a FIFO
blocks until a writer appears, which hangs the Jetty thread, and repeated
requests can exhaust the pool. The fix is to check
`Files.readAttributes(...)`/`isRegularFile` on the opened channel (or open with
`O_NONBLOCK`-equivalent semantics and verify afterwards). Dump downloads have
the same problem.
3. **The stream passed to `buildDownloadFile` is never closed**
(`LogviewerResponseBuilder`). `wrapWithStreamingOutput` copies and flushes but
doesn't close the input, and Jersey doesn't close it for a `StreamingOutput`
entity. Every log/dump download, including an aborted one, leaks a file
descriptor, and deleted or rotated logs keep their disk space until the process
exits. Closing it in a `finally` inside the `StreamingOutput` lambda would fix
it. The existing "do not close" comment should go too.
4. **`listDumpFiles` doesn't validate the port directory**
(`LogviewerProfileHandler`). Unlike `listLogFiles`, it never calls
`isWorkerDirWithoutSymbolicLink`, and `topologyId` can contain an encoded `/`
(e.g. `topoA/1111` with hostPort `h:123`). The resulting `topoA/1111/123` can
be a symlink to an arbitrary directory, so the names of the regular files in it
end up on the dump list page.
5. **`getPort` breaks for hosts containing `:`**
(`LogviewerProfileHandler`). `hostPort.split(":")[1]` is wrong for IPv6
(`[::1]:6700` gives `""`), and since the port must now be numeric this becomes
a 404. Using `lastIndexOf(':')` would fix both the port and the host part used
in the download file name.
### API / maintainability
6. The public static `LogviewerResponseBuilder.buildDownloadFile(String,
File, Meter)` is removed without deprecation in a 2.8.x patch release, while
`ServerUtils.zipFileSize` is only deprecated. Could the old signature be kept
(deprecated) for consistency?
7. Each handler repeats the "`isServableWorkerFile` → `openForReading` → 404
on null → wrap → `buildDownloadFile`" sequence, which is how deep search and
`listDumpFiles` (points 1 and 4) missed the checks. A single helper, e.g.
`LogFilePaths.openWorkerFile(logRoot, relativeName)` that resolves, validates
and opens the file, would enforce this in one place.
8. Minor: `rootDir.toRealPath()` runs on every request but could be computed
once per handler. The `Paths.get(fileName).getNameCount() != 1` check in
`downloadDumpFile` looks redundant with the `isServableWorkerFile` path-depth
check that follows.
### Tests
9. The new tests call `Files.createSymbolicLink` without an
`assumeTrue`/`assumeFalse` guard, so they will error rather than skip on
Windows or anywhere creating symlinks needs privilege. Also,
`testDeepSearchLogsForTopologyDoesNotFollowSymbolicLinks` only covers a
symlinked *file* (`worker.log.1`), not a symlinked port directory, which is why
point 1 isn't caught. A test for that case would be good.
--
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]
