Author SHA1 Message Date
sneak f703b72ce0 Next (#364)
check / check (push) Waiting to run
Reviewed-on: #364
2026-09-29 13:05:57 +02:00
clawbot b79e4649a1 Container sets its data directory's owner and mode itself (#353)
check / check (push) Waiting to run
Closes #340.

The image no longer sets `USER`. Its new `ENTRYPOINT`, `deploy/docker-entrypoint.sh`, starts as root, creates `DATA_DIR` if missing, gives the directory and anything in it owned by another user to `webhooker` (UID 1000), sets the directory to `0750`, and runs the command as `webhooker` through `su-exec`. An empty root-owned bind mount, or data left by another UID, now works as mounted; the app never runs as root and is still PID 1. `CMD` is still `/app/webhooker`, so the `resetpw` commands are unchanged. Started with `--user`, the script only runs the command. It is in `/usr/local/bin`, not `/app`, which belongs to `webhooker`.

README: the UID 1000 ownership block, the upaas pre-deploy commands and the restore ownership step are gone; the upaas volume bullet names only the path.

- Judgement call: `su-exec` over `setpriv`: Alpine's small tool for this, needing only musl; busybox's `setpriv` cannot change user, and util-linux's adds `libcap-ng`.
- Deviation: `su-exec` is pinned by version (`0.2-r3`), not by hash; `ca-certificates` beside it is unpinned.
- Judgement call: each start reads every entry's owner but changes only entries owned by someone else.
- `docker exec` and the health check now run as root, since the image sets no `USER`.
- No automated test covers the script: the suite runs inside `docker build`, which cannot start a container.
- A missing host directory under upaas is sneak/upaas#235.

Model: opus-5-5
Reviewed-on: #353
Co-authored-by: clawbot <35+clawbot@noreply.example.org>
2026-09-29 13:05:16 +02:00
clawbot 1428154bbd Let a browser with cookies from an earlier database log in (closes #359)
check / check (push) Waiting to run
A new database brings a new session key. A browser still holding the
old session cookie got a 500 on a correct login: Session.Get returned
the cookie's decode error and the login handler answered it with a
500. Get now treats a cookie that does not decode as absent, and
logging in replaces it. gorilla/csrf already did the same for the
CSRF cookie.

A start that creates webhooker.db now logs "created a new, empty
database" at WARN with its path, shortly before the first-boot banner,
so an unexpectedly empty DATA_DIR is noticed.

The codec tests now decode through the store, since Get no longer
reports the codec's reason.

Model: opus-5-5
2026-09-29 12:58:44 +02:00
sneak 8ad2a86e4b Sneak/testdeploy (#356)
check / check (push) Successful in 11s
Reviewed-on: #356
2026-09-29 12:01:33 +02:00
sneak a891b726e5 Milestone: next into main (#342)
check / check (push) Successful in 9s
Reviewed-on: #342
2026-09-29 11:53:19 +02:00
clawbot ab63b5f777 Restrict /s/* to GET and HEAD (closes #169)
check / check (push) Successful in 3m37s
The static file server was attached with Mount, which registers every
method, so POST, PUT and DELETE on an asset were answered 200 with the
file. It is now registered for GET and HEAD only, inside a /s group
whose method-not-allowed handler answers 405 with Allow: GET, HEAD.
A method chi does not route at all, such as PROPFIND, still gets 405
from the top-level router, without Allow. The inverted test and the
README route table say the same.

Model: opus-5-5
2026-09-29 11:10:27 +02:00
sneak 9cf9cdd8eb 1.0.0 milestone: next into main (#321)
check / check (push) Successful in 8s
Reviewed-on: #321
2026-09-29 11:04:58 +02:00
17 changed files with 539 additions and 248 deletions
+9 -2
View File
@@ -88,7 +88,9 @@ RUN CGO_ENABLED=1 make build VERSION="$VERSION" GO_LDFLAGS='-extldflags "-static
# alpine:3.21, 2026-03-17 # alpine:3.21, 2026-03-17
FROM alpine:3.21@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4709 FROM alpine:3.21@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4709
RUN apk --no-cache add ca-certificates # su-exec 0.2-r3 (Alpine 3.21), 2026-09-29: the entrypoint runs the app
# as webhooker with it.
RUN apk --no-cache add ca-certificates su-exec=0.2-r3
# Create non-root user # Create non-root user
RUN addgroup -g 1000 -S webhooker && \ RUN addgroup -g 1000 -S webhooker && \
@@ -99,13 +101,17 @@ WORKDIR /app
# Copy binary from builder # Copy binary from builder
COPY --from=builder /build/bin/webhooker /app/webhooker COPY --from=builder /build/bin/webhooker /app/webhooker
# Not under /app, which belongs to webhooker: this script runs as root.
COPY deploy/docker-entrypoint.sh /usr/local/bin/docker-entrypoint.sh
# Create data directory for all SQLite databases (main app DB + # Create data directory for all SQLite databases (main app DB +
# per-webhook event DBs). DATA_DIR defaults to /var/lib/webhooker. # per-webhook event DBs). DATA_DIR defaults to /var/lib/webhooker.
RUN mkdir -p /var/lib/webhooker RUN mkdir -p /var/lib/webhooker
RUN chown -R webhooker:webhooker /app /var/lib/webhooker RUN chown -R webhooker:webhooker /app /var/lib/webhooker
USER webhooker # No USER: the entrypoint starts as root to make the data directory
# webhooker's, then runs the app as webhooker.
EXPOSE 8080 EXPOSE 8080
@@ -124,4 +130,5 @@ ENV BIND_ADDRESS=0.0.0.0
HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
CMD wget --no-verbose --tries=1 --spider http://localhost:8080/.well-known/healthcheck || exit 1 CMD wget --no-verbose --tries=1 --spider http://localhost:8080/.well-known/healthcheck || exit 1
ENTRYPOINT ["/usr/local/bin/docker-entrypoint.sh"]
CMD ["/app/webhooker"] CMD ["/app/webhooker"]
+114 -155
View File
@@ -147,7 +147,7 @@ TTY detection, and security headers are always applied.
| `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` | | `RETENTION_SWEEP_INTERVAL` | How often the retention reaper and archive sweeper run (Go duration, must be positive) | `1h` |
| `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` | | `SESSION_IDLE_TIMEOUT` | Idle session timeout (Go duration) | `24h` |
| `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` | | `RECEIVER_RATE_LIMIT` | Receiver requests/minute per IP per entrypoint (10x that per IP across the route) | `120` |
| `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted. A set value replaces the default. Under the default, any client with a private address, whether it connects directly or through the proxy, can choose its own rate-limit key by sending its own `X-Forwarded-For`; if any clients have private addresses, set it to the proxy's address alone. See [Trusted proxies](#trusted-proxies) | `10.0.0.0/8,172.16.0.0/12,192.168.0.0/16` (RFC 1918) | | `TRUSTED_PROXIES` | CIDRs whose forwarded headers are trusted (unset: all clients behind a proxy share one rate-limit bucket; a correct login password is never throttled either way) | `""` (none) |
| `ALLOWED_EGRESS_CIDRS` | CIDRs that delivery targets may reach despite the SSRF blocklist. Read [Allowing egress to your own network](#allowing-egress-to-your-own-network) before setting it | `""` (none) | | `ALLOWED_EGRESS_CIDRS` | CIDRs that delivery targets may reach despite the SSRF blocklist. Read [Allowing egress to your own network](#allowing-egress-to-your-own-network) before setting it | `""` (none) |
#### Allowing egress to your own network #### Allowing egress to your own network
@@ -379,48 +379,41 @@ unlocked.
`TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare `TRUSTED_PROXIES` is a comma-separated list of CIDR blocks (a bare
address such as `192.168.1.7` is accepted and treated as a single address such as `192.168.1.7` is accepted and treated as a single
host), for example `192.168.1.7, 2001:db8::5`. It decides whose host), for example `192.168.1.7, 2001:db8::5`. It decides whose
`X-Forwarded-For` header the rate limiters believe, so it should cover `X-Forwarded-For` header the rate limiters believe, so it should name
the addresses of your reverse proxies. the addresses of your reverse proxies and nothing else.
`X-Forwarded-For` is honoured **only** when the connecting peer is `X-Forwarded-For` is honoured **only** when the connecting peer is
inside one of these blocks; for every other peer the client identity is inside one of these blocks; for every other peer the client identity is
the connection's own address and the header is ignored. Unset (or the connection's own address and the header is ignored. The default is
empty), the list is the RFC 1918 private ranges: `10.0.0.0/8`, the empty list, which trusts nobody — anything else would let any
`172.16.0.0/12` and `192.168.0.0/16`. That covers a reverse proxy client pick its own rate limit bucket, minting a fresh one per request
reaching webhooker over a Docker network or a private LAN without or draining someone else's. Set it to the address of your reverse
anything set. A set value replaces the default entirely. A set but proxy, and to nothing wider. A set but unparseable value aborts
unparseable value aborts startup. startup.
Trusting those ranges has two consequences for clients with private That default is safe against forged headers, but leaving it unset in
addresses: production has a cost you must know about. Production runs behind a
TLS-terminating reverse proxy, so with `TRUSTED_PROXIES` unset every
- Any such client, whether it connects directly or through the proxy, request keys on the proxy's own address and all clients share a single
can choose its own rate-limit key by sending its own
`X-Forwarded-For`. A direct client's header is walked because the
client is itself trusted; behind the proxy, the client's own address
is skipped as a trusted hop when the chain is walked (below), so the
entry it wrote is taken as the client. If any of your clients have
private addresses, you must set `TRUSTED_PROXIES` to the proxy's
address alone.
- A client behind the proxy that sends no `X-Forwarded-For` of its own
shares the proxy's bucket, because its own address is skipped too.
Setting the list to the proxy's address alone gives each its own
bucket.
A proxy the list does not cover, such as nginx on the same host
reaching webhooker over loopback, is not trusted: every request through
it keys on the proxy's own address and all clients share a single
bucket per limit. The receiver limits become service-wide ceilings, bucket per limit. The receiver limits become service-wide ceilings,
and the login endpoint's failure counting collapses onto one key, so a and the login endpoint's failure counting collapses onto one key, so a
stranger's wrong passwords throttle every other client's wrong stranger's wrong passwords throttle every other client's wrong
passwords. Set `TRUSTED_PROXIES` to that proxy's address to restore passwords.
per-client buckets.
What it cannot do is lock the operator out. The login endpoint What it cannot do is lock the operator out. The login endpoint
verifies credentials **before** it consults any limit and charges only verifies credentials **before** it consults any limit and charges only
failures, so a correct password is never throttled no matter how full failures, so a correct password is never throttled no matter how full
the bucket is. See [Rate Limiting](#rate-limiting). the bucket is. See [Rate Limiting](#rate-limiting).
The remedy is to set `TRUSTED_PROXIES` to your reverse proxy's
address, which restores per-client buckets. webhooker logs a warning
at startup whenever `TRUSTED_PROXIES` is empty, in every environment,
because behind a proxy every client shares one bucket in `dev` and
`prod` alike. The warning is informational when nothing proxies to the
process: with no proxy in front, the peer address is the client's own
and the buckets are already per-client. See
[Rate Limiting](#rate-limiting) for what each limit shares.
`X-Real-IP` and `True-Client-IP` are **never** read, from any peer. `X-Real-IP` and `True-Client-IP` are **never** read, from any peer.
Reverse proxies append to `X-Forwarded-For` but forward other client Reverse proxies append to `X-Forwarded-For` but forward other client
headers verbatim, so a single-valued header is client-controlled even headers verbatim, so a single-valued header is client-controlled even
@@ -442,14 +435,14 @@ Two operator requirements follow:
(nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`, (nginx `$proxy_add_x_forwarded_for`, HAProxy `option forwardfor`,
Caddy and AWS ALB by default), and must append a bare address with Caddy and AWS ALB by default), and must append a bare address with
no port. no port.
- Keep clients out of the list. Any address inside `TRUSTED_PROXIES` - List proxy hosts **only**. Any address inside `TRUSTED_PROXIES`
chooses its own rate-limit key: its `X-Forwarded-For` is walked, so chooses its own rate-limit key: its `X-Forwarded-For` is walked, so
it can name a different address on every request to get a fresh it can name a different address on every request to get a fresh
bucket each time, or name another client's address to drain that bucket each time, or name another client's address to drain that
client's bucket. A block that also covers clients — the default, on client's bucket. Never list a block that also covers clients — a
a network where clients have private addresses — makes all three broad `10.0.0.0/8` on a network where clients live in the same range
limits, including the unauthenticated webhook receiver, silently makes all three limits, including the unauthenticated webhook
bypassable by every client in the block. receiver, silently bypassable by every client in the block.
#### Sessions #### Sessions
@@ -545,6 +538,12 @@ its Argon2id hash. There is no second account and no forgot-password
flow, so the banner and the reset command below are the only two ways flow, so the banner and the reset command below are the only two ways
in. in.
A start that finds no `webhooker.db` in `DATA_DIR` also logs
`created a new, empty database` at `WARN`, with the file's path,
shortly before the banner. On a deployment that has run before, that
line means `DATA_DIR` was empty, most often because its volume is not
mounted.
#### Recovering a lost admin password #### Recovering a lost admin password
`webhooker resetpw` sets an existing account's password from the `webhooker resetpw` sets an existing account's password from the
@@ -559,8 +558,9 @@ printf '%s' "$NEW_PASSWORD" | \
DATA_DIR=/var/lib/webhooker webhooker resetpw admin DATA_DIR=/var/lib/webhooker webhooker resetpw admin
``` ```
In a container it is the same binary, which the image sets as `CMD` In a container it is the same binary. The image's `CMD` is
rather than `ENTRYPOINT`, so the whole command has to be given: `/app/webhooker`, and a command given to `docker run` replaces all of
it, so the whole command has to be given:
```bash ```bash
docker run --rm -v webhooker-data:/var/lib/webhooker \ docker run --rm -v webhooker-data:/var/lib/webhooker \
@@ -697,38 +697,22 @@ those three values rather than trusting the figure. Measured at 65s on
Docker 29.7.2.) A container `unhealthy` with `connection refused` in Docker 29.7.2.) A container `unhealthy` with `connection refused` in
its health log, or a published port that resets connections, is this. its health log, or a published port that resets connections, is this.
The container runs as a non-root user (`webhooker`, UID 1000), exposes The app runs as a non-root user (`webhooker`, UID 1000), exposes port
port 8080, and includes a health check against 8080, and includes a health check against `/.well-known/healthcheck`.
`/.well-known/healthcheck`. The `/var/lib/webhooker` volume holds all The `/var/lib/webhooker` volume holds all SQLite databases: the main
SQLite databases: the main application database (`webhooker.db`), the application database (`webhooker.db`), the per-webhook event databases
per-webhook event databases (`events-{uuid}.db`), and any archive (`events-{uuid}.db`), and any archive databases written by `database`
databases written by `database` targets (`archive-{uuid}.db`). Mount targets (`archive-{uuid}.db`). Mount this as a persistent volume to
this as a persistent volume to preserve data across container preserve data across container restarts.
restarts.
**The bind-mounted directory must be owned by UID 1000, or the **The container sets its data directory's owner and mode itself
container does not start.** Docker creates a `-v` source path that before the app starts**, so a host directory can be mounted as it is,
does not exist yet as `root:root`, and the process runs as UID 1000, whoever owns it. The image's `ENTRYPOINT`,
so it cannot take its `DATA_DIR` lock: `deploy/docker-entrypoint.sh`, starts as root, creates `DATA_DIR` if
it is missing, gives the directory and anything in it that belongs to
``` another user to `webhooker`, sets the directory to `0750`, and only
webhooker: locking data directory /var/lib/webhooker: open then runs the app as `webhooker`. Started with `--user`, it changes
/var/lib/webhooker/webhooker.lock: permission denied nothing and runs the app as that user.
```
It exits non-zero at that point, before opening any database. Create
the directory ahead of the first `docker run`:
```bash
mkdir -p /path/to/data
chown 1000:1000 /path/to/data
chmod 750 /path/to/data
```
The same `chown` is what a restore needs — see step 4 of
[Restore](#restore). A **named volume** does not have this problem:
Docker copies the image's ownership onto a volume it initializes, and
the image creates `/var/lib/webhooker` owned by `webhooker`.
**The file modes are not yours to set, and do not depend on the **The file modes are not yours to set, and do not depend on the
directory.** `webhooker.db` holds target configuration in plaintext — directory.** `webhooker.db` holds target configuration in plaintext —
@@ -736,13 +720,10 @@ bearer tokens, API keys, Slack webhook URLs — along with the session
encryption key, so webhooker creates every SQLite file it owns `0600`: encryption key, so webhooker creates every SQLite file it owns `0600`:
each database and both of its `-wal` and `-shm` sidecars, across all each database and both of its `-wal` and `-shm` sidecars, across all
three tiers. Files an earlier build left `0644` are tightened when three tiers. Files an earlier build left `0644` are tightened when
they are opened. A `DATA_DIR` webhooker creates itself is `0750`, but they are opened. The directory's `0750` is defence in depth — it stops
a bind mount supplies its own directory and Docker's default for one other local users listing the directory and learning your webhook
it creates is `0755`; the `0600` files hold there regardless. The UUIDs from the `events-{uuid}.db` filenames — not the barrier
`chmod 750` above is defence in depth — it stops other local users protecting the credentials.
listing the directory and learning your webhook UUIDs from the
`events-{uuid}.db` filenames — not the barrier protecting the
credentials.
### Running under upaas ### Running under upaas
@@ -758,29 +739,12 @@ repository's `Dockerfile` and runs it. The app needs:
app name, port `8080`. Leave `PORT` unset: the image's health check app name, port `8080`. Leave `PORT` unset: the image's health check
probes `8080`. probes `8080`.
- **Volume:** one host directory mounted at `/var/lib/webhooker`. - **Volume:** one host directory mounted at `/var/lib/webhooker`.
upaas bind-mounts the host path it is given and does not create it,
and the container does not start unless UID 1000 owns it (see
[Running with Docker](#running-with-docker)). Create it before the
first deploy:
```bash
mkdir -p /path/to/data
chown 1000:1000 /path/to/data
chmod 750 /path/to/data
```
- **Environment variables:** - **Environment variables:**
- `WEBHOOKER_ENVIRONMENT=prod` - `WEBHOOKER_ENVIRONMENT=prod`
- `TRUSTED_PROXIES`: Docker networks use private addresses, so the - `TRUSTED_PROXIES`: your reverse proxy's address on that Docker
default covers your reverse proxy on that network. Under the network. The `remoteIP` field of the `http request` log line for a
default, any client with a private address, whether it connects request that came through the proxy shows it; the health check's
directly or through the proxy, can choose its own rate-limit key own lines show `::1`. See [Trusted proxies](#trusted-proxies).
by sending its own `X-Forwarded-For`. If any clients have private
addresses, or the network's addresses are outside the RFC 1918
ranges, set it to the proxy's address there. The `remoteIP` field
of the `http request` log line for a request that came through
the proxy shows it; the health check's own lines show `::1`. See
[Trusted proxies](#trusted-proxies).
- Leave `BIND_ADDRESS` and `DATA_DIR` unset: the image sets - Leave `BIND_ADDRESS` and `DATA_DIR` unset: the image sets
`BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to `BIND_ADDRESS` to `0.0.0.0`, and `DATA_DIR` defaults to
`/var/lib/webhooker`. `/var/lib/webhooker`.
@@ -843,14 +807,12 @@ reports.
behind a proxy means the `X-Forwarded-Proto` header. The block below behind a proxy means the `X-Forwarded-Proto` header. The block below
sets it; without it every request is read as plaintext and cookies sets it; without it every request is read as plaintext and cookies
ship without `Secure`. See [Configuration](#configuration). ship without `Secure`. See [Configuration](#configuration).
3. **Make sure `TRUSTED_PROXIES` covers the proxy's address.** Unset, 3. **Set `TRUSTED_PROXIES` to the proxy's address.** Unset, every rate
it covers the RFC 1918 private ranges, so a proxy on a Docker limiter keys on the connecting peer, which behind a proxy is the
network or a private LAN is covered and one on loopback is not. For proxy on every request: all clients collapse into one global bucket
a proxy it does not cover, every rate limiter keys on the connecting per limit and the receiver's per-IP limits become service-wide
peer, which is the proxy on every request: all clients collapse into ceilings. See [Trusted proxies](#trusted-proxies). List the proxy
one global bucket per limit and the receiver's per-IP limits become and nothing else.
service-wide ceilings. See [Trusted proxies](#trusted-proxies). If
any clients have private addresses, list the proxy and nothing else.
4. **Send `Host` as `$http_host`, not `$host`.** `$host` strips the 4. **Send `Host` as `$http_host`, not `$host`.** `$host` strips the
port. webhooker's Origin/Referer check compares against the host it port. webhooker's Origin/Referer check compares against the host it
was given, so on any port other than 443 `$host` makes every form was given, so on any port other than 443 `$host` makes every form
@@ -1035,12 +997,12 @@ done
`.backup` reads through the WAL and writes a single consistent file with `.backup` reads through the WAL and writes a single consistent file with
no sidecars of its own, so the destination is complete as it stands. no sidecars of its own, so the destination is complete as it stands.
Two caveats. First, the runtime image is `alpine:3.21` with only Two caveats. First, the runtime image is `alpine:3.21` with only
`ca-certificates` added — the `sqlite3` CLI is **not** in it, so run `ca-certificates` and `su-exec` added — the `sqlite3` CLI is **not** in
this on the host against the volume path, or from a throwaway container it, so run this on the host against the volume path, or from a
that mounts the volume. Second, each file is captured at its own throwaway container that mounts the volume. Second, each file is
instant, so a webhook created or an event delivered between two files captured at its own instant, so a webhook created or an event delivered
being copied lands in one and not the other. If you need the whole set between two files being copied lands in one and not the other. If you
coherent as of a single moment, stop the service. need the whole set coherent as of a single moment, stop the service.
Note that `sqlite3 <db> .dump` is **not** one of these procedures: it is Note that `sqlite3 <db> .dump` is **not** one of these procedures: it is
an export, it holds a read transaction open for as long as it runs, and an export, it holds a read transaction open for as long as it runs, and
@@ -1094,21 +1056,11 @@ with any `-wal`/`-shm` beside it, or wait until there are none.
archive not opened since a crash. A copy salvaged from a crashed archive not opened since a crash. A copy salvaged from a crashed
instance has them for everything, and needs all of them. instance has them for everything, and needs all of them.
4. **Fix ownership.** The container runs as the non-root `webhooker` 4. Start the service. The container gives the directory and the
user, UID 1000 / GID 1000. Restored files must be owned by (or restored files to the `webhooker` user before the app starts,
writable by) that UID, and so must the directory itself — SQLite whoever restored them (see
creates the `-wal` and `-shm` sidecars beside the database, so a [Running with Docker](#running-with-docker)). `AutoMigrate` runs
writable file inside a directory it cannot write is not enough: against each restored database as it is opened.
```bash
chown -R 1000:1000 /path/to/data
```
Restoring as `root` on the host and forgetting this step is the
usual way a restore fails.
5. Start the service. `AutoMigrate` runs against each restored database
as it is opened.
### Upgrades ### Upgrades
@@ -1411,11 +1363,10 @@ It uses:
- **[go-chi/httprate](https://github.com/go-chi/httprate)** for - **[go-chi/httprate](https://github.com/go-chi/httprate)** for
sliding-window rate limiting of the password-change and webhook sliding-window rate limiting of the password-change and webhook
receiver endpoints. The bucket is per client IP only when receiver endpoints. The bucket is per client IP only when
`TRUSTED_PROXIES` covers the reverse proxy (by default it covers the `TRUSTED_PROXIES` names the reverse proxy; unset, every client
RFC 1918 private ranges); otherwise every client behind that proxy behind that proxy shares one bucket per limit. The login endpoint
shares one bucket per limit. The login endpoint counts failed counts failed attempts itself instead, so that a correct password is
attempts itself instead, so that a correct password is never never throttled (see [Rate Limiting](#rate-limiting))
throttled (see [Rate Limiting](#rate-limiting))
- **[Prometheus](https://prometheus.io)** for metrics, served at - **[Prometheus](https://prometheus.io)** for metrics, served at
`/metrics` behind basic auth `/metrics` behind basic auth
- **[Sentry](https://sentry.io)** for optional error reporting - **[Sentry](https://sentry.io)** for optional error reporting
@@ -2596,30 +2547,30 @@ let one subscriber rotate source addresses and mint a fresh bucket per
request, evading these limits at the network layer without spoofing request, evading these limits at the network layer without spoofing
anything; the cost is that distinct clients inside one `/64` share a anything; the cost is that distinct clients inside one `/64` share a
bucket. IPv4-mapped addresses (`::ffff:1.2.3.4`) key as the IPv4 address bucket. IPv4-mapped addresses (`::ffff:1.2.3.4`) key as the IPv4 address
they carry. See [Trusted proxies](#trusted-proxies). When that variable they carry. See [Trusted proxies](#trusted-proxies). Deployed without that
does not cover the reverse proxy, a client behind it shares one bucket variable set, a client behind a reverse proxy shares one bucket with
with every other client behind the same proxy. Set `TRUSTED_PROXIES` to every other client behind the same proxy. Set `TRUSTED_PROXIES` to the
the proxy's address to get per-client limits back. What the shared bucket proxy's address to get per-client limits back. What the shared bucket
costs is not the same for every limiter, and the two cases pull in costs is not the same for every limiter, and the two cases pull in
opposite directions: opposite directions:
- For the **receiver** limits it costs throughput, which is the safe - For the **receiver** limits it costs throughput, which is the safe
direction to be wrong in: sharing can only make a limit bind sooner, direction to be wrong in: sharing can only make a limit bind sooner,
never let a sender past it. It matters more for the aggregate limit never let a sender past it. It matters more for the aggregate limit
than for the per-entrypoint one: when `TRUSTED_PROXIES` does not than for the per-entrypoint one: with `TRUSTED_PROXIES` unset behind
cover the reverse proxy a production deployment is required to run the reverse proxy a production deployment is required to run behind,
behind, every request keys on the proxy, so the aggregate limit every request keys on the proxy, so the aggregate limit becomes a
becomes a service-wide ceiling of 1200 requests per minute across all service-wide ceiling of 1200 requests per minute across all senders
senders and all entrypoints, where the per-entrypoint limit's and all entrypoints, where the per-entrypoint limit's capacity still
capacity still grows with the number of entrypoints. Any deployment grows with the number of entrypoints. Any deployment with more than a
with more than a handful of busy entrypoints must make sure handful of busy entrypoints must set `TRUSTED_PROXIES`.
`TRUSTED_PROXIES` covers its proxy.
- For the **login and password-change** limits it costs precision, not - For the **login and password-change** limits it costs precision, not
availability. Login failures from every client land in one counter, availability. Login failures from every client land in one counter,
so a stranger's wrong passwords make the operator's own wrong so a stranger's wrong passwords make the operator's own wrong
passwords answer `429` sooner; the operator's _correct_ password is passwords answer `429` sooner; the operator's _correct_ password is
never affected, because it is never counted. Production deployments never affected, because it is never counted. Production deployments
should still make sure `TRUSTED_PROXIES` covers their proxy. should still set `TRUSTED_PROXIES`; webhooker warns at startup
whenever it is empty, in any environment.
#### The login endpoint #### The login endpoint
@@ -2722,10 +2673,8 @@ re-fills both verification slots on its first two requests. The
remedies are to block the source at the reverse proxy, or to remedies are to block the source at the reverse proxy, or to
rate-limit `POST /pages/login` there — the one place a limit can be rate-limit `POST /pages/login` there — the one place a limit can be
applied without reintroducing the lockout, because the proxy sees the applied without reintroducing the lockout, because the proxy sees the
real client address. `TRUSTED_PROXIES` does not stop the saturation. real client address. Setting `TRUSTED_PROXIES` does not stop the
The flood's source is in the proxy's access log: webhooker's own logs saturation, but it makes the source visible in the failure logs.
record the proxy's address, not the client's (see
[Deployment behind a reverse proxy](#deployment-behind-a-reverse-proxy)).
Finer-grained per-webhook rate limits (configured in the web UI and Finer-grained per-webhook rate limits (configured in the web UI and
enforced in the webhook handler) can layer on top of this env-level enforced in the webhook handler) can layer on top of this env-level
@@ -2739,7 +2688,7 @@ abuse limit later; they are tracked as future work.
| ------ | --------------------------- | ----------- | | ------ | --------------------------- | ----------- |
| `GET` | `/` | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) | | `GET` | `/` | Root redirect, 303 (authenticated → `/sources`, unauthenticated → `/pages/login`) |
| `GET` | `/.well-known/healthcheck` | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) | | `GET` | `/.well-known/healthcheck` | Health check (JSON: `status`, `now`, `uptimeSeconds`, `uptimeHuman`, `version`, `appname`, `maintenanceMode`) |
| any | `/s/*` | Static file serving (embedded CSS, JS). Mounted for every method, not just `GET`/`HEAD`: chi's `Mount` registers all methods and `http.FileServer` special-cases only `HEAD` (by omitting the body), so a `POST` or `DELETE` to an asset is answered `200` with the file. Pinned by `TestStaticServesEveryMethod` | | `GET`, `HEAD` | `/s/*` | Static file serving (embedded CSS, JS). `GET` and `HEAD` only — `POST`, `PUT`, `PATCH`, `DELETE`, `OPTIONS`, `TRACE` and `CONNECT` are answered `405 Method Not Allowed` with `Allow: GET, HEAD`. Any other method (such as `PROPFIND`) is refused by chi before it reaches this route, and gets `405` without an `Allow` header. Pinned by `TestStaticServesOnlyGetAndHead` |
| `POST` | `/webhook/{uuid}` | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) | | `POST` | `/webhook/{uuid}` | Webhook receiver endpoint. `POST` only — every other method is answered `405 Method Not Allowed` with `Allow: POST`. Rate limited (see [Rate Limiting](#rate-limiting)) |
#### Authentication Endpoints #### Authentication Endpoints
@@ -3082,13 +3031,18 @@ check, see [The login endpoint](#the-login-endpoint).
It runs behind session auth, so only a client already holding a It runs behind session auth, so only a client already holding a
valid session reaches it, and an operator throttled out of changing valid session reaches it, and an operator throttled out of changing
a password can still log in. The bucket is per client IP only when a password can still log in. The bucket is per client IP only when
`TRUSTED_PROXIES` covers the reverse proxy; otherwise every client `TRUSTED_PROXIES` names the reverse proxy; unset, every client
shares one bucket, which costs precision rather than availability shares one bucket, which costs precision rather than availability
(see [Rate Limiting](#rate-limiting)) (see [Rate Limiting](#rate-limiting)). webhooker warns at startup
whenever `TRUSTED_PROXIES` is empty
- Prometheus metrics behind basic auth - Prometheus metrics behind basic auth
- Static assets embedded in binary (no filesystem access needed at - Static assets embedded in binary (no filesystem access needed at
runtime) runtime)
- Container runs as non-root user (UID 1000) - The app runs as the non-root `webhooker` user (UID 1000) in the
container. The image sets no `USER`, so these run as root: the
`ENTRYPOINT` script, which sets the data directory's owner and mode
before the app starts; the image's health check; and `docker exec`,
unless given `--user`
- GORM soft deletes on every entity that carries `BaseModel`, which is - GORM soft deletes on every entity that carries `BaseModel`, which is
all of them but `Setting` (data preserved for audit) all of them but `Setting` (data preserved for audit)
@@ -3215,10 +3169,13 @@ version is fixed independently of the compiler's:
`GO_LDFLAGS`, so neither can drop the `-X` that stamps the version. `GO_LDFLAGS`, so neither can drop the `-X` that stamps the version.
The version arrives as the `VERSION` build arg, since the context The version arrives as the `VERSION` build arg, since the context
has no `.git` (see [Version stamping](#version-stamping)). has no `.git` (see [Version stamping](#version-stamping)).
3. **Runtime stage** (`alpine:3.21`) — copies the static binary, 3. **Runtime stage** (`alpine:3.21`) — copies the static binary and
creates the `/var/lib/webhooker` directory for all SQLite databases, `deploy/docker-entrypoint.sh`, creates the `/var/lib/webhooker`
runs as the non-root `webhooker` user (UID 1000), exposes port 8080, directory for all SQLite databases, exposes port 8080, and includes
and includes a health check against `/.well-known/healthcheck`. a health check against `/.well-known/healthcheck`. It sets no
`USER`: the `ENTRYPOINT` script starts as root, sets the data
directory's owner and mode, and runs the app as the non-root
`webhooker` user (UID 1000) through `su-exec`.
The lint stage invokes `golangci-lint` directly rather than `make lint`: The lint stage invokes `golangci-lint` directly rather than `make lint`:
it is already the pinned linter image, and `make lint` builds it is already the pinned linter image, and `make lint` builds
@@ -3297,3 +3254,5 @@ MIT
## Author ## Author
[@sneak](https://sneak.berlin) [@sneak](https://sneak.berlin)
+22
View File
@@ -0,0 +1,22 @@
#!/bin/sh
# deploy/docker-entrypoint.sh: the image's ENTRYPOINT. A bind-mounted
# data directory keeps its owner from the host, often root, and the app
# could not write to it. Started as root, this creates DATA_DIR if
# needed, gives it and everything in it to webhooker, sets its mode, and
# runs the command as webhooker, so the app never runs as root. Started
# as another user, it only runs the command.
set -eu
main() {
if [ "$(id -u)" != 0 ]; then
exec "$@"
fi
dir="${DATA_DIR:-/var/lib/webhooker}"
mkdir -p "$dir"
find "$dir" ! -user webhooker -exec chown -h webhooker:webhooker {} +
chmod 750 "$dir"
exec su-exec webhooker "$@"
}
main "$@"
+60 -22
View File
@@ -75,11 +75,6 @@ const (
// internet-exposed endpoint. // internet-exposed endpoint.
defaultReceiverRateLimit = 120 defaultReceiverRateLimit = 120
// defaultTrustedProxies is TRUSTED_PROXIES when it is unset: the
// RFC 1918 private ranges, which a reverse proxy reaching the
// process over a Docker network or a private LAN connects from.
defaultTrustedProxies = "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16"
// maxPort is the highest valid TCP port number. The lower // maxPort is the highest valid TCP port number. The lower
// bound (at least 1) is enforced by envPositiveInt. // bound (at least 1) is enforced by envPositiveInt.
maxPort = 65535 maxPort = 65535
@@ -177,14 +172,13 @@ type Config struct {
// TrustedProxies is the set of networks whose members are // TrustedProxies is the set of networks whose members are
// allowed to speak for the client with X-Forwarded-For, the // allowed to speak for the client with X-Forwarded-For, the
// only forwarded header read. Unless TRUSTED_PROXIES is set it // only forwarded header read. It is empty unless
// is the RFC 1918 private ranges (defaultTrustedProxies). // TRUSTED_PROXIES is set, and empty means no peer is
// Other peers' forwarded headers are ignored and they are // trusted: forwarded headers are then ignored entirely and
// identified by the connection's own address. Under the // clients are identified by the connection's own address.
// default any client with a private address, directly or // Members can choose their own rate-limit key, so this must
// through a proxy, can choose its own rate-limit key, so // name proxy hosts only, never a block that also covers
// where any clients have private addresses this must be set // clients.
// to the proxy hosts alone.
TrustedProxies []netip.Prefix TrustedProxies []netip.Prefix
// AllowedEgressCIDRs is the set of networks a delivery target // AllowedEgressCIDRs is the set of networks a delivery target
@@ -466,15 +460,14 @@ func parseCIDR(entry string) (netip.Prefix, error) {
// envPrefixList returns the value of the named environment variable // envPrefixList returns the value of the named environment variable
// parsed as a comma-separated list of CIDR blocks (bare addresses // parsed as a comma-separated list of CIDR blocks (bare addresses
// allowed). An unset, empty, or blank value is read as defaultValue // allowed). An unset, empty, or blank value yields an empty list. A
// instead. A set value containing an unparseable entry is a hard // set value containing an unparseable entry is a hard error naming
// error naming the key and the bad entry, so startup fails loudly // the key and the bad entry, so startup fails loudly rather than
// rather than silently running with a list the operator did not // silently running with a list the operator did not intend.
// intend. func envPrefixList(key string) ([]netip.Prefix, error) {
func envPrefixList(key, defaultValue string) ([]netip.Prefix, error) {
v := strings.TrimSpace(os.Getenv(key)) v := strings.TrimSpace(os.Getenv(key))
if v == "" { if v == "" {
v = defaultValue return nil, nil
} }
var prefixes []netip.Prefix var prefixes []netip.Prefix
@@ -688,12 +681,12 @@ func loadFromEnv() (*Config, error) {
return nil, err return nil, err
} }
trustedProxies, err := envPrefixList("TRUSTED_PROXIES", defaultTrustedProxies) trustedProxies, err := envPrefixList("TRUSTED_PROXIES")
if err != nil { if err != nil {
return nil, err return nil, err
} }
allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS", "") allowedEgressCIDRs, err := envPrefixList("ALLOWED_EGRESS_CIDRS")
if err != nil { if err != nil {
return nil, err return nil, err
} }
@@ -767,6 +760,50 @@ func (c *Config) warnEgressAllowlist(log *slog.Logger) {
) )
} }
// warnSharedRateLimitBucket logs a startup warning whenever
// TRUSTED_PROXIES is empty, in any environment.
//
// With no trusted proxies every rate limiter keys on the connecting
// peer's address. Whether that is harmless or dangerous depends on
// what is in front of the process, which this code cannot observe:
// with nothing in front, the peer is the client and the limits are
// per-client as intended; behind a reverse proxy the peer is the proxy
// for every request, so all clients share one bucket per limiter.
//
// The login endpoint no longer spends budget on arrival — it verifies
// credentials first and charges only failures — so a shared bucket
// cannot deny the operator a correct password. What it does collapse
// is the failure counting: one client's wrong passwords throttle
// everyone else's wrong passwords, and the receiver's limits become
// service-wide ceilings.
//
// The warning is deliberately not gated on WEBHOOKER_ENVIRONMENT:
// behind a proxy every client shares one bucket in dev and prod alike.
//
// The default of trusting nobody is deliberate — trusting forwarded
// headers from arbitrary peers lets any client choose its own bucket —
// so this warns rather than failing startup or changing the key.
func (c *Config) warnSharedRateLimitBucket(log *slog.Logger) {
if len(c.TrustedProxies) > 0 {
return
}
log.Warn(
"TRUSTED_PROXIES is empty: every rate limit keys on the "+
"connecting peer's address. With nothing proxying to "+
"this process that is the client itself and the limits "+
"are per-client as intended. Behind a reverse proxy the "+
"peer is the proxy on every request, so all clients "+
"share one bucket per limit: the receiver limits become "+
"service-wide ceilings, and one client's failed logins "+
"throttle every other client's failed logins — a "+
"correct password still gets in. If anything proxies to "+
"this process, set TRUSTED_PROXIES to its address.",
"environment", c.Environment,
"trustedProxies", len(c.TrustedProxies),
)
}
// New creates a Config by reading environment variables. // New creates a Config by reading environment variables.
// //
//nolint:revive // lc parameter is required by fx even if unused. //nolint:revive // lc parameter is required by fx even if unused.
@@ -812,6 +849,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
"hasMetricsAuth", s.MetricsAuthEnabled(), "hasMetricsAuth", s.MetricsAuthEnabled(),
) )
s.warnSharedRateLimitBucket(log)
s.warnEgressAllowlist(log) s.warnEgressAllowlist(log)
return s, nil return s, nil
+101 -14
View File
@@ -551,11 +551,6 @@ func testReceiverRateLimitSuccess(
} }
func TestTrustedProxies(t *testing.T) { func TestTrustedProxies(t *testing.T) {
// Unset, the RFC 1918 private ranges are trusted, so a reverse
// proxy on a Docker network or a private LAN is covered without
// configuration.
defaultProxies := []string{cidrPrivateV4, "172.16.0.0/12", "192.168.0.0/16"}
tests := []struct { tests := []struct {
name string name string
set bool set bool
@@ -564,21 +559,18 @@ func TestTrustedProxies(t *testing.T) {
expected []string expected []string
}{ }{
{ {
// The default must be "trust nobody": an empty list
// means forwarded headers are ignored, never that
// every peer may speak for the client.
name: caseUnsetUsesDefault, name: caseUnsetUsesDefault,
set: false, set: false,
expected: defaultProxies, expected: []string{},
}, },
{ {
name: "blank value uses default", name: "blank value trusts nothing",
set: true, set: true,
value: " ", value: " ",
expected: defaultProxies, expected: []string{},
},
{
name: "set value replaces the default entirely",
set: true,
value: "203.0.113.7",
expected: []string{"203.0.113.7/32"},
}, },
{ {
name: caseValidValueParsed, name: caseValidValueParsed,
@@ -853,6 +845,101 @@ func TestEgressAllowlistWarning(t *testing.T) {
} }
} }
// TestSharedRateLimitBucketWarning covers the startup warning that
// tells an operator a deployment behind a reverse proxy shares one
// rate-limit bucket between every client, which turns the receiver
// limits into service-wide ceilings and collapses login failure
// counting. It must fire whenever TRUSTED_PROXIES is empty, in any
// environment, because behind a proxy every client shares one bucket
// in dev and prod alike. It stays quiet once proxies are named.
func TestSharedRateLimitBucketWarning(t *testing.T) {
tests := []struct {
name string
environment string
trustedProxies string
expectWarning bool
}{
{
name: "prod without trusted proxies warns",
environment: config.EnvironmentProd,
expectWarning: true,
},
{
name: "prod with trusted proxies is quiet",
environment: config.EnvironmentProd,
trustedProxies: cidrPrivateV4,
expectWarning: false,
},
{
name: "dev without trusted proxies warns",
environment: config.EnvironmentDev,
expectWarning: true,
},
{
name: "dev with trusted proxies is quiet",
environment: config.EnvironmentDev,
trustedProxies: cidrPrivateV4,
expectWarning: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
// Cannot use t.Parallel() here because t.Setenv
// is incompatible with parallel subtests.
t.Setenv("WEBHOOKER_ENVIRONMENT", tt.environment)
if tt.trustedProxies == "" {
require.NoError(
t, os.Unsetenv("TRUSTED_PROXIES"),
)
} else {
t.Setenv("TRUSTED_PROXIES", tt.trustedProxies)
}
var buf bytes.Buffer
log := slog.New(slog.NewJSONHandler(
&buf, &slog.HandlerOptions{
Level: slog.LevelDebug,
},
))
require.NoError(
t,
config.WarnSharedRateLimitBucketForTest(log),
)
if !tt.expectWarning {
assert.Empty(t, buf.String())
return
}
logged := buf.String()
assert.Contains(t, logged, `"level":"WARN"`)
assert.Contains(t, logged, "TRUSTED_PROXIES")
assert.Contains(t, logged, "share one bucket")
assert.Contains(
t, logged, "throttle every other client's failed logins",
)
// The warning must not claim a lockout the login
// endpoint no longer permits: credentials are verified
// before any budget is spent.
assert.Contains(
t, logged, "a correct password still gets in",
)
// The text must stay accurate for a developer with
// nothing in front of the process, where an empty
// list costs nothing.
assert.Contains(
t, logged, "nothing proxying to this process",
)
})
}
}
// metricsEnv describes what one subtest below puts in the // metricsEnv describes what one subtest below puts in the
// environment for a single METRICS_ variable. A variable that is // environment for a single METRICS_ variable. A variable that is
// set to the empty string and one that is not set at all are // set to the empty string and one that is not set at all are
+15
View File
@@ -6,6 +6,21 @@ import "log/slog"
// the external config_test package so each helper can be covered by // the external config_test package so each helper can be covered by
// its own table-driven test without weakening the package API. // its own table-driven test without weakening the package API.
// WarnSharedRateLimitBucketForTest loads a Config from the current
// environment and emits its startup warnings to log. The real logger
// writes to stdout, so this lets the warning's firing condition be
// asserted against a handler the test controls.
func WarnSharedRateLimitBucketForTest(log *slog.Logger) error {
c, err := loadFromEnv()
if err != nil {
return err
}
c.warnSharedRateLimitBucket(log)
return nil
}
// WarnEgressAllowlistForTest loads a Config from the current // WarnEgressAllowlistForTest loads a Config from the current
// environment and emits its egress-allowlist startup warning to // environment and emits its egress-allowlist startup warning to
// log, so a test can assert both that the warning fires only when // log, so a test can assert both that the warning fires only when
@@ -3,6 +3,8 @@ package database_test
import ( import (
"bytes" "bytes"
"context" "context"
"log/slog"
"path/filepath"
"strings" "strings"
"testing" "testing"
@@ -83,3 +85,37 @@ func TestFirstBoot_PrintsTheAdminPasswordAsABanner(t *testing.T) {
t, ok, "the printed password must open the seeded account", t, ok, "the printed password must open the seeded account",
) )
} }
// TestNewDatabase_IsLoggedWithItsPath is the log half of
// https://git.eeqj.de/sneak/webhooker/issues/359. A DATA_DIR that is
// unexpectedly empty boots exactly like a first start, so the start
// that creates the database must say so, and where. Opening that
// database again must not.
func TestNewDatabase_IsLoggedWithItsPath(t *testing.T) {
t.Parallel()
dir := t.TempDir()
open := func() string {
var out bytes.Buffer
db, err := database.Open(dir, slog.New(slog.NewTextHandler(&out, nil)))
require.NoError(t, err)
require.NoError(t, db.Close())
return out.String()
}
const created = `level=WARN msg="created a new, empty database"`
first := open()
second := open()
assert.Contains(
t, first,
created+" path="+filepath.Join(dir, database.MainDBFileName),
)
assert.NotContains(
t, second, created, "an existing database is not new",
)
}
+12
View File
@@ -8,6 +8,7 @@ import (
"errors" "errors"
"fmt" "fmt"
"io" "io"
"io/fs"
"log/slog" "log/slog"
"os" "os"
"path/filepath" "path/filepath"
@@ -199,6 +200,12 @@ func (d *Database) connectTo(dataDir string) error {
// Construct the main application database path inside DATA_DIR. // Construct the main application database path inside DATA_DIR.
dbPath := filepath.Join(dataDir, MainDBFileName) dbPath := filepath.Join(dataDir, MainDBFileName)
// Checked before opening, which creates the file. A DATA_DIR that
// is unexpectedly empty -- its volume not mounted, say -- looks
// exactly like a first start, so a new database is a warning.
_, statErr := os.Stat(dbPath)
created := errors.Is(statErr, fs.ErrNotExist)
// Opened through OpenSQLite so this handle carries the same WAL // Opened through OpenSQLite so this handle carries the same WAL
// journaling, busy timeout, immediate-transaction locking, and pool // journaling, busy timeout, immediate-transaction locking, and pool
// bounds as every other database file. See sqlite_open.go. // bounds as every other database file. See sqlite_open.go.
@@ -229,7 +236,12 @@ func (d *Database) connectTo(dataDir string) error {
} }
d.db = db d.db = db
if created {
d.log.Warn("created a new, empty database", "path", dbPath)
} else {
d.log.Info("connected to database", "path", dbPath) d.log.Info("connected to database", "path", dbPath)
}
// Run migrations // Run migrations
return d.migrate() return d.migrate()
+3 -4
View File
@@ -103,10 +103,9 @@ func (h *Handlers) renderLoginError(
// The credential check runs BEFORE any rate-limit budget is // The credential check runs BEFORE any rate-limit budget is
// consulted, and only a failed check spends budget. That is what // consulted, and only a failed check spends budget. That is what
// keeps the single administrative path reachable: behind the reverse // keeps the single administrative path reachable: behind the reverse
// proxy this deployment requires, when TRUSTED_PROXIES does not cover // proxy this deployment requires, with TRUSTED_PROXIES unset, every
// it, every client shares one bucket, so a limiter spent on arrival // client shares one bucket, so a limiter spent on arrival lets any
// lets any stranger deny the operator's own correct password // stranger deny the operator's own correct password indefinitely.
// indefinitely.
// //
// Verifying first means every login POST costs an Argon2id hash, so // Verifying first means every login POST costs an Argon2id hash, so
// the work is taken under a bounded number of verification slots. // the work is taken under a bounded number of verification slots.
+6 -6
View File
@@ -25,7 +25,7 @@ const (
// sharedProxyPeer is the whole point of this file. Production is // sharedProxyPeer is the whole point of this file. Production is
// required to run behind a TLS-terminating reverse proxy, and // required to run behind a TLS-terminating reverse proxy, and
// when TRUSTED_PROXIES does not cover it every client — attacker // TRUSTED_PROXIES defaults to empty, so every client — attacker
// and operator alike — reaches the process from the proxy's // and operator alike — reaches the process from the proxy's
// address and shares one rate-limit bucket. Both parties in // address and shares one rate-limit bucket. Both parties in
// these tests therefore use the same RemoteAddr. // these tests therefore use the same RemoteAddr.
@@ -115,11 +115,11 @@ func floodFailures(
// done-criterion of https://git.eeqj.de/sneak/webhooker/issues/150. // done-criterion of https://git.eeqj.de/sneak/webhooker/issues/150.
// //
// The attacker and the operator share one rate-limit bucket, because // The attacker and the operator share one rate-limit bucket, because
// behind the mandated reverse proxy, when TRUSTED_PROXIES does not // behind the mandated reverse proxy with TRUSTED_PROXIES unset every
// cover it, every client keys on the proxy's address. The attacker // client keys on the proxy's address. The attacker floods the
// floods the operator's own username — a single-admin product has a // operator's own username — a single-admin product has a predictable
// predictable one — far past the failure limit. The operator must // one — far past the failure limit. The operator must still be able
// still be able to log in with the correct password. // to log in with the correct password.
// //
// This fails if credentials stop being verified ahead of the limiter. // This fails if credentials stop being verified ahead of the limiter.
func TestLogin_StrangersFloodCannotLockOutTheOperator(t *testing.T) { func TestLogin_StrangersFloodCannotLockOutTheOperator(t *testing.T) {
+4 -4
View File
@@ -108,10 +108,10 @@ type failureWindow struct {
// //
// A limiter that spends budget on arrival cannot protect a // A limiter that spends budget on arrival cannot protect a
// single-admin product: behind the reverse proxy the deployment // single-admin product: behind the reverse proxy the deployment
// requires, when TRUSTED_PROXIES does not cover it, every client // requires, with TRUSTED_PROXIES unset, every client keys on the
// keys on the proxy, so a stranger trickling five POSTs a minute // proxy, so a stranger trickling five POSTs a minute keeps the one
// keeps the one bucket full and the operator's own correct password // bucket full and the operator's own correct password is answered 429
// is answered 429 forever. There is no second administrative path. // forever. There is no second administrative path.
// //
// So budget is spent only by a FAILED verification. A correct // So budget is spent only by a FAILED verification. A correct
// password is never throttled, whatever the counters say, which is // password is never throttled, whatever the counters say, which is
+3 -2
View File
@@ -123,8 +123,9 @@ func bucketKey(addr netip.Addr) string {
return prefix.String() return prefix.String()
} }
// isTrustedProxy reports whether addr belongs to a network in // isTrustedProxy reports whether addr belongs to a network the
// TRUSTED_PROXIES, which by default is the RFC 1918 private ranges. // operator listed in TRUSTED_PROXIES. The list is empty by default,
// so by default nothing is trusted.
func (m *Middleware) isTrustedProxy(addr netip.Addr) bool { func (m *Middleware) isTrustedProxy(addr netip.Addr) bool {
for _, prefix := range m.params.Config.TrustedProxies { for _, prefix := range m.params.Config.TrustedProxies {
if prefix.Contains(addr) { if prefix.Contains(addr) {
+2 -2
View File
@@ -426,8 +426,8 @@ func assertSharedBucket(
} }
// TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test // TestRateLimitKey_SpoofedForwardedFromUntrustedPeer is the test
// this gating exists for: from a peer that is not a trusted // this gating exists for: with no trusted proxies configured (the
// proxy, a client that rotates a forwarded header on every // default), a client that rotates a forwarded header on every
// request must stay in one bucket. If forwarded headers were // request must stay in one bucket. If forwarded headers were
// trusted unconditionally, each spoofed value would mint a fresh // trusted unconditionally, each spoofed value would mint a fresh
// bucket and the limit would stop no one. // bucket and the limit would stop no one.
+22 -9
View File
@@ -92,11 +92,25 @@ func (s *Server) setupGlobalMiddleware() {
func (s *Server) setupRoutes() { func (s *Server) setupRoutes() {
s.router.Get("/", s.h.HandleIndex()) s.router.Get("/", s.h.HandleIndex())
s.router.Mount( // Static assets answer GET and HEAD only. chi's default 405
"/s", // carries no Allow header, so this group supplies its own.
http.StripPrefix("/s", http.FileServer(http.FS(static.Static))), staticFiles := http.StripPrefix(
"/s", http.FileServer(http.FS(static.Static)),
) )
s.router.Route("/s", func(r chi.Router) {
r.MethodNotAllowed(func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Allow", "GET, HEAD")
http.Error(
w,
"Method Not Allowed",
http.StatusMethodNotAllowed,
)
})
r.Method(http.MethodGet, "/*", staticFiles)
r.Method(http.MethodHead, "/*", staticFiles)
})
s.router.Route("/api/v1", func(_ chi.Router) { s.router.Route("/api/v1", func(_ chi.Router) {
// API routes will be added here. // API routes will be added here.
}) })
@@ -140,12 +154,11 @@ func (s *Server) setupPageRoutes() {
r.Use(s.mw.NoCache()) r.Use(s.mw.NoCache())
// The login POST carries no pre-emptive rate limiter. Behind // The login POST carries no pre-emptive rate limiter. Behind
// the reverse proxy production requires, when TRUSTED_PROXIES // the reverse proxy production requires, with TRUSTED_PROXIES
// does not cover it, every client shares one bucket, so a // unset, every client shares one bucket, so a limiter spent
// limiter spent on arrival lets any stranger deny the operator // on arrival lets any stranger deny the operator the only
// the only administrative path. The handler verifies // administrative path. The handler verifies credentials first
// credentials first and charges only failures; see // and charges only failures; see Handlers.authenticateUser.
// Handlers.authenticateUser.
r.Get("/login", s.h.HandleLoginPage()) r.Get("/login", s.h.HandleLoginPage())
r.Post("/login", s.h.HandleLoginSubmit()) r.Post("/login", s.h.HandleLoginSubmit())
+109 -20
View File
@@ -7,6 +7,7 @@ import (
"net/http/httptest" "net/http/httptest"
"net/url" "net/url"
"regexp" "regexp"
"slices"
"strconv" "strconv"
"strings" "strings"
"testing" "testing"
@@ -220,9 +221,21 @@ func (e *testEnv) csrfFrom(
// out of the markup has to be unescaped before it is submitted. // out of the markup has to be unescaped before it is submitted.
token := html.UnescapeString(match[1]) token := html.UnescapeString(match[1])
combined := make([]*http.Cookie, 0, len(cookies)) // A cookie the page sets replaces the one of the same name, as in
combined = append(combined, cookies...) // a browser. Sent both, the server would read the first, older one.
combined = append(combined, w.Result().Cookies()...) set := w.Result().Cookies()
combined := make([]*http.Cookie, 0, len(cookies)+len(set))
for _, c := range cookies {
replaced := slices.ContainsFunc(set, func(n *http.Cookie) bool {
return n.Name == c.Name
})
if !replaced {
combined = append(combined, c)
}
}
combined = append(combined, set...)
return token, combined return token, combined
} }
@@ -396,13 +409,15 @@ func (e *testEnv) storedHash(t *testing.T, username string) string {
// --- /s static group --- // --- /s static group ---
// TestStaticServesEveryMethod pins what the static mount actually // TestStaticServesOnlyGetAndHead pins the methods the static group
// answers. chi's Mount registers the handler for all methods and // answers: GET and HEAD are served the asset, and the other methods
// http.FileServer only special-cases HEAD (by suppressing the body), // chi routes (POST, PUT, DELETE and the rest) are refused with 405
// so a POST or a DELETE to an asset is served the file rather than // and an Allow header naming those two. A method chi does not route,
// refused. The README documents this; the test is what keeps the two // such as PROPFIND, is refused with 405 by the top-level router
// from drifting. // before it reaches the static group, so it gets no Allow header.
func TestStaticServesEveryMethod(t *testing.T) { // The README documents this; the test is what keeps the two from
// drifting.
func TestStaticServesOnlyGetAndHead(t *testing.T) {
t.Parallel() t.Parallel()
env := newTestEnv(t) env := newTestEnv(t)
@@ -417,6 +432,7 @@ func TestStaticServesEveryMethod(t *testing.T) {
http.MethodPost, http.MethodPost,
http.MethodPut, http.MethodPut,
http.MethodDelete, http.MethodDelete,
"PROPFIND",
} { } {
t.Run(method, func(t *testing.T) { t.Run(method, func(t *testing.T) {
t.Parallel() t.Parallel()
@@ -428,18 +444,38 @@ func TestStaticServesEveryMethod(t *testing.T) {
w := httptest.NewRecorder() w := httptest.NewRecorder()
env.router.ServeHTTP(w, req) env.router.ServeHTTP(w, req)
assert.Equal(t, http.StatusOK, w.Code, switch method {
"static mount answers every method") case http.MethodGet:
assert.Equal(t, http.StatusOK, w.Code)
if method == http.MethodHead {
assert.Empty(t, w.Body.Bytes(),
"HEAD must not carry a body")
return
}
assert.Equal(t, body, w.Body.Bytes(), assert.Equal(t, body, w.Body.Bytes(),
"the asset itself is returned") "the asset itself is returned")
case http.MethodHead:
assert.Equal(t, http.StatusOK, w.Code)
assert.Empty(t, w.Body.Bytes(),
"HEAD must not carry a body")
case "PROPFIND":
assert.Equal(
t, http.StatusMethodNotAllowed, w.Code,
)
assert.Empty(t, w.Header().Get("Allow"),
"chi refuses a method it does not route "+
"before the static group runs")
assert.NotContains(
t, w.Body.String(), string(body),
"a refused method must not get the asset",
)
default:
assert.Equal(
t, http.StatusMethodNotAllowed, w.Code,
)
assert.Equal(
t, "GET, HEAD", w.Header().Get("Allow"),
)
assert.NotContains(
t, w.Body.String(), string(body),
"a refused method must not get the asset",
)
}
}) })
} }
} }
@@ -591,6 +627,59 @@ func TestPagesLogin_CorrectPasswordSurvivesASpentBudget(
) )
} }
// TestPagesLogin_CookiesFromAnEarlierDatabase is
// https://git.eeqj.de/sneak/webhooker/issues/359. A new database
// brings a new session key, and the operator's browser still holds
// the session and CSRF cookies signed with the old one. Logging in
// must work as from a fresh browser and leave cookies the new key
// accepts.
func TestPagesLogin_CookiesFromAnEarlierDatabase(t *testing.T) {
t.Parallel()
const (
username = "operator"
password = "correct-horse-battery-staple"
)
earlier := newTestEnv(t)
earlierID, _ := earlier.seedUser(t, username, password)
_, stale := earlier.csrfFrom(t, "/pages/login", nil)
stale = append(stale, earlier.authCookies(t, earlierID, username)...)
env := newTestEnv(t)
env.seedUser(t, username, password)
token, cookies := env.csrfFrom(t, "/pages/login", stale)
form := url.Values{}
form.Set("csrf_token", token)
form.Set("username", username)
form.Set("password", password)
w := env.post("/pages/login", form, cookies)
require.Equal(
t, http.StatusSeeOther, w.Code,
"a session cookie from another key must not fail the login",
)
// The response deletes the old session cookie and then sets the
// new one; a browser keeps the last.
var fresh *http.Cookie
for _, c := range w.Result().Cookies() {
if c.Name == session.SessionName {
fresh = c
}
}
require.NotNil(t, fresh, "login must set a session cookie")
assert.Equal(
t, "/sources",
env.get("/", []*http.Cookie{fresh}).Header().Get("Location"),
"the new session cookie must authenticate",
)
}
// --- /user/{username} group --- // --- /user/{username} group ---
// TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged // TestPasswordChange_OversizeBody_RejectedAndPasswordUnchanged
+8 -7
View File
@@ -19,8 +19,8 @@ import (
) )
// The tests below exercise the securecookie codecs underneath the // The tests below exercise the securecookie codecs underneath the
// store and nothing else: Session.Get only decodes, so no server-side // store and nothing else: they decode through the store itself, so no
// expiry check takes part in the result. They exist because // server-side expiry check takes part in the result. They exist because
// NewCookieStore gives its codecs a 30-day max age that assigning // NewCookieStore gives its codecs a 30-day max age that assigning
// store.Options does not override, which would let the codec accept a // store.Options does not override, which would let the codec accept a
// cookie weeks past the cap the cookie attribute advertises. // cookie weeks past the cap the cookie attribute advertises.
@@ -75,10 +75,11 @@ func restamp(
return base64.URLEncoding.EncodeToString(payload) return base64.URLEncoding.EncodeToString(payload)
} }
// decodeCookie feeds value back through the store's decode path. // decodeCookie feeds value back through the store's decode path. It
// asks the store rather than Session.Get, which treats a cookie that
// does not decode as absent and so hides the codec's reason.
func decodeCookie( func decodeCookie(
t *testing.T, t *testing.T,
s *session.Session,
value string, value string,
) (*sessions.Session, error) { ) (*sessions.Session, error) {
t.Helper() t.Helper()
@@ -94,7 +95,7 @@ func decodeCookie(
SameSite: http.SameSiteLaxMode, SameSite: http.SameSiteLaxMode,
}) })
sess, err := s.Get(req) sess, err := session.NewStore(testKey()).Get(req, session.SessionName)
require.NotNil(t, sess) require.NotNil(t, sess)
return sess, err return sess, err
@@ -105,7 +106,7 @@ func TestCodec_AcceptsCookieInsideAbsoluteCap(t *testing.T) {
s := testSession(t) s := testSession(t)
sess, err := decodeCookie(t, s, restamp( sess, err := decodeCookie(t, restamp(
t, t,
issuedCookie(t, s), issuedCookie(t, s),
time.Now().Add(-(testAbsoluteMaxAge-time.Hour)), time.Now().Add(-(testAbsoluteMaxAge-time.Hour)),
@@ -126,7 +127,7 @@ func TestCodec_RejectsCookiePastAbsoluteCap(t *testing.T) {
s := testSession(t) s := testSession(t)
sess, err := decodeCookie(t, s, restamp( sess, err := decodeCookie(t, restamp(
t, t,
issuedCookie(t, s), issuedCookie(t, s),
time.Now().Add(-(testAbsoluteMaxAge+time.Hour)), time.Now().Add(-(testAbsoluteMaxAge+time.Hour)),
+13 -1
View File
@@ -224,10 +224,22 @@ func New(
} }
// Get retrieves a session for the request. // Get retrieves a session for the request.
//
// A session cookie that does not decode -- one signed with an earlier
// session key, say, because the database was made anew -- is treated
// as absent: the caller gets a new, empty session and no error, and
// the next save replaces the cookie.
func (s *Session) Get( func (s *Session) Get(
r *http.Request, r *http.Request,
) (*sessions.Session, error) { ) (*sessions.Session, error) {
return s.store.Get(r, SessionName) sess, err := s.store.Get(r, SessionName)
if sess == nil {
return nil, err
}
// For a cookie that does not decode, gorilla/sessions returns a
// new, empty session alongside the error that is dropped here.
return sess, nil
} }
// GetKey returns the raw 32-byte authentication key used for // GetKey returns the raw 32-byte authentication key used for