From 25d9a8ed7e7843503cb2ba486268942e72b343d2 Mon Sep 17 00:00:00 2001 From: James Pogran Date: Fri, 26 Aug 2022 10:51:06 -0400 Subject: [PATCH 1/5] Handle Invalid WSL Uri This adds another piece of logic to attempt to detect UNC WSL Uri as the workspace to open, and return a more informative error so the client can handle prompting the user. The UNC WSL Uri being passed is in the form of `\\wsl$\Ubuntu\home\james\some\path`. If the user wants to edit files in the WSL instance, they should be opening the workspace in the Remote WSL extension. When the VS Code sends the path to terraform-ls, it is urlencoded as `\\wsl%24\Ubuntu\home\james\some\path`. This fails `uri.IsURIValid`, which is incorrect because it is a valid path that needs to be decoded. Even if it is decoded, further on in the `uri` package we make assumptions about file paths that ignore the server portion of a UNC path, resulting in terraform-ls attempting to use `Ubuntu\home\james\some\path`. A future PR can address that. --- internal/langserver/handlers/initialize.go | 12 ++++++++++ internal/uri/uri.go | 26 ++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/internal/langserver/handlers/initialize.go b/internal/langserver/handlers/initialize.go index 8f751b01e..94c6a4676 100644 --- a/internal/langserver/handlers/initialize.go +++ b/internal/langserver/handlers/initialize.go @@ -134,6 +134,18 @@ func (svc *service) Initialize(ctx context.Context, params lsp.InitializeParams) rootURI := string(params.RootURI) if !uri.IsURIValid(rootURI) { properties["root_uri"] = "invalid" + + if uri.IsURLEncodedPath(rootURI) { + uri, err := uri.UnEncodeURI(rootURI) + if err == nil { + if uri.Scheme == "file" && uri.Host == "wsl$" { + // panic("foo") + return serverCaps, fmt.Errorf("Unsupported WSLPATH: %q "+ + "This is most likely bug, please report it.", rootURI) + } + } + } + return serverCaps, fmt.Errorf("Unsupported or invalid URI: %q "+ "This is most likely bug, please report it.", rootURI) } diff --git a/internal/uri/uri.go b/internal/uri/uri.go index 8e1f386d5..63f42371b 100644 --- a/internal/uri/uri.go +++ b/internal/uri/uri.go @@ -69,6 +69,32 @@ func IsURIValid(uri string) bool { return true } +// IsURIValid checks whether uri is a valid URI per RFC 8089 +func IsURLEncodedPath(uri string) bool { + unescapedPath, err := url.PathUnescape(uri) + if err != nil { + return false + } + + _, err = url.ParseRequestURI(unescapedPath) + return err == nil +} + +// IsURIValid checks whether uri is a valid URI per RFC 8089 +func UnEncodeURI(uri string) (*url.URL, error) { + unescapedPath, err := url.PathUnescape(uri) + if err != nil { + return nil, err + } + + parsed, err := url.ParseRequestURI(unescapedPath) + if err != nil { + return nil, err + } + + return parsed, nil +} + // PathFromURI extracts OS-specific path from an RFC 8089 "file" URI Scheme func PathFromURI(rawUri string) (string, error) { uri, err := parseUri(rawUri) From 78a86371533b46e69898f2ef92cbdc4f333cf34c Mon Sep 17 00:00:00 2001 From: James Pogran Date: Fri, 2 Sep 2022 13:13:05 -0400 Subject: [PATCH 2/5] updates --- internal/langserver/handlers/initialize.go | 28 +++++++------ internal/uri/uri.go | 46 ++++++++++------------ 2 files changed, 35 insertions(+), 39 deletions(-) diff --git a/internal/langserver/handlers/initialize.go b/internal/langserver/handlers/initialize.go index 94c6a4676..8dc74696e 100644 --- a/internal/langserver/handlers/initialize.go +++ b/internal/langserver/handlers/initialize.go @@ -7,6 +7,7 @@ import ( "strings" "github.com/creachadair/jrpc2" + "github.com/creachadair/jrpc2/code" lsctx "github.com/hashicorp/terraform-ls/internal/context" "github.com/hashicorp/terraform-ls/internal/document" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" @@ -132,22 +133,23 @@ func (svc *service) Initialize(ctx context.Context, params lsp.InitializeParams) } } else { rootURI := string(params.RootURI) - if !uri.IsURIValid(rootURI) { + + invalidUriErr := jrpc2.Errorf(code.InvalidParams, + "Unsupported or invalid URI: %q "+ + "This is most likely client bug, please report it.", rootURI) + + if uri.IsWSLURI(rootURI) { properties["root_uri"] = "invalid" + // For WSL URIs we return additional error data + // such that clients (e.g. VS Code) can provide better UX + // and nudge users to open in the WSL Remote Extension instead. + return serverCaps, invalidUriErr.WithData("INVALID_URI_WSL") + } - if uri.IsURLEncodedPath(rootURI) { - uri, err := uri.UnEncodeURI(rootURI) - if err == nil { - if uri.Scheme == "file" && uri.Host == "wsl$" { - // panic("foo") - return serverCaps, fmt.Errorf("Unsupported WSLPATH: %q "+ - "This is most likely bug, please report it.", rootURI) - } - } - } + if !uri.IsURIValid(rootURI) { + properties["root_uri"] = "invalid" - return serverCaps, fmt.Errorf("Unsupported or invalid URI: %q "+ - "This is most likely bug, please report it.", rootURI) + return serverCaps, invalidUriErr } err := svc.setupWalker(ctx, params, cfgOpts) diff --git a/internal/uri/uri.go b/internal/uri/uri.go index 63f42371b..74fddf0eb 100644 --- a/internal/uri/uri.go +++ b/internal/uri/uri.go @@ -69,32 +69,6 @@ func IsURIValid(uri string) bool { return true } -// IsURIValid checks whether uri is a valid URI per RFC 8089 -func IsURLEncodedPath(uri string) bool { - unescapedPath, err := url.PathUnescape(uri) - if err != nil { - return false - } - - _, err = url.ParseRequestURI(unescapedPath) - return err == nil -} - -// IsURIValid checks whether uri is a valid URI per RFC 8089 -func UnEncodeURI(uri string) (*url.URL, error) { - unescapedPath, err := url.PathUnescape(uri) - if err != nil { - return nil, err - } - - parsed, err := url.ParseRequestURI(unescapedPath) - if err != nil { - return nil, err - } - - return parsed, nil -} - // PathFromURI extracts OS-specific path from an RFC 8089 "file" URI Scheme func PathFromURI(rawUri string) (string, error) { uri, err := parseUri(rawUri) @@ -211,3 +185,23 @@ func isLikelyWindowsDriveURIPath(uriPath string) bool { } return uriPath[0] == '/' && unicode.IsLetter(rune(uriPath[1])) && uriPath[2] == ':' } + +// IsWSLURI checks whether URI represents a WSL (Windows Subsystem for Linux) +// UNC path on Windows, such as \\wsl$\Ubuntu\path. +// +// Such a URI represents a common user error since the LS is generally +// expected to run in the same environment where files are located +// (i.e. within the Linux subsystem with Linux paths such as /Ubuntu/path). +func IsWSLURI(uri string) bool { + unescapedPath, err := url.PathUnescape(uri) + if err != nil { + return false + } + + u, err := url.ParseRequestURI(unescapedPath) + if err != nil { + return false + } + + return u.Scheme == "file" && u.Host == "wsl$" +} From 2b10e21aac803edaf3115d99753ed88229a221b8 Mon Sep 17 00:00:00 2001 From: James Pogran Date: Fri, 2 Sep 2022 13:20:01 -0400 Subject: [PATCH 3/5] fix test --- internal/langserver/handlers/initialize_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/langserver/handlers/initialize_test.go b/internal/langserver/handlers/initialize_test.go index d69a12064..2aaa91e36 100644 --- a/internal/langserver/handlers/initialize_test.go +++ b/internal/langserver/handlers/initialize_test.go @@ -113,7 +113,7 @@ func TestInitialize_withInvalidRootURI(t *testing.T) { "capabilities": {}, "processId": 12345, "rootUri": "meh" - }`}, code.SystemError.Err()) + }`}, code.InvalidParams.Err()) } func TestInitialize_multipleFolders(t *testing.T) { From 451869b178cfd2c97b75c6736c299a206e950c21 Mon Sep 17 00:00:00 2001 From: James Pogran Date: Fri, 2 Sep 2022 13:32:53 -0400 Subject: [PATCH 4/5] new test --- internal/uri/uri_test.go | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/internal/uri/uri_test.go b/internal/uri/uri_test.go index a3b3386da..93934d117 100644 --- a/internal/uri/uri_test.go +++ b/internal/uri/uri_test.go @@ -178,3 +178,36 @@ func TestMustParseURI(t *testing.T) { }) } } + +func TestIsWSLURI(t *testing.T) { + type args struct { + uri string + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "UNC WSL Path should return true", + args: args{ + uri: `file://wsl%24/Ubuntu/home/james/some/path`, + }, + want: true, + }, + { + name: "Regular file path should return false", + args: args{ + uri: `file://C:/foo/james/foo`, + }, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := IsWSLURI(tt.args.uri); got != tt.want { + t.Errorf("IsWSLURI() = %v, want %v", got, tt.want) + } + }) + } +} From 6cd7bd09aefbd6b99dd670d9ec8d454ce3fa2a7c Mon Sep 17 00:00:00 2001 From: James Pogran Date: Tue, 6 Sep 2022 13:11:41 -0400 Subject: [PATCH 5/5] feedback --- internal/uri/uri_test.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/internal/uri/uri_test.go b/internal/uri/uri_test.go index 93934d117..b3b99db61 100644 --- a/internal/uri/uri_test.go +++ b/internal/uri/uri_test.go @@ -180,32 +180,25 @@ func TestMustParseURI(t *testing.T) { } func TestIsWSLURI(t *testing.T) { - type args struct { - uri string - } tests := []struct { name string - args args + uri string want bool }{ { name: "UNC WSL Path should return true", - args: args{ - uri: `file://wsl%24/Ubuntu/home/james/some/path`, - }, + uri: `file://wsl%24/Ubuntu/home/james/some/path`, want: true, }, { name: "Regular file path should return false", - args: args{ - uri: `file://C:/foo/james/foo`, - }, + uri: `file://C:/foo/james/foo`, want: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := IsWSLURI(tt.args.uri); got != tt.want { + if got := IsWSLURI(tt.uri); got != tt.want { t.Errorf("IsWSLURI() = %v, want %v", got, tt.want) } })