From 30e5f15dfbda616bf78675e936cf399db4bce0cb Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Tue, 5 May 2026 12:23:26 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=8D=20feat:=20`GET=20/api/files/:file?= =?UTF-8?q?=5Fid/preview`=20endpoint=20and=20boot=20orphan=20sweep?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - New `GET /api/files/:file_id/preview` route returns `{ status, text?, textFormat?, previewError? }`. The frontend's `useFilePreview` React Query hook polls this while phase-2 is in flight, then auto-stops on terminal status. ACL identical to the download route (reuses `fileAccess` middleware). Defaults `status` to `'ready'` for legacy records so back-compat is implicit. `text` only included when `status === 'ready'` and non-null — preserves the HTML-or-null security contract from #12934. - `sweepOrphanedPreviews()` invoked on boot in both `server/index.js` and `server/experimental.js`. Recovers any `pending` records left behind by a process restart mid-extraction (the only case the in-process two-phase flow can't handle on its own). Fire-and-forget so a transient sweep failure doesn't block startup. --- api/server/experimental.js | 14 +- api/server/index.js | 17 +- api/server/routes/files/files.js | 54 ++++++ api/server/routes/files/preview.spec.js | 242 ++++++++++++++++++++++++ 4 files changed, 325 insertions(+), 2 deletions(-) create mode 100644 api/server/routes/files/preview.spec.js diff --git a/api/server/experimental.js b/api/server/experimental.js index cd594c1696..6192aea087 100644 --- a/api/server/experimental.js +++ b/api/server/experimental.js @@ -26,7 +26,12 @@ const initializeOAuthReconnectManager = require('./services/initializeOAuthRecon const createValidateImageRequest = require('./middleware/validateImageRequest'); const { jwtLogin, ldapLogin, passportLogin } = require('~/strategies'); const { updateInterfacePermissions: updateInterfacePerms } = require('@librechat/api'); -const { getRoleByName, updateAccessPermissions, seedDatabase } = require('~/models'); +const { + getRoleByName, + updateAccessPermissions, + seedDatabase, + sweepOrphanedPreviews, +} = require('~/models'); const { checkMigrations } = require('./services/start/migration'); const initializeMCPs = require('./services/initializeMCPs'); const configureSocialLogins = require('./socialLogins'); @@ -220,6 +225,13 @@ if (cluster.isMaster) { /** Seed database (idempotent) */ await seedDatabase(); + /* Recover any code-execution file records left at `status: 'pending'` + * by a prior process restart mid-extraction. Mirror of the path in + * `server/index.js`; see comment there for context. */ + sweepOrphanedPreviews().catch((err) => { + logger.error('[sweepOrphanedPreviews] Background sweep failed:', err); + }); + /** Initialize app configuration */ const appConfig = await getAppConfig(); initializeFileStorage(appConfig); diff --git a/api/server/index.js b/api/server/index.js index d798f1a166..28e4ec7a94 100644 --- a/api/server/index.js +++ b/api/server/index.js @@ -25,7 +25,12 @@ const { } = require('@librechat/api'); const { connectDb, indexSync } = require('~/db'); const initializeOAuthReconnectManager = require('./services/initializeOAuthReconnectManager'); -const { getRoleByName, updateAccessPermissions, seedDatabase } = require('~/models'); +const { + getRoleByName, + updateAccessPermissions, + seedDatabase, + sweepOrphanedPreviews, +} = require('~/models'); const { capabilityContextMiddleware } = require('./middleware/roles/capabilities'); const createValidateImageRequest = require('./middleware/validateImageRequest'); const { jwtLogin, ldapLogin, passportLogin } = require('~/strategies'); @@ -69,6 +74,16 @@ const startServer = async () => { } await runAsSystem(seedDatabase); + /* Recover any code-execution file records left at `status: 'pending'` + * by a prior process restart mid-extraction. The two-phase preview + * flow runs phase-2 in-process; if the backend died before it + * finished, those records would otherwise stay pending forever and + * the frontend would poll indefinitely. Cheap — `status` is indexed + * and the candidate set is bounded by what was in flight at shutdown. + * Fire-and-forget so a transient error here doesn't block boot. */ + sweepOrphanedPreviews().catch((err) => { + logger.error('[sweepOrphanedPreviews] Background sweep failed:', err); + }); const appConfig = await getAppConfig({ baseOnly: true }); initializeFileStorage(appConfig); await runAsSystem(async () => { diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index 5c26f65b81..326e8a0bbc 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -295,6 +295,60 @@ router.get('/code/download/:session_id/:fileId', async (req, res) => { } }); +/** + * Poll the lifecycle status of a code-execution file's inline preview. + * + * Two-phase code-execution flow: phase-1 persists the file blob and + * emits the attachment record at `status: 'pending'`; phase-2 runs + * HTML extraction in the background and transitions the record to + * `'ready'` (with `text` + `textFormat`) or `'failed'` (with + * `previewError`). The frontend's `useFilePreview` React Query hook + * polls this endpoint at ~2.5s intervals while `status === 'pending'` + * and `isSubmitting === true`, then auto-stops on terminal status. + * + * Returns the smallest viable shape: + * - `status` always present (defaults to `'ready'` for legacy records + * that never had the field — clients treat absent as ready). + * - `text` and `textFormat` only when status is 'ready' AND text + * is non-null (preserves the security contract from PR #12934 — + * office bucket files MUST NOT receive plain-text fallbacks). + * - `previewError` only when status is 'failed'. + * + * Reuses the `fileAccess` middleware so ACL is identical to download. + * + * @route GET /files/:file_id/preview + */ +router.get('/:file_id/preview', fileAccess, async (req, res) => { + try { + const { file_id } = req.params; + /* `fileAccess` returns the file via `getFiles` which excludes `text` + * by default. Re-fetch via `findFileById` here so the response can + * include the extracted preview when ready. By-id lookup is indexed, + * so the second hit is cheap. */ + const file = await db.findFileById(file_id); + if (!file) { + return res.status(404).json({ error: 'Not Found', message: 'File not found' }); + } + /* Default to 'ready' for back-compat: legacy records pre-date the + * field, and non-office files never get a status set in phase-1. */ + const status = file.status ?? 'ready'; + const payload = { file_id, status }; + if (status === 'ready' && file.text != null) { + payload.text = file.text; + payload.textFormat = file.textFormat ?? null; + } + if (status === 'failed' && file.previewError) { + payload.previewError = file.previewError; + } + return res.status(200).json(payload); + } catch (error) { + logger.error('[/files/:file_id/preview] Error fetching preview status:', error); + return res + .status(500) + .json({ error: 'Internal Server Error', message: 'Failed to fetch preview status' }); + } +}); + router.get('/download/:userId/:file_id', fileAccess, async (req, res) => { try { const { userId, file_id } = req.params; diff --git a/api/server/routes/files/preview.spec.js b/api/server/routes/files/preview.spec.js new file mode 100644 index 0000000000..8aec71c4ac --- /dev/null +++ b/api/server/routes/files/preview.spec.js @@ -0,0 +1,242 @@ +/** + * Coverage for the new GET /files/:file_id/preview endpoint. + * + * Two-phase code-execution flow: phase-1 emits a file record at + * `status: 'pending'`; phase-2 transitions it to `'ready'` (with text) + * or `'failed'` (with previewError). The frontend polls this endpoint + * until status is terminal. This suite asserts the response shape + * across all four states (pending, ready, failed, legacy/back-compat) + * and the auth boundary (404 vs 403). + */ + +jest.mock('@librechat/data-schemas', () => ({ + logger: { warn: jest.fn(), debug: jest.fn(), error: jest.fn(), info: jest.fn() }, + SystemCapabilities: {}, +})); + +jest.mock('@librechat/api', () => ({ + refreshS3FileUrls: jest.fn(), + resolveUploadErrorMessage: jest.fn(), + verifyAgentUploadPermission: jest.fn(), +})); + +const mockFindFileById = jest.fn(); +const mockGetFiles = jest.fn(); +jest.mock('~/models', () => ({ + findFileById: (...args) => mockFindFileById(...args), + getFiles: (...args) => mockGetFiles(...args), + getAgents: jest.fn().mockResolvedValue([]), + batchUpdateFiles: jest.fn(), +})); + +jest.mock('~/server/services/Files/process', () => ({ + filterFile: jest.fn(), + processFileUpload: jest.fn(), + processDeleteRequest: jest.fn(), + processAgentFileUpload: jest.fn(), +})); + +jest.mock('~/server/services/Files/strategies', () => ({ + getStrategyFunctions: jest.fn(() => ({})), +})); + +jest.mock('~/server/controllers/assistants/helpers', () => ({ + getOpenAIClient: jest.fn(), +})); + +jest.mock('~/server/middleware/roles/capabilities', () => ({ + hasCapability: jest.fn(() => (_req, _res, next) => next()), +})); + +jest.mock('~/server/services/PermissionService', () => ({ + checkPermission: jest.fn(() => (_req, _res, next) => next()), + getEffectivePermissions: jest.fn().mockResolvedValue(0), +})); + +jest.mock('~/server/services/Files', () => ({ + hasAccessToFilesViaAgent: jest.fn(), +})); + +jest.mock('~/server/utils/files', () => ({ + cleanFileName: (name) => name, +})); + +jest.mock('~/cache', () => ({ + getLogStores: jest.fn(() => ({ get: jest.fn(), set: jest.fn() })), +})); + +const express = require('express'); +const request = require('supertest'); +const filesRouter = require('./files'); + +/** + * Mount the router with a per-request user injector so we can simulate + * a logged-in user without spinning up the full auth stack. + */ +function buildApp({ user = { id: 'user-123', role: 'user' } } = {}) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + req.user = user; + req.config = { fileStrategy: 'local' }; + next(); + }); + app.use('/files', filesRouter); + return app; +} + +const OWNER_USER_ID = 'user-123'; + +describe('GET /files/:file_id/preview', () => { + beforeEach(() => { + mockFindFileById.mockReset(); + mockGetFiles.mockReset(); + }); + + it('returns 404 when the file does not exist (auth check fails first via fileAccess)', async () => { + /* `fileAccess` middleware does its own getFiles lookup and returns + * 404 before our handler ever runs. This test asserts the boundary + * lives there, not that the handler duplicates the check. */ + mockGetFiles.mockResolvedValueOnce([]); + const res = await request(buildApp()).get('/files/missing-id/preview'); + expect(res.status).toBe(404); + expect(res.body).toMatchObject({ error: 'Not Found' }); + expect(mockFindFileById).not.toHaveBeenCalled(); + }); + + it('returns 403 when the requester does not own the file and has no agent-based access', async () => { + /* fileAccess returns 403 — the file exists but belongs to someone + * else and no agent grants access. The preview handler should + * never run. */ + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'someone-elses', user: 'other-user', filename: 'x.xlsx' }, + ]); + const res = await request(buildApp()).get('/files/someone-elses/preview'); + expect(res.status).toBe(403); + expect(mockFindFileById).not.toHaveBeenCalled(); + }); + + it('returns status:pending without text/textFormat while phase-2 is in flight', async () => { + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-pending', user: OWNER_USER_ID, filename: 'data.xlsx' }, + ]); + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-pending', + status: 'pending', + text: null, + textFormat: null, + }); + const res = await request(buildApp()).get('/files/fid-pending/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ file_id: 'fid-pending', status: 'pending' }); + /* Critical: pending must NOT leak `text` even if a stale value + * exists on the record. The client gates routing on status. */ + expect(res.body).not.toHaveProperty('text'); + expect(res.body).not.toHaveProperty('textFormat'); + expect(res.body).not.toHaveProperty('previewError'); + }); + + it('returns status:ready with text + textFormat when phase-2 succeeded', async () => { + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-ready', user: OWNER_USER_ID, filename: 'data.xlsx' }, + ]); + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-ready', + status: 'ready', + text: '
1
', + textFormat: 'html', + }); + const res = await request(buildApp()).get('/files/fid-ready/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + file_id: 'fid-ready', + status: 'ready', + text: '
1
', + textFormat: 'html', + }); + }); + + it('returns status:failed with previewError when phase-2 errored', async () => { + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-failed', user: OWNER_USER_ID, filename: 'data.xlsx' }, + ]); + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-failed', + status: 'failed', + text: null, + textFormat: null, + previewError: 'parser-error', + }); + const res = await request(buildApp()).get('/files/fid-failed/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + file_id: 'fid-failed', + status: 'failed', + previewError: 'parser-error', + }); + expect(res.body).not.toHaveProperty('text'); + expect(res.body).not.toHaveProperty('textFormat'); + }); + + it('defaults to status:ready for legacy records with no status field (back-compat)', async () => { + /* Records pre-dating PR #12952 have no `status` field. The endpoint + * MUST treat that as 'ready' so legacy code-execution attachments + * don't suddenly poll forever. If text exists, surface it. */ + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-legacy', user: OWNER_USER_ID, filename: 'old.csv' }, + ]); + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-legacy', + // status intentionally absent + text: 'csv,header\n1,2', + textFormat: 'text', + }); + const res = await request(buildApp()).get('/files/fid-legacy/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ + file_id: 'fid-legacy', + status: 'ready', + text: 'csv,header\n1,2', + textFormat: 'text', + }); + }); + + it('returns status:ready with no text when the record is ready but text is null (binary/oversized)', async () => { + /* Non-office files (binary, oversized, etc.) complete phase-1 with + * `text: null` and no `status` field. The endpoint surfaces them + * as ready with no text — frontend renders download-only. */ + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-binary', user: OWNER_USER_ID, filename: 'image.bin' }, + ]); + mockFindFileById.mockResolvedValueOnce({ + file_id: 'fid-binary', + text: null, + textFormat: null, + }); + const res = await request(buildApp()).get('/files/fid-binary/preview'); + expect(res.status).toBe(200); + expect(res.body).toEqual({ file_id: 'fid-binary', status: 'ready' }); + }); + + it('returns 404 if findFileById returns null even after fileAccess passed (race with delete)', async () => { + /* fileAccess sees the file via getFiles, but findFileById returns + * null because a concurrent delete just removed it. Don't 500 — + * return 404 so the client stops polling. */ + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-race', user: OWNER_USER_ID, filename: 'data.xlsx' }, + ]); + mockFindFileById.mockResolvedValueOnce(null); + const res = await request(buildApp()).get('/files/fid-race/preview'); + expect(res.status).toBe(404); + }); + + it('returns 500 with a stable shape if findFileById throws unexpectedly', async () => { + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'fid-boom', user: OWNER_USER_ID, filename: 'data.xlsx' }, + ]); + mockFindFileById.mockRejectedValueOnce(new Error('mongo down')); + const res = await request(buildApp()).get('/files/fid-boom/preview'); + expect(res.status).toBe(500); + expect(res.body).toMatchObject({ error: 'Internal Server Error' }); + }); +});