Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review Findings
This PR adds _request_path() to use the ASGI-dispatched scope["path"] instead of request.url.path for auth decisions in the dashboard OAuth gate and legacy API-token middleware. This is a defense-in-depth hardening following the Starlette BadHost fix.
✅ Looks Good
- Security: Switching auth gates from URL reconstruction to ASGI scope path is the correct approach —
scope["path"]is the routing source of truth and is not affected by host/header parsing quirks. - The
_safe_next_targetfix: Using_request_path()there also prevents protocol-relative open redirect attacks via thenext=parameter. Well spotted. - Tests: Two new async tests (
test_gate_uses_scope_path_for_auth_decision,test_legacy_token_gate_uses_scope_path_for_api_decision) use mock objects to simulate a request whererequest.url.pathlies butscope["path"]reveals the real path — good regression coverage. - API design:
_request_path()has a clean fallback torequest.url.pathifscopeis absent, making it safe to introduce. - Minimal diff: 94 additions, only 4 deletions — mostly test code, very low risk.
No Issues Found
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Changes
hermes_cli/dashboard_auth/middleware.py: Added_request_path()helper that usesrequest.scope["path"](ASGI source of truth) instead ofrequest.url.path(URL reconstruction) for all auth decisionshermes_cli/web_server.py: Same fix in legacy token middlewaretests/hermes_cli/test_dashboard_auth_middleware.py: 2 regression tests + 1 new import
Review
🔒 Security ✅
- Correctly uses the ASGI-dispatched scope path instead of URL-reconstructed path for all auth gates
- Defense-in-depth against Starlette BadHost — host/header parsing quirks cannot desync auth checks from dispatch
_request_path()has a fallback torequest.url.pathif scope path is unavailable (defensive)- Both
gated_auth_middlewareand legacyauth_middlewareare covered
✅ Correctness
_safe_next_target()also uses the scope path — important so redirect-after-login sends users to the right place- Public path checks (
_path_is_public) use the same helper _unauth_response()uses scope path for thenextredirect target
✅ Testing
test_gate_uses_scope_path_for_auth_decision: verifies gated middleware rejects when scope path is/api/sessionsbut URL path says/login(would bypass without this fix)test_legacy_token_gate_uses_scope_path_for_api_decision: same for legacy token middleware- Both tests are well-structured with clear fake classes
✅ Code Quality
- Helper function with docstring explaining why (ASGI source of truth vs URL reconstruction)
- Import is scoped in
web_server.py(minimal coupling) - Small, focused diff — 3 files, clear responsibility per file
Summary
Clean security hardening fix. Replaces request.url.path with ASGI scope path for auth gate decisions — defense-in-depth against URL reconstruction desync. Good test coverage.
Reviewed by Hermes Agent (cron job)
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused defense-in-depth hardening. The two original gates on current main still derive their authorization path from request.url.path (hermes_cli/dashboard_auth/middleware.py:274, hermes_cli/web_server.py:584), so the core approach remains relevant.
Problems
- Current main added a non-interactive token-auth seam after this PR; it performs exact token-route authorization using
request.url.pathathermes_cli/dashboard_auth/token_auth.py:162-164. A dispatched token route can therefore miss that seam under the same path-desynchronization class. - Current main also added
_plugin_api_runtime_gate, whose disabled-plugin policy begins fromrequest.url.pathathermes_cli/web_server.py:505-506. The PR does not cover this sibling authorization decision.
Suggested changes
- Reuse the ASGI-path accessor for both current sibling gates and add mismatch tests for each, alongside the existing OAuth and legacy-token cases.
Automated hermes-sweeper review.
| ) | ||
|
|
||
|
|
||
| def _request_path(request: Request) -> str: |
There was a problem hiding this comment.
Please make this the shared path source for the current sibling authorization checks as well: hermes_cli/dashboard_auth/token_auth.py:162 selects exact bearer-token routes from request.url.path, and hermes_cli/web_server.py:505 selects the runtime disabled-plugin gate from it. Both make authorization decisions from the same divergent value this helper is intended to avoid.
Summary
Validation
Notes
This is a defense-in-depth follow-up to the Starlette BadHost dependency floor: auth decisions should use the ASGI routing source of truth even when URL reconstruction behavior changes.