From 6f68dab93758f39e002e16ccdc825e9f43364a67 Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Thu, 23 Jul 2026 10:11:06 +0530 Subject: [PATCH 1/3] fix: harden untrusted filesystem operations --- cli/pkg/installscript/installscript.go | 211 ++++++++++-- cli/pkg/installscript/installscript_test.go | 360 ++++++++++++++++++++ runtime/pkg/archive/archive.go | 132 +++++-- runtime/pkg/archive/archive_test.go | 287 ++++++++++++++++ runtime/pkg/archive/download_test.go | 279 +++++++++++++++ runtime/storage/storage.go | 33 +- runtime/storage/storage_test.go | 245 +++++++++++++ 7 files changed, 1495 insertions(+), 52 deletions(-) create mode 100644 cli/pkg/installscript/installscript_test.go create mode 100644 runtime/pkg/archive/archive_test.go create mode 100644 runtime/pkg/archive/download_test.go diff --git a/cli/pkg/installscript/installscript.go b/cli/pkg/installscript/installscript.go index a997405593f2..9488a75c7f79 100644 --- a/cli/pkg/installscript/installscript.go +++ b/cli/pkg/installscript/installscript.go @@ -2,73 +2,228 @@ package installscript import ( "context" + "errors" "fmt" "io" "net/http" + "net/url" "os" "os/exec" + "strings" ) +const ( + cdnBaseURL = "https://cdn.rilldata.com/rill" + releaseBaseURL = "https://raw.githubusercontent.com/rilldata/rill" + maxScriptBytes = int64(1 << 20) +) + +var errUnsafeRedirect = errors.New("refusing install script redirect to a different origin") + +type processRunner func(ctx context.Context, executable string, args ...string) error + +type installer struct { + client *http.Client + cdnBaseURL string + releaseBaseURL string + tempDir string + run processRunner +} + +var defaultInstaller = installer{ + client: http.DefaultClient, + cdnBaseURL: cdnBaseURL, + releaseBaseURL: releaseBaseURL, + run: runProcess, +} + func Install(ctx context.Context, version string) error { - return execScript(ctx, version, "--version", version) + return defaultInstaller.install(ctx, version) } func Uninstall(ctx context.Context) error { - return execScript(ctx, "", "--uninstall") + return defaultInstaller.uninstall(ctx) } -func execScript(ctx context.Context, version string, args ...string) error { - script, err := createScriptFile(ctx, version) +func (i installer) install(ctx context.Context, version string) error { + return i.execScript(ctx, version, "--version", version) +} + +func (i installer) uninstall(ctx context.Context) error { + return i.execScript(ctx, "", "--uninstall") +} + +func (i installer) execScript(ctx context.Context, version string, args ...string) error { + script, err := i.createScriptFile(ctx, version) if err != nil { return err } defer os.Remove(script) - scriptArgs := append([]string{script}, args...) - cmd := exec.CommandContext(ctx, "/bin/sh", scriptArgs...) - cmd.Stdin = os.Stdin - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr - return cmd.Run() + if err := ctx.Err(); err != nil { + return err + } + + // Execution invariant: the downloaded file is only passed to /bin/sh, followed by the caller's exact arguments. + scriptArgs := make([]string, 0, len(args)+1) + scriptArgs = append(scriptArgs, script) + scriptArgs = append(scriptArgs, args...) + if err := i.run(ctx, "/bin/sh", scriptArgs...); err != nil { + // Cancellation invariant: one context controls download and execution, and cancellation wins over a process error. + if ctxErr := ctx.Err(); ctxErr != nil { + return fmt.Errorf("install script canceled: %w", ctxErr) + } + return fmt.Errorf("install script failed: %w", err) + } + if err := ctx.Err(); err != nil { + return fmt.Errorf("install script canceled: %w", err) + } + return nil } -func createScriptFile(ctx context.Context, version string) (string, error) { - var url string - switch version { - case "nightly": - url = "https://cdn.rilldata.com/rill/nightly/install.sh" - case "latest", "": - url = "https://cdn.rilldata.com/rill/install.sh" - default: - url = fmt.Sprintf("https://raw.githubusercontent.com/rilldata/rill/%s/scripts/install.sh", version) +func (i installer) createScriptFile(ctx context.Context, version string) (string, error) { + scriptURL, err := i.scriptURL(version) + if err != nil { + return "", err } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, http.NoBody) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, scriptURL, http.NoBody) if err != nil { return "", fmt.Errorf("failed to create request: %w", err) } - client := &http.Client{} + // Download invariant: only a complete, bounded HTTP 200 response from the selected release URL can become executable input. + client := i.redirectSafeClient() resp, err := client.Do(req) if err != nil { - return "", fmt.Errorf("failed to download install script from %s: %w", url, err) + return "", fmt.Errorf("failed to download install script from %s: %w", scriptURL, err) } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("failed to download install script from %s: HTTP %d", url, resp.StatusCode) + return "", fmt.Errorf("failed to download install script from %s: HTTP %d", scriptURL, resp.StatusCode) + } + if resp.ContentLength > maxScriptBytes { + return "", fmt.Errorf("failed to download install script from %s: response exceeds %d bytes", scriptURL, maxScriptBytes) } - out, err := os.CreateTemp("", "install*.sh") + // Integrity invariant: this package defines no detached checksum contract for these URLs, so partial or oversized transfers fail closed. + contents, err := io.ReadAll(io.LimitReader(resp.Body, maxScriptBytes+1)) if err != nil { + if errors.Is(err, io.ErrUnexpectedEOF) { + return "", fmt.Errorf("failed to download install script from %s: truncated response: %w", scriptURL, err) + } + return "", fmt.Errorf("failed to read install script from %s: %w", scriptURL, err) + } + if int64(len(contents)) > maxScriptBytes { + return "", fmt.Errorf("failed to download install script from %s: response exceeds %d bytes", scriptURL, maxScriptBytes) + } + if resp.ContentLength >= 0 && int64(len(contents)) != resp.ContentLength { + return "", fmt.Errorf("failed to download install script from %s: truncated response (expected %d bytes, got %d)", scriptURL, resp.ContentLength, len(contents)) + } + if len(contents) == 0 { + return "", fmt.Errorf("failed to download install script from %s: empty response", scriptURL) + } + if err := ctx.Err(); err != nil { return "", err } - defer out.Close() - _, err = io.Copy(out, resp.Body) + // Temp-file invariant: CreateTemp provides mode 0600, and every write or close failure removes the file. + out, err := os.CreateTemp(i.tempDir, "rill-install-*.sh") if err != nil { - return "", err + return "", fmt.Errorf("failed to create install script file: %w", err) + } + path := out.Name() + keep := false + defer func() { + _ = out.Close() + if !keep { + _ = os.Remove(path) + } + }() + + if _, err := out.Write(contents); err != nil { + return "", fmt.Errorf("failed to write install script file: %w", err) + } + if err := out.Close(); err != nil { + return "", fmt.Errorf("failed to close install script file: %w", err) + } + keep = true + return path, nil +} + +func (i installer) scriptURL(version string) (string, error) { + baseURL := i.cdnBaseURL + path := []string{"install.sh"} + switch version { + case "nightly": + path = []string{"nightly", "install.sh"} + case "latest", "": + default: + if version == "." || version == ".." || strings.ContainsAny(version, "/\\") { + return "", fmt.Errorf("invalid release version %q", version) + } + baseURL = i.releaseBaseURL + path = []string{version, "scripts", "install.sh"} + } + + u, err := url.JoinPath(baseURL, path...) + if err != nil { + return "", fmt.Errorf("failed to construct install script URL: %w", err) } + parsed, err := url.Parse(u) + if err != nil || !parsed.IsAbs() || parsed.Host == "" { + return "", fmt.Errorf("failed to construct install script URL from %q", baseURL) + } + return u, nil +} - return out.Name(), nil +func (i installer) redirectSafeClient() *http.Client { + client := http.Client{} + if i.client != nil { + client = *i.client + } + previousCheck := client.CheckRedirect + client.CheckRedirect = func(req *http.Request, via []*http.Request) error { + if len(via) >= 10 { + return errors.New("stopped after 10 redirects") + } + // Redirect invariant: a redirect may change the path, but not the scheme, host, or effective port. + if len(via) > 0 && !sameOrigin(via[0].URL, req.URL) { + return fmt.Errorf("%w: %s", errUnsafeRedirect, req.URL.Redacted()) + } + if previousCheck != nil { + return previousCheck(req, via) + } + return nil + } + return &client +} + +func sameOrigin(a, b *url.URL) bool { + return strings.EqualFold(a.Scheme, b.Scheme) && + strings.EqualFold(a.Hostname(), b.Hostname()) && + effectivePort(a) == effectivePort(b) +} + +func effectivePort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + switch strings.ToLower(u.Scheme) { + case "http": + return "80" + case "https": + return "443" + default: + return "" + } +} + +func runProcess(ctx context.Context, executable string, args ...string) error { + cmd := exec.CommandContext(ctx, executable, args...) + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + return cmd.Run() } diff --git a/cli/pkg/installscript/installscript_test.go b/cli/pkg/installscript/installscript_test.go new file mode 100644 index 000000000000..8e0cb95cdd9f --- /dev/null +++ b/cli/pkg/installscript/installscript_test.go @@ -0,0 +1,360 @@ +package installscript + +import ( + "bytes" + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" + "time" +) + +func TestScriptURL(t *testing.T) { + // Version selection must resolve to the exact trusted release channel or tagged source URL. + tests := []struct { + name string + version string + want string + }{ + {name: "empty means latest", version: "", want: "https://cdn.rilldata.com/rill/install.sh"}, + {name: "latest", version: "latest", want: "https://cdn.rilldata.com/rill/install.sh"}, + {name: "nightly", version: "nightly", want: "https://cdn.rilldata.com/rill/nightly/install.sh"}, + {name: "tag", version: "v1.2.3", want: "https://raw.githubusercontent.com/rilldata/rill/v1.2.3/scripts/install.sh"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := defaultInstaller.scriptURL(tt.version) + if err != nil { + t.Fatalf("scriptURL(%q) returned error: %v", tt.version, err) + } + if got != tt.want { + t.Fatalf("scriptURL(%q) = %q, want %q", tt.version, got, tt.want) + } + }) + } +} + +func TestInstallRejects404(t *testing.T) { + // An HTTP error page is never valid executable input and must leave no temporary script behind. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "not found", http.StatusNotFound) + })) + defer server.Close() + + tempDir := t.TempDir() + runnerCalled := false + i := testInstaller(server, tempDir, func(context.Context, string, ...string) error { + runnerCalled = true + return nil + }) + + err := i.install(context.Background(), "latest") + if err == nil || !strings.Contains(err.Error(), "HTTP 404") { + t.Fatalf("install error = %v, want HTTP 404", err) + } + if runnerCalled { + t.Fatal("runner was called for a 404 response") + } + assertDirEmpty(t, tempDir) +} + +func TestInstallRejectsCrossHostRedirect(t *testing.T) { + // Redirects cannot move an installer download to a different host controlled outside the selected channel. + destinationHit := make(chan struct{}, 1) + destination := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + destinationHit <- struct{}{} + _, _ = io.WriteString(w, "#!/bin/sh\n") + })) + defer destination.Close() + + source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, destination.URL+"/install.sh", http.StatusFound) + })) + defer source.Close() + + tempDir := t.TempDir() + i := testInstaller(source, tempDir, func(context.Context, string, ...string) error { + t.Fatal("runner was called after an unsafe redirect") + return nil + }) + + err := i.install(context.Background(), "latest") + if !errors.Is(err, errUnsafeRedirect) { + t.Fatalf("install error = %v, want errUnsafeRedirect", err) + } + select { + case <-destinationHit: + t.Fatal("redirect destination was contacted") + default: + } + assertDirEmpty(t, tempDir) +} + +func TestInstallRejectsOversizedResponse(t *testing.T) { + // Enforce the response limit for both declared and chunked bodies before invoking the shell. + tests := []struct { + name string + handler http.HandlerFunc + }{ + { + name: "declared size", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", strconv.FormatInt(maxScriptBytes+1, 10)) + w.WriteHeader(http.StatusOK) + }, + }, + { + name: "streamed size", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + w.(http.Flusher).Flush() + _, _ = w.Write(bytes.Repeat([]byte("x"), int(maxScriptBytes)+1)) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(tt.handler) + defer server.Close() + + tempDir := t.TempDir() + runnerCalled := false + i := testInstaller(server, tempDir, func(context.Context, string, ...string) error { + runnerCalled = true + return nil + }) + + err := i.install(context.Background(), "latest") + if err == nil || !strings.Contains(err.Error(), "response exceeds") { + t.Fatalf("install error = %v, want oversized response error", err) + } + if runnerCalled { + t.Fatal("runner was called for an oversized response") + } + assertDirEmpty(t, tempDir) + }) + } +} + +func TestInstallFailsClosedOnTruncatedResponse(t *testing.T) { + // A short transfer must be detected before incomplete remote content can be executed. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Length", "100") + w.WriteHeader(http.StatusOK) + _, _ = io.WriteString(w, "#!/bin/sh\n") + })) + defer server.Close() + + tempDir := t.TempDir() + runnerCalled := false + i := testInstaller(server, tempDir, func(context.Context, string, ...string) error { + runnerCalled = true + return nil + }) + + // This package's release URL contract has no detached checksum; transfer truncation must therefore never reach execution. + err := i.install(context.Background(), "latest") + if err == nil || !errors.Is(err, io.ErrUnexpectedEOF) { + t.Fatalf("install error = %v, want unexpected EOF", err) + } + if runnerCalled { + t.Fatal("runner was called for a truncated response") + } + assertDirEmpty(t, tempDir) +} + +func TestInstallCancellation(t *testing.T) { + // Cancellation must interrupt either network transfer or process execution and always clean temporary files. + t.Run("during download", func(t *testing.T) { + started := make(chan struct{}) + server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + close(started) + <-r.Context().Done() + })) + defer server.Close() + + tempDir := t.TempDir() + runnerCalled := false + i := testInstaller(server, tempDir, func(context.Context, string, ...string) error { + runnerCalled = true + return nil + }) + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { + done <- i.install(ctx, "latest") + }() + + <-started + cancel() + select { + case err := <-done: + if !errors.Is(err, context.Canceled) { + t.Fatalf("install error = %v, want context.Canceled", err) + } + case <-time.After(5 * time.Second): + t.Fatal("install did not stop after cancellation") + } + if runnerCalled { + t.Fatal("runner was called after download cancellation") + } + assertDirEmpty(t, tempDir) + }) + + t.Run("during execution", func(t *testing.T) { + server := scriptServer("#!/bin/sh\n") + defer server.Close() + + tempDir := t.TempDir() + ctx, cancel := context.WithCancel(context.Background()) + var scriptPath string + i := testInstaller(server, tempDir, func(runCtx context.Context, _ string, args ...string) error { + scriptPath = args[0] + cancel() + <-runCtx.Done() + return errors.New("process stopped") + }) + + err := i.install(ctx, "latest") + if !errors.Is(err, context.Canceled) { + t.Fatalf("install error = %v, want context.Canceled", err) + } + if _, statErr := os.Stat(scriptPath); !errors.Is(statErr, os.ErrNotExist) { + t.Fatalf("temporary script still exists after cancellation: %v", statErr) + } + assertDirEmpty(t, tempDir) + }) +} + +func TestInstallReturnsNonzeroScriptExitAndCleansUp(t *testing.T) { + // Preserve the script's nonzero exit status for callers while still deleting the downloaded file. + server := scriptServer("#!/bin/sh\nexit 7\n") + defer server.Close() + + tempDir := t.TempDir() + i := testInstaller(server, tempDir, runProcess) + err := i.install(context.Background(), "latest") + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) { + t.Fatalf("install error = %v, want exec.ExitError", err) + } + if exitErr.ExitCode() != 7 { + t.Fatalf("exit code = %d, want 7", exitErr.ExitCode()) + } + assertDirEmpty(t, tempDir) +} + +func TestInstallPassesExactArgumentsAndCleansUp(t *testing.T) { + // A tagged install executes the downloaded script once with the exact version arguments and restrictive mode. + requestedPath := make(chan string, 1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestedPath <- r.URL.Path + _, _ = io.WriteString(w, "#!/bin/sh\n") + })) + defer server.Close() + + tempDir := t.TempDir() + var executable string + var args []string + i := testInstaller(server, tempDir, func(_ context.Context, gotExecutable string, gotArgs ...string) error { + executable = gotExecutable + args = append([]string(nil), gotArgs...) + contents, err := os.ReadFile(gotArgs[0]) + if err != nil { + t.Fatalf("read temporary script: %v", err) + } + if string(contents) != "#!/bin/sh\n" { + t.Fatalf("temporary script contents = %q", contents) + } + info, err := os.Stat(gotArgs[0]) + if err != nil { + t.Fatalf("stat temporary script: %v", err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("temporary script mode = %o, want 600", got) + } + return nil + }) + + err := i.install(context.Background(), "v1.2.3") + if err != nil { + t.Fatalf("install returned error: %v", err) + } + if got := <-requestedPath; got != "/v1.2.3/scripts/install.sh" { + t.Fatalf("request path = %q, want tag install script path", got) + } + if executable != "/bin/sh" { + t.Fatalf("executable = %q, want /bin/sh", executable) + } + if len(args) != 3 || args[1] != "--version" || args[2] != "v1.2.3" { + t.Fatalf("runner args = %q, want [script --version v1.2.3]", args) + } + if filepath.Dir(args[0]) != tempDir { + t.Fatalf("script path = %q, want file in %q", args[0], tempDir) + } + info, err := os.Stat(args[0]) + if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("temporary script was not removed: %v", err) + } + if info != nil { + t.Fatalf("temporary script still exists: %s", info.Name()) + } + assertDirEmpty(t, tempDir) +} + +func TestUninstallSucceedsAndCleansUp(t *testing.T) { + // Uninstall uses the stable installer with only the uninstall flag and removes its temporary copy afterward. + requestedPath := make(chan string, 1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requestedPath <- r.URL.Path + _, _ = fmt.Fprint(w, "#!/bin/sh\n[ \"$#\" -eq 1 ] && [ \"$1\" = \"--uninstall\" ]\n") + })) + defer server.Close() + + tempDir := t.TempDir() + i := testInstaller(server, tempDir, runProcess) + if err := i.uninstall(context.Background()); err != nil { + t.Fatalf("uninstall returned error: %v", err) + } + if got := <-requestedPath; got != "/install.sh" { + t.Fatalf("request path = %q, want /install.sh", got) + } + assertDirEmpty(t, tempDir) +} + +func testInstaller(server *httptest.Server, tempDir string, run processRunner) installer { + return installer{ + client: server.Client(), + cdnBaseURL: server.URL, + releaseBaseURL: server.URL, + tempDir: tempDir, + run: run, + } +} + +func scriptServer(script string) *httptest.Server { + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = io.WriteString(w, script) + })) +} + +func assertDirEmpty(t *testing.T, dir string) { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read temp directory: %v", err) + } + if len(entries) != 0 { + t.Fatalf("temporary files were not cleaned up: %v", entries) + } +} diff --git a/runtime/pkg/archive/archive.go b/runtime/pkg/archive/archive.go index b4652f5af18f..e2d422bdebd4 100644 --- a/runtime/pkg/archive/archive.go +++ b/runtime/pkg/archive/archive.go @@ -17,6 +17,11 @@ import ( "github.com/rilldata/rill/runtime/drivers" ) +const ( + maxArchiveBytes = int64(100 * 1024 * 1024) + maxExtractedFileBytes = int64(datasize.GB) +) + var ignoreFileList = []string{ "/.env", "/.git", @@ -36,6 +41,9 @@ func Download(ctx context.Context, downloadURL, downloadDst, projPath string, cl if resp.StatusCode != http.StatusOK { return fmt.Errorf("failed to download file: status code %d", resp.StatusCode) } + if resp.ContentLength > maxArchiveBytes { + return fmt.Errorf("failed to download file: archive exceeds %d byte limit", maxArchiveBytes) + } out, err := os.Create(downloadDst) if err != nil { @@ -43,25 +51,73 @@ func Download(ctx context.Context, downloadURL, downloadDst, projPath string, cl } defer os.Remove(downloadDst) - // Writer the body to file - _, err = io.Copy(out, resp.Body) + // Read one byte beyond the limit so chunked responses cannot bypass the + // Content-Length check. + written, err := io.Copy(out, io.LimitReader(resp.Body, maxArchiveBytes+1)) if err != nil { out.Close() return err } - out.Close() + if err := out.Close(); err != nil { + return err + } + if written > maxArchiveBytes { + return fmt.Errorf("failed to download file: archive exceeds %d byte limit", maxArchiveBytes) + } - // clean the projPath first to remove any files from previous download - if clean { - _ = os.RemoveAll(projPath) + if !clean { + return untar(downloadDst, filepath.Clean(projPath), ignorePaths) } - // untar to the project path - err = untar(downloadDst, filepath.Clean(projPath), ignorePaths) + // Validate and extract beside the destination before touching the live + // project. Keeping staging on the same filesystem also makes the final rename + // atomic for readers that observe the project path. + projPath = filepath.Clean(projPath) + parent := filepath.Dir(projPath) + if err := os.MkdirAll(parent, 0o755); err != nil { + return err + } + staging, err := os.MkdirTemp(parent, ".rill-project-staging-*") if err != nil { return err } - return nil + defer os.RemoveAll(staging) + if err := untar(downloadDst, staging, ignorePaths); err != nil { + return err + } + return replaceDirectory(staging, projPath) +} + +func replaceDirectory(staging, dest string) error { + _, err := os.Lstat(dest) + if errors.Is(err, os.ErrNotExist) { + return os.Rename(staging, dest) + } + if err != nil { + return err + } + + // Reserve a unique sibling name, then remove the placeholder so Rename can + // move the existing project there without overwriting another path. + backupFile, err := os.CreateTemp(filepath.Dir(dest), ".rill-project-backup-*") + if err != nil { + return err + } + backup := backupFile.Name() + if err := backupFile.Close(); err != nil { + return err + } + if err := os.Remove(backup); err != nil { + return err + } + if err := os.Rename(dest, backup); err != nil { + return err + } + if err := os.Rename(staging, dest); err != nil { + rollbackErr := os.Rename(backup, dest) + return errors.Join(err, rollbackErr) + } + return os.RemoveAll(backup) } func CreateAndUpload(ctx context.Context, files []drivers.DirEntry, root, url string, headers map[string]string) error { @@ -161,6 +217,10 @@ func untar(src, dest string, ignorePaths bool) error { } defer gz.Close() tarReader := tar.NewReader(gz) + var directoryModes []struct { + path string + mode os.FileMode + } for { header, err := tarReader.Next() if err != nil { @@ -183,37 +243,65 @@ func untar(src, dest string, ignorePaths bool) error { switch header.Typeflag { case tar.TypeDir: - // Handle directory - if err := os.MkdirAll(target, header.FileInfo().Mode()); err != nil { + // Keep directories writable during extraction, then restore their + // archived modes after all children have been created. + if err := os.MkdirAll(target, 0o755); err != nil { return err } + directoryModes = append(directoryModes, struct { + path string + mode os.FileMode + }{path: target, mode: header.FileInfo().Mode()}) case tar.TypeReg: - // Handle regular file - if err := os.MkdirAll(filepath.Dir(target), header.FileInfo().Mode()); err != nil { + if header.Size > maxExtractedFileBytes { + return fmt.Errorf("archive entry %q exceeds %d byte extraction limit", header.Name, maxExtractedFileBytes) + } + // Implicit parent directories need execute bits independent of the + // file's archived mode, otherwise a 0644 file creates an unusable 0644 directory. + if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { return err } - outFile, err := os.Create(target) + outFile, err := os.OpenFile(target, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, header.FileInfo().Mode()) if err != nil { return err } - // Setting a limit of 1GB to avoid G110: Potential DoS vulnerability via decompression bomb - // The max file size allowed via upload path is 100MB. Assume that 100MB tar file can't be decompressed to more than 1GB. - _, err = io.CopyN(outFile, tarReader, int64(datasize.GB)) - if err != nil && !errors.Is(err, io.EOF) { - outFile.Close() + _, copyErr := io.Copy(outFile, tarReader) + closeErr := outFile.Close() + if copyErr != nil || closeErr != nil { + return errors.Join(copyErr, closeErr) + } + // Chmod after creation so the process umask does not silently strip + // executable or group permission bits stored in the archive. + if err := os.Chmod(target, header.FileInfo().Mode()); err != nil { return err } - outFile.Close() default: return fmt.Errorf("unsupported header type: %c", header.Typeflag) } } + for i := len(directoryModes) - 1; i >= 0; i-- { + if err := os.Chmod(directoryModes[i].path, directoryModes[i].mode); err != nil { + return err + } + } return nil } func sanitizeArchivePath(dest, tarPath string) (v string, err error) { - v = filepath.Join(dest, tarPath) - if strings.HasPrefix(v, dest) { + if tarPath == "" || filepath.IsAbs(tarPath) || filepath.VolumeName(tarPath) != "" { + return "", fmt.Errorf("%s: %s", "content filepath is tainted", tarPath) + } + + dest, err = filepath.Abs(filepath.Clean(dest)) + if err != nil { + return "", err + } + v = filepath.Join(dest, filepath.Clean(tarPath)) + rel, err := filepath.Rel(dest, v) + if err != nil { + return "", err + } + if rel != ".." && !filepath.IsAbs(rel) && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { return v, nil } diff --git a/runtime/pkg/archive/archive_test.go b/runtime/pkg/archive/archive_test.go new file mode 100644 index 000000000000..6821e56dfeae --- /dev/null +++ b/runtime/pkg/archive/archive_test.go @@ -0,0 +1,287 @@ +package archive + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "os" + "path/filepath" + "testing" +) + +type testTarEntry struct { + name string + typeflag byte + mode int64 + linkname string + body []byte +} + +func TestUntarRejectsPathTraversal(t *testing.T) { + // Every archive member must remain within the destination after path normalization. + t.Run("parent traversal", func(t *testing.T) { + // A parent-relative member must be rejected before it can write beside the extraction root. + root := t.TempDir() + dest := filepath.Join(root, "project") + archivePath := writeTestArchive(t, []testTarEntry{{ + name: "../escaped.txt", + typeflag: tar.TypeReg, + mode: 0o644, + body: []byte("escaped"), + }}) + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar accepted a parent-relative archive member") + } + assertPathDoesNotExist(t, filepath.Join(root, "escaped.txt")) + }) + + t.Run("sibling prefix traversal", func(t *testing.T) { + // A shared prefix is not containment; executable mode keeps an unsafe parent searchable so the bug is not masked. + root := t.TempDir() + dest := filepath.Join(root, "project") + archivePath := writeTestArchive(t, []testTarEntry{{ + name: "../project-copy/escaped.txt", + typeflag: tar.TypeReg, + mode: 0o755, + body: []byte("escaped"), + }}) + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar accepted traversal into a sibling with a shared path prefix") + } + assertPathDoesNotExist(t, filepath.Join(root, "project-copy", "escaped.txt")) + }) + + t.Run("absolute path", func(t *testing.T) { + // Absolute names must be rejected; a temp target and executable mode make a vulnerable rebase safe to observe. + root := t.TempDir() + dest := filepath.Join(root, "project") + absoluteTarget := filepath.Join(root, "absolute.txt") + archivePath := writeTestArchive(t, []testTarEntry{{ + name: filepath.ToSlash(absoluteTarget), + typeflag: tar.TypeReg, + mode: 0o755, + body: []byte("absolute"), + }}) + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar accepted an absolute archive member") + } + assertPathDoesNotExist(t, absoluteTarget) + }) +} + +func TestUntarRejectsLinkAndUnsupportedEntries(t *testing.T) { + // Extraction only supports directories and regular files, so special entries must fail closed. + t.Run("symbolic link", func(t *testing.T) { + // Rejecting symlinks prevents later members from following a link outside the extraction root. + root := t.TempDir() + dest := filepath.Join(root, "project") + archivePath := writeTestArchive(t, []testTarEntry{{ + name: "link", + typeflag: tar.TypeSymlink, + mode: 0o777, + linkname: "../outside", + }}) + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar accepted a symbolic-link member") + } + assertPathDoesNotExist(t, filepath.Join(dest, "link")) + }) + + t.Run("hard link", func(t *testing.T) { + // Hard links receive the same rejection as symlinks because their target is archive-controlled. + root := t.TempDir() + dest := filepath.Join(root, "project") + archivePath := writeTestArchive(t, []testTarEntry{{ + name: "hard-link", + typeflag: tar.TypeLink, + mode: 0o644, + linkname: "../outside", + }}) + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar accepted a hard-link member") + } + assertPathDoesNotExist(t, filepath.Join(dest, "hard-link")) + }) + + t.Run("unsupported type", func(t *testing.T) { + // A FIFO represents a non-file/non-directory type and must fail closed rather than be ignored. + root := t.TempDir() + dest := filepath.Join(root, "project") + archivePath := writeTestArchive(t, []testTarEntry{{ + name: "pipe", + typeflag: tar.TypeFifo, + mode: 0o600, + }}) + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar accepted an unsupported FIFO member") + } + assertPathDoesNotExist(t, filepath.Join(dest, "pipe")) + }) +} + +func TestUntarRejectsTruncatedArchive(t *testing.T) { + // This is a valid gzip stream containing a tar header that promises more bytes than it provides. + root := t.TempDir() + dest := filepath.Join(root, "project") + archivePath := filepath.Join(root, "truncated.tar.gz") + if err := os.WriteFile(archivePath, makeTruncatedTestArchive(t), 0o600); err != nil { + t.Fatalf("write truncated archive: %v", err) + } + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar treated a truncated regular-file member as complete") + } +} + +func TestUntarRejectsOversizedEntryBeforeWriting(t *testing.T) { + // The tar header is enough to reject a decompression bomb; extraction must + // not write a truncated 1 GiB prefix and then continue as if it succeeded. + root := t.TempDir() + dest := filepath.Join(root, "project") + archivePath := filepath.Join(root, "oversized.tar.gz") + if err := os.WriteFile(archivePath, makeHeaderOnlyTestArchive(t, maxExtractedFileBytes+1), 0o600); err != nil { + t.Fatalf("write oversized archive: %v", err) + } + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar accepted an entry larger than the extraction limit") + } + assertPathDoesNotExist(t, filepath.Join(dest, "oversized.bin")) +} + +func TestUntarCreatesSafeImplicitParentsAndPreservesFileMode(t *testing.T) { + // Archives do not have to contain directory headers. A regular file's 0644 + // mode must not be reused for its parent, which would remove directory search access. + if os.PathSeparator == '\\' { + t.Skip("Unix permission bits are not portable to Windows") + } + dest := filepath.Join(t.TempDir(), "project") + archivePath := writeTestArchive(t, []testTarEntry{{ + name: "nested/config.yaml", + typeflag: tar.TypeReg, + mode: 0o640, + body: []byte("version: 1\n"), + }}) + + if err := untar(archivePath, dest, false); err != nil { + t.Fatalf("untar regular file with implicit parent: %v", err) + } + info, err := os.Stat(filepath.Join(dest, "nested")) + if err != nil { + t.Fatalf("stat implicit parent: %v", err) + } + if info.Mode().Perm()&0o100 == 0 { + t.Errorf("implicit parent mode %#o is not searchable", info.Mode().Perm()) + } + info, err = os.Stat(filepath.Join(dest, "nested", "config.yaml")) + if err != nil { + t.Fatalf("stat extracted file: %v", err) + } + if got := info.Mode().Perm(); got != 0o640 { + t.Errorf("extracted file mode %#o, want %#o", got, os.FileMode(0o640)) + } +} + +func makeTestArchive(t *testing.T, entries []testTarEntry) []byte { + t.Helper() + + var buf bytes.Buffer + gzipWriter := gzip.NewWriter(&buf) + tarWriter := tar.NewWriter(gzipWriter) + for _, entry := range entries { + header := &tar.Header{ + Name: entry.name, + Mode: entry.mode, + Size: int64(len(entry.body)), + Typeflag: entry.typeflag, + Linkname: entry.linkname, + } + if err := tarWriter.WriteHeader(header); err != nil { + t.Fatalf("write tar header %q: %v", entry.name, err) + } + if len(entry.body) > 0 { + if _, err := tarWriter.Write(entry.body); err != nil { + t.Fatalf("write tar body %q: %v", entry.name, err) + } + } + } + if err := tarWriter.Close(); err != nil { + t.Fatalf("close tar writer: %v", err) + } + if err := gzipWriter.Close(); err != nil { + t.Fatalf("close gzip writer: %v", err) + } + return buf.Bytes() +} + +func writeTestArchive(t *testing.T, entries []testTarEntry) string { + t.Helper() + + archivePath := filepath.Join(t.TempDir(), "fixture.tar.gz") + if err := os.WriteFile(archivePath, makeTestArchive(t, entries), 0o600); err != nil { + t.Fatalf("write archive fixture: %v", err) + } + return archivePath +} + +func makeTruncatedTestArchive(t *testing.T) []byte { + t.Helper() + + var buf bytes.Buffer + gzipWriter := gzip.NewWriter(&buf) + tarWriter := tar.NewWriter(gzipWriter) + header := &tar.Header{ + Name: "partial.txt", + Mode: 0o644, + Size: 1024, + Typeflag: tar.TypeReg, + } + if err := tarWriter.WriteHeader(header); err != nil { + t.Fatalf("write truncated tar header: %v", err) + } + if _, err := tarWriter.Write([]byte("partial")); err != nil { + t.Fatalf("write truncated tar body: %v", err) + } + // Intentionally leave the tar writer open so the promised body and trailer are absent. + if err := gzipWriter.Close(); err != nil { + t.Fatalf("close truncated gzip stream: %v", err) + } + return buf.Bytes() +} + +func makeHeaderOnlyTestArchive(t *testing.T, size int64) []byte { + t.Helper() + + var buf bytes.Buffer + gzipWriter := gzip.NewWriter(&buf) + tarWriter := tar.NewWriter(gzipWriter) + if err := tarWriter.WriteHeader(&tar.Header{ + Name: "oversized.bin", + Mode: 0o600, + Size: size, + Typeflag: tar.TypeReg, + }); err != nil { + t.Fatalf("write oversized tar header: %v", err) + } + // The extractor should reject from the header, so no enormous body is needed. + if err := gzipWriter.Close(); err != nil { + t.Fatalf("close oversized gzip stream: %v", err) + } + return buf.Bytes() +} + +func assertPathDoesNotExist(t *testing.T, path string) { + t.Helper() + + if _, err := os.Lstat(path); err == nil { + t.Errorf("unexpected archive output at %q", path) + } else if !os.IsNotExist(err) { + t.Errorf("stat unexpected archive output %q: %v", path, err) + } +} diff --git a/runtime/pkg/archive/download_test.go b/runtime/pkg/archive/download_test.go new file mode 100644 index 000000000000..bebeea97dd5e --- /dev/null +++ b/runtime/pkg/archive/download_test.go @@ -0,0 +1,279 @@ +package archive + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "runtime" + "strings" + "testing" +) + +func TestDownloadCleanPreservesProjectOnFailure(t *testing.T) { + // clean=true must behave transactionally: any pre-install failure preserves the active project. + t.Run("HTTP transport failure", func(t *testing.T) { + // A request that never produces a response must leave the live project and temp path untouched. + root, projectPath, downloadPath := prepareExistingProject(t) + transportErr := errors.New("transport unavailable") + setDefaultHTTPClient(t, &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return nil, transportErr + })}) + + err := Download(context.Background(), "http://archive.test/project.tar.gz", downloadPath, projectPath, true, false) + if !errors.Is(err, transportErr) { + t.Errorf("Download error = %v, want transport error", err) + } + assertExistingProjectPreserved(t, root, projectPath, downloadPath) + }) + + t.Run("non-200 response", func(t *testing.T) { + // An HTTP error response is not an archive and must be rejected before replacement begins. + root, projectPath, downloadPath := prepareExistingProject(t) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "unavailable", http.StatusServiceUnavailable) + })) + t.Cleanup(server.Close) + + err := Download(context.Background(), server.URL, downloadPath, projectPath, true, false) + if err == nil { + t.Error("Download accepted a non-200 response") + } + assertExistingProjectPreserved(t, root, projectPath, downloadPath) + }) + + t.Run("corrupt archive", func(t *testing.T) { + // Gzip-compressed garbage reaches archive validation but must not delete the prior project. + root, projectPath, downloadPath := prepareExistingProject(t) + corruptBody := makeCorruptGzip(t) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(corruptBody) + })) + t.Cleanup(server.Close) + + err := Download(context.Background(), server.URL, downloadPath, projectPath, true, false) + if err == nil { + t.Error("Download accepted a corrupt archive") + } + assertExistingProjectPreserved(t, root, projectPath, downloadPath) + }) + + t.Run("interrupted response body", func(t *testing.T) { + // The synthetic body returns bytes and then an error, modeling a connection lost mid-download. + root, projectPath, downloadPath := prepareExistingProject(t) + bodyErr := errors.New("response body interrupted") + setDefaultHTTPClient(t, &http.Client{Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Status: "200 OK", + Header: make(http.Header), + Body: io.NopCloser(io.MultiReader( + strings.NewReader("partial archive bytes"), + errorReader{err: bodyErr}, + )), + Request: req, + }, nil + })}) + + err := Download(context.Background(), "http://archive.test/project.tar.gz", downloadPath, projectPath, true, false) + if !errors.Is(err, bodyErr) { + t.Errorf("Download error = %v, want interrupted-body error", err) + } + assertExistingProjectPreserved(t, root, projectPath, downloadPath) + }) + + t.Run("context canceled while body is delayed", func(t *testing.T) { + // Cancellation during a stalled response must abort before staging or replacing the live project. + root, projectPath, downloadPath := prepareExistingProject(t) + requestStarted := make(chan struct{}) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + close(requestStarted) + <-r.Context().Done() + })) + t.Cleanup(server.Close) + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan error, 1) + go func() { + done <- Download(ctx, server.URL, downloadPath, projectPath, true, false) + }() + <-requestStarted + cancel() + if err := <-done; !errors.Is(err, context.Canceled) { + t.Errorf("Download error = %v, want context cancellation", err) + } + assertExistingProjectPreserved(t, root, projectPath, downloadPath) + }) + + t.Run("oversized response", func(t *testing.T) { + // A declared oversized body is rejected before reading attacker-controlled data or touching the project. + root, projectPath, downloadPath := prepareExistingProject(t) + setDefaultHTTPClient(t, &http.Client{Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Status: "200 OK", + Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("not read")), + ContentLength: maxArchiveBytes + 1, + Request: req, + }, nil + })}) + + err := Download(context.Background(), "http://archive.test/project.tar.gz", downloadPath, projectPath, true, false) + if err == nil { + t.Error("Download accepted an oversized archive") + } + assertExistingProjectPreserved(t, root, projectPath, downloadPath) + }) +} + +func TestDownloadCleanReplacesProject(t *testing.T) { + // A fully validated download should remove stale files and install the archive as the new project. + _, projectPath, downloadPath := prepareExistingProject(t) + archiveBody := makeTestArchive(t, []testTarEntry{ + {name: "bin/", typeflag: tar.TypeDir, mode: 0o700}, + {name: "bin/run.sh", typeflag: tar.TypeReg, mode: 0o751, body: []byte("#!/bin/sh\n")}, + {name: "rill.yaml", typeflag: tar.TypeReg, mode: 0o640, body: []byte("olap_connector: duckdb\n")}, + }) + serverURL := serveTestArchive(t, archiveBody) + + if err := Download(context.Background(), serverURL, downloadPath, projectPath, true, false); err != nil { + t.Fatalf("Download returned an error: %v", err) + } + assertPathDoesNotExist(t, filepath.Join(projectPath, "keep.txt")) + assertPathDoesNotExist(t, filepath.Join(projectPath, "state", "current.json")) + assertFileContent(t, filepath.Join(projectPath, "bin", "run.sh"), "#!/bin/sh\n") + assertFileContent(t, filepath.Join(projectPath, "rill.yaml"), "olap_connector: duckdb\n") + assertPathDoesNotExist(t, downloadPath) +} + +func TestDownloadPreservesArchivedModes(t *testing.T) { + // Unix mode bits carry executable and access intent; platforms without them skip this contract. + if runtime.GOOS == "windows" { + t.Skip("Windows does not preserve Unix archive permission bits") + } + + root := t.TempDir() + projectPath := filepath.Join(root, "project") + downloadPath := filepath.Join(root, "download.tar.gz") + // The explicit directory header isolates archived-mode restoration from implicit-parent creation. + archiveBody := makeTestArchive(t, []testTarEntry{ + {name: "bin/", typeflag: tar.TypeDir, mode: 0o700}, + {name: "bin/run.sh", typeflag: tar.TypeReg, mode: 0o751, body: []byte("#!/bin/sh\n")}, + {name: "rill.yaml", typeflag: tar.TypeReg, mode: 0o640, body: []byte("olap_connector: duckdb\n")}, + }) + serverURL := serveTestArchive(t, archiveBody) + + if err := Download(context.Background(), serverURL, downloadPath, projectPath, true, false); err != nil { + t.Fatalf("Download returned an error: %v", err) + } + assertFileMode(t, filepath.Join(projectPath, "bin"), 0o700) + assertFileMode(t, filepath.Join(projectPath, "bin", "run.sh"), 0o751) + assertFileMode(t, filepath.Join(projectPath, "rill.yaml"), 0o640) +} + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { + return f(req) +} + +type errorReader struct { + err error +} + +func (r errorReader) Read([]byte) (int, error) { + return 0, r.err +} + +func setDefaultHTTPClient(t *testing.T, client *http.Client) { + t.Helper() + + previous := http.DefaultClient + http.DefaultClient = client + t.Cleanup(func() { + http.DefaultClient = previous + }) +} + +func serveTestArchive(t *testing.T, body []byte) string { + t.Helper() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(body) + })) + t.Cleanup(server.Close) + return server.URL +} + +func prepareExistingProject(t *testing.T) (root, projectPath, downloadPath string) { + t.Helper() + + root = t.TempDir() + projectPath = filepath.Join(root, "project") + downloadPath = filepath.Join(root, "download.tar.gz") + if err := os.MkdirAll(filepath.Join(projectPath, "state"), 0o750); err != nil { + t.Fatalf("create existing project: %v", err) + } + if err := os.WriteFile(filepath.Join(projectPath, "keep.txt"), []byte("keep me"), 0o640); err != nil { + t.Fatalf("write existing project file: %v", err) + } + if err := os.WriteFile(filepath.Join(projectPath, "state", "current.json"), []byte("{\"version\":1}"), 0o600); err != nil { + t.Fatalf("write existing nested project file: %v", err) + } + return root, projectPath, downloadPath +} + +func assertExistingProjectPreserved(t *testing.T, root, projectPath, downloadPath string) { + t.Helper() + + assertFileContent(t, filepath.Join(projectPath, "keep.txt"), "keep me") + assertFileContent(t, filepath.Join(projectPath, "state", "current.json"), "{\"version\":1}") + assertPathDoesNotExist(t, filepath.Join(root, "unexpected.txt")) + assertPathDoesNotExist(t, downloadPath) +} + +func assertFileContent(t *testing.T, path, want string) { + t.Helper() + + got, err := os.ReadFile(path) + if err != nil { + t.Errorf("read %q: %v", path, err) + return + } + if string(got) != want { + t.Errorf("content of %q = %q, want %q", path, got, want) + } +} + +func assertFileMode(t *testing.T, path string, want os.FileMode) { + t.Helper() + + info, err := os.Stat(path) + if err != nil { + t.Errorf("stat %q: %v", path, err) + return + } + if got := info.Mode().Perm(); got != want { + t.Errorf("mode of %q = %#o, want %#o", path, got, want) + } +} + +func makeCorruptGzip(t *testing.T) []byte { + t.Helper() + + var buf bytes.Buffer + writer := gzip.NewWriter(&buf) + if _, err := writer.Write([]byte("this is not a tar stream")); err != nil { + t.Fatalf("write corrupt gzip fixture: %v", err) + } + if err := writer.Close(); err != nil { + t.Fatalf("close corrupt gzip fixture: %v", err) + } + return buf.Bytes() +} diff --git a/runtime/storage/storage.go b/runtime/storage/storage.go index 04e4dca11db5..d700a6593e8e 100644 --- a/runtime/storage/storage.go +++ b/runtime/storage/storage.go @@ -7,6 +7,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/mitchellh/mapstructure" "github.com/rilldata/rill/runtime/pkg/gcputil" @@ -27,6 +28,12 @@ func New(dataDir string, bucketCfg map[string]any) (*Client, error) { if err != nil { return nil, err } + cleanupTempDir := true + defer func() { + if cleanupTempDir { + _ = os.RemoveAll(tempDirPath) + } + }() c := &Client{ dataDirPath: dataDir, tempDirPath: tempDirPath, @@ -40,6 +47,7 @@ func New(dataDir string, bucketCfg map[string]any) (*Client, error) { } c.bucketConfig = gcsBucketConfig } + cleanupTempDir = false return c, nil } @@ -66,12 +74,20 @@ func (c *Client) RemovePrefix(ctx context.Context, prefix ...string) error { if c.prefixes != nil { return fmt.Errorf("storage: RemovePrefix is not supported for prefixed client") } + dataPath, err := containedPath(c.dataDirPath, prefix...) + if err != nil { + return fmt.Errorf("storage: invalid removal prefix: %w", err) + } + tempPath, err := containedPath(c.tempDirPath, prefix...) + if err != nil { + return fmt.Errorf("storage: invalid removal prefix: %w", err) + } // clean data dir - removeErr := os.RemoveAll(c.path(c.dataDirPath, prefix...)) + removeErr := os.RemoveAll(dataPath) // clean temp dir - removeErr = errors.Join(removeErr, os.RemoveAll(c.path(c.tempDirPath, prefix...))) + removeErr = errors.Join(removeErr, os.RemoveAll(tempPath)) // clean bucket bkt, ok, err := c.OpenBucket(ctx, prefix...) @@ -166,6 +182,19 @@ func (c *Client) path(base string, elem ...string) string { return filepath.Join(paths...) } +func containedPath(base string, elem ...string) (string, error) { + base = filepath.Clean(base) + target := filepath.Join(append([]string{base}, elem...)...) + rel, err := filepath.Rel(base, target) + if err != nil { + return "", err + } + if rel == ".." || filepath.IsAbs(rel) || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("path %q escapes root %q", target, base) + } + return target, nil +} + func (c *Client) newGCPClient(ctx context.Context) (*gcp.HTTPClient, error) { creds, err := gcputil.Credentials(ctx, c.bucketConfig.GoogleApplicationCredentialsJSON, false) if err != nil { diff --git a/runtime/storage/storage_test.go b/runtime/storage/storage_test.go index 2aebc854b714..cfb467cbaac1 100644 --- a/runtime/storage/storage_test.go +++ b/runtime/storage/storage_test.go @@ -1,13 +1,258 @@ package storage import ( + "context" "os" "path/filepath" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestNew(t *testing.T) { + // These cases pin the constructor's ownership of its scratch directory and its error paths. + t.Run("initializes local client", func(t *testing.T) { + // A successful client must have a private scratch directory without eagerly creating its data directory. + dataDir := filepath.Join(t.TempDir(), "data") + client, err := New(dataDir, nil) + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(client.tempDirPath) }) + + require.Equal(t, dataDir, client.dataDirPath) + require.Nil(t, client.bucketConfig) + require.Nil(t, client.prefixes) + require.DirExists(t, client.tempDirPath) + require.NoDirExists(t, dataDir) + }) + + t.Run("decodes bucket configuration", func(t *testing.T) { + // Decoding is tested without opening the bucket so the constructor remains a local-only operation. + client, err := New(t.TempDir(), map[string]any{ + "bucket": "test-bucket", + "google_application_credentials_json": "test-credentials", + }) + require.NoError(t, err) + t.Cleanup(func() { _ = os.RemoveAll(client.tempDirPath) }) + + require.Equal(t, &gcsBucketConfig{ + Bucket: "test-bucket", + GoogleApplicationCredentialsJSON: "test-credentials", + }, client.bucketConfig) + }) + + t.Run("propagates temp directory creation failure", func(t *testing.T) { + // Pointing TMPDIR at a regular file deterministically makes MkdirTemp fail before client construction. + tmpFile := filepath.Join(t.TempDir(), "not-a-directory") + require.NoError(t, os.WriteFile(tmpFile, []byte("fixture"), 0o600)) + t.Setenv("TMPDIR", tmpFile) + + client, err := New(filepath.Join(t.TempDir(), "data"), nil) + require.Error(t, err) + require.Nil(t, client) + }) + + t.Run("cleans temp directory after configuration failure", func(t *testing.T) { + // New owns the scratch directory, so a later decode error must not leak it. + tmpParent := filepath.Join(t.TempDir(), "scratch") + require.NoError(t, os.Mkdir(tmpParent, 0o700)) + t.Setenv("TMPDIR", tmpParent) + + client, err := New(filepath.Join(t.TempDir(), "data"), map[string]any{ + "bucket": make(chan struct{}), + }) + require.Error(t, err) + require.Nil(t, client) + + entries, readErr := os.ReadDir(tmpParent) + require.NoError(t, readErr) + require.Empty(t, entries, "constructor-owned scratch directory leaked after decode failure") + }) +} + +func TestClient_WithPrefix(t *testing.T) { + // Derived clients must compose prefixes without mutating the clients they were derived from. + root := t.TempDir() + bucketConfig := &gcsBucketConfig{Bucket: "test-bucket"} + client := &Client{ + dataDirPath: filepath.Join(root, "data"), + tempDirPath: filepath.Join(root, "temp"), + bucketConfig: bucketConfig, + } + + tenantClient := client.WithPrefix("tenant") + projectClient := tenantClient.WithPrefix("project", "run") + + require.Nil(t, client.prefixes) + require.Equal(t, []string{"tenant"}, tenantClient.prefixes) + require.Equal(t, []string{"tenant", "project", "run"}, projectClient.prefixes) + require.Same(t, bucketConfig, projectClient.bucketConfig) + + dataDir, err := projectClient.DataDir("artifact") + require.NoError(t, err) + tempDir, err := projectClient.TempDir("artifact") + require.NoError(t, err) + require.Equal(t, filepath.Join(root, "data", "tenant", "project", "run", "artifact"), dataDir) + require.Equal(t, filepath.Join(root, "temp", "tenant", "project", "run", "artifact"), tempDir) +} + +func TestClient_RemovePrefixContainment(t *testing.T) { + // Each case proves that removal is confined to the client's two local roots. + t.Run("empty prefix removes only storage roots", func(t *testing.T) { + // Separate parents make the sibling sentinels meaningful even when both roots are intentionally removed. + fixture := newRemovePrefixFixture(t) + + err := fixture.client.RemovePrefix(context.Background()) + require.NoError(t, err) + require.NoDirExists(t, fixture.dataRoot) + require.NoDirExists(t, fixture.tempRoot) + require.FileExists(t, fixture.dataOutsideSentinel) + require.FileExists(t, fixture.tempOutsideSentinel) + }) + + t.Run("normal prefix removes only matching subtrees", func(t *testing.T) { + // Target and sibling files distinguish the requested prefix from the rest of each root. + fixture := newRemovePrefixFixture(t) + dataTarget := writeFixtureFile(t, fixture.dataRoot, "target", "remove-me") + tempTarget := writeFixtureFile(t, fixture.tempRoot, "target", "remove-me") + dataSibling := writeFixtureFile(t, fixture.dataRoot, "keep", "sentinel") + tempSibling := writeFixtureFile(t, fixture.tempRoot, "keep", "sentinel") + + err := fixture.client.RemovePrefix(context.Background(), "target") + require.NoError(t, err) + require.NoFileExists(t, dataTarget) + require.NoFileExists(t, tempTarget) + require.FileExists(t, dataSibling) + require.FileExists(t, tempSibling) + require.FileExists(t, fixture.dataOutsideSentinel) + require.FileExists(t, fixture.tempOutsideSentinel) + }) + + traversalCases := []struct { + name string + prefix []string + }{ + {name: "parent traversal", prefix: []string{".."}}, + {name: "nested parent traversal", prefix: []string{"nested", "..", ".."}}, + } + for _, tt := range traversalCases { + t.Run(tt.name, func(t *testing.T) { + // Roots live one level below sentinels so an unvalidated traversal would visibly delete outside data. + fixture := newRemovePrefixFixture(t) + + err := fixture.client.RemovePrefix(context.Background(), tt.prefix...) + assert.Error(t, err, "traversal prefix should be rejected") + assert.FileExists(t, fixture.dataRootSentinel) + assert.FileExists(t, fixture.tempRootSentinel) + assert.FileExists(t, fixture.dataOutsideSentinel) + assert.FileExists(t, fixture.tempOutsideSentinel) + }) + } +} + +func TestClient_RemovePrefixRejectsPrefixedClient(t *testing.T) { + // A derived client must not reinterpret a removal prefix relative to its hidden base prefix. + fixture := newRemovePrefixFixture(t) + dataSentinel := writeFixtureFile(t, fixture.dataRoot, "tenant", "child", "data") + tempSentinel := writeFixtureFile(t, fixture.tempRoot, "tenant", "child", "temp") + + err := fixture.client.WithPrefix("tenant").RemovePrefix(context.Background(), "child") + require.EqualError(t, err, "storage: RemovePrefix is not supported for prefixed client") + require.FileExists(t, dataSentinel) + require.FileExists(t, tempSentinel) + require.FileExists(t, fixture.dataOutsideSentinel) + require.FileExists(t, fixture.tempOutsideSentinel) +} + +func TestClient_RemovePrefixLocalFailureCleanup(t *testing.T) { + // Removal must report either local failure while still attempting cleanup of the other root. + t.Run("data removal failure still cleans temp prefix", func(t *testing.T) { + // A regular file used as dataDir makes removal below it fail with ENOTDIR on every local filesystem. + root := t.TempDir() + dataFile := filepath.Join(root, "data-file") + require.NoError(t, os.WriteFile(dataFile, []byte("fixture"), 0o600)) + tempRoot := filepath.Join(root, "temp") + tempTarget := writeFixtureFile(t, tempRoot, "target", "remove-me") + client := &Client{dataDirPath: dataFile, tempDirPath: tempRoot} + + err := client.RemovePrefix(context.Background(), "target") + require.Error(t, err) + require.FileExists(t, dataFile) + require.NoFileExists(t, tempTarget) + }) + + t.Run("temp removal failure follows data cleanup", func(t *testing.T) { + // A regular file used as tempDir forces the second removal to fail after data cleanup succeeds. + root := t.TempDir() + dataRoot := filepath.Join(root, "data") + dataTarget := writeFixtureFile(t, dataRoot, "target", "remove-me") + tempFile := filepath.Join(root, "temp-file") + require.NoError(t, os.WriteFile(tempFile, []byte("fixture"), 0o600)) + client := &Client{dataDirPath: dataRoot, tempDirPath: tempFile} + + err := client.RemovePrefix(context.Background(), "target") + require.Error(t, err) + require.NoFileExists(t, dataTarget) + require.FileExists(t, tempFile) + }) +} + +func TestClient_RemovePrefixBucketFailureAfterLocalCleanup(t *testing.T) { + // Bucket setup failures must be returned after deterministic local cleanup has completed. + fixture := newRemovePrefixFixture(t) + dataTarget := writeFixtureFile(t, fixture.dataRoot, "target", "remove-me") + tempTarget := writeFixtureFile(t, fixture.tempRoot, "target", "remove-me") + fixture.client.bucketConfig = &gcsBucketConfig{ + Bucket: "unused-test-bucket", + GoogleApplicationCredentialsJSON: "{invalid-json", + } + + err := fixture.client.RemovePrefix(context.Background(), "target") + require.ErrorContains(t, err, "could not create GCP client") + require.NoFileExists(t, dataTarget) + require.NoFileExists(t, tempTarget) + require.FileExists(t, fixture.dataOutsideSentinel) + require.FileExists(t, fixture.tempOutsideSentinel) +} + +type removePrefixFixture struct { + client *Client + dataRoot string + tempRoot string + dataRootSentinel string + tempRootSentinel string + dataOutsideSentinel string + tempOutsideSentinel string +} + +func newRemovePrefixFixture(t *testing.T) *removePrefixFixture { + t.Helper() + // Independent parents prevent one traversal deletion from masking whether cleanup escaped the other root. + sandbox := t.TempDir() + dataParent := filepath.Join(sandbox, "data-parent") + tempParent := filepath.Join(sandbox, "temp-parent") + dataRoot := filepath.Join(dataParent, "root") + tempRoot := filepath.Join(tempParent, "root") + + return &removePrefixFixture{ + client: &Client{dataDirPath: dataRoot, tempDirPath: tempRoot}, + dataRoot: dataRoot, + tempRoot: tempRoot, + dataRootSentinel: writeFixtureFile(t, dataRoot, "root-sentinel"), + tempRootSentinel: writeFixtureFile(t, tempRoot, "root-sentinel"), + dataOutsideSentinel: writeFixtureFile(t, dataParent, "outside-sentinel"), + tempOutsideSentinel: writeFixtureFile(t, tempParent, "outside-sentinel"), + } +} + +func writeFixtureFile(t *testing.T, elem ...string) string { + t.Helper() + path := filepath.Join(elem...) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + require.NoError(t, os.WriteFile(path, []byte("sentinel"), 0o600)) + return path +} + func TestClient_DataDir(t *testing.T) { tempDir := t.TempDir() client := &Client{ From 5d02919a876a160ca4b85b60f0fd48481f863d6a Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Thu, 23 Jul 2026 10:26:08 +0530 Subject: [PATCH 2/3] fix: make archive copy bound explicit --- runtime/pkg/archive/archive.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/runtime/pkg/archive/archive.go b/runtime/pkg/archive/archive.go index e2d422bdebd4..32776df8f952 100644 --- a/runtime/pkg/archive/archive.go +++ b/runtime/pkg/archive/archive.go @@ -265,7 +265,9 @@ func untar(src, dest string, ignorePaths bool) error { if err != nil { return err } - _, copyErr := io.Copy(outFile, tarReader) + // Copy exactly the validated header size so both the implementation and + // static analysis retain the decompression bound. + _, copyErr := io.CopyN(outFile, tarReader, header.Size) closeErr := outFile.Close() if copyErr != nil || closeErr != nil { return errors.Join(copyErr, closeErr) From 349e5b51291e33ff29f6277018b4104570541aa5 Mon Sep 17 00:00:00 2001 From: Nishant Bangarwa Date: Thu, 23 Jul 2026 11:49:41 +0530 Subject: [PATCH 3/3] fix: preserve project-root archive paths --- runtime/pkg/archive/archive.go | 13 +++++- runtime/pkg/archive/archive_test.go | 69 ++++++++++++++++++++++++++--- 2 files changed, 76 insertions(+), 6 deletions(-) diff --git a/runtime/pkg/archive/archive.go b/runtime/pkg/archive/archive.go index 32776df8f952..32cdeef61718 100644 --- a/runtime/pkg/archive/archive.go +++ b/runtime/pkg/archive/archive.go @@ -290,7 +290,18 @@ func untar(src, dest string, ignorePaths bool) error { } func sanitizeArchivePath(dest, tarPath string) (v string, err error) { - if tarPath == "" || filepath.IsAbs(tarPath) || filepath.VolumeName(tarPath) != "" { + if tarPath == "" { + return "", fmt.Errorf("%s: %s", "content filepath is tainted", tarPath) + } + // Project archives historically use a leading slash for paths relative to + // the project root. Strip that archive marker before applying the containment + // check; traversal such as "/../outside" remains outside and is rejected. + tarPath = strings.TrimLeft(tarPath, "/") + if tarPath == "" { + tarPath = "." + } + tarPath = filepath.FromSlash(tarPath) + if filepath.IsAbs(tarPath) || filepath.VolumeName(tarPath) != "" { return "", fmt.Errorf("%s: %s", "content filepath is tainted", tarPath) } diff --git a/runtime/pkg/archive/archive_test.go b/runtime/pkg/archive/archive_test.go index 6821e56dfeae..aa6832b42798 100644 --- a/runtime/pkg/archive/archive_test.go +++ b/runtime/pkg/archive/archive_test.go @@ -4,9 +4,12 @@ import ( "archive/tar" "bytes" "compress/gzip" + "context" "os" "path/filepath" "testing" + + "github.com/rilldata/rill/runtime/drivers" ) type testTarEntry struct { @@ -53,25 +56,81 @@ func TestUntarRejectsPathTraversal(t *testing.T) { assertPathDoesNotExist(t, filepath.Join(root, "project-copy", "escaped.txt")) }) - t.Run("absolute path", func(t *testing.T) { - // Absolute names must be rejected; a temp target and executable mode make a vulnerable rebase safe to observe. + t.Run("root-relative parent traversal", func(t *testing.T) { + // Removing Rill's leading project-root marker must not normalize a parent + // traversal into an accepted path. + root := t.TempDir() + dest := filepath.Join(root, "project") + archivePath := writeTestArchive(t, []testTarEntry{{ + name: "/../escaped.txt", + typeflag: tar.TypeReg, + mode: 0o644, + body: []byte("escaped"), + }}) + + if err := untar(archivePath, dest, false); err == nil { + t.Error("untar accepted a root-relative parent traversal") + } + assertPathDoesNotExist(t, filepath.Join(root, "escaped.txt")) + }) + + t.Run("root-relative path", func(t *testing.T) { + // Rill archives use a leading slash as a project-root marker. Extraction + // must remove that marker without writing to the matching filesystem path. root := t.TempDir() dest := filepath.Join(root, "project") absoluteTarget := filepath.Join(root, "absolute.txt") archivePath := writeTestArchive(t, []testTarEntry{{ - name: filepath.ToSlash(absoluteTarget), + name: "/absolute.txt", typeflag: tar.TypeReg, mode: 0o755, body: []byte("absolute"), }}) - if err := untar(archivePath, dest, false); err == nil { - t.Error("untar accepted an absolute archive member") + if err := untar(archivePath, dest, false); err != nil { + t.Fatalf("untar root-relative archive member: %v", err) } assertPathDoesNotExist(t, absoluteTarget) + assertFileContent(t, filepath.Join(dest, "absolute.txt"), "absolute") }) } +func TestCreateAndUntarRoundTripWithProjectRootPaths(t *testing.T) { + // This exercises the producer and consumer together because repository + // listings use slash-prefixed paths, including a root directory entry. + source := t.TempDir() + if err := os.MkdirAll(filepath.Join(source, "models"), 0o755); err != nil { + t.Fatalf("create source directory: %v", err) + } + if err := os.WriteFile(filepath.Join(source, "rill.yaml"), []byte("title: Example\n"), 0o644); err != nil { + t.Fatalf("write source project file: %v", err) + } + if err := os.WriteFile(filepath.Join(source, "models", "orders.sql"), []byte("select 1\n"), 0o644); err != nil { + t.Fatalf("write source model: %v", err) + } + + contents, err := Create(context.Background(), []drivers.DirEntry{ + {Path: "/", IsDir: true}, + {Path: "/rill.yaml"}, + {Path: "/models", IsDir: true}, + {Path: "/models/orders.sql"}, + }, source) + if err != nil { + t.Fatalf("create project archive: %v", err) + } + archivePath := filepath.Join(t.TempDir(), "project.tar.gz") + if err := os.WriteFile(archivePath, contents.Bytes(), 0o600); err != nil { + t.Fatalf("write project archive: %v", err) + } + + dest := filepath.Join(t.TempDir(), "project") + if err := untar(archivePath, dest, false); err != nil { + t.Fatalf("untar project archive: %v", err) + } + assertFileContent(t, filepath.Join(dest, "rill.yaml"), "title: Example\n") + assertFileContent(t, filepath.Join(dest, "models", "orders.sql"), "select 1\n") +} + func TestUntarRejectsLinkAndUnsupportedEntries(t *testing.T) { // Extraction only supports directories and regular files, so special entries must fail closed. t.Run("symbolic link", func(t *testing.T) {