-
Notifications
You must be signed in to change notification settings - Fork 202
egor/vscode-ext #2079
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
base: canary
Are you sure you want to change the base?
egor/vscode-ext #2079
Conversation
…pens playground panel
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
this.isConnected = false | ||
} | ||
this.ws.onmessage = (event) => { | ||
console.log('RPC WebSocket message received:', event.data) |
Check warning
Code scanning / CodeQL
Log injection Medium
user-provided value
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 17 days ago
To fix the issue, we need to sanitize the event.data
before logging it. Since the logs are in plain text, we should remove any newline characters (\n
or \r
) from the input using String.prototype.replace
. This ensures that malicious input cannot forge new log entries. Additionally, we can clearly mark the user-controlled input in the log message to distinguish it from other log content.
The changes will be made in the onmessage
handler of the WebSocket, specifically on line 78 where event.data
is logged.
-
Copy modified lines R78-R79
@@ -77,3 +77,4 @@ | ||
this.ws.onmessage = (event) => { | ||
console.log('RPC WebSocket message received:', event.data) | ||
const sanitizedData = typeof event.data === 'string' ? event.data.replace(/\n|\r/g, "") : event.data; | ||
console.log('RPC WebSocket message received:', sanitizedData); | ||
let data = event.data |
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.
Caution
Changes requested ❌
Reviewed everything up to 53ceadd in 3 minutes and 20 seconds. Click for details.
- Reviewed
3789
lines of code in51
files - Skipped
3
files when reviewing. - Skipped posting
9
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. engine/language_server/src/playground/proxy.rs:210
- Draft comment:
The 'sanitizePath' function uses a simple 'replace("..", "")' operation which may not fully prevent path traversal. Consider using a more robust path normalization and validation strategy. - Reason this comment was not posted:
Comment was on unchanged code.
2. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlLanguageServerInstaller.kt:108
- Draft comment:
The installer falls back to a hardcoded version ('0.89.0') if the GitHub fetch fails. While a fallback is useful, consider logging more details or providing a configuration option for a fallback version to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. engine/Cargo.lock:787
- Draft comment:
Is 'sync_wrapper 1.0.2' intended to include the version number inline? It deviates from the other dependency naming patterns; please verify if this is a typographical error. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. engine/baml-schema-wasm/src/runtime_wasm/mod.rs:61
- Draft comment:
Typo: "I dont think we need this line anymore" should be "I don't think we need this line anymore". - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and focuses on a minor typo in a comment, which doesn't affect the functionality or logic of the code. It doesn't provide any actionable feedback related to the code itself.
5. engine/language_server/src/playground/playground_server_helpers.rs:21
- Draft comment:
Typo: In the comment, "needs to be ran." should be "needs to be run.". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the grammar correction is technically correct, comments about grammar/typos in documentation are generally low value and can be distracting from more important code review feedback. The meaning is still clear despite the grammatical error. The rules say to focus on logic issues and only keep comments that require clear code changes. The grammar is incorrect and fixing it would improve code quality. Documentation quality is important for maintainability. While documentation quality matters, this minor grammatical error doesn't impact code functionality or understanding. The rules specifically say to focus on logic issues and not make purely informative comments. Delete this comment as it's a minor grammatical correction that doesn't impact code functionality or clarity.
6. engine/language_server/src/server/api.rs:142
- Draft comment:
Typo: "signle" should be corrected to "single". - Reason this comment was not posted:
Comment was on unchanged code.
7. jetbrains/src/main/kotlin/com/boundaryml/jetbrains_ext/BamlLanguageServerFactory.kt:25
- Draft comment:
There's a potential typo in the commented code on line 25. It currently readsBamlCustomServerAPI.kt::class.java
, but typically you'd reference the class without the.kt
extension (i.e.BamlCustomServerAPI::class.java
). - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The code is completely commented out and appears to be template/example code. The typo, while technically correct (Kotlin class references shouldn't include .kt), is in inactive code that isn't actually part of the implementation. Making suggestions about commented code isn't valuable since it's not actually in use. The comment is technically correct about Kotlin syntax. Maybe the commented code is intended as a template for future implementation? Even if it's meant as a template, commenting on inactive example code doesn't provide immediate value. If someone uncomments this later, they'll naturally fix the syntax. Delete the comment as it's addressing a minor syntax issue in commented-out template code that isn't actually part of the active implementation.
8. typescript/playground-common/src/baml_wasm_web/EventListener.tsx:148
- Draft comment:
Typographical error and inconsistent naming: "Websocket recieved message!" should be updated to "WebSocket received message!" - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is technically correct about the spelling and capitalization, this is just a console.log message used for debugging. Console.log messages are typically not considered production code and often get removed. The typo doesn't affect functionality and isn't visible to end users. This seems like an overly pedantic comment that doesn't add meaningful value. The consistent capitalization of "WebSocket" elsewhere in the file could suggest we should maintain that consistency throughout. The misspelling of "received" is a legitimate error that could be worth fixing. While consistency is good, this is just debug logging that may be temporary. The benefit of fixing this is extremely low compared to the overhead of making a PR comment about it. This comment should be removed as it focuses on an unimportant typo in debug logging that doesn't impact functionality or user experience.
9. typescript/vscode-ext/packages/vscode/src/plugins/language-server-client/index.ts:222
- Draft comment:
Typographical Error: The comment contains 'dont' without an apostrophe. Consider changing it to "don't" for proper grammar. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_LgV9Wrm0NkLJJWzd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
let proxy_server = ProxyServer::new(proxy_port); | ||
|
||
// Spawn the proxy server in a separate task | ||
let proxy_handle = tokio::spawn(async move { |
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.
Consider capturing and managing the JoinHandle returned by 'tokio::spawn' for the proxy server task. This would allow proper error handling and graceful shutdown of the proxy task.
@@ -135,15 +134,38 @@ export const EventListener: React.FC<{ children: React.ReactNode }> = ({ childre | |||
setOrchestratorIndex(0) | |||
} | |||
}, [selectedFunc]) | |||
console.log('selectedFunc', selectedFunc) | |||
// console.log('selectedFunc', selectedFunc) | |||
|
|||
useEffect(() => { |
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.
There appear to be two separate useEffect hooks that each create a WebSocket connection to the '/ws' endpoint. This duplication may lead to redundant connections and unexpected behavior. Consider consolidating the WebSocket connection logic into a single useEffect.
Plugs in the LSP hosted playground into the vscode extension logic.
Important
Integrates LSP hosted playground into VSCode extension with WebSocket communication, new commands, and enhanced UI for real-time interaction with BAML code.
WebviewPanelHost
to manage playground port and display loading state.baml.openBamlPanel
andbaml.runBamlTest
commands for opening playground and running tests.viewFunctionInPlayground
andrunTestInPlayground
functions for LSP communication.BamlToolWindowFactory
to load playground URL based on LSP port.WebviewPanelHost
to show loading state and update content based on LSP notifications.build.gradle.kts
andgradle.properties
for plugin configuration.BamlLanguageServerInstaller
for managing CLI installation and updates.vscode.ts
to use WebSocket for RPC communication.extension.ts
.This description was created by
for 53ceadd. You can customize this summary. It will automatically update as commits are pushed.