From 07cccf90330d40ebca1eac0f8f15e15cdd2bb0fe Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 20 Feb 2024 15:50:30 -0600 Subject: [PATCH] feat: disable directory listings for static files (#12229) * feat: disable directory listings for static files Static file server handles serving static asset files (js, css, etc). The default file server would also list all files in a directory. This has been changed to only serve files. --- coderd/coderd.go | 8 ++++++++ coderd/coderd_test.go | 10 ++-------- site/site.go | 45 +++++++++++++++++++++++++++++++++++++++++-- site/site_slim.go | 15 +++++++++++---- site/site_test.go | 27 ++++++++++++++++++++++++++ 5 files changed, 91 insertions(+), 14 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 7084d76f56..068bec51ea 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1067,6 +1067,14 @@ func New(options *Options) *API { // See globalHTTPSwaggerHandler comment as to why we use a package // global variable here. r.Get("/swagger/*", globalHTTPSwaggerHandler) + } else { + swaggerDisabled := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + httpapi.Write(context.Background(), rw, http.StatusNotFound, codersdk.Response{ + Message: "Swagger documentation is disabled.", + }) + }) + r.Get("/swagger", swaggerDisabled) + r.Get("/swagger/*", swaggerDisabled) } // Add CSP headers to all static assets and pages. CSP headers only affect diff --git a/coderd/coderd_test.go b/coderd/coderd_test.go index a5f91fe6fd..a632b399e8 100644 --- a/coderd/coderd_test.go +++ b/coderd/coderd_test.go @@ -312,12 +312,9 @@ func TestSwagger(t *testing.T) { resp, err := requestWithRetries(ctx, t, client, http.MethodGet, swaggerEndpoint, nil) require.NoError(t, err) - - body, err := io.ReadAll(resp.Body) - require.NoError(t, err) defer resp.Body.Close() - require.Equal(t, "
\n
\n", string(body)) + require.Equal(t, http.StatusNotFound, resp.StatusCode) }) t.Run("doc.json disabled by default", func(t *testing.T) { t.Parallel() @@ -329,12 +326,9 @@ func TestSwagger(t *testing.T) { resp, err := requestWithRetries(ctx, t, client, http.MethodGet, swaggerEndpoint+"/doc.json", nil) require.NoError(t, err) - - body, err := io.ReadAll(resp.Body) - require.NoError(t, err) defer resp.Body.Close() - require.Equal(t, "
\n
\n", string(body)) + require.Equal(t, http.StatusNotFound, resp.StatusCode) }) } diff --git a/site/site.go b/site/site.go index 0476565521..4da69e6b3a 100644 --- a/site/site.go +++ b/site/site.go @@ -102,7 +102,8 @@ func New(opts *Options) *Handler { // Set ETag header to the SHA1 hash of the file contents. name := filePath(r.URL.Path) if name == "" || name == "/" { - // Serve the directory listing. + // Serve the directory listing. This intentionally allows directory listings to + // be served. This file system should not contain anything sensitive. http.FileServer(opts.BinFS).ServeHTTP(rw, r) return } @@ -129,7 +130,15 @@ func New(opts *Options) *Handler { // If-Match and If-None-Match headers on the request properly. http.FileServer(opts.BinFS).ServeHTTP(rw, r) }))) - mux.Handle("/", http.FileServer(http.FS(opts.SiteFS))) + mux.Handle("/", http.FileServer( + http.FS( + // OnlyFiles is a wrapper around the file system that prevents directory + // listings. Directory listings are not required for the site file system, so we + // exclude it as a security measure. In practice, this file system comes from our + // open source code base, but this is considered a best practice for serving + // static files. + OnlyFiles(opts.SiteFS))), + ) buildInfo := codersdk.BuildInfoResponse{ ExternalURL: buildinfo.ExternalURL(), @@ -873,3 +882,35 @@ func applicationNameOrDefault(cfg codersdk.AppearanceConfig) string { } return "Coder" } + +// OnlyFiles returns a new fs.FS that only contains files. If a directory is +// requested, os.ErrNotExist is returned. This prevents directory listings from +// being served. +func OnlyFiles(files fs.FS) fs.FS { + return justFilesSystem{FS: files} +} + +type justFilesSystem struct { + FS fs.FS +} + +func (jfs justFilesSystem) Open(name string) (fs.File, error) { + f, err := jfs.FS.Open(name) + if err != nil { + return nil, err + } + + stat, err := f.Stat() + if err != nil { + return nil, err + } + + // Returning a 404 here does prevent the http.FileServer from serving + // index.* files automatically. Coder handles this above as all index pages + // are considered template files. So we never relied on this behavior. + if stat.IsDir() { + return nil, os.ErrNotExist + } + + return f, nil +} diff --git a/site/site_slim.go b/site/site_slim.go index 414da032fc..82cbd7dd4d 100644 --- a/site/site_slim.go +++ b/site/site_slim.go @@ -4,12 +4,19 @@ package site import ( - "embed" "io/fs" + "testing/fstest" + "time" ) -var slim embed.FS - func FS() fs.FS { - return slim + // This is required to contain an index.html file for unit tests. + // Our unit tests frequently just hit `/` and expect to get a 200. + // So a valid index.html file should be expected to be served. + return fstest.MapFS{ + "index.html": &fstest.MapFile{ + Data: []byte("Slim build of Coder, does not contain the frontend static files."), + ModTime: time.Now(), + }, + } } diff --git a/site/site_test.go b/site/site_test.go index b240a065fe..2acf303b2c 100644 --- a/site/site_test.go +++ b/site/site_test.go @@ -18,6 +18,7 @@ import ( "testing/fstest" "time" + "github.com/go-chi/chi/v5" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -659,3 +660,29 @@ func TestRenderStaticErrorPageNoStatus(t *testing.T) { require.Contains(t, bodyStr, "Retry") require.Contains(t, bodyStr, d.DashboardURL) } + +func TestJustFilesSystem(t *testing.T) { + t.Parallel() + + tfs := fstest.MapFS{ + "dir/foo.txt": &fstest.MapFile{ + Data: []byte("hello world"), + }, + "dir/bar.txt": &fstest.MapFile{ + Data: []byte("hello world"), + }, + } + + mux := chi.NewRouter() + mux.Mount("/onlyfiles/", http.StripPrefix("/onlyfiles", http.FileServer(http.FS(site.OnlyFiles(tfs))))) + mux.Mount("/all/", http.StripPrefix("/all", http.FileServer(http.FS(tfs)))) + + // The /all/ endpoint should serve the directory listing. + resp := httptest.NewRecorder() + mux.ServeHTTP(resp, httptest.NewRequest("GET", "/all/dir/", nil)) + require.Equal(t, http.StatusOK, resp.Code, "all serves the directory") + + resp = httptest.NewRecorder() + mux.ServeHTTP(resp, httptest.NewRequest("GET", "/onlyfiles/dir/", nil)) + require.Equal(t, http.StatusNotFound, resp.Code, "onlyfiles does not serve the directory") +}