diff --git a/CHANGELOG.md b/CHANGELOG.md index c00a0f0..6896670 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,14 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); version and so is an address over 256 bytes, which OpenDKIM would match against its signing table cut short - cut there, a quoted local part could end in another domain's name. +- **The `From:` line must also read as one address to a mail program, at the + domain that is signed** (review of the stage). OpenDKIM's parser takes + lines no mail program reads as one address - `ceo@bank.com:me@mine.com`, + `ceo@bank.com ` - and signs for the last address while a + lenient program shows the first; such lines are refused. A quoted display + name may still say anything. And the address a reader sees, its local part + unquoted, is the one held against the application's addresses: `"x"@…` no + longer passes where `x@…` belongs to another application. - **Over IPv6 every client looks the same to SelfPost** - documented, not changed. The compose network has no IPv6, so Docker's proxy carries an IPv6 connection and the container sees the network's gateway diff --git a/build/Dockerfile b/build/Dockerfile index 7c2d24b..1e418af 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -120,6 +120,7 @@ COPY --chmod=0644 internal/web/view/static/OFL.txt /usr/share/doc/selfpost/OFL.t COPY --chmod=0644 build/opendkim.conf /etc/opendkim.conf COPY --chmod=0644 build/logrotate-mail.conf /etc/logrotate.d/mail COPY --chmod=0755 build/postfix-wrapper.sh /usr/local/bin/postfix-wrapper.sh +COPY --chmod=0755 build/postfix-reload.sh /usr/local/bin/postfix-reload.sh COPY --chmod=0755 build/postfix-config.sh /usr/local/bin/postfix-config.sh COPY --chmod=0755 build/postfix-cert-reload.sh /usr/local/bin/postfix-cert-reload.sh COPY --chmod=0755 build/logrotate-loop.sh /usr/local/bin/logrotate-loop.sh diff --git a/build/crashexit.py b/build/crashexit.py index 445e5e9..b0f1a33 100644 --- a/build/crashexit.py +++ b/build/crashexit.py @@ -24,8 +24,10 @@ import os import signal import sys -# Programs whose FATAL does not take the container down (build/supervisord.conf). -SPARED = {"logrotate"} +# Programs whose FATAL does not take the container down (build/supervisord.conf): +# logrotate is not on the mail path, and postfix-reload runs on demand - a +# reload it could not start is reported to whoever asked for it. +SPARED = {"logrotate", "postfix-reload"} def write_stdout(s): diff --git a/build/entrypoint.sh b/build/entrypoint.sh index 05e5930..a870261 100644 --- a/build/entrypoint.sh +++ b/build/entrypoint.sh @@ -101,6 +101,10 @@ mkdir -p /data/sasl chown -R panel:selfpost /data/sasl chmod 2750 /data/sasl [ -e /data/sasl/sasldb2 ] && chmod 0640 /data/sasl/sasldb2 +# The copy a password change is made on (internal/app, replaceSASLDB), left +# by a panel killed mid-change: a second copy of every password, which the +# next change would remove anyway. +rm -f /data/sasl/sasldb2.new # Postfix state under /data (architecture.md § Persistence). The panel writes # sender_login_maps; Postfix owns the on-disk queue tree under queue/. @@ -211,6 +215,8 @@ case "$wait_timeout" in exit 1 ;; esac -export POSTFIX_STARTSECS=$((wait_timeout + 10)) +# expr, not $((...)): dash's arithmetic reads "010" as octal and refuses +# "08"; expr takes decimal (review of stage 2c, F11). +export POSTFIX_STARTSECS=$(expr "$wait_timeout" + 10) exec /usr/bin/supervisord -c /etc/supervisor/supervisord.conf diff --git a/build/postfix-reload.sh b/build/postfix-reload.sh new file mode 100644 index 0000000..ddefeca --- /dev/null +++ b/build/postfix-reload.sh @@ -0,0 +1,19 @@ +#!/bin/sh +# The reload the panel asks supervisord for (program postfix-reload, +# build/supervisord.conf; internal/supervisor RunOnce reads its exit status). +# +# Exit 75 says no Postfix master runs - it is stopped, or still in its start +# behind the milter wait (build/postfix-wrapper.sh) - and the master that +# starts next reads the files as they are now, so the panel takes that as +# done. Anything else is "postfix reload" itself: 0, or its failure (Postfix +# 3.7.11, conf/postfix-script, "reload": exit 1 when not running or when +# postsuper fails; src/postfix/postfix.c, a main.cf it cannot read). "master +# -t" exits 0, silently, while no master holds the lock (src/master/master.c, +# test_lock). A master that stops between the two is a failure the panel +# reports - the rarer mistake (review of stage 2c, F5). +d=$(postconf -h daemon_directory) || exit 1 +if "$d/master" -t 2>/dev/null; then + echo "postfix-reload: Postfix is not running; its start reads the files" >&2 + exit 75 +fi +exec /usr/sbin/postfix reload diff --git a/build/postfix-wrapper.sh b/build/postfix-wrapper.sh index f204936..d577fa3 100644 --- a/build/postfix-wrapper.sh +++ b/build/postfix-wrapper.sh @@ -63,9 +63,15 @@ echo "postfix-wrapper: milter sockets ready, starting postfix" # holds the lock (src/master/master.c, test_lock). daemon_directory=$(postconf -h daemon_directory) child= +stop_asked= stop_postfix() { + if [ -z "$child" ]; then + # Between "&" and "child=$!": stopped right after the pid is known. + stop_asked=1 + return + fi trap '' TERM INT - while [ -n "$child" ] && kill -0 "$child" 2>/dev/null; do + while kill -0 "$child" 2>/dev/null; do if "$daemon_directory/master" -t 2>/dev/null; then sleep 1 else @@ -77,6 +83,9 @@ stop_postfix() { trap stop_postfix TERM INT postfix start-fg & child=$! +if [ -n "$stop_asked" ]; then + stop_postfix +fi # A trapped signal makes wait return at once, the trap is taken right after # (POSIX, Shell Command Language, 2.11 Signals and Error Handling) and exits. # Otherwise wait returns when Postfix ended on its own: its status is this diff --git a/build/supervisord.conf b/build/supervisord.conf index 7860ec5..ba6b6d8 100644 --- a/build/supervisord.conf +++ b/build/supervisord.conf @@ -99,16 +99,18 @@ stderr_logfile_maxbytes=0 ; foreground process never reaches it (unlike OpenDKIM, which runs in the ; foreground as the supervised process itself). Instead the panel asks ; supervisord — over the group-accessible control socket — to run this program, -; which executes the canonical `postfix reload` as root. autostart is off (it -; only runs on demand) and a fast, clean exit is expected, so it never trips the -; crashexit listener. (start-fg: see build/postfix-wrapper.sh. supervisor +; which runs the canonical `postfix reload` as root - or exits 75 when no +; master runs (build/postfix-reload.sh). autostart is off (it only runs on +; demand). A program that cannot even be spawned twice in a row ends FATAL +; (startretries=1); crashexit leaves that to the page that asked for the +; reload, which reports it (build/crashexit.py; review of stage 2c, F6). (start-fg: see build/postfix-wrapper.sh. supervisor ; 4.2.5, supervisor/process.py, Subprocess.finish(): with startsecs=0 no exit ; counts as "too quickly", so even a failed run ends EXITED - never BACKOFF, ; never FATAL - with its exit status kept, which the panel reads ; (internal/supervisor, RunOnce). Its resting state is STOPPED until the first ; run, EXITED after it; assumptions C-V10.) [program:postfix-reload] -command=/usr/sbin/postfix reload +command=/usr/local/bin/postfix-reload.sh autostart=false autorestart=false startsecs=0 diff --git a/docs/architecture.md b/docs/architecture.md index cca09d2..5affeab 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -296,7 +296,10 @@ server itself. holds; a parse failure, no address, or a second one after it is refused, and so is an address over 256 bytes, which OpenDKIM would match its `SigningTable` against cut short (`dkimf_apply_signtable()`, - `MAXADDRESS`). Encoded words are not decoded, by either. + `MAXADDRESS`). Encoded words are not decoded, by either. The line must also + be one address in RFC 5322's grammar (`net/mail`), at the domain OpenDKIM + signs for, so that a mail program shows the address that is signed; that + address, its local part unquoted, is what the bindings are asked about. - **The verdict** comes at the end of the headers: `550 5.7.1`, permanent, naming the line, as a reply code of the milter's own. What the client wrote is reduced to printable ASCII and cut before it is said back, logged or diff --git a/docs/security.md b/docs/security.md index 42ba128..1899bd3 100644 --- a/docs/security.md +++ b/docs/security.md @@ -303,6 +303,11 @@ operator can act on it. private key from `certs/` when present; a domain export carries the DKIM key and **working** application passwords in the clear (otherwise a transfer without recreating credentials would be impossible). +- Links under `certs/` (and `certs` itself as a link) are followed, in the + container's view: the archive carries whatever regular file they lead to + that the panel's user may read. `certs/` is the operator's, mounted + read-only; only someone who can write the project directory places a link + there, and only a global user can download the archive (`2.5.2`). - Since 2.2.0 a full backup also carries, in the database, what Postfix logged about each message still in the send log (`send_log_lines`): the submitting client's address and SASL login, the message id, the receiving server and diff --git a/internal/app/sasl.go b/internal/app/sasl.go index ff49fbe..b270197 100644 --- a/internal/app/sasl.go +++ b/internal/app/sasl.go @@ -9,6 +9,7 @@ import ( "io" "os" "os/exec" + "path/filepath" "strings" "sync" ) @@ -113,6 +114,14 @@ func replaceSASLDB(path string, edit func(path string) error) error { _ = os.Remove(tmp) return fmt.Errorf("rename %s -> %s: %w", tmp, path, err) } + // The rename is the directory's change; without its own sync a power + // loss may bring the old database back, out of step with the panel's + // records (review of stage 2c, F10). A directory that cannot be synced + // - not on every system - leaves the change as durable as before. + if dir, err := os.Open(filepath.Dir(path)); err == nil { + _ = dir.Sync() + dir.Close() + } return nil } diff --git a/internal/backup/backup.go b/internal/backup/backup.go index ba8b9d0..9c8a585 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -200,9 +200,16 @@ func validateDeployRoot(deployRoot string) error { // left out of the archive. A directory left out is not descended into. type skipFunc func(path, rel string, d fs.DirEntry) bool -// skipQueue leaves the Postfix queue out of every archive. +// skipQueue leaves the Postfix queue out of every archive, and the copy of +// sasldb2 a change is made on (internal/app, replaceSASLDB): a panel killed +// mid-change leaves it, a second copy of every application's password, until +// the next start removes it (build/entrypoint.sh; review of stage 2c). func skipQueue(_, rel string, _ fs.DirEntry) bool { - return filepath.ToSlash(rel) == QueueDir + switch filepath.ToSlash(rel) { + case QueueDir, "sasl/sasldb2.new": + return true + } + return false } // Check reads through everything Create would put into the archive, without @@ -258,6 +265,13 @@ func addDeployFiles(tw *tar.Writer, deployRoot string, onWarn func(string)) erro } certsDir := filepath.Join(deployRoot, CertsDirName) + // certs/ itself may be a link - the guide once had it linked to + // certbot's live/ - and the walk below does not follow its root + // (path/filepath, WalkDir: "WalkDir does not follow symbolic links"), so + // the link is resolved first (review of stage 2c, F7). + if resolved, err := filepath.EvalSymlinks(certsDir); err == nil { + certsDir = resolved + } if _, err := os.Stat(certsDir); err != nil { if os.IsNotExist(err) { if onWarn != nil { diff --git a/internal/dmarc/parse.go b/internal/dmarc/parse.go index 81c8698..c4fb5da 100644 --- a/internal/dmarc/parse.go +++ b/internal/dmarc/parse.go @@ -7,6 +7,7 @@ import ( "encoding/xml" "fmt" "io" + "strconv" "strings" "time" "unicode/utf8" @@ -57,12 +58,12 @@ type feedbackXML struct { } `xml:"date_range"` } `xml:"report_metadata"` Policy struct { - Domain string `xml:"domain"` - P string `xml:"p"` - SP string `xml:"sp"` - Pct *int `xml:"pct"` - ADKIM string `xml:"adkim"` - ASPF string `xml:"aspf"` + Domain string `xml:"domain"` + P string `xml:"p"` + SP string `xml:"sp"` + Pct *string `xml:"pct"` + ADKIM string `xml:"adkim"` + ASPF string `xml:"aspf"` } `xml:"policy_published"` Records []struct { Row struct { @@ -139,11 +140,15 @@ func ParseAggregate(raw []byte) (ParsedReport, error) { PolicyASPF: field(doc.Policy.ASPF), } // RFC 7489 § 6.3: pct is "between 0 and 100, inclusive", 100 when absent. - // An explicit 0 is a policy applied to no message, and stays 0; only a - // report without the element gets the default (assumptions C-M7). + // An explicit 0 is a policy applied to no message, and stays 0; a report + // without the element, or with an empty or unreadable one, gets the + // default, and a value outside the range is held to it (assumptions + // C-M7; review of stage 2c, F9). out.PolicyPct = 100 if doc.Policy.Pct != nil { - out.PolicyPct = *doc.Policy.Pct + if n, err := strconv.Atoi(strings.TrimSpace(*doc.Policy.Pct)); err == nil { + out.PolicyPct = min(max(n, 0), 100) + } } for _, rec := range doc.Records { row := ParsedRecord{ diff --git a/internal/dmarc/parse_test.go b/internal/dmarc/parse_test.go index 7cf24a6..9cd74b1 100644 --- a/internal/dmarc/parse_test.go +++ b/internal/dmarc/parse_test.go @@ -271,6 +271,11 @@ func TestPolicyPct(t *testing.T) { {"0", 0}, {"25", 25}, {"", 100}, + {"", 100}, + {" 50 ", 50}, + {"150", 100}, + {"-5", 0}, + {"x", 100}, } { doc := strings.Replace(sampleXML, "100", c.pct, 1) got, err := ParseAggregate([]byte(doc)) diff --git a/internal/logtail/logtail.go b/internal/logtail/logtail.go index 98a8172..45ce17e 100644 --- a/internal/logtail/logtail.go +++ b/internal/logtail/logtail.go @@ -758,6 +758,10 @@ func follow(ctx context.Context, path string, tr *tracker, caughtUp chan struct{ select { case <-ctx.Done(): blind.see(time.Now(), false) + // What postlogd wrote to the rotated file since the last tick: + // the next start resumes in the new file only (review of stage + // 2c, F8). + drainOld(time.Now()) tr.record(f, read(), true) // shutdown: the next start resumes here return nil case <-ticker.C: diff --git a/internal/milter/sender.go b/internal/milter/sender.go index cda8970..3ec93f2 100644 --- a/internal/milter/sender.go +++ b/internal/milter/sender.go @@ -3,8 +3,11 @@ package milter import ( "context" "fmt" + "io" "log" + "mime" "net" + "net/mail" "net/textproto" "os" "strings" @@ -201,13 +204,19 @@ func splitTableLine(line string) (key, value string, ok bool) { // whole domain (assumptions B-PS1). The bare local part MA_FIND_DEFAULT also // tries for a local domain is never a key here: every key the panel writes // has an "@" (internal/postfix renderSenderLoginMaps). +// +// Case is folded in ASCII only. Postfix folds as Unicode (src/util/casefold.c +// with ICU, which Debian builds with), but every key and login the panel +// writes is ASCII; a Unicode fold would only let a look-alike pass - the +// Kelvin sign for "k" - for an address OpenDKIM, folding ASCII, does not +// sign (review of stage 2c). func (b *Bindings) Allowed(login, addr string) (bool, error) { b.mu.Lock() defer b.mu.Unlock() if err := b.fresh(); err != nil { return false, err } - login, addr = strings.ToLower(login), strings.ToLower(addr) + login, addr = asciiLower(login), asciiLower(addr) keys := []string{addr} // recipient_delimiter is "+" (build/postfix-config.sh), so an // application listed as invoices@ may also send as invoices+march@. @@ -256,7 +265,7 @@ func (b *Bindings) DomainOf(login string) string { if b.fresh() != nil { return "" } - return b.domainOf[strings.ToLower(login)] + return b.domainOf[asciiLower(login)] } // Refusals is where the check leaves the send-log rows of what it refused. @@ -384,7 +393,8 @@ func (s *senderSession) judge() (shown, why string, err error) { // OpenDKIM takes the first; a reader may show the others. return shown, "it must be one address", nil } - addr := r.user + "@" + asciiLower(r.domain) + signedDomain := asciiLower(r.domain) + addr := r.user + "@" + signedDomain // OpenDKIM matches its SigningTable against "user@domain" as // snprintf() leaves it in a buffer of MAXADDRESS + 1 = 257 bytes // (opendkim.c, dkimf_apply_signtable(); libopendkim/dkim-internal.h). @@ -394,8 +404,26 @@ func (s *senderSession) judge() (shown, why string, err error) { if len(addr) > 256 { return printable(addr), "the address is longer than OpenDKIM reads whole", nil } - shown = printable(addr) - ok, err := s.bind.Allowed(s.login, asciiLower(addr)) + // The address a mail program shows. OpenDKIM's parser reads lines no + // standard reader takes for one address - "ceo@bank.com:me@mine.com", + // "ceo@bank.com ", "" - and signs + // for the last of them while a lenient program shows the first. So the + // line must also be one address in RFC 5322's grammar (net/mail), at the + // domain OpenDKIM signs for; a quoted display name may say anything. + // And that address, its local part unquoted as Postfix holds the + // envelope's (Postfix 3.7.11, src/global/tok822_parse.c), is the one + // held against the bindings: "x"@mine.com is x@mine.com, which may be + // another application's (review of stage 2c, F1, F2). + list, perr := fromParser.ParseList(unfold(raw)) + if perr != nil || len(list) != 1 { + return shown, "it must be one address, as a mail program reads it", nil + } + seen := asciiLower(list[0].Address) + if domainOf(seen) != signedDomain { + return shown, "a mail program would show another address than the one signed", nil + } + shown = printable(seen) + ok, err := s.bind.Allowed(s.login, seen) if err != nil { return shown, "", fmt.Errorf("sender bindings: %w", err) } @@ -405,6 +433,20 @@ func (s *senderSession) judge() (shown, why string, err error) { return shown, "", nil } +// fromParser reads the From: line as a mail program would. A display name +// encoded in a charset Go does not convert (windows-1251, koi8-r, ...) is +// still a display name: its bytes are passed through unread. +var fromParser = mail.AddressParser{WordDecoder: &mime.WordDecoder{ + CharsetReader: func(charset string, input io.Reader) (io.Reader, error) { return input, nil }, +}} + +// unfold joins the lines of a folded header value (RFC 5322 2.2.3): a line +// break is dropped, the white space that follows it stays. Postfix hands a +// milter the line breaks as LF (assumptions B-PM12). +func unfold(v string) string { + return strings.NewReplacer("\r\n", "", "\n", "", "\r", "").Replace(v) +} + // asciiLower lower-cases A-Z only, as OpenDKIM's dkimf_lowercase() and // Postfix's table lookups do; strings.ToLower would also fold characters // outside ASCII - the Kelvin sign to "k" - into names they are not. diff --git a/internal/milter/sender_test.go b/internal/milter/sender_test.go index 437ac0f..96d1744 100644 --- a/internal/milter/sender_test.go +++ b/internal/milter/sender_test.go @@ -20,6 +20,8 @@ import ( // example.org, and two applications share shop.example.org. const senderMaps = `@news.example.net news @shop.example.org shop,shop2 +desk@shop.example.org shop2 +@kiosk.example.net kiosk invoices@example.org billing billing@example.org billing ` @@ -102,9 +104,24 @@ func TestSenderCheck(t *testing.T) { {"a stranger's address in the display name", "shop", [][2]string{from(`"someone@gmail.example" `)}, true, ""}, {"another sender in an encoded display name", "shop", [][2]string{from("=?utf-8?q?director=40example=2Eorg?= ")}, true, ""}, {"a stranger's address in a comment", "shop", [][2]string{from("a@shop.example.org (someone@gmail.example)")}, true, ""}, - {"an unquoted address as the display name", "shop", [][2]string{from("director@example.org ")}, true, ""}, + {"an unquoted address as the display name", "shop", [][2]string{from("director@example.org ")}, false, "as a mail program reads it"}, {"another domain inside a quoted local part", "shop", [][2]string{from(`"director@example.org"@shop.example.org`)}, true, ""}, - {"no local part: OpenDKIM takes the domain for it", "shop", [][2]string{from("@shop.example.org")}, true, ""}, + {"no local part: OpenDKIM takes the domain for it", "shop", [][2]string{from("@shop.example.org")}, false, "as a mail program reads it"}, + // Lines OpenDKIM signs for the application's domain while a lenient + // mail program shows another address first (review of stage 2c, F1). + {"another address before a colon", "shop", [][2]string{from("ceo@bank.example:me@shop.example.org")}, false, "as a mail program reads it"}, + {"a route-like colon inside the brackets", "shop", [][2]string{from("Name ")}, false, "as a mail program reads it"}, + {"a list inside the brackets", "shop", [][2]string{from("")}, false, "as a mail program reads it"}, + {"an address with two @", "shop", [][2]string{from("ceo@bank.example@shop.example.org")}, false, "as a mail program reads it"}, + // An address bound to another application, its local part quoted + // (F2): a reader and Postfix take "desk" for desk. + {"another application's address, quoted", "shop", [][2]string{from(`"desk"@shop.example.org`)}, false, "not an address this application sends as"}, + {"its own bound address, quoted", "shop2", [][2]string{from(`"desk"@shop.example.org`)}, true, ""}, + // A look-alike of the domain in a character a Unicode fold maps + // to ASCII (F3): OpenDKIM would not sign it, and it is not the + // application's. + {"the Kelvin sign for k in the domain", "kiosk", [][2]string{from("a@\u212Aiosk.example.net")}, false, "a mail program would show another address"}, + {"the same domain in ASCII", "kiosk", [][2]string{from("a@kiosk.example.net")}, true, ""}, {"another registered domain", "shop", [][2]string{from("director@example.org")}, false, "not an address this application sends as"}, {"the server's own name", "shop", [][2]string{from("postmaster@mail.example.org")}, false, "not an address this application sends as"}, diff --git a/internal/postfix/postfix.go b/internal/postfix/postfix.go index 02e709f..9affbf5 100644 --- a/internal/postfix/postfix.go +++ b/internal/postfix/postfix.go @@ -6,6 +6,7 @@ package postfix import ( + "errors" "fmt" "log" "path/filepath" @@ -194,18 +195,22 @@ func assertMapSafe(address, login string) error { // saved shows. Before, supervisorctl's start returned as the reload was // spawned, and a reload already running counted as done though it may have // read the files before this change (supervisor.RunOnce; assumptions B-U2). -// A reload that fails while program postfix is not RUNNING - stopped, or -// still in its start, the wrapper waiting for the milter sockets - is no -// error: "the Postfix mail system is not running", and the master that starts -// next reads the files as they are now. +// A reload while no master runs - Postfix stopped, or still in its start, +// the wrapper waiting for the milter sockets - is no error: the program exits +// 75 then (build/postfix-reload.sh), and the master that starts next reads +// the files as they are now. Before, the program's state stood in for that, +// and for the whole of startsecs after a start any failed reload passed as +// one (review of stage 2c, F5). func reloadViaSupervisor() error { err := supervisor.RunOnce("postfix-reload") - if err == nil { - return nil - } - if state, serr := supervisor.State("postfix"); serr == nil && state != "RUNNING" { - log.Printf("postfix: reload while program postfix is %s, left to its start: %v", state, err) + var end *supervisor.EndError + if errors.As(err, &end) && end.State == "EXITED" && end.Status == reloadNotRunning { + log.Printf("postfix: no Postfix master runs; its start reads the files") return nil } return err } + +// reloadNotRunning is build/postfix-reload.sh's exit status when no master +// runs. +const reloadNotRunning = 75 diff --git a/internal/supervisor/rpc.go b/internal/supervisor/rpc.go index 19b6fb9..ff36574 100644 --- a/internal/supervisor/rpc.go +++ b/internal/supervisor/rpc.go @@ -66,16 +66,6 @@ func RunOnce(program string) error { return newRPC(socketPath).runOnce(program) } -// State is the state supervisord gives a program, by its name: RUNNING, -// STARTING, EXITED ... (supervisor 4.2.5, supervisor/states.py). -func State(program string) (string, error) { - v, err := newRPC(socketPath).call("supervisor.getProcessInfo", program) - if err != nil { - return "", fmt.Errorf("supervisor: state of %s: %w", program, err) - } - return v.members()["statename"].str(), nil -} - type rpc struct { client *http.Client } @@ -125,11 +115,25 @@ func (c *rpc) runOnce(program string) error { if info.state == stateExited && info.exitStatus == 0 { return nil } - msg := fmt.Sprintf("supervisor: %s ended %s, exit status %d", program, info.stateName, info.exitStatus) - if info.spawnErr != "" { - msg += ": " + info.spawnErr + return &EndError{Program: program, State: info.stateName, Status: info.exitStatus, SpawnErr: info.spawnErr} +} + +// EndError is a run of RunOnce that did not end EXITED with status 0: the +// state it ended in, its exit status, and supervisord's account of a failed +// spawn. +type EndError struct { + Program string + State string + Status int + SpawnErr string +} + +func (e *EndError) Error() string { + msg := fmt.Sprintf("supervisor: %s ended %s, exit status %d", e.Program, e.State, e.Status) + if e.SpawnErr != "" { + msg += ": " + e.SpawnErr } - return errors.New(msg) + return msg } // processInfo is the part of getProcessInfo's struct RunOnce reads. diff --git a/internal/supervisor/rpc_test.go b/internal/supervisor/rpc_test.go index 860b4b2..d8cc6ac 100644 --- a/internal/supervisor/rpc_test.go +++ b/internal/supervisor/rpc_test.go @@ -1,6 +1,7 @@ package supervisor import ( + "errors" "fmt" "io" "net" @@ -120,6 +121,10 @@ func TestRunOnceReportsAFailedRun(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "ended EXITED, exit status 1") { t.Fatalf("runOnce: %v; want the exit status 1", err) } + var end *EndError + if !errors.As(err, &end) || end.State != "EXITED" || end.Status != 1 { + t.Fatalf("runOnce: %#v; want an EndError, EXITED, status 1", err) + } } // A run in progress may have read the files before the caller wrote them: diff --git a/internal/web/view/static/panel.js b/internal/web/view/static/panel.js index 0233958..adb616f 100644 --- a/internal/web/view/static/panel.js +++ b/internal/web/view/static/panel.js @@ -74,8 +74,14 @@ // traversal (its salvageable state, the event's persisted). window.addEventListener("pagehide", function () { var secrets = document.querySelectorAll("[data-secret]"); + // defaultValue too: it is the input's value attribute and the + // textarea's text, which the kept page still holds after .value is + // emptied (HTML, "The input element": defaultValue reflects the value + // content attribute; "The textarea element": its setter replaces the + // child text). for (var i = 0; i < secrets.length; i++) { secrets[i].value = ""; + secrets[i].defaultValue = ""; } });