dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.19k stars 1.57k forks source link

Add validation for DTD service names #56177

Open kenzieschmoll opened 3 months ago

kenzieschmoll commented 3 months ago

DTD clients can register a service by calling the registerService method, which currently takes two String parameters for the service name and the method name, and concatenates them with a . character to create the service method name (e.g. 'foo.bar').

https://github.com/dart-lang/sdk/blob/master/pkg/dtd_impl/lib/src/dtd_client.dart/#L192-L193

In the current state, the service and method name have no validation. Some things we may want to validate / restrict:

dart-github-bot commented 3 months ago

Summary: The registerService method in the DTD client lacks validation for service and method names, leading to potential ambiguity and conflicts. Proposed improvements include restricting the use of '.' in service names, preventing the use of protected names, and defining allowed character sets.

DanTup commented 3 months ago

I've a change at https://dart-review.googlesource.com/c/sdk/+/374980 that disallows dots, but I haven't implemented the other suggestions here yet because they seem like questions.

prevent any protected names from being used

What does "protected" mean here?

should we allow any character or only alpha numeric characters?

@bkonyi does the VM Service have any kind of restriction on these names? (Maybe some thought already went into whether it's a good and what should be allowed/disallowed?).

bkonyi commented 3 months ago

should we allow any character or only alpha numeric characters?

@bkonyi does the VM Service have any kind of restriction on these names? (Maybe some thought already went into whether it's a good and what should be allowed/disallowed?).

There really aren't many restrictions on service extension names. For the registerService API in dart:developer, we only require that the service extension name starts with ext. and then encourage that developers follow the format of ext.{package name}.{method name}. I don't think we have any restrictions on service extensions registered via the service protocol.

kenzieschmoll commented 3 months ago

What does "protected" mean here?

I would need to look a little deeper at how all this code is implemented, but I was referring to first part services provided by DTD (like the FileSystem and UnifiedAnalytics services). We'd want to avoid naming conflicts if these go through the same service + method name call path, but we'd need to look at the code to verify whether or not that is a risk.

DanTup commented 3 months ago

I would need to look a little deeper at how all this code is implemented, but I was referring to first part services provided by DTD (like the FileSystem and UnifiedAnalytics services).

Ah, got it! You're right - the way these services are currently registered, the existing check for whether a service name is used by another client does not prevent this (since they're registered by a client). I've included a fix for this in https://dart-review.googlesource.com/c/sdk/+/374980.