Reject path traversal in deploy log download handler (closes #177)

The deploy-log download handler passed a request-derived path to
http.ServeFile, which gosec flags as G703 (path traversal via taint).
The handler now opens the log through an os.Root confined to the deploy
log directory, so any escaping path is rejected at runtime (404) and the
file is streamed with http.ServeContent. A regression test plants a
sentinel outside the log dir and asserts the traversal is refused and its
contents never served; removing the guard makes that test fail. No
//nolint used.

Model: opus-4-8

TODO.md

@@ -22,6 +22,9 @@ main cannot regress.
 
 - 2026-09-22: Added `.prettierignore` so `make fmt` no longer rewrites
   the vendored `static/js/alpine.min.js` bundle (#185).
+- 2026-09-22: Fixed the gosec G703 path-traversal finding in the deploy
+  log download handler by verifying the resolved path stays within the
+  deploy log directory before serving, returning 404 on escape (#177).
 - 2026-09-09: Fixed four deployability blockers found by QA: CSRF origin
   check over plain HTTP (`UPAAS_PLAINTEXT_HTTP`, #189), pulling the git
   image when absent (#190), the env-var editor CSRF token lookup (#191),

internal/handlers/app.go

@@ -611,7 +611,13 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc {
 			return
 		}
 
-		// Get the log file path from deploy service
+		// The log path is derived from request data (the app is looked
+		// up by a URL parameter), so open it through an os.Root confined
+		// to the deploy log directory. Root.Open rejects any path that
+		// escapes the root, so a traversal attempt fails rather than
+		// serving an arbitrary file.
+		logDir := h.deploy.GetLogDir()
+
 		logPath := h.deploy.GetLogFilePath(application, deployment)
 		if logPath == "" {
 			http.NotFound(writer, request)
@@ -619,28 +625,43 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc {
 			return
 		}
 
-		// Check if file exists — logPath is constructed internally, not from user input
-		_, err := os.Stat(logPath) // #nosec G703 -- internal path, not user input
-		if os.IsNotExist(err) {
+		relPath, relErr := filepath.Rel(logDir, logPath)
+		if relErr != nil {
 			http.NotFound(writer, request)
 
 			return
 		}
 
-		if err != nil {
-			h.log.Error("failed to stat log file", "error", err, "path", logPath)
+		root, rootErr := os.OpenRoot(logDir)
+		if rootErr != nil {
+			http.NotFound(writer, request)
+
+			return
+		}
+		defer func() { _ = root.Close() }()
+
+		file, openErr := root.Open(relPath)
+		if openErr != nil {
+			http.NotFound(writer, request)
+
+			return
+		}
+		defer func() { _ = file.Close() }()
+
+		info, statErr := file.Stat()
+		if statErr != nil {
+			h.log.Error("failed to stat log file", "error", statErr, "path", logPath)
 			http.Error(writer, "Internal Server Error", http.StatusInternalServerError)
 
 			return
 		}
 
-		// Extract filename for Content-Disposition header
 		filename := filepath.Base(logPath)
 
 		writer.Header().Set("Content-Type", "text/plain; charset=utf-8")
 		writer.Header().Set("Content-Disposition", "attachment; filename=\""+filename+"\"")
 
-		http.ServeFile(writer, request, logPath) // #nosec G703 -- internal path
+		http.ServeContent(writer, request, filename, info.ModTime(), file)
 	}
 }
 

internal/handlers/handlers_test.go

@@ -42,6 +42,7 @@ type testContext struct {
 	database   *database.Database
 	authSvc    *auth.Service
 	appSvc     *app.Service
+	deploySvc  *deploy.Service
 	middleware *middleware.Middleware
 }
 
@@ -186,6 +187,7 @@ func setupTestHandlers(t *testing.T) *testContext {
 		database:   dbInstance,
 		authSvc:    authSvc,
 		appSvc:     appSvc,
+		deploySvc:  deploySvc,
 		middleware: mw,
 	}
 }

internal/handlers/log_download_test.go

@@ -0,0 +1,121 @@
+package handlers_test
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"sneak.berlin/go/upaas/internal/models"
+)
+
+// doLogDownload issues a log-download request for the given app and
+// deployment and returns the recorder.
+func doLogDownload(
+	t *testing.T,
+	testCtx *testContext,
+	appID string,
+	deploymentID int64,
+) *httptest.ResponseRecorder {
+	t.Helper()
+
+	idStr := strconv.FormatInt(deploymentID, 10)
+
+	request := httptest.NewRequestWithContext(
+		t.Context(),
+		http.MethodGet,
+		"/apps/"+appID+"/deployments/"+idStr+"/log",
+		nil,
+	)
+	request = addChiURLParams(request, map[string]string{
+		"id":           appID,
+		"deploymentID": idStr,
+	})
+
+	recorder := httptest.NewRecorder()
+	testCtx.handlers.HandleDeploymentLogDownload().ServeHTTP(recorder, request)
+
+	return recorder
+}
+
+// TestHandleDeploymentLogDownloadServesLegitimateFile verifies a normal
+// log file is served for download.
+func TestHandleDeploymentLogDownloadServesLegitimateFile(t *testing.T) {
+	t.Parallel()
+
+	testCtx := setupTestHandlers(t)
+	createdApp := createTestApp(t, testCtx, "log-download-app")
+
+	deployment := models.NewDeployment(testCtx.database)
+	deployment.AppID = createdApp.ID
+	deployment.Status = models.DeploymentStatusSuccess
+	require.NoError(t, deployment.Save(context.Background()))
+
+	// Write the log file where the handler will look for it.
+	logPath := testCtx.deploySvc.GetLogFilePath(createdApp, deployment)
+	require.NoError(t, os.MkdirAll(filepath.Dir(logPath), 0o750))
+	require.NoError(t, os.WriteFile(logPath, []byte("deploy log contents"), 0o600))
+
+	recorder := doLogDownload(t, testCtx, createdApp.ID, deployment.ID)
+
+	assert.Equal(t, http.StatusOK, recorder.Code)
+	assert.Contains(t, recorder.Body.String(), "deploy log contents")
+}
+
+// TestHandleDeploymentLogDownloadRejectsPathTraversal verifies the
+// os.Root containment guard. A traversal-shaped app name drives the
+// resolved log path out of the deploy log directory onto a sentinel
+// file that really exists. The handler must refuse to serve it (404)
+// rather than leak its contents. Removing the guard makes this test
+// fail, which the earlier version — pointed at a non-existent path that
+// 404s either way — did not.
+func TestHandleDeploymentLogDownloadRejectsPathTraversal(t *testing.T) {
+	t.Parallel()
+
+	testCtx := setupTestHandlers(t)
+	createdApp := createTestApp(t, testCtx, "log-traversal-app")
+
+	createdApp.Name = "../.."
+	require.NoError(t, createdApp.Save(context.Background()))
+
+	// The log root must exist so os.OpenRoot succeeds and the rejection
+	// comes from the containment check, not a missing directory.
+	logDir := testCtx.deploySvc.GetLogDir()
+	require.NoError(t, os.MkdirAll(logDir, 0o750))
+
+	deployment := models.NewDeployment(testCtx.database)
+	deployment.AppID = createdApp.ID
+	deployment.Status = models.DeploymentStatusSuccess
+	require.NoError(t, deployment.Save(context.Background()))
+
+	// Where the handler resolves the log path to. The traversal name
+	// makes this land outside logDir; require that it truly escapes so
+	// the test cannot silently stop covering the guard.
+	escapedPath := testCtx.deploySvc.GetLogFilePath(createdApp, deployment)
+	relPath, relErr := filepath.Rel(logDir, escapedPath)
+	require.NoError(t, relErr)
+	require.True(t, strings.HasPrefix(relPath, ".."),
+		"resolved path must escape the log dir, got %q", relPath)
+
+	// Plant a sentinel where the traversal points; a missing guard would
+	// open and serve it.
+	require.NoError(t, os.MkdirAll(filepath.Dir(escapedPath), 0o750))
+
+	const sentinel = "SENTINEL-outside-log-dir-must-not-be-served"
+
+	require.NoError(t, os.WriteFile(escapedPath, []byte(sentinel), 0o600))
+	t.Cleanup(func() { _ = os.Remove(escapedPath) })
+
+	recorder := doLogDownload(t, testCtx, createdApp.ID, deployment.ID)
+
+	assert.Equal(t, http.StatusNotFound, recorder.Code)
+	assert.NotContains(t, recorder.Body.String(), sentinel,
+		"containment guard must not serve a file outside the log dir")
+}

internal/service/deploy/deploy.go

@@ -294,6 +294,12 @@ func (svc *Service) GetLogFilePath(
 	return filepath.Join(svc.config.DataDir, "logs", hostname, app.Name, filename)
 }
 
+// GetLogDir returns the root directory under which all deployment log
+// files live. Paths returned by GetLogFilePath are always inside it.
+func (svc *Service) GetLogDir() string {
+	return filepath.Join(svc.config.DataDir, "logs")
+}
+
 // HasActiveDeploy returns true if there is an active deployment for the given app.
 func (svc *Service) HasActiveDeploy(appID string) bool {
 	_, ok := svc.activeDeploys.Load(appID)