Skip to content

fix: avoid 500 when password-only auth provider hits /auth/login - #56666

Closed
codxt wants to merge 1 commit into
NousResearch:mainfrom
codxt:fix/basic-auth-provider-start-login-500
Closed

codxt wants to merge 1 commit into
NousResearch:mainfrom
codxt:fix/basic-auth-provider-start-login-500

Conversation

@codxt

@codxt codxt commented Jul 1, 2026

Copy link
Copy Markdown

Problem

The dashboard login flow calls GET /auth/login?provider=<name> for interactive providers. In hermes_cli/dashboard_auth/routes.py, auth_login() unconditionally calls p.start_login(...) once the provider exists and supports_session is true.

That breaks for BasicAuthProvider, whose start_login() intentionally raises:

NotImplementedError: BasicAuthProvider is password-only; there is no OAuth redirect flow.

Result: clicking the dashboard login button for the basic/password provider returns 500 Internal Server Error instead of a clean client error, and the stack trace bubbles all the way to ASGI.

Root cause

auth_login() only catches ProviderError, but BasicAuthProvider.start_login() raises NotImplementedError, so the request falls through as an unhandled exception.

Fix

  • Reject password-capable providers in GET /auth/login with a clean 400 explaining that the client must POST /auth/password-login instead.
  • Also catch NotImplementedError defensively and convert it into a 400 rather than a 500.

This preserves the intended split:

  • OAuth/redirect providers → GET /auth/login
  • Password providers → POST /auth/password-login

Repro

  1. Configure Hermes dashboard auth with the basic provider.
  2. Open /login.
  3. Click sign-in for that provider.
  4. Current behavior: 500 with BasicAuthProvider is password-only; there is no OAuth redirect flow.
  5. With this patch: clean 400 response instead of an internal server error.

@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
Contributor

This was generated by AI during triage.

Related to the dashboard-500-on-password-only-auth cluster (regression from #54846). This PR is NOT a duplicate of #54887 / #56639 / #56306 / #54958: those fix _auto_sso_response in middleware.py to stop the auto-SSO redirect from ever hitting /auth/login for a password-only provider. This PR instead hardens the /auth/login route itself (hermes_cli/dashboard_auth/routes.py::auth_login) to return a clean 400 (rejecting supports_password providers early + catching NotImplementedError) so the endpoint fails gracefully even when reached directly. Complementary fixes at different code sites — a maintainer may want both. Note: the diff also strips ~142 lines of comments unrelated to the fix.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for identifying the password-provider failure mode. This is now implemented on main through a redirect to the intended password form rather than a 400 response.

Automated hermes-sweeper review evidence:

  • hermes_cli/dashboard_auth/routes.py:195-202 checks supports_password before start_login() and returns 302 /login, so a pure password provider cannot raise its NotImplementedError through this route.
  • tests/hermes_cli/test_dashboard_auth_password_login.py:211-217 directly verifies GET /auth/login?provider=testpw redirects to the password login form.
  • The guard was added by 3e24b16f566045399012bc1185fe0cdb6e1a1be9 (fix(dashboard): support mobile OAuth login).

The existing behavior satisfies the PR's reported no-500 guarantee while guiding the browser to the usable password-login UI.

@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
@ericchuhk

Copy link
Copy Markdown

This PR is a critical lifesaver! Currently, the NotImplementedError inside BasicAuthProvider completely breaks the dashboard UI for any self-hosted container setups (like TrueNAS community edition 25.04.2.6) that rely solely on password-only credentials.

Tested and verified that this correctly bypasses the faulty auto-SSO flow. Please merge this fix ASAP so we can deploy the next hotfix release! Huge thanks to @codxt! 👍🚀

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.

4 participants