[obvious-autobuild]

fleet-packet: burst10-land-bs-security

Stacked on #247 — fixes the two Greptile apex findings from that review, fixes the Codex P1 race found on review of this PR, and merges alpha (Sam decided 2026-10-16 04:40Z that alpha is the end-to-end test branch where finished work lands) so the branch can be reviewed as mergeable. Changes are confined to backend/.

Human author: Sam Gu (sgu07966@gmail.com)

What changed and why

Finding 1 — P1 security: capture files bypass access checks (uploads)

[Greptile on backend/server/api/routes/uploads.py:23] — the old code registered the token-only /v1/uploads/{token} routes on every storage backend and verified tokens against an empty HMAC key when UPLOAD_URL_HMAC_SECRET was unset. On the dev deploy (STORAGE_BACKEND=gcs, no secret) anyone who knew a storage key could mint a valid empty-key op: "get" token and read the object — no bearer credential involved. New code fails closed at three layers: routes register only when a signing secret is configured (404 otherwise, with a startup log line), a volume deployment without the secret refuses to start, and _secret() raises 503 uploads_disabled as defense in depth. The interim UPLOAD_TOKENS_ALLOW_MISSING_SECRET escape hatch added during the first round of review was removed again at review recommendation (3728d1b1): its only consumer was one test, every other test opts in with a real secret, and if ever set it re-opened the exact forgery this PR fixes. No empty-key path exists.

Finding 2 — P1 security: deleted photos remain stored (packet cache)

[Greptile on backend/server/api/routes/captures.py:213] — the old DELETE removed only the uploads/, captures/ and work/ object-storage prefixes, while workers copy and commit the full packet into /vol/packet-cache/{capture_id}/{sha} on the shared volume, so photo copies survived a 204. New: purge_packet_cache(capture_id) removes the capture's entire cache slot; a partial purge is reported via a storage.warning (up to 20 surviving paths) while DELETE stays 204 because object storage — the system of record — is clean; hydrate_packet returns immediately for a tombstoned capture so a worker mid-run after DELETE neither re-downloads nor re-publishes.

Codex P1 — a delete racing a cache refresh could resurrect the deleted capture's files

[Codex on backend/server/orchestrator/artifacts.py:236] — the old code checked the tombstone once at hydrate entry, so a worker that passed the check before DELETE wrote the tombstone continued through the download and its later _publish_packet_cache recreated the deleted capture's files after the purge. Fixed on both sides:

• hydrate_packet re-checks _capture_tombstoned immediately before _publish_packet_cache, so a delete landing mid-hydrate is the last word. The run's own working copy is unaffected; only the shared-cache publish is suppressed.
• The DELETE route now writes tomb:{capture_id} before it purges, so a publish that raced the entry check lands inside the purge instead of after it. The old order (purge → tombstone) left that window open.
• Each side is pinned by a regression test that fails against the old code.

Merged alpha

• Merged origin/alpha at 72062663 into the branch (merge commit e2c942d0; normal merge, no rebase, no force-push).
• Conflict: backend/server/orchestrator/artifacts.py only. fix(backend): address Greptile review findings on the orchestrator #251's packet_key amendment plumbing and this branch's tombstone guard both landed in hydrate_packet. Resolution keeps alpha's version for alpha's change (packet_key parameter, manifest_key default, 4-arg _packet_file_jobs) and this branch's for its own (entry tombstone check via _capture_tombstoned), with the docstring combining both.
• fix(backend): address Greptile review findings on the orchestrator #251's orchestrator fixes (amendment runs hydrate the packet's own key; device confirmations applied) are preserved; this branch's fail-closed token gates and cache purge sit on top.

How you verified it

All in backend/ of a fresh clone (~/work/burst10-land-bs-security) on the BaseScanning repo sandbox; branch head c9230d03:

• Touched test files: uv run --frozen --extra api pytest server/orchestrator/tests/test_hydrate_inputs.py server/api/tests/test_uploads_token_gate.py → 17 passed (15 pre-existing + 2 new).
• Regression proof without the fix: with artifacts.py and captures.py stashed and tests kept, both new tests fail — test_delete_mid_hydrate_does_not_republish_the_cache (old code republished the purged cache slot, so assert not cache.exists() fails) and test_delete_tombstones_before_purging_the_cache (old code purged before the tombstone existed). Restored, both pass.
• Scoped suites: uv run --frozen --extra api pytest server/api/tests server/orchestrator/tests -q → 352 passed.
• Lint/contracts: ruff check . → all checks passed; ruff format --check . → 489 files formatted; lint-imports → contracts 5 kept / 0 broken; python -m server.contracts.export_schemas --check → ok.
• Full suite: uv run --frozen --extra api pytest -q → 1372 passed, 19 skipped, 5 failed, 14 errors (6:24). The 5 failures (tests/test_ssr_overlap_check.py, 4× server/eval/tests/test_gates.py) are the same pre-existing set already verified identical on the pre-merge head of this branch. The 14 errors are ImportError: cannot import name 'load_receipt' from 'scripts.replay_capture' in tests/test_capture_cold_restart.py — proven pre-existing on pure alpha itself (pytest tests/test_capture_cold_restart.py on a 72062663 worktree → the same 14 errors); both files arrive from alpha and are untouched by this merge or the fix.

What is not verified

• No live deployment exercised: /vol/packet-cache behavior on a real Modal volume (permissions, cross-process visibility) is covered via the HSS_PACKET_CACHE override, not a live volume; purge failure modes use a monkeypatched rmtree.
• The tombstone is advisory KV coordination across processes; a writer that ignores it can always write files. The two checks close the realistic interleavings (delete lands mid-hydrate; publish races the purge). A fully atomic close would need a cross-process lock that does not exist between the API and worker processes.
• scripts/check_repo_hygiene.py currently fails on backend/server/orchestrator/tests/data/packets/synthetic-meter.tar: deny-list — pre-existing on alpha (verified on a 72062663 worktree), not caused by this PR; it needs a separate alpha-side fix.

Assumptions

• Moving the tombstone write before the purge is part of the P1 fix (it is the DELETE half of the same race); the capture.deleted log and the purge-warning semantics are unchanged, and no test pinned the old purge→tombstone order.
• The review-recommended removal of UPLOAD_TOKENS_ALLOW_MISSING_SECRET from 3728d1b1 stands; this burst did not reopen it.
• Work ran in a fresh clone on the existing BaseScanning repo sandbox; nothing merged, no default branch touched, no other checkout used.

🔗 Obvious Project · 🧵 Obvious Thread · 🔀 Obvious PR Artifact

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 6e82db61-d061-4a8a-96ed-28017dbdf995

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[SamGu-NRX]

@coderabbitai review

[coderabbitai]

⚠️ Action not completed

Review skipped.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[obvious-autobuild]

Independent review (Obvious, Autobuild + Max)

Reviewed head d79830c from a fresh clone; no code changed, nothing pushed. Verdict: ready — both fixes do what the body claims, the tests exercise the real paths, and I found no regression. The findings below are non-blocking; I'd take finding 1 before this pattern gets copied.

Findings (ranked)

1. Medium — UPLOAD_TOKENS_ALLOW_MISSING_SECRET is unnecessary config surface and a production footgun — backend/server/common/config.py:65, backend/server/api/routes/uploads.py:22,32,37

Evidence: the flag's only consumer in the entire tree is the test that exercises it (server/api/tests/test_uploads_token_gate.py:62). Every existing test that needs working token routes uses a real test secret instead — server/api/tests/conftest.py:28, test_auth_none.py:32, test_fixture_meter.py:19, test_shared_create.py:69 all set UPLOAD_URL_HMAC_SECRET="h"*32. Nothing needs empty-key routes except to demonstrate empty-key verification. Meanwhile, if the flag is ever set on a deployment, routes_enabled re-registers the routes and _secret returns b"", re-opening the exact empty-key forgery this PR fixes — with no environment guard (SMP_ENVIRONMENT is not checked); only a code comment stands between the flag and prod. My pre-fix run also showed the flag test passes against the old code (the old Settings silently accepts the unknown kwarg), so that test isn't a regression test — it only locks in the flag's own behavior.

Smallest fix: drop the setting (the flag branches in uploads.py, the field in config.py, and the flag sentence in the resolve_storage message), and in the one opt-in test set a test secret like the conftest does and mint with the same key. The other five gate tests don't touch the flag and keep passing. If the flag must stay, gate it to non-prod environments or at least log a loud startup warning when set.

2. Low — the volume RuntimeError advertises an escape hatch that cannot work there — backend/server/common/config.py:130-132

Evidence: this message is raised by the volume branch when UPLOAD_URL_HMAC_SECRET is missing, and it tells the operator they "may set UPLOAD_TOKENS_ALLOW_MISSING_SECRET=1 … on non-volume backends". The volume branch requires the secret unconditionally and the app never starts, so the suggestion is a dead end for exactly the audience reading this error.

Smallest fix: drop that sentence from this message (or say "not applicable to volume storage").

3. Low — purge survivor report lists directories among files — backend/server/orchestrator/artifacts.py:200

Evidence: survivors = sorted(base.rglob("*")) is not file-filtered, so the storage.warning "N path(s) could not be removed" can name subdirectories (e.g. photos/) while the removed count is file-based; the numbers won't reconcile for an operator.

Smallest fix: filter the reported list to files: [p for p in survivors if p.is_file() and not p.is_symlink()].

What I verified (fresh clone at d79830c, backend/)

• New/touched tests at head: uv run --frozen --extra api pytest server/api/tests/test_uploads_token_gate.py server/orchestrator/tests/test_hydrate_inputs.py -q → 15 passed.
• Regression proof: with the six source files reverted to d79830c^ and tests kept, 4 of 6 gate tests fail (routes present without a secret; forged empty-key token served the file's bytes with 200; packet cache not purged; no purge warning) and the orchestrator file fails collection with ImportError: cannot import name 'purge_packet_cache'. Files restored afterwards; tree clean. The two that pass pre-fix are the volume-startup test (pre-existing behavior) and the flag test (finding 1).
• Scoped suites: pytest server/api/tests server/orchestrator/tests -q → 285 passed.
• Full suite: uv run --frozen --extra api pytest -q → 1212 passed, 17 skipped, 5 failed. The 5 (tests/test_ssr_overlap_check.py, 4× server/eval/tests/test_gates.py) fail identically on d79830c^ — pre-existing, exactly as the body claims.
• Lint/contracts: ruff check all passed; ruff format --check 460 files; lint-imports 5 kept / 0 broken (the captures.py → orchestrator.artifacts import is inside the permitted boundary); server.contracts.export_schemas --check ok; scripts/check_repo_hygiene.py ok.
• Vulnerability matches the deployment: server/api/modal_entry.py capture_runtime_env (line 43) bakes STORAGE_BACKEND=gcs (line 50) with no UPLOAD_URL_HMAC_SECRET, and dev runs HSS_AUTH_MODE=none — pre-fix the deployed capture-api served /v1/uploads/{token} with empty-key verification. Post-fix the routes are absent there, and non-volume backends never mint these tokens.
• Purge fires in production: capture-api runs as a Modal function with the house-capture-pipeline volume mounted at /vol, so MODAL_TASK_ID is set and packet_cache_dir returns the real slot; the tombstone guard works on prod state backends (ModalDictState extends MemoryState, which implements get_kv).
• The msg= usage is correct: msg is in log_event's fixed key set (server/common/logging.py:48); other extra kwargs are dropped, so the body's rationale holds.
• No republish race found: _publish_packet_cache runs only at the end of a successful hydrate_packet (artifacts.py:246), and once delete_prefix succeeds the GCS packet key is gone, so a hydrate in the purge→tombstone window fails rather than republishing; DELETE also holds the per-capture lock and the finish_async gate.
• Scope: only the eight backend/ files in the diff; no workflows, no repo-root files. Checks on the PR: label-size passed; no checks are required by branch rules per CONTRIBUTING.

Couldn't verify

• Modal secret contents (hss-dev-gcp / hss-dev-api): the baked env has no UPLOAD_URL_HMAC_SECRET, but I can't see inside the attached Modal secrets, so I can't rule out the secret being supplied there in some environment.
• Real volume failure modes (read-only mount, busy files) are covered by a monkeypatched rmtree only, and no live Modal deployment was exercised — the same limits the body already declares.

Bottom line: mergeable as-is; finding 1 is the one worth acting on — the test suite demonstrably doesn't need the flag, and a test secret makes the footgun impossible.

[SamGu-NRX]

@codex review

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-16T05:31:25.459048Z	c9230d0	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d79830c38c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Serialize the tombstone check with cache hydration

A worker can pass this tombstone check while DELETE is still in progress, then continue through _copy_packet_cache or download already-buffered packet bytes after the delete purges the cache and writes the tombstone; its later _publish_packet_cache can recreate the deleted capture's files. The check needs to be coordinated with deletion or repeated immediately before publishing so an in-flight worker cannot restore the cache.

Useful? React with 👍 / 👎.

[obvious-autobuild]

Fixed on the branch (fix 9135675, format-only c9230d0; head c9230d0 after merging alpha in e2c942d). hydrate_packet now re-checks _capture_tombstoned immediately before _publish_packet_cache — the entry check ran before the tombstone existed — and the DELETE route writes tomb:{capture_id} before calling purge_packet_cache, so a publish that raced the entry check lands inside the purge instead of after it. Each side is pinned by a regression test that fails against the old code: test_delete_mid_hydrate_does_not_republish_the_cache (delete interleaved mid-download; the finishing hydrate used to publish the slot right back) and test_delete_tombstones_before_purging_the_cache (the tombstone must already be visible when the cache purge runs).

[codemetrics-ai]

Note

Review skipped

Codemetrics is installed on this repository, but it hasn't been added to your team yet.

To enable AI code reviews for this repo, go to your dashboard and add BaseScanning to your team's repositories.

[chatgpt-codex-connector]

💡 Codex Review

BaseScanning/backend/server/orchestrator/artifacts.py

Lines 253 to 254 in c9230d0

	if cache_dir is not None and (packet_dir / "packet.json").is_file() and not _capture_tombstoned(state, capture_id):
	_publish_packet_cache(packet_dir, cache_dir)

Serialize the final tombstone check with cache publication

The final _capture_tombstoned check and _publish_packet_cache call are still separate operations, so a worker can observe no tombstone, then DELETE can write the tombstone and finish purge_packet_cache, and the worker can publish immediately afterward, recreating the deleted capture's files. Fresh evidence in this revision is that the new guard only moves the race to the check at line 253; it does not coordinate that check with publication. Use a cross-process lock or make publication conditional/atomic with the tombstone state.

---

BaseScanning/backend/server/api/routes/captures.py

Lines 218 to 228 in c9230d0

	packet_id = record.packet_id
	deps.state.delete_capture(capture_id)
	deps.state.put_kv(f"tomb:{capture_id}", {"deleted_at": isoformat(deps.now()), "packet_id": packet_id})
	# Tombstone before purging the cache: a hydrate that passed the
	# entry tombstone check while this delete was running can only be
	# stopped from republishing once the tombstone is visible, so every
	# publish it attempts after that must land inside this purge.
	# Object storage is clean, but workers publish packet copies to the
	# shared volume too. Purge them; report what could not be removed
	# instead of claiming a clean deletion.
	cache_removed, cache_failed = purge_packet_cache(capture_id)

Retry failed packet-cache purges after deleting the capture

When purge_packet_cache returns surviving paths, this handler still removes the capture record, writes the tombstone, and returns 204. A subsequent DELETE cannot retry the purge because it exits at the record is None check, leaving deleted photos on the shared volume indefinitely; logging the warning does not provide cleanup. Persist a retryable cleanup task/state or fail the deletion before making it irreversible.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

Confidence Score: 3/5

[Critical impact] Hardens upload token verification and fixes delete-hydrate race.

The PR needs a fix before merging because cache deletion can leave photos stored on the shared Modal volume.

Findings

1.   Deleted photos remain stored ▶

Fix with agent prompt

### Issue 1
backend/server/orchestrator/artifacts.py:194-196
`purge_packet_cache` checks the API container's current view of `/vol` without reloading the Modal volume. A warm API container can miss a cache directory that a worker created and committed after the container started. Then `base.is_dir()` returns false, the purge reports `(0, [])`, and DELETE returns `204` while the cached photos remain stored.

Reload the shared volume before checking the directory, and commit successful deletions before reporting completion. Add a test with separate API and worker volume views; the current temporary-directory tests cannot catch this.

**How this was verified:** The API mounts the same named Modal volume as the workers but never reloads it before deletion, while workers explicitly reload and commit their own view around each stage.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

The PR disables upload-token routes without a signing secret and rejects empty secrets in VolumeStorage. It also adds packet-cache deletion and checks for deleted captures before hydration and cache publication.

• Signed upload routes require a configured secret.
• Deleting a capture also clears its shared packet cache.

Acknowledgments from obvious-autobuild[bot]:

• Partial cache-removal failures intentionally log a warning while DELETE returns 204.
• The remaining check-then-publish race is acknowledged; fully closing it requires coordination across processes.
• Tombstone coordination is explicitly advisory.
• Live Modal volume behavior is explicitly unverified.

_{Reviews (1) · Last reviewed commit: "style(backend): ruff format on the publi..." · Reviewed by Greptile}

[greptile-apps]

Deleted photos remain stored

purge_packet_cache checks the API container's current view of /vol without reloading the Modal volume. A warm API container can miss a cache directory that a worker created and committed after the container started. Then base.is_dir() returns false, the purge reports (0, []), and DELETE returns 204 while the cached photos remain stored.

Reload the shared volume before checking the directory, and commit successful deletions before reporting completion. Add a test with separate API and worker volume views; the current temporary-directory tests cannot catch this.

How this was verified: The API mounts the same named Modal volume as the workers but never reloads it before deletion, while workers explicitly reload and commit their own view around each stage.

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/server/orchestrator/artifacts.py
Line: 194-196

Comment:
**Deleted photos remain stored**

`purge_packet_cache` checks the API container's current view of `/vol` without reloading the Modal volume. A warm API container can miss a cache directory that a worker created and committed after the container started. Then `base.is_dir()` returns false, the purge reports `(0, [])`, and DELETE returns `204` while the cached photos remain stored.

Reload the shared volume before checking the directory, and commit successful deletions before reporting completion. Add a test with separate API and worker volume views; the current temporary-directory tests cannot catch this.

**How this was verified:** The API mounts the same named Modal volume as the workers but never reloads it before deletion, while workers explicitly reload and commit their own view around each stage.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.