Skip to content

Fix search, mail handling and bridge errors; propose separate access-control changes - #27

Open
reinhardullrich wants to merge 7 commits into
vitalio-sh:mainfrom
reinhardullrich:main
Open

reinhardullrich wants to merge 7 commits into
vitalio-sh:mainfrom
reinhardullrich:main

Conversation

@reinhardullrich

Copy link
Copy Markdown

Scope and review order

Hi Vitalii, thanks for Thunderbird CLI. We have been using and testing a fork with a real Thunderbird installation and would like to contribute the results back.

This is a mixed contribution, not a bug-fix-only PR. The sections below deliberately separate corrections to existing behavior from new capabilities and intentional, potentially breaking policy changes. Acceptance of the bug fixes does not imply agreement with the access-control design or removal of deletion operations.

Base: vitalio-sh/thunderbird-cli:main at 465613d. Head: reinhardullrich/thunderbird-cli:main at 879d596. The fork uses only main; some commits combine fixes and features, so the sections below are a review map, not a claim that every category is already independently cherry-pickable. In particular, 93178cb and 0b169db mix categories.

Existing upstream work

I checked open PR #26 before submitting this. It already addresses overlapping areas: whole-folder sorting, extension socket takeover, and signing documentation. This PR does not claim those fixes are new or ask you to replace your implementations. Please reconcile those overlaps when reviewing. This branch does not incorporate #26's separate full-text-search, timeout, or MCP tag-write changes.

1. Bug fixes: existing behavior was incorrect

Area Existing problem Correction
Search filtering Tag/size filters ran after limiting, missing matching messages outside the first batch. Apply native query filters before collection; preserve zero-valued size bounds.
Pagination hasMore could misreport truncation, including within a final native page. Check for one additional matching message; abort unfinished lists and validate pagination arguments.
Sorted lists/recent messages Sorting a limited batch could select the wrong messages. Sort the complete matching header set before offset/limit. Overlaps #26.
Threads Native query continuation pages were ignored and query errors could become incomplete successful results. Consume all pages and propagate failures. Subject-based grouping remains heuristic.
Bulk move/tag Filters applied after limiting; already-tagged messages consumed the tag batch. Filter before limiting. Bulk move fails explicitly with older add-ons that cannot confirm filtering support.
Replies and compose fields Reply body replacement lost the native quotation; the CLI HTML flag was not carried through correctly; priority/header options were not wired to the correct native fields. Preserve the genuine native reply and quotation, honor HTML input, and use native priority/custom-header fields. New attachment and identity features are listed separately below.
Message/attachment reading Text/HTML attachments could be mixed into body extraction or missed by attachment listing. Use native attachment metadata and keep attachment content separate from the body.
Download status Empty/attachment-only messages and native errors could be reported as headers-only; attachment presence was always false. Use headersOnly after successful retrieval, query attachment presence, and propagate errors. This is not a guarantee of permanent offline caching and may fetch mail.
Folder counts Counts were read from unsuitable fields or native failures were swallowed as zero. Use getFolderInfo; skip unsupported account-root count requests but traverse children and propagate real-folder failures.
Output caps Caps missed HTML/raw/batch output and CLI body-only strings. Apply the relevant limits consistently.
CLI/MCP validation and errors Some invalid IDs/flags were accepted, zero values were lost, and error payloads could be presented as success. Validate inputs, preserve zero values, reject HTTP-200 error payloads, and mark MCP handler failures as tool errors.
Bridge transport UTF-8 characters could be split across chunks; interrupted uploads/disconnects were poorly handled; another socket could replace the active add-on. Decode collected bytes once, handle interrupted requests, fail pending calls on disconnect, and retain the existing socket. Takeover protection overlaps #26.
Attachment export Untrusted filenames could escape the requested directory; downloads overwrote existing files. Reject unsafe names and use exclusive, private file creation.
XPI output The ZIP writer produced world-writable output. Write explicitly with private permissions.
Unsupported operations/docs sync reported success without syncing; unsupported bridge flags were ignored; skill examples described unavailable arguments/protections. Return explicit failures and align documentation with implemented behavior.

Tests for these paths are in the existing and added suites under test/, especially review-regressions.test.mjs, extension.test.mjs, quick-test.mjs, mcp-test.mjs, and the bridge tests.

2. New feature: installation-wide access control

This is a proposed feature/design choice, not an upstream bug.

  • One JSON policy is validated and embedded into the add-on at build time. The add-on checks it before native API calls, so the same policy covers CLI, optional MCP, and direct bridge requests.
  • Reading and searching are always available. Attachment exports are separately switchable and enabled by default.
  • Draft preparation, sending, move/copy/archive, marking, tagging, tag creation, and folder creation/rename have explicit permissions. Defaults allow downloads and drafts, but disable sending and changes to existing mailbox contents.
  • Unknown configuration keys, non-booleans, and inconsistent combinations fail explicitly. Mixed operations require all relevant grants.
  • The build derives native Thunderbird permissions from the policy. tb access reports the loaded policy; editing a JSON file alone does not alter the running add-on.

See docs/ACCESS-CONTROL.md, extension/src/access-control.js, access.example.json, and test/access-control.test.mjs.

Boundaries: this is not per-user/per-account authorization or data-loss prevention. Raw MIME reads can contain attachment bytes even if dedicated exports are disabled. Thunderbird may autosave drafts. Disabling connector sending does not disable a person's Send button or protect against another program modifying the profile/add-on.

3. Other improvements and additions

  • Repeatable local-file attachments for compose/reply, explicit reply identity/recipient selection, and inspect-compose for checking native reply linkage and attachments.
  • A shared local token authenticates both HTTP and the add-on WebSocket. The generated private XPI carries that token and must not be published. Malformed-handshake rejection is a regression guard for this new token-parsing path, not a claim that the old unauthenticated implementation had that same parser bug.
  • A repeatable local setup script, user-service template, CLI wrapper, and private build options for stable add-on ID/output path/version. MCP remains supported but optional.
  • Calendar-style private add-on versions are documented as an option; npm package versioning is not changed to calendar versioning.
  • Attachment payload validation/size limits and build/service integration tests support these new features. They should not be mistaken for independent pre-existing upstream bugs.

4. Deliberate breaking changes / fork policy

These are intentional changes and require separate agreement. They are not bug fixes.

  • Message/folder deletion and bulk-deletion commands, API handlers, MCP deletion operations, and deletion permissions are removed. There is no switch to re-enable them. Attachment deletion and emptying Trash are not provided. Moving mail to Trash remains possible when the move permission is enabled, via the move API only.
  • Previously available writes are denied by default under the new policy. Existing users need to configure/rebuild the add-on to enable them.
  • Enabling bridge authentication now requires a matching token in the add-on as well as HTTP clients; an old upstream XPI is not a drop-in replacement for that installation.
  • Older bundled XPIs were removed from this fork because they do not implement its policy. Upstream release/distribution decisions should be reviewed separately; this PR is not evidence that the existing signed-release workflow works unchanged after that removal.
  • AGENTS.md includes our fork's main-only workflow. That paragraph is fork-specific, not a request to impose our Git workflow upstream.

Verification and limitations

All ten local suites passed on the submitted code: CLI/mock, review regressions, access control, thread utilities, extension, optional MCP, bridge authentication, bridge security, draft integration, and local transport/build/service tests.

npm test
npm run test:mcp
npm run test:extension
npm run test:bridge-auth
npm run test:bridge-security
npm run test:local

Live checks were performed on Fedora Asahi ARM64 with Thunderbird 153.0.2. The installed private add-on is 2026.9.16.1; installed source matches the tested source. Checks covered account/folder listings, statistics, search/pagination, attachment metadata/download, native quoted reply/new-message preparation with attachments, and loaded policy. Test drafts were unsent; no messages were sent. The latest verification pass was read-only.

No live destructive-operation tests were performed. Compatibility with every supported Thunderbird version, other operating systems, and upstream release publishing is not established by these checks. Sorted queries may scan many headers; search completeness still depends on Thunderbird's available data.

No private tokens, account configuration, mail contents, profile data, or private token-bearing XPIs are included in the submitted diff. Please review the correctness fixes independently from the policy choices above.

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