fix: support current Claude Code keychain auth - #110
Conversation
|
Thanks for the PR. |
|
Thanks for the question! My setup is a bit different from the typical user flow: |
|
@andycostintoma checks out, just get lint/build to pass and I can get this through. |
|
@griffinmartin done |
griffinmartin
left a comment
griffinmartin
left a comment
There was a problem hiding this comment.
Hey, nice work on this — the overall approach is solid and the discriminated union design is the right call. A couple things I'd want addressed before merging though:
Must fix
authType should be required, not optional
Right now ClaudeCredentials.authType is optional (authType?: "api" | "oauth"), but syncToPath and refreshIfNeeded both branch on it. The problem is writeBackCredentials constructs newCreds from creds.accessToken/refreshToken/expiresAt without propagating authType — so any credentials flowing through that path lose their type discriminator and silently fall through to the OAuth branch. Making it required forces all construction sites to be explicit and eliminates a whole class of silent misbehavior.
Guard writeBackCredentials against API key entries
writeBackCredentials is exported but has no check for API key credentials. If it's ever called with an API key source (raw sk-ant-... string), updateCredentialBlob will try to JSON.parse it, fail, and return false silently. I know refreshIfNeeded has the early return so this won't happen through that path today, but the function should protect itself. Something like if (creds.authType === "api") return false at the top would do it.
Other things worth noting
- The test file mirrors
parseCredentialsandextractServicesFromDumplocally rather than importing the actual exports — works fine now but it's a drift risk over time Number.MAX_SAFE_INTEGERas the expiry sentinel is clever but a comment explaining the intent would help future readers- The
sk-ant-prefix check is a reasonable heuristic but worth a comment acknowledging the assumption - No test coverage for
syncToPathwith API key credentials — that's the most user-visible behavior change in this PR - Minor:
authTypeon the interface vstypein auth.json serve the same purpose but have different names, a cross-reference comment would help
|
@griffinmartin implemented the changes. |
Greptile SummaryThis PR extends Claude auth support to handle the
Confidence Score: 4/5Safe to merge; all changed code paths are covered by tests and the new authType branching is consistent across sync, refresh, and writeback. The authType discriminant is threaded correctly through every consumer. Two gaps in src/keychain.ts: module-level g-flag regex objects share mutable lastIndex state (safe today because the while loop always exhausts matches, but fragile if the loop body ever grows), and readCredentialsFile passes untrimmed file content to parseCredentials while the keychain path already trims. Files Needing Attention: src/keychain.ts — the module-level SERVICE_PATTERNS regexes and the untrimmed readCredentialsFile path.
|
| Filename | Overview |
|---|---|
| src/keychain.ts | Adds authType discriminant to ClaudeCredentials, exports parseCredentials and extractServicesFromDump, extends service discovery to include the Claude Code keychain entry alongside legacy Claude Code-credentials* entries, and short-circuits writeback for API key credentials. Two minor issues: module-level g-flag regex objects share mutable lastIndex state, and readCredentialsFile passes untrimmed content to parseCredentials while the keychain path already trims. |
| src/credentials.ts | Adds authType branching in syncToPath to emit the api or oauth shape into auth.json, adds authType oauth to parseOAuthResponse return, and adds an early-return guard in refreshIfNeeded for API key credentials — all clean and consistent. |
| src/keychain.test.ts | Removes previously duplicated local implementations of parseCredentials and extractServicesFromDump in favour of the now-exported real functions, extends coverage for both keychain shapes, and adds a test verifying writeBackCredentials returns false without modifying API key entries. |
| src/credentials.test.ts | Adds authType field to all existing credential fixtures and adds a new end-to-end test verifying syncAuthJson writes the api-type shape into auth.json when given an API key credential. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[readAllClaudeAccounts] --> B{macOS?}
B -- Yes --> C[listClaudeKeychainServices]
B -- No --> D[readCredentialsFile]
C --> E[extractServicesFromDump]
E --> F[Pattern 1 - Claude Code service]
E --> G[Pattern 2 - Claude Code-credentials variants]
F & G --> H[Deduplicate and order primaries first]
H --> I[readKeychainService per service - trims output]
D --> J[parseCredentials raw content]
I --> J
J --> K{starts with sk-ant-?}
K -- Yes --> L[authType api - expiresAt MAX_SAFE_INT]
K -- No --> M[Parse JSON - authType oauth]
L & M --> N[ClaudeCredentials]
N --> O[syncAuthJson]
O --> P{authType?}
P -- api --> Q[auth.json type api shape]
P -- oauth --> R[auth.json type oauth shape]
N --> S[refreshIfNeeded]
S --> T{authType api?}
T -- Yes --> U[return creds as-is - no refresh]
T -- No --> V[Check expiry and OAuth refresh]
N --> W[writeBackCredentials]
W --> X{authType api?}
X -- Yes --> Y[return false - no writeback]
X -- No --> Z[Write updated token blob]
Comments Outside Diff (1)
-
src/keychain.ts, line 199-210 (link)readCredentialsFilepasses untrimmed content toparseCredentialsreadKeychainService(the macOS keychain path) already calls.trim()before returning. ButreadCredentialsFilereads the file as-is and passes the raw string directly toparseCredentials. If.credentials.jsoncontains a rawsk-ant-*key with a trailing newline — common when the file is created by a text editor or a shellecho—raw.startsWith("sk-ant-")still matches, butaccessTokenis stored as"sk-ant-api03-...\n"(including the newline). That newline would later be embedded in theAuthorizationheader or written intoauth.json. Adding.trim()before passingrawtoparseCredentialscloses the gap consistently with the keychain path.Prompt To Fix With AI
This is a comment left during a code review. Path: src/keychain.ts Line: 199-210 Comment: **`readCredentialsFile` passes untrimmed content to `parseCredentials`** `readKeychainService` (the macOS keychain path) already calls `.trim()` before returning. But `readCredentialsFile` reads the file as-is and passes the raw string directly to `parseCredentials`. If `.credentials.json` contains a raw `sk-ant-*` key with a trailing newline — common when the file is created by a text editor or a shell `echo` — `raw.startsWith("sk-ant-")` still matches, but `accessToken` is stored as `"sk-ant-api03-...\n"` (including the newline). That newline would later be embedded in the `Authorization` header or written into `auth.json`. Adding `.trim()` before passing `raw` to `parseCredentials` closes the gap consistently with the keychain path. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/keychain.ts:23-26
**Module-level stateful regex objects**
`SERVICE_PATTERNS` stores regex literals with the `g` flag as a module-level constant. `g`-flag regexes carry mutable `lastIndex` state — each `.exec()` call advances it, and it only resets to 0 when `.exec()` returns `null`. The current `while (m !== null)` loop always exhausts matches before exiting, so `lastIndex` is naturally reset and subsequent calls to `extractServicesFromDump` start cleanly. But if anyone adds a `break`, a thrown exception, or an early return inside the loop body, the stranded `lastIndex` would cause the next call to start mid-string and silently miss earlier entries. Using `String.prototype.matchAll()` or constructing a `new RegExp(...)` inside `extractServicesFromDump` on each call would eliminate the shared-state risk entirely.
### Issue 2 of 2
src/keychain.ts:199-210
**`readCredentialsFile` passes untrimmed content to `parseCredentials`**
`readKeychainService` (the macOS keychain path) already calls `.trim()` before returning. But `readCredentialsFile` reads the file as-is and passes the raw string directly to `parseCredentials`. If `.credentials.json` contains a raw `sk-ant-*` key with a trailing newline — common when the file is created by a text editor or a shell `echo` — `raw.startsWith("sk-ant-")` still matches, but `accessToken` is stored as `"sk-ant-api03-...\n"` (including the newline). That newline would later be embedded in the `Authorization` header or written into `auth.json`. Adding `.trim()` before passing `raw` to `parseCredentials` closes the gap consistently with the keychain path.
Reviews (1): Last reviewed commit: "fix: harden auth type handling" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const SERVICE_PATTERNS = [ | ||
| /"Claude Code"/g, | ||
| /"Claude Code-credentials(?:-[0-9a-f]+)?"/g, | ||
| ] |
There was a problem hiding this comment.
SERVICE_PATTERNS stores regex literals with the g flag as a module-level constant. g-flag regexes carry mutable lastIndex state — each .exec() call advances it, and it only resets to 0 when .exec() returns null. The current while (m !== null) loop always exhausts matches before exiting, so lastIndex is naturally reset and subsequent calls to extractServicesFromDump start cleanly. But if anyone adds a break, a thrown exception, or an early return inside the loop body, the stranded lastIndex would cause the next call to start mid-string and silently miss earlier entries. Using String.prototype.matchAll() or constructing a new RegExp(...) inside extractServicesFromDump on each call would eliminate the shared-state risk entirely.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/keychain.ts
Line: 23-26
Comment:
**Module-level stateful regex objects**
`SERVICE_PATTERNS` stores regex literals with the `g` flag as a module-level constant. `g`-flag regexes carry mutable `lastIndex` state — each `.exec()` call advances it, and it only resets to 0 when `.exec()` returns `null`. The current `while (m !== null)` loop always exhausts matches before exiting, so `lastIndex` is naturally reset and subsequent calls to `extractServicesFromDump` start cleanly. But if anyone adds a `break`, a thrown exception, or an early return inside the loop body, the stranded `lastIndex` would cause the next call to start mid-string and silently miss earlier entries. Using `String.prototype.matchAll()` or constructing a `new RegExp(...)` inside `extractServicesFromDump` on each call would eliminate the shared-state risk entirely.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
2 participants
Summary
Claude Code-credentials*andClaude Codekeychain entries when discovering Claude auth sourcesClaude Codekeychain entry stores a raw managedsk-ant-...key instead of OAuth JSONClaude Codeentry into OpenCode astype: \"api\"and add tests covering both keychain formatsTesting
node --test --experimental-strip-types src/**/*.test.ts