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

Add validation for DTD service names #56177

Open
kenzieschmoll opened this issue Jul 8, 2024 · 5 comments
Open

Add validation for DTD service names #56177

kenzieschmoll opened this issue Jul 8, 2024 · 5 comments
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. pkg-dtd For issues related to the Dart Tooling Daemon (package:dtd or pkg/dtd_impl within the Dart SDK) type-enhancement A request for a change that isn't a bug

Comments

@kenzieschmoll
Copy link
Contributor

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:

  • restrict service names from using . characters and only allow method names to contain .. This will prevent confusion about what the service name is for something like 'foo.bar.baz'. In this example, we wouldn't know if this is the 'foo.bar' service with method 'baz' or the 'foo' service with method 'bar.baz'.
  • prevent any protected names from being used
  • should we allow any character or only alpha numeric characters?
@kenzieschmoll kenzieschmoll added the pkg-dtd For issues related to the Dart Tooling Daemon (package:dtd or pkg/dtd_impl within the Dart SDK) label Jul 8, 2024
@dart-github-bot
Copy link
Collaborator

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.

@dart-github-bot dart-github-bot added area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-enhancement A request for a change that isn't a bug labels Jul 8, 2024
@sigmundch sigmundch added area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. and removed area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. labels Jul 8, 2024
@DanTup
Copy link
Collaborator

DanTup commented Jul 9, 2024

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
Copy link
Contributor

bkonyi commented Jul 9, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

DanTup commented Jul 10, 2024

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.

copybara-service bot pushed a commit that referenced this issue Jul 11, 2024
…rvice names

See #56177

Change-Id: I9765f232e71026a0ae6ee2ce95248472fb69ba3e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/374980
Reviewed-by: Kenzie Davisson <[email protected]>
Commit-Queue: Ben Konyi <[email protected]>
Reviewed-by: Ben Konyi <[email protected]>
@lrhn lrhn removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-pkg Used for miscellaneous pkg/ packages not associated with specific area- teams. pkg-dtd For issues related to the Dart Tooling Daemon (package:dtd or pkg/dtd_impl within the Dart SDK) type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants