phase1-3: apply review fixes and docs alignment
test / test (push) Has been cancelled

Implement phase 1 and 2 review actions (DMARC UI CSS fixes, bounded DMARC ingest stdin,
rate-limit eviction hardening, settings/user handler cleanup, shared atomic/supervisor helpers)
and phase 3 documentation/changelog updates including roadmap and accepted-risk notes.

Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
mix
2026-08-19 19:06:33 +03:00
co-authored by Cursor
parent 5a41d34cac
commit 81190a7aad
16 changed files with 241 additions and 223 deletions
+45
View File
@@ -0,0 +1,45 @@
package atomicfile
import (
"fmt"
"os"
"path/filepath"
)
// Write writes data to path via a temp file in the same directory followed by
// a rename, so readers only ever see the complete old or new file.
func Write(path string, data []byte, perm os.FileMode) error {
dir := filepath.Dir(path)
tmp, err := os.CreateTemp(dir, ".tmp-*")
if err != nil {
return fmt.Errorf("create temp in %s: %w", dir, err)
}
tmpName := tmp.Name()
cleanup := true
defer func() {
if cleanup {
_ = os.Remove(tmpName)
}
}()
if _, err := tmp.Write(data); err != nil {
tmp.Close()
return fmt.Errorf("write temp %s: %w", tmpName, err)
}
if err := tmp.Chmod(perm); err != nil {
tmp.Close()
return fmt.Errorf("chmod temp %s: %w", tmpName, err)
}
if err := tmp.Sync(); err != nil {
tmp.Close()
return fmt.Errorf("sync temp %s: %w", tmpName, err)
}
if err := tmp.Close(); err != nil {
return fmt.Errorf("close temp %s: %w", tmpName, err)
}
if err := os.Rename(tmpName, path); err != nil {
return fmt.Errorf("rename %s -> %s: %w", tmpName, path, err)
}
cleanup = false
return nil
}
+3 -34
View File
@@ -13,7 +13,8 @@ import (
"encoding/pem"
"fmt"
"os"
"path/filepath"
"github.com/mixeme/selfpost/internal/atomicfile"
)
// dkimKeyBits is the RSA key size for DKIM signing keys. 2048 is the DKIM
@@ -91,37 +92,5 @@ func dkimRecord(selector, domainName string, pub *rsa.PublicKey) (DKIMRecord, er
// It is the single safe-write primitive for DKIM keys and OpenDKIM tables
// (security.md).
func writeFileAtomic(path string, data []byte, perm os.FileMode) error {
dir := filepath.Dir(path)
tmp, err := os.CreateTemp(dir, ".tmp-*")
if err != nil {
return fmt.Errorf("create temp in %s: %w", dir, err)
}
tmpName := tmp.Name()
cleanup := true
defer func() {
if cleanup {
_ = os.Remove(tmpName)
}
}()
if _, err := tmp.Write(data); err != nil {
tmp.Close()
return fmt.Errorf("write temp %s: %w", tmpName, err)
}
if err := tmp.Chmod(perm); err != nil {
tmp.Close()
return fmt.Errorf("chmod temp %s: %w", tmpName, err)
}
if err := tmp.Sync(); err != nil {
tmp.Close()
return fmt.Errorf("sync temp %s: %w", tmpName, err)
}
if err := tmp.Close(); err != nil {
return fmt.Errorf("close temp %s: %w", tmpName, err)
}
if err := os.Rename(tmpName, path); err != nil {
return fmt.Errorf("rename %s -> %s: %w", tmpName, path, err)
}
cleanup = false
return nil
return atomicfile.Write(path, data, perm)
}
+5 -10
View File
@@ -5,10 +5,12 @@ import (
"encoding/pem"
"fmt"
"os"
"os/exec"
"path"
"path/filepath"
"sort"
"strings"
"github.com/mixeme/selfpost/internal/supervisor"
)
// OpenDKIM manages the on-disk OpenDKIM state the panel is responsible for
@@ -191,7 +193,7 @@ func renderTables(keysDir string, domains []SigningDomain) (keyTable, signingTab
}
keyName := d.Name // one key per domain; the domain name is a fine handle
// Absolute key path so OpenDKIM resolves it independently of its CWD.
keyFile := filepath.Join(keysDir, d.Name, d.Selector+".private")
keyFile := path.Join(keysDir, d.Name, d.Selector+".private")
// KeyTable: <key-name> <domain>:<selector>:<key-path>
fmt.Fprintf(&kt, "%s %s:%s:%s\n", keyName, d.Name, d.Selector, keyFile)
// SigningTable (refile): <address-pattern> <key-name>
@@ -226,12 +228,5 @@ func assertConfigSafe(domainName, selector string) error {
// Arguments are fixed literals — no user input is interpolated into the command,
// and it never goes through a shell (security.md).
func reloadViaSupervisor() error {
cmd := exec.Command("supervisorctl",
"-c", "/etc/supervisor/supervisord.conf",
"signal", "USR1", "opendkim")
out, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("reload opendkim via supervisor: %w: %s", err, strings.TrimSpace(string(out)))
}
return nil
return supervisor.Signal("USR1", "opendkim")
}
+3 -14
View File
@@ -7,10 +7,11 @@ package postfix
import (
"fmt"
"os/exec"
"path/filepath"
"sort"
"strings"
"github.com/mixeme/selfpost/internal/supervisor"
)
// Postfix manages the on-disk Postfix state the panel is responsible for. After
@@ -154,17 +155,5 @@ func assertMapSafe(address, login string) error {
// Arguments are fixed literals — no user input is interpolated into the command,
// and it never goes through a shell (security.md).
func reloadViaSupervisor() error {
cmd := exec.Command("supervisorctl",
"-c", "/etc/supervisor/supervisord.conf",
"start", "postfix-reload")
out, err := cmd.CombinedOutput()
if err != nil {
// A reload already in flight is not a failure: that pending run reloads
// Postfix after our file is in place (the file is written before this).
if strings.Contains(string(out), "already started") {
return nil
}
return fmt.Errorf("reload postfix via supervisor: %w: %s", err, strings.TrimSpace(string(out)))
}
return nil
return supervisor.Start("postfix-reload")
}
+4 -37
View File
@@ -1,47 +1,14 @@
package postfix
import (
"fmt"
"os"
"path/filepath"
"github.com/mixeme/selfpost/internal/atomicfile"
)
// writeFileAtomic writes data to path via a temp file in the same directory
// followed by a rename, so a concurrent Postfix reload only ever sees the
// complete old or new map, never a partial write (security.md). It mirrors the
// same primitive used for the OpenDKIM tables.
// complete old or new map, never a partial write (security.md).
func writeFileAtomic(path string, data []byte, perm os.FileMode) error {
dir := filepath.Dir(path)
tmp, err := os.CreateTemp(dir, ".tmp-*")
if err != nil {
return fmt.Errorf("create temp in %s: %w", dir, err)
}
tmpName := tmp.Name()
cleanup := true
defer func() {
if cleanup {
_ = os.Remove(tmpName)
}
}()
if _, err := tmp.Write(data); err != nil {
tmp.Close()
return fmt.Errorf("write temp %s: %w", tmpName, err)
}
if err := tmp.Chmod(perm); err != nil {
tmp.Close()
return fmt.Errorf("chmod temp %s: %w", tmpName, err)
}
if err := tmp.Sync(); err != nil {
tmp.Close()
return fmt.Errorf("sync temp %s: %w", tmpName, err)
}
if err := tmp.Close(); err != nil {
return fmt.Errorf("close temp %s: %w", tmpName, err)
}
if err := os.Rename(tmpName, path); err != nil {
return fmt.Errorf("rename %s -> %s: %w", tmpName, path, err)
}
cleanup = false
return nil
return atomicfile.Write(path, data, perm)
}
+36
View File
@@ -0,0 +1,36 @@
// Package supervisor provides helpers for controlling processes through
// supervisord's control socket (architecture.md, security.md).
package supervisor
import (
"fmt"
"os/exec"
"strings"
)
const confPath = "/etc/supervisor/supervisord.conf"
// Start issues a `supervisorctl start` for the named program. A program that
// is already running is not an error (the pending run covers the caller's
// intent).
func Start(program string) error {
cmd := exec.Command("supervisorctl", "-c", confPath, "start", program)
out, err := cmd.CombinedOutput()
if err != nil {
if strings.Contains(string(out), "already started") {
return nil
}
return fmt.Errorf("supervisor start %s: %w: %s", program, err, strings.TrimSpace(string(out)))
}
return nil
}
// Signal sends a signal to a supervised program via `supervisorctl signal`.
func Signal(sig, program string) error {
cmd := exec.Command("supervisorctl", "-c", confPath, "signal", sig, program)
out, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("supervisor signal %s %s: %w: %s", sig, program, err, strings.TrimSpace(string(out)))
}
return nil
}
+19 -9
View File
@@ -69,24 +69,34 @@ func (r *rateLimiter) makeRoom(now time.Time) {
}
r.sweep(now)
for len(r.buckets) >= r.maxBuckets {
r.evictOldest()
if !r.evictNearest() {
break
}
}
}
func (r *rateLimiter) evictOldest() {
var oldestKey string
var oldestEnds time.Time
// evictNearest removes the bucket whose window ends soonest, skipping buckets
// that are currently blocked (count >= max) so an attacker cannot flush active
// rate-limit state by filling the map with new keys.
func (r *rateLimiter) evictNearest() bool {
var bestKey string
var bestEnds time.Time
first := true
for k, b := range r.buckets {
if first || b.windowEnds.Before(oldestEnds) {
oldestKey = k
oldestEnds = b.windowEnds
if b.count >= r.max {
continue
}
if first || b.windowEnds.Before(bestEnds) {
bestKey = k
bestEnds = b.windowEnds
first = false
}
}
if oldestKey != "" {
delete(r.buckets, oldestKey)
if bestKey == "" {
return false
}
delete(r.buckets, bestKey)
return true
}
func (r *rateLimiter) sweep(now time.Time) {
+20 -67
View File
@@ -61,7 +61,7 @@ func parseRateLimitMode(raw string) (string, error) {
return raw, nil
}
func parseDomainRateLimitForm(r *http.Request, l1Max int) (rateLimitInput, error) {
func parseRateLimitForm(r *http.Request, l1Max int) (rateLimitInput, error) {
if err := r.ParseForm(); err != nil {
return rateLimitInput{}, fmt.Errorf("invalid form submission")
}
@@ -98,42 +98,12 @@ func parseDomainRateLimitForm(r *http.Request, l1Max int) (rateLimitInput, error
return rateLimitInput{mode: store.RateLimitModeManual, maxMessages: maxMessages, windowSeconds: windowSeconds}, nil
}
func parseDomainRateLimitForm(r *http.Request, l1Max int) (rateLimitInput, error) {
return parseRateLimitForm(r, l1Max)
}
func parseAppRateLimitForm(r *http.Request, l1Max int) (rateLimitInput, error) {
if err := r.ParseForm(); err != nil {
return rateLimitInput{}, fmt.Errorf("invalid form submission")
}
if r.PostFormValue("clear") != "" {
return rateLimitInput{clear: true}, nil
}
mode, err := parseRateLimitMode(r.PostFormValue("mode"))
if err != nil {
return rateLimitInput{}, err
}
if mode == store.RateLimitModeAuto {
mult, err := parseAutoMultiplier(r.PostFormValue("auto_multiplier"))
if err != nil {
return rateLimitInput{}, err
}
return rateLimitInput{mode: mode, autoMultiplier: mult}, nil
}
rawMax := strings.TrimSpace(r.PostFormValue("max_messages"))
if rawMax == "" {
return rateLimitInput{clear: true}, nil
}
maxMessages, err := parsePositiveInt(rawMax, 0)
if err != nil || maxMessages <= 0 {
return rateLimitInput{}, fmt.Errorf("enter a message limit greater than zero")
}
if maxMessages > l1Max {
return rateLimitInput{}, fmt.Errorf("message limit cannot exceed the level-1 backstop (%d)", l1Max)
}
windowSeconds, err := parsePositiveInt(r.PostFormValue("window_seconds"), defaultRateLimitWindowSeconds)
if err != nil || windowSeconds <= 0 {
return rateLimitInput{}, fmt.Errorf("enter a time window greater than zero seconds")
}
return rateLimitInput{mode: store.RateLimitModeManual, maxMessages: maxMessages, windowSeconds: windowSeconds}, nil
return parseRateLimitForm(r, l1Max)
}
func parseAppAuthIPsForm(r *http.Request) (bool, []string, error) {
@@ -187,7 +157,7 @@ func (h *Handlers) HandleDomainRateLimit(w http.ResponseWriter, r *http.Request)
if !ok {
return
}
in, err := parseDomainRateLimitForm(r, h.l1Messages())
in, err := parseRateLimitForm(r, h.l1Messages())
if err != nil {
h.renderDomainDetail(w, r, http.StatusBadRequest, d, detailView{
FormMode: store.AddressModeWildcard,
@@ -195,7 +165,7 @@ func (h *Handlers) HandleDomainRateLimit(w http.ResponseWriter, r *http.Request)
})
return
}
if err := h.applyDomainRateLimit(in, d.ID); err != nil {
if err := h.applyRateLimit(in, store.RateLimitScopeDomain, d.ID, h.domains); err != nil {
logf("panel: domain %d: save rate limit: %v", d.ID, err)
http.Error(w, "internal error", http.StatusInternalServerError)
return
@@ -213,7 +183,7 @@ func (h *Handlers) HandleAppRateLimit(w http.ResponseWriter, r *http.Request) {
http.Error(w, "internal error", http.StatusInternalServerError)
return
}
in, err := parseAppRateLimitForm(r, h.l1Messages())
in, err := parseRateLimitForm(r, h.l1Messages())
if err != nil {
h.renderDomainDetail(w, r, http.StatusBadRequest, d, detailView{
FormMode: store.AddressModeWildcard,
@@ -221,7 +191,7 @@ func (h *Handlers) HandleAppRateLimit(w http.ResponseWriter, r *http.Request) {
})
return
}
if err := h.applyAppRateLimit(in, a.ID); err != nil {
if err := h.applyRateLimit(in, store.RateLimitScopeApp, a.ID, h.apps); err != nil {
logf("panel: application %d: save rate limit: %v", a.ID, err)
http.Error(w, "internal error", http.StatusInternalServerError)
return
@@ -291,35 +261,18 @@ func (h *Handlers) recalcRateLimit(scope string, refID int64) error {
return h.store.RecalcAutoRateLimit(scope, refID, h.sendLogRetentionDays(), h.l1Messages(), h.l1Window())
}
func (h *Handlers) applyDomainRateLimit(in rateLimitInput, domainID int64) error {
if in.clear {
return h.domains.ClearRateLimit(domainID)
}
rl := store.RateLimit{
Scope: store.RateLimitScopeDomain,
RefID: domainID,
Mode: in.mode,
MaxMessages: in.maxMessages,
WindowSeconds: in.windowSeconds,
AutoMultiplier: in.autoMultiplier,
}
if in.mode == store.RateLimitModeAuto {
rl.WindowSeconds = h.l1Window()
if err := h.domains.SaveRateLimit(domainID, rl); err != nil {
return err
}
return h.recalcRateLimit(store.RateLimitScopeDomain, domainID)
}
return h.domains.SaveRateLimit(domainID, rl)
type rateLimitSaver interface {
ClearRateLimit(id int64) error
SaveRateLimit(id int64, rl store.RateLimit) error
}
func (h *Handlers) applyAppRateLimit(in rateLimitInput, appID int64) error {
func (h *Handlers) applyRateLimit(in rateLimitInput, scope string, refID int64, saver rateLimitSaver) error {
if in.clear {
return h.apps.ClearRateLimit(appID)
return saver.ClearRateLimit(refID)
}
rl := store.RateLimit{
Scope: store.RateLimitScopeApp,
RefID: appID,
Scope: scope,
RefID: refID,
Mode: in.mode,
MaxMessages: in.maxMessages,
WindowSeconds: in.windowSeconds,
@@ -327,10 +280,10 @@ func (h *Handlers) applyAppRateLimit(in rateLimitInput, appID int64) error {
}
if in.mode == store.RateLimitModeAuto {
rl.WindowSeconds = h.l1Window()
if err := h.apps.SaveRateLimit(appID, rl); err != nil {
if err := saver.SaveRateLimit(refID, rl); err != nil {
return err
}
return h.recalcRateLimit(store.RateLimitScopeApp, appID)
return h.recalcRateLimit(scope, refID)
}
return h.apps.SaveRateLimit(appID, rl)
return saver.SaveRateLimit(refID, rl)
}
+29 -38
View File
@@ -71,26 +71,29 @@ func (h *Handlers) renderSettings(w http.ResponseWriter, r *http.Request, status
}
func settingsFlash(r *http.Request) string {
switch r.URL.Query().Get("updated") {
case "username":
return "Username changed."
case "password":
return "Password changed. Any other signed-in sessions were signed out."
case "both":
return "Username and password changed. Any other signed-in sessions were signed out."
case "email":
return "DMARC report address updated."
case "retention":
return "Send log retention updated."
case "username-email":
return "Username and DMARC report address updated."
case "password-email":
return "Password and DMARC report address updated. Any other signed-in sessions were signed out."
case "all":
return "Settings updated. Any other signed-in sessions were signed out."
default:
q := r.URL.Query()
var parts []string
if q.Has("u") {
parts = append(parts, "username")
}
if q.Has("p") {
parts = append(parts, "password")
}
if q.Has("e") {
parts = append(parts, "DMARC report address")
}
if q.Has("r") {
parts = append(parts, "send log retention")
}
if len(parts) == 0 {
return ""
}
msg := strings.Join(parts, ", ") + " updated."
msg = strings.ToUpper(msg[:1]) + msg[1:]
if q.Has("p") {
msg += " Any other signed-in sessions were signed out."
}
return msg
}
func (h *Handlers) submitSettings(w http.ResponseWriter, r *http.Request) {
@@ -240,36 +243,24 @@ func (h *Handlers) submitSettings(w http.ResponseWriter, r *http.Request) {
}
logf("panel: user %d settings updated (username: %t, password: %t, dmarc email: %t, retention: %t)", user.ID, renaming, repassword, emailChanging, retentionChanging)
http.Redirect(w, r, "/settings?updated="+updatedFlag(renaming, repassword, emailChanging, retentionChanging), http.StatusSeeOther)
http.Redirect(w, r, "/settings?"+updatedQuery(renaming, repassword, emailChanging, retentionChanging), http.StatusSeeOther)
}
func updatedFlag(renamed, repassword, emailChanged, retentionChanged bool) string {
changed := 0
func updatedQuery(renamed, repassword, emailChanged, retentionChanged bool) string {
var params []string
if renamed {
changed++
params = append(params, "u=1")
}
if repassword {
changed++
params = append(params, "p=1")
}
if emailChanged {
changed++
params = append(params, "e=1")
}
if retentionChanged {
changed++
}
if changed > 1 {
return "all"
}
switch {
case renamed:
return "username"
case repassword:
return "password"
case emailChanged:
return "email"
default:
return "retention"
params = append(params, "r=1")
}
return strings.Join(params, "&")
}
// sendLogRetentionDays returns the effective delivery-journal retention window.
+5 -9
View File
@@ -225,15 +225,11 @@ func (h *Handlers) submitUserUpdate(w http.ResponseWriter, r *http.Request, u st
if u.Role == store.RoleGlobal && role == store.RoleDomainAdmin {
n, err := h.store.CountGlobalUsers()
if err != nil || n <= 1 {
h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "Cannot demote the last global administrator.", FormUsername: username, FormRole: string(u.Role), FormDomains: selected})
return
}
}
if u.ID == p.ID && u.Role == store.RoleGlobal && role == store.RoleDomainAdmin {
n, err := h.store.CountGlobalUsers()
if err != nil || n <= 1 {
h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: "You cannot demote yourself without another global administrator.", FormUsername: username, FormRole: string(u.Role), FormDomains: selected})
msg := "Cannot demote the last global administrator."
if u.ID == p.ID {
msg = "You cannot demote yourself without another global administrator."
}
h.renderUserForm(w, r, http.StatusBadRequest, u.ID, userFormView{FormErr: msg, FormUsername: username, FormRole: string(u.Role), FormDomains: selected})
return
}
}
+36
View File
@@ -824,3 +824,39 @@ html:has(#help-export:checked) .help-pane-export { display: block; }
@media (max-width: 66rem) {
.help-drawer { width: 100vw; }
}
/* DMARC pages: paired cards side by side, same layout as .split but for two
summary cards at the top of a page. */
.pair {
display: grid; grid-template-columns: repeat(auto-fit, minmax(18rem, 1fr));
gap: 1.2rem; margin-top: 1.2rem;
}
.pair > .card { margin-left: 0; margin-right: 0; min-width: 0; }
.pair > .card + .card { margin-top: 0; }
/* Attention card: elevated border when a DMARC report has failures. */
.card.attn { border-color: var(--st-warn-fg); }
/* Responsive visibility: hide elements on narrow/wide screens. */
.desk-only { display: revert; }
.phone-only, .phone-list { display: none; }
@media (max-width: 48rem) {
.desk-only { display: none !important; }
.phone-only, .phone-list { display: revert !important; }
}
.phone-list { list-style: none; margin: 0; padding: 0; }
.phone-list li { border-bottom: 1px solid var(--border); }
.phone-list li:last-child { border-bottom: none; }
.phone-list a {
display: grid; grid-template-columns: auto 1fr; gap: 0.15rem 0.6rem;
padding: 0.7rem 0; text-decoration: none; color: inherit;
}
.phone-list .when { grid-column: 1; font-family: var(--font-mono); font-size: 0.75rem; color: var(--muted); }
.phone-list .meta { grid-column: 2; font-size: 0.85rem; }
.phone-list .subj { grid-column: 1 / -1; font-size: 0.85rem; color: var(--muted); }
/* Checkbox-label row inside a form (IP restriction toggle). */
.check-row {
display: flex; align-items: center; gap: 0.5rem;
margin: 0.5rem 0; cursor: pointer;
}