fix: resolve all 22 gosec G710 open-redirect findings
All checks were successful
Check / check (pull_request) Successful in 3m25s
All checks were successful
Check / check (pull_request) Successful in 3m25s
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/<id> 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
This commit is contained in:
22
TODO.md
22
TODO.md
@@ -11,20 +11,23 @@
|
|||||||
# Status
|
# Status
|
||||||
|
|
||||||
1.0+. Tagged 1.0.0 on 2026-02-26. Policy violation: main currently
|
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:
|
fails make check under golangci-lint >= 2.12 (25 lint issues remaining:
|
||||||
23 gosec, 24 goconst), so the tree is out of compliance until fixed. CI
|
1 gosec G703, 24 goconst), so the tree is out of compliance until
|
||||||
(Dockerfile lint stage, pinned golangci-lint v2.10.1) is green; the pin
|
fixed. CI (Dockerfile lint stage, pinned golangci-lint v2.10.1) is
|
||||||
bump is tracked in issue #179. The road to release 1.1.0 is tracked in
|
green; the pin bump is tracked in issue #179. The road to release
|
||||||
Gitea issues #175-#182 (milestone 1.1.0).
|
1.1.0 is tracked in Gitea issues #175-#185 (milestone 1.1.0).
|
||||||
|
|
||||||
# Next Step
|
# Next Step
|
||||||
|
|
||||||
Fix the 22 gosec G710 open-redirect findings in
|
Fix the gosec G703 path traversal finding in the deploy log download
|
||||||
internal/handlers/app.go (issue #176) by validating app IDs in a
|
handler (issue #177): canonicalize and containment-check the log path
|
||||||
shared redirect helper.
|
before http.ServeFile, with a traversal-rejection test.
|
||||||
|
|
||||||
# Completed Steps
|
# 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
|
- 2026-08-07: Fixed all 47 noctx lint findings: tests now use
|
||||||
httptest.NewRequestWithContext with t.Context() (#175).
|
httptest.NewRequestWithContext with t.Context() (#175).
|
||||||
- 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints,
|
- 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints,
|
||||||
@@ -50,8 +53,7 @@ shared redirect helper.
|
|||||||
# Future Steps
|
# Future Steps
|
||||||
|
|
||||||
- Get main green (compliance, ordered):
|
- Get main green (compliance, ordered):
|
||||||
- Fix 22 gosec G710 findings (Next Step, #176).
|
- Fix 1 gosec G703 finding (Next Step, #177).
|
||||||
- Fix 1 gosec G703 finding (#177).
|
|
||||||
- Fix 24 goconst findings (#178).
|
- Fix 24 goconst findings (#178).
|
||||||
- Bump Dockerfile golangci-lint pin to v2.12.x (#179).
|
- Bump Dockerfile golangci-lint pin to v2.12.x (#179).
|
||||||
- Run make check clean on main and keep it green; main must always
|
- Run make check clean on main and keep it green; main must always
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ import (
|
|||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"net/url"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
"strconv"
|
"strconv"
|
||||||
@@ -14,6 +15,7 @@ import (
|
|||||||
"time"
|
"time"
|
||||||
|
|
||||||
"github.com/go-chi/chi/v5"
|
"github.com/go-chi/chi/v5"
|
||||||
|
"github.com/oklog/ulid/v2"
|
||||||
|
|
||||||
"sneak.berlin/go/upaas/internal/models"
|
"sneak.berlin/go/upaas/internal/models"
|
||||||
"sneak.berlin/go/upaas/internal/service/app"
|
"sneak.berlin/go/upaas/internal/service/app"
|
||||||
@@ -27,6 +29,27 @@ const (
|
|||||||
deploymentsHistoryLimit = 50
|
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.
|
// HandleAppNew returns the new app form handler.
|
||||||
func (h *Handlers) HandleAppNew() http.HandlerFunc {
|
func (h *Handlers) HandleAppNew() http.HandlerFunc {
|
||||||
tmpl := templates.GetParsed()
|
tmpl := templates.GetParsed()
|
||||||
@@ -119,7 +142,7 @@ func (h *Handlers) HandleAppCreate() http.HandlerFunc { //nolint:funlen // valid
|
|||||||
return
|
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
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
redirectURL := "/apps/" + application.ID + "?success=updated"
|
redirectToApp(writer, request, application.ID, "?success=updated")
|
||||||
http.Redirect(writer, request, redirectURL, http.StatusSeeOther)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -371,12 +393,7 @@ func (h *Handlers) HandleAppDeploy() http.HandlerFunc {
|
|||||||
}
|
}
|
||||||
}(deployCtx, application)
|
}(deployCtx, application)
|
||||||
|
|
||||||
http.Redirect(
|
redirectToApp(writer, request, application.ID, "/deployments")
|
||||||
writer,
|
|
||||||
request,
|
|
||||||
"/apps/"+application.ID+"/deployments",
|
|
||||||
http.StatusSeeOther,
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -397,12 +414,7 @@ func (h *Handlers) HandleCancelDeploy() http.HandlerFunc {
|
|||||||
h.log.Info("deployment cancelled by user", "app", application.Name)
|
h.log.Info("deployment cancelled by user", "app", application.Name)
|
||||||
}
|
}
|
||||||
|
|
||||||
http.Redirect(
|
redirectToApp(writer, request, application.ID, "")
|
||||||
writer,
|
|
||||||
request,
|
|
||||||
"/apps/"+application.ID,
|
|
||||||
http.StatusSeeOther,
|
|
||||||
)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -421,12 +433,12 @@ func (h *Handlers) HandleAppRollback() http.HandlerFunc {
|
|||||||
rollbackErr := h.deploy.Rollback(request.Context(), application)
|
rollbackErr := h.deploy.Rollback(request.Context(), application)
|
||||||
if rollbackErr != nil {
|
if rollbackErr != nil {
|
||||||
h.log.Error("rollback failed", "error", rollbackErr, "app", application.Name)
|
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
|
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)
|
containerInfo, containerErr := h.docker.FindContainerByAppID(ctx, appID)
|
||||||
if containerErr != nil || containerInfo == nil {
|
if containerErr != nil || containerInfo == nil {
|
||||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
redirectToApp(writer, request, appID, "")
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -832,7 +844,7 @@ func (h *Handlers) handleContainerAction(
|
|||||||
"action", action, "app", application.Name, "container", containerID)
|
"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.
|
// HandleAppRestart handles restarting an app's container.
|
||||||
@@ -886,7 +898,7 @@ func (h *Handlers) addKeyValueToApp(
|
|||||||
value := request.FormValue("value")
|
value := request.FormValue("value")
|
||||||
|
|
||||||
if key == "" || value == "" {
|
if key == "" || value == "" {
|
||||||
http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther)
|
redirectToApp(writer, request, application.ID, "")
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -896,7 +908,7 @@ func (h *Handlers) addKeyValueToApp(
|
|||||||
h.log.Error("failed to add key-value pair", "error", saveErr)
|
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.
|
// 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)
|
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"
|
readOnly := request.FormValue("readonly") == "1"
|
||||||
|
|
||||||
if hostPath == "" || containerPath == "" {
|
if hostPath == "" || containerPath == "" {
|
||||||
http.Redirect(
|
redirectToApp(writer, request, application.ID, "")
|
||||||
writer,
|
|
||||||
request,
|
|
||||||
"/apps/"+application.ID,
|
|
||||||
http.StatusSeeOther,
|
|
||||||
)
|
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -1072,7 +1079,7 @@ func (h *Handlers) HandleVolumeAdd() http.HandlerFunc {
|
|||||||
pathErr := validateVolumePaths(hostPath, containerPath)
|
pathErr := validateVolumePaths(hostPath, containerPath)
|
||||||
if pathErr != nil {
|
if pathErr != nil {
|
||||||
h.log.Error("invalid volume path", "error", pathErr)
|
h.log.Error("invalid volume path", "error", pathErr)
|
||||||
http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther)
|
redirectToApp(writer, request, application.ID, "")
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -1088,7 +1095,7 @@ func (h *Handlers) HandleVolumeAdd() http.HandlerFunc {
|
|||||||
h.log.Error("failed to add volume", "error", saveErr)
|
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)
|
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"),
|
request.FormValue("container_port"),
|
||||||
)
|
)
|
||||||
if !valid {
|
if !valid {
|
||||||
http.Redirect(writer, request, "/apps/"+application.ID, http.StatusSeeOther)
|
redirectToApp(writer, request, application.ID, "")
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -1166,7 +1173,7 @@ func (h *Handlers) HandlePortAdd() http.HandlerFunc {
|
|||||||
h.log.Error("failed to save port", "error", saveErr)
|
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)
|
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")
|
value := request.FormValue("value")
|
||||||
|
|
||||||
if key == "" || value == "" {
|
if key == "" || value == "" {
|
||||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
redirectToApp(writer, request, appID, "")
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -1287,7 +1294,7 @@ func (h *Handlers) HandleLabelEdit() http.HandlerFunc {
|
|||||||
h.log.Error("failed to update label", "error", saveErr)
|
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"
|
readOnly := request.FormValue("readonly") == "1"
|
||||||
|
|
||||||
if hostPath == "" || containerPath == "" {
|
if hostPath == "" || containerPath == "" {
|
||||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
redirectToApp(writer, request, appID, "")
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -1331,7 +1338,7 @@ func (h *Handlers) HandleVolumeEdit() http.HandlerFunc {
|
|||||||
pathErr := validateVolumePaths(hostPath, containerPath)
|
pathErr := validateVolumePaths(hostPath, containerPath)
|
||||||
if pathErr != nil {
|
if pathErr != nil {
|
||||||
h.log.Error("invalid volume path", "error", pathErr)
|
h.log.Error("invalid volume path", "error", pathErr)
|
||||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
redirectToApp(writer, request, appID, "")
|
||||||
|
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
@@ -1345,7 +1352,7 @@ func (h *Handlers) HandleVolumeEdit() http.HandlerFunc {
|
|||||||
h.log.Error("failed to update volume", "error", saveErr)
|
h.log.Error("failed to update volume", "error", saveErr)
|
||||||
}
|
}
|
||||||
|
|
||||||
http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther)
|
redirectToApp(writer, request, appID, "")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user