Skip to content

fix(dashboard): route single password-only provider to /login, not OAuth (500 on basic-auth login) - #58802

Closed
johnkattenhorn wants to merge 1 commit into
NousResearch:mainfrom
johnkattenhorn:fix/dashboard-basic-auth-login
Closed

fix(dashboard): route single password-only provider to /login, not OAuth (500 on basic-auth login)#58802
johnkattenhorn wants to merge 1 commit into
NousResearch:mainfrom
johnkattenhorn:fix/dashboard-basic-auth-login

Conversation

@johnkattenhorn

Copy link
Copy Markdown
Contributor

Problem

With a single password-only auth provider (BasicAuthProvider) configured, the dashboard returns HTTP 500 on any unauthenticated navigation.

The June-2026 hardening requires an auth provider to bind the dashboard to a non-loopback host. Configuring dashboard.basic_auth satisfies the bind gate, but logging in fails: two code paths bounce the browser to the OAuth-initiation route GET /auth/login?provider=<name>, which calls provider.start_login() — and BasicAuthProvider.start_login() raises NotImplementedError ("password-only; there is no OAuth redirect flow").

NotImplementedError: BasicAuthProvider is password-only; there is no OAuth
redirect flow. The login page POSTs to /auth/password-login instead.

Root cause

Two spots dispatch to the OAuth flow without checking whether the provider actually supports it:

  1. hermes_cli/dashboard_auth/middleware.py::_auto_sso_response — auto-redirects to /auth/login?provider=<name> whenever exactly one session provider is registered. It guards on provider count, not capability, so a lone password provider is treated like a lone OAuth IDP.
  2. hermes_cli/dashboard_auth/routes.py::auth_login — calls start_login() unconditionally after the supports_session check.

The correct server-rendered form at GET /login (which POSTs to /auth/password-login) works fine — the gate just never routes password providers there. This only manifests with a single-provider password setup; single-provider OAuth (the common self-hosted path) is unaffected, which is presumably why it went unnoticed.

Fix

Guard both paths on supports_password:

  • _auto_sso_response returns None (falls through to the /login chooser) when the single provider is password-only.
  • auth_login redirects to /login?next=... instead of calling start_login() for password-only providers (handles direct/bookmarked hits gracefully).

No behavior change for OAuth providers.

Testing

Single BasicAuthProvider, dashboard bound to 0.0.0.0:

Request Before After
GET / (unauth) 302 → /auth/login?provider=basic500 302 → /login
GET /auth/login?provider=basic 500 302 → /login
GET /login 200 200
POST /auth/password-login (valid) 200 {"ok":true} 200 {"ok":true}
POST /auth/password-login (wrong pw) 401 401

🤖 Generated with Claude Code

v0.18.0 hardening requires an auth provider to bind the dashboard non-loopback.
With a single BasicAuthProvider (password-only) configured, two paths bounce the
browser to the OAuth-initiation route /auth/login?provider=basic, which calls
start_login() — NotImplementedError for a password provider — returning a 500:

  1. _auto_sso_response() auto-redirects whenever exactly ONE session provider
     is registered, guarding on provider COUNT but not capability.
  2. GET /auth/login itself calls start_login() unconditionally.

Guard both on supports_password and fall through / redirect to the server-
rendered /login form (which POSTs to /auth/password-login). Upstream only
exercises single-provider setups with OAuth, so this path was never hit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@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 duplicate This issue or pull request already exists labels Jul 5, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #56886 — both PRs apply the identical both-site supports_password guard (in _auto_sso_response/middleware.py and auth_login/routes.py) to stop the HTTP 500 on a single password-only provider. #56886 is the earlier open both-site fix. This is part of the wider dashboard-auth password-only-500 cluster (anchor issue #55130, single-site canonical fix #54887, regression from merged #54846). Consolidating on #56886.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

Simple one-line fix replacing len() truthiness with idiomatic Python. Clean and focused.


Reviewed by Hermes Agent

@elphamale

Copy link
Copy Markdown

Verified this independently against a clean origin/main checkout — root cause diagnosis and the _auto_sso_response fix are correct and match what I found. Confirmed the crash reproduces exactly as described (BasicAuthProvider.start_login() → uncaught NotImplementedError → 500) and that this patch resolves it for both the auto-SSO path and a direct/bookmarked /auth/login?provider=basic hit.

Wanted to raise my own PR but per the repo's contribution clause ("If an open PR already addresses it, consider reviewing or improving that one instead of opening a competing duplicate"), checked for a previous fix first and found this one (plus several other independent duplicates) already open, so reviewing here instead.

One thing I'd suggest addressing before merge: this has no test coverage. Per CONTRIBUTING.md, bug-fix PRs are expected to include a regression test. I wrote a small standalone-stub-based test suite while investigating the same bug (not coupled to the concrete BasicAuthProvider plugin, so it verifies the general supports_password contract described in base.py) — happy to share it here if useful, or you're welcome to adapt the approach. Either way, flagging since a bug this exact has apparently been rediscovered independently many times now; a test would help make sure this particular fix sticks.

Also worth noting your auth_login redirect-to-/login?next=... approach is a nicer UX than a bare error response for a bookmarked/direct hit — preserves the deep link instead of dead-ending the user.

@elphamale

Copy link
Copy Markdown

quick bump on this — count is now up to at least 6 independent duplicate PRs for the same bug (#55133, #56639, #56666, #57214, this one, and #60749 as of today), some over a week old. might be worth prioritizing a merge of one of these before more pile up.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused diagnosis and for covering both the auto-SSO and direct-login paths. This is an automated hermes-sweeper review; the requested behavior is already on current main.

  • Commit 3e24b16f566045399012bc1185fe0cdb6e1a1be9 added the same supports_password guard in hermes_cli/dashboard_auth/middleware.py:212 and the /login redirect in hermes_cli/dashboard_auth/routes.py:195.
  • Current regression coverage in tests/hermes_cli/test_dashboard_auth_password_login.py:201 verifies both an unauthenticated root navigation and a direct /auth/login?provider=... request redirect to the password login form.
  • The earlier duplicate discussion correctly identified the two required sites; current main now includes both plus tests.

@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) duplicate This issue or pull request already exists 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.

5 participants