diff --git a/CHANGELOG.md b/CHANGELOG.md index ec8b3a8..1df6b66 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security +- `tb attachment-download --all` now strips directory parts and unsafe characters from attachment names before writing, so a crafted name such as `../../.bashrc` can no longer write outside `--output-dir`. Unnamed attachments are saved as `attachment-`. Idea from DreatNU-oss/thunderbird-cli `651d9f1` (reimplemented, not copied). + ## [1.3.0] — 2026-09-28 Needs add-on 2.4.0 for the calendar, task and Fast Actions commands. Until ATN finishes its manual review of 2.4.0 (required for Experiment APIs), this release attaches the signed 2.1.0 XPI plus an unsigned 2.4.0 build. The email, contacts and notes commands work with either. diff --git a/cli/src/cli.js b/cli/src/cli.js index fa28112..56dead4 100755 --- a/cli/src/cli.js +++ b/cli/src/cli.js @@ -9,7 +9,7 @@ import { createRequire } from "module"; import { Command } from "commander"; -import { api, output, outputError, getConfig, parseRelativeDate } from "./client.js"; +import { api, output, outputError, getConfig, parseRelativeDate, safeFileName } from "./client.js"; import { basename, extname } from "path"; import { listNotes, @@ -1049,7 +1049,7 @@ program const results = []; for (const att of attachments) { const data = await api("POST", `/messages/${messageId}/attachment`, { partName: att.partName }, timeout); - let fileName = data.name || att.name || att.partName; + let fileName = safeFileName(data.name || att.name, `attachment-${att.partName}`); if (!extname(fileName)) { fileName += mimeToExt(data.contentType || att.contentType); } diff --git a/cli/src/client.js b/cli/src/client.js index 16fb5be..3bc536a 100644 --- a/cli/src/client.js +++ b/cli/src/client.js @@ -863,6 +863,18 @@ export function outputError(err, format = "json", opts = {}) { process.exit(1); } +/** + * Turn a sender-controlled attachment name into a bare file name that stays inside the + * output directory: drops any directory part (either separator), control characters and + * characters Windows rejects, and falls back when nothing usable is left ("", ".", ".."). + */ +export function safeFileName(name, fallback = "attachment") { + const base = String(name ?? "").split(/[\\/]/).pop(); + const cleaned = base.replace(/[\x00-\x1f\x7f<>:"|?*]/g, "_").trim(); + if (!cleaned || /^\.+$/.test(cleaned)) return fallback; + return cleaned; +} + /** * Parse relative date strings (7d, 2w, 3m, 1y, today, yesterday) to ISO dates */ diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index fac4b71..7382610 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -229,6 +229,10 @@ tb attachment-download --output # download one tb attachment-download --all --output-dir # download all ``` +With `--all`, each file is named after the attachment with any directory part and +unsafe characters stripped, so a crafted name like `../../.bashrc` lands in `` +(as `.bashrc`). Unnamed attachments are saved as `attachment-`. + ## Fetch & Sync ```bash diff --git a/test/quick-test.mjs b/test/quick-test.mjs index e6348e6..45d92e5 100644 --- a/test/quick-test.mjs +++ b/test/quick-test.mjs @@ -7,6 +7,7 @@ import { createServer } from "http"; import { WebSocketServer, WebSocket } from "ws"; import { randomUUID } from "crypto"; +import { safeFileName } from "../cli/src/client.js"; const PORT = 19700; const WS_PORT = 19701; @@ -319,6 +320,16 @@ await new Promise(r => setTimeout(r, 200)); // ensure the waiter is registered b servers.mock.send(JSON.stringify({ type: "event", name: "test-event", data: { hello: "world" } })); test("GET /bridge/events?wait=test-event (live push)", await liveWait, r => r.event?.name === "test-event" && r.event?.data?.hello === "world"); +console.log("\n\x1b[1mAttachment file names\x1b[0m"); +test("safeFileName keeps a plain name", safeFileName("report.pdf"), r => r === "report.pdf"); +test("safeFileName strips ../ traversal", safeFileName("../../.bashrc"), r => r === ".bashrc"); +test("safeFileName strips absolute paths", safeFileName("/etc/cron.d/job"), r => r === "job"); +test("safeFileName strips Windows separators", safeFileName("..\\..\\evil.exe"), r => r === "evil.exe"); +test("safeFileName replaces control and reserved chars", safeFileName("a\x00b:c?.txt"), r => r === "a_b_c_.txt"); +test("safeFileName falls back on '..'", safeFileName("..", "attachment-1.2"), r => r === "attachment-1.2"); +test("safeFileName falls back on trailing separator", safeFileName("dir/", "attachment-1.2"), r => r === "attachment-1.2"); +test("safeFileName falls back on missing name", safeFileName(undefined, "attachment-1.2"), r => r === "attachment-1.2"); + console.log(`\n\x1b[1m${"─".repeat(40)}\x1b[0m`); console.log(`\x1b[1m${passed} passed, ${failed} failed, ${passed + failed} total\x1b[0m\n`);