feat: disable directory listings for static files (#12229)

Emyrk · web-flow · commit 07cccf90330d · 2024-02-20T15:50:30.000-06:00
* 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.
diff --git 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
@@ -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, "<pre>\n</pre>\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, "<pre>\n</pre>\n", string(body))
+		require.Equal(t, http.StatusNotFound, resp.StatusCode)
 	})
 }
 
diff --git 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
@@ -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
@@ -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")
+}
