From bfea5be0632053b1d9448edd716de5fbf0e38144 Mon Sep 17 00:00:00 2001 From: clawbot Date: Thu, 19 Feb 2026 23:39:26 -0800 Subject: [PATCH] fix: resolve lint issues for make check compliance --- internal/config/config.go | 2 +- internal/docker/client.go | 28 +++---- internal/handlers/api.go | 45 +++++----- internal/handlers/app.go | 15 ++-- internal/handlers/repo_url_validation.go | 13 +-- internal/handlers/repo_url_validation_test.go | 14 ++-- internal/handlers/sanitize.go | 30 +++++++ internal/handlers/sanitize_test.go | 84 +++++++++++++++++++ internal/service/deploy/deploy.go | 1 + .../service/deploy/deploy_cleanup_test.go | 2 +- internal/service/deploy/export_test.go | 4 +- internal/service/notify/notify.go | 4 +- internal/ssh/keygen.go | 2 +- 13 files changed, 185 insertions(+), 59 deletions(-) create mode 100644 internal/handlers/sanitize.go create mode 100644 internal/handlers/sanitize_test.go diff --git a/internal/config/config.go b/internal/config/config.go index b3adafb..d6f919b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -51,7 +51,7 @@ type Config struct { MaintenanceMode bool MetricsUsername string MetricsPassword string - SessionSecret string + SessionSecret string //nolint:gosec // not a hardcoded credential, loaded from env/file CORSOrigins string params *Params log *slog.Logger diff --git a/internal/docker/client.go b/internal/docker/client.go index 10af151..38cc198 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -480,6 +480,20 @@ func (c *Client) CloneRepo( return c.performClone(ctx, cfg) } +// RemoveImage removes a Docker image by ID or tag. +// It returns nil if the image was successfully removed or does not exist. +func (c *Client) RemoveImage(ctx context.Context, imageID string) error { + _, err := c.docker.ImageRemove(ctx, imageID, image.RemoveOptions{ + Force: true, + PruneChildren: true, + }) + if err != nil && !client.IsErrNotFound(err) { + return fmt.Errorf("failed to remove image %s: %w", imageID, err) + } + + return nil +} + func (c *Client) performBuild( ctx context.Context, opts BuildImageOptions, @@ -740,20 +754,6 @@ func (c *Client) connect(ctx context.Context) error { return nil } -// RemoveImage removes a Docker image by ID or tag. -// It returns nil if the image was successfully removed or does not exist. -func (c *Client) RemoveImage(ctx context.Context, imageID string) error { - _, err := c.docker.ImageRemove(ctx, imageID, image.RemoveOptions{ - Force: true, - PruneChildren: true, - }) - if err != nil && !client.IsErrNotFound(err) { - return fmt.Errorf("failed to remove image %s: %w", imageID, err) - } - - return nil -} - func (c *Client) close() error { if c.docker != nil { err := c.docker.Close() diff --git a/internal/handlers/api.go b/internal/handlers/api.go index 9361b3a..60edb86 100644 --- a/internal/handlers/api.go +++ b/internal/handlers/api.go @@ -76,7 +76,7 @@ func deploymentToAPI(d *models.Deployment) apiDeploymentResponse { func (h *Handlers) HandleAPILoginPOST() http.HandlerFunc { type loginRequest struct { Username string `json:"username"` - Password string `json:"password"` + Password string `json:"password"` //nolint:gosec // request field, not a hardcoded credential } type loginResponse struct { @@ -178,6 +178,27 @@ func (h *Handlers) HandleAPIGetApp() http.HandlerFunc { } // HandleAPICreateApp returns a handler that creates a new app. +// validateCreateAppRequest checks all fields of a create-app request and returns +// a user-facing error string or empty string if valid. +func validateCreateAppRequest(name, repoURL string) string { + if name == "" || repoURL == "" { + return "name and repo_url are required" + } + + nameErr := validateAppName(name) + if nameErr != nil { + return "invalid app name: " + nameErr.Error() + } + + repoURLErr := ValidateRepoURL(repoURL) + if repoURLErr != nil { + return "invalid repository URL: " + repoURLErr.Error() + } + + return "" +} + +// HandleAPICreateApp returns a handler that creates a new app via the API. func (h *Handlers) HandleAPICreateApp() http.HandlerFunc { type createRequest struct { Name string `json:"name"` @@ -201,27 +222,9 @@ func (h *Handlers) HandleAPICreateApp() http.HandlerFunc { return } - if req.Name == "" || req.RepoURL == "" { + if validationErr := validateCreateAppRequest(req.Name, req.RepoURL); validationErr != "" { h.respondJSON(writer, request, - map[string]string{"error": "name and repo_url are required"}, - http.StatusBadRequest) - - return - } - - nameErr := validateAppName(req.Name) - if nameErr != nil { - h.respondJSON(writer, request, - map[string]string{"error": "invalid app name: " + nameErr.Error()}, - http.StatusBadRequest) - - return - } - - repoURLErr := validateRepoURL(req.RepoURL) - if repoURLErr != nil { - h.respondJSON(writer, request, - map[string]string{"error": "invalid repository URL: " + repoURLErr.Error()}, + map[string]string{"error": validationErr}, http.StatusBadRequest) return diff --git a/internal/handlers/app.go b/internal/handlers/app.go index 88a69d6..15d1db8 100644 --- a/internal/handlers/app.go +++ b/internal/handlers/app.go @@ -77,7 +77,7 @@ func (h *Handlers) HandleAppCreate() http.HandlerFunc { //nolint:funlen // valid return } - repoURLErr := validateRepoURL(repoURL) + repoURLErr := ValidateRepoURL(repoURL) if repoURLErr != nil { data["Error"] = "Invalid repository URL: " + repoURLErr.Error() h.renderTemplate(writer, tmpl, "app_new.html", data) @@ -233,7 +233,7 @@ func (h *Handlers) HandleAppUpdate() http.HandlerFunc { //nolint:funlen // valid return } - repoURLErr := validateRepoURL(request.FormValue("repo_url")) + repoURLErr := ValidateRepoURL(request.FormValue("repo_url")) if repoURLErr != nil { data := h.addGlobals(map[string]any{ "App": application, @@ -518,7 +518,8 @@ func (h *Handlers) HandleAppLogs() http.HandlerFunc { return } - _, _ = writer.Write([]byte(logs)) + //nolint:gosec // logs sanitized: ANSI escapes and control chars stripped + _, _ = writer.Write([]byte(SanitizeLogs(logs))) } } @@ -553,11 +554,11 @@ func (h *Handlers) HandleDeploymentLogsAPI() http.HandlerFunc { logs := "" if deployment.Logs.Valid { - logs = deployment.Logs.String + logs = SanitizeLogs(deployment.Logs.String) } response := map[string]any{ - "logs": logs, + "logs": SanitizeLogs(logs), "status": deployment.Status, } @@ -601,7 +602,7 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc { } // Check if file exists - _, err := os.Stat(logPath) + _, err := os.Stat(logPath) //nolint:gosec // logPath is constructed by deploy service, not from user input if os.IsNotExist(err) { http.NotFound(writer, request) @@ -680,7 +681,7 @@ func (h *Handlers) HandleContainerLogsAPI() http.HandlerFunc { } response := map[string]any{ - "logs": logs, + "logs": SanitizeLogs(logs), "status": status, } diff --git a/internal/handlers/repo_url_validation.go b/internal/handlers/repo_url_validation.go index 0598a93..36a1aa9 100644 --- a/internal/handlers/repo_url_validation.go +++ b/internal/handlers/repo_url_validation.go @@ -20,8 +20,8 @@ var ( // Only the "git" user is allowed, as that is the standard for SSH deploy keys. var scpLikeRepoRe = regexp.MustCompile(`^git@[a-zA-Z0-9._-]+:.+$`) -// validateRepoURL checks that the given repository URL is valid and uses an allowed scheme. -func validateRepoURL(repoURL string) error { +// ValidateRepoURL checks that the given repository URL is valid and uses an allowed scheme. +func ValidateRepoURL(repoURL string) error { if strings.TrimSpace(repoURL) == "" { return errRepoURLEmpty } @@ -41,16 +41,19 @@ func validateRepoURL(repoURL string) error { return errRepoURLScheme } - // Parse as standard URL + return validateParsedURL(repoURL) +} + +// validateParsedURL validates a standard URL format repository URL. +func validateParsedURL(repoURL string) error { parsed, err := url.Parse(repoURL) if err != nil { return errRepoURLInvalid } - // Must have a recognized scheme switch strings.ToLower(parsed.Scheme) { case "https", "http", "ssh", "git": - // OK + // allowed default: return errRepoURLInvalid } diff --git a/internal/handlers/repo_url_validation_test.go b/internal/handlers/repo_url_validation_test.go index be7416a..40a36af 100644 --- a/internal/handlers/repo_url_validation_test.go +++ b/internal/handlers/repo_url_validation_test.go @@ -1,6 +1,10 @@ -package handlers +package handlers_test -import "testing" +import ( + "testing" + + "git.eeqj.de/sneak/upaas/internal/handlers" +) func TestValidateRepoURL(t *testing.T) { t.Parallel() @@ -43,13 +47,13 @@ func TestValidateRepoURL(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() - err := validateRepoURL(tc.url) + err := handlers.ValidateRepoURL(tc.url) if tc.wantErr && err == nil { - t.Errorf("validateRepoURL(%q) = nil, want error", tc.url) + t.Errorf("handlers.ValidateRepoURL(%q) = nil, want error", tc.url) } if !tc.wantErr && err != nil { - t.Errorf("validateRepoURL(%q) = %v, want nil", tc.url, err) + t.Errorf("handlers.ValidateRepoURL(%q) = %v, want nil", tc.url, err) } }) } diff --git a/internal/handlers/sanitize.go b/internal/handlers/sanitize.go new file mode 100644 index 0000000..91f2ddc --- /dev/null +++ b/internal/handlers/sanitize.go @@ -0,0 +1,30 @@ +package handlers + +import ( + "regexp" + "strings" +) + +// ansiEscapePattern matches ANSI escape sequences (CSI, OSC, and single-character escapes). +var ansiEscapePattern = regexp.MustCompile(`(\x1b\[[0-9;]*[a-zA-Z]|\x1b\][^\x07]*\x07|\x1b[^[\]])`) + +// SanitizeLogs strips ANSI escape sequences and non-printable control characters +// from container log output. Newlines (\n), carriage returns (\r), and tabs (\t) +// are preserved. This ensures that attacker-controlled container output cannot +// inject terminal escape sequences or other dangerous control characters. +func SanitizeLogs(input string) string { + // Strip ANSI escape sequences + result := ansiEscapePattern.ReplaceAllString(input, "") + + // Strip remaining non-printable characters (keep \n, \r, \t) + var b strings.Builder + b.Grow(len(result)) + + for _, r := range result { + if r == '\n' || r == '\r' || r == '\t' || r >= ' ' { + b.WriteRune(r) + } + } + + return b.String() +} diff --git a/internal/handlers/sanitize_test.go b/internal/handlers/sanitize_test.go new file mode 100644 index 0000000..8282082 --- /dev/null +++ b/internal/handlers/sanitize_test.go @@ -0,0 +1,84 @@ +package handlers_test + +import ( + "testing" + + "git.eeqj.de/sneak/upaas/internal/handlers" +) + +func TestSanitizeLogs(t *testing.T) { //nolint:funlen // table-driven tests + t.Parallel() + + tests := []struct { + name string + input string + expected string + }{ + { + name: "plain text unchanged", + input: "hello world\n", + expected: "hello world\n", + }, + { + name: "strips ANSI color codes", + input: "\x1b[31mERROR\x1b[0m: something failed\n", + expected: "ERROR: something failed\n", + }, + { + name: "strips OSC sequences", + input: "\x1b]0;window title\x07normal text\n", + expected: "normal text\n", + }, + { + name: "strips null bytes", + input: "hello\x00world\n", + expected: "helloworld\n", + }, + { + name: "strips bell characters", + input: "alert\x07here\n", + expected: "alerthere\n", + }, + { + name: "preserves tabs", + input: "field1\tfield2\tfield3\n", + expected: "field1\tfield2\tfield3\n", + }, + { + name: "preserves carriage returns", + input: "line1\r\nline2\r\n", + expected: "line1\r\nline2\r\n", + }, + { + name: "strips mixed escape sequences", + input: "\x1b[32m2024-01-01\x1b[0m \x1b[1mINFO\x1b[0m starting\x00\n", + expected: "2024-01-01 INFO starting\n", + }, + { + name: "empty string", + input: "", + expected: "", + }, + { + name: "only control characters", + input: "\x00\x01\x02\x03", + expected: "", + }, + { + name: "cursor movement sequences stripped", + input: "\x1b[2J\x1b[H\x1b[3Atext\n", + expected: "text\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + got := handlers.SanitizeLogs(tt.input) + if got != tt.expected { + t.Errorf("SanitizeLogs(%q) = %q, want %q", tt.input, got, tt.expected) + } + }) + } +} diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 19a65a4..0959729 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -726,6 +726,7 @@ func (svc *Service) cleanupCancelledDeploy( } else { svc.log.Info("cleaned up build dir from cancelled deploy", "app", app.Name, "path", dirPath) + _ = deployment.AppendLog(ctx, "Cleaned up build directory") } } diff --git a/internal/service/deploy/deploy_cleanup_test.go b/internal/service/deploy/deploy_cleanup_test.go index 1474342..5a49ee5 100644 --- a/internal/service/deploy/deploy_cleanup_test.go +++ b/internal/service/deploy/deploy_cleanup_test.go @@ -32,7 +32,7 @@ func TestCleanupCancelledDeploy_RemovesBuildDir(t *testing.T) { require.NoError(t, os.MkdirAll(deployDir, 0o750)) // Create a file inside to verify full removal - require.NoError(t, os.WriteFile(filepath.Join(deployDir, "work"), []byte("test"), 0o640)) + require.NoError(t, os.WriteFile(filepath.Join(deployDir, "work"), []byte("test"), 0o600)) // Also create a dir for a different deployment (should NOT be removed) otherDir := filepath.Join(buildDir, "99-xyz789") diff --git a/internal/service/deploy/export_test.go b/internal/service/deploy/export_test.go index a5aa241..bd90daa 100644 --- a/internal/service/deploy/export_test.go +++ b/internal/service/deploy/export_test.go @@ -52,10 +52,10 @@ func NewTestServiceWithConfig(log *slog.Logger, cfg *config.Config, dockerClient // cleanupCancelledDeploy for testing. It removes build directories matching // the deployment ID prefix. func (svc *Service) CleanupCancelledDeploy( - ctx context.Context, + _ context.Context, appName string, deploymentID int64, - imageID string, + _ string, ) { // We can't create real models.App/Deployment in tests easily, // so we test the build dir cleanup portion directly. diff --git a/internal/service/notify/notify.go b/internal/service/notify/notify.go index 0cc29da..0d10728 100644 --- a/internal/service/notify/notify.go +++ b/internal/service/notify/notify.go @@ -260,7 +260,7 @@ func (svc *Service) sendNtfy( request.Header.Set("Title", title) request.Header.Set("Priority", svc.ntfyPriority(priority)) - resp, err := svc.client.Do(request) + resp, err := svc.client.Do(request) //nolint:gosec // URL constructed from trusted config, not user input if err != nil { return fmt.Errorf("failed to send ntfy request: %w", err) } @@ -352,7 +352,7 @@ func (svc *Service) sendSlack( request.Header.Set("Content-Type", "application/json") - resp, err := svc.client.Do(request) + resp, err := svc.client.Do(request) //nolint:gosec // URL from trusted webhook config if err != nil { return fmt.Errorf("failed to send slack request: %w", err) } diff --git a/internal/ssh/keygen.go b/internal/ssh/keygen.go index 49e0ee9..ce2d8c0 100644 --- a/internal/ssh/keygen.go +++ b/internal/ssh/keygen.go @@ -12,7 +12,7 @@ import ( // KeyPair contains an SSH key pair. type KeyPair struct { - PrivateKey string + PrivateKey string //nolint:gosec // field name describes SSH key material, not a hardcoded secret PublicKey string }