fix(auth): ensure pending credential is rehydrated before linkWithCredential#1406
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces credential rehydration in packages/core/src/auth.ts using OAuthProvider and SAMLAuthProvider to ensure plain objects from sessionStorage are converted back into real AuthCredential instances before calling linkWithCredential. It also adds comprehensive unit tests for handlePendingCredential in packages/core/src/auth.test.ts. The reviewer feedback suggests improving type safety in credentialFromJSON by typing the input parameter as unknown and validating it, as well as adding an additional test case to handle invalid JSON in sessionStorage.
mikehardy
left a comment
There was a problem hiding this comment.
Nice one - pity about e2e intractability here:
Reproducing this bug end-to-end would require a real account-exists-with-different-credential collision across two federated providers (e.g. Google then GitHub on the same email), which the Auth emulator doesn't support and would mean driving live provider consent screens in CI — a disproportionate cost for a scenario the unit tests already pin down precisely.
A take issue with "disproportionate cost" since agents can produce the test practically for free but zeroing in
the Auth emulator doesn't support
That's the pity. Wish Auth emulator did support multiple providers (and MFA, and passkeys etc...)
Not this PRs fault though. LGTM
Problem
In the
auth/account-exists-with-different-credentialrecovery flow, the pending credential is captured witherror.credential.toJSON()and stored insessionStorage(errors.ts). When the user later signs back in,handlePendingCredential(auth.ts) read that string back with a bareJSON.parseand passed the resulting plain object straight tolinkWithCredential.A plain object doesn't have the internal methods (e.g.
_linkToIdToken) that Firebase Auth'sAuthCredentialimplementations rely on, so linking failed. Thecatchblock then silently discarded the pending credential and returned the already-signed-in user — so the account-linking recovery flow appeared to work but never actually linked the second provider, with no error surfaced to the app or the user.Fix
credentialFromJSONhelper inauth.tsthat rehydrates the parsed JSON back into a realAuthCredential, tryingOAuthProvider.credentialFromJSONfirst and falling back toSAMLAuthProvider.credentialFromJSON.unknown(notobject) and validates the input is a non-null object before calling either provider, since the value is really the untyped output ofJSON.parse.handlePendingCredentialnow rehydrates before callinglinkWithCredential, and unconditionally clearspendingCredfromsessionStorageup front so it's never left dangling regardless of outcome.errors.tsat the point the credential is serialized, pointing at the rehydration step inauth.ts.Testing
Added a
describe("handlePendingCredential", ...)block inauth.test.ts(usingsignInAnonymouslyas the calling vehicle, since it invokeshandlePendingCredentialinternally) covering:OAuthProvider.credentialFromJSONand links successfully.SAMLAuthProvider.credentialFromJSONwhen OAuth rehydration throws.linkWithCredentialitself fails.All 247 tests in
packages/corepass;pnpm lint:checkandpnpm buildare unaffected (no new type errors).Why no e2e (Playwright) coverage
This is a pure logic fix with no rendering surface —
handlePendingCredentialruns silently in the background after a successful re-sign-in; it doesn't drive a screen, message, or visible state that Playwright could assert on distinctly from "sign-in succeeded." The unit tests above mock the exact Firebase SDK contract points (OAuthProvider/SAMLAuthProvider/linkWithCredential) and exercise every branch of the fix directly.It's also out of this repo's current e2e scope: per the
developer-docsarchitecture decision log (AD-3), Playwright smoke tests are UI-render-only (form fields, validation, navigation) and explicitly exclude OAuth and real sign-in. Reproducing this bug end-to-end would require a realaccount-exists-with-different-credentialcollision across two federated providers (e.g. Google then GitHub on the same email), which the Auth emulator doesn't support and would mean driving live provider consent screens in CI — a disproportionate cost for a scenario the unit tests already pin down precisely.Why no
developer-docsupdatedeveloper-docs/is scoped (see documentation-policy.md) to architecture shape, hard-to-reverse decisions, step-by-step playbooks, and living backlogs — not individual bug fixes. Every existingAD-*entry in decisions.md covers dependency/CI/build-tooling tradeoffs that are expensive to revisit later. This fix has no alternative path that was considered and rejected — once the bug is understood, the fix is the only reasonable one — so there's no decision to record.Per the policy's "single owner per fact" rule, the explanation lives where a future reader will actually encounter it: a JSDoc comment on
credentialFromJSONexplaining why rehydration is required, a short pointer comment inerrors.ts, and the test suite itself documenting the expected behavior for each branch.