fix(utils): normalize credential storage key to protocol+host - #527
Open
SleepySML wants to merge 5 commits into
Open
fix(utils): normalize credential storage key to protocol+host#527SleepySML wants to merge 5 commits into
SleepySML wants to merge 5 commits into
Conversation
CredentialStore.getUrlStorageKey() only lowercased and trimmed a trailing slash, so the storage key depended on whatever URL path a caller happened to pass. CodeMieSSO.getStoredCredentials() (used by the proxy on startup) normalizes to protocol+host before deriving its lookup key, but CodeMieSSO.authenticate() stored credentials keyed by the raw, unnormalized URL. Running `codemie profile login --url <api-url-with-path>` therefore stored credentials under a key the proxy could never find, surfacing "SSO credentials not found" even immediately after a successful login. `codemie setup` avoided the bug only because it prompts for the bare portal URL. Normalizing inside getUrlStorageKey makes every caller (SSO store/ retrieve/clear and JWT store/retrieve/clear) agree on the same key regardless of path.
Adds regression tests for the storage-key asymmetry fixed in the previous commit: the write path keyed credentials by the raw URL while the read path normalized to protocol+host, so `profile login --url <api-url>` wrote a key the proxy could never read. keytar is mocked with an in-memory map because retrieveSSOCredentials reads the real OS keychain before the file store, which lets a stale entry mask the bug. Carries over the call-site comment from PR codemie-ai#506. Supersedes codemie-ai#506 and codemie-ai#525. Refs: EPMCDME-14132 Co-authored-by: Nikolay Sulimov <crowar@gmail.com>
Review of the storage-key normalization surfaced two defects in it. new URL() does not throw on `scheme:rest`, it yields an empty host, so `localhost:8080` and `localhost:9090` both normalized to `localhost://` and shared one credential entry — a second login overwrote the first and lookups returned another instance's session cookie. Normalization now requires a non-empty host and an http(s) protocol, otherwise it keeps the raw form. Reachable because `profile login --url` and CODEMIE_URL apply no scheme validation, unlike the interactive prompt. Credentials written under the pre-normalization key were also left unreachable: clearSSOCredentials derived only the new key, so logout reported success while a live token blob stayed in the keychain and on disk. Retrieval now falls back to the legacy key and migrates the entry, and clearing removes both keys. Tests assert exact storage filenames instead of counting entries in a directory shared across the cli vitest project, and cover endpoint distinctness and the legacy-key path. Refs: EPMCDME-14132
…ead path getStoredCredentials passed an already-normalized URL to retrieveSSOCredentials, so the store computed an identical legacy key and skipped the migration entirely. The orphaned-credential cleanup added in the previous commit could therefore never fire for a real caller, and the tests covering it passed only because they called CredentialStore directly with a path-bearing URL. Pass the caller's URL through unnormalized — the store derives the key itself and needs the original form to find entries written under the raw URL. normalizeToBase is still used for the global-fallback comparison. The two legacy-key tests now run through CodeMieSSO so they exercise the path a user actually takes. Refs: EPMCDME-14132
Technical analysis, complexity assessment, spec, plan, code-review verdicts, QA report and gate plan for the SSO credential storage-key consolidation. Refs: EPMCDME-14132
There was a problem hiding this comment.
🔵 Needs a closer look
It changes security-sensitive credential storage semantics with legacy migration behavior and still has an explicit Windows PowerShell verification step pending per the PR context.
Pull request overview
Fixes an SSO credential lookup/storage mismatch by normalizing the URL at the CredentialStore key-derivation chokepoint (protocol + host), adds legacy-key fallback + migration so previously-stored entries remain reachable/deletable, and introduces regression integration tests to prevent reintroduction.
Changes:
- Normalize credential storage keys to
protocol//host(with guards) and migrate legacy raw-URL keys on first read / clear. - Update SSO credential retrieval/expiry-clearing to pass the original URL shape so legacy-key probing remains reachable.
- Add integration tests that mock keytar and cover path-independence, endpoint distinctness, and legacy-key migration/cleanup.
File summaries
| File | Description |
|---|---|
| tests/integration/sso-credential-key-normalization.test.ts | Adds regression coverage for URL key normalization, collision-avoidance, and legacy-key migration/cleanup. |
| src/utils/security.ts | Normalizes credential storage keys to protocol+host, adds legacy-key fallback/migration, and refactors SSO clear/read helpers. |
| src/providers/plugins/sso/sso.auth.ts | Adjusts SSO credential read/expiry-clear call sites to preserve legacy-key discovery while keeping fallback validation. |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/technical-analysis.md | Records the technical investigation and rationale for the fix and test strategy. |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/spec.md | Defines the scoped spec for consolidating the prior fixes and adding regression coverage. |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/qa-report.md | Captures local QA gate results and what remains owed to CI/manual validation. |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/plan.md | Documents the execution plan (including vitest/keytar isolation constraints). |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/events.jsonl | Logs workflow events/decisions for the task run. |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/decisions.jsonl | Records approval/waiver decisions and follow-ups (incl. Windows verification). |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/complexity-assessment.json | Initial complexity assessment for the task scope. |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/code-review-final.json | Captures the initial review findings and acceptance-criteria mapping. |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/code-review-check.json | Captures the follow-up review confirming which findings were resolved. |
| docs/superpowers/tasks/2026-09-02-epmcdme-14132-sso-login-url-key-mismatch/actual-complexity.json | Records the final complexity/risk assessment after implementation details emerged. |
Review details
- Files reviewed: 13/13 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| }; | ||
|
|
||
| const store = CredentialStore.getInstance(); | ||
| // Key must match getStoredCredentials() lookup, which normalizes to protocol+host |
Comment on lines
+333
to
+334
| * Reduce a URL to protocol+host, or return it unchanged if it is not an | ||
| * http(s) URL with a host. |
Comment on lines
+48
to
+52
| /** The storage file the implementation must use for a given normalized key input. */ | ||
| function credentialFile(normalized: string): string { | ||
| const hash = createHash('sha256').update(normalized).digest('hex'); | ||
| return join(getCodemiePath('credentials'), `sso-${hash}.enc`); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates #506 (@crowar) and #525 (@AntonYeromin) — two independent fixes for the same bug, EPMCDME-14132 — into one branch, adds the regression tests neither PR shipped, and fixes three further defects that code review found in the consolidated change.
The bug: SSO credentials were stored under a key derived from the raw URL (
sso.auth.ts), but looked up under a key derived fromnormalizeToBase(url)→ protocol+host.getUrlStorageKeyonly lowercased and stripped a trailing slash, so the URL path stayed in the SHA-256 input. Runningcodemie profile login --url https://host/code-assistant-apitherefore wrote a key the proxy could never read — and sinceproxy connect desktopprints that same path-bearing URL in its suggestion, following the CLI's own advice could never work.profile statusrecovered only because its re-auth path passesconfig.codeMieUrl, which carries no API path.Supersedes #506 and #525 — please close both in favour of this branch. The fix itself is Anton's commit, cherry-picked with authorship preserved; the call-site comment is Nikolay's, credited via
Co-authored-by.Changes
getUrlStorageKeynormalizes toprotocol//hostbefore hashing (fix(utils): normalize URL to host before hashing credential storage key #525,1b836521), so store, retrieve and clear agree on one key regardless of path. This also closes a second asymmetry: logout cleared the raw key while credential expiry cleared the normalized one.new URL()does not throw onscheme:rest—new URL('localhost:8080')parses as protocollocalhost:with an empty host. Without a guard, every scheme-lesshost:portcollapsed to the same key, so two local instances would share and overwrite each other's tokens. Normalization now requires a non-empty host and anhttp:/https:protocol, else keeps the raw form. Reachable becauseprofile login --urlandCODEMIE_URLapply no scheme validation, unlike the interactive prompt.logoutwould report success while a live session-cookie blob stayed in the OS keychain and in~/.codemie/credentials. Retrieval now falls back to the legacy key and migrates the entry; clearing removes both.keytaris mocked with an in-memory map becauseretrieveSSOCredentialsconsults the real OS keychain before the file store, which lets a stale entry mask the bug.getUrlStorageKeyand normalizes at no extra cost.storeJWTCredentialshas no call sites today, so this is inert — but it stops a future JWT writer reintroducing the mismatch.No documentation change:
docs/AUTHENTICATION.md:272-277already documents both URL forms as equivalent, so this restores the code to an already-documented contract.Impact
Bare portal URLs, trailing slashes, mixed case, explicit ports, IPv6 literals and userinfo all hash to the same key as before — verified across 14 URL forms. Only URL forms that were already broken for lookup change key, and those are migrated on first read.
Testing
npm run test:unit— 3956 tests / 265 files passnpm run lint,npm run typecheck,npm run build,npm run license-check, commitlint — all passnpm run test:integrationdeferred to CI (hangs locally on subprocess/PTY specs, a pre-existing environment issue)Pending before merge: ticket AC5 asks for verification on Windows PowerShell with the
defaultprofile. That has not been done — the change was verified on macOS. The key derivation has no platform branch, and CI'stest-windowsjob will run these tests onwindows-latest, but that does not cover the manualprofile loginbrowser-SSO flow against the Windows Credential Vault. A manual Windows run is still owed.Known gap (not fixed here)
proxy connect --claude-desktophas a second failure branch — a profile with nocodeMieUrl— that still suggestscodemie profile login, which cannot fix it, becausehandleLoginnever persistscodeMieUrlto the profile. Those users have no recovery path at all, not even theprofile statusworkaround from this ticket. Deliberately out of scope; worth its own ticket.Checklist
Refs: EPMCDME-14132