diff --git a/.prettierignore b/.prettierignore new file mode 100644 index 0000000..d87c685 --- /dev/null +++ b/.prettierignore @@ -0,0 +1,2 @@ +# Vendored, minified third-party bundles must never be reformatted. +*.min.js diff --git a/Dockerfile b/Dockerfile index f31025e..1a996e8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -8,8 +8,12 @@ RUN go mod download COPY . . +# golangci-lint is invoked directly here, not via `make lint`: script/lint +# now runs the linter by building Dockerfile.lint, and shelling out to +# `docker build` from inside this image build would be docker-in-docker. +# This image is golangci/golangci-lint, so the pinned linter is on PATH. RUN make fmt-check -RUN make lint +RUN golangci-lint run --config .golangci.yml ./... # Build stage — tests and compilation # golang:1.25-alpine diff --git a/Dockerfile.lint b/Dockerfile.lint new file mode 100644 index 0000000..9836aef --- /dev/null +++ b/Dockerfile.lint @@ -0,0 +1,23 @@ +# Lint image — runs golangci-lint inside a container so every lint uses +# the pinned linter, never a host binary. Linting is a build step, so a +# successful build is a clean lint. Built by script/lint. +# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-07 +FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 + +WORKDIR /src + +COPY go.mod go.sum ./ +RUN go mod download + +COPY . . + +# Caching is waived for linting: on an unchanged tree a cached build runs +# no linter and still exits 0 in under a second. script/lint passes a +# fresh GATE_RUN every time, and referencing it here forces this step to +# re-run, so the linter always executes. +# +# `golangci-lint config verify` is deliberately NOT run: it fetches its +# JSON schema over an unpinned live HTTPS call, which REPO_POLICIES.md +# forbids (all external references must be pinned by hash). +ARG GATE_RUN +RUN echo "lint run: ${GATE_RUN}"; golangci-lint run --config .golangci.yml ./... diff --git a/TODO.md b/TODO.md index 2f051a0..ae65dff 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,24 @@ main cannot regress. # Completed Steps +- 2026-09-22: Fixed the flaky `t.TempDir` cleanup race in + `internal/service/webhook` by tracking the async deployment goroutine + in a `sync.WaitGroup` and exposing `WaitForDeployments`; tests now + synchronize on completion instead of sleeping (#198). +- 2026-09-22: Linting now runs only in Docker. Added `Dockerfile.lint` + (pinned golangci-lint v2.12.2, cache-busted via a `GATE_RUN` build arg + so the linter always executes), reduced `script/lint` to building it, + dropped the golangci-lint install from `script/bootstrap`, and switched + the `Dockerfile` lint stage to invoke `golangci-lint` directly instead + of `make lint` to avoid docker-in-docker (#188). +- 2026-09-22: Added `.prettierignore` so `make fmt` no longer rewrites + the vendored `static/js/alpine.min.js` bundle (#185). +- 2026-09-22: Fixed the gosec G703 path-traversal finding in the deploy + log download handler by verifying the resolved path stays within the + deploy log directory before serving, returning 404 on escape (#177). +- 2026-09-22: `script/bootstrap` now installs a pinned `goimports` + (`golang.org/x/tools` v0.49.0) into `/usr/local/bin`, so `make fmt` + succeeds on a fresh machine after `make bootstrap` (#184). - 2026-09-09: Fixed four deployability blockers found by QA: CSRF origin check over plain HTTP (`UPAAS_PLAINTEXT_HTTP`, #189), pulling the git image when absent (#190), the env-var editor CSRF token lookup (#191), diff --git a/internal/handlers/app.go b/internal/handlers/app.go index aa83aed..a985c11 100644 --- a/internal/handlers/app.go +++ b/internal/handlers/app.go @@ -611,7 +611,13 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc { return } - // Get the log file path from deploy service + // The log path is derived from request data (the app is looked + // up by a URL parameter), so open it through an os.Root confined + // to the deploy log directory. Root.Open rejects any path that + // escapes the root, so a traversal attempt fails rather than + // serving an arbitrary file. + logDir := h.deploy.GetLogDir() + logPath := h.deploy.GetLogFilePath(application, deployment) if logPath == "" { http.NotFound(writer, request) @@ -619,28 +625,43 @@ func (h *Handlers) HandleDeploymentLogDownload() http.HandlerFunc { return } - // Check if file exists — logPath is constructed internally, not from user input - _, err := os.Stat(logPath) // #nosec G703 -- internal path, not user input - if os.IsNotExist(err) { + relPath, relErr := filepath.Rel(logDir, logPath) + if relErr != nil { http.NotFound(writer, request) return } - if err != nil { - h.log.Error("failed to stat log file", "error", err, "path", logPath) + root, rootErr := os.OpenRoot(logDir) + if rootErr != nil { + http.NotFound(writer, request) + + return + } + defer func() { _ = root.Close() }() + + file, openErr := root.Open(relPath) + if openErr != nil { + http.NotFound(writer, request) + + return + } + defer func() { _ = file.Close() }() + + info, statErr := file.Stat() + if statErr != nil { + h.log.Error("failed to stat log file", "error", statErr, "path", logPath) http.Error(writer, "Internal Server Error", http.StatusInternalServerError) return } - // Extract filename for Content-Disposition header filename := filepath.Base(logPath) writer.Header().Set("Content-Type", "text/plain; charset=utf-8") writer.Header().Set("Content-Disposition", "attachment; filename=\""+filename+"\"") - http.ServeFile(writer, request, logPath) // #nosec G703 -- internal path + http.ServeContent(writer, request, filename, info.ModTime(), file) } } diff --git a/internal/handlers/handlers_test.go b/internal/handlers/handlers_test.go index 6e0624a..68e116e 100644 --- a/internal/handlers/handlers_test.go +++ b/internal/handlers/handlers_test.go @@ -42,6 +42,7 @@ type testContext struct { database *database.Database authSvc *auth.Service appSvc *app.Service + deploySvc *deploy.Service middleware *middleware.Middleware } @@ -186,6 +187,7 @@ func setupTestHandlers(t *testing.T) *testContext { database: dbInstance, authSvc: authSvc, appSvc: appSvc, + deploySvc: deploySvc, middleware: mw, } } diff --git a/internal/handlers/log_download_test.go b/internal/handlers/log_download_test.go new file mode 100644 index 0000000..cbbfd08 --- /dev/null +++ b/internal/handlers/log_download_test.go @@ -0,0 +1,121 @@ +package handlers_test + +import ( + "context" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/upaas/internal/models" +) + +// doLogDownload issues a log-download request for the given app and +// deployment and returns the recorder. +func doLogDownload( + t *testing.T, + testCtx *testContext, + appID string, + deploymentID int64, +) *httptest.ResponseRecorder { + t.Helper() + + idStr := strconv.FormatInt(deploymentID, 10) + + request := httptest.NewRequestWithContext( + t.Context(), + http.MethodGet, + "/apps/"+appID+"/deployments/"+idStr+"/log", + nil, + ) + request = addChiURLParams(request, map[string]string{ + "id": appID, + "deploymentID": idStr, + }) + + recorder := httptest.NewRecorder() + testCtx.handlers.HandleDeploymentLogDownload().ServeHTTP(recorder, request) + + return recorder +} + +// TestHandleDeploymentLogDownloadServesLegitimateFile verifies a normal +// log file is served for download. +func TestHandleDeploymentLogDownloadServesLegitimateFile(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "log-download-app") + + deployment := models.NewDeployment(testCtx.database) + deployment.AppID = createdApp.ID + deployment.Status = models.DeploymentStatusSuccess + require.NoError(t, deployment.Save(context.Background())) + + // Write the log file where the handler will look for it. + logPath := testCtx.deploySvc.GetLogFilePath(createdApp, deployment) + require.NoError(t, os.MkdirAll(filepath.Dir(logPath), 0o750)) + require.NoError(t, os.WriteFile(logPath, []byte("deploy log contents"), 0o600)) + + recorder := doLogDownload(t, testCtx, createdApp.ID, deployment.ID) + + assert.Equal(t, http.StatusOK, recorder.Code) + assert.Contains(t, recorder.Body.String(), "deploy log contents") +} + +// TestHandleDeploymentLogDownloadRejectsPathTraversal verifies the +// os.Root containment guard. A traversal-shaped app name drives the +// resolved log path out of the deploy log directory onto a sentinel +// file that really exists. The handler must refuse to serve it (404) +// rather than leak its contents. Removing the guard makes this test +// fail, which the earlier version — pointed at a non-existent path that +// 404s either way — did not. +func TestHandleDeploymentLogDownloadRejectsPathTraversal(t *testing.T) { + t.Parallel() + + testCtx := setupTestHandlers(t) + createdApp := createTestApp(t, testCtx, "log-traversal-app") + + createdApp.Name = "../.." + require.NoError(t, createdApp.Save(context.Background())) + + // The log root must exist so os.OpenRoot succeeds and the rejection + // comes from the containment check, not a missing directory. + logDir := testCtx.deploySvc.GetLogDir() + require.NoError(t, os.MkdirAll(logDir, 0o750)) + + deployment := models.NewDeployment(testCtx.database) + deployment.AppID = createdApp.ID + deployment.Status = models.DeploymentStatusSuccess + require.NoError(t, deployment.Save(context.Background())) + + // Where the handler resolves the log path to. The traversal name + // makes this land outside logDir; require that it truly escapes so + // the test cannot silently stop covering the guard. + escapedPath := testCtx.deploySvc.GetLogFilePath(createdApp, deployment) + relPath, relErr := filepath.Rel(logDir, escapedPath) + require.NoError(t, relErr) + require.True(t, strings.HasPrefix(relPath, ".."), + "resolved path must escape the log dir, got %q", relPath) + + // Plant a sentinel where the traversal points; a missing guard would + // open and serve it. + require.NoError(t, os.MkdirAll(filepath.Dir(escapedPath), 0o750)) + + const sentinel = "SENTINEL-outside-log-dir-must-not-be-served" + + require.NoError(t, os.WriteFile(escapedPath, []byte(sentinel), 0o600)) + t.Cleanup(func() { _ = os.Remove(escapedPath) }) + + recorder := doLogDownload(t, testCtx, createdApp.ID, deployment.ID) + + assert.Equal(t, http.StatusNotFound, recorder.Code) + assert.NotContains(t, recorder.Body.String(), sentinel, + "containment guard must not serve a file outside the log dir") +} diff --git a/internal/service/deploy/deploy.go b/internal/service/deploy/deploy.go index 887a094..ab1f2c8 100644 --- a/internal/service/deploy/deploy.go +++ b/internal/service/deploy/deploy.go @@ -294,6 +294,12 @@ func (svc *Service) GetLogFilePath( return filepath.Join(svc.config.DataDir, "logs", hostname, app.Name, filename) } +// GetLogDir returns the root directory under which all deployment log +// files live. Paths returned by GetLogFilePath are always inside it. +func (svc *Service) GetLogDir() string { + return filepath.Join(svc.config.DataDir, "logs") +} + // HasActiveDeploy returns true if there is an active deployment for the given app. func (svc *Service) HasActiveDeploy(appID string) bool { _, ok := svc.activeDeploys.Load(appID) diff --git a/internal/service/webhook/webhook.go b/internal/service/webhook/webhook.go index 69c1f6c..02a51ee 100644 --- a/internal/service/webhook/webhook.go +++ b/internal/service/webhook/webhook.go @@ -6,6 +6,7 @@ import ( "database/sql" "fmt" "log/slog" + "sync" "go.uber.org/fx" @@ -31,6 +32,10 @@ type Service struct { db *database.Database deploy *deploy.Service params *ServiceParams + + // deployments tracks the deployment goroutines started by + // triggerDeployment so callers can wait for them to finish. + deployments sync.WaitGroup } // New creates a new webhook Service. @@ -108,6 +113,14 @@ func (svc *Service) HandleWebhook( return nil } +// WaitForDeployments blocks until every deployment goroutine started by +// HandleWebhook has finished, including all writes under the data +// directory. It exists so callers and tests can synchronize on async +// deployment completion instead of polling or sleeping. +func (svc *Service) WaitForDeployments() { + svc.deployments.Wait() +} + func (svc *Service) triggerDeployment( ctx context.Context, app *models.App, @@ -117,7 +130,7 @@ func (svc *Service) triggerDeployment( eventID := event.ID appName := app.Name - go func() { + svc.deployments.Go(func() { // Use context.WithoutCancel to ensure deployment completes // even if the HTTP request context is cancelled. deployCtx := context.WithoutCancel(ctx) @@ -130,5 +143,5 @@ func (svc *Service) triggerDeployment( // Mark event as processed event.Processed = true _ = event.Save(deployCtx) - }() + }) } diff --git a/internal/service/webhook/webhook_test.go b/internal/service/webhook/webhook_test.go index bda7a98..619b271 100644 --- a/internal/service/webhook/webhook_test.go +++ b/internal/service/webhook/webhook_test.go @@ -7,7 +7,6 @@ import ( "os" "path/filepath" "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -828,8 +827,9 @@ func TestExtractBranch(testingT *testing.T) { ) require.NoError(t, err) - // Allow async deployment goroutine to complete before test cleanup - time.Sleep(100 * time.Millisecond) + // Wait for the async deployment goroutine to finish so its + // writes under the temp dir complete before test cleanup. + svc.WaitForDeployments() events, err := app.GetWebhookEvents(context.Background(), 10) require.NoError(t, err) @@ -867,8 +867,9 @@ func TestHandleWebhookMatchingBranch(t *testing.T) { ) require.NoError(t, err) - // Allow async deployment goroutine to complete before test cleanup - time.Sleep(100 * time.Millisecond) + // Wait for the async deployment goroutine to finish so its writes + // under the temp dir complete before test cleanup. + svc.WaitForDeployments() events, err := app.GetWebhookEvents(context.Background(), 10) require.NoError(t, err) @@ -962,8 +963,9 @@ func assertHandleWebhookDeploys( 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) + // Wait for the async deployment goroutine to finish so its writes + // under the temp dir complete before test cleanup. + svc.WaitForDeployments() events, err := app.GetWebhookEvents(context.Background(), 10) require.NoError(t, err) diff --git a/script/bootstrap b/script/bootstrap index f1b3e30..095afa8 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -3,18 +3,18 @@ # this repo. Idempotent: every install is guarded by a check so already # installed tools are skipped. Base tooling comes from nix, apt, brew, # or apk (detected in that order); assumes NOTHING is present (not git, -# make, or go). golangci-lint is packaged in nix, brew, and apk; on apt -# it is installed from a hash-verified GitHub release archive (never -# curl | sh). +# make, or go). goimports is installed with `go install` at a pinned +# version (integrity via the Go module checksum database) into +# /usr/local/bin so it is on PATH. The linter is not installed here: it +# runs only in Docker via script/lint, so docker is its sole prerequisite. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" -# 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" +# Pinned versions. Never "latest"; exact versions only. +# golang.org/x/tools goimports, 2026-08-13. v0.49.0 requires Go 1.25 (matches +# go.mod); v0.50.0 needs Go 1.26. Integrity via the Go module checksum database. +GOIMPORTS_VERSION="v0.49.0" PKGMGR="" SUDO="" @@ -56,50 +56,17 @@ missing() { ! command -v "$1" >/dev/null 2>&1 } -# verify_sha256 -verify_sha256() { - if command -v sha256sum >/dev/null 2>&1; then - actual="$(sha256sum "$1" | cut -d' ' -f1)" - else - actual="$(shasum -a 256 "$1" | cut -d' ' -f1)" - fi - if [ "$actual" != "$2" ]; then - echo "bootstrap: sha256 mismatch for $1" >&2 - echo " expected: $2" >&2 - echo " actual: $actual" >&2 - exit 1 - fi -} - -# apt has no golangci-lint package: install a pinned release archive -# from GitHub, verified by hardcoded sha256 (never curl | sh). -install_golangci_lint_release() { - case "$(uname -m)" in - x86_64) goarch="amd64"; sha="$GOLANGCI_LINT_SHA256_AMD64" ;; - aarch64|arm64) goarch="arm64"; sha="$GOLANGCI_LINT_SHA256_ARM64" ;; - *) - echo "bootstrap: unsupported architecture $(uname -m)" >&2 - exit 1 - ;; - esac - if missing curl; then pkg_install curl curl curl curl; fi - name="golangci-lint-${GOLANGCI_LINT_VERSION}-linux-${goarch}" - tmp="$(mktemp -d)" - curl -fsSL -o "$tmp/$name.tar.gz" \ - "https://github.com/golangci/golangci-lint/releases/download/v${GOLANGCI_LINT_VERSION}/${name}.tar.gz" - verify_sha256 "$tmp/$name.tar.gz" "$sha" - tar -xzf "$tmp/$name.tar.gz" -C "$tmp" - $SUDO install -m 0755 "$tmp/$name/golangci-lint" /usr/local/bin/golangci-lint - rm -rf "$tmp" -} - -ensure_golangci_lint() { - if ! missing golangci-lint; then return 0; fi +# goimports is not packaged uniformly across nix/apt/brew/apk, so install it +# with `go install` at a pinned version and place the binary in /usr/local/bin +# so it is on PATH regardless of shell config. Requires go, which main +# installs first. +ensure_goimports() { + if ! missing goimports; then return 0; fi detect_pkgmgr - case "$PKGMGR" in - apt) install_golangci_lint_release ;; - *) pkg_install golangci-lint golangci-lint golangci-lint golangci-lint ;; - esac + tmp="$(mktemp -d)" + GOBIN="$tmp" go install "golang.org/x/tools/cmd/goimports@${GOIMPORTS_VERSION}" + $SUDO install -m 0755 "$tmp/goimports" /usr/local/bin/goimports + rm -rf "$tmp" } main() { @@ -109,9 +76,17 @@ main() { if missing git; then pkg_install git git git git; fi if missing make; then pkg_install gnumake make make make; fi - # Go toolchain and linter + # Go toolchain if missing go; then pkg_install go golang go go; fi - ensure_golangci_lint + ensure_goimports + + # The linter runs only in Docker (script/lint). Warn, don't fail: the + # rest of the repo works without it. + if missing docker; then + echo "bootstrap: WARNING: docker not found; make lint and" >&2 + echo "bootstrap: make check require it. Install docker to run" >&2 + echo "bootstrap: the linter." >&2 + fi go mod download diff --git a/script/lint b/script/lint index 8017180..1d56b58 100755 --- a/script/lint +++ b/script/lint @@ -1,12 +1,24 @@ #!/bin/sh -# script/lint: run the linter. +# script/lint: run golangci-lint. The linter is never installed on the +# host; it runs only inside Docker, from the pinned image in +# Dockerfile.lint, so every run uses the same linter version everywhere. +# Linting is a build step there, so a successful build is a clean lint. +# +# GATE_RUN differs every run so the lint layer always executes; a cached +# build would otherwise exit 0 in under a second having linted nothing. +# --output=type=cacheonly discards the image and keeps only build cache, +# so no tagged image is left behind. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - golangci-lint run --config .golangci.yml ./... + docker build \ + --build-arg GATE_RUN="$(date +%s)-$$" \ + --output=type=cacheonly \ + -f Dockerfile.lint \ + . } main "$@"