From 064086dcab723c58f5fafbe9bfd86fa9184914f8 Mon Sep 17 00:00:00 2001 From: hughcube Date: Fri, 13 Nov 2026 23:03:23 +0800 Subject: [PATCH 1/6] feat(ingress): add --file to serve a local directory Adds a built-in read-only file server origin, so a local directory can be shared through a tunnel without running a separate web server: cloudflared tunnel --file ./public The same origin is available from a configuration file: ingress: - hostname: files.example.com service: file:/abs/path The origin never listens on a public interface: cloudflared starts an in-process HTTP server on 127.0.0.1:0 and points the embedded httpService at it, the same way --hello-world does. Hardening, deliberately different from http.FileServer defaults: - GET / HEAD only, everything else 405 - no directory listings (directories are 404) - dotfiles and dot directories are always 404 - symlinks are resolved and must stay inside the root - X-Content-Type-Options: nosniff - --file is mutually exclusive with --url and --unix-socket Tests cover traversal (raw and percent-encoded), dotfiles, directory listings, symlink escape, method filtering and invalid roots. Refs https://github.com/cloudflare/cloudflared/issues/611 --- cmd/cloudflared/tunnel/cmd.go | 8 +- cmd/cloudflared/tunnel/quick_tunnel.go | 3 + ingress/ingress.go | 14 ++- ingress/origin_file.go | 139 +++++++++++++++++++++++++ ingress/origin_file_test.go | 120 +++++++++++++++++++++ 5 files changed, 282 insertions(+), 2 deletions(-) create mode 100644 ingress/origin_file.go create mode 100644 ingress/origin_file_test.go diff --git a/cmd/cloudflared/tunnel/cmd.go b/cmd/cloudflared/tunnel/cmd.go index 8896c8d800e..23f6058eafa 100644 --- a/cmd/cloudflared/tunnel/cmd.go +++ b/cmd/cloudflared/tunnel/cmd.go @@ -252,7 +252,7 @@ func TunnelCommand(c *cli.Context) error { // Run a quick tunnel // A unauthenticated named tunnel hosted on ..com - shouldRunQuickTunnel := c.IsSet("url") || c.IsSet(ingress.HelloWorldFlag) + shouldRunQuickTunnel := c.IsSet("url") || c.IsSet(ingress.HelloWorldFlag) || c.IsSet(ingress.FileFlag) if c.String("quick-service") != "" && shouldRunQuickTunnel { return RunQuickTunnel(sc) } @@ -967,6 +967,12 @@ func configureProxyFlags(shouldHide bool) []cli.Flag { EnvVars: []string{"TUNNEL_URL"}, Hidden: shouldHide, }), + altsrc.NewStringFlag(&cli.StringFlag{ + Name: ingress.FileFlag, + Usage: "Serve the local directory at PATH read-only instead of connecting to --url.", + EnvVars: []string{"TUNNEL_FILE"}, + Hidden: shouldHide, + }), altsrc.NewBoolFlag(&cli.BoolFlag{ Name: ingress.HelloWorldFlag, Value: false, diff --git a/cmd/cloudflared/tunnel/quick_tunnel.go b/cmd/cloudflared/tunnel/quick_tunnel.go index 805b2ba9cfd..ba88bc0942e 100644 --- a/cmd/cloudflared/tunnel/quick_tunnel.go +++ b/cmd/cloudflared/tunnel/quick_tunnel.go @@ -50,6 +50,9 @@ func describeQuickTunnelLocalOrigin(c *cli.Context, hasConfiguredIngress bool) ( if c.IsSet(ingress.HelloWorldFlag) { return quickTunnelHelloWorldOrigin, nil } + if c.IsSet(ingress.FileFlag) { + return "file:" + c.String(ingress.FileFlag), nil + } originURL, err := config.ValidateUrl(c, false) if err != nil { diff --git a/ingress/ingress.go b/ingress/ingress.go index f5758fc91d3..b92179ce12a 100644 --- a/ingress/ingress.go +++ b/ingress/ingress.go @@ -276,6 +276,12 @@ func newDefaultOrigin(c *cli.Context, log *zerolog.Logger) Ingress { // Get a single origin service from the CLI/config. func parseSingleOriginService(c *cli.Context, allowURLFromArgs bool) (OriginService, error) { + if c.IsSet(FileFlag) { + if c.IsSet("url") || c.IsSet("unix-socket") { + return nil, errors.New("--file must be used exclusively: it conflicts with --url and --unix-socket") + } + return newFileServer(c.String(FileFlag)) + } if c.IsSet(HelloWorldFlag) { return new(helloWorld), nil } @@ -363,7 +369,13 @@ func validateIngress(ingress []config.UnvalidatedIngressRule, defaults OriginReq cfg := setConfig(defaults, r.OriginRequest) var service OriginService - if prefix := "unix:"; strings.HasPrefix(r.Service, prefix) { + if prefix := "file:"; strings.HasPrefix(r.Service, prefix) { + fileServer, err := newFileServer(strings.TrimPrefix(r.Service, prefix)) + if err != nil { + return Ingress{}, err + } + service = fileServer + } else if prefix := "unix:"; strings.HasPrefix(r.Service, prefix) { // No validation necessary for unix socket filepath services path := strings.TrimPrefix(r.Service, prefix) service = &unixSocketPath{path: path, scheme: "http"} diff --git a/ingress/origin_file.go b/ingress/origin_file.go new file mode 100644 index 00000000000..c824ed75849 --- /dev/null +++ b/ingress/origin_file.go @@ -0,0 +1,139 @@ +package ingress + +import ( + "encoding/json" + "fmt" + "net" + "net/http" + "net/url" + "os" + "path" + "path/filepath" + "strings" + "time" + + "github.com/pkg/errors" + "github.com/rs/zerolog" +) + +// FileFlag is the CLI flag used to serve a local directory through the tunnel. +const FileFlag = "file" + +// fileServer is an OriginService that serves a local directory read-only. +// +// It starts an in-process HTTP server bound to loopback and points the embedded +// httpService at it, in the same way helloWorld does. Nothing is ever written. +type fileServer struct { + httpService + root string + server net.Listener +} + +func newFileServer(dir string) (*fileServer, error) { + if dir == "" { + return nil, errors.New("file service requires a directory") + } + abs, err := filepath.Abs(dir) + if err != nil { + return nil, errors.Wrap(err, "invalid directory") + } + root, err := filepath.EvalSymlinks(abs) + if err != nil { + return nil, errors.Wrap(err, "cannot resolve directory") + } + info, err := os.Stat(root) + if err != nil { + return nil, errors.Wrap(err, "cannot stat directory") + } + if !info.IsDir() { + return nil, fmt.Errorf("%s is not a directory", root) + } + return &fileServer{root: root}, nil +} + +func (o *fileServer) String() string { return "file:" + o.root } + +func (o fileServer) MarshalJSON() ([]byte, error) { return json.Marshal(o.String()) } + +func (o *fileServer) start(log *zerolog.Logger, shutdownC <-chan struct{}, cfg OriginRequestConfig) error { + if err := o.httpService.start(log, shutdownC, cfg); err != nil { + return err + } + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + return errors.Wrap(err, "cannot start local file server") + } + o.server = listener + server := &http.Server{ + Handler: o, + ReadHeaderTimeout: 10 * time.Second, + } + go func() { _ = server.Serve(listener) }() + go func() { + <-shutdownC + _ = listener.Close() + }() + o.httpService.url = &url.URL{Scheme: "http", Host: listener.Addr().String()} + log.Info(). + Str("root", o.root). + Str("addr", listener.Addr().String()). + Msg("Serving local directory") + return nil +} + +// ServeHTTP serves files under root. It is deliberately stricter than +// http.FileServer: no directory listings, no dotfiles, no symlink escapes and +// only GET/HEAD. +func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet && r.Method != http.MethodHead { + w.Header().Set("Allow", "GET, HEAD") + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + + rel := strings.TrimPrefix(path.Clean("/"+r.URL.Path), "/") + if rel == "" { + rel = "index.html" + } + for _, segment := range strings.Split(rel, "/") { + if segment == "" || segment == "." || segment == ".." || strings.HasPrefix(segment, ".") { + http.NotFound(w, r) + return + } + } + + target := filepath.Join(o.root, filepath.FromSlash(rel)) + resolved, err := filepath.EvalSymlinks(target) + if err != nil { + http.NotFound(w, r) + return + } + if !isWithin(o.root, resolved) { + http.NotFound(w, r) + return + } + + file, err := os.Open(resolved) + if err != nil { + http.NotFound(w, r) + return + } + defer file.Close() + + info, err := file.Stat() + if err != nil || info.IsDir() { + http.NotFound(w, r) + return + } + + w.Header().Set("X-Content-Type-Options", "nosniff") + http.ServeContent(w, r, info.Name(), info.ModTime(), file) +} + +func isWithin(root, target string) bool { + rel, err := filepath.Rel(root, target) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) +} diff --git a/ingress/origin_file_test.go b/ingress/origin_file_test.go new file mode 100644 index 00000000000..bd0461139c2 --- /dev/null +++ b/ingress/origin_file_test.go @@ -0,0 +1,120 @@ +package ingress + +import ( + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestFileServerServesFiles(t *testing.T) { + root := t.TempDir() + mustWrite(t, filepath.Join(root, "index.html"), "INDEX") + mustWrite(t, filepath.Join(root, "a.txt"), "AAA") + if err := os.MkdirAll(filepath.Join(root, "sub"), 0o700); err != nil { + t.Fatal(err) + } + mustWrite(t, filepath.Join(root, "sub", "b.txt"), "BBB") + + svc, err := newFileServer(root) + if err != nil { + t.Fatalf("newFileServer: %v", err) + } + if !strings.HasPrefix(svc.String(), "file:") { + t.Fatalf("unexpected String(): %s", svc.String()) + } + + cases := []struct { + name string + path string + code int + body string + }{ + {"root serves index", "/", 200, "INDEX"}, + {"plain file", "/a.txt", 200, "AAA"}, + {"nested file", "/sub/b.txt", 200, "BBB"}, + {"missing", "/nope.txt", 404, ""}, + } + for _, c := range cases { + rec := httptest.NewRecorder() + svc.ServeHTTP(rec, httptest.NewRequest("GET", "http://example.com"+c.path, nil)) + if rec.Code != c.code { + t.Errorf("%s: code=%d want %d", c.name, rec.Code, c.code) + } + if c.body != "" && !strings.Contains(rec.Body.String(), c.body) { + t.Errorf("%s: body=%q", c.name, rec.Body.String()) + } + } +} + +func TestFileServerRejectsUnsafePaths(t *testing.T) { + root := t.TempDir() + mustWrite(t, filepath.Join(root, "a.txt"), "AAA") + mustWrite(t, filepath.Join(root, ".env"), "SECRET=1") + if err := os.MkdirAll(filepath.Join(root, "sub"), 0o700); err != nil { + t.Fatal(err) + } + outside := t.TempDir() + mustWrite(t, filepath.Join(outside, "secret.txt"), "OUTSIDE") + linkErr := os.Symlink(filepath.Join(outside, "secret.txt"), filepath.Join(root, "evil.txt")) + + svc, err := newFileServer(root) + if err != nil { + t.Fatalf("newFileServer: %v", err) + } + + paths := []struct { + name string + method string + path string + want int + }{ + {"dotdot", "GET", "/../secret.txt", 404}, + {"dotdot encoded", "GET", "/%2e%2e/secret.txt", 404}, + {"dotfile", "GET", "/.env", 404}, + {"dot directory", "GET", "/.git/config", 404}, + {"directory listing", "GET", "/sub/", 404}, + {"post not allowed", "POST", "/a.txt", 405}, + {"put not allowed", "PUT", "/a.txt", 405}, + } + for _, p := range paths { + rec := httptest.NewRecorder() + svc.ServeHTTP(rec, httptest.NewRequest(p.method, "http://example.com"+p.path, nil)) + if rec.Code != p.want { + t.Errorf("%s: code=%d want %d (body=%q)", p.name, rec.Code, p.want, rec.Body.String()) + } + } + + if linkErr != nil { + t.Skipf("symlink unavailable: %v", linkErr) + } + rec := httptest.NewRecorder() + svc.ServeHTTP(rec, httptest.NewRequest("GET", "http://example.com/evil.txt", nil)) + if rec.Code != 404 { + t.Errorf("symlink escape: code=%d want 404", rec.Code) + } +} + +func TestNewFileServerRejectsInvalidRoot(t *testing.T) { + root := t.TempDir() + file := filepath.Join(root, "f.txt") + mustWrite(t, file, "x") + + if _, err := newFileServer(file); err == nil { + t.Error("expected error for a file, got nil") + } + if _, err := newFileServer(filepath.Join(root, "missing")); err == nil { + t.Error("expected error for a missing path, got nil") + } + if _, err := newFileServer(""); err == nil { + t.Error("expected error for an empty path, got nil") + } +} + +func mustWrite(t *testing.T, path, content string) { + t.Helper() + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatal(err) + } +} From 5be4cc2b1324002a68c1695bf125f26e8ad2de14 Mon Sep 17 00:00:00 2001 From: hughcube Date: Fri, 13 Nov 2026 23:20:32 +0800 Subject: [PATCH 2/6] fix(ingress): harden the file origin after review - reject path components with trailing dots or spaces: Windows strips them, so "/a.txt." used to alias "a.txt" - set IdleTimeout on the in-process server so a slow client cannot pin a connection indefinitely - move the filesystem checks from parsing to start(), so validating a configuration file no longer requires the directory to exist on the machine doing the validation - print the resolved absolute directory in the quick tunnel banner instead of the raw flag value - add tests for CLI flag parsing, the config service form, flag exclusivity and the trailing-dot alias --- cmd/cloudflared/tunnel/quick_tunnel.go | 7 ++- ingress/origin_file.go | 35 ++++++++--- ingress/origin_file_flag_test.go | 80 ++++++++++++++++++++++++++ ingress/origin_file_test.go | 55 ++++++++++++------ 4 files changed, 153 insertions(+), 24 deletions(-) create mode 100644 ingress/origin_file_flag_test.go diff --git a/cmd/cloudflared/tunnel/quick_tunnel.go b/cmd/cloudflared/tunnel/quick_tunnel.go index ba88bc0942e..bdcd9ff9f58 100644 --- a/cmd/cloudflared/tunnel/quick_tunnel.go +++ b/cmd/cloudflared/tunnel/quick_tunnel.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "path/filepath" "strings" "time" @@ -51,7 +52,11 @@ func describeQuickTunnelLocalOrigin(c *cli.Context, hasConfiguredIngress bool) ( return quickTunnelHelloWorldOrigin, nil } if c.IsSet(ingress.FileFlag) { - return "file:" + c.String(ingress.FileFlag), nil + dir, err := filepath.Abs(c.String(ingress.FileFlag)) + if err != nil { + return "", fmt.Errorf("resolve --file directory: %w", err) + } + return "file:" + dir, nil } originURL, err := config.ValidateUrl(c, false) diff --git a/ingress/origin_file.go b/ingress/origin_file.go index c824ed75849..2dfaea8aef7 100644 --- a/ingress/origin_file.go +++ b/ingress/origin_file.go @@ -29,26 +29,37 @@ type fileServer struct { server net.Listener } +// newFileServer records the directory to serve. +// +// The path is only made absolute here. Resolving and validating it happens in +// start(), so that validating a configuration file does not require the +// directory to exist on the machine doing the validation. func newFileServer(dir string) (*fileServer, error) { - if dir == "" { + if strings.TrimSpace(dir) == "" { return nil, errors.New("file service requires a directory") } abs, err := filepath.Abs(dir) if err != nil { return nil, errors.Wrap(err, "invalid directory") } - root, err := filepath.EvalSymlinks(abs) + return &fileServer{root: abs}, nil +} + +// resolveRoot resolves symlinks and checks that root is an existing directory. +func (o *fileServer) resolveRoot() error { + resolved, err := filepath.EvalSymlinks(o.root) if err != nil { - return nil, errors.Wrap(err, "cannot resolve directory") + return errors.Wrapf(err, "cannot resolve directory %s", o.root) } - info, err := os.Stat(root) + info, err := os.Stat(resolved) if err != nil { - return nil, errors.Wrap(err, "cannot stat directory") + return errors.Wrapf(err, "cannot stat directory %s", resolved) } if !info.IsDir() { - return nil, fmt.Errorf("%s is not a directory", root) + return fmt.Errorf("%s is not a directory", resolved) } - return &fileServer{root: root}, nil + o.root = resolved + return nil } func (o *fileServer) String() string { return "file:" + o.root } @@ -56,6 +67,9 @@ func (o *fileServer) String() string { return "file:" + o.root } func (o fileServer) MarshalJSON() ([]byte, error) { return json.Marshal(o.String()) } func (o *fileServer) start(log *zerolog.Logger, shutdownC <-chan struct{}, cfg OriginRequestConfig) error { + if err := o.resolveRoot(); err != nil { + return err + } if err := o.httpService.start(log, shutdownC, cfg); err != nil { return err } @@ -67,6 +81,7 @@ func (o *fileServer) start(log *zerolog.Logger, shutdownC <-chan struct{}, cfg O server := &http.Server{ Handler: o, ReadHeaderTimeout: 10 * time.Second, + IdleTimeout: 2 * time.Minute, } go func() { _ = server.Serve(listener) }() go func() { @@ -100,6 +115,12 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } + // Windows silently strips trailing dots and spaces from path + // components, so "/a.txt." would alias "a.txt". Reject instead. + if strings.TrimRight(segment, ". ") != segment { + http.NotFound(w, r) + return + } } target := filepath.Join(o.root, filepath.FromSlash(rel)) diff --git a/ingress/origin_file_flag_test.go b/ingress/origin_file_flag_test.go new file mode 100644 index 00000000000..b7f41634e53 --- /dev/null +++ b/ingress/origin_file_flag_test.go @@ -0,0 +1,80 @@ +package ingress + +import ( + "flag" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v2" + + "github.com/cloudflare/cloudflared/config" +) + +func newFileFlagContext(t *testing.T, args ...string) *cli.Context { + t.Helper() + flagSet := flag.NewFlagSet(t.Name(), flag.ContinueOnError) + for _, cliFlag := range []cli.Flag{ + &cli.StringFlag{Name: FileFlag}, + &cli.StringFlag{Name: "url"}, + &cli.StringFlag{Name: "unix-socket"}, + &cli.StringFlag{Name: "hostname"}, + } { + require.NoError(t, cliFlag.Apply(flagSet)) + } + require.NoError(t, flagSet.Parse(args)) + return cli.NewContext(cli.NewApp(), flagSet, nil) +} + +func TestParseCLIIngressFileFlag(t *testing.T) { + dir := t.TempDir() + ing, err := parseCLIIngress(newFileFlagContext(t, "--"+FileFlag, dir), false) + require.NoError(t, err) + require.Len(t, ing.Rules, 1) + + svc, ok := ing.Rules[0].Service.(*fileServer) + require.Truef(t, ok, "expected *fileServer, got %T", ing.Rules[0].Service) + + abs, err := filepath.Abs(dir) + require.NoError(t, err) + require.Equal(t, abs, svc.root) + require.Equal(t, "file:"+abs, svc.String()) +} + +func TestFileFlagIsExclusive(t *testing.T) { + dir := t.TempDir() + + cases := map[string][]string{ + "with --url": {"--" + FileFlag, dir, "--url", "http://localhost:8080"}, + "with --unix-socket": {"--" + FileFlag, dir, "--unix-socket", "/tmp/sock"}, + } + for name, args := range cases { + t.Run(name, func(t *testing.T) { + _, err := parseCLIIngress(newFileFlagContext(t, args...), false) + require.Error(t, err) + }) + } +} + +func TestValidateIngressFileService(t *testing.T) { + dir := t.TempDir() + ing, err := validateIngress([]config.UnvalidatedIngressRule{ + {Service: "file:" + dir}, + }, OriginRequestConfig{}) + require.NoError(t, err) + require.Len(t, ing.Rules, 1) + + svc, ok := ing.Rules[0].Service.(*fileServer) + require.Truef(t, ok, "expected *fileServer, got %T", ing.Rules[0].Service) + + abs, err := filepath.Abs(dir) + require.NoError(t, err) + require.Equal(t, abs, svc.root) +} + +func TestValidateIngressFileServiceRejectsEmptyPath(t *testing.T) { + _, err := validateIngress([]config.UnvalidatedIngressRule{ + {Service: "file:"}, + }, OriginRequestConfig{}) + require.Error(t, err) +} diff --git a/ingress/origin_file_test.go b/ingress/origin_file_test.go index bd0461139c2..7987a07d4bc 100644 --- a/ingress/origin_file_test.go +++ b/ingress/origin_file_test.go @@ -10,17 +10,20 @@ import ( func TestFileServerServesFiles(t *testing.T) { root := t.TempDir() - mustWrite(t, filepath.Join(root, "index.html"), "INDEX") - mustWrite(t, filepath.Join(root, "a.txt"), "AAA") + mustWriteFile(t, filepath.Join(root, "index.html"), "INDEX") + mustWriteFile(t, filepath.Join(root, "a.txt"), "AAA") if err := os.MkdirAll(filepath.Join(root, "sub"), 0o700); err != nil { t.Fatal(err) } - mustWrite(t, filepath.Join(root, "sub", "b.txt"), "BBB") + mustWriteFile(t, filepath.Join(root, "sub", "b.txt"), "BBB") svc, err := newFileServer(root) if err != nil { t.Fatalf("newFileServer: %v", err) } + if err := svc.resolveRoot(); err != nil { + t.Fatalf("resolveRoot: %v", err) + } if !strings.HasPrefix(svc.String(), "file:") { t.Fatalf("unexpected String(): %s", svc.String()) } @@ -50,19 +53,22 @@ func TestFileServerServesFiles(t *testing.T) { func TestFileServerRejectsUnsafePaths(t *testing.T) { root := t.TempDir() - mustWrite(t, filepath.Join(root, "a.txt"), "AAA") - mustWrite(t, filepath.Join(root, ".env"), "SECRET=1") + mustWriteFile(t, filepath.Join(root, "a.txt"), "AAA") + mustWriteFile(t, filepath.Join(root, ".env"), "SECRET=1") if err := os.MkdirAll(filepath.Join(root, "sub"), 0o700); err != nil { t.Fatal(err) } outside := t.TempDir() - mustWrite(t, filepath.Join(outside, "secret.txt"), "OUTSIDE") + mustWriteFile(t, filepath.Join(outside, "secret.txt"), "OUTSIDE") linkErr := os.Symlink(filepath.Join(outside, "secret.txt"), filepath.Join(root, "evil.txt")) svc, err := newFileServer(root) if err != nil { t.Fatalf("newFileServer: %v", err) } + if err := svc.resolveRoot(); err != nil { + t.Fatalf("resolveRoot: %v", err) + } paths := []struct { name string @@ -75,6 +81,8 @@ func TestFileServerRejectsUnsafePaths(t *testing.T) { {"dotfile", "GET", "/.env", 404}, {"dot directory", "GET", "/.git/config", 404}, {"directory listing", "GET", "/sub/", 404}, + {"trailing dot", "GET", "/a.txt.", 404}, + {"trailing dot on dotfile", "GET", "/.env.", 404}, {"post not allowed", "POST", "/a.txt", 405}, {"put not allowed", "PUT", "/a.txt", 405}, } @@ -96,23 +104,38 @@ func TestFileServerRejectsUnsafePaths(t *testing.T) { } } -func TestNewFileServerRejectsInvalidRoot(t *testing.T) { +func TestNewFileServerOnlyChecksSyntax(t *testing.T) { + // Configuration parsing must not touch the filesystem: a config file may be + // validated on a machine where the directory does not exist. + if _, err := newFileServer(""); err == nil { + t.Error("expected error for an empty path, got nil") + } + + missing := filepath.Join(t.TempDir(), "does-not-exist") + svc, err := newFileServer(missing) + if err != nil { + t.Fatalf("newFileServer must not stat the directory, got: %v", err) + } + if err := svc.resolveRoot(); err == nil { + t.Error("resolveRoot must fail for a missing directory") + } +} + +func TestResolveRootRejectsNonDirectory(t *testing.T) { root := t.TempDir() file := filepath.Join(root, "f.txt") - mustWrite(t, file, "x") + mustWriteFile(t, file, "x") - if _, err := newFileServer(file); err == nil { - t.Error("expected error for a file, got nil") - } - if _, err := newFileServer(filepath.Join(root, "missing")); err == nil { - t.Error("expected error for a missing path, got nil") + svc, err := newFileServer(file) + if err != nil { + t.Fatalf("newFileServer: %v", err) } - if _, err := newFileServer(""); err == nil { - t.Error("expected error for an empty path, got nil") + if err := svc.resolveRoot(); err == nil { + t.Error("resolveRoot must reject a regular file") } } -func mustWrite(t *testing.T, path, content string) { +func mustWriteFile(t *testing.T, path, content string) { t.Helper() if err := os.WriteFile(path, []byte(content), 0o600); err != nil { t.Fatal(err) From 28d13c91881f3ddbf9a7c788d7b4c3291e76eb11 Mon Sep 17 00:00:00 2001 From: hughcube Date: Fri, 13 Nov 2026 23:46:52 +0800 Subject: [PATCH 3/6] fix(ingress): close Windows path bypasses in the file origin Red team review found that the segment checks validated the request path while the filesystem resolved it differently: - a backslash separates path components on Windows but is opaque to path.Clean, so "/%5c.env" collapsed into a single segment and bypassed both the dotfile rule and the ".." rule, serving .env and .git/config from a directory that was supposed to hide them - Windows 8.3 short names ("GIT~1") aliased dot directories without any backslash involved - reserved device names ("NUL") resolved successfully and are not regular files Fixes: - reject any request path containing a backslash outright - factor the rules into safeRelativePath and apply it to the request path AND the resolved filesystem path, which catches short names and symlink indirection - reject Windows reserved device names - require a per-instance random header on the loopback listener, so other processes on the machine cannot read the directory by connecting to the port directly; RoundTrip injects it for traffic that came via the tunnel - only serve regular files Tests cover every vector above, plus a request without the loopback header. --- ingress/origin_file.go | 117 ++++++++++++++++++++++++++++++++---- ingress/origin_file_test.go | 83 ++++++++++++++++++------- 2 files changed, 164 insertions(+), 36 deletions(-) diff --git a/ingress/origin_file.go b/ingress/origin_file.go index 2dfaea8aef7..cac831ec123 100644 --- a/ingress/origin_file.go +++ b/ingress/origin_file.go @@ -1,6 +1,9 @@ package ingress import ( + "crypto/rand" + "crypto/subtle" + "encoding/hex" "encoding/json" "fmt" "net" @@ -9,6 +12,7 @@ import ( "os" "path" "path/filepath" + "runtime" "strings" "time" @@ -19,6 +23,10 @@ import ( // FileFlag is the CLI flag used to serve a local directory through the tunnel. const FileFlag = "file" +// localFileAuthHeader marks requests that came through the tunnel proxy rather +// than from another process on the machine listening on the loopback port. +const localFileAuthHeader = "X-Cloudflared-File-Server" + // fileServer is an OriginService that serves a local directory read-only. // // It starts an in-process HTTP server bound to loopback and points the embedded @@ -26,6 +34,7 @@ const FileFlag = "file" type fileServer struct { httpService root string + token string server net.Listener } @@ -42,7 +51,19 @@ func newFileServer(dir string) (*fileServer, error) { if err != nil { return nil, errors.Wrap(err, "invalid directory") } - return &fileServer{root: abs}, nil + token, err := newLocalToken() + if err != nil { + return nil, err + } + return &fileServer{root: abs, token: token}, nil +} + +func newLocalToken() (string, error) { + buf := make([]byte, 16) + if _, err := rand.Read(buf); err != nil { + return "", errors.Wrap(err, "cannot generate loopback token") + } + return hex.EncodeToString(buf), nil } // resolveRoot resolves symlinks and checks that root is an existing directory. @@ -66,6 +87,14 @@ func (o *fileServer) String() string { return "file:" + o.root } func (o fileServer) MarshalJSON() ([]byte, error) { return json.Marshal(o.String()) } +// RoundTrip marks requests that come from the tunnel so the loopback listener +// can tell them apart from other local processes. +func (o *fileServer) RoundTrip(req *http.Request) (*http.Response, error) { + clone := req.Clone(req.Context()) + clone.Header.Set(localFileAuthHeader, o.token) + return o.httpService.RoundTrip(clone) +} + func (o *fileServer) start(log *zerolog.Logger, shutdownC <-chan struct{}, cfg OriginRequestConfig) error { if err := o.resolveRoot(); err != nil { return err @@ -106,21 +135,27 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + // Only the tunnel may talk to this listener. + if subtle.ConstantTimeCompare([]byte(r.Header.Get(localFileAuthHeader)), []byte(o.token)) != 1 { + http.Error(w, "forbidden", http.StatusForbidden) + return + } + + // A backslash is never valid in a URL path. On Windows it is a separator + // for the filesystem but not for path.Clean, so allowing it would let + // "/%5c.env" slip past the segment checks below and resolve to ".env". + if strings.ContainsRune(r.URL.Path, '\\') { + http.NotFound(w, r) + return + } + rel := strings.TrimPrefix(path.Clean("/"+r.URL.Path), "/") if rel == "" { rel = "index.html" } - for _, segment := range strings.Split(rel, "/") { - if segment == "" || segment == "." || segment == ".." || strings.HasPrefix(segment, ".") { - http.NotFound(w, r) - return - } - // Windows silently strips trailing dots and spaces from path - // components, so "/a.txt." would alias "a.txt". Reject instead. - if strings.TrimRight(segment, ". ") != segment { - http.NotFound(w, r) - return - } + if !safeRelativePath(rel) { + http.NotFound(w, r) + return } target := filepath.Join(o.root, filepath.FromSlash(rel)) @@ -134,6 +169,14 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } + // Re-check the resolved path: the filesystem may have expanded the request + // into something the checks above never saw, for example a Windows 8.3 + // short name such as "GIT~1" that aliases ".git". + if resolvedRel, err := filepath.Rel(o.root, resolved); err != nil || !safeRelativePath(resolvedRel) { + http.NotFound(w, r) + return + } + file, err := os.Open(resolved) if err != nil { http.NotFound(w, r) @@ -142,7 +185,7 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { defer file.Close() info, err := file.Stat() - if err != nil || info.IsDir() { + if err != nil || !info.Mode().IsRegular() { http.NotFound(w, r) return } @@ -151,6 +194,54 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.ServeContent(w, r, info.Name(), info.ModTime(), file) } +// safeRelativePath reports whether every component of rel is something we are +// willing to serve. It is applied both to the request path and to the resolved +// filesystem path. +func safeRelativePath(rel string) bool { + for _, segment := range strings.Split(filepath.ToSlash(rel), "/") { + if segment == "" || segment == "." || segment == ".." { + return false + } + // No hidden files or directories. + if strings.HasPrefix(segment, ".") { + return false + } + // Windows silently strips trailing dots and spaces, so "a.txt." would + // alias "a.txt". Rejecting everywhere keeps behaviour identical across + // platforms rather than aliasing on one of them. + if strings.TrimRight(segment, ". ") != segment { + return false + } + if isReservedName(segment) { + return false + } + } + return true +} + +// isReservedName reports Windows device names, which are not regular files and +// can block on open. +func isReservedName(segment string) bool { + if runtime.GOOS != "windows" { + return false + } + name := strings.ToUpper(segment) + if idx := strings.IndexByte(name, '.'); idx >= 0 { + name = name[:idx] + } + switch name { + case "CON", "PRN", "AUX", "NUL": + return true + } + if len(name) == 4 { + prefix, digit := name[:3], name[3] + if digit >= '1' && digit <= '9' && (prefix == "COM" || prefix == "LPT") { + return true + } + } + return false +} + func isWithin(root, target string) bool { rel, err := filepath.Rel(root, target) if err != nil { diff --git a/ingress/origin_file_test.go b/ingress/origin_file_test.go index 7987a07d4bc..559cef8801c 100644 --- a/ingress/origin_file_test.go +++ b/ingress/origin_file_test.go @@ -8,6 +8,28 @@ import ( "testing" ) +// serve issues a request the way the tunnel proxy would: with the loopback +// auth header the file server expects. +func serve(svc *fileServer, method, target string) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, "http://example.com"+target, nil) + req.Header.Set(localFileAuthHeader, svc.token) + rec := httptest.NewRecorder() + svc.ServeHTTP(rec, req) + return rec +} + +func newTestFileServer(t *testing.T, root string) *fileServer { + t.Helper() + svc, err := newFileServer(root) + if err != nil { + t.Fatalf("newFileServer: %v", err) + } + if err := svc.resolveRoot(); err != nil { + t.Fatalf("resolveRoot: %v", err) + } + return svc +} + func TestFileServerServesFiles(t *testing.T) { root := t.TempDir() mustWriteFile(t, filepath.Join(root, "index.html"), "INDEX") @@ -17,13 +39,7 @@ func TestFileServerServesFiles(t *testing.T) { } mustWriteFile(t, filepath.Join(root, "sub", "b.txt"), "BBB") - svc, err := newFileServer(root) - if err != nil { - t.Fatalf("newFileServer: %v", err) - } - if err := svc.resolveRoot(); err != nil { - t.Fatalf("resolveRoot: %v", err) - } + svc := newTestFileServer(t, root) if !strings.HasPrefix(svc.String(), "file:") { t.Fatalf("unexpected String(): %s", svc.String()) } @@ -40,8 +56,7 @@ func TestFileServerServesFiles(t *testing.T) { {"missing", "/nope.txt", 404, ""}, } for _, c := range cases { - rec := httptest.NewRecorder() - svc.ServeHTTP(rec, httptest.NewRequest("GET", "http://example.com"+c.path, nil)) + rec := serve(svc, "GET", c.path) if rec.Code != c.code { t.Errorf("%s: code=%d want %d", c.name, rec.Code, c.code) } @@ -55,22 +70,21 @@ func TestFileServerRejectsUnsafePaths(t *testing.T) { root := t.TempDir() mustWriteFile(t, filepath.Join(root, "a.txt"), "AAA") mustWriteFile(t, filepath.Join(root, ".env"), "SECRET=1") + if err := os.MkdirAll(filepath.Join(root, ".git"), 0o700); err != nil { + t.Fatal(err) + } + mustWriteFile(t, filepath.Join(root, ".git", "config"), "TOKEN=1") if err := os.MkdirAll(filepath.Join(root, "sub"), 0o700); err != nil { t.Fatal(err) } + outside := t.TempDir() mustWriteFile(t, filepath.Join(outside, "secret.txt"), "OUTSIDE") linkErr := os.Symlink(filepath.Join(outside, "secret.txt"), filepath.Join(root, "evil.txt")) - svc, err := newFileServer(root) - if err != nil { - t.Fatalf("newFileServer: %v", err) - } - if err := svc.resolveRoot(); err != nil { - t.Fatalf("resolveRoot: %v", err) - } + svc := newTestFileServer(t, root) - paths := []struct { + cases := []struct { name string method string path string @@ -83,12 +97,16 @@ func TestFileServerRejectsUnsafePaths(t *testing.T) { {"directory listing", "GET", "/sub/", 404}, {"trailing dot", "GET", "/a.txt.", 404}, {"trailing dot on dotfile", "GET", "/.env.", 404}, + {"backslash dotfile", "GET", "/%5c.env", 404}, + {"backslash dot directory", "GET", "/%5c.git%5cconfig", 404}, + {"backslash dotdot", "GET", "/sub%5c..%5c.env", 404}, + {"8.3 short name", "GET", "/GIT~1/config", 404}, + {"reserved device", "GET", "/NUL", 404}, {"post not allowed", "POST", "/a.txt", 405}, {"put not allowed", "PUT", "/a.txt", 405}, } - for _, p := range paths { - rec := httptest.NewRecorder() - svc.ServeHTTP(rec, httptest.NewRequest(p.method, "http://example.com"+p.path, nil)) + for _, p := range cases { + rec := serve(svc, p.method, p.path) if rec.Code != p.want { t.Errorf("%s: code=%d want %d (body=%q)", p.name, rec.Code, p.want, rec.Body.String()) } @@ -97,13 +115,32 @@ func TestFileServerRejectsUnsafePaths(t *testing.T) { if linkErr != nil { t.Skipf("symlink unavailable: %v", linkErr) } - rec := httptest.NewRecorder() - svc.ServeHTTP(rec, httptest.NewRequest("GET", "http://example.com/evil.txt", nil)) - if rec.Code != 404 { + if rec := serve(svc, "GET", "/evil.txt"); rec.Code != 404 { t.Errorf("symlink escape: code=%d want 404", rec.Code) } } +func TestFileServerRequiresLoopbackToken(t *testing.T) { + root := t.TempDir() + mustWriteFile(t, filepath.Join(root, "a.txt"), "AAA") + svc := newTestFileServer(t, root) + + req := httptest.NewRequest("GET", "http://example.com/a.txt", nil) + rec := httptest.NewRecorder() + svc.ServeHTTP(rec, req) + if rec.Code != 403 { + t.Errorf("request without the loopback header: code=%d want 403", rec.Code) + } + + wrong := httptest.NewRequest("GET", "http://example.com/a.txt", nil) + wrong.Header.Set(localFileAuthHeader, "not-the-token") + rec = httptest.NewRecorder() + svc.ServeHTTP(rec, wrong) + if rec.Code != 403 { + t.Errorf("request with a wrong header: code=%d want 403", rec.Code) + } +} + func TestNewFileServerOnlyChecksSyntax(t *testing.T) { // Configuration parsing must not touch the filesystem: a config file may be // validated on a machine where the directory does not exist. From 572e33809063c4ca3ec571e46c1f52632ff3d884 Mon Sep 17 00:00:00 2001 From: hughcube Date: Sat, 14 Nov 2026 00:24:57 +0800 Subject: [PATCH 4/6] fix(ingress): tighter flag exclusivity and mutation-resistant tests Second round of adversarial review found that the first round of tests could not actually fail: on Linux, deleting three of the four path defences left the suite green, because the payloads 404 anyway when the file does not exist. - move the path policy into safeRelativePath and test it directly, so every rule is pinned on every platform instead of relying on filesystem state - make safeRelativePath itself reject backslashes, removing the platform dependency from the rule (the HTTP layer still rejects them early) - add unit tests for reserved device names, which previously had no coverage - --file is now rejected together with --hello-world and --bastion too; before this it silently won because its check came first - call server.Close() on shutdown so keep-alive connections the shared transport may hold are torn down, and drop the write-only listener field - document that only "/" maps to index.html, so "/sub/" is a 404 by design Verified by mutation: deleting the backslash rule now fails TestSafeRelativePath. --- ingress/ingress.go | 4 +-- ingress/origin_file.go | 19 ++++++++--- ingress/origin_file_flag_test.go | 4 +++ ingress/origin_file_policy_test.go | 53 ++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 7 deletions(-) create mode 100644 ingress/origin_file_policy_test.go diff --git a/ingress/ingress.go b/ingress/ingress.go index b92179ce12a..ba0627a32f1 100644 --- a/ingress/ingress.go +++ b/ingress/ingress.go @@ -277,8 +277,8 @@ func newDefaultOrigin(c *cli.Context, log *zerolog.Logger) Ingress { // Get a single origin service from the CLI/config. func parseSingleOriginService(c *cli.Context, allowURLFromArgs bool) (OriginService, error) { if c.IsSet(FileFlag) { - if c.IsSet("url") || c.IsSet("unix-socket") { - return nil, errors.New("--file must be used exclusively: it conflicts with --url and --unix-socket") + if c.IsSet("url") || c.IsSet("unix-socket") || c.IsSet(HelloWorldFlag) || c.IsSet(config.BastionFlag) { + return nil, errors.New("--file must be used exclusively: it conflicts with --url, --unix-socket, --hello-world and --bastion") } return newFileServer(c.String(FileFlag)) } diff --git a/ingress/origin_file.go b/ingress/origin_file.go index cac831ec123..9d0a0b976d7 100644 --- a/ingress/origin_file.go +++ b/ingress/origin_file.go @@ -33,9 +33,8 @@ const localFileAuthHeader = "X-Cloudflared-File-Server" // httpService at it, in the same way helloWorld does. Nothing is ever written. type fileServer struct { httpService - root string - token string - server net.Listener + root string + token string } // newFileServer records the directory to serve. @@ -106,7 +105,6 @@ func (o *fileServer) start(log *zerolog.Logger, shutdownC <-chan struct{}, cfg O if err != nil { return errors.Wrap(err, "cannot start local file server") } - o.server = listener server := &http.Server{ Handler: o, ReadHeaderTimeout: 10 * time.Second, @@ -115,7 +113,9 @@ func (o *fileServer) start(log *zerolog.Logger, shutdownC <-chan struct{}, cfg O go func() { _ = server.Serve(listener) }() go func() { <-shutdownC - _ = listener.Close() + // Close also tears down keep-alive connections the shared transport + // may still be holding open. + _ = server.Close() }() o.httpService.url = &url.URL{Scheme: "http", Host: listener.Addr().String()} log.Info(). @@ -128,6 +128,10 @@ func (o *fileServer) start(log *zerolog.Logger, shutdownC <-chan struct{}, cfg O // ServeHTTP serves files under root. It is deliberately stricter than // http.FileServer: no directory listings, no dotfiles, no symlink escapes and // only GET/HEAD. +// +// Only the root path maps to index.html. Requests for a directory such as +// "/sub/" are 404 by design, so a site whose links end in a slash needs the +// page to be requested explicitly ("/sub/index.html"). func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet && r.Method != http.MethodHead { w.Header().Set("Allow", "GET, HEAD") @@ -198,6 +202,11 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { // willing to serve. It is applied both to the request path and to the resolved // filesystem path. func safeRelativePath(rel string) bool { + // A backslash is a separator on Windows but not on Unix. Reject it + // everywhere so the policy does not depend on the platform. + if strings.ContainsRune(rel, 0x5C) { + return false + } for _, segment := range strings.Split(filepath.ToSlash(rel), "/") { if segment == "" || segment == "." || segment == ".." { return false diff --git a/ingress/origin_file_flag_test.go b/ingress/origin_file_flag_test.go index b7f41634e53..e892532e5dd 100644 --- a/ingress/origin_file_flag_test.go +++ b/ingress/origin_file_flag_test.go @@ -19,6 +19,8 @@ func newFileFlagContext(t *testing.T, args ...string) *cli.Context { &cli.StringFlag{Name: "url"}, &cli.StringFlag{Name: "unix-socket"}, &cli.StringFlag{Name: "hostname"}, + &cli.BoolFlag{Name: HelloWorldFlag}, + &cli.BoolFlag{Name: config.BastionFlag}, } { require.NoError(t, cliFlag.Apply(flagSet)) } @@ -47,6 +49,8 @@ func TestFileFlagIsExclusive(t *testing.T) { cases := map[string][]string{ "with --url": {"--" + FileFlag, dir, "--url", "http://localhost:8080"}, "with --unix-socket": {"--" + FileFlag, dir, "--unix-socket", "/tmp/sock"}, + "with --hello-world": {"--" + FileFlag, dir, "--" + HelloWorldFlag}, + "with --bastion": {"--" + FileFlag, dir, "--" + config.BastionFlag}, } for name, args := range cases { t.Run(name, func(t *testing.T) { diff --git a/ingress/origin_file_policy_test.go b/ingress/origin_file_policy_test.go new file mode 100644 index 00000000000..779da828ff4 --- /dev/null +++ b/ingress/origin_file_policy_test.go @@ -0,0 +1,53 @@ +package ingress + +import ( + "runtime" + "testing" + + "github.com/stretchr/testify/require" +) + +// safeRelativePath is the single place that decides what may be served. It is +// tested directly, because going through ServeHTTP cannot distinguish "the rule +// rejected this" from "the file simply does not exist on this platform". +func TestSafeRelativePath(t *testing.T) { + always := []struct { + rel string + want bool + }{ + {"index.html", true}, + {"sub/ok.txt", true}, + {"a.txt", true}, + {"", false}, + {".", false}, + {"..", false}, + {"a/../b", false}, + {".env", false}, + {".git/config", false}, + {"sub/.hidden", false}, + {"a.txt.", false}, + {"a.txt ", false}, + {"sub/a.txt. ", false}, + {"a\\b", false}, + {"\\.env", false}, + {"sub\\ok.txt", false}, + } + for _, c := range always { + require.Equalf(t, c.want, safeRelativePath(c.rel), "safeRelativePath(%q)", c.rel) + } +} + +func TestSafeRelativePathReservedNames(t *testing.T) { + reserved := []string{"NUL", "nul", "CON", "PRN", "AUX", "COM1", "com9", "LPT1", "NUL.txt", "con.log"} + for _, name := range reserved { + got := safeRelativePath(name) + if runtime.GOOS == "windows" { + require.Falsef(t, got, "reserved device name %q must be rejected on Windows", name) + } else { + require.Truef(t, got, "%q is an ordinary name outside Windows", name) + } + } + // Names that merely start with a device name are fine on every platform. + require.True(t, safeRelativePath("console.txt")) + require.True(t, safeRelativePath("nullable.md")) +} From a87c24d2002322fb5dc09c72542fbade670184e9 Mon Sep 17 00:00:00 2001 From: hughcube Date: Sat, 14 Nov 2026 00:28:23 +0800 Subject: [PATCH 5/6] fix(ingress): keep the backslash rule on the request path only filepath.Rel returns OS-separated paths, so rejecting every backslash also rejected nested files on Windows. Split the policy: safeRequestPath rejects backslashes for request-derived paths, safeRelativePath normalises and validates filesystem-derived paths. --- ingress/origin_file.go | 14 +++++++++---- ingress/origin_file_policy_test.go | 32 ++++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/ingress/origin_file.go b/ingress/origin_file.go index 9d0a0b976d7..62080dc5e6e 100644 --- a/ingress/origin_file.go +++ b/ingress/origin_file.go @@ -157,7 +157,7 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { if rel == "" { rel = "index.html" } - if !safeRelativePath(rel) { + if !safeRequestPath(rel) { http.NotFound(w, r) return } @@ -201,12 +201,18 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { // safeRelativePath reports whether every component of rel is something we are // willing to serve. It is applied both to the request path and to the resolved // filesystem path. -func safeRelativePath(rel string) bool { - // A backslash is a separator on Windows but not on Unix. Reject it - // everywhere so the policy does not depend on the platform. +// safeRequestPath applies the policy to a path that came straight from the +// request. A backslash is a separator on Windows but opaque to path.Clean, +// so it must never appear here; on Unix it is rejected as well so the rule +// does not depend on the platform. +func safeRequestPath(rel string) bool { if strings.ContainsRune(rel, 0x5C) { return false } + return safeRelativePath(rel) +} + +func safeRelativePath(rel string) bool { for _, segment := range strings.Split(filepath.ToSlash(rel), "/") { if segment == "" || segment == "." || segment == ".." { return false diff --git a/ingress/origin_file_policy_test.go b/ingress/origin_file_policy_test.go index 779da828ff4..61aa2613387 100644 --- a/ingress/origin_file_policy_test.go +++ b/ingress/origin_file_policy_test.go @@ -7,17 +7,18 @@ import ( "github.com/stretchr/testify/require" ) -// safeRelativePath is the single place that decides what may be served. It is +// The path policy is the single place that decides what may be served. It is // tested directly, because going through ServeHTTP cannot distinguish "the rule // rejected this" from "the file simply does not exist on this platform". func TestSafeRelativePath(t *testing.T) { - always := []struct { + cases := []struct { rel string want bool }{ {"index.html", true}, {"sub/ok.txt", true}, {"a.txt", true}, + {"sub/deeper/x.css", true}, {"", false}, {".", false}, {"..", false}, @@ -28,12 +29,35 @@ func TestSafeRelativePath(t *testing.T) { {"a.txt.", false}, {"a.txt ", false}, {"sub/a.txt. ", false}, + } + for _, c := range cases { + require.Equalf(t, c.want, safeRelativePath(c.rel), "safeRelativePath(%q)", c.rel) + } + + // Windows hands back OS-separated paths from filepath.Rel; they must be + // accepted after normalisation rather than rejected as unsafe. + if runtime.GOOS == "windows" { + require.True(t, safeRelativePath("sub\\ok.txt")) + require.False(t, safeRelativePath("sub\\.hidden")) + } +} + +func TestSafeRequestPath(t *testing.T) { + cases := []struct { + rel string + want bool + }{ + {"index.html", true}, + {"sub/ok.txt", true}, {"a\\b", false}, {"\\.env", false}, {"sub\\ok.txt", false}, + {"sub\\..\\.env", false}, + {".env", false}, + {"a.txt.", false}, } - for _, c := range always { - require.Equalf(t, c.want, safeRelativePath(c.rel), "safeRelativePath(%q)", c.rel) + for _, c := range cases { + require.Equalf(t, c.want, safeRequestPath(c.rel), "safeRequestPath(%q)", c.rel) } } From 0f00740bff62218493790e43577bd10eb8488125 Mon Sep 17 00:00:00 2001 From: hughcube Date: Sat, 14 Nov 2026 00:49:15 +0800 Subject: [PATCH 6/6] refactor(ingress): drop duplicate defences and pin the ones that matter Adversarial review, round three, ran mutation tests over every layer of the path policy. Two of them were provably dead and one comment was wrong about why a check exists: - the explicit backslash block in ServeHTTP tested the same predicate as safeRequestPath (path.Clean never produces or removes a backslash), so it is removed; the policy function keeps the rule - isWithin was fully subsumed by the resolved-path check, which rejects both a ".." escape and a symlink pointing outside the root; removed, and the remaining block now says that it is the containment check - the comment claimed the backslash rule stopped "/%5c.env" from reaching ".env". It does not: safeRelativePath maps the backslash through filepath.ToSlash, so that payload is rejected by the empty-segment rule. What the backslash rule actually buys is a single URL per file. Comment corrected rather than the code changed. New tests, both verified to fail when the defence they cover is removed: - a symlink inside the root pointing at a dotfile, which only the resolved check catches - a request through RoundTrip versus a direct hit on the loopback port; without this, deleting RoundTrip would keep the suite green while every real request returns 403 --- ingress/origin_file.go | 57 +++++++++++----------------- ingress/origin_file_loopback_test.go | 56 +++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 35 deletions(-) create mode 100644 ingress/origin_file_loopback_test.go diff --git a/ingress/origin_file.go b/ingress/origin_file.go index 62080dc5e6e..a18f63c859e 100644 --- a/ingress/origin_file.go +++ b/ingress/origin_file.go @@ -86,8 +86,8 @@ func (o *fileServer) String() string { return "file:" + o.root } func (o fileServer) MarshalJSON() ([]byte, error) { return json.Marshal(o.String()) } -// RoundTrip marks requests that come from the tunnel so the loopback listener -// can tell them apart from other local processes. +// RoundTrip marks requests that come through the tunnel so that the loopback +// listener can tell them apart from other local processes. func (o *fileServer) RoundTrip(req *http.Request) (*http.Response, error) { clone := req.Clone(req.Context()) clone.Header.Set(localFileAuthHeader, o.token) @@ -145,14 +145,6 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - // A backslash is never valid in a URL path. On Windows it is a separator - // for the filesystem but not for path.Clean, so allowing it would let - // "/%5c.env" slip past the segment checks below and resolve to ".env". - if strings.ContainsRune(r.URL.Path, '\\') { - http.NotFound(w, r) - return - } - rel := strings.TrimPrefix(path.Clean("/"+r.URL.Path), "/") if rel == "" { rel = "index.html" @@ -168,15 +160,14 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.NotFound(w, r) return } - if !isWithin(o.root, resolved) { - http.NotFound(w, r) - return - } - // Re-check the resolved path: the filesystem may have expanded the request - // into something the checks above never saw, for example a Windows 8.3 - // short name such as "GIT~1" that aliases ".git". - if resolvedRel, err := filepath.Rel(o.root, resolved); err != nil || !safeRelativePath(resolvedRel) { + // This is the check that actually contains the request. The filesystem may + // have expanded it into something the policy above never saw: a Windows 8.3 + // short name such as "GIT~1" that aliases ".git", or a symlink inside the + // root pointing at a dotfile. Applying the same policy to the resolved path + // rejects both, and an escape from the root surfaces here as a leading "..". + resolvedRel, err := filepath.Rel(o.root, resolved) + if err != nil || !safeRelativePath(resolvedRel) { http.NotFound(w, r) return } @@ -198,20 +189,24 @@ func (o *fileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { http.ServeContent(w, r, info.Name(), info.ModTime(), file) } -// safeRelativePath reports whether every component of rel is something we are -// willing to serve. It is applied both to the request path and to the resolved -// filesystem path. // safeRequestPath applies the policy to a path that came straight from the -// request. A backslash is a separator on Windows but opaque to path.Clean, -// so it must never appear here; on Unix it is rejected as well so the rule -// does not depend on the platform. +// request. It additionally rejects backslashes: on Windows they separate path +// components for the filesystem but are opaque to path.Clean, which would give +// one file two URLs ("/sub%5ca.txt" and "/sub/a.txt"). +// +// That is a normalisation rule rather than the safety boundary: what stops a +// backslash from reaching a dotfile is safeRelativePath mapping it through +// filepath.ToSlash, and ultimately the resolved-path check in ServeHTTP. func safeRequestPath(rel string) bool { - if strings.ContainsRune(rel, 0x5C) { + if strings.ContainsRune(rel, '\\') { return false } return safeRelativePath(rel) } +// safeRelativePath reports whether every component of rel is something we are +// willing to serve. It is applied both to the request path and to the path the +// filesystem resolved to. func safeRelativePath(rel string) bool { for _, segment := range strings.Split(filepath.ToSlash(rel), "/") { if segment == "" || segment == "." || segment == ".." { @@ -222,8 +217,8 @@ func safeRelativePath(rel string) bool { return false } // Windows silently strips trailing dots and spaces, so "a.txt." would - // alias "a.txt". Rejecting everywhere keeps behaviour identical across - // platforms rather than aliasing on one of them. + // alias "a.txt". Rejecting on every platform keeps behaviour identical + // rather than aliasing on one of them. if strings.TrimRight(segment, ". ") != segment { return false } @@ -256,11 +251,3 @@ func isReservedName(segment string) bool { } return false } - -func isWithin(root, target string) bool { - rel, err := filepath.Rel(root, target) - if err != nil { - return false - } - return rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) -} diff --git a/ingress/origin_file_loopback_test.go b/ingress/origin_file_loopback_test.go new file mode 100644 index 00000000000..4e60fe64dc1 --- /dev/null +++ b/ingress/origin_file_loopback_test.go @@ -0,0 +1,56 @@ +package ingress + +import ( + "io" + "net/http" + "os" + "path/filepath" + "testing" + + "github.com/rs/zerolog" + "github.com/stretchr/testify/require" +) + +// A symlink inside the root that points at a dotfile is only caught by the +// resolved-path check, so this pins that step. +func TestFileServerRejectsSymlinkToDotfile(t *testing.T) { + root := t.TempDir() + mustWriteFile(t, filepath.Join(root, ".env"), "SECRET=1") + if err := os.Symlink(filepath.Join(root, ".env"), filepath.Join(root, "link_env")); err != nil { + t.Skipf("symlink unavailable: %v", err) + } + + svc := newTestFileServer(t, root) + if rec := serve(svc, "GET", "/link_env"); rec.Code != 404 { + t.Errorf("symlink to dotfile: code=%d want 404", rec.Code) + } +} + +// The loopback listener must only answer requests that came through the tunnel. +// This fails if RoundTrip stops injecting the header, which no other test covers. +func TestFileServerLoopbackRequiresTunnelRoundTrip(t *testing.T) { + root := t.TempDir() + mustWriteFile(t, filepath.Join(root, "a.txt"), "AAA") + svc := newTestFileServer(t, root) + + shutdown := make(chan struct{}) + defer close(shutdown) + logger := zerolog.Nop() + require.NoError(t, svc.start(&logger, shutdown, OriginRequestConfig{})) + + // Through the tunnel proxy: RoundTrip adds the header. + client := &http.Client{Transport: svc} + resp, err := client.Get("http://example.com/a.txt") + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, 200, resp.StatusCode) + require.Equal(t, "AAA", string(body)) + + // Straight at the loopback port, the way another local process would. + direct, err := http.Get("http://" + svc.httpService.url.Host + "/a.txt") + require.NoError(t, err) + defer direct.Body.Close() + require.Equal(t, http.StatusForbidden, direct.StatusCode) +}