Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Auth cookie persistence #8839

Merged
merged 26 commits into from
Mar 27, 2025
Merged

Auth cookie persistence #8839

merged 26 commits into from
Mar 27, 2025

Conversation

jamesdaniels
Copy link
Member

@jamesdaniels jamesdaniels commented Mar 10, 2025 •
edited
Loading

go/firebase-auth-cookie-persistence, pair with this NextJS middleware.

Demo here—login restricted to @google.com Google accounts.

go/firebase-cookie-auth-bug-bash for bug bash instructions / feedback.

Principles of operation:

  1. New persistence option COOKIE
  2. When persistence is cookie, sign-in and token refresh requests are proxied to /__cookie__, which is handled by the NextJS middleware. The middleware this redacts the refreshToken and stores it in an HTTP-only cookie, the idToken is stored in an JS-readable cookie
  3. getCurrentUser() internally can accept an idToken and will initiated a fetch request to get the user-details
  4. The NextJS middleware can itself freshen the idToken, if it's expired on a full-page load
  5. During logout the client will set the JS-readable cookie to "" and will make a best-effort attempt to hit /__cookie__, the middleware treats the blank string as a logout and will delete the refreshToken cookie if seen

There's a lot to clean up here, some high level things that need to be addressed

  • Types
  • Fix tests
  • The cookie persistence class depends on cookieStore, we need to fall back to navigator.cookie
  • Need to test with other sign in methods
  • Sanitize the cookie names
  • Use sentinel value for logout
  • Confirm functionality in other UAs
  • Double check this functions correctly with diff tenants

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 1e50af6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
firebase Minor
@firebase/auth Minor
@firebase/auth-compat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jamesdaniels
jamesdaniels commented Mar 13, 2025
View reviewed changes
packages/auth/package.json
@@ -136,6 +136,7 @@
"@rollup/plugin-strip": "2.1.0",
"@types/express": "4.17.21",
"chromedriver": "119.0.1",
"cookie-store": "4.0.0-next.4",
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI using this dev-dep for types, somewhere else I tried this ponyfill and found it unsuitable for prod as it can't be webpacked.

@github-actions GitHub Actions

This comment was marked as resolved.

Sign in to view
@jamesdaniels jamesdaniels marked this pull request as ready for review March 25, 2025 21:04
@jamesdaniels jamesdaniels requested review from a team as code owners March 25, 2025 21:04
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 25, 2025 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (dcc62c0)Merge (01c5d15)Diff
    browser189 kB193 kB+3.96 kB (+2.1%)
    cordova164 kB166 kB+1.29 kB (+0.8%)
    main146 kB147 kB+1.52 kB (+1.0%)
    module189 kB193 kB+3.96 kB (+2.1%)
    react-native164 kB165 kB+1.29 kB (+0.8%)

  • @firebase/auth-compat

    TypeBase (dcc62c0)Merge (01c5d15)Diff
    browser20.2 kB20.2 kB+4 B (+0.0%)
    main22.4 kB22.4 kB+4 B (+0.0%)
    module20.2 kB20.2 kB+4 B (+0.0%)

  • @firebase/auth-cordova

    TypeBase (dcc62c0)Merge (01c5d15)Diff
    browser164 kB166 kB+1.29 kB (+0.8%)
    module164 kB166 kB+1.29 kB (+0.8%)

  • @firebase/auth-web-extension

    TypeBase (dcc62c0)Merge (01c5d15)Diff
    browser141 kB142 kB+1.29 kB (+0.9%)
    main158 kB159 kB+1.29 kB (+0.8%)
    module141 kB142 kB+1.29 kB (+0.9%)

  • @firebase/auth/internal

    TypeBase (dcc62c0)Merge (01c5d15)Diff
    browser200 kB204 kB+3.96 kB (+2.0%)
    main172 kB173 kB+1.53 kB (+0.9%)
    module200 kB204 kB+3.96 kB (+2.0%)

  • bundle

    TypeBase (dcc62c0)Merge (01c5d15)Diff
    auth (Anonymous)76.7 kB77.7 kB+997 B (+1.3%)
    auth (EmailAndPassword)86.8 kB87.8 kB+997 B (+1.1%)
    auth (GoogleFBTwitterGitHubPopup)104 kB105 kB+997 B (+1.0%)
    auth (GooglePopup)101 kB102 kB+997 B (+1.0%)
    auth (GoogleRedirect)101 kB102 kB+997 B (+1.0%)
    auth (Phone)94.2 kB95.2 kB+997 B (+1.1%)

  • firebase

    TypeBase (dcc62c0)Merge (01c5d15)Diff
    firebase-auth-compat.js140 kB141 kB+893 B (+0.6%)
    firebase-auth-cordova.js137 kB138 kB+973 B (+0.7%)
    firebase-auth-web-extension.js119 kB120 kB+973 B (+0.8%)
    firebase-auth.js155 kB158 kB+2.96 kB (+1.9%)
    firebase-compat.js793 kB794 kB+895 B (+0.1%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/yzioDUGmst.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 25, 2025 •
edited
Loading

Size Analysis Report 1

This report is too large (139,584 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/lye9reBjpy.html

@jamesdaniels
Copy link
Member Author

@sam-gc @DellaBitta @hsubox76 PTAL at the latest iteration, cleaned up a lot. I'll be adding comments, fixing the test I broke with the function rename, and addressing my last few TODOs in the next change-set.

DellaBitta
DellaBitta requested changes Mar 26, 2025
View reviewed changes
@jamesdaniels jamesdaniels requested a review from DellaBitta March 27, 2025 12:33
jamesdaniels
jamesdaniels commented Mar 27, 2025
View reviewed changes
packages/auth/src/core/auth/auth_impl.ts
// persistenceMananger to be available. see _getFinalTarget for more context
this._persistenceManagerAvailable = new Promise<void>(
resolve => (this._resolvePersistenceManagerAvailable = resolve)
);
Copy link
Member Author

@jamesdaniels jamesdaniels Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Can i haz Promise.withResolvers yet? 🤣

egilmorez
egilmorez approved these changes Mar 27, 2025
View reviewed changes
Copy link
Contributor

@egilmorez egilmorez left a comment

Choose a reason for hiding this comment

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

LG, just a tiny nit!

@jamesdaniels jamesdaniels merged commit fb5d422 into main Mar 27, 2025
46 of 48 checks passed
@jamesdaniels jamesdaniels deleted the jamesdaniels_authCookiePersistence branch March 27, 2025 21:54
@google-oss-bot google-oss-bot mentioned this pull request Mar 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egilmorez egilmorez egilmorez approved these changes

@DellaBitta DellaBitta DellaBitta approved these changes

@hsubox76 hsubox76 hsubox76 approved these changes

@lisajian lisajian Awaiting requested review from lisajian lisajian is a code owner

@Xiaoshouzi-gh Xiaoshouzi-gh Awaiting requested review from Xiaoshouzi-gh Xiaoshouzi-gh is a code owner

@pashanka pashanka Awaiting requested review from pashanka pashanka is a code owner

@mansisampat mansisampat Awaiting requested review from mansisampat mansisampat is a code owner

@NhienLam NhienLam Awaiting requested review from NhienLam NhienLam is a code owner

@kevinthecheung kevinthecheung Awaiting requested review from kevinthecheung

@sam-gc sam-gc Awaiting requested review from sam-gc sam-gc is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jamesdaniels @google-oss-bot @sam-gc @egilmorez @DellaBitta @hsubox76