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.

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

Restrict JSON-RPC request/notification params to objects and arrays #384

Open
ebkalderon opened this issue Mar 10, 2023 · 2 comments
Open
Labels
bug Something isn't working

Comments

@ebkalderon
Copy link
Owner

ebkalderon commented Mar 10, 2023

According to both the JSON-RPC 2.0 specification and the Language Server Protocol specification, the params field in all JSON-RPC requests and notifications must be either:

  • by-position: params MUST be an Array, containing the values in the Server expected order.
  • by-name: params MUST be an Object, with member names that match the Server expected parameter names. The absence of expected names MAY result in an error being generated. The names MUST match exactly, including case, to the method's expected parameters.

At the time of writing, tower-lsp's Request type does not reject messages with params fields that are not either object-like or array-like. This was done to accommodate the telemetry/event notification which, until recently, erroneously mandated a params type of object | number | boolean | string. Thankfully, this was corrected in the spec: microsoft/language-server-protocol#1686

We already restrict users from emitting non-object and non-array telemetry/event payloads in client.rs, so there's no additional work necessary on that front.

match serde_json::to_value(data) {
Err(e) => error!("invalid JSON in `telemetry/event` notification: {}", e),
Ok(mut value) => {
if !value.is_null() && !value.is_array() && !value.is_object() {
value = Value::Array(vec![value]);
}
self.send_notification_unchecked::<TelemetryEvent>(value)
.await;
}

However, we should still update the Request type and LspServiceBuilder::custom_method() to reject params values that are not serializable/deserializable to and from objects/arrays in order to fully comply.

@ebkalderon ebkalderon added the bug Something isn't working label Mar 10, 2023
@ebkalderon ebkalderon changed the title Restrict JSON-RPC request/notification params only to objects and arrays Restrict JSON-RPC request/notification params to objects and arrays Mar 10, 2023
@ebkalderon
Copy link
Owner Author

I've opened gluon-lang/lsp-types#259 to correct this at the lsp-types level.

@ebkalderon
Copy link
Owner Author

This issue was previously touched on in PR #379. Most of the code in improve-jsonrpc-deserialization that hasn't yet been integrated into mainline covers a big portion of this work already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant