docs/chore: Phase 1 doc/code hygiene (code-review.md § Phase 1)
Removes ~30 stale "Phase N" / historical-staging comment references from code and shell scripts now that v1.0 is done; fixes a stale dashboard comment claiming applications/send-log were unimplemented; adds a CSRF ADR to security.md documenting the Origin-check-over-tokens decision; resolves docs/logo in roadmap.md (directory doesn't exist, criterion already met); adds a gofmt -l check to CI so unformatted Go fails the build. The known-limitations write-up for the log-tailer offset gap (the other Phase 1 item) was already present in architecture.md § Log tailer, so no change was needed there. gofmt/go vet/go test clean on both Go modules (main + test/e2e), verified on the dev server. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -28,7 +28,7 @@ type SASLDB struct {
|
||||
|
||||
// NewSASLDB builds a manager for the sasldb2 at path with the given realm. The
|
||||
// realm should match SELFPOST_HOSTNAME so the account identity lines up with
|
||||
// Postfix's SASL configuration in Phase 5.
|
||||
// Postfix's SASL configuration.
|
||||
func NewSASLDB(path, realm string) *SASLDB {
|
||||
return &SASLDB{path: path, realm: realm, run: runSaslpasswd2, dump: dumpSASLDB}
|
||||
}
|
||||
|
||||
@@ -37,7 +37,7 @@ type Store interface {
|
||||
|
||||
// session accumulates the fields of one message as the milter callbacks fire.
|
||||
// Milter macros arrive per-stage and do not accumulate, so each value is
|
||||
// captured at the stage that carries it (spec 7.3 / Phase 0 spike): SASL login
|
||||
// captured at the stage that carries it (spec 7.3): SASL login
|
||||
// and From at MAIL, each recipient at RCPT, Subject in the headers, and the
|
||||
// queue-id at end-of-message. go-milter creates one session per connection; a
|
||||
// connection may carry several messages, so per-message fields are reset at
|
||||
@@ -55,8 +55,8 @@ type session struct {
|
||||
}
|
||||
|
||||
// Connect captures the client IP, which comes from the addr parameter rather
|
||||
// than a macro (the {client_addr} macro was empty in the spike). It is the
|
||||
// rate-limit key for Phase 8; here it is recorded for completeness.
|
||||
// than a macro (the {client_addr} macro was empty in testing). It is the
|
||||
// rate-limit key; here it is recorded for completeness.
|
||||
func (s *session) Connect(host, family string, port uint16, addr net.IP, m *milter.Modifier) (milter.Response, error) {
|
||||
if addr != nil {
|
||||
s.clientIP = addr.String()
|
||||
@@ -133,8 +133,7 @@ func (s *session) Body(m *milter.Modifier) (milter.Response, error) {
|
||||
// macro reads a milter macro, tolerating Postfix's convention of wrapping
|
||||
// multi-character macro names in curly braces (e.g. {auth_authen}) while
|
||||
// single-character names (e.g. i) arrive bare. go-milter stores whatever name
|
||||
// Postfix sends verbatim, so a lookup must try both forms — this is exactly the
|
||||
// distinction the SASL-less Phase 0 spike could not observe.
|
||||
// Postfix sends verbatim, so a lookup must try both forms.
|
||||
func macro(m *milter.Modifier, name string) string {
|
||||
if v, ok := m.Macros[name]; ok {
|
||||
return v
|
||||
@@ -180,7 +179,7 @@ func cleanAddress(a string) string {
|
||||
}
|
||||
|
||||
// domainOf returns the lower-cased domain of an email address, or "" if there
|
||||
// is no domain part. Sender binding (Phase 4) guarantees the From domain equals
|
||||
// is no domain part. Sender binding guarantees the From domain equals
|
||||
// the application's domain, so this is the sending domain (spec 7.3).
|
||||
func domainOf(addr string) string {
|
||||
if i := strings.LastIndexByte(addr, '@'); i >= 0 {
|
||||
|
||||
@@ -1,8 +1,7 @@
|
||||
// Package postfix owns the Postfix configuration files the panel edits at
|
||||
// runtime and the privileged reload that applies them (spec 5.1, 7.6.3-4). In
|
||||
// Phase 4 that is the smtpd_sender_login_maps table binding each application's
|
||||
// SASL login to the sender addresses it may use; the full relay configuration
|
||||
// lands in Phase 5.
|
||||
// runtime and the privileged reload that applies them (spec 5.1, 7.6.3-4): the
|
||||
// smtpd_sender_login_maps table binding each application's SASL login to the
|
||||
// sender addresses it may use, plus the relay configuration in main.cf.
|
||||
package postfix
|
||||
|
||||
import (
|
||||
@@ -33,8 +32,8 @@ func New(dir string) *Postfix {
|
||||
}
|
||||
}
|
||||
|
||||
// SenderLoginMapsPath is the absolute path of the generated map, so the Postfix
|
||||
// main.cf written in Phase 5 can point smtpd_sender_login_maps at it.
|
||||
// SenderLoginMapsPath is the absolute path of the generated map, so main.cf
|
||||
// can point smtpd_sender_login_maps at it.
|
||||
func (p *Postfix) SenderLoginMapsPath() string {
|
||||
return p.senderLoginMapsPath
|
||||
}
|
||||
|
||||
@@ -62,8 +62,8 @@ func TestDeleteDomainCascadesApplications(t *testing.T) {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// Insert an application + address directly (the AddApplication API lands in
|
||||
// Phase 4); this verifies the ON DELETE CASCADE wiring now.
|
||||
// Insert an application + address directly (bypassing the AddApplication
|
||||
// API) to verify the ON DELETE CASCADE wiring.
|
||||
now := time.Now().UTC().Format(time.RFC3339)
|
||||
res, err := st.db.Exec(
|
||||
"INSERT INTO applications (domain_id, login, address_mode, created_at) VALUES (?, ?, 'wildcard', ?)",
|
||||
|
||||
@@ -5,8 +5,8 @@ import (
|
||||
"time"
|
||||
)
|
||||
|
||||
// readSendLog returns every send_log row ordered by id. Phase 6 has no read
|
||||
// query yet (the monitoring UI is Phase 7), so tests read the table directly.
|
||||
// readSendLog returns every send_log row ordered by id. The monitoring UI has
|
||||
// no equivalent read query, so tests read the table directly.
|
||||
type sendLogRow struct {
|
||||
QueueID string
|
||||
Domain string
|
||||
|
||||
@@ -97,8 +97,8 @@ func (s *Server) renderDomainDetail(w http.ResponseWriter, r *http.Request, stat
|
||||
}
|
||||
|
||||
// What DNS actually publishes for the domain today, checked against the key
|
||||
// this server signs with (phase 13.B). Cached by the checker, so
|
||||
// re-rendering the page after a form post costs nothing.
|
||||
// this server signs with. Cached by the checker, so re-rendering the page
|
||||
// after a form post costs nothing.
|
||||
dns, srv := s.domainDNS(d, record, false)
|
||||
|
||||
s.render(w, status, "domain_detail", map[string]any{
|
||||
@@ -139,7 +139,7 @@ func (s *Server) renderDomainDetail(w http.ResponseWriter, r *http.Request, stat
|
||||
}
|
||||
|
||||
// domainDNS resolves what the world sees for a domain: its DKIM, SPF and DMARC
|
||||
// records (phase 13.B). The server's own address comes from the (separately
|
||||
// records. The server's own address comes from the (separately
|
||||
// cached) hostname check, so the SPF heuristic knows which IP it is looking for
|
||||
// and no extra environment variable is needed. That server result is returned
|
||||
// alongside, because the page's suggested SPF record is built from the same
|
||||
|
||||
@@ -13,7 +13,7 @@ import (
|
||||
// TLS in front, so CookieSecure — it carries the __Host- prefix, which turns
|
||||
// what the cookie's attributes merely promise into something the browser
|
||||
// enforces: Secure, Path=/ and, the point of the exercise, no Domain
|
||||
// attribute, so no other host may set a cookie by this name (phase 14.B).
|
||||
// attribute, so no other host may set a cookie by this name.
|
||||
// The prefix is only valid on a Secure cookie, so a development instance on
|
||||
// plain HTTP has to keep the bare name: with the prefix the browser would
|
||||
// discard the Set-Cookie outright and logging in would silently never stick.
|
||||
@@ -42,7 +42,7 @@ func (s *Server) sessionCookie() string {
|
||||
// is denial of service, not compromise; refusing the request and saying so in
|
||||
// the log is what makes it diagnosable instead of an endless login loop. The
|
||||
// __Host- prefix prevents this outright, but only where it applies — this
|
||||
// check also covers the plain-HTTP development shape (phase 14.B).
|
||||
// check also covers the plain-HTTP development shape.
|
||||
func (s *Server) sessionToken(r *http.Request) (string, bool) {
|
||||
name := s.sessionCookie()
|
||||
var token string
|
||||
|
||||
@@ -11,7 +11,7 @@ import (
|
||||
|
||||
// handleDashboard is the authenticated landing page: the list of sending
|
||||
// domains with their DKIM/selector and application counts, plus the add-domain
|
||||
// form (spec 7.2.2). Applications and the send log arrive in later phases.
|
||||
// form (spec 7.2.2).
|
||||
func (s *Server) handleDashboard(w http.ResponseWriter, r *http.Request) {
|
||||
s.renderDashboard(w, r, http.StatusOK, "", "")
|
||||
}
|
||||
@@ -118,7 +118,7 @@ func (s *Server) handleDeleteDomain(w http.ResponseWriter, r *http.Request) {
|
||||
// handleReload re-applies both the OpenDKIM configuration and the Postfix
|
||||
// sender map on demand (spec 7.2.12). Each Resync regenerates its files from the
|
||||
// database and reloads its daemon, so the button doubles as a drift-recovery.
|
||||
// The button lives on the status page (phase 13.D): it is a "put the daemons
|
||||
// The button lives on the status page: it is a "put the daemons
|
||||
// back in the state the database describes" action, which belongs with the rest
|
||||
// of the server-health screen rather than in the domain list's top bar.
|
||||
func (s *Server) handleReload(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
@@ -8,8 +8,8 @@ import (
|
||||
)
|
||||
|
||||
// handleStatus renders the server status page: the panel's landing page and the
|
||||
// one screen that answers "is the service healthy and will mail be accepted"
|
||||
// (phase 13.A). The cheap local checks live in the polled "status_body"
|
||||
// one screen that answers "is the service healthy and will mail be accepted".
|
||||
// The cheap local checks live in the polled "status_body"
|
||||
// fragment; the hostname/PTR lookup and the configuration reload sit outside it,
|
||||
// because neither belongs on a five-second timer.
|
||||
func (s *Server) handleStatus(w http.ResponseWriter, r *http.Request) {
|
||||
|
||||
@@ -8,7 +8,7 @@ import (
|
||||
// contentSecurityPolicy is the panel's CSP. Everything the pages load —
|
||||
// stylesheet, HTMX, the panel's own script, the favicon — is served from
|
||||
// /static by this same origin, and no template carries an inline <script>,
|
||||
// an inline event handler or a style="..." attribute (phase 14.A; the
|
||||
// an inline event handler or a style="..." attribute (the
|
||||
// template guard test enforces that), so 'self' needs no exemptions:
|
||||
//
|
||||
// - default-src 'self' covers scripts, styles, images and the XHR that
|
||||
@@ -38,8 +38,8 @@ const contentSecurityPolicy = "default-src 'self'; " +
|
||||
// subdomain of that domain for a year, which is not SelfPost's call to make.
|
||||
const strictTransportSecurity = "max-age=31536000"
|
||||
|
||||
// secure wraps the whole router with the panel's two transport-level defences
|
||||
// (phase 14.A): the security response headers, and an origin check on every
|
||||
// secure wraps the whole router with the panel's two transport-level
|
||||
// defences: the security response headers, and an origin check on every
|
||||
// state-changing request.
|
||||
//
|
||||
// It sits outside the authentication middleware on purpose, so that POST
|
||||
@@ -81,7 +81,7 @@ func (s *Server) secure(next http.Handler) http.Handler {
|
||||
}
|
||||
|
||||
// originAllowed reports whether a state-changing request came from the panel's
|
||||
// own origin (phase 14.A). This is what the session cookie's
|
||||
// own origin. This is what the session cookie's
|
||||
// SameSite=Lax attribute cannot do on its own: SameSite is judged per *site*
|
||||
// (registrable domain), so a neighbouring subdomain — a CMS on the same
|
||||
// domain, a forgotten staging host — counts as same-site and its forged POST
|
||||
@@ -111,9 +111,9 @@ func originAllowed(r *http.Request) bool {
|
||||
origin := r.Header.Get("Origin")
|
||||
if origin == "" {
|
||||
// Neither header. A client this old cannot be checked at all; it is
|
||||
// let through as the risk consciously accepted for phase 14.A
|
||||
// (single-admin panel, the administrator picks the browser). Turning
|
||||
// this return into false is the whole of the stricter policy.
|
||||
// let through as the risk consciously accepted (single-admin panel,
|
||||
// the administrator picks the browser). Turning this return into
|
||||
// false is the whole of the stricter policy.
|
||||
return true
|
||||
}
|
||||
u, err := url.Parse(origin)
|
||||
|
||||
@@ -32,7 +32,7 @@ func post(secFetchSite, origin string) *http.Request {
|
||||
return r
|
||||
}
|
||||
|
||||
// The full matrix the origin check has to get right (phase 14.A). The row that
|
||||
// The full matrix the origin check has to get right. The row that
|
||||
// matters most is "same-site": a neighbouring host on example.com is same-site
|
||||
// as far as the session cookie's SameSite=Lax is concerned, so this check is
|
||||
// the only thing standing between it and a forged POST.
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
/* Panel stylesheet. It lives in a file rather than in a <style> block in the
|
||||
layout so the panel's Content-Security-Policy can be a plain
|
||||
"default-src 'self'" with no inline-style exemption (phase 14.A). Any rule
|
||||
"default-src 'self'" with no inline-style exemption. Any rule
|
||||
added here must therefore stay here: an inline style="..." attribute in a
|
||||
template is blocked by that policy and silently does nothing. */
|
||||
|
||||
|
||||
@@ -36,7 +36,7 @@
|
||||
// Forms that delete something or invalidate a working credential carry a
|
||||
// data-confirm message. The prompt lives here rather than in an inline
|
||||
// onsubmit attribute because the panel's Content-Security-Policy allows no
|
||||
// inline script (phase 14.A). The listener is delegated from the document,
|
||||
// inline script. The listener is delegated from the document,
|
||||
// so it also covers markup swapped in by HTMX. With JavaScript disabled the
|
||||
// form submits without asking — exactly as the inline handler behaved.
|
||||
document.addEventListener("submit", function (ev) {
|
||||
|
||||
@@ -15,7 +15,7 @@
|
||||
{{/* Unless told otherwise, htmx injects a stylesheet element of its own into
|
||||
the head for the request-indicator classes. The panel uses no
|
||||
hx-indicator, and that injected element would be the one thing the
|
||||
Content-Security-Policy (phase 14.A) has to make an exception for, so it
|
||||
Content-Security-Policy has to make an exception for, so it
|
||||
is switched off here. */}}
|
||||
<meta name="htmx-config" content='{"includeIndicatorStyles":false}'>
|
||||
<script src="/static/htmx.min.js" defer></script>
|
||||
|
||||
@@ -142,9 +142,9 @@ func TestNavLeadsWithStatusAndPointsDomainsAtItsOwnPath(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// Since the panel root now redirects to the status page, a link left pointing at
|
||||
// Since the panel root redirects to the status page, a link left pointing at
|
||||
// "/" silently lands on the wrong screen instead of failing — so no template may
|
||||
// contain one (phase 13.C).
|
||||
// contain one.
|
||||
func TestNoTemplateLinksToTheBareRoot(t *testing.T) {
|
||||
forEachTemplate(t, func(name, body string) {
|
||||
if strings.Contains(body, `href="/"`) {
|
||||
@@ -154,7 +154,7 @@ func TestNoTemplateLinksToTheBareRoot(t *testing.T) {
|
||||
}
|
||||
|
||||
// The reload action is a server-health control and lives only on the status
|
||||
// page (phase 13.D).
|
||||
// page.
|
||||
func TestReloadFormLivesOnlyOnTheStatusPage(t *testing.T) {
|
||||
forEachTemplate(t, func(name, body string) {
|
||||
if strings.Contains(body, `action="/reload"`) && name != "status.html" {
|
||||
@@ -164,7 +164,7 @@ func TestReloadFormLivesOnlyOnTheStatusPage(t *testing.T) {
|
||||
}
|
||||
|
||||
// The panel's Content-Security-Policy is a plain default-src 'self' with no
|
||||
// inline exemption (phase 14.A), which makes inline script and inline style a
|
||||
// inline exemption, which makes inline script and inline style a
|
||||
// failure mode rather than a style question: an onclick= handler or a
|
||||
// style="..." attribute added to a template does not error, it silently stops
|
||||
// working in the browser. Behaviour belongs in static/panel.js (triggered from
|
||||
|
||||
+4
-4
@@ -110,7 +110,7 @@ func New(st *store.Store, domains *domain.Service, apps *app.Service, cfg Config
|
||||
sessions: newSessionStore(st, time.Duration(idleDays)*24*time.Hour),
|
||||
// Published-DNS checks for the status page and the domain pages. The
|
||||
// checker caches its own results, so page views do not each pay for a
|
||||
// round of lookups (phase 13).
|
||||
// round of lookups.
|
||||
dns: dnscheck.New(cfg.DNSResolvers),
|
||||
// Setup: a handful of attempts per minute per IP is plenty for a
|
||||
// legitimate admin and blunts automated probing (spec 7.6.1).
|
||||
@@ -152,7 +152,7 @@ func (s *Server) Handler() http.Handler {
|
||||
// above falls through to this sub-mux, wrapped once in the auth middleware.
|
||||
authed := http.NewServeMux()
|
||||
|
||||
// The landing page is the server status (phase 13.C): the first thing an
|
||||
// The landing page is the server status: the first thing an
|
||||
// administrator should see after logging in is whether the service is
|
||||
// healthy, not the domain list. handleLogin still redirects to "/".
|
||||
authed.HandleFunc("GET /{$}", redirectToStatus)
|
||||
@@ -196,12 +196,12 @@ func (s *Server) Handler() http.Handler {
|
||||
mux.Handle("/", s.requireAuth(authed))
|
||||
|
||||
// Security headers and the origin check wrap everything, including the
|
||||
// unauthenticated login and setup routes (phase 14.A).
|
||||
// unauthenticated login and setup routes.
|
||||
return s.secure(mux)
|
||||
}
|
||||
|
||||
// redirectToStatus points the panel root at the status page, so there is one
|
||||
// canonical URL for that content instead of two (phase 13.C).
|
||||
// canonical URL for that content instead of two.
|
||||
func redirectToStatus(w http.ResponseWriter, r *http.Request) {
|
||||
http.Redirect(w, r, "/status", http.StatusSeeOther)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user