From f2ff2e3e19cd438a9d819ff0fbb007050b063769 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 22 Sep 2026 07:20:58 +0000 Subject: [PATCH] Reject path traversal in deploy log download handler (closes #177) gosec flagged G703 (path traversal via taint analysis) on the log download handler because the served path derives from a URL parameter. Open the log file through an os.Root confined to the deploy log directory instead of passing the path to http.ServeFile; Root.Open rejects any path that escapes the root, so traversal attempts return 404. Serve the opened file with http.ServeContent. Adds GetLogDir on the deploy service and unit tests covering a legitimate download and a traversal-shaped app name. Model: opus-4-8 --- TODO.md | 3 + internal/handlers/app.go | 37 +++++++--- internal/handlers/handlers_test.go | 2 + internal/handlers/log_download_test.go | 96 ++++++++++++++++++++++++++ internal/service/deploy/deploy.go | 6 ++ 5 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 internal/handlers/log_download_test.go diff --git a/TODO.md b/TODO.md index 2f051a0..72e4f9a 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,9 @@ main cannot regress. # Completed Steps +- 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), diff --git a/internal/handlers/app.go b/internal/handlers/app.go index aa83aed..a985c11 100644 --- a/internal/handlers/app.go +++ b/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) } } diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 6e0624a..68e116e 100644 --- a/internal/handlers/handlers_test.go +++ b/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, } } diff --git a/internal/handlers/log_download_test.go b/internal/handlers/log_download_test.go new file mode 100644 index 0000000..73d04d8 --- /dev/null +++ b/internal/handlers/log_download_test.go @@ -0,0 +1,96 @@ +package handlers_test + +import ( + "context" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "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 that a +// traversal-shaped app name (containing "..") — which would make the +// resolved log path escape the deploy log directory — is rejected with +// 404 rather than serving an arbitrary file. +func TestHandleDeploymentLogDownloadRejectsPathTraversal(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "log-traversal-app") + + createdApp.Name = "../../../../etc" + require.NoError(t, createdApp.Save(context.Background())) + + // Ensure the log root exists so the rejection comes from the + // containment check, not from a missing directory. + require.NoError(t, os.MkdirAll(testCtx.deploySvc.GetLogDir(), 0o750)) + + deployment := models.NewDeployment(testCtx.database) + deployment.AppID = createdApp.ID + deployment.Status = models.DeploymentStatusSuccess + require.NoError(t, deployment.Save(context.Background())) + + recorder := doLogDownload(t, testCtx, createdApp.ID, deployment.ID) + + assert.Equal(t, http.StatusNotFound, recorder.Code) +} diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 887a094..ab1f2c8 100644 --- a/internal/service/deploy/deploy.go +++ b/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)