Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an integration with "lsp4ij" to proxy hover requests to the Dart Analysis Server, serving as a proof-of-concept for incrementally migrating to LSP. However, it contains several security and robustness issues, including a JSON injection vulnerability when faking the "initialize" response, and sensitive information being logged at default levels. The custom JSON-RPC message parsing logic is also fragile. Additionally, the "DartLanguageServerFactory" requires improvements in robustness, adherence to platform best practices, and code maintainability, such as refactoring the long "start()" method, using managed thread pools, and improving exception handling.
| // A very naive detection of the end of a JSON-RPC message. | ||
| // A robust implementation should parse Content-Length properly. | ||
| String content = buffer.toString(); | ||
| if (content.endsWith("}")) { | ||
| int braceCount = 0; | ||
| int startIdx = content.indexOf("{"); | ||
| if (startIdx != -1) { | ||
| for (int i = startIdx; i < content.length(); i++) { | ||
| if (content.charAt(i) == '{') | ||
| braceCount++; | ||
| else if (content.charAt(i) == '}') | ||
| braceCount--; | ||
| } | ||
| if (braceCount == 0 && content.substring(0, startIdx).contains("Content-Length")) { | ||
| String jsonPart = content.substring(startIdx); |
There was a problem hiding this comment.
[MUST-FIX] The current implementation for parsing LSP messages is not robust. As noted in the code comment, it's a naive implementation that relies on checking for a closing brace and counting braces. This approach is brittle and can easily fail if:
- The JSON payload contains a
}character within a string value. - Multiple messages arrive in a single read, or a single message arrives in multiple reads.
A correct implementation must adhere to the Language Server Protocol specification by:
- Reading the
Content-Length: NNN\r\n\r\nheader. - Reading exactly
NNNbytes for the JSON content.
While this is a proof-of-concept, this is a fundamental correctness issue that should be addressed before this approach is built upon further.
References
- This is a logical bug that can lead to incorrect behavior and instability, which falls under the [MUST-FIX] severity category. (link)
| String fakeResponse = "{\"jsonrpc\":\"2.0\",\"id\":" + id | ||
| + ",\"result\":{\"capabilities\":{\"hoverProvider\":true,\"completionProvider\":{\"resolveProvider\":false}}}}"; |
There was a problem hiding this comment.
The id field from an incoming JSON-RPC request is concatenated directly into a JSON response string without proper escaping or quoting, leading to a JSON injection vulnerability. An attacker could inject arbitrary keys into the JSON response, causing unexpected behavior. This issue is located within the start() method, which is also over 100 lines long, violating the repository style guide and making it difficult to read and maintain. Consider refactoring this method into smaller, focused private methods to improve readability and address the injection vulnerability.
| LOG.info("Ignored lsp4ij request: " + method); | ||
| } | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to parse lsp4ij message: " + jsonPart, e); |
There was a problem hiding this comment.
The application logs raw LSP payloads and messages at INFO and WARN levels, which can expose sensitive information like source code or file paths, leading to intellectual property leakage if logs are shared. This logging occurs within a section where a raw new Thread() is created. It's a best practice in IntelliJ Platform development to use managed thread pools via ApplicationManager.getApplication().executeOnPooledThread() for proper thread lifecycle and resource management, especially when handling potentially sensitive data streams.
|
|
||
| if (lspPayload != null) { | ||
| String lspMessageString = lspPayload.toString(); | ||
| LOG.info("Dart server sent lsp payload: " + lspMessageString); |
There was a problem hiding this comment.
The application logs raw LSP payloads and messages at INFO and WARN levels. LSP messages frequently contain sensitive information, including source code snippets, file paths, and symbol names from the project being analyzed. Logging this data can lead to the exposure of intellectual property or other sensitive information if the IDE logs are shared for debugging or support purposes.
| } catch (Exception e) { | ||
| LOG.warn("Failed to parse lspToDartStream", e); | ||
| // Ignore parse errors from non-json or unrelated traffic | ||
| } |
There was a problem hiding this comment.
[CONCERN] Catching the generic Exception is overly broad and can hide unexpected runtime errors. It's better to catch more specific exceptions that you expect to handle, such as com.google.gson.JsonParseException, to make the error handling more explicit and robust. The log message is also a bit misleading, as it's parsing a response from the Dart Analysis Server, not reading from lspToDartStream.
| } catch (Exception e) { | |
| LOG.warn("Failed to parse lspToDartStream", e); | |
| // Ignore parse errors from non-json or unrelated traffic | |
| } | |
| } catch (com.google.gson.JsonParseException e) { | |
| LOG.warn("Failed to parse message from Dart Analysis Server", e); | |
| // Ignore parse errors from non-json or unrelated traffic | |
| } |
References
- This change improves maintainability by making error handling more specific, which aligns with the goal of reducing "'clever' code that is hard to read" and improving overall code quality. (link)
| } catch (Exception e) { | ||
| LOG.warn("Failed to parse lsp4ij message: " + jsonPart, e); | ||
| } |
There was a problem hiding this comment.
[CONCERN] Similar to the other comment, catching the generic Exception is too broad. Please catch a more specific exception, like com.google.gson.JsonParseException, to make the error handling more precise and avoid masking other potential issues.
| } catch (Exception e) { | |
| LOG.warn("Failed to parse lsp4ij message: " + jsonPart, e); | |
| } | |
| } catch (com.google.gson.JsonParseException e) { | |
| LOG.warn("Failed to parse lsp4ij message: " + jsonPart, e); | |
| } |
References
- This change improves maintainability by making error handling more specific, which aligns with the goal of reducing "'clever' code that is hard to read" and improving overall code quality. (link)
| import java.io.PipedOutputStream; | ||
| import java.nio.charset.StandardCharsets; | ||
|
|
||
| public class DartLanguageServerDefinition extends LanguageServerDefinition { |
There was a problem hiding this comment.
You should never create your own LanguageServerDefinition. You should use https://github.com/redhat-developer/lsp4ij/blob/main/docs/DeveloperGuide.md#languageserverfactory
| class DartStartupActivity : ProjectActivity { | ||
| override suspend fun execute(project: Project) { | ||
| val definition = DartLanguageServerDefinition(project) | ||
| val mapping = ServerLanguageMapping(Language.findLanguageByID("Dart")!!, "Dart", "dart", object : com.redhat.devtools.lsp4ij.DocumentMatcher { |
There was a problem hiding this comment.
Defines your mapping in plugin.xml, see https://github.com/redhat-developer/lsp4ij/blob/main/docs/DeveloperGuide.md#declare-file-mappings
| override fun match(vFile: VirtualFile, project: Project): Boolean = true | ||
| }) | ||
| PluginLogger.createLogger(DartStartupActivity::class.java).info("Registering Dart lsp4ij ServerLanguageMapping!") | ||
| LanguageServersRegistry.getInstance().registerAssociation(definition, mapping) |
There was a problem hiding this comment.
Remove this line code and register your language server factory in plugin.xml, see https://github.com/redhat-developer/lsp4ij/blob/main/docs/DeveloperGuide.md#extension-point-declaration
DanTup
left a comment
DanTup
left a comment
There was a problem hiding this comment.
I'm not familiar with this codebase, but from the changes here this sounds promising to me :-)
| JsonObject lspMessage = JsonParser.parseString(jsonPayload).getAsJsonObject(); | ||
| String method = lspMessage.has("method") ? lspMessage.get("method").getAsString() : null; | ||
|
|
||
| if ("initialize".equals(method)) { |
There was a problem hiding this comment.
I don't know how hard it would be, but it could be good if we could capture the clientCapabilities here, and maybe use them when initializing the legacy server (see lspClientCapabilities on setClientCapabilities). That way, the server will respond in ways that better match what lsp4ij expects (for example it will honour the supported completion kinds, symbol kinds, etc.).
It may mean stalling the legacy server initialization until this happens (and handling it not happening gracefully) though.
| while ((c = in.read()) != -1) { | ||
| headers.append((char) c); | ||
| if (headers.toString().endsWith("\r\n\r\n")) { | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
You might have already tried this, but could we avoid doing this ourselves and use existing code in the library?
I haven't tried to use it (and I don't know java), but Gemini had previously mentioned the consumer/producer classes, and a quick search turned up this which I think is the lsp4j version of this that might be reusable?
3 participants
Follow up to #166 to show that this can be a path for incremental changeover to LSP instead of the legacy protocol.
This is intercepting messages over streams instead of using
MessageProduceretc as mentioned in #166 (comment). The agent tells me thatMessageProduceris more performant (vs this is more a proof of concept), so I will try to set up that way in the next iteration.This is the hover content generated from lsp4ij:
(ignore the strange coloring. Something I did here broke the editor colors)