diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index b275e437b..7694d8f27 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -2,6 +2,15 @@ ## Unreleased +**Fixed:** `e2a login` no longer accumulates an indistinguishable new API key +on every re-authentication. The browser login URL now includes the machine's +hostname as `device_name`; the server uses it to replace that device's own +prior "CLI login on " key instead of minting another one, so +re-running `e2a login` from the same machine (or a config wipe) leaves one +live key per device rather than growing the list forever. A hostname lookup +failure just omits the parameter and falls back to the previous behavior for +that login, so it can never fail the command. + **Changed:** `e2a sending-access status` prints an `available unlocks: ...` line from the deployment's `sending_access.available_unlocks`, and both it and `e2a whoami` offer only the recovery routes the deployment honors. A paid plan diff --git a/cli/src/__tests__/login.test.ts b/cli/src/__tests__/login.test.ts index b63cf0430..5506e1ccd 100644 --- a/cli/src/__tests__/login.test.ts +++ b/cli/src/__tests__/login.test.ts @@ -6,6 +6,7 @@ const mockLoadConfig = vi.fn(); const mockSaveConfig = vi.fn(); const mockCreateServer = vi.fn(); const mockFetch = vi.fn(); +const mockHostname = vi.fn(); const originalFetch = globalThis.fetch; let currentServerHandler: ((req: any, res: any) => void | Promise) | null = null; @@ -43,6 +44,10 @@ vi.mock("../config.js", () => ({ saveConfig: mockSaveConfig, })); +vi.mock("node:os", () => ({ + hostname: mockHostname, +})); + // login probes GET /v1/info with a raw fetch (pre-auth, before a key exists), // so we stub the global fetch. infoResponse() builds the success shape; tests // override per scenario (unreachable -> reject; older deployment -> ok:false). @@ -100,6 +105,10 @@ describe("login", () => { mockExecFile.mockReset(); mockCreateServer.mockClear(); mockFetch.mockReset(); + mockHostname.mockReset(); + // Default: a normal, resolvable hostname. Override per-test for the + // lookup-failure scenario. + mockHostname.mockReturnValue("laptop.local"); // Default: deployment exposes the hosted shared domain. Override per-test // for self-host / older-deployment / unreachable scenarios. mockFetch.mockResolvedValue(infoResponse("agents.e2a.dev")); @@ -156,6 +165,53 @@ describe("login", () => { ); }); + it("sends the machine's hostname as device_name on the browser login URL", async () => { + mockHostname.mockReturnValue("laptop.local"); + let deviceName: string | null = null; + mockExecFile.mockImplementation((_cmd: string, args: string[], cb?: (err: Error | null) => void) => { + const loginUrl = new URL(args[args.length - 1]); + deviceName = loginUrl.searchParams.get("device_name"); + void simulateBrowserCallback({ + cli_state: loginUrl.searchParams.get("cli_state")!, + api_key: "e2a_browser_key", + agent_email: "bot@agents.e2a.dev", + }); + cb?.(null); + return { unref: vi.fn() }; + }); + + const { login } = await import("../commands/login.js"); + await login(); + + expect(deviceName).toBe("laptop.local"); + }); + + it("omits device_name rather than failing the login when the hostname lookup throws", async () => { + mockHostname.mockImplementation(() => { + throw new Error("ENOTFOUND: no hostname available"); + }); + let sawDeviceNameParam = true; + mockExecFile.mockImplementation((_cmd: string, args: string[], cb?: (err: Error | null) => void) => { + const loginUrl = new URL(args[args.length - 1]); + sawDeviceNameParam = loginUrl.searchParams.has("device_name"); + void simulateBrowserCallback({ + cli_state: loginUrl.searchParams.get("cli_state")!, + api_key: "e2a_browser_key", + agent_email: "bot@agents.e2a.dev", + }); + cb?.(null); + return { unref: vi.fn() }; + }); + + const { login } = await import("../commands/login.js"); + await login(); + + expect(sawDeviceNameParam).toBe(false); + expect(mockSaveConfig).toHaveBeenCalledWith( + expect.objectContaining({ api_key: "e2a_browser_key" }), + ); + }); + it("reports an explicitly-set agent_email as the default inbox", async () => { mockLoadConfig.mockReturnValue({ api_key: "", diff --git a/cli/src/commands/login.ts b/cli/src/commands/login.ts index 00e703c84..0aeb5414c 100644 --- a/cli/src/commands/login.ts +++ b/cli/src/commands/login.ts @@ -1,6 +1,7 @@ import { execFile } from "node:child_process"; import { randomBytes } from "node:crypto"; import { createServer, type IncomingMessage, type Server } from "node:http"; +import { hostname } from "node:os"; import { loadConfig, saveConfig } from "../config.js"; /** @@ -108,6 +109,19 @@ function buildBrowserLoginURL(apiUrl: string, callbackUrl: string, cliState: str const loginUrl = new URL("/api/auth/login", apiUrl); loginUrl.searchParams.set("cli_callback", callbackUrl); loginUrl.searchParams.set("cli_state", cliState); + // Lets the server replace this device's own prior "CLI login" key on + // re-auth instead of minting an indistinguishable extra one every time + // (server sanitizes and ignores this if empty or unusable). os.hostname() + // is not documented to throw on any supported platform, but a login must + // never fail over a device label, so a lookup failure just omits it. + try { + const deviceName = hostname(); + if (deviceName) { + loginUrl.searchParams.set("device_name", deviceName); + } + } catch { + // omit device_name + } return loginUrl.toString(); } diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 2db315899..f44ba4b6e 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -72,6 +72,9 @@ type UserAuth struct { type cliLoginHandoff struct { CallbackURL string State string + // DeviceName is the sanitized CLI hostname, or "" if the CLI did not + // supply one. See writeCLIHandoffPage. + DeviceName string } var cliLoginTemplate = template.Must(template.New("cli-login").Parse(` @@ -272,16 +275,40 @@ func validateCLICallbackURL(raw string) (*url.URL, error) { return u, nil } +// maxDeviceNameLen bounds the CLI-supplied device_name before it is used as +// (part of) an API key name shown in the dashboard. +const maxDeviceNameLen = 64 + +// sanitizeDeviceName keeps only printable ASCII from raw, capped at +// maxDeviceNameLen runes; anything else degrades to "" rather than +// failing the login. +func sanitizeDeviceName(raw string) string { + var b strings.Builder + for _, r := range raw { + if r < 0x20 || r > 0x7e { + continue + } + b.WriteRune(r) + if b.Len() >= maxDeviceNameLen { + break + } + } + return strings.TrimSpace(b.String()) +} + // OAuthState is encoded into the OAuth state parameter. It carries the CSRF // nonce and, for CLI-initiated logins, the callback URL and CLI state token. // ReturnTo, if set, is a same-origin server-path the user is bounced back to // after callback succeeds — used by the MCP authorize flow to resume after -// a session is established. Validated at HandleLogin time. +// a session is established. DeviceName, if set, identifies the CLI's host so +// writeCLIHandoffPage can replace that device's own prior key instead of +// minting an indistinguishable extra one. Validated at HandleLogin time. type OAuthState struct { Nonce string `json:"n"` CLICallback string `json:"cb,omitempty"` CLIState string `json:"cs,omitempty"` ReturnTo string `json:"rt,omitempty"` + DeviceName string `json:"dn,omitempty"` } func EncodeOAuthState(s *OAuthState) string { @@ -321,12 +348,18 @@ func defaultAgentEmail(ctx context.Context, store *identity.Store, userID string return agents[0].EmailAddress() } -// writeCLIHandoffPage mints a fresh "CLI login" API key for the user and -// renders the auto-submitting page that POSTs it (plus the CLI's state -// token) to the loopback listener the CLI opened. Shared by every browser -// login door that supports the CLI handoff. +// writeCLIHandoffPage mints an API key for the user and renders the +// auto-submitting page the CLI's loopback listener consumes. A device name +// replaces that device's own prior key by name instead of adding another. func writeCLIHandoffPage(store *identity.Store, w http.ResponseWriter, r *http.Request, user *identity.User, handoff *cliLoginHandoff) error { - key, err := store.CreateAPIKey(r.Context(), user.ID, "CLI login", nil) + name := "CLI login" + if handoff.DeviceName != "" { + name = "CLI login on " + handoff.DeviceName + if err := store.RevokeAPIKeysByName(r.Context(), user.ID, name); err != nil { + return fmt.Errorf("failed to revoke prior device key: %w", err) + } + } + key, err := store.CreateAPIKey(r.Context(), user.ID, name, nil) if err != nil { return fmt.Errorf("failed to create api key: %w", err) } @@ -365,6 +398,7 @@ func (ua *UserAuth) HandleLogin(w http.ResponseWriter, r *http.Request) { } state.CLICallback = callbackURL.String() state.CLIState = cliState + state.DeviceName = sanitizeDeviceName(r.URL.Query().Get("device_name")) } if returnTo := r.URL.Query().Get("return_to"); returnTo != "" { @@ -527,6 +561,7 @@ func (ua *UserAuth) HandleCallback(w http.ResponseWriter, r *http.Request) { handoff := &cliLoginHandoff{ CallbackURL: callbackURL.String(), State: state.CLIState, + DeviceName: state.DeviceName, } if err := writeCLIHandoffPage(ua.store, w, r, user, handoff); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) diff --git a/internal/auth/cli_login_test.go b/internal/auth/cli_login_test.go index 619eb964f..ea36c35a4 100644 --- a/internal/auth/cli_login_test.go +++ b/internal/auth/cli_login_test.go @@ -571,3 +571,169 @@ func TestHandleCallback_RecordsOwnerEmailProof(t *testing.T) { t.Fatalf("a mismatched address must not be recorded: ok=%v err=%v", ok, err) } } + +// TestHandleLogin_EncodesSanitizedDeviceNameInOAuthState: device_name rides +// the OAuth state alongside cli_callback/cli_state, sanitized per +// sanitizeDeviceName's contract (printable ASCII only, capped at 64 runes). +func TestHandleLogin_EncodesSanitizedDeviceNameInOAuthState(t *testing.T) { + raw := "joshzhang-MBP\x07\x1b[31m" + strings.Repeat("x", 100) // \x07/\x1b are control bytes and dropped; + // "[31m" is ordinary printable text (the rest of what would be an ANSI + // color escape, minus its non-printable lead-in) and survives, so the + // printable stream is "joshzhang-MBP[31m" (17 runes) + 100 x's, capped at 64. + want := "joshzhang-MBP[31m" + strings.Repeat("x", 47) // 17 + 47 = 64 + + req := httptest.NewRequest( + http.MethodGet, + "/api/auth/login?cli_callback=http://127.0.0.1:43123/callback&cli_state=cli_state_123&device_name="+url.QueryEscape(raw), + nil, + ) + ua, _, _ := setupUserAuth(t) + w := httptest.NewRecorder() + ua.HandleLogin(w, req) + + if w.Code != http.StatusFound { + t.Fatalf("status = %d, want %d", w.Code, http.StatusFound) + } + u, err := url.Parse(w.Result().Header.Get("Location")) + if err != nil { + t.Fatalf("parse redirect URL: %v", err) + } + stateJSON, err := base64.URLEncoding.DecodeString(u.Query().Get("state")) + if err != nil { + t.Fatalf("decode state: %v", err) + } + var state struct { + DeviceName string `json:"dn"` + } + if err := json.Unmarshal(stateJSON, &state); err != nil { + t.Fatalf("unmarshal state: %v", err) + } + if state.DeviceName != want { + t.Fatalf("device name = %q (len %d), want %q (len %d)", state.DeviceName, len(state.DeviceName), want, len(want)) + } +} + +// TestHandleLogin_WebLoginIgnoresDeviceName: device_name only means anything +// alongside a CLI handoff; a plain web login (no cli_callback) must not +// carry it into the state even if the query string sends one. +func TestHandleLogin_WebLoginIgnoresDeviceName(t *testing.T) { + ua, _, _ := setupUserAuth(t) + req := httptest.NewRequest(http.MethodGet, "/api/auth/login?device_name=some-host", nil) + w := httptest.NewRecorder() + ua.HandleLogin(w, req) + + u, _ := url.Parse(w.Result().Header.Get("Location")) + stateJSON, _ := base64.URLEncoding.DecodeString(u.Query().Get("state")) + var state struct { + DeviceName string `json:"dn"` + } + json.Unmarshal(stateJSON, &state) + if state.DeviceName != "" { + t.Fatalf("web login should not carry a device name, got %q", state.DeviceName) + } +} + +// TestHandleCallback_CLILogin_SameDeviceReplacesPriorKey is the regression +// test for the bug this fix targets: two logins from the same named device +// leave exactly one live "CLI login on " key, and it is the second +// mint, not the first. +func TestHandleCallback_CLILogin_SameDeviceReplacesPriorKey(t *testing.T) { + ua, store, srv := setupUserAuthWithFakeOAuth(t) + _ = srv + ctx := context.Background() + + login := func(nonce string) { + t.Helper() + state := auth.EncodeOAuthState(&auth.OAuthState{ + Nonce: nonce, + CLICallback: "http://127.0.0.1:43123/callback", + CLIState: "cli_state_abc", + DeviceName: "joshzhang-MBP", + }) + req := httptest.NewRequest( + http.MethodGet, + fmt.Sprintf("/api/auth/callback?code=fake-code&state=%s", url.QueryEscape(state)), + nil, + ) + req.AddCookie(&http.Cookie{Name: "e2a_oauth_state", Value: nonce}) + w := httptest.NewRecorder() + ua.HandleCallback(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + } + + login("nonce-device-1") + login("nonce-device-2") + + user, err := store.CreateOrGetUser(ctx, "cliuser@test.com", "CLI User", "google-sub-cli-test") + if err != nil { + t.Fatalf("CreateOrGetUser: %v", err) + } + keys, err := store.ListAPIKeys(ctx, user.ID, 0, time.Time{}, "") + if err != nil { + t.Fatalf("ListAPIKeys: %v", err) + } + var named []identity.APIKey + for _, k := range keys { + if k.Name == "CLI login on joshzhang-MBP" { + named = append(named, k) + } + } + if len(named) != 1 { + t.Fatalf("live keys named %q = %d, want 1 (re-login from the same device must replace, not accumulate)", "CLI login on joshzhang-MBP", len(named)) + } +} + +// TestHandleCallback_CLILogin_NoDeviceName_PreservesLegacyAccumulation pins +// the deliberate scope boundary: without a device name (older CLI binaries, +// or a login door that never plumbs one) writeCLIHandoffPage must keep +// minting distinct "CLI login" keys rather than revoking by that +// un-device-scoped shared name, which could otherwise revoke a different, +// still-live device's key out from under it. +func TestHandleCallback_CLILogin_NoDeviceName_PreservesLegacyAccumulation(t *testing.T) { + ua, store, srv := setupUserAuthWithFakeOAuth(t) + _ = srv + ctx := context.Background() + + login := func(nonce string) { + t.Helper() + state := auth.EncodeOAuthState(&auth.OAuthState{ + Nonce: nonce, + CLICallback: "http://127.0.0.1:43123/callback", + CLIState: "cli_state_abc", + }) + req := httptest.NewRequest( + http.MethodGet, + fmt.Sprintf("/api/auth/callback?code=fake-code&state=%s", url.QueryEscape(state)), + nil, + ) + req.AddCookie(&http.Cookie{Name: "e2a_oauth_state", Value: nonce}) + w := httptest.NewRecorder() + ua.HandleCallback(w, req) + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body: %s", w.Code, http.StatusOK, w.Body.String()) + } + } + + login("nonce-legacy-1") + login("nonce-legacy-2") + + user, err := store.CreateOrGetUser(ctx, "cliuser@test.com", "CLI User", "google-sub-cli-test") + if err != nil { + t.Fatalf("CreateOrGetUser: %v", err) + } + keys, err := store.ListAPIKeys(ctx, user.ID, 0, time.Time{}, "") + if err != nil { + t.Fatalf("ListAPIKeys: %v", err) + } + var plain int + for _, k := range keys { + if k.Name == "CLI login" { + plain++ + } + } + if plain != 2 { + t.Fatalf("live \"CLI login\" keys = %d, want 2 (unnamed-device logins are unchanged by this fix)", plain) + } +} diff --git a/internal/identity/store.go b/internal/identity/store.go index 0f4629009..bb1b4fd0e 100644 --- a/internal/identity/store.go +++ b/internal/identity/store.go @@ -6736,6 +6736,18 @@ func (s *Store) DeleteAPIKey(ctx context.Context, keyID, userID string) error { return nil } +// RevokeAPIKeysByName soft-deletes every live key the user owns with the +// exact given name. Unlike DeleteAPIKey, matching zero rows is not an error: +// the caller (writeCLIHandoffPage) uses this to take over a device's own +// prior key before minting its replacement, and "no prior key yet" is the +// ordinary first-login case, not a failure. +func (s *Store) RevokeAPIKeysByName(ctx context.Context, userID, name string) error { + _, err := s.pool.Exec(ctx, + `UPDATE api_keys SET revoked_at = now() WHERE user_id = $1 AND name = $2 AND revoked_at IS NULL`, userID, name, + ) + return err +} + // GetUserByAPIKey authenticates a bearer token and returns the owning // user. Rejects revoked keys and time-expired keys; touches last_used_at // only on the success path so the column stays a real "last successful