Conversation
Summary of ChangesHello @qevolg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new authentication mechanism by adding support for bearer tokens in the WebSocket client. This enhancement provides a more flexible and secure way for clients to connect to the service, aligning with modern authentication practices. The changes involve modifications across client-side logic, configuration, and URL parsing to seamlessly integrate this new token type, alongside new tests to ensure its reliability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds support for bearer token authentication, spanning client logic, configuration, utility functions, and tests. A critical security vulnerability has been identified where sensitive information, including passwords and bearer tokens, is logged in plain text, and tokens are not removed from the URL after being read, potentially exposing them in application logs. Remediation is required to mask or remove sensitive data before logging. Additionally, a small piece of redundant logic in a utility function could be simplified for clarity. The new functionality is covered by new tests.
There was a problem hiding this comment.
Pull request overview
Adds bearer token authentication support to the Node.js WebSocket SQL client, enabling connections using a bearer_token (via config or URL query param) alongside existing auth mechanisms.
Changes:
- Extend
WSConfig/URL building to supportbearer_tokenand propagate it into the WebSocket connect message. - Add Jest coverage for bearer-token-based connections and invalid token errors.
- Minor cleanup/formatting (remove unused import, adjust log/message formatting).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs/test/bulkPulling/sql.test.ts | Adds bearer token connection tests and creates a dedicated token user for testing. |
| nodejs/src/common/utils.ts | Adds bearer_token handling in getUrl() via query params and config propagation. |
| nodejs/src/common/config.ts | Adds _bearerToken field with getter/setter on WSConfig. |
| nodejs/src/client/wsClient.ts | Parses bearer_token from URL, includes it in conn args, and relaxes auth validation to accept bearer token. |
| nodejs/src/client/wsConnectorPool.ts | Formatting/log message tweaks (behavior unchanged) but still logs URL-derived connection keys. |
| nodejs/src/client/wsConnector.ts | Formatting and error message punctuation normalization. |
| nodejs/src/common/taosResult.ts | Removes an unused import. |
| nodejs/src/client/wsResponse.ts | Removes an outdated header comment and minor spacing. |
| nodejs/src/client/wsEventCallback.ts | Minor formatting (line wrapping/spacing). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sts for bearer token connection
962d72c to
645721f
Compare
645721f to
757ade8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
- Coverage 80.61% 80.16% -0.46%
==========================================
Files 30 30
Lines 2435 2485 +50
Branches 423 439 +16
==========================================
+ Hits 1963 1992 +29
- Misses 364 379 +15
- Partials 108 114 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
nodejs/src/client/wsClient.ts:52
- The debug log redacts only the password; when connecting with a bearer token,
connMsgwill includebearer_tokenand it will be logged in cleartext. Please redactbearer_token(and any other auth tokens) in this debug log similarly to the password to avoid leaking credentials in logs.
...(this._bearerToken && { bearer_token: this._bearerToken }),
},
};
if (logger.isDebugEnabled()) {
logger.debug("[wsClient.connect.connMsg]===>" + JSONBig.stringify(connMsg, (key, value) =>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request adds support for bearer token authentication for both standard WebSocket connections and TMQ consumers, correctly handling token passing through URL parameters and configuration objects, and includes new tests. A critical security vulnerability has been identified where sensitive bearer tokens are exposed in debug logs because the URL object containing the token is logged without sanitization in one instance. The review also suggests improving code consistency.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for bearer token authentication for the WebSocket client and TMQ consumer. While the changes are well-implemented with comprehensive tests and good overall code quality, a critical security vulnerability has been identified: sensitive information (passwords and tokens) could be leaked to logs due to insufficient masking, and a potential SQL injection point exists in the stmtInit method. It is recommended to address these by applying consistent masking across all logging paths and sanitizing inputs used in SQL construction. Additionally, the URL validation logic could be made more robust.
|
/gemini review |
97fe3d1 to
44388f7
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces support for bearer token authentication across the WebSocket client and TMQ consumer. Key changes include adding a _bearerToken property to WsClient and TmqConfig, updating constructors to parse bearer_token from URLs, and modifying connection messages to include the token. Logging of sensitive information has been enhanced by replacing maskPasswordForLog with a more generic maskSensitiveForLog (which now covers both passwords and bearer tokens) and introducing maskUrlForLog to redact credentials from URLs in log messages. Several method signatures were refactored for brevity, and new test cases were added to validate bearer token functionality, including scenarios for valid and invalid tokens. Review comments specifically requested using maskUrlForLog for consistent and secure URL logging in wsConnector.ts to properly mask credentials beyond just query parameters.
…Connector and WebSocketConnectionPool
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for bearer token authentication, a valuable security enhancement. The implementation is well-integrated across the WebSocket client and TMQ consumer. I particularly appreciate the thoroughness in masking sensitive information like passwords and tokens in logs, which significantly improves security. The accompanying tests are comprehensive, covering various scenarios for token-based connections.
My feedback includes a couple of suggestions to enhance the efficiency of the new logging mechanism for TMQ configuration. Please review the detailed comments.
|
Code review summary Solid update overall. The token support is wired through WS and TMQ flows, sensitive logging is now masked more consistently, and the enterprise workflow/tests provide good coverage for token auth. The refactors around formatting/readability also help. Suggested improvements (non-blocking):
Overall, this looks ready once docs are aligned. |
Description
feat: support token
Issue(s)
Checklist
Please check the items in the checklist if applicable.