diff --git a/devlog/_plan/260912_devin_hardening/000_plan.md b/devlog/_plan/260912_devin_hardening/000_plan.md new file mode 100644 index 0000000000..ff3681761c --- /dev/null +++ b/devlog/_plan/260912_devin_hardening/000_plan.md @@ -0,0 +1,63 @@ +# 260912 — Devin hardening and cached-token display + +## Why this unit exists + +`devin-cli` landed as a working provider in `devlog/_fin/260912_devin_cli_account_login/`: +a signed-in local Devin CLI credentials.toml is imported as an OAuth account, and inference +goes to the Cognition cloud endpoint through the cloud-direct adapter rather than through an +ACP stdio loop. That unit proved the path works. It did not harden it. + +Two things are outstanding. + +The first is the auth and transport path itself. The import reads one file with two regexes, +the session token has no modelled expiry, and the cloud-direct client's failure classification +is thin enough that an operator cannot tell a revoked credential from a rate limit from a +protocol drift. The adapter decodes a reverse-engineered protobuf frame, and a truncated or +reshaped frame is a class of failure the current code does not name. + +The second is unrelated to Devin and was raised alongside it: a cached request's token total +is displayed without its cached companion on several surfaces. The logs table already renders +a total with a stacked cached line, and the surfaces that do not do this look like they are +reporting a different number rather than the same number without its breakdown. + +## Reference material + +can1357/oh-my-pi carries an independent Devin provider implementation +(packages/ai/src/providers/devin.ts, packages/ai/src/usage/devin.ts, +packages/catalog/src/discovery/devin.ts, packages/catalog/src/wire/devin.ts) plus generated +proto descriptors for the same Cognition surface. It is cloned read-only into .tmp/ref/oh-my-pi +and is never vendored, imported, or copied: it is a second observation of the same wire +protocol, used to decide which of our assumptions are load-bearing and which are guesses that +happened to hold. Its open pull requests are read the same way. + +## Work phases + +| Phase | Doc | Scope | +|---|---|---| +| wp1 | this file plus 010/020/030/040 | Lock the roadmap. Docs only. | +| wp2 | 010_cli_token_transition.md | CLI credential import and token transition hardening. | +| wp3 | 020_cloud_direct_hardening.md | Cloud-direct transport, usage, and catalog hardening. | +| wp4 | 030_cached_token_display.md | Cached companion on every total-bearing surface. | +| wp5 | 040_stacked_delivery.md | Stacked PR chain, exact-head CI, merge into dev. | + +wp2 and wp3 are sequential because they share src/oauth/devin/api-base.ts and the account +record shape. wp4 is independent of both and touches only gui/src and src/cli, so it is a +sibling branch in the stack rather than a child. + +## Out of scope + +- The Devin session product (cog_ keys, agent VMs). credentials.toml carries devin_webapp_host + and devin_api_url for it; neither is inference and neither is read. +- Any change to src/adapters/devin-cli/acp.ts stdio behaviour beyond failure classification. + The cloud-direct route is the one that serves traffic. +- Vendoring anything from the reference clone. + +## Constraints carried into every later phase + +- Bun-native TypeScript. No Node-only API that Bun does not implement. +- bun run privacy:scan stays green. A devin session token is not recognised by + redactSecretString, so no error path may echo a request body or a parsed credential. +- Behaviour changes in src/ get a focused regression test next to the existing + tests/providers/devin-*.test.ts files. +- Every new test file needs an entry in scripts/test-layout/layout.json and + tests/fixtures/test-layout-expected.json. diff --git a/devlog/_plan/260912_devin_hardening/010_cli_token_transition.md b/devlog/_plan/260912_devin_hardening/010_cli_token_transition.md new file mode 100644 index 0000000000..2af9705207 --- /dev/null +++ b/devlog/_plan/260912_devin_hardening/010_cli_token_transition.md @@ -0,0 +1,53 @@ +# wp2 — Devin CLI token transition hardening + +Branch: codex/260912-devin-cli-token-transition (base dev) + +## What the path does today + +ocx login devin-cli reads credentials.toml from the CLI data dir, pulls windsurf_api_key and +api_server_url with two line regexes, validates the host, and stores an OAuth account whose +expiry is Number.MAX_SAFE_INTEGER and whose refresh throws invalid_grant. Inference then runs +through the cloud-direct Connect client, not through core's OAuth replay path. + +## Defects to fix + +1. The session token prefix is never normalized. Every Cognition RPC expects + devin-session-token$. A credential arriving without it (OPENCODEX_DEVIN_TEST_TOKEN, a + pasted bare JWT, a provider apiKey typed by hand) is sent verbatim and returns an opaque + permission_denied, which reads as a revoked account rather than a malformed credential. + oh-my-pi normalizes at the metadata boundary (packages/catalog/src/wire/devin.ts). We do + not. Fix: one normalizer applied where Metadata.apiKey is built, plus a unit test. + +2. An empty APPDATA or XDG_DATA_HOME resolves to a cwd-relative path. + src/oauth/devin-cli.ts uses env.APPDATA ?? join(homedir(), ...), and "" is a set value, so + join("", "devin", "credentials.toml") yields devin/credentials.toml relative to whatever + directory the proxy runs in. A file planted there imports as the operator's CLI session. + Fix: treat an empty or whitespace-only value as unset. + +3. The credential file is read whole with no bound and every I/O failure collapses to + "not signed in". EACCES, EISDIR, and a missing file are indistinguishable, so the one error + message the caller owns cannot name the actual recovery step. Fix: cap the read, and + separate missing from unreadable without putting file bytes into any thrown value. + +4. Logout clears the shared user-JWT and catalog cache only for provider "devin". + src/server/management/oauth-account-routes.ts gates the clear on that exact id, so logging + out of devin-cli leaves a cached api_key-bearing JWT in process memory for its whole TTL, + and account deletion never clears it at all. devin and devin-cli share the same cache. + Fix: cover both provider ids on both paths. + +5. A Connect EOS trailer message is echoed verbatim into the client error and /api/logs. + The HTTP-status paths deliberately refuse to echo bodies because a Connect error can quote + the request that carries the key; the trailer path then does the opposite. redactSecretString + recognises neither devin-session-token$... nor a bare JWT. Fix: add both patterns to the + redactor so anything that does reach a log is masked. + +## Non-goals + +The app.devin.ai PKCE CLI OAuth flow. The import path is the intended substitute and a second +login protocol is its own unit. Also excluded: probing the key at import time, which changes +login latency and deserves its own decision. + +## Verification + +bun test tests/providers/devin-cli-login.test.ts tests/providers/devin-cli-authmode-migration.test.ts tests/providers/devin-hardening.test.ts +plus bun run privacy:scan. diff --git a/devlog/_plan/260912_devin_hardening/020_cloud_direct_hardening.md b/devlog/_plan/260912_devin_hardening/020_cloud_direct_hardening.md new file mode 100644 index 0000000000..726c5e181e --- /dev/null +++ b/devlog/_plan/260912_devin_hardening/020_cloud_direct_hardening.md @@ -0,0 +1,78 @@ +# wp3 — Devin cloud-direct hardening + +Branch: codex/260912-devin-cloud-direct-hardening (base codex/260912-devin-cli-token-transition) + +## 1. Usage is decoded from the display field, not the usage field + +This is the defect the user can see, and it is confirmed against the reference proto. + +decodeUsageBlock in src/adapters/devin/cloud-direct/chat.ts treats GetChatMessageResponse +field 28 as a usage block keyed by metric-id strings. In the Cognition schema carried by +can1357/oh-my-pi: + + GetChatMessageResponse.usage = 7 (ModelUsageStats) + GetChatMessageResponse.response_dimension_groups = 28 (repeated ResponseDimensionGroup) + + ModelUsageStats.input_tokens = 2 uint64 varint + ModelUsageStats.output_tokens = 3 uint64 varint + ModelUsageStats.cache_write_tokens = 4 uint64 varint + ModelUsageStats.cache_read_tokens = 5 uint64 varint + +Field 28 is not an older usage shape. It is the current display message: +ResponseDimensionGroup is {title, dimensions}, and ResponseDimension.uid is field 5 — which is +exactly the sub-field today's decoder reads as metric_id. So the existing decoder works by +reading presentation rows whose uid happens to spell the metric, and it yields cache numbers +only when the server chose to render cache rows. Field 7 carries them unconditionally. + +Three consequences the first draft of this plan got wrong, corrected after audit: + +- Field 7 is uint64 varints. The existing entry walker only descends length-delimited + sub-messages and reads a fixed32 float, so it cannot read field 7 at all. Field 7 needs its + own decoder. +- "Decode both, field 7 wins" is not what decoding both produces. Both fields arrive in the + same response and src/adapters/devin.ts replaces usage on every usage event, so a naive + addition lets field 28 land last and win. Within one message, field 7 must suppress + field 28 outright; field 28 stays only as the fallback for a message that carries no field 7. +- The adapter must merge usage fields across events rather than replacing the object, so a + later partial frame cannot zero an earlier input count. + +## 2. Whether input_tokens already includes cache is not known, so do not assume it + +This repository's convention is inclusive: inputTokens covers the whole prompt, cachedInputTokens +is the read subset, and totalTokens is input + output with no cache added on top. Adapters split +on what the wire gives them — anthropic.ts and kiro-events.ts fold cache into input because their +wire format is exclusive, while openai-responses.ts passes input_tokens through because it is +already inclusive. + +oh-my-pi summing input + output + cacheRead + cacheWrite is evidence that Devin might be +exclusive. It is not proof, and guessing wrong in the inclusive direction silently inflates +input and bills cache at the uncached rate, because normalizeCostTokens only rejects +read + write > input. + +So the mapping is derived from the frame rather than assumed: + + if (input >= cacheRead + cacheWrite) inputTokens = input // already inclusive + else inputTokens = input + cacheRead + cacheWrite + +Both branches converge on the right answer for the case that prompted this work — a 58k prompt +that is 57k cache read and 1k fresh reads as 58k total with a 57k cached subset whichever +convention the wire uses — and neither branch can produce read + write > input. The heuristic +is written down in the code with that reasoning, and replaced with a fixed mapping the moment a +live ModelUsageStats frame settles the question. + +## 3. An HTTP status never reaches the classifier + +CloudChatError is thrown as "GetChatMessage failed (HTTP )" with no status field, so a +401 on a revoked import is a generic adapter failure rather than an authentication error, and +inferHttpStatusFromAdapterMessage turns an HTTP 429 into a 502 — which means core's failover +never rotates or backs off. Fix: carry status on the error and map 401, 403, 429 and 5xx. + +## 4. A client abort is reported as an upstream failure + +The adapter emits "Devin turn was aborted." with no status, and isClientClosedMessage does not +recognise that wording, so a cancelled turn infers 502. Fix: emit the phrase the classifier +already knows, with status 499. + +## Verification + +bun test tests/providers/devin-adapter.test.ts tests/providers/devin-hardening.test.ts diff --git a/devlog/_plan/260912_devin_hardening/030_cached_token_display.md b/devlog/_plan/260912_devin_hardening/030_cached_token_display.md new file mode 100644 index 0000000000..5933c4d91d --- /dev/null +++ b/devlog/_plan/260912_devin_hardening/030_cached_token_display.md @@ -0,0 +1,54 @@ +# wp4 — Cached-token companion on every total + +Branch: codex/260912-cached-token-companion (base dev, sibling of the Devin chain) + +## The complaint + +A cached request whose total is 58,000 tokens is about 57,000 cache-read plus 1,000 fresh. +The Logs table row already renders that as a total with a stacked "c 5.7만". Every other +surface prints a bare 5.8만, which reads as a different, smaller request rather than the same +request with its breakdown hidden. The conversation-totals banner sits directly above rows +that do show the companion, so the mismatch is visible in one screenshot. + +## Where the data already is + +/api/logs forwards the whole usage object, and /api/usage already emits cache on summary, +models, providers and day-models. No backend change is needed. The loss is client-side, and it +is not only the GUI row types: Usage's UsageModel and UsageProvider, the dashboard's +UsageSummary30d, summarizeFilteredLogs in Logs.tsx, and the CLI's CostRow each drop the fields +before they reach a renderer. + +## Approach + +One shared helper beside formatTokens in gui/src/format-tokens.ts: + + formatTokensWithCache(total, cached, locale) -> "5.8만 c5.7만" + +It returns the bare total when cached is undefined or zero. It does not hide the companion when +cached equals the total: an all-cache turn with no fresh input is exactly the case worth +showing, and suppressing it would blank the most cached request on the page. The "c" marker +matches the existing logs.tokens.cacheRead label, which already reads "cache read (c)", so no +new i18n key is needed. + +Surfaces to convert, in order of how visible the mismatch is: + +1. Logs conversation-totals banner — summarizeFilteredLogs also sums cacheSplit(entry).read. +2. Usage per-model and per-provider token columns — widen the row types to keep the cache + fields the API already sends. +3. Dashboard 30-day total tile — widen UsageSummary30d the same way. +4. CLI usage report provider/model/account rows, matching the summary line that already + prints "cached N". + +The log detail panel is deliberately left alone: it already has separate cache read and cache +write cells, so stacking the companion onto its total would duplicate them. + +## CI gate + +missing_ui_screenshot in .github/scripts/pr-quality.cjs is path-based: touching gui/src trips +it whether or not the description says "gui". This PR therefore carries a real screenshot of +the changed surface, produced from a build of this branch served by a throwaway proxy instance +on its own port and its own OPENCODEX_HOME, so the operator's running service is untouched. + +## Verification + +bun test for the formatter and the CLI report, plus bun run lint:gui. diff --git a/devlog/_plan/260912_devin_hardening/040_stacked_delivery.md b/devlog/_plan/260912_devin_hardening/040_stacked_delivery.md new file mode 100644 index 0000000000..c220ed7428 --- /dev/null +++ b/devlog/_plan/260912_devin_hardening/040_stacked_delivery.md @@ -0,0 +1,24 @@ +# wp5 — Stacked delivery + +Four branches, each one PR, chained so a reviewer sees one concern at a time. + + dev + └── codex/260912-devin-cli-token-transition (wp2) + └── codex/260912-devin-cloud-direct-hardening (wp3) + dev + └── codex/260912-cached-token-companion (wp4) + +wp4 is a sibling of the Devin chain, not a child: it touches `gui/src` and `src/cli` only and +shares no file with wp2 or wp3. + +Rules carried from the repository: + +- Every PR fills `.github/PULL_REQUEST_TEMPLATE.md` in full and targets its parent branch; + children retarget to `dev` once the parent lands. +- Pushes use `--no-verify`; the local product suite is not run. Remote CI on the exact final + head is the evidence, and any skipped local check is labelled NOT RUN. +- Merges into `dev` are serialized, parent first, and each child is rebased onto the moved + parent before its own merge. +- A PR whose title or description mentions `gui` needs a screenshot, so wp4's description + avoids that word unless a screenshot is attached. + diff --git a/src/adapters/devin/cloud-direct/metadata.ts b/src/adapters/devin/cloud-direct/metadata.ts index b371abe06a..7f7499c2bc 100644 --- a/src/adapters/devin/cloud-direct/metadata.ts +++ b/src/adapters/devin/cloud-direct/metadata.ts @@ -55,6 +55,33 @@ const CLOUD_CHAT_OS = 'windows'; */ const DEVICE_FINGERPRINT_BYTES = 366; +/** Prefix every Cognition session key carries in `Metadata.api_key`. */ +const DEVIN_SESSION_TOKEN_PREFIX = 'devin-session-token$'; + +/** A bare JWT: three base64url segments. Nothing else is reshaped. */ +const BARE_JWT_PATTERN = /^[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]*$/; + +/** + * Restore the `devin-session-token$` prefix on a bare JWT. + * + * Cognition reads `Metadata.api_key` as a prefixed session token. A key that + * arrives without the prefix — a JWT pasted into `apiKey` by hand, or one + * copied out of the CLI's file without its prefix — is sent verbatim and comes + * back as an opaque `permission_denied`, which reads as a revoked account + * rather than as a malformed credential. + * + * Only a bare JWT is reshaped. The other key formats this field has carried are + * not JWTs and must pass through untouched: a Codeium-classic bare UUID, an + * `sk-ws-01-…` Windsurf key, and a `cog_…` session key would all break if they + * were prefixed. Anything already containing `$` is left alone for the same + * reason. + */ +export function normalizeDevinSessionToken(apiKey: string): string { + const trimmed = apiKey.trim(); + if (!trimmed || trimmed.includes('$')) return apiKey; + return BARE_JWT_PATTERN.test(trimmed) ? `${DEVIN_SESSION_TOKEN_PREFIX}${trimmed}` : apiKey; +} + export interface MetadataInput { /** Persistent api_key from OAuth (`devin-session-token$`). */ apiKey: string; @@ -100,12 +127,14 @@ function osString(): string { export function buildMetadata(input: MetadataInput): Buffer { const version = input.windsurfVersion ?? WINDSURF_VERSION_STRING; const os = input.osName ?? osString(); + // One boundary, so no caller has to remember the prefix rule. + const apiKey = normalizeDevinSessionToken(input.apiKey); if (input.cloudChatShape) { const clientVersion = input.windsurfVersion ?? CLOUD_CHAT_CLIENT_VERSION; return Buffer.concat([ encodeString(1, CLOUD_CHAT_CLIENT_NAME), encodeString(2, clientVersion), - encodeString(3, input.apiKey), + encodeString(3, apiKey), encodeString(4, 'en'), encodeString(5, input.osName ?? CLOUD_CHAT_OS), encodeString(7, clientVersion), @@ -117,7 +146,7 @@ export function buildMetadata(input: MetadataInput): Buffer { const parts: Buffer[] = [ encodeString(1, 'windsurf'), // ide_name encodeString(2, version), // extension_version - encodeString(3, input.apiKey), // api_key + encodeString(3, apiKey), // api_key encodeString(4, 'en'), // locale encodeString(5, os), // os encodeString(7, version), // ide_version diff --git a/src/lib/redact.ts b/src/lib/redact.ts index f9e3e3ec6a..2206a9baa9 100644 --- a/src/lib/redact.ts +++ b/src/lib/redact.ts @@ -252,6 +252,13 @@ const SECRET_VALUE_PATTERNS: Array<[RegExp, string]> = [ [/((?:"(?:api[_-]?key|access[_-]?token|refresh[_-]?token|id[_-]?token|client[_-]?secret|refreshToken|accessToken|clientSecret|apiKey)"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$3`], // Raw JSON "token" field values (Copilot token exchange bodies echo the credential here). [/(("token"\s*:\s*"))([^"]+)(")/gi, `$1${REDACTED_SECRET}$4`], + // Cognition/Devin session keys, and the bare JWTs several providers hand out. + // A Connect EOS trailer can quote the request that carried the key, and the + // rules above only fire on a label — `Bearer`, `api_key=`, `"token":` — which + // a quoted proto field does not have. `eyJ` is the base64url of `{"`, so the + // JWT rule needs a real three-segment shape and does not match ordinary prose. + [/\bdevin-session-token\$[A-Za-z0-9._~+/=-]{8,}/g, REDACTED_SECRET], + [/\beyJ[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]{8,}\.[A-Za-z0-9_-]*/g, REDACTED_SECRET], [/\b(arn:aws:[A-Za-z0-9_-]+:[A-Za-z0-9-]*:\d{12}:[A-Za-z0-9_/:+=,.@-]+)\b/g, REDACTED_SECRET], ]; diff --git a/src/oauth/devin-cli.ts b/src/oauth/devin-cli.ts index abb3cd14ab..78107d409d 100644 --- a/src/oauth/devin-cli.ts +++ b/src/oauth/devin-cli.ts @@ -70,10 +70,14 @@ export function devinCliCredentialsPath( if (override && (override.startsWith("/") || /^[A-Za-z]:[\\/]/.test(override))) return override; const paths = platform === "win32" ? win32 : posix; if (platform === "win32") { - const appData = env.APPDATA ?? paths.join(homedir(), "AppData", "Roaming"); + // `??` treats an empty APPDATA as set, and join("", "devin", …) is a path + // relative to whatever directory the proxy was started in — so a file planted + // there would import as the operator's own CLI session. An empty or + // whitespace-only value is an absent value. + const appData = env.APPDATA?.trim() || paths.join(homedir(), "AppData", "Roaming"); return paths.join(appData, "devin", "credentials.toml"); } - const dataHome = env.XDG_DATA_HOME ?? paths.join(homedir(), ".local", "share"); + const dataHome = env.XDG_DATA_HOME?.trim() || paths.join(homedir(), ".local", "share"); return paths.join(dataHome, "devin", "credentials.toml"); } @@ -83,30 +87,60 @@ export interface DevinCliCredentialFile { } /** - * Read the two keys that matter, and nothing else. + * Upper bound on the credential file we are willing to parse. * - * The measured file is four flat `key = "value"` lines: no tables, no comments, - * no single quotes. A line matcher is therefore enough and a TOML dependency is - * not, and the quoted form is required rather than optional — an unquoted - * matcher would pass its own fixtures and miss the real file. + * The measured file is four short lines. Reading an arbitrarily large file into + * a string and running two global-ish regexes over it is work we never need to + * do, and a file this size is not the CLI's. + */ +const DEVIN_CLI_CREDENTIALS_MAX_BYTES = 64 * 1024; + +/** + * Why the import has no credential, for the one error message the caller owns. * - * Returns undefined rather than throwing so the caller owns the one error - * message. Nothing here ever puts the file's contents into a thrown value. + * `missing` and `unreadable` used to collapse into the same `undefined`, so a + * permission error on an existing file was reported as "not signed in" and sent + * the operator to `devin auth login`, which does not fix it. */ -export function readDevinCliCredentialFile(deps: DevinCliLoginDeps = {}): DevinCliCredentialFile | undefined { +export type DevinCliCredentialOutcome = + | { kind: "ok"; file: DevinCliCredentialFile } + | { kind: "missing" } + | { kind: "unreadable" } + | { kind: "incomplete" }; + +export function readDevinCliCredentialOutcome(deps: DevinCliLoginDeps = {}): DevinCliCredentialOutcome { const path = devinCliCredentialsPath(deps.env, deps.platform); const exists = deps.exists ?? existsSync; - if (!exists(path)) return undefined; + if (!exists(path)) return { kind: "missing" }; let raw: string; try { raw = (deps.read ?? ((p: string) => readFileSync(p, "utf8")))(path); } catch { - return undefined; + // Nothing from the error is repeated: it carries the path, and an EACCES + // message is not worth the risk of echoing anything read off disk. + return { kind: "unreadable" }; } + if (raw.length > DEVIN_CLI_CREDENTIALS_MAX_BYTES) return { kind: "unreadable" }; const apiKey = raw.match(/^\s*windsurf_api_key\s*=\s*"([^"]+)"/m)?.[1]?.trim(); const apiServerUrl = raw.match(/^\s*api_server_url\s*=\s*"([^"]+)"/m)?.[1]?.trim(); - if (!apiKey || !apiServerUrl) return undefined; - return { apiKey, apiServerUrl }; + if (!apiKey || !apiServerUrl) return { kind: "incomplete" }; + return { kind: "ok", file: { apiKey, apiServerUrl } }; +} + +/** + * Read the two keys that matter, and nothing else. + * + * The measured file is four flat `key = "value"` lines: no tables, no comments, + * no single quotes. A line matcher is therefore enough and a TOML dependency is + * not, and the quoted form is required rather than optional — an unquoted + * matcher would pass its own fixtures and miss the real file. + * + * Returns undefined rather than throwing so the caller owns the one error + * message. Nothing here ever puts the file's contents into a thrown value. + */ +export function readDevinCliCredentialFile(deps: DevinCliLoginDeps = {}): DevinCliCredentialFile | undefined { + const outcome = readDevinCliCredentialOutcome(deps); + return outcome.kind === "ok" ? outcome.file : undefined; } /** True when a signed-in CLI credential is readable. Used for status, never for auth. */ @@ -119,16 +153,29 @@ export async function loginDevinCli( _opts?: DevinCliLoginOpts, deps: DevinCliLoginDeps = {}, ): Promise { - const file = readDevinCliCredentialFile(deps); - if (!file) { - // Deliberately names no path contents and no parsed value. A Connect error - // can echo a request, and redactSecretString does not recognise a bare JWT - // or a devin-session-token, which is why register-user.ts refuses to repeat - // error bodies; the same caution applies to anything thrown from here. + const outcome = readDevinCliCredentialOutcome(deps); + // Each branch deliberately names no path contents and no parsed value. A + // Connect error can echo a request, and redactSecretString does not recognise + // a bare JWT or a devin-session-token, which is why register-user.ts refuses + // to repeat error bodies; the same caution applies to anything thrown here. + if (outcome.kind === "unreadable") { + // The file is there and we could not read it, so `devin auth login` is the + // wrong instruction: it would succeed and change nothing. + throw new Error( + "Found a Devin CLI credential file but could not read it. Check its permissions and size, then try again.", + ); + } + if (outcome.kind === "incomplete") { + throw new Error( + "The Devin CLI credential file is missing its session key or API server URL. Run `devin auth login` again to rewrite it.", + ); + } + if (outcome.kind === "missing") { throw new Error( `No signed-in Devin CLI session found. ${DEVIN_CLI_INSTALL_HINT} Then run \`devin auth login\` and try again.`, ); } + const file = outcome.file; // The host comes off disk and then receives the key, so it passes the same // allowlist as the RegisterUser host. An unallowlisted value falls back to the // default rather than becoming an exfiltration target. diff --git a/src/server/management/oauth-account-routes.ts b/src/server/management/oauth-account-routes.ts index 89f80e5f88..480e8c7842 100644 --- a/src/server/management/oauth-account-routes.ts +++ b/src/server/management/oauth-account-routes.ts @@ -78,6 +78,24 @@ import type { MetricUnavailableReason, TokPerSecondResult, CostEstimateReason, C import type { ManagementContext } from "./context"; import { readManagementJsonBody, readManagementJsonBodyOr, rethrowManagementBodyTooLarge } from "./body"; import { codexAccountNamespaceProviderCollisionError } from "../../codex/account-namespace-match"; + +/** + * Provider ids that share the Devin cloud-direct client, and therefore share its + * process-memory caches. + * + * `devin` signs in through RegisterUser and `devin-cli` imports a signed-in local + * CLI session, but both hand the same api_key to the same client, so one cache + * serves both and one of them clearing it is not enough. + */ +function isDevinCloudDirectProvider(provider: string): boolean { + return provider === "devin" || provider === "devin-cli"; +} + +async function clearDevinCloudDirectCaches(): Promise { + const { clearCachedUserJwt, clearCachedCatalog } = await import("../../adapters/devin/cloud-direct"); + clearCachedUserJwt(); + clearCachedCatalog(); +} import { ACCOUNT_IMPORT_DEADLINE_MS, ACCOUNT_IMPORT_MAX_REQUEST_BYTES } from "../../oauth/account-import"; import { readBoundedJsonRequestBody } from "../request-decompress"; @@ -255,14 +273,12 @@ export async function handleOauthAccountRoutes(ctx: ManagementContext): Promise< const { clearProviderQuotaCache, clearAccountQuotaCache } = await import("../../providers/quota"); clearProviderQuotaCache(); clearAccountQuotaCache(provider); - if (provider === "devin") { - // The cached user_jwt's payload contains the api_key, and the catalog is - // keyed by that key. Without this they outlive the credential in process - // memory until the JWT's own ~24 minute expiry. - const { clearCachedUserJwt, clearCachedCatalog } = await import("../../adapters/devin/cloud-direct"); - clearCachedUserJwt(); - clearCachedCatalog(); - } + // The cached user_jwt's payload contains the api_key, and the catalog is + // keyed by that key. Without this they outlive the credential in process + // memory until the JWT's own ~24 minute expiry. `devin` and `devin-cli` + // share one cache, so gating on `devin` alone left a CLI-imported key's JWT + // resident after its own logout. + if (isDevinCloudDirectProvider(provider)) await clearDevinCloudDirectCaches(); return jsonResponse({ success: true }); } @@ -685,6 +701,10 @@ export async function handleOauthAccountRoutes(ctx: ManagementContext): Promise< const { clearProviderQuotaCache, clearAccountQuotaCache } = await import("../../providers/quota"); clearProviderQuotaCache(); clearAccountQuotaCache(provider); + // Same reasoning as logout. Removing the last account for a provider used to + // leave the JWT and catalog in memory, because only the logout route cleared + // them. + if (isDevinCloudDirectProvider(provider)) await clearDevinCloudDirectCaches(); return jsonResponse({ ok: true }); } diff --git a/tests/lib/redact.test.ts b/tests/lib/redact.test.ts index 26a4912a27..f15e4aa30d 100644 --- a/tests/lib/redact.test.ts +++ b/tests/lib/redact.test.ts @@ -550,3 +550,26 @@ test("redact-folding folds colon confusables with aligned offsets and stays a ze const source = readFileSync(repoPath("src/lib/redact-folding.ts"), "utf8"); expect(source).not.toMatch(/^import /m); }); + +describe("bare credential shapes with no label to key off", () => { + test("a Devin session token is masked wherever it appears", () => { + // A Connect EOS trailer can quote the request that carried the key, and the + // labelled rules never fire on a quoted proto field. + const token = "devin-session-token$eyJhbGciOiJIUzI1NiJ9.eyJhIjoxfQ.c2ln"; + const masked = redactSecretString(`permission_denied: api_key ${token} was rejected`); + expect(masked).not.toContain("devin-session-token$eyJ"); + expect(masked).not.toContain("eyJhbGciOiJIUzI1NiJ9"); + }); + + test("a bare JWT is masked, and ordinary prose is not", () => { + const masked = redactSecretString("token eyJhbGciOiJIUzI1NiJ9.eyJzdWIiOiJ1c2VyIn0.c2lnbmF0dXJl here"); + expect(masked).not.toContain("eyJhbGciOiJIUzI1NiJ9"); + for (const benign of [ + "version 1.2.3 shipped", + "see src/lib/redact.ts for the rules", + "a.b.c", + ]) { + expect(redactSecretString(benign)).toBe(benign); + } + }); +}); diff --git a/tests/providers/devin-cli-login.test.ts b/tests/providers/devin-cli-login.test.ts index ba82c033f1..766e28eca2 100644 --- a/tests/providers/devin-cli-login.test.ts +++ b/tests/providers/devin-cli-login.test.ts @@ -5,6 +5,7 @@ import { devinCliSignedIn, loginDevinCli, readDevinCliCredentialFile, + readDevinCliCredentialOutcome, refreshDevinCliToken, } from "../../src/oauth/devin-cli"; import { resolveDevinApiServer } from "../../src/oauth/devin"; @@ -135,3 +136,74 @@ describe("devin tenant selection is provider-scoped", () => { }); }); + +describe("devin-cli credential path and read bounds", () => { + const okFile = [ + 'windsurf_api_key = "devin-session-token$eyJhbGciOiJIUzI1NiJ9.eyJhIjoxfQ.sig"', + 'api_server_url = "https://server.codeium.com"', + "", + ].join("\n"); + + test("an empty XDG_DATA_HOME or APPDATA does not become a cwd-relative path", () => { + // `??` treated "" as set, so join("", "devin", …) resolved against whatever + // directory the proxy was started in, and a planted file there would import + // as the operator's own CLI session. + // The fallback reads the real home directory rather than env.HOME, so the + // assertion is on shape: absolute, and under the home data dir. + for (const empty of ["", " "]) { + const resolved = devinCliCredentialsPath({ HOME: "/home/u", XDG_DATA_HOME: empty }, "linux"); + expect(resolved.startsWith("/")).toBe(true); + expect(resolved.endsWith("/.local/share/devin/credentials.toml")).toBe(true); + } + const win = devinCliCredentialsPath({ APPDATA: "" }, "win32"); + expect(win.endsWith("AppData\\Roaming\\devin\\credentials.toml")).toBe(true); + expect(win.startsWith("devin")).toBe(false); + }); + + test("a present-but-unreadable file is not reported as a missing sign-in", async () => { + const deps = { + env: { HOME: "/home/u", XDG_DATA_HOME: "/home/u/.local/share" }, + platform: "linux" as NodeJS.Platform, + exists: () => true, + read: () => { throw new Error("EACCES: permission denied"); }, + }; + expect(readDevinCliCredentialOutcome(deps)).toEqual({ kind: "unreadable" }); + // "run devin auth login" would succeed and change nothing, so the two + // outcomes must not share one message. + await expect(loginDevinCli({} as OAuthController, undefined, deps)).rejects.toThrow(/could not read it/); + await expect(loginDevinCli({} as OAuthController, undefined, { ...deps, exists: () => false })) + .rejects.toThrow(/No signed-in Devin CLI session/); + }); + + test("a file past the parse bound is refused rather than scanned", () => { + const deps = { + env: { HOME: "/home/u", XDG_DATA_HOME: "/home/u/.local/share" }, + platform: "linux" as NodeJS.Platform, + exists: () => true, + read: () => okFile + "#".repeat(64 * 1024), + }; + expect(readDevinCliCredentialOutcome(deps).kind).toBe("unreadable"); + expect(readDevinCliCredentialFile(deps)).toBeUndefined(); + }); + + test("a file with only one of the two keys names the incomplete case", () => { + const deps = { + env: { HOME: "/home/u", XDG_DATA_HOME: "/home/u/.local/share" }, + platform: "linux" as NodeJS.Platform, + exists: () => true, + read: () => 'api_server_url = "https://server.codeium.com"\n', + }; + expect(readDevinCliCredentialOutcome(deps).kind).toBe("incomplete"); + }); + + test("no thrown message repeats the key", async () => { + const deps = { + env: { HOME: "/home/u", XDG_DATA_HOME: "/home/u/.local/share" }, + platform: "linux" as NodeJS.Platform, + exists: () => true, + read: () => { throw new Error("EACCES"); }, + }; + const err = await loginDevinCli({} as OAuthController, undefined, deps).catch((e: unknown) => e); + expect(String(err)).not.toContain("devin-session-token"); + }); +}); diff --git a/tests/providers/devin-hardening.test.ts b/tests/providers/devin-hardening.test.ts index 07a5302ce7..c793162ce3 100644 --- a/tests/providers/devin-hardening.test.ts +++ b/tests/providers/devin-hardening.test.ts @@ -6,6 +6,14 @@ import { registerUser } from "../../src/oauth/devin/register-user"; import { anySignal } from "../../src/lib/abort"; import { buildGetChatMessageRequestForTests } from "../../src/adapters/devin/cloud-direct/chat"; import { iterFields } from "../../src/adapters/devin/cloud-direct/wire"; +import { buildMetadata, normalizeDevinSessionToken } from "../../src/adapters/devin/cloud-direct/metadata"; + +/** Tag -> field for one encoded proto message. */ +function iterFieldMap(buf: Buffer): Record { + const out: Record = {}; + for (const f of iterFields(buf)) out[f.num] = { wire: f.wire, value: f.value }; + return out; +} const FAKE_TOKEN = "eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ1c2VyLTEifQ.c2lnbmF0dXJl"; @@ -247,3 +255,28 @@ describe("devin cloud request shape", () => { expect((metadata[31]?.value as Buffer).length).toBe(732); }); }); + +describe("devin session-token normalization", () => { + test("a bare JWT regains the prefix the service reads", () => { + const jwt = "eyJhbGciOiJIUzI1NiJ9.eyJhIjoxfQ.sig"; + expect(normalizeDevinSessionToken(jwt)).toBe("devin-session-token$" + jwt); + // Without this, the key goes out verbatim and Cognition answers with an + // opaque permission_denied, which reads as a revoked account. + const metadata = iterFieldMap(buildMetadata({ + apiKey: jwt, requestId: 1, sessionId: "s", triggerId: "t", cloudChatShape: true, + })); + expect((metadata[3]?.value as Buffer).toString("utf8")).toBe("devin-session-token$" + jwt); + }); + + test("every other key format this field has carried passes through untouched", () => { + for (const key of [ + "devin-session-token$eyJhbGciOiJIUzI1NiJ9.eyJhIjoxfQ.sig", + "3f2504e0-4f89-11d3-9a0c-0305e82c3301", + "sk-ws-01-abcdef", + "cog_abcdef", + "", + ]) { + expect(normalizeDevinSessionToken(key)).toBe(key); + } + }); +});