RexXiong commented on PR #3674: URL: https://github.com/apache/celeborn/pull/3674#issuecomment-4446486094
Thanks for the fix! The path traversal issue is real and the approach is solid. One suggestion: I think the `validateAppId` check is only needed in `Controller` (ReserveSlots, CommitFiles, DestroyWorkerSlots). The reason is that PushData and Fetch operations require the shuffle to already be registered via `ReserveSlots` — if the appId was never accepted by the Controller, subsequent push/fetch calls will fail anyway because the shuffle metadata doesn't exist. So the checks added in `PushDataHandler.handleCore` and `FetchHandler` are effectively redundant — a malicious appId can never reach the filesystem layer through those paths without first passing through the Controller. That said, the canonical path containment check in `StorageManager` is worth keeping as defense-in-depth — it's cheap and guards against potential future RPC entry points that might bypass the Controller. -- 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]
