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
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -196,7 +196,6 @@ export interface FileEntry {
|
||||
/** Response from the volume browse endpoint. */
|
||||
export interface BrowseResult {
|
||||
path: string;
|
||||
root: string;
|
||||
entries: FileEntry[];
|
||||
}
|
||||
|
||||
|
||||
@@ -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 -->
|
||||
|
||||
Reference in New Issue
Block a user