fix: dashboard auth crash with NotImplementedError for password-only providers - #56886
fix: dashboard auth crash with NotImplementedError for password-only providers#56886zxcasongs wants to merge 5 commits into
Conversation
Related to the dashboard password-only-provider This PR is a superset: it guards both sites in one change — |
…providers When only a BasicAuthProvider (password-only, no OAuth IDP) is registered, every unauthenticated request crashes with 500 NotImplementedError because: 1. _auto_sso_response auto-redirects to /auth/login?provider=basic (assumes the sole provider supports the OAuth redirect flow) 2. /auth/login route calls p.start_login() which raises NotImplementedError for password-only providers Fix: check supports_password before attempting the OAuth path in both locations. Password-only providers fall through to /login which renders the credential form (the already-working POST /auth/password-login path). OAuth and mixed providers are completely unaffected — the new checks are no-ops when supports_password is False (the default).
603570a to
fa114d2
Compare
|
I encountered this exact bug with my deployment and (with an agent) validated a local fix end-to-end. Environment / repro shape on our side:
Observed behavior matched this PR:
We carried a local fix for this on our branch and it ended up very close to this PR, but with a few small deltas that may be worth folding in here:
In our local patch the route-level recovery uses Happy to post the exact minimal test cases / follow-up patch if that would help move this forward. |
|
Thanks for the detailed feedback — all three points are valid and I'm folding them in:
Also switching the route-level recovery to Pushing the update shortly. |
…irect, regression tests - routes.py: use _validate_post_login_target(next) instead of raw quote(next) to prevent open-redirect via malicious next= values - routes.py: use _prefix(request) for prefix-aware fallback redirect so reverse-proxied deployments with X-Forwarded-Prefix work correctly - routes.py: switch to 303 status (POST→GET semantics for the redirect) - middleware.py: move supports_password check before prefix import to avoid unnecessary import on the early-return path - Add regression tests covering both broken entry points
5c78cd0 to
52c3a54
Compare
Related to the password-only-provider |
…y-provider Resolve conflict in middleware.py: keep PR branch's supports_password guard (with explanatory comment) over upstream's duplicate.
|
Thanks for the thorough report and for covering both the automatic and direct-login entry points. Automated hermes-sweeper review found this behavior already implemented on current
Closing as implemented on main. |
Fix: Dashboard auth crashes with NotImplementedError for password-only providers
Summary
When a dashboard is configured with only a
BasicAuthProvider(username/password auth, no OAuth IDP), every unauthenticated request crashes with a 500NotImplementedErrorinstead of showing the login page. The user can never reach the dashboard.Root cause: The auto-SSO middleware (
_auto_sso_response) and the/auth/loginroute both assume the sole registered provider supports the OAuth redirect flow (start_login).BasicAuthProvidersetssupports_password = Trueand intentionally raisesNotImplementedErrorfromstart_login— it has no OAuth flow, only a credential form. The code never checkssupports_passwordbefore callingstart_login, so it blindly invokes a method that raises.Trace:
gated_auth_middleware→ no cookie → calls_auto_sso_response()_auto_sso_responsesees exactly 1 provider (basic) → auto-redirects to/auth/login?provider=basic/auth/loginroute callsp.start_login(redirect_uri=...)BasicAuthProvider.start_login()→ raisesNotImplementedErrorThe fix
Two changes, both checking
supports_passwordbefore attempting the OAuth path:1.
hermes_cli/dashboard_auth/middleware.py—_auto_sso_responseWhen the sole provider is password-only, return
None(fall through to the normal/loginredirect). The login page already renders a credential form forsupports_passwordproviders.2.
hermes_cli/dashboard_auth/routes.py—auth_loginWhen someone hits
/auth/login?provider=basicdirectly (e.g. bookmark, manual URL), redirect to/logininstead of callingstart_loginand crashing.if not getattr(p, "supports_session", True): raise HTTPException( status_code=404, detail=f"Provider does not support interactive login: {provider!r}", ) + if getattr(p, "supports_password", False): + # Password-only providers have no OAuth redirect flow. + # Redirect to the login page which renders the credential form. + from urllib.parse import quote + target = "/login" + if next: + target = f"{target}?next={quote(next, safe='')}" + return RedirectResponse(url=target, status_code=302) + try: ls = p.start_login(redirect_uri=_redirect_uri(request)) except ProviderError as e:Why check
supports_password(not provider name)?Both checks use
getattr(p, "supports_password", False)— the same flag the login page already uses to decide between rendering a credential form vs. an OAuth button. This keeps the fix generic: any future password-only provider benefits automatically, with no special-casing by name.Impact
/auth/password-login(the existing, already-working path), gets session cookies.supports_passwordisFalsefor OAuth providers, so both new checks are no-ops._auto_sso_responsealready returnsNonewhenlen(providers) != 1.Testing
Reproduce the original crash with a minimal BasicAuthProvider-only setup:
# Start dashboard with only basic auth configured HERMES_DASHBOARD_BASIC_AUTH_USERNAME=admin \ HERMES_DASHBOARD_BASIC_AUTH_PASSWORD=secret \ python -m hermes_cli dashboard --host 0.0.0.0 --port 9119 --no-openBefore fix:
curl http://localhost:9119/→ 500NotImplementedError: BasicAuthProvider is password-onlyAfter fix:
curl http://localhost:9119/→ 302 →/login(renders credential form)Backwards compatibility
No config changes, no API changes, no migration. Existing OAuth-only and mixed deployments are completely unaffected — the new
supports_passwordchecks are no-ops for any provider wheresupports_passwordisFalse(the default).