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

Use base64url encoding for encrypted username #6830

Merged
merged 1 commit into from
Mar 21, 2024
Merged

Use base64url encoding for encrypted username #6830

merged 1 commit into from
Mar 21, 2024

Conversation

brendanny
Copy link
Contributor

@brendanny brendanny commented Mar 14, 2024 •
edited
Loading

First time contributor checklist:

Contributor checklist:

  • My contribution is not related to translations.
  • My commits are in nice logical chunks with good commit messages
  • My changes are rebased on the latest main branch
  • A yarn ready run passes successfully (more about tests here)
  • My changes are ready to be shipped to users

Description

Changes the base64 encoding used for encrypted username links (https://signal.me/#eu/<encrypted username>) from base64 to base64url encoding to match Android and iOS behavior. (https://community.signalusers.org/t/beta-feedback-for-the-upcoming-desktop-7-3-release/59611/4?u=bjoel)

@brendanny
Copy link
Contributor Author

brendanny commented Mar 14, 2024 •
edited
Loading

I didn't change any fromBase64() calls because Buffer can correctly handle base64url (RFC 4648 section 5) and base64 (RFC 4648 section 4) encoded strings. [1]

I didn't use toWebSafeBase64() [2] because Buffer already has support for base64url. If needed, I can open a separate PR to switch the group link generator to mirror this PR's behavior.

[1] https://nodejs.org/api/buffer.html#buffers-and-character-encodings
[2] https://github.com/signalapp/Signal-Desktop/blob/main/ts/util/webSafeBase64.ts

@brendanny
Copy link
Contributor Author

brendanny commented Mar 14, 2024 •
edited
Loading

Also, separate issue, but as I noted in the beta feedback thread, yarn test on macOS fails timestamp tests if your locale date/time settings aren't default

@indutny-signal
Copy link
Contributor

Thank you so much for this contribution! We merged this internally and it will be released to beta in about one week!

@scottnonnenberg-signal scottnonnenberg-signal merged commit 62e33b4 into signalapp:main Mar 21, 2024
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@indutny-signal indutny-signal Awaiting requested review from indutny-signal

Assignees

@indutny-signal indutny-signal

Labels
PR: Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brendanny @indutny-signal @scottnonnenberg-signal