fix(oauth): harden generated account ids against collisions - #1513
Conversation
📝 WalkthroughWalkthroughOAuth account IDs now use 32 hexadecimal SHA-256 characters for new and normalized IDs. Existing persisted 32-bit IDs remain unchanged. Regression tests cover historical collisions and legacy credential updates. ChangesOAuth account ID handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/oauth-account-id-collision.test.ts`:
- Around line 69-99: Add a focused test near the existing account-ID collision
tests that persists a raw Anthropic credential without an accounts array, calls
getAccountSet("anthropic") twice, and asserts both derived account IDs are
identical and exactly 32 characters long. Cover the legacy normalization path in
normalizeAccountSet while preserving the existing test setup and cleanup
conventions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c26674e-9c62-4737-b232-2e9123c27753
📒 Files selected for processing (2)
src/oauth/store.tstests/oauth-account-id-collision.test.ts
| test("existing persisted 32-bit account ids remain valid and are not rewritten", async () => { | ||
| const authPath = join(TEST_DIR, "auth.json"); | ||
| writeFileSync(authPath, JSON.stringify({ | ||
| anthropic: { | ||
| activeAccountId: "deadbeef", | ||
| accounts: [{ | ||
| id: "deadbeef", | ||
| credential: { | ||
| access: "old-access", | ||
| refresh: "old-refresh", | ||
| expires: Date.now() + 3600_000, | ||
| accountId: "existing-account", | ||
| }, | ||
| }], | ||
| }, | ||
| })); | ||
|
|
||
| expect(getAccountSet("anthropic")?.activeAccountId).toBe("deadbeef"); | ||
|
|
||
| await saveCredential("anthropic", { | ||
| access: "rotated-access", | ||
| refresh: "rotated-refresh", | ||
| expires: Date.now() + 7200_000, | ||
| accountId: "existing-account", | ||
| }); | ||
|
|
||
| const set = getAccountSet("anthropic"); | ||
| expect(set?.activeAccountId).toBe("deadbeef"); | ||
| expect(set?.accounts[0]?.id).toBe("deadbeef"); | ||
| expect(getCredential("anthropic")?.access).toBe("rotated-access"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Cover the legacy single-credential normalization path.
Lines 71-84 persist an account set. This bypasses normalizeAccountSet lines 327-331 in src/oauth/store.ts, where a raw legacy credential derives its ID on every load.
Add a test that persists a raw provider credential without accounts, calls getAccountSet("anthropic") twice, and asserts that both derived IDs are equal and 32 characters long. This verifies the deterministic legacy-normalization contract described by this PR.
As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/oauth-account-id-collision.test.ts` around lines 69 - 99, Add a focused
test near the existing account-ID collision tests that persists a raw Anthropic
credential without an accounts array, calls getAccountSet("anthropic") twice,
and asserts both derived account IDs are identical and exactly 32 characters
long. Cover the legacy normalization path in normalizeAccountSet while
preserving the existing test setup and cleanup conventions.
Source: Path instructions
|
CI note: the initial 25 GUI failures are pre-existing on the PR's exact base |
1 participant
Summary
Follow-up security hardening for the OAuth multi-account store.
newAccountId()historically kept only 8 hex characters of SHA-256, giving generated account slots a 32-bit collision space. Two distinct identities could therefore receive the same persisted account id. BecauseactiveAccountIdand per-account mutations address slots by that id, a collision can make lookups or updates resolve the wrong account.This PR:
slice(0, 32))da1e26d232-bit prefixSecurity rationale
This is pre-existing hardening, separate from #1505. #1505's reasoning replay boundary also includes credential generation, so the old 32-bit slot id did not by itself cross that replay boundary. The wider issue is the OAuth store itself: duplicate generated slot ids can alias account selection and account-scoped mutations.
128 bits keeps the identifier deterministic where legacy normalization requires that property while making accidental or practical collision search infeasible for this use case.
Scope
Only OAuth account-id derivation and focused regression coverage are changed. Existing stored ids remain backwards-compatible.
Summary by CodeRabbit
Bug Fixes
Tests