fix(attachments): download/view attachments on cross-account All-Mail messages
Blobs are scoped per JMAP account, but the attachment download/preview path always used the active account's client and accountId. Opening a message from a different account in the unified / All-Mail view and downloading (or previewing) an attachment therefore 404'd against the active account. Route the blob fetch to the message's source instead: - resolveBlobSource() picks the owning login's client (getClientForAccount(sourceClientAccountId)) and the owner accountId (sourceAccountId) for delegated/shared blobs, in the unified view; - handleDownloadAttachment + the attachment-preview handlers use it; - downloadBlob / fetchBlobAsObjectUrl / fetchBlobArrayBuffer gain an accountId param (getBlobDownloadUrl/fetchBlob already had one). Adds 10-attachments: an attachment on another account's All-Mail message downloads with the correct bytes (verified to fail without the routing).
This commit is contained in:
@@ -110,7 +110,7 @@ export default function Home() {
|
|||||||
const [conversationEmails, setConversationEmails] = useState<Email[]>([]);
|
const [conversationEmails, setConversationEmails] = useState<Email[]>([]);
|
||||||
const [isLoadingConversation, setIsLoadingConversation] = useState(false);
|
const [isLoadingConversation, setIsLoadingConversation] = useState(false);
|
||||||
const [rateLimitSecondsLeft, setRateLimitSecondsLeft] = useState<number | null>(null);
|
const [rateLimitSecondsLeft, setRateLimitSecondsLeft] = useState<number | null>(null);
|
||||||
const [previewAttachment, setPreviewAttachment] = useState<{ blobId: string; name: string; type?: string } | null>(null);
|
const [previewAttachment, setPreviewAttachment] = useState<{ blobId: string; name: string; type?: string; accountId?: string; clientAccountId?: string } | null>(null);
|
||||||
const [pendingMailtoAccountChoice, setPendingMailtoAccountChoice] = useState<ParsedMailto | null>(null);
|
const [pendingMailtoAccountChoice, setPendingMailtoAccountChoice] = useState<ParsedMailto | null>(null);
|
||||||
const [isProtocolAccountSwitching, setIsProtocolAccountSwitching] = useState(false);
|
const [isProtocolAccountSwitching, setIsProtocolAccountSwitching] = useState(false);
|
||||||
const markAsReadTimeoutRef = useRef<NodeJS.Timeout | null>(null);
|
const markAsReadTimeoutRef = useRef<NodeJS.Timeout | null>(null);
|
||||||
@@ -2292,41 +2292,64 @@ export default function Home() {
|
|||||||
};
|
};
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
|
// Blobs are scoped per JMAP account. In the unified/All-Mail view the open
|
||||||
|
// message may belong to another login (route to its client) or to a delegated
|
||||||
|
// shared account (same client, but the owner's accountId in the download URL).
|
||||||
|
// Resolve both from the email's source so attachments on cross-account
|
||||||
|
// messages can be viewed/downloaded instead of 404ing against the active
|
||||||
|
// account.
|
||||||
|
const resolveBlobSource = useCallback((email: typeof selectedEmail) => {
|
||||||
|
const clientAccountId = isUnifiedView ? email?.sourceClientAccountId : undefined;
|
||||||
|
const blobClient = clientAccountId
|
||||||
|
? (useAuthStore.getState().getClientForAccount(clientAccountId) ?? client)
|
||||||
|
: client;
|
||||||
|
const accountId = isUnifiedView ? email?.sourceAccountId : undefined;
|
||||||
|
return { blobClient, accountId, clientAccountId };
|
||||||
|
}, [isUnifiedView, client]);
|
||||||
|
|
||||||
const handleDownloadAttachment = async (blobId: string, name: string, type?: string, forceDownload?: boolean) => {
|
const handleDownloadAttachment = async (blobId: string, name: string, type?: string, forceDownload?: boolean) => {
|
||||||
if (!client) return;
|
const { blobClient, accountId, clientAccountId } = resolveBlobSource(selectedEmail);
|
||||||
|
if (!blobClient) return;
|
||||||
|
|
||||||
try {
|
try {
|
||||||
const { mailAttachmentAction } = useSettingsStore.getState();
|
const { mailAttachmentAction } = useSettingsStore.getState();
|
||||||
|
|
||||||
if (!forceDownload && mailAttachmentAction === 'preview' && isFilePreviewable(name, type)) {
|
if (!forceDownload && mailAttachmentAction === 'preview' && isFilePreviewable(name, type)) {
|
||||||
setPreviewAttachment({ blobId, name, type });
|
setPreviewAttachment({ blobId, name, type, accountId, clientAccountId });
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
await client.downloadBlob(blobId, name, type);
|
await blobClient.downloadBlob(blobId, name, type, accountId);
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
console.error("Failed to download attachment:", error);
|
console.error("Failed to download attachment:", error);
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
const handlePreviewAttachmentDownload = useCallback(async () => {
|
const previewBlobClient = useCallback(() => {
|
||||||
if (!client || !previewAttachment) return;
|
const id = previewAttachment?.clientAccountId;
|
||||||
|
return id ? (useAuthStore.getState().getClientForAccount(id) ?? client) : client;
|
||||||
|
}, [previewAttachment, client]);
|
||||||
|
|
||||||
await client.downloadBlob(previewAttachment.blobId, previewAttachment.name, previewAttachment.type);
|
const handlePreviewAttachmentDownload = useCallback(async () => {
|
||||||
}, [client, previewAttachment]);
|
const c = previewBlobClient();
|
||||||
|
if (!c || !previewAttachment) return;
|
||||||
|
|
||||||
|
await c.downloadBlob(previewAttachment.blobId, previewAttachment.name, previewAttachment.type, previewAttachment.accountId);
|
||||||
|
}, [previewBlobClient, previewAttachment]);
|
||||||
|
|
||||||
const getPreviewAttachmentContent = useCallback(async () => {
|
const getPreviewAttachmentContent = useCallback(async () => {
|
||||||
if (!client || !previewAttachment) {
|
const c = previewBlobClient();
|
||||||
|
if (!c || !previewAttachment) {
|
||||||
throw new Error('No attachment selected');
|
throw new Error('No attachment selected');
|
||||||
}
|
}
|
||||||
|
|
||||||
const blob = await client.fetchBlob(previewAttachment.blobId, previewAttachment.name, previewAttachment.type);
|
const blob = await c.fetchBlob(previewAttachment.blobId, previewAttachment.name, previewAttachment.type, previewAttachment.accountId);
|
||||||
|
|
||||||
return {
|
return {
|
||||||
blob,
|
blob,
|
||||||
contentType: previewAttachment.type || blob.type || 'application/octet-stream',
|
contentType: previewAttachment.type || blob.type || 'application/octet-stream',
|
||||||
};
|
};
|
||||||
}, [client, previewAttachment]);
|
}, [previewBlobClient, previewAttachment]);
|
||||||
|
|
||||||
const handleQuickReply = async (body: string) => {
|
const handleQuickReply = async (body: string) => {
|
||||||
if (!client || !selectedEmail) return;
|
if (!client || !selectedEmail) return;
|
||||||
|
|||||||
@@ -3704,6 +3704,8 @@ export function EmailViewer({
|
|||||||
)}
|
)}
|
||||||
title={`${opensPreview ? tFiles('preview') : t('download')} ${getAttachmentDisplayName(attachment.name, attachment.type)}`}
|
title={`${opensPreview ? tFiles('preview') : t('download')} ${getAttachmentDisplayName(attachment.name, attachment.type)}`}
|
||||||
onClick={() => handleEffectiveAttachmentOpen(attachment)}
|
onClick={() => handleEffectiveAttachmentOpen(attachment)}
|
||||||
|
data-testid="attachment"
|
||||||
|
data-attachment-name={attachment.name}
|
||||||
draggable={dragProps.draggable}
|
draggable={dragProps.draggable}
|
||||||
onPointerEnter={dragProps.onPointerEnter}
|
onPointerEnter={dragProps.onPointerEnter}
|
||||||
onDragStart={dragProps.onDragStart}
|
onDragStart={dragProps.onDragStart}
|
||||||
@@ -4478,6 +4480,8 @@ export function EmailViewer({
|
|||||||
)}
|
)}
|
||||||
title={`${opensPreview ? tFiles('preview') : t('download')} ${getAttachmentDisplayName(attachment.name, attachment.type)}`}
|
title={`${opensPreview ? tFiles('preview') : t('download')} ${getAttachmentDisplayName(attachment.name, attachment.type)}`}
|
||||||
onClick={() => handleEffectiveAttachmentOpen(attachment)}
|
onClick={() => handleEffectiveAttachmentOpen(attachment)}
|
||||||
|
data-testid="attachment"
|
||||||
|
data-attachment-name={attachment.name}
|
||||||
draggable={dragProps.draggable}
|
draggable={dragProps.draggable}
|
||||||
onPointerEnter={dragProps.onPointerEnter}
|
onPointerEnter={dragProps.onPointerEnter}
|
||||||
onDragStart={dragProps.onDragStart}
|
onDragStart={dragProps.onDragStart}
|
||||||
@@ -4622,6 +4626,8 @@ export function EmailViewer({
|
|||||||
)}
|
)}
|
||||||
title={`${opensPreview ? tFiles('preview') : t('download')} ${getAttachmentDisplayName(attachment.name, attachment.type)}`}
|
title={`${opensPreview ? tFiles('preview') : t('download')} ${getAttachmentDisplayName(attachment.name, attachment.type)}`}
|
||||||
onClick={() => handleEffectiveAttachmentOpen(attachment)}
|
onClick={() => handleEffectiveAttachmentOpen(attachment)}
|
||||||
|
data-testid="attachment"
|
||||||
|
data-attachment-name={attachment.name}
|
||||||
draggable={dragProps.draggable}
|
draggable={dragProps.draggable}
|
||||||
onPointerEnter={dragProps.onPointerEnter}
|
onPointerEnter={dragProps.onPointerEnter}
|
||||||
onDragStart={dragProps.onDragStart}
|
onDragStart={dragProps.onDragStart}
|
||||||
|
|||||||
@@ -85,7 +85,9 @@ integration/
|
|||||||
├── 05-actions.spec.ts # context-menu read/unread, delete, spam (inbox)
|
├── 05-actions.spec.ts # context-menu read/unread, delete, spam (inbox)
|
||||||
├── 06-shared-folders.spec.ts # delegated folder: appears + read/unread/delete/spam
|
├── 06-shared-folders.spec.ts # delegated folder: appears + read/unread/delete/spam
|
||||||
├── 07-drafts.spec.ts # multiple recipients, changed sender, continue-draft button
|
├── 07-drafts.spec.ts # multiple recipients, changed sender, continue-draft button
|
||||||
└── 08-shared-moves.spec.ts # moving mail across own/shared and shared/shared
|
├── 08-shared-moves.spec.ts # moving mail across own/shared and shared/shared
|
||||||
|
├── 09-live-counters.spec.ts # live unified/All-Mail counters (login + shared)
|
||||||
|
└── 10-attachments.spec.ts # cross-account attachment download from All Mail
|
||||||
```
|
```
|
||||||
|
|
||||||
## Findings surfaced by the suite
|
## Findings surfaced by the suite
|
||||||
@@ -110,6 +112,10 @@ because the UI behaviour is currently incomplete. Worth a look:
|
|||||||
"Move to" submenu offers the shared folder, but clicking it is a no-op.
|
"Move to" submenu offers the shared folder, but clicking it is a no-op.
|
||||||
Shared ⇆ shared (same owner) moves work. Pinned with `test.fail` in
|
Shared ⇆ shared (same owner) moves work. Pinned with `test.fail` in
|
||||||
`08-shared-moves`.
|
`08-shared-moves`.
|
||||||
|
- **Cross-account attachments (fixed).** Blobs are account-scoped, so viewing/
|
||||||
|
downloading an attachment on an All-Mail message from another account 404'd
|
||||||
|
against the active account. The download/preview path now routes to the
|
||||||
|
message's owning client + accountId (`10-attachments`).
|
||||||
|
|
||||||
## How the tests work
|
## How the tests work
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,77 @@
|
|||||||
|
import { test, expect } from '@playwright/test';
|
||||||
|
import { ACCOUNTS } from './helpers/config';
|
||||||
|
import { sendMail } from './helpers/smtp';
|
||||||
|
import { JmapClient } from './helpers/jmap';
|
||||||
|
import {
|
||||||
|
login,
|
||||||
|
addAccount,
|
||||||
|
switchAccount,
|
||||||
|
seedSettings,
|
||||||
|
folderRow,
|
||||||
|
openFolder,
|
||||||
|
emailItem,
|
||||||
|
expectEmailVisible,
|
||||||
|
forceSync,
|
||||||
|
} from './helpers/app';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Attachments on a message that belongs to a *different* account, opened from
|
||||||
|
* the cross-account All-Mail view. Blobs are account-scoped, so downloading one
|
||||||
|
* must route to the owning account's client + accountId — otherwise it 404s
|
||||||
|
* against the active account (the reported bug).
|
||||||
|
*/
|
||||||
|
const { alice, bob } = ACCOUNTS;
|
||||||
|
const ATT = { filename: 'report.bin', contentType: 'application/octet-stream', content: 'hello-attachment-content-12345' };
|
||||||
|
|
||||||
|
test.describe('Cross-account attachments', () => {
|
||||||
|
test.beforeEach(async () => {
|
||||||
|
for (const a of [alice, bob]) {
|
||||||
|
const j = await JmapClient.connect(a.email, a.password);
|
||||||
|
await j.reset();
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('an attachment on another account\'s All-Mail message downloads correctly', async ({ page }) => {
|
||||||
|
const subject = `IT attach ${Date.now()}`;
|
||||||
|
// Deliver a message with an attachment to bob.
|
||||||
|
await sendMail({ from: bob.email, authPass: bob.password, to: bob.email, subject, body: 'see attachment', attachment: ATT });
|
||||||
|
|
||||||
|
// Cross-account All Mail + always download attachments (don't preview).
|
||||||
|
await seedSettings(page, {
|
||||||
|
enableUnifiedMailbox: true,
|
||||||
|
enableCrossAllView: true,
|
||||||
|
unifiedCrossAccount: true,
|
||||||
|
includeGroupInUnified: true,
|
||||||
|
mailAttachmentAction: 'download',
|
||||||
|
});
|
||||||
|
|
||||||
|
// Make alice the active account, with bob added, so bob's message is
|
||||||
|
// genuinely cross-account when opened.
|
||||||
|
await login(page, alice);
|
||||||
|
await addAccount(page, bob);
|
||||||
|
await switchAccount(page, alice.email);
|
||||||
|
await forceSync(page);
|
||||||
|
|
||||||
|
// Open the All-Mail view and bob's message.
|
||||||
|
await expect(folderRow(page, { name: '__cross_all__' }).first()).toBeVisible();
|
||||||
|
await openFolder(page, { name: '__cross_all__' });
|
||||||
|
await forceSync(page);
|
||||||
|
await expectEmailVisible(page, subject);
|
||||||
|
await emailItem(page, subject).first().click();
|
||||||
|
|
||||||
|
// The attachment chip is present; clicking it downloads the blob from bob's
|
||||||
|
// account (pre-fix this 404s against alice and no download fires).
|
||||||
|
const chip = page.locator(`[data-testid="attachment"][data-attachment-name="${ATT.filename}"]`).first();
|
||||||
|
await chip.waitFor({ state: 'visible', timeout: 15000 });
|
||||||
|
|
||||||
|
const [download] = await Promise.all([
|
||||||
|
page.waitForEvent('download', { timeout: 15000 }),
|
||||||
|
chip.click(),
|
||||||
|
]);
|
||||||
|
|
||||||
|
const stream = await download.createReadStream();
|
||||||
|
const chunks: Buffer[] = [];
|
||||||
|
for await (const c of stream) chunks.push(c as Buffer);
|
||||||
|
expect(Buffer.concat(chunks).toString()).toContain(ATT.content);
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -24,6 +24,8 @@ interface SendOptions {
|
|||||||
body: string;
|
body: string;
|
||||||
/** Extra headers (e.g. custom Message-ID / In-Reply-To for threading). */
|
/** Extra headers (e.g. custom Message-ID / In-Reply-To for threading). */
|
||||||
headers?: Record<string, string>;
|
headers?: Record<string, string>;
|
||||||
|
/** Optional single attachment (sent as multipart/mixed, base64). */
|
||||||
|
attachment?: { filename: string; contentType: string; content: string };
|
||||||
}
|
}
|
||||||
|
|
||||||
class SmtpError extends Error {}
|
class SmtpError extends Error {}
|
||||||
@@ -110,14 +112,38 @@ export async function sendMail(opts: SendOptions): Promise<void> {
|
|||||||
From: opts.from,
|
From: opts.from,
|
||||||
To: recipients.join(', '),
|
To: recipients.join(', '),
|
||||||
Subject: opts.subject,
|
Subject: opts.subject,
|
||||||
'Content-Type': 'text/plain; charset=utf-8',
|
|
||||||
...opts.headers,
|
...opts.headers,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
let mime: string;
|
||||||
|
if (opts.attachment) {
|
||||||
|
const boundary = 'itmixed_boundary_0001';
|
||||||
|
headers['MIME-Version'] = '1.0';
|
||||||
|
headers['Content-Type'] = `multipart/mixed; boundary="${boundary}"`;
|
||||||
|
const b64 = Buffer.from(opts.attachment.content).toString('base64').replace(/(.{76})/g, '$1\r\n');
|
||||||
|
mime = [
|
||||||
|
`--${boundary}`,
|
||||||
|
'Content-Type: text/plain; charset=utf-8',
|
||||||
|
'',
|
||||||
|
crlf(opts.body),
|
||||||
|
`--${boundary}`,
|
||||||
|
`Content-Type: ${opts.attachment.contentType}; name="${opts.attachment.filename}"`,
|
||||||
|
`Content-Disposition: attachment; filename="${opts.attachment.filename}"`,
|
||||||
|
'Content-Transfer-Encoding: base64',
|
||||||
|
'',
|
||||||
|
b64,
|
||||||
|
`--${boundary}--`,
|
||||||
|
].join('\r\n');
|
||||||
|
} else {
|
||||||
|
headers['Content-Type'] = 'text/plain; charset=utf-8';
|
||||||
|
mime = crlf(opts.body);
|
||||||
|
}
|
||||||
|
|
||||||
const headerBlock = Object.entries(headers)
|
const headerBlock = Object.entries(headers)
|
||||||
.map(([k, v]) => `${k}: ${v}`)
|
.map(([k, v]) => `${k}: ${v}`)
|
||||||
.join('\r\n');
|
.join('\r\n');
|
||||||
// Dot-stuff any line that begins with '.'
|
// Dot-stuff any line that begins with '.'
|
||||||
const safeBody = crlf(opts.body).replace(/\r\n\./g, '\r\n..');
|
const safeBody = mime.replace(/\r\n\./g, '\r\n..');
|
||||||
send(`${headerBlock}\r\n\r\n${safeBody}\r\n.`);
|
send(`${headerBlock}\r\n\r\n${safeBody}\r\n.`);
|
||||||
await waitReply('250');
|
await waitReply('250');
|
||||||
send('QUIT');
|
send('QUIT');
|
||||||
|
|||||||
@@ -222,9 +222,9 @@ export interface IJMAPClient {
|
|||||||
): Promise<{ blobId: string; size: number; type: string }>;
|
): Promise<{ blobId: string; size: number; type: string }>;
|
||||||
getBlobDownloadUrl(blobId: string, name?: string, type?: string, accountId?: string): string;
|
getBlobDownloadUrl(blobId: string, name?: string, type?: string, accountId?: string): string;
|
||||||
fetchBlob(blobId: string, name?: string, type?: string, accountId?: string): Promise<Blob>;
|
fetchBlob(blobId: string, name?: string, type?: string, accountId?: string): Promise<Blob>;
|
||||||
fetchBlobAsObjectUrl(blobId: string, name?: string, type?: string): Promise<string>;
|
fetchBlobAsObjectUrl(blobId: string, name?: string, type?: string, accountId?: string): Promise<string>;
|
||||||
fetchBlobArrayBuffer(blobId: string, name?: string, type?: string): Promise<ArrayBuffer>;
|
fetchBlobArrayBuffer(blobId: string, name?: string, type?: string, accountId?: string): Promise<ArrayBuffer>;
|
||||||
downloadBlob(blobId: string, name?: string, type?: string): Promise<void>;
|
downloadBlob(blobId: string, name?: string, type?: string, accountId?: string): Promise<void>;
|
||||||
|
|
||||||
// ── Identities ────────────────────────────────────────────────
|
// ── Identities ────────────────────────────────────────────────
|
||||||
getIdentities(): Promise<Identity[]>;
|
getIdentities(): Promise<Identity[]>;
|
||||||
|
|||||||
+6
-6
@@ -3382,8 +3382,8 @@ export class JMAPClient implements IJMAPClient {
|
|||||||
return response.blob();
|
return response.blob();
|
||||||
}
|
}
|
||||||
|
|
||||||
async fetchBlobAsObjectUrl(blobId: string, name?: string, type?: string): Promise<string> {
|
async fetchBlobAsObjectUrl(blobId: string, name?: string, type?: string, accountId?: string): Promise<string> {
|
||||||
const blob = await this.fetchBlob(blobId, name, type);
|
const blob = await this.fetchBlob(blobId, name, type, accountId);
|
||||||
return URL.createObjectURL(blob);
|
return URL.createObjectURL(blob);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -5662,8 +5662,8 @@ export class JMAPClient implements IJMAPClient {
|
|||||||
return created as FileNode;
|
return created as FileNode;
|
||||||
}
|
}
|
||||||
|
|
||||||
async downloadBlob(blobId: string, name?: string, type?: string): Promise<void> {
|
async downloadBlob(blobId: string, name?: string, type?: string, accountId?: string): Promise<void> {
|
||||||
const blob = await this.fetchBlob(blobId, name, type);
|
const blob = await this.fetchBlob(blobId, name, type, accountId);
|
||||||
const blobUrl = URL.createObjectURL(blob);
|
const blobUrl = URL.createObjectURL(blob);
|
||||||
|
|
||||||
const a = document.createElement('a');
|
const a = document.createElement('a');
|
||||||
@@ -6153,8 +6153,8 @@ export class JMAPClient implements IJMAPClient {
|
|||||||
// ── S/MIME raw-email helpers ─────────────────────────────────────
|
// ── S/MIME raw-email helpers ─────────────────────────────────────
|
||||||
|
|
||||||
/** Fetch blob content as an ArrayBuffer (for S/MIME byte processing). */
|
/** Fetch blob content as an ArrayBuffer (for S/MIME byte processing). */
|
||||||
async fetchBlobArrayBuffer(blobId: string, name?: string, type?: string): Promise<ArrayBuffer> {
|
async fetchBlobArrayBuffer(blobId: string, name?: string, type?: string, accountId?: string): Promise<ArrayBuffer> {
|
||||||
const url = this.getBlobDownloadUrl(blobId, name, type);
|
const url = this.getBlobDownloadUrl(blobId, name, type, accountId);
|
||||||
const response = await this.authenticatedFetch(url, {});
|
const response = await this.authenticatedFetch(url, {});
|
||||||
if (!response.ok) {
|
if (!response.ok) {
|
||||||
throw new Error(`Failed to fetch blob: ${response.status}`);
|
throw new Error(`Failed to fetch blob: ${response.status}`);
|
||||||
|
|||||||
Reference in New Issue
Block a user