diff --git a/contributors/emails/akulayash1996@gmail.com b/contributors/emails/akulayash1996@gmail.com new file mode 100644 index 0000000000..0f43a8fe19 --- /dev/null +++ b/contributors/emails/akulayash1996@gmail.com @@ -0,0 +1,2 @@ +buffpesos +# PR #75808 diff --git a/hermes_cli/dashboard_auth/native_flow.py b/hermes_cli/dashboard_auth/native_flow.py index f7c471b9f0..5dfe800c27 100644 --- a/hermes_cli/dashboard_auth/native_flow.py +++ b/hermes_cli/dashboard_auth/native_flow.py @@ -37,6 +37,15 @@ Wire shape (all gateway-side state lives in this module): ``Authorization: Bearer `` (via the existing ``token_auth`` seam) and mints ws-tickets the same way — no cookies anywhere. +Password providers ride the same broker with step 2 swapped: there is no +upstream IDP, so ``/auth/native/authorize`` sends the system browser to the +interactive ``/login`` form (broker_state in the PKCE cookie) and a successful +``/auth/password-login`` plays the role of the upstream callback — it calls +:func:`complete_pending` and bounces the browser to the loopback redirect. +Steps 4–5 are identical. The point of brokering a password login at all is +that the system browser can autofill from the OS password manager (macOS +Passwords, etc.), which no embedded desktop webview can. + Security properties this module guarantees: * **PKCE binding (RFC 7636).** A gateway code is redeemable only by the client diff --git a/hermes_cli/dashboard_auth/routes.py b/hermes_cli/dashboard_auth/routes.py index 15af8077fd..91bcec3e84 100644 --- a/hermes_cli/dashboard_auth/routes.py +++ b/hermes_cli/dashboard_auth/routes.py @@ -304,6 +304,15 @@ async def auth_native_authorize( ``/auth/callback``), carrying the broker_state in the same PKCE cookie the cookie flow uses. On the callback we mint a loopback code (see ``auth_callback``); no browser session cookie is ever set for the desktop. + + Password providers have no upstream IDP round trip to broker, but the + native flow is still exactly what they want: it moves sign-in out of the + desktop's embedded webview (where OS password managers cannot autofill) + into the SYSTEM browser (where they can). For a ``supports_password`` + provider we redirect to the interactive ``/login`` form instead of an + IDP, carrying the broker_state in the PKCE cookie; a successful + ``/auth/password-login`` then completes the pending authorization and + bounces the browser to the loopback redirect (see that route). """ # PKCE method must be S256 (RFC 7636 — plain is disallowed for native apps). if code_challenge_method.upper() != "S256": @@ -344,14 +353,11 @@ async def auth_native_authorize( raise HTTPException( status_code=404, detail=f"Unknown provider: {provider!r}" ) - if not getattr(p, "supports_session", True) or getattr( - p, "supports_password", False - ): - # Native PKCE brokering is only meaningful for redirect/OAuth - # providers; a password provider has no IDP round trip to broker. + if not getattr(p, "supports_session", True): + # Token-only credentials (e.g. drain) are not interactive sign-ins. raise HTTPException( status_code=400, - detail=f"Provider does not support native OAuth login: {p.name!r}", + detail=f"Provider does not support native login: {p.name!r}", ) from hermes_cli.dashboard_auth import native_flow @@ -366,6 +372,30 @@ async def auth_native_authorize( except native_flow.NativeFlowError as e: raise HTTPException(status_code=503, detail=str(e)) + if getattr(p, "supports_password", False): + # Password provider: no IDP to redirect through. Land the system + # browser on the interactive /login form with the broker_state in + # the PKCE cookie (the same server-controlled channel the OAuth + # branch uses); /auth/password-login picks it up on success and + # 302s the browser to the desktop's loopback redirect_uri. The + # desktop's challenge/state never touch the cookie — only our + # opaque broker_state does. + audit_log( + AuditEvent.NATIVE_AUTHORIZE_START, + provider=p.name, + ip=_client_ip(request), + ) + resp = RedirectResponse( + url=f"{_prefix(request)}/login", status_code=302 + ) + set_pkce_cookie( + resp, + payload=f"provider={p.name};broker={broker_state}", + use_https=detect_https(request), + prefix=_prefix(request), + ) + return resp + try: ls = p.start_login(redirect_uri=_redirect_uri(request)) except ProviderError as e: @@ -674,6 +704,15 @@ async def auth_password_login(request: Request, body: _PasswordLoginBody): the credential form POSTs via fetch and navigates client-side, so a 302 (which fetch follows opaquely) is the wrong shape here. + RFC 8252 native-app branch: when ``/auth/native/authorize`` sent this + browser to ``/login`` (password provider), the PKCE cookie carries the + opaque ``broker=`` handle. Mirroring the ``/auth/callback`` native + branch, success then mints a one-time loopback code instead of a + browser session: ``next`` is the desktop's loopback redirect_uri + (validated at authorize time) carrying ``code`` + ``state``, and NO + session cookies are set — the desktop redeems the code at + ``/auth/native/token`` for bearer tokens it stores itself. + Failure modes, all deliberately generic so the endpoint can't be used as a username oracle or a provider-enumeration oracle: * unknown provider / provider lacks password support → 404 @@ -706,6 +745,41 @@ async def auth_password_login(request: Request, body: _PasswordLoginBody): ) raise HTTPException(status_code=404, detail="Unknown provider") + # Native-app branch discriminator (see docstring): a broker handle in + # the PKCE cookie means this sign-in was initiated by + # /auth/native/authorize for a desktop app, not a browser session. The + # cookie is server-set (never client-supplied), so it is trustworthy — + # and it also records WHICH provider the native flow was initiated for. + # /login renders a form for every session provider, so without this + # check a flow started for provider A could be completed with provider + # B's credentials, binding B's session into A's pending authorization. + # Enforce equality BEFORE verifying credentials: nothing is minted, the + # pending authorization is preserved, and the user can submit the form + # the flow was actually started for. + broker_state = "" + cookie_provider = "" + pkce_raw = read_pkce_cookie(request) + if pkce_raw: + pkce_parts = dict( + seg.split("=", 1) for seg in pkce_raw.split(";") if "=" in seg + ) + broker_state = pkce_parts.get("broker", "") + cookie_provider = pkce_parts.get("provider", "") + if broker_state and cookie_provider != body.provider: + audit_log( + AuditEvent.NATIVE_TOKEN_FAILURE, + provider=body.provider, + reason="provider_mismatch", + ip=ip, + ) + raise HTTPException( + status_code=400, + detail=( + "This native sign-in was started for a different provider; " + "use that provider's form or restart sign-in." + ), + ) + try: session = p.complete_password_login( username=body.username, password=body.password @@ -741,6 +815,48 @@ async def auth_password_login(request: Request, body: _PasswordLoginBody): ip=ip, ) + # Native-app branch: the broker handle was parsed (and its provider + # binding enforced) above, before credential verification. + if broker_state: + from hermes_cli.dashboard_auth import native_flow + + try: + pending = native_flow.get_pending(broker_state) + gw_code = native_flow.complete_pending( + broker_state, session=session + ) + except native_flow.NativeFlowError: + audit_log( + AuditEvent.NATIVE_TOKEN_FAILURE, + provider=body.provider, + reason="pending_not_found", + ip=ip, + ) + raise HTTPException( + status_code=400, + detail="Native login expired or unknown; restart sign-in.", + ) + from urllib.parse import urlencode + + sep = "&" if "?" in pending.redirect_uri else "?" + loopback = ( + f"{pending.redirect_uri}{sep}" + f"{urlencode({'code': gw_code, 'state': pending.client_state})}" + ) + audit_log( + AuditEvent.NATIVE_CODE_ISSUED, + provider=body.provider, + user_id=session.user_id, + ip=ip, + ) + # The login page's form script navigates to ``next`` — here the + # loopback listener, which answers with its own "you can close + # this window" page. No session cookies: the desktop is not a + # browser session (mirrors the /auth/callback native branch). + resp = JSONResponse({"ok": True, "next": loopback}) + clear_pkce_cookie(resp, prefix=_prefix(request)) + return resp + expires_in = max(60, session.expires_at - int(time.time())) landing = _validate_post_login_target(body.next) or "/" resp = JSONResponse({"ok": True, "next": landing}) diff --git a/hermes_cli/web_server.py b/hermes_cli/web_server.py index 3a832ceb35..7a42a601b4 100644 --- a/hermes_cli/web_server.py +++ b/hermes_cli/web_server.py @@ -3225,10 +3225,13 @@ async def get_status(profile: Optional[str] = None): # to decide whether it can use the system-browser + loopback + PKCE # flow (no embedded webview, no session cookies) or must fall back to # the legacy embedded-webview cookie flow. "cookie" is always available - # in gated mode; "native_pkce" is present only when at least one - # registered session provider is a brokerable OAuth provider (not a - # password or token-only credential). Absent field / missing - # "native_pkce" ⇒ older gateway ⇒ desktop falls back automatically. + # in gated mode; "native_pkce" is present when at least one interactive + # session provider is registered — OAuth providers broker the upstream + # IDP round trip, password providers complete interactively at /login + # in the system browser (where OS password managers can autofill; an + # embedded webview cannot reach them). Token-only credentials (e.g. + # drain) don't count. Absent field / missing "native_pkce" ⇒ older + # gateway ⇒ desktop falls back automatically. auth_flows: list[str] = [] try: from hermes_cli.dashboard_auth import ( @@ -3238,11 +3241,7 @@ async def get_status(profile: Optional[str] = None): auth_providers = [p.name for p in _list_providers()] if auth_required: auth_flows.append("cookie") - brokerable = [ - p for p in _list_session_providers() - if not getattr(p, "supports_password", False) - ] - if brokerable: + if _list_session_providers(): auth_flows.append("native_pkce") except Exception: # Module not importable yet (early startup) — leave as []. diff --git a/tests/hermes_cli/test_dashboard_auth_native_flow.py b/tests/hermes_cli/test_dashboard_auth_native_flow.py index dd24caecbf..182b096a66 100644 --- a/tests/hermes_cli/test_dashboard_auth_native_flow.py +++ b/tests/hermes_cli/test_dashboard_auth_native_flow.py @@ -269,10 +269,13 @@ def test_native_authorize_empty_provider_ambiguous_multiple_oauth_404(gated_clie assert r.status_code == 404 -def test_native_authorize_empty_provider_password_only_rejected_400(gated_client): - """Password-only deployment: an empty ``provider`` must still select the - lone session provider and fail with the explicit 400 explaining that - password providers have no native OAuth flow — not a bare 404.""" +def test_native_authorize_empty_provider_password_only_brokers_to_login( + gated_client, +): + """Password-only deployment: an empty ``provider`` selects the lone + session provider and — now that native sign-in brokers password + providers through the system browser — 302s to ``/login`` with the + broker in the PKCE cookie, rather than the old 400.""" clear_providers() register_provider(_PasswordOnlyProvider()) _verifier, challenge = _make_pkce() @@ -280,8 +283,10 @@ def test_native_authorize_empty_provider_password_only_rejected_400(gated_client "/auth/native/authorize", params=_native_authorize_params(challenge), ) - assert r.status_code == 400 - assert "does not support native OAuth login" in r.json()["detail"] + assert r.status_code == 302, r.text + assert r.headers["location"].endswith("/login") + set_cookie = r.headers.get("set-cookie", "") + assert "broker=" in set_cookie # --------------------------------------------------------------------------- @@ -335,6 +340,242 @@ def test_status_loopback_mode_has_no_auth_flows(): web_server.app.state.auth_required = prev_required +# --------------------------------------------------------------------------- +# Native flow for password providers (system-browser autofill path) +# --------------------------------------------------------------------------- +# +# A password provider has no IDP round trip, but the native flow still buys +# the desktop the one thing an embedded webview can never have: the system +# browser's OS-password-manager autofill. /auth/native/authorize lands the +# browser on /login (broker_state in the PKCE cookie) and a successful +# /auth/password-login completes the pending authorization exactly like the +# OAuth callback does. + + +@pytest.fixture +def pw_gated_client(): + from hermes_cli.dashboard_auth.routes import _reset_password_rate_limit + from tests.hermes_cli.test_dashboard_auth_password_login import ( + PasswordProvider, + ) + + clear_providers() + register_provider(PasswordProvider()) + _reset_password_rate_limit() + prev_host = getattr(web_server.app.state, "bound_host", None) + prev_port = getattr(web_server.app.state, "bound_port", None) + prev_required = getattr(web_server.app.state, "auth_required", None) + web_server.app.state.bound_host = "fly-app.fly.dev" + web_server.app.state.bound_port = 443 + web_server.app.state.auth_required = True + client = TestClient( + web_server.app, base_url="https://fly-app.fly.dev", + follow_redirects=False, + ) + yield client + clear_providers() + _reset_password_rate_limit() + web_server.app.state.bound_host = prev_host + web_server.app.state.bound_port = prev_port + web_server.app.state.auth_required = prev_required + + +def test_status_advertises_native_pkce_for_password_only_gateway( + pw_gated_client, +): + body = pw_gated_client.get("/api/status").json() + assert body["auth_required"] is True + assert "cookie" in body["auth_flows"] + assert "native_pkce" in body["auth_flows"] + + +def test_native_authorize_password_provider_redirects_to_login( + pw_gated_client, +): + """Empty ``provider`` auto-picks the single password provider and lands + the system browser on /login with the broker in the PKCE cookie.""" + _verifier, challenge = _make_pkce() + r = pw_gated_client.get( + "/auth/native/authorize", + params={ + "code_challenge": challenge, + "code_challenge_method": "S256", + "redirect_uri": "http://127.0.0.1:53999/cb", + "state": "desk-state", + }, + ) + assert r.status_code == 302, r.text + assert r.headers["location"].endswith("/login") + set_cookie = r.headers.get("set-cookie", "") + assert "pkce" in set_cookie + assert "broker=" in set_cookie + + +def _start_native_password_login(client, *, challenge, state="desk-state"): + r = client.get( + "/auth/native/authorize", + params={ + "provider": "testpw", + "code_challenge": challenge, + "code_challenge_method": "S256", + "redirect_uri": "http://127.0.0.1:53999/cb", + "state": state, + }, + ) + assert r.status_code == 302, r.text + return r.cookies + + +def test_native_password_login_full_roundtrip(pw_gated_client): + """authorize → /login → password-login → loopback code → bearer tokens.""" + verifier, challenge = _make_pkce() + cookies = _start_native_password_login(pw_gated_client, challenge=challenge) + + # The browser form POSTs the credentials; the PKCE cookie rides along. + r = pw_gated_client.post( + "/auth/password-login", + json={"provider": "testpw", "username": "admin", "password": "hunter2"}, + cookies=cookies, + ) + assert r.status_code == 200, r.text + body = r.json() + assert body["ok"] is True + # ``next`` is the desktop's loopback redirect carrying code + state — + # NOT a dashboard path. + assert body["next"].startswith("http://127.0.0.1:53999/cb?") + qs = parse_qs(urlparse(body["next"]).query) + assert qs["state"][0] == "desk-state" + code = qs["code"][0] + # No browser session on the native branch; the PKCE cookie is cleared. + set_cookie = r.headers.get("set-cookie", "") + assert "hermes_session_at" not in set_cookie, ( + f"native password login must NOT set a session cookie; got {set_cookie!r}" + ) + assert "pkce" in set_cookie # the clearing Set-Cookie + + # Desktop redeems the loopback code with its PKCE verifier. + tokens = pw_gated_client.post( + "/auth/native/token", + json={"code": code, "code_verifier": verifier}, + ).json() + assert tokens["provider"] == "testpw" + assert tokens["user_id"] == "admin" + + # Cookieless bearer auth of a gated route — the point of the flow. + r2 = pw_gated_client.get( + "/api/auth/me", + headers={"Authorization": f"Bearer {tokens['access_token']}"}, + ) + assert r2.status_code == 200, r2.text + assert r2.json()["user_id"] == "admin" + + +def test_native_password_login_wrong_password_keeps_pending(pw_gated_client): + """A failed credential attempt must not consume the pending + authorization — the user retypes and succeeds on the same broker.""" + verifier, challenge = _make_pkce() + cookies = _start_native_password_login(pw_gated_client, challenge=challenge) + + r = pw_gated_client.post( + "/auth/password-login", + json={"provider": "testpw", "username": "admin", "password": "wrong"}, + cookies=cookies, + ) + assert r.status_code == 401 + + r2 = pw_gated_client.post( + "/auth/password-login", + json={"provider": "testpw", "username": "admin", "password": "hunter2"}, + cookies=cookies, + ) + assert r2.status_code == 200, r2.text + assert r2.json()["next"].startswith("http://127.0.0.1:53999/cb?") + + +def test_native_password_login_expired_broker_returns_400(pw_gated_client): + """A broker cookie whose pending entry lapsed (TTL) is a clean 400 + telling the user to restart sign-in — never a silent cookie login.""" + _verifier, challenge = _make_pkce() + cookies = _start_native_password_login(pw_gated_client, challenge=challenge) + + native_flow._reset_for_tests() # simulate the pending TTL lapsing + + r = pw_gated_client.post( + "/auth/password-login", + json={"provider": "testpw", "username": "admin", "password": "hunter2"}, + cookies=cookies, + ) + assert r.status_code == 400 + assert "restart" in r.json()["detail"].lower() + + +def test_native_password_login_rejects_cross_provider_completion( + pw_gated_client, +): + """A native flow started for provider A must not be completable with + provider B's credentials: /login renders every provider's form, and the + pending authorization is bound to the provider recorded in the + server-set PKCE cookie. The mismatch is rejected BEFORE credential + verification and preserves the pending entry, so the user can still + submit the form the flow was started for.""" + from tests.hermes_cli.test_dashboard_auth_password_login import ( + PasswordProvider, + ) + + class SecondPasswordProvider(PasswordProvider): + name = "testpw2" + display_name = "Test Password 2" + + register_provider(SecondPasswordProvider()) + + verifier, challenge = _make_pkce() + # Native flow initiated for provider A ("testpw"). + cookies = _start_native_password_login(pw_gated_client, challenge=challenge) + + # Valid credentials for provider B ("testpw2") must NOT complete A's + # pending authorization. + r = pw_gated_client.post( + "/auth/password-login", + json={ + "provider": "testpw2", "username": "admin", "password": "hunter2", + }, + cookies=cookies, + ) + assert r.status_code == 400, r.text + assert "different provider" in r.json()["detail"] + set_cookie = r.headers.get("set-cookie", "") + assert "hermes_session_at" not in set_cookie + + # The pending entry survived — provider A completes normally. + r2 = pw_gated_client.post( + "/auth/password-login", + json={ + "provider": "testpw", "username": "admin", "password": "hunter2", + }, + cookies=cookies, + ) + assert r2.status_code == 200, r2.text + qs = parse_qs(urlparse(r2.json()["next"]).query) + tokens = pw_gated_client.post( + "/auth/native/token", + json={"code": qs["code"][0], "code_verifier": verifier}, + ).json() + assert tokens["provider"] == "testpw" + + +def test_password_login_without_broker_still_mints_cookies(pw_gated_client): + """Guard: an ordinary browser password login (no native broker cookie) + keeps the existing cookie-minting behaviour.""" + r = pw_gated_client.post( + "/auth/password-login", + json={"provider": "testpw", "username": "admin", "password": "hunter2"}, + ) + assert r.status_code == 200, r.text + assert r.json()["next"] == "/" + set_cookie = r.headers.get("set-cookie", "") + assert "hermes_session_at" in set_cookie + + # --------------------------------------------------------------------------- # Native refresh # --------------------------------------------------------------------------- diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 4ab91c2a12..bd72ff9bf9 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -1861,11 +1861,13 @@ export interface StatusResponse { * fail-closed state (the dashboard will refuse to bind). */ auth_providers?: string[]; /** Supported dashboard auth flows for the client to choose from. In gated - * mode always includes ``"cookie"``; includes ``"native_pkce"`` when a - * brokerable OAuth provider is registered, signalling that the desktop can - * use the RFC 8252 system-browser + loopback + PKCE flow (no embedded - * webview, no session cookies). Absent / missing ``"native_pkce"`` ⇒ an - * older gateway ⇒ the desktop falls back to the embedded-webview flow. */ + * mode always includes ``"cookie"``; includes ``"native_pkce"`` when any + * interactive session provider is registered (OAuth providers broker the + * IDP redirect; password providers complete at /login in the system + * browser), signalling that the desktop can use the RFC 8252 + * system-browser + loopback + PKCE flow (no embedded webview, no session + * cookies). Absent / missing ``"native_pkce"`` ⇒ an older gateway ⇒ the + * desktop falls back to the embedded-webview flow. */ auth_flows?: string[]; /** False when the dashboard is running in a hosted/managed layout where * updates are handled by the outer launcher instead of ``hermes update``. */ diff --git a/website/docs/guides/desktop-native-signin.md b/website/docs/guides/desktop-native-signin.md index 96c59048c7..186acec154 100644 --- a/website/docs/guides/desktop-native-signin.md +++ b/website/docs/guides/desktop-native-signin.md @@ -97,13 +97,16 @@ tool blocks the loopback listener, or you close the browser tab — the app ## For gateway operators -Native sign-in is available automatically on any gated gateway that has a -brokerable OAuth provider registered (e.g. the bundled **Nous** provider). No -configuration is required — the `/auth/native/*` routes and the `auth_flows` -advertisement are part of the dashboard-auth subsystem. Password-only and -token-only providers do not advertise `native_pkce` (there is no upstream -redirect to broker), and those deployments continue to use their existing -login. +Native sign-in is available automatically on any gated gateway with an +interactive session provider registered. No configuration is required — the +`/auth/native/*` routes and the `auth_flows` advertisement are part of the +dashboard-auth subsystem. OAuth providers (e.g. the bundled **Nous** provider) +broker the upstream IDP redirect; password providers (e.g. the bundled +**basic-auth** plugin) land the system browser on the gateway's `/login` +credential form instead — which is what lets OS password managers (macOS +Passwords, etc.) autofill the form, something no embedded desktop webview can +offer. Token-only credentials (e.g. drain) are not interactive sign-ins and do +not advertise `native_pkce`. The relevant endpoints (all public, pre-auth bootstrap, same as the existing `/auth/*` OAuth routes):