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, + ) + }) + } + }) }