Merge pull request 'mesh-mail: byte-bounded fetch, accounts only to granted callers, grants error kept, secret only stat'ed (hq issue 733)' (#315) from adr-359-mesh-mail-733 into main

modules/mesh-mail/cmd/mesh-mail/config.go

@@ -181,14 +181,12 @@ func ReadSecret(dir string, a Account) (string, bool, error) {
 }
 
 // SecretGiven says whether the account's own secret was given.
+//
+// It looks at the file and never reads it: whoever judges whether a password was given has no need of the password.
+// A file that is there and not empty was given; whether it signs in is the keeper's to say.
 func SecretGiven(dir string, a Account) bool {
-	_, given, err := ReadSecret(dir, a)
-	if err != nil {
-		// A file there that cannot be read was given: the keeper says what is wrong with it.
-		_, statErr := os.Stat(SecretPath(dir, a))
-		return statErr == nil
-	}
-	return given
+	info, err := os.Stat(SecretPath(dir, a))
+	return err == nil && info.Mode().IsRegular() && info.Size() > 0
 }
 
 // ---- grants ------------------------------------------------------------------------------------------------------

modules/mesh-mail/cmd/mesh-mail/config_test.go

@@ -91,13 +91,15 @@ func TestGrantsAreByAccountAndByProvenCaller(t *testing.T) {
 	}
 }
 
-func TestASecretGivenEmptyIsNotGiven(t *testing.T) {
+// Whether a password was given is read from the file's presence and size alone (issue 733): an empty file is not
+// given, a file the judge cannot read is still given, and the password is never read to say so.
+func TestASecretIsGivenByItsFileNotByReadingIt(t *testing.T) {
 	dir := t.TempDir()
 	a := testAccount()
 	if SecretGiven(dir, a) {
 		t.Fatal("given before anything was written")
 	}
-	_ = os.WriteFile(SecretPath(dir, a), []byte("\n"), 0o600)
+	_ = os.WriteFile(SecretPath(dir, a), nil, 0o600)
 	if SecretGiven(dir, a) {
 		t.Fatal("an empty file is a password")
 	}
@@ -105,4 +107,18 @@ func TestASecretGivenEmptyIsNotGiven(t *testing.T) {
 	if pw, given, _ := ReadSecret(dir, a); !given || pw != "pw" || strings.Contains(pw, "\n") {
 		t.Fatalf("%q %v", pw, given)
 	}
+	// Unreadable to the judge, as a secret owned by root is to the watch: still given.
+	if err := os.Chmod(SecretPath(dir, a), 0); err != nil {
+		t.Fatal(err)
+	}
+	if !SecretGiven(dir, a) {
+		t.Fatal("a password the judge cannot read is said not given")
+	}
+	src, _ := os.ReadFile("config.go")
+	body := string(src)
+	body = body[strings.Index(body, "func SecretGiven("):]
+	body = body[:strings.Index(body, "\n}\n")]
+	if strings.Contains(body, "ReadSecret") || strings.Contains(body, "ReadFile") || strings.Contains(body, "os.Open") {
+		t.Fatalf("SecretGiven reads the password:\n%s", body)
+	}
 }

modules/mesh-mail/cmd/mesh-mail/imap.go

@@ -33,8 +33,48 @@ const (
 	RetryFirst  = 30 * time.Second
 	RetryMost   = 5 * time.Minute
 	FetchBatch  = 25
+	// BatchBytes bounds the mail one fetch of bodies holds; SizeBatch is how many sizes one fetch asks.
+	BatchBytes = 64 << 20
+	SizeBatch  = 500
 )
 
+// Batches cuts the uids, in order, into batches of at most most messages whose sizes add up to at most limit bytes. A
+// message larger than limit is a batch of its own. A size not known counts as nothing: the server listed the message,
+// and the fetch says what it holds.
+func Batches(uids []uint32, sizes map[uint32]int64, limit int64, most int) [][]uint32 {
+	var out [][]uint32
+	var cur []uint32
+	var bytes int64
+	for _, u := range uids {
+		size := sizes[u]
+		if len(cur) > 0 && (len(cur) >= most || bytes+size > limit) {
+			out = append(out, cur)
+			cur, bytes = nil, 0
+		}
+		cur = append(cur, u)
+		bytes += size
+	}
+	if len(cur) > 0 {
+		out = append(out, cur)
+	}
+	return out
+}
+
+func uidSet(uids []uint32) imap.UIDSet {
+	set := make([]imap.UID, len(uids))
+	for i, u := range uids {
+		set[i] = imap.UID(u)
+	}
+	return imap.UIDSetNum(set...)
+}
+
+func orBytes(n, def int64) int64 {
+	if n > 0 {
+		return n
+	}
+	return def
+}
+
 // reader is the part of an IMAP client the keeper uses: nothing in it changes a mailbox.
 type reader interface {
 	Login(username, password string) *imapclient.Command
@@ -81,6 +121,8 @@ type Keeper struct {
 
 	// Paces, for the tests; the constants above when zero.
 	IdleCycle, PollEvery, RetryFirst time.Duration
+	// BatchBytes, for the tests; the constant when zero.
+	BatchBytes int64
 }
 
 func (k *Keeper) now() time.Time {
@@ -384,15 +426,29 @@ func (k *Keeper) syncFolder(ctx context.Context, c reader, a Account, folder str
 			return err
 		}
 	}
-	// Added: on the server, not yet in the index; fetched whole, in batches, without marking them seen.
+	// Added: on the server, not yet in the index. Their sizes first, then their bodies in batches bounded by bytes as
+	// well as by count, so one batch never holds more than BatchBytes of mail in memory; a message larger than that
+	// is fetched alone, and said. Fetched without marking them seen.
 	sort.Slice(added, func(i, j int) bool { return added[i] < added[j] })
-	for start := 0; start < len(added); start += FetchBatch {
-		batch := added[start:min(start+FetchBatch, len(added))]
-		uids := make([]imap.UID, len(batch))
-		for i, u := range batch {
-			uids[i] = imap.UID(u)
+	sizes := map[uint32]int64{}
+	for start := 0; start < len(added); start += SizeBatch {
+		part := added[start:min(start+SizeBatch, len(added))]
+		got, err := c.Fetch(uidSet(part), &imap.FetchOptions{UID: true, RFC822Size: true}).Collect()
+		if err != nil {
+			return fmt.Errorf("the folder %s's new messages cannot be measured: %v", folder, err)
 		}
-		msgs, err := c.Fetch(imap.UIDSetNum(uids...), &imap.FetchOptions{
+		answered()
+		for _, m := range got {
+			sizes[uint32(m.UID)] = m.RFC822Size
+		}
+	}
+	cap := orBytes(k.BatchBytes, BatchBytes)
+	for _, batch := range Batches(added, sizes, cap, FetchBatch) {
+		if len(batch) == 1 && sizes[batch[0]] > cap {
+			k.logf("[mesh-mail] the account %s's message %d in %s is %d bytes, more than a batch holds; it is fetched alone",
+				a.Name, batch[0], folder, sizes[batch[0]])
+		}
+		msgs, err := c.Fetch(uidSet(batch), &imap.FetchOptions{
 			UID: true, Flags: true, Envelope: true, RFC822Size: true, InternalDate: true,
 			BodySection: []*imap.FetchItemBodySection{{Peek: true}},
 		}).Collect()

modules/mesh-mail/cmd/mesh-mail/imap_test.go

@@ -5,10 +5,12 @@ import (
 	"crypto/sha256"
 	"encoding/hex"
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
 	"regexp"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
@@ -237,3 +239,54 @@ func TestNothingButTheTestsWritesToAMailbox(t *testing.T) {
 		t.Fatal("folders are not opened read-only, or bodies not peeked")
 	}
 }
+
+// A fetch of bodies is bounded by bytes (issue 733): batches cut by size and by count, and a message larger than a
+// batch holds alone.
+func TestBatchesAreBoundedByBytesAndALargerMessageGoesAlone(t *testing.T) {
+	sizes := map[uint32]int64{1: 40, 2: 40, 3: 500, 4: 10, 5: 10, 6: 10}
+	got := Batches([]uint32{1, 2, 3, 4, 5, 6}, sizes, 100, 2)
+	want := [][]uint32{{1, 2}, {3}, {4, 5}, {6}}
+	if fmt.Sprint(got) != fmt.Sprint(want) {
+		t.Fatalf("%v, not %v", got, want)
+	}
+	if got := Batches(nil, nil, 100, 25); len(got) != 0 {
+		t.Fatalf("%v", got)
+	}
+}
+
+// Over a real conversation: with a cap of a few hundred bytes, a large mail is fetched alone and said so, and every
+// mail is still indexed and said.
+func TestTheKeeperFetchesUnderAByteCap(t *testing.T) {
+	s := newMailServer(t)
+	op := s.operator(t)
+	st := newMemStore()
+	secrets := t.TempDir()
+	giveSecret(t, secrets, testAccount(), testPassword)
+	var mu sync.Mutex
+	var said []string
+	k := &Keeper{Store: st, Objects: newMemObjects(), Dial: s.dialer(), SecretsDir: secrets,
+		Logf:      func(f string, a ...any) { mu.Lock(); said = append(said, fmt.Sprintf(f, a...)); mu.Unlock() },
+		IdleCycle: 300 * time.Millisecond, PollEvery: 400 * time.Millisecond, RetryFirst: 100 * time.Millisecond, BatchBytes: 600}
+	ctx, cancel := context.WithCancel(context.Background())
+	done := make(chan struct{})
+	go func() { k.Watch(ctx, testAccount()); close(done) }()
+	t.Cleanup(func() { cancel(); <-done })
+	eventually(t, 10*time.Second, "the first look", func() (bool, string) { return stateOf(st, "mesh").State == AccountCurrent, describe(st) })
+	deliver(t, op, "INBOX", plainMail("small one", "short"))
+	deliver(t, op, "INBOX", plainMail("large one", strings.Repeat("a long letter ", 200)))
+	deliver(t, op, "INBOX", plainMail("small two", "short"))
+	eventually(t, 5*time.Second, "three mails said", func() (bool, string) {
+		return len(eventsNamed(st, "mail.mesh.message.added")) == 3, describe(st)
+	})
+	mu.Lock()
+	defer mu.Unlock()
+	alone := 0
+	for _, line := range said {
+		if strings.Contains(line, "fetched alone") {
+			alone++
+		}
+	}
+	if alone != 1 {
+		t.Fatalf("said %d message(s) fetched alone: %v", alone, said)
+	}
+}

modules/mesh-mail/cmd/mesh-mail/tools.go

@@ -46,27 +46,48 @@ func (b *Bundle) now() time.Time {
 	return time.Now()
 }
 
-func (b *Bundle) settings() ([]Account, Grants, error) {
+// settings are the accounts and the grants. A grants setting that cannot be read grants nothing, and its reason is
+// kept, so a refusal says why rather than only that the caller is not granted.
+func (b *Bundle) settings() ([]Account, access, error) {
 	accounts, err := ReadAccounts(b.Env.Accounts)
 	if err != nil {
-		return nil, nil, err
+		return nil, access{}, err
 	}
 	grants, err := ReadGrants(b.Env.Grants)
 	if err != nil {
-		// A grants setting that cannot be read grants nothing; the refusal says why.
-		return accounts, Grants{}, nil
+		return accounts, access{grants: Grants{}, unread: err}, nil
 	}
-	return accounts, grants, nil
+	return accounts, access{grants: grants}, nil
+}
+
+// access is the grants as read, and why they could not be, when they could not.
+type access struct {
+	grants Grants
+	unread error
+}
+
+func (a access) Allows(caller, account string) bool { return a.grants.Allows(caller, account) }
+
+func (a access) Granted(caller string, accounts []Account) []string {
+	return a.grants.Granted(caller, accounts)
+}
+
+// refuse is the refusal of a caller for an account, saying why: the grants unread, or the caller not in them.
+func (a access) refuse(caller, account string) error {
+	if a.unread != nil {
+		return fmt.Errorf("refused: the setting grants cannot be read, so it grants nobody: %v", a.unread)
+	}
+	return refused(caller, account)
 }
 
 // Tools are the module's tools.
 func (b *Bundle) Tools() []stdio.Tool {
 	return []stdio.Tool{
 		{Name: AccountsTool,
-			Description: "The operator's mail accounts this module watches, each with its kind, its state (syncing, current, " +
-				"waits-for-sign-in or failing, and why), when its server last answered, when it was last brought up to date, and " +
-				"its folders with how many messages each holds. An account's mail address is said only to a caller granted that " +
-				"account; whether you are is said per account. Read-only.",
+			Description: "The operator's mail accounts you are granted, each with its kind, its address, its state (syncing, " +
+				"current, waits-for-sign-in or failing, and why), when its server last answered, when it was last brought up to " +
+				"date, and its folders with how many messages each holds. An account you are not granted is not listed, and " +
+				"nothing of it is said. How the module as a whole is, is mail_health's to say. Read-only.",
 			Input: schema(nil, map[string]any{}),
 			Run:   b.accountsTool},
 		{Name: SearchTool,
@@ -128,9 +149,13 @@ func (b *Bundle) accountsTool(args map[string]any) (any, error) {
 		return nil, err
 	}
 	caller := b.caller(args)
-	var out []map[string]any
+	out := []map[string]any{}
 	for _, a := range accounts {
-		entry := map[string]any{"account": a.Name, "kind": a.Kind, "granted": grants.Allows(caller, a.Name)}
+		// Only the accounts the caller is granted: of any other it learns nothing, not even that it exists.
+		if !grants.Allows(caller, a.Name) {
+			continue
+		}
+		entry := map[string]any{"account": a.Name, "kind": a.Kind, "address": a.Address}
 		if s, ok := states[a.Name]; ok {
 			entry["state"], entry["why"] = s.State, s.Why
 			if !s.Heard.IsZero() {
@@ -142,9 +167,6 @@ func (b *Bundle) accountsTool(args map[string]any) (any, error) {
 		} else {
 			entry["state"], entry["why"] = "unknown", "its keeper has said nothing of it yet"
 		}
-		if grants.Allows(caller, a.Name) {
-			entry["address"] = a.Address
-		}
 		folders, err := st.Folders(ctx, a.Name)
 		if err != nil {
 			return nil, err
@@ -152,7 +174,11 @@ func (b *Bundle) accountsTool(args map[string]any) (any, error) {
 		entry["folders"] = folders
 		out = append(out, entry)
 	}
-	return map[string]any{"accounts": out, "caller": caller}, nil
+	answer := map[string]any{"accounts": out, "caller": caller}
+	if grants.unread != nil {
+		answer["why"] = "the setting grants cannot be read, so it grants nobody: " + grants.unread.Error()
+	}
+	return answer, nil
 }
 
 func (b *Bundle) searchTool(args map[string]any) (any, error) {
@@ -180,11 +206,14 @@ func (b *Bundle) searchTool(args map[string]any) (any, error) {
 	granted := grants.Granted(caller, accounts)
 	if account != "" {
 		if !grants.Allows(caller, account) {
-			return nil, refused(caller, account)
+			return nil, grants.refuse(caller, account)
 		}
 		granted = []string{account}
 	}
 	if len(granted) == 0 {
+		if grants.unread != nil {
+			return nil, grants.refuse(caller, "")
+		}
 		return nil, fmt.Errorf("refused: %s is granted no mail account; the operator grants one through the setting grants", caller)
 	}
 	ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
@@ -224,7 +253,7 @@ func (b *Bundle) readTool(args map[string]any) (any, error) {
 	}
 	caller := b.caller(args)
 	if !grants.Allows(caller, account) {
-		return nil, refused(caller, account)
+		return nil, grants.refuse(caller, account)
 	}
 	ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second)
 	defer cancel()

modules/mesh-mail/cmd/mesh-mail/tools_test.go

@@ -193,15 +193,24 @@ func TestTheBodyToolsAnswerAGrantedCallerAndRefuseAnyOther(t *testing.T) {
 }
 
 // mail_accounts says each account's state to anyone, and its address only to a caller granted it.
-func TestTheAccountsToolSaysTheAddressOnlyToAGrantedCaller(t *testing.T) {
+func TestTheAccountsToolListsOnlyTheAccountsTheCallerIsGranted(t *testing.T) {
 	b, _, _ := aBundle(t)
 	rt := startBundle(t, b.Tools())
+	_ = b.Env // the index holds a folder of the account, so an ungranted answer has something to leak
+	st, _ := b.Index(context.Background())
+	_ = st.SetFolder(context.Background(), Folder{Account: "mesh", Name: "Secret-Project", Messages: 41, Synced: judgedAt})
+	_ = st.SetState(context.Background(), AccountState{Account: "mesh", State: AccountFailing, Why: "the server said NO [AUTHENTICATIONFAILED]", Said: judgedAt})
 	text, isErr := rt.call(AccountsTool, "novox.other", map[string]any{})
-	if isErr || !strings.Contains(text, `"state":"current"`) || strings.Contains(text, "jschoubben@") || !strings.Contains(text, `"granted":false`) {
+	if isErr || !strings.Contains(text, `"accounts":[]`) {
 		t.Fatalf("ungranted: %s", text)
 	}
+	for _, leak := range []string{"mesh\"", "jschoubben@", "Secret-Project", "41", "AUTHENTICATIONFAILED", "failing"} {
+		if strings.Contains(text, leak) {
+			t.Fatalf("an ungranted caller learns %q: %s", leak, text)
+		}
+	}
 	text, _ = rt.call(AccountsTool, "novox.triage", map[string]any{})
-	if !strings.Contains(text, `"address":"jschoubben@novox.be"`) || !strings.Contains(text, `"granted":true`) {
+	if !strings.Contains(text, `"address":"jschoubben@novox.be"`) || !strings.Contains(text, "Secret-Project") || !strings.Contains(text, "AUTHENTICATIONFAILED") {
 		t.Fatalf("granted: %s", text)
 	}
 }
@@ -245,3 +254,22 @@ func TestTheEventsAreSaidOnTheBusThroughTheRuntime(t *testing.T) {
 		t.Fatalf("published %s", raw)
 	}
 }
+
+// A grants setting that cannot be read grants nobody, and every refusal says why (issue 733).
+func TestARefusalSaysWhyTheGrantsCouldNotBeRead(t *testing.T) {
+	b, _, _ := aBundle(t)
+	writeFile(t, b.Env.Grants, `{"grants": {"mesh": "triage"}}`)
+	rt := startBundle(t, b.Tools())
+	text, isErr := rt.call(ReadTool, "novox.triage", map[string]any{"account": "mesh", "folder": "INBOX", "uid": 7})
+	if !isErr || !strings.Contains(text, "the setting grants cannot be read") || !strings.Contains(text, "not the module's") || strings.Contains(text, "Please find") {
+		t.Fatalf("read: %v %s", isErr, text)
+	}
+	text, isErr = rt.call(SearchTool, "novox.triage", map[string]any{"query": "invoice"})
+	if !isErr || !strings.Contains(text, "the setting grants cannot be read") {
+		t.Fatalf("search: %v %s", isErr, text)
+	}
+	text, _ = rt.call(AccountsTool, "novox.triage", map[string]any{})
+	if !strings.Contains(text, "the setting grants cannot be read") || !strings.Contains(text, `"accounts":[]`) {
+		t.Fatalf("accounts: %s", text)
+	}
+}