Comment blocks in cmd/minstrel and 30 smaller internal packages, rewritten
to rule 32's standard with each citation read in context: task, milestone
and rule ids go, and so do the stories they were attached to ("used to",
"Before #N", measurements from one install, quotes, pointers to Fabled
Librarian's records and to docs/superpowers files that are not in the
repo). Where a story wrapped a real constraint, the constraint stays as a
present-tense fact.
Comments that had gone stale are corrected:
- taste.Config: the tuned values are overlaid by internal/recsettings;
it is no longer "a later follow-up".
- StorageConfig: the data dir holds art, caches and the stream secret,
not only playlist covers.
- the Subsonic album list and getUser comments describe what they do
today rather than a milestone that would change it.
Checked: the non-comment token stream of every changed file matches
HEAD, and gofmt -l is empty.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
buildResetURL used r.Host and r.TLS, so a forgot-password request with a
forged Host emailed the victim a real reset token on a link to the
attacker's server. Links now come only from network_settings.public_url
(migration 0062), and no reset email is sent while it is empty; the response
stays the same opaque 200 and the log says why.
The address is set on a new "Public address" card under Admin → Integrations,
which offers the page's own origin and warns while unset. PUT
/api/admin/network-settings takes either field alone, so the proxy card and
this one can't overwrite each other.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TestSetHops_RejectsOutOfRange caught a real ordering bug in code I wrote in
the same commit: the nil-pool guard sat ahead of the range check, so
SetHops(-1) on a service with no pool returned "network settings unavailable"
instead of ErrHopsOutOfRange.
Range first is correct, and the distinction is user-visible rather than
cosmetic: the argument is invalid regardless of whether the database is
reachable, and admin_network.go maps ErrHopsOutOfRange to 400 while anything
else becomes 500. The old order blamed the server for the caller's input.
Note this is the first failure in this sequence that wasn't a missed call
site — vet and golangci-lint both passed, and a test asserting a specific
sentinel error found it. Worth the extra assertion; `err != nil` would have
passed happily.
Third attempt at the same class of mistake, so worth naming precisely.
TestRoutesRegisteredInMount calls Mount() from INSIDE package api, so the
call reads `Mount(...)` unqualified. My verification grep was `api.Mount(`,
which cannot match it. Same shape as the previous failure, where I grepped
`auth.ClientIP(` and missed nothing — but only because those callers happened
to be in other packages.
The lesson generalises: after changing an exported signature, search for the
bare identifier, not the package-qualified form. In-package callers — which
in Go means most tests — are invisible to the qualified pattern.
This time I swept every signature I touched (Mount, RequireUser, ClientIP,
TouchSessionLastSeen) with an unqualified pattern before pushing, rather than
letting CI enumerate them one per run.
Passing h.netSettings (nil in test handlers) is deliberate, not a placeholder:
this test asserts route registration, and Hops() is nil-safe by design so the
middleware reads "trust nothing" rather than panicking.
Also gave netsettings' logger field a use — it was assigned and never read,
which staticcheck's unused pass can flag. A hop-count change alters how much
of a client-supplied header the server believes, so it earns a log line for
anyone later debugging odd addresses in the sessions list.
Fixes the defect the operator spotted in #370 immediately after it shipped:
auth.ClientIP ignored X-Forwarded-For whenever RemoteAddr was public, so a
proxy on a public address — a separate host, or a CDN, i.e. anyone running
this publicly, since public means TLS means a proxy — recorded the PROXY for
every session. created_ip and last_ip were then always equal and the
"Address changed" signal could never fire. The feature looked like it worked
and reported nothing.
Replaced with the standard trusted-hop model (Rails, Caddy, Traefik, nginx).
XFF grows left-to-right as each proxy appends the peer it received from, so
for client -> CDN -> own-proxy -> app the app sees [client, CDN] with
RemoteAddr = own-proxy, and the client sits at XFF[len - hops]:
0 RemoteAddr, XFF ignored — no proxy
1 the address your own proxy observed
2 through a CDN in front of your proxy
Default 1, per the operator: publicly reachable means a TLS terminator in
front.
The cost is real and stated rather than hidden. hops >= 1 DECLARES that a
proxy exists; set it with no proxy, or deeper than the actual chain, and the
index reaches attacker-supplied entries, letting a visitor choose which
address their own session shows — defeating exactly the detection #370 is
for. That's inherent to the model, which is why 0 is a first-class value and
the admin card says "count your proxies, don't guess high" instead of just
exposing a number. Both mis-set shapes are pinned by tests so they stay known
consequences rather than surprises.
Migration 0053 + internal/netsettings, cached under an RWMutex. That's not an
optimisation: ClientIP runs in RequireUser for every authenticated request, so
a per-request query would put the database on the critical path of the whole
API. New() always returns a usable service so a boot-time DB hiccup degrades
to the default instead of breaking that path (rule #131), and Hops() is
nil-safe because test routers construct middleware without it.
RequireUser now takes a func() int rather than an int — the value is
operator-editable at runtime while the middleware is built once at boot, and
reading it per request is what makes a save take effect with no restart
(rule #25).
The admin card is verifiable, not just configurable: it reports the address
the CURRENT setting resolves THIS request to, the raw forwarded chain, and the
socket peer — so you set the number, save, and confirm the address matches the
machine you're on. It also counts the arriving chain and says how many proxies
that implies. GET/PUT both return that payload, PUT recomputed under the new
value, so the effect is visible without a reload.
Also fixes styling in the #370 card that CI could not catch: text-destructive
and bg-destructive don't exist in this Tailwind config — the palette is
colors.action.destructive — so the "Address changed" warning and the
sign-out-others button were rendering unstyled. Both now use
text-action-destructive / bg-action-destructive / text-action-fg.
Not done here: requestlog.go still logs raw RemoteAddr and will disagree with
the sessions UI about who connected. Left for its own change.
bvandeusenandClaude Opus 5.5