Skip to content

Conversation

@taise-hikawa
Copy link

@taise-hikawa taise-hikawa commented Dec 7, 2025

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

Firebase Scrypt password hash and salt decoding uses base64.StdEncoding.
When users are exported from Firebase, their PasswordHash may contain URL-safe Base64 characters (- and _). These characters cannot be decoded by base64.StdEncoding.DecodeString(), causing decoding errors.
Related issue: firebase/firebase-admin-go#479

What is the new behavior?

  • Changed PasswordHash and salt decoding to use base64.URLEncoding
  • signerKey and saltSeparator continue to use base64.StdEncoding (as they are provided in standard Base64 format)

Test updates:

  • Updated test data to use salt and hash containing URL-safe Base64 characters (_)
  • Added negative test cases to verify that standard Base64 (containing /) in salt and hash results in errors

Additional context

According to Firebase Admin SDK Issue firebase/firebase-admin-go#479, when exporting user data from Firebase, the PasswordHash is encoded in URL-safe Base64, while the signerKey provided in the Firebase Console uses standard Base64.
When importing users from Firebase Auth, the exported data contains URL-safe Base64 encoded values like:

{  
  "passwordHash": "cxbDzJu2W1DU20SXh--UNtR9Tu2tH2GSKQ91hGBAEN49OJJpuZbdhz-3GdYnBujQXxkXdqnhUehXVeLA6zoF-g==",  
  "passwordSalt": "G-s6NpQQJdtvlQ=="
}

Both passwordHash and passwordSalt contain - characters, which are URL-safe Base64 specific and cannot be decoded with standard Base64.

@taise-hikawa taise-hikawa requested a review from a team as a code owner December 7, 2025 15:37
@hf
Copy link
Contributor

hf commented Dec 8, 2025

Instead of using Base64.URLEncoding this should normalize URL-encoded strings to regular ones (or vice versa). Clearly there's ones where regular URL encoding is used.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 20006493610

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 68.435%

Totals Coverage Status
Change from base Build 19871602451: 0.0%
Covered Lines: 14641
Relevant Lines: 21394

💛 - Coveralls

@taise-hikawa
Copy link
Author

@hf thanks!
I agreed. I pushed new commit d738166

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.

3 participants