Merge pull request #78 from LINUXexpert-org/fix-sieve-overwrite

Never overwrite filters we could not read

web/src/store/__tests__/sieve-overwrite.test.ts

@@ -0,0 +1,85 @@
+import { beforeEach, describe, expect, it } from "vitest";
+import { useSieve } from "@/store/sieve";
+import { newRule, rulesToSieve } from "@/lib/sieve";
+import type { SieveScript } from "@/jmap/types";
+
+/**
+ * Issue #76: adding a filter from a message reported success, and the script
+ * on the server never held more than two rules.
+ *
+ * The chain was three links long, and each looked reasonable alone:
+ *
+ *   1. `load()` recorded a *failed* blob fetch as `contents[id] = ""`.
+ *   2. `sieveToRules("")` returns `[]` — "this script has no rules", which is
+ *      indistinguishable from "we could not read this script".
+ *   3. Saving writes the whole script from that baseline, so every existing
+ *      rule was deleted, and the UI reported success because the write worked.
+ *
+ * The fix is to keep "unknown" and "empty" apart at every step. These pin that:
+ * an unreadable script must never present as an empty one.
+ */
+
+const SCRIPT: SieveScript = { id: "s1", name: "ihasmail", isActive: true, blobId: "b1" } as SieveScript;
+const threeRules = [newRule({ name: "One" }), newRule({ name: "Two" }), newRule({ name: "Three" })];
+
+beforeEach(() => {
+  useSieve.setState({ accountId: "a1", scripts: [SCRIPT], contents: {}, loading: false, error: null });
+});
+
+describe("a script whose content could not be read", () => {
+  it("reports its rules as unknown, not as none", () => {
+    // contents is empty: the fetch failed, or has not happened yet.
+    const { rules, loaded } = useSieve.getState().rules();
+    expect(rules).toBeNull();
+    expect(loaded).toBe(false);
+  });
+
+  it("refuses to save rather than overwriting what it cannot see", async () => {
+    await expect(useSieve.getState().saveRules([newRule({ name: "New" })])).rejects.toThrow(/could not be read/i);
+  });
+
+  it("says so in terms that point at the fix", async () => {
+    // "Reload and try again" is recoverable advice; a generic failure is not.
+    await expect(useSieve.getState().saveRules([newRule({ name: "New" })])).rejects.toThrow(/reload/i);
+  });
+});
+
+describe("a script that is genuinely empty", () => {
+  it("is distinguishable from one that could not be read", () => {
+    useSieve.setState({ contents: { s1: "" } });
+    const { rules, loaded } = useSieve.getState().rules();
+    expect(loaded).toBe(true);
+    expect(rules).toEqual([]);
+  });
+});
+
+describe("a script that was read", () => {
+  it("hands back every rule in it", () => {
+    useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } });
+    const { rules, loaded } = useSieve.getState().rules();
+    expect(loaded).toBe(true);
+    expect(rules).toHaveLength(3);
+    expect(rules?.map((r) => r.name)).toEqual(["One", "Two", "Three"]);
+  });
+
+  it("does not lose rules across a save-shaped round trip", () => {
+    // The regression in one line: N rules in, N + 1 out after adding one.
+    useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } });
+    const before = useSieve.getState().rules().rules!;
+    const after = [...before, newRule({ name: "Four" })];
+    useSieve.setState({ contents: { s1: rulesToSieve(after) } });
+    expect(useSieve.getState().rules().rules).toHaveLength(4);
+  });
+});
+
+describe("reloading", () => {
+  it("does not discard content it already holds when a refetch yields nothing", () => {
+    // saveScript caches what it just wrote, then reloads. A reload whose fetch
+    // failed used to replace the whole map and wipe that.
+    useSieve.setState({ contents: { s1: rulesToSieve(threeRules) } });
+    const kept = useSieve.getState().contents.s1;
+    useSieve.setState((st) => ({ contents: { ...st.contents } })); // merge, not replace
+    expect(useSieve.getState().contents.s1).toBe(kept);
+    expect(useSieve.getState().rules().rules).toHaveLength(3);
+  });
+});

web/src/store/sieve.ts

@@ -18,7 +18,8 @@ interface SieveState {
   load(): Promise<void>;
   getContent(id: Id): Promise<string>;
   /** Rules derived from the "ihasmail" script (null = the active script is hand-written). */
-  rules(): { script: SieveScript | null; rules: SieveRule[] | null; content: string };
+  /** `loaded` distinguishes "this script is hand-written" from "we could not read it". */
+  rules(): { script: SieveScript | null; rules: SieveRule[] | null; content: string; loaded: boolean };
   saveRules(rules: SieveRule[]): Promise<void>;
   saveScript(id: Id | null, name: string, content: string, activate: boolean): Promise<Id>;
   activate(id: Id | null): Promise<void>;
@@ -49,18 +50,29 @@ export const useSieve = create<SieveState>((set, get) => ({
     try {
       const res = await client.call<GetResponse<SieveScript>>("SieveScript/get", { accountId, ids: null });
       set({ scripts: res.list, loading: false, error: null });
-      // Preload contents
+      // Preload contents.
-      const contents: Record<Id, string> = {};
+      //
+      // A fetch that fails must not be recorded as "". An empty script parses
+      // to an empty rule list, which reads as "this script has no rules" and is
+      // indistinguishable from "we could not read this script" -- and the next
+      // save then writes the whole script out from that empty baseline,
+      // destroying every rule in it. That is issue #76.
+      //
+      // Leaving the key absent instead means `rules()` reports the content as
+      // unknown, and `saveRules` refuses rather than guessing.
+      const fetched: Record<Id, string> = {};
       await Promise.all(
         res.list.map(async (s) => {
           try {
-            contents[s.id] = await client.fetchBlobText(accountId, s.blobId, "application/sieve");
+            fetched[s.id] = await client.fetchBlobText(accountId, s.blobId, "application/sieve");
           } catch {
-            contents[s.id] = "";
+            /* leave absent: unknown, not empty */
           }
         }),
       );
-      set({ contents });
+      // Merged, not replaced: saveScript caches the content it just wrote, and
+      // a reload whose fetch failed must not throw that away.
+      set((st) => ({ contents: { ...st.contents, ...fetched } }));
     } catch (err) {
       set({ loading: false, error: (err as Error).message });
     }
@@ -79,12 +91,24 @@ export const useSieve = create<SieveState>((set, get) => ({
   rules() {
     const { scripts, contents } = get();
     const script = scripts.find((s) => s.name === IHASMAIL_SCRIPT) ?? scripts.find((s) => s.isActive) ?? null;
-    const content = script ? (contents[script.id] ?? "") : "";
+    if (!script) return { script: null, rules: [], content: "", loaded: true };
-    return { script, rules: script ? sieveToRules(content) : [], content };
+    const content = contents[script.id];
+    // Not loaded, or the fetch failed. `null` means "cannot say", which every
+    // caller already treats as "do not edit this script" -- as opposed to `[]`,
+    // which means "this script genuinely has no rules" and invites a save that
+    // would overwrite whatever is really in it.
+    if (content === undefined) return { script, rules: null, content: "", loaded: false };
+    return { script, rules: sieveToRules(content), content, loaded: true };
   },
 
   async saveRules(rules) {
     const existing = get().scripts.find((s) => s.name === IHASMAIL_SCRIPT) ?? null;
+    // The last line of defence. Writing rules replaces the whole script, so
+    // doing it from a baseline we never managed to read deletes whatever was
+    // there. Refusing is recoverable; overwriting is not.
+    if (existing && get().contents[existing.id] === undefined) {
+      throw new Error("Your filter script could not be read, so saving would overwrite it. Reload and try again.");
+    }
     await get().saveScript(existing?.id ?? null, IHASMAIL_SCRIPT, rulesToSieve(rules), true);
   },
 

web/src/views/mail/FilterFromMessage.tsx

@@ -33,11 +33,21 @@ export function FilterFromMessageDialog({ email, mailboxId, onClose }: { email:
   }
   if (!ready) return <Dialog open onClose={onClose} title="Create filter" size="sm"><Spinner /></Dialog>;
 
-  const { rules } = sieve.rules();
+  const { rules, loaded } = sieve.rules();
   if (rules === null) {
     return (
       <Dialog open onClose={onClose} title="Create filter" size="sm" footer={<button className="btn" onClick={onClose}>Close</button>}>
-        <p>Your active Sieve script was written by hand, so rules can't be added automatically. Open <b>Settings → Filters & rules</b> to edit the script or switch to managed rules.</p>
+        {/*
+          Two different situations, and telling them apart matters: one is
+          permanent and one is a reload away. Saying "written by hand" when the
+          script merely failed to fetch sends someone looking for a problem
+          they do not have.
+        */}
+        {loaded ? (
+          <p>Your active Sieve script was written by hand, so rules can't be added automatically. Open <b>Settings → Filters & rules</b> to edit the script or switch to managed rules.</p>
+        ) : (
+          <p>Your filter script couldn't be read just now, so adding a rule would risk overwriting it. Reload the page and try again.</p>
+        )}
       </Dialog>
     );
   }