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

pathContext.fromUri() mishandles encoded colons after drive letters #545

Closed
DanTup opened this issue Jul 21, 2023 · 4 comments
Closed

pathContext.fromUri() mishandles encoded colons after drive letters #545

DanTup opened this issue Jul 21, 2023 · 4 comments

Comments

@DanTup
Copy link
Contributor

DanTup commented Jul 21, 2023

This came up via dart-lang/sdk#53000. I had swapped some code from doing uri.toFilePath() to pathContext.fromUri(uri) to support testing Windows-style paths on MacOS.

However it turns out there is a difference in behaviour when the colon after a drive letter is percent-encoded:

  final fileUris = [
    'file:///C%3A/Dev/Test%20Projects/dart_application_3',
    'file:///C:/Dev/Test%20Projects/dart_application_3'
  ];
  for (final fileUri in fileUris) {
    final path1 = Uri.parse(fileUri).toFilePath();
    final path2 = pathContext.fromUri(fileUri);
    print('Uri.parse($fileUri).toFilePath():\n    $path1');
    print('pathContext.fromUri($fileUri):\n    $path2');
  }
Uri.parse(file:///C%3A/Dev/Test%20Projects/dart_application_3).toFilePath():
    C:\Dev\Test Projects\dart_application_3

pathContext.fromUri(file:///C%3A/Dev/Test%20Projects/dart_application_3):
    \C:\Dev\Test Projects\dart_application_3

Uri.parse(file:///C:/Dev/Test%20Projects/dart_application_3).toFilePath():
    C:\Dev\Test Projects\dart_application_3

pathContext.fromUri(file:///C:/Dev/Test%20Projects/dart_application_3):
    C:\Dev\Test Projects\dart_application_3

Since toFilePath() handles this (and VS Code has been producing URIs in this format forever), I believe the encoding of the colon is valid (although I've failed to find anything that seems explicit in the spec) and should be handled.

@DanTup
Copy link
Contributor Author

DanTup commented Jul 21, 2023

I found this:

https://url.spec.whatwg.org/#windows-drive-letter

A Windows drive letter is two code points, of which the first is an ASCII alpha and the second is either U+003A (:) or U+007C (|).

Though I found this comment in the VS Code source (which claims it is not incorrect):

		 * *Note* that the implementation will encode _aggressive_ which often leads to unexpected,
		 * but not incorrect, results. For instance, colons are encoded to `%3A` which might be unexpected
		 * in file-uri.

So now I'm less certain. I'm going to file an issue with the VS Code LSP client and see what response I get there.

@DanTup
Copy link
Contributor Author

DanTup commented Aug 9, 2023

@natebosch thanks! Any issues with me bumping this in the Dart SDK's DEPs?

@natebosch
Copy link
Member

@natebosch thanks! Any issues with me bumping this in the Dart SDK's DEPs?

No concerns at all - I would bump it soon if no one else did 😄

After we bump in the SDK and it gets synced to google I will also get it published.

@DanTup
Copy link
Contributor Author

DanTup commented Aug 9, 2023

Thanks! I've opened https://dart-review.googlesource.com/c/sdk/+/319522. You (or another Googler) will need to merge once you're happy. Please let me know if any issues come up that you think may be related.

copybara-service bot referenced this issue in dart-lang/sdk Aug 9, 2023
Gets a fix for supporting encoded colons in fromUri() (https://github.com/dart-lang/path/issues/148).

Change-Id: I191b770f3ca3b616ee862a640ad2c17600871fbe
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/319522
Reviewed-by: Nate Bosch <nbosch@google.com>
Commit-Queue: Nate Bosch <nbosch@google.com>
copybara-service bot referenced this issue in dart-lang/sdk Aug 9, 2023
…le URIs in the LSP server

This reverts be4189f plus adds an additional test to verify pkg:path to behaviour (to catch future regressions or if pkg:path has to be reverted, this will need reverting too).

This relies on the fix made at https://github.com/dart-lang/path/issues/148 which rolled into the SDK in f1de897.

Change-Id: I1dea45e2017f7505bc4aca97f6c07c1a6e445a5e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/319523
Commit-Queue: Brian Wilkerson <brianwilkerson@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
mosuem referenced this issue Oct 16, 2024
Fixes dart-lang/path#148

Using `Uri().toFilePath()` handles escaped colons in drive letters, but
`Context.fromUri()` currently does not.

Allow the percent encoded colon by checking both formats and adjusting
to the correct index for the following character when the percent
encoding makes the drive letter longer than the typical two characters.
@mosuem mosuem transferred this issue from dart-lang/path Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants