Author SHA1 Message Date
clawbot 4de726f109 Abort startup on a config file pixa cannot read (closes #176)
check / check (push) Waiting to run
Of the places pixa looks for its config file on its own, it passed over
any place where os.Stat failed, so a file in a directory pixa may not
enter was skipped without a word and pixa started on a later file or on
the environment and defaults. Now only a file that does not exist is
passed over; any other error aborts startup naming the file, as a file
that does not parse already did. README.md says so where it gives the
search order.

Model: opus-5-5
2026-10-04 11:12:48 +00:00
clawbot daa24d8b74 Test that a config file pixa may not read aborts startup
Of the places pixa looks for its config file on its own, a file in a
directory pixa may not enter is passed over today and pixa starts
without it. This test puts the config file in such a directory and
expects startup to abort with an error naming the file. It is skipped
when run as root, which may enter any directory.

Model: opus-5-5
2026-10-04 11:12:48 +00:00
clawbot c7173c47d8 Return and pass on request IDs, and give /v1/e/ ETag, 304 and HEAD (closes #84)
check / check (push) Waiting to run
Every response carries X-Request-Id, the upstream fetch sends it, and
the "upstream fetched", "image converted" and "image served" lines log
it as request_id. pixa's own RequestID middleware keeps a request's own
ID only when it is at most 64 letters, digits, '-', '_' or '.', and
otherwise makes a random one with crypto/rand, so nothing a client
chooses freely and nothing about the host reaches upstream. /v1/e/ now
sets ETag, answers a matching If-None-Match with 304 and is routed for
HEAD, through notModified, which both image handlers call. No Vary is
added: go-chi/cors already sends Vary: Origin.

Model: opus-5-5
2026-10-04 12:41:54 +02:00
20 changed files with 700 additions and 381 deletions
+19 -7
View File
@@ -120,8 +120,9 @@ path under `/v1/` answers 200, in maintenance mode too.
`blocked_networks`); 502 when the upstream answered with an error status, and
for 5 minutes after that for the same source URL; 503 when pixa is busy or in
maintenance mode; 500 for any other failure.
- `GET /v1/e/<token>/<name>` — an image through an encrypted URL (see Encrypted
URLs). Needs: nothing but the URL. Answers: 200; 400 for a token that does not
- `GET` or `HEAD` `/v1/e/<token>/<name>` — an image through an encrypted URL
(see Encrypted URLs). Needs: nothing but the URL. Answers: 200; 304 when
`If-None-Match` matches the image's `ETag`; 400 for a token that does not
decrypt, or that asks for a size or fit that is not valid; 410 once it has
expired; 504 when the upstream has not sent its response headers within
`upstream_fetch_timeout`, but 500 when that time runs out while the image
@@ -137,6 +138,15 @@ path under `/v1/` answers 200, in maintenance mode too.
authentication with `metrics.username` and `metrics.password`. Answers: 200;
401 without them; 404 when they are not set, as the route then does not exist.
Every response carries an `X-Request-ID` header holding the request's ID, which
a client can quote when reporting a problem: the request's own `X-Request-ID`,
as a reverse proxy in front of pixa may send, when it is at most 64 letters,
digits, `-`, `_` or `.`; otherwise a random one pixa makes for the request,
which tells nothing about the machine or the other requests. pixa's log line for
the request carries the same ID as `request_id`, and so do the lines it logs
when it fetches, converts and serves an image; the fetch sends it to the
upstream host as `X-Request-ID`.
Both `POST` routes accept only a form that pixa's own page served: the page puts
a token in the form and sets a cookie to match, and a request without both is
refused with 403, so another site cannot submit the form from a visitor's
@@ -186,7 +196,9 @@ source) and one transcode: the first request does the work, and the others wait
for its image or its error, holding no upstream connection or processing slot
of their own. A waiting request stops waiting when its own client goes away.
The work goes on for the others even if the first request's client goes away,
until that request's `downstream_timeout` ends.
until that request's `downstream_timeout` ends. The shared fetch sends the first
request's ID upstream, and the lines logged for the fetch and the transcode
carry that ID.
The login form (`POST /`) is limited to 5 attempts per minute per client
address, counting an IPv6 client by its /64; an attempt over the limit is
@@ -342,10 +354,10 @@ pixa finds: `/etc/pixa/config.yml`, `/etc/pixa/config.yaml`,
`~/.config/pixa/config.yml`, `~/.config/pixa/config.yaml`, then `config.yml`
and `config.yaml` in the working directory. A named file that does not exist,
cannot be read or does not parse aborts startup. Of the files pixa looks for on
its own, one it finds but cannot read or parse aborts startup; one it cannot
find, for any reason, is passed over without a message, even when the file is
there in a directory pixa may not enter. With no file, pixa uses the environment
and the defaults.
its own, only one that does not exist is passed over, without a message. One
that pixa cannot read or parse aborts startup, naming the file. So does one in a
directory pixa may not enter, whether or not it is there, since pixa cannot
tell. With no file, pixa uses the environment and the defaults.
| Variable | Config key | Meaning |
| ------------------------------------ | ------------------------------- | ---------------------------------------------------------------------------- |
+19 -16
View File
@@ -29,21 +29,25 @@ P2: security: referer blacklist
# Completed Steps
- 2026-10-04 the metrics basic auth, CORS preflight, request logging and
metrics recording have tests (closes #79): `MetricsAuth` on its own answers
401 with a challenge without credentials or with a wrong username or password
and lets the configured ones through; a preflight request gets `*` for any
origin when `access_control_allow_origin` is `*` and no
`Access-Control-Allow-Origin` from another origin than the configured one; a
`POST /` carrying the signing key leaves no trace of it in the request log
line, and the login handler's own log lines leave out the submitted key; the
metrics middleware on its own records a request it served, and the router
records nothing while no metrics username is set. Not tested: that the router
puts the basic auth in front of `/metrics` and records requests when a
metrics username is set. Only one test per package can set up `/metrics`, and
in `internal/server` that is `TestMaintenanceModeKeepsOtherRoutes`, which
needs the owner's approval to change; #180 holds it. Tests only; the basic
auth library already compares the password in constant time.
- 2026-10-04 a config file pixa cannot read aborts startup (closes #176): of the
places pixa looks for its config file on its own, only one where the file does
not exist is passed over; any other error, such as a directory on the path
that pixa may not enter, aborts startup naming the file, as a file that does
not parse already did.
- 2026-10-04 request IDs returned and passed on, and `/v1/e/` revalidates
(closes #84): pixa's own `RequestID` middleware, in place of chi's, gives each
request an ID, its own `X-Request-ID` when that is at most 64 letters, digits,
`-`, `_` or `.` and a random one otherwise, stores it where chi's did and
sends it back as `X-Request-ID` on every response; the upstream fetch sends
that ID, and the "upstream fetched", "image converted" and "image served" log
lines carry it as `request_id`, a fetch shared by several requests carrying
the first request's; `/v1/e/` sets `ETag`, answers a matching `If-None-Match`
with 304 and is routed for `HEAD`, the `ETag` and 304 code being
`notModified`, which `/v1/image/` calls too; its token checks moved unchanged
into `parseImageEncRequest` to keep `HandleImageEnc` within the line limit; no
`Vary` is added, as no response depends on a request header except the image
routes' CORS headers, for which `go-chi/cors` already sends `Vary: Origin`;
`Vary: Accept` is left to #88.
- 2026-10-04 routes, encrypted URLs and config file documented (closes #75):
"Routes" in `README.md` lists every route with its method, purpose, what it
needs and the status codes it answers with, and says `q` and `fit` are part
@@ -453,7 +457,6 @@ P2: security: referer blacklist
- P2: HTTP response handling
- Last-Modified headers
- Vary header for content negotiation
- X-Request-ID propagation
- P2: auto format selection (format=auto based on Accept header)
- P2: configuration
- YAML config file support
+18 -11
View File
@@ -4,6 +4,7 @@ package config
import (
"errors"
"fmt"
"io/fs"
"log/slog"
"math"
"net/netip"
@@ -778,19 +779,25 @@ func loadConfigFile(log *slog.Logger, appName string) (*smartconfig.Config, erro
for _, path := range configPaths {
cleanPath := filepath.Clean(path)
// Only a config file that does not exist is skipped. One that
// cannot be read or does not parse is a fatal startup error.
_, statErr := os.Stat(cleanPath)
if statErr == nil {
// A config file that exists but does not parse is a fatal
// startup error, never something to skip over.
sc, err := smartconfig.NewFromConfigPath(path)
if err != nil {
return nil, fmt.Errorf("failed to parse config file %s: %w", path, err)
}
log.Info("loaded config file", "path", path)
return sc, nil
if errors.Is(statErr, fs.ErrNotExist) {
continue
}
if statErr != nil {
return nil, fmt.Errorf("failed to read config file %s: %w", path, statErr)
}
sc, err := smartconfig.NewFromConfigPath(path)
if err != nil {
return nil, fmt.Errorf("failed to parse config file %s: %w", path, err)
}
log.Info("loaded config file", "path", path)
return sc, nil
}
return nil, nil //nolint:nilnil // nil config is valid (use defaults)
@@ -564,6 +564,60 @@ func TestMalformedConfigFileAbortsStartup(t *testing.T) {
t.Logf("got expected error: %v", err)
}
// TestConfigFileInDirectoryPixaMayNotEnterAbortsStartup checks that a
// config file pixa cannot read because it may not enter its directory
// aborts startup instead of being passed over.
func TestConfigFileInDirectoryPixaMayNotEnterAbortsStartup(t *testing.T) {
if os.Geteuid() == 0 {
t.Skip("root may enter any directory")
}
home := t.TempDir()
configDir := filepath.Join(home, ".config", "pixa-test-nonexistent-app")
configPath := filepath.Join(configDir, "config.yml")
err := os.MkdirAll(configDir, 0o700)
if err != nil {
t.Fatalf("failed to create config directory: %v", err)
}
err = os.WriteFile(configPath, []byte(signingKeyLine), 0o600)
if err != nil {
t.Fatalf("failed to write config: %v", err)
}
err = os.Chmod(configDir, 0)
if err != nil {
t.Fatalf("failed to remove the config directory's permissions: %v", err)
}
// Give the directory back its permissions so t.TempDir can remove it.
t.Cleanup(func() {
//nolint:gosec // G302: a directory needs its execute bit to be removed
_ = os.Chmod(configDir, 0o700)
})
// The ~/.config candidate is the only one that exists: the appname
// rules out /etc, and the working directory is empty.
t.Setenv("PIXA_CONFIG_PATH", "")
t.Setenv("HOME", home)
t.Chdir(t.TempDir())
log := slog.New(slog.DiscardHandler)
sc, err := loadConfigFile(log, "pixa-test-nonexistent-app")
if err == nil {
t.Fatalf("config file pixa cannot read must abort startup, got config: %v",
sc)
}
t.Logf("got expected error: %v", err)
if !strings.Contains(err.Error(), configPath) {
t.Errorf("error %q does not name the config file %s", err.Error(), configPath)
}
}
func TestEnsureStateDirCreatesDirectory(t *testing.T) {
t.Parallel()
@@ -1,59 +0,0 @@
package handlers
import (
"bytes"
"log/slog"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"sneak.berlin/go/pixa/internal/config"
"sneak.berlin/go/pixa/internal/session"
)
// TestLoginLogLeavesOutSubmittedKey verifies that the log lines for a
// failed and for a successful login do not contain the submitted key.
func TestLoginLogLeavesOutSubmittedKey(t *testing.T) {
t.Parallel()
const wrongKey = "wrong-signing-key-fedcba9876543210"
var buf bytes.Buffer
sessMgr, err := session.NewManager(testSigningKey)
if err != nil {
t.Fatalf("session.NewManager() error = %v", err)
}
h := &Handlers{
log: slog.New(slog.NewJSONHandler(&buf, nil)),
config: &config.Config{SigningKey: testSigningKey},
sessMgr: sessMgr,
}
submittedKeys := []string{wrongKey, testSigningKey}
for _, key := range submittedKeys {
form := url.Values{loginKeyField: {key}}
req := httptest.NewRequestWithContext(
t.Context(), http.MethodPost, "/",
strings.NewReader(form.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
h.handleLoginPost(httptest.NewRecorder(), req)
}
for _, msg := range []string{"failed login attempt", "successful login"} {
if !strings.Contains(buf.String(), msg) {
t.Fatalf("log missing %q; got %q", msg, buf.String())
}
}
for _, key := range submittedKeys {
if strings.Contains(buf.String(), key) {
t.Errorf("log contains submitted key %q; got %q", key, buf.String())
}
}
}
+23 -11
View File
@@ -10,6 +10,7 @@ import (
"time"
"github.com/go-chi/chi/v5"
"github.com/go-chi/chi/v5/middleware"
"sneak.berlin/go/pixa/internal/encurl"
"sneak.berlin/go/pixa/internal/httpfetcher"
"sneak.berlin/go/pixa/internal/imageprocessor"
@@ -247,6 +248,25 @@ func cacheControl(expires time.Time) string {
return fmt.Sprintf("public, max-age=%d, immutable", int64(maxAge/time.Second))
}
// notModified sets the ETag header to etag and, when the request's
// If-None-Match is that ETag, answers 304 Not Modified. It reports whether it
// answered. An empty etag sets no header and never answers.
func notModified(w http.ResponseWriter, r *http.Request, etag string) bool {
if etag == "" {
return false
}
w.Header().Set("ETag", etag)
if r.Header.Get("If-None-Match") != etag {
return false
}
w.WriteHeader(http.StatusNotModified)
return true
}
// writeImageResponse writes headers and streams the image content,
// handling conditional and HEAD requests.
func (s *Handlers) writeImageResponse(
@@ -265,17 +285,8 @@ func (s *Handlers) writeImageResponse(
w.Header().Set("Cache-Control", cacheControl(req.Expires))
w.Header().Set("X-Pixa-Cache", string(resp.CacheStatus))
if resp.ETag != "" {
w.Header().Set("ETag", resp.ETag)
// Check for conditional request (If-None-Match)
if ifNoneMatch := r.Header.Get("If-None-Match"); ifNoneMatch != "" {
if ifNoneMatch == resp.ETag {
w.WriteHeader(http.StatusNotModified)
return
}
}
if notModified(w, r, resp.ETag) {
return
}
// Handle HEAD request - return headers only
@@ -298,6 +309,7 @@ func (s *Handlers) writeImageResponse(
// Log cache status and timing after serving
duration := time.Since(startTime)
s.log.Info("image served",
"request_id", middleware.GetReqID(r.Context()),
"cache_key", cacheKey,
"cache_status", resp.CacheStatus,
"duration_ms", duration.Milliseconds(),
@@ -23,8 +23,10 @@ const photoPath = "/images/photo.jpg"
// newSignedHostServer returns a router for both image routes, and the Handlers
// behind it, whose fetcher serves a JPEG at photoPath on signedHost. signedHost
// is not on the allowlist, so a /v1/image/ URL for it is served only with a
// valid signature.
func newSignedHostServer(t *testing.T) (*Handlers, http.Handler) {
// valid signature. The handlers and the image service log to log.
func newSignedHostServer(
t *testing.T, log *slog.Logger,
) (*Handlers, http.Handler) {
t.Helper()
cache, err := imgcache.NewCache(setupTestDB(t), imgcache.CacheConfig{
@@ -44,6 +46,7 @@ func newSignedHostServer(t *testing.T) (*Handlers, http.Handler) {
signedHost + photoPath: &fstest.MapFile{Data: jpegData},
}),
SigningKey: testSigningKey,
Logger: log,
})
if err != nil {
t.Fatalf("imgcache.NewService() error = %v", err)
@@ -55,7 +58,7 @@ func newSignedHostServer(t *testing.T) (*Handlers, http.Handler) {
}
h := &Handlers{
log: slog.New(slog.DiscardHandler),
log: log,
imgSvc: svc,
encGen: encGen,
}
@@ -103,7 +106,7 @@ func getMaxAge(t *testing.T, srv http.Handler, target string) int {
func TestHandleImage_SignedURL_MaxAgeEndsAtExp(t *testing.T) {
t.Parallel()
h, srv := newSignedHostServer(t)
h, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler))
signedURL, err := h.imgSvc.GenerateSignedURL("", &imgcache.ImageRequest{
SourceHost: signedHost,
@@ -179,7 +182,7 @@ func TestHandleImageEnc_MaxAge(t *testing.T) {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
h, srv := newSignedHostServer(t)
h, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler))
token, err := h.encGen.Generate(&encurl.Payload{
SourceHost: signedHost,
+65 -37
View File
@@ -9,6 +9,7 @@ import (
"time"
"github.com/go-chi/chi/v5"
"github.com/go-chi/chi/v5/middleware"
"sneak.berlin/go/pixa/internal/encurl"
"sneak.berlin/go/pixa/internal/httpfetcher"
@@ -24,43 +25,8 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc {
ctx := r.Context()
start := time.Now()
// Extract token from URL
token := chi.URLParam(r, "token")
if token == "" {
s.respondError(w, "missing token", http.StatusBadRequest)
return
}
// Decrypt and validate the payload
payload, err := s.encGen.Parse(token)
if err != nil {
if errors.Is(err, encurl.ErrExpired) {
s.log.Debug("encrypted URL expired", "error", err)
s.respondError(w, "URL has expired", http.StatusGone)
return
}
s.log.Debug("failed to decrypt URL", "error", err)
s.respondError(w, "invalid encrypted URL", http.StatusBadRequest)
return
}
// Convert payload to ImageRequest
req := payload.ToImageRequest()
// Apply the same dimension and fit-mode bounds as the plain image
// route: a sealed payload is trusted for its origin, not for staying
// within limits, so an over-limit size or unknown fit mode is a 400
// here rather than an out-of-memory or a 500 from the processor.
err = imgcache.ValidateImageRequest(req)
if err != nil {
s.log.Debug("encrypted URL failed validation", "error", err)
s.respondError(w, "invalid encrypted URL: "+err.Error(),
http.StatusBadRequest)
req, ok := s.parseImageEncRequest(w, r)
if !ok {
return
}
@@ -94,6 +60,17 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc {
w.Header().Set("Cache-Control", cacheControl(req.Expires))
w.Header().Set("X-Pixa-Cache", string(resp.CacheStatus))
if notModified(w, r, resp.ETag) {
return
}
// A HEAD request gets the headers only
if r.Method == http.MethodHead {
w.WriteHeader(http.StatusOK)
return
}
// Stream the response
written, err := io.Copy(w, resp.Content)
if err != nil {
@@ -105,6 +82,7 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc {
// Log completion
duration := time.Since(start)
s.log.Info("image served",
"request_id", middleware.GetReqID(ctx),
"cache_key", imgcache.CacheKey(req),
"host", req.SourceHost,
"path", req.SourcePath,
@@ -116,6 +94,56 @@ func (s *Handlers) HandleImageEnc() http.HandlerFunc {
}
}
// parseImageEncRequest decrypts the token of an encrypted image URL into an
// ImageRequest and checks it. On a token that is missing, does not decrypt,
// has expired or asks for something not valid, it writes an error response
// and returns false.
func (s *Handlers) parseImageEncRequest(
w http.ResponseWriter, r *http.Request,
) (*imgcache.ImageRequest, bool) {
// Extract token from URL
token := chi.URLParam(r, "token")
if token == "" {
s.respondError(w, "missing token", http.StatusBadRequest)
return nil, false
}
// Decrypt and validate the payload
payload, err := s.encGen.Parse(token)
if err != nil {
if errors.Is(err, encurl.ErrExpired) {
s.log.Debug("encrypted URL expired", "error", err)
s.respondError(w, "URL has expired", http.StatusGone)
return nil, false
}
s.log.Debug("failed to decrypt URL", "error", err)
s.respondError(w, "invalid encrypted URL", http.StatusBadRequest)
return nil, false
}
// Convert payload to ImageRequest
req := payload.ToImageRequest()
// Apply the same dimension and fit-mode bounds as the plain image
// route: a sealed payload is trusted for its origin, not for staying
// within limits, so an over-limit size or unknown fit mode is a 400
// here rather than an out-of-memory or a 500 from the processor.
err = imgcache.ValidateImageRequest(req)
if err != nil {
s.log.Debug("encrypted URL failed validation", "error", err)
s.respondError(w, "invalid encrypted URL: "+err.Error(),
http.StatusBadRequest)
return nil, false
}
return req, true
}
// handleImageError converts image service errors to HTTP responses.
func (s *Handlers) handleImageError(w http.ResponseWriter, err error) {
switch {
@@ -96,3 +96,70 @@ func TestHandleImageEnc_InvalidFitMode_Returns400(t *testing.T) {
t.Fatalf("status = %d, want %d", rec.Code, http.StatusBadRequest)
}
}
// TestHandleImageEnc_IfNoneMatch_Returns304 verifies that an image served
// through an encrypted URL carries an ETag, and that a request whose
// If-None-Match is that ETag is answered 304 Not Modified with no body.
func TestHandleImageEnc_IfNoneMatch_Returns304(t *testing.T) {
t.Parallel()
h, srv := newSignedHostServer(t, slog.New(slog.DiscardHandler))
target := encPhotoURL(t, h)
rec := httptest.NewRecorder()
srv.ServeHTTP(rec, httptest.NewRequestWithContext(
t.Context(), http.MethodGet, target, nil))
etag := rec.Header().Get("ETag")
t.Logf("GET: %d, ETag %q", rec.Code, etag)
if rec.Code != http.StatusOK || etag == "" {
t.Fatalf("GET: status = %d, ETag = %q, want %d and an ETag",
rec.Code, etag, http.StatusOK)
}
req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, target, nil)
req.Header.Set("If-None-Match", etag)
rec = httptest.NewRecorder()
srv.ServeHTTP(rec, req)
t.Logf("GET with If-None-Match: %d, %d body bytes", rec.Code, rec.Body.Len())
if rec.Code != http.StatusNotModified || rec.Body.Len() != 0 {
t.Errorf("status = %d with %d body bytes, want %d with none",
rec.Code, rec.Body.Len(), http.StatusNotModified)
}
}
// TestHandleImageEnc_HEAD_ReturnsHeadersOnly verifies that HEAD on an
// encrypted URL is answered 200 with the headers GET sends and no body.
func TestHandleImageEnc_HEAD_ReturnsHeadersOnly(t *testing.T) {
t.Parallel()
h, _ := newSignedHostServer(t, slog.New(slog.DiscardHandler))
r := chi.NewRouter()
r.Head("/v1/e/{token}/*", h.HandleImageEnc())
rec := httptest.NewRecorder()
r.ServeHTTP(rec, httptest.NewRequestWithContext(
t.Context(), http.MethodHead, encPhotoURL(t, h), nil))
t.Logf("HEAD: %d, headers %v, %d body bytes",
rec.Code, rec.Header(), rec.Body.Len())
if rec.Code != http.StatusOK {
t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK)
}
for _, name := range []string{
"Content-Type", "Content-Length", "Cache-Control", "ETag",
} {
if rec.Header().Get(name) == "" {
t.Errorf("HEAD response has no %s", name)
}
}
if rec.Body.Len() != 0 {
t.Errorf("HEAD response body has %d bytes, want none", rec.Body.Len())
}
}
@@ -0,0 +1,131 @@
package handlers
import (
"bytes"
"context"
"encoding/json"
"io"
"log/slog"
"net/http"
"net/http/httptest"
"testing"
"time"
"github.com/go-chi/chi/v5/middleware"
"sneak.berlin/go/pixa/internal/encurl"
"sneak.berlin/go/pixa/internal/imgcache"
)
// signedPhotoURL returns a signed /v1/image/ URL, valid for a minute, for the
// JPEG at photoPath on signedHost at 50x50, made with h's image service.
func signedPhotoURL(t *testing.T, h *Handlers) string {
t.Helper()
signedURL, err := h.imgSvc.GenerateSignedURL("", &imgcache.ImageRequest{
SourceHost: signedHost,
SourcePath: photoPath,
Size: imgcache.Size{Width: 50, Height: 50},
Format: imgcache.FormatJPEG,
}, time.Minute)
if err != nil {
t.Fatalf("GenerateSignedURL() error = %v", err)
}
return signedURL
}
// encPhotoURL returns an encrypted /v1/e/ URL, which never expires, for the
// JPEG at photoPath on signedHost at 50x50, made with h's generator.
func encPhotoURL(t *testing.T, h *Handlers) string {
t.Helper()
token, err := h.encGen.Generate(&encurl.Payload{
SourceHost: signedHost,
SourcePath: photoPath,
Width: 50,
Height: 50,
Format: imgcache.FormatJPEG,
})
if err != nil {
t.Fatalf("Generate() error = %v", err)
}
return "/v1/e/" + token + "/img.jpg"
}
// requestIDByMessage reads the JSON log lines in logs and returns the
// request_id of each line, by its message.
func requestIDByMessage(t *testing.T, logs io.Reader) map[string]string {
t.Helper()
logged := make(map[string]string)
dec := json.NewDecoder(logs)
for dec.More() {
var line map[string]any
err := dec.Decode(&line)
if err != nil {
t.Fatalf("decoding log line: %v", err)
}
msg, _ := line["msg"].(string)
requestID, _ := line["request_id"].(string)
logged[msg] = requestID
}
return logged
}
// TestImageLogLinesCarryRequestID verifies that the lines logged when an image
// is fetched, converted and served through either image route carry the
// request's ID as request_id, as the request log line does, so they can be
// found from it.
func TestImageLogLinesCarryRequestID(t *testing.T) {
t.Parallel()
const requestID = "test-request-id"
imageURLs := map[string]func(*testing.T, *Handlers) string{
"/v1/image/": signedPhotoURL,
"/v1/e/": encPhotoURL,
}
for route, imageURL := range imageURLs {
t.Run(route, func(t *testing.T) {
t.Parallel()
var logs bytes.Buffer
h, srv := newSignedHostServer(t,
slog.New(slog.NewJSONHandler(&logs, nil)))
ctx := context.WithValue(t.Context(),
middleware.RequestIDKey, requestID)
rec := httptest.NewRecorder()
srv.ServeHTTP(rec, httptest.NewRequestWithContext(
ctx, http.MethodGet, imageURL(t, h), nil))
if rec.Code != http.StatusOK {
t.Fatalf("status = %d, want %d", rec.Code, http.StatusOK)
}
t.Logf("logged:\n%s", logs.String())
logged := requestIDByMessage(t, &logs)
for _, msg := range []string{
"upstream fetched", "image converted", "image served",
} {
got, ok := logged[msg]
if !ok {
t.Errorf("no %q line logged", msg)
} else if got != requestID {
t.Errorf("%q line has request_id %q, want %q",
msg, got, requestID)
}
}
})
}
}
+9
View File
@@ -18,6 +18,8 @@ import (
"strings"
"sync"
"time"
"github.com/go-chi/chi/v5/middleware"
)
// Fetcher configuration constants.
@@ -267,6 +269,13 @@ func (f *HTTPFetcher) Fetch(ctx context.Context, url string) (*FetchResult, erro
req.Header.Set("User-Agent", f.config.UserAgent)
req.Header.Set("Accept", strings.Join(f.config.AllowedContentTypes, ", "))
// The ID of the request this fetch serves, so the fetch can be found in
// the upstream host's logs
requestID := middleware.GetReqID(ctx)
if requestID != "" {
req.Header.Set(middleware.RequestIDHeader, requestID)
}
// Use httptrace to capture connection details
var remoteAddr string
@@ -0,0 +1,50 @@
package httpfetcher
import (
"context"
"io"
"net/http"
"net/http/httptest"
"testing"
"github.com/go-chi/chi/v5/middleware"
)
// TestFetchSendsRequestID verifies that a fetch sends the ID of the request
// it serves, which the RequestID middleware stores in the request context,
// to the upstream host as X-Request-Id, so the fetch can be found in that
// host's logs.
func TestFetchSendsRequestID(t *testing.T) {
t.Parallel()
const requestID = "test-request-id"
received := make(chan string, 1)
srv := httptest.NewServer(http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
received <- r.Header.Get("X-Request-Id")
w.Header().Set("Content-Type", contentTypeJPEG)
_, _ = io.WriteString(w, imagePayload)
}))
t.Cleanup(srv.Close)
f, _ := newServerFetcher(t, srv, nil)
ctx := context.WithValue(testContext(t), middleware.RequestIDKey, requestID)
res, err := f.Fetch(ctx, upstreamURL("/image"))
if err != nil {
t.Fatalf("Fetch() error = %v", err)
}
_ = res.Content.Close()
got := <-received
t.Logf("upstream received X-Request-Id %q", got)
if got != requestID {
t.Errorf("upstream X-Request-Id = %q, want %q", got, requestID)
}
}
+3
View File
@@ -13,6 +13,7 @@ import (
"github.com/dustin/go-humanize"
"github.com/getsentry/sentry-go"
"github.com/go-chi/chi/v5/middleware"
"golang.org/x/sync/singleflight"
"sneak.berlin/go/pixa/internal/allowlist"
"sneak.berlin/go/pixa/internal/httpfetcher"
@@ -461,6 +462,7 @@ func (s *Service) fetchAndProcess(
// Log upstream fetch details
s.log.Info("upstream fetched",
"request_id", middleware.GetReqID(ctx),
"host", req.SourceHost,
"path", req.SourcePath,
"bytes", fetchBytes,
@@ -545,6 +547,7 @@ func (s *Service) processAndStore(
}
s.log.Info("image converted",
"request_id", middleware.GetReqID(ctx),
"host", req.SourceHost,
"path", req.SourcePath,
"src_format", processResult.InputFormat,
+29
View File
@@ -2,9 +2,12 @@
package middleware
import (
"context"
"crypto/rand"
"log/slog"
"net/http"
"net/netip"
"regexp"
"time"
basicauth "github.com/99designs/basicauth-go"
@@ -115,6 +118,32 @@ func (s *Middleware) RateLimit(
})
}
// requestIDPattern is what a request's own X-Request-Id must look like to be
// kept as its ID: 1 to 64 letters, digits, '-', '_' or '.'.
var requestIDPattern = regexp.MustCompile(`^[A-Za-z0-9._-]{1,64}$`)
// RequestID returns a middleware that gives each request an ID and sends it as
// the X-Request-Id response header, so a client can quote it when reporting a
// problem. The ID is the request's own X-Request-Id when that matches
// requestIDPattern, and otherwise a random one, which tells nothing about the
// machine or the traffic. It is stored in the request context under chi's
// RequestIDKey, where the logging middleware, the handlers and the upstream
// fetch read it.
func (s *Middleware) RequestID() func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
id := r.Header.Get(middleware.RequestIDHeader)
if !requestIDPattern.MatchString(id) {
id = rand.Text()
}
w.Header().Set(middleware.RequestIDHeader, id)
ctx := context.WithValue(r.Context(), middleware.RequestIDKey, id)
next.ServeHTTP(w, r.WithContext(ctx))
})
}
}
type loggingResponseWriter struct {
http.ResponseWriter
@@ -1,16 +1,11 @@
package middleware
import (
"bytes"
"log/slog"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"github.com/prometheus/client_golang/prometheus/promhttp"
"sneak.berlin/go/pixa/internal/config"
)
@@ -61,203 +56,6 @@ func TestCORSAnswersWithConfiguredOrigin(t *testing.T) {
}
}
// TestCORSAnswersPreflightWithConfiguredOrigin checks that the CORS
// middleware answers a preflight request, which the CORS library handles
// apart from other requests, the same way: "*" lets any origin read
// responses and a single origin lets only that origin read them.
func TestCORSAnswersPreflightWithConfiguredOrigin(t *testing.T) {
t.Parallel()
const appOrigin = "https://app.example.com"
cases := []struct {
configured string
requestOrigin string
want string
}{
{"*", "https://any.example.com", "*"},
{appOrigin, appOrigin, appOrigin},
{appOrigin, "https://other.example.com", ""},
}
testHandler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
})
for _, tc := range cases {
mw := &Middleware{
log: slog.Default(),
config: &config.Config{AccessControlAllowOrigin: tc.configured},
}
handler := mw.CORS()(testHandler)
// An OPTIONS request naming the method it asks about is the
// preflight a browser sends before some cross-origin requests.
req := httptest.NewRequestWithContext(
t.Context(), http.MethodOptions, "/v1/image/example.com/a.jpg/1x1.png", nil)
req.Header.Set("Origin", tc.requestOrigin)
req.Header.Set("Access-Control-Request-Method", http.MethodGet)
rec := httptest.NewRecorder()
handler.ServeHTTP(rec, req)
got := rec.Header().Get("Access-Control-Allow-Origin")
if got != tc.want {
t.Errorf("configured %q, preflight from %q: "+
"Access-Control-Allow-Origin = %q, want %q",
tc.configured, tc.requestOrigin, got, tc.want)
}
}
}
// TestMetricsAuthRequiresConfiguredCredentials checks that MetricsAuth on
// its own answers 401 with a challenge to a request without credentials or
// with a wrong username or password, and lets a request with the configured
// username and password through. That the router puts it in front of
// /metrics is not tested.
func TestMetricsAuthRequiresConfiguredCredentials(t *testing.T) {
t.Parallel()
const (
username = "metricsuser"
password = "metricspass"
challenge = `Basic realm="metrics"`
)
// An empty username stands for a request sent without credentials.
cases := []struct {
name string
username string
password string
wantReached bool
}{
{"no credentials", "", "", false},
{"wrong username", "someone", password, false},
{"wrong password", username, "wrongpass", false},
{"configured credentials", username, password, true},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
mw := &Middleware{
log: slog.Default(),
config: &config.Config{
MetricsUsername: username,
MetricsPassword: password,
},
}
reached := false
handler := mw.MetricsAuth()(http.HandlerFunc(
func(http.ResponseWriter, *http.Request) {
reached = true
}))
req := httptest.NewRequestWithContext(
t.Context(), http.MethodGet, "/metrics", nil)
if tc.username != "" {
req.SetBasicAuth(tc.username, tc.password)
}
rec := httptest.NewRecorder()
handler.ServeHTTP(rec, req)
if reached != tc.wantReached {
t.Fatalf("request reached /metrics = %v, want %v",
reached, tc.wantReached)
}
if tc.wantReached {
return
}
if rec.Code != http.StatusUnauthorized {
t.Errorf("status = %d, want %d",
rec.Code, http.StatusUnauthorized)
}
if got := rec.Header().Get("WWW-Authenticate"); got != challenge {
t.Errorf("WWW-Authenticate = %q, want %q", got, challenge)
}
})
}
}
// TestMetricsRecordsServedRequest checks that the metrics middleware
// records a request it served, so /metrics reports it. It is the only test
// in this package that sets up the metrics middleware, which registers with
// the process-wide Prometheus registry and can do so only once.
func TestMetricsRecordsServedRequest(t *testing.T) {
t.Parallel()
// The line /metrics shows once one GET /test has been served.
const want = `http_request_duration_seconds_count{` +
`code="200",handler="/test",method="GET",service=""} 1`
mw := &Middleware{log: slog.Default(), config: &config.Config{}}
handler := mw.Metrics()(http.HandlerFunc(
func(w http.ResponseWriter, _ *http.Request) {
w.WriteHeader(http.StatusOK)
}))
handler.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(
t.Context(), http.MethodGet, "/test", nil))
rec := httptest.NewRecorder()
promhttp.Handler().ServeHTTP(rec, httptest.NewRequestWithContext(
t.Context(), http.MethodGet, "/metrics", nil))
if !strings.Contains(rec.Body.String(), want) {
t.Errorf("/metrics does not report the GET /test served; "+
"want the line %q in:\n%s", want, rec.Body.String())
}
}
// TestLoggingLeavesOutSubmittedSigningKey checks that a login, a POST /
// whose form carries the signing key, leaves no trace of the key in the
// request's log line.
func TestLoggingLeavesOutSubmittedSigningKey(t *testing.T) {
t.Parallel()
const signingKey = "test-signing-key-0123456789abcdef"
var buf bytes.Buffer
mw := newTestMiddleware(t, &buf)
// The handler reads the key from the form, as the login handler does.
handler := mw.Logging()(http.HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
if got := r.FormValue("key"); got != signingKey {
t.Errorf("key in form = %q, want %q", got, signingKey)
}
w.WriteHeader(http.StatusSeeOther)
}))
form := url.Values{"key": {signingKey}}
req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/",
strings.NewReader(form.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
handler.ServeHTTP(httptest.NewRecorder(), req)
if !strings.Contains(buf.String(), `"method":"POST"`) {
t.Fatalf("no log line for the request; got %q", buf.String())
}
if strings.Contains(buf.String(), signingKey) {
t.Errorf("log output contains the signing key; got %q", buf.String())
}
}
func TestSecurityHeaders(t *testing.T) {
t.Parallel()
@@ -0,0 +1,118 @@
package middleware
import (
"log/slog"
"net/http"
"net/http/httptest"
"os"
"strings"
"testing"
"github.com/go-chi/chi/v5/middleware"
"sneak.berlin/go/pixa/internal/config"
)
// sendRequestID sends a request through the RequestID middleware, carrying
// incoming as its X-Request-Id unless that is empty. It returns the ID the
// next handler found in the request context, which the upstream fetch sends
// and the log lines carry, and the X-Request-Id of the response.
func sendRequestID(t *testing.T, incoming string) (string, string) {
t.Helper()
mw := &Middleware{log: slog.Default(), config: &config.Config{}}
var inContext string
handler := mw.RequestID()(http.HandlerFunc(
func(_ http.ResponseWriter, r *http.Request) {
inContext = middleware.GetReqID(r.Context())
}))
req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/", nil)
if incoming != "" {
req.Header.Set("X-Request-Id", incoming)
}
rec := httptest.NewRecorder()
handler.ServeHTTP(rec, req)
return inContext, rec.Header().Get("X-Request-Id")
}
// TestRequestIDKeepsShortPlainID verifies that a request's own X-Request-Id of
// at most 64 letters, digits, '-', '_' or '.' is kept as its ID.
func TestRequestIDKeepsShortPlainID(t *testing.T) {
t.Parallel()
for _, incoming := range []string{
"client-request-id",
"A1_b2.c3-d4",
strings.Repeat("a", 64),
} {
inContext, inResponse := sendRequestID(t, incoming)
if inContext != incoming || inResponse != incoming {
t.Errorf("incoming %q: context has %q, response %q, want both %q",
incoming, inContext, inResponse, incoming)
}
}
}
// TestRequestIDReplacesLongOrUnusualID verifies that a request's own
// X-Request-Id that is over 64 characters or holds anything but letters,
// digits, '-', '_' or '.' is neither sent back nor sent upstream: the request
// gets a fresh ID instead.
func TestRequestIDReplacesLongOrUnusualID(t *testing.T) {
t.Parallel()
for _, incoming := range []string{
strings.Repeat("a", 65),
strings.Repeat("a", 9000),
"has space",
"a/b",
"a,b",
"<script>",
"ünicode",
} {
inContext, inResponse := sendRequestID(t, incoming)
t.Logf("incoming %.20q: made up %q", incoming, inResponse)
if inResponse == "" || inResponse == incoming {
t.Errorf("incoming %.20q: response has %q, want a fresh ID",
incoming, inResponse)
}
if inContext != inResponse {
t.Errorf("incoming %.20q: context has %q, want the response's %q",
incoming, inContext, inResponse)
}
}
}
// TestRequestIDMadeUpTellsNothing verifies that the ID made up for a request
// that sent none differs for every request and does not hold the host name.
func TestRequestIDMadeUpTellsNothing(t *testing.T) {
t.Parallel()
hostname, err := os.Hostname()
if err != nil {
t.Fatalf("os.Hostname() error = %v", err)
}
firstInContext, first := sendRequestID(t, "")
_, second := sendRequestID(t, "")
t.Logf("host %q, made up %q and %q", hostname, first, second)
if first == "" || first == second {
t.Errorf("made up %q and %q, want two different IDs", first, second)
}
if firstInContext != first {
t.Errorf("context has %q, want the response's %q", firstInContext, first)
}
if strings.Contains(first, hostname) {
t.Errorf("made-up ID %q holds the host name %q", first, hostname)
}
}
-32
View File
@@ -1,32 +0,0 @@
package server
import (
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/prometheus/client_golang/prometheus/promhttp"
)
// TestNoMetricsRecordedWithoutMetricsUsername checks that with no metrics
// username set the router records nothing about the requests it serves.
// /metrics is not served then, so the process-wide Prometheus registry is
// read directly. TestMaintenanceModeKeepsOtherRoutes records into the same
// registry, but never a GET /robots.txt.
func TestNoMetricsRecordedWithoutMetricsUsername(t *testing.T) {
t.Parallel()
s := newTestServer(t)
s.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(
t.Context(), http.MethodGet, "/robots.txt", nil))
rec := httptest.NewRecorder()
promhttp.Handler().ServeHTTP(rec, httptest.NewRequestWithContext(
t.Context(), http.MethodGet, "/metrics", nil))
if strings.Contains(rec.Body.String(), `handler="/robots.txt"`) {
t.Errorf("with no metrics username GET /robots.txt was recorded:\n%s",
rec.Body.String())
}
}
@@ -0,0 +1,58 @@
package server
import (
"net/http"
"net/http/httptest"
"testing"
)
// requestIDHeader is the header that carries a request's ID.
const requestIDHeader = "X-Request-Id"
// TestResponsesCarryRequestID verifies that every response, whatever its route
// and status, carries the request's ID as X-Request-Id, so a client can quote
// it when reporting a problem: one pixa made up when the request brought none,
// and the request's own X-Request-Id when it brought one.
func TestResponsesCarryRequestID(t *testing.T) {
t.Parallel()
const clientRequestID = "client-request-id"
s := newTestServer(t)
paths := []string{
"/robots.txt",
"/no-such-path",
unsignedImagePath,
encryptedImagePath,
}
for _, path := range paths {
t.Run(path, func(t *testing.T) {
t.Parallel()
rec := httptest.NewRecorder()
s.ServeHTTP(rec, httptest.NewRequestWithContext(
t.Context(), http.MethodGet, path, nil))
t.Logf("status %d, %s %q",
rec.Code, requestIDHeader, rec.Header().Get(requestIDHeader))
if rec.Header().Get(requestIDHeader) == "" {
t.Errorf("response has no %s", requestIDHeader)
}
req := httptest.NewRequestWithContext(
t.Context(), http.MethodGet, path, nil)
req.Header.Set(requestIDHeader, clientRequestID)
rec = httptest.NewRecorder()
s.ServeHTTP(rec, req)
got := rec.Header().Get(requestIDHeader)
if got != clientRequestID {
t.Errorf("%s = %q, want the request's own %q",
requestIDHeader, got, clientRequestID)
}
})
}
}
+2 -1
View File
@@ -28,7 +28,7 @@ func (s *Server) SetupRoutes() {
s.router = chi.NewRouter()
s.router.Use(middleware.Recoverer)
s.router.Use(middleware.RequestID)
s.router.Use(s.mw.RequestID())
s.router.Use(s.mw.ClientIP())
s.router.Use(s.mw.SecurityHeaders())
s.router.Use(s.mw.Logging())
@@ -97,6 +97,7 @@ func (s *Server) SetupRoutes() {
// The trailing filename (e.g., /img.jpg) is ignored but helps
// browsers with content type
r.Get("/e/{token}/*", s.h.HandleImageEnc())
r.Head("/e/{token}/*", s.h.HandleImageEnc())
})
})
+27
View File
@@ -0,0 +1,27 @@
package server
import (
"net/http"
"net/http/httptest"
"testing"
)
// TestEncryptedImageRouteAnswersHEAD verifies that HEAD on the encrypted image
// route reaches its handler, as GET does, instead of being answered 405 Method
// Not Allowed. The handler refuses a token it cannot decrypt with 400, so that
// status shows the request got through.
func TestEncryptedImageRouteAnswersHEAD(t *testing.T) {
t.Parallel()
s := newTestServer(t)
rec := httptest.NewRecorder()
s.ServeHTTP(rec, httptest.NewRequestWithContext(
t.Context(), http.MethodHead, encryptedImagePath, nil))
t.Logf("status %d", rec.Code)
if rec.Code != http.StatusBadRequest {
t.Errorf("status = %d, want %d from the encrypted image handler",
rec.Code, http.StatusBadRequest)
}
}