test: prove the auth guard for every route instead of 14 hand-listed handlers - #198
Conversation
…handlers tests/test_audit_guards.py names 14 handlers; test_main_routes.py adds one. The app registers 75 routes. A guard deleted from any of the other 61 kept the whole suite green — verified by removing _require_auth_api from api_exchange_rates, _require_writer from api_collect and _require_auth_api from api_earnings_flatlines: 2300 passed each time, and an anonymous GET to /api/exchange-rates then returned the full rate table. Every endpoint test patches the guard away before calling the handler, so nothing notices when the real one is gone. This stops maintaining a list. It drives the real ASGI app with no session and no key and requires every registered route to refuse, so a new endpoint is covered the moment it exists and an author who wants one public has to say so here, with a reason. Two things were needed to make it mean anything. Eleven POST routes answered 422 to an empty body — validation runs before the handler, so the guard was never reached and the 422 proved nothing; bodies are now built from each route's own model. And /api/workers/heartbeat answered 503 "fleet key not configured", a fail-closed refusal rather than a guard, which would have left the one route a stolen shared key targets untested. An earlier attempt inspected each handler's AST for a guard call and reported 17 unguarded routes, all false: api_stop delegates to _svc_stop, which guards, and the page routes redirect to /login. Driving the app sees both. Measured, not assumed: each of the 55 _require_*(request) call sites in main.py was deleted in turn. 52 turn this file red; the other 3 leave a module that will not import. Nothing survived. Boundary stated in the file: a guard on a branch (_require_owner inside `if allow_delete_critical`, the deploy arm of api_worker_command) is unreachable anonymously, so role separation stays with TestMutatingEndpointsSitAboveViewer — and a test here fails if that class is ever deleted. Closes CashPilot-zfd
|
@coderabbitai review |
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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. Comment |
✅ Action performedReview finished.
|
…tarlette CI failed on test_the_public_list_only_names_routes_that_exist: every page route — /, /login, /logout, /register, /onboarding — was reported as "no longer registered". It did its job. app.routes is not a complete enumeration on Starlette 1.3, which is what CI installs because requirements.txt pins only fastapi>=0.136.1 (locally 0.136.1/1.0.1, on CI 0.141.1/1.3.1). Measured rather than assumed: with 1.3.1, re-including a 6-route APIRouter grew app.routes by ONE, and /login never appeared at all. Routing itself is unaffected — /login, /catalog and the rest all still serve, verified through a TestClient — so this is an enumeration change, not a broken app. The effect on this file was the failure mode it exists to prevent, returning as a quiet under-count: the whole HTML surface dropped out of the sweep while it still reported 65 routes and passed. A sweep that stops sweeping is worse than the hand-written list it replaced, because it still looks thorough. Routes are now unioned from the app and from each included router (bound in main as modules, so the APIRouter is one attribute further in), and seven page routes are asserted present by name, so this can fail loudly instead of shrinking in silence. 75 routes enumerated on the CI versions. Local FastAPI/Starlette were upgraded to match CI, which is how this was reproduced at all.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #198 +/- ##
==========================================
- Coverage 95.09% 95.07% -0.02%
==========================================
Files 46 46
Lines 5893 5912 +19
==========================================
+ Hits 5604 5621 +17
- Misses 289 291 +2 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
The defect
tests/test_audit_guards.pychecks 14 handler names typed out by hand.test_main_routes.pyadds one more. The app registers 75 routes.A guard deleted from any of the other 61 kept the whole suite green. Verified by removing:
_require_auth_apifromapi_exchange_rates_require_writerfromapi_collect_require_auth_apifromapi_earnings_flatlines2300 passed each time. An anonymous
GET /api/exchange-ratesagainst the mutated tree then returned the full rate table. Every endpoint test patches the guard away before calling the handler, so nothing anywhere notices when the real one is gone.The fix
Stop maintaining a list. This drives the real ASGI app with no session and no key, and requires every registered route to refuse — so a new endpoint is covered the moment it exists, and an author who wants one public has to come here and say why.
Two things were needed to make that mean anything:
/api/workers/heartbeatanswered 503 "Fleet key not configured" — a fail-closed refusal, not a guard. Accepting it would have left the one route a stolen shared key targets effectively untested.An earlier attempt inspected each handler's AST for a guard call and reported 17 unguarded routes, all false:
api_stopdelegates to_svc_stop, which guards, and the page routes redirect to/login. Driving the app sees both; reading the handler cannot.Evidence
Full mutation sweep. Each of the 55
_require_*(request)call sites inmain.pywas deleted in turn and this file re-run:if/elsebody)For the three endpoints named in the bead, the old allowlist test stayed green (40 passed) while this one went red — both halves proven.
ruff check .+ruff format --check .clean. 2620 passed, 6 skipped.Stated boundary
A guard that only fires on a branch —
_require_ownerinsideif allow_delete_critical, the deploy arm ofapi_worker_command— is unreachable for an anonymous caller, who is refused by the outer_require_writerfirst. Role separation is a different question from authentication and stays withTestMutatingEndpointsSitAboveViewer. A test here fails if that class is ever deleted, so the division of labour cannot silently become a hole.Closes CashPilot-zfd