From 2bcf85beb41e53ec21b80392dbe2896a8491ed18 Mon Sep 17 00:00:00 2001 From: Artyom Savchenko Date: Fri, 10 Jul 2026 17:53:48 +0700 Subject: [PATCH] Fix export issues (#10967) * Fix export Signed-off-by: Artyom Savchenko * Fix copyright error Signed-off-by: Artyom Savchenko * Potential fix for pull request finding 'CodeQL / Uncontrolled data used in path expression' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Artyom Savchenko * Format fixes Signed-off-by: Artyom Savchenko * Sanitize names Signed-off-by: Artyom Savchenko * Fix file format Signed-off-by: Artyom Savchenko * Update copyright year Signed-off-by: Artyom Savchenko --------- Signed-off-by: Artyom Savchenko Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --- .../src/__tests__/converter.test.ts | 276 ++++++++++++++++++ .../pod-export/src/__tests__/exporter.test.ts | 49 ++++ services/export/pod-export/src/converter.ts | 70 ++++- services/export/pod-export/src/exporter.ts | 19 +- services/export/pod-export/src/server.ts | 48 ++- 5 files changed, 439 insertions(+), 23 deletions(-) create mode 100644 services/export/pod-export/src/__tests__/converter.test.ts create mode 100644 services/export/pod-export/src/__tests__/exporter.test.ts diff --git a/services/export/pod-export/src/__tests__/converter.test.ts b/services/export/pod-export/src/__tests__/converter.test.ts new file mode 100644 index 0000000000..15ff2ac389 --- /dev/null +++ b/services/export/pod-export/src/__tests__/converter.test.ts @@ -0,0 +1,276 @@ +// +// Copyright © 2026 Hardcore Engineering Inc. +// +// Licensed under the Eclipse Public License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. You may +// obtain a copy of the License at https://www.eclipse.org/legal/epl-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import { + type Class, + type Client, + type Doc, + type Hierarchy, + type MarkupBlobRef, + type MeasureContext, + type Ref, + type WorkspaceIds +} from '@hcengineering/core' +import { type StorageAdapter } from '@hcengineering/server-core' +import { Buffer } from 'buffer' +import { UnifiedConverter } from '../converter' + +// isId requires a 24-character hex string +const refA = '6763a1b2c3d4e5f601234567' as Ref +const refB = '6763a1b2c3d4e5f601234568' as Ref +const targetClass = 'test:class:Target' as Ref> +const abstractClass = 'core:class:Doc' as Ref> + +function createMockMeasureContext (): MeasureContext { + return { + info: jest.fn(), + error: jest.fn(), + warn: jest.fn(), + newChild: jest.fn(), + with: jest.fn(), + withSync: jest.fn(), + measure: jest.fn(), + end: jest.fn() + } as unknown as MeasureContext +} + +function createMockHierarchy (): Hierarchy { + return { + getAllAttributes: jest.fn(() => new Map()), + isMixin: jest.fn(() => false), + hasMixin: jest.fn(() => false), + isDerived: jest.fn(() => true), + getBaseClass: jest.fn((c: any) => c), + findDomain: jest.fn(() => 'test-domain'), + getClass: jest.fn(), + findAttribute: jest.fn() + } as unknown as Hierarchy +} + +function createMockClient (hierarchy: Hierarchy): Client { + return { + findAll: jest.fn().mockResolvedValue([]), + findOne: jest.fn(), + getHierarchy: jest.fn(() => hierarchy) + } as unknown as Client +} + +function createMockStorageAdapter (): StorageAdapter { + return { + read: jest.fn(), + stat: jest.fn(), + put: jest.fn(), + remove: jest.fn(), + exists: jest.fn() + } as unknown as StorageAdapter +} + +describe('UnifiedConverter', () => { + let context: MeasureContext + let hierarchy: Hierarchy + let client: Client + let storage: StorageAdapter + let wsIds: WorkspaceIds + let converter: UnifiedConverter + + beforeEach(() => { + jest.clearAllMocks() + context = createMockMeasureContext() + hierarchy = createMockHierarchy() + client = createMockClient(hierarchy) + storage = createMockStorageAdapter() + wsIds = { uuid: 'test-ws' as any, url: 'ws://test' } + converter = new UnifiedConverter(context, client, storage, wsIds) + }) + + describe('resolveReference', () => { + it('should return raw ref for abstract classes without domain instead of querying', async () => { + ;(hierarchy.findDomain as jest.Mock).mockReturnValue(undefined) + + const result = await (converter as any).resolveReference(refA, abstractClass) + + expect(result).toBe(refA) + // Must not attempt findAll: abstract classes have no domain and the query would fail + expect(client.findAll).not.toHaveBeenCalled() + expect(context.error).not.toHaveBeenCalled() + expect(context.warn).not.toHaveBeenCalled() + }) + + it('should resolve a reference to a meaningful identifier', async () => { + const doc = { _id: refA, _class: targetClass, identifier: 'TSK-1' } + ;(client.findAll as jest.Mock) + .mockResolvedValueOnce([{ _id: refA }]) // ids query + .mockResolvedValueOnce([doc]) // batch query + + const result = await (converter as any).resolveReference(refA, targetClass) + + expect(result).toBe('TSK-1') + }) + + it('should warn only once per missing reference', async () => { + ;(client.findAll as jest.Mock).mockResolvedValue([]) + + const first = await (converter as any).resolveReference(refA, targetClass) + const second = await (converter as any).resolveReference(refA, targetClass) + + expect(first).toBe(refA) + expect(second).toBe(refA) + const warns = (context.warn as jest.Mock).mock.calls.filter((c) => String(c[0]).includes(refA)) + expect(warns).toHaveLength(1) + }) + + it('should not retry cache loading after a failure and should log the failure once', async () => { + ;(client.findAll as jest.Mock).mockRejectedValue(new Error('domain not found: core:class:Doc ')) + + const first = await (converter as any).resolveReference(refA, targetClass) + const second = await (converter as any).resolveReference(refB, targetClass) + + // References are kept as-is, export continues + expect(first).toBe(refA) + expect(second).toBe(refB) + + // The failed query must not be retried for every reference + expect(client.findAll).toHaveBeenCalledTimes(1) + + // Cache load failure is logged once + const cacheErrors = (context.error as jest.Mock).mock.calls.filter((c) => + String(c[0]).includes('Failed to load document cache') + ) + expect(cacheErrors).toHaveLength(1) + + // No 'Failed to resolve reference' spam per each reference + const refErrors = (context.error as jest.Mock).mock.calls.filter((c) => + String(c[0]).includes('Failed to resolve reference') + ) + expect(refErrors).toHaveLength(0) + }) + + it('should return non-id values as-is', async () => { + const result = await (converter as any).resolveReference('not-an-id', targetClass) + + expect(result).toBe('not-an-id') + expect(client.findAll).not.toHaveBeenCalled() + }) + }) + + describe('resolveMarkdown', () => { + const blobRef = 'blob-description-1' as MarkupBlobRef + + it('should return markup content when blob exists', async () => { + ;(storage.stat as jest.Mock).mockResolvedValue({ size: 4, contentType: 'application/json' }) + ;(storage.read as jest.Mock).mockResolvedValue([Buffer.from('test')]) + + const result = await (converter as any).resolveMarkdown(blobRef) + + expect(result).toBe('test') + expect(context.warn).not.toHaveBeenCalled() + expect(context.error).not.toHaveBeenCalled() + }) + + it('should warn and return empty string when blob does not exist (stat undefined)', async () => { + ;(storage.stat as jest.Mock).mockResolvedValue(undefined) + + const result = await (converter as any).resolveMarkdown(blobRef) + + expect(result).toBe('') + expect(storage.read).not.toHaveBeenCalled() + expect(context.warn).toHaveBeenCalledWith(expect.stringContaining(`Blob not found: ${blobRef}`)) + expect(context.error).not.toHaveBeenCalled() + }) + + it('should treat "missing" read errors as warnings, not export errors', async () => { + ;(storage.stat as jest.Mock).mockResolvedValue({ size: 4, contentType: 'application/json' }) + ;(storage.read as jest.Mock).mockRejectedValue(new Error(`uuid=x dataId=y missing ${blobRef}`)) + + const result = await (converter as any).resolveMarkdown(blobRef) + + expect(result).toBe('') + expect(context.warn).toHaveBeenCalledWith(expect.stringContaining(`Blob content not found: ${blobRef}`)) + expect(context.error).not.toHaveBeenCalled() + }) + + it('should warn only once per missing blob', async () => { + ;(storage.stat as jest.Mock).mockResolvedValue(undefined) + + await (converter as any).resolveMarkdown(blobRef) + await (converter as any).resolveMarkdown(blobRef) + + const warns = (context.warn as jest.Mock).mock.calls.filter((c) => String(c[0]).includes(blobRef)) + expect(warns).toHaveLength(1) + }) + + it('should log unexpected read errors as errors and return empty string', async () => { + ;(storage.stat as jest.Mock).mockResolvedValue({ size: 4, contentType: 'application/json' }) + ;(storage.read as jest.Mock).mockRejectedValue(new Error('connection refused')) + + const result = await (converter as any).resolveMarkdown(blobRef) + + expect(result).toBe('') + expect(context.error).toHaveBeenCalledWith( + expect.stringContaining(`Failed to resolve markup content: ${blobRef}`), + expect.objectContaining({ error: 'connection refused' }) + ) + }) + }) + + describe('resolveAttachments', () => { + const docId = refA + const docClass = 'test:class:Issue' as Ref> + + function mockAttachmentCache (): void { + const att = { + _id: refB, + _class: 'attachment:class:Attachment' as Ref>, + attachedTo: docId, + attachedToClass: docClass, + collection: 'attachments', + name: 'file.txt', + size: 4, + type: 'text/plain', + file: 'blob-file-1' + } + ;(client.findAll as jest.Mock) + .mockResolvedValueOnce([{ _id: att._id }]) // ids query + .mockResolvedValueOnce([att]) // batch query + } + + it('should return empty buffer and warn when attachment read fails', async () => { + mockAttachmentCache() + ;(storage.read as jest.Mock).mockRejectedValue(new Error('missing blob-file-1')) + + const attachments = await (converter as any).resolveAttachments(docId, docClass) + + expect(attachments).toHaveLength(1) + const data = await attachments[0].getData() + + expect(data).toEqual(Buffer.from([])) + expect(context.warn).toHaveBeenCalledWith( + expect.stringContaining(`Failed to read attachment: ${refB}`), + expect.objectContaining({ error: 'missing blob-file-1' }) + ) + }) + + it('should return attachment data when read succeeds', async () => { + mockAttachmentCache() + ;(storage.read as jest.Mock).mockResolvedValue([Buffer.from('test')]) + + const attachments = await (converter as any).resolveAttachments(docId, docClass) + const data = await attachments[0].getData() + + expect(data).toEqual(Buffer.from('test')) + expect(context.warn).not.toHaveBeenCalled() + }) + }) +}) diff --git a/services/export/pod-export/src/__tests__/exporter.test.ts b/services/export/pod-export/src/__tests__/exporter.test.ts new file mode 100644 index 0000000000..ce865c8661 --- /dev/null +++ b/services/export/pod-export/src/__tests__/exporter.test.ts @@ -0,0 +1,49 @@ +// +// Copyright © 2026 Hardcore Engineering Inc. +// +// Licensed under the Eclipse Public License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. You may +// obtain a copy of the License at https://www.eclipse.org/legal/epl-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import { sanitizeSpaceFileName } from '../exporter' + +describe('sanitizeSpaceFileName', () => { + it('should keep ordinary names unchanged', () => { + expect(sanitizeSpaceFileName('DEV')).toBe('DEV') + expect(sanitizeSpaceFileName('Wellness Vault')).toBe('Wellness Vault') + }) + + it('should replace path separators to avoid nested directories', () => { + expect(sanitizeSpaceFileName('UI/UX Portfolio')).toBe('UI_UX Portfolio') + expect(sanitizeSpaceFileName('Ventas/Marketing')).toBe('Ventas_Marketing') + expect(sanitizeSpaceFileName('back\\slash')).toBe('back_slash') + }) + + it('should prevent path traversal', () => { + expect(sanitizeSpaceFileName('../../etc/passwd')).not.toContain('..') + expect(sanitizeSpaceFileName('../../etc/passwd')).not.toContain('/') + expect(sanitizeSpaceFileName('..')).toBe('_') + }) + + it('should strip control characters and reserved characters', () => { + expect(sanitizeSpaceFileName('name with\u0007control')).toBe('name withcontrol') + expect(sanitizeSpaceFileName('a:b*c?d"eg|h')).toBe('a_b_c_d_e_f_g_h') + }) + + it('should not produce hidden files from leading dots', () => { + expect(sanitizeSpaceFileName('.hidden')).toBe('hidden') + }) + + it('should fall back to a placeholder for empty results', () => { + expect(sanitizeSpaceFileName('')).toBe('unnamed') + expect(sanitizeSpaceFileName('\u0000\u0001')).toBe('unnamed') + }) +}) diff --git a/services/export/pod-export/src/converter.ts b/services/export/pod-export/src/converter.ts index 6f9ee7a138..e1b439b050 100644 --- a/services/export/pod-export/src/converter.ts +++ b/services/export/pod-export/src/converter.ts @@ -46,6 +46,10 @@ export class UnifiedConverter { // Fields that should not be resolved private readonly skipResolveFields = new Set(['_class', '_id', 'collection', 'attachedTo', 'attachedToClass']) + // Deduplication sets to avoid flooding logs with repeated warnings + private readonly reportedMissingRefs = new Set() + private readonly reportedMissingBlobs = new Set() + constructor ( private readonly context: MeasureContext, private readonly client: Client, @@ -264,12 +268,21 @@ export class UnifiedConverter { private async resolveReference (ref: Ref, to: Ref>): Promise { if (!isId(ref)) return ref + // References to abstract classes (e.g. core:class:Doc) cannot be resolved + // via findAll since they have no associated domain, keep the raw identifier + if (this.client.getHierarchy().findDomain(to) === undefined) { + return ref + } + try { const { byId } = await this.getCache(to) const doc = byId.get(ref) if (doc === undefined) { - this.context.warn(`Referenced document not found: ${ref}`) + if (!this.reportedMissingRefs.has(ref)) { + this.reportedMissingRefs.add(ref) + this.context.warn(`Referenced document not found: ${ref}`) + } return ref } @@ -290,7 +303,15 @@ export class UnifiedConverter { p = await p } if (p === undefined) { - p = this.loadCache(to) + p = this.loadCache(to).catch((err) => { + // Cache an empty result on failure, otherwise every reference + // to this class retries the query and floods logs with errors + this.context.error(`Failed to load document cache for ${to}`, { + error: err instanceof Error ? err.message : String(err) + }) + const empty: DocCache = { byId: new Map(), byAttached: new Map() } + return empty + }) this.documentCache.set(to, p) p = await p this.documentCache.set(to, p) @@ -324,6 +345,17 @@ export class UnifiedConverter { private async resolveMarkdown (blobRef: MarkupBlobRef): Promise { try { + // Check blob existence first: storage adapters throw on missing blobs, + // and a missing description blob should not be treated as an export error + const stat = await this.storage.stat(this.context, this.wsIds, blobRef) + if (stat === undefined) { + if (!this.reportedMissingBlobs.has(blobRef)) { + this.reportedMissingBlobs.add(blobRef) + this.context.warn(`Blob not found: ${blobRef}`) + } + return '' + } + const buffer = await this.storage.read(this.context, this.wsIds, blobRef) if (buffer === undefined) { this.context.warn(`Blob not found: ${blobRef}`) @@ -334,10 +366,19 @@ export class UnifiedConverter { // const markdown = await markupToMarkdown(markup, '', '') return markup // todo: test it is a markdown } catch (err) { - this.context.error(`Failed to resolve markup content: ${blobRef}`, { - error: err instanceof Error ? err.message : String(err), - blobRef - }) + const message = err instanceof Error ? err.message : String(err) + // Missing blob content is a data consistency issue, not an export failure + if (message.includes('missing')) { + if (!this.reportedMissingBlobs.has(blobRef)) { + this.reportedMissingBlobs.add(blobRef) + this.context.warn(`Blob content not found: ${blobRef}`) + } + } else { + this.context.error(`Failed to resolve markup content: ${blobRef}`, { + error: message, + blobRef + }) + } return '' } } @@ -371,14 +412,21 @@ export class UnifiedConverter { size: att.size, contentType: att.type, getData: async () => { - const buffer = await this.storage.read(this.context, this.wsIds, att.file) + try { + const buffer = await this.storage.read(this.context, this.wsIds, att.file) - if (buffer === undefined) { - this.context.warn(`Attachment not found: ${att._id}`) + if (buffer === undefined) { + this.context.warn(`Attachment not found: ${att._id}`) + return Buffer.from([]) + } + + return Buffer.concat(buffer as any) + } catch (err) { + this.context.warn(`Failed to read attachment: ${att._id}`, { + error: err instanceof Error ? err.message : String(err) + }) return Buffer.from([]) } - - return Buffer.concat(buffer as any) } }) ) diff --git a/services/export/pod-export/src/exporter.ts b/services/export/pod-export/src/exporter.ts index fffe57f967..2b6306b384 100644 --- a/services/export/pod-export/src/exporter.ts +++ b/services/export/pod-export/src/exporter.ts @@ -44,6 +44,17 @@ export interface ExportOptions { query?: DocumentQuery } +export function sanitizeSpaceFileName (name: string): string { + const sanitized = name + // eslint-disable-next-line no-control-regex + .replace(/[\u0000-\u001f]/g, '') + .replace(/[/\\:*?"<>|]/g, '_') + .replace(/\.{2,}/g, '_') + .trim() + .replace(/^\.+/, '') + return sanitized.length > 0 ? sanitized : 'unnamed' +} + export class WorkspaceExporter { private readonly jsonSerializer: UnifiedJsonSerializer private readonly csvSerializer: UnifiedCsvSerializer @@ -81,6 +92,7 @@ export class WorkspaceExporter { ) const limiter = new RateLimiter(50) + const usedSpaceNames = new Set() // Process each space for (const [spaceId, spaceDocs] of docsBySpace) { const space = allSpaces.get(spaceId) @@ -89,7 +101,12 @@ export class WorkspaceExporter { continue } - const spaceName = space.name ?? spaceId + let spaceName = sanitizeSpaceFileName(space.name ?? spaceId) + if (usedSpaceNames.has(spaceName)) { + // Different spaces may share the same (sanitized) name, avoid overwriting + spaceName = `${spaceName}-${spaceId}` + } + usedSpaceNames.add(spaceName) const spaceDir = path.join(outputDir, spaceName) // Convert all docs to UnifiedDoc format diff --git a/services/export/pod-export/src/server.ts b/services/export/pod-export/src/server.ts index 82e449a45e..56dc5b004f 100644 --- a/services/export/pod-export/src/server.ts +++ b/services/export/pod-export/src/server.ts @@ -67,7 +67,7 @@ import archiver from 'archiver' import { sendExportCompletionNotification } from './notifications' import cors from 'cors' import express, { type Express, type NextFunction, type Request, type Response } from 'express' -import { createWriteStream } from 'fs' +import { createReadStream, createWriteStream } from 'fs' import fs from 'fs/promises' import { IncomingHttpHeaders, type Server } from 'http' import { tmpdir } from 'os' @@ -254,6 +254,16 @@ const wrapRequest = (fn: AsyncRequestHandler) => (req: Request, res: Response, n handleRequest(fn, req, res, next) } +// Only formats actually supported by WorkspaceExporter +const supportedExportFormats: readonly ExportFormat[] = [ExportFormat.JSON, ExportFormat.CSV] + +function parseExportFormat (rawFormat: unknown): ExportFormat { + if (typeof rawFormat !== 'string' || !supportedExportFormats.includes(rawFormat as ExportFormat)) { + throw new ApiError(400, `Invalid format. Supported formats: ${supportedExportFormats.join(', ')}`) + } + return rawFormat as ExportFormat +} + export function createServer ( storageConfig: StorageConfiguration, dbUrl: string, @@ -269,7 +279,7 @@ export function createServer ( app.post( '/exportAsync', wrapRequest(async (req, res, wsIds, token, socialId) => { - const format = req.query.format as ExportFormat + const format = parseExportFormat(req.query.format) const { _class, @@ -281,7 +291,7 @@ export function createServer ( attributesOnly: boolean } = req.body - if (_class == null || format == null) { + if (_class == null) { throw new ApiError(400, 'Missing required parameters') } @@ -343,9 +353,23 @@ export function createServer ( await sendSuccessNotification(txOperations, account, exportDrive, archiveName) } catch (err: any) { measureCtx.error('Export failed:', err) - await sendFailureNotification(txOperations, account, err.message ?? 'Unknown error when exporting') + try { + // Attach the failure notification to the export drive, otherwise + // the user is never notified that the export has failed + const exportDrive = await ensureExportDrive(txOperations, account) + await sendFailureNotification( + txOperations, + account, + err.message ?? 'Unknown error when exporting', + drive.class.Drive, + exportDrive, + core.space.Space + ) + } catch (notifyErr: any) { + measureCtx.error('Failed to send export failure notification:', notifyErr) + } } finally { - await fs.rmdir(exportDir, { recursive: true }) + await fs.rm(exportDir, { recursive: true, force: true }) } })() }) @@ -354,7 +378,7 @@ export function createServer ( app.post( '/exportSync', wrapRequest(async (req, res, wsIds, token, socialId) => { - const format = req.query.format as ExportFormat + const format = parseExportFormat(req.query.format) const { _class, query, @@ -367,7 +391,7 @@ export function createServer ( config?: TransformConfig } = req.body - if (_class == null || format == null) { + if (_class == null) { throw new ApiError(400, 'Missing required parameters') } @@ -394,7 +418,7 @@ export function createServer ( measureCtx.error('Export failed:', err) throw err } finally { - void fs.rmdir(exportDir, { recursive: true }) + void fs.rm(exportDir, { recursive: true, force: true }) } }) ) @@ -655,14 +679,16 @@ async function saveToDrive ( ): Promise> { const exportDrive = await ensureExportDrive(client, account) - const fileContent = await fs.readFile(archivePath) + // Stream the archive instead of reading it into memory: + // fs.readFile fails with ERR_FS_FILE_TOO_LARGE for archives larger than 2 GiB + const { size } = await fs.stat(archivePath) const blobId = uuid() as Ref - await storage.put(ctx, wsIds, blobId, fileContent, 'application/zip', fileContent.length) + await storage.put(ctx, wsIds, blobId, createReadStream(archivePath), 'application/zip', size) await createFile(client, exportDrive, drive.ids.Root, { title: basename(archivePath), file: blobId, - size: fileContent.length, + size, type: 'application/zip', lastModified: Date.now() })