gridlook-jupyter: server extension + S3 range proxy (phase 4 of issue #1)#5
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
| result = await obstore.get_async(store, key, options=options) | ||
| except (NotFoundError, FileNotFoundError) as e: | ||
| raise web.HTTPError(404, f"no such object: s3://{bucket}/{key}") from e | ||
| except (BaseError, OSError) as e: |
There was a problem hiding this comment.
🤖 from Claude (review)
Malformed keys raise an uncaught ValueError → HTTP 500 (should be 400). obstore's path parser rejects keys containing .. or empty segments with a plain builtins.ValueError (Could not parse path), which is neither NotFoundError/FileNotFoundError nor BaseError/OSError, so it escapes both except clauses here (and the identical block in head()). Probed against the live proxy with the test fixtures:
s3/test-bucket/../secret/passwd.bin -> 500
s3/test-bucket/data/../secret/x -> 500
s3/test-bucket/....//x -> 500
Good news first: this is not a traversal breach — obstore rejects the path before any I/O, so nothing escapes the bucket prefix, and your write_error override suppresses the traceback body (client sees only error 500). But the status is wrong (a malformed client request is a 400, not a 500) and each hit logs a full traceback server-side. No test covers a malformed key. Suggest catching ValueError here and in head() → HTTPError(400, ...).
There was a problem hiding this comment.
🤖 from Claude
Fixed in 16796da: both get() and head() now catch obstore's ValueError and return 400 invalid key: <key> (no traceback logged). Added parametrized GET+HEAD tests for ../secret/x, a/../b, ....//x asserting 400 (no 500). Also gitignored the stray jupyter/uv.lock in the same commit.
| raise web.HTTPError(404, f"no such object: s3://{bucket}/{key}") from e | ||
| except (BaseError, OSError) as e: | ||
| # Includes out-of-bounds ranges; obstore surfaces them as generic errors. | ||
| raise web.HTTPError(502, f"S3 error for s3://{bucket}/{key}: {e}") from e |
There was a problem hiding this comment.
🤖 from Claude (review)
Unsatisfiable ranges surface as 502 instead of 416. An out-of-bounds range is raised by obstore as GenericError (a BaseError subclass, "Requested range was invalid"), so it is caught here and mapped to 502 Bad Gateway — mislabeling a client range error as an upstream/gateway fault. RFC 9110 §15.5.17 calls for 416 Range Not Satisfiable. Probed against the live proxy (object is 1024 B):
Range: bytes=999999-1000000 -> 502
Related parser edge: Range: bytes=-0 yields {"suffix": 0} → a 206 with a malformed Content-Range: bytes 1024-1023/1024 (last-byte-pos < first-byte-pos) and a 0-byte body. Both are arguably 416. zarrita only issues bounded in-range requests, so this is low severity, but a client probing EOF gets a confusing 502 rather than a clean 416. Consider mapping the "range invalid" error → 416 and treating bytes=-0 as unsatisfiable.
There was a problem hiding this comment.
🤖 from Claude
Fixed in 2b7a4ae: an out-of-range obstore error on a range request now returns 416 with Content-Range: bytes */<size>, and bytes=-0 is rejected as unsatisfiable (416) at parse time. On the size choice: I emit the numeric bytes */<size> (one HEAD on this cold error path), not bytes */* — RFC 9110 §14.4 requires a numeric complete-length in the unsatisfied-range form, so */* is only a fallback if that HEAD itself fails. Added tests: OOB and bytes=-0 → 416 with bytes */1024; existing 206 byte-exact tests stay green.
| raise RuntimeError(f"gridlook-jupyter: npm run build produced no {dist}/index.html") | ||
| if static.exists(): | ||
| shutil.rmtree(static) | ||
| shutil.copytree(dist, static) |
There was a problem hiding this comment.
🤖 from Claude (review)
The wheel ships JS source maps. vite.config.ts sets build.sourcemap: true, so npm run build emits dist/assets/*.js.map, and this copytree(dist, static) copies them wholesale into gridlook_jupyter/static/ → they ride along in the distributed pip wheel. That is avoidable bloat (maps are often comparable in size to the bundles themselves) in a server-extension artifact that never needs them, and the CI unzip -l | grep index.html assertion would not notice them. Low severity — the frontend is open source, so this is not a secret disclosure — but worth stripping the .map files after copy (or building the wheel with sourcemaps off) so the wheel stays lean.
There was a problem hiding this comment.
🤖 from Claude
Fixed in e0231b5: the build hook now copies dist → static with ignore_patterns("*.map"), so the wheel no longer ships sourcemaps (vite.config.ts is untouched — dev/standalone builds keep their maps). Added a CI step in jupyter.yml asserting ! unzip -l *.whl | grep .map$. Verified locally: fresh uv build --wheel jupyter builds clean, wheel contains static/index.html and zero .map files.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax
Refs #1 (phase 4)
gridlook-jupyter: an in-forkjupyter/package shipping one jupyter-server extension that serves the built Vite app and a streaming S3 byte-range proxy, per the ratified phase 4 plan on #1.Ratified leans → what landed
s3://rewrite as a same-PR follow-up commit if trivial/gridlook/s3/<bucket>/<key>works with zero frontend involvement; the last commit adds the rewrite: ans3://bucket/prefixdata input rewrites to<app-base>/s3/bucket/prefixonly when the app detects it is extension-served (one cached probe ofapi/health, with a body check so SPA-fallback hosts that answer 200 to any path don't false-positive). Standalone serving leaves input untouched.S3ProxyHandlerstreams obstore chunks with per-chunk flush — whole objects are never buffered, no presigned URLs, credentials stay hub-side (ambient chain). GET/HEAD only,Rangepass-through with 206 +Content-Range, no LIST.hatch_build.pyrunsnpm ci && npm run buildat the repo root and copiesdist/into the wheel asgridlook_jupyter/static/(gitignored; re-included via hatchlingartifacts). Honest failure modes: empty static + missing frontend sources, or no npm on PATH → the build errors with a pointed message; it never ships an empty wheel, and CI assertsstatic/index.htmlis present in the built wheel. Editable installs skip the hook —GridlookProxy.static_dirpoints at a devdist/. Node requirement documented injupyter/README.md.GridlookProxy.allowed_buckets,.region) with env fallback (GRIDLOOK_ALLOWED_BUCKETS,GRIDLOOK_S3_REGION); a configured traitlet wins over env.Also per plan: route family
/gridlook/(static SPA viaAuthenticatedFileHandler) +/gridlook/api/health+/gridlook/s3/<bucket>/<key>; the/gridlook/hive/namespace is reserved with a routing comment pointing at the phase-6 moczarr virtual-store endpoint — not implemented here. v1 launcher is the documented URL (launcher card is phase 5).pip installauto-enables the extension viajupyter-config/jupyter_server_config.dshipped as wheel shared-data. The README names the cryocloud/2i2c deployment target (englacial/zagg#301 context).Store-factory seam
GridlookProxy.store_factoryis a plain attribute (default builds an obstoreS3Storeper allowlisted bucket, cached). Tests point it atLocalStore(prefix=tmp/<bucket>)and exercise the whole proxy surface with zero real S3.Phases
jupyter/orsrc/s3://→ proxy rewrite in the SPA data input (follow-up commit per lean (1))How tested
pytest -v, pytest-jupyter server fixtures) — extension load/health, static index + asset serving, bare-route 301, proxy 200 full GET, 206 with exact byte slices verified (bounded, open-ended, and suffix ranges), HEAD, content-type passthrough/guess, 404 missing key, 403 non-allowlisted (bucket named), 403 disabled-when-empty-allowlist, allowlist via traitlets and via env, and a range-header parser matrix.ruff check+ruff format --checkclean (line-length 100, E/F/W/I/N injupyter/pyproject.toml).npm run test(79 tests incl. 12 new rewrite tests covering served/standalone/probe-failure/fallback-host/caching),typecheck,lint-ci,buildall green.uv build --wheel jupyterfrom the repo checkout runs the npm hook and produces a wheel containing the SPA plus theetc/jupyterauto-enable config (verified by listing the wheel); the no-npm failure path was also exercised and errors with the pointed message.Questions for review
/gridlook(no trailing slash) 301s to/gridlook/. The test asserts the 301 +Locationwithout following it — following redirects in tornado's test client requires a DNS lookup that sandboxed dev environments can block (the follow works fine in real deployments and CI).Rangeheaders are ignored and the full object is served with 200 (RFC 9110 permits ignoringRange; zarrita only issues single ranges). Flag if you'd rather see 416.S3ProxyHandler._store_for, whereself.current_useris available.type-enumhere has nocitype, so the workflow commit ischore(config).🤖 Generated with Claude Code
https://claude.ai/code/session_01N32Mx4rWhbTK8xXm7Mt2ax