diff --git a/.golangci.yml b/.golangci.yml index 34a8e31..26b1610 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,5 +1,9 @@ version: "2" +# Config schema uses the golangci-lint v2 layout (settings live under +# linters.settings, not top-level linters-settings) so that the +# thresholds below are actually applied by golangci-lint >= v2. + run: timeout: 5m modules-download-mode: readonly @@ -14,19 +18,17 @@ linters: - wsl # Deprecated, replaced by wsl_v5 - wrapcheck # Too verbose for internal packages - varnamelen # Short names like db, id are idiomatic Go - -linters-settings: - lll: - line-length: 88 - funlen: - lines: 80 - statements: 50 - cyclop: - max-complexity: 15 - dupl: - threshold: 100 + settings: + lll: + line-length: 88 + funlen: + lines: 80 + statements: 50 + cyclop: + max-complexity: 15 + dupl: + threshold: 100 issues: - exclude-use-default: false max-issues-per-linter: 0 max-same-issues: 0 diff --git a/Dockerfile b/Dockerfile index e05b8c6..f31025e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,6 +1,6 @@ # Lint stage — fast feedback on formatting and lint issues -# golangci/golangci-lint:v2.10.1 -FROM golangci/golangci-lint@sha256:ea84d14c2fef724411be7dc45e09e6ef721d748315252b02df19a7e3113ee763 AS lint +# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-07 +FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS lint WORKDIR /src COPY go.mod go.sum ./ diff --git a/TODO.md b/TODO.md index 6b9d428..7e6d229 100644 --- a/TODO.md +++ b/TODO.md @@ -10,18 +10,20 @@ # Status -1.0+. Tagged 1.0.0 on 2026-02-26; 8 commits on main since. Policy -violation: main currently fails make check (91 lint issues), so the tree -is out of compliance until fixed. +1.0+. Tagged 1.0.0 on 2026-02-26; 8 commits on main since. `make check` +is green as of the golangci-lint v2.12.2 update. # Next Step -Fix the 47 noctx lint findings (HTTP requests without context) in one -commit and confirm the count drops under make check. This is the largest -of the three lint classes blocking a green main. +Confirm `.gitea/workflows/check.yml` gates merges on `make check` so +main cannot regress. # Completed Steps +- 2026-08-07: Updated golangci-lint to v2.12.2 (canonical + `.golangci.yml`, `Dockerfile` lint stage pin, `script/bootstrap` + release-archive pins) and fixed all resulting lint findings (noctx, + gosec, goconst, lll, dupl, nolintlint); `make check` green. - 2026-07-07 Adopted scripts-to-rule-them-all: `script/` entrypoints, Makefile shims, README Entrypoints section - 2026-03-11: Monolithic env var editing with bulk save (#158). @@ -44,12 +46,4 @@ of the three lint classes blocking a green main. # Future Steps -- Get main green (compliance, ordered): - - Fix 47 noctx findings (Next Step). - - Fix 23 gosec findings. - - Fix 21 goconst findings. - - Run make check clean on main and keep it green; main must always - pass. -- Confirm .gitea/workflows/check.yml gates merges on make check so main - cannot regress. - Resume feature work only after main is green. diff --git a/internal/config/config.go b/internal/config/config.go index bd82a0e..f82b13c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -45,7 +45,7 @@ type Config struct { Port int Debug bool DataDir string - HostDataDir string // Host path for DataDir (for Docker bind mounts when running in container) + HostDataDir string // Host path for DataDir (Docker bind mounts in container) DockerHost string SentryDSN string MaintenanceMode bool diff --git a/internal/database/database.go b/internal/database/database.go index f7dfb3a..929ef5e 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -178,7 +178,8 @@ func HashWebhookSecret(secret string) string { func (d *Database) backfillWebhookSecretHashes(ctx context.Context) error { rows, err := d.database.QueryContext(ctx, - "SELECT id, webhook_secret FROM apps WHERE webhook_secret_hash = '' AND webhook_secret != ''") + "SELECT id, webhook_secret FROM apps"+ + " WHERE webhook_secret_hash = '' AND webhook_secret != ''") if err != nil { return fmt.Errorf("querying apps for backfill: %w", err) } diff --git a/internal/database/migrations.go b/internal/database/migrations.go index 0982824..df66c08 100644 --- a/internal/database/migrations.go +++ b/internal/database/migrations.go @@ -32,7 +32,10 @@ var ErrInvalidMigrationFilename = errors.New("invalid migration filename") func ParseMigrationVersion(filename string) (int, error) { name := strings.TrimSuffix(filename, ".sql") if name == "" || name == filename { - return 0, fmt.Errorf("%w: %q has no .sql extension or is empty", ErrInvalidMigrationFilename, filename) + return 0, fmt.Errorf( + "%w: %q has no .sql extension or is empty", + ErrInvalidMigrationFilename, filename, + ) } // Split on underscore to separate version from description. @@ -40,7 +43,10 @@ func ParseMigrationVersion(filename string) (int, error) { versionStr, _, _ := strings.Cut(name, "_") if versionStr == "" { - return 0, fmt.Errorf("%w: %q has empty version prefix", ErrInvalidMigrationFilename, filename) + return 0, fmt.Errorf( + "%w: %q has empty version prefix", + ErrInvalidMigrationFilename, filename, + ) } // Validate the version is purely numeric. @@ -177,7 +183,12 @@ func ApplyMigrations(ctx context.Context, db *sql.DB, log *slog.Logger) error { // applyMigrationTx reads and executes a migration file within a transaction, // recording the version in schema_migrations on success. -func applyMigrationTx(ctx context.Context, db *sql.DB, filename string, version int) error { +func applyMigrationTx( + ctx context.Context, + db *sql.DB, + filename string, + version int, +) error { content, err := migrationsFS.ReadFile("migrations/" + filename) if err != nil { return fmt.Errorf("failed to read migration %s: %w", filename, err) diff --git a/internal/docker/client.go b/internal/docker/client.go index ef78527..e1f4c19 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -41,7 +41,8 @@ const stopTimeoutSeconds = 10 // gitImage is the Docker image used for git operations. // alpine/git v2.47.2 - pulled 2025-12-30 -const gitImage = "alpine/git@sha256:d86f367afb53d022acc4377741e7334bc20add161bb10234272b91b459b4b7d8" +const gitImage = "alpine/git@sha256:" + + "d86f367afb53d022acc4377741e7334bc20add161bb10234272b91b459b4b7d8" // ErrNotConnected is returned when Docker client is not connected. var ErrNotConnected = errors.New("docker client not connected") @@ -145,7 +146,7 @@ type CreateContainerOptions struct { Volumes []VolumeMount Ports []PortMapping Network string - CPULimit float64 // CPU cores (e.g. 0.5 = half a core, 2.0 = two cores). 0 means unlimited. + CPULimit float64 // CPU cores (0.5 = half a core). 0 means unlimited. MemoryLimit int64 // Memory in bytes. 0 means unlimited. } @@ -303,7 +304,11 @@ func (c *Client) StopContainer(ctx context.Context, containerID ContainerID) err timeout := stopTimeoutSeconds - err := c.docker.ContainerStop(ctx, containerID.String(), container.StopOptions{Timeout: &timeout}) + err := c.docker.ContainerStop( + ctx, + containerID.String(), + container.StopOptions{Timeout: &timeout}, + ) if err != nil { return fmt.Errorf("failed to stop container: %w", err) } @@ -323,7 +328,11 @@ func (c *Client) RemoveContainer( c.log.Info("removing container", "id", containerID, "force", force) - err := c.docker.ContainerRemove(ctx, containerID.String(), container.RemoveOptions{Force: force}) + err := c.docker.ContainerRemove( + ctx, + containerID.String(), + container.RemoveOptions{Force: force}, + ) if err != nil { return fmt.Errorf("failed to remove container: %w", err) } @@ -469,7 +478,8 @@ type CloneResult struct { CommitSHA string // The HEAD commit SHA after clone/checkout } -// CloneRepo clones a git repository using SSH and optionally checks out a specific commit. +// CloneRepo clones a git repository using SSH and optionally checks out a +// specific commit. // containerDir is the path inside the upaas container (for writing files). // hostDir is the corresponding path on the Docker host (for bind mounts). // If commitSHA is provided, that specific commit will be checked out. @@ -584,11 +594,13 @@ func (c *Client) performBuild( // scannerInitialBufferSize is the initial buffer size for the build log scanner. const scannerInitialBufferSize = 64 * 1024 // 64KB -// scannerMaxBufferSize is the max buffer size for build log lines (base64 layers can be large). +// scannerMaxBufferSize is the max buffer size for build log lines +// (base64 layers can be large). const scannerMaxBufferSize = 1024 * 1024 // 1MB -// streamBuildOutput reads Docker build output line by line and writes to stdout and optional log writer. -// Docker sends newline-delimited JSON, so reading line by line ensures each log entry is written immediately. +// streamBuildOutput reads Docker build output line by line and writes to +// stdout and optional log writer. Docker sends newline-delimited JSON, so +// reading line by line ensures each log entry is written immediately. func (c *Client) streamBuildOutput(body io.Reader, logWriter io.Writer) error { scanner := bufio.NewScanner(body) buf := make([]byte, 0, scannerInitialBufferSize) @@ -616,7 +628,10 @@ func (c *Client) streamBuildOutput(body io.Reader, logWriter io.Writer) error { return nil } -func (c *Client) performClone(ctx context.Context, cfg *cloneConfig) (*CloneResult, error) { +func (c *Client) performClone( + ctx context.Context, + cfg *cloneConfig, +) (*CloneResult, error) { // Create work directory for clone destination err := os.MkdirAll(cfg.containerDir, workDirPermissions) if err != nil { @@ -642,7 +657,11 @@ func (c *Client) performClone(ctx context.Context, cfg *cloneConfig) (*CloneResu } defer func() { - _ = c.docker.ContainerRemove(ctx, gitContainerID.String(), container.RemoveOptions{Force: true}) + _ = c.docker.ContainerRemove( + ctx, + gitContainerID.String(), + container.RemoveOptions{Force: true}, + ) }() return c.runGitClone(ctx, gitContainerID) @@ -680,7 +699,8 @@ func (c *Client) createGitContainer( entrypoint := []string{} cmd := []string{"sh", "-c", script} - // Use host paths for Docker bind mounts (Docker runs on the host, not in our container) + // Use host paths for Docker bind mounts + // (Docker runs on the host, not in our container) resp, err := c.docker.ContainerCreate(ctx, &container.Config{ Image: gitImage, @@ -711,13 +731,20 @@ func (c *Client) createGitContainer( return ContainerID(resp.ID), nil } -func (c *Client) runGitClone(ctx context.Context, containerID ContainerID) (*CloneResult, error) { +func (c *Client) runGitClone( + ctx context.Context, + containerID ContainerID, +) (*CloneResult, error) { err := c.docker.ContainerStart(ctx, containerID.String(), container.StartOptions{}) if err != nil { return nil, fmt.Errorf("failed to start git container: %w", err) } - statusCh, errCh := c.docker.ContainerWait(ctx, containerID.String(), container.WaitConditionNotRunning) + statusCh, errCh := c.docker.ContainerWait( + ctx, + containerID.String(), + container.WaitConditionNotRunning, + ) select { case err := <-errCh: diff --git a/internal/docker/validation_test.go b/internal/docker/validation_test.go index 785f3ed..2d033c8 100644 --- a/internal/docker/validation_test.go +++ b/internal/docker/validation_test.go @@ -6,11 +6,14 @@ import ( "testing" ) +// mainBranch is the branch name used across validation tests. +const mainBranch = "main" + func TestValidBranchRegex(t *testing.T) { t.Parallel() valid := []string{ - "main", + mainBranch, "develop", "feature/my-feature", "release-1.0", @@ -70,7 +73,7 @@ func TestValidCommitSHARegex(t *testing.T) { } } -func TestCloneRepoRejectsInjection(t *testing.T) { //nolint:funlen // table-driven test +func TestCloneRepoRejectsInjection(t *testing.T) { t.Parallel() c := &Client{ @@ -100,25 +103,25 @@ func TestCloneRepoRejectsInjection(t *testing.T) { //nolint:funlen // table-driv }, { name: "injection in commitSHA", - branch: "main", + branch: mainBranch, commitSHA: "not-a-sha; rm -rf /", wantErr: ErrInvalidCommitSHA, }, { name: "short SHA rejected", - branch: "main", + branch: mainBranch, commitSHA: "abc123", wantErr: ErrInvalidCommitSHA, }, { name: "valid inputs pass validation (hit NotConnected)", - branch: "main", + branch: mainBranch, commitSHA: "abc123def456789012345678901234567890abcd", wantErr: ErrNotConnected, }, { name: "valid branch no SHA passes validation (hit NotConnected)", - branch: "main", + branch: mainBranch, wantErr: ErrNotConnected, }, } diff --git a/internal/handlers/api.go b/internal/handlers/api.go index 3e3a537..dde4f6a 100644 --- a/internal/handlers/api.go +++ b/internal/handlers/api.go @@ -84,7 +84,7 @@ func (h *Handlers) HandleAPILoginPOST() http.HandlerFunc { decodeErr := json.NewDecoder(request.Body).Decode(&req) if decodeErr != nil { h.respondJSON(writer, request, - map[string]string{"error": "invalid JSON body"}, + map[string]string{jsonKeyError: "invalid JSON body"}, http.StatusBadRequest) return @@ -95,7 +95,7 @@ func (h *Handlers) HandleAPILoginPOST() http.HandlerFunc { if username == "" || credential == "" { h.respondJSON(writer, request, - map[string]string{"error": "username and password are required"}, + map[string]string{jsonKeyError: "username and password are required"}, http.StatusBadRequest) return @@ -104,7 +104,7 @@ func (h *Handlers) HandleAPILoginPOST() http.HandlerFunc { user, authErr := h.auth.Authenticate(request.Context(), username, credential) if authErr != nil { h.respondJSON(writer, request, - map[string]string{"error": "invalid credentials"}, + map[string]string{jsonKeyError: "invalid credentials"}, http.StatusUnauthorized) return @@ -114,7 +114,7 @@ func (h *Handlers) HandleAPILoginPOST() http.HandlerFunc { if sessionErr != nil { h.log.Error("api: failed to create session", "error", sessionErr) h.respondJSON(writer, request, - map[string]string{"error": "failed to create session"}, + map[string]string{jsonKeyError: "failed to create session"}, http.StatusInternalServerError) return @@ -133,7 +133,7 @@ func (h *Handlers) HandleAPIListApps() http.HandlerFunc { apps, err := h.appService.ListApps(request.Context()) if err != nil { h.respondJSON(writer, request, - map[string]string{"error": "failed to list apps"}, + map[string]string{jsonKeyError: "failed to list apps"}, http.StatusInternalServerError) return @@ -156,7 +156,7 @@ func (h *Handlers) HandleAPIGetApp() http.HandlerFunc { application, err := h.appService.GetApp(request.Context(), appID) if err != nil { h.respondJSON(writer, request, - map[string]string{"error": "internal server error"}, + map[string]string{jsonKeyError: "internal server error"}, http.StatusInternalServerError) return @@ -164,7 +164,7 @@ func (h *Handlers) HandleAPIGetApp() http.HandlerFunc { if application == nil { h.respondJSON(writer, request, - map[string]string{"error": "app not found"}, + map[string]string{jsonKeyError: "app not found"}, http.StatusNotFound) return @@ -185,7 +185,7 @@ func (h *Handlers) HandleAPIListDeployments() http.HandlerFunc { application, err := h.appService.GetApp(request.Context(), appID) if err != nil || application == nil { h.respondJSON(writer, request, - map[string]string{"error": "app not found"}, + map[string]string{jsonKeyError: "app not found"}, http.StatusNotFound) return @@ -205,7 +205,7 @@ func (h *Handlers) HandleAPIListDeployments() http.HandlerFunc { ) if deployErr != nil { h.respondJSON(writer, request, - map[string]string{"error": "failed to list deployments"}, + map[string]string{jsonKeyError: "failed to list deployments"}, http.StatusInternalServerError) return @@ -231,7 +231,7 @@ func (h *Handlers) HandleAPIWhoAmI() http.HandlerFunc { user, err := h.auth.GetCurrentUser(request.Context(), request) if err != nil || user == nil { h.respondJSON(writer, request, - map[string]string{"error": "unauthorized"}, + map[string]string{jsonKeyError: "unauthorized"}, http.StatusUnauthorized) return diff --git a/internal/handlers/api_test.go b/internal/handlers/api_test.go index 8efd7be..3bcf6ee 100644 --- a/internal/handlers/api_test.go +++ b/internal/handlers/api_test.go @@ -47,7 +47,12 @@ func setupAPITest(t *testing.T) (*testContext, []*http.Cookie) { r := apiRouter(tc) loginBody := `{"username":"admin","password":"password123"}` - req := httptest.NewRequest(http.MethodPost, "/api/v1/login", strings.NewReader(loginBody)) + req := httptest.NewRequestWithContext( + t.Context(), + http.MethodPost, + "/api/v1/login", + strings.NewReader(loginBody), + ) req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() @@ -70,7 +75,7 @@ func apiGet( ) *httptest.ResponseRecorder { t.Helper() - req := httptest.NewRequest(http.MethodGet, path, nil) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, path, nil) for _, c := range cookies { req.AddCookie(c) @@ -95,7 +100,12 @@ func TestAPILoginSuccess(t *testing.T) { r := apiRouter(tc) body := `{"username":"admin","password":"password123"}` - req := httptest.NewRequest(http.MethodPost, "/api/v1/login", strings.NewReader(body)) + req := httptest.NewRequestWithContext( + t.Context(), + http.MethodPost, + "/api/v1/login", + strings.NewReader(body), + ) req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() @@ -122,7 +132,12 @@ func TestAPILoginInvalidCredentials(t *testing.T) { r := apiRouter(tc) body := `{"username":"admin","password":"wrong"}` - req := httptest.NewRequest(http.MethodPost, "/api/v1/login", strings.NewReader(body)) + req := httptest.NewRequestWithContext( + t.Context(), + http.MethodPost, + "/api/v1/login", + strings.NewReader(body), + ) req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() @@ -139,7 +154,12 @@ func TestAPILoginMissingFields(t *testing.T) { r := apiRouter(tc) body := `{"username":"","password":""}` - req := httptest.NewRequest(http.MethodPost, "/api/v1/login", strings.NewReader(body)) + req := httptest.NewRequestWithContext( + t.Context(), + http.MethodPost, + "/api/v1/login", + strings.NewReader(body), + ) req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() @@ -155,7 +175,9 @@ func TestAPIRejectsUnauthenticated(t *testing.T) { r := apiRouter(tc) - req := httptest.NewRequest(http.MethodGet, "/api/v1/apps", nil) + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/api/v1/apps", nil, + ) rr := httptest.NewRecorder() r.ServeHTTP(rr, req) diff --git a/internal/handlers/app.go b/internal/handlers/app.go index 6b4b00d..aa83aed 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" @@ -15,6 +16,7 @@ import ( "github.com/go-chi/chi/v5" + "sneak.berlin/go/upaas/internal/database" "sneak.berlin/go/upaas/internal/models" "sneak.berlin/go/upaas/internal/service/app" "sneak.berlin/go/upaas/templates" @@ -27,6 +29,23 @@ const ( deploymentsHistoryLimit = 50 ) +// redirectToApp issues a SeeOther redirect to the page for the given +// app ID, with an optional suffix such as "/deployments" or +// "?success=updated". The ID is path-escaped so the target is always +// a relative application URL. +func redirectToApp( + writer http.ResponseWriter, + request *http.Request, + appID, suffix string, +) { + http.Redirect( + writer, + request, + "/apps/"+url.PathEscape(appID)+suffix, + http.StatusSeeOther, + ) +} + // HandleAppNew returns the new app form handler. func (h *Handlers) HandleAppNew() http.HandlerFunc { tmpl := templates.GetParsed() @@ -39,7 +58,9 @@ func (h *Handlers) HandleAppNew() http.HandlerFunc { } // HandleAppCreate handles app creation. -func (h *Handlers) HandleAppCreate() http.HandlerFunc { //nolint:funlen // validation adds necessary length +// +//nolint:funlen // validation adds necessary length +func (h *Handlers) HandleAppCreate() http.HandlerFunc { tmpl := templates.GetParsed() return func(writer http.ResponseWriter, request *http.Request) { @@ -160,10 +181,14 @@ func (h *Handlers) HandleAppDetail() http.HandlerFunc { } webhookURL := "https://" + request.Host + "/webhook/" + application.WebhookSecret - deployKey := formatDeployKey(application.SSHPublicKey, application.CreatedAt, application.Name) + deployKey := formatDeployKey( + application.SSHPublicKey, + application.CreatedAt, + application.Name, + ) data := h.addGlobals(map[string]any{ - "App": application, + dataKeyApp: application, "EnvVars": envVars, "Labels": labels, "Volumes": volumes, @@ -201,7 +226,7 @@ func (h *Handlers) HandleAppEdit() http.HandlerFunc { } data := h.addGlobals(map[string]any{ - "App": application, + dataKeyApp: application, }, request) h.renderTemplate(writer, tmpl, "app_edit.html", data) @@ -209,7 +234,7 @@ func (h *Handlers) HandleAppEdit() http.HandlerFunc { } // HandleAppUpdate handles app updates. -func (h *Handlers) HandleAppUpdate() http.HandlerFunc { //nolint:funlen // validation adds necessary length +func (h *Handlers) HandleAppUpdate() http.HandlerFunc { tmpl := templates.GetParsed() return func(writer http.ResponseWriter, request *http.Request) { @@ -234,8 +259,8 @@ func (h *Handlers) HandleAppUpdate() http.HandlerFunc { //nolint:funlen // valid nameErr := validateAppName(newName) if nameErr != nil { data := h.addGlobals(map[string]any{ - "App": application, - "Error": "Invalid app name: " + nameErr.Error(), + dataKeyApp: application, + dataKeyError: "Invalid app name: " + nameErr.Error(), }, request) h.renderTemplate(writer, tmpl, "app_edit.html", data) @@ -245,8 +270,8 @@ func (h *Handlers) HandleAppUpdate() http.HandlerFunc { //nolint:funlen // valid repoURLErr := validateRepoURL(request.FormValue("repo_url")) if repoURLErr != nil { data := h.addGlobals(map[string]any{ - "App": application, - "Error": "Invalid repository URL: " + repoURLErr.Error(), + dataKeyApp: application, + dataKeyError: "Invalid repository URL: " + repoURLErr.Error(), }, request) h.renderTemplate(writer, tmpl, "app_edit.html", data) @@ -264,8 +289,8 @@ func (h *Handlers) HandleAppUpdate() http.HandlerFunc { //nolint:funlen // valid limitsErr := applyResourceLimits(application, request) if limitsErr != "" { data := h.addGlobals(map[string]any{ - "App": application, - "Error": limitsErr, + dataKeyApp: application, + dataKeyError: limitsErr, }, request) h.renderTemplate(writer, tmpl, "app_edit.html", data) @@ -277,16 +302,15 @@ func (h *Handlers) HandleAppUpdate() http.HandlerFunc { //nolint:funlen // valid h.log.Error("failed to update app", "error", saveErr) data := h.addGlobals(map[string]any{ - "App": application, - "Error": "Failed to update app", + dataKeyApp: application, + dataKeyError: "Failed to update app", }, request) h.renderTemplate(writer, tmpl, "app_edit.html", data) return } - redirectURL := "/apps/" + application.ID + "?success=updated" - http.Redirect(writer, request, redirectURL, http.StatusSeeOther) + redirectToApp(writer, request, application.ID, "?success=updated") } } @@ -371,12 +395,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 +416,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 +435,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") } } @@ -450,7 +464,7 @@ func (h *Handlers) HandleAppDeployments() http.HandlerFunc { ) data := h.addGlobals(map[string]any{ - "App": application, + dataKeyApp: application, "Deployments": deployments, }, request) @@ -523,7 +537,7 @@ func (h *Handlers) HandleAppLogs() http.HandlerFunc { return } - _, _ = writer.Write([]byte(SanitizeLogs(logs))) // #nosec G705 -- logs sanitized, Content-Type is text/plain + _, _ = writer.Write([]byte(SanitizeLogs(logs))) // #nosec G705 -- output sanitized } } @@ -562,8 +576,8 @@ func (h *Handlers) HandleDeploymentLogsAPI() http.HandlerFunc { } response := map[string]any{ - "logs": logs, - "status": deployment.Status, + jsonKeyLogs: logs, + jsonKeyStatus: deployment.Status, } _ = json.NewEncoder(writer).Encode(response) @@ -606,7 +620,7 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc { } // Check if file exists — logPath is constructed internally, not from user input - _, err := os.Stat(logPath) // #nosec G703 -- path from internal GetLogFilePath, not user input + _, err := os.Stat(logPath) // #nosec G703 -- internal path, not user input if os.IsNotExist(err) { http.NotFound(writer, request) @@ -626,7 +640,7 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc { writer.Header().Set("Content-Type", "text/plain; charset=utf-8") writer.Header().Set("Content-Disposition", "attachment; filename=\""+filename+"\"") - http.ServeFile(writer, request, logPath) + http.ServeFile(writer, request, logPath) // #nosec G703 -- internal path } } @@ -650,8 +664,8 @@ func (h *Handlers) HandleContainerLogsAPI() http.HandlerFunc { containerInfo, containerErr := h.docker.FindContainerByAppID(request.Context(), appID) if containerErr != nil || containerInfo == nil { response := map[string]any{ - "logs": "No container running\n", - "status": "stopped", + jsonKeyLogs: "No container running\n", + jsonKeyStatus: "stopped", } _ = json.NewEncoder(writer).Encode(response) @@ -671,8 +685,8 @@ func (h *Handlers) HandleContainerLogsAPI() http.HandlerFunc { ) response := map[string]any{ - "logs": "Failed to fetch container logs\n", - "status": "error", + jsonKeyLogs: "Failed to fetch container logs\n", + jsonKeyStatus: "error", } _ = json.NewEncoder(writer).Encode(response) @@ -685,8 +699,8 @@ func (h *Handlers) HandleContainerLogsAPI() http.HandlerFunc { } response := map[string]any{ - "logs": SanitizeLogs(logs), - "status": status, + jsonKeyLogs: SanitizeLogs(logs), + jsonKeyStatus: status, } _ = json.NewEncoder(writer).Encode(response) @@ -720,7 +734,7 @@ func (h *Handlers) HandleAppStatusAPI() http.HandlerFunc { } response := map[string]any{ - "status": string(application.Status), + jsonKeyStatus: string(application.Status), "latestDeploymentID": latestDeploymentID, "latestDeploymentStatus": latestDeploymentStatus, } @@ -757,7 +771,7 @@ func (h *Handlers) HandleRecentDeploymentsAPI() http.HandlerFunc { for _, d := range deployments { deploymentsData = append(deploymentsData, map[string]any{ "id": d.ID, - "status": string(d.Status), + jsonKeyStatus: string(d.Status), "duration": d.Duration(), "shortCommit": d.ShortCommit(), "finishedAtISO": d.FinishedAtISO(), @@ -799,7 +813,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 +846,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 +900,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 +910,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. @@ -957,7 +971,7 @@ func (h *Handlers) HandleEnvVarSave() http.HandlerFunc { decodeErr := json.NewDecoder(request.Body).Decode(&pairs) if decodeErr != nil { h.respondJSON(writer, request, map[string]string{ - "error": "invalid request body", + jsonKeyError: "invalid request body", }, http.StatusBadRequest) return @@ -966,7 +980,7 @@ func (h *Handlers) HandleEnvVarSave() http.HandlerFunc { modelPairs, validationErr := validateEnvPairs(pairs) if validationErr != "" { h.respondJSON(writer, request, map[string]string{ - "error": validationErr, + jsonKeyError: validationErr, }, http.StatusBadRequest) return @@ -978,7 +992,7 @@ func (h *Handlers) HandleEnvVarSave() http.HandlerFunc { if replaceErr != nil { h.log.Error("failed to replace env vars", "error", replaceErr) h.respondJSON(writer, request, map[string]string{ - "error": "failed to save environment variables", + jsonKeyError: "failed to save environment variables", }, http.StatusInternalServerError) return @@ -1006,32 +1020,77 @@ func (h *Handlers) HandleLabelAdd() http.HandlerFunc { } } +// deleteAppResource handles deletion of an app-owned resource (label, +// volume, or port) identified by an int64 URL parameter. The +// deleteByID closure reports whether the resource was found to belong +// to the app, and returns the deletion error if one occurred. +func (h *Handlers) deleteAppResource( + writer http.ResponseWriter, + request *http.Request, + idParam, logName string, + deleteByID deleteByIDFunc, +) { + appID := chi.URLParam(request, "id") + idStr := chi.URLParam(request, idParam) + + id, parseErr := strconv.ParseInt(idStr, 10, 64) + if parseErr != nil { + http.NotFound(writer, request) + + return + } + + found, deleteErr := deleteByID(request.Context(), appID, id) + if !found { + http.NotFound(writer, request) + + return + } + + if deleteErr != nil { + h.log.Error("failed to delete "+logName, "error", deleteErr) + } + + redirectToApp(writer, request, appID, "") +} + +// deleteByIDFunc looks up an app-owned resource by ID and deletes it +// when it belongs to the given app. It reports whether the resource +// was found, and returns lookup or deletion errors. +type deleteByIDFunc func(ctx context.Context, appID string, id int64) (bool, error) + +// makeDeleteByID builds a deleteByIDFunc from a model's find +// function, its app-ID accessor, and its delete method. +func makeDeleteByID[T any]( + db *database.Database, + find func(context.Context, *database.Database, int64) (*T, error), + appIDOf func(*T) string, + del func(*T, context.Context) error, +) deleteByIDFunc { + return func(ctx context.Context, appID string, id int64) (bool, error) { + resource, findErr := find(ctx, db, id) + if findErr != nil { + return false, findErr + } + + if resource == nil || appIDOf(resource) != appID { + return false, nil + } + + return true, del(resource, ctx) + } +} + // HandleLabelDelete handles deleting a label. func (h *Handlers) HandleLabelDelete() http.HandlerFunc { return func(writer http.ResponseWriter, request *http.Request) { - appID := chi.URLParam(request, "id") - labelIDStr := chi.URLParam(request, "labelID") - - labelID, parseErr := strconv.ParseInt(labelIDStr, 10, 64) - if parseErr != nil { - http.NotFound(writer, request) - - return - } - - label, findErr := models.FindLabel(request.Context(), h.db, labelID) - if findErr != nil || label == nil || label.AppID != appID { - http.NotFound(writer, request) - - return - } - - deleteErr := label.Delete(request.Context()) - if deleteErr != nil { - h.log.Error("failed to delete label", "error", deleteErr) - } - - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + h.deleteAppResource( + writer, request, "labelID", "label", + makeDeleteByID(h.db, models.FindLabel, + func(l *models.Label) string { return l.AppID }, + (*models.Label).Delete, + ), + ) } } @@ -1059,12 +1118,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 +1126,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,36 +1142,20 @@ 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, "") } } // HandleVolumeDelete handles deleting a volume mount. func (h *Handlers) HandleVolumeDelete() http.HandlerFunc { return func(writer http.ResponseWriter, request *http.Request) { - appID := chi.URLParam(request, "id") - volumeIDStr := chi.URLParam(request, "volumeID") - - volumeID, parseErr := strconv.ParseInt(volumeIDStr, 10, 64) - if parseErr != nil { - http.NotFound(writer, request) - - return - } - - volume, findErr := models.FindVolume(request.Context(), h.db, volumeID) - if findErr != nil || volume == nil || volume.AppID != appID { - http.NotFound(writer, request) - - return - } - - deleteErr := volume.Delete(request.Context()) - if deleteErr != nil { - h.log.Error("failed to delete volume", "error", deleteErr) - } - - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + h.deleteAppResource( + writer, request, "volumeID", "volume", + makeDeleteByID(h.db, models.FindVolume, + func(v *models.Volume) string { return v.AppID }, + (*models.Volume).Delete, + ), + ) } } @@ -1145,7 +1183,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 +1204,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, "") } } @@ -1190,29 +1228,13 @@ func parsePortValues(hostPortStr, containerPortStr string) (int, int, bool) { // HandlePortDelete handles deleting a port mapping. func (h *Handlers) HandlePortDelete() http.HandlerFunc { return func(writer http.ResponseWriter, request *http.Request) { - appID := chi.URLParam(request, "id") - portIDStr := chi.URLParam(request, "portID") - - portID, parseErr := strconv.ParseInt(portIDStr, 10, 64) - if parseErr != nil { - http.NotFound(writer, request) - - return - } - - port, findErr := models.FindPort(request.Context(), h.db, portID) - if findErr != nil || port == nil || port.AppID != appID { - http.NotFound(writer, request) - - return - } - - deleteErr := port.Delete(request.Context()) - if deleteErr != nil { - h.log.Error("failed to delete port", "error", deleteErr) - } - - http.Redirect(writer, request, "/apps/"+appID, http.StatusSeeOther) + h.deleteAppResource( + writer, request, "portID", "port", + makeDeleteByID(h.db, models.FindPort, + func(p *models.Port) string { return p.AppID }, + (*models.Port).Delete, + ), + ) } } @@ -1274,7 +1296,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 +1309,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 +1345,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 +1353,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 +1367,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, "") } } @@ -1389,8 +1411,9 @@ func optionalNullString(s string) sql.NullString { return sql.NullString{} } -// applyResourceLimits parses CPU and memory limit form values and applies them to the app. -// Returns an error message string if validation fails, or empty string on success. +// applyResourceLimits parses CPU and memory limit form values and +// applies them to the app. Returns an error message string if +// validation fails, or empty string on success. func applyResourceLimits(application *models.App, request *http.Request) string { cpuLimit, cpuErr := parseOptionalFloat64(request.FormValue("cpu_limit")) if cpuErr != nil { @@ -1425,7 +1448,8 @@ func memoryUnitMultiplier(suffix byte) int64 { } // parseOptionalFloat64 parses an optional float64 form field. -// Returns a valid NullFloat64 if the string is non-empty and parses to a positive number. +// Returns a valid NullFloat64 if the string is non-empty and parses +// to a positive number. // Returns an empty NullFloat64 if the string is empty. // Returns an error if the string is non-empty but invalid or non-positive. func parseOptionalFloat64(s string) (sql.NullFloat64, error) { @@ -1447,7 +1471,8 @@ func parseOptionalFloat64(s string) (sql.NullFloat64, error) { } // parseOptionalMemoryBytes parses an optional memory limit string into bytes. -// Accepts plain bytes (e.g. "536870912") or suffixed values (e.g. "512m", "1g", "256k"). +// Accepts plain bytes (e.g. "536870912") or suffixed values +// (e.g. "512m", "1g", "256k"). // Returns a valid NullInt64 with bytes if non-empty, empty NullInt64 if blank. func parseOptionalMemoryBytes(s string) (sql.NullInt64, error) { s = strings.TrimSpace(s) diff --git a/internal/handlers/app_name_validation_test.go b/internal/handlers/app_name_validation_test.go index 2811116..b4120fb 100644 --- a/internal/handlers/app_name_validation_test.go +++ b/internal/handlers/app_name_validation_test.go @@ -21,8 +21,16 @@ func TestValidateAppName(t *testing.T) { {"empty", "", true}, {"single char", "a", true}, {"too long", "a" + string(make([]byte, 63)), true}, - {"exactly 63 chars", "a23456789012345678901234567890123456789012345678901234567890123", false}, - {"64 chars", "a234567890123456789012345678901234567890123456789012345678901234", true}, + { + "exactly 63 chars", + "a23456789012345678901234567890123456789012345678901234567890123", + false, + }, + { + "64 chars", + "a234567890123456789012345678901234567890123456789012345678901234", + true, + }, {"uppercase", "MyApp", true}, {"spaces", "my app", true}, {"starts with hyphen", "-myapp", true}, diff --git a/internal/handlers/handlers.go b/internal/handlers/handlers.go index 728eeb9..773e0bd 100644 --- a/internal/handlers/handlers.go +++ b/internal/handlers/handlers.go @@ -22,6 +22,19 @@ import ( "sneak.berlin/go/upaas/templates" ) +// Template data keys shared across handlers. +const ( + dataKeyApp = "App" + dataKeyError = "Error" +) + +// JSON response keys shared across handlers. +const ( + jsonKeyError = "error" + jsonKeyLogs = "logs" + jsonKeyStatus = "status" +) + // Params contains dependencies for Handlers. type Params struct { fx.In diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 6a7d0b3..6e0624a 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -32,6 +32,11 @@ import ( "sneak.berlin/go/upaas/internal/service/webhook" ) +const ( + branchMain = "main" + paramSecret = "secret" +) + type testContext struct { handlers *handlers.Handlers database *database.Database @@ -193,7 +198,8 @@ func TestHandleHealthCheck(t *testing.T) { testCtx := setupTestHandlers(t) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/.well-known/healthcheck.json", nil, @@ -210,6 +216,26 @@ func TestHandleHealthCheck(t *testing.T) { }) } +// assertPageRenders serves a GET request for path with the given +// handler and asserts a 200 response containing want. +func assertPageRenders( + t *testing.T, + handler http.Handler, + path, want string, +) { + t.Helper() + + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, path, nil, + ) + recorder := httptest.NewRecorder() + + handler.ServeHTTP(recorder, request) + + assert.Equal(t, http.StatusOK, recorder.Code) + assert.Contains(t, recorder.Body.String(), want) +} + func TestHandleSetupGET(t *testing.T) { t.Parallel() @@ -217,15 +243,7 @@ func TestHandleSetupGET(t *testing.T) { t.Parallel() testCtx := setupTestHandlers(t) - - request := httptest.NewRequest(http.MethodGet, "/setup", nil) - recorder := httptest.NewRecorder() - - handler := testCtx.handlers.HandleSetupGET() - handler.ServeHTTP(recorder, request) - - assert.Equal(t, http.StatusOK, recorder.Code) - assert.Contains(t, recorder.Body.String(), "setup") + assertPageRenders(t, testCtx.handlers.HandleSetupGET(), "/setup", "setup") }) } @@ -237,7 +255,8 @@ func createSetupFormRequest( form.Set("password", password) form.Set("password_confirm", confirm) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, "/setup", strings.NewReader(form.Encode()), @@ -314,15 +333,7 @@ func TestHandleLoginGET(t *testing.T) { t.Parallel() testCtx := setupTestHandlers(t) - - request := httptest.NewRequest(http.MethodGet, "/login", nil) - recorder := httptest.NewRecorder() - - handler := testCtx.handlers.HandleLoginGET() - handler.ServeHTTP(recorder, request) - - assert.Equal(t, http.StatusOK, recorder.Code) - assert.Contains(t, recorder.Body.String(), "login") + assertPageRenders(t, testCtx.handlers.HandleLoginGET(), "/login", "login") }) } @@ -331,7 +342,8 @@ func createLoginFormRequest(username, password string) *http.Request { form.Set("username", username) form.Set("password", password) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + context.Background(), http.MethodPost, "/login", strings.NewReader(form.Encode()), @@ -395,7 +407,9 @@ func TestHandleDashboard(t *testing.T) { testCtx := setupTestHandlers(t) - request := httptest.NewRequest(http.MethodGet, "/", nil) + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil, + ) recorder := httptest.NewRecorder() handler := testCtx.handlers.HandleDashboard() @@ -413,7 +427,9 @@ func TestHandleDashboard(t *testing.T) { // Create an app so the template iterates over AppStats and hits .CSRFField createTestApp(t, testCtx, "csrf-test-app") - request := httptest.NewRequest(http.MethodGet, "/", nil) + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil, + ) recorder := httptest.NewRecorder() handler := testCtx.handlers.HandleDashboard() @@ -433,7 +449,9 @@ func TestHandleAppNew(t *testing.T) { testCtx := setupTestHandlers(t) - request := httptest.NewRequest(http.MethodGet, "/apps/new", nil) + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/apps/new", nil, + ) recorder := httptest.NewRecorder() handler := testCtx.handlers.HandleAppNew() @@ -472,7 +490,7 @@ func createTestApp( app.CreateAppInput{ Name: name, RepoURL: "git@example.com:user/" + name + ".git", - Branch: "main", + Branch: branchMain, }, ) require.NoError(t, err) @@ -493,7 +511,7 @@ func TestHandleWebhookRejectsOversizedBody(t *testing.T) { app.CreateAppInput{ Name: "oversize-test-app", RepoURL: "git@example.com:user/repo.git", - Branch: "main", + Branch: branchMain, }, ) require.NoError(t, createErr) @@ -501,14 +519,15 @@ func TestHandleWebhookRejectsOversizedBody(t *testing.T) { // Create a body larger than 1MB - it should be silently truncated // and the webhook should still process (or fail gracefully on parse) largePayload := strings.Repeat("x", 2*1024*1024) // 2MB - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/webhook/"+createdApp.WebhookSecret, strings.NewReader(largePayload), ) request = addChiURLParams( request, - map[string]string{"secret": createdApp.WebhookSecret}, + map[string]string{paramSecret: createdApp.WebhookSecret}, ) request.Header.Set("Content-Type", "application/json") request.Header.Set("X-Gitea-Event", "push") @@ -544,7 +563,8 @@ func testOwnershipVerification(t *testing.T, cfg ownedResourceTestConfig) { resourceID := cfg.createFn(t, testCtx, app1) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, cfg.deletePath(app2.ID, resourceID), nil, @@ -583,7 +603,8 @@ func TestHandleEnvVarSaveBulk(t *testing.T) { r := chi.NewRouter() r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+createdApp.ID+"/env", strings.NewReader(body), @@ -625,7 +646,8 @@ func TestHandleEnvVarSaveAppNotFound(t *testing.T) { r := chi.NewRouter() r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/nonexistent-id/env", strings.NewReader(body), @@ -651,7 +673,8 @@ func TestHandleEnvVarSaveEmptyKeyRejected(t *testing.T) { r := chi.NewRouter() r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+createdApp.ID+"/env", strings.NewReader(body), @@ -673,12 +696,14 @@ func TestHandleEnvVarSaveDuplicateKeyRejected(t *testing.T) { createdApp := createTestApp(t, testCtx, "envvar-dedup-app") // Send two entries with the same key — should be rejected - body := `[{"key":"FOO","value":"first"},{"key":"BAR","value":"bar"},{"key":"FOO","value":"second"}]` + body := `[{"key":"FOO","value":"first"},{"key":"BAR","value":"bar"},` + + `{"key":"FOO","value":"second"}]` r := chi.NewRouter() r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+createdApp.ID+"/env", strings.NewReader(body), @@ -716,7 +741,8 @@ func TestHandleEnvVarSaveCrossAppIsolation(t *testing.T) { r := chi.NewRouter() r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+appA.ID+"/env", strings.NewReader(body), @@ -779,7 +805,8 @@ func TestHandleEnvVarSaveBodySizeLimit(t *testing.T) { r := chi.NewRouter() r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+createdApp.ID+"/env", strings.NewReader(sb.String()), @@ -848,7 +875,8 @@ func TestDeleteVolumeOwnershipVerification(t *testing.T) { require.NoError(t, volume.Save(context.Background())) // Try to delete app1's volume using app2's URL path - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+app2.ID+"/volumes/"+strconv.FormatInt(volume.ID, 10)+"/delete", nil, @@ -889,7 +917,8 @@ func TestDeletePortOwnershipVerification(t *testing.T) { require.NoError(t, port.Save(context.Background())) // Try to delete app1's port using app2's URL path - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+app2.ID+"/ports/"+strconv.FormatInt(port.ID, 10)+"/delete", nil, @@ -930,7 +959,8 @@ func TestHandleEnvVarSaveEmptyClears(t *testing.T) { r := chi.NewRouter() r.Post("/apps/{id}/env", testCtx.handlers.HandleEnvVarSave()) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+createdApp.ID+"/env", strings.NewReader("[]"), @@ -979,7 +1009,8 @@ func TestHandleVolumeAddValidatesPaths(t *testing.T) { form.Set("host_path", tt.hostPath) form.Set("container_path", tt.containerPath) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+createdApp.ID+"/volumes", strings.NewReader(form.Encode()), @@ -1016,7 +1047,8 @@ func TestHandleVolumeAddValidatesPaths(t *testing.T) { } // TestSetupRequiredExemptsHealthAndStaticAndAPI verifies that the SetupRequired -// middleware allows /health, /s/*, and /api/* paths through even when setup is required. +// middleware allows /health, /s/*, and /api/* paths through even when setup is +// required. func TestSetupRequiredExemptsHealthAndStaticAndAPI(t *testing.T) { t.Parallel() @@ -1032,13 +1064,21 @@ func TestSetupRequiredExemptsHealthAndStaticAndAPI(t *testing.T) { wrapped := mw(okHandler) - exemptPaths := []string{"/health", "/s/style.css", "/s/js/app.js", "/api/v1/apps", "/api/v1/login"} + exemptPaths := []string{ + "/health", + "/s/style.css", + "/s/js/app.js", + "/api/v1/apps", + "/api/v1/login", + } for _, path := range exemptPaths { t.Run(path, func(t *testing.T) { t.Parallel() - req := httptest.NewRequest(http.MethodGet, path, nil) + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, path, nil, + ) rr := httptest.NewRecorder() wrapped.ServeHTTP(rr, req) @@ -1051,7 +1091,9 @@ func TestSetupRequiredExemptsHealthAndStaticAndAPI(t *testing.T) { t.Run("non-exempt redirects", func(t *testing.T) { t.Parallel() - req := httptest.NewRequest(http.MethodGet, "/", nil) + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil, + ) rr := httptest.NewRecorder() wrapped.ServeHTTP(rr, req) @@ -1067,7 +1109,8 @@ func TestHandleCancelDeployRedirects(t *testing.T) { createdApp := createTestApp(t, testCtx, "cancel-deploy-app") - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/"+createdApp.ID+"/deployments/cancel", nil, @@ -1087,7 +1130,8 @@ func TestHandleCancelDeployReturns404ForUnknownApp(t *testing.T) { testCtx := setupTestHandlers(t) - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "/apps/nonexistent/deployments/cancel", nil, @@ -1108,12 +1152,16 @@ func TestHandleWebhookReturns404ForUnknownSecret(t *testing.T) { webhookURL := "/webhook/unknown-secret" payload := `{"ref": "refs/heads/main"}` - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, webhookURL, strings.NewReader(payload), ) - request = addChiURLParams(request, map[string]string{"secret": "unknown-secret"}) + request = addChiURLParams( + request, + map[string]string{paramSecret: "unknown-secret"}, + ) request.Header.Set("Content-Type", "application/json") request.Header.Set("X-Gitea-Event", "push") @@ -1136,21 +1184,22 @@ func TestHandleWebhookProcessesValidWebhook(t *testing.T) { app.CreateAppInput{ Name: "webhook-test-app", RepoURL: "git@example.com:user/repo.git", - Branch: "main", + Branch: branchMain, }, ) require.NoError(t, createErr) payload := `{"ref": "refs/heads/main", "after": "abc123"}` webhookURL := "/webhook/" + createdApp.WebhookSecret - request := httptest.NewRequest( + request := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, webhookURL, strings.NewReader(payload), ) request = addChiURLParams( request, - map[string]string{"secret": createdApp.WebhookSecret}, + map[string]string{paramSecret: createdApp.WebhookSecret}, ) request.Header.Set("Content-Type", "application/json") request.Header.Set("X-Gitea-Event", "push") diff --git a/internal/handlers/render_template_test.go b/internal/handlers/render_template_test.go index 98b8fc0..8d3c55b 100644 --- a/internal/handlers/render_template_test.go +++ b/internal/handlers/render_template_test.go @@ -16,7 +16,9 @@ func TestRenderTemplateBuffersOutput(t *testing.T) { testCtx := setupTestHandlers(t) // The setup page is simple and has no DB dependencies - request := httptest.NewRequest(http.MethodGet, "/setup", nil) + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/setup", nil, + ) recorder := httptest.NewRecorder() handler := testCtx.handlers.HandleSetupGET() @@ -39,7 +41,7 @@ func TestDashboardRenderTemplateBuffersOutput(t *testing.T) { testCtx := setupTestHandlers(t) - request := httptest.NewRequest(http.MethodGet, "/", nil) + request := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) recorder := httptest.NewRecorder() handler := testCtx.handlers.HandleDashboard() @@ -59,7 +61,9 @@ func TestLoginRenderTemplateBuffersOutput(t *testing.T) { testCtx := setupTestHandlers(t) - request := httptest.NewRequest(http.MethodGet, "/login", nil) + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/login", nil, + ) recorder := httptest.NewRecorder() handler := testCtx.handlers.HandleLoginGET() diff --git a/internal/handlers/repo_url_validation.go b/internal/handlers/repo_url_validation.go index b4fea90..080bf24 100644 --- a/internal/handlers/repo_url_validation.go +++ b/internal/handlers/repo_url_validation.go @@ -11,13 +11,17 @@ import ( var ( errRepoURLEmpty = errors.New("repository URL must not be empty") errRepoURLScheme = errors.New("file:// URLs are not allowed for security reasons") - errRepoURLInvalid = errors.New("repository URL must use https://, http://, ssh://, git://, or git@host:path format") - errRepoURLNoHost = errors.New("repository URL must include a host") - errRepoURLNoPath = errors.New("repository URL must include a path") + errRepoURLInvalid = errors.New( + "repository URL must use https://, http://, ssh://, git://, " + + "or git@host:path format", + ) + errRepoURLNoHost = errors.New("repository URL must include a host") + errRepoURLNoPath = errors.New("repository URL must include a path") ) -// scpLikeRepoRe matches SCP-like git URLs: git@host:path (e.g. git@github.com:user/repo.git). -// Only the "git" user is allowed, as that is the standard for SSH deploy keys. +// scpLikeRepoRe matches SCP-like git URLs: git@host:path +// (e.g. git@github.com:user/repo.git). Only the "git" user is allowed, +// as that is the standard for SSH deploy keys. var scpLikeRepoRe = regexp.MustCompile(`^git@[a-zA-Z0-9._-]+:.+$`) // allowedRepoSchemes lists the URL schemes accepted for repository URLs. @@ -30,7 +34,8 @@ var allowedRepoSchemes = map[string]bool{ "git": true, } -// validateRepoURL checks that the given repository URL is valid and uses an allowed scheme. +// 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 diff --git a/internal/handlers/repo_url_validation_test.go b/internal/handlers/repo_url_validation_test.go index 5e0c28f..9ddd9f3 100644 --- a/internal/handlers/repo_url_validation_test.go +++ b/internal/handlers/repo_url_validation_test.go @@ -22,7 +22,11 @@ func TestValidateRepoURL(t *testing.T) { {name: "SCP-like URL", url: "git@github.com:user/repo.git", wantErr: false}, {name: "SCP-like with dots", url: "git@git.example.com:org/repo.git", wantErr: false}, {name: "https without .git", url: "https://github.com/user/repo", wantErr: false}, - {name: "https with port", url: "https://git.example.com:8443/user/repo.git", wantErr: false}, + { + name: "https with port", + url: "https://git.example.com:8443/user/repo.git", + wantErr: false, + }, // Invalid URLs {name: "empty string", url: "", wantErr: true}, @@ -37,10 +41,22 @@ func TestValidateRepoURL(t *testing.T) { {name: "no path https", url: "https://github.com", wantErr: true}, {name: "no path https trailing slash", url: "https://github.com/", wantErr: true}, {name: "SCP-like non-git user", url: "root@github.com:user/repo.git", wantErr: true}, - {name: "SCP-like arbitrary user", url: "admin@github.com:user/repo.git", wantErr: true}, + { + name: "SCP-like arbitrary user", + url: "admin@github.com:user/repo.git", + wantErr: true, + }, {name: "path traversal SCP", url: "git@github.com:../../etc/passwd", wantErr: true}, - {name: "path traversal https", url: "https://github.com/user/../../../etc/passwd", wantErr: true}, - {name: "path traversal in middle", url: "https://github.com/user/repo/../secret", wantErr: true}, + { + name: "path traversal https", + url: "https://github.com/user/../../../etc/passwd", + wantErr: true, + }, + { + name: "path traversal in middle", + url: "https://github.com/user/repo/../secret", + wantErr: true, + }, } for _, tc := range tests { diff --git a/internal/handlers/sanitize.go b/internal/handlers/sanitize.go index 91f2ddc..2dc96e7 100644 --- a/internal/handlers/sanitize.go +++ b/internal/handlers/sanitize.go @@ -5,8 +5,11 @@ import ( "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[^[\]])`) +// 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) diff --git a/internal/handlers/sanitize_test.go b/internal/handlers/sanitize_test.go index 7e24851..a6af2e1 100644 --- a/internal/handlers/sanitize_test.go +++ b/internal/handlers/sanitize_test.go @@ -6,7 +6,7 @@ import ( "sneak.berlin/go/upaas/internal/handlers" ) -func TestSanitizeLogs(t *testing.T) { //nolint:funlen // table-driven tests +func TestSanitizeLogs(t *testing.T) { t.Parallel() tests := []struct { diff --git a/internal/handlers/setup.go b/internal/handlers/setup.go index e84fc47..abb5009 100644 --- a/internal/handlers/setup.go +++ b/internal/handlers/setup.go @@ -55,8 +55,8 @@ func (h *Handlers) renderSetupError( errorMsg string, ) { data := h.addGlobals(map[string]any{ - "Username": username, - "Error": errorMsg, + "Username": username, + dataKeyError: errorMsg, }, request) h.renderTemplate(writer, tmpl, "setup.html", data) } diff --git a/internal/handlers/webhook_events.go b/internal/handlers/webhook_events.go index d455cda..1820c86 100644 --- a/internal/handlers/webhook_events.go +++ b/internal/handlers/webhook_events.go @@ -47,8 +47,8 @@ func (h *Handlers) HandleAppWebhookEvents() http.HandlerFunc { } data := h.addGlobals(map[string]any{ - "App": application, - "Events": events, + dataKeyApp: application, + "Events": events, }, request) h.renderTemplate(writer, tmpl, "webhook_events.html", data) diff --git a/internal/middleware/cors_test.go b/internal/middleware/cors_test.go index 2b7eba6..554b8c3 100644 --- a/internal/middleware/cors_test.go +++ b/internal/middleware/cors_test.go @@ -24,21 +24,30 @@ func newCORSTestMiddleware(corsOrigins string) *Middleware { } } -func TestCORS_NoOriginsConfigured_NoCORSHeaders(t *testing.T) { - t.Parallel() +// assertNoCORSHeaders runs a request with the given Origin header through +// CORS middleware configured with corsOrigins and asserts that no +// Access-Control-Allow-Origin header is set. +func assertNoCORSHeaders(t *testing.T, corsOrigins, origin, msg string) { + t.Helper() - m := newCORSTestMiddleware("") + m := newCORSTestMiddleware(corsOrigins) handler := m.CORS()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) })) - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.Header.Set("Origin", "https://evil.com") + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) + req.Header.Set("Origin", origin) rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) - assert.Empty(t, rec.Header().Get("Access-Control-Allow-Origin"), + assert.Empty(t, rec.Header().Get("Access-Control-Allow-Origin"), msg) +} + +func TestCORS_NoOriginsConfigured_NoCORSHeaders(t *testing.T) { + t.Parallel() + + assertNoCORSHeaders(t, "", "https://evil.com", "expected no CORS headers when no origins configured") } @@ -50,7 +59,7 @@ func TestCORS_OriginsConfigured_AllowsMatchingOrigin(t *testing.T) { w.WriteHeader(http.StatusOK) })) - req := httptest.NewRequest(http.MethodGet, "/", nil) + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) req.Header.Set("Origin", "https://app.example.com") rec := httptest.NewRecorder() @@ -65,17 +74,6 @@ func TestCORS_OriginsConfigured_AllowsMatchingOrigin(t *testing.T) { func TestCORS_OriginsConfigured_RejectsNonMatchingOrigin(t *testing.T) { t.Parallel() - m := newCORSTestMiddleware("https://app.example.com") - handler := m.CORS()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - })) - - req := httptest.NewRequest(http.MethodGet, "/", nil) - req.Header.Set("Origin", "https://evil.com") - - rec := httptest.NewRecorder() - handler.ServeHTTP(rec, req) - - assert.Empty(t, rec.Header().Get("Access-Control-Allow-Origin"), + assertNoCORSHeaders(t, "https://app.example.com", "https://evil.com", "expected no CORS headers for non-matching origin") } diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index b0ad2d7..63a6c2e 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -370,8 +370,9 @@ func (m *Middleware) LoginRateLimit() func(http.Handler) http.Handler { } } -// APISessionAuth returns middleware that requires session authentication for API routes. -// Unlike SessionAuth, it returns JSON 401 responses instead of redirecting to /login. +// APISessionAuth returns middleware that requires session authentication +// for API routes. Unlike SessionAuth, it returns JSON 401 responses instead +// of redirecting to /login. func (m *Middleware) APISessionAuth() func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func( diff --git a/internal/middleware/ratelimit_test.go b/internal/middleware/ratelimit_test.go index ab997e8..4dfa552 100644 --- a/internal/middleware/ratelimit_test.go +++ b/internal/middleware/ratelimit_test.go @@ -30,13 +30,15 @@ func TestLoginRateLimitAllowsUpToBurst(t *testing.T) { mw := newTestMiddleware(t) - handler := mw.LoginRateLimit()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - })) + handler := mw.LoginRateLimit()(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + )) // First 5 requests should succeed (burst) for i := range 5 { - req := httptest.NewRequest(http.MethodPost, "/login", nil) + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/login", nil) req.RemoteAddr = "192.168.1.1:12345" rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) @@ -44,11 +46,12 @@ func TestLoginRateLimitAllowsUpToBurst(t *testing.T) { } // 6th request should be rate limited - req := httptest.NewRequest(http.MethodPost, "/login", nil) + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/login", nil) req.RemoteAddr = "192.168.1.1:12345" rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) - assert.Equal(t, http.StatusTooManyRequests, rec.Code, "6th request should be rate limited") + assert.Equal(t, http.StatusTooManyRequests, rec.Code, + "6th request should be rate limited") } //nolint:paralleltest // mutates global loginLimiter @@ -57,27 +60,29 @@ func TestLoginRateLimitIsolatesIPs(t *testing.T) { mw := newTestMiddleware(t) - handler := mw.LoginRateLimit()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - })) + handler := mw.LoginRateLimit()(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + )) // Exhaust IP1's budget for range 5 { - req := httptest.NewRequest(http.MethodPost, "/login", nil) - req.RemoteAddr = "10.0.0.1:1234" + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/login", nil) + req.RemoteAddr = testProxyAddr rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) } // IP1 should be blocked - req := httptest.NewRequest(http.MethodPost, "/login", nil) - req.RemoteAddr = "10.0.0.1:1234" + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/login", nil) + req.RemoteAddr = testProxyAddr rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) assert.Equal(t, http.StatusTooManyRequests, rec.Code) // IP2 should still work - req2 := httptest.NewRequest(http.MethodPost, "/login", nil) + req2 := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/login", nil) req2.RemoteAddr = "10.0.0.2:1234" rec2 := httptest.NewRecorder() handler.ServeHTTP(rec2, req2) @@ -90,25 +95,28 @@ func TestLoginRateLimitReturns429Body(t *testing.T) { mw := newTestMiddleware(t) - handler := mw.LoginRateLimit()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - })) + handler := mw.LoginRateLimit()(http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + )) // Exhaust burst for range 5 { - req := httptest.NewRequest(http.MethodPost, "/login", nil) + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/login", nil) req.RemoteAddr = "172.16.0.1:5555" rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) } - req := httptest.NewRequest(http.MethodPost, "/login", nil) + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/login", nil) req.RemoteAddr = "172.16.0.1:5555" rec := httptest.NewRecorder() handler.ServeHTTP(rec, req) assert.Equal(t, http.StatusTooManyRequests, rec.Code) assert.Contains(t, rec.Body.String(), "Too Many Requests") - assert.NotEmpty(t, rec.Header().Get("Retry-After"), "should include Retry-After header") + assert.NotEmpty(t, rec.Header().Get("Retry-After"), + "should include Retry-After header") } func TestIPLimiterEvictsStaleEntries(t *testing.T) { diff --git a/internal/middleware/realip_test.go b/internal/middleware/realip_test.go index f38aa48..a7fb597 100644 --- a/internal/middleware/realip_test.go +++ b/internal/middleware/realip_test.go @@ -7,6 +7,16 @@ import ( "testing" ) +// Shared test addresses (also used by ratelimit_test.go). +const ( + testProxyAddr = "10.0.0.1:1234" + testRealIP = "203.0.113.5" + testXFFIP = "198.51.100.1" + testPrivateIP = "192.168.1.1" + testPublicIP = "93.184.216.34" + testPublicDNSIP = "8.8.8.8" +) + func TestRealIP(t *testing.T) { //nolint:funlen // table-driven test t.Parallel() @@ -20,63 +30,63 @@ func TestRealIP(t *testing.T) { //nolint:funlen // table-driven test // === Trusted proxy (RFC1918 / loopback) — headers ARE honoured === { name: "trusted: X-Real-IP from 10.x", - remoteAddr: "10.0.0.1:1234", - xRealIP: "203.0.113.5", + remoteAddr: testProxyAddr, + xRealIP: testRealIP, xff: "198.51.100.1, 10.0.0.1", - want: "203.0.113.5", + want: testRealIP, }, { name: "trusted: XFF from 10.x when no X-Real-IP", - remoteAddr: "10.0.0.1:1234", + remoteAddr: testProxyAddr, xff: "198.51.100.1, 10.0.0.1", - want: "198.51.100.1", + want: testXFFIP, }, { name: "trusted: XFF single IP from 10.x", - remoteAddr: "10.0.0.1:1234", + remoteAddr: testProxyAddr, xff: "203.0.113.10", want: "203.0.113.10", }, { name: "trusted: falls back to RemoteAddr (192.168.x)", remoteAddr: "192.168.1.1:5678", - want: "192.168.1.1", + want: testPrivateIP, }, { name: "trusted: RemoteAddr without port", - remoteAddr: "192.168.1.1", - want: "192.168.1.1", + remoteAddr: testPrivateIP, + want: testPrivateIP, }, { name: "trusted: X-Real-IP with whitespace from 10.x", - remoteAddr: "10.0.0.1:1234", + remoteAddr: testProxyAddr, xRealIP: " 203.0.113.5 ", - want: "203.0.113.5", + want: testRealIP, }, { name: "trusted: XFF with whitespace from 10.x", - remoteAddr: "10.0.0.1:1234", + remoteAddr: testProxyAddr, xff: " 198.51.100.1 , 10.0.0.1", - want: "198.51.100.1", + want: testXFFIP, }, { name: "trusted: empty X-Real-IP falls through to XFF from 10.x", - remoteAddr: "10.0.0.1:1234", + remoteAddr: testProxyAddr, xRealIP: " ", - xff: "198.51.100.1", - want: "198.51.100.1", + xff: testXFFIP, + want: testXFFIP, }, { name: "trusted: loopback honours X-Real-IP", remoteAddr: "127.0.0.1:9999", - xRealIP: "93.184.216.34", - want: "93.184.216.34", + xRealIP: testPublicIP, + want: testPublicIP, }, { name: "trusted: 172.16.x honours XFF", remoteAddr: "172.16.0.1:4321", - xff: "8.8.8.8", - want: "8.8.8.8", + xff: testPublicDNSIP, + want: testPublicDNSIP, }, // === Untrusted proxy (public IP) — headers IGNORED, use RemoteAddr === @@ -97,17 +107,17 @@ func TestRealIP(t *testing.T) { //nolint:funlen // table-driven test remoteAddr: "8.8.8.8:443", xRealIP: "1.2.3.4", xff: "5.6.7.8", - want: "8.8.8.8", + want: testPublicDNSIP, }, { name: "untrusted: no headers, public RemoteAddr", remoteAddr: "93.184.216.34:8080", - want: "93.184.216.34", + want: testPublicIP, }, { name: "untrusted: public RemoteAddr without port", - remoteAddr: "93.184.216.34", - want: "93.184.216.34", + remoteAddr: testPublicIP, + want: testPublicIP, }, } @@ -139,7 +149,9 @@ func TestIsTrustedProxy(t *testing.T) { trusted := []string{"10.0.0.1", "10.255.255.255", "172.16.0.1", "172.31.255.255", "192.168.0.1", "192.168.255.255", "127.0.0.1", "127.255.255.255", "::1"} - untrusted := []string{"8.8.8.8", "203.0.113.1", "172.32.0.1", "11.0.0.1", "2001:db8::1"} + untrusted := []string{ + testPublicDNSIP, "203.0.113.1", "172.32.0.1", "11.0.0.1", "2001:db8::1", + } for _, addr := range trusted { ip := net.ParseIP(addr) diff --git a/internal/models/env_var.go b/internal/models/env_var.go index a6967b9..8ce56c4 100644 --- a/internal/models/env_var.go +++ b/internal/models/env_var.go @@ -93,6 +93,41 @@ func FindEnvVar( return envVar, nil } +// findAllByAppID loads all rows for an app, scanning each row into a +// new model created by newFn. entity names the model in error messages. +func findAllByAppID[T interface{ scanDest() []any }]( + ctx context.Context, + db *database.Database, + query, appID, entity string, + newFn func(*database.Database) T, +) ([]T, error) { + rows, err := db.Query(ctx, query, appID) + if err != nil { + return nil, fmt.Errorf("querying %s by app: %w", entity, err) + } + + defer func() { _ = rows.Close() }() + + var items []T + + for rows.Next() { + item := newFn(db) + + scanErr := rows.Scan(item.scanDest()...) + if scanErr != nil { + return nil, scanErr + } + + items = append(items, item) + } + + return items, rows.Err() +} + +func (e *EnvVar) scanDest() []any { + return []any{&e.ID, &e.AppID, &e.Key, &e.Value} +} + // FindEnvVarsByAppID finds all env vars for an app. func FindEnvVarsByAppID( ctx context.Context, @@ -103,29 +138,7 @@ func FindEnvVarsByAppID( SELECT id, app_id, key, value FROM app_env_vars WHERE app_id = ? ORDER BY key` - rows, err := db.Query(ctx, query, appID) - if err != nil { - return nil, fmt.Errorf("querying env vars by app: %w", err) - } - - defer func() { _ = rows.Close() }() - - var envVars []*EnvVar - - for rows.Next() { - envVar := NewEnvVar(db) - - scanErr := rows.Scan( - &envVar.ID, &envVar.AppID, &envVar.Key, &envVar.Value, - ) - if scanErr != nil { - return nil, scanErr - } - - envVars = append(envVars, envVar) - } - - return envVars, rows.Err() + return findAllByAppID(ctx, db, query, appID, "env vars", NewEnvVar) } // EnvVarPair is a key-value pair for bulk env var operations. diff --git a/internal/models/label.go b/internal/models/label.go index 6910421..9af2ae1 100644 --- a/internal/models/label.go +++ b/internal/models/label.go @@ -93,6 +93,10 @@ func FindLabel( return label, nil } +func (l *Label) scanDest() []any { + return []any{&l.ID, &l.AppID, &l.Key, &l.Value} +} + // FindLabelsByAppID finds all labels for an app. func FindLabelsByAppID( ctx context.Context, @@ -103,27 +107,7 @@ func FindLabelsByAppID( SELECT id, app_id, key, value FROM app_labels WHERE app_id = ? ORDER BY key` - rows, err := db.Query(ctx, query, appID) - if err != nil { - return nil, fmt.Errorf("querying labels by app: %w", err) - } - - defer func() { _ = rows.Close() }() - - var labels []*Label - - for rows.Next() { - label := NewLabel(db) - - scanErr := rows.Scan(&label.ID, &label.AppID, &label.Key, &label.Value) - if scanErr != nil { - return nil, scanErr - } - - labels = append(labels, label) - } - - return labels, rows.Err() + return findAllByAppID(ctx, db, query, appID, "labels", NewLabel) } // DeleteLabelsByAppID deletes all labels for an app. diff --git a/internal/models/models_test.go b/internal/models/models_test.go index 727a746..a4ac37f 100644 --- a/internal/models/models_test.go +++ b/internal/models/models_test.go @@ -317,33 +317,54 @@ func TestAllApps(t *testing.T) { // EnvVar Tests. +// testKVCreateAndFind exercises the create-and-find round trip shared +// by key-value models (env vars, labels). +func testKVCreateAndFind[T any]( + t *testing.T, + wantKey string, + create func(db *database.Database, appID string) (int64, error), + find func(context.Context, *database.Database, string) ([]T, error), + keyOf func(T) string, +) { + t.Helper() + + testDB, cleanup := setupTestDB(t) + defer cleanup() + + // Create app first. + app := createTestApp(t, testDB) + + id, err := create(testDB, app.ID) + require.NoError(t, err) + assert.NotZero(t, id) + + found, err := find(context.Background(), testDB, app.ID) + require.NoError(t, err) + require.Len(t, found, 1) + assert.Equal(t, wantKey, keyOf(found[0])) +} + +func saveTestEnvVar(db *database.Database, appID string) (int64, error) { + envVar := models.NewEnvVar(db) + envVar.AppID = appID + envVar.Key = "DATABASE_URL" + envVar.Value = "postgres://localhost/db" + + err := envVar.Save(context.Background()) + + return envVar.ID, err +} + func TestEnvVarCRUD(t *testing.T) { t.Parallel() t.Run("creates and finds env vars", func(t *testing.T) { t.Parallel() - testDB, cleanup := setupTestDB(t) - defer cleanup() - - // Create app first. - app := createTestApp(t, testDB) - - envVar := models.NewEnvVar(testDB) - envVar.AppID = app.ID - envVar.Key = "DATABASE_URL" - envVar.Value = "postgres://localhost/db" - - err := envVar.Save(context.Background()) - require.NoError(t, err) - assert.NotZero(t, envVar.ID) - - envVars, err := models.FindEnvVarsByAppID( - context.Background(), testDB, app.ID, + testKVCreateAndFind(t, "DATABASE_URL", saveTestEnvVar, + models.FindEnvVarsByAppID, + func(e *models.EnvVar) string { return e.Key }, ) - require.NoError(t, err) - require.Len(t, envVars, 1) - assert.Equal(t, "DATABASE_URL", envVars[0].Key) }) t.Run("deletes env var", func(t *testing.T) { @@ -375,32 +396,27 @@ func TestEnvVarCRUD(t *testing.T) { // Label Tests. +func saveTestLabel(db *database.Database, appID string) (int64, error) { + label := models.NewLabel(db) + label.AppID = appID + label.Key = "traefik.enable" + label.Value = "true" + + err := label.Save(context.Background()) + + return label.ID, err +} + func TestLabelCRUD(t *testing.T) { t.Parallel() t.Run("creates and finds labels", func(t *testing.T) { t.Parallel() - testDB, cleanup := setupTestDB(t) - defer cleanup() - - app := createTestApp(t, testDB) - - label := models.NewLabel(testDB) - label.AppID = app.ID - label.Key = "traefik.enable" - label.Value = "true" - - err := label.Save(context.Background()) - require.NoError(t, err) - assert.NotZero(t, label.ID) - - labels, err := models.FindLabelsByAppID( - context.Background(), testDB, app.ID, + testKVCreateAndFind(t, "traefik.enable", saveTestLabel, + models.FindLabelsByAppID, + func(l *models.Label) string { return l.Key }, ) - require.NoError(t, err) - require.Len(t, labels, 1) - assert.Equal(t, "traefik.enable", labels[0].Key) }) } @@ -569,7 +585,9 @@ func TestDeploymentFindByAppID(t *testing.T) { require.NoError(t, err) } - deployments, err := models.FindDeploymentsByAppID(context.Background(), testDB, app.ID, 3) + deployments, err := models.FindDeploymentsByAppID( + context.Background(), testDB, app.ID, 3, + ) require.NoError(t, err) assert.Len(t, deployments, 3) } @@ -706,7 +724,6 @@ func TestAppGetWebhookEvents(t *testing.T) { // Cascade Delete Tests. -//nolint:funlen // Test function with many assertions - acceptable for integration tests func TestCascadeDelete(t *testing.T) { t.Parallel() @@ -783,7 +800,8 @@ func TestCascadeDelete(t *testing.T) { // Resource Limits Tests. -func TestAppResourceLimits(t *testing.T) { //nolint:funlen // integration test with multiple subtests +//nolint:funlen // integration test with multiple subtests +func TestAppResourceLimits(t *testing.T) { t.Parallel() t.Run("saves and loads CPU limit", func(t *testing.T) { diff --git a/internal/models/port.go b/internal/models/port.go index 5696df5..d1b80f9 100644 --- a/internal/models/port.go +++ b/internal/models/port.go @@ -112,6 +112,12 @@ func FindPort( return port, nil } +func (p *Port) scanDest() []any { + return []any{ + &p.ID, &p.AppID, &p.HostPort, &p.ContainerPort, &p.Protocol, + } +} + // FindPortsByAppID finds all ports for an app. func FindPortsByAppID( ctx context.Context, @@ -122,30 +128,7 @@ func FindPortsByAppID( SELECT id, app_id, host_port, container_port, protocol FROM app_ports WHERE app_id = ? ORDER BY host_port` - rows, err := db.Query(ctx, query, appID) - if err != nil { - return nil, fmt.Errorf("querying ports by app: %w", err) - } - - defer func() { _ = rows.Close() }() - - var ports []*Port - - for rows.Next() { - port := NewPort(db) - - scanErr := rows.Scan( - &port.ID, &port.AppID, &port.HostPort, - &port.ContainerPort, &port.Protocol, - ) - if scanErr != nil { - return nil, scanErr - } - - ports = append(ports, port) - } - - return ports, rows.Err() + return findAllByAppID(ctx, db, query, appID, "ports", NewPort) } // DeletePortsByAppID deletes all ports for an app. diff --git a/internal/models/volume.go b/internal/models/volume.go index f16f210..d31ae1a 100644 --- a/internal/models/volume.go +++ b/internal/models/volume.go @@ -103,6 +103,12 @@ func FindVolume( return vol, nil } +func (v *Volume) scanDest() []any { + return []any{ + &v.ID, &v.AppID, &v.HostPath, &v.ContainerPath, &v.ReadOnly, + } +} + // FindVolumesByAppID finds all volumes for an app. func FindVolumesByAppID( ctx context.Context, @@ -113,30 +119,7 @@ func FindVolumesByAppID( SELECT id, app_id, host_path, container_path, readonly FROM app_volumes WHERE app_id = ? ORDER BY container_path` - rows, err := db.Query(ctx, query, appID) - if err != nil { - return nil, fmt.Errorf("querying volumes by app: %w", err) - } - - defer func() { _ = rows.Close() }() - - var volumes []*Volume - - for rows.Next() { - vol := NewVolume(db) - - scanErr := rows.Scan( - &vol.ID, &vol.AppID, &vol.HostPath, - &vol.ContainerPath, &vol.ReadOnly, - ) - if scanErr != nil { - return nil, scanErr - } - - volumes = append(volumes, vol) - } - - return volumes, rows.Err() + return findAllByAppID(ctx, db, query, appID, "volumes", NewVolume) } // DeleteVolumesByAppID deletes all volumes for an app. diff --git a/internal/server/routes.go b/internal/server/routes.go index ebddba9..46506d7 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -71,8 +71,14 @@ func (s *Server) SetupRoutes() { r.Post("/apps/{id}/deployments/cancel", s.handlers.HandleCancelDeploy()) r.Get("/apps/{id}/deployments", s.handlers.HandleAppDeployments()) r.Get("/apps/{id}/webhooks", s.handlers.HandleAppWebhookEvents()) - r.Get("/apps/{id}/deployments/{deploymentID}/logs", s.handlers.HandleDeploymentLogsAPI()) - r.Get("/apps/{id}/deployments/{deploymentID}/download", s.handlers.HandleDeploymentLogDownload()) + r.Get( + "/apps/{id}/deployments/{deploymentID}/logs", + s.handlers.HandleDeploymentLogsAPI(), + ) + r.Get( + "/apps/{id}/deployments/{deploymentID}/download", + s.handlers.HandleDeploymentLogDownload(), + ) r.Get("/apps/{id}/logs", s.handlers.HandleAppLogs()) r.Get("/apps/{id}/container-logs", s.handlers.HandleContainerLogsAPI()) r.Get("/apps/{id}/status", s.handlers.HandleAppStatusAPI()) diff --git a/internal/service/app/app_test.go b/internal/service/app/app_test.go index c5e788d..745fe63 100644 --- a/internal/service/app/app_test.go +++ b/internal/service/app/app_test.go @@ -16,6 +16,12 @@ import ( "sneak.berlin/go/upaas/internal/service/app" ) +// testRepoURL is the default repository URL used across tests. +const testRepoURL = "git@example.com:user/repo.git" + +// giteaRepoURL is the gitea repository URL used across tests. +const giteaRepoURL = "git@gitea.example.com:user/repo.git" + func setupTestService(t *testing.T) (*app.Service, func()) { t.Helper() @@ -58,7 +64,8 @@ func setupTestService(t *testing.T) (*app.Service, func()) { } // deleteItemTestHelper is a generic helper for testing delete operations. -// It creates an app, adds an item, verifies it exists, deletes it, and verifies it's gone. +// It creates an app, adds an item, verifies it exists, deletes it, and +// verifies it's gone. func deleteItemTestHelper( t *testing.T, appName string, @@ -73,7 +80,7 @@ func deleteItemTestHelper( createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: appName, - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -92,6 +99,35 @@ func deleteItemTestHelper( assert.Equal(t, 0, count) } +// runDeleteItemTest adapts typed list/delete callbacks so delete tests for +// different item types can share deleteItemTestHelper. +func runDeleteItemTest[T any]( + t *testing.T, + appName string, + addItem func(ctx context.Context, svc *app.Service, appID string) error, + listItems func(ctx context.Context, application *models.App) ([]T, error), + deleteFirst func(ctx context.Context, svc *app.Service, item T) error, +) { + t.Helper() + + deleteItemTestHelper(t, appName, + addItem, + func(ctx context.Context, application *models.App) (int, error) { + items, err := listItems(ctx, application) + + return len(items), err + }, + func(ctx context.Context, svc *app.Service, application *models.App) error { + items, err := listItems(ctx, application) + if err != nil { + return err + } + + return deleteFirst(ctx, svc, items[0]) + }, + ) +} + func TestCreateAppWithGeneratedKeys(t *testing.T) { t.Parallel() @@ -100,7 +136,7 @@ func TestCreateAppWithGeneratedKeys(t *testing.T) { input := app.CreateAppInput{ Name: "test-app", - RepoURL: "git@gitea.example.com:user/repo.git", + RepoURL: giteaRepoURL, Branch: "main", DockerfilePath: "Dockerfile", } @@ -110,7 +146,7 @@ func TestCreateAppWithGeneratedKeys(t *testing.T) { require.NotNil(t, createdApp) assert.Equal(t, "test-app", createdApp.Name) - assert.Equal(t, "git@gitea.example.com:user/repo.git", createdApp.RepoURL) + assert.Equal(t, giteaRepoURL, createdApp.RepoURL) assert.Equal(t, "main", createdApp.Branch) assert.Equal(t, "Dockerfile", createdApp.DockerfilePath) assert.NotEmpty(t, createdApp.ID) @@ -130,7 +166,7 @@ func TestCreateAppDefaults(t *testing.T) { input := app.CreateAppInput{ Name: "test-app-defaults", - RepoURL: "git@gitea.example.com:user/repo.git", + RepoURL: giteaRepoURL, } createdApp, err := svc.CreateApp(context.Background(), input) @@ -148,7 +184,7 @@ func TestCreateAppOptionalFields(t *testing.T) { input := app.CreateAppInput{ Name: "test-app-full", - RepoURL: "git@gitea.example.com:user/repo.git", + RepoURL: giteaRepoURL, Branch: "develop", DockerNetwork: "my-network", NtfyTopic: "https://ntfy.sh/my-topic", @@ -176,7 +212,7 @@ func TestUpdateApp(testingT *testing.T) { createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "original-name", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -208,7 +244,7 @@ func TestUpdateApp(testingT *testing.T) { createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "test-clear", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, NtfyTopic: "https://ntfy.sh/topic", SlackWebhook: "https://slack.com/hook", }) @@ -216,7 +252,7 @@ func TestUpdateApp(testingT *testing.T) { err = svc.UpdateApp(context.Background(), createdApp, app.UpdateAppInput{ Name: "test-clear", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, Branch: "main", }) require.NoError(t, err) @@ -240,7 +276,7 @@ func TestDeleteApp(testingT *testing.T) { createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "to-delete", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -264,7 +300,7 @@ func TestGetApp(testingT *testing.T) { created, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "findable-app", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -299,7 +335,7 @@ func TestGetAppByWebhookSecret(testingT *testing.T) { created, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "webhook-app", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -378,7 +414,7 @@ func TestEnvVarsAddAndRetrieve(t *testing.T) { createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "env-test", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -411,29 +447,33 @@ func TestEnvVarsAddAndRetrieve(t *testing.T) { assert.Equal(t, "secret123", keys["API_KEY"]) } +// addDeletableEnvVar seeds the env var removed in the delete test. +func addDeletableEnvVar( + ctx context.Context, svc *app.Service, appID string, +) error { + return svc.AddEnvVar(ctx, appID, "TO_DELETE", "value") +} + func TestEnvVarsDelete(t *testing.T) { t.Parallel() - deleteItemTestHelper(t, "env-delete-test", - func(ctx context.Context, svc *app.Service, appID string) error { - return svc.AddEnvVar(ctx, appID, "TO_DELETE", "value") + runDeleteItemTest(t, "env-delete-test", addDeletableEnvVar, + func(ctx context.Context, application *models.App) ([]*models.EnvVar, error) { + return application.GetEnvVars(ctx) }, - func(ctx context.Context, application *models.App) (int, error) { - envVars, err := application.GetEnvVars(ctx) - - return len(envVars), err - }, - func(ctx context.Context, svc *app.Service, application *models.App) error { - envVars, err := application.GetEnvVars(ctx) - if err != nil { - return err - } - - return svc.DeleteEnvVar(ctx, envVars[0].ID) + func(ctx context.Context, svc *app.Service, item *models.EnvVar) error { + return svc.DeleteEnvVar(ctx, item.ID) }, ) } +// addDeletableLabel seeds the label removed in the delete test. +func addDeletableLabel( + ctx context.Context, svc *app.Service, appID string, +) error { + return svc.AddLabel(ctx, appID, "to.delete", "value") +} + func TestLabels(testingT *testing.T) { testingT.Parallel() @@ -445,7 +485,7 @@ func TestLabels(testingT *testing.T) { createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "label-test", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -468,22 +508,12 @@ func TestLabels(testingT *testing.T) { testingT.Run("deletes label", func(t *testing.T) { t.Parallel() - deleteItemTestHelper(t, "label-delete-test", - func(ctx context.Context, svc *app.Service, appID string) error { - return svc.AddLabel(ctx, appID, "to.delete", "value") + runDeleteItemTest(t, "label-delete-test", addDeletableLabel, + func(ctx context.Context, application *models.App) ([]*models.Label, error) { + return application.GetLabels(ctx) }, - func(ctx context.Context, application *models.App) (int, error) { - labels, err := application.GetLabels(ctx) - - return len(labels), err - }, - func(ctx context.Context, svc *app.Service, application *models.App) error { - labels, err := application.GetLabels(ctx) - if err != nil { - return err - } - - return svc.DeleteLabel(ctx, labels[0].ID) + func(ctx context.Context, svc *app.Service, item *models.Label) error { + return svc.DeleteLabel(ctx, item.ID) }, ) }) @@ -497,7 +527,7 @@ func TestVolumesAddAndRetrieve(t *testing.T) { createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "volume-test", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -547,7 +577,7 @@ func TestVolumesDelete(t *testing.T) { createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "volume-delete-test", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) @@ -583,7 +613,7 @@ func TestUpdateAppStatus(testingT *testing.T) { createdApp, err := svc.CreateApp(context.Background(), app.CreateAppInput{ Name: "status-test", - RepoURL: "git@example.com:user/repo.git", + RepoURL: testRepoURL, }) require.NoError(t, err) assert.Equal(t, models.AppStatusPending, createdApp.Status) diff --git a/internal/service/auth/auth_test.go b/internal/service/auth/auth_test.go index f399585..3e0c8c2 100644 --- a/internal/service/auth/auth_test.go +++ b/internal/service/auth/auth_test.go @@ -121,7 +121,7 @@ func getSessionCookie(t *testing.T, svc *auth.Service) *http.Cookie { require.NoError(t, err) recorder := httptest.NewRecorder() - request := httptest.NewRequest(http.MethodGet, "/", nil) + request := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil) err = svc.CreateSession(recorder, request, user) require.NoError(t, err) @@ -144,7 +144,11 @@ func TestSessionCookieSecureFlag(testingT *testing.T) { svc := setupAuthService(t, false) cookie := getSessionCookie(t, svc) require.NotNil(t, cookie, "session cookie should exist") - assert.True(t, cookie.Secure, "session cookie should have Secure flag in production mode") + assert.True( + t, + cookie.Secure, + "session cookie should have Secure flag in production mode", + ) }) } @@ -324,7 +328,12 @@ func TestCreateUserRaceCondition(testingT *testing.T) { } assert.Equal(t, 1, successes, "exactly one goroutine should succeed") - assert.Equal(t, goroutines-1, failures, "all other goroutines should fail with ErrUserExists") + assert.Equal( + t, + goroutines-1, + failures, + "all other goroutines should fail with ErrUserExists", + ) }) } @@ -380,7 +389,9 @@ func TestDestroySessionMaxAge(testingT *testing.T) { defer cleanup() recorder := httptest.NewRecorder() - request := httptest.NewRequest(http.MethodGet, "/", nil) + request := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil, + ) err := svc.DestroySession(recorder, request) require.NoError(t, err) diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 2bc983c..887a094 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -66,7 +66,8 @@ const logFilePermissions = 0o640 // logTimestampFormat is the format for log file timestamps. const logTimestampFormat = "20060102T150405Z" -// logFileShortSHALength is the number of characters to use for commit SHA in log filenames. +// logFileShortSHALength is the number of characters to use for commit SHA +// in log filenames. const logFileShortSHALength = 12 // dockerLogMessage represents a Docker build log message. @@ -87,7 +88,10 @@ type deploymentLogWriter struct { flushCtx context.Context //nolint:containedctx // needed for async flush goroutine } -func newDeploymentLogWriter(ctx context.Context, deployment *models.Deployment) *deploymentLogWriter { +func newDeploymentLogWriter( + ctx context.Context, + deployment *models.Deployment, +) *deploymentLogWriter { w := &deploymentLogWriter{ deployment: deployment, done: make(chan struct{}), @@ -257,7 +261,10 @@ func (svc *Service) GetBuildDir(appName string) string { // GetLogFilePath returns the path to the log file for a deployment. // Returns empty string if the path cannot be determined. -func (svc *Service) GetLogFilePath(app *models.App, deployment *models.Deployment) string { +func (svc *Service) GetLogFilePath( + app *models.App, + deployment *models.Deployment, +) string { hostname, err := os.Hostname() if err != nil { hostname = "unknown" @@ -275,7 +282,8 @@ func (svc *Service) GetLogFilePath(app *models.App, deployment *models.Deploymen // Use started_at timestamp timestamp := deployment.StartedAt.UTC().Format(logTimestampFormat) - // Build filename: appname_sha_timestamp.log.txt (or appname_timestamp.log.txt if no SHA) + // Build filename: appname_sha_timestamp.log.txt + // (or appname_timestamp.log.txt if no SHA) var filename string if sha != "" { filename = fmt.Sprintf("%s_%s_%s.log.txt", app.Name, sha, timestamp) @@ -308,7 +316,8 @@ func (svc *Service) CancelDeploy(appID string) bool { // Deploy deploys an app. If cancelExisting is true (e.g. webhook-triggered), // any in-progress deploy for the same app will be cancelled before starting. -// If cancelExisting is false and a deploy is in progress, ErrDeploymentInProgress is returned. +// If cancelExisting is false and a deploy is in progress, +// ErrDeploymentInProgress is returned. func (svc *Service) Deploy( ctx context.Context, app *models.App, @@ -342,7 +351,8 @@ func (svc *Service) Deploy( // Fetch webhook event and create deployment record webhookEvent := svc.fetchWebhookEvent(deployCtx, webhookEventID) - // Use a background context for DB operations that must complete regardless of cancellation + // Use a background context for DB operations that must complete + // regardless of cancellation bgCtx := context.WithoutCancel(deployCtx) deployment, err := svc.createDeploymentRecord(bgCtx, app, webhookEventID, webhookEvent) @@ -401,7 +411,10 @@ func (svc *Service) createRollbackDeployment( return nil, fmt.Errorf("failed to create rollback deployment: %w", saveErr) } - _ = deployment.AppendLog(ctx, "Rolling back to previous image: "+app.PreviousImageID.String) + _ = deployment.AppendLog( + ctx, + "Rolling back to previous image: "+app.PreviousImageID.String, + ) return deployment, nil } @@ -417,7 +430,11 @@ func (svc *Service) executeRollback( svc.removeOldContainer(ctx, app, deployment) - rollbackOpts, err := svc.buildContainerOptions(ctx, app, docker.ImageID(previousImageID)) + rollbackOpts, err := svc.buildContainerOptions( + ctx, + app, + docker.ImageID(previousImageID), + ) if err != nil { svc.failDeployment(bgCtx, app, deployment, err) @@ -426,7 +443,12 @@ func (svc *Service) executeRollback( containerID, err := svc.docker.CreateContainer(ctx, rollbackOpts) if err != nil { - svc.failDeployment(bgCtx, app, deployment, fmt.Errorf("failed to create rollback container: %w", err)) + svc.failDeployment( + bgCtx, + app, + deployment, + fmt.Errorf("failed to create rollback container: %w", err), + ) return fmt.Errorf("failed to create rollback container: %w", err) } @@ -436,7 +458,12 @@ func (svc *Service) executeRollback( startErr := svc.docker.StartContainer(ctx, containerID) if startErr != nil { - svc.failDeployment(bgCtx, app, deployment, fmt.Errorf("failed to start rollback container: %w", startErr)) + svc.failDeployment( + bgCtx, + app, + deployment, + fmt.Errorf("failed to start rollback container: %w", startErr), + ) return fmt.Errorf("failed to start rollback container: %w", startErr) } @@ -695,7 +722,11 @@ func (svc *Service) cleanupCancelledDeploy( if removeErr != nil { svc.log.Error("failed to remove image from cancelled deploy", "error", removeErr, "app", app.Name, "image", imageID) - _ = deployment.AppendLog(ctx, "WARNING: failed to clean up image "+imageID.String()+": "+removeErr.Error()) + _ = deployment.AppendLog( + ctx, + "WARNING: failed to clean up image "+ + imageID.String()+": "+removeErr.Error(), + ) } else { svc.log.Info("cleaned up image from cancelled deploy", "app", app.Name, "image", imageID) @@ -870,14 +901,24 @@ func (svc *Service) cloneRepository( err := os.MkdirAll(appBuildsDir, buildsDirPermissions) if err != nil { - svc.failDeployment(ctx, app, deployment, fmt.Errorf("failed to create builds dir: %w", err)) + svc.failDeployment( + ctx, + app, + deployment, + fmt.Errorf("failed to create builds dir: %w", err), + ) return "", nil, fmt.Errorf("failed to create builds dir: %w", err) } buildDir, err := os.MkdirTemp(appBuildsDir, fmt.Sprintf("%d-*", deployment.ID)) if err != nil { - svc.failDeployment(ctx, app, deployment, fmt.Errorf("failed to create temp dir: %w", err)) + svc.failDeployment( + ctx, + app, + deployment, + fmt.Errorf("failed to create temp dir: %w", err), + ) return "", nil, fmt.Errorf("failed to create temp dir: %w", err) } @@ -908,7 +949,12 @@ func (svc *Service) cloneRepository( ) if cloneErr != nil { cleanup() - svc.failDeployment(ctx, app, deployment, fmt.Errorf("failed to clone repo: %w", cloneErr)) + svc.failDeployment( + ctx, + app, + deployment, + fmt.Errorf("failed to clone repo: %w", cloneErr), + ) return "", nil, fmt.Errorf("failed to clone repo: %w", cloneErr) } diff --git a/internal/service/deploy/deploy_cleanup_test.go b/internal/service/deploy/deploy_cleanup_test.go index ece6b86..42f7954 100644 --- a/internal/service/deploy/deploy_cleanup_test.go +++ b/internal/service/deploy/deploy_cleanup_test.go @@ -32,7 +32,10 @@ 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"), 0o600)) + 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/deploy_container_test.go b/internal/service/deploy/deploy_container_test.go index 1bc4c77..4519352 100644 --- a/internal/service/deploy/deploy_container_test.go +++ b/internal/service/deploy/deploy_container_test.go @@ -31,7 +31,9 @@ func TestBuildContainerOptionsUsesImageID(t *testing.T) { const expectedImageID = docker.ImageID("sha256:abc123def456") - opts, err := svc.BuildContainerOptionsExported(context.Background(), app, expectedImageID) + opts, err := svc.BuildContainerOptionsExported( + context.Background(), app, expectedImageID, + ) if err != nil { t.Fatalf("buildContainerOptions returned error: %v", err) } @@ -77,14 +79,20 @@ func TestBuildContainerOptionsNoResourceLimits(t *testing.T) { } } -func TestBuildContainerOptionsCPULimit(t *testing.T) { - t.Parallel() +// buildOptsForApp saves an app configured by setup and returns the container +// options built for it. +func buildOptsForApp( + t *testing.T, + name string, + setup func(app *models.App), +) docker.CreateContainerOptions { + t.Helper() db := database.NewTestDatabase(t) app := models.NewApp(db) - app.Name = "cpulimit" - app.CPULimit = sql.NullFloat64{Float64: 0.5, Valid: true} + app.Name = name + setup(app) err := app.Save(context.Background()) if err != nil { @@ -101,6 +109,16 @@ func TestBuildContainerOptionsCPULimit(t *testing.T) { t.Fatalf("buildContainerOptions returned error: %v", err) } + return opts +} + +func TestBuildContainerOptionsCPULimit(t *testing.T) { + t.Parallel() + + opts := buildOptsForApp(t, "cpulimit", func(app *models.App) { + app.CPULimit = sql.NullFloat64{Float64: 0.5, Valid: true} + }) + if opts.CPULimit != 0.5 { t.Errorf("expected CPULimit=0.5, got %v", opts.CPULimit) } @@ -109,26 +127,9 @@ func TestBuildContainerOptionsCPULimit(t *testing.T) { func TestBuildContainerOptionsMemoryLimit(t *testing.T) { t.Parallel() - db := database.NewTestDatabase(t) - - app := models.NewApp(db) - app.Name = "memlimit" - app.MemoryLimit = sql.NullInt64{Int64: 536870912, Valid: true} // 512m - - err := app.Save(context.Background()) - if err != nil { - t.Fatalf("failed to save app: %v", err) - } - - log := slog.New(slog.NewTextHandler(os.Stderr, nil)) - svc := deploy.NewTestService(log) - - opts, err := svc.BuildContainerOptionsExported( - context.Background(), app, docker.ImageID("test:latest"), - ) - if err != nil { - t.Fatalf("buildContainerOptions returned error: %v", err) - } + opts := buildOptsForApp(t, "memlimit", func(app *models.App) { + app.MemoryLimit = sql.NullInt64{Int64: 536870912, Valid: true} // 512m + }) if opts.MemoryLimit != 536870912 { t.Errorf("expected MemoryLimit=536870912, got %v", opts.MemoryLimit) diff --git a/internal/service/deploy/export_test.go b/internal/service/deploy/export_test.go index 261d085..1d86a25 100644 --- a/internal/service/deploy/export_test.go +++ b/internal/service/deploy/export_test.go @@ -26,7 +26,11 @@ func (svc *Service) CancelActiveDeploy(appID string) { } // RegisterActiveDeploy registers an active deploy for testing. -func (svc *Service) RegisterActiveDeploy(appID string, cancel context.CancelFunc, done chan struct{}) { +func (svc *Service) RegisterActiveDeploy( + appID string, + cancel context.CancelFunc, + done chan struct{}, +) { svc.activeDeploys.Store(appID, &activeDeploy{cancel: cancel, done: done}) } @@ -41,7 +45,11 @@ func (svc *Service) UnlockApp(appID string) { } // NewTestServiceWithConfig creates a Service with config and docker client for testing. -func NewTestServiceWithConfig(log *slog.Logger, cfg *config.Config, dockerClient *docker.Client) *Service { +func NewTestServiceWithConfig( + log *slog.Logger, + cfg *config.Config, + dockerClient *docker.Client, +) *Service { return &Service{ log: log, config: cfg, diff --git a/internal/service/notify/notify.go b/internal/service/notify/notify.go index fba2b3a..279248f 100644 --- a/internal/service/notify/notify.go +++ b/internal/service/notify/notify.go @@ -159,7 +159,8 @@ func (svc *Service) NotifyDeployFailed( ) { duration := time.Since(deployment.StartedAt) title := "Deploy failed: " + app.Name - message := "Deployment failed after " + formatDuration(duration) + ": " + deployErr.Error() + message := "Deployment failed after " + formatDuration(duration) + + ": " + deployErr.Error() svc.sendNotifications(ctx, app, title, message, message, "error") } @@ -266,7 +267,8 @@ func (svc *Service) sendNtfy( request.Header.Set("Title", title) request.Header.Set("Priority", svc.ntfyPriority(priority)) - resp, err := svc.client.Do(request) // #nosec G704 -- URL from validated config, not user input + // #nosec G704 -- URL from validated config, not user input + resp, err := svc.client.Do(request) if err != nil { return fmt.Errorf("failed to send ntfy request: %w", err) } @@ -363,7 +365,8 @@ func (svc *Service) sendSlack( request.Header.Set("Content-Type", "application/json") - resp, err := svc.client.Do(request) // #nosec G704 -- URL from validated config, not user input + // #nosec G704 -- URL from validated config, not user input + resp, err := svc.client.Do(request) if err != nil { return fmt.Errorf("failed to send slack request: %w", err) } diff --git a/internal/service/webhook/payloads.go b/internal/service/webhook/payloads.go index 1b9d1c1..5763eb5 100644 --- a/internal/service/webhook/payloads.go +++ b/internal/service/webhook/payloads.go @@ -98,88 +98,77 @@ type GitLabPushPayload struct { func ParsePushPayload(source Source, payload []byte) (*PushEvent, error) { switch source { case SourceGitHub: - return parseGitHubPush(payload) + return parsePush(payload, githubPushEvent) case SourceGitLab: - return parseGitLabPush(payload) + return parsePush(payload, gitlabPushEvent) case SourceGitea, SourceUnknown: // Gitea and unknown both use Gitea format for backward compatibility. - return parseGiteaPush(payload) + return parsePush(payload, giteaPushEvent) } // Unreachable for known source values, but satisfies exhaustive checker. - return parseGiteaPush(payload) + return parsePush(payload, giteaPushEvent) } -func parseGiteaPush(payload []byte) (*PushEvent, error) { - var p GiteaPushPayload +// parsePush unmarshals payload into P and converts it into a normalized +// PushEvent via build. +func parsePush[P any](payload []byte, build func(P) *PushEvent) (*PushEvent, error) { + var p P unmarshalErr := json.Unmarshal(payload, &p) if unmarshalErr != nil { return nil, unmarshalErr } - commitURL := extractGiteaCommitURL(p) - - return &PushEvent{ - Source: SourceGitea, - Ref: p.Ref, - Before: p.Before, - After: p.After, - Branch: extractBranch(p.Ref), - RepoName: p.Repository.FullName, - CloneURL: p.Repository.CloneURL, - HTMLURL: p.Repository.HTMLURL, - CommitURL: commitURL, - Pusher: p.Pusher.Username, - }, nil + return build(p), nil } -func parseGitHubPush(payload []byte) (*PushEvent, error) { - var p GitHubPushPayload - - unmarshalErr := json.Unmarshal(payload, &p) - if unmarshalErr != nil { - return nil, unmarshalErr - } - - commitURL := extractGitHubCommitURL(p) - +// basePushEvent builds a PushEvent populated with the fields shared by all +// webhook sources. +func basePushEvent(source Source, ref, before, after string) *PushEvent { return &PushEvent{ - Source: SourceGitHub, - Ref: p.Ref, - Before: p.Before, - After: p.After, - Branch: extractBranch(p.Ref), - RepoName: p.Repository.FullName, - CloneURL: p.Repository.CloneURL, - HTMLURL: p.Repository.HTMLURL, - CommitURL: commitURL, - Pusher: p.Pusher.Name, - }, nil + Source: source, + Ref: ref, + Before: before, + After: after, + Branch: extractBranch(ref), + } } -func parseGitLabPush(payload []byte) (*PushEvent, error) { - var p GitLabPushPayload +// giteaPushEvent converts a Gitea push payload to a normalized PushEvent. +func giteaPushEvent(p GiteaPushPayload) *PushEvent { + event := basePushEvent(SourceGitea, p.Ref, p.Before, p.After) + event.RepoName = p.Repository.FullName + event.CloneURL = p.Repository.CloneURL + event.HTMLURL = p.Repository.HTMLURL + event.CommitURL = extractGiteaCommitURL(p) + event.Pusher = p.Pusher.Username - unmarshalErr := json.Unmarshal(payload, &p) - if unmarshalErr != nil { - return nil, unmarshalErr - } + return event +} - commitURL := extractGitLabCommitURL(p) +// gitlabPushEvent converts a GitLab push payload to a normalized PushEvent. +func gitlabPushEvent(p GitLabPushPayload) *PushEvent { + event := basePushEvent(SourceGitLab, p.Ref, p.Before, p.After) + event.RepoName = p.Project.PathWithNamespace + event.CloneURL = p.Project.GitHTTPURL + event.HTMLURL = p.Project.WebURL + event.CommitURL = extractGitLabCommitURL(p) + event.Pusher = p.UserName - return &PushEvent{ - Source: SourceGitLab, - Ref: p.Ref, - Before: p.Before, - After: p.After, - Branch: extractBranch(p.Ref), - RepoName: p.Project.PathWithNamespace, - CloneURL: p.Project.GitHTTPURL, - HTMLURL: p.Project.WebURL, - CommitURL: commitURL, - Pusher: p.UserName, - }, nil + return event +} + +// githubPushEvent converts a GitHub push payload to a normalized PushEvent. +func githubPushEvent(p GitHubPushPayload) *PushEvent { + event := basePushEvent(SourceGitHub, p.Ref, p.Before, p.After) + event.RepoName = p.Repository.FullName + event.CloneURL = p.Repository.CloneURL + event.HTMLURL = p.Repository.HTMLURL + event.CommitURL = extractGitHubCommitURL(p) + event.Pusher = p.Pusher.Name + + return event } // extractBranch extracts the branch name from a git ref. diff --git a/internal/service/webhook/webhook_test.go b/internal/service/webhook/webhook_test.go index 7cf4d5c..bda7a98 100644 --- a/internal/service/webhook/webhook_test.go +++ b/internal/service/webhook/webhook_test.go @@ -24,6 +24,18 @@ import ( "sneak.berlin/go/upaas/internal/service/webhook" ) +const ( + giteaEventHeader = "X-Gitea-Event" + githubEventHeader = "X-GitHub-Event" + gitlabEventHeader = "X-Gitlab-Event" + gitlabPushHook = "Push Hook" + pushEventType = "push" + branchMain = "main" + refMain = "refs/heads/main" + testCommitSHA = "abc123def456789" + testPusher = "developer" +) + type testDeps struct { logger *logger.Logger config *config.Config @@ -45,9 +57,14 @@ func setupTestDeps(t *testing.T) *testDeps { loggerInst, err := logger.New(fx.Lifecycle(nil), logger.Params{Globals: globalsInst}) require.NoError(t, err) - cfg := &config.Config{Port: 8080, DataDir: tmpDir, SessionSecret: "test-secret-key-at-least-32-chars"} + cfg := &config.Config{ + Port: 8080, DataDir: tmpDir, + SessionSecret: "test-secret-key-at-least-32-chars", + } - dbInst, err := database.New(fx.Lifecycle(nil), database.Params{Logger: loggerInst, Config: cfg}) + dbInst, err := database.New( + fx.Lifecycle(nil), database.Params{Logger: loggerInst, Config: cfg}, + ) require.NoError(t, err) return &testDeps{logger: loggerInst, config: cfg, db: dbInst, tmpDir: tmpDir} @@ -58,14 +75,19 @@ func setupTestService(t *testing.T) (*webhook.Service, *database.Database, func( deps := setupTestDeps(t) - dockerClient, err := docker.New(fx.Lifecycle(nil), docker.Params{Logger: deps.logger, Config: deps.config}) + dockerClient, err := docker.New( + fx.Lifecycle(nil), docker.Params{Logger: deps.logger, Config: deps.config}, + ) require.NoError(t, err) - notifySvc, err := notify.New(fx.Lifecycle(nil), notify.ServiceParams{Logger: deps.logger}) + notifySvc, err := notify.New( + fx.Lifecycle(nil), notify.ServiceParams{Logger: deps.logger}, + ) require.NoError(t, err) deploySvc, err := deploy.New(fx.Lifecycle(nil), deploy.ServiceParams{ - Logger: deps.logger, Config: deps.config, Database: deps.db, Docker: dockerClient, Notify: notifySvc, + Logger: deps.logger, Config: deps.config, Database: deps.db, + Docker: dockerClient, Notify: notifySvc, }) require.NoError(t, err) @@ -104,8 +126,6 @@ func createTestApp( } // TestDetectWebhookSource tests auto-detection of webhook source from HTTP headers. -// -//nolint:funlen // table-driven test with comprehensive test cases func TestDetectWebhookSource(testingT *testing.T) { testingT.Parallel() @@ -116,17 +136,17 @@ func TestDetectWebhookSource(testingT *testing.T) { }{ { name: "detects Gitea from X-Gitea-Event header", - headers: map[string]string{"X-Gitea-Event": "push"}, + headers: map[string]string{giteaEventHeader: pushEventType}, expected: webhook.SourceGitea, }, { name: "detects GitHub from X-GitHub-Event header", - headers: map[string]string{"X-GitHub-Event": "push"}, + headers: map[string]string{githubEventHeader: pushEventType}, expected: webhook.SourceGitHub, }, { name: "detects GitLab from X-Gitlab-Event header", - headers: map[string]string{"X-Gitlab-Event": "Push Hook"}, + headers: map[string]string{gitlabEventHeader: gitlabPushHook}, expected: webhook.SourceGitLab, }, { @@ -142,16 +162,16 @@ func TestDetectWebhookSource(testingT *testing.T) { { name: "Gitea takes precedence over GitHub", headers: map[string]string{ - "X-Gitea-Event": "push", - "X-GitHub-Event": "push", + giteaEventHeader: pushEventType, + githubEventHeader: pushEventType, }, expected: webhook.SourceGitea, }, { name: "GitHub takes precedence over GitLab", headers: map[string]string{ - "X-GitHub-Event": "push", - "X-Gitlab-Event": "Push Hook", + githubEventHeader: pushEventType, + gitlabEventHeader: gitlabPushHook, }, expected: webhook.SourceGitHub, }, @@ -184,33 +204,33 @@ func TestDetectEventType(testingT *testing.T) { }{ { name: "extracts Gitea event type", - headers: map[string]string{"X-Gitea-Event": "push"}, + headers: map[string]string{giteaEventHeader: pushEventType}, source: webhook.SourceGitea, - expected: "push", + expected: pushEventType, }, { name: "extracts GitHub event type", - headers: map[string]string{"X-GitHub-Event": "push"}, + headers: map[string]string{githubEventHeader: pushEventType}, source: webhook.SourceGitHub, - expected: "push", + expected: pushEventType, }, { name: "extracts GitLab event type", - headers: map[string]string{"X-Gitlab-Event": "Push Hook"}, + headers: map[string]string{gitlabEventHeader: gitlabPushHook}, source: webhook.SourceGitLab, - expected: "Push Hook", + expected: gitlabPushHook, }, { name: "returns push for unknown source", headers: map[string]string{}, source: webhook.SourceUnknown, - expected: "push", + expected: pushEventType, }, { name: "returns push when header missing for source", headers: map[string]string{}, source: webhook.SourceGitea, - expected: "push", + expected: pushEventType, }, } @@ -250,11 +270,54 @@ func TestUnparsedURLString(t *testing.T) { assert.Empty(t, empty.String()) } -// TestParsePushPayloadGitea tests parsing of Gitea push payloads. -func TestParsePushPayloadGitea(t *testing.T) { - t.Parallel() +// pushEventExpectation describes the expected normalized fields of a parsed +// push payload. +type pushEventExpectation struct { + source webhook.Source + ref string + branch string + after string + repoName string + cloneURL webhook.UnparsedURL + htmlURL webhook.UnparsedURL + commitURL webhook.UnparsedURL + pusher string +} - payload := []byte(`{ +// assertPushEvent parses payload for want.source and asserts every +// normalized PushEvent field matches want. +func assertPushEvent(t *testing.T, payload []byte, want pushEventExpectation) { + t.Helper() + + event, err := webhook.ParsePushPayload(want.source, payload) + require.NoError(t, err) + + assert.Equal(t, want.source, event.Source) + assert.Equal(t, want.ref, event.Ref) + assert.Equal(t, want.branch, event.Branch) + assert.Equal(t, want.after, event.After) + + assertPushEventOrigin(t, event, want) +} + +// assertPushEventOrigin asserts the repository and pusher fields of event. +func assertPushEventOrigin( + t *testing.T, + event *webhook.PushEvent, + want pushEventExpectation, +) { + t.Helper() + + assert.Equal(t, want.repoName, event.RepoName) + assert.Equal(t, want.cloneURL, event.CloneURL) + assert.Equal(t, want.htmlURL, event.HTMLURL) + assert.Equal(t, want.commitURL, event.CommitURL) + assert.Equal(t, want.pusher, event.Pusher) +} + +// giteaPushJSON returns a realistic Gitea push webhook payload. +func giteaPushJSON() []byte { + return []byte(`{ "ref": "refs/heads/main", "before": "0000000000000000000000000000000000000000", "after": "abc123def456789", @@ -275,29 +338,11 @@ func TestParsePushPayloadGitea(t *testing.T) { } ] }`) - - event, err := webhook.ParsePushPayload(webhook.SourceGitea, payload) - require.NoError(t, err) - - assert.Equal(t, webhook.SourceGitea, event.Source) - assert.Equal(t, "refs/heads/main", event.Ref) - assert.Equal(t, "main", event.Branch) - assert.Equal(t, "abc123def456789", event.After) - assert.Equal(t, "myorg/myrepo", event.RepoName) - assert.Equal(t, webhook.UnparsedURL("https://gitea.example.com/myorg/myrepo.git"), event.CloneURL) - assert.Equal(t, webhook.UnparsedURL("https://gitea.example.com/myorg/myrepo"), event.HTMLURL) - assert.Equal(t, - webhook.UnparsedURL("https://gitea.example.com/myorg/myrepo/commit/abc123def456789"), - event.CommitURL, - ) - assert.Equal(t, "developer", event.Pusher) } -// TestParsePushPayloadGitHub tests parsing of GitHub push payloads. -func TestParsePushPayloadGitHub(t *testing.T) { - t.Parallel() - - payload := []byte(`{ +// githubPushJSON returns a realistic GitHub push webhook payload. +func githubPushJSON() []byte { + return []byte(`{ "ref": "refs/heads/main", "before": "0000000000000000000000000000000000000000", "after": "abc123def456789", @@ -323,29 +368,11 @@ func TestParsePushPayloadGitHub(t *testing.T) { } ] }`) - - event, err := webhook.ParsePushPayload(webhook.SourceGitHub, payload) - require.NoError(t, err) - - assert.Equal(t, webhook.SourceGitHub, event.Source) - assert.Equal(t, "refs/heads/main", event.Ref) - assert.Equal(t, "main", event.Branch) - assert.Equal(t, "abc123def456789", event.After) - assert.Equal(t, "myorg/myrepo", event.RepoName) - assert.Equal(t, webhook.UnparsedURL("https://github.com/myorg/myrepo.git"), event.CloneURL) - assert.Equal(t, webhook.UnparsedURL("https://github.com/myorg/myrepo"), event.HTMLURL) - assert.Equal(t, - webhook.UnparsedURL("https://github.com/myorg/myrepo/commit/abc123def456789"), - event.CommitURL, - ) - assert.Equal(t, "developer", event.Pusher) } -// TestParsePushPayloadGitLab tests parsing of GitLab push payloads. -func TestParsePushPayloadGitLab(t *testing.T) { - t.Parallel() - - payload := []byte(`{ +// gitlabPushJSON returns a realistic GitLab push webhook payload. +func gitlabPushJSON() []byte { + return []byte(`{ "ref": "refs/heads/develop", "before": "0000000000000000000000000000000000000000", "after": "abc123def456789", @@ -366,25 +393,78 @@ func TestParsePushPayloadGitLab(t *testing.T) { } ] }`) - - event, err := webhook.ParsePushPayload(webhook.SourceGitLab, payload) - require.NoError(t, err) - - assert.Equal(t, webhook.SourceGitLab, event.Source) - assert.Equal(t, "refs/heads/develop", event.Ref) - assert.Equal(t, "develop", event.Branch) - assert.Equal(t, "abc123def456789", event.After) - assert.Equal(t, "mygroup/myproject", event.RepoName) - assert.Equal(t, webhook.UnparsedURL("https://gitlab.com/mygroup/myproject.git"), event.CloneURL) - assert.Equal(t, webhook.UnparsedURL("https://gitlab.com/mygroup/myproject"), event.HTMLURL) - assert.Equal(t, - webhook.UnparsedURL("https://gitlab.com/mygroup/myproject/-/commit/abc123def456789"), - event.CommitURL, - ) - assert.Equal(t, "developer", event.Pusher) } -// TestParsePushPayloadUnknownFallsBackToGitea tests that unknown source uses Gitea parser. +// pushPayloadJSON returns the push payload fixture for source. +func pushPayloadJSON(t *testing.T, source webhook.Source) []byte { + t.Helper() + + switch source { + case webhook.SourceGitHub: + return githubPushJSON() + case webhook.SourceGitLab: + return gitlabPushJSON() + case webhook.SourceGitea, webhook.SourceUnknown: + return giteaPushJSON() + } + + t.Fatalf("no push payload fixture for source %v", source) + + return nil +} + +// TestParsePushPayload tests parsing of Gitea, GitHub, and GitLab push +// payloads into normalized PushEvents. +func TestParsePushPayload(testingT *testing.T) { + testingT.Parallel() + + tests := []pushEventExpectation{ + { + source: webhook.SourceGitea, + ref: refMain, + branch: branchMain, + after: testCommitSHA, + repoName: "myorg/myrepo", + cloneURL: "https://gitea.example.com/myorg/myrepo.git", + htmlURL: "https://gitea.example.com/myorg/myrepo", + commitURL: "https://gitea.example.com/myorg/myrepo/commit/abc123def456789", + pusher: testPusher, + }, + { + source: webhook.SourceGitHub, + ref: refMain, + branch: branchMain, + after: testCommitSHA, + repoName: "myorg/myrepo", + cloneURL: "https://github.com/myorg/myrepo.git", + htmlURL: "https://github.com/myorg/myrepo", + commitURL: "https://github.com/myorg/myrepo/commit/abc123def456789", + pusher: testPusher, + }, + { + source: webhook.SourceGitLab, + ref: "refs/heads/develop", + branch: "develop", + after: testCommitSHA, + repoName: "mygroup/myproject", + cloneURL: "https://gitlab.com/mygroup/myproject.git", + htmlURL: "https://gitlab.com/mygroup/myproject", + commitURL: "https://gitlab.com/mygroup/myproject/-/commit/abc123def456789", + pusher: testPusher, + }, + } + + for _, testCase := range tests { + testingT.Run(testCase.source.String(), func(t *testing.T) { + t.Parallel() + + assertPushEvent(t, pushPayloadJSON(t, testCase.source), testCase) + }) + } +} + +// TestParsePushPayloadUnknownFallsBackToGitea tests that unknown source +// uses the Gitea parser. func TestParsePushPayloadUnknownFallsBackToGitea(t *testing.T) { t.Parallel() @@ -399,7 +479,7 @@ func TestParsePushPayloadUnknownFallsBackToGitea(t *testing.T) { require.NoError(t, err) assert.Equal(t, webhook.SourceGitea, event.Source) - assert.Equal(t, "main", event.Branch) + assert.Equal(t, branchMain, event.Branch) assert.Equal(t, "abc123", event.After) } @@ -462,7 +542,10 @@ func TestGitHubCommitURLFallback(t *testing.T) { event, err := webhook.ParsePushPayload(webhook.SourceGitHub, payload) require.NoError(t, err) - assert.Equal(t, webhook.UnparsedURL("https://github.com/u/r/commit/abc123"), event.CommitURL) + assert.Equal(t, + webhook.UnparsedURL("https://github.com/u/r/commit/abc123"), + event.CommitURL, + ) }) t.Run("falls back to commits list", func(t *testing.T) { @@ -477,7 +560,10 @@ func TestGitHubCommitURLFallback(t *testing.T) { event, err := webhook.ParsePushPayload(webhook.SourceGitHub, payload) require.NoError(t, err) - assert.Equal(t, webhook.UnparsedURL("https://github.com/u/r/commit/abc123"), event.CommitURL) + assert.Equal(t, + webhook.UnparsedURL("https://github.com/u/r/commit/abc123"), + event.CommitURL, + ) }) t.Run("constructs URL from repo HTML URL", func(t *testing.T) { @@ -491,7 +577,10 @@ func TestGitHubCommitURLFallback(t *testing.T) { event, err := webhook.ParsePushPayload(webhook.SourceGitHub, payload) require.NoError(t, err) - assert.Equal(t, webhook.UnparsedURL("https://github.com/u/r/commit/abc123"), event.CommitURL) + assert.Equal(t, + webhook.UnparsedURL("https://github.com/u/r/commit/abc123"), + event.CommitURL, + ) }) } @@ -511,7 +600,10 @@ func TestGitLabCommitURLFallback(t *testing.T) { event, err := webhook.ParsePushPayload(webhook.SourceGitLab, payload) require.NoError(t, err) - assert.Equal(t, webhook.UnparsedURL("https://gitlab.com/g/p/-/commit/abc123"), event.CommitURL) + assert.Equal(t, + webhook.UnparsedURL("https://gitlab.com/g/p/-/commit/abc123"), + event.CommitURL, + ) }) t.Run("constructs URL from project web URL", func(t *testing.T) { @@ -525,7 +617,10 @@ func TestGitLabCommitURLFallback(t *testing.T) { event, err := webhook.ParsePushPayload(webhook.SourceGitLab, payload) require.NoError(t, err) - assert.Equal(t, webhook.UnparsedURL("https://gitlab.com/g/p/-/commit/abc123"), event.CommitURL) + assert.Equal(t, + webhook.UnparsedURL("https://gitlab.com/g/p/-/commit/abc123"), + event.CommitURL, + ) }) } @@ -588,7 +683,8 @@ func TestGiteaPushPayloadParsing(testingT *testing.T) { }) } -// TestGitHubPushPayloadParsing tests direct deserialization of the GitHub payload struct. +// TestGitHubPushPayloadParsing tests deserialization of the GitHub payload +// struct. func TestGitHubPushPayloadParsing(t *testing.T) { t.Parallel() @@ -633,7 +729,8 @@ func TestGitHubPushPayloadParsing(t *testing.T) { assert.Len(t, p.Commits, 1) } -// TestGitLabPushPayloadParsing tests direct deserialization of the GitLab payload struct. +// TestGitLabPushPayloadParsing tests deserialization of the GitLab payload +// struct. func TestGitLabPushPayloadParsing(t *testing.T) { t.Parallel() @@ -671,9 +768,8 @@ func TestGitLabPushPayloadParsing(t *testing.T) { assert.Len(t, p.Commits, 1) } -// TestExtractBranch tests branch extraction via HandleWebhook integration (extractBranch is unexported). -// -//nolint:funlen // table-driven test with comprehensive test cases +// TestExtractBranch tests branch extraction via HandleWebhook integration +// (extractBranch is unexported). func TestExtractBranch(testingT *testing.T) { testingT.Parallel() @@ -684,8 +780,8 @@ func TestExtractBranch(testingT *testing.T) { }{ { name: "extracts main branch", - ref: "refs/heads/main", - expected: "main", + ref: refMain, + expected: branchMain, }, { name: "extracts feature branch", @@ -699,8 +795,8 @@ func TestExtractBranch(testingT *testing.T) { }, { name: "returns raw ref if no prefix", - ref: "main", - expected: "main", + ref: branchMain, + expected: branchMain, }, { name: "handles empty ref", @@ -728,7 +824,7 @@ func TestExtractBranch(testingT *testing.T) { payload := []byte(`{"ref": "` + testCase.ref + `"}`) err := svc.HandleWebhook( - context.Background(), app, webhook.SourceGitea, "push", payload, + context.Background(), app, webhook.SourceGitea, pushEventType, payload, ) require.NoError(t, err) @@ -750,7 +846,7 @@ func TestHandleWebhookMatchingBranch(t *testing.T) { svc, dbInst, cleanup := setupTestService(t) defer cleanup() - app := createTestApp(t, dbInst, "main") + app := createTestApp(t, dbInst, branchMain) payload := []byte(`{ "ref": "refs/heads/main", @@ -767,7 +863,7 @@ func TestHandleWebhookMatchingBranch(t *testing.T) { }`) err := svc.HandleWebhook( - context.Background(), app, webhook.SourceGitea, "push", payload, + context.Background(), app, webhook.SourceGitea, pushEventType, payload, ) require.NoError(t, err) @@ -779,8 +875,8 @@ func TestHandleWebhookMatchingBranch(t *testing.T) { require.Len(t, events, 1) event := events[0] - assert.Equal(t, "push", event.EventType) - assert.Equal(t, "main", event.Branch) + assert.Equal(t, pushEventType, event.EventType) + assert.Equal(t, branchMain, event.Branch) assert.True(t, event.Matched) assert.Equal(t, "abc123def456", event.CommitSHA.String) } @@ -791,12 +887,12 @@ func TestHandleWebhookNonMatchingBranch(t *testing.T) { svc, dbInst, cleanup := setupTestService(t) defer cleanup() - app := createTestApp(t, dbInst, "main") + app := createTestApp(t, dbInst, branchMain) payload := []byte(`{"ref": "refs/heads/develop", "after": "def789ghi012"}`) err := svc.HandleWebhook( - context.Background(), app, webhook.SourceGitea, "push", payload, + context.Background(), app, webhook.SourceGitea, pushEventType, payload, ) require.NoError(t, err) @@ -814,10 +910,11 @@ func TestHandleWebhookInvalidJSON(t *testing.T) { svc, dbInst, cleanup := setupTestService(t) defer cleanup() - app := createTestApp(t, dbInst, "main") + app := createTestApp(t, dbInst, branchMain) err := svc.HandleWebhook( - context.Background(), app, webhook.SourceGitea, "push", []byte(`{invalid json}`), + context.Background(), app, webhook.SourceGitea, pushEventType, + []byte(`{invalid json}`), ) require.NoError(t, err) @@ -832,10 +929,10 @@ func TestHandleWebhookEmptyPayload(t *testing.T) { svc, dbInst, cleanup := setupTestService(t) defer cleanup() - app := createTestApp(t, dbInst, "main") + app := createTestApp(t, dbInst, branchMain) err := svc.HandleWebhook( - context.Background(), app, webhook.SourceGitea, "push", []byte(`{}`), + context.Background(), app, webhook.SourceGitea, pushEventType, []byte(`{}`), ) require.NoError(t, err) @@ -845,14 +942,43 @@ func TestHandleWebhookEmptyPayload(t *testing.T) { assert.False(t, events[0].Matched) } -// TestHandleWebhookGitHubSource tests HandleWebhook with a GitHub push payload. -func TestHandleWebhookGitHubSource(t *testing.T) { - t.Parallel() +// assertHandleWebhookDeploys runs HandleWebhook for payload against a fresh +// app on branchMain and asserts the recorded event matched with the given +// commit SHA and commit URL. +func assertHandleWebhookDeploys( + t *testing.T, + source webhook.Source, + payload []byte, + wantSHA string, + wantCommitURL string, +) { + t.Helper() svc, dbInst, cleanup := setupTestService(t) defer cleanup() - app := createTestApp(t, dbInst, "main") + app := createTestApp(t, dbInst, branchMain) + + err := svc.HandleWebhook(context.Background(), app, source, pushEventType, payload) + require.NoError(t, err) + + // Allow async deployment goroutine to complete before test cleanup + time.Sleep(100 * time.Millisecond) + + events, err := app.GetWebhookEvents(context.Background(), 10) + require.NoError(t, err) + require.Len(t, events, 1) + + event := events[0] + assert.Equal(t, branchMain, event.Branch) + assert.True(t, event.Matched) + assert.Equal(t, wantSHA, event.CommitSHA.String) + assert.Equal(t, wantCommitURL, event.CommitURL.String) +} + +// TestHandleWebhookGitHubSource tests HandleWebhook with a GitHub push payload. +func TestHandleWebhookGitHubSource(t *testing.T) { + t.Parallel() payload := []byte(`{ "ref": "refs/heads/main", @@ -870,34 +996,16 @@ func TestHandleWebhookGitHubSource(t *testing.T) { } }`) - err := svc.HandleWebhook( - context.Background(), app, webhook.SourceGitHub, "push", payload, + assertHandleWebhookDeploys( + t, webhook.SourceGitHub, payload, + "github123", "https://github.com/org/repo/commit/github123", ) - require.NoError(t, err) - - // Allow async deployment goroutine to complete before test cleanup - time.Sleep(100 * time.Millisecond) - - events, err := app.GetWebhookEvents(context.Background(), 10) - require.NoError(t, err) - require.Len(t, events, 1) - - event := events[0] - assert.Equal(t, "main", event.Branch) - assert.True(t, event.Matched) - assert.Equal(t, "github123", event.CommitSHA.String) - assert.Equal(t, "https://github.com/org/repo/commit/github123", event.CommitURL.String) } // TestHandleWebhookGitLabSource tests HandleWebhook with a GitLab push payload. func TestHandleWebhookGitLabSource(t *testing.T) { t.Parallel() - svc, dbInst, cleanup := setupTestService(t) - defer cleanup() - - app := createTestApp(t, dbInst, "main") - payload := []byte(`{ "ref": "refs/heads/main", "after": "gitlab456", @@ -917,23 +1025,10 @@ func TestHandleWebhookGitLabSource(t *testing.T) { ] }`) - err := svc.HandleWebhook( - context.Background(), app, webhook.SourceGitLab, "push", payload, + assertHandleWebhookDeploys( + t, webhook.SourceGitLab, payload, + "gitlab456", "https://gitlab.com/group/project/-/commit/gitlab456", ) - require.NoError(t, err) - - // Allow async deployment goroutine to complete before test cleanup - time.Sleep(100 * time.Millisecond) - - events, err := app.GetWebhookEvents(context.Background(), 10) - require.NoError(t, err) - require.Len(t, events, 1) - - event := events[0] - assert.Equal(t, "main", event.Branch) - assert.True(t, event.Matched) - assert.Equal(t, "gitlab456", event.CommitSHA.String) - assert.Equal(t, "https://gitlab.com/group/project/-/commit/gitlab456", event.CommitURL.String) } // TestSetupTestService verifies the test helper creates a working test service. @@ -962,10 +1057,10 @@ func TestPushEventConstruction(t *testing.T) { event := webhook.PushEvent{ Source: webhook.SourceGitHub, - Ref: "refs/heads/main", + Ref: refMain, Before: "000", After: "abc", - Branch: "main", + Branch: branchMain, RepoName: "org/repo", CloneURL: webhook.UnparsedURL("https://github.com/org/repo.git"), HTMLURL: webhook.UnparsedURL("https://github.com/org/repo"), @@ -973,7 +1068,7 @@ func TestPushEventConstruction(t *testing.T) { Pusher: "user", } - assert.Equal(t, "main", event.Branch) + assert.Equal(t, branchMain, event.Branch) assert.Equal(t, webhook.SourceGitHub, event.Source) assert.Equal(t, "abc", event.After) } diff --git a/script/bootstrap b/script/bootstrap index b848b23..f1b3e30 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -10,11 +10,11 @@ set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" -# Pinned versions, 2026-07-07. Never "latest"; exact versions only. -GOLANGCI_LINT_VERSION="2.10.1" -# sha256 of golangci-lint-2.10.1-linux-.tar.gz release archives -GOLANGCI_LINT_SHA256_AMD64="dfa775874cf0561b404a02a8f4481fc69b28091da95aa697259820d429b09c99" -GOLANGCI_LINT_SHA256_ARM64="6652b42ae02915eb2f9cb2a2e0cac99514c8eded8388d88ae3e06e1a52c00de8" +# Pinned versions, 2026-08-07. Never "latest"; exact versions only. +GOLANGCI_LINT_VERSION="2.12.2" +# sha256 of golangci-lint-2.12.2-linux-.tar.gz release archives +GOLANGCI_LINT_SHA256_AMD64="8df580d2670fed8fa984aac0507099af8df275e665215f5c7a2ae3943893a553" +GOLANGCI_LINT_SHA256_ARM64="44cd40a8c76c86755375adfeea52cfd3533cb43d7bd647771e0ae065e166df3a" PKGMGR="" SUDO=""