Skip to content

Close the anonymous admin bypass - #376

Open
Babissimo wants to merge 2 commits into
mainfrom
feat/close-anonymous-admin
Open

Close the anonymous admin bypass#376
Babissimo wants to merge 2 commits into
mainfrom
feat/close-anonymous-admin

Conversation

@Babissimo

Copy link
Copy Markdown
Contributor

Closes the anonymous-admin bypass, completing ClickUp
123zgec1gtf after #371 (CI's passage
through Access) and #372 (the origin-side verifier). Unlike those two, this one
changes behaviour: /api/admin/* starts refusing anonymous callers on all seven
vhosts.

The Cloudflare side is already done. Applications now front all three admin
hostnames, each with the reusable allow policy for the node-support group and
the Service Auth policy for CI's token, and each verified returning a 302 to the
team login. api.retina.fm deliberately has none: it is the fleet's ingest
hostname and a node cannot complete an interactive login, which is the whole
reason enforcement is backend-side.

The flag goes in one commit

deploy/check-env-parity.py refuses to let the three deployed overlays disagree
about AUTH_ALLOW_ANONYMOUS_ADMIN, and that guard is right: closing the bypass
in one environment and not the others is drift rather than a decision. So all
three lose it together, and staging and production gain the audience of the
application created for each. docker-compose.local.yml keeps the flag, since a
laptop has no Access assertion and no OAuth and local sits outside the checker's
scope.

test_anonymous_admin_closed.py covers what parity cannot. Parity catches the
flag returning to one environment; it cannot catch it returning to all three at
once.

What was verified

  • Both new applications probed unauthenticated: 302 to
    offworldlab.cloudflareaccess.com, with the redirect's kid matching the
    audience pinned here. api.retina.fm, dash, map, towers and the staging
    and testmap surfaces confirmed unchanged.
  • deploy/check-env-parity.py clean, pre-commit run --all-files clean,
    typecheck:e2e clean, and the previously-red guard test now green.
  • Every endpoint the smoke suites probe checked against its auth dependency:
    /api/test/dashboard, /api/radar/analytics and /api/radar/nodes are
    unauthenticated and stay so, and the /api/auth/me probe is a rate-limit
    burst that judges on 429 rather than on 200.
  • JWT_SECRET was on the list of things to set on staging and test. It is
    already set: all three droplets carry distinct 64-character secrets and the
    running containers read them, so there was nothing to change.

Known consequences, not fixed here

  • The e2e node teardown stops working. nodes.spec.ts's afterAll retires
    the nodes it registers through DELETE /api/admin/nodes/{id}/state, with no
    credential and no way to get one on an ungated hostname. It will 401 and only
    console.warn, so runs stay green while staging accumulates e2e- nodes.
    Raised separately; it wants a decision rather than a quick patch.
  • PUT /api/simulation/config is require_admin, and the map's physics
    panel calls it from a bundle with no login path, so that panel goes read-only
    on staging and test.
  • /api/admin/leaderboard and /api/admin/alerts are get_current_user
    rather than require_admin: ordinary-user features under an admin prefix.
    They 401 too. Nothing regresses today, with no real accounts, but they want
    re-homing before any external operator is onboarded.

Deploy

Staging first, then left a day as every change of this class here is, then
production. The staging run of this PR is also the first real test of CI's
service token against the new applications.

🤖 Generated with Claude Code

@claude

This comment has been minimized.

Babissimo and others added 2 commits September 14, 2026 17:10
AUTH_ALLOW_ANONYMOUS_ADMIN made require_admin hand a superuser to any caller on
every vhost, since one FastAPI app answers all of them. With the origin-side
verifier in place (#372) and an Access application now in front of all three
admin hostnames, this flag was the last thing holding the door open.

All three deployed overlays lose it in a single commit because
check-env-parity.py refuses to let them disagree about it, and that guard is
right: closing the bypass in one environment and not the others is drift rather
than a decision. docker-compose.local.yml keeps it deliberately, since a laptop
has no Access assertion and no OAuth, and local sits outside the checker's scope.

Staging and production gain the audience of the application created for each, so
the verifier is configured everywhere the bypass has gone. An environment with
neither would refuse everybody, including the people meant to get in, and failing
closed should be reached on purpose rather than by omission.

test_anonymous_admin_closed.py covers what parity cannot. Parity catches the flag
returning to one environment; it cannot catch it returning to all three at once.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
These hit api.retina.fm, which carries no Access application and never can: it is
the fleet's ingest hostname and a node cannot complete an interactive login. The
401 therefore comes from require_admin in this codebase rather than from the
edge, which is the whole reason enforcement is backend-side. Response shape is no
longer assertable from here, because nothing in CI can authenticate against that
hostname; the backend suite still covers it.

The admin surface-selection block would otherwise have stopped running
altogether. It skipped itself once the server reported enforced auth, and with a
login card in the way the sidebar that names the surface is never reached, so the
assertion PR #335 added would have vanished quietly rather than failed. It now
makes the strongest claim each auth mode allows: the surface name where the
bypass is still on, and otherwise that the host resolved, Access admitted the run
and nginx served this bundle. Which surface a hostname selects is resolveSurface's
answer and stays covered in dashboard/src/test/surface.test.ts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Babissimo
Babissimo force-pushed the feat/close-anonymous-admin branch from fc4abc1 to 3543ccd Compare September 14, 2026 16:11
@claude

claude Bot commented Sep 14, 2026

Copy link
Copy Markdown

Claude finished @Babissimo's task in 21m 39s —— View job


Review: Close the anonymous admin bypass (#376)

Tasks

  • Gather context (PR description, diff, CLAUDE.md/ONBOARDING guidance)
  • Trace backend/core/users.py auth-flag derivation (_derive_auth_flags, get_current_user, require_admin)
  • Verify no deployed compose overlay sets GOOGLE_CLIENT_ID/GITHUB_CLIENT_ID
  • Review backend/tests/test_anonymous_admin_closed.py
  • Review deploy/check-env-parity.py and compose overlay changes
  • Review docs updates (docs/architecture.md, docs/runbook.md)
  • Review e2e test updates (api.spec.ts, dashboard.spec.ts) and frontend 401 handling
  • Post final review

Findings

1. Closing the bypass also removes the only working login path for the non-admin dashboard, on every deployed environment — not disclosed in the PR's "Known consequences" section.

Verified directly in code, not just from the PR's own framing:

  • backend/core/users.py:43-64 (_derive_auth_flags): auth_enabled = bool(GOOGLE_CLIENT_ID or GITHUB_CLIENT_ID); the bypass only ever activated when not auth_enabled and AUTH_ALLOW_ANONYMOUS_ADMIN=1.
  • No compose file in this repo (base, prod, staging, test) sets GOOGLE_CLIENT_ID/GITHUB_CLIENT_ID — OAuth has never actually been configured in any deployed environment (confirmed via grep across docker-compose*.yml).
  • get_current_user (backend/core/users.py:360-370) isn't admin-only — it also backs /api/auth/me (backend/routes/auth.py:231-233) and node-ownership self-service. This PR's own "what was verified" section even notes /api/auth/me's smoke check had to be changed to judge on 429 rather than 200, which is a symptom of this.
  • dashboard/src/pages/LoginPage.tsx offers only "Continue with Google"/"Continue with GitHub" (/api/auth/login/google, /github), both driven by the same empty client IDs — so the login page a locked-out visitor lands on doesn't work either.
  • Only the three admin hostnames get a Cloudflare Access application (per the PR body); the plain dashboard vhost gets none.

Net effect: before this PR, AUTH_ALLOW_ANONYMOUS_ADMIN=1 on prod/staging/test meant every anonymous visitor silently became ANONYMOUS_USER — the whole dashboard worked with no login, not just /api/admin/*. After this PR, anonymous /api/auth/me 401s, RequireAuth bounces to /login, and both login buttons dead-end. That's a materially bigger regression than the three consequences the PR body does disclose (physics panel read-only, /api/admin/leaderboard/alerts 401ing) — it's the entire non-admin surface becoming unreachable in prod, staging, and test simultaneously. Worth confirming with the author whether this is accepted (the PR body separately notes "no real accounts" exist yet) or needs a mitigation — e.g. provisioning real OAuth client IDs, or an Access app for the dash vhost — before the staging deploy.

2. (Altitude, plausible) The flag's removal only closes the bypass in the committed compose text, not necessarily on the live droplets.

docker-compose.yml documents that environment: wins over env_file: for the same key. Removing the explicit AUTH_ALLOW_ANONYMOUS_ADMIN=1 override from the three overlays means the effective value now falls through to each droplet's live, gitignored backend/.env — invisible to this repo and to test_anonymous_admin_closed.py, which only reads the committed overlay files. backend/.env.example already documents this exact failure mode. Worth a post-deploy check on each droplet's resolved config (e.g. via adm) confirming backend/.env doesn't independently carry the flag, per CLAUDE.md's "verify on the live map after deploying."

3. Test/comment accuracy nits in the new coverage:

  • frontend/e2e/api.spec.ts (~line 140) comments that GET /api/admin/leaderboard's 401 comes from require_admin, but backend/routes/admin.py:616 actually guards it with Depends(get_current_user)dashboard.spec.ts's own comment (same PR) gets this right. Worth fixing so a future reader doesn't trust the wrong dependency.
  • backend/tests/test_anonymous_admin_closed.py:61-64 (_sets_the_flag)'s regex matches AUTH_ALLOW_ANONYMOUS_ADMIN= being set to any value, not specifically "1" — the only value that activates the bypass per _derive_auth_flags. An explicit AUTH_ALLOW_ANONYMOUS_ADMIN=0 (e.g. added later to document "definitely off") would fail this test as a false positive.
  • Same file, test_no_deployed_overlay_leaves_the_verifier_unconfigured (~line 86): its CF_ACCESS_AUD=\S regex treats a quoted empty string (CF_ACCESS_AUD="") as "configured." Low-likelihood edge case, but the guarantee the test claims wouldn't quite hold there.
  • The module docstring's ClickUp citation (86cb1emcx) is used 8 other times in this repo exclusively to mean "staging/test runs RETINA_ENV=test," not an anonymous-admin incident — worth double-checking that's the ticket you meant to cite.

4. Frontend 401 handling, now exercised for the first time in production (medium confidence, follows from #1):

  • dashboard/src/pages/user/LeaderboardPage.tsx and AlertsPage.tsx catch fetch failures with console.error only and no error state, so a 401 renders as "No nodes found" / "your nodes are running smoothly!" — misleading rather than a visible failure, once anonymous visitors hit this path per Add Claude Code GitHub Workflow #1.
  • dashboard/src/context/AuthContext.tsx's fetchUser retries up to 4 times (1.5s/3s/4.5s backoff) on any thrown error including 401, per a comment that assumes client.ts's redirect fires first. Worth confirming this doesn't produce a visible retry flash now that 401 is the default outcome for anonymous dashboard visits rather than a rare edge case.

5. (Minor, reuse) test_anonymous_admin_closed.py hand-rolls YAML regex matching instead of reusing deploy/check-env-parity.py's existing compose_config() (the real docker compose config parser), which test_env_parity_scoping.py already loads via importlib for the same three overlays. The regex approach would miss a mapping-style or YAML-anchor reintroduction of the flag that the real parser would still catch.

Confirmed clean (per the PR's own claims)

  • backend/routes/admin.py: leaderboard/user_alerts use get_current_user, list_events/storage_stats use require_admin — matches what the PR body and e2e comments claim.
  • /api/test/dashboard, /api/radar/analytics, /api/radar/nodes remain unauthenticated, matching CI's smoke-suite expectations.
  • No /v1/nodes route or backend/services/node_config.py touched, so the generated-contract rule doesn't apply here.
  • docker-compose.local.yml still sets the bypass (pinned by test_local_may_keep_the_bypass), consistent with local having no Access/OAuth.

Fix this →


I don't have deployment/network access in this sandbox to actually run pre-commit, pytest, deploy/check-env-parity.py, or hit a live droplet, so finding #1 and #2 are based on static tracing of the auth code path and compose files, not a live probe — recommend verifying #1 concretely (e.g. hit /api/auth/me anonymously against staging once deployed) before promoting to production.
· branch

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.

1 participant