fix: skip POSIX permission check on Windows when writing connection info - #199
Open
svnscha wants to merge 1 commit into
Open
fix: skip POSIX permission check on Windows when writing connection info#199svnscha wants to merge 1 commit into
svnscha wants to merge 1 commit into
Conversation
writeConnectionInfo hardens the tmp directory against a shared-/tmp race
by rejecting group/world bits. The guard assumes nsIFile.permissions
returns 0 where POSIX modes are unavailable, so that the check is inert
on Windows.
That assumption does not hold. nsLocalFileWin synthesises a POSIX-looking
mode from the read-only attribute alone -- 0777 for any directory,
regardless of the ACL -- and assigning .permissions is a no-op on NTFS.
So mode is truthy, (mode & 0o077) is non-zero, the write-back cannot
clear it, and the guard throws:
Failed to start MCP server: Error: thunderbird-mcp tmp directory has
group/world permissions -- refusing to write connection info
This fires for the directory the same function just created with 0o700,
so the server never starts and the bridge reports "connection discovery
failed". It recurs on every launch: shutdown removes connection.json but
leaves the directory, so the !exists() branch is only taken once.
Gate the check on Services.appinfo.OS !== "WINNT". Access on Windows is
governed by the ACL inherited from the user's own %TEMP%, which is
user-only, so the hardening has nothing to add there.
Verified on Thunderbird 154.0 / Windows 11: before the change the server
failed to start on every restart unless the tmp directory was deleted by
hand; after it, startup and connection discovery work with the directory
left in place.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The POSIX permission hardening in
writeConnectionInfothrows unconditionally on Windows, so the MCP server cannot start there.Symptom
The add-on stays enabled and shows no error in the Add-ons Manager. Nothing listens on 8765–8774,
connection.jsonis never written, and every MCP client just reports connection discovery failure — so from the outside it looks like the bridge is misconfigured rather than like the server never started.Cause
The guard assumes
nsIFile.permissionsreturns0where POSIX modes are unavailable — that is what the existing comment says, and it is why the check was believed inert on Windows:nsLocalFileWindoesn't do that. It synthesises a POSIX-looking mode from the read-only attribute alone —0777for any directory, regardless of the ACL — and assigning.permissionsis a no-op on NTFS. So:modeis truthy (0777)mode & 0o077is non-zero0o700silently does nothingThe directory that trips this is the one this same function created moments earlier with
0o700. Its ACL is inherited from the user's own%TEMP%—SYSTEM,Administrators, and the user, with noEveryoneentry — so there is nothing actually wrong with it.It also recurs on every launch rather than being a one-time glitch: shutdown removes
connection.jsonbut leaves the directory behind, so the!exists()branch that skips the check is only ever taken once. Deleting%LOCALAPPDATA%\Temp\thunderbird-mcpbefore each start works around it.Fix
Gate the check on
Services.appinfo.OS !== "WINNT", and correct the stale comment. The POSIX hardening is unchanged everywhere it can actually do something — the shared-/tmprace it defends against has no Windows analogue here, since access is governed by the inherited user-only ACL.Verification
Thunderbird 154.0 on Windows 11. Before: the server failed to start on every restart unless the tmp directory was deleted by hand. After: startup and connection discovery work with the directory left in place across restarts.
node --test test/*.cjs→ 500 passing, 0 failures (17 skipped while a real Thunderbird holds the port).eslint .→ 0 errors.No test is included: the surrounding logic reads
Services.dirsvcand livensIFilestate, and it isn't inside one of theBEGIN/ENDmarker blocks that the existing suite extracts for sandboxed testing. Happy to add one if you'd like the block factored out for that.