From 8f92ec012cf119acb6bbe0b1d55f22eb2e69ed7a Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 7 May 2026 13:36:57 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=A7=AD=20fix:=20Navigate=20Signed=20CDN?= =?UTF-8?q?=20Downloads=20(#12998)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(files): navigate signed CDN downloads * fix(files): avoid popup target for signed downloads * test(files): restore download URL mock --- api/server/services/Files/Code/process.js | 3 + .../services/Files/Code/process.spec.js | 69 +++++++++++- .../Messages/Content/FilePreviewDialog.tsx | 12 +- .../Messages/Content/MarkdownComponents.tsx | 12 +- .../Chat/Messages/Content/Parts/LogLink.tsx | 16 +-- .../Content/Parts/__tests__/LogLink.test.tsx | 104 ++++++++++++++++++ client/src/components/Web/Sources.tsx | 12 +- .../src/utils/__tests__/downloadFile.test.ts | 71 ++++++++++++ client/src/utils/downloadFile.ts | 15 +++ client/src/utils/index.ts | 1 + 10 files changed, 279 insertions(+), 36 deletions(-) create mode 100644 client/src/components/Chat/Messages/Content/Parts/__tests__/LogLink.test.tsx create mode 100644 client/src/utils/__tests__/downloadFile.test.ts create mode 100644 client/src/utils/downloadFile.ts diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 016d8815c1..a94981f721 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -621,6 +621,9 @@ const processCodeOutput = async ({ message: 'Error downloading/processing code environment file', error, }); + logger.warn( + `[processCodeOutput] Falling back to Code API download URL for strategy ${appConfig.fileStrategy}`, + ); // Fallback for download errors - return download URL so user can still manually download return { diff --git a/api/server/services/Files/Code/process.spec.js b/api/server/services/Files/Code/process.spec.js index 93b76abffd..e816cde3fb 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -138,7 +138,7 @@ const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const { convertImage } = require('~/server/services/Files/images/convert'); const { determineFileType } = require('~/server/utils'); const { logger } = require('@librechat/data-schemas'); -const { codeServerHttpAgent, codeServerHttpsAgent } = require('@librechat/api'); +const { codeServerHttpAgent, codeServerHttpsAgent, getStorageMetadata } = require('@librechat/api'); const { processCodeOutput, getSessionInfo, readSandboxFile, primeFiles } = require('./process'); @@ -320,6 +320,70 @@ describe('Code Process', () => { expect(result.bytes).toBe(100); }); + it.each([ + [ + 'slides.pptx', + 'application/vnd.openxmlformats-officedocument.presentationml.presentation', + ], + ['sheet.xlsx', 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'], + [ + 'document.docx', + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + ], + ])('preserves stored metadata for code-generated office file %s', async (name, mime) => { + const cloudfrontReq = { + ...mockReq, + user: { ...mockReq.user, tenantId: 'tenantA' }, + config: { ...mockReq.config, fileStrategy: 'cloudfront' }, + }; + const smallBuffer = Buffer.alloc(100); + const filepath = `https://cdn.example.com/r/us-east-2/t/tenantA/uploads/user-123/mock-uuid-1234__${name}`; + const storageKey = `r/us-east-2/t/tenantA/uploads/user-123/mock-uuid-1234__${name}`; + mockAxios.mockResolvedValue({ data: smallBuffer }); + determineFileType.mockResolvedValue({ mime }); + mockHasOfficeHtmlPath.mockReturnValueOnce(true); + getStorageMetadata.mockReturnValueOnce({ storageKey, storageRegion: 'us-east-2' }); + const mockSaveBuffer = jest.fn().mockResolvedValue(filepath); + getStrategyFunctions.mockReturnValue({ saveBuffer: mockSaveBuffer }); + + const { file: result, finalize } = await processCodeOutput({ + ...baseParams, + req: cloudfrontReq, + name, + }); + + expect(mockSaveBuffer).toHaveBeenCalledWith( + expect.objectContaining({ + userId: 'user-123', + basePath: 'uploads', + tenantId: 'tenantA', + }), + ); + expect(result).toMatchObject({ + file_id: 'mock-uuid-1234', + user: 'user-123', + tenantId: 'tenantA', + source: 'cloudfront', + filename: name, + filepath, + storageKey, + storageRegion: 'us-east-2', + status: 'pending', + }); + expect(createFile).toHaveBeenCalledWith( + expect.objectContaining({ + file_id: 'mock-uuid-1234', + user: 'user-123', + tenantId: 'tenantA', + source: 'cloudfront', + storageKey, + storageRegion: 'us-east-2', + }), + true, + ); + expect(typeof finalize).toBe('function'); + }); + it('passes and persists tenantId for non-image code output records', async () => { const tenantReq = { ...mockReq, user: { ...mockReq.user, tenantId: 'tenantA' } }; const smallBuffer = Buffer.alloc(100); @@ -595,6 +659,9 @@ describe('Code Process', () => { const { file: result } = await processCodeOutput(baseParams); + expect(logger.warn).toHaveBeenCalledWith( + expect.stringContaining('Falling back to Code API download URL for strategy local'), + ); expect(result.filepath).toContain('/api/files/code/download/session-123/file-id-123'); expect(result.conversationId).toBe('conv-123'); expect(result.messageId).toBe('msg-123'); diff --git a/client/src/components/Chat/Messages/Content/FilePreviewDialog.tsx b/client/src/components/Chat/Messages/Content/FilePreviewDialog.tsx index 43e0abd7d6..45881749f7 100644 --- a/client/src/components/Chat/Messages/Content/FilePreviewDialog.tsx +++ b/client/src/components/Chat/Messages/Content/FilePreviewDialog.tsx @@ -4,8 +4,8 @@ import { useRecoilValue } from 'recoil'; import { Download } from 'lucide-react'; import { OGDialog, OGDialogContent, OGDialogTitle, OGDialogDescription } from '@librechat/client'; import CopyButton from '~/components/Messages/Content/CopyButton'; -import { logger, sortPagesByRelevance } from '~/utils'; -import { revokeDownloadURL, useFileDownload } from '~/data-provider'; +import { logger, sortPagesByRelevance, triggerDownload } from '~/utils'; +import { useFileDownload } from '~/data-provider'; import { useLocalize } from '~/hooks'; import store from '~/store'; @@ -201,13 +201,7 @@ export default function FilePreviewDialog({ if (!result.data) { return; } - const a = document.createElement('a'); - a.href = result.data; - a.setAttribute('download', fileName); - document.body.appendChild(a); - a.click(); - document.body.removeChild(a); - setTimeout(() => revokeDownloadURL(result.data), 1000); + triggerDownload(result.data, fileName); } catch (err) { logger.error('[FilePreviewDialog] Download failed:', err); } diff --git a/client/src/components/Chat/Messages/Content/MarkdownComponents.tsx b/client/src/components/Chat/Messages/Content/MarkdownComponents.tsx index e98b648013..c38c40f1e1 100644 --- a/client/src/components/Chat/Messages/Content/MarkdownComponents.tsx +++ b/client/src/components/Chat/Messages/Content/MarkdownComponents.tsx @@ -5,9 +5,9 @@ import { PermissionTypes, Permissions, apiBaseUrl } from 'librechat-data-provide import Mermaid, { MermaidErrorBoundary } from '~/components/Messages/Content/Mermaid'; import CodeBlock from '~/components/Messages/Content/CodeBlock'; import useHasAccess from '~/hooks/Roles/useHasAccess'; -import { revokeDownloadURL, useFileDownload } from '~/data-provider'; +import { useFileDownload } from '~/data-provider'; import { useCodeBlockContext } from '~/Providers'; -import { handleDoubleClick } from '~/utils'; +import { handleDoubleClick, triggerDownload } from '~/utils'; import { useLocalize } from '~/hooks'; import store from '~/store'; @@ -150,13 +150,7 @@ export const a: React.ElementType = memo(function MarkdownAnchor({ href, childre }); return; } - const link = document.createElement('a'); - link.href = stream.data; - link.setAttribute('download', filename); - document.body.appendChild(link); - link.click(); - document.body.removeChild(link); - revokeDownloadURL(stream.data); + triggerDownload(stream.data, filename); } catch (error) { console.error('Error downloading file:', error); } diff --git a/client/src/components/Chat/Messages/Content/Parts/LogLink.tsx b/client/src/components/Chat/Messages/Content/Parts/LogLink.tsx index d45e3d38d4..50716f05c8 100644 --- a/client/src/components/Chat/Messages/Content/Parts/LogLink.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/LogLink.tsx @@ -1,7 +1,8 @@ import React from 'react'; import { FileSources } from 'librechat-data-provider'; import { useToastContext } from '@librechat/client'; -import { revokeDownloadURL, useCodeOutputDownload, useFileDownload } from '~/data-provider'; +import { useCodeOutputDownload, useFileDownload } from '~/data-provider'; +import { isHttpDownloadTarget, triggerDownload } from '~/utils'; interface LogLinkProps { href: string; @@ -54,6 +55,11 @@ export const useAttachmentLink = ({ const handleDownload = async (event: React.MouseEvent) => { event.preventDefault(); try { + if (!useLocalDownload && isHttpDownloadTarget(href)) { + triggerDownload(href, filename); + return; + } + const stream = useLocalDownload ? await downloadFromApi() : await downloadFromUrl(); if (stream.data == null || stream.data === '') { console.error('Error downloading file: No data found'); @@ -63,13 +69,7 @@ export const useAttachmentLink = ({ }); return; } - const link = document.createElement('a'); - link.href = stream.data; - link.setAttribute('download', filename); - document.body.appendChild(link); - link.click(); - document.body.removeChild(link); - revokeDownloadURL(stream.data); + triggerDownload(stream.data, filename); } catch (error) { console.error('Error downloading file:', error); } diff --git a/client/src/components/Chat/Messages/Content/Parts/__tests__/LogLink.test.tsx b/client/src/components/Chat/Messages/Content/Parts/__tests__/LogLink.test.tsx new file mode 100644 index 0000000000..6b408acb33 --- /dev/null +++ b/client/src/components/Chat/Messages/Content/Parts/__tests__/LogLink.test.tsx @@ -0,0 +1,104 @@ +import React from 'react'; +import { render, screen, fireEvent, waitFor } from '@testing-library/react'; +import { FileSources } from 'librechat-data-provider'; +import LogLink from '../LogLink'; + +const mockShowToast = jest.fn(); +const mockDownloadFromApi = jest.fn(); +const mockDownloadFromUrl = jest.fn(); +const mockTriggerDownload = jest.fn(); + +jest.mock('@librechat/client', () => ({ + useToastContext: () => ({ showToast: mockShowToast }), +})); + +jest.mock('~/data-provider', () => ({ + useFileDownload: () => ({ refetch: mockDownloadFromApi }), + useCodeOutputDownload: () => ({ refetch: mockDownloadFromUrl }), +})); + +jest.mock('~/utils', () => ({ + isHttpDownloadTarget: (target?: string | null) => /^https?:\/\//i.test(target ?? ''), + triggerDownload: (...args: Parameters) => + mockTriggerDownload(...args), +})); + +describe('LogLink download routing', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('navigates directly to http URLs when no stored file metadata is available', async () => { + const filename = 'report.pptx'; + render( + + {filename} + , + ); + + fireEvent.click(screen.getByRole('link', { name: filename })); + + await waitFor(() => { + expect(mockTriggerDownload).toHaveBeenCalledWith( + 'https://cdn.example.com/uploads/report.pptx', + 'report.pptx', + ); + }); + expect(mockDownloadFromUrl).not.toHaveBeenCalled(); + expect(mockDownloadFromApi).not.toHaveBeenCalled(); + }); + + it('uses the authorized file download route when stored metadata is available', async () => { + const filename = 'file.pdf'; + mockDownloadFromApi.mockResolvedValue({ data: 'https://cdn.example.com/signed/file.pdf' }); + + render( + + {filename} + , + ); + + fireEvent.click(screen.getByRole('link', { name: filename })); + + await waitFor(() => { + expect(mockTriggerDownload).toHaveBeenCalledWith( + 'https://cdn.example.com/signed/file.pdf', + 'file.pdf', + ); + }); + expect(mockDownloadFromApi).toHaveBeenCalledTimes(1); + expect(mockDownloadFromUrl).not.toHaveBeenCalled(); + }); + + it('keeps legacy code-output handles on the blob download path', async () => { + const filename = 'legacy.txt'; + mockDownloadFromUrl.mockResolvedValue({ data: 'blob:https://app.example.com/file' }); + + render( + + {filename} + , + ); + + fireEvent.click(screen.getByRole('link', { name: filename })); + + await waitFor(() => { + expect(mockTriggerDownload).toHaveBeenCalledWith( + 'blob:https://app.example.com/file', + 'legacy.txt', + ); + }); + expect(mockDownloadFromUrl).toHaveBeenCalledTimes(1); + expect(mockDownloadFromApi).not.toHaveBeenCalled(); + }); +}); diff --git a/client/src/components/Web/Sources.tsx b/client/src/components/Web/Sources.tsx index 06f8a576b0..81467e4b2f 100644 --- a/client/src/components/Web/Sources.tsx +++ b/client/src/components/Web/Sources.tsx @@ -16,10 +16,10 @@ import { import type { ValidSource, ImageResult } from 'librechat-data-provider'; import { FaviconImage, getCleanDomain } from '~/components/Web/SourceHovercard'; import SourcesErrorBoundary from './SourcesErrorBoundary'; -import { revokeDownloadURL, useFileDownload } from '~/data-provider'; +import { useFileDownload } from '~/data-provider'; import { useSearchContext } from '~/Providers'; import { useLocalize } from '~/hooks'; -import { cn } from '~/utils'; +import { cn, triggerDownload } from '~/utils'; import store from '~/store'; interface SourceItemProps { @@ -257,13 +257,7 @@ const FileItem = React.memo(function FileItem({ }); return; } - const link = document.createElement('a'); - link.href = stream.data; - link.setAttribute('download', file.filename); - document.body.appendChild(link); - link.click(); - document.body.removeChild(link); - revokeDownloadURL(stream.data); + triggerDownload(stream.data, file.filename); } catch (error) { console.error('Error downloading file:', error); } diff --git a/client/src/utils/__tests__/downloadFile.test.ts b/client/src/utils/__tests__/downloadFile.test.ts new file mode 100644 index 0000000000..9b80b2c424 --- /dev/null +++ b/client/src/utils/__tests__/downloadFile.test.ts @@ -0,0 +1,71 @@ +import { isHttpDownloadTarget, triggerDownload } from '../downloadFile'; + +describe('downloadFile utilities', () => { + let clickSpy: jest.SpyInstance; + let appendSpy: jest.SpyInstance; + let removeSpy: jest.SpyInstance; + let revokeSpy: jest.Mock; + let appendedLink: HTMLAnchorElement | null; + let originalRevokeObjectURL: typeof URL.revokeObjectURL | undefined; + + beforeEach(() => { + jest.useFakeTimers(); + appendedLink = null; + originalRevokeObjectURL = URL.revokeObjectURL; + clickSpy = jest.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(); + appendSpy = jest.spyOn(document.body, 'appendChild').mockImplementation((node: Node) => { + appendedLink = node as HTMLAnchorElement; + return node; + }); + removeSpy = jest.spyOn(document.body, 'removeChild').mockImplementation((node: Node) => node); + revokeSpy = jest.fn(); + Object.defineProperty(URL, 'revokeObjectURL', { + configurable: true, + value: revokeSpy, + }); + }); + + afterEach(() => { + clickSpy.mockRestore(); + appendSpy.mockRestore(); + removeSpy.mockRestore(); + Object.defineProperty(URL, 'revokeObjectURL', { + configurable: true, + value: originalRevokeObjectURL, + }); + jest.useRealTimers(); + }); + + it('detects absolute http download targets', () => { + expect(isHttpDownloadTarget('https://cdn.example.com/file.pdf')).toBe(true); + expect(isHttpDownloadTarget('http://cdn.example.com/file.pdf')).toBe(true); + expect(isHttpDownloadTarget('blob:https://app.example.com/id')).toBe(false); + expect(isHttpDownloadTarget('/api/files/code/download/session/file')).toBe(false); + expect(isHttpDownloadTarget(undefined)).toBe(false); + }); + + it('navigates http URLs in the same tab without revoking them', () => { + triggerDownload('https://cdn.example.com/file.pdf?Policy=abc', 'file.pdf'); + + expect(clickSpy).toHaveBeenCalledTimes(1); + expect(appendSpy).toHaveBeenCalledTimes(1); + expect(removeSpy).toHaveBeenCalledTimes(1); + expect(appendedLink?.href).toBe('https://cdn.example.com/file.pdf?Policy=abc'); + expect(appendedLink?.download).toBe('file.pdf'); + expect(appendedLink?.target).toBe(''); + expect(appendedLink?.rel).toBe(''); + jest.runOnlyPendingTimers(); + expect(revokeSpy).not.toHaveBeenCalled(); + }); + + it('revokes blob URLs after the click', () => { + triggerDownload('blob:https://app.example.com/download-id', 'file.pdf'); + + expect(clickSpy).toHaveBeenCalledTimes(1); + expect(appendedLink?.target).toBe(''); + expect(appendedLink?.rel).toBe(''); + expect(revokeSpy).not.toHaveBeenCalled(); + jest.advanceTimersByTime(1000); + expect(revokeSpy).toHaveBeenCalledWith('blob:https://app.example.com/download-id'); + }); +}); diff --git a/client/src/utils/downloadFile.ts b/client/src/utils/downloadFile.ts new file mode 100644 index 0000000000..3ee164fccd --- /dev/null +++ b/client/src/utils/downloadFile.ts @@ -0,0 +1,15 @@ +export const isHttpDownloadTarget = (target?: string | null): boolean => + /^https?:\/\//i.test(target ?? ''); + +export function triggerDownload(target: string, filename: string): void { + const isBlob = target.startsWith('blob:'); + const link = document.createElement('a'); + link.href = target; + link.setAttribute('download', filename); + document.body.appendChild(link); + link.click(); + document.body.removeChild(link); + if (isBlob) { + setTimeout(() => URL.revokeObjectURL(target), 1000); + } +} diff --git a/client/src/utils/index.ts b/client/src/utils/index.ts index 1918ca16d8..643cb76183 100644 --- a/client/src/utils/index.ts +++ b/client/src/utils/index.ts @@ -24,6 +24,7 @@ export * from './redirect'; export * from './languages'; export * from './endpoints'; export * from './resources'; +export * from './downloadFile'; export * from './scaleImage'; export * from './timestamps'; export * from './localStorage';