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.

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

Allow http redirect for localhost testing #7783

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

bhparijat
Copy link
Contributor

@bhparijat bhparijat commented Nov 15, 2023

PR for fixing github issue #7342 to allow http redirect for localhost authdomain.
Tested using demo app that on redirect url uses http instead of https.

Copy link

changeset-bot bot commented Nov 15, 2023

🦋 Changeset detected

Latest commit: 0c05811

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

This PR includes changesets to release 3 packages
Name Type
@firebase/auth Patch
@firebase/auth-compat Patch
firebase 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

@bhparijat bhparijat force-pushed the baseUrl-fix branch 2 times, most recently from 66da067 to 3349186 Compare November 15, 2023 07:56
@bhparijat bhparijat requested review from a team as code owners November 15, 2023 07:56
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Nov 15, 2023

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (4f157b4)Merge (fe1e195)Diff
    browser182 kB183 kB+349 B (+0.2%)
    cordova210 kB210 kB+347 B (+0.2%)
    esm5236 kB237 kB+347 B (+0.1%)
    module182 kB183 kB+349 B (+0.2%)
  • @firebase/auth-cordova

    TypeBase (4f157b4)Merge (fe1e195)Diff
    browser210 kB210 kB+347 B (+0.2%)
    module210 kB210 kB+347 B (+0.2%)
  • @firebase/auth/internal

    TypeBase (4f157b4)Merge (fe1e195)Diff
    browser193 kB193 kB+349 B (+0.2%)
    esm5250 kB250 kB+347 B (+0.1%)
    main215 kB215 kB+347 B (+0.2%)
    module193 kB193 kB+349 B (+0.2%)
  • bundle

    TypeBase (4f157b4)Merge (fe1e195)Diff
    auth (GoogleFBTwitterGitHubPopup)103 kB104 kB+292 B (+0.3%)
    auth (GooglePopup)100 kB101 kB+292 B (+0.3%)
    auth (GoogleRedirect)101 kB101 kB+292 B (+0.3%)
  • firebase

    TypeBase (4f157b4)Merge (fe1e195)Diff
    firebase-auth-compat.js140 kB140 kB+311 B (+0.2%)
    firebase-auth-cordova.js177 kB178 kB+298 B (+0.2%)
    firebase-auth.js151 kB151 kB+294 B (+0.2%)
    firebase-compat.js786 kB786 kB+311 B (+0.0%)

Test Logs

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

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.

Don't see doc impact here, but otherwise LGTM!

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Dec 4, 2023

Size Analysis Report 1

Affected Products

  • @firebase/auth

    • browserPopupRedirectResolver

      Size

      TypeBase (4f157b4)Merge (fe1e195)Diff
      size64.1 kB64.4 kB+292 B (+0.5%)
      size-with-ext-deps85.9 kB86.2 kB+292 B (+0.3%)
    • getAuth

      Size

      TypeBase (4f157b4)Merge (fe1e195)Diff
      size74.3 kB74.6 kB+292 B (+0.4%)
      size-with-ext-deps103 kB103 kB+292 B (+0.3%)

Test Logs

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

@@ -129,8 +129,11 @@ export async function _getRedirectUrl(

function getHandlerBase({ config }: AuthInternal): string {
if (!config.emulator) {
return `https://${config.authDomain}/${WIDGET_PATH}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will now hardcode http:// for the localhost authDomain. It is possible that this doesn't work for some folks who are setting up TLS on localhost. But i think it is worth merging this and seeing what the feedback is.

Also adding @renkelvin to take a look.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I think the fix Parijat just made should fix that.

@hackedXD
Copy link

hackedXD commented Jan 9, 2024

+1 makes development lot easier pls merge

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.

None yet

6 participants