setup local zarr server#8
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to serve local Zarr stores via a built-in HTTP server. Key changes include the addition of serve_zarr.jl for handling file requests and range headers, updates to browzarr to automatically spin up this server when a directory is provided, and new dependencies on Zarr and Sockets. Feedback focused on security and robustness in the new server implementation, specifically addressing a path traversal vulnerability, improving Zarr metadata detection, validating HTTP range requests, and optimizing file serving for large datasets.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces functionality to automatically serve local Zarr stores via a built-in HTTP server, adding dependencies on Sockets and Zarr and updating the main entry point to handle directory-based stores. The review highlights several important issues, including a file descriptor leak when responding with open IO objects, a regression in hostname resolution for server binding, and potential crashes when parsing URLs without explicit ports. Additionally, feedback was provided regarding insecure network interface binding and potential integer overflow on 32-bit systems when handling large file offsets.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for browsing local Zarr directories by implementing a local HTTP server that handles range requests and CORS. Key changes include the addition of the serve_zarr utility, automatic server initialization for directory-based stores, and dynamic port allocation. The code review identifies several critical and medium-severity issues, including a path traversal vulnerability in the Zarr server, potential memory exhaustion when handling large range requests, and missing error monitoring for asynchronous tasks. Additionally, feedback was provided regarding a protocol hardcoding bug in the server health check and the need to handle failure states when waiting for the server to start.
With this PR now we can
browzarr()callsopening directly the
ncfiles still fails, but that was an issue in the upstream package, solved here: EarthyScience/Browzarr#645 It should work on the next npm release artifacts.