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

textDocument/hover sends escaped colon after drive letter #58985

Closed
rjmholt opened this issue Sep 19, 2018 · 15 comments
Closed

textDocument/hover sends escaped colon after drive letter #58985

rjmholt opened this issue Sep 19, 2018 · 15 comments
Assignees
Labels
*as-designed Described behavior is as designed uri
Milestone

Comments

@rjmholt
Copy link

rjmholt commented Sep 19, 2018

  • VSCode Version:
1.28.0-insider
29e6dce4352db76d8956035990b8630580226588
x64
  • OS Version:
10.0.18242 N/A Build 18242

I work on the PowerShell extension/language service for VSCode, and we have a bug report where PowerShellEditorServices crashes because we render the wrong path; there's a # character in the path and System.Uri in C# drops everything after the # as a fragment. Easy fix -- we shouldn't escape the path prematurely, but I wondered why it was done in the first place (since it's more effort).

I was working on the fix PR and @SeeminglyScience noticed I haven't escaped the : after drive letters in the URIs in the tests. I escape it and everything crashes.

But looking at the logs I notice that VSCode itself is sending the colon escaped:

2018-09-19 09:21:46.574 [DIAGNOSTIC] C:\PowerShellEditorServices\src\PowerShellEditorServices.Protocol\MessageProtocol\MessageReader.cs: In method 'ReadMessage', line 114:
    READ MESSAGE:
    
    {
      "jsonrpc": "2.0",
      "id": 18,
      "method": "textDocument/hover",
      "params": {
        "textDocument": {
          "uri": "file:///e%3A/%23Libraries/iCloudDrive/%23%23%23Travail%23%23%23/CurrentWork/Powershell/DSC/EssaiDSC/Validation/DSQ_ITEC_CAIS_TRAITEMENT/DSQ_INSTALL_CAIS_Traitement.ps1"
        },
        "position": {
          "line": 45,
          "character": 34
        }
      }
    }

2018-09-19 09:21:46.574 [VERBOSE] C:\PowerShellEditorServices\src\PowerShellEditorServices.Protocol\MessageProtocol\MessageReader.cs: In method 'ReadMessage', line 123:
    Received Request 'textDocument/hover' with id 18

2018-09-19 09:21:46.574 [VERBOSE] C:\PowerShellEditorServices\src\PowerShellEditorServices\Workspace\Workspace.cs: In method 'ResolveFilePath', line 378:
    Resolved path: e:\

2018-09-19 09:21:46.580 [ERROR] C:\PowerShellEditorServices\src\PowerShellEditorServices.Protocol\MessageProtocol\ProtocolEndpoint.cs: In method 'OnListenTaskCompleted', line 391:
    ProtocolEndpoint message loop terminated due to unhandled exception:
    
    System.AggregateException: One or more errors occurred. ---> System.UnauthorizedAccessException: Access to the path 'e:\' is denied.
       at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
       at System.IO.FileStream.Init(String path, FileMode mode, FileAccess access, Int32 rights, Boolean useRights, FileShare share, Int32 bufferSize, FileOptions options, SECURITY_ATTRIBUTES secAttrs, String msgPath, Boolean bFromProxy, Boolean useLongPath, Boolean checkHost)
       at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access)
       at Microsoft.PowerShell.EditorServices.Workspace.GetFile(String filePath)
       at Microsoft.PowerShell.EditorServices.Protocol.Server.LanguageServer.<HandleHoverRequest>d__41.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.MessageDispatcher.<DispatchMessage>d__7.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.ProtocolEndpoint.<ListenForMessages>d__36.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Utility.AsyncContext.Start(Func`1 asyncMainFunc, ILogger logger)
       at System.Threading.Tasks.Task.Execute()
       --- End of inner exception stack trace ---
    ---> (Inner Exception #0) System.UnauthorizedAccessException: Access to the path 'e:\' is denied.
       at System.IO.__Error.WinIOError(Int32 errorCode, String maybeFullPath)
       at System.IO.FileStream.Init(String path, FileMode mode, FileAccess access, Int32 rights, Boolean useRights, FileShare share, Int32 bufferSize, FileOptions options, SECURITY_ATTRIBUTES secAttrs, String msgPath, Boolean bFromProxy, Boolean useLongPath, Boolean checkHost)
       at System.IO.FileStream..ctor(String path, FileMode mode, FileAccess access)
       at Microsoft.PowerShell.EditorServices.Workspace.GetFile(String filePath)
       at Microsoft.PowerShell.EditorServices.Protocol.Server.LanguageServer.<HandleHoverRequest>d__41.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.MessageDispatcher.<DispatchMessage>d__7.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Protocol.MessageProtocol.ProtocolEndpoint.<ListenForMessages>d__36.MoveNext()
    --- End of stack trace from previous location where exception was thrown ---
       at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
       at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
       at Microsoft.PowerShell.EditorServices.Utility.AsyncContext.Start(Func`1 asyncMainFunc, ILogger logger)
       at System.Threading.Tasks.Task.Execute()<---
    

2018-09-19 09:21:46.580 [ERROR] C:\PowerShellEditorServices\src\PowerShellEditorServices.Host\EditorServicesHost.cs: In method 'ProtocolEndpoint_UnhandledException', line 435:
    PowerShell Editor Services is terminating due to an unhandled exception, see previous logs for details.

I notice that in the examples I can find of file:// scheme URIs, the drive letter's colon is never escaped:

The only issue I can find on this with VSCode is about needing to use the right method on the node Uri class to avoid this issue.

But we're seeing this problem in the encoding of a message VSCode itself is sending.

Is this intended behaviour? Is there a standard way to handle this?

@octref octref assigned jrieken and dbaeumer and unassigned jrieken Sep 20, 2018
@octref
Copy link
Contributor

octref commented Sep 20, 2018

Although I guess the escaping happens in vscode-languageclient -> @dbaeumer, still @jrieken FYI.

@dbaeumer
Copy link
Member

@jrieken assigning to you as our URI expert. I call on the URI I get in the vscode API toString() before sending it to the LSP server.

@dbaeumer dbaeumer assigned jrieken and unassigned dbaeumer Sep 21, 2018
@dbaeumer
Copy link
Member

Please assign back if there is something you think needs to be done in the LSP client.

@jrieken jrieken added uri *as-designed Described behavior is as designed info-needed Issue requires more information from poster labels Sep 21, 2018
@jrieken
Copy link
Member

jrieken commented Sep 21, 2018

This is the design, as we do minimal to none scheme specific encoding. To encode minimal you can use someUri.toString(true).

@rjmholt How do you use the URI? What library to you use to parse an uri-object from that string? I am asking because escaping too much shouldn't be a problem.

@rjmholt
Copy link
Author

rjmholt commented Sep 21, 2018

We're using the URI to resolve a workspace file on the filesystem.

We use .NET's System.Uri (I linked to .NET Framework 4.5.1, but we also use .NET Core 2.1 to do the same thing).

System.Uri has the following behaviour:

  • new Uri("file:///e%3A/%23Folder/").LocalPath is /e:/#Folder/ (not a valid location on Windows)
  • new Uri("file:///e:/%23Folder/").LocalPath is e:\#Folder\ (a valid Windows path)

If you have a Windows machine, the easiest way to verify this is with PowerShell:

> [uri]::new('file:///e%3A/%23Folder/')
AbsolutePath   : /e%3A/%23Folder
AbsoluteUri    : file:///e%3A/%23Folder
LocalPath      : /e:/#Folder
Authority      :
HostNameType   : Basic
IsDefaultPort  : True
IsFile         : True
IsLoopback     : True
PathAndQuery   : /e%3A/%23Folder
Segments       : {/, e%3A/, %23Folder}
IsUnc          : False
Host           :
Port           : -1
Query          :
Fragment       :
Scheme         : file
OriginalString : file:///e%3A/%23Folder
DnsSafeHost    :
IdnHost        :
IsAbsoluteUri  : True
UserEscaped    : False
UserInfo       :

RFC 8089 Appendix E.2 is the best resource I've found that describes the unescaped colon expected for DOS-style paths.

The mismatch between the URI that VSCode sends and the one that System.Uri expects means we have to do special handling of the URI ourselves (that's in a PR to fix the fact that we used to crash).

@jrieken
Copy link
Member

jrieken commented Sep 24, 2018

RFC 8089 Appendix E.2 is the best resource I've found that describes the unescaped colon expected for DOS-style paths.

The things is there are so many RFCs that you always find one for whatever point you wanna make. We mostly stick to https://tools.ietf.org/html/rfc3986 which doesn't define any scheme-specific things and actually try to avoid that...

So, from playing with dotnetfiddle it seems that the URI is all happy and can be parsed, no exception or so. The LocalPath property seems to be a bit magic as it seems to turn slashes around and it might do other things as well. This is related to our URI#fsPath property which we introduced for more or less that reason. Unsure whom to blame here...

screen shot 2018-09-21 at 18 52 50

@jrieken jrieken added under-discussion Issue is under discussion for relevance, priority, approach and removed *as-designed Described behavior is as designed info-needed Issue requires more information from poster labels Sep 24, 2018
@rjmholt
Copy link
Author

rjmholt commented Sep 24, 2018

Unsure whom to blame here...

Honestly, the RFCs are pretty complex as you say.

The things is there are so many RFCs that you always find one for whatever point you wanna make.

But the real problem is that .NET (Core and Framework) expect something different to what we receive — and even if we got traction in corefx (where we also open the odd issue), it's still this way forever in .NET 451 (and .NET 461, which we're changing to later). In the original issue description I listed a few other references that expect an unescaped colon.

So, from playing with dotnetfiddle it seems that the URI is all happy and can be parsed, no exception or so.

While true, only the ones with the unescaped colon yield a path from either LocalPath or AbsolutePath that is valid on Windows and doesn't require further processing. The magic LocalPath is an OS-specific conversion of the URI.

At the end of the day, we've handled the issue. In fact, we've handled it so that if this behaviour changes, we're still doing the right thing.

I was surprised that nobody had opened this issue before, but perhaps that means everyone else has adapted. It looks like Omnisharp does something similar to us — see OmniSharp/omnisharp-roslyn#1125. Looks like the Haskell LSP hit this too. The other language servers I looked at either use the VSCode TypeScript APIs to decode the URI (the OCaml and MS C++ LSPs) or seem to handle it properly in the standard library (Java (notice the special handling for Windows), Python (although MS's python language server is written in C# so might have a similar issue) and Rust LSPs).

@dbaeumer
Copy link
Member

@rjmholt there is a way in the LSP client were you can intercept all translations from vscode.URI => string and string => vscode.URI. (see https://github.com/Microsoft/vscode-languageserver-node/blob/master/client/src/client.ts#L463)

So you could plug in our own converters to overcome this problem for now. But please be aware that this might make the server dependent on running in VS Code.

@jrieken
Copy link
Member

jrieken commented Sep 28, 2018

But the real problem is that .NET (Core and Framework) expect something different to what we receive

Acknowledged - lets see what we can do during October. It's not the first time this comes up and the also weird .NET behaviour is reason to change this

@jrieken jrieken added the feature-request Request for new features or functionality label Oct 1, 2018
@jrieken jrieken closed this as completed in 258f8dc Oct 1, 2018
@jrieken
Copy link
Member

jrieken commented Oct 1, 2018

I have pushed a fix for this and we will see how well it works, depending on that there is chance that we revert this.

@jrieken jrieken removed the under-discussion Issue is under discussion for relevance, priority, approach label Oct 1, 2018
jrieken added a commit that referenced this issue Oct 1, 2018
@rjmholt
Copy link
Author

rjmholt commented Oct 1, 2018

Thanks @jrieken

@jrieken
Copy link
Member

jrieken commented Oct 16, 2018

It happened: #60163 (comment)

@rjmholt
Copy link
Author

rjmholt commented Oct 16, 2018

If you want to revert, there's no harm to us (or, I don't think, omnisharp. I looked at the F# extension, but I couldn't find any handling). Stability on this is probably more important -- I don't want to have broken all the other extensions out there.

Ideally this is documented wherever VSCode's message URI format is so that anyone else writing a .NET back end doesn't hit the System.Uri difference the hard way. I can provide a PR if you point me to the right document.

Thanks for giving it a shot @jrieken

@jrieken
Copy link
Member

jrieken commented Oct 17, 2018

Not yet reverted. I have pushed a different fix for #60163 (comment) and I am still optimistic :-)

@jrieken jrieken added the *as-designed Described behavior is as designed label Oct 23, 2018
@jrieken
Copy link
Member

jrieken commented Oct 23, 2018

@rjmholt this will be reverted because it causes too much trouble (#61504). We will clarify the documentation. What you can do is call toString(true) which makes the URI only encode minimally or use the uri-transformer logic @dbaeumer has pointed out: #58985 (comment)

jrieken added a commit that referenced this issue Oct 23, 2018
jrieken added a commit that referenced this issue Oct 23, 2018
This reverts commit 258f8dc.
jrieken added a commit that referenced this issue Oct 23, 2018
@jrieken jrieken removed the feature-request Request for new features or functionality label Oct 29, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
*as-designed Described behavior is as designed uri
Projects
None yet
Development

No branches or pull requests

4 participants