From 7bedd9c254652e8ca29c29b04b4cdc7fcf15269f Mon Sep 17 00:00:00 2001 From: Juha Litola Date: Wed, 29 Apr 2026 09:09:59 +0300 Subject: [PATCH] fix: require explicit plaintext auth storage Default OAuth credential persistence now fails safely when the system keychain is unavailable, while explicit file mode supports headless environments with clear plaintext warnings. --- README.md | 15 +- bun.lock | 3 + docs/implementation/auth.md | 53 +- docs/implementation/config.md | 50 +- package.json | 1 + src/cli.ts | 25 +- src/commands/auth-status.ts | 7 +- src/commands/init/init.ts | 4 +- src/commands/login.test.ts | 46 +- src/commands/login.ts | 45 +- src/commands/logout.ts | 7 +- src/container.test.ts | 67 ++ src/container.ts | 127 +++- src/services/app-config-paths.test.ts | 79 +++ src/services/app-config-paths.ts | 39 ++ src/services/auth-config.test.ts | 118 ++++ src/services/auth-config.ts | 96 +++ src/services/auth-storage.test.ts | 28 +- src/services/auth-storage.ts | 19 +- src/services/filesystem-service.ts | 3 +- src/services/index.ts | 16 + src/services/migrating-auth-storage.test.ts | 606 +++++++++--------- src/services/migrating-auth-storage.ts | 404 ++++++++---- .../mode-aware-file-auth-storage.test.ts | 49 ++ src/services/mode-aware-file-auth-storage.ts | 78 +++ 25 files changed, 1464 insertions(+), 521 deletions(-) create mode 100644 src/container.test.ts create mode 100644 src/services/app-config-paths.test.ts create mode 100644 src/services/app-config-paths.ts create mode 100644 src/services/auth-config.test.ts create mode 100644 src/services/auth-config.ts create mode 100644 src/services/mode-aware-file-auth-storage.test.ts create mode 100644 src/services/mode-aware-file-auth-storage.ts diff --git a/README.md b/README.md index 705e30ba..120d5f46 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,7 @@ GitHits requires authentication. There are two options: npx githits login ``` -Opens your browser for secure OAuth authentication. Tokens are stored locally and refreshed automatically on next use. If a refresh fails (e.g., after an extended idle period), run `githits login` again. +Opens your browser for secure OAuth authentication. Tokens are stored in the system keychain by default and refreshed automatically on next use. If a refresh fails (e.g., after an extended idle period), run `githits login` again. Useful flags: @@ -148,6 +148,18 @@ For CI or environments where browser login isn't practical, set an environment v export GITHITS_API_TOKEN=ghi-your-token-here ``` +For machines without a usable keychain, OAuth file storage must be explicit: + +```toml +# ~/.config/githits/config.toml on Linux +[auth] +storage = "file" +``` + +File storage is plaintext on disk. Prefer `GITHITS_API_TOKEN` for CI and automation. + +You can also opt in for one process with `GITHITS_AUTH_STORAGE=file`. Use file storage only on machines where local file access is trusted. + ## Commands ``` @@ -177,6 +189,7 @@ githits code ... Dependency source inspection: search, files, read, grep | Variable | Purpose | Default | |---|---|---| | `GITHITS_API_TOKEN` | API token for authentication | — | +| `GITHITS_AUTH_STORAGE` | Override OAuth storage mode (`keychain` or `file`) | `keychain` | | `GITHITS_MCP_URL` | Override MCP server URL | `https://mcp.githits.com` | | `GITHITS_API_URL` | Override REST API URL | `https://api.githits.com` | | `GITHITS_CODE_NAV_URL` | Override package/source service URL | `https://pkgseer.dev` | diff --git a/bun.lock b/bun.lock index 54a5e357..db759d0e 100644 --- a/bun.lock +++ b/bun.lock @@ -12,6 +12,7 @@ "jsonc-parser": "^3.3.1", "open": "^11.0.0", "semver": "^7.7.4", + "smol-toml": "^1.6.1", "zod": "^4.1.13", }, "devDependencies": { @@ -512,6 +513,8 @@ "semver": ["semver@7.7.4", "", { "bin": { "semver": "bin/semver.js" } }, "sha512-vFKC2IEtQnVhpT78h1Yp8wzwrf8CM+MzKMHGJZfBtzhZNycRFnXsHk6E5TxIkkMsgNS7mdX3AGB7x2QM2di4lA=="], + "smol-toml": ["smol-toml@1.6.1", "", {}, "sha512-dWUG8F5sIIARXih1DTaQAX4SsiTXhInKf1buxdY9DIg4ZYPZK5nGM1VRIYmEbDbsHt7USo99xSLFu5Q1IqTmsg=="], + "send": ["send@1.2.1", "", { "dependencies": { "debug": "^4.4.3", "encodeurl": "^2.0.0", "escape-html": "^1.0.3", "etag": "^1.8.1", "fresh": "^2.0.0", "http-errors": "^2.0.1", "mime-types": "^3.0.2", "ms": "^2.1.3", "on-finished": "^2.4.1", "range-parser": "^1.2.1", "statuses": "^2.0.2" } }, "sha512-1gnZf7DFcoIcajTjTwjwuDjzuz4PPcY2StKPlsGAQ1+YH20IRVrBaXSWmdjowTJ6u8Rc01PoYOGHXfP1mYcZNQ=="], "serve-static": ["serve-static@2.2.1", "", { "dependencies": { "encodeurl": "^2.0.0", "escape-html": "^1.0.3", "parseurl": "^1.3.3", "send": "^1.2.0" } }, "sha512-xRXBn0pPqQTVQiC8wyQrKs2MOlX24zQ0POGaj0kultvoOCstBQM5yvOhAVSUwOMjQtTvsPWoNCHfPGwaaQJhTw=="], diff --git a/docs/implementation/auth.md b/docs/implementation/auth.md index 7db059a2..0c3ea091 100644 --- a/docs/implementation/auth.md +++ b/docs/implementation/auth.md @@ -31,7 +31,7 @@ The login command (`src/commands/login.ts`) orchestrates a 9-step OAuth flow (ma 6. **Open browser or print URL** — Navigate user to auth URL, or print it if `--no-browser` is set 7. **Verify state** — CSRF protection check on the callback 8. **Exchange code for tokens** — POST to token endpoint with PKCE verifier -9. **Save tokens** — Store to system keychain (or fallback file storage) +9. **Save tokens** — Store to the configured auth store The flow has a 5-minute timeout. The callback server must start before the browser opens so it's ready to receive the redirect. @@ -53,7 +53,9 @@ To clear tokens manually, use `githits logout`. This removes stored tokens for t ## Storage -Credentials are stored in the **system keychain** (macOS Keychain, Windows Credential Manager, Linux Secret Service) via `@napi-rs/keyring`. If the keychain is unavailable (headless Linux, CI), the CLI falls back to file-based storage in `~/.githits/` with a stderr warning. +Credentials are stored in the **system keychain** by default (macOS Keychain, Windows Credential Manager, Linux Secret Service) via `@napi-rs/keyring`. The CLI does not silently downgrade OAuth credentials to plaintext files when the keychain is unavailable. + +Machines without a usable keychain can explicitly opt into plaintext OAuth storage with `auth.storage = "file"` in `config.toml` or `GITHITS_AUTH_STORAGE=file`. `GITHITS_API_TOKEN` remains the preferred automation/CI path because it avoids storing OAuth refresh credentials. ### Keychain storage (primary) @@ -84,36 +86,58 @@ Values under 1200 characters are stored directly with no sentinel, maintaining f The `getStorageLocation()` method returns a platform-specific label: "macOS Keychain (githits)" on macOS, "Windows Credential Manager (githits)" on Windows, and "System keychain (githits)" on Linux. -### File storage (fallback) +### File storage (explicit) -When the keychain is unavailable, auth data is stored in `~/.githits/` with two files: +When `auth.storage = "file"`, auth data is stored under the platform config auth directory with two files: | File | Content | Structure | |---|---|---| | `auth.json` | OAuth tokens | `{ version: 1, tokens: { [mcpUrl]: { accessToken, refreshToken, expiresAt (string\|null), createdAt } } }` | | `client.json` | DCR client registration | `{ version: 1, clients: { [mcpUrl]: { clientId, clientSecret, redirectUri, registeredAt } } }` | -Both files use 0600 permissions. The directory uses 0700. +Both files use 0600 permissions. The directory uses 0700. Rewrites use `FileSystemService.atomicWriteFile()` so a crash does not leave half-written JSON. This protects against other local users but does not encrypt credentials at rest. + +Typical Linux layout: + +```text +~/.config/githits/ + config.toml + auth/ + auth.json + client.json +``` + +The legacy `~/.githits/auth.json` and `~/.githits/client.json` path is still read for migration and cleared by logout, but new file-mode writes go to the platform config auth directory. ### Migration -On first use after upgrading, the `MigratingAuthStorage` decorator transparently migrates credentials from files to the keychain: +On first use after upgrading, `MigratingAuthStorage` transparently migrates credentials according to the configured storage mode: + +Keychain mode: 1. Check keychain — if found, return it -2. Check file — if found, write to keychain, delete from file, return it +2. Check new file path, then legacy `~/.githits` — if found, write to keychain, delete the migrated plaintext entry, return it 3. Both empty — return null -Keychain write must succeed before the file entry is deleted. Tokens and client registrations migrate independently. +File mode: + +1. Check new file path — if found, return it +2. Check legacy `~/.githits` — if found, write to new file path, delete legacy entry, return it +3. Check keychain only as a last-resort migration source, then warn before exporting encrypted credentials to plaintext + +The configured target write must succeed before the source entry is deleted. Tokens and client registrations migrate independently. If both plaintext paths contain entries, the newer timestamp wins; ambiguous ties prefer the new file path and leave the other entry intact with a warning. ### Architecture ``` Container (createAuthStorage) └─ MigratingAuthStorage (decorator) - ├─ KeychainAuthStorage (primary) + ├─ KeychainAuthStorage │ └─ ChunkingKeyringService (Windows only, decorator) │ └─ KeyringServiceImpl ← @napi-rs/keyring - └─ AuthStorageImpl (legacy) ← file-based + ├─ ModeAwareFileAuthStorage + │ └─ AuthStorageImpl ← platform config auth path + └─ AuthStorageImpl ← legacy ~/.githits path ``` All credential types are keyed by normalized MCP base URL (trailing slashes stripped), supporting multiple environments simultaneously. @@ -147,7 +171,7 @@ The `hasValidToken` flag is checked by `requireAuth()` in `src/commands/mcp.ts` - **Port conflicts on login** — The callback server uses the port from the stored client registration. On first login, a random port (8000–9999) is chosen and saved. Use `--port ` to change it (triggers re-registration). - **Token refresh fails silently** — By design. The container clears stale auth and `hasValidToken` becomes false, prompting re-login. - **Clearing auth** — Run `githits logout` to remove stored tokens and client registration for the current environment. -- **Keychain unavailable warning** — If the system keychain is not accessible (headless Linux, CI), the CLI falls back to file storage in `~/.githits/` and prints a warning to stderr. +- **System keychain unavailable** — In default keychain mode, OAuth login/refresh fails rather than writing plaintext credentials. Use `GITHITS_API_TOKEN`, fix/unlock the keychain, or explicitly configure `auth.storage = "file"` if plaintext local storage is acceptable. - **Windows "password encoded as UTF-16 is longer than platform limit"** — The Windows Credential Manager limits credential blobs to 2560 bytes (`CRED_MAX_CREDENTIAL_BLOB_SIZE`). Since passwords are stored as UTF-16 (2 bytes per char), the effective limit is 1280 characters. The `ChunkingKeyringService` decorator handles this automatically by splitting large values across multiple entries. If this error occurs on an older CLI version, upgrade to get chunked storage support. ## Key Reference Files @@ -156,17 +180,20 @@ The `hasValidToken` flag is checked by `requireAuth()` in `src/commands/mcp.ts` |---|---| | `src/commands/login.ts` | Full OAuth PKCE flow orchestration | | `src/commands/logout.ts` | Token and client removal and storage cleanup | -| `src/container.ts` | Dependency wiring, keychain probe with fallback | +| `src/container.ts` | Dependency wiring and auth-command container without eager token refresh | | `src/services/token-manager.ts` | `TokenProvider` interface, `TokenManager` (proactive refresh, coalescing) | | `src/services/refreshing-githits-service.ts` | `GitHitsService` decorator with token refresh and 401 retry | | `src/services/execute-with-token-refresh.ts` | Shared helper for token-authenticated retry-on-refresh flows | | `src/services/code-navigation-service.ts` | Package/source service client using the shared refresh helper | | `src/services/auth-service.ts` | OAuth operations (DCR, PKCE, token exchange, callback server) | | `src/services/auth-storage.ts` | `AuthStorage` interface and file-based implementation | +| `src/services/auth-config.ts` | `config.toml` and `GITHITS_AUTH_STORAGE` parsing | +| `src/services/app-config-paths.ts` | Platform-specific config/auth path resolution | +| `src/services/mode-aware-file-auth-storage.ts` | File-write guard for `auth.storage` policy | | `src/services/keyring-service.ts` | `KeyringService` interface wrapping `@napi-rs/keyring` | | `src/services/chunking-keyring-service.ts` | `KeyringService` decorator for chunked storage (Windows 2560-char limit) | | `src/services/keychain-auth-storage.ts` | `AuthStorage` implementation backed by system keychain | -| `src/services/migrating-auth-storage.ts` | Migration decorator (keychain primary + file legacy) | +| `src/services/migrating-auth-storage.ts` | Mode-aware migration across keychain, config file storage, and legacy file storage | | `src/services/filesystem-service.ts` | File system abstraction for testable storage | | `src/auth/pkce.ts` | PKCE cryptographic primitives | | `src/services/config.ts` | URL and API token configuration | diff --git a/docs/implementation/config.md b/docs/implementation/config.md index 0d638853..a8123961 100644 --- a/docs/implementation/config.md +++ b/docs/implementation/config.md @@ -18,7 +18,7 @@ GitHits separates its MCP server (which handles OAuth discovery and the MCP prot > **These are different services.** Override every URL that differs from production when pointing to a non-production backend. -The MCP URL is also used as the storage key for tokens and client registrations in `~/.githits/` (trailing slashes are stripped for consistent key matching). This means tokens from one environment don't leak into another. +The MCP URL is also used as the storage key for tokens and client registrations (trailing slashes are stripped for consistent key matching). This means tokens from one environment don't leak into another. ## Authentication Modes @@ -26,7 +26,7 @@ The container (`src/container.ts`) resolves authentication in priority order: 1. **`GITHITS_API_TOKEN`** — If set, uses this token directly. No OAuth flow needed. Quick to set up for CI and automation environments. -2. **Stored OAuth JWT** — Loaded from `~/.githits/auth.json`. If expired, the container automatically attempts a refresh using the stored refresh token. If refresh fails, auth is cleared silently. +2. **Stored OAuth JWT** — Loaded from the configured auth store. If expired, the container automatically attempts a refresh using the stored refresh token. If refresh fails, auth is cleared silently. 3. **Unauthenticated** — No token available. Auth-required CLI commands fail on use, and the MCP server can start but every authenticated tool call will fail. Commands like `auth status` still work to help the user diagnose the issue. @@ -48,19 +48,47 @@ Package/source access uses the package/source service URL from `GITHITS_CODE_NAV | `GITHITS_API_URL` | Override REST API URL | `http://localhost:8000` | | `GITHITS_CODE_NAV_URL` | Override package/source service URL | `http://localhost:4000` | | `GITHITS_API_TOKEN` | API token for authentication | `ghi-abc123...` | +| `GITHITS_AUTH_STORAGE` | Override OAuth credential storage for the current process (`keychain` or `file`) | `file` | | `GITHITS_TELEMETRY` | Emit end-of-run timing spans to stderr for local profiling | `1` | ## Local Storage -Authentication state lives in `~/.githits/`: +GitHits config uses the platform config directory: +```toml +# ~/.config/githits/config.toml on Linux +[auth] +storage = "keychain" ``` -~/.githits/ (0700) - auth.json (0600) — OAuth tokens keyed by MCP URL - client.json (0600) — DCR client registrations keyed by MCP URL + +| `auth.storage` | Meaning | +|---|---| +| `keychain` | Default. Store OAuth tokens and DCR client secrets in the system keychain only. | +| `file` | Store OAuth tokens and DCR client secrets as plaintext JSON under the platform config auth path. | + +Invalid `GITHITS_AUTH_STORAGE` or `auth.storage` values fail fast with a message that includes the expected values. Runtime keychain-unavailable errors include the exact config file path and the `[auth] storage = "file"` snippet, plus a plaintext-storage warning. + +When file mode is enabled, storage uses secure file permissions but is not encrypted: + +```text +~/.config/githits/ (0700 on Linux) + config.toml (0600 when written by GitHits) + auth/ (0700) + auth.json (0600) — OAuth tokens keyed by MCP URL + client.json (0600) — DCR client registrations keyed by MCP URL ``` -The secure file permissions prevent other users from reading tokens. When writing new files to `~/.githits/`, use `FileSystemService` rather than `node:fs` directly — this enables testing via mock implementations from `src/services/test-helpers.ts`. +Platform roots: + +| Platform | Config root | +|---|---| +| Linux/Unix | `$XDG_CONFIG_HOME/githits`, or `~/.config/githits` | +| macOS | `~/Library/Application Support/githits` | +| Windows | `%APPDATA%\githits`, or `~/AppData/Roaming/githits` | + +Legacy `~/.githits/auth.json` and `~/.githits/client.json` are still read for migration and cleared by logout, but new plaintext writes use the platform config auth path. + +When writing config or auth files, use `FileSystemService` rather than `node:fs` directly — this enables testing via mock implementations from `src/services/test-helpers.ts`. Auth credential rewrites should use `atomicWriteFile()` to avoid truncated JSON on crashes. Non-secret update-check state uses the XDG config location: @@ -76,12 +104,13 @@ eligibility rules. ## How Config Flows Through the System ``` -Environment variables - └─ src/services/config.ts (getMcpUrl, getApiUrl, getCodeNavigationUrl, getEnvApiToken) +Environment variables + config.toml + └─ src/services/config.ts / auth-config.ts └─ src/container.ts (createContainer) ├─ mcpUrl → passed to auth commands, used as storage key ├─ apiUrl → passed to GitHitsServiceImpl constructor ├─ codeNavigationUrl → passed to CodeNavigationServiceImpl and PackageIntelligenceServiceImpl + ├─ auth.storage → controls OAuth credential persistence ├─ apiToken → resolved from env var or OAuth storage └─ hasValidToken → gates authenticated commands ``` @@ -93,6 +122,7 @@ Commands receive the full `Dependencies` object. Services receive only what they - **"Authentication required" despite having a token** — Token may be expired and refresh failed. Run `githits login` to re-authenticate. - **Custom environment not working** — Make sure both `GITHITS_MCP_URL` and `GITHITS_API_URL` are set. They point to different services. - **Tokens from wrong environment** — Tokens are stored per MCP URL. If you switched `GITHITS_MCP_URL`, you need to re-authenticate for the new URL. +- **System keychain unavailable** — Default keychain mode fails rather than writing plaintext OAuth credentials. Use `GITHITS_API_TOKEN`, fix/unlock the keychain, or set `auth.storage = "file"` / `GITHITS_AUTH_STORAGE=file` if unencrypted file storage is acceptable. ### Init config parsing behavior @@ -107,6 +137,8 @@ Commands receive the full `Dependencies` object. Services receive only what they | File | What it demonstrates | |---|---| | `src/services/config.ts` | URL and token resolution from environment | +| `src/services/auth-config.ts` | `config.toml` and `GITHITS_AUTH_STORAGE` auth storage mode parsing | +| `src/services/app-config-paths.ts` | Platform config path resolution | | `src/container.ts` | Auth priority logic and dependency wiring | | `src/services/auth-storage.ts` | File-based token storage with secure permissions | | `src/services/filesystem-service.ts` | File system abstraction for testable storage | diff --git a/package.json b/package.json index 23421528..d3d2e455 100644 --- a/package.json +++ b/package.json @@ -75,6 +75,7 @@ "jsonc-parser": "^3.3.1", "open": "^11.0.0", "semver": "^7.7.4", + "smol-toml": "^1.6.1", "zod": "^4.1.13" }, "devDependencies": { diff --git a/src/cli.ts b/src/cli.ts index 9070346c..ccecf369 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -19,6 +19,7 @@ import { registerPkgCommandGroup, registerUnifiedSearchCommands, } from "./commands/index.js"; +import { AuthConfigError, AuthStoragePolicyError } from "./services/index.js"; import { FileSystemServiceImpl, NpmRegistryUpdateCheckService, @@ -133,11 +134,25 @@ const authCommand = program .description("Manage authentication with GitHits."); registerAuthStatusCommand(authCommand); -await runWithUpdateCheckFlush( - () => withTelemetrySpan("cli.parse", () => program.parseAsync()), - updateCheckTask, - { stderr: process.stderr }, -); +try { + await runWithUpdateCheckFlush( + () => withTelemetrySpan("cli.parse", () => program.parseAsync()), + updateCheckTask, + { stderr: process.stderr }, + ); +} catch (error) { + if (isUserFacingError(error)) { + console.error(`${error.message}\n`); + process.exit(1); + } + throw error; +} + +function isUserFacingError(error: unknown): error is Error { + return ( + error instanceof AuthConfigError || error instanceof AuthStoragePolicyError + ); +} /** * Commander supports root options before subcommands, e.g. diff --git a/src/commands/auth-status.ts b/src/commands/auth-status.ts index 40eb0e18..949b6593 100644 --- a/src/commands/auth-status.ts +++ b/src/commands/auth-status.ts @@ -1,5 +1,5 @@ import type { Command } from "commander"; -import { createContainer } from "../container.js"; +import { createAuthStatusDependencies } from "../container.js"; import type { AuthService, AuthStorage } from "../services/index.js"; import { refreshExpiredToken } from "../services/index.js"; @@ -95,7 +95,8 @@ export async function authStatusAction( const STATUS_DESCRIPTION = `Show current authentication status. Displays details about the stored token including environment -and expiration. Useful for debugging authentication issues.`; +and expiration. If GITHITS_API_TOKEN is set, reports that source +without reading local OAuth storage. Useful for debugging authentication issues.`; /** * Register the auth status command on the given program. @@ -107,7 +108,7 @@ export function registerAuthStatusCommand(program: Command) { .summary("Show authentication status") .description(STATUS_DESCRIPTION) .action(async () => { - const deps = await createContainer(); + const deps = await createAuthStatusDependencies(); await authStatusAction(deps); }); } diff --git a/src/commands/init/init.ts b/src/commands/init/init.ts index 88e0cc4e..912540cc 100644 --- a/src/commands/init/init.ts +++ b/src/commands/init/init.ts @@ -1,6 +1,6 @@ import { ExitPromptError } from "@inquirer/core"; import type { Command } from "commander"; -import { createContainer } from "../../container.js"; +import { createAuthCommandDependencies } from "../../container.js"; import type { ExecService } from "../../services/exec-service.js"; import { ExecServiceImpl } from "../../services/exec-service.js"; import type { FileSystemService } from "../../services/filesystem-service.js"; @@ -320,7 +320,7 @@ export function registerInitCommand(program: Command) { fileSystemService, promptService, execService, - createLoginDeps: () => createContainer(), + createLoginDeps: () => createAuthCommandDependencies(), }); }); } diff --git a/src/commands/login.test.ts b/src/commands/login.test.ts index 72aa0e70..5f30c237 100644 --- a/src/commands/login.test.ts +++ b/src/commands/login.test.ts @@ -30,6 +30,10 @@ describe("loginAction", () => { expect(browserService.open).toHaveBeenCalled(); expect(authService.exchangeCodeForTokens).toHaveBeenCalled(); expect(authStorage.saveTokens).toHaveBeenCalled(); + expect(authStorage.saveClient).toHaveBeenCalledWith( + expect.stringContaining("__githits_storage_probe__"), + expect.any(Object), + ); consoleSpy.mockRestore(); }); @@ -148,7 +152,31 @@ describe("loginAction", () => { ); expect(authService.registerClient).toHaveBeenCalled(); - expect(authStorage.saveClient).toHaveBeenCalled(); + expect(authStorage.saveClient).toHaveBeenCalledWith( + mcpUrl, + expect.any(Object), + ); + consoleSpy.mockRestore(); + }); + + it("fails before remote registration when storage preflight fails", async () => { + const consoleSpy = spyOn(console, "log").mockImplementation(() => {}); + const authStorage = createMockAuthStorage({ + saveClient: mock(() => Promise.reject(new Error("keychain locked"))), + }); + const authService = createMockAuthService(); + const browserService = createMockBrowserService(); + + const result = await loginFlow( + { port: 8080 }, + { authService, authStorage, browserService, mcpUrl }, + ); + + expect(result.status).toBe("failed"); + expect(result.message).toContain("Cannot persist OAuth credentials"); + expect(authService.discoverEndpoints).not.toHaveBeenCalled(); + expect(authService.registerClient).not.toHaveBeenCalled(); + expect(browserService.open).not.toHaveBeenCalled(); consoleSpy.mockRestore(); }); @@ -216,13 +244,13 @@ describe("loginAction", () => { throw new Error("process.exit"); }); - let clearClientCallCount = 0; + let mcpClearClientCallCount = 0; const authStorage = createMockAuthStorage({ - clearClient: mock(() => { - clearClientCallCount++; - // First call is from the !existing path (Part 1), let it succeed. - // Second call is from the catch block (Part 2), make it fail. - if (clearClientCallCount > 1) { + clearClient: mock((baseUrl: string) => { + if (baseUrl === mcpUrl) { + mcpClearClientCallCount++; + } + if (baseUrl === mcpUrl && mcpClearClientCallCount > 1) { throw new Error("fs error"); } return Promise.resolve(); @@ -278,7 +306,7 @@ describe("loginAction", () => { }, ); - expect(authStorage.clearClient).not.toHaveBeenCalled(); + expect(authStorage.clearClient).not.toHaveBeenCalledWith(mcpUrl); expect(authStorage.saveTokens).not.toHaveBeenCalled(); consoleSpy.mockRestore(); }); @@ -305,7 +333,7 @@ describe("loginAction", () => { }, ); - expect(authStorage.clearClient).not.toHaveBeenCalled(); + expect(authStorage.clearClient).not.toHaveBeenCalledWith(mcpUrl); expect(authStorage.saveTokens).toHaveBeenCalled(); consoleSpy.mockRestore(); }); diff --git a/src/commands/login.ts b/src/commands/login.ts index 8534baab..8a39cd94 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -1,5 +1,5 @@ import type { Command } from "commander"; -import { createContainer } from "../container.js"; +import { createAuthCommandDependencies } from "../container.js"; import type { AuthService, AuthStorage, @@ -31,6 +31,40 @@ export interface LoginDependencies { mcpUrl: string; } +async function preflightAuthPersistence( + authStorage: AuthStorage, + mcpUrl: string, +): Promise { + const probeUrl = `${mcpUrl.replace(/\/+$/, "")}/__githits_storage_probe__`; + const probeClient = { + clientId: "__githits_storage_probe__", + clientSecret: "__githits_storage_probe__", + redirectUri: "http://127.0.0.1:1/callback", + registeredAt: new Date(0).toISOString(), + }; + const probeTokens = { + accessToken: "__githits_storage_probe__", + refreshToken: "__githits_storage_probe__", + expiresAt: new Date(0).toISOString(), + createdAt: new Date(0).toISOString(), + }; + try { + await authStorage.saveClient(probeUrl, probeClient); + await authStorage.saveTokens(probeUrl, probeTokens); + await authStorage.clearTokens(probeUrl); + await authStorage.clearClient(probeUrl); + return null; + } catch (error) { + await authStorage.clearTokens(probeUrl).catch(() => {}); + await authStorage.clearClient(probeUrl).catch(() => {}); + const message = error instanceof Error ? error.message : String(error); + return { + status: "failed", + message: `Cannot persist OAuth credentials: ${message}`, + }; + } +} + /** * Core login logic that returns a result instead of calling process.exit. * Used by both the standalone `login` command and the `init` command. @@ -71,6 +105,9 @@ export async function loginFlow( await authStorage.clearClient(mcpUrl); } + const persistenceError = await preflightAuthPersistence(authStorage, mcpUrl); + if (persistenceError) return persistenceError; + // Step 1: Discover OAuth endpoints console.log("Discovering OAuth endpoints..."); const metadata = await authService.discoverEndpoints(mcpUrl); @@ -259,7 +296,9 @@ export async function loginAction( const LOGIN_DESCRIPTION = `Authenticate with your GitHits account via browser. Opens your browser to complete authentication securely using OAuth. -The CLI receives tokens stored locally and used for API requests. +OAuth credentials are stored in the system keychain by default. If your +machine has no usable keychain, use GITHITS_API_TOKEN or explicitly configure +auth.storage = "file". File storage is plaintext on disk. Use --no-browser in environments without a display (CI, SSH sessions) to get a URL you can open on another device.`; @@ -277,7 +316,7 @@ export function registerLoginCommand(program: Command) { .option("--port ", "Port for local callback server", parseInt) .option("--force", "Re-authenticate even if already logged in") .action(async (options: LoginOptions) => { - const deps = await createContainer(); + const deps = await createAuthCommandDependencies(); await loginAction(options, deps); }); } diff --git a/src/commands/logout.ts b/src/commands/logout.ts index edd9ac43..81434d3a 100644 --- a/src/commands/logout.ts +++ b/src/commands/logout.ts @@ -1,5 +1,5 @@ import type { Command } from "commander"; -import { createContainer } from "../container.js"; +import { createAuthCommandDependencies } from "../container.js"; import type { AuthStorage } from "../services/index.js"; export interface LogoutDependencies { @@ -49,7 +49,8 @@ const LOGOUT_DESCRIPTION = `Remove stored credentials. Clears all locally stored authentication data including tokens and client registrations. OAuth tokens expire naturally; this -removes the local copies from the keychain (or fallback file storage).`; +removes local copies from the keychain, explicit file storage, and +legacy auth file storage.`; /** * Register the logout command on the given program. @@ -61,7 +62,7 @@ export function registerLogoutCommand(program: Command) { .summary("Remove stored credentials") .description(LOGOUT_DESCRIPTION) .action(async () => { - const deps = await createContainer(); + const deps = await createAuthCommandDependencies(); await logoutAction(deps); }); } diff --git a/src/container.test.ts b/src/container.test.ts new file mode 100644 index 00000000..0024a195 --- /dev/null +++ b/src/container.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "bun:test"; +import { + createAuthCommandDependencies, + createAuthStatusDependencies, +} from "./container.js"; +import { AuthConfigError } from "./services/auth-config.js"; + +async function withAuthStorageEnv( + value: string | undefined, + fn: () => Promise, +): Promise { + const original = process.env.GITHITS_AUTH_STORAGE; + if (value === undefined) delete process.env.GITHITS_AUTH_STORAGE; + else process.env.GITHITS_AUTH_STORAGE = value; + try { + return await fn(); + } finally { + if (original === undefined) delete process.env.GITHITS_AUTH_STORAGE; + else process.env.GITHITS_AUTH_STORAGE = original; + } +} + +async function withApiToken( + value: string | undefined, + fn: () => Promise, +): Promise { + const original = process.env.GITHITS_API_TOKEN; + if (value === undefined) delete process.env.GITHITS_API_TOKEN; + else process.env.GITHITS_API_TOKEN = value; + try { + return await fn(); + } finally { + if (original === undefined) delete process.env.GITHITS_API_TOKEN; + else process.env.GITHITS_API_TOKEN = original; + } +} + +describe("container auth dependencies", () => { + it("login/logout auth dependencies still honor auth storage config with env token set", async () => { + await withApiToken("ghi-test", async () => { + await withAuthStorageEnv("file", async () => { + const deps = await createAuthCommandDependencies(); + expect(deps.envApiToken).toBe("ghi-test"); + expect(deps.authStorage.getStorageLocation()).toContain("githits/auth"); + }); + }); + }); + + it("auth status env-token path bypasses invalid auth storage config", async () => { + await withApiToken("ghi-test", async () => { + await withAuthStorageEnv("invalid", async () => { + const deps = await createAuthStatusDependencies(); + expect(deps.envApiToken).toBe("ghi-test"); + }); + }); + }); + + it("auth command dependencies reject invalid auth storage config without env token", async () => { + await withApiToken(undefined, async () => { + await withAuthStorageEnv("invalid", async () => { + await expect(createAuthCommandDependencies()).rejects.toThrow( + AuthConfigError, + ); + }); + }); + }); +}); diff --git a/src/container.ts b/src/container.ts index 28e39754..639e3937 100644 --- a/src/container.ts +++ b/src/container.ts @@ -4,6 +4,7 @@ import { AuthServiceImpl, type AuthStorage, AuthStorageImpl, + type AuthStorageMode, type BrowserService, BrowserServiceImpl, ChunkingKeyringService, @@ -13,13 +14,16 @@ import { FileSystemServiceImpl, GitHitsServiceImpl, getApiUrl, + getAuthFileStorageDir, getCodeNavigationUrl, getEnvApiToken, + getLegacyAuthStorageDir, getMcpUrl, KeychainAuthStorage, - KeychainUnavailableError, KeyringServiceImpl, + loadAuthConfig, MigratingAuthStorage, + ModeAwareFileAuthStorage, type PackageIntelligenceService, PackageIntelligenceServiceImpl, RefreshingGitHitsService, @@ -27,33 +31,102 @@ import { type TokenProvider, WINDOWS_MAX_ENTRY_SIZE, } from "./services/index.js"; -import { - withTelemetrySpan, - withTelemetrySpanSync, -} from "./shared/telemetry.js"; +import { withTelemetrySpan } from "./shared/telemetry.js"; /** - * Create an AuthStorage instance, preferring keychain with file-based fallback. - * Falls back to file storage only if a real keychain operation fails. + * Create an AuthStorage instance using the configured auth storage mode. + * Keychain mode never silently downgrades writes to plaintext files. */ -function createAuthStorage(fileSystemService: FileSystemService): AuthStorage { - return withTelemetrySpanSync("container.create-auth-storage", () => { - const fileStorage = new AuthStorageImpl(fileSystemService); +async function createAuthStorage( + fileSystemService: FileSystemService, +): Promise { + return withTelemetrySpan("container.create-auth-storage", async () => { + const authConfig = await loadAuthConfig(fileSystemService); + return createAuthStorageForMode( + fileSystemService, + authConfig.storage, + authConfig.configPath, + ); + }); +} - const rawKeyring = new KeyringServiceImpl(); - // Windows Credential Manager limits entries to 2560 UTF-16 chars. - // Wrap with chunking decorator to split large values across multiple entries. - const keyring = - process.platform === "win32" - ? new ChunkingKeyringService(rawKeyring, WINDOWS_MAX_ENTRY_SIZE) - : rawKeyring; - const keychainStorage = new KeychainAuthStorage(keyring); - return new MigratingAuthStorage(keychainStorage, fileStorage, (error) => { - if (!(error instanceof KeychainUnavailableError)) return; - console.error( - "Warning: System keychain unavailable. Falling back to file-based credential storage.", - ); - }); +function createAuthStorageForMode( + fileSystemService: FileSystemService, + mode: AuthStorageMode, + configPath = "your GitHits config.toml", +): AuthStorage { + const fileStorage = new ModeAwareFileAuthStorage( + new AuthStorageImpl( + fileSystemService, + getAuthFileStorageDir(fileSystemService), + ), + mode, + configPath, + ); + const legacyStorage = new AuthStorageImpl( + fileSystemService, + getLegacyAuthStorageDir(fileSystemService), + ); + + const rawKeyring = new KeyringServiceImpl(); + // Windows Credential Manager limits entries to 2560 UTF-16 chars. + // Wrap with chunking decorator to split large values across multiple entries. + const keyring = + process.platform === "win32" + ? new ChunkingKeyringService(rawKeyring, WINDOWS_MAX_ENTRY_SIZE) + : rawKeyring; + const keychainStorage = new KeychainAuthStorage(keyring); + + return new MigratingAuthStorage( + keychainStorage, + fileStorage, + legacyStorage, + mode, + configPath, + (message) => console.error(message), + ); +} + +export interface AuthCommandDependencies { + authStorage: AuthStorage; + authService: AuthService; + browserService: BrowserService; + fileSystemService: FileSystemService; + mcpUrl: string; + apiUrl: string; + envApiToken: string | undefined; +} + +export async function createAuthCommandDependencies(): Promise { + return withTelemetrySpan("container.create-auth-command", async () => { + const fileSystemService = new FileSystemServiceImpl(); + return { + authStorage: await createAuthStorage(fileSystemService), + authService: new AuthServiceImpl(), + browserService: new BrowserServiceImpl(), + fileSystemService, + mcpUrl: getMcpUrl(), + apiUrl: getApiUrl(), + envApiToken: getEnvApiToken(), + }; + }); +} + +export async function createAuthStatusDependencies(): Promise { + return withTelemetrySpan("container.create-auth-status", async () => { + const fileSystemService = new FileSystemServiceImpl(); + const envApiToken = getEnvApiToken(); + return { + authStorage: envApiToken + ? createAuthStorageForMode(fileSystemService, "keychain") + : await createAuthStorage(fileSystemService), + authService: new AuthServiceImpl(), + browserService: new BrowserServiceImpl(), + fileSystemService, + mcpUrl: getMcpUrl(), + apiUrl: getApiUrl(), + envApiToken, + }; }); } @@ -104,13 +177,16 @@ export async function createContainer(): Promise { const apiUrl = getApiUrl(); const codeNavigationUrl = getCodeNavigationUrl(); const fileSystemService = new FileSystemServiceImpl(); - const authStorage = createAuthStorage(fileSystemService); const authService = new AuthServiceImpl(); const browserService = new BrowserServiceImpl(); // Check for env API token first const envToken = getEnvApiToken(); if (envToken) { + const authStorage = createAuthStorageForMode( + fileSystemService, + "keychain", + ); const tokenProvider = createStaticTokenProvider(envToken); const codeNavigationService = new CodeNavigationServiceImpl( codeNavigationUrl, @@ -139,6 +215,7 @@ export async function createContainer(): Promise { } // Create token manager for stored auth with auto-refresh + const authStorage = await createAuthStorage(fileSystemService); const tokenManager = new TokenManager({ authService, authStorage, mcpUrl }); const apiToken = await withTelemetrySpan("container.token.get", () => tokenManager.getToken(), diff --git a/src/services/app-config-paths.test.ts b/src/services/app-config-paths.test.ts new file mode 100644 index 00000000..14f8fb7f --- /dev/null +++ b/src/services/app-config-paths.test.ts @@ -0,0 +1,79 @@ +import { describe, expect, it } from "bun:test"; +import { + getAppConfigDir, + getAuthConfigPath, + getAuthFileStorageDir, + getLegacyAuthStorageDir, +} from "./app-config-paths.js"; +import { createMockFileSystemService } from "./test-helpers.js"; + +function withPlatform(platform: NodeJS.Platform, fn: () => T): T { + const originalPlatform = process.platform; + Object.defineProperty(process, "platform", { value: platform }); + try { + return fn(); + } finally { + Object.defineProperty(process, "platform", { value: originalPlatform }); + } +} + +describe("app config paths", () => { + it("uses XDG_CONFIG_HOME on linux", () => { + const original = process.env.XDG_CONFIG_HOME; + process.env.XDG_CONFIG_HOME = "/xdg/config"; + try { + withPlatform("linux", () => { + const fs = createMockFileSystemService(); + expect(getAppConfigDir(fs)).toBe("/xdg/config/githits"); + expect(getAuthConfigPath(fs)).toBe("/xdg/config/githits/config.toml"); + expect(getAuthFileStorageDir(fs)).toBe("/xdg/config/githits/auth"); + }); + } finally { + if (original === undefined) delete process.env.XDG_CONFIG_HOME; + else process.env.XDG_CONFIG_HOME = original; + } + }); + + it("falls back to ~/.config on linux", () => { + const original = process.env.XDG_CONFIG_HOME; + delete process.env.XDG_CONFIG_HOME; + try { + withPlatform("linux", () => { + expect(getAppConfigDir(createMockFileSystemService())).toBe( + "/home/test/.config/githits", + ); + }); + } finally { + if (original !== undefined) process.env.XDG_CONFIG_HOME = original; + } + }); + + it("uses Application Support on macOS", () => { + withPlatform("darwin", () => { + expect(getAppConfigDir(createMockFileSystemService())).toBe( + "/home/test/Library/Application Support/githits", + ); + }); + }); + + it("uses APPDATA on Windows", () => { + const original = process.env.APPDATA; + process.env.APPDATA = "C:\\Users\\test\\AppData\\Roaming"; + try { + withPlatform("win32", () => { + expect(getAppConfigDir(createMockFileSystemService())).toBe( + "C:\\Users\\test\\AppData\\Roaming/githits", + ); + }); + } finally { + if (original === undefined) delete process.env.APPDATA; + else process.env.APPDATA = original; + } + }); + + it("keeps legacy auth storage under ~/.githits", () => { + expect(getLegacyAuthStorageDir(createMockFileSystemService())).toBe( + "/home/test/.githits", + ); + }); +}); diff --git a/src/services/app-config-paths.ts b/src/services/app-config-paths.ts new file mode 100644 index 00000000..ba794f67 --- /dev/null +++ b/src/services/app-config-paths.ts @@ -0,0 +1,39 @@ +import type { FileSystemService } from "./filesystem-service.js"; + +const APP_DIR = "githits"; + +/** + * Resolve GitHits' platform-specific config directory. + * + * This is intentionally shared by auth config and auth file storage so local + * state does not drift across multiple hidden directories. + */ +export function getAppConfigDir(fs: FileSystemService): string { + const home = fs.getHomeDir(); + switch (process.platform) { + case "win32": + return fs.joinPath( + process.env.APPDATA ?? fs.joinPath(home, "AppData", "Roaming"), + APP_DIR, + ); + case "darwin": + return fs.joinPath(home, "Library", "Application Support", APP_DIR); + default: + return fs.joinPath( + process.env.XDG_CONFIG_HOME ?? fs.joinPath(home, ".config"), + APP_DIR, + ); + } +} + +export function getAuthConfigPath(fs: FileSystemService): string { + return fs.joinPath(getAppConfigDir(fs), "config.toml"); +} + +export function getAuthFileStorageDir(fs: FileSystemService): string { + return fs.joinPath(getAppConfigDir(fs), "auth"); +} + +export function getLegacyAuthStorageDir(fs: FileSystemService): string { + return fs.joinPath(fs.getHomeDir(), ".githits"); +} diff --git a/src/services/auth-config.test.ts b/src/services/auth-config.test.ts new file mode 100644 index 00000000..418fb136 --- /dev/null +++ b/src/services/auth-config.test.ts @@ -0,0 +1,118 @@ +import { describe, expect, it, mock } from "bun:test"; +import { + AuthConfigError, + loadAuthConfig, + parseAuthStorageMode, +} from "./auth-config.js"; +import { createMockFileSystemService } from "./test-helpers.js"; + +async function withEnv( + value: string | undefined, + fn: () => Promise, +): Promise { + const original = process.env.GITHITS_AUTH_STORAGE; + if (value === undefined) delete process.env.GITHITS_AUTH_STORAGE; + else process.env.GITHITS_AUTH_STORAGE = value; + try { + return await fn(); + } finally { + if (original === undefined) delete process.env.GITHITS_AUTH_STORAGE; + else process.env.GITHITS_AUTH_STORAGE = original; + } +} + +describe("auth config", () => { + it("parses storage modes case-insensitively", () => { + expect(parseAuthStorageMode("keychain")).toBe("keychain"); + expect(parseAuthStorageMode(" KEYCHAIN ")).toBe("keychain"); + expect(parseAuthStorageMode("file")).toBe("file"); + }); + + it("rejects invalid storage mode", () => { + expect(() => parseAuthStorageMode("plaintext")).toThrow(AuthConfigError); + }); + + it("defaults to keychain when config file is missing", async () => { + await withEnv(undefined, async () => { + const config = await loadAuthConfig( + createMockFileSystemService({ + exists: mock(() => Promise.resolve(false)), + }), + ); + expect(config.storage).toBe("keychain"); + expect(config.configPath).toContain("githits/config.toml"); + }); + }); + + it("reads file mode from config.toml", async () => { + await withEnv(undefined, async () => { + const config = await loadAuthConfig( + createMockFileSystemService({ + exists: mock(() => Promise.resolve(true)), + readFile: mock(() => Promise.resolve('[auth]\nstorage = "file"\n')), + }), + ); + expect(config.storage).toBe("file"); + }); + }); + + it("uses env override before config file", async () => { + await withEnv("file", async () => { + const fs = createMockFileSystemService({ + exists: mock(() => Promise.resolve(true)), + readFile: mock(() => Promise.resolve('[auth]\nstorage = "keychain"\n')), + }); + const config = await loadAuthConfig(fs); + expect(config.storage).toBe("file"); + expect(fs.readFile).not.toHaveBeenCalled(); + }); + }); + + it("rejects invalid env override", async () => { + await withEnv("plaintext", async () => { + await expect( + loadAuthConfig(createMockFileSystemService()), + ).rejects.toThrow(/GITHITS_AUTH_STORAGE/); + }); + }); + + it("ignores blank env override and falls back to config", async () => { + await withEnv(" ", async () => { + const config = await loadAuthConfig( + createMockFileSystemService({ + exists: mock(() => Promise.resolve(true)), + readFile: mock(() => Promise.resolve('[auth]\nstorage = "file"\n')), + }), + ); + expect(config.storage).toBe("file"); + }); + }); + + it("rejects invalid TOML", async () => { + await withEnv(undefined, async () => { + await expect( + loadAuthConfig( + createMockFileSystemService({ + exists: mock(() => Promise.resolve(true)), + readFile: mock(() => Promise.resolve("[auth\n")), + }), + ), + ).rejects.toThrow(/Cannot parse GitHits config/); + }); + }); + + it("rejects invalid auth.storage in config", async () => { + await withEnv(undefined, async () => { + await expect( + loadAuthConfig( + createMockFileSystemService({ + exists: mock(() => Promise.resolve(true)), + readFile: mock(() => + Promise.resolve('[auth]\nstorage = "plaintext"\n'), + ), + }), + ), + ).rejects.toThrow(/Invalid GitHits config/); + }); + }); +}); diff --git a/src/services/auth-config.ts b/src/services/auth-config.ts new file mode 100644 index 00000000..4ed1f679 --- /dev/null +++ b/src/services/auth-config.ts @@ -0,0 +1,96 @@ +import { parse as parseToml } from "smol-toml"; +import { z } from "zod"; +import { getAuthConfigPath } from "./app-config-paths.js"; +import type { FileSystemService } from "./filesystem-service.js"; + +export const AUTH_STORAGE_MODES = ["keychain", "file"] as const; +export type AuthStorageMode = (typeof AUTH_STORAGE_MODES)[number]; + +const AUTH_STORAGE_MODE_VALUES = new Set(AUTH_STORAGE_MODES); +const CONFIG_SCHEMA = z + .object({ + auth: z + .object({ + storage: z.string().optional(), + }) + .optional(), + }) + .passthrough(); + +export class AuthConfigError extends Error { + constructor(message: string) { + super(message); + this.name = "AuthConfigError"; + } +} + +export interface AuthConfig { + storage: AuthStorageMode; + configPath: string; +} + +export function parseAuthStorageMode(value: string): AuthStorageMode { + const normalized = value.trim().toLowerCase(); + if (AUTH_STORAGE_MODE_VALUES.has(normalized)) { + return normalized as AuthStorageMode; + } + throw new AuthConfigError( + `Invalid auth storage mode "${value}". Use "keychain" or "file". File mode stores OAuth credentials unencrypted on disk.`, + ); +} + +export async function loadAuthConfig( + fs: FileSystemService, +): Promise { + const configPath = getAuthConfigPath(fs); + const envMode = process.env.GITHITS_AUTH_STORAGE; + if (envMode !== undefined && envMode.trim() !== "") { + try { + return { storage: parseAuthStorageMode(envMode), configPath }; + } catch (error) { + if (error instanceof AuthConfigError) { + throw new AuthConfigError( + `Invalid GITHITS_AUTH_STORAGE: ${error.message}`, + ); + } + throw error; + } + } + + if (!(await fs.exists(configPath))) { + return { storage: "keychain", configPath }; + } + + let rawConfig: unknown; + try { + rawConfig = parseToml(await fs.readFile(configPath)); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new AuthConfigError( + `Cannot parse GitHits config at ${configPath}: ${message}`, + ); + } + + const parsed = CONFIG_SCHEMA.safeParse(rawConfig); + if (!parsed.success) { + throw new AuthConfigError( + `Invalid GitHits config at ${configPath}: ${z.prettifyError(parsed.error)}`, + ); + } + + const configuredMode = parsed.data.auth?.storage; + if (configuredMode === undefined || configuredMode.trim() === "") { + return { storage: "keychain", configPath }; + } + + try { + return { storage: parseAuthStorageMode(configuredMode), configPath }; + } catch (error) { + if (error instanceof AuthConfigError) { + throw new AuthConfigError( + `Invalid GitHits config at ${configPath}: ${error.message}`, + ); + } + throw error; + } +} diff --git a/src/services/auth-storage.test.ts b/src/services/auth-storage.test.ts index 86af4f72..4eb298b1 100644 --- a/src/services/auth-storage.test.ts +++ b/src/services/auth-storage.test.ts @@ -6,6 +6,12 @@ describe("AuthStorageImpl", () => { const BASE_URL = "https://mcp.githits.com"; describe("loadTokens", () => { + it("defaults to the platform config auth directory", () => { + const storage = new AuthStorageImpl(createMockFileSystemService()); + + expect(storage.getStorageLocation()).toContain("githits/auth"); + }); + it("returns null when auth file does not exist", async () => { const fs = createMockFileSystemService({ exists: mock(() => Promise.resolve(false)), @@ -121,14 +127,13 @@ describe("AuthStorageImpl", () => { }); expect(fs.ensureDir).toHaveBeenCalledWith("/test/.githits", 0o700); - expect(fs.writeFile).toHaveBeenCalledWith( + expect(fs.atomicWriteFile).toHaveBeenCalledWith( "/test/.githits/auth.json", expect.any(String), - 0o600, ); // Verify the written content - const calls = (fs.writeFile as ReturnType).mock.calls; + const calls = (fs.atomicWriteFile as ReturnType).mock.calls; const writtenContent = JSON.parse(calls[0]?.[1] as string); expect(writtenContent.version).toBe(1); expect(writtenContent.tokens[BASE_URL].accessToken).toBe("eyJ-new"); @@ -159,7 +164,7 @@ describe("AuthStorageImpl", () => { createdAt: "2025-01-15T10:00:00Z", }); - const calls = (fs.writeFile as ReturnType).mock.calls; + const calls = (fs.atomicWriteFile as ReturnType).mock.calls; const writtenContent = JSON.parse(calls[0]?.[1] as string); expect(Object.keys(writtenContent.tokens)).toHaveLength(2); expect(writtenContent.tokens[BASE_URL].accessToken).toBe("eyJ-new"); @@ -219,7 +224,7 @@ describe("AuthStorageImpl", () => { await storage.clearTokens(BASE_URL); expect(fs.deleteFile).not.toHaveBeenCalled(); - const calls = (fs.writeFile as ReturnType).mock.calls; + const calls = (fs.atomicWriteFile as ReturnType).mock.calls; const writtenContent = JSON.parse(calls[0]?.[1] as string); expect(Object.keys(writtenContent.tokens)).toHaveLength(1); expect(writtenContent.tokens[BASE_URL]).toBeUndefined(); @@ -233,7 +238,7 @@ describe("AuthStorageImpl", () => { await storage.clearTokens(BASE_URL); expect(fs.deleteFile).not.toHaveBeenCalled(); - expect(fs.writeFile).not.toHaveBeenCalled(); + expect(fs.atomicWriteFile).not.toHaveBeenCalled(); }); }); @@ -288,10 +293,9 @@ describe("AuthStorageImpl", () => { }); expect(fs.ensureDir).toHaveBeenCalledWith("/test/.githits", 0o700); - expect(fs.writeFile).toHaveBeenCalledWith( + expect(fs.atomicWriteFile).toHaveBeenCalledWith( "/test/.githits/client.json", expect.any(String), - 0o600, ); }); }); @@ -346,7 +350,7 @@ describe("AuthStorageImpl", () => { await storage.clearClient(BASE_URL); expect(fs.deleteFile).not.toHaveBeenCalled(); - const calls = (fs.writeFile as ReturnType).mock.calls; + const calls = (fs.atomicWriteFile as ReturnType).mock.calls; const writtenContent = JSON.parse(calls[0]?.[1] as string); expect(Object.keys(writtenContent.clients)).toHaveLength(1); expect(writtenContent.clients[BASE_URL]).toBeUndefined(); @@ -360,7 +364,7 @@ describe("AuthStorageImpl", () => { await storage.clearClient(BASE_URL); expect(fs.deleteFile).not.toHaveBeenCalled(); - expect(fs.writeFile).not.toHaveBeenCalled(); + expect(fs.atomicWriteFile).not.toHaveBeenCalled(); }); }); @@ -371,13 +375,13 @@ describe("AuthStorageImpl", () => { expect(storage.getStorageLocation()).toBe("/test/.githits"); }); - it("defaults to ~/.githits", () => { + it("defaults to the platform config auth path", () => { const fs = createMockFileSystemService({ getHomeDir: mock(() => "/home/user"), joinPath: mock((...segments: string[]) => segments.join("/")), }); const storage = new AuthStorageImpl(fs); - expect(storage.getStorageLocation()).toBe("/home/user/.githits"); + expect(storage.getStorageLocation()).toContain("/githits/auth"); }); }); }); diff --git a/src/services/auth-storage.ts b/src/services/auth-storage.ts index 098125a0..4acb78d4 100644 --- a/src/services/auth-storage.ts +++ b/src/services/auth-storage.ts @@ -1,3 +1,4 @@ +import { getAuthFileStorageDir } from "./app-config-paths.js"; import type { FileSystemService } from "./filesystem-service.js"; /** @@ -65,15 +66,13 @@ export interface AuthStorage { getStorageLocation(): string; } -const CONFIG_DIR = ".githits"; const AUTH_FILE = "auth.json"; const CLIENT_FILE = "client.json"; const DIR_MODE = 0o700; -const FILE_MODE = 0o600; /** * File-based auth storage implementation. - * Stores auth in ~/.githits/ with secure permissions. + * Stores auth under the platform config directory with secure permissions. */ export class AuthStorageImpl implements AuthStorage { private readonly configDir: string; @@ -84,7 +83,7 @@ export class AuthStorageImpl implements AuthStorage { private readonly fs: FileSystemService, configDir?: string, ) { - this.configDir = configDir ?? fs.joinPath(fs.getHomeDir(), CONFIG_DIR); + this.configDir = configDir ?? getAuthFileStorageDir(fs); this.authPath = fs.joinPath(this.configDir, AUTH_FILE); this.clientPath = fs.joinPath(this.configDir, CLIENT_FILE); } @@ -107,10 +106,9 @@ export class AuthStorageImpl implements AuthStorage { stored.tokens[normalizeBaseUrl(baseUrl)] = data; await this.fs.ensureDir(this.configDir, DIR_MODE); - await this.fs.writeFile( + await this.fs.atomicWriteFile( this.authPath, JSON.stringify(stored, null, 2), - FILE_MODE, ); } @@ -123,10 +121,9 @@ export class AuthStorageImpl implements AuthStorage { if (Object.keys(stored.tokens).length === 0) { await this.fs.deleteFile(this.authPath); } else { - await this.fs.writeFile( + await this.fs.atomicWriteFile( this.authPath, JSON.stringify(stored, null, 2), - FILE_MODE, ); } } @@ -146,10 +143,9 @@ export class AuthStorageImpl implements AuthStorage { if (Object.keys(stored.clients).length === 0) { await this.fs.deleteFile(this.clientPath); } else { - await this.fs.writeFile( + await this.fs.atomicWriteFile( this.clientPath, JSON.stringify(stored, null, 2), - FILE_MODE, ); } } @@ -162,10 +158,9 @@ export class AuthStorageImpl implements AuthStorage { stored.clients[normalizeBaseUrl(baseUrl)] = data; await this.fs.ensureDir(this.configDir, DIR_MODE); - await this.fs.writeFile( + await this.fs.atomicWriteFile( this.clientPath, JSON.stringify(stored, null, 2), - FILE_MODE, ); } diff --git a/src/services/filesystem-service.ts b/src/services/filesystem-service.ts index 6cba98aa..c76bd5e8 100644 --- a/src/services/filesystem-service.ts +++ b/src/services/filesystem-service.ts @@ -1,3 +1,4 @@ +import { randomUUID } from "node:crypto"; import { mkdir, readdir, @@ -127,7 +128,7 @@ export class FileSystemServiceImpl implements FileSystemService { } async atomicWriteFile(path: string, contents: string): Promise { - const tmpPath = `${path}.${process.pid}.${Date.now()}.tmp`; + const tmpPath = `${path}.${process.pid}.${randomUUID()}.tmp`; // Preserve existing file permissions; default to 0o600 for new files // (config files may contain sensitive data from other MCP servers) let mode = 0o600; diff --git a/src/services/index.ts b/src/services/index.ts index 96194732..c40e6cd2 100644 --- a/src/services/index.ts +++ b/src/services/index.ts @@ -1,3 +1,15 @@ +export { + getAppConfigDir, + getAuthConfigPath, + getAuthFileStorageDir, + getLegacyAuthStorageDir, +} from "./app-config-paths.js"; +export type { AuthConfig, AuthStorageMode } from "./auth-config.js"; +export { + AuthConfigError, + loadAuthConfig, + parseAuthStorageMode, +} from "./auth-config.js"; export type { AuthService, BuildAuthUrlParams, @@ -101,6 +113,10 @@ export { KeyringServiceImpl, } from "./keyring-service.js"; export { MigratingAuthStorage } from "./migrating-auth-storage.js"; +export { + AuthStoragePolicyError, + ModeAwareFileAuthStorage, +} from "./mode-aware-file-auth-storage.js"; export type { ChangelogEntry, ChangelogEntryDetail, diff --git a/src/services/migrating-auth-storage.test.ts b/src/services/migrating-auth-storage.test.ts index ff2848b1..a7bb09e1 100644 --- a/src/services/migrating-auth-storage.test.ts +++ b/src/services/migrating-auth-storage.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, mock } from "bun:test"; import { KeychainUnavailableError } from "./keyring-service.js"; import { MigratingAuthStorage } from "./migrating-auth-storage.js"; +import { AuthStoragePolicyError } from "./mode-aware-file-auth-storage.js"; import { createMockAuthStorage, createValidTokenData, @@ -10,368 +11,349 @@ import { describe("MigratingAuthStorage", () => { const BASE_URL = "https://mcp.githits.com"; - describe("loadTokens", () => { - it("returns from primary when found", async () => { - const tokenData = createValidTokenData(); - const primary = createMockAuthStorage({ - loadTokens: mock(() => Promise.resolve(tokenData)), - }); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadTokens(BASE_URL); - expect(result).toEqual(tokenData); - expect(legacy.loadTokens).not.toHaveBeenCalled(); + it("keychain mode returns from keychain first", async () => { + const token = createValidTokenData(); + const primary = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(token)), }); + const file = createMockAuthStorage(); + const legacy = createMockAuthStorage(); + const storage = new MigratingAuthStorage(primary, file, legacy, "keychain"); - it("migrates from legacy when primary returns null", async () => { - const tokenData = createValidTokenData(); - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage({ - loadTokens: mock(() => Promise.resolve(tokenData)), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadTokens(BASE_URL); + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(token); + expect(file.loadTokens).not.toHaveBeenCalled(); + expect(legacy.loadTokens).not.toHaveBeenCalled(); + }); - expect(result).toEqual(tokenData); - expect(primary.saveTokens).toHaveBeenCalledWith(BASE_URL, tokenData); - expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); + it("keychain mode migrates new file tokens into keychain", async () => { + const token = createValidTokenData(); + const primary = createMockAuthStorage(); + const file = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(token)), }); + const legacy = createMockAuthStorage(); + const storage = new MigratingAuthStorage(primary, file, legacy, "keychain"); - it("returns null when both return null", async () => { - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadTokens(BASE_URL); - expect(result).toBeNull(); - }); + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(token); + expect(primary.saveTokens).toHaveBeenCalledWith(BASE_URL, token); + expect(file.clearTokens).toHaveBeenCalledWith(BASE_URL); + expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); + }); - it("keeps legacy entry intact if primary write fails", async () => { - const tokenData = createValidTokenData(); - const primary = createMockAuthStorage({ - saveTokens: mock(() => - Promise.reject(new KeychainUnavailableError("Keychain write failed")), - ), - }); - const legacy = createMockAuthStorage({ - loadTokens: mock(() => Promise.resolve(tokenData)), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadTokens(BASE_URL); - - expect(result).toEqual(tokenData); - expect(legacy.clearTokens).not.toHaveBeenCalled(); + it("keychain mode migrates legacy tokens into keychain", async () => { + const token = createValidTokenData(); + const primary = createMockAuthStorage(); + const file = createMockAuthStorage(); + const legacy = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(token)), }); + const storage = new MigratingAuthStorage(primary, file, legacy, "keychain"); - it("succeeds when primary write succeeds but legacy clear fails", async () => { - const tokenData = createValidTokenData(); - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage({ - loadTokens: mock(() => Promise.resolve(tokenData)), - clearTokens: mock(() => Promise.reject(new Error("file locked"))), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadTokens(BASE_URL); - - expect(result).toEqual(tokenData); - expect(primary.saveTokens).toHaveBeenCalledWith(BASE_URL, tokenData); - // Legacy clear failed but was swallowed — migration still succeeded - expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); - }); + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(token); + expect(primary.saveTokens).toHaveBeenCalledWith(BASE_URL, token); + expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); }); - describe("saveTokens", () => { - it("writes only to primary", async () => { - const tokenData = createValidTokenData(); - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.saveTokens(BASE_URL, tokenData); - - expect(primary.saveTokens).toHaveBeenCalledWith(BASE_URL, tokenData); - expect(legacy.saveTokens).not.toHaveBeenCalled(); + it("keychain mode keeps plaintext entry if keychain migration write fails", async () => { + const token = createValidTokenData(); + const primary = createMockAuthStorage({ + saveTokens: mock(() => + Promise.reject(new KeychainUnavailableError("keychain locked")), + ), }); + const file = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(token)), + }); + const legacy = createMockAuthStorage(); + const storage = new MigratingAuthStorage(primary, file, legacy, "keychain"); - it("falls back to legacy when primary save fails with keychain unavailable", async () => { - const tokenData = createValidTokenData(); - const primary = createMockAuthStorage({ - saveTokens: mock(() => - Promise.reject(new KeychainUnavailableError("keychain locked")), - ), - }); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.saveTokens(BASE_URL, tokenData); + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(token); + expect(file.clearTokens).not.toHaveBeenCalled(); + }); - expect(legacy.saveTokens).toHaveBeenCalledWith(BASE_URL, tokenData); + it("keychain mode save fails instead of writing plaintext when keychain is unavailable", async () => { + const token = createValidTokenData(); + const primary = createMockAuthStorage({ + saveTokens: mock(() => + Promise.reject(new KeychainUnavailableError("keychain locked")), + ), }); + const file = createMockAuthStorage(); + const storage = new MigratingAuthStorage( + primary, + file, + createMockAuthStorage(), + "keychain", + "/home/test/.config/githits/config.toml", + ); + + await expect(storage.saveTokens(BASE_URL, token)).rejects.toThrow( + AuthStoragePolicyError, + ); + await expect(storage.saveTokens(BASE_URL, token)).rejects.toThrow( + /Warning: file storage is plaintext/, + ); + await expect(storage.saveTokens(BASE_URL, token)).rejects.toThrow( + /\[auth\]\n {5}storage = "file"/, + ); + await expect(storage.saveTokens(BASE_URL, token)).rejects.toThrow( + /\/home\/test\/\.config\/githits\/config\.toml/, + ); + expect(file.saveTokens).not.toHaveBeenCalled(); }); - describe("clearTokens", () => { - it("clears from both primary and legacy", async () => { - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.clearTokens(BASE_URL); + it("file mode writes only to new file storage", async () => { + const token = createValidTokenData(); + const primary = createMockAuthStorage(); + const file = createMockAuthStorage(); + const storage = new MigratingAuthStorage( + primary, + file, + createMockAuthStorage(), + "file", + ); + + await storage.saveTokens(BASE_URL, token); + expect(file.saveTokens).toHaveBeenCalledWith(BASE_URL, token); + expect(primary.saveTokens).not.toHaveBeenCalled(); + }); - expect(primary.clearTokens).toHaveBeenCalledWith(BASE_URL); - expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); + it("file mode migrates legacy tokens into new file storage", async () => { + const token = createValidTokenData(); + const file = createMockAuthStorage(); + const legacy = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(token)), }); + const storage = new MigratingAuthStorage( + createMockAuthStorage(), + file, + legacy, + "file", + ); + + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(token); + expect(file.saveTokens).toHaveBeenCalledWith(BASE_URL, token); + expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); + }); - it("attempts legacy clear even when primary throws", async () => { - const primary = createMockAuthStorage({ - clearTokens: mock(() => - Promise.reject(new KeychainUnavailableError("keychain locked")), - ), - }); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.clearTokens(BASE_URL); - expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); + it("file mode chooses newer legacy tokens when both plaintext stores exist", async () => { + const older = createValidTokenData({ createdAt: "2025-01-01T00:00:00Z" }); + const newer = createValidTokenData({ createdAt: "2025-02-01T00:00:00Z" }); + const file = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(older)), }); - - it("swallows keychain-unavailable primary clear errors even when legacy clear also throws", async () => { - const primaryError = new KeychainUnavailableError("keychain locked"); - const primary = createMockAuthStorage({ - clearTokens: mock(() => Promise.reject(primaryError)), - }); - const legacy = createMockAuthStorage({ - clearTokens: mock(() => Promise.reject(new Error("file locked"))), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.clearTokens(BASE_URL); + const legacy = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(newer)), }); + const storage = new MigratingAuthStorage( + createMockAuthStorage(), + file, + legacy, + "file", + ); + + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(newer); + expect(file.saveTokens).toHaveBeenCalledWith(BASE_URL, newer); + expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); }); - describe("loadClient", () => { - it("returns from primary when found", async () => { - const primary = createMockAuthStorage({ - loadClient: mock(() => Promise.resolve(defaultClientRegistration)), - }); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadClient(BASE_URL); - expect(result).toEqual(defaultClientRegistration); - expect(legacy.loadClient).not.toHaveBeenCalled(); + it("file mode keeps newer file tokens when legacy is older", async () => { + const newer = createValidTokenData({ createdAt: "2025-02-01T00:00:00Z" }); + const older = createValidTokenData({ createdAt: "2025-01-01T00:00:00Z" }); + const file = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(newer)), }); - - it("migrates from legacy when primary returns null", async () => { - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage({ - loadClient: mock(() => Promise.resolve(defaultClientRegistration)), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadClient(BASE_URL); - - expect(result).toEqual(defaultClientRegistration); - expect(primary.saveClient).toHaveBeenCalledWith( - BASE_URL, - defaultClientRegistration, - ); - expect(legacy.clearClient).toHaveBeenCalledWith(BASE_URL); + const legacy = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(older)), }); + const storage = new MigratingAuthStorage( + createMockAuthStorage(), + file, + legacy, + "file", + ); + + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(newer); + expect(file.saveTokens).not.toHaveBeenCalled(); + expect(legacy.clearTokens).not.toHaveBeenCalled(); + }); - it("returns null when both return null", async () => { - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadClient(BASE_URL); - expect(result).toBeNull(); + it("file mode uses keychain only as last-resort migration source", async () => { + const token = createValidTokenData(); + const primary = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(token)), }); + const file = createMockAuthStorage(); + const legacy = createMockAuthStorage(); + const warning = mock(() => {}); + const storage = new MigratingAuthStorage( + primary, + file, + legacy, + "file", + "test-config.toml", + warning, + ); + + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(token); + expect(file.saveTokens).toHaveBeenCalledWith(BASE_URL, token); + expect(warning).toHaveBeenCalledWith(expect.stringContaining("exporting")); + }); - it("keeps legacy entry intact if primary write fails", async () => { - const primary = createMockAuthStorage({ - saveClient: mock(() => - Promise.reject(new KeychainUnavailableError("Keychain write failed")), - ), - }); - const legacy = createMockAuthStorage({ - loadClient: mock(() => Promise.resolve(defaultClientRegistration)), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadClient(BASE_URL); - - expect(result).toEqual(defaultClientRegistration); - expect(legacy.clearClient).not.toHaveBeenCalled(); + it("chooses newer plaintext token and leaves ambiguous other entry intact", async () => { + const older = createValidTokenData({ createdAt: "2025-01-01T00:00:00Z" }); + const newer = createValidTokenData({ createdAt: "2025-02-01T00:00:00Z" }); + const primary = createMockAuthStorage(); + const file = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(older)), }); - - it("succeeds when primary write succeeds but legacy clear fails", async () => { - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage({ - loadClient: mock(() => Promise.resolve(defaultClientRegistration)), - clearClient: mock(() => Promise.reject(new Error("file locked"))), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - const result = await storage.loadClient(BASE_URL); - - expect(result).toEqual(defaultClientRegistration); - expect(primary.saveClient).toHaveBeenCalledWith( - BASE_URL, - defaultClientRegistration, - ); - expect(legacy.clearClient).toHaveBeenCalledWith(BASE_URL); + const legacy = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(newer)), }); - }); + const storage = new MigratingAuthStorage(primary, file, legacy, "keychain"); - describe("saveClient", () => { - it("writes only to primary", async () => { - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.saveClient(BASE_URL, defaultClientRegistration); + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(newer); + expect(primary.saveTokens).toHaveBeenCalledWith(BASE_URL, newer); + expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); + expect(file.clearTokens).toHaveBeenCalledWith(BASE_URL); + }); - expect(primary.saveClient).toHaveBeenCalledWith( - BASE_URL, - defaultClientRegistration, - ); - expect(legacy.saveClient).not.toHaveBeenCalled(); + it("prefers new file path and warns when plaintext timestamps are tied", async () => { + const token = createValidTokenData({ createdAt: "2025-01-01T00:00:00Z" }); + const primary = createMockAuthStorage(); + const file = createMockAuthStorage({ + loadTokens: mock(() => Promise.resolve(token)), }); - - it("falls back to legacy when primary client save fails with keychain unavailable", async () => { - const primary = createMockAuthStorage({ - saveClient: mock(() => - Promise.reject(new KeychainUnavailableError("keychain locked")), - ), - }); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.saveClient(BASE_URL, defaultClientRegistration); - - expect(legacy.saveClient).toHaveBeenCalledWith( - BASE_URL, - defaultClientRegistration, - ); + const legacy = createMockAuthStorage({ + loadTokens: mock(() => + Promise.resolve(createValidTokenData({ createdAt: token.createdAt })), + ), }); + const warning = mock(() => {}); + const storage = new MigratingAuthStorage( + primary, + file, + legacy, + "keychain", + "test-config.toml", + warning, + ); + + await expect(storage.loadTokens(BASE_URL)).resolves.toEqual(token); + expect(file.clearTokens).toHaveBeenCalledWith(BASE_URL); + expect(legacy.clearTokens).not.toHaveBeenCalled(); + expect(warning).toHaveBeenCalledWith(expect.stringContaining("ambiguous")); }); - describe("clearClient", () => { - it("clears from both primary and legacy", async () => { - const primary = createMockAuthStorage(); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); + it("keychain mode clears both plaintext clients after unambiguous migration", async () => { + const older = { + ...defaultClientRegistration, + registeredAt: "2025-01-01T00:00:00Z", + }; + const newer = { + ...defaultClientRegistration, + clientId: "newer-client", + registeredAt: "2025-02-01T00:00:00Z", + }; + const primary = createMockAuthStorage(); + const file = createMockAuthStorage({ + loadClient: mock(() => Promise.resolve(older)), + }); + const legacy = createMockAuthStorage({ + loadClient: mock(() => Promise.resolve(newer)), + }); + const storage = new MigratingAuthStorage(primary, file, legacy, "keychain"); - await storage.clearClient(BASE_URL); + await expect(storage.loadClient(BASE_URL)).resolves.toEqual(newer); + expect(primary.saveClient).toHaveBeenCalledWith(BASE_URL, newer); + expect(file.clearClient).toHaveBeenCalledWith(BASE_URL); + expect(legacy.clearClient).toHaveBeenCalledWith(BASE_URL); + }); - expect(primary.clearClient).toHaveBeenCalledWith(BASE_URL); - expect(legacy.clearClient).toHaveBeenCalledWith(BASE_URL); + it("migrates clients according to configured mode", async () => { + const primary = createMockAuthStorage(); + const file = createMockAuthStorage(); + const legacy = createMockAuthStorage({ + loadClient: mock(() => Promise.resolve(defaultClientRegistration)), }); + const storage = new MigratingAuthStorage(primary, file, legacy, "file"); + + await expect(storage.loadClient(BASE_URL)).resolves.toEqual( + defaultClientRegistration, + ); + expect(file.saveClient).toHaveBeenCalledWith( + BASE_URL, + defaultClientRegistration, + ); + expect(legacy.clearClient).toHaveBeenCalledWith(BASE_URL); + }); - it("attempts legacy clear even when primary throws", async () => { - const primary = createMockAuthStorage({ - clearClient: mock(() => - Promise.reject(new KeychainUnavailableError("keychain locked")), - ), - }); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.clearClient(BASE_URL); - expect(legacy.clearClient).toHaveBeenCalledWith(BASE_URL); + it("file mode chooses newer legacy client when both plaintext stores exist", async () => { + const older = { + ...defaultClientRegistration, + registeredAt: "2025-01-01T00:00:00Z", + }; + const newer = { + ...defaultClientRegistration, + clientId: "newer-client", + registeredAt: "2025-02-01T00:00:00Z", + }; + const file = createMockAuthStorage({ + loadClient: mock(() => Promise.resolve(older)), }); - - it("swallows keychain-unavailable primary client clear errors even when legacy clear also throws", async () => { - const primaryError = new KeychainUnavailableError("keychain locked"); - const primary = createMockAuthStorage({ - clearClient: mock(() => Promise.reject(primaryError)), - }); - const legacy = createMockAuthStorage({ - clearClient: mock(() => Promise.reject(new Error("file locked"))), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.clearClient(BASE_URL); + const legacy = createMockAuthStorage({ + loadClient: mock(() => Promise.resolve(newer)), }); + const storage = new MigratingAuthStorage( + createMockAuthStorage(), + file, + legacy, + "file", + ); + + await expect(storage.loadClient(BASE_URL)).resolves.toEqual(newer); + expect(file.saveClient).toHaveBeenCalledWith(BASE_URL, newer); + expect(legacy.clearClient).toHaveBeenCalledWith(BASE_URL); }); - describe("migration independence", () => { - it("client migration succeeds independently when token migration fails", async () => { - const tokenData = createValidTokenData(); - const primary = createMockAuthStorage({ - saveTokens: mock(() => - Promise.reject(new KeychainUnavailableError("fail")), - ), - }); - const legacy = createMockAuthStorage({ - loadTokens: mock(() => Promise.resolve(tokenData)), - loadClient: mock(() => Promise.resolve(defaultClientRegistration)), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - const tokenResult = await storage.loadTokens(BASE_URL); - expect(tokenResult).toEqual(tokenData); - - // Client migration succeeds independently - const client = await storage.loadClient(BASE_URL); - expect(client).toEqual(defaultClientRegistration); - expect(primary.saveClient).not.toHaveBeenCalled(); + it("clears keychain, file, and legacy stores best-effort", async () => { + const primary = createMockAuthStorage({ + clearTokens: mock(() => + Promise.reject(new KeychainUnavailableError("keychain locked")), + ), }); - }); + const file = createMockAuthStorage(); + const legacy = createMockAuthStorage(); + const storage = new MigratingAuthStorage(primary, file, legacy, "keychain"); - describe("getStorageLocation", () => { - it("returns primary storage location", () => { - const primary = createMockAuthStorage({ - getStorageLocation: mock(() => "System keychain (githits)"), - }); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage(primary, legacy); + await storage.clearTokens(BASE_URL); + expect(file.clearTokens).toHaveBeenCalledWith(BASE_URL); + expect(legacy.clearTokens).toHaveBeenCalledWith(BASE_URL); + }); - expect(storage.getStorageLocation()).toBe("System keychain (githits)"); + it("reports active storage location", () => { + const primary = createMockAuthStorage({ + getStorageLocation: mock(() => "System keychain (githits)"), }); - - it("switches to legacy storage location after primary keychain failure", async () => { - const primary = createMockAuthStorage({ - loadTokens: mock(() => - Promise.reject(new KeychainUnavailableError("keychain locked")), - ), - getStorageLocation: mock(() => "System keychain (githits)"), - }); - const legacy = createMockAuthStorage({ - getStorageLocation: mock(() => "/mock/.githits"), - }); - const storage = new MigratingAuthStorage(primary, legacy); - - await storage.loadTokens(BASE_URL); - - expect(storage.getStorageLocation()).toBe("/mock/.githits"); + const file = createMockAuthStorage({ + getStorageLocation: mock(() => "/home/test/.config/githits/auth"), }); - it("warns only once when the primary keychain becomes unavailable", async () => { - const onPrimaryUnavailable = mock(() => {}); - const primary = createMockAuthStorage({ - loadTokens: mock(() => - Promise.reject(new KeychainUnavailableError("keychain locked")), - ), - loadClient: mock(() => - Promise.reject(new KeychainUnavailableError("keychain locked")), - ), - }); - const legacy = createMockAuthStorage(); - const storage = new MigratingAuthStorage( + expect( + new MigratingAuthStorage( primary, - legacy, - onPrimaryUnavailable, - ); - - await storage.loadTokens(BASE_URL); - await storage.loadClient(BASE_URL); - - expect(onPrimaryUnavailable).toHaveBeenCalledTimes(1); - }); + file, + createMockAuthStorage(), + "keychain", + ).getStorageLocation(), + ).toBe("System keychain (githits)"); + expect( + new MigratingAuthStorage( + primary, + file, + createMockAuthStorage(), + "file", + ).getStorageLocation(), + ).toBe("/home/test/.config/githits/auth"); }); }); diff --git a/src/services/migrating-auth-storage.ts b/src/services/migrating-auth-storage.ts index 0dec72c7..edbfd335 100644 --- a/src/services/migrating-auth-storage.ts +++ b/src/services/migrating-auth-storage.ts @@ -1,177 +1,359 @@ +import type { AuthStorageMode } from "./auth-config.js"; import type { AuthStorage, ClientRegistration, TokenData, } from "./auth-storage.js"; import { KeychainUnavailableError } from "./keyring-service.js"; +import { + AuthStoragePolicyError, + createFileAuthStorageGuidance, +} from "./mode-aware-file-auth-storage.js"; + +type CredentialKind = "tokens" | "client"; + +interface Candidate { + data: T; + source: "file" | "legacy" | "keychain"; + timestamp: string; + ambiguous: boolean; +} /** - * AuthStorage decorator that migrates credentials from a legacy (file-based) - * backend to a primary (keychain) backend transparently on first load per URL. - * - * Migration flow per credential: - * 1. Check primary → if found, return it - * 2. Check legacy → if found, write to primary → delete from legacy → return it - * 3. Both empty → return null - * - * If the primary keychain becomes unavailable at runtime, the storage falls back - * to the legacy backend for the lifetime of the process and stops attempting - * migrations. + * AuthStorage implementation that coordinates the configured active store with + * legacy plaintext locations so credentials migrate without silent downgrades. */ export class MigratingAuthStorage implements AuthStorage { - private primaryAvailable = true; - private warnedOnPrimaryFailure = false; + private warnedFileModeKeychainExport = false; + private warnedAmbiguousPlaintext = false; constructor( private readonly primary: AuthStorage, + private readonly file: AuthStorage, private readonly legacy: AuthStorage, - private readonly onPrimaryUnavailable: ( - error: KeychainUnavailableError, - ) => void = () => {}, + private readonly mode: AuthStorageMode, + private readonly configPath = "your GitHits config.toml", + private readonly onWarning: (message: string) => void = () => {}, ) {} async loadTokens(baseUrl: string): Promise { - if (this.primaryAvailable) { - try { - const tokens = await this.primary.loadTokens(baseUrl); - if (tokens) return tokens; - } catch (error) { - if (!this.handlePrimaryFailure(error)) throw error; - } + if (this.mode === "file") { + return this.loadTokensFileMode(baseUrl); } + return this.loadTokensKeychainMode(baseUrl); + } - const legacyTokens = await this.legacy.loadTokens(baseUrl); - if (!legacyTokens) return null; - if (!this.primaryAvailable) return legacyTokens; - + async saveTokens(baseUrl: string, data: TokenData): Promise { + if (this.mode === "file") { + await this.file.saveTokens(baseUrl, data); + return; + } try { - await this.primary.saveTokens(baseUrl, legacyTokens); + await this.primary.saveTokens(baseUrl, data); } catch (error) { - if (!this.handlePrimaryFailure(error)) throw error; - return legacyTokens; + throw this.toPolicyError(error); } + } + async clearTokens(baseUrl: string): Promise { + const primaryError = await this.clearBestEffort(() => + this.primary.clearTokens(baseUrl), + ); + await this.clearBestEffort(() => this.file.clearTokens(baseUrl)); + await this.clearBestEffort(() => this.legacy.clearTokens(baseUrl)); + if (primaryError && !(primaryError instanceof KeychainUnavailableError)) { + throw primaryError; + } + } + + async loadClient(baseUrl: string): Promise { + if (this.mode === "file") { + return this.loadClientFileMode(baseUrl); + } + return this.loadClientKeychainMode(baseUrl); + } + + async saveClient(baseUrl: string, data: ClientRegistration): Promise { + if (this.mode === "file") { + await this.file.saveClient(baseUrl, data); + return; + } try { - await this.legacy.clearTokens(baseUrl); - } catch { - // Non-fatal: legacy entry persists but primary is now authoritative. - // Next load will return from primary, skipping migration entirely. + await this.primary.saveClient(baseUrl, data); + } catch (error) { + throw this.toPolicyError(error); } - return legacyTokens; } - async saveTokens(baseUrl: string, data: TokenData): Promise { - if (this.primaryAvailable) { - try { - await this.primary.saveTokens(baseUrl, data); - return; - } catch (error) { - if (!this.handlePrimaryFailure(error)) throw error; - } + async clearClient(baseUrl: string): Promise { + const primaryError = await this.clearBestEffort(() => + this.primary.clearClient(baseUrl), + ); + await this.clearBestEffort(() => this.file.clearClient(baseUrl)); + await this.clearBestEffort(() => this.legacy.clearClient(baseUrl)); + if (primaryError && !(primaryError instanceof KeychainUnavailableError)) { + throw primaryError; } + } - await this.legacy.saveTokens(baseUrl, data); + getStorageLocation(): string { + return this.mode === "file" + ? this.file.getStorageLocation() + : this.primary.getStorageLocation(); } - async clearTokens(baseUrl: string): Promise { - let primaryError: unknown; - if (this.primaryAvailable) { - try { - await this.primary.clearTokens(baseUrl); - } catch (error) { - if (!this.handlePrimaryFailure(error)) { - primaryError = error; - } - } + private async loadTokensKeychainMode( + baseUrl: string, + ): Promise { + try { + const primaryTokens = await this.primary.loadTokens(baseUrl); + if (primaryTokens) return primaryTokens; + } catch (error) { + if (!(error instanceof KeychainUnavailableError)) throw error; } + const candidate = await this.selectPlaintextTokenCandidate(baseUrl); + if (!candidate) return null; + try { - await this.legacy.clearTokens(baseUrl); - } catch { - // Best-effort legacy cleanup + await this.primary.saveTokens(baseUrl, candidate.data); + } catch (error) { + if (error instanceof KeychainUnavailableError) return candidate.data; + throw error; } - if (primaryError) throw primaryError; + + await this.clearMigratedPlaintext( + baseUrl, + "tokens", + candidate.source, + candidate.ambiguous, + ); + return candidate.data; } - async loadClient(baseUrl: string): Promise { - if (this.primaryAvailable) { - try { - const client = await this.primary.loadClient(baseUrl); - if (client) return client; - } catch (error) { - if (!this.handlePrimaryFailure(error)) throw error; + private async loadTokensFileMode(baseUrl: string): Promise { + const candidate = await this.selectPlaintextTokenCandidate(baseUrl); + if (candidate) { + if (candidate.source === "legacy") { + await this.file.saveTokens(baseUrl, candidate.data); + await this.clearBestEffort(() => this.legacy.clearTokens(baseUrl)); } + return candidate.data; } - const legacyClient = await this.legacy.loadClient(baseUrl); - if (!legacyClient) return null; - if (!this.primaryAvailable) return legacyClient; + let primaryTokens: TokenData | null = null; + try { + primaryTokens = await this.primary.loadTokens(baseUrl); + } catch (error) { + if (!(error instanceof KeychainUnavailableError)) throw error; + } + if (!primaryTokens) return null; + + this.warnKeychainExport(); + await this.file.saveTokens(baseUrl, primaryTokens); + return primaryTokens; + } + private async loadClientKeychainMode( + baseUrl: string, + ): Promise { try { - await this.primary.saveClient(baseUrl, legacyClient); + const primaryClient = await this.primary.loadClient(baseUrl); + if (primaryClient) return primaryClient; } catch (error) { - if (!this.handlePrimaryFailure(error)) throw error; - return legacyClient; + if (!(error instanceof KeychainUnavailableError)) throw error; } + const candidate = await this.selectPlaintextClientCandidate(baseUrl); + if (!candidate) return null; + try { - await this.legacy.clearClient(baseUrl); - } catch { - // Non-fatal: legacy entry persists but primary is now authoritative. - // Next load will return from primary, skipping migration entirely. + await this.primary.saveClient(baseUrl, candidate.data); + } catch (error) { + if (error instanceof KeychainUnavailableError) return candidate.data; + throw error; } - return legacyClient; + + await this.clearMigratedPlaintext( + baseUrl, + "client", + candidate.source, + candidate.ambiguous, + ); + return candidate.data; } - async saveClient(baseUrl: string, data: ClientRegistration): Promise { - if (this.primaryAvailable) { - try { - await this.primary.saveClient(baseUrl, data); - return; - } catch (error) { - if (!this.handlePrimaryFailure(error)) throw error; + private async loadClientFileMode( + baseUrl: string, + ): Promise { + const candidate = await this.selectPlaintextClientCandidate(baseUrl); + if (candidate) { + if (candidate.source === "legacy") { + await this.file.saveClient(baseUrl, candidate.data); + await this.clearBestEffort(() => this.legacy.clearClient(baseUrl)); } + return candidate.data; } - await this.legacy.saveClient(baseUrl, data); + let primaryClient: ClientRegistration | null = null; + try { + primaryClient = await this.primary.loadClient(baseUrl); + } catch (error) { + if (!(error instanceof KeychainUnavailableError)) throw error; + } + if (!primaryClient) return null; + + this.warnKeychainExport(); + await this.file.saveClient(baseUrl, primaryClient); + return primaryClient; } - async clearClient(baseUrl: string): Promise { - let primaryError: unknown; - if (this.primaryAvailable) { - try { - await this.primary.clearClient(baseUrl); - } catch (error) { - if (!this.handlePrimaryFailure(error)) { - primaryError = error; - } - } + private async selectPlaintextTokenCandidate( + baseUrl: string, + ): Promise | null> { + const candidates: Array> = []; + const fileTokens = await this.file.loadTokens(baseUrl); + if (fileTokens) { + candidates.push({ + data: fileTokens, + source: "file", + timestamp: fileTokens.createdAt, + ambiguous: false, + }); + } + const legacyTokens = await this.legacy.loadTokens(baseUrl); + if (legacyTokens) { + candidates.push({ + data: legacyTokens, + source: "legacy", + timestamp: legacyTokens.createdAt, + ambiguous: false, + }); } + return this.selectNewestCandidate(candidates); + } - try { - await this.legacy.clearClient(baseUrl); - } catch { - // Best-effort legacy cleanup + private async selectPlaintextClientCandidate( + baseUrl: string, + ): Promise | null> { + const candidates: Array> = []; + const fileClient = await this.file.loadClient(baseUrl); + if (fileClient) { + candidates.push({ + data: fileClient, + source: "file", + timestamp: fileClient.registeredAt, + ambiguous: false, + }); + } + const legacyClient = await this.legacy.loadClient(baseUrl); + if (legacyClient) { + candidates.push({ + data: legacyClient, + source: "legacy", + timestamp: legacyClient.registeredAt, + ambiguous: false, + }); } - if (primaryError) throw primaryError; + return this.selectNewestCandidate(candidates); } - getStorageLocation(): string { - return this.primaryAvailable - ? this.primary.getStorageLocation() - : this.legacy.getStorageLocation(); + private selectNewestCandidate( + candidates: Array>, + ): Candidate | null { + if (candidates.length === 0) return null; + if (candidates.length === 1) return candidates[0] ?? null; + + const parsed = candidates.map((candidate) => ({ + candidate, + timestampMs: Date.parse(candidate.timestamp), + })); + if (parsed.some((entry) => Number.isNaN(entry.timestampMs))) { + this.warnAmbiguousPlaintext(); + const selected = + candidates.find((candidate) => candidate.source === "file") ?? + candidates[0] ?? + null; + if (selected) selected.ambiguous = true; + return selected; + } + + const sorted = [...parsed].sort((a, b) => b.timestampMs - a.timestampMs); + const first = sorted[0]; + const second = sorted[1]; + if (!first) return candidates[0] ?? null; + if (second && first.timestampMs === second.timestampMs) { + this.warnAmbiguousPlaintext(); + const selected = + candidates.find((candidate) => candidate.source === "file") ?? + first.candidate; + selected.ambiguous = true; + return selected; + } + return first.candidate; } - private handlePrimaryFailure(error: unknown): boolean { - if (!(error instanceof KeychainUnavailableError)) { - return false; + private async clearMigratedPlaintext( + baseUrl: string, + kind: CredentialKind, + source: "file" | "legacy" | "keychain", + ambiguous = false, + ): Promise { + if (!ambiguous) { + await this.clearPlaintextSource(this.file, baseUrl, kind); + await this.clearPlaintextSource(this.legacy, baseUrl, kind); + return; } + if (source === "file") { + await this.clearPlaintextSource(this.file, baseUrl, kind); + return; + } + if (source === "legacy") { + await this.clearPlaintextSource(this.legacy, baseUrl, kind); + } + } + + private async clearPlaintextSource( + storage: AuthStorage, + baseUrl: string, + kind: CredentialKind, + ): Promise { + await this.clearBestEffort(() => + kind === "tokens" + ? storage.clearTokens(baseUrl) + : storage.clearClient(baseUrl), + ); + } - this.primaryAvailable = false; - if (!this.warnedOnPrimaryFailure) { - this.warnedOnPrimaryFailure = true; - this.onPrimaryUnavailable(error); + private async clearBestEffort(fn: () => Promise): Promise { + try { + await fn(); + return undefined; + } catch (error) { + return error; } - return true; + } + + private toPolicyError(error: unknown): unknown { + if (!(error instanceof KeychainUnavailableError)) return error; + return new AuthStoragePolicyError( + `System keychain is unavailable. ${createFileAuthStorageGuidance(this.configPath)}`, + ); + } + + private warnKeychainExport(): void { + if (this.warnedFileModeKeychainExport) return; + this.warnedFileModeKeychainExport = true; + this.onWarning( + "Warning: auth.storage=file is exporting existing keychain credentials to plaintext file storage.", + ); + } + + private warnAmbiguousPlaintext(): void { + if (this.warnedAmbiguousPlaintext) return; + this.warnedAmbiguousPlaintext = true; + this.onWarning( + "Warning: multiple plaintext auth entries exist with ambiguous timestamps; using the new config auth path and leaving the other entry intact.", + ); } } diff --git a/src/services/mode-aware-file-auth-storage.test.ts b/src/services/mode-aware-file-auth-storage.test.ts new file mode 100644 index 00000000..7702fde0 --- /dev/null +++ b/src/services/mode-aware-file-auth-storage.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from "bun:test"; +import { ModeAwareFileAuthStorage } from "./mode-aware-file-auth-storage.js"; +import { createMockAuthStorage, createValidTokenData } from "./test-helpers.js"; + +describe("ModeAwareFileAuthStorage", () => { + const BASE_URL = "https://mcp.githits.com"; + + it("allows reads and clears in keychain mode", async () => { + const inner = createMockAuthStorage(); + const storage = new ModeAwareFileAuthStorage(inner, "keychain"); + + await storage.loadTokens(BASE_URL); + await storage.clearTokens(BASE_URL); + await storage.loadClient(BASE_URL); + await storage.clearClient(BASE_URL); + + expect(inner.loadTokens).toHaveBeenCalledWith(BASE_URL); + expect(inner.clearTokens).toHaveBeenCalledWith(BASE_URL); + expect(inner.loadClient).toHaveBeenCalledWith(BASE_URL); + expect(inner.clearClient).toHaveBeenCalledWith(BASE_URL); + }); + + it("rejects writes in keychain mode", async () => { + const inner = createMockAuthStorage(); + const storage = new ModeAwareFileAuthStorage( + inner, + "keychain", + "/home/test/.config/githits/config.toml", + ); + + await expect( + storage.saveTokens(BASE_URL, createValidTokenData()), + ).rejects.toThrow(/Warning: file storage is plaintext/); + await expect( + storage.saveTokens(BASE_URL, createValidTokenData()), + ).rejects.toThrow(/\/home\/test\/\.config\/githits\/config\.toml/); + expect(inner.saveTokens).not.toHaveBeenCalled(); + }); + + it("allows writes in file mode", async () => { + const inner = createMockAuthStorage(); + const storage = new ModeAwareFileAuthStorage(inner, "file"); + const token = createValidTokenData(); + + await storage.saveTokens(BASE_URL, token); + + expect(inner.saveTokens).toHaveBeenCalledWith(BASE_URL, token); + }); +}); diff --git a/src/services/mode-aware-file-auth-storage.ts b/src/services/mode-aware-file-auth-storage.ts new file mode 100644 index 00000000..fb44e9b0 --- /dev/null +++ b/src/services/mode-aware-file-auth-storage.ts @@ -0,0 +1,78 @@ +import type { AuthStorageMode } from "./auth-config.js"; +import type { + AuthStorage, + ClientRegistration, + TokenData, +} from "./auth-storage.js"; + +export class AuthStoragePolicyError extends Error { + constructor(message: string) { + super(message); + this.name = "AuthStoragePolicyError"; + } +} + +export function createFileAuthStorageGuidance(configPath: string): string { + return `OAuth credentials were not saved to plaintext file storage. + +Options: + 1. Unlock or fix your system keychain. + 2. Use GITHITS_API_TOKEN for CI/automation. + 3. If you accept storing OAuth credentials unencrypted on disk, set: + + [auth] + storage = "file" + + in ${configPath}, or run with GITHITS_AUTH_STORAGE=file. + +Warning: file storage is plaintext. Use it only on machines where local file access is trusted.`; +} + +/** + * Enforces the auth.storage policy at the plaintext file boundary. + * Reads and clears stay available for migration/logout; writes require file mode. + */ +export class ModeAwareFileAuthStorage implements AuthStorage { + constructor( + private readonly storage: AuthStorage, + private readonly mode: AuthStorageMode, + private readonly configPath = "your GitHits config.toml", + ) {} + + loadTokens(baseUrl: string): Promise { + return this.storage.loadTokens(baseUrl); + } + + async saveTokens(baseUrl: string, data: TokenData): Promise { + this.assertFileMode(); + await this.storage.saveTokens(baseUrl, data); + } + + clearTokens(baseUrl: string): Promise { + return this.storage.clearTokens(baseUrl); + } + + loadClient(baseUrl: string): Promise { + return this.storage.loadClient(baseUrl); + } + + async saveClient(baseUrl: string, data: ClientRegistration): Promise { + this.assertFileMode(); + await this.storage.saveClient(baseUrl, data); + } + + clearClient(baseUrl: string): Promise { + return this.storage.clearClient(baseUrl); + } + + getStorageLocation(): string { + return this.storage.getStorageLocation(); + } + + private assertFileMode(): void { + if (this.mode === "file") return; + throw new AuthStoragePolicyError( + createFileAuthStorageGuidance(this.configPath), + ); + } +}