Overnight hardening: SSRF, availability, and data-integrity fixes#53
Merged
Conversation
The always-on link-local guard for op.http matched only IPv4 169.254.0.0/16, so an IPv6 metadata endpoint (AWS IMDSv2 on fd00:ec2::254), an IPv6 link-local host, or the IPv4-mapped form [::ffff:169.254.169.254] slipped past it. The private-range block that would otherwise catch them is opt-in and off by default, and _pin_http_url carried a second copy of the same IPv4-only check. Route both check_url and the _pin_http_url DNS-rebinding guard through one _is_metadata_ip classifier that unwraps mapped/6to4/Teredo forms and covers both families, bringing this guard to parity with the evidence-locker SSRF guard. Private LAN and loopback stay open per the BYO posture.
hideLive() reset hiddenLive to [] and then skipped every already-hidden source, so a second load() (two Pattern-of-life clicks with no exit in between) left the saved set empty. restoreLive() on exit then un-hid nothing and the live globe (ADS-B, AIS, quakes, everything) stayed blank until a page reload. Restore the previously-hidden set before re-capturing the currently-visible sources, making hideLive() idempotent across re-entrant loads.
enforce_size_cap ran right after prune and measured os.path.getsize(), but a DELETE only moves pages to the freelist — auto_vacuum is off, so the file stays at its high-water mark until the VACUUM that runs immediately afterwards. Sizing off that inflated file made the cap drop an extra oldest slice the VACUUM was about to reclaim for free, chronically shortening the replay window on every pass the file sat over cap pre-vacuum. Measure in-use bytes instead — (page_count - freelist_count) * page_size, which is what the file becomes after the VACUUM — so only genuine overage is dropped.
EntityPanel is toggled by display and never re-keyed, so its cards persist across selections and must reset on id themselves. Three didn't: - DossierNarrativeCard kept the generated assessment in state with no reset effect, so selecting a new contact showed the previous one's analytic assessment (button reading "Regenerate"). Added the reset effect its siblings already have, plus a regression test. - AiAssessmentCard's manual "refresh" created an AbortController it never stored, so a slow refresh for A resolved after B was selected and painted A's brief into B's card. Track it in a ref and abort it on selection change. - EntityPanel's 1 Hz track-refresh gated only on ring length, which is capped, so a selected contact past the cap froze the sparkline. Gate on the newest fix's timestamp too, honouring the guarantee that block was added for.
Each of these turned a bad upstream response into a 500 or a dropped feed: - adsb.lol /point and adsb.fi /snapshot called r.json() after only checking the status, so the documented 200 + text/plain rate-limit body raised ValueError through cache.get_or_fetch and 500'd the route. Require a JSON content-type, the same guard _parse_ac already uses on the grid path. - The AISStream normalizer ran int(mmsi) outside its try, so one message with a non-numeric MMSI raised out of the websocket async-for and tore down the shared upstream socket for every client. Coerce once, up front, and skip the message on failure. - The Digitraffic /locations parser ran float(coords) unguarded per feature, so a single null coordinate 500'd the route. Wrap it like the sibling parsers and skip the bad feature.
/api/imagery/splat launches a GPU 3D Gaussian Splat reconstruction (register_image_job) but was missing from _COMPUTE_PREFIXES, so is_compute_path returned False and the fail-closed auth gate served it on a keyless box — an anonymous caller could spend GPU on recon jobs through the back door, and the request also skipped the compute rate limit. Add it to the shared prefix list so both gates cover it, matching the sibling /api/recon and /api/imagery/detect.
quakes() called r.json() after only a status check, so a 200 with a non-JSON body (a CDN error page) would raise out of the cache.get_or_fetch loader and 500 this sacred keyless layer. Require a JSON content-type, degrading to 502 like a bad status, matching the adsb.lol / adsb.fi guards.
_from_object rebuilds a Situation / SavedMap from an ontology object whose props are a keyless user-writable blob (POST /api/ontology/object). Two fields were unguarded while every sibling was hardened: radius_km ran a bare float() that a non-numeric value crashed, and updated_at (typed str | None) is not coerced by pydantic v2, so a non-string value raised a ValidationError. Either one, on a single poisoned row, 500'd the whole GET /api/situations or /api/maps list (the loop has no per-object guard). Coerce both to safe defaults, matching the file's existing per-field hardening.
_doc_acl returned None both for a brand-new doc (no ACL row) and for a store error (RPC down / non-200 / non-JSON), and _collab_join_allowed treated None as allow — so during a Supabase hiccup an under-cleared user who knew a classified doc id could join its live channel and receive every Yjs update. Distinguish the two: _doc_acl now raises _AclUnavailable on a configured-store failure (deny), and returns None only for a genuinely new doc or single-tenant API-key auth with no ACL store (allow).
result.ply, result.spz and camera.json served from disk with only the hex job_id validated — unlike get_job/job_events/list_jobs, which reject a request whose _owner_key doesn't match the job's owner. On a multi-user deployment a leaked or guessed job id let another analyst pull the reconstruction output. Route the three downloads through _owned_job_dir, which enforces the same owner check while the job is tracked and falls through to disk only for an evicted/restart-surviving artifact whose owner record is already gone.
…bject sync_binding builds a sparse props dict from only the binding's prop_map and upserts it onto the resolved target. Because ontology upsert replaces props wholesale by contract, resolving onto a pre-existing object minted by another source (target_id != the foundry id) destroyed every prop the binding did not map — name, flag, even the natural key when key_column wasn't mapped. Overlay the mapped props onto the existing blob before upserting for the resolve-onto-foreign case; a foundry-minted object stays sole-writer and wholesale-replaced.
Two ways a transform step could take down the whole build or the API: - _step_join materialized the full fan-out before the dataset row cap (which only runs after run_steps), so a low-cardinality join key — left rows times N right matches — could reach hundreds of millions of dicts and OOM the process. Bound the product to MAX_ROWS_PER_DATASET inside the join, raising 422 instead. - join / aggregate / pivot used a raw cell value as a dict key, which raises TypeError on an unhashable JSON list or dict (NDJSON columns) and aborts the whole run — unlike filter / derive, which quarantine a bad row. Canonicalize nested key values via _hashable so grouping and joining on such a column work.
_load_table ran outside the try that maps sqlite errors to SqlError, so two valid-but-awkward inputs escaped run_sql unwrapped and 500'd the SQL route: a column literally named a"b produced malformed DDL (the identifier wasn't quote-doubled) and an int larger than 2^63 (a 20-digit id) overflowed executemany. Escape embedded quotes in column identifiers, store an oversize int as text, and wrap the load so any residual failure surfaces as a clean SqlError.
fetch_og_image validated a URL's resolved IP with _is_public_url, then let httpx re-resolve the hostname on the actual GET — a check-then-fetch DNS-rebinding gap a short-TTL host could use to steer the fetch to the metadata endpoint or an internal service after passing the check. Resolve once via _resolve_public and pin the http connection to that IP with the original Host header, matching workflows/control.py; https keeps its hostname for cert validation (documented residual risk). The per-hop re-validation on redirects is unchanged.
_maybe_enforce_size_cap runs after the caller's upsert / assert_props has already committed, so a concurrent writer holding the DB during its VACUUM (which needs an exclusive lock) raised database-is-locked out of the write path — a spurious 500 for a change that already persisted. Make the soft cap best-effort: swallow and log a sqlite error, and let the next gated attempt retry.
register_sat_job created the job dir and copied chips before registering the job in _JOBS, so a .tif missing its rpc_*.txt sidecar raised 502 and left an orphaned .recon_jobs/<hex>/ dir that _evict_jobs (which only iterates _JOBS) could never reclaim — repeated calls leak disk. Wrap the setup so a failure removes the partial dir before surfacing the error.
_insert_assertion_sync deduped on (value, source), but a removal tombstone encodes value=None as "null" — identical to an explicit prop=null write — so a remove following a null write was dropped and never landed in the append-only provenance trail, contradicting the materialized blob (which correctly removed the prop). Include removal-ness in the dedup so a tombstone and a null value are kept as the distinct facts they are; a true duplicate (same value, source, and removal-ness) is still dropped.
_opensky_loop, _mil_loop, _quake_loop and _emerg_loop caught only httpx.HTTPError, so the documented airplanes.live 200 + text/plain throttle (and any USGS 200-non-JSON body) made r.json() raise a ValueError that escaped the loop and killed the task with no respawn — the platform went blind to mil contacts and 7500/7600/7700 emergency squawks until a process restart. Broaden the guard to Exception, matching the sibling loops (_global / _rule / _baseline) that already survive any transient error.
Follow-ups from the third audit wave, all classes already fixed on the core feeds: - weather (swpc/openmeteo/metar/nws) and seismic parsed r.json() after a status-only check, so a 200 + non-JSON body (a CDN maintenance / rate-limit page) raised out of the cache loader and 500'd these keyless routes. Parse defensively (try/except -> 502) via a shared _json_or_502 helper. - cams digitraffic ran float(coords) unguarded (unlike the caltrans sibling) — a station with null coords 500'd /api/cams and every snapshot route. - detectors.haversine_nm didn't clamp sqrt(a) before asin, so a near-antipodal pair could raise a domain ValueError and drop the whole behavioral sweep (the clamp correlate/rules.py already carries). - watch.evaluate_rules stored a False geofence-exit entry that was never removed, growing _STATE.inside without bound; pop the key instead (get already defaults an absent key to outside).
Third-wave follow-ups to the entity-panel fixes, same class: - CoaCards kept proposed COAs in state with no reset on situationId, and SituationPanel isn't re-keyed — so proposing COAs for situation A then selecting B and clicking Verify filed A's course of action under B (a wrong-data write). Reset on situationId, with a regression test. - CountriesPanel.selectCountry landed its detail fetch unconditionally, so a slow response for country A could paint under a newly-selected B and Ingest (keyed off the selection) would act on B with A's resources shown. Guard every state write against the current selection. - DatasetsView's ChecksSection effect had no cancellation guard, so a superseded dataset's check results could land last; add the cancelled flag its siblings use.
…ror copy Fourth-wave AI/LLM follow-ups (all MED/LOW): - _NARRATIVE_CACHE was a plain dict with no eviction; a stream of distinct entity ids grew it without limit. Cap it and drop expired / soonest-expiring on write. - _run_job ran target_dir.mkdir() before its try, so a mkdir failure killed the fire-and-forget task with an unretrieved exception and stranded the job in "queued" forever. Guard the mkdir and set status=error. - intel_dossier_narrative passed res.error through, and DossierNarrativeCard renders a non-"model unavailable" string verbatim — so a backend exception leaked into the dashboard against the copy rule. Return the clean sentinel; the detail stays in the llm.py logs.
intel_dossier_narrative awaited the reason-tier ladder (MiniMax -> DeepSeek -> local, ~180s each, sequential) with no total cap, so a stalled-but-reachable backend could pin the worker ~360s and return a 524 after the client is gone — llm.py notes the calling route must bound this, as news/* do. Wrap the call in asyncio.wait_for(85s) and degrade to "model unavailable" on timeout.
post_selection_brief awaited llm.chat with no total cap (only the enrichment gather was bounded), so a stalled selection backend pinned the worker. Wrap it in asyncio.wait_for(60s) and return a 502 on timeout. Also stop putting the raw res.error string in the 502 detail — the copy rule bars raw internals in a client-facing error; the detail stays in the llm.py logs.
Complete the route-level LLM time bound (dossier + selection brief already done): country_profile.country_brief and extract._run_llm awaited the fast-tier ladder with only a per-backend timeout, so a stalled backend still pinned the worker past the 100 s edge limit. Wrap both in asyncio.wait_for(90s) and degrade to their existing graceful path on timeout; also drop the raw res.error from the country-brief "reason" (copy rule).
start() awaited _load_hot_models() inline in the lifespan, and each ensure_hot is a POST with a 300s timeout — so two pinned large GGUFs stalling on load froze everything sequenced after it at boot (adsb snapshot, AIS feeders, watch loops) for minutes. Fire the warm as a background task (models load on demand via ensure_hot anyway) and make it per-model resilient so one failed warm doesn't abort the rest or strand an unretrieved exception on the task.
- DossierNarrativeCard now aborts an in-flight Generate on selection change (an inflightRef like AiAssessmentCard), closing a residual late-response leak the reset effect alone did not cover. - The background hot-model warm task holds a strong reference (_warm_task, cancelled in stop()) instead of being fire-and-forget, so it can't be GC'd mid-run.
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
1 participant
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.
Summary
A hardening pass across the platform: four review sweeps (security/auth, backend
correctness, frontend correctness, and the AI/LLM layer) plus a review of the diff
itself. 30 defects fixed, each with a regression test.
bash scripts/verify.shisgreen (1750 passed, 1 skipped) and the app boots and serves live.
Severity trended down across the sweeps, so the later ones are mostly MED/LOW —
the platform was already in good shape; this closes the sharp edges.
Security / auth
169.254.0.0/16was blocked but IPv6cloud-metadata (
fd00:ec2::254) and the IPv4-mapped form[::ffff:169.254.169.254]slipped past. Both
check_urland the DNS-rebinding pin now share one classifierthat unwraps mapped forms and covers both families.
/api/imagery/splatbypassed the compute fail-closed gate — it launches a GPUjob but was missing from
_COMPUTE_PREFIXES, so a keyless box served anonymousrecon jobs. Added it (also closes the rate-limit exemption).
_doc_aclreturned
Nonefor both "new doc" and "store error". Split them — a store errornow denies.
result.ply/spz/camera.json),unlike the job-metadata routes. Owner-scoped now.
validated IP.
a pre-existing object (sparse
prop_map+ wholesale upsert). It now merges ontothe existing blob in the resolve-onto-foreign case; foundry-minted objects stay
wholesale-replaced.
Availability / correctness
_mil/_emerg/ _quake/_opensky_loopcaught onlyhttpx.HTTPError, so the documentedairplanes.live "200 + text/plain" throttle raised a
ValueErrorthat killed the(unrespawned) loop — the platform went blind to 7500/7600/7700 squawks until a
restart. Broadened to match the sibling loops.
weather/seismic/AIS/Digitraffic/cams now guard the JSON parse and per-row coord
conversions.
extra history every pass. Now sizes off in-use pages
(page_count - freelist) * page_size.(
hideLivelost its saved set on a re-entrant load).CoaCards(which couldfile a course of action under the wrong situation),
CountriesPanel, andDatasetsViewnow reset/guard on selection change.keys aborted a build (now canonicalized), and the SQL console 500'd on odd input
(now a clean 4xx).
write (now best-effort), and a removal tombstone was deduped away (provenance gap,
now recorded).
and could pin a worker past the 100s edge limit — each is now wrapped in
asyncio.wait_for. Boot no longer freezes on hot-model loads (the warm runs in thebackground). The dossier cache is bounded, a stranded-download-on-mkdir-failure is
fixed, and a raw model-error string no longer leaks into the dashboard.
Verification
bash scripts/verify.shgreen: 1750 passed, 1 skipped (baseline was 1719).(BYO-posture SSRF ranges, wholesale-replace-vs-merge, two-sources-coexist dedup,
the join cap boundary, fail-closed collab) were each verified.
Not included / needs a decision
The
history.dbbyte cap is enforced hourly, so the file overshoots to ~4 GBagainst a 2 GB cap between prunes (measured). A mid-cycle vacuum valve would bound
it, but that trades vacuum frequency for disk headroom on retention code with a
scarred history — left out deliberately as a tuning call.