Skip to content

fix(dashboard): handle NotImplementedError on logout for password-only providers (#55985) - #55993

Closed
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/dashboard-logout-notimplemented
Closed

fix(dashboard): handle NotImplementedError on logout for password-only providers (#55985)#55993
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/dashboard-logout-notimplemented

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Prevent dashboard crash when logging out with a password-only auth provider (BasicAuthProvider).

Problem

BasicAuthProvider.start_login() raises NotImplementedError because it has no OAuth redirect flow — it is password-only. The auth_login route (/auth/login) only catches ProviderError, so the NotImplementedError propagates through the ASGI stack, crashing the dashboard server (exit 0). With restart: unless-stopped, the container enters a restart loop.

The logout flow triggers this because it redirects to /auth/login?provider=basic, which unconditionally calls p.start_login().

Fix

Catch NotImplementedError separately in auth_login and redirect to the login form page (/login) instead of crashing. The next parameter is preserved via cookie so the user lands on the right page after re-authenticating.

Reproduction

  1. Start container with restart: unless-stopped
  2. Log in to dashboard with basic auth
  3. Click Log Out
  4. Server crashes with NotImplementedError: BasicAuthProvider is password-only

Fixes #55985

…y providers

BasicAuthProvider.start_login() raises NotImplementedError because it
is password-only and has no OAuth redirect flow. The auth_login route
only caught ProviderError, so the NotImplementedError propagated through
the ASGI stack, crashing the dashboard server (exit 0) and triggering a
container restart loop with restart: unless-stopped.

Catch NotImplementedError separately and redirect to the login form
page (/login) instead of crashing. This preserves the 'next' parameter
via cookie so the user lands on the right page after re-authenticating.

Fixes NousResearch#55985
@alt-glitch alt-glitch added type/bug Something isn't working comp/dashboard Web dashboard / control panel UI (dashboard/, landing) area/auth Authentication, OAuth, credential pools P2 Medium — degraded but workaround exists labels Jul 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: competes with #55988 for the same issue #55985. Both fix the NotImplementedError on a password-only provider at auth_login, but via different mechanisms: #55988 pre-guards and returns HTTP 400 before start_login(), while this PR catches the exception and redirects to the /login form (preserving next). Same goal, different approach — not a duplicate. Maintainer to pick.

@benskls

benskls commented Jul 3, 2026

Copy link
Copy Markdown

Nice fix! I hit the same bug independently and opened #57445 before finding yours.

One improvement suggestion: your fix preserves next= via cookie, but the redirect URL loses the query parameter. Consider building the redirect URL with next= in the query string too:

except NotImplementedError:
    from urllib.parse import quote
    next_path = request.query_params.get("next", "")
    safe = _validate_post_login_target(next_path)
    target = f"/login?next={quote(safe, safe='')}" if safe else "/login"
    return RedirectResponse(url=target, status_code=302)

This way the login page can read next= from both the URL query and the cookie, which is more resilient (e.g. if the cookie is blocked by Safari's ITP).

Closing #57445 as duplicate.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused dashboard-auth fix.

Automated hermes-sweeper review found that current main already provides this guarantee through a stronger pre-guard:

  • 3e24b16f5 added hermes_cli/dashboard_auth/routes.py:195-202, which detects supports_password before start_login() can raise and redirects to the prefix-aware /login form.
  • The route validates and retains next in the redirect query at hermes_cli/dashboard_auth/routes.py:198-201, addressing the resilience concern raised in the prior discussion.
  • tests/hermes_cli/test_dashboard_auth_password_login.py:211-217 covers the password-provider /auth/login request and asserts the /login?next=%2F redirect.

Closing as implemented on main; no contributor action is needed.

@teknium1 teknium1 closed this Jul 15, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/dashboard Web dashboard / control panel UI (dashboard/, landing) P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dashboard logout crashes container via NotImplementedError in BasicAuthProvider

4 participants