Skip to content

fix(auth): ensure pending credential is rehydrated before linkWithCredential#1406

Merged
russellwheatley merged 2 commits into
mainfrom
auth-1391
Jul 9, 2026
Merged

fix(auth): ensure pending credential is rehydrated before linkWithCredential#1406
russellwheatley merged 2 commits into
mainfrom
auth-1391

Conversation

@russellwheatley

@russellwheatley russellwheatley commented Jul 9, 2026

Copy link
Copy Markdown
Member

Problem

In the auth/account-exists-with-different-credential recovery flow, the pending credential is captured with error.credential.toJSON() and stored in sessionStorage (errors.ts). When the user later signs back in, handlePendingCredential (auth.ts) read that string back with a bare JSON.parse and passed the resulting plain object straight to linkWithCredential.

A plain object doesn't have the internal methods (e.g. _linkToIdToken) that Firebase Auth's AuthCredential implementations rely on, so linking failed. The catch block 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

  • Added a credentialFromJSON helper in auth.ts that rehydrates the parsed JSON back into a real AuthCredential, trying OAuthProvider.credentialFromJSON first and falling back to SAMLAuthProvider.credentialFromJSON.
  • The helper takes unknown (not object) and validates the input is a non-null object before calling either provider, since the value is really the untyped output of JSON.parse.
  • handlePendingCredential now rehydrates before calling linkWithCredential, and unconditionally clears pendingCred from sessionStorage up front so it's never left dangling regardless of outcome.
  • Added a clarifying comment in errors.ts at the point the credential is serialized, pointing at the rehydration step in auth.ts.

Testing

Added a describe("handlePendingCredential", ...) block in auth.test.ts (using signInAnonymously as the calling vehicle, since it invokes handlePendingCredential internally) covering:

  • No pending credential → user returned unchanged, no rehydration/linking attempted.
  • OAuth credential rehydrates via OAuthProvider.credentialFromJSON and links successfully.
  • Falls back to SAMLAuthProvider.credentialFromJSON when OAuth rehydration throws.
  • Returns the original user and clears storage when neither provider can rehydrate the credential.
  • Returns the original user and clears storage when linkWithCredential itself fails.
  • Returns the original user and clears storage when the stored value is invalid/unparseable JSON.

All 247 tests in packages/core pass; pnpm lint:check and pnpm build are unaffected (no new type errors).

Why no e2e (Playwright) coverage

This is a pure logic fix with no rendering surface — handlePendingCredential runs 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-docs architecture 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 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.

Why no developer-docs update

developer-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 existing AD-* 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 credentialFromJSON explaining why rehydration is required, a short pointer comment in errors.ts, and the test suite itself documenting the expected behavior for each branch.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/core/src/auth.ts Outdated
Comment thread packages/core/src/auth.test.ts

@mikehardy mikehardy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@russellwheatley russellwheatley merged commit a78cb64 into main Jul 9, 2026
17 checks passed
@russellwheatley russellwheatley deleted the auth-1391 branch July 9, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pending credential is not rehydrated before linkWithCredential, so account-exists-with-different-credential linking silently fails

2 participants