-
Notifications
You must be signed in to change notification settings - Fork 64
fix: ensure connection encodes utf8 correctly #4493
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR replaces the btoa polyfill with a custom utf8ToBase64 utility function to properly handle UTF-8 encoding in authentication headers. The original btoa implementation in Node.js incorrectly encoded multi-byte UTF-8 characters, which could cause authentication failures when usernames or passwords contain non-ASCII characters.
Key Changes:
- Created a new
utf8ToBase64utility function with proper UTF-8 encoding support for both Node.js and browser environments - Replaced
btoausage in the authentication header generation with the new utility - Removed the problematic
btoapolyfill from Node.js initialization code
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/util/src/utf8ToBase64.ts | New utility function that properly encodes UTF-8 strings to Base64, with fallbacks for different environments |
| packages/util/tests/utf8ToBase64.spec.ts | Browser-compatible tests for ASCII and nullish value handling |
| packages/util/tests/utf8ToBase64.node.spec.ts | Node.js-specific tests for multi-byte character encoding and fallback behavior |
| packages/util/src/index.ts | Exports the new utf8ToBase64 function |
| packages/comms/src/connection.ts | Updated to import and use utf8ToBase64 instead of btoa for authentication header |
| packages/comms/src/index.node.ts | Removed the problematic btoa polyfill and moved Buffer import |
| packages/comms/src/index.common.ts | Minor whitespace cleanup |
| packages/comms/index.html | Added console logging for debugging workunit operations |
| packages/comms/.vscode/launch.json | Updated local development server port |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
b099bbb to
a6c56eb
Compare
a6c56eb to
470004e
Compare
jeclrsg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GordonSmith looked good to me, just a couple of nitpicks
Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
470004e to
eea7d7f
Compare
Checklist:
Testing: