Skip to content

Keep real node identities and receiver positions out of the tree - #351

Merged
jehanazad merged 1 commit into
mainfrom
split/1-scrub-real-identities
Sep 10, 2026
Merged

jehanazad merged 1 commit into
mainfrom
split/1-scrub-real-identities

Conversation

@Babissimo

Copy link
Copy Markdown
Contributor

retina-server is public, and it carried the surveyed positions of live receiver sites, the ids and hostnames of live nodes, and one site's name. services/public_location.py displaces every receiver coordinate the API publishes, so a copy of the true one in the tree hands that displacement back for the node it names. Two files were already enough to do it: pipeline/passive_radar.py paired a real position with a node id, and routes/output.py's worked example named a production node.

First of five, split out of #349. It changes no behaviour, so it can land on its own.

The test is the deliverable

backend/tests/test_no_real_identities.py fails if any of it returns. It found 111 occurrences across 26 files when first run against main, including two that #350 added last week.

Three things about how it is built:

  • Coordinates and site names are held as SHA-256 digests, so the guard does not itself publish the list of what it forbids. The offending token is still named in the failure, so a reader who trips it knows what to change, and the docstring gives the one-liner for adding a value.
  • Every tracked path is read, and one that cannot be read fails rather than being skipped, so the guard cannot report clean on files it never opened. It also asserts the file list is non-empty, so it cannot pass vacuously outside a repository.
  • Submodule contents are out of reach and declared in UNSCANNED_SUBMODULES, with an assertion that the set has not changed, so a sixth submodule fails the build until someone acknowledges it. libs/retina-geolocator and libs/retina-simulation still carry the same coordinate and are tracked separately as 123zgec25ag.

What moved

Real ids, hostnames and site names out of production code, tests, docs and the dashboard's example copy. Fixture geometry is translated as a piece rather than blanked: the arc and region assertions are worthless against a shape the builder would decline, so the shape survives even though the position is invented.

routes/output.py's example moves to a synthetic node id rather than to the ref that eventually replaces it, since the rename is a later PR in this series.

One allow-list addition rather than a change: ret0a1b2c3d in dashboard/src/test/RetnodeLink.test.tsx is a synthetic fixture #350 added, and the guard's list simply did not know it.

Verification

Backend 3210 passed, 2 skipped. Frontend 190. Dashboard tests and typecheck clean. pre-commit run --all-files green.


🤖 Generated with Claude Code

This repository is public, and it carried the surveyed positions of live
receiver sites, the ids and hostnames of live nodes, and one site's name. The
API displaces every receiver coordinate it publishes; a copy of the true one in
the tree hands that displacement straight back for the node it names.

The test is the point. Coordinates and site names are held as SHA-256 digests
so the file does not itself list what it forbids, and the walk covers every
tracked path, failing on one it cannot read rather than passing over it.
Submodule contents are out of reach and declared as such, so a sixth submodule
fails until someone acknowledges it.

Fixture geometry is translated as a piece rather than blanked: the arc and
region assertions are worthless against a shape the builder would decline, so
the shape has to survive even though the position is invented.

Nothing here changes behaviour, and the published API example moves from a real
node id to a synthetic one rather than to the ref that will replace it, which
is a later change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 10, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 7m 6s —— View job


Claude finished reviewing PR #351

  • Read repo instructions (CLAUDE.md) and gather PR context
  • Review the diff and the new test_no_real_identities.py guard
  • Cross-check the guard's regex/digest logic against the rest of the tree for anything it misses
  • Post review findings

Summary

The mechanics of this PR are solid: test_no_real_identities.py is carefully built (digest-based bans so the test doesn't itself disclose the values, submodule gap tracked via an assertion, non-vacuous file-list assertion, unreadable-file failure instead of skip), and the fixture-geometry translations I spot-checked (test_track_gates.py, test_adsb_single_node.py, test_radar3_truth_units.py) are a consistent rigid offset applied to rx/tx/track/fix together, which correctly preserves the bistatic geometry the arc/region assertions depend on. TX/illuminator coordinates are correctly left alone since those are licensed public broadcast towers, not the private receive site. The UNSCANNED_SUBMODULES set matches the actual gitlinks in the tree (libs/retina-analytics, -custody, -geolocator, -simulation, -tracker).

Finding: real node identity still leaks in files this PR didn't touch

The PR's stated goal is to get real ids/hostnames/site names out of the tree, but a few pre-existing spots outside the 30 changed files still name the real node, and the new guard's regex doesn't catch them:

  • docs/simulation.md:129Two real hardware nodes (`radar3*-retnode`, via the blah2 bridge near Atlanta) connect... — names the real hostname pattern and the metro area in one sentence.
  • docs/runbook.md:702Real nodes (radar3/radar3a) legitimately learn near-omni.
  • backend/core/state.py:931 and backend/services/tasks/solver.py:3469-3470 — comments referring to "the radar3 recovery number".
  • backend/tests/test_solver_trimming.py:815 — same "radar3 recovery number" comment.
  • frontend/src/components/LiveAircraftMap.css:1225,1236,1242 and LiveAircraftMap.tsx:265,462 — CSS class radar3-error-label, which ships in the production frontend bundle served to every visitor of the live map.

These slip past REAL = re.compile(r"ret[0-9a-f]{8}|[a-z0-9][a-z0-9-]*[-.]retnode(?:\.com)?", ...) for two reasons: bare radar3/radar3a (no -retnode/.retnode suffix) isn't shaped like the regex expects, and the backtick-wrapped `radar3*-retnode` in simulation.md has a literal * sitting between radar3 and -retnode, which breaks the [a-z0-9-]* run right before [-.]retnode needs to match. Since this is the PR that adds the guard and claims the tree is now clean of this, it's worth sweeping these before merging, and possibly widening the regex (or adding a plain radar3 token check) so it would have caught them.

Fix this →

Secondary observation: unsalted SHA-256 for coordinate/name digests

BANNED_COORDS/BANNED_COORD_AXES/BANNED_NAMES in test_no_real_identities.py are plain sha256(token) with no salt. The docstring frames this as "the guard does not itself publish the list of what it forbids," but for low-entropy inputs that's weaker than it reads:

  • Coordinates are only banned at 2–3 decimal places (≈110 m–1 km precision). Anyone who already has a rough idea of the metro area (plausible for an org whose public repo, domain, or staff are geographically identifiable) can brute-force every lat.lon pair in that bounding box at 3-decimal precision — a few hundred thousand SHA-256 checks, sub-second — and recover the exact digest match, i.e., the true coordinate.
  • Site/node names are hashed per lowercase word ≥3 chars. A dictionary of city/place names hashed once offline instantly reveals which entries in BANNED_NAMES correspond to real words.

A salt wouldn't fully fix this since the salt would have to live in this same public repo to let CI compute digests, so it's visible to the same attacker. This isn't a regression this PR introduces — the ban is still much better than plaintext — but it's worth knowing the digests are a speed bump against casual grep, not a cryptographic guarantee, in case that gap matters for the later PRs in this series (e.g. once real ids are being actively renamed away).

Everything else checked out

  • DEFAULT_NODE_CONFIG in backend/pipeline/passive_radar.py (net13default-node, real rx coords → 34.0/-84.0) has no remaining references to the old node_id anywhere in the tree.
  • .gitignore's new /data/ entry is anchored to the repo root and doesn't shadow any currently-tracked path.
  • The contracts/nodes-v1.openapi.yaml regen requirement doesn't apply — no route under /v1/nodes or node_config.py binding changed.
  • routes/output.py's worked example still uses a plausible-but-unrelated aircraft lat/lon (pre-existing, untouched) — that's an aircraft position, not the receiver site, so it isn't in scope for this guard.

@jehanazad
jehanazad merged commit ce64416 into main Sep 10, 2026
16 checks passed
jehanazad added a commit that referenced this pull request Sep 13, 2026
Resolves 21 conflicting files against 53 commits on main, including the
two pieces already split out of this PR (#351 identity scrub, #355
receiver-geometry withholding), #358 site markers / empirical coverage,
#348 location privacy, #350/#353/#354 owner contact and retnode links,
and the #343/#346/#347/#357 palette work.

Rules applied: main's version is the base for the add/add files
(public_geometry.py, test_no_real_identities.py); the PR's ref-keying is
layered on top of main's rework everywhere else; substitution still runs
last on every publication path and fails closed for unregistered real
nodes.

Follow-on edits outside the conflicted set, needed to compile or to keep
main's tests honest under ref addressing: frontend nodeSites.ts (+test)
now reads is_synthetic; backend test_node_ref.py and
test_public_geometry.py address routes by ref / synthetic id;
analytics.py's owner-private listing merge keys on the owner identity.

Verified: backend 3495 passed / 2 skipped, ruff clean; frontend tsc,
lint, vite build, vitest 232 passed; dashboard tsc, eslint, vitest 53
passed, build.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Babissimo added a commit that referenced this pull request Sep 14, 2026
…eploy

`deploy-staging` was the only job in the `staging-deploy` concurrency group.
`staging-smoke-tests` and `e2e-staging` carried no concurrency at all, so the
deploy was serialised while everything that verifies the deploy was not: a
following run's deploy was free to land between one run's smoke tests and its
E2E, restarting the box underneath a suite that was testing it.

Three consecutive merges failed that way on 2026-09-10 (#348, #351, #352), and
four runs within three minutes produced the same thing on 2026-08-21 (#223).
None of them failed for a fault in the change. The failure does not present as
a flake either: a test hitting a half-swapped application gets a specific,
plausible assertion failure, which reads as a content regression and costs
someone real time before they conclude otherwise. It also blocks releases, as a
red staging E2E skips `deploy-production`.

Adding the group to the two verification jobs does not close it. Concurrency is
acquired and released per job, so a deploy can still slot in between one run's
smoke tests and its E2E. The environment has to stay held across the sequence.

So the three jobs move to `staging-deploy-verify.yml` and ci.yml calls it from
a single `Staging` job that carries the group. A calling job is not complete
until every job in the called workflow has finished, so the group is held from
the first byte of the deploy to the last E2E assertion. The step bodies move
unchanged; only the job headers differ.

Two things the move forced:

- `env` does not cross a `workflow_call` boundary and `with:` cannot read the
  `env` context, so APP_DIR is re-exported as a `changes` output rather than
  restated in the new file, keeping one definition of the deploy directory.
- `playwright-image` joins the staging job's prerequisites. An unpublished
  image now fails the run before the droplet is touched rather than after a
  deploy and a smoke suite have landed on it.

Two comments that named the old job IDs move with them: the DNS-tolerance
notes in frontend/e2e/dashboard.spec.ts and deploy/staging-smoke-test.sh both
explained themselves by their job being a direct `needs:` of deploy-production,
which is now indirect through the calling job.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants