audit 4: findings of the two reviews
test / test (push) Canceled after 0s

- Two inbound domains on one upstream could never change its TLS mode,
  each save refused for the other (medium): SetTransportAs changes the
  mode for every domain on the upstream when the one saving may change
  them all; a domain user who may not is refused with the mode, not the
  other tenant's domain name (security F1: the first version named it).
- A deletion's leftover DKIM key is removed by Resync (Reload
  configuration, a restore's first start) for names no domain has.
- The backup's write deadline moves before Check.
- hasUnsavedEdit skips password and file fields and compares a multiple
  select's options; htmx.config.timeout 30 s, so a refresh never answered
  ends and is tried again; a failed refresh's promise is handled.
- An empty filtered Outbound log says no message matches the filters.
- The dummy bcrypt hash is made at start; a DMARC part over 8 MiB is
  refused for its size like the other two limits.
- Health's Configuration box says what Reload writes (design first,
  32dfe4a).
- Docs: CHANGELOG, guide § Inbound, security.md (the shared upstream's
  mode, the profile form's "username taken"), architecture.md (owed to an
  address), the plan's notes corrected.
Tests: the shared upstream (service, handler), the leftover key, all eight
Health boxes ordered, the subject from before migration 0011, the new JS
cases.

Tested on the test server: go vet + go test of 25 packages, make e2e (all
pass, 636 s) on this tree.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
mix
2026-10-15 12:46:52 +03:00
co-authored by Claude Opus 5.5
parent 32dfe4a2cf
commit eaa7de0754
23 changed files with 359 additions and 83 deletions
+11 -9
View File
@@ -79,6 +79,17 @@ func (h *Handlers) HandleBackup(w http.ResponseWriter, r *http.Request) {
logf("panel: full backup: %s", msg)
},
}
// The server's write deadline is for pages (cmd/panel newHTTPServer); an
// archive streams for as long as it takes, and the read-through before it
// (Check) is as long as the archive: the deadline moves first, so a
// refusal after a long Check can still be written (review of stage 4).
// Two hours carry a gigabyte at a little over 1 Mbit/s, and still end a
// client that keeps the connection and stops reading, which TCP alone
// would not (net/http, Go 1.26: ResponseController.SetWriteDeadline).
if err := http.NewResponseController(w).SetWriteDeadline(time.Now().Add(2 * time.Hour)); err != nil {
logf("panel: full backup: lift the write deadline: %v", err)
}
if err := backup.ValidateDeployRoot(h.cfg.DeployRoot); err != nil {
logf("panel: full backup: %v", err)
h.renderBackupPageWith(w, r, http.StatusBadRequest, "", deployBackupErr(err))
@@ -107,15 +118,6 @@ func (h *Handlers) HandleBackup(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Disposition", fmt.Sprintf("attachment; filename=%q", filename))
w.Header().Set("Cache-Control", "no-store")
// The server's write deadline is for pages (cmd/panel newHTTPServer); an
// archive streams for as long as it takes. Two hours carry a gigabyte at
// a little over 1 Mbit/s, and still end a client that keeps the
// connection and stops reading, which TCP alone would not (net/http, Go
// 1.26: ResponseController.SetWriteDeadline).
if err := http.NewResponseController(w).SetWriteDeadline(time.Now().Add(2 * time.Hour)); err != nil {
logf("panel: full backup: lift the write deadline: %v", err)
}
// Everything below streams: past this point headers (and possibly some
// bytes) are already on the wire, so a failure cannot switch to a clean
// error page. Log it and let the truncated download fail loudly on the
+21 -1
View File
@@ -250,6 +250,8 @@ func firstNonEmpty(ss ...string) string {
func inboundFlash(r *http.Request) string {
switch {
case r.URL.Query().Get("shared") != "":
return "Upstream saved. Its TLS mode is one for every domain forwarded to it, and the other domains on it were changed too."
case r.URL.Query().Get("saved") != "":
return "Upstream saved."
case r.URL.Query().Get("recipients") != "":
@@ -284,12 +286,30 @@ func (h *Handlers) HandleInboundTransport(w http.ResponseWriter, r *http.Request
host := r.PostFormValue("host")
port := r.PostFormValue("port")
tlsMode := r.PostFormValue("tls_mode")
if err := h.inbound.SetTransport(d.ID, host, port, tlsMode); err != nil {
p, _ := h.principal(r)
changed, err := h.inbound.SetTransportAs(d.ID, host, port, tlsMode, func(o store.InboundDomain) bool {
return p.CanAccessInboundDomain(o.ID)
})
var shared *inbound.UpstreamShared
if errors.As(err, &shared) {
// The other domain is another tenant's: its mode is said, its name
// is not (security review of audit stage 4).
logf("panel: inbound: save upstream of %s: %v", d.Name, err)
h.renderInboundDetail(w, r, http.StatusBadRequest, d, inboundDetailView{
TransportErr: "This upstream is also used, with TLS mode " + shared.Mode + ", by an inbound domain you do not manage, and an upstream has one TLS mode: choose " + shared.Mode + ", or ask the administrator to change it for both.",
})
return
}
if err != nil {
h.renderInboundDetail(w, r, http.StatusBadRequest, d, inboundDetailView{
TransportErr: inboundErrorMessage(err, "upstream", "the upstream"),
})
return
}
if changed > 0 {
http.Redirect(w, r, fmt.Sprintf("/inbound/domains/%d?saved=1&shared=%d", d.ID, changed), http.StatusSeeOther)
return
}
http.Redirect(w, r, fmt.Sprintf("/inbound/domains/%d?saved=1", d.ID), http.StatusSeeOther)
}
@@ -1,6 +1,7 @@
package handlers
import (
"fmt"
"net/http"
"net/http/httptest"
"net/url"
@@ -227,3 +228,37 @@ func TestInboundDeletePageNamesWhatGoesWithTheDomain(t *testing.T) {
has(t, "delete page, any recipient", body, `remove its upstream.`)
lacks(t, "delete page, any recipient", body, `recipient`)
}
// A domain user saving an upstream another tenant's inbound domain shares,
// with another TLS mode, is told the mode and not the domain; the global
// role changes the mode for both (security and technical reviews of audit
// stage 4).
func TestSharedUpstreamTLSModeForADomainUser(t *testing.T) {
h, st := inboundHandlers(t)
mine, _ := h.inbound.Add("mine.example")
theirs, _ := h.inbound.Add("theirs.example")
if err := h.inbound.SetTransport(theirs.ID, "10.0.0.8", "25", "encrypt"); err != nil {
t.Fatal(err)
}
p := userWith(t, st, "tenant", store.RoleDomain, store.Reach{InboundDomainIDs: []int64{mine.ID}})
form := url.Values{"host": {"10.0.0.8"}, "port": {"25"}, "tls_mode": {"may"}}
rec := inboundCall(h, http.MethodPost, fmt.Sprintf("/inbound/domains/%d/upstream", mine.ID), form, p)
body := rec.Body.String()
if rec.Code != http.StatusBadRequest || !strings.Contains(body, "with TLS mode encrypt, by an inbound domain you do not manage") {
t.Fatalf("a domain user on another's upstream = %d:\n%s", rec.Code, body)
}
if strings.Contains(body, "theirs.example") {
t.Error("the refusal names the other tenant's domain")
}
if err := h.inbound.SetTransport(mine.ID, "10.0.0.8", "25", "encrypt"); err != nil {
t.Fatal(err)
}
rec = inboundCall(h, http.MethodPost, fmt.Sprintf("/inbound/domains/%d/upstream", mine.ID), form, globalPrincipal)
if rec.Code != http.StatusSeeOther || !strings.Contains(rec.Header().Get("Location"), "shared=1") {
t.Fatalf("the global role changing a shared mode = %d %q", rec.Code, rec.Header().Get("Location"))
}
if d, _ := st.GetInboundDomain(theirs.ID); d.TLSMode != "may" {
t.Errorf("the other domain kept %s", d.TLSMode)
}
}
@@ -268,9 +268,12 @@ func TestOutLogFiltersByStatusAndDay(t *testing.T) {
t.Errorf("today shows %d rows, want 4", got)
}
for _, q := range []string{"from=" + tomorrow, "to=" + yesterday} {
if body := getBody(t, h.HandleDeliveries, "/outbound/log?"+q); strings.Contains(body, `">Details</a>`) {
body := getBody(t, h.HandleDeliveries, "/outbound/log?"+q)
if strings.Contains(body, `">Details</a>`) {
t.Errorf("%s shows rows from today", q)
}
// Filtered to nothing is said so, not "no messages logged yet".
has(t, q, body, "No messages match these filters.")
}
body := getBody(t, h.HandleDeliveries, "/outbound/log?status=rejected&from="+today)
has(t, "filtered", body, `name="from" value="`+today+`"`,