From cdcf527b250ba2053cad551a2ebc94cb96fa9cc3 Mon Sep 17 00:00:00 2001 From: sneak Date: Wed, 9 Sep 2026 14:12:09 +0000 Subject: [PATCH] Fix four deployability blockers found by QA (closes #189, closes #190, closes #191, closes #192) - CSRF over plain HTTP (#189): gorilla/csrf assumed https for its same-origin check, so setup and every POST returned 403 over plain HTTP. Gate csrf.PlaintextHTTPRequest on a new UPAAS_PLAINTEXT_HTTP config value; the default keeps https, correct for a TLS-terminating reverse proxy. The README plain-HTTP recipe now sets it. - git image never pulled (#190): ensureImage pulls alpine/git (pinned digest unchanged) when absent, before the clone container is created. - port-mapping 500 (#192): the ports delete form used {{ .CSRFField }} inside {{range .Ports}}, where the dot is a *models.Port; use {{ $.CSRFField }} like the labels and volumes blocks. - env-var 403 (#191): the editor read the CSRF token from $el (the submitting form, which has none) instead of $root, sending an empty token; read from $root. Model: opus-4-8 --- README.md | 8 ++++ TODO.md | 4 ++ internal/config/config.go | 3 ++ internal/docker/client.go | 41 +++++++++++++++++++ internal/middleware/csrf_test.go | 67 +++++++++++++++++++++++++++++++ internal/middleware/middleware.go | 26 +++++++++++- static/js/app-detail.js | 2 +- templates/app_detail.html | 2 +- 8 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 internal/middleware/csrf_test.go diff --git a/README.md b/README.md index 832a4a0..09ea48f 100644 --- a/README.md +++ b/README.md @@ -191,6 +191,7 @@ Environment variables: | `UPAAS_DATA_DIR` | Data directory for SQLite and keys | `./data` (local dev only — use absolute path for Docker) | | `UPAAS_HOST_DATA_DIR` | Host path for DATA_DIR (when running in container) | *(none — must be set to an absolute path)* | | `UPAAS_DOCKER_HOST` | Docker socket path | unix:///var/run/docker.sock | +| `UPAAS_PLAINTEXT_HTTP` | Set when µPaaS is reached over plain HTTP (no TLS-terminating proxy in front) so CSRF origin checks use `http://`. Leave unset behind a TLS-terminating reverse proxy. | false | | `DEBUG` | Enable debug logging | false | | `SENTRY_DSN` | Sentry error reporting DSN | "" | | `METRICS_USERNAME` | Basic auth for /metrics | "" | @@ -204,9 +205,14 @@ docker run -d \ -v /var/run/docker.sock:/var/run/docker.sock \ -v /path/on/host/upaas-data:/var/lib/upaas \ -e UPAAS_HOST_DATA_DIR=/path/on/host/upaas-data \ + -e UPAAS_PLAINTEXT_HTTP=true \ upaas ``` +This recipe serves plain HTTP, so `UPAAS_PLAINTEXT_HTTP=true` is required for +setup and every other form to pass the CSRF origin check. Behind a +TLS-terminating reverse proxy, drop that line. + ### Docker Compose ```yaml @@ -221,6 +227,8 @@ services: - ${HOST_DATA_DIR}:/var/lib/upaas environment: - UPAAS_HOST_DATA_DIR=${HOST_DATA_DIR} + # Set when serving plain HTTP (no TLS-terminating proxy); drop behind one + - UPAAS_PLAINTEXT_HTTP=true # Optional: uncomment to enable debug logging # - DEBUG=true # Optional: Sentry error reporting diff --git a/TODO.md b/TODO.md index 7e6d229..2f051a0 100644 --- a/TODO.md +++ b/TODO.md @@ -20,6 +20,10 @@ main cannot regress. # Completed Steps +- 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), + and the port-mapping delete form's CSRF field (#192). - 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, diff --git a/internal/config/config.go b/internal/config/config.go index f82b13c..6d42b14 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -49,6 +49,7 @@ type Config struct { DockerHost string SentryDSN string MaintenanceMode bool + PlaintextHTTP bool // clients reach µPaaS over plain HTTP (no TLS-terminating proxy) MetricsUsername string MetricsPassword string SessionSecret string `json:"-"` @@ -100,6 +101,7 @@ func setupViper(name string) { viper.SetDefault("DOCKER_HOST", "unix:///var/run/docker.sock") viper.SetDefault("SENTRY_DSN", "") viper.SetDefault("MAINTENANCE_MODE", false) + viper.SetDefault("PLAINTEXT_HTTP", false) viper.SetDefault("METRICS_USERNAME", "") viper.SetDefault("METRICS_PASSWORD", "") viper.SetDefault("SESSION_SECRET", "") @@ -135,6 +137,7 @@ func buildConfig(log *slog.Logger, params *Params) (*Config, error) { DockerHost: viper.GetString("DOCKER_HOST"), SentryDSN: viper.GetString("SENTRY_DSN"), MaintenanceMode: viper.GetBool("MAINTENANCE_MODE"), + PlaintextHTTP: viper.GetBool("PLAINTEXT_HTTP"), MetricsUsername: viper.GetString("METRICS_USERNAME"), MetricsPassword: viper.GetString("METRICS_PASSWORD"), SessionSecret: viper.GetString("SESSION_SECRET"), diff --git a/internal/docker/client.go b/internal/docker/client.go index e1f4c19..168f3fa 100644 --- a/internal/docker/client.go +++ b/internal/docker/client.go @@ -667,10 +667,51 @@ func (c *Client) performClone( return c.runGitClone(ctx, gitContainerID) } +// ensureImage pulls ref if it is not already present locally. The pinned +// digest is preserved: a pull of an image already present is a no-op, and a +// missing one is fetched before it is used to create a container. +func (c *Client) ensureImage(ctx context.Context, ref string) error { + _, _, err := c.docker.ImageInspectWithRaw(ctx, ref) + if err == nil { + return nil + } + + if !client.IsErrNotFound(err) { + return fmt.Errorf("failed to inspect image %s: %w", ref, err) + } + + c.log.Info("pulling image", "image", ref) + + reader, err := c.docker.ImagePull(ctx, ref, image.PullOptions{}) + if err != nil { + return fmt.Errorf("failed to pull image %s: %w", ref, err) + } + + defer func() { + closeErr := reader.Close() + if closeErr != nil { + c.log.Error("failed to close image pull reader", "error", closeErr) + } + }() + + // The pull only completes once its response stream is fully drained. + _, err = io.Copy(io.Discard, reader) + if err != nil { + return fmt.Errorf("failed to pull image %s: %w", ref, err) + } + + return nil +} + func (c *Client) createGitContainer( ctx context.Context, cfg *cloneConfig, ) (ContainerID, error) { + err := c.ensureImage(ctx, gitImage) + if err != nil { + return "", err + } + gitSSHCmd := "ssh -i /keys/deploy_key -o StrictHostKeyChecking=no" // Build the git command using environment variables to avoid shell injection. diff --git a/internal/middleware/csrf_test.go b/internal/middleware/csrf_test.go new file mode 100644 index 0000000..fd13ed7 --- /dev/null +++ b/internal/middleware/csrf_test.go @@ -0,0 +1,67 @@ +package middleware //nolint:testpackage // tests internal CSRF behavior + +import ( + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + + "sneak.berlin/go/upaas/internal/config" +) + +//nolint:gosec // test credentials +func newCSRFTestMiddleware(plaintextHTTP bool) *Middleware { + return &Middleware{ + log: slog.Default(), + params: &Params{ + Config: &config.Config{ + SessionSecret: "test-secret-32-bytes-long-enough", + PlaintextHTTP: plaintextHTTP, + }, + }, + } +} + +// postWithPlainHTTPOrigin drives a tokenless POST carrying a plain-HTTP Origin +// through the CSRF middleware and returns the "Forbidden - " body. +// gorilla/csrf checks the Origin before the token, so the reason reveals which +// check rejected the request. +func postWithPlainHTTPOrigin(t *testing.T, plaintextHTTP bool) string { + t.Helper() + + m := newCSRFTestMiddleware(plaintextHTTP) + handler := m.CSRF()(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + })) + + req := httptest.NewRequestWithContext( + t.Context(), http.MethodPost, "http://example.com/setup", nil) + req.Header.Set("Origin", "http://example.com") + + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusForbidden, rec.Code) + + return rec.Body.String() +} + +// Without PlaintextHTTP the origin check assumes https and rejects a browser's +// http:// Origin, which is what broke setup over plain HTTP. +func TestCSRF_PlaintextDisabled_RejectsPlainHTTPOrigin(t *testing.T) { + t.Parallel() + + assert.Contains(t, postWithPlainHTTPOrigin(t, false), "origin invalid") +} + +// With PlaintextHTTP the origin check uses http, so a matching http:// Origin +// passes it and the request only fails later for the missing token. +func TestCSRF_PlaintextEnabled_AllowsPlainHTTPOrigin(t *testing.T) { + t.Parallel() + + body := postWithPlainHTTPOrigin(t, true) + assert.NotContains(t, body, "origin invalid") + assert.Contains(t, body, "CSRF token not found") +} diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 63a6c2e..7a3ea15 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -255,12 +255,34 @@ func (m *Middleware) SessionAuth() func(http.Handler) http.Handler { } // CSRF returns CSRF protection middleware using gorilla/csrf. +// +// gorilla/csrf assumes the request scheme is https for its same-origin check +// unless the request is marked plaintext. A TLS-terminating reverse proxy +// (the default deployment) presents https to the browser, so the default is +// correct there. When µPaaS is reached over plain HTTP — directly, or behind a +// proxy that does not terminate TLS — set UPAAS_PLAINTEXT_HTTP so the origin +// check compares against http:// and setup over plain HTTP works. func (m *Middleware) CSRF() func(http.Handler) http.Handler { - return csrf.Protect( + protect := csrf.Protect( []byte(m.params.Config.SessionSecret), - csrf.Secure(false), // Allow HTTP for development; reverse proxy handles TLS + csrf.Secure(false), // cookie Secure flag; TLS is terminated upstream csrf.Path("/"), ) + + if !m.params.Config.PlaintextHTTP { + return protect + } + + return func(next http.Handler) http.Handler { + protected := protect(next) + + return http.HandlerFunc(func( + writer http.ResponseWriter, + request *http.Request, + ) { + protected.ServeHTTP(writer, csrf.PlaintextHTTPRequest(request)) + }) + } } // loginRateLimit configures the login rate limiter. diff --git a/static/js/app-detail.js b/static/js/app-detail.js index eec57c5..381a769 100644 --- a/static/js/app-detail.js +++ b/static/js/app-detail.js @@ -59,7 +59,7 @@ document.addEventListener("alpine:init", () => { }, submitAll() { - const csrfInput = this.$el.querySelector( + const csrfInput = this.$root.querySelector( 'input[name="gorilla.csrf.Token"]', ); const csrfToken = csrfInput ? csrfInput.value : ""; diff --git a/templates/app_detail.html b/templates/app_detail.html index b80ad87..ecb085e 100644 --- a/templates/app_detail.html +++ b/templates/app_detail.html @@ -322,7 +322,7 @@
- {{ .CSRFField }} + {{ $.CSRFField }}