From 0491849f0fde22ff01f6336af280df2bc87f902b Mon Sep 17 00:00:00 2001
From: "alexei.dolgolyov" <dolgolyov.alexei@gmail.com>
Date: Wed, 1 Apr 2026 23:17:35 +0300
Subject: [PATCH] fix(volume-browser): address security review findings

Critical fixes:
- IDOR: verify volume belongs to project before resolving path
- Upload: override global 1MB body limit for upload endpoint (100MB)

High-priority fixes:
- Symlink escape: use filepath.EvalSymlinks in safePath validation
- Remove host filesystem path from browse API response
- Sanitize Content-Disposition filenames, force application/octet-stream
- Strip directory components from upload filenames
---
 internal/api/volume_browser.go                | 35 +++++++++++++++----
 internal/volume/browser.go                    | 12 ++++++-
 web/src/lib/types.ts                          |  1 -
 .../[id]/volumes/[volId]/browse/+page.svelte  |  5 ---
 4 files changed, 40 insertions(+), 13 deletions(-)

diff --git a/internal/api/volume_browser.go b/internal/api/volume_browser.go
index aecdc0a..41dc4ee 100644
--- a/internal/api/volume_browser.go
+++ b/internal/api/volume_browser.go
@@ -7,6 +7,7 @@ import (
 	"log/slog"
 	"net/http"
 	"path/filepath"
+	"strings"
 
 	"github.com/go-chi/chi/v5"
 
@@ -14,6 +15,16 @@ import (
 	"github.com/alexei/docker-watcher/internal/volume"
 )
 
+// sanitizeFilename removes characters unsafe for Content-Disposition headers.
+func sanitizeFilename(name string) string {
+	return strings.Map(func(r rune) rune {
+		if r == '"' || r == '\\' || r == '\n' || r == '\r' {
+			return '_'
+		}
+		return r
+	}, name)
+}
+
 const maxUploadSize = 100 * 1024 * 1024 // 100MB
 
 // resolveVolumeRoot looks up a volume and resolves its host path.
@@ -43,6 +54,12 @@ func (s *Server) resolveVolumeRoot(w http.ResponseWriter, r *http.Request) (stri
 		return "", false
 	}
 
+	// Verify volume belongs to this project.
+	if vol.ProjectID != projectID {
+		respondNotFound(w, "volume")
+		return "", false
+	}
+
 	if vol.Scope == "ephemeral" {
 		respondError(w, http.StatusBadRequest, "ephemeral volumes have no host path to browse")
 		return "", false
@@ -89,7 +106,6 @@ func (s *Server) browseVolume(w http.ResponseWriter, r *http.Request) {
 
 	respondJSON(w, http.StatusOK, map[string]any{
 		"path":    relPath,
-		"root":    rootPath,
 		"entries": entries,
 	})
 }
@@ -126,16 +142,19 @@ func (s *Server) downloadVolume(w http.ResponseWriter, r *http.Request) {
 	}
 	defer f.Close()
 
-	// Serve single file.
-	w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, filepath.Base(relPath)))
+	// Serve single file with forced download.
+	safeName := sanitizeFilename(filepath.Base(relPath))
+	w.Header().Set("Content-Type", "application/octet-stream")
+	w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s"`, safeName))
 	w.Header().Set("Content-Length", fmt.Sprintf("%d", info.Size()))
 	w.WriteHeader(http.StatusOK)
 	io.Copy(w, f)
 }
 
 func (s *Server) serveZip(w http.ResponseWriter, rootPath, relPath, name string) {
+	safeName := sanitizeFilename(name)
 	w.Header().Set("Content-Type", "application/zip")
-	w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s.zip"`, name))
+	w.Header().Set("Content-Disposition", fmt.Sprintf(`attachment; filename="%s.zip"`, safeName))
 	w.WriteHeader(http.StatusOK)
 	if err := volume.WriteZip(rootPath, relPath, w); err != nil {
 		slog.Error("failed to write zip", "root", rootPath, "path", relPath, "error", err)
@@ -143,8 +162,11 @@ func (s *Server) serveZip(w http.ResponseWriter, rootPath, relPath, name string)
 }
 
 // uploadToVolume handles POST /api/projects/{id}/volumes/{volId}/upload?path=&stage=&tag=
-// Accepts multipart form uploads.
+// Accepts multipart form uploads. Overrides the global body limit for large files.
 func (s *Server) uploadToVolume(w http.ResponseWriter, r *http.Request) {
+	// Override the global 1MB body limit for uploads.
+	r.Body = http.MaxBytesReader(w, r.Body, maxUploadSize)
+
 	rootPath, ok := s.resolveVolumeRoot(w, r)
 	if !ok {
 		return
@@ -166,7 +188,8 @@ func (s *Server) uploadToVolume(w http.ResponseWriter, r *http.Request) {
 				continue
 			}
 
-			targetRel := filepath.Join(relPath, fh.Filename)
+			// Strip directory components from filename to prevent directory creation attacks.
+			targetRel := filepath.Join(relPath, filepath.Base(fh.Filename))
 			if err := volume.SaveFile(rootPath, targetRel, f); err != nil {
 				f.Close()
 				slog.Error("failed to save upload", "filename", fh.Filename, "error", err)
diff --git a/internal/volume/browser.go b/internal/volume/browser.go
index d02e256..35cd5ca 100644
--- a/internal/volume/browser.go
+++ b/internal/volume/browser.go
@@ -168,6 +168,7 @@ func SaveFile(rootPath, relativePath string, r io.Reader) error {
 }
 
 // safePath resolves a relative path within rootPath and validates it doesn't escape.
+// Resolves symlinks to prevent symlink-based traversal attacks.
 func safePath(rootPath, relativePath string) (string, error) {
 	if relativePath == "" {
 		return rootPath, nil
@@ -181,15 +182,24 @@ func safePath(rootPath, relativePath string) (string, error) {
 
 	absPath := filepath.Join(rootPath, cleaned)
 
-	// Double-check the resolved path is within the root.
+	// Resolve the root path (follow symlinks in the root itself).
 	absRoot, err := filepath.Abs(rootPath)
 	if err != nil {
 		return "", fmt.Errorf("resolve root: %w", err)
 	}
+	if realRoot, err := filepath.EvalSymlinks(absRoot); err == nil {
+		absRoot = realRoot
+	}
+
+	// Resolve the target path including symlinks.
 	absResolved, err := filepath.Abs(absPath)
 	if err != nil {
 		return "", fmt.Errorf("resolve path: %w", err)
 	}
+	if realResolved, err := filepath.EvalSymlinks(absResolved); err == nil {
+		absResolved = realResolved
+	}
+
 	if !strings.HasPrefix(absResolved, absRoot) {
 		return "", fmt.Errorf("path traversal not allowed")
 	}
diff --git a/web/src/lib/types.ts b/web/src/lib/types.ts
index 60bf8ec..c8945ef 100644
--- a/web/src/lib/types.ts
+++ b/web/src/lib/types.ts
@@ -196,7 +196,6 @@ export interface FileEntry {
 /** Response from the volume browse endpoint. */
 export interface BrowseResult {
 	path: string;
-	root: string;
 	entries: FileEntry[];
 }
 
diff --git a/web/src/routes/projects/[id]/volumes/[volId]/browse/+page.svelte b/web/src/routes/projects/[id]/volumes/[volId]/browse/+page.svelte
index 5214ebc..d28f5f5 100644
--- a/web/src/routes/projects/[id]/volumes/[volId]/browse/+page.svelte
+++ b/web/src/routes/projects/[id]/volumes/[volId]/browse/+page.svelte
@@ -12,7 +12,6 @@
 
 	let entries = $state<FileEntry[]>([]);
 	let currentPath = $state('');
-	let rootPath = $state('');
 	let loading = $state(true);
 	let error = $state('');
 	let uploading = $state(false);
@@ -62,7 +61,6 @@
 			const result = await api.browseVolume(projectId, volId, { path, stage, tag });
 			entries = result.entries;
 			currentPath = result.path || '';
-			rootPath = result.root;
 		} catch (e) {
 			error = e instanceof Error ? e.message : $t('volumeBrowser.loadFailed');
 		} finally {
@@ -153,9 +151,6 @@
 				</label>
 			</div>
 		</div>
-		{#if rootPath}
-			<p class="mt-1 text-xs text-[var(--text-tertiary)] font-mono">{rootPath}</p>
-		{/if}
 	</div>
 
 	<!-- Breadcrumbs -->