jnioche commented on PR #9182: URL: https://github.com/apache/storm/pull/9182#issuecomment-6335425353
AI assisted review Verdict: approve with minor changes. The approach is sound: worker files are limited to topologyId/port/file, checked with toRealPath against the real root, opened with NOFOLLOW_LINKS, and the length is read from the opened channel. The race argument holds because only the port dir is handed to the topology user (Container.setupWorkerArtifactsDir). The topology dir stays owned by the supervisor. That assumption is what makes the race argument work, so it’s worth stating explicitly in the LogFilePaths javadoc. Should fix /listLogs isn’t covered. listLogFiles and WorkerLogs.getAllLogsForRootDir still use getFilesForDir and Files::isDirectory without filtering links. Symlinks therefore still show up in that listing, which contradicts “left out of listings”. Only names leak, not content, but it’s inconsistent and there’s no test for it. Losing the race gives a 500, not a 404. If the file is swapped for a link between the check and the open, NOFOLLOW_LINKS throws ELOOP. That surfaces as an IOException and marks the open/download exception meters. It’s safe, but it isn’t “answered like a missing file”. You could catch FileSystemException there and return not-found. Worth noting 3. Behaviour change in LogFileDownloader. It used to serve files at depth ≠ 3 (the old getNameCount() != 3 branch); those now return 404. That’s probably fine, since listings were never recursive, but it should go in the PR description or release notes. 4. Hard links aren’t detected. toRealPath can’t tell a hard link from the original, so protection relies on fs.protected_hardlinks=1. One line in the javadoc would cover it. Nits The gzip size logic now exists twice: the daemon page still uses getFileLength/ServerUtils.zipFileSize. It could use LogFilePaths.fileLength instead. In LogFileDownloader, the duplicated isDaemon branch around buildDownloadFile could compute the open options once. In LogviewerProfileHandler, the getNameCount() != 1 check is redundant with the depth check in isServableWorkerFile. The 28 new tests cover links to another topology, links outside the root, a linked root, nested files, .gz lengths, and deep search. The only gap is item 1. -- 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]
