diff --git a/internal/serving/disk/reader.go b/internal/serving/disk/reader.go index b8dce5421123b670f3807c4cabcb3288872232f0..67a21b85442707e4f57238fefcbc63edb259c665 100644 --- a/internal/serving/disk/reader.go +++ b/internal/serving/disk/reader.go @@ -13,6 +13,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "gitlab.com/gitlab-org/gitlab-pages/internal/serving" + "gitlab.com/gitlab-org/gitlab-pages/internal/serving/fileresolver" ) // Reader is a disk access driver @@ -22,33 +23,6 @@ type Reader struct { func (reader *Reader) tryFile(h serving.Handler) error { fullPath, err := reader.resolvePath(h.LookupPath.Path, h.SubPath) - - request := h.Request - host := request.Host - urlPath := request.URL.Path - - if locationError, _ := err.(*locationDirectoryError); locationError != nil { - if endsWithSlash(urlPath) { - fullPath, err = reader.resolvePath(h.LookupPath.Path, h.SubPath, "index.html") - } else { - // TODO why are we doing that? In tests it redirects to HTTPS. This seems wrong, - // issue about this: https://gitlab.com/gitlab-org/gitlab-pages/issues/273 - - // Concat Host with URL.Path - redirectPath := "//" + host + "/" - redirectPath += strings.TrimPrefix(urlPath, "/") - - // Ensure that there's always "/" at end - redirectPath = strings.TrimSuffix(redirectPath, "/") + "/" - http.Redirect(h.Writer, h.Request, redirectPath, 302) - return nil - } - } - - if locationError, _ := err.(*locationFileNoExtensionError); locationError != nil { - fullPath, err = reader.resolvePath(h.LookupPath.Path, strings.TrimSuffix(h.SubPath, "/")+".html") - } - if err != nil { return err } @@ -71,31 +45,12 @@ func (reader *Reader) tryNotFound(h serving.Handler) error { // Resolve the HTTP request to a path on disk, converting requests for // directories to requests for index.html inside the directory if appropriate. -func (reader *Reader) resolvePath(publicPath string, subPath ...string) (string, error) { - // Ensure that publicPath always ends with "/" - publicPath = strings.TrimSuffix(publicPath, "/") + "/" - - // Don't use filepath.Join as cleans the path, - // where we want to traverse full path as supplied by user - // (including ..) - testPath := publicPath + strings.Join(subPath, "/") - fullPath, err := filepath.EvalSymlinks(testPath) - +func (reader *Reader) resolvePath(publicPath, subPath string) (string, error) { + fullPath, err := fileresolver.ResolveFilePath(publicPath, subPath, filepath.EvalSymlinks) if err != nil { - if endsWithoutHTMLExtension(testPath) { - return "", &locationFileNoExtensionError{ - FullPath: fullPath, - } - } - return "", err } - // The requested path resolved to somewhere outside of the public/ directory - if !strings.HasPrefix(fullPath, publicPath) && fullPath != filepath.Clean(publicPath) { - return "", fmt.Errorf("%q should be in %q", fullPath, publicPath) - } - fi, err := os.Lstat(fullPath) if err != nil { return "", err diff --git a/internal/serving/fileresolver/fileresolver.go b/internal/serving/fileresolver/fileresolver.go new file mode 100644 index 0000000000000000000000000000000000000000..9eb6822ae5e3472b16cc3099bacbec8a9fce9eb2 --- /dev/null +++ b/internal/serving/fileresolver/fileresolver.go @@ -0,0 +1,89 @@ +package fileresolver + +import ( + "errors" + "path/filepath" + "strings" +) + +var ( + errIsDirectory = errors.New("location error accessing directory where file expected") + errNoExtension = errors.New("error accessing a path without an extension") + errFileNotFound = errors.New("file not found") + errNotRegularFile = errors.New("not a regular file") + errFileNotInPublicDir = errors.New("file found outside of public directory") +) + +type evalSymlinkFunc func(string) (string, error) + +// ResolveFilePath takes a archivePath and any subPath to determine the file location. +// Requires the original requestURLPath to try to resolve index.html +// Requires an evalSymlinkFunc to determine if the file exists or not. Useful for resolving files in disk +func ResolveFilePath(lookupPath, subPath string, evalSymLink evalSymlinkFunc) (string, error) { + fullPath, err := resolvePath(evalSymLink, lookupPath, subPath) + if err != nil { + if err == errIsDirectory { + // try to resolve index.html from the path we're currently in + return resolvePath(evalSymLink, lookupPath, subPath, "index.html") + } else if err == errNoExtension { + // assume .html extension and try to resolve + return resolvePath(evalSymLink, lookupPath, strings.TrimSuffix(subPath, "/")+".html") + } + + return "", err + } + + return fullPath, nil +} + +// Resolve the HTTP request to a path on disk, converting requests for +// directories to requests for index.html inside the directory if appropriate. +// Takes a `evalSymLinkFunc` to try to follow any symlinks. For disk use `filepath.EvalSymlinks`. +// Returns the resolved fullPath, fileName (filepath.Base) and error +func resolvePath(evalSymLink evalSymlinkFunc, publicPath string, subPath ...string) (string, error) { + // Ensure that publicPath always ends with "/" + publicPath = strings.TrimSuffix(publicPath, "/") + "/" + + // Don't use filepath.Join as cleans the path, + // where we want to traverse full path as supplied by user + // (including ..) + testPath := publicPath + strings.Join(cleanEmpty(subPath), "/") + if endsWithSlash(testPath) { + return "", errIsDirectory + } else if endsWithoutHTMLExtension(testPath) { + return "", errNoExtension + } + + fullPath, err := evalSymLink(testPath) + if err != nil { + return "", errFileNotFound + } + + // The requested path resolved to somewhere outside of the public/ directory + if !strings.HasPrefix(fullPath, publicPath) && fullPath != filepath.Clean(publicPath) { + return "", errFileNotInPublicDir + } + + return fullPath, nil +} + +func endsWithSlash(path string) bool { + return strings.HasSuffix(path, "/") +} + +func endsWithoutHTMLExtension(path string) bool { + return !strings.HasSuffix(path, ".html") +} + +// cleanEmpty removes empty string elements in the slice +func cleanEmpty(in []string) []string { + var out []string + + for _, x := range in { + if x != "" { + out = append(out, x) + } + } + + return out +} diff --git a/internal/serving/fileresolver/fileresolver_test.go b/internal/serving/fileresolver/fileresolver_test.go new file mode 100644 index 0000000000000000000000000000000000000000..b78317c9d2717a8402d1418027e97a2855fec539 --- /dev/null +++ b/internal/serving/fileresolver/fileresolver_test.go @@ -0,0 +1,138 @@ +package fileresolver + +import ( + "archive/zip" + "io/ioutil" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestResolveFilePathFromDisk(t *testing.T) { + cleanup := setUpTests(t) + defer cleanup() + + tests := []struct { + name string + lookupPath string + subPath string + expectedFullPath string + expectedContent string + expectedErr error + }{ + { + name: "file_exists_with_subpath_and_extension", + lookupPath: "group/group.test.io/public/", + subPath: "index.html", + expectedFullPath: "group/group.test.io/public/index.html", + expectedContent: "main-dir\n", + }, + { + name: "file_exists_without_extension", + lookupPath: "group/group.test.io/public/", + subPath: "index", + expectedFullPath: "group/group.test.io/public/index.html", + expectedContent: "main-dir\n", + }, + { + name: "file_exists_without_subpath", + lookupPath: "group/group.test.io/public/", + subPath: "", + expectedFullPath: "group/group.test.io/public/index.html", + expectedContent: "main-dir\n", + }, + { + name: "file_does_not_exist_without_subpath", + lookupPath: "group.no.projects/", + subPath: "", + expectedErr: errFileNotFound, + }, + { + name: "file_does_not_exist", + lookupPath: "group/group.test.io/public/", + subPath: "unknown_file.html", + expectedErr: errFileNotFound, + }, + { + name: "symlink_inside_public", + lookupPath: "group/symlink/public/", + subPath: "index.html", + expectedFullPath: "group/symlink/public/content/index.html", + expectedContent: "group/symlink/public/content/index.html\n", + }, + { + name: "symlink_outside_of_public_dir", + lookupPath: "group/symlink/public/", + subPath: "outside.html", + expectedErr: errFileNotInPublicDir, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fullPath, err := ResolveFilePath(tt.lookupPath, tt.subPath, filepath.EvalSymlinks) + if tt.expectedErr != nil { + require.Equal(t, tt.expectedErr, err) + return + } + + require.Equal(t, tt.expectedFullPath, fullPath) + + file, err := openFSFile(fullPath) + require.NoError(t, err) + defer file.Close() + + content, err := ioutil.ReadAll(file) + require.NoError(t, err) + require.Contains(t, string(content), tt.expectedContent) + }) + } +} + +func setUpTests(t *testing.T) func() { + t.Helper() + + return chdirInPath(t, "../../../shared/pages") +} + +func chdirInPath(t *testing.T, path string) func() { + t.Helper() + + cwd, err := os.Getwd() + require.NoError(t, err, "Cannot Getwd") + + err = os.Chdir(path) + require.NoError(t, err, "Cannot Chdir") + + return func() { + err := os.Chdir(cwd) + require.NoError(t, err, "Cannot Chdir in cleanup") + } +} + +func openZipFile(t *testing.T, fullPath string, archive *zip.Reader) (*zip.File, error) { + t.Helper() + + return nil, nil +} +func openFSFile(fullPath string) (*os.File, error) { + fi, err := os.Lstat(fullPath) + if err != nil { + return nil, errFileNotFound + } + + // The requested path is a directory, so try index.html via recursion + if fi.IsDir() { + return nil, errIsDirectory + } + + // The file exists, but is not a supported type to serve. Perhaps a block + // special device or something else that may be a security risk. + if !fi.Mode().IsRegular() { + return nil, errNotRegularFile + } + + return os.Open(fullPath) +} diff --git a/internal/source/disk/domain_test.go b/internal/source/disk/domain_test.go index efe450430fd034aa17e78fdfba9b24722b9da3de..1cf0365797cafd2eafee59af120a88b8eed5c759 100644 --- a/internal/source/disk/domain_test.go +++ b/internal/source/disk/domain_test.go @@ -48,12 +48,14 @@ func testGroupServeHTTPHost(t *testing.T, host string) { require.HTTPBodyContains(t, serve, "GET", makeURL("/"), nil, "main-dir") require.HTTPBodyContains(t, serve, "GET", makeURL("/index"), nil, "main-dir") require.HTTPBodyContains(t, serve, "GET", makeURL("/index.html"), nil, "main-dir") - testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project"), nil, "//"+host+"/project/") + // testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project"), nil, "//"+host+"/project/") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project"), nil, "project-subdir") require.HTTPBodyContains(t, serve, "GET", makeURL("/project/"), nil, "project-subdir") require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index"), nil, "project-subdir") require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index/"), nil, "project-subdir") require.HTTPBodyContains(t, serve, "GET", makeURL("/project/index.html"), nil, "project-subdir") - testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project/subdir"), nil, "//"+host+"/project/subdir/") + // testhelpers.AssertRedirectTo(t, serve, "GET", makeURL("/project/subdir"), nil, "//"+host+"/project/subdir/") + require.HTTPBodyContains(t, serve, "GET", makeURL("/project/subdir"), nil, "project-subsubdir") require.HTTPBodyContains(t, serve, "GET", makeURL("/project/subdir/"), nil, "project-subsubdir") require.HTTPBodyContains(t, serve, "GET", makeURL("/project2/"), nil, "project2-main") require.HTTPBodyContains(t, serve, "GET", makeURL("/project2/index"), nil, "project2-main") @@ -90,9 +92,8 @@ func TestDomainServeHTTP(t *testing.T) { require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/", nil, "project2-main") require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/index.html", nil, "project2-main") - require.HTTPRedirect(t, serveFileOrNotFound(testDomain), "GET", "/subdir", nil) - require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/subdir", nil, - `Found`) + // require.HTTPRedirect(t, serveFileOrNotFound(testDomain), "GET", "/subdir", nil) + require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/subdir", nil, "project2-subdir") require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/subdir/", nil, "project2-subdir") require.HTTPBodyContains(t, serveFileOrNotFound(testDomain), "GET", "/subdir/index.html", nil, "project2-subdir") require.HTTPError(t, serveFileOrNotFound(testDomain), "GET", "//about.gitlab.com/%2e%2e", nil) @@ -268,8 +269,10 @@ func TestGroupServeHTTPGzip(t *testing.T) { } for _, tt := range testSet { - URL := "http://group.test.io" + tt.url - testHTTPGzip(t, serveFileOrNotFound(testGroup), tt.mode, URL, nil, tt.acceptEncoding, tt.body, tt.contentType, tt.ungzip) + t.Run(tt.url, func(t *testing.T) { + URL := "http://group.test.io" + tt.url + testHTTPGzip(t, serveFileOrNotFound(testGroup), tt.mode, URL, nil, tt.acceptEncoding, tt.body, tt.contentType, tt.ungzip) + }) } } diff --git a/shared/pages/group/symlink/public/content/index.html b/shared/pages/group/symlink/public/content/index.html new file mode 100644 index 0000000000000000000000000000000000000000..4ff00de637be6596f777b8f0103297218090d415 --- /dev/null +++ b/shared/pages/group/symlink/public/content/index.html @@ -0,0 +1 @@ +group/symlink/public/content/index.html diff --git a/shared/pages/group/symlink/public/index.html b/shared/pages/group/symlink/public/index.html new file mode 120000 index 0000000000000000000000000000000000000000..ea8c18b652291c37ea694a6a7e69112cac75e47f --- /dev/null +++ b/shared/pages/group/symlink/public/index.html @@ -0,0 +1 @@ +./content/index.html \ No newline at end of file diff --git a/shared/pages/group/symlink/public/outside.html b/shared/pages/group/symlink/public/outside.html new file mode 120000 index 0000000000000000000000000000000000000000..0fa189033938ff48792926f4ebac10e8c28dea28 --- /dev/null +++ b/shared/pages/group/symlink/public/outside.html @@ -0,0 +1 @@ +../../serving/public/index.html \ No newline at end of file