Compare commits

2 Commits

Author SHA1 Message Date
0598f1dc04 Read form fields from the POST body only (closes #160)
All checks were successful
check / check (push) Successful in 2m55s
internal/handlers/source_management.go read the target destination
with r.FormValue, which falls back to the URL query string when the
field is absent from the body. So

    POST /source/{id}/targets?url=https://hooks.slack.com/services/T/B/S

created a working target from a value carried on the request line,
where logs, proxies, Referer headers and error trackers record it.
That is the remaining ingress path of the credential-exposure class
the render, delivery-error and log-line paths were each closed for.

Every form read in these handlers is now r.PostFormValue, so no
query-string value can populate stored configuration or be taken as a
credential. The one deliberate query read, `page` on the authenticated
pagination links, is untouched: it uses r.URL.Query().Get already.

The access log no longer carries the query on any branch, so the log
half of the report is already mitigated; the Sentry half is not, and
making the body the only place these fields are read from aims every
credential at Sentry's request context. The SDK attaches the request
to every captured event, and SendDefaultPII=false does not cover all
of what it copies: Scope.SetRequest tees the first 10 KiB of the body
into a buffer that ParseForm then fills, and Scope.ApplyToEvent copies
both that buffer and r.URL.RawQuery into the event with no guard,
before BeforeSend runs.

So the BeforeSend hook replaces the query string and the body with a
marker, drops cookies and the remote-address environment, and reduces
the headers to an allowlist. The body is replaced rather than filtered
by route because the SDK hands the hook no request to identify the
route with, and an unrecognised route must not leak; nothing is lost,
since the receiver route's body is already stored on the event and
served from the UI. The headers need an allowlist because the SDK's
own filter removes four names and passes everything else, including
X-Csrf-Token and the shared secrets senders put on the receiver route.
Scheme, host, path, method and X-Request-Id stay, which is what names
the failing route and ties it to the access log line.

Second barrier, for the JSON path that does not exist yet: the fields
that hold a credential are tagged json:"-" so the first handler to
marshal a model cannot serialise one. Target.Config holds the
incoming-webhook URL, APIKey.Key is a bearer token, and Setting.Value
holds the session encryption key. delivery.TargetView remains the
masking barrier for the HTML path, which is unaffected.
2026-08-17 23:21:09 +00:00
992b3c68f5 Run all linting in Docker via Dockerfile.lint (closes #109)
All checks were successful
check / check (push) Successful in 2m45s
golangci-lint no longer runs on the host. script/lint builds
Dockerfile.lint, which copies the repo into the digest-pinned linter
image, so the container holds only this repo and the cross-worktree
cache contamination of #106 becomes structurally impossible rather than
filtered after the fact. Five workers hit that contamination in one
evening, in both directions.

Three properties are load-bearing. --no-cache-filter=lint forces the
lint stage to re-execute while deps keeps its cache, because a cached
build lints nothing in 0.27s and exits 0. script/lint does not trust
that flag, since Docker silently ignores an unmatched stage name: it
asserts golangci-lint's own summary line appears, so no summary means no
lint whatever the exit code says. And both lint steps run
--network=none, which enforces rather than assumes that config verify
does not fetch its schema — verify is kept, because golangci-lint run
silently ignores unrecognized config keys and it is the only thing that
catches a typo that disables a setting.

Independently reviewed four times. Three passed the behaviour; the
remaining rounds were a README merge against #151, whose premise was
that the file contains no false statements. Six statements this change
falsified were found and corrected across those rounds — the last
reviewer re-derived every countable claim against the tree rather than
reading for plausibility, and found no seventh.

Supersedes #106.
2026-08-18 01:07:16 +02:00
17 changed files with 912 additions and 110 deletions

View File

@@ -19,9 +19,14 @@ RUN go mod download
# .dockerignore.
COPY . .
# Run formatting check and linter
# Run formatting check and linter. golangci-lint is invoked directly rather
# than through `make lint`: this stage is already the pinned linter image, and
# script/lint is a wrapper that builds Dockerfile.lint, so calling it here
# would need a docker daemon inside the build. Keep these steps in step with
# Dockerfile.lint, including --network=none (see its header for why).
RUN make fmt-check
RUN make lint
RUN --network=none golangci-lint config verify --config .golangci.yml
RUN --network=none golangci-lint run --config .golangci.yml ./...
# Build stage
# golang:1.26.1-bookworm (Debian-based), 2026-03-17

37
Dockerfile.lint Normal file
View File

@@ -0,0 +1,37 @@
# Lint-only image, built by script/lint. golangci-lint is never installed on
# the host: the repo is COPYed into the pinned image and linted as a build
# step, so a successful build IS a clean lint. This works even when the docker
# daemon is remote and bind mounts are impossible.
#
# script/lint passes --no-cache-filter=lint. Without it an unchanged tree
# replays the lint stage from cache and the build succeeds in under a second
# having run no linter at all. Do not drop that flag.
#
# The lint steps run with --network=none. `golangci-lint config verify` is
# documented as fetching its JSON schema over HTTPS, which would make linting
# depend on an unpinned remote artifact; this pinned image resolves the schema
# without any network, and --network=none enforces that rather than trusting
# it. It also proves no linter reaches out at analysis time. If a future image
# bump makes either step need the network, this build fails loudly instead of
# quietly acquiring an unpinned dependency.
# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-07
# Using Debian-based image because mattn/go-sqlite3 (CGO) does not
# compile on Alpine musl (off64_t is a glibc type).
FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS deps
WORKDIR /src
# Copy go mod files first for better layer caching. This stage is cacheable;
# only the lint stage below is forced to re-execute.
COPY go.mod go.sum ./
RUN go mod download
FROM deps AS lint
COPY . .
# `run` silently ignores config keys it does not recognize, so a typo would
# disable a setting without a word. `config verify` is what catches that.
RUN --network=none golangci-lint config verify --config .golangci.yml
RUN --network=none golangci-lint run --config .golangci.yml ./...

105
README.md
View File

@@ -12,14 +12,16 @@ with retry support, logging, and observability. Category: infrastructure
### Prerequisites
- Go 1.26.1+ (the version in `go.mod`)
- golangci-lint v2.12.2 (the version pinned in `script/bootstrap` and
in the `Dockerfile`'s lint stage; `make bootstrap` installs it)
- Docker (for containerized deployment, and for the lint and test
stages of the CI gate)
- Docker (for linting, for the test stage of the CI gate, and for
containerized deployment)
- `curl`, used by `script/fetch-assets` to download the third-party
browser assets, which are not committed (`make bootstrap` installs
it if missing)
golangci-lint is not a prerequisite and must not be installed on the
host: `script/bootstrap` does not install it, and `make lint` runs the
digest-pinned linter image via `Dockerfile.lint`.
### Quick Start
```bash
@@ -27,9 +29,9 @@ with retry support, logging, and observability. Category: infrastructure
git clone https://git.eeqj.de/sneak/webhooker.git
cd webhooker
# Install Go dependencies, the pinned linter, and the third-party
# browser assets. `make deps` alone is not enough: it only runs
# go mod download/tidy, and the checks below need the fetched assets.
# Install Go dependencies and the third-party browser assets.
# `make deps` alone is not enough: it only runs go mod download/tidy,
# and the checks below need the fetched assets.
make bootstrap
# Run all checks (test, lint, format check)
@@ -52,7 +54,7 @@ make setup # Bootstrap + install git pre-commit hook
make assets # Fetch + verify third-party browser assets
make fmt # Format code (gofmt + goimports)
make fmt-check # Fail if gofmt would change anything (writes nothing)
make lint # Run golangci-lint
make lint # Run golangci-lint in Docker (Dockerfile.lint)
make test # Run tests with race detection
make check # test + lint + fmt-check (CI gate)
make build # Build binary to bin/webhooker
@@ -275,7 +277,7 @@ are inline commands with no script behind them. We provide:
- `script/fetch-assets` — download the third-party browser assets into
`static/`, verifying each against its pinned sha256
- `script/test` — run the test suite
- `script/lint` — run golangci-lint
- `script/lint` — run golangci-lint in Docker (see Linting below)
- `script/fmt` — format all code (writes)
- `script/fmt-check` — check formatting (read-only)
- `script/check` — run test, lint, and fmt-check
@@ -1020,6 +1022,33 @@ buy the same amplification as an invented path. Nothing debuggable is
lost: `page`, on the authenticated pagination links, is the only query
parameter this service reads.
Client-supplied request content does not leave the host by the other
route either. The Sentry SDK attaches the request to every event it
captures, independently of the access log, and `SendDefaultPII=false`
does not cover all of what it copies: the raw query string and the
first 10 KiB of the request body are both taken unconditionally, the
body precisely because these handlers call `ParseForm`. A `BeforeSend`
hook therefore replaces the query string and the body with
`(redacted)`, drops cookies and the remote-address environment, and
reduces the headers to a fixed allowlist — `Accept`, `Content-Length`,
`Content-Type`, `Host`, `Origin`, `Referer`, `User-Agent` and
`X-Request-Id`.
The body is replaced rather than filtered because the hook cannot tell
which route it is on: the SDK hands `BeforeSend` no request, so a
route-conditional rule would have to guess, and an unrecognised route
must not leak. Nothing debuggable is lost by it. Every handler reads
its fields with `PostFormValue`, so the body is exactly where the
credentials are — the target destination URL, the login password, both
password-change fields — and on the receiver route, the one route
whose body is genuine signal, that body is already stored on the event
and served from the UI. The headers are an allowlist for the same
reason: the SDK's own filter removes four names and passes everything
else, which would ship `X-CSRF-Token` and the shared secrets senders
put on the receiver route. What survives still names the failing
route — scheme, host, path, method — and `X-Request-Id` ties the event
to the local access log line that holds the rest.
The remaining client-supplied fields are truncated rather than dropped,
each to a fixed budget: 512 bytes for `url`, `useragent` and `referer`,
128 for `request_id` (chi passes an inbound `X-Request-Id` header
@@ -1247,6 +1276,7 @@ webhooker/
├── templates/ # Go HTML templates (base, login, sources, etc.)
├── script/ # Scripts to Rule Them All entrypoints
├── Dockerfile # Three stages: lint, test+build, Alpine runtime
├── Dockerfile.lint # Lint-only image built by script/lint
├── Makefile # 10 of 16 targets shim script/; 6 are inline
├── go.mod / go.sum
└── .golangci.yml # Linter configuration
@@ -1452,6 +1482,37 @@ Two operational consequences follow from bounding the sequence:
no shutdown diagnostics at all. Keep the deployment's grace above
the stop timeout.
### Linting
golangci-lint never runs on the host. `script/lint` builds
`Dockerfile.lint`, which copies the repo into the digest-pinned
golangci-lint image and lints as a build step, so a successful build is
a clean lint. A host binary would share one cache and one lock with
every other checkout on the machine, which has produced both invented
findings attributed to other worktrees and unearned passes.
Three properties are load-bearing:
- `script/lint` passes `--no-cache-filter=lint`. Without it an unchanged
tree replays the lint layer from cache and the build exits 0 in under
a second having linted nothing. The `deps` stage stays cacheable, so
module downloads are not repeated. Invalidation is scoped to the one
stage; never prune the shared build cache.
- `script/lint` does not trust that flag. Docker silently ignores
`--no-cache-filter` for a stage name that does not match, so a stage
rename or a one-character typo would restore the cached false green
with no warning and a fast exit 0. The script therefore tees the
build output and treats a run as a pass only if golangci-lint's own
summary line (`N issues.` / `N issues:`) appears in it: no summary,
no lint, whatever the exit code says.
- Both lint steps use `RUN --network=none`. `golangci-lint config
verify` is documented as fetching its JSON schema over HTTPS, which
would be an unpinned remote dependency; the pinned image resolves the
schema without network access, and `--network=none` enforces that
instead of trusting it. Verify is worth keeping because
`golangci-lint run` silently ignores config keys it does not
recognize, so a typo would disable a setting with no warning.
### Docker
The Dockerfile uses a three-stage build. Each stage is pinned by
@@ -1460,7 +1521,8 @@ version is fixed independently of the compiler's:
1. **Lint stage** (`golangci/golangci-lint:v2.12.2`, Debian-based) —
installs `make`, downloads dependencies, copies the source, and runs
`make fmt-check` then `make lint`.
`make fmt-check`, then `golangci-lint config verify` and
`golangci-lint run`, both with `--network=none`.
2. **Builder stage** (`golang:1.26.1-bookworm`) — depends on the lint
stage passing (it copies a file from it), runs `script/fetch-assets`
to download and verify the third-party browser assets, then runs
@@ -1471,20 +1533,21 @@ version is fixed independently of the compiler's:
runs as the non-root `webhooker` user (UID 1000), exposes port 8080,
and includes a health check against `/.well-known/healthcheck`.
The lint stage invokes `golangci-lint` directly rather than `make lint`:
it is already the pinned linter image, and `make lint` builds
`Dockerfile.lint`, which would need a docker daemon inside this build.
Both check stages use Debian rather than Alpine because
`gorm.io/driver/sqlite` pulls in `mattn/go-sqlite3`, which needs CGO
and does not compile against musl. Only the final binary is statically
linked, which is what lets it run on the Alpine runtime image.
`script/cibuild``docker build .` — is the CI gate: the four check
targets run inside the image, so a build that succeeds is a repo that
is formatted, linted, tested and compiled. Only `script/cibuild` and
`script/docker` involve Docker. `script/lint`, and therefore
`make lint` and `make check`, run whatever `golangci-lint` is on the
host, which can be a different version from the pinned one — so the
container is the authoritative lint result
([issue #109](https://git.eeqj.de/sneak/webhooker/issues/109) tracks
routing local linting through it as well).
`script/cibuild` — `docker build .` — is the CI gate: the checks run
inside the image, so a build that succeeds is a repo that is formatted,
linted, tested and compiled. `script/lint` also uses Docker
(`Dockerfile.lint`, see Linting above), so `make lint` and `make check`
run the same pinned linter version the gate does; only `script/test`
and `script/fmt-check` run on the host.
#### CI gate honesty
@@ -1497,8 +1560,8 @@ the hash of the last commit that touched the build context, so:
- Any commit that changes code (including a squash merge whose tree
matches an already-built branch) gets a new fingerprint, invalidates
the `COPY . .` layer of both check stages, and really runs
`make fmt-check`, `make lint`, `make test`, and `make build`. A run
that reports success ran them.
`make fmt-check`, `golangci-lint`, `make test`, and `make build`. A
run that reports success ran them.
- A docs-only commit leaves the fingerprint unchanged — `.dockerignore`
excludes `*.md`, `LICENSE` and `.editorconfig` from the context
anyway — so the image replays from cache and costs seconds.

View File

@@ -2,12 +2,16 @@ package database
import "time"
// APIKey represents an API key for a user
// APIKey represents an API key for a user.
//
// Key is a bearer credential, so it is never marshalled with the
// model. A creation handler that has to show it once returns it in its
// own response type.
type APIKey struct {
BaseModel
UserID string `gorm:"type:uuid;not null" json:"userId"`
Key string `gorm:"uniqueIndex;not null" json:"key"`
Key string `gorm:"uniqueIndex;not null" json:"-"`
Description string `json:"description"`
LastUsedAt *time.Time `json:"lastUsedAt,omitempty"`

View File

@@ -0,0 +1,107 @@
package database_test
import (
"encoding/json"
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"sneak.berlin/go/webhooker/internal/database"
)
// keptField is a non-secret value planted alongside each secret, so
// the assertions below cannot pass by the model marshalling to nothing.
const keptField = "keepme"
// marshalModel encodes a model the way a future JSON handler would.
func marshalModel(t *testing.T, v any) string {
t.Helper()
encoded, err := json.Marshal(v)
require.NoError(t, err)
return string(encoded)
}
// TestModelsDoNotMarshalTheirSecrets pins the barrier for the JSON
// path. The /api/v1 route group exists and is empty; delivery's
// TargetView masks the credential for the HTML path only, so without
// these tags the first handler that marshals a model serialises the
// secret with it. Each field below is a live credential:
//
// - Target.Config holds an incoming-webhook URL whose path segments
// are the bearer token.
// - APIKey.Key is a bearer token outright.
// - Setting.Value holds the session encryption key.
// - User.Password holds the Argon2 hash, and was already tagged.
func TestModelsDoNotMarshalTheirSecrets(t *testing.T) {
t.Parallel()
const marker = "QQMODELMARKERQQ"
cases := []struct {
name string
model any
}{
{
name: "target config",
model: database.Target{
Name: keptField,
Type: database.TargetTypeSlack,
Config: `{"webhookUrl":"https://h/s/` + marker + `"}`,
},
},
{
name: "api key",
model: database.APIKey{
Description: keptField,
Key: marker,
},
},
{
name: "setting value",
model: database.Setting{
Key: keptField,
Value: marker,
},
},
{
name: "user password hash",
model: database.User{
Username: keptField,
Password: marker,
},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
encoded := marshalModel(t, tc.model)
assert.NotContains(t, encoded, marker)
assert.Contains(t, encoded, keptField)
})
}
}
// TestWebhookMarshalsNoTargetConfig covers the nested case: a webhook
// marshalled with its targets preloaded must not carry the credential
// through the association either.
func TestWebhookMarshalsNoTargetConfig(t *testing.T) {
t.Parallel()
const marker = "QQNESTEDMARKERQQ"
encoded := marshalModel(t, database.Webhook{
Name: keptField,
Targets: []database.Target{{
Name: "slack",
Config: `{"webhookUrl":"https://h/s/` + marker + `"}`,
}},
})
assert.NotContains(t, encoded, marker)
assert.Contains(t, encoded, keptField)
}

View File

@@ -4,5 +4,8 @@ package database
// Used for auto-generated values like the session encryption key.
type Setting struct {
Key string `gorm:"primaryKey" json:"key"`
Value string `gorm:"type:text;not null" json:"value"`
// Value holds the session encryption key, so it is never
// marshalled with the model.
Value string `gorm:"type:text;not null" json:"-"`
}

View File

@@ -20,8 +20,14 @@ type Target struct {
Type TargetType `gorm:"not null" json:"type"`
Active bool `gorm:"default:true" json:"active"`
// Configuration fields (JSON stored based on type)
Config string `gorm:"type:text" json:"config"` // JSON configuration
// Configuration fields (JSON stored based on type).
//
// json:"-" because the blob holds the target's credential — a
// Slack incoming-webhook URL, or an http destination whose path
// segments are the secret. delivery.TargetView is the masking
// barrier for the HTML path; this tag is the barrier for any
// handler that marshals the model itself.
Config string `gorm:"type:text" json:"-"` // JSON configuration
// For HTTP targets (max_retries=0 means fire-and-forget,
// >0 enables retries with backoff)

View File

@@ -39,8 +39,10 @@ func (h *Handlers) HandleLoginSubmit() http.HandlerFunc {
return
}
username := r.FormValue("username")
password := r.FormValue("password")
// PostFormValue, not FormValue: the credential must come
// from the body, never from the query string.
username := r.PostFormValue("username")
password := r.PostFormValue("password")
// Validate input
if username == "" || password == "" {

View File

@@ -44,9 +44,11 @@ func (h *Handlers) HandlePasswordChange() http.HandlerFunc {
successMessage, errorMessage, handled := h.applyPasswordChange(
w,
sessionUsername,
r.FormValue("current_password"),
r.FormValue("new_password"),
r.FormValue("confirm_password"),
// PostFormValue, not FormValue: the credential must
// come from the body, never from the query string.
r.PostFormValue("current_password"),
r.PostFormValue("new_password"),
r.PostFormValue("confirm_password"),
)
if !handled {
return

View File

@@ -227,9 +227,9 @@ func (h *Handlers) HandleSourceCreateSubmit() http.HandlerFunc {
return
}
name := r.FormValue("name")
description := r.FormValue("description")
retentionStr := r.FormValue("retention_days")
name := r.PostFormValue("name")
description := r.PostFormValue("description")
retentionStr := r.PostFormValue("retention_days")
if name == "" {
w.WriteHeader(http.StatusBadRequest)
@@ -509,7 +509,7 @@ func (h *Handlers) applyWebhookEdit(
) {
// The body size cap is enforced by the MaxBodySize middleware,
// which runs before CSRF parses the form.
name := r.FormValue("name")
name := r.PostFormValue("name")
if name == "" {
data := map[string]any{
tmplKeyWebhook: webhook,
@@ -523,12 +523,12 @@ func (h *Handlers) applyWebhookEdit(
}
webhook.Name = name
webhook.Description = r.FormValue("description")
webhook.Description = r.PostFormValue("description")
// An empty field falls back to the stored value, so submitting the
// form without touching retention leaves the policy alone.
retentionDays, retErr := parseRetentionDays(
r.FormValue("retention_days"), webhook.RetentionDays,
r.PostFormValue("retention_days"), webhook.RetentionDays,
)
if retErr != nil {
data := map[string]any{
@@ -950,7 +950,7 @@ func (h *Handlers) HandleEntrypointCreate() http.HandlerFunc {
return
}
description := r.FormValue("description")
description := r.PostFormValue("description")
entrypoint := &database.Entrypoint{
WebhookID: webhook.ID,
@@ -1020,11 +1020,18 @@ func (h *Handlers) processTargetCreate(
) {
// The body size cap is enforced by the MaxBodySize middleware,
// which runs before CSRF parses the form.
name := r.FormValue("name")
targetType := database.TargetType(r.FormValue("type"))
targetURL := r.FormValue("url")
maxRetriesStr := r.FormValue("max_retries")
expiry := r.FormValue("expiry")
//
// Every field here is read with PostFormValue, not FormValue.
// FormValue falls back to the query string, which would let
// `POST /source/{id}/targets?url=https://hooks.slack.com/...`
// configure a target from a value the request line carries — and
// the request line, unlike the body, is what logs, proxies,
// Referer headers and error trackers record.
name := r.PostFormValue("name")
targetType := database.TargetType(r.PostFormValue("type"))
targetURL := r.PostFormValue("url")
maxRetriesStr := r.PostFormValue("max_retries")
expiry := r.PostFormValue("expiry")
if name == "" {
http.Error(

View File

@@ -0,0 +1,206 @@
package handlers_test
import (
"bytes"
"context"
"log/slog"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
"github.com/go-chi/chi"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/database"
"sneak.berlin/go/webhooker/internal/middleware"
)
// targetSecretSegments are the path segments of an incoming-webhook
// URL. For Slack, Discord and Teams the path IS the bearer credential,
// so this string must not reach storage or the access log by way of
// the request line.
const targetSecretSegments = "T00000000/B00000000/QQTARGETSECRETQQ"
// targetSecretURL is a destination whose secret lives in its path. It
// uses a literal public address rather than a hostname so the SSRF
// check resolves nothing: with a hostname, a sandbox without DNS would
// reject the URL for the wrong reason and the test would pass even
// with the defect reintroduced.
const targetSecretURL = "https://93.184.216.34/services/" +
targetSecretSegments
// targetsForWebhook returns every target stored against a webhook.
func targetsForWebhook(
t *testing.T,
db *database.Database,
webhookID string,
) []database.Target {
t.Helper()
var targets []database.Target
require.NoError(
t,
db.DB().Where("webhook_id = ?", webhookID).
Find(&targets).Error,
)
return targets
}
// postTargetCreate drives HandleTargetCreate through the production
// access-log middleware and a chi route, so the logged url field is
// produced exactly as it ships, and returns the recorder plus the
// captured log.
func postTargetCreate(
t *testing.T,
env *sourceTestEnv,
webhookID string,
query string,
form url.Values,
) (*httptest.ResponseRecorder, string) {
t.Helper()
logBuf := new(bytes.Buffer)
mw := middleware.NewForTest(
slog.New(slog.NewJSONHandler(
logBuf, &slog.HandlerOptions{Level: slog.LevelInfo},
)),
&config.Config{Environment: config.EnvironmentDev},
nil,
)
router := chi.NewRouter()
router.Use(mw.Logging())
router.Post(
"/source/{sourceID}/targets",
env.handlers.HandleTargetCreate(),
)
target := "/source/" + webhookID + "/targets"
if query != "" {
target += "?" + query
}
body := ""
if form != nil {
body = form.Encode()
}
req := httptest.NewRequestWithContext(
context.Background(),
http.MethodPost,
target,
strings.NewReader(body),
)
req.Header.Set(
"Content-Type", "application/x-www-form-urlencoded",
)
for _, c := range env.cookies {
req.AddCookie(c)
}
w := httptest.NewRecorder()
router.ServeHTTP(w, req)
return w, logBuf.String()
}
// TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget is the
// regression test for the ingress leak. r.FormValue falls back to the
// query string when a field is absent from the POST body, so
//
// POST /source/{id}/targets?url=https://hooks.slack.com/services/...
//
// with an empty url field used to create a working target from a value
// carried on the request line — where logs, proxies, Referer headers
// and error trackers record it. The handler reads the body only, so
// the request is rejected for a missing URL and stores nothing.
//
// name and type are sent in the BODY on purpose: the request has to
// get past those two validations for the assertion to be about the url
// read specifically.
func TestHandleTargetCreate_QueryStringURLDoesNotConfigureATarget(
t *testing.T,
) {
t.Parallel()
env := setupSourceTest(t)
webhook := seedWebhookWithRetention(t, env.db, 30)
body := url.Values{}
body.Set("name", "leaky")
body.Set("type", string(database.TargetTypeSlack))
w, logged := postTargetCreate(
t, env, webhook.ID,
"url="+url.QueryEscape(targetSecretURL),
body,
)
assert.Equal(t, http.StatusBadRequest, w.Code)
targets := targetsForWebhook(t, env.db, webhook.ID)
assert.Empty(
t, targets,
"a query-string value must not populate a target config",
)
assert.NotContains(t, logged, targetSecretSegments)
assert.NotContains(t, logged, "93.184.216.34")
assert.NotEmpty(t, logged, "the access log line must still be written")
}
// TestHandleTargetCreate_BodyURLStillCreatesTheTarget is the positive
// control for the test above: the rejection has to come from where the
// value was read, not from the handler being broken.
func TestHandleTargetCreate_BodyURLStillCreatesTheTarget(t *testing.T) {
t.Parallel()
env := setupSourceTest(t)
webhook := seedWebhookWithRetention(t, env.db, 30)
body := url.Values{}
body.Set("name", "legit")
body.Set("type", string(database.TargetTypeSlack))
body.Set("url", targetSecretURL)
w, logged := postTargetCreate(t, env, webhook.ID, "", body)
assert.Equal(t, http.StatusSeeOther, w.Code)
targets := targetsForWebhook(t, env.db, webhook.ID)
require.Len(t, targets, 1)
assert.Contains(t, targets[0].Config, targetSecretSegments)
// The body carried the credential, so the access log must still
// not have it: the log records the request line only.
assert.NotContains(t, logged, targetSecretSegments)
}
// TestHandleTargetCreate_QueryStringCannotSupplyNameOrType covers the
// rest of the converted reads on this handler in one request: with an
// empty body, nothing the query carries is visible to it.
func TestHandleTargetCreate_QueryStringCannotSupplyNameOrType(
t *testing.T,
) {
t.Parallel()
env := setupSourceTest(t)
webhook := seedWebhookWithRetention(t, env.db, 30)
w, _ := postTargetCreate(
t, env, webhook.ID,
"name=leaky&type=slack&max_retries=9&expiry=30d&url="+
url.QueryEscape(targetSecretURL),
url.Values{},
)
assert.Equal(t, http.StatusBadRequest, w.Code)
assert.Contains(t, w.Body.String(), "Name is required")
assert.Empty(t, targetsForWebhook(t, env.db, webhook.ID))
}

View File

@@ -4,6 +4,7 @@ import (
"log/slog"
"net/http"
"github.com/getsentry/sentry-go"
"sneak.berlin/go/webhooker/internal/config"
"sneak.berlin/go/webhooker/internal/handlers"
"sneak.berlin/go/webhooker/internal/middleware"
@@ -13,6 +14,25 @@ import (
// build requests that sit exactly at, below, and above it.
const MaxFormBodySizeForTest = maxFormBodySize
// ScrubSentryRequestForTest exposes the BeforeSend hook that
// enableSentry installs, so a test can assert on what it leaves in an
// event without standing up a Sentry client.
func ScrubSentryRequestForTest(
event *sentry.Event,
hint *sentry.EventHint,
) *sentry.Event {
return scrubSentryRequest(event, hint)
}
// SentryClientOptionsForTest exposes the exact options enableSentry
// initialises the SDK with, so a test can capture events through the
// production hook wiring rather than a hand-built equivalent.
func SentryClientOptionsForTest(
dsn, release string,
) sentry.ClientOptions {
return sentryClientOptions(dsn, release)
}
// NewRouterForTest builds the real route tree via SetupRoutes with
// the supplied middleware and handlers, bypassing the fx lifecycle
// and the HTTP listener. Tests use it so that route-group middleware

117
internal/server/sentry.go Normal file
View File

@@ -0,0 +1,117 @@
package server
import (
"net/http"
"github.com/getsentry/sentry-go"
)
// sentryRedacted stands in for a withheld field on every event shipped
// to Sentry. It is a marker rather than an empty string so a reader
// can tell a suppressed value from an absent one.
const sentryRedacted = "(redacted)"
// sentryClientOptions builds the options the SDK is initialised with.
// It is its own function so a test can stand up a client wired exactly
// as production is, with only the transport swapped.
func sentryClientOptions(dsn, release string) sentry.ClientOptions {
return sentry.ClientOptions{
Dsn: dsn,
Release: release,
// Both hooks, because the SDK runs one for error events
// and the other for transactions.
BeforeSend: scrubSentryRequest,
BeforeSendTransaction: scrubSentryRequest,
}
}
// scrubSentryRequest strips client-supplied content from an event's
// request context before it leaves the process.
//
// sentryhttp attaches the whole *http.Request to the scope
// (sentryhttp.go:113), and Scope.ApplyToEvent fills the event's
// Request from it inside prepareEvent, which runs before this hook.
// Two of the fields it fills are copied with no SendDefaultPII guard:
//
// - QueryString, verbatim from r.URL.RawQuery.
// - Data, the first 10 KiB of the request body, teed off r.Body by
// SetRequest and filled precisely because the handlers call
// ParseForm.
//
// Since every form field in this service is read with PostFormValue,
// the body is the only place a credential is submitted: a target's
// destination URL, whose path segments are the bearer token, plus the
// login password and both password-change fields. None of that may
// reach a third-party service.
//
// This hook is a floor, not a default: the fields it clears stay
// cleared even if SendDefaultPII is ever turned on.
func scrubSentryRequest(
event *sentry.Event,
_ *sentry.EventHint,
) *sentry.Event {
if event == nil || event.Request == nil {
return event
}
req := event.Request
if req.QueryString != "" {
req.QueryString = sentryRedacted
}
if req.Data != "" {
req.Data = sentryRedacted
}
req.Cookies = ""
req.Env = nil
req.Headers = keptSentryHeaders(req.Headers)
return event
}
// keptSentryHeaders returns the subset of headers an event may carry
// off-host. Dropping by allowlist rather than by blocklist is what
// makes an unrecognised header safe: the SDK's own filter removes four
// names and passes everything else, so X-Csrf-Token — which
// gorilla/csrf accepts in place of the form field — and the shared
// secrets senders put on the receiver route (X-Gitlab-Token and the
// per-provider signature headers) would otherwise ship verbatim.
func keptSentryHeaders(headers map[string]string) map[string]string {
if len(headers) == 0 {
return headers
}
kept := make(map[string]string, len(headers))
for name, value := range headers {
if sentryKeepsHeader(name) {
kept[name] = value
}
}
return kept
}
// sentryKeepsHeader reports whether a request header is routing or
// content metadata rather than client-chosen payload. Referer is kept
// on the reasoning that it is browser-set, that this service emits
// only ?page= in its own links, and that Referrer-Policy is set to
// strict-origin-when-cross-origin. X-Request-Id ties the event to the
// local access log line, which holds the rest of the detail.
func sentryKeepsHeader(name string) bool {
switch http.CanonicalHeaderKey(name) {
case "Accept",
"Content-Length",
"Content-Type",
"Host",
"Origin",
"Referer",
"User-Agent",
"X-Request-Id":
return true
default:
return false
}
}

View File

@@ -0,0 +1,227 @@
package server_test
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"sync"
"testing"
"time"
"github.com/getsentry/sentry-go"
sentryhttp "github.com/getsentry/sentry-go/http"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"sneak.berlin/go/webhooker/internal/server"
)
// The three markers below are the credentials a captured event could
// carry off-host, one per field of sentry.Request that the SDK fills
// from the request without a SendDefaultPII guard.
const (
// sentryBodyMarker is submitted as a form value. Since every
// handler reads its fields with PostFormValue, the body is the
// only place a password or a target URL is ever supplied.
sentryBodyMarker = "QQSENTRYBODYMARKERQQ"
// sentryQueryMarker rides the request line.
sentryQueryMarker = "T00000000/B00000000/QQSENTRYQUERYMARKERQQ"
// sentryHeaderMarker rides X-Csrf-Token, which gorilla/csrf
// accepts in place of the form field.
sentryHeaderMarker = "QQSENTRYHEADERMARKERQQ"
)
// sentryKeptUserAgent is a non-secret header value planted so the
// assertions below cannot pass by the event carrying no headers at
// all.
const sentryKeptUserAgent = "webhooker-test-agent"
// captureTransport records events instead of shipping them, so a test
// sees exactly the payload the SDK would have put on the wire.
type captureTransport struct {
mu sync.Mutex
events []*sentry.Event
}
func (c *captureTransport) Configure(sentry.ClientOptions) {}
func (c *captureTransport) Flush(time.Duration) bool { return true }
func (c *captureTransport) SendEvent(event *sentry.Event) {
c.mu.Lock()
defer c.mu.Unlock()
c.events = append(c.events, event)
}
// captureThroughSentryHTTP panics inside a form handler wrapped in the
// real sentryhttp middleware and returns the event the SDK produced.
//
// This is the only construction path on which Request.Data appears:
// sentryhttp calls Scope.SetRequest, which tees r.Body into a 10 KiB
// buffer, ParseForm drains the tee, and Scope.ApplyToEvent copies the
// buffer into the event inside prepareEvent — before BeforeSend runs.
// A hand-built sentry.NewRequest never reads the body and so cannot
// regress-test any of it.
//
// scrub selects whether the production BeforeSend hooks are installed,
// so the same path shows both what the SDK collects and what survives.
func captureThroughSentryHTTP(t *testing.T, scrub bool) *sentry.Event {
t.Helper()
transport := &captureTransport{}
opts := server.SentryClientOptionsForTest(
"https://public@sentry.invalid/1", "webhooker-test",
)
opts.Transport = transport
if !scrub {
opts.BeforeSend = nil
opts.BeforeSendTransaction = nil
}
client, err := sentry.NewClient(opts)
require.NoError(t, err)
handler := sentryhttp.New(sentryhttp.Options{}).Handle(
http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
// This call is what drains the tee and fills the
// buffer. Its success is asserted by the unscrubbed
// case below, which sees the body in the event.
_ = r.ParseForm()
panic("boom")
}),
)
handler.ServeHTTP(
httptest.NewRecorder(),
sentryLoginRequest(client),
)
require.Len(t, transport.events, 1)
return transport.events[0]
}
// sentryLoginRequest builds the password POST the capture above drives,
// with a credential planted in the body, the query and a header.
func sentryLoginRequest(client *sentry.Client) *http.Request {
form := url.Values{}
form.Set("username", "admin")
form.Set("password", sentryBodyMarker)
req := httptest.NewRequestWithContext(
sentry.SetHubOnContext(
context.Background(),
sentry.NewHub(client, sentry.NewScope()),
),
http.MethodPost,
"/pages/login?url=https://hooks.slack.com/services/"+
sentryQueryMarker,
strings.NewReader(form.Encode()),
)
req.Header.Set(
"Content-Type", "application/x-www-form-urlencoded",
)
req.Header.Set("X-Csrf-Token", sentryHeaderMarker)
req.Header.Set("User-Agent", sentryKeptUserAgent)
return req
}
// marshalEvent encodes an event the way the transport does.
func marshalEvent(t *testing.T, event *sentry.Event) string {
t.Helper()
encoded, err := json.Marshal(event)
require.NoError(t, err)
return string(encoded)
}
// TestSentryScrub_SDKCollectsTheRequestUnscrubbed pins the premise the
// hook exists for. Without it the SDK ships the whole POST body, the
// raw query and the CSRF header, none of which SendDefaultPII=false
// suppresses.
func TestSentryScrub_SDKCollectsTheRequestUnscrubbed(t *testing.T) {
t.Parallel()
event := captureThroughSentryHTTP(t, false)
require.NotNil(t, event.Request)
assert.Contains(
t, event.Request.Data, sentryBodyMarker,
"the SDK is expected to collect the POST body; if it no "+
"longer does, the scrub hook's premise changed",
)
assert.Contains(t, event.Request.QueryString, sentryQueryMarker)
assert.Contains(
t, marshalEvent(t, event), sentryHeaderMarker,
)
}
// TestSentryScrub_RedactsTheCapturedRequest is the regression test: no
// byte of any planted credential may survive into the marshalled event
// that leaves the process.
func TestSentryScrub_RedactsTheCapturedRequest(t *testing.T) {
t.Parallel()
event := captureThroughSentryHTTP(t, true)
require.NotNil(t, event.Request)
encoded := marshalEvent(t, event)
assert.NotContains(t, encoded, sentryBodyMarker)
assert.NotContains(t, encoded, sentryQueryMarker)
assert.NotContains(t, encoded, sentryHeaderMarker)
assert.NotContains(t, encoded, "hooks.slack.com")
assert.Equal(t, "(redacted)", event.Request.Data)
assert.Equal(t, "(redacted)", event.Request.QueryString)
assert.Empty(t, event.Request.Cookies)
assert.Empty(t, event.Request.Env)
}
// TestSentryScrub_KeepsTheRoutingContext checks the hook does not cost
// the debugging signal: the route, the method and the metadata headers
// still identify what failed.
func TestSentryScrub_KeepsTheRoutingContext(t *testing.T) {
t.Parallel()
event := captureThroughSentryHTTP(t, true)
require.NotNil(t, event.Request)
assert.Contains(t, event.Request.URL, "/pages/login")
assert.Equal(t, http.MethodPost, event.Request.Method)
assert.Equal(
t,
sentryKeptUserAgent,
event.Request.Headers["User-Agent"],
)
assert.Equal(
t,
"application/x-www-form-urlencoded",
event.Request.Headers["Content-Type"],
)
}
// TestSentryScrub_ToleratesEventsWithoutARequest covers the events the
// hook sees outside an HTTP handler, where no request is attached.
func TestSentryScrub_ToleratesEventsWithoutARequest(t *testing.T) {
t.Parallel()
scrubbed := server.ScrubSentryRequestForTest(
sentry.NewEvent(), nil,
)
require.NotNil(t, scrubbed)
assert.Nil(t, scrubbed.Request)
assert.Nil(t, server.ScrubSentryRequestForTest(nil, nil))
}

View File

@@ -141,14 +141,14 @@ func (s *Server) enableSentry() {
return
}
err := sentry.Init(sentry.ClientOptions{
Dsn: s.params.Config.SentryDSN,
Release: fmt.Sprintf(
err := sentry.Init(sentryClientOptions(
s.params.Config.SentryDSN,
fmt.Sprintf(
"%s-%s",
s.params.Globals.Appname,
s.params.Globals.Version,
),
})
))
if err != nil {
s.log.Error("sentry init failure", "error", err)
// Don't use fatal since we still want the service to run

View File

@@ -3,20 +3,14 @@
# 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). Finishes by running script/fetch-assets, which installs the
# hash-pinned third-party browser assets the repo does not commit.
# make, or go). golangci-lint is deliberately not installed: linting runs
# only in docker, via script/lint and Dockerfile.lint. Finishes by running
# script/fetch-assets, which installs the hash-pinned third-party browser
# assets the repo does not commit.
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-<arch>.tar.gz release archives
GOLANGCI_LINT_SHA256_AMD64="8df580d2670fed8fa984aac0507099af8df275e665215f5c7a2ae3943893a553"
GOLANGCI_LINT_SHA256_ARM64="44cd40a8c76c86755375adfeea52cfd3533cb43d7bd647771e0ae065e166df3a"
PKGMGR=""
SUDO=""
@@ -57,52 +51,6 @@ missing() {
! command -v "$1" >/dev/null 2>&1
}
# verify_sha256 <file> <expected-hash>
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
detect_pkgmgr
case "$PKGMGR" in
apt) install_golangci_lint_release ;;
*) pkg_install golangci-lint golangci-lint golangci-lint golangci-lint ;;
esac
}
main() {
cd "$ROOT"
@@ -110,9 +58,14 @@ 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
# Not installed here: docker is platform-specific and out of scope for a
# package-manager bootstrap, but script/lint needs it.
if missing docker; then
echo "bootstrap: docker not found; script/lint requires it" >&2
fi
go mod download

View File

@@ -1,12 +1,55 @@
#!/bin/sh
# script/lint: run the linter.
# script/lint: run the linter. golangci-lint is never installed locally: it
# runs via docker only, one way, everywhere — script/lint builds
# Dockerfile.lint, which COPYs the repo into the pinned golangci-lint image
# and lints as a build step. This works even when the docker daemon is remote
# and bind mounts are impossible, and it removes the host linter's shared
# cache, which has attributed other checkouts' findings to this one.
#
# --no-cache-filter=lint forces the lint stage to re-execute on every run; a
# cached lint stage exits 0 in under a second having linted nothing. The deps
# stage keeps its cache, so module downloads are not repeated.
# --progress=plain keeps the linter's own output visible on success, so a
# passing run shows the issue count rather than nothing.
# --output=type=cacheonly leaves no image behind to clean up.
#
# docker silently ignores --no-cache-filter for a stage name that does not
# match, so a rename or a typo would restore the cached false green with no
# warning and a fast exit 0. The flag is therefore not trusted: the build
# output is teed to a log and a run is only a pass if golangci-lint's own
# summary line ("N issues." / "N issues:") is in it. No summary, no lint,
# whatever the exit code says.
set -eu
ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
main() {
cd "$ROOT"
golangci-lint run --config .golangci.yml ./...
log="$(mktemp -t webhooker-lint.XXXXXXXX)"
rcfile="$(mktemp -t webhooker-lint-rc.XXXXXXXX)"
trap 'rm -f "$log" "$rcfile"' EXIT INT TERM
# The pipeline's status is tee's, and POSIX sh has no pipefail, so the
# build's status travels via a file. Output still streams live.
{
docker build \
-f Dockerfile.lint \
--no-cache-filter=lint \
--progress=plain \
--output=type=cacheonly \
. 2>&1 && echo 0 >"$rcfile" || echo $? >"$rcfile"
} | tee "$log" >&2
rc="$(cat "$rcfile")"
[ "$rc" -eq 0 ] || exit "$rc"
if ! grep -qE '[0-9]+ issues[.:]' "$log"; then
echo "script/lint: golangci-lint printed no summary line; the linter" >&2
echo " did not run. Check that the stage named in --no-cache-filter" >&2
echo " still matches a stage in Dockerfile.lint." >&2
exit 1
fi
}
main "$@"