Compare commits

...
2 Commits
Author SHA1 Message Date
sneak 1e77b8d4f2 Name the cause on the page for a refused form (closes #358)
check / check (push) Waiting to run
The CSRF middleware answered a refused login or generator form with the
library's bare text, such as "Forbidden - origin invalid". It now answers
403 with a page that names the cause and what to do, and logs a warning
with the request ID and the library's reason. A page opened over plain
HTTP with debug off and a form from another host both fail the origin
check; the page tells them apart by the host in the Origin or Referer
header.

Model: opus-5-5
2026-11-11 21:30:01 +00:00
clawbot 851a374e53 Test that pixad writes only its own errors to stderr (closes #359)
check / check (push) Canceled after 0s
Both tests run pixad as a child process. They fail against this code:
every start writes the dependency-injection library's event log to
stderr, and a startup error is printed twice inside its frames.

Model: opus-5-5
2026-11-11 22:26:52 +01:00
11 changed files with 629 additions and 9 deletions

No files matched your search

+4
View File
@@ -376,6 +376,10 @@ instead. Plain HTTP is for development on the browser's own machine. The login
session cookie is always marked `Secure`. Over plain HTTP a browser keeps such a
cookie only for its own machine (`localhost`), if at all.
A form refused with 403 gets a page that names the cause and what to do about
it. pixa logs a `form refused` warning with the request ID and the form-token
library's reason, such as `origin invalid`.
The image routes answer the errors listed for them with a JSON body holding
`error`, `status` and `timestamp`.
+1 -1
View File
@@ -17,6 +17,7 @@ require (
github.com/prometheus/client_golang v1.23.2
github.com/slok/go-http-metrics v0.13.0
github.com/spf13/cobra v1.10.2
go.uber.org/dig v1.19.0
go.uber.org/fx v1.24.0
golang.org/x/crypto v0.41.0
golang.org/x/sync v0.19.0
@@ -126,7 +127,6 @@ require (
go.opentelemetry.io/otel v1.36.0 // indirect
go.opentelemetry.io/otel/metric v1.36.0 // indirect
go.opentelemetry.io/otel/trace v1.36.0 // indirect
go.uber.org/dig v1.19.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.27.0 // indirect
go.yaml.in/yaml/v2 v2.4.2 // indirect
+43 -2
View File
@@ -2,13 +2,16 @@
package app
import (
"context"
"fmt"
"io"
"os"
"os/signal"
"syscall"
"github.com/prometheus/client_golang/prometheus"
"github.com/spf13/cobra"
"go.uber.org/dig"
"go.uber.org/fx"
"sneak.berlin/go/pixa/internal/config"
"sneak.berlin/go/pixa/internal/database"
@@ -53,7 +56,10 @@ func run(_ *cobra.Command, _ []string) {
// A write to a closed stdout or stderr must not end the process.
signal.Ignore(syscall.SIGPIPE)
fx.New(
app := fx.New(
// pixad logs with slog only; runApp prints a startup or shutdown
// error itself.
fx.NopLogger,
fx.Provide(
config.New,
database.New,
@@ -71,5 +77,40 @@ func run(_ *cobra.Command, _ []string) {
func(log *logger.Logger) { log.Identify() },
func(*server.Server) {},
),
).Run()
)
exitCode := runApp(app, os.Stderr)
if exitCode != 0 {
os.Exit(exitCode)
}
}
// runApp starts app, waits for a signal or a shutdown request, stops app,
// and returns the status pixad exits with, as fx.App.Run does. It prints
// a start or stop error to stderr, once, as the error that caused it:
// dig.RootCause drops the frames fx and dig wrap around it.
func runApp(app *fx.App, stderr io.Writer) int {
startCtx, cancelStart := context.WithTimeout(context.Background(), app.StartTimeout())
defer cancelStart()
err := app.Start(startCtx)
if err != nil {
_, _ = fmt.Fprintln(stderr, dig.RootCause(err))
return 1
}
shutdown := <-app.Wait()
stopCtx, cancelStop := context.WithTimeout(context.Background(), app.StopTimeout())
defer cancelStop()
err = app.Stop(stopCtx)
if err != nil {
_, _ = fmt.Fprintln(stderr, dig.RootCause(err))
return 1
}
return shutdown.ExitCode
}
+65
View File
@@ -0,0 +1,65 @@
package app
import (
"bytes"
"context"
"errors"
"io"
"testing"
"go.uber.org/fx"
)
var errTestStopFailed = errors.New("test stop hook failed")
// TestRunAppPrintsStopErrorOnce checks that when a stop hook fails, runApp
// prints the hook's error once and returns exit status 1.
func TestRunAppPrintsStopErrorOnce(t *testing.T) {
t.Parallel()
app := fx.New(
fx.NopLogger,
fx.Invoke(func(lifecycle fx.Lifecycle, shutdowner fx.Shutdowner) {
lifecycle.Append(fx.Hook{
// Ask to shut down as soon as the app has started, so
// that runApp goes on to stop it.
OnStart: func(context.Context) error { return shutdowner.Shutdown() },
OnStop: func(context.Context) error { return errTestStopFailed },
})
}),
)
var stderr bytes.Buffer
exitCode := runApp(app, &stderr)
if exitCode != 1 {
t.Errorf("runApp() = %d, want 1", exitCode)
}
want := errTestStopFailed.Error() + "\n"
if stderr.String() != want {
t.Errorf("stderr = %q, want %q", stderr.String(), want)
}
}
// TestRunAppReturnsShutdownExitCode checks that runApp returns the exit
// code a shutdown request names.
func TestRunAppReturnsShutdownExitCode(t *testing.T) {
t.Parallel()
app := fx.New(
fx.NopLogger,
fx.Invoke(func(lifecycle fx.Lifecycle, shutdowner fx.Shutdowner) {
lifecycle.Append(fx.Hook{
OnStart: func(context.Context) error {
return shutdowner.Shutdown(fx.ExitCode(3))
},
})
}),
)
exitCode := runApp(app, io.Discard)
if exitCode != 3 {
t.Errorf("runApp() = %d, want 3", exitCode)
}
}
+177
View File
@@ -0,0 +1,177 @@
package app_test
import (
"bytes"
"errors"
"fmt"
"net"
"net/http"
"os"
"os/exec"
"strconv"
"syscall"
"testing"
"time"
"sneak.berlin/go/pixa/internal/app"
)
// These tests run pixad in a child process: the test binary itself,
// started again with runPixadInChildEnvVar set, which makes TestMain run
// pixad instead of the tests.
const runPixadInChildEnvVar = "TEST_RUN_PIXAD_IN_CHILD"
const validTestSigningKey = "test-signing-key-for-app-tests-0123456789"
func TestMain(m *testing.M) {
if os.Getenv(runPixadInChildEnvVar) == "1" {
os.Args = []string{"pixad"}
app.Run("test")
os.Exit(0)
}
os.Exit(m.Run())
}
// pixadCommand returns a command that runs pixad with only the given
// environment, in an empty directory with an empty HOME, so that no
// config file is found.
func pixadCommand(t *testing.T, env ...string) (*exec.Cmd, *bytes.Buffer) {
t.Helper()
testBinary := os.Args[0]
cmd := exec.CommandContext(t.Context(), testBinary) //nolint:gosec // not tainted
cmd.Dir = t.TempDir()
// A test binary built with -cover warns on stderr at exit when
// GOCOVERDIR is not set.
cmd.Env = append([]string{
runPixadInChildEnvVar + "=1",
"HOME=" + t.TempDir(),
"GOCOVERDIR=" + t.TempDir(),
}, env...)
var stderr bytes.Buffer
cmd.Stderr = &stderr
return cmd, &stderr
}
// TestRunPrintsStartupErrorOnce checks that an invalid setting is
// printed once, as the line that names it, and nothing else goes to
// stderr.
func TestRunPrintsStartupErrorOnce(t *testing.T) {
t.Parallel()
cmd, stderr := pixadCommand(t, "PIXA_SIGNING_KEY=short")
err := cmd.Run()
var exitErr *exec.ExitError
if !errors.As(err, &exitErr) || exitErr.ExitCode() != 1 {
t.Fatalf("pixad: got %v, want exit status 1; stderr:\n%s", err, stderr)
}
want := `config key "signing_key" (environment variable PIXA_SIGNING_KEY): ` +
"value too short: must be at least 32 characters, got 5\n"
if stderr.String() != want {
t.Errorf("stderr:\n%s\nwant:\n%s", stderr, want)
}
}
// TestRunWritesNothingToStderrOnStartAndStop starts pixad, waits until
// it answers its health check, stops it with SIGTERM, and checks that it
// wrote nothing to stderr.
func TestRunWritesNothingToStderrOnStartAndStop(t *testing.T) {
t.Parallel()
port := freePort(t)
cmd, stderr := pixadCommand(t,
"PORT="+strconv.Itoa(port),
"PIXA_STATE_DIR="+t.TempDir(),
"PIXA_SIGNING_KEY="+validTestSigningKey,
)
err := cmd.Start()
if err != nil {
t.Fatalf("starting pixad: %v", err)
}
waitForHealthCheck(t, cmd, port, stderr)
err = cmd.Process.Signal(syscall.SIGTERM)
if err != nil {
t.Fatalf("signalling pixad: %v", err)
}
err = cmd.Wait()
if err != nil {
t.Fatalf("pixad: got %v, want exit status 0; stderr:\n%s", err, stderr)
}
if stderr.Len() != 0 {
t.Errorf("stderr is not empty:\n%s", stderr)
}
}
// freePort returns a TCP port on the loopback address that nothing
// listens on.
func freePort(t *testing.T) int {
t.Helper()
var listenConfig net.ListenConfig
listener, err := listenConfig.Listen(t.Context(), "tcp", "127.0.0.1:0")
if err != nil {
t.Fatalf("finding a free port: %v", err)
}
addr, ok := listener.Addr().(*net.TCPAddr)
if !ok {
t.Fatalf("listener address %v is not a TCP address", listener.Addr())
}
err = listener.Close()
if err != nil {
t.Fatalf("closing listener: %v", err)
}
return addr.Port
}
// waitForHealthCheck waits up to 30 seconds for pixad, started as cmd, to
// answer its health check on port. If it does not, it stops pixad and
// fails the test with what pixad wrote to stderr.
func waitForHealthCheck(t *testing.T, cmd *exec.Cmd, port int, stderr *bytes.Buffer) {
t.Helper()
url := fmt.Sprintf("http://127.0.0.1:%d/.well-known/healthcheck.json", port)
deadline := time.Now().Add(30 * time.Second)
for time.Now().Before(deadline) {
req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, url, nil)
if err != nil {
t.Fatalf("health check request: %v", err)
}
resp, err := http.DefaultClient.Do(req)
if err == nil {
_ = resp.Body.Close()
if resp.StatusCode == http.StatusOK {
return
}
}
time.Sleep(100 * time.Millisecond)
}
// Stop pixad first: stderr is complete, and safe to read, only once
// cmd.Wait has returned.
_ = cmd.Process.Kill()
_ = cmd.Wait()
t.Fatalf("pixad did not answer its health check on port %d; stderr:\n%s",
port, stderr)
}
+1 -1
View File
@@ -55,7 +55,7 @@ func newCSRFTestRouter(t *testing.T) (*Handlers, http.Handler) {
config: cfg,
sessMgr: session.NewManager(keys.SessionHashKey, keys.SessionBlockKey),
encGen: encurl.NewGenerator(keys.EncryptedURLKey),
csrfProtect: newCSRFProtect(keys.CSRFKey, cfg.Debug),
csrfProtect: newCSRFProtect(keys.CSRFKey, cfg.Debug, slog.New(slog.DiscardHandler)),
}
r := chi.NewRouter()
+1 -1
View File
@@ -43,7 +43,7 @@ func newBodyLimitTestRouter(
config: cfg,
sessMgr: session.NewManager(keys.SessionHashKey, keys.SessionBlockKey),
encGen: encurl.NewGenerator(keys.EncryptedURLKey),
csrfProtect: newCSRFProtect(keys.CSRFKey, cfg.Debug),
csrfProtect: newCSRFProtect(keys.CSRFKey, cfg.Debug, slog.New(slog.DiscardHandler)),
}
r := chi.NewRouter()
+106 -2
View File
@@ -1,18 +1,43 @@
package handlers
import (
"bytes"
"errors"
"html/template"
"log/slog"
"net/http"
"net/url"
"github.com/go-chi/chi/v5/middleware"
"github.com/gorilla/csrf"
"sneak.berlin/go/pixa/internal/seal"
"sneak.berlin/go/pixa/internal/templates"
)
// The causes the form-refused page names, each with what to do about it.
const (
causePlainHTTP = "This page was opened over plain HTTP while debug is " +
"off. Open it over HTTPS, or turn debug on to use plain HTTP on " +
"your own machine."
causeHTTPSWithDebug = "This page was opened over HTTPS while debug is " +
"on. Turn debug off, or open the page over plain HTTP."
causeOtherHost = "The form was sent from another host than the one in " +
"the request's Host header. A reverse proxy in front of pixa must " +
"pass the Host header on unchanged."
causeNoReferer = "The browser sent neither an Origin nor a Referer " +
"header, and over HTTPS pixa needs one to check where the form " +
"came from. Let the browser send the Referer header to this site."
causeStaleToken = "The form token is missing or out of date. Reload " +
"the page and submit the form again."
)
// newCSRFProtect builds the CSRF-protection middleware for the
// state-mutating HTML form routes. The token lives in its own cookie,
// independent of the session cookie, so it also protects the login POST
// where no session exists yet (login CSRF). key signs that cookie.
// where no session exists yet (login CSRF). key signs that cookie. A
// refused form is answered by the handler that handleFormRefused returns.
// log gets a warning with the library's reason, such as "origin invalid".
//
// When plaintext is true (local HTTP development), requests are marked
// plaintext so the library neither demands an https Referer nor sets a
@@ -20,13 +45,14 @@ import (
// service runs behind a TLS-terminating proxy, so plaintext is false and
// the library enforces its https Referer origin check.
func newCSRFProtect(
key [seal.KeySize]byte, plaintext bool,
key [seal.KeySize]byte, plaintext bool, log *slog.Logger,
) func(http.Handler) http.Handler {
protect := csrf.Protect(
key[:],
csrf.Path("/"),
csrf.Secure(!plaintext),
csrf.SameSite(csrf.SameSiteStrictMode),
csrf.ErrorHandler(handleFormRefused(plaintext, log)),
)
if !plaintext {
@@ -42,6 +68,84 @@ func newCSRFProtect(
}
}
// handleFormRefused returns the handler the CSRF middleware calls when it
// refuses a request. It logs the library's reason with the request ID and
// answers 403 with a page naming the cause and what to do. A reason that is
// not a refusal, such as a failure to make a token, answers 500.
func handleFormRefused(plaintext bool, log *slog.Logger) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
reason := csrf.FailureReason(r)
requestID := middleware.GetReqID(r.Context())
cause := formRefusedCause(r, reason, plaintext)
if cause == "" {
log.Error("CSRF middleware failed",
"request_id", requestID, "error", reason)
http.Error(w, "Internal server error",
http.StatusInternalServerError)
return
}
log.Warn("form refused", "request_id", requestID, "reason", reason)
var page bytes.Buffer
err := templates.Render(&page, "form_refused.html",
struct{ Cause string }{Cause: cause})
if err != nil {
log.Error("failed to render form refused template", "error", err)
http.Error(w, "Internal server error",
http.StatusInternalServerError)
return
}
w.Header().Set("Content-Type", "text/html; charset=utf-8")
w.WriteHeader(http.StatusForbidden)
_, err = page.WriteTo(w)
if err != nil {
log.Error("failed to write form refused page", "error", err)
}
})
}
// formRefusedCause returns the cause the form-refused page names for the
// CSRF middleware's reason, or "" when the reason is not a refusal.
func formRefusedCause(r *http.Request, reason error, plaintext bool) string {
switch {
case errors.Is(reason, csrf.ErrNoToken), errors.Is(reason, csrf.ErrBadToken):
return causeStaleToken
case errors.Is(reason, csrf.ErrNoReferer):
return causeNoReferer
case errors.Is(reason, csrf.ErrBadOrigin):
return pageAddressCause(r.Header.Get("Origin"), r.Host, plaintext)
case errors.Is(reason, csrf.ErrBadReferer):
return pageAddressCause(r.Referer(), r.Host, plaintext)
default:
return ""
}
}
// pageAddressCause returns the cause for a form whose page address, the
// Origin or Referer header, the CSRF middleware refused. The middleware
// expects the request's own host, with https when plaintext is false and
// http when it is true. So on the request's own host, only the scheme was
// wrong.
func pageAddressCause(pageAddress, host string, plaintext bool) string {
page, err := url.Parse(pageAddress)
if err != nil || page.Host != host {
return causeOtherHost
}
if plaintext {
return causeHTTPSWithDebug
}
return causePlainHTTP
}
// CSRF returns the CSRF-protection middleware for the login and generator
// form routes.
func (s *Handlers) CSRF() func(http.Handler) http.Handler {
@@ -0,0 +1,205 @@
package handlers
import (
"bytes"
"context"
"encoding/json"
"html/template"
"log/slog"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"github.com/go-chi/chi/v5/middleware"
"github.com/gorilla/csrf"
"sneak.berlin/go/pixa/internal/seal"
)
// refusedFormRequestID is the request ID of the requests postRefusedForm
// sends, which the warning for a refused form must carry.
const refusedFormRequestID = "test-request-id"
// TestFormRefusedNamesCause verifies that a form the CSRF middleware refuses
// answers 403 with a page naming the cause, and logs a warning with the
// request ID and the middleware's reason.
func TestFormRefusedNamesCause(t *testing.T) {
t.Parallel()
tests := []struct {
name string
debug bool
origin string
referer string
wantCause string
wantReason error
}{
{
name: "plain HTTP origin with debug off",
origin: "http://example.com",
wantCause: causePlainHTTP,
wantReason: csrf.ErrBadOrigin,
},
{
name: "plain HTTP referer with debug off",
referer: "http://example.com/",
wantCause: causePlainHTTP,
wantReason: csrf.ErrBadReferer,
},
{
name: "origin on another host",
origin: "https://proxy.example.net",
wantCause: causeOtherHost,
wantReason: csrf.ErrBadOrigin,
},
{
name: "referer on another host",
referer: "https://proxy.example.net/",
wantCause: causeOtherHost,
wantReason: csrf.ErrBadReferer,
},
{
name: "HTTPS origin with debug on",
debug: true,
origin: "https://example.com",
wantCause: causeHTTPSWithDebug,
wantReason: csrf.ErrBadOrigin,
},
{
name: "no origin or referer with debug off",
wantCause: causeNoReferer,
wantReason: csrf.ErrNoReferer,
},
{
name: "missing token",
debug: true,
origin: "http://example.com",
wantCause: causeStaleToken,
wantReason: csrf.ErrNoToken,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
rec, logLine := postRefusedForm(t, tt.debug, tt.origin, tt.referer,
nil, "")
checkFormRefused(t, rec, logLine, tt.wantCause, tt.wantReason)
})
}
}
// TestFormRefusedNamesCauseForForeignToken verifies that a form whose token
// does not match its CSRF cookie answers 403 with a page naming
// causeStaleToken, and logs a warning with the request ID and the
// middleware's reason.
func TestFormRefusedNamesCauseForForeignToken(t *testing.T) {
t.Parallel()
// The router and postRefusedForm derive their CSRF key from
// testSigningKey, so cookiesA is valid there and only tokenB is wrong.
_, router := newCSRFTestRouter(t)
cookiesA, _ := csrfCredentials(t, router, nil)
_, tokenB := csrfCredentials(t, router, nil)
rec, logLine := postRefusedForm(t, true, "http://example.com", "",
cookiesA, tokenB)
checkFormRefused(t, rec, logLine, causeStaleToken, csrf.ErrBadToken)
}
// postRefusedForm sends a POST with the given cookies to the CSRF
// middleware built for debug. It adds the form token and the Origin and
// Referer headers when they are not empty. It returns the response and the
// one JSON log line the middleware wrote.
func postRefusedForm(
t *testing.T, debug bool, origin, referer string,
cookies []*http.Cookie, token string,
) (*httptest.ResponseRecorder, map[string]any) {
t.Helper()
keys, err := seal.DeriveKeys(testSigningKey)
if err != nil {
t.Fatalf("seal.DeriveKeys() error = %v", err)
}
var logs bytes.Buffer
log := slog.New(slog.NewJSONHandler(&logs, nil))
reached := http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) {
t.Error("the refused form reached the handler behind the middleware")
})
srv := newCSRFProtect(keys.CSRFKey, debug, log)(reached)
form := url.Values{loginKeyField: {"x"}}
if token != "" {
form.Set(csrfTokenField, token)
}
ctx := context.WithValue(context.Background(),
middleware.RequestIDKey, refusedFormRequestID)
req := httptest.NewRequestWithContext(ctx, http.MethodPost,
"http://example.com/", strings.NewReader(form.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
for _, c := range cookies {
req.AddCookie(c)
}
if origin != "" {
req.Header.Set("Origin", origin)
}
if referer != "" {
req.Header.Set("Referer", referer)
}
rec := httptest.NewRecorder()
srv.ServeHTTP(rec, req)
var logLine map[string]any
err = json.Unmarshal(logs.Bytes(), &logLine)
if err != nil {
t.Fatalf("decoding log line %q: %v", logs.String(), err)
}
return rec, logLine
}
// checkFormRefused checks that rec is a 403 page naming wantCause and that
// logLine is a warning carrying the request ID and wantReason.
func checkFormRefused(
t *testing.T, rec *httptest.ResponseRecorder, logLine map[string]any,
wantCause string, wantReason error,
) {
t.Helper()
if rec.Code != http.StatusForbidden {
t.Errorf("status = %d, want %d", rec.Code, http.StatusForbidden)
}
body := rec.Body.String()
t.Logf("body: %s", body)
if !strings.Contains(body, template.HTMLEscapeString(wantCause)) {
t.Errorf("page does not name the cause %q", wantCause)
}
t.Logf("log line: %v", logLine)
if logLine["level"] != "WARN" || logLine["msg"] != "form refused" {
t.Errorf("log line = %v, want a WARN \"form refused\"", logLine)
}
if logLine["request_id"] != refusedFormRequestID {
t.Errorf("logged request_id = %v, want %q",
logLine["request_id"], refusedFormRequestID)
}
if logLine["reason"] != wantReason.Error() {
t.Errorf("logged reason = %v, want %q", logLine["reason"], wantReason)
}
}
+4 -2
View File
@@ -56,14 +56,16 @@ type Handlers struct {
// New creates a new Handlers instance.
func New(_ fx.Lifecycle, params Params) *Handlers {
log := params.Logger.Get()
return &Handlers{
log: params.Logger.Get(),
log: log,
hc: params.Healthcheck,
config: params.Config,
imgSvc: params.ImageService,
sessMgr: params.SessionManager,
encGen: params.EncryptedURLGenerator,
csrfProtect: newCSRFProtect(params.Keys.CSRFKey, params.Config.Debug),
csrfProtect: newCSRFProtect(params.Keys.CSRFKey, params.Config.Debug, log),
signer: signature.New(params.Config.SigningKey),
allowlist: allowlist.New(params.Config.AllowlistHosts),
refererBlocklist: allowlist.New(params.Config.RefererBlocklist),
+22
View File
@@ -0,0 +1,22 @@
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Pixa - Form Refused</title>
<link rel="stylesheet" href="/static/style.css">
</head>
<body class="login">
<div class="card">
<h1>Form Refused</h1>
<div class="error">
{{.Cause}}
</div>
<p>
<a href="/">Back to the start page</a>
</p>
</div>
</body>
</html>