diff --git a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts index 20edf2393..6e589354b 100644 --- a/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts +++ b/frontend/editor/src/core/hooks/tools/shared/useToolOperation.ts @@ -21,6 +21,7 @@ import { StirlingFileStub, } from "@app/types/fileContext"; import { FILE_EVENTS } from "@app/services/errorUtils"; +import { zipFileService } from "@app/services/zipFileService"; import { getFilenameWithoutExtension } from "@app/utils/fileUtils"; import { createChildStub, @@ -274,29 +275,36 @@ export const useToolOperation = ( responseType: "blob", }); - // Multi-file responses are typically ZIP files that need extraction, but some may return single PDFs + const responseBlob: Blob = response.data; + const contentTypeHeader = response.headers?.["content-type"]; + if (config.responseHandler) { - // Use custom responseHandler for multi-file (handles ZIP extraction) processedFiles = await config.responseHandler( - response.data, + responseBlob, filesForAPI, ); } else if ( - response.data.type === "application/pdf" || - (response.headers && - response.headers["content-type"] === "application/pdf") + await zipFileService.isZipResponse( + responseBlob, + typeof contentTypeHeader === "string" + ? contentTypeHeader + : undefined, + ) ) { - // Single PDF response (e.g. split with merge option) - add prefix to first original filename - const filename = `${config.filePrefix}${filesForAPI[0]?.name || "document.pdf"}`; - const singleFile = new File([response.data], filename, { - type: "application/pdf", - }); - processedFiles = [singleFile]; + processedFiles = await extractZipFiles(responseBlob); } else { - // Default: assume ZIP response for multi-file endpoints - // Note: extractZipFiles will check preferences.autoUnzip setting - processedFiles = await extractZipFiles(response.data); + const filename = `${config.filePrefix}${filesForAPI[0]?.name || "document.pdf"}`; + processedFiles = [ + new File([responseBlob], filename, { type: "application/pdf" }), + ]; } + + if (processedFiles.length === 0) { + throw new Error( + "The server processed the request but returned no files.", + ); + } + // Assume all inputs succeeded together unless server provided an error earlier successSourceIds = validFiles.map((f) => f.fileId); break; diff --git a/frontend/editor/src/core/services/zipFileService.test.ts b/frontend/editor/src/core/services/zipFileService.test.ts new file mode 100644 index 000000000..0d76ab9b0 --- /dev/null +++ b/frontend/editor/src/core/services/zipFileService.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, test } from "vitest"; +import { zipFileService } from "@app/services/zipFileService"; + +// jsdom's Blob.slice(...).arrayBuffer() returns an empty buffer in this +// version, so a real Blob would never reach the magic-byte branch under test. +// Duck-type the parts isZipResponse actually touches. +function fakeBlob(bytes: number[], type = ""): Blob { + const u8 = new Uint8Array(bytes); + const slice = (start: number, end?: number) => + fakeBlob(Array.from(u8.slice(start, end)), type); + return { + size: u8.byteLength, + type, + slice, + arrayBuffer: async () => + u8.buffer.slice(u8.byteOffset, u8.byteOffset + u8.byteLength), + } as unknown as Blob; +} + +const PDF_BYTES = [0x25, 0x50, 0x44, 0x46, 0x2d, 0x31, 0x2e, 0x37]; // "%PDF-1.7" + +describe("zipFileService.isZipResponse", () => { + describe("signature is authoritative", () => { + test("PK\\x03\\x04 -> ZIP", async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob([0x50, 0x4b, 0x03, 0x04, 0x00, 0x00]), + ), + ).toBe(true); + }); + + test("PK\\x05\\x06 (empty archive) -> ZIP", async () => { + expect( + await zipFileService.isZipResponse(fakeBlob([0x50, 0x4b, 0x05, 0x06])), + ).toBe(true); + }); + + test("PK\\x07\\x08 (spanned marker) -> ZIP", async () => { + expect( + await zipFileService.isZipResponse(fakeBlob([0x50, 0x4b, 0x07, 0x08])), + ).toBe(true); + }); + + test("%PDF signature overrides a ZIP Content-Type", async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob(PDF_BYTES), + "application/zip", + ), + ).toBe(false); + }); + }); + + // Bug-trigger Content-Types: previously misrouted PDFs into ZIP extraction. + describe("bug-trigger Content-Types must not misroute a PDF", () => { + test('"application/pdf;charset=UTF-8" + %PDF -> NOT ZIP', async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob(PDF_BYTES), + "application/pdf;charset=UTF-8", + ), + ).toBe(false); + }); + + test('"application/octet-stream" + %PDF -> NOT ZIP', async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob(PDF_BYTES), + "application/octet-stream", + ), + ).toBe(false); + }); + + test('"APPLICATION/PDF" + %PDF -> NOT ZIP', async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob(PDF_BYTES), + "APPLICATION/PDF", + ), + ).toBe(false); + }); + }); + + describe("Content-Type fallback when signature is inconclusive", () => { + const UNKNOWN_BYTES = [0xff, 0xff, 0xff, 0xff]; + + test('unknown bytes + "application/zip" -> ZIP', async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob(UNKNOWN_BYTES), + "application/zip", + ), + ).toBe(true); + }); + + test('unknown bytes + "application/x-zip-compressed" -> ZIP', async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob(UNKNOWN_BYTES), + "application/x-zip-compressed", + ), + ).toBe(true); + }); + + test('unknown bytes + "application/json" -> NOT ZIP', async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob(UNKNOWN_BYTES), + "application/json", + ), + ).toBe(false); + }); + + test("unknown bytes, no hint, blob.type 'application/zip' -> ZIP", async () => { + expect( + await zipFileService.isZipResponse( + fakeBlob(UNKNOWN_BYTES, "application/zip"), + ), + ).toBe(true); + }); + }); +}); diff --git a/frontend/editor/src/core/services/zipFileService.ts b/frontend/editor/src/core/services/zipFileService.ts index ffe5d9758..8ce3d90ad 100644 --- a/frontend/editor/src/core/services/zipFileService.ts +++ b/frontend/editor/src/core/services/zipFileService.ts @@ -303,6 +303,37 @@ export class ZipFileService { return hasValidType || hasValidExtension; } + // Signature-first ZIP detection for HTTP response bodies. Content-Type alone + // is unreliable: proxies and some backends ship merged PDFs as + // "application/pdf;charset=UTF-8" or "application/octet-stream", which an + // exact-equality check would misroute into ZIP extraction. + public async isZipResponse( + blob: Blob, + contentTypeHint?: string, + ): Promise { + try { + const sig = new Uint8Array(await blob.slice(0, 4).arrayBuffer()); + if ( + sig[0] === 0x50 && + sig[1] === 0x4b && + (sig[2] === 0x03 || sig[2] === 0x05 || sig[2] === 0x07) + ) { + return true; + } + if ( + sig[0] === 0x25 && + sig[1] === 0x50 && + sig[2] === 0x44 && + sig[3] === 0x46 + ) { + return false; + } + } catch { + // Unreadable blob - fall back to Content-Type. + } + return (contentTypeHint || blob.type || "").toLowerCase().includes("zip"); + } + /** * Check if a filename indicates a PDF file */ diff --git a/frontend/editor/src/core/tests/stubbed/merge-response-handling.spec.ts b/frontend/editor/src/core/tests/stubbed/merge-response-handling.spec.ts new file mode 100644 index 000000000..8dd5662c3 --- /dev/null +++ b/frontend/editor/src/core/tests/stubbed/merge-response-handling.spec.ts @@ -0,0 +1,71 @@ +import type { Page, Route } from "@playwright/test"; +import { test, expect } from "@app/tests/helpers/stub-test-base"; +import { + uploadFiles, + switchToEditorIfViewerMode, + runToolAndWaitForReview, +} from "@app/tests/helpers/ui-helpers"; +import path from "path"; +import fs from "fs"; + +// Regression coverage: the merge endpoint used to misroute a PDF into JSZip +// when the Content-Type wasn't an exact `application/pdf`, producing a bogus +// `result.zip` instead of the merged file. The UI fix uses signature-based +// detection - %PDF wins regardless of Content-Type. + +const FIXTURES_DIR = path.join(__dirname, "../test-fixtures"); +const SAMPLE_PDF = path.join(FIXTURES_DIR, "sample.pdf"); +const SAMPLE_PDF_BYTES = fs.readFileSync(SAMPLE_PDF); + +async function mockMergeWithContentType(page: Page, contentType: string) { + await page.route("**/api/v1/general/merge-pdfs", (route: Route) => + route.fulfill({ + status: 200, + contentType, + headers: { + "Content-Disposition": 'attachment; filename="merged_unsigned.pdf"', + }, + body: SAMPLE_PDF_BYTES, + }), + ); +} + +async function runMergeAndOpenReview(page: Page) { + await page.goto("/merge"); + await uploadFiles(page, [SAMPLE_PDF, SAMPLE_PDF]); + await switchToEditorIfViewerMode(page); + await runToolAndWaitForReview(page); +} + +test.describe("Merge multi-file response detection", () => { + test('"application/pdf;charset=UTF-8" -> merged PDF, not result.zip', async ({ + page, + }) => { + await mockMergeWithContentType(page, "application/pdf;charset=UTF-8"); + await runMergeAndOpenReview(page); + + const review = page.locator('[data-testid="review-panel-container"]'); + await expect(review).toBeVisible(); + await expect(review.getByText(/result\.zip/i)).toHaveCount(0); + }); + + test('"application/octet-stream" -> merged PDF, not result.zip', async ({ + page, + }) => { + await mockMergeWithContentType(page, "application/octet-stream"); + await runMergeAndOpenReview(page); + + const review = page.locator('[data-testid="review-panel-container"]'); + await expect(review).toBeVisible(); + await expect(review.getByText(/result\.zip/i)).toHaveCount(0); + }); + + test('"APPLICATION/PDF" -> merged PDF, not result.zip', async ({ page }) => { + await mockMergeWithContentType(page, "APPLICATION/PDF"); + await runMergeAndOpenReview(page); + + const review = page.locator('[data-testid="review-panel-container"]'); + await expect(review).toBeVisible(); + await expect(review.getByText(/result\.zip/i)).toHaveCount(0); + }); +}); diff --git a/frontend/editor/src/core/utils/automationExecutor.test.ts b/frontend/editor/src/core/utils/automationExecutor.test.ts new file mode 100644 index 000000000..5cff7b2ab --- /dev/null +++ b/frontend/editor/src/core/utils/automationExecutor.test.ts @@ -0,0 +1,58 @@ +import { describe, expect, test } from "vitest"; +import { processMultiFileResponse } from "@app/utils/automationExecutor"; + +// Regression coverage for the automation-side mirror of the merge bug: +// non-canonical Content-Types previously misrouted a PDF into ZIP extraction +// and yielded a bogus `automation_*.zip`. + +const PDF_BYTES = new Uint8Array([ + 0x25, + 0x50, + 0x44, + 0x46, + 0x2d, + 0x31, + 0x2e, + 0x37, // "%PDF-1.7" + 0x0a, + 0x25, + 0xe2, + 0xe3, + 0xcf, + 0xd3, + 0x0a, +]); + +const inputFiles = [ + new File(["fake"], "input.pdf", { type: "application/pdf" }), +]; + +async function run(contentType: string) { + return processMultiFileResponse( + new Blob([PDF_BYTES]), + { "content-type": contentType }, + inputFiles, + "automated_", + false, + ); +} + +describe("processMultiFileResponse (automation execution)", () => { + test('PDF body + "application/octet-stream" -> PDF, not .zip', async () => { + const result = await run("application/octet-stream"); + expect(result.length).toBe(1); + expect(result[0].name).not.toMatch(/\.zip$/); + }); + + test('PDF body + "application/pdf;charset=UTF-8" -> PDF, not .zip', async () => { + const result = await run("application/pdf;charset=UTF-8"); + expect(result.length).toBe(1); + expect(result[0].name).not.toMatch(/\.zip$/); + }); + + test('PDF body + "APPLICATION/PDF" -> PDF, not .zip', async () => { + const result = await run("APPLICATION/PDF"); + expect(result.length).toBe(1); + expect(result[0].name).not.toMatch(/\.zip$/); + }); +}); diff --git a/frontend/editor/src/core/utils/automationExecutor.ts b/frontend/editor/src/core/utils/automationExecutor.ts index 806674568..139fca981 100644 --- a/frontend/editor/src/core/utils/automationExecutor.ts +++ b/frontend/editor/src/core/utils/automationExecutor.ts @@ -4,54 +4,48 @@ import { ToolId } from "@app/types/toolId"; import { AUTOMATION_CONSTANTS } from "@app/constants/automation"; import { AutomationFileProcessor } from "@app/utils/automationFileProcessor"; import { ToolType } from "@app/hooks/tools/shared/useToolOperation"; +import { zipFileService } from "@app/services/zipFileService"; import { processResponse } from "@app/utils/toolResponseProcessor"; -/** - * Process multi-file tool response (handles ZIP or single PDF responses) - */ -const processMultiFileResponse = async ( +export const processMultiFileResponse = async ( responseData: Blob, responseHeaders: any, files: File[], filePrefix: string, preserveBackendFilename?: boolean, ): Promise => { - // Multi-file responses are typically ZIP files, but may be single files (e.g. split with merge=true) - if ( - responseData.type === "application/pdf" || - (responseHeaders && responseHeaders["content-type"] === "application/pdf") - ) { - // Single PDF response - use processResponse to respect preserveBackendFilename - const processedFiles = await processResponse( + const contentTypeHeader = responseHeaders?.["content-type"]; + const looksLikeZip = await zipFileService.isZipResponse( + responseData, + typeof contentTypeHeader === "string" ? contentTypeHeader : undefined, + ); + + if (!looksLikeZip) { + return processResponse( responseData, files, filePrefix, undefined, preserveBackendFilename ? responseHeaders : undefined, ); - return processedFiles; - } else { - // ZIP response - const result = - await AutomationFileProcessor.extractAutomationZipFiles(responseData); - - if (result.errors.length > 0) { - console.warn(`⚠️ File processing warnings:`, result.errors); - } - - // Apply prefix to files, replacing any existing prefix - const processedFiles = - filePrefix && !preserveBackendFilename - ? result.files.map((file) => { - const nameWithoutPrefix = file.name.replace(/^[^_]*_/, ""); - return new File([file], `${filePrefix}${nameWithoutPrefix}`, { - type: file.type, - }); - }) - : result.files; - - return processedFiles; } + + const result = + await AutomationFileProcessor.extractAutomationZipFiles(responseData); + + if (result.errors.length > 0) { + console.warn(`⚠️ File processing warnings:`, result.errors); + } + + if (!filePrefix || preserveBackendFilename) { + return result.files; + } + return result.files.map((file) => { + const nameWithoutPrefix = file.name.replace(/^[^_]*_/, ""); + return new File([file], `${filePrefix}${nameWithoutPrefix}`, { + type: file.type, + }); + }); }; /** diff --git a/frontend/editor/src/core/utils/automationFileProcessor.ts b/frontend/editor/src/core/utils/automationFileProcessor.ts index cf3ff7715..5326abcdc 100644 --- a/frontend/editor/src/core/utils/automationFileProcessor.ts +++ b/frontend/editor/src/core/utils/automationFileProcessor.ts @@ -19,18 +19,6 @@ export interface AutomationProcessingResult { } export class AutomationFileProcessor { - /** - * Check if a blob is a ZIP file by examining its header - */ - static isZipFile(blob: Blob): boolean { - // This is a simple check - in a real implementation you might want to read the first few bytes - // For now, we'll rely on the extraction attempt and fallback - return ( - blob.type === "application/zip" || - blob.type === "application/x-zip-compressed" - ); - } - /** * Extract files from a ZIP blob during automation execution, with fallback for non-ZIP files * Extracts all file types (PDFs, images, etc.) except HTML files which stay zipped diff --git a/frontend/editor/src/desktop/services/apiClientSetup.test.ts b/frontend/editor/src/desktop/services/apiClientSetup.test.ts new file mode 100644 index 000000000..140657018 --- /dev/null +++ b/frontend/editor/src/desktop/services/apiClientSetup.test.ts @@ -0,0 +1,175 @@ +import type { AxiosInstance } from "axios"; +import { describe, expect, test, vi, beforeEach, afterEach } from "vitest"; +import { OPEN_SIGN_IN_EVENT } from "@app/constants/signInEvents"; + +// Exercise the error-interceptor logic against a hand-rolled axios-like +// client; transitive imports are mocked out so this is a pure unit test. + +vi.mock("@app/components/toast", () => ({ alert: vi.fn() })); +vi.mock("@core/services/apiClientSetup", () => ({ + setupApiInterceptors: vi.fn(), + getAuthHeaders: vi.fn(), +})); +vi.mock("@app/services/tauriBackendService", () => ({ + tauriBackendService: { + isOnline: true, + getBackendStatus: () => "healthy", + getBackendPort: () => 8080, + }, +})); +vi.mock("@app/constants/backendErrors", () => ({ + createBackendNotReadyError: () => new Error("backend not ready"), +})); +vi.mock("@app/services/operationRouter", () => ({ + operationRouter: { + isSelfHostedMode: vi.fn().mockResolvedValue(true), + isSaaSMode: vi.fn().mockResolvedValue(false), + getBaseUrl: vi.fn().mockResolvedValue("https://example.test"), + shouldSkipBackendReadyCheck: vi.fn().mockResolvedValue(false), + }, +})); +vi.mock("@app/services/authService", () => ({ + authService: { + awaitRefreshIfInProgress: vi.fn().mockResolvedValue(undefined), + getAuthToken: vi.fn().mockResolvedValue(null), + refreshToken: vi.fn().mockResolvedValue(false), + refreshSupabaseToken: vi.fn().mockResolvedValue(false), + }, +})); +vi.mock("@app/services/connectionModeService", () => ({ + connectionModeService: { + getServerConfig: vi.fn().mockResolvedValue({ url: "https://example.test" }), + }, +})); +vi.mock("@app/constants/connection", () => ({ + STIRLING_SAAS_URL: "https://saas.test", + STIRLING_SAAS_BACKEND_API_URL: "https://api.saas.test", +})); +vi.mock("@app/i18n", () => ({ + default: { t: (_key: string, fallback: string) => fallback || _key }, +})); + +import { setupApiInterceptors } from "@app/services/apiClientSetup"; + +type Interceptor = (value: unknown) => unknown; + +type Handlers = { + request: Interceptor[]; + responseFulfilled: Interceptor[]; + responseRejected: Interceptor[]; +}; + +type MockClient = { + interceptors: { + request: { use: (onFulfilled: Interceptor) => void }; + response: { + use: (onFulfilled: Interceptor, onRejected: Interceptor) => void; + }; + }; + request: ReturnType; +}; + +function makeMockClient(): { client: MockClient; handlers: Handlers } { + const handlers: Handlers = { + request: [], + responseFulfilled: [], + responseRejected: [], + }; + const client: MockClient = { + interceptors: { + request: { + use: (onFulfilled) => { + handlers.request.push(onFulfilled); + }, + }, + response: { + use: (onFulfilled, onRejected) => { + handlers.responseFulfilled.push(onFulfilled); + handlers.responseRejected.push(onRejected); + }, + }, + }, + // Must exist on the mock; not exercised by these tests. + request: vi.fn(), + }; + return { client, handlers }; +} + +async function triggerErrorInterceptor( + handlers: Handlers, + error: unknown, +): Promise { + const handler = handlers.responseRejected[0]; + expect(handler).toBeTypeOf("function"); + // The interceptor re-rejects after handling; swallow it. + await Promise.resolve(handler(error)).catch(() => {}); +} + +describe("desktop apiClientSetup - 401 silent-path", () => { + let events: Event[]; + let listener: (e: Event) => void; + + beforeEach(() => { + events = []; + listener = (e: Event) => events.push(e); + window.addEventListener(OPEN_SIGN_IN_EVENT, listener); + }); + + afterEach(() => { + window.removeEventListener(OPEN_SIGN_IN_EVENT, listener); + }); + + test("POST 401 without Authorization dispatches sign-in modal event", async () => { + const { client, handlers } = makeMockClient(); + setupApiInterceptors(client as unknown as AxiosInstance); + await triggerErrorInterceptor(handlers, { + response: { status: 401 }, + config: { + method: "post", + url: "/api/v1/general/merge-pdfs", + headers: {}, + }, + }); + expect(events).toHaveLength(1); + expect(events[0].type).toBe(OPEN_SIGN_IN_EVENT); + }); + + test("GET 401 without Authorization stays silent (background probe)", async () => { + const { client, handlers } = makeMockClient(); + setupApiInterceptors(client as unknown as AxiosInstance); + await triggerErrorInterceptor(handlers, { + response: { status: 401 }, + config: { + method: "get", + url: "/api/v1/config/endpoint-enabled", + headers: {}, + }, + }); + expect(events).toHaveLength(0); + }); + + test("401 on auth probe (/api/v1/auth/me) never dispatches the modal", async () => { + const { client, handlers } = makeMockClient(); + setupApiInterceptors(client as unknown as AxiosInstance); + await triggerErrorInterceptor(handlers, { + response: { status: 401 }, + config: { method: "get", url: "/api/v1/auth/me", headers: {} }, + }); + expect(events).toHaveLength(0); + }); + + test("401 with skipAuthRedirect set never dispatches the modal", async () => { + const { client, handlers } = makeMockClient(); + setupApiInterceptors(client as unknown as AxiosInstance); + await triggerErrorInterceptor(handlers, { + response: { status: 401 }, + config: { + method: "post", + url: "/api/v1/general/merge-pdfs", + headers: {}, + skipAuthRedirect: true, + }, + }); + expect(events).toHaveLength(0); + }); +}); diff --git a/frontend/editor/src/desktop/services/apiClientSetup.ts b/frontend/editor/src/desktop/services/apiClientSetup.ts index 790d8feef..16d340b49 100644 --- a/frontend/editor/src/desktop/services/apiClientSetup.ts +++ b/frontend/editor/src/desktop/services/apiClientSetup.ts @@ -188,10 +188,18 @@ export function setupApiInterceptors(client: AxiosInstance): void { if (originalRequest.skipAuthRedirect) { return Promise.reject(error); } - // If no Authorization header was sent, the user was never authenticated — - // the 401 is expected (e.g. endpoint availability checks when not signed in). - // Don't attempt a refresh or open the sign-in modal in that case. + // No token attached: stay silent for background GETs (endpoint probes + // before sign-in), but surface the sign-in modal for mutations so the + // user isn't left with a no-op click. if (!originalRequest.headers.Authorization) { + const method = (originalRequest.method || "get").toLowerCase(); + if (method !== "get") { + window.dispatchEvent( + new CustomEvent(OPEN_SIGN_IN_EVENT, { + detail: { locked: false }, + }), + ); + } return Promise.reject(error); } originalRequest._retry = true;