Claude/security fixes master - #4
Merged
Merged
Conversation
… eviction panic, broken downloads
Five defects, each with a regression test.
1. Arbitrary file read via GET /api/chunks/{sha} (unauthenticated)
ChunkStore::chunk_path never validated its argument, which arrives straight
from a URL path segment. hash[2..4] of "../../../etc/passwd" is "/.", and
PathBuf::join on an absolute component discards the store prefix, so the
path resolved to /etc/passwd. Verified against a running node before the
fix. chunk_path now returns Option<PathBuf> and rejects anything that is
not 64 lowercase hex chars; get/has/verify/delete all handle the rejection.
2. Remote panic on the same endpoint
hash[..2] panicked for input shorter than two bytes or on a non-ASCII char
boundary, so GET /api/chunks/a reset the connection. Same validation covers
it; the endpoint now returns 404.
3. POST /api/replicate and POST /api/peers were unauthenticated
/api/replicate takes a caller-supplied peer_url, fetches it and stores the
response — an SSRF primitive plus arbitrary store pollution. A registered
peer is not passive either: the heartbeat loop contacts it every 60s and
auto-replication pulls from it every 5 minutes, so one anonymous request
set up a standing ingestion channel from an attacker's server. Both now go
through authorize() at AccessLevel::Write.
4. Auto-eviction task panicked on first run
eviction::build_replica_map_from_beacon used reqwest::blocking, which
builds and drops its own runtime. Called from the tokio::spawn eviction
loop in server.rs, that panics with "Cannot drop a runtime in a context
where blocking is not allowed" — the task died the first time storage
exceeded the limit, so nodes never evicted and grew past their cap
indefinitely. Switched to ureq (already a dependency, genuinely
synchronous, safe from the CLI too) and wrapped the call in
block_in_place so it no longer stalls an async worker.
5. GET /api/download/{collection}/{item} returned 500 for every item
reconstruct_cog only understood the tiled layout and bailed on
"Missing: earthgrid:width". Nothing writes that metadata: ingest_raster is
the sole producer and has no callers, so every item comes from ingest_file,
which stores sequential raw byte chunks. Those concatenate back to the
original COG exactly, so reconstruct_cog now falls back to reconstruct_raw,
which reassembles in order and checks the result against the
earthgrid:file_hash recorded at ingest — a missing chunk is a hard error
rather than a truncated download.
Also: the beacon federation WebSocket (/api/beacon/ws) accepted any client and
handed it apply_remote_event, including NodePruned, which deletes any node from
the registry by ID. It now requires the shared grid key when one is configured,
and the outbound dialer presents that key. With no key set the socket stays open,
matching the node's open-mode behaviour elsewhere — federated beacons must set
EARTHGRID_API_KEY to the same value on each host.
Tests: 156 passing, up from 147. Traversal, panic and the auth changes were also
verified end to end against a running node from a non-loopback address.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Qjes4orjaT3k6ShNXj5Axm
The previous commit gated /api/beacon/ws on EARTHGRID_API_KEY. That was wrong
in two ways.
1. It reused the node's write credential. The dialer has to transmit whatever
the listener checks, so every beacon you federated with would learn your
EARTHGRID_API_KEY and could then drive your whole node API — /api/fetch,
/api/replicate, the fetch queue. A federated peer needs exactly one
capability, registry sync, so it now gets exactly one credential:
EARTHGRID_FEDERATION_KEY, sent in x-earthgrid-federation-key.
2. It failed open. The check only engaged when a key happened to be set, so a
beacon in the default keyless configuration still accepted any client — and
a client on that socket can add nodes to the registry and, via NodePruned,
delete any node by ID. Federation now fails closed: with no key configured
the endpoint returns 503 and spawn_peer_connections refuses to dial. An
unconfigured beacon federates with nobody instead of with everybody.
Key comparison is constant-time.
Verified against two live beacons:
no key configured — 503 for no credential, for a guessed one, and for the
old x-api-key header
key configured — 401 for no credential, 401 for a wrong key,
101 Switching Protocols for the right one
docs/beacon-guide.md documents the new variable, that it must match across
beacons, that it is not the grid key, and that peer URLs should be https since
the key travels in a handshake header.
Tests: 158 passing.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Qjes4orjaT3k6ShNXj5Axm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.