From d4b071fb506c27bd00d6f1927f057d667ca6dfbc Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 11 Mar 2026 11:52:46 +0100 Subject: [PATCH 1/3] feat: files plugin with multi-volume support and signed URLs - Multi-volume file browser plugin with directory listing, upload, download, delete - File browser UI components (DirectoryList, FileBreadcrumb, FileEntry, FilePreviewPanel) - Signed URL support for secure file access - useSignedUrl React hook - Dev-playground integration and documentation Co-Authored-By: Claude Opus 4.6 Signed-off-by: Atila Fassina --- .../client/src/routes/files.route.tsx | 94 ++++- docs/docs/plugins/files.md | 66 +++- packages/appkit-ui/src/react/hooks/index.ts | 5 + .../src/react/hooks/use-signed-url.ts | 100 ++++++ .../appkit/src/connectors/files/client.ts | 151 ++++++++ .../src/connectors/files/tests/client.test.ts | 322 +++++++++++++++++- packages/appkit/src/plugins/files/plugin.ts | 113 ++++++ .../src/plugins/files/tests/plugin.test.ts | 293 +++++++++++++++- packages/appkit/src/plugins/files/types.ts | 36 +- 9 files changed, 1150 insertions(+), 30 deletions(-) create mode 100644 packages/appkit-ui/src/react/hooks/use-signed-url.ts diff --git a/apps/dev-playground/client/src/routes/files.route.tsx b/apps/dev-playground/client/src/routes/files.route.tsx index 4f4d8b87..24b61751 100644 --- a/apps/dev-playground/client/src/routes/files.route.tsx +++ b/apps/dev-playground/client/src/routes/files.route.tsx @@ -1,13 +1,14 @@ import type { DirectoryEntry, FilePreview } from "@databricks/appkit-ui/react"; import { Button, + Card, DirectoryList, FileBreadcrumb, FilePreviewPanel, NewFolderInput, } from "@databricks/appkit-ui/react"; import { createFileRoute, retainSearchParams } from "@tanstack/react-router"; -import { FolderPlus, Loader2, Upload } from "lucide-react"; +import { FolderPlus, Link, Loader2, Upload } from "lucide-react"; import { type RefObject, useCallback, @@ -27,6 +28,7 @@ function nextSignal(ref: RefObject): AbortSignal { return ref.current.signal; } +import { useSignedUrl } from "@databricks/appkit-ui/react"; import { Header } from "@/components/layout/header"; export const Route = createFileRoute("/files")({ @@ -71,6 +73,17 @@ function FilesRoute() { [volumeKey], ); + const { + signedUrl, + loading: signedUrlLoading, + error: signedUrlError, + copied, + generate: generateSignedUrl, + copyToClipboard, + reset: resetSignedUrl, + } = useSignedUrl(apiUrl); + + const loadDirectory = useCallback( async (path?: string) => { if (!volumeKey) return; @@ -173,6 +186,7 @@ function FilesRoute() { loadDirectory(entryPath); } else { setSelectedFile(entryPath); + resetSignedUrl(); loadPreview(entryPath); } }; @@ -391,18 +405,72 @@ function FilesRoute() { } /> - - window.open(apiUrl("download", { path }), "_blank") - } - onDelete={handleDelete} - deleting={deleting} - imagePreviewSrc={(p) => apiUrl("raw", { path: p })} - /> +
+ + window.open(apiUrl("download", { path }), "_blank") + } + onDelete={handleDelete} + deleting={deleting} + imagePreviewSrc={(p) => apiUrl("raw", { path: p })} + /> + + {selectedFile && !previewLoading && preview && ( + +
+

+ Pre-signed URL +

+ +
+ + {signedUrlError && ( +

{signedUrlError}

+ )} + + {signedUrl && ( +
+
+ (e.target as HTMLInputElement).select()} + /> + +
+

+ Expires: {new Date(signedUrl.expiresAt).toLocaleString()} +

+
+ )} + + )} +
diff --git a/docs/docs/plugins/files.md b/docs/docs/plugins/files.md index ccc91265..4a1ac8bd 100644 --- a/docs/docs/plugins/files.md +++ b/docs/docs/plugins/files.md @@ -123,6 +123,7 @@ Routes are mounted at `/api/files/*`. All routes except `/volumes` execute in us | GET | `/:volumeKey/list` | `?path` (optional) | `DirectoryEntry[]` | | GET | `/:volumeKey/read` | `?path` (required) | `text/plain` body | | GET | `/:volumeKey/download` | `?path` (required) | Binary stream (`Content-Disposition: attachment`) | +| GET | `/:volumeKey/download-url` | `?path` (required), `?expireInSeconds` (optional, 1–3600) | `PresignedDownloadUrl` | | GET | `/:volumeKey/raw` | `?path` (required) | Binary stream (inline for safe types, attachment for unsafe) | | GET | `/:volumeKey/exists` | `?path` (required) | `{ exists: boolean }` | | GET | `/:volumeKey/metadata` | `?path` (required) | `FileMetadata` | @@ -154,7 +155,7 @@ Every operation runs through the interceptor pipeline with tier-specific default | Tier | Cache | Retry | Timeout | Operations | | ------------ | ----- | ----- | ------- | ------------------------------------- | | **Read** | 60 s | 3x | 30 s | list, read, exists, metadata, preview | -| **Download** | none | 3x | 30 s | download, raw | +| **Download** | none | 3x | 30 s | download, download-url, raw | | **Write** | none | none | 600 s | upload, mkdir, delete | Retry uses exponential backoff with a 1 s initial delay. @@ -169,15 +170,13 @@ Write operations (`upload`, `mkdir`, `delete`) automatically invalidate the cach ## Programmatic API -The `exports()` API is a callable that accepts a volume key and returns a `VolumeHandle`. The handle exposes all `VolumeAPI` methods directly (service principal, logs a warning) and an `asUser(req)` method for OBO access (recommended). +The `files()` export is a callable that accepts a volume key and returns a `VolumeHandle`. All methods require OBO access via `asUser(req)` — calling them without a user context throws an error. ```ts -// OBO access (recommended) +// OBO access (required) const entries = await appkit.files("uploads").asUser(req).list(); const content = await appkit.files("exports").asUser(req).read("report.csv"); -// Service principal access (logs a warning encouraging OBO) -const entries = await appkit.files("uploads").list(); // Named accessor const vol = appkit.files.volume("uploads"); @@ -191,6 +190,7 @@ await vol.asUser(req).list(); | `list` | `(directoryPath?: string)` | `DirectoryEntry[]` | | `read` | `(filePath: string, options?: { maxSize?: number })` | `string` | | `download` | `(filePath: string)` | `DownloadResponse` | +| `createDownloadUrl` | `(filePath: string, options?: { expireInSeconds?: number })` | `PresignedDownloadUrl` | | `exists` | `(filePath: string)` | `boolean` | | `metadata` | `(filePath: string)` | `FileMetadata` | | `upload` | `(filePath: string, contents: ReadableStream \| Buffer \| string, options?: { overwrite?: boolean })` | `void` | @@ -200,6 +200,44 @@ await vol.asUser(req).list(); > `read()` loads the entire file into memory as a string. Files larger than 10 MB (default) are rejected — use `download()` for large files, or pass `{ maxSize: }` to override. +### `download` vs `createDownloadUrl` + +| | `download` | `createDownloadUrl` | +| --- | --- | --- | +| **How it works** | Streams the file through the AppKit server | Returns a pre-signed URL pointing to cloud storage (S3/ADLS/GCS) | +| **Best for** | Server-side processing, or clients that can't reach cloud storage | Large file downloads, reducing server load | +| **Proxy** | Yes — file bytes flow through the server | No — client fetches directly from cloud storage | + +### Pre-signed download URLs + +`createDownloadUrl` requests a short-lived, pre-signed URL from Unity Catalog. The URL points directly to cloud storage and bypasses the Databricks Apps proxy, making it ideal for large file downloads. + +```ts +const { url, headers, expiresAt } = await appkit + .files("uploads") + .asUser(req) + .createDownloadUrl("data/report.parquet"); + +// Client fetches directly from cloud storage using `url` and `headers`. +``` + +- **Expiration**: defaults to 900 seconds (15 min), configurable from 1 to 3600 via `expireInSeconds`. +- **OBO-only**: throws if called without user context. +- **Fallback**: if the workspace does not support pre-signed URLs, the `/download-url` HTTP route returns an error with `"fallback": "download"` so clients can fall back to the proxied `/download` endpoint. + +**Security: response headers contain cloud credentials.** +The `headers` object returned by `createDownloadUrl` may contain cloud-provider authentication tokens (e.g., SAS tokens for ADLS, signed headers for S3). Do not log, cache, or expose these headers beyond the immediate client download. + +#### Known error codes + +| Code | Meaning | +| --- | --- | +| `PRESIGNED_URL_NOT_ENABLED` | Pre-signed URL feature is not enabled on this workspace | +| `PRESIGNED_URL_NETWORK_ZONE_UNKNOWN` | Requester's network zone is unknown (private link / firewall) | +| `PRESIGNED_URL_NOT_AVAILABLE` | Endpoint not available (older workspace version) | +| `PRESIGNED_URL_FAILED` | Generic / unrecognised error | + + ## Path resolution Paths can be **absolute** or **relative**: @@ -218,6 +256,19 @@ The `list()` method with no arguments lists the volume root. type DirectoryEntry = files.DirectoryEntry; type DownloadResponse = files.DownloadResponse; +interface PresignedDownloadUrl { + /** Pre-signed URL pointing directly to cloud storage. */ + url: string; + /** + * Headers the client must include when fetching the pre-signed URL. + * May contain cloud-provider authentication tokens — do not log or expose. + */ + headers: Record; + /** ISO 8601 timestamp when the pre-signed URL expires. */ + expiresAt: string; +} + + interface FileMetadata { /** File size in bytes. */ contentLength: number | undefined; @@ -247,6 +298,7 @@ interface VolumeAPI { list(directoryPath?: string): Promise; read(filePath: string, options?: { maxSize?: number }): Promise; download(filePath: string): Promise; + createDownloadUrl(filePath: string, options?: { expireInSeconds?: number }): Promise; exists(filePath: string): Promise; metadata(filePath: string): Promise; upload(filePath: string, contents: ReadableStream | Buffer | string, options?: { overwrite?: boolean }): Promise; @@ -255,7 +307,7 @@ interface VolumeAPI { preview(filePath: string): Promise; } -/** Volume handle: all VolumeAPI methods (service principal) + asUser() for OBO. */ +/** Volume handle: methods require OBO via asUser(req). Throws without user context. */ type VolumeHandle = VolumeAPI & { asUser: (req: Request) => VolumeAPI; }; @@ -275,7 +327,7 @@ Built-in extensions: `.png`, `.jpg`, `.jpeg`, `.gif`, `.webp`, `.svg`, `.bmp`, ` Routes use `this.asUser(req)` so operations execute with the requesting user's Databricks credentials (on-behalf-of / OBO). The `/volumes` route is the only exception since it only reads plugin config. -The programmatic API returns a `VolumeHandle` that exposes all `VolumeAPI` methods directly (service principal) and an `asUser(req)` method for OBO access. Calling any method without `asUser()` logs a warning encouraging OBO usage but does not throw. OBO access is strongly recommended for production use. +The programmatic API returns a `VolumeHandle` whose methods require OBO access via `asUser(req)`. Calling any method without a user context throws an error. ## Resource requirements diff --git a/packages/appkit-ui/src/react/hooks/index.ts b/packages/appkit-ui/src/react/hooks/index.ts index f4c52e6b..1620d477 100644 --- a/packages/appkit-ui/src/react/hooks/index.ts +++ b/packages/appkit-ui/src/react/hooks/index.ts @@ -14,3 +14,8 @@ export { type UseChartDataResult, useChartData, } from "./use-chart-data"; +export { + type SignedUrl, + type UseSignedUrlResult, + useSignedUrl, +} from "./use-signed-url"; diff --git a/packages/appkit-ui/src/react/hooks/use-signed-url.ts b/packages/appkit-ui/src/react/hooks/use-signed-url.ts new file mode 100644 index 00000000..1073d838 --- /dev/null +++ b/packages/appkit-ui/src/react/hooks/use-signed-url.ts @@ -0,0 +1,100 @@ +import { useCallback, useRef, useState } from "react"; + +// ============================================================================ +// Types +// ============================================================================ + +export interface SignedUrl { + url: string; + expiresAt: string; +} + +export interface UseSignedUrlResult { + signedUrl: SignedUrl | null; + loading: boolean; + error: string | null; + copied: boolean; + generate: (filePath: string) => Promise; + copyToClipboard: () => Promise; + reset: () => void; +} + +// ============================================================================ +// Hook +// ============================================================================ + +export function useSignedUrl( + apiUrl: (action: string, params?: Record) => string, +): UseSignedUrlResult { + const [signedUrl, setSignedUrl] = useState(null); + const [loading, setLoading] = useState(false); + const [error, setError] = useState(null); + const [copied, setCopied] = useState(false); + const copiedTimer = useRef>(undefined); + + const reset = useCallback(() => { + setSignedUrl(null); + setError(null); + setCopied(false); + }, []); + + const generate = useCallback( + async (filePath: string) => { + setLoading(true); + setError(null); + setSignedUrl(null); + + try { + const response = await fetch( + apiUrl("download-url", { path: filePath }), + ); + + if (!response.ok) { + const data = await response.json().catch(() => ({})); + throw new Error( + data.error ?? `Failed to get signed URL (${response.status})`, + ); + } + + const data = await response.json(); + setSignedUrl({ url: data.url, expiresAt: data.expiresAt }); + } catch (err) { + setError(err instanceof Error ? err.message : String(err)); + } finally { + setLoading(false); + } + }, + [apiUrl], + ); + + const copyToClipboard = useCallback(async () => { + if (!signedUrl) return; + + try { + await navigator.clipboard.writeText(signedUrl.url); + } catch { + const textarea = document.createElement("textarea"); + textarea.value = signedUrl.url; + textarea.style.position = "fixed"; + textarea.style.opacity = "0"; + document.body.appendChild(textarea); + textarea.select(); + document.execCommand("copy"); + document.body.removeChild(textarea); + } + + setCopied(true); + clearTimeout(copiedTimer.current); + copiedTimer.current = setTimeout(() => setCopied(false), 2000); + }, [signedUrl]); + + return { + signedUrl, + loading, + error, + copied, + generate, + copyToClipboard, + reset, + }; +} diff --git a/packages/appkit/src/connectors/files/client.ts b/packages/appkit/src/connectors/files/client.ts index cf983e2f..43115d61 100644 --- a/packages/appkit/src/connectors/files/client.ts +++ b/packages/appkit/src/connectors/files/client.ts @@ -6,6 +6,7 @@ import type { DownloadResponse, FileMetadata, FilePreview, + PresignedDownloadUrl, } from "../../plugins/files/types"; import type { TelemetryProvider } from "../../telemetry"; import { @@ -206,6 +207,98 @@ export class FilesConnector { }); } + /** + * Requests a pre-signed download URL from Unity Catalog. + * The returned URL points directly to cloud storage (S3/ADLS/GCS), + * bypassing the Databricks Apps proxy. + * + * Uses the same direct-fetch pattern as {@link upload} to call the + * undocumented `/api/2.0/fs/create-download-url` endpoint used by + * the Databricks Python SDK. + * + * Known error modes (mirroring the Python SDK): + * - 403 with `FILES_API_API_IS_NOT_ENABLED` → feature not enabled on workspace + * - 500 with `FILES_API_REQUESTER_NETWORK_ZONE_UNKNOWN` → network zone issue + * - 404 → endpoint not available (older workspace version) + * - Other errors → transient or auth failures + */ + async createDownloadUrl( + client: WorkspaceClient, + filePath: string, + options?: { expireInSeconds?: number }, + ): Promise { + const resolvedPath = this.resolvePath(filePath); + const expireInSeconds = options?.expireInSeconds ?? 900; + + return this.traced( + "createDownloadUrl", + { "files.path": resolvedPath }, + async () => { + const hostValue = client.config.host; + if (!hostValue) { + throw new Error( + "Databricks host is not configured. Set DATABRICKS_HOST or configure client.config.host.", + ); + } + const host = hostValue.startsWith("http") + ? hostValue + : `https://${hostValue}`; + + const expiresAt = new Date( + Date.now() + expireInSeconds * 1000, + ).toISOString(); + + const url = new URL("/api/2.0/fs/create-download-url", host); + url.searchParams.set("path", resolvedPath); + url.searchParams.set("expire_time", expiresAt); + + const headers = new Headers({ + "Content-Type": "application/json", + }); + await client.config.authenticate(headers); + + const res = await fetch(url.toString(), { + method: "POST", + headers, + }); + + if (!res.ok) { + const text = await res.text(); + const errorCode = parsePresignErrorCode(res.status, text); + + logger.error( + `create-download-url failed (${res.status}, code=${errorCode}): ${text}`, + ); + + const safeMessage = + text.length > 200 ? `${text.slice(0, 200)}…` : text; + throw new ApiError( + `Failed to create download URL: ${safeMessage}`, + errorCode, + res.status, + undefined, + [], + ); + } + + const body = (await res.json()) as { + url: string; + headers?: Array<{ name: string; value: string }>; + }; + + const responseHeaders: Record = {}; + if (body.headers) { + for (const h of body.headers) { + responseHeaders[h.name] = h.value; + } + } + + return { url: body.url, headers: responseHeaders, expiresAt }; + }, + ); + } + + async exists(client: WorkspaceClient, filePath: string): Promise { const resolvedPath = this.resolvePath(filePath); return this.traced("exists", { "files.path": resolvedPath }, async () => { @@ -371,3 +464,61 @@ export class FilesConnector { }); } } + +/** + * Well-known error codes from the `create-download-url` endpoint. + * These mirror the error reasons the Databricks Python SDK checks for. + */ +export const PRESIGN_ERROR_CODES = { + /** Pre-signed URL feature is not enabled on this workspace. */ + NOT_ENABLED: "PRESIGNED_URL_NOT_ENABLED", + /** The requester's network zone is unknown — typically a private link or firewall issue. */ + NETWORK_ZONE_UNKNOWN: "PRESIGNED_URL_NETWORK_ZONE_UNKNOWN", + /** The endpoint itself is not available (404) — workspace may be on an older version. */ + NOT_AVAILABLE: "PRESIGNED_URL_NOT_AVAILABLE", + /** Generic failure for unrecognised error shapes. */ + FAILED: "PRESIGNED_URL_FAILED", +} as const; + +/** + * Parses the Databricks API error response from `create-download-url` and + * returns a well-known error code. + * + * The Python SDK checks for two specific `error_info` reasons: + * - `FILES_API_API_IS_NOT_ENABLED` (PermissionDenied / 403) + * - `FILES_API_REQUESTER_NETWORK_ZONE_UNKNOWN` (InternalError / 500) + */ +function parsePresignErrorCode(status: number, responseText: string): string { + if (status === 404) { + return PRESIGN_ERROR_CODES.NOT_AVAILABLE; + } + + try { + const body = JSON.parse(responseText) as { + error_code?: string; + message?: string; + error_info?: Array<{ reason?: string }>; + }; + + // Check error_info array (same structure the Python SDK inspects) + if (body.error_info) { + for (const info of body.error_info) { + if (info.reason === "FILES_API_API_IS_NOT_ENABLED") { + return PRESIGN_ERROR_CODES.NOT_ENABLED; + } + if (info.reason === "FILES_API_REQUESTER_NETWORK_ZONE_UNKNOWN") { + return PRESIGN_ERROR_CODES.NETWORK_ZONE_UNKNOWN; + } + } + } + + // Fallback: check error_code field + if (body.error_code === "PERMISSION_DENIED" && status === 403) { + return PRESIGN_ERROR_CODES.NOT_ENABLED; + } + } catch { + // Response wasn't JSON — use generic code + } + + return PRESIGN_ERROR_CODES.FAILED; +} diff --git a/packages/appkit/src/connectors/files/tests/client.test.ts b/packages/appkit/src/connectors/files/tests/client.test.ts index e7a4264c..d8d20f34 100644 --- a/packages/appkit/src/connectors/files/tests/client.test.ts +++ b/packages/appkit/src/connectors/files/tests/client.test.ts @@ -1,7 +1,7 @@ import type { WorkspaceClient } from "@databricks/sdk-experimental"; import { createMockTelemetry } from "@tools/test-helpers"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; -import { FilesConnector } from "../client"; +import { FilesConnector, PRESIGN_ERROR_CODES } from "../client"; import { streamFromChunks, streamFromString } from "./utils"; const { mockFilesApi, mockConfig, mockClient, MockApiError } = vi.hoisted( @@ -796,4 +796,324 @@ describe("FilesConnector", () => { expect(result.textPreview).toBe(content); }); }); + + describe("createDownloadUrl()", () => { + let connector: FilesConnector; + let fetchSpy: ReturnType; + + beforeEach(() => { + vi.clearAllMocks(); + connector = new FilesConnector({ + defaultVolume: "/Volumes/catalog/schema/vol", + }); + mockConfig.authenticate.mockResolvedValue(undefined); + fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + test("calls POST /api/2.0/fs/create-download-url with path and expire_time", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => + Promise.resolve({ + url: "https://s3.amazonaws.com/bucket/file", + headers: [{ name: "x-amz-token", value: "abc123" }], + }), + }); + + await connector.createDownloadUrl(mockClient, "file.txt"); + + const calledUrl = new URL(fetchSpy.mock.calls[0][0]); + expect(calledUrl.pathname).toBe("/api/2.0/fs/create-download-url"); + expect(calledUrl.searchParams.get("path")).toBe( + "/Volumes/catalog/schema/vol/file.txt", + ); + expect(calledUrl.searchParams.get("expire_time")).toBeTruthy(); + expect(fetchSpy.mock.calls[0][1].method).toBe("POST"); + }); + + test("authenticates via client.config.authenticate", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ url: "https://s3/file", headers: [] }), + }); + + await connector.createDownloadUrl(mockClient, "file.txt"); + + expect(mockConfig.authenticate).toHaveBeenCalledWith(expect.any(Headers)); + }); + + test("transforms headers array to Record", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => + Promise.resolve({ + url: "https://s3/file", + headers: [ + { name: "x-amz-token", value: "abc" }, + { name: "x-amz-date", value: "20250101" }, + ], + }), + }); + + const result = await connector.createDownloadUrl(mockClient, "file.txt"); + + expect(result.headers).toEqual({ + "x-amz-token": "abc", + "x-amz-date": "20250101", + }); + }); + + test("handles response with no headers array", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ url: "https://s3/file" }), + }); + + const result = await connector.createDownloadUrl(mockClient, "file.txt"); + + expect(result.headers).toEqual({}); + }); + + test("uses default 15-minute expiration", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ url: "https://s3/file", headers: [] }), + }); + + const before = Date.now(); + const result = await connector.createDownloadUrl(mockClient, "file.txt"); + const expiresAt = new Date(result.expiresAt).getTime(); + + // Should be ~15 minutes (900s) in the future + expect(expiresAt - before).toBeGreaterThanOrEqual(899_000); + expect(expiresAt - before).toBeLessThanOrEqual(901_000); + }); + + test("respects custom expireInSeconds", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ url: "https://s3/file", headers: [] }), + }); + + const before = Date.now(); + const result = await connector.createDownloadUrl(mockClient, "file.txt", { + expireInSeconds: 300, + }); + const expiresAt = new Date(result.expiresAt).getTime(); + + expect(expiresAt - before).toBeGreaterThanOrEqual(299_000); + expect(expiresAt - before).toBeLessThanOrEqual(301_000); + }); + + test("returns url and expiresAt from response", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => + Promise.resolve({ + url: "https://s3.amazonaws.com/bucket/key?sig=xyz", + headers: [], + }), + }); + + const result = await connector.createDownloadUrl(mockClient, "file.txt"); + + expect(result.url).toBe("https://s3.amazonaws.com/bucket/key?sig=xyz"); + expect(result.expiresAt).toBeTruthy(); + }); + + test("throws ApiError with NOT_AVAILABLE on 404", async () => { + fetchSpy.mockResolvedValue({ + ok: false, + status: 404, + text: () => Promise.resolve("Not Found"), + }); + + try { + await connector.createDownloadUrl(mockClient, "file.txt"); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(MockApiError); + expect((error as any).errorCode).toBe( + PRESIGN_ERROR_CODES.NOT_AVAILABLE, + ); + expect((error as any).statusCode).toBe(404); + } + }); + + test("throws ApiError with NOT_ENABLED when error_info contains FILES_API_API_IS_NOT_ENABLED", async () => { + fetchSpy.mockResolvedValue({ + ok: false, + status: 403, + text: () => + Promise.resolve( + JSON.stringify({ + error_code: "PERMISSION_DENIED", + message: "Presigned URLs are not enabled", + error_info: [{ reason: "FILES_API_API_IS_NOT_ENABLED" }], + }), + ), + }); + + try { + await connector.createDownloadUrl(mockClient, "file.txt"); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(MockApiError); + expect((error as any).errorCode).toBe(PRESIGN_ERROR_CODES.NOT_ENABLED); + expect((error as any).statusCode).toBe(403); + } + }); + + test("throws ApiError with NETWORK_ZONE_UNKNOWN when error_info contains FILES_API_REQUESTER_NETWORK_ZONE_UNKNOWN", async () => { + fetchSpy.mockResolvedValue({ + ok: false, + status: 500, + text: () => + Promise.resolve( + JSON.stringify({ + error_code: "INTERNAL_ERROR", + message: "Network zone unknown", + error_info: [ + { reason: "FILES_API_REQUESTER_NETWORK_ZONE_UNKNOWN" }, + ], + }), + ), + }); + + try { + await connector.createDownloadUrl(mockClient, "file.txt"); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(MockApiError); + expect((error as any).errorCode).toBe( + PRESIGN_ERROR_CODES.NETWORK_ZONE_UNKNOWN, + ); + expect((error as any).statusCode).toBe(500); + } + }); + + test("throws ApiError with NOT_ENABLED on 403 with PERMISSION_DENIED error_code (no error_info)", async () => { + fetchSpy.mockResolvedValue({ + ok: false, + status: 403, + text: () => + Promise.resolve( + JSON.stringify({ + error_code: "PERMISSION_DENIED", + message: "Access denied", + }), + ), + }); + + try { + await connector.createDownloadUrl(mockClient, "file.txt"); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(MockApiError); + expect((error as any).errorCode).toBe(PRESIGN_ERROR_CODES.NOT_ENABLED); + } + }); + + test("throws ApiError with FAILED on non-JSON error response", async () => { + fetchSpy.mockResolvedValue({ + ok: false, + status: 500, + text: () => Promise.resolve("Internal Server Error"), + }); + + try { + await connector.createDownloadUrl(mockClient, "file.txt"); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(MockApiError); + expect((error as any).errorCode).toBe(PRESIGN_ERROR_CODES.FAILED); + expect((error as any).statusCode).toBe(500); + } + }); + + test("throws ApiError with FAILED on unknown JSON error shape", async () => { + fetchSpy.mockResolvedValue({ + ok: false, + status: 400, + text: () => + Promise.resolve( + JSON.stringify({ + error_code: "BAD_REQUEST", + message: "Invalid request", + }), + ), + }); + + try { + await connector.createDownloadUrl(mockClient, "file.txt"); + expect.unreachable("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(MockApiError); + expect((error as any).errorCode).toBe(PRESIGN_ERROR_CODES.FAILED); + } + }); + + test("truncates long error messages to 200 chars", async () => { + const longMessage = "X".repeat(500); + fetchSpy.mockResolvedValue({ + ok: false, + status: 500, + text: () => Promise.resolve(longMessage), + }); + + try { + await connector.createDownloadUrl(mockClient, "file.txt"); + expect.unreachable("should have thrown"); + } catch (error) { + expect((error as any).message.length).toBeLessThan(300); + } + }); + + test("throws when client.config.host is not set", async () => { + const noHostClient = { + files: mockFilesApi, + config: { host: undefined, authenticate: vi.fn() }, + } as unknown as WorkspaceClient; + + await expect( + connector.createDownloadUrl(noHostClient, "file.txt"), + ).rejects.toThrow("Databricks host is not configured"); + }); + + test("resolves relative paths via defaultVolume", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ url: "https://s3/file", headers: [] }), + }); + + await connector.createDownloadUrl(mockClient, "subdir/file.txt"); + + const calledUrl = new URL(fetchSpy.mock.calls[0][0]); + expect(calledUrl.searchParams.get("path")).toBe( + "/Volumes/catalog/schema/vol/subdir/file.txt", + ); + }); + + test("passes absolute path directly", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ url: "https://s3/file", headers: [] }), + }); + + await connector.createDownloadUrl( + mockClient, + "/Volumes/other/vol/file.txt", + ); + + const calledUrl = new URL(fetchSpy.mock.calls[0][0]); + expect(calledUrl.searchParams.get("path")).toBe( + "/Volumes/other/vol/file.txt", + ); + }); + }); }); diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index e10f5d42..5e6ab049 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -6,6 +6,7 @@ import { contentTypeFromPath, FilesConnector, isSafeInlineContentType, + PRESIGN_ERROR_CODES, validateCustomContentTypes, } from "../../connectors/files"; import { getWorkspaceClient, isInUserContext } from "../../context"; @@ -26,6 +27,7 @@ import type { DownloadResponse, FilesExport, IFilesConfig, + PresignedDownloadUrl, VolumeAPI, VolumeConfig, VolumeHandle, @@ -168,6 +170,17 @@ export class FilesPlugin extends Plugin { this.throwIfNoUserContext(volumeKey, `download`); return connector.download(getWorkspaceClient(), filePath); }, + createDownloadUrl: ( + filePath: string, + options?: { expireInSeconds?: number }, + ): Promise => { + this.throwIfNoUserContext(volumeKey, `createDownloadUrl`); + return connector.createDownloadUrl( + getWorkspaceClient(), + filePath, + options, + ); + }, exists: (filePath: string) => { this.throwIfNoUserContext(volumeKey, `exists`); return connector.exists(getWorkspaceClient(), filePath); @@ -258,6 +271,17 @@ export class FilesPlugin extends Plugin { }, }); + this.route(router, { + name: "download-url", + method: "get", + path: "/:volumeKey/download-url", + handler: async (req: express.Request, res: express.Response) => { + const { connector, volumeKey } = this._resolveVolume(req, res); + if (!connector) return; + await this._handleDownloadUrl(req, res, connector, volumeKey); + }, + }); + this.route(router, { name: "exists", method: "get", @@ -513,6 +537,95 @@ export class FilesPlugin extends Plugin { }); } + private async _handleDownloadUrl( + req: express.Request, + res: express.Response, + connector: FilesConnector, + volumeKey: string, + ): Promise { + const path = req.query.path as string; + const valid = this._isValidPath(path); + if (valid !== true) { + res.status(400).json({ error: valid, plugin: this.name }); + return; + } + + const rawExpire = req.query.expireInSeconds as string | undefined; + const expireInSeconds = rawExpire + ? Number.parseInt(rawExpire, 10) + : undefined; + if ( + expireInSeconds !== undefined && + (Number.isNaN(expireInSeconds) || + expireInSeconds < 1 || + expireInSeconds > 3600) + ) { + res.status(400).json({ + error: "expireInSeconds must be between 1 and 3600", + plugin: this.name, + }); + return; + } + + // Capture the error from inside execute() so we can surface + // well-known presign error codes (execute() swallows errors). + let capturedError: unknown; + + try { + const userPlugin = this.asUser(req); + const settings: PluginExecutionSettings = { + default: FILES_DOWNLOAD_DEFAULTS, + }; + const result = await userPlugin.execute(async () => { + this.warnIfNoUserContext(volumeKey, "createDownloadUrl"); + try { + return await connector.createDownloadUrl( + getWorkspaceClient(), + path, + expireInSeconds ? { expireInSeconds } : undefined, + ); + } catch (err) { + capturedError = err; + throw err; + } + }, settings); + + if (result === undefined) { + // Check if execute() swallowed a presign-specific error so the + // client can decide to fall back to the proxy-based /download endpoint. + if (capturedError instanceof ApiError) { + const code = capturedError.errorCode; + const isNotSupported = + code === PRESIGN_ERROR_CODES.NOT_ENABLED || + code === PRESIGN_ERROR_CODES.NOT_AVAILABLE || + code === PRESIGN_ERROR_CODES.NETWORK_ZONE_UNKNOWN; + + if (isNotSupported) { + const status = capturedError.statusCode ?? 501; + res.status(status).json({ + error: capturedError.message, + errorCode: code, + fallback: "download", + plugin: this.name, + }); + return; + } + } + + res.status(500).json({ + error: "Failed to create download URL", + plugin: this.name, + }); + return; + } + res.setHeader("Cache-Control", "no-store"); + res.json(result); + } catch (error) { + this._handleApiError(res, error, "Failed to create download URL"); + } + } + + /** * Shared handler for `/download` and `/raw` endpoints. * - `download`: always forces `Content-Disposition: attachment`. diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 27b55cd5..f1746e89 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -1,5 +1,6 @@ import { mockServiceContext, setupDatabricksEnv } from "@tools/test-helpers"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { PRESIGN_ERROR_CODES } from "../../../connectors/files"; import { ServiceContext } from "../../../context/service-context"; import { AuthenticationError } from "../../../errors"; import { ResourceType } from "../../../registry"; @@ -29,11 +30,23 @@ const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { }; class MockApiError extends Error { + errorCode: string | undefined; statusCode: number; - constructor(message: string, statusCode: number) { + constructor( + message: string, + errorCodeOrStatus: string | number, + statusCode?: number, + ) { super(message); this.name = "ApiError"; - this.statusCode = statusCode; + if (typeof errorCodeOrStatus === "number") { + // Legacy 2-arg form: (message, statusCode) + this.statusCode = errorCodeOrStatus; + } else { + // Full form: (message, errorCode, statusCode) + this.errorCode = errorCodeOrStatus; + this.statusCode = statusCode ?? 500; + } } } @@ -330,9 +343,9 @@ describe("FilesPlugin", () => { plugin.injectRoutes(mockRouter); - // 1 GET /volumes + 7 GET /:volumeKey/* routes - // (list, read, download, raw, exists, metadata, preview) - expect(mockRouter.get).toHaveBeenCalledTimes(8); + // 1 GET /volumes + 8 GET /:volumeKey/* routes + // (list, read, download, raw, download-url, exists, metadata, preview) + expect(mockRouter.get).toHaveBeenCalledTimes(9); // 2 POST /:volumeKey/* routes (upload, mkdir) expect(mockRouter.post).toHaveBeenCalledTimes(2); // 1 DELETE /:volumeKey route @@ -409,6 +422,276 @@ describe("FilesPlugin", () => { }); }); + describe("download-url route", () => { + let fetchSpy: ReturnType; + + function getDownloadUrlHandler(plugin: FilesPlugin) { + const mockRouter = { + use: vi.fn(), + get: vi.fn(), + post: vi.fn(), + put: vi.fn(), + delete: vi.fn(), + patch: vi.fn(), + } as any; + + plugin.injectRoutes(mockRouter); + + const call = mockRouter.get.mock.calls.find( + (c: unknown[]) => + typeof c[0] === "string" && + (c[0] as string).endsWith("/download-url"), + ); + return call[call.length - 1] as (req: any, res: any) => Promise; + } + + function mockRes() { + const res: any = {}; + res.status = vi.fn().mockReturnValue(res); + res.json = vi.fn().mockReturnValue(res); + res.setHeader = vi.fn().mockReturnValue(res); + return res; + } + + function mockReq(volumeKey: string, query: Record = {}) { + const headers: Record = { + "x-forwarded-access-token": "test-token", + "x-forwarded-user": "test-user", + }; + return { + params: { volumeKey }, + query, + headers, + header: (name: string) => headers[name.toLowerCase()], + }; + } + + beforeEach(() => { + fetchSpy = vi.fn(); + vi.stubGlobal("fetch", fetchSpy); + mockClient.config.authenticate.mockResolvedValue(undefined); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + test("returns presigned URL on success", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + fetchSpy.mockResolvedValue({ + ok: true, + json: () => + Promise.resolve({ + url: "https://s3.amazonaws.com/bucket/file", + headers: [{ name: "x-amz-token", value: "abc" }], + }), + }); + + await handler(mockReq("uploads", { path: "file.pdf" }), res); + + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + url: "https://s3.amazonaws.com/bucket/file", + headers: { "x-amz-token": "abc" }, + expiresAt: expect.any(String), + }), + ); + }); + + test("sets Cache-Control: no-store on success", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ url: "https://s3/file", headers: [] }), + }); + + await handler(mockReq("uploads", { path: "file.pdf" }), res); + + expect(res.setHeader).toHaveBeenCalledWith("Cache-Control", "no-store"); + }); + + test("returns 400 when path is missing", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + await handler(mockReq("uploads", {}), res); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ error: "path is required" }), + ); + }); + + test("returns 400 when expireInSeconds is invalid", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + await handler( + mockReq("uploads", { path: "file.txt", expireInSeconds: "abc" }), + res, + ); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "expireInSeconds must be between 1 and 3600", + }), + ); + }); + + test("returns 400 when expireInSeconds is 0", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + await handler( + mockReq("uploads", { path: "file.txt", expireInSeconds: "0" }), + res, + ); + + expect(res.status).toHaveBeenCalledWith(400); + }); + + test("returns 400 when expireInSeconds exceeds 3600", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + await handler( + mockReq("uploads", { path: "file.txt", expireInSeconds: "7200" }), + res, + ); + + expect(res.status).toHaveBeenCalledWith(400); + }); + + test("returns 404 for unknown volume", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + await handler(mockReq("unknown", { path: "file.txt" }), res); + + expect(res.status).toHaveBeenCalledWith(404); + }); + + test("returns fallback hint when presigned URLs are not enabled (403)", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + fetchSpy.mockResolvedValue({ + ok: false, + status: 403, + text: () => + Promise.resolve( + JSON.stringify({ + error_code: "PERMISSION_DENIED", + message: "Presigned URLs not enabled", + error_info: [{ reason: "FILES_API_API_IS_NOT_ENABLED" }], + }), + ), + }); + + await handler(mockReq("uploads", { path: "file.txt" }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + errorCode: PRESIGN_ERROR_CODES.NOT_ENABLED, + fallback: "download", + plugin: "files", + }), + ); + }); + + test("returns fallback hint when endpoint is not available (404)", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + fetchSpy.mockResolvedValue({ + ok: false, + status: 404, + text: () => Promise.resolve("Not Found"), + }); + + await handler(mockReq("uploads", { path: "file.txt" }), res); + + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + errorCode: PRESIGN_ERROR_CODES.NOT_AVAILABLE, + fallback: "download", + }), + ); + }); + + test("returns fallback hint for network zone errors (500)", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + fetchSpy.mockResolvedValue({ + ok: false, + status: 500, + text: () => + Promise.resolve( + JSON.stringify({ + error_code: "INTERNAL_ERROR", + error_info: [ + { reason: "FILES_API_REQUESTER_NETWORK_ZONE_UNKNOWN" }, + ], + }), + ), + }); + + await handler(mockReq("uploads", { path: "file.txt" }), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + errorCode: PRESIGN_ERROR_CODES.NETWORK_ZONE_UNKNOWN, + fallback: "download", + }), + ); + }); + + test("returns generic 500 for unknown errors (no fallback hint)", async () => { + const plugin = new FilesPlugin(VOLUMES_CONFIG); + const handler = getDownloadUrlHandler(plugin); + const res = mockRes(); + + fetchSpy.mockResolvedValue({ + ok: false, + status: 502, + text: () => Promise.resolve("Bad Gateway"), + }); + + await handler(mockReq("uploads", { path: "file.txt" }), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Failed to create download URL", + plugin: "files", + }), + ); + // Generic errors should NOT have fallback hint + const body = res.json.mock.calls[0][0]; + expect(body.fallback).toBeUndefined(); + }); + }); + + describe("Upload Size Validation", () => { function getUploadHandler(plugin: FilesPlugin) { const mockRouter = { diff --git a/packages/appkit/src/plugins/files/types.ts b/packages/appkit/src/plugins/files/types.ts index 9a37a4a1..f35df5fa 100644 --- a/packages/appkit/src/plugins/files/types.ts +++ b/packages/appkit/src/plugins/files/types.ts @@ -18,7 +18,19 @@ export interface VolumeConfig { export interface VolumeAPI { list(directoryPath?: string): Promise; read(filePath: string, options?: { maxSize?: number }): Promise; + /** Streams the file through the AppKit server (proxied). Use when the server needs to process the file or when clients can't reach cloud storage directly. */ download(filePath: string): Promise; + /** + * Returns a pre-signed URL pointing directly to cloud storage (S3/ADLS/GCS), bypassing the server proxy. + * Use when the client should fetch the file directly — better for large files and reducing server load. + * OBO-only: throws if called without user context. + * @param filePath - Path to the file within the volume. + * @param options.expireInSeconds - URL lifetime in seconds (1–3600, default 900). + */ + createDownloadUrl( + filePath: string, + options?: { expireInSeconds?: number }, + ): Promise; exists(filePath: string): Promise; metadata(filePath: string): Promise; upload( @@ -31,6 +43,22 @@ export interface VolumeAPI { preview(filePath: string): Promise; } +/** + * Response from the UC pre-signed download URL endpoint. + * The `url` points directly to cloud storage (S3/ADLS/GCS) and bypasses the Databricks Apps proxy. + */ +export interface PresignedDownloadUrl { + /** Pre-signed URL pointing directly to cloud storage. */ + url: string; + /** + * Headers that the client must include when fetching the pre-signed URL. + * May contain cloud-provider authentication tokens — do not log or expose beyond the immediate download. + */ + headers: Record; + /** ISO 8601 timestamp when the pre-signed URL expires. */ + expiresAt: string; +} + /** * Configuration for the Files plugin. */ @@ -78,8 +106,8 @@ export interface FilePreview extends FileMetadata { /** * Volume handle returned by `app.files("volumeKey")`. * - * - `asUser(req)` — executes on behalf of the user (recommended). - * - Direct methods (e.g. `.list()`) — execute as the service principal (logs a warning encouraging OBO). + * - `asUser(req)` — executes on behalf of the user (required). + * - Direct methods (e.g. `.list()`) — throw if called without user context. */ export type VolumeHandle = VolumeAPI & { asUser: (req: IAppRequest) => VolumeAPI; @@ -91,10 +119,10 @@ export type VolumeHandle = VolumeAPI & { * * @example * ```ts - * // OBO access (recommended) + * // OBO access (required) * appKit.files("uploads").asUser(req).list() * - * // Service principal access (logs a warning) + * // Service principal access (throws — use asUser instead) * appKit.files("uploads").list() * * // Named accessor From af605ab9072ffa0cfd0bdc0280cda62aa76b306c Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 11 Mar 2026 12:59:46 +0100 Subject: [PATCH 2/3] chore: improve validation and telemetry --- .../client/src/routes/files.route.tsx | 1 - .../appkit/src/connectors/files/client.ts | 28 ++++++++++-- .../src/connectors/files/tests/client.test.ts | 43 +++++++++++++++++++ packages/appkit/src/plugins/files/plugin.ts | 1 - .../src/plugins/files/tests/plugin.test.ts | 1 - 5 files changed, 67 insertions(+), 7 deletions(-) diff --git a/apps/dev-playground/client/src/routes/files.route.tsx b/apps/dev-playground/client/src/routes/files.route.tsx index 24b61751..646a87ce 100644 --- a/apps/dev-playground/client/src/routes/files.route.tsx +++ b/apps/dev-playground/client/src/routes/files.route.tsx @@ -83,7 +83,6 @@ function FilesRoute() { reset: resetSignedUrl, } = useSignedUrl(apiUrl); - const loadDirectory = useCallback( async (path?: string) => { if (!volumeKey) return; diff --git a/packages/appkit/src/connectors/files/client.ts b/packages/appkit/src/connectors/files/client.ts index 43115d61..e62e2f8a 100644 --- a/packages/appkit/src/connectors/files/client.ts +++ b/packages/appkit/src/connectors/files/client.ts @@ -230,10 +230,19 @@ export class FilesConnector { const resolvedPath = this.resolvePath(filePath); const expireInSeconds = options?.expireInSeconds ?? 900; + if (expireInSeconds < 1 || expireInSeconds > 3600) { + throw new Error( + `expireInSeconds must be between 1 and 3600 (got ${expireInSeconds}).`, + ); + } + return this.traced( "createDownloadUrl", - { "files.path": resolvedPath }, - async () => { + { + "files.path": resolvedPath, + "files.presign.expire_seconds": String(expireInSeconds), + }, + async (span) => { const hostValue = client.config.host; if (!hostValue) { throw new Error( @@ -266,6 +275,9 @@ export class FilesConnector { const text = await res.text(); const errorCode = parsePresignErrorCode(res.status, text); + span.setAttribute("files.presign.error_code", errorCode); + span.setAttribute("files.presign.status_code", res.status); + logger.error( `create-download-url failed (${res.status}, code=${errorCode}): ${text}`, ); @@ -286,6 +298,11 @@ export class FilesConnector { headers?: Array<{ name: string; value: string }>; }; + // Compute expiresAt after the successful response to reduce clock drift + const actualExpiresAt = new Date( + Date.now() + expireInSeconds * 1000, + ).toISOString(); + const responseHeaders: Record = {}; if (body.headers) { for (const h of body.headers) { @@ -293,12 +310,15 @@ export class FilesConnector { } } - return { url: body.url, headers: responseHeaders, expiresAt }; + return { + url: body.url, + headers: responseHeaders, + expiresAt: actualExpiresAt, + }; }, ); } - async exists(client: WorkspaceClient, filePath: string): Promise { const resolvedPath = this.resolvePath(filePath); return this.traced("exists", { "files.path": resolvedPath }, async () => { diff --git a/packages/appkit/src/connectors/files/tests/client.test.ts b/packages/appkit/src/connectors/files/tests/client.test.ts index d8d20f34..05fa861b 100644 --- a/packages/appkit/src/connectors/files/tests/client.test.ts +++ b/packages/appkit/src/connectors/files/tests/client.test.ts @@ -1115,5 +1115,48 @@ describe("FilesConnector", () => { "/Volumes/other/vol/file.txt", ); }); + + test("throws when expireInSeconds is 0", async () => { + await expect( + connector.createDownloadUrl(mockClient, "file.txt", { + expireInSeconds: 0, + }), + ).rejects.toThrow("expireInSeconds must be between 1 and 3600"); + }); + + test("throws when expireInSeconds is negative", async () => { + await expect( + connector.createDownloadUrl(mockClient, "file.txt", { + expireInSeconds: -1, + }), + ).rejects.toThrow("expireInSeconds must be between 1 and 3600"); + }); + + test("throws when expireInSeconds exceeds 3600", async () => { + await expect( + connector.createDownloadUrl(mockClient, "file.txt", { + expireInSeconds: 7200, + }), + ).rejects.toThrow("expireInSeconds must be between 1 and 3600"); + }); + + test("accepts expireInSeconds at boundary values (1 and 3600)", async () => { + fetchSpy.mockResolvedValue({ + ok: true, + json: () => Promise.resolve({ url: "https://s3/file", headers: [] }), + }); + + await expect( + connector.createDownloadUrl(mockClient, "file.txt", { + expireInSeconds: 1, + }), + ).resolves.toBeDefined(); + + await expect( + connector.createDownloadUrl(mockClient, "file.txt", { + expireInSeconds: 3600, + }), + ).resolves.toBeDefined(); + }); }); }); diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 5e6ab049..29da3359 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -625,7 +625,6 @@ export class FilesPlugin extends Plugin { } } - /** * Shared handler for `/download` and `/raw` endpoints. * - `download`: always forces `Content-Disposition: attachment`. diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index f1746e89..5851a000 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -691,7 +691,6 @@ describe("FilesPlugin", () => { }); }); - describe("Upload Size Validation", () => { function getUploadHandler(plugin: FilesPlugin) { const mockRouter = { From e3e03f55405c299227713d81397e9644974f9c59 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 11 Mar 2026 14:13:32 +0100 Subject: [PATCH 3/3] chore: improve the UI components --- .../client/src/routes/files.route.tsx | 11 +++- .../src/react/hooks/use-signed-url.ts | 65 +++++++++++++++---- .../appkit/src/connectors/files/client.ts | 4 +- packages/appkit/src/plugins/files/plugin.ts | 15 +++-- .../src/plugins/files/tests/plugin.test.ts | 21 +++--- 5 files changed, 84 insertions(+), 32 deletions(-) diff --git a/apps/dev-playground/client/src/routes/files.route.tsx b/apps/dev-playground/client/src/routes/files.route.tsx index 646a87ce..c00478be 100644 --- a/apps/dev-playground/client/src/routes/files.route.tsx +++ b/apps/dev-playground/client/src/routes/files.route.tsx @@ -6,6 +6,7 @@ import { FileBreadcrumb, FilePreviewPanel, NewFolderInput, + useSignedUrl, } from "@databricks/appkit-ui/react"; import { createFileRoute, retainSearchParams } from "@tanstack/react-router"; import { FolderPlus, Link, Loader2, Upload } from "lucide-react"; @@ -28,7 +29,6 @@ function nextSignal(ref: RefObject): AbortSignal { return ref.current.signal; } -import { useSignedUrl } from "@databricks/appkit-ui/react"; import { Header } from "@/components/layout/header"; export const Route = createFileRoute("/files")({ @@ -78,6 +78,7 @@ function FilesRoute() { loading: signedUrlLoading, error: signedUrlError, copied, + expired: signedUrlExpired, generate: generateSignedUrl, copyToClipboard, reset: resetSignedUrl, @@ -462,8 +463,12 @@ function FilesRoute() { {copied ? "Copied!" : "Copy"} -

- Expires: {new Date(signedUrl.expiresAt).toLocaleString()} +

+ {signedUrlExpired + ? "Expired — generate a new URL" + : `Expires: ${new Date(signedUrl.expiresAt).toLocaleString()}`}

)} diff --git a/packages/appkit-ui/src/react/hooks/use-signed-url.ts b/packages/appkit-ui/src/react/hooks/use-signed-url.ts index 1073d838..57b69d0f 100644 --- a/packages/appkit-ui/src/react/hooks/use-signed-url.ts +++ b/packages/appkit-ui/src/react/hooks/use-signed-url.ts @@ -1,4 +1,4 @@ -import { useCallback, useRef, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; // ============================================================================ // Types @@ -14,6 +14,8 @@ export interface UseSignedUrlResult { loading: boolean; error: string | null; copied: boolean; + /** Whether the signed URL has expired based on `expiresAt`. */ + expired: boolean; generate: (filePath: string) => Promise; copyToClipboard: () => Promise; reset: () => void; @@ -30,7 +32,34 @@ export function useSignedUrl( const [loading, setLoading] = useState(false); const [error, setError] = useState(null); const [copied, setCopied] = useState(false); + const [expired, setExpired] = useState(false); const copiedTimer = useRef>(undefined); + const expiryTimer = useRef>(undefined); + + useEffect(() => { + return () => { + clearTimeout(copiedTimer.current); + clearTimeout(expiryTimer.current); + }; + }, []); + + // Schedule expiry transition when a signed URL is set + useEffect(() => { + clearTimeout(expiryTimer.current); + if (!signedUrl) { + setExpired(false); + return; + } + + const msUntilExpiry = new Date(signedUrl.expiresAt).getTime() - Date.now(); + if (msUntilExpiry <= 0) { + setExpired(true); + return; + } + + setExpired(false); + expiryTimer.current = setTimeout(() => setExpired(true), msUntilExpiry); + }, [signedUrl]); const reset = useCallback(() => { setSignedUrl(null); @@ -45,9 +74,11 @@ export function useSignedUrl( setSignedUrl(null); try { - const response = await fetch( - apiUrl("download-url", { path: filePath }), - ); + const response = await fetch(apiUrl("download-url"), { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ path: filePath }), + }); if (!response.ok) { const data = await response.json().catch(() => ({})); @@ -70,17 +101,24 @@ export function useSignedUrl( const copyToClipboard = useCallback(async () => { if (!signedUrl) return; - try { + // navigator.clipboard requires a secure context (HTTPS). + // Fall back to Selection API for http:// (e.g. local dev). + if (navigator.clipboard && globalThis.isSecureContext) { await navigator.clipboard.writeText(signedUrl.url); - } catch { - const textarea = document.createElement("textarea"); - textarea.value = signedUrl.url; - textarea.style.position = "fixed"; - textarea.style.opacity = "0"; - document.body.appendChild(textarea); - textarea.select(); + } else { + const range = document.createRange(); + const span = document.createElement("span"); + span.textContent = signedUrl.url; + span.style.position = "fixed"; + span.style.opacity = "0"; + document.body.appendChild(span); + range.selectNodeContents(span); + const selection = getSelection(); + selection?.removeAllRanges(); + selection?.addRange(range); document.execCommand("copy"); - document.body.removeChild(textarea); + selection?.removeAllRanges(); + document.body.removeChild(span); } setCopied(true); @@ -93,6 +131,7 @@ export function useSignedUrl( loading, error, copied, + expired, generate, copyToClipboard, reset, diff --git a/packages/appkit/src/connectors/files/client.ts b/packages/appkit/src/connectors/files/client.ts index e62e2f8a..8dd7bd01 100644 --- a/packages/appkit/src/connectors/files/client.ts +++ b/packages/appkit/src/connectors/files/client.ts @@ -253,13 +253,13 @@ export class FilesConnector { ? hostValue : `https://${hostValue}`; - const expiresAt = new Date( + const requestExpireTime = new Date( Date.now() + expireInSeconds * 1000, ).toISOString(); const url = new URL("/api/2.0/fs/create-download-url", host); url.searchParams.set("path", resolvedPath); - url.searchParams.set("expire_time", expiresAt); + url.searchParams.set("expire_time", requestExpireTime); const headers = new Headers({ "Content-Type": "application/json", diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 29da3359..f2aa1ae2 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -273,7 +273,7 @@ export class FilesPlugin extends Plugin { this.route(router, { name: "download-url", - method: "get", + method: "post", path: "/:volumeKey/download-url", handler: async (req: express.Request, res: express.Response) => { const { connector, volumeKey } = this._resolveVolume(req, res); @@ -543,17 +543,20 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { - const path = req.query.path as string; + const path = + typeof req.body?.path === "string" + ? req.body.path + : (req.query.path as string | undefined); const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } - const rawExpire = req.query.expireInSeconds as string | undefined; - const expireInSeconds = rawExpire - ? Number.parseInt(rawExpire, 10) - : undefined; + const rawExpire = + req.body?.expireInSeconds ?? req.query.expireInSeconds ?? undefined; + const expireInSeconds = + rawExpire != null ? Number.parseInt(String(rawExpire), 10) : undefined; if ( expireInSeconds !== undefined && (Number.isNaN(expireInSeconds) || diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 5851a000..9d4d0907 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -265,6 +265,7 @@ describe("FilesPlugin", () => { "list", "read", "download", + "createDownloadUrl", "exists", "metadata", "upload", @@ -343,11 +344,11 @@ describe("FilesPlugin", () => { plugin.injectRoutes(mockRouter); - // 1 GET /volumes + 8 GET /:volumeKey/* routes - // (list, read, download, raw, download-url, exists, metadata, preview) - expect(mockRouter.get).toHaveBeenCalledTimes(9); - // 2 POST /:volumeKey/* routes (upload, mkdir) - expect(mockRouter.post).toHaveBeenCalledTimes(2); + // 1 GET /volumes + 7 GET /:volumeKey/* routes + // (list, read, download, raw, exists, metadata, preview) + expect(mockRouter.get).toHaveBeenCalledTimes(8); + // 3 POST /:volumeKey/* routes (upload, mkdir, download-url) + expect(mockRouter.post).toHaveBeenCalledTimes(3); // 1 DELETE /:volumeKey route expect(mockRouter.delete).toHaveBeenCalledTimes(1); expect(mockRouter.put).not.toHaveBeenCalled(); @@ -437,7 +438,7 @@ describe("FilesPlugin", () => { plugin.injectRoutes(mockRouter); - const call = mockRouter.get.mock.calls.find( + const call = mockRouter.post.mock.calls.find( (c: unknown[]) => typeof c[0] === "string" && (c[0] as string).endsWith("/download-url"), @@ -453,14 +454,18 @@ describe("FilesPlugin", () => { return res; } - function mockReq(volumeKey: string, query: Record = {}) { + function mockReq( + volumeKey: string, + body: Record = {}, + ) { const headers: Record = { "x-forwarded-access-token": "test-token", "x-forwarded-user": "test-user", }; return { params: { volumeKey }, - query, + query: {}, + body, headers, header: (name: string) => headers[name.toLowerCase()], };