From b580dbbd2ce0c7e99e31347bc45ca0aaf3d072eb Mon Sep 17 00:00:00 2001 From: sneak Date: Fri, 7 Aug 2026 16:58:10 +0000 Subject: [PATCH] fix: resolve all 22 gosec G710 open-redirect findings Route every app redirect in internal/handlers/app.go through a new redirectToApp helper that parses the app ID with ulid.ParseStrict (404 on failure), re-serializes it, and path-escapes it before building the /apps/ target, so no unvalidated request input can reach http.Redirect. Also converts the equivalent unflagged redirect in HandleAppCreate for consistency. make lint under golangci-lint 2.12.2 drops from 47 findings to 25 (remaining: 1 gosec G703 tracked in #177, 24 goconst tracked in #178). make test and make fmt-check pass unchanged. Closes #176 --- TODO.md | 22 ++++++----- internal/handlers/app.go | 85 ++++++++++++++++++++++------------------ 2 files changed, 58 insertions(+), 49 deletions(-) diff --git a/TODO.md b/TODO.md index 174b68f..e2abecf 100644 --- a/TODO.md +++ b/TODO.md @@ -11,20 +11,23 @@ # Status 1.0+. Tagged 1.0.0 on 2026-02-26. Policy violation: main currently -fails make check under golangci-lint >= 2.12 (47 lint issues remaining: -23 gosec, 24 goconst), so the tree is out of compliance until fixed. CI -(Dockerfile lint stage, pinned golangci-lint v2.10.1) is green; the pin -bump is tracked in issue #179. The road to release 1.1.0 is tracked in -Gitea issues #175-#182 (milestone 1.1.0). +fails make check under golangci-lint >= 2.12 (25 lint issues remaining: +1 gosec G703, 24 goconst), so the tree is out of compliance until +fixed. CI (Dockerfile lint stage, pinned golangci-lint v2.10.1) is +green; the pin bump is tracked in issue #179. The road to release +1.1.0 is tracked in Gitea issues #175-#185 (milestone 1.1.0). # Next Step -Fix the 22 gosec G710 open-redirect findings in -internal/handlers/app.go (issue #176) by validating app IDs in a -shared redirect helper. +Fix the gosec G703 path traversal finding in the deploy log download +handler (issue #177): canonicalize and containment-check the log path +before http.ServeFile, with a traversal-rejection test. # Completed Steps +- 2026-08-07: Fixed all 22 gosec G710 open-redirect findings: app + redirects go through a redirectToApp helper that ULID-validates the + app ID (#176). - 2026-08-07: Fixed all 47 noctx lint findings: tests now use httptest.NewRequestWithContext with t.Context() (#175). - 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints, @@ -50,8 +53,7 @@ shared redirect helper. # Future Steps - Get main green (compliance, ordered): - - Fix 22 gosec G710 findings (Next Step, #176). - - Fix 1 gosec G703 finding (#177). + - Fix 1 gosec G703 finding (Next Step, #177). - Fix 24 goconst findings (#178). - Bump Dockerfile golangci-lint pin to v2.12.x (#179). - Run make check clean on main and keep it green; main must always diff --git a/internal/handlers/app.go b/internal/handlers/app.go index 6b4b00d..f37460d 100644 --- a/internal/handlers/app.go +++ b/internal/handlers/app.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "os" "path/filepath" "strconv" @@ -14,6 +15,7 @@ import ( "time" "github.com/go-chi/chi/v5" + "github.com/oklog/ulid/v2" "sneak.berlin/go/upaas/internal/models" "sneak.berlin/go/upaas/internal/service/app" @@ -27,6 +29,27 @@ const ( deploymentsHistoryLimit = 50 ) +// redirectToApp issues a See Other redirect to the detail page of the +// given app, plus an optional suffix (a sub-path like "/deployments" +// or a query string like "?success=updated"). App IDs are ULIDs: the +// ID is parsed and re-serialized so the redirect target never +// contains unvalidated request input; an invalid ID yields 404. +func redirectToApp( + writer http.ResponseWriter, + request *http.Request, + appID, suffix string, +) { + id, parseErr := ulid.ParseStrict(appID) + if parseErr != nil { + http.NotFound(writer, request) + + return + } + + target := "/apps/" + url.PathEscape(id.String()) + suffix + http.Redirect(writer, request, target, http.StatusSeeOther) +} + // HandleAppNew returns the new app form handler. func (h *Handlers) HandleAppNew() http.HandlerFunc { tmpl := templates.GetParsed() @@ -119,7 +142,7 @@ func (h *Handlers) HandleAppCreate() http.HandlerFunc { //nolint:funlen // valid return } - http.Redirect(writer, request, "/apps/"+createdApp.ID, http.StatusSeeOther) + redirectToApp(writer, request, createdApp.ID, "") } } @@ -285,8 +308,7 @@ func (h *Handlers) HandleAppUpdate() http.HandlerFunc { //nolint:funlen // valid return } - redirectURL := "/apps/" + application.ID + "?success=updated" - http.Redirect(writer, request, redirectURL, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "?success=updated") } } @@ -371,12 +393,7 @@ func (h *Handlers) HandleAppDeploy() http.HandlerFunc { } }(deployCtx, application) - http.Redirect( - writer, - request, - "/apps/"+application.ID+"/deployments", - http.StatusSeeOther, - ) + redirectToApp(writer, request, application.ID, "/deployments") } } @@ -397,12 +414,7 @@ func (h *Handlers) HandleCancelDeploy() http.HandlerFunc { h.log.Info("deployment cancelled by user", "app", application.Name) } - http.Redirect( - writer, - request, - "/apps/"+application.ID, - http.StatusSeeOther, - ) + redirectToApp(writer, request, application.ID, "") } } @@ -421,12 +433,12 @@ func (h *Handlers) HandleAppRollback() http.HandlerFunc { rollbackErr := h.deploy.Rollback(request.Context(), application) if rollbackErr != nil { h.log.Error("rollback failed", "error", rollbackErr, "app", application.Name) - http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "") return } - http.Redirect(writer, request, "/apps/"+application.ID+"?success=rolledback", http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "?success=rolledback") } } @@ -799,7 +811,7 @@ func (h *Handlers) handleContainerAction( containerInfo, containerErr := h.docker.FindContainerByAppID(ctx, appID) if containerErr != nil || containerInfo == nil { - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") return } @@ -832,7 +844,7 @@ func (h *Handlers) handleContainerAction( "action", action, "app", application.Name, "container", containerID) } - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") } // HandleAppRestart handles restarting an app's container. @@ -886,7 +898,7 @@ func (h *Handlers) addKeyValueToApp( value := request.FormValue("value") if key == "" || value == "" { - http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "") return } @@ -896,7 +908,7 @@ func (h *Handlers) addKeyValueToApp( h.log.Error("failed to add key-value pair", "error", saveErr) } - http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "") } // envPairJSON represents a key-value pair in the JSON request body. @@ -1031,7 +1043,7 @@ func (h *Handlers) HandleLabelDelete() http.HandlerFunc { h.log.Error("failed to delete label", "error", deleteErr) } - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") } } @@ -1059,12 +1071,7 @@ func (h *Handlers) HandleVolumeAdd() http.HandlerFunc { readOnly := request.FormValue("readonly") == "1" if hostPath == "" || containerPath == "" { - http.Redirect( - writer, - request, - "/apps/"+application.ID, - http.StatusSeeOther, - ) + redirectToApp(writer, request, application.ID, "") return } @@ -1072,7 +1079,7 @@ func (h *Handlers) HandleVolumeAdd() http.HandlerFunc { pathErr := validateVolumePaths(hostPath, containerPath) if pathErr != nil { h.log.Error("invalid volume path", "error", pathErr) - http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "") return } @@ -1088,7 +1095,7 @@ func (h *Handlers) HandleVolumeAdd() http.HandlerFunc { h.log.Error("failed to add volume", "error", saveErr) } - http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "") } } @@ -1117,7 +1124,7 @@ func (h *Handlers) HandleVolumeDelete() http.HandlerFunc { h.log.Error("failed to delete volume", "error", deleteErr) } - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") } } @@ -1145,7 +1152,7 @@ func (h *Handlers) HandlePortAdd() http.HandlerFunc { request.FormValue("container_port"), ) if !valid { - http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "") return } @@ -1166,7 +1173,7 @@ func (h *Handlers) HandlePortAdd() http.HandlerFunc { h.log.Error("failed to save port", "error", saveErr) } - http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "") } } @@ -1212,7 +1219,7 @@ func (h *Handlers) HandlePortDelete() http.HandlerFunc { h.log.Error("failed to delete port", "error", deleteErr) } - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") } } @@ -1274,7 +1281,7 @@ func (h *Handlers) HandleLabelEdit() http.HandlerFunc { value := request.FormValue("value") if key == "" || value == "" { - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") return } @@ -1287,7 +1294,7 @@ func (h *Handlers) HandleLabelEdit() http.HandlerFunc { h.log.Error("failed to update label", "error", saveErr) } - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") } } @@ -1323,7 +1330,7 @@ func (h *Handlers) HandleVolumeEdit() http.HandlerFunc { readOnly := request.FormValue("readonly") == "1" if hostPath == "" || containerPath == "" { - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") return } @@ -1331,7 +1338,7 @@ func (h *Handlers) HandleVolumeEdit() http.HandlerFunc { pathErr := validateVolumePaths(hostPath, containerPath) if pathErr != nil { h.log.Error("invalid volume path", "error", pathErr) - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") return } @@ -1345,7 +1352,7 @@ func (h *Handlers) HandleVolumeEdit() http.HandlerFunc { h.log.Error("failed to update volume", "error", saveErr) } - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + redirectToApp(writer, request, appID, "") } }