From 6c3422a0b31669b6db9a5571bb7bb69a9b6d3c0b Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Tue, 5 Dec 2023 11:09:46 +0100 Subject: [PATCH] fix: windows support for loading files On windows, there are quite a few specifics when it comes to resolving a local file. This complexity stemmed from the desire to support common, albeit invalid URI constructs for windows. Overall, the original sin of this function has been to ignore the host part of an URI, but now we can't really break this behavior. * fixes #72 * refactored the local path pre-processing, so the windows-specifics appear more clearly * fixed a few inconsistencies on windows file URIs and guarded against a panic * test: refactored tests for the local file strategy, now table-driven and more systematic * doc: added detailed documentation to LoadStrategy() * ci: re-enacted tests on windows * ci: temporarily muted past linting issues: relinting is deferred to another PR Signed-off-by: Frederic BIDON --- .github/workflows/ci.yaml | 6 +- .golangci.yml | 6 +- loading.go | 105 ++++++++++++----- loading_test.go | 229 ++++++++++++++++++++++++++++++-------- 4 files changed, 273 insertions(+), 73 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 4c99b3e..4f4e9c8 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -12,9 +12,11 @@ jobs: steps: - uses: actions/checkout@v2 - - uses: golangci/golangci-lint-action@v2 + - uses: golangci/golangci-lint-action@v3 with: args: --timeout=5m + version: latest + only-new-issues: true build: runs-on: ${{ matrix.os }} @@ -24,7 +26,7 @@ jobs: strategy: matrix: # No Windows this time. Some tests expect Unix-style paths. - os: [ ubuntu-latest, macos-latest ] + os: [ ubuntu-latest, macos-latest, windows-latest] fail-fast: false steps: diff --git a/.golangci.yml b/.golangci.yml index bf503e4..e68ce6a 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -11,7 +11,7 @@ linters-settings: threshold: 100 goconst: min-len: 3 - min-occurrences: 2 + min-occurrences: 3 linters: enable-all: true @@ -52,3 +52,7 @@ linters: - nilnil - nonamedreturns - nosnakecase + - depguard + # temporarily disabled + - goconst + - testifylint diff --git a/loading.go b/loading.go index 00038c3..1e66abe 100644 --- a/loading.go +++ b/loading.go @@ -21,6 +21,7 @@ import ( "net/http" "net/url" "os" + "path" "path/filepath" "runtime" "strings" @@ -40,43 +41,97 @@ var LoadHTTPBasicAuthPassword = "" var LoadHTTPCustomHeaders = map[string]string{} // LoadFromFileOrHTTP loads the bytes from a file or a remote http server based on the path passed in -func LoadFromFileOrHTTP(path string) ([]byte, error) { - return LoadStrategy(path, os.ReadFile, loadHTTPBytes(LoadHTTPTimeout))(path) +func LoadFromFileOrHTTP(pth string) ([]byte, error) { + return LoadStrategy(pth, os.ReadFile, loadHTTPBytes(LoadHTTPTimeout))(pth) } // LoadFromFileOrHTTPWithTimeout loads the bytes from a file or a remote http server based on the path passed in // timeout arg allows for per request overriding of the request timeout -func LoadFromFileOrHTTPWithTimeout(path string, timeout time.Duration) ([]byte, error) { - return LoadStrategy(path, os.ReadFile, loadHTTPBytes(timeout))(path) +func LoadFromFileOrHTTPWithTimeout(pth string, timeout time.Duration) ([]byte, error) { + return LoadStrategy(pth, os.ReadFile, loadHTTPBytes(timeout))(pth) } -// LoadStrategy returns a loader function for a given path or uri -func LoadStrategy(path string, local, remote func(string) ([]byte, error)) func(string) ([]byte, error) { - if strings.HasPrefix(path, "http") { +// LoadStrategy returns a loader function for a given path or URI. +// +// The load strategy returns the remote load for any path starting with `http`. +// So this works for any URI with a scheme `http` or `https`. +// +// The fallback strategy is to call the local loader. +// +// The local loader takes a local file system path (absolute or relative) as argument, +// or alternatively a `file://...` URI, **without host** (see also below for windows). +// +// There are a few liberalities, initially intended to be tolerant regarding the URI syntax, +// especially on windows. +// +// Before the local loader is called, the given path is transformed: +// - percent-encoded characters are unescaped +// - simple paths (e.g. `./folder/file`) are passed as-is +// - on windows, occurrences of `/` are replaced by `\`, so providing a relative path such a `folder/file` works too. +// +// For paths provided as URIs with the "file" scheme, please note that: +// - `file://` is simply stripped. +// This means that the host part of the URI is not parsed at all. +// For example, `file:///folder/file" becomes "/folder/file`, +// but `file://localhost/folder/file` becomes `localhost/folder/file` on unix systems. +// Similarly, `file://./folder/file` yields `./folder/file`. +// - on windows, `file://...` can take a host so as to specify an UNC share location. +// +// Reminder about windows-specifics: +// - `file://host/folder/file` becomes an UNC path like `\\host\folder\file` (no port specification is supported) +// - `file:///c:/folder/file` becomes `C:\folder\file` +// - `file://c:/folder/file` is tolerated (without leading `/`) and becomes `c:\folder\file` +func LoadStrategy(pth string, local, remote func(string) ([]byte, error)) func(string) ([]byte, error) { + if strings.HasPrefix(pth, "http") { return remote } - return func(pth string) ([]byte, error) { - upth, err := pathUnescape(pth) + + return func(p string) ([]byte, error) { + upth, err := pathUnescape(p) if err != nil { return nil, err } - if strings.HasPrefix(pth, `file://`) { - if runtime.GOOS == "windows" { - // support for canonical file URIs on windows. - // Zero tolerance here for dodgy URIs. - u, _ := url.Parse(upth) - if u.Host != "" { - // assume UNC name (volume share) - // file://host/share/folder\... ==> \\host\share\path\folder - // NOTE: UNC port not yet supported - upth = strings.Join([]string{`\`, u.Host, u.Path}, `\`) - } else { - // file:///c:/folder/... ==> just remove the leading slash - upth = strings.TrimPrefix(upth, `file:///`) - } - } else { - upth = strings.TrimPrefix(upth, `file://`) + if !strings.HasPrefix(p, `file://`) { + // regular file path provided: just normalize slashes + return local(filepath.FromSlash(upth)) + } + + if runtime.GOOS != "windows" { + // crude processing: this leaves full URIs with a host with a (mostly) unexpected result + upth = strings.TrimPrefix(upth, `file://`) + + return local(filepath.FromSlash(upth)) + } + + // windows-only pre-processing of file://... URIs + + // support for canonical file URIs on windows. + u, err := url.Parse(filepath.ToSlash(upth)) + if err != nil { + return nil, err + } + + if u.Host != "" { + // assume UNC name (volume share) + // NOTE: UNC port not yet supported + + // when the "host" segment is a drive letter: + // file://C:/folder/... => C:\folder + upth = path.Clean(strings.Join([]string{u.Host, u.Path}, `/`)) + if !strings.HasSuffix(u.Host, ":") && u.Host[0] != '.' { + // tolerance: if we have a leading dot, this can't be a host + // file://host/share/folder\... ==> \\host\share\path\folder + upth = "//" + upth + } + } else { + // no host, let's figure out if this is a drive letter + upth = strings.TrimPrefix(upth, `file://`) + first, _, _ := strings.Cut(strings.TrimPrefix(u.Path, "/"), "/") + if strings.HasSuffix(first, ":") { + // drive letter in the first segment: + // file:///c:/folder/... ==> strip the leading slash + upth = strings.TrimPrefix(upth, `/`) } } diff --git a/loading_test.go b/loading_test.go index dda4606..fad3e77 100644 --- a/loading_test.go +++ b/loading_test.go @@ -17,9 +17,11 @@ package swag import ( "net/http" "net/http/httptest" + "runtime" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const ( @@ -31,9 +33,8 @@ const ( ) func TestLoadFromHTTP(t *testing.T) { - _, err := LoadFromFileOrHTTP("httx://12394:abd") - assert.Error(t, err) + require.Error(t, err) serv := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { rw.WriteHeader(http.StatusNotFound) @@ -41,7 +42,7 @@ func TestLoadFromHTTP(t *testing.T) { defer serv.Close() _, err = LoadFromFileOrHTTP(serv.URL) - assert.Error(t, err) + require.Error(t, err) ts2 := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { rw.WriteHeader(http.StatusOK) @@ -50,7 +51,7 @@ func TestLoadFromHTTP(t *testing.T) { defer ts2.Close() d, err := LoadFromFileOrHTTP(ts2.URL) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, []byte("the content"), d) ts3 := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { @@ -65,21 +66,21 @@ func TestLoadFromHTTP(t *testing.T) { // no auth _, err = LoadFromFileOrHTTP(ts3.URL) - assert.Error(t, err) + require.Error(t, err) // basic auth, invalide credentials LoadHTTPBasicAuthUsername = validUsername LoadHTTPBasicAuthPassword = invalidPassword _, err = LoadFromFileOrHTTP(ts3.URL) - assert.Error(t, err) + require.Error(t, err) // basic auth, valid credentials LoadHTTPBasicAuthUsername = validUsername LoadHTTPBasicAuthPassword = validPassword _, err = LoadFromFileOrHTTP(ts3.URL) - assert.NoError(t, err) + require.NoError(t, err) ts4 := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { myHeaders := r.Header[sharedHeaderKey] @@ -99,12 +100,12 @@ func TestLoadFromHTTP(t *testing.T) { defer ts4.Close() _, err = LoadFromFileOrHTTP(ts4.URL) - assert.Error(t, err) + require.Error(t, err) LoadHTTPCustomHeaders[sharedHeaderKey] = sharedHeaderValue _, err = LoadFromFileOrHTTP(ts4.URL) - assert.NoError(t, err) + require.NoError(t, err) // clean up for future tests LoadHTTPBasicAuthUsername = "" @@ -114,29 +115,28 @@ func TestLoadFromHTTP(t *testing.T) { func TestLoadHTTPBytes(t *testing.T) { _, err := LoadFromFileOrHTTP("httx://12394:abd") - assert.Error(t, err) + require.Error(t, err) - serv := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + serv := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { rw.WriteHeader(http.StatusNotFound) })) defer serv.Close() _, err = LoadFromFileOrHTTP(serv.URL) - assert.Error(t, err) + require.Error(t, err) - ts2 := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + ts2 := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { rw.WriteHeader(http.StatusOK) _, _ = rw.Write([]byte("the content")) })) defer ts2.Close() d, err := LoadFromFileOrHTTP(ts2.URL) - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, []byte("the content"), d) } func TestLoadStrategy(t *testing.T) { - loader := func(p string) ([]byte, error) { return []byte(yamlPetStore), nil } @@ -152,49 +152,188 @@ func TestLoadStrategy(t *testing.T) { defer serv.Close() s, err := YAMLDoc(serv.URL) - assert.NoError(t, err) - assert.NotNil(t, s) + require.NoError(t, err) + require.NotNil(t, s) - ts2 := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + ts2 := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, _ *http.Request) { rw.WriteHeader(http.StatusNotFound) _, _ = rw.Write([]byte("\n")) })) defer ts2.Close() _, err = YAMLDoc(ts2.URL) - assert.Error(t, err) + require.Error(t, err) } func TestLoadStrategyFile(t *testing.T) { const ( - thisIsIt = "thisIsIt" + thisIsIt = "thisIsIt" + thisIsNotIt = "not it" ) - called, pth := false, "" - loader := func(p string) ([]byte, error) { - called = true - pth = p - return []byte(thisIsIt), nil - } - remLoader := func(p string) ([]byte, error) { - return []byte("not it"), nil + type strategyTest struct { + Title string + Path string + Expected string + ExpectedWindows string + ExpectError bool } - ld := LoadStrategy("blah", loader, remLoader) + t.Run("with local file strategy", func(t *testing.T) { + loader := func(called *bool, pth *string) func(string) ([]byte, error) { + return func(p string) ([]byte, error) { + *called = true + *pth = p + return []byte(thisIsIt), nil + } + } - b, _ := ld("file:///a/c/myfile.yaml") - assert.True(t, called) - assert.Equal(t, "/a/c/myfile.yaml", pth) - assert.Equal(t, []byte(thisIsIt), b) - - called, pth = false, "" - b, _ = ld(`file://C:\a\c\myfile.yaml`) - assert.True(t, called) - assert.Equal(t, `C:\a\c\myfile.yaml`, pth) - assert.Equal(t, []byte(thisIsIt), b) - - called, pth = false, "" - b, _ = ld(`file://C%3A%5Ca%5Cc%5Cmyfile.yaml`) - assert.True(t, called) - assert.Equal(t, `C:\a\c\myfile.yaml`, pth) - assert.Equal(t, []byte(thisIsIt), b) + remLoader := func(p string) ([]byte, error) { + return []byte(thisIsNotIt), nil + } + + for _, toPin := range []strategyTest{ + { + Title: "valid fully qualified local URI, with rooted path", + Path: "file:///a/c/myfile.yaml", + Expected: "/a/c/myfile.yaml", + ExpectedWindows: `\a\c\myfile.yaml`, + }, + { + Title: "local URI with scheme, with host segment before path", + Path: "file://a/c/myfile.yaml", + Expected: "a/c/myfile.yaml", + ExpectedWindows: `\\a\c\myfile.yaml`, // UNC host + }, + { + Title: "local URI with scheme, with escaped characters", + Path: "file://a/c/myfile%20%28x86%29.yaml", + Expected: "a/c/myfile (x86).yaml", + ExpectedWindows: `\\a\c\myfile (x86).yaml`, + }, + { + Title: "local URI with scheme, rooted, with escaped characters", + Path: "file:///a/c/myfile%20%28x86%29.yaml", + Expected: "/a/c/myfile (x86).yaml", + ExpectedWindows: `\a\c\myfile (x86).yaml`, + }, + { + Title: "local URI with scheme, unescaped, with host", + Path: "file://a/c/myfile (x86).yaml", + Expected: "a/c/myfile (x86).yaml", + ExpectedWindows: `\\a\c\myfile (x86).yaml`, + }, + { + Title: "local URI with scheme, rooted, unescaped", + Path: "file:///a/c/myfile (x86).yaml", + Expected: "/a/c/myfile (x86).yaml", + ExpectedWindows: `\a\c\myfile (x86).yaml`, + }, + { + Title: "file URI with drive letter and backslashes, as a relative Windows path", + Path: `file://C:\a\c\myfile.yaml`, + Expected: `C:\a\c\myfile.yaml`, // outcome on all platforms, not only windows + }, + { + Title: "file URI with drive letter and backslashes, as a rooted Windows path", + Path: `file:///C:\a\c\myfile.yaml`, + Expected: `/C:\a\c\myfile.yaml`, // on non-windows, this results most likely in a wrong path + ExpectedWindows: `C:\a\c\myfile.yaml`, // on windows, we know that C: is a drive letter, so /C: becomes C: + }, + { + Title: "file URI with escaped backslashes", + Path: `file://C%3A%5Ca%5Cc%5Cmyfile.yaml`, + Expected: `C:\a\c\myfile.yaml`, // outcome on all platforms, not only windows + }, + { + Title: "file URI with escaped backslashes, rooted", + Path: `file:///C%3A%5Ca%5Cc%5Cmyfile.yaml`, + Expected: `/C:\a\c\myfile.yaml`, // outcome on non-windows (most likely not a desired path) + ExpectedWindows: `C:\a\c\myfile.yaml`, // outcome on windows + }, + { + Title: "URI with the file scheme, host omitted: relative path with extra dots", + Path: `file://./a/c/d/../myfile.yaml`, + Expected: `./a/c/d/../myfile.yaml`, + ExpectedWindows: `a\c\myfile.yaml`, // on windows, extra processing cleans the path + }, + { + Title: "relative URI without the file scheme, rooted path", + Path: `/a/c/myfile.yaml`, + Expected: `/a/c/myfile.yaml`, + ExpectedWindows: `\a\c\myfile.yaml`, // there is no drive letter, this would probably result in a wrong path on Windows + }, + { + Title: "relative URI without the file scheme, relative path", + Path: `a/c/myfile.yaml`, + Expected: `a/c/myfile.yaml`, + ExpectedWindows: `a\c\myfile.yaml`, + }, + { + Title: "relative URI without the file scheme, relative path with dots", + Path: `./a/c/myfile.yaml`, + Expected: `./a/c/myfile.yaml`, + ExpectedWindows: `.\a\c\myfile.yaml`, + }, + { + Title: "relative URI without the file scheme, relative path with extra dots", + Path: `./a/c/../myfile.yaml`, + Expected: `./a/c/../myfile.yaml`, + ExpectedWindows: `.\a\c\..\myfile.yaml`, + }, + { + Title: "relative URI without the file scheme, windows slashed-path with drive letter", + Path: `A:/a/c/myfile.yaml`, + Expected: `A:/a/c/myfile.yaml`, // on non-windows, this results most likely in a wrong path + ExpectedWindows: `A:\a\c\myfile.yaml`, // on windows, slashes are converted + }, + { + Title: "relative URI without the file scheme, windows backslashed-path with drive letter", + Path: `A:\a\c\myfile.yaml`, + Expected: `A:\a\c\myfile.yaml`, // on non-windows, this results most likely in a wrong path + ExpectedWindows: `A:\a\c\myfile.yaml`, + }, + { + Title: "URI with file scheme, host as Windows UNC name", + Path: `file://host/share/folder/myfile.yaml`, + Expected: `host/share/folder/myfile.yaml`, // there is no host component accounted for + ExpectedWindows: `\\host\share\folder\myfile.yaml`, // on windows, the host is interpreted as an UNC host for a file share + }, + // TODO: invalid URI (cannot unescape/parse path) + } { + tc := toPin + t.Run(tc.Title, func(t *testing.T) { + var ( + called bool + pth string + ) + + ld := LoadStrategy("local", loader(&called, &pth), remLoader) + b, err := ld(tc.Path) + if tc.ExpectError { + require.Error(t, err) + assert.True(t, called) + + return + } + + require.NoError(t, err) + assert.True(t, called) + assert.Equal(t, []byte(thisIsIt), b) + + if tc.ExpectedWindows != "" && runtime.GOOS == "windows" { + assert.Equalf(t, tc.ExpectedWindows, pth, + "expected local LoadStrategy(%q) to open: %q (windows)", + tc.Path, tc.ExpectedWindows, + ) + + return + } + + assert.Equalf(t, tc.Expected, pth, + "expected local LoadStrategy(%q) to open: %q (any OS)", + tc.Path, tc.Expected, + ) + }) + } + }) }