feat(menus): context-menu attachment download and Save Open Document#2654
feat(menus): context-menu attachment download and Save Open Document#2654kedzierp wants to merge 2 commits into
Conversation
Split out of IsmaelMartinez#2645 as its own PR for a focused security review — this is the most security-sensitive piece (authenticated-session downloads, clipboard writes, hidden helper windows, SharePoint/MCAS URL rewriting). - "Download Attachment & Copy to Clipboard" (links) and "Download File from Clipboard Link" (non-editable contexts): download via the authenticated session, save under Documents/Teams-Downloads with name-dedup, and place a platform-native file reference on the clipboard (NSFilenamesPboardType / text/uri-list). - "Save Open Document to File…": capture the rendered content of the document open in the Teams viewer (Office/SharePoint frame) and write it to a user-picked path (HTML keeps formatting, .txt is plain text). - SharePoint viewer URL -> download.aspx rewriting, MCAS proxy support, and Monaco-viewer text extraction for text files. Security posture: hidden helper windows keep webSecurity, contextIsolation and sandbox enabled; filenames from Content-Disposition / page titles are sanitized via path.basename; M365 host detection uses strict suffix matching; downloads set item.teamsForLinuxExternallyManaged so DownloadManager skips double-handling. macOS menu rendering goes through Menu.setApplicationMenu (it ignores BrowserWindow.setMenu) so the new items appear on the global menu bar. Tests: lint clean; npm run test:unit 292/292. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive attachment download pipeline (app/menus/attachmentDownload.js) and integrates it into the application menus, allowing users to download attachments, copy them to the clipboard, and save open documents. It also adds corresponding unit tests. The review feedback highlights several critical improvement opportunities: streaming generic binary files to disk instead of loading them entirely into memory to prevent OOM crashes, using Electron's native clipboard API on macOS instead of manually constructing XML plists, preventing promise hangs during text extraction and hidden window downloads by handling failures early, and avoiding disruption to the user's text selection in the document viewer.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const arrayBuffer = await response.arrayBuffer(); | ||
| const buffer = Buffer.from(arrayBuffer); | ||
|
|
||
| let targetFilePath; | ||
| if (destinationPath) { | ||
| // The user already chose the exact path in the save dialog (which | ||
| // confirms its own overwrites), so honour it verbatim. | ||
| fs.mkdirSync(path.dirname(destinationPath), { recursive: true }); | ||
| targetFilePath = destinationPath; | ||
| } else { | ||
| const documentsDir = path.join(app.getPath("documents"), "Teams-Downloads"); | ||
| fs.mkdirSync(documentsDir, { recursive: true }); | ||
| // The filename came from a server header or URL — never let it carry | ||
| // path segments out of the target directory. | ||
| filename = sanitizeFilename(filename); | ||
| targetFilePath = uniqueFilePath(documentsDir, filename); | ||
| } | ||
| fs.writeFileSync(targetFilePath, buffer); |
There was a problem hiding this comment.
Downloading generic binary files using response.arrayBuffer() loads the entire file into memory before writing it to disk. For large files, this can cause extremely high memory usage or crash the Electron process with an Out-Of-Memory (OOM) error. It is highly recommended to stream the response body directly to disk using a write stream.
let targetFilePath;
if (destinationPath) {
// The user already chose the exact path in the save dialog (which
// confirms its own overwrites), so honour it verbatim.
fs.mkdirSync(path.dirname(destinationPath), { recursive: true });
targetFilePath = destinationPath;
} else {
const documentsDir = path.join(app.getPath("documents"), "Teams-Downloads");
fs.mkdirSync(documentsDir, { recursive: true });
// The filename came from a server header or URL — never let it carry
// path segments out of the target directory.
filename = sanitizeFilename(filename);
targetFilePath = uniqueFilePath(documentsDir, filename);
}
if (!response.body) {
throw new Error("Response body is empty");
}
const { Readable } = require("node:stream");
const { finished } = require("node:stream/promises");
const fileStream = fs.createWriteStream(targetFilePath);
await finished(Readable.fromWeb(response.body).pipe(fileStream));| if (process.platform === "darwin") { | ||
| const escaped = filePath | ||
| .replaceAll("&", "&") | ||
| .replaceAll("<", "<") | ||
| .replaceAll(">", ">"); | ||
| const plist = | ||
| '<?xml version="1.0" encoding="UTF-8"?>' + | ||
| '<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">' + | ||
| `<plist version="1.0"><array><string>${escaped}</string></array></plist>`; | ||
| clipboard.writeBuffer("NSFilenamesPboardType", Buffer.from(plist)); |
There was a problem hiding this comment.
Manually constructing an XML plist string to write to NSFilenamesPboardType on macOS is error-prone and fragile. Electron natively supports writing file paths to the clipboard using clipboard.write({ filenames: [filePath] }) on macOS and Windows, which is much cleaner and safer.
if (process.platform === "darwin") {
clipboard.write({ filenames: [filePath] });
} else if (process.platform === "linux") {| if (pageInfo && pageInfo.success) { | ||
| let filename = stripViewerTitleSuffix(pageInfo.title || "attachment.txt"); | ||
| // The title is page-controlled — strip any path segments before | ||
| // building the save path. | ||
| filename = sanitizeFilename(filename, "attachment.txt"); | ||
| if (!filename.includes(".")) { | ||
| filename += ".txt"; | ||
| } | ||
|
|
||
| const targetFilePath = destinationPath || uniqueFilePath(documentsDir, filename); | ||
| fs.writeFileSync(targetFilePath, pageInfo.text, "utf8"); | ||
|
|
||
| // Copy the extracted text; a file reference would overwrite it | ||
| // (the clipboard holds one payload at a time). | ||
| clipboard.writeText(pageInfo.text); | ||
|
|
||
| resolved = true; | ||
| cleanup(); | ||
| resolve({ filename: path.basename(targetFilePath), targetFilePath }); | ||
| } |
There was a problem hiding this comment.
If the text extraction loop finishes 40 iterations without finding any text, pageInfo.success remains false. The code currently has no else block to handle this failure, so the promise hangs until the 30-second timeout fires. Rejecting the promise immediately with a descriptive error is much cleaner and avoids unnecessary delay.
if (pageInfo && pageInfo.success) {
let filename = stripViewerTitleSuffix(pageInfo.title || "attachment.txt");
// The title is page-controlled — strip any path segments before
// building the save path.
filename = sanitizeFilename(filename, "attachment.txt");
if (!filename.includes(".")) {
filename += ".txt";
}
const targetFilePath = destinationPath || uniqueFilePath(documentsDir, filename);
fs.writeFileSync(targetFilePath, pageInfo.text, "utf8");
// Copy the extracted text; a file reference would overwrite it
// (the clipboard holds one payload at a time).
clipboard.writeText(pageInfo.text);
resolved = true;
cleanup();
resolve({ filename: path.basename(targetFilePath), targetFilePath });
} else {
throw new Error("No text content could be extracted from the viewer.");
}| const cleanup = () => { | ||
| if (cleanupTimeout) { | ||
| clearTimeout(cleanupTimeout); | ||
| cleanupTimeout = null; | ||
| } | ||
| activeSession.removeListener("will-download", handleWillDownload); | ||
| if (!tempWin.isDestroyed()) { | ||
| tempWin.destroy(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
If the URL loaded in the hidden window is a normal webpage (e.g., an expired link, login page, or error page) instead of a file download, will-download is never triggered. Since there is no listener for page load completion, the promise hangs for 30 seconds until the timeout fires. Listening to the did-finish-load event on tempWin.webContents allows us to fail early if the page finishes loading and downloadInitiated is still false.
const handleDidFinishLoad = () => {
setTimeout(() => {
if (!downloadInitiated && !settled) {
fail(new Error("The URL loaded as a webpage but did not initiate a download."));
}
}, 1000);
};
const cleanup = () => {
if (cleanupTimeout) {
clearTimeout(cleanupTimeout);
cleanupTimeout = null;
}
activeSession.removeListener("will-download", handleWillDownload);
if (!tempWin.isDestroyed()) {
tempWin.webContents.removeListener("did-finish-load", handleDidFinishLoad);
tempWin.destroy();
}
};| } | ||
| }; | ||
|
|
||
| activeSession.on("will-download", handleWillDownload); |
There was a problem hiding this comment.
Register the did-finish-load listener on the hidden window's webContents to handle cases where the page loads successfully but does not trigger a download.
| activeSession.on("will-download", handleWillDownload); | |
| activeSession.on("will-download", handleWillDownload); | |
| tempWin.webContents.on("did-finish-load", handleDidFinishLoad); |
| const capture = `(() => { | ||
| const sel = window.getSelection(); | ||
| const range = document.createRange(); | ||
| range.selectNodeContents(document.body); | ||
| sel.removeAllRanges(); | ||
| sel.addRange(range); | ||
| const holder = document.createElement("div"); | ||
| for (let i = 0; i < sel.rangeCount; i++) holder.appendChild(sel.getRangeAt(i).cloneContents()); | ||
| const html = holder.innerHTML; | ||
| const text = sel.toString(); | ||
| sel.removeAllRanges(); | ||
| return { html, text, title: document.title }; | ||
| })()`; |
There was a problem hiding this comment.
The capture script in saveRenderedDocument modifies the user's text selection in the document viewer frame by calling window.getSelection().selectAllChildren and then clearing it. This is disruptive to the user's experience if they had text selected, and can trigger selection-related event listeners on the page. Using document.body.innerHTML and document.body.innerText directly captures the HTML and plain text of the document without modifying the user's selection.
const capture = `(() => {
return {
html: document.body.innerHTML,
text: document.body.innerText,
title: document.title
};
})()`;- Stream generic-host downloads to disk (Readable.fromWeb(response.body) →
createWriteStream) instead of buffering the whole file via arrayBuffer,
which could OOM the main process on large attachments.
- extractTextFromBrowserWindow: reject as soon as the poll loop exhausts its
attempts without finding text, instead of hanging until the 30s timeout.
- downloadWithBrowserWindow: add a did-finish-load handler that fails fast when
the URL renders as a normal page (expired link / login / error) and no
download starts, instead of waiting out the 30s timeout.
- saveRenderedDocument: capture document.body.innerHTML/innerText directly
rather than driving window.getSelection(), so the user's selection in the
viewer isn't clobbered.
- Tests: stream via response.body in the fake session, match the new capture
script, and use a mkdtemp path instead of a hardcoded /tmp file.
Kept the manual NSFilenamesPboardType / text-uri-list clipboard writes:
clipboard.write({ filenames }) is a silent no-op in this Electron version.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @gemini-code-assist — addressed in 5ba8181: Applied
Not applied (with rationale)
All green: lint clean, |
|
Thanks for the detailed breakdown, @kedzierp. The improvements to the streaming pipeline, the fail-fast logic in the browser window handlers, and the preservation of user selection in |
|
|
Thanks @gemini-code-assist — appreciated, and agreed on the clipboard tradeoff. Nothing further to change here; CI is green. |
|
Thanks for the update, @kedzierp. Glad to hear the CI is green and the clipboard behavior is settled. Since everything is in order, this looks ready to go. Great work on this feature. |
|
@IsmaelMartinez ready for review — this is the security-sensitive split from #2645 you asked to isolate for a focused security review (authenticated-session downloads, clipboard writes, hidden helper windows, SharePoint/MCAS rewriting).
|
|
Thanks @kedzierp, and thanks for isolating the security-sensitive piece. The handling is genuinely careful: locked-down window, path.basename plus a ./.. reject, with a test. One thing I want to check, and I may be missing context from your side. We already have a DownloadManager (app/downloadManager, the one #2652 extends) that owns the will-download handler on the same session and does the save dialog, unique naming, notifications and open-when-done. The navigation-download branch here fires a real will-download on that same session, then sets teamsForLinuxExternallyManaged to opt out and re-implements that tail itself. Was that deliberate, or was the manager just not on your radar? If nothing argues against it, letting DownloadManager own the save/naming/notify tail (this branch adding only the clipboard step) would avoid two parallel paths, and uniqueFilePath plus the M365 host-suffix list could be one shared helper with #2652. The hidden window is fair for the SharePoint/Monaco render-and-extract case, which fetch cannot do, so no concern there. Just flagging ADR-010 (we rejected general multi-window) so we keep new windows to that one path. Keen to hear your thoughts. Thanks again! |
…#2655) * docs(roadmap): note in-review download, macOS, and device-switch work Add Downloads and macOS Desktop Integration themes (and a Media line) for the three PRs split out of #2645: #2652 (download save options + policy-block notification + preventDeviceSwitching), #2653 (Dock overlay + gated perf switches), #2654 (context-menu attachment download). Marked in review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(roadmap): fully-qualify download.* config keys Per bot review: write download.alwaysAskWhereToSave / download.openWhenDone in full, matching download.saveDirectory. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| const host = url.hostname.toLowerCase(); | ||
|
|
||
| // Only target SharePoint and OneDrive domains | ||
| if (host.includes("sharepoint.com") || host.includes("onedrive.com") || host.includes("mcas.ms")) { |
| const host = url.hostname.toLowerCase(); | ||
|
|
||
| // Only target SharePoint and OneDrive domains | ||
| if (host.includes("sharepoint.com") || host.includes("onedrive.com") || host.includes("mcas.ms")) { |
|
|
||
| // Tell DownloadManager (which listens on the same session) to skip | ||
| // its own save path, notifications and openWhenDone for this item. | ||
| item.teamsForLinuxExternallyManaged = true; |
There was a problem hiding this comment.
Setting teamsForLinuxExternallyManaged here opts the item out of DownloadManager, which already owns will-download on this session and already does unique name (1).ext naming, the Save As dialog, the saveDirectory drop, completion notifications and open-when-done. The code below then re-implements the save-path, naming and notification tail. Could the download instead reuse DownloadManager for that tail, so there's one place doing it, with this module adding only the new parts (the entry points, clipboard copy, viewer-URL handling, Save Open Document)? Two paths doing the same download tail slightly differently is the duplication I'd like to resolve before merge, and is also why I lean toward putting the feature behind a config flag.
| } | ||
| menu.append( | ||
| new MenuItem({ | ||
| label: "Save Open Document to File…", |
There was a problem hiding this comment.
This appends "Save Open Document to File…" to every non-editable context menu, and combined with menu.popup() now firing unconditionally (line 568, it was gated on items.length > 0), the app's native context menu now shows on every non-editable right-click in Teams, on all platforms. Since the item is also in the View menu (appMenu.js), this context-menu copy is a permanently-present entry that's usually a no-op (it triggers the "no rendered document" notification) and can shadow Teams' own right-click menus. Could it be gated to when an Office viewer frame is actually open, or dropped from the context menu in favour of the View-menu entry?
| if (isMS) { | ||
| result = await fetchViaHiddenWindow(); | ||
| } else { | ||
| const response = await session.fetch(downloadURL); |
There was a problem hiding this comment.
The clipboard-link path checks the URL is http(s) first, but the two link-context items pass params.linkURL straight through to here, and isMicrosoftHost returns false for file:// (you test that), so a file:///… page link reaches this session.fetch with no scheme check. Chromium's net stack most likely refuses file:// here, so this is defense-in-depth rather than a confirmed read, but mirroring the clipboard path's http(s) guard before the fetch would close it and make the two entry points consistent.
📦 PR Snap Build Artifacts✅ Snap builds successful! Download artifacts: 🐧 Linux Snap Packagesx86_64 (116.13 MB) arm64 (111.51 MB) armv7l (111.56 MB) 📝 Note: Other package formats (.deb, .rpm, .AppImage, .dmg, .exe) are built in the main workflow |
📦 PR Build Artifacts✅ Build successful! Download artifacts: 🐧 Linuxx86_64 (468.32 MB) - Contains: .deb, .rpm, .tar.gz, .AppImage arm64 (452.96 MB) - Contains: .deb, .rpm, .tar.gz, .AppImage armv7l (456.13 MB) - Contains: .deb, .rpm, .tar.gz, .AppImage 🍎 macOSx86_64 (129.80 MB) - Contains: .dmg 🪟 Windowsx86_64 (111.20 MB) - Contains: .exe installer 📝 Note: Snap packages (.snap) are built in a separate workflow 🕐 Last updated: 2026-06-22 09:13 UTC |
3 participants
Third of three PRs splitting #2645. This is the security-sensitive piece the maintainer asked to isolate for a focused security review: authenticated-session downloads, clipboard writes, hidden helper windows, and SharePoint/MCAS URL rewriting.
What's here
Context-menu attachment download
Documents/Teams-Downloadswithname (1).extde-duplication, and place a platform-native file reference on the clipboard (NSFilenamesPboardType/text/uri-list)..txtwrites plain text. This is the "let the app do the select-all/copy and paste into a file" flow.download.aspxrewriting, MCAS proxy support, and Monaco-viewer text extraction for text files.Security posture
webSecurity,contextIsolationandsandboxenabled.Content-Dispositionheaders / page titles are sanitized viapath.basename(no path traversal out of the target dir).item.teamsForLinuxExternallyManagedso the sharedDownloadManagerskips its own save path / notifications and a single download isn't double-handled. (The consuming guard ships in the download-manager PR; harmless no-op until then.)error.messageonly — never the full error, which can embed the download URL.macOS menu rendering
macOS ignores
BrowserWindow.setMenu(), so the menu is applied viaMenu.setApplicationMenu(with standard Edit/View/Window roles) so the new items actually appear on the global menu bar.Testing
npm run lintclean.npm run test:unit— 292/292 (newattachmentDownload.test.js: URL rewriting, host matching incl. lookalikes, filename sanitization/dedup, generic-download end-to-end, andsaveRenderedDocumentframe selection / HTML-vs-txt / no-content-found).Depends on / relates to
teamsForLinuxExternallyManagedopt-out guard) but is independently mergeable.Refs #2645
🤖 Generated with Claude Code