diff --git a/lib/sieve/__tests__/generator.test.ts b/lib/sieve/__tests__/generator.test.ts index 6ad885628..1fb0c51c1 100644 --- a/lib/sieve/__tests__/generator.test.ts +++ b/lib/sieve/__tests__/generator.test.ts @@ -241,22 +241,53 @@ describe('generateScript', () => { expect(matches).toHaveLength(1); }); - it('does not append stop after discard', () => { + // discard and reject only cancel the implicit keep (RFC 5228 4.4, + // RFC 5429): without a stop, the rules below still act on the message. + it('appends stop after discard', () => { const script = generateScript([makeRule({ actions: [{ type: 'discard' }], stopProcessing: true, })]); - const matches = script.match(/stop;/g); - expect(matches).toBeNull(); + expect(script).toContain(' discard;\n stop;\n}'); + expect(script.match(/stop;/g)).toHaveLength(1); }); - it('does not append stop after reject', () => { + it('appends stop after reject', () => { const script = generateScript([makeRule({ actions: [{ type: 'reject', value: 'No' }], stopProcessing: true, })]); - const matches = script.match(/stop;/g); - expect(matches).toBeNull(); + expect(script).toContain(' reject "No";\n stop;\n}'); + expect(script.match(/stop;/g)).toHaveLength(1); + }); + + it('leaves a rule that does not stop processing open after discard', () => { + const script = generateScript([makeRule({ + actions: [{ type: 'discard' }], + stopProcessing: false, + })]); + expect(script).not.toContain('stop;'); + }); + + it('writes one stop when the stop action ends up last behind the flags', () => { + // Flags are written first, so the stop action placed before a flag in + // the list still comes last. + const script = generateScript([makeRule({ + actions: [{ type: 'stop' }, { type: 'mark_read' }], + stopProcessing: true, + })]); + expect(script).toContain(' addflag "\\\\Seen";\n stop;\n}'); + expect(script.match(/stop;/g)).toHaveLength(1); + }); + + it('adds no second stop behind a stop action earlier in the block', () => { + // The block runs straight through, so that stop ends the script. + const script = generateScript([makeRule({ + actions: [{ type: 'stop' }, { type: 'discard' }], + stopProcessing: true, + })]); + expect(script).toContain(' stop;\n discard;\n}'); + expect(script.match(/stop;/g)).toHaveLength(1); }); }); diff --git a/lib/sieve/__tests__/parser.test.ts b/lib/sieve/__tests__/parser.test.ts index 3d6af5484..ddd49f3d4 100644 --- a/lib/sieve/__tests__/parser.test.ts +++ b/lib/sieve/__tests__/parser.test.ts @@ -122,6 +122,53 @@ describe('parseScript', () => { expect(result.rules).toEqual(rules); }); + it('reads a rule whose name has runs of whitespace back as one rule, save after save', () => { + // A hand-written block, and one that only looks like Bulwark's. + const external = '# Hand\nif header :contains "subject" "hand" {\n keep;\n}\n' + + '# Rule: Someone else\nif header :contains "subject" "other" {\n keep;\n}\n'; + const rules = [ + makeRule({ id: '1', name: 'Foo Bar' }), + makeRule({ id: '2', name: ' Tab\tand trailing ' }), + // No-break and ideographic spaces count as whitespace too. + makeRule({ id: '3', name: 'Büro  Post' }), + // Not possible in the editor, but in metadata written elsewhere. + makeRule({ id: '4', name: '' }), + ]; + let script = `${generateScript(rules)}\n${external}`; + for (let save = 0; save < 3; save++) { + const result = parseScript(script); + expect(result.isOpaque).toBe(false); + // The four rules, and each other block once. + const own = result.rules.filter((r) => (r.origin ?? 'bulwark') === 'bulwark'); + expect(own.map((r) => r.id)).toEqual(['1', '2', '3', '4']); + expect(result.rules.length - own.length).toBe(2); + script = generateScript(result.rules, result.vacation, { externalRequires: result.externalRequires }); + } + expect(script.split('# Rule: Foo Bar\n').length).toBe(2); + expect(script.split('# Rule: Büro Post\n').length).toBe(2); + expect(script.split('# Rule: \n').length).toBe(2); + expect(script.split('if header :contains "subject" "hand"').length).toBe(2); + expect(script.split('# Rule: Someone else').length).toBe(2); + }); + + it('takes a block written before names were collapsed for the rule itself', () => { + // Up to 1.11.0 the "# Rule:" line carried the name as typed. + const rule = makeRule({ id: '1', name: 'Foo Bar' }); + const asWritten = generateScript([rule]).replace('# Rule: Foo Bar', '# Rule: Foo Bar'); + expect(asWritten).toContain('# Rule: Foo Bar'); + expect(parseScript(asWritten).rules).toEqual([rule]); + }); + + it('drops the copies of such a rule an earlier save left behind', () => { + const rule = makeRule({ id: '1', name: 'Foo Bar' }); + const block = generateScript([rule]).slice(generateScript([rule]).indexOf('# Rule: Foo Bar')); + // As written before: the rule, and two copies read back as someone else's. + const withCopies = `${generateScript([rule])}\n# --- External rules (managed outside Bulwark) ---\n${block}\n${block}`; + const result = parseScript(withCopies); + expect(result.rules).toEqual([rule]); + expect(generateScript(result.rules).split('# Rule: Foo Bar').length).toBe(2); + }); + it('preserves rules with special characters', () => { const rules = [makeRule({ conditions: [{ field: 'subject', comparator: 'contains', value: 'say "hello" \\ world' }], diff --git a/lib/sieve/generator.ts b/lib/sieve/generator.ts index 9575f1eb3..f1dae3d5b 100644 --- a/lib/sieve/generator.ts +++ b/lib/sieve/generator.ts @@ -359,11 +359,13 @@ export function generateScript( const actionLines = generateActions(rule.actions, useMailboxId); - if (rule.stopProcessing) { - const lastAction = rule.actions[rule.actions.length - 1]; - if (!lastAction || !['stop', 'discard', 'reject'].includes(lastAction.type)) { - actionLines.push('stop;'); - } + // discard and reject only cancel the implicit keep (RFC 5228 4.4, + // RFC 5429); the script goes on, and the rules below would still act on + // the message. So "stop processing" always writes a stop, unless the + // block has one already: it runs straight through, so any stop in it ends + // the script. + if (rule.stopProcessing && !actionLines.includes('stop;')) { + actionLines.push('stop;'); } lines.push(`if ${conditionStr} {`); diff --git a/lib/sieve/parser.ts b/lib/sieve/parser.ts index d60f21c88..f6580f8e4 100644 --- a/lib/sieve/parser.ts +++ b/lib/sieve/parser.ts @@ -847,10 +847,15 @@ export function parseScript(content: string): ParseResult { // identifies it as ours. const filteredExternal = external.rules.filter(r => { const raw = r.rawBlock || ''; - const match = raw.match(/#\s*Rule:\s*(.+?)\s*$/m); + // The name ends with its line, and may be empty. + const match = raw.match(/#\s*Rule:[ \t]*(.*?)[ \t]*$/m); if (match) { - const name = match[1].trim(); - if (bulwarkRules.some(b => b.name === name)) return false; + // The generator writes the name on one line, its whitespace runs + // collapsed; compared as written, "Foo Bar" would come back as + // someone else's rule, and as one more copy on every save. + const oneLine = (name: string) => name.replace(/\s+/g, ' ').trim(); + const name = oneLine(match[1]); + if (bulwarkRules.some(b => oneLine(b.name) === name)) return false; } if (/#\s*Vacation auto-reply/i.test(raw)) return false; return true;