Skip to content

fix: renaming a tag leaves filter rules tagging under the old id - #854

Open
dev-hive-kazniisa wants to merge 1 commit into
bulwarkmail:mainfrom
dev-hive-kazniisa:fix/keyword-rename-sieve-sync
Open

fix: renaming a tag leaves filter rules tagging under the old id#854
dev-hive-kazniisa wants to merge 1 commit into
bulwarkmail:mainfrom
dev-hive-kazniisa:fix/keyword-rename-sieve-sync

Conversation

@dev-hive-kazniisa

Copy link
Copy Markdown
Contributor

fix: renaming a tag leaves filter rules tagging under the old id

The bug

A tag's id is a slug of its display name (composeKeywordId in lib/keyword-nesting.ts), and the id is what messages carry: $label:<id>. Renaming a tag therefore changes the keyword, and the tag settings migrate every message with client.migrateKeyword so nothing is orphaned.

Sieve rules name the same id, and nothing migrated those. A rule with an add_label action keeps writing the old keyword, in both places that matter: the generated addflag "$label:<old>" line and the metadata block the visual editor reads its rules back from.

What the user sees after renaming "Red" to "ISO":

  • mail arriving afterwards is still tagged $label:red. No definition names that id any more, so formatKeywordLabels falls back to printing the raw id and useKeywordFormat falls back to grey — a tag called "red" that no longer exists in settings.
  • filtering or searching by the renamed tag looks for $label:iso and misses everything that has arrived since the rename.
  • opening the rule shows an empty tag selector, because the <select> in filter-rule-modal.tsx only offers tags that have definitions.

Together this reads as "the filter stopped working", while the rule is running exactly as written.

Reproduce: define a filter rule whose action is Add tag → Red, rename the tag Red to ISO in Settings → Tags, then send yourself a message matching the rule.

The fix

lib/sieve/keyword-rename.ts (new) rewrites the rules:

  • every add_label action naming the old id, disabled rules included — leaving one behind would have it tag under the old id the moment it is switched back on;
  • the rawBlock of an external rule, since that is what actually reaches the generated script for those. Only the operand of an addflag is touched: a test comparing a header against the same literal is the rule's own matching logic, not the tag it applies.

useFilterStore.renameKeywordInFilters fetches, rewrites and uploads the account's active script:

  • it does not go through fetchFilters/saveFilters, so a rename started from the tag settings cannot change which account an open filter view is showing. That view is updated in place when it happens to be showing the same account.
  • the write runs through onSieveScriptGenerate.transform and emits onSieveScriptChange, exactly as a normal save does — skipping the transform would drop a plugin's managed section from the script this write makes active.
  • a script that is not the active one is left alone entirely. updateSieveScript activates what it writes, and a tag rename must not switch someone's filters back on.
  • whatever still names the old tag in the script that ends up being uploaded — a rule kept verbatim, a test comparing against the keyword, a section a plugin just grafted back on — is counted. The tag settings then warn that some rules still use the old tag, since only the user can fix those. A hand-edited (opaque) script is never regenerated, for the same reason.

Mail delivered between the message migration and the rewrite was still tagged by the old rule, and the migration has already passed it by, so a successful rewrite is followed by one more migration sweep — after which no rewritten rule can write the old keyword again.

Tests

lib/sieve/__tests__/keyword-rename.test.ts covers the rewrite (disabled rules, other action types, prefix collisions such as red vs red-alert, nested ids, addflag-only substitution in raw blocks, counting what it deliberately leaves alone).

stores/__tests__/filter-store-keyword-rename.test.ts covers the store path: the script and its metadata rewritten and uploaded, no write when nothing references the tag, an inactive script left alone, a server without Sieve, an opaque script reported instead of regenerated, the plugin transform applied and its own references counted, another account's loaded state untouched, an upload failure propagated.

npm run typecheck and npm run lint pass. npx vitest run has no new failures.

Notes

  • One new string, settings.keywords.filters_migration_error, added to en/common.json and to the other 24 locales in the same PR.
  • The only UI surface is that warning toast; there is no layout change to screenshot.
  • The account rewritten is client.getSieveAccountId() — the account that runs the rules. migrateKeyword is bound to the mail primary. RFC 8620 permits those to be different accounts; where they are, a rule still only tags mail arriving in its own account, so it is the right script to rewrite, and the messages it tags were never in the migration's scope to begin with.

Known limits, left alone deliberately — happy to take any of them on if you would rather they were closed here:

  • renameKeywordInFilters has no server-side precondition, so it can still race a concurrent saveFilters, as any two writers of the same script can today.
  • A rename spans several server calls and cannot be atomic. Every partial outcome is reported to the user rather than hidden, but a partial outcome is still possible: mail the server refused to retag keeps the old keyword while the definition has moved on.
  • An addflag whose argument is a multi-flag list ("$label:red \\Seen", valid per RFC 5232 §3.2) parses to a single action value that no longer equals the tag id, so it is not rewritten. It is counted and warned about rather than silently missed.
  • integration/ is the home for multi-account and synchronisation behaviour, and this touches which account a Sieve write goes to. I covered that in unit tests rather than there; say the word and I will add an integration spec for the rename path.
  • This stands on its own, but it overlaps in spirit with fix: keyword patches use an unescaped JSON Pointer, and a failed migration reports as a clean one #853, which makes migrateKeyword report what it actually did. With both in, the two migrateKeyword calls here should report mail the server refused to retag; I will follow up with that once you have taken a view on either.

Renaming a tag changes its id - the id is a slug of the display name -
and with it the `$label:` keyword the tag is stored under. The rename
migrated the keyword on every message but left Sieve rules naming the old
id, in both the generated `addflag` line and the metadata block the visual
editor reads back. The rule then went on tagging new mail under an id no
definition names any more: the tag renders as its raw id in grey and a
search for the renamed tag misses everything that arrived since, which
reads as the filter having quietly stopped working.

`renameKeywordInRules` rewrites every `add_label` action naming the old id,
disabled rules included. An external rule reaches the script through its
verbatim `rawBlock`, so that is rewritten too - but only the operand of an
`addflag`, since a test comparing a header against the same literal is the
rule's own logic rather than the tag it applies.

The store fetches, rewrites and uploads the account's active script on its
own rather than through fetchFilters/saveFilters, so a rename from the tag
settings cannot disturb which account an open filter view is showing; that
view is updated in place when it shows the same account. The write still
runs through the same plugin pipeline a save does, or a plugin's managed
section would be dropped from the script this activates. A script that is
not the active one is left alone entirely: writing to it would activate it,
and a tag rename must not switch someone's filters back on.

Whatever still names the old tag in the script that ends up being written -
a rule kept verbatim, a test comparing against the keyword, a section a
plugin just grafted back on - is counted, and the tag settings warn that
some rules still use the old tag, because only the user can fix those.
Mail delivered between the message migration and the rewrite was still
tagged by the old rule, so a successful rewrite is followed by one more
migration sweep, which no rewritten rule can race any more.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant