Step 3 isolation-safe hardening (W1–W6): fail-closed authz, dead-letter tiering, /status behind auth, Neo4j browser-URL flag#45
Draft
colombod wants to merge 4 commits into
Draft
Conversation
colombod
force-pushed
the
step2-auth-spike
branch
from
8218590 to
8dd171f
Compare
colombod
force-pushed
the
step3-isolation-safe
branch
from
fc5b34f to
31afae5
Compare
colombod
force-pushed
the
step2-auth-spike
branch
2 times, most recently
from
12c1e92 to
af46484
Compare
colombod
force-pushed
the
step3-isolation-safe
branch
from
31afae5 to
b342dcc
Compare
…-letter tiering, /status behind auth
Coupled behavior slice (W1+W2+W3+W5+W6), all provable under pytest. No Azure,
no live EasyAuth, no browser. Spec: workspace docs/16.
W1 fail-closed authz: _is_write_capable now denies on unpopulated request scope
state unless app.state.allow_unauthenticated (the explicit dev/test opt-out).
Converts an accidental implicit default into a named flag; prod empty-state now
fails closed. In auth-enabled prod the branch is unreachable (middleware always
sets is_service; boot guard keeps capability routes non-exempt).
W2 dead-letter tiering (per product decision + council TB-1):
- GET /queues/dead-letter (list) -> require_read: any authenticated principal.
- purge/replay RELOCATED to POST /admin/queues/dead-letter/{worker}/{purge,replay}
under a new /admin-prefixed require_admin router. Relocation is load-bearing:
the static admin key is recognized ONLY on /admin/* (auth._is_admin_route), so
admin-gated mutations must live under /admin to be reachable by the static
(local/dev/test) admin credential. Boot-guard watch-list extended to require_admin.
W3 /status behind auth: removed from both auth-exempt sets; /version stays as the
unauthenticated liveness carve-out. get_status carries no capability dep.
W5 deploy-surface coherence (product's own files, corrected): README production
command main:app -> main:asgi_app (bare app has no auth middleware); docker-compose
healthcheck /status -> /version; docs/service-setup.md stale "/status always
unauthenticated" claims fixed.
W6 admin controls off the general dashboard (doc 04 §3): removed the dead-letter
DRAIN (replay/purge) buttons + fetch wrappers + confirm flow from the general
dashboard queues-panel; kept the read-only dead-letter LIST. Admin drain
functionality lives only behind the admin surface. NOTE: this is the REMOVAL half
of doc 04 §3 — the /admin browser UI that will house these controls is deferred
(Azure-gated, doc 04); the endpoints exist and are admin-gated now.
BREAKING CHANGES:
- GET /status now requires authentication (was unauthenticated). Repoint external
health/liveness monitors to /version (config-free, unauthenticated, unchanged).
- Dead-letter purge/replay moved: /queues/dead-letter/{w}/{purge,replay} ->
/admin/queues/dead-letter/{w}/{purge,replay}. Old paths now 404.
Interim admin drain path (until the doc-04 /admin UI ships) — API is reachable now:
curl -X POST -H "Authorization: Bearer <admin_key_or_admin_role_token>" \
http://<host>/admin/queues/dead-letter/<worker_key>/purge
curl -X POST -H "Authorization: Bearer <admin_key_or_admin_role_token>" \
http://<host>/admin/queues/dead-letter/<worker_key>/replay
Verification: isolated suite `pytest -m "not neo4j and not integration"` = 1838
passed, 2 skipped, 0 failed (independently re-run). Web JS suite 101/101. The two
strict-xfail authz tripwires flipped XFAIL->XPASS and their markers were removed.
Council-reviewed (two rounds); no FAIL; TB-1 static-admin-lockout closed by the
relocation and pinned by positive tests (static admin key + entra admin role reach
the handler through the real gate). Tracked non-blocking follow-ups: conftest
client->asgi_app fixture swap (Commit 3), the /admin UI (doc 04), and error-copy UX.
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…t — W4
doc 16 W4, independent of the Step 3 auth-coupled slice. The Neo4j 7474 HTTP
browser URL points into a private Azure VNet (doc 05 requirement 5b) and must not
be surfaced on /status. The Bolt URL (neo4j_url) and health flags stay visible.
- config.py: add `is_development: bool = False` and `show_neo4j_browser_url: bool
= False`, plus a `neo4j_browser_url_visible()` predicate (fail-safe hidden: only
development OR the explicit opt-in reveals the URL).
- main.py /status: gate `neo4j_browser_url` behind the predicate — value when
visible, null when hidden. Key is always present so the /status JSON shape stays
stable (hidden is a value state, not a schema change).
Design note (council W4): the dev-detection field is a scoped `is_development: bool`,
NOT a general `environment: str`. The panel (cranky-old-sam + tester-breaker) flagged
the open string as speculative generality for a single yes/no decision — one consumer
today — and as a case-sensitivity foot-gun ("Development" silently hiding). A bool
eliminates both. Widen to an environment enum later IF a real second consumer appears.
Fail-safe verified: a zero-config fresh deploy (production, no opt-in) hides the URL;
garbage/unset values hide; only `is_development=true` or `show_neo4j_browser_url=true`
reveal. The sole emit path is the gated /status line — no other endpoint, template, or
log surfaces the URL.
Tracked (non-blocking, from review): the /status `neo4j_browser_url` key type changes
from always-str to str|null (in-tree consumers are null-safe); and the browser URL
still resolves via the flat `neo4j_browser_url` field while the adjacent Bolt URL
resolves via the structured neo4j block — a latent display-correctness mismatch to
reconcile if a structured browser-url field is ever added.
Verification: isolated suite `pytest -m "not neo4j and not integration"` = 1843
passed, 2 skipped, 0 failed (independently re-run); 5 new W4 tests pass. python_check
clean.
🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)
Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
… middleware) — Commit 3 doc 16 §9 Commit 3 (tracked, not a blocker in Commit 1). The default `client` fixture routed through the bare, unwrapped `app` — so the ~146 tests using it never traversed the auth middleware and passed gated routes via "no middleware ran". Route it through `asgi_app` (the auth-wrapped ASGI app) so the default suite exercises the real wired stack. This closes the recorded crusty-old-engineer dissent from Commit 1. Under the suite's ALLOW_UNAUTHENTICATED=true opt-out the middleware short-circuits (no scope state populated) and W1's _is_write_capable honours the flag, so these tests stay green while now traversing the real middleware. TB-N1 guard (proven load-bearing): there is a single module-level FastAPI `app`; create_asgi_app(settings=...) mutates its shared app.state on every call (173 calls in the suite). A sibling building an auth-enabled app (allow_unauthenticated=False) leaves app.state.allow_unauthenticated=False on the singleton; because `client` now wraps that singleton and W1 reads the flag live, a later gated-route `client` test would fail-close (403). The fixture resets app.state.allow_unauthenticated=True before each yield. Proof: adversarial ordering (flipper files then test_main.py client/gated tests) = 0 failures WITH the guard, 28 failures WITHOUT it (guard temporarily disabled). Also removes a now-redundant function-local `import asgi_app` in auth_client (module-level now). Council-reviewed (crusty-old-engineer / tester-breaker / cranky-old-sam): no FAIL. cranky-old-sam PASS (minimal, net-subtractive, the long comment earns its place as non-obvious why-not-delete rationale). crusty dissent CLOSED, merge. The fixture comment was narrowed after review to NOT overclaim full order-independence: the guard resets ONLY allow_unauthenticated; other create_asgi_app-leaked app.state (auth_mode, roles, stores) is not reset and is not currently observed by any `client` test. Tracked follow-ups (deeper, not patched here — SCRATCH): - create_asgi_app mutates a shared module-level singleton with no teardown (design smell; candidate: fixture-scoped app.state snapshot/restore, or a fresh-app factory). - Latent TB-N1 siblings: auth_mode / reader_role / service_data_role leak onto the same singleton and are read live by /status; not live today (client /status tests assert only status_code + neo4j fields), but the same class of hazard. - asgi_app.resolver is the true auth-enforcement lever (auth.py short-circuit), protected today only by the monkeypatch/separate-instance conventions of sibling tests. Verification: isolated suite `pytest -m "not neo4j and not integration"` = 1843 passed, 2 skipped, 0 failed (independently re-run after the comment narrowing). 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
…/status-behind-auth W3 (commit 3aeb581) moved GET /status behind authentication. This is WRONG and breaks the Azure Container Apps deployment: ACA uses /status as its UNAUTHENTICATED liveness/health probe. An authenticated /status makes every liveness check 401 → the revision never goes healthy → deployment failure. It also broke the existing dashboard (its api.js fetchStatus() sends no token), which surfaced this in live browser testing. /status MUST be a public, unauthenticated route at every level and must NEVER be gated again. Reverted (W3 + its W5 coherence artifacts only — W1 fail-closed authz, W2 dead-letter tiering, W6 dashboard removal are UNTOUCHED): - auth.py: restored "/status" to both _EXEMPT_PATHS and _EXEMPT_PATHS_API_ONLY. - docker-compose.yml: healthcheck reverted /version -> /status. - docs/service-setup.md: reverted the "/status requires auth" edits (verify curl drops the bearer; exempt-list re-includes /status; health example unauthenticated). - Tests that asserted /status was gated (test_auth, test_web_ui_switch, test_entra_integration, test_docker_infrastructure, test_m2_service_auth, test_main) reverted to assert /status is exempt / 200 without a token. - test_status_requires_auth.py (a W3 tripwire enforcing the wrong policy) DELETED; replaced by test_status_stays_public.py — an INVERTED tripwire that fails loudly if /status is ever removed from either exempt set. Protects against re-regression. Note: /version remains an additional unauthenticated liveness carve-out. The neo4j_browser_url hide flag (W4) still hides the browser URL on the public /status. Verification (independently re-run): isolated pytest -m "not neo4j and not integration" = 1845 passed, 2 skipped, 0 failed. Targeted status/auth suite 114 passed. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
colombod
force-pushed
the
step2-auth-spike
branch
from
af46484 to
f53b9a9
Compare
colombod
force-pushed
the
step3-isolation-safe
branch
from
b342dcc to
d8cbb92
Compare
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.
Stacked on
step2-auth-spike(PR #42, draft). Review after / alongside #42; the diff shown here is step3 vs step2.Three council-reviewed local commits, all provable under pytest — no Azure, no live EasyAuth, no browser. Spec: workspace
docs/16.Commits
3aeb581Commit 1 (coupled slice W1+W2+W3+W5+W6)74e330eW4 — Neo4j browser-URL hide flag7c91abeCommit 3 — conftestclientfixture →asgi_app(real middleware) + TB-N1 guardWhat changed
_is_write_capabledenies on unpopulated request scope state unlessapp.state.allow_unauthenticated(explicit dev/test opt-out). Prod empty-state now fails closed; in auth-enabled prod the branch is unreachable.GET /queues/dead-letter(list) →require_read(any authenticated principal). purge/replay RELOCATED toPOST /admin/queues/dead-letter/{worker}/{purge,replay}under a new/admin-prefixedrequire_adminrouter. Relocation is load-bearing: the static admin key is recognized ONLY on/admin/*, so admin-gated mutations must live there. Closes council TB-1 (static-admin lockout), pinned by positive tests./versionstays as the unauthenticated liveness carve-out./status.neo4j_browser_urlgated byis_developmentbool; value when visible,nullwhen hidden (stable shape). Bolt URL + health stay visible. Fail-safe = hidden (zero-config prod hides).main:app→main:asgi_app; docker-compose healthcheck/status→/version;docs/service-setup.mdstale "/status always unauthenticated" claims fixed./adminUI that will HOUSE these controls is deferred (Azure-gated, doc 04).⚠ BREAKING CHANGES
GET /statusnow requires authentication (was unauthenticated). Repoint external health/liveness monitors to/version(config-free, unauthenticated, unchanged)./queues/dead-letter/{w}/{purge,replay}→/admin/queues/dead-letter/{w}/{purge,replay}. Old paths now 404.Interim admin drain (until the doc-04 /admin UI ships — API is reachable now)
Verification
Isolated suite
pytest -m "not neo4j and not integration"= 1843 passed, 2 skipped, 0 failed (independently re-run). Web JS suite 101/101. Council-reviewed across the slices; no FAIL.Tracked follow-ups (non-blocking)
create_asgi_appmutates a shared module-level singletonappwith no teardown; the Commit-3 guard patches onlyapp.state.allow_unauthenticated. Real fix = fixture-scoped snapshot/restore or fresh-app factory (needs owner)./status.neo4j_browser_urlcontract is nowstr|null; browser URL reads the FLAT field while Bolt URL resolves via the STRUCTURED neo4j block.Not in this PR (deferred)
The
/adminbrowser UI (doc 04/06), live EasyAuth in Azure + Test B deploy gate (doc 15), and the Step 4 coherence+test gate (doc 07) are designed but NOT built here.