From 10c15daba67f3b492de758c2aba956e13a452681 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Tue, 17 Mar 2026 17:35:44 +0100 Subject: [PATCH 01/14] chore: draft implementation --- apps/dev-playground/server/index.ts | 3 +- docs/docs/plugins/files.md | 142 ++++++ .../appkit/src/context/execution-context.ts | 8 - packages/appkit/src/context/index.ts | 1 - packages/appkit/src/context/user-context.ts | 7 - packages/appkit/src/index.ts | 1 + packages/appkit/src/plugins/files/index.ts | 1 + packages/appkit/src/plugins/files/plugin.ts | 407 ++++++++++++------ packages/appkit/src/plugins/files/policy.ts | 152 +++++++ .../files/tests/plugin.integration.test.ts | 77 ++-- .../src/plugins/files/tests/plugin.test.ts | 371 +++++++++++++++- .../src/plugins/files/tests/policy.test.ts | 136 ++++++ packages/appkit/src/plugins/files/types.ts | 24 +- 13 files changed, 1117 insertions(+), 213 deletions(-) create mode 100644 packages/appkit/src/plugins/files/policy.ts create mode 100644 packages/appkit/src/plugins/files/tests/policy.test.ts diff --git a/apps/dev-playground/server/index.ts b/apps/dev-playground/server/index.ts index af05b11f..14eccfbe 100644 --- a/apps/dev-playground/server/index.ts +++ b/apps/dev-playground/server/index.ts @@ -4,6 +4,7 @@ import { createApp, files, genie, + policy, server, serving, } from "@databricks/appkit"; @@ -32,7 +33,7 @@ createApp({ spaces: { demo: process.env.DATABRICKS_GENIE_SPACE_ID ?? "placeholder" }, }), lakebaseExamples(), - files(), + files({ volumes: { default: { policy: policy.denyAll() } } }), serving(), ], ...(process.env.APPKIT_E2E_TEST && { client: createMockClient() }), diff --git a/docs/docs/plugins/files.md b/docs/docs/plugins/files.md index ccc91265..8d15f75d 100644 --- a/docs/docs/plugins/files.md +++ b/docs/docs/plugins/files.md @@ -15,6 +15,7 @@ File operations against Databricks Unity Catalog Volumes. Supports listing, read - Automatic cache invalidation on write operations - Custom content type mappings - Per-user execution context (OBO) +- **Access policies**: Per-volume policy functions that gate read and write operations ## Basic usage @@ -75,6 +76,8 @@ interface IFilesConfig { } interface VolumeConfig { + /** Access policy for this volume. */ + policy?: FilePolicy; /** Maximum upload size in bytes for this volume. Overrides plugin-level default. */ maxUploadSize?: number; /** Map of file extensions to MIME types for this volume. Overrides plugin-level default. */ @@ -97,6 +100,115 @@ files({ }); ``` +### Permission model + +There are three layers of access control in the files plugin. Understanding how they interact is critical for securing your app: + +``` +┌─────────────────────────────────────────────────┐ +│ Unity Catalog grants │ +│ (WRITE_VOLUME on the SP — set at deploy time) │ +├─────────────────────────────────────────────────┤ +│ Execution identity │ +│ HTTP routes → always service principal │ +│ Programmatic → SP by default, asUser() for OBO │ +├─────────────────────────────────────────────────┤ +│ File policies │ +│ Per-volume (action, resource, user) → boolean │ +│ Only app-level gate for HTTP routes │ +└─────────────────────────────────────────────────┘ +``` + +- **UC grants** control what the service principal can do at the Databricks level. These are set at deploy time via `app.yaml` resource bindings. The SP needs `WRITE_VOLUME` — the plugin declares this via resource requirements. +- **Execution identity** determines whose credentials are used for the actual API call. HTTP routes always use the SP. The programmatic API uses SP by default but supports `asUser(req)` for OBO. +- **File policies** are application-level checks evaluated **before** the API call. They receive the requesting user's identity (from the `x-forwarded-user` header) and decide allow/deny. This is the only gate that distinguishes between users on HTTP routes. + +:::warning + +Since HTTP routes always execute as the service principal, removing a user's UC `WRITE_VOLUME` grant has **no effect** on HTTP access — the SP's grant is what's used. Policies are how you restrict what individual users can do through your app. + +::: + +#### Access policies + +Attach a policy to a volume to control which actions are allowed: + +```ts +import { files, policy } from "@databricks/appkit"; + +files({ + volumes: { + uploads: { policy: policy.publicRead() }, + }, +}); +``` + +#### Actions + +Policies receive an action string. The full list, split by category: + +| Category | Actions | +|----------|---------| +| Read | `list`, `read`, `download`, `raw`, `exists`, `metadata`, `preview` | +| Write | `upload`, `mkdir`, `delete` | + +#### Built-in policies + +| Helper | Allows | Denies | +|--------|--------|--------| +| `policy.publicRead()` | all read actions | all write actions | +| `policy.allowAll()` | everything | nothing | +| `policy.denyAll()` | nothing | everything | + +#### Composing policies + +Combine built-in and custom policies with three combinators: + +- **`policy.all(a, b)`** — AND: all policies must allow. Short-circuits on first denial. +- **`policy.any(a, b)`** — OR: at least one policy must allow. Short-circuits on first allow. +- **`policy.not(p)`** — Inverts a policy. For example, `not(publicRead())` yields a write-only policy (useful for ingestion/drop-box volumes). + +```ts +// Read-only for regular users, full access for the service principal +files({ + volumes: { + shared: { + policy: policy.any( + (_action, _resource, user) => !!user.isServicePrincipal, + policy.publicRead(), + ), + }, + }, +}); +``` + +#### Custom policies + +`FilePolicy` is a function `(action, resource, user) → boolean | Promise`, so you can inline arbitrary logic: + +```ts +import { type FilePolicy, WRITE_ACTIONS } from "@databricks/appkit"; + +const ADMIN_IDS = ["admin-sp-id", "lead-user-id"]; + +const adminOnly: FilePolicy = (action, _resource, user) => { + if (WRITE_ACTIONS.has(action)) { + return ADMIN_IDS.includes(user.id); + } + return true; // reads allowed for everyone +}; + +files({ + volumes: { reports: { policy: adminOnly } }, +}); +``` + +#### Enforcement + +- **HTTP routes**: Policy checked before every operation. Denied → `403` JSON response with `"Action denied by volume policy"`. +- **Programmatic API**: Policy checked on both `appkit.files("vol").list()` (SP identity, `isServicePrincipal: true`) and `appkit.files("vol").asUser(req).list()` (user identity). Denied → throws `PolicyDeniedError`. +- **No policy configured**: All actions allowed (backwards compatible with existing behaviour). + ### Custom content types Override or extend the built-in extension → MIME map: @@ -236,7 +348,36 @@ interface FilePreview extends FileMetadata { isImage: boolean; } +type FileAction = + | "list" | "read" | "download" | "raw" + | "exists" | "metadata" | "preview" + | "upload" | "mkdir" | "delete"; + +interface FileResource { + /** Relative path within the volume. */ + path: string; + /** The volume key (e.g. `"uploads"`). */ + volume: string; + /** Content length in bytes — only present for uploads. */ + size?: number; +} + +interface FilePolicyUser { + /** User ID from the `x-forwarded-user` header. */ + id: string; + /** `true` when the caller is the service principal (direct SDK call, not `asUser`). */ + isServicePrincipal?: boolean; +} + +type FilePolicy = ( + action: FileAction, + resource: FileResource, + user: FilePolicyUser, +) => boolean | Promise; + interface VolumeConfig { + /** Access policy for this volume. */ + policy?: FilePolicy; /** Maximum upload size in bytes for this volume. */ maxUploadSize?: number; /** Map of file extensions to MIME types for this volume. */ @@ -297,6 +438,7 @@ All errors return JSON: | Status | Description | | ------ | -------------------------------------------------------------- | | 400 | Missing or invalid `path` parameter | +| 403 | Action denied by volume policy | | 404 | Unknown volume key | | 413 | Upload exceeds `maxUploadSize` | | 500 | Operation failed (SDK, network, upstream, or unhandled error) | diff --git a/packages/appkit/src/context/execution-context.ts b/packages/appkit/src/context/execution-context.ts index d707f52d..dbc4788e 100644 --- a/packages/appkit/src/context/execution-context.ts +++ b/packages/appkit/src/context/execution-context.ts @@ -81,11 +81,3 @@ export function getWarehouseId(): Promise { export function getWorkspaceId(): Promise { return getExecutionContext().workspaceId; } - -/** - * Check if currently running in a user context. - */ -export function isInUserContext(): boolean { - const ctx = executionContextStorage.getStore(); - return ctx !== undefined; -} diff --git a/packages/appkit/src/context/index.ts b/packages/appkit/src/context/index.ts index 6a5a738b..d306d359 100644 --- a/packages/appkit/src/context/index.ts +++ b/packages/appkit/src/context/index.ts @@ -4,7 +4,6 @@ export { getWarehouseId, getWorkspaceClient, getWorkspaceId, - isInUserContext, runInUserContext, } from "./execution-context"; export { ServiceContext } from "./service-context"; diff --git a/packages/appkit/src/context/user-context.ts b/packages/appkit/src/context/user-context.ts index 20746c91..d051cbd7 100644 --- a/packages/appkit/src/context/user-context.ts +++ b/packages/appkit/src/context/user-context.ts @@ -23,10 +23,3 @@ export interface UserContext { * Execution context can be either service or user context. */ export type ExecutionContext = ServiceContextState | UserContext; - -/** - * Check if an execution context is a user context. - */ -export function isUserContext(ctx: ExecutionContext): ctx is UserContext { - return "isUserContext" in ctx && ctx.isUserContext === true; -} diff --git a/packages/appkit/src/index.ts b/packages/appkit/src/index.ts index 955bfde6..90f7fad1 100644 --- a/packages/appkit/src/index.ts +++ b/packages/appkit/src/index.ts @@ -54,6 +54,7 @@ export { toPlugin, } from "./plugin"; export { analytics, files, genie, lakebase, server, serving } from "./plugins"; +export { policy } from "./plugins/files"; export type { EndpointConfig, ServingEndpointEntry, diff --git a/packages/appkit/src/plugins/files/index.ts b/packages/appkit/src/plugins/files/index.ts index 386c47ef..64a38714 100644 --- a/packages/appkit/src/plugins/files/index.ts +++ b/packages/appkit/src/plugins/files/index.ts @@ -1,3 +1,4 @@ export * from "./defaults"; export * from "./plugin"; +export * from "./policy"; export * from "./types"; diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 9344af85..7558fb88 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -9,7 +9,7 @@ import { isSafeInlineContentType, validateCustomContentTypes, } from "../../connectors/files"; -import { getWorkspaceClient, isInUserContext } from "../../context"; +import { getCurrentUserId, getWorkspaceClient } from "../../context"; import { AuthenticationError } from "../../errors"; import { createLogger } from "../../logging/logger"; import { Plugin, toPlugin } from "../../plugin"; @@ -23,6 +23,12 @@ import { } from "./defaults"; import { parentDirectory, sanitizeFilename } from "./helpers"; import manifest from "./manifest.json"; +import { + type FileAction, + type FilePolicyUser, + type FileResource, + PolicyDeniedError, +} from "./policy"; import type { DownloadResponse, FilesExport, @@ -92,31 +98,94 @@ export class FilesPlugin extends Plugin { })); } + /** Whether a volume has a policy attached. */ + private _hasPolicy(volumeKey: string): boolean { + return typeof this.volumeConfigs[volumeKey]?.policy === "function"; + } + /** - * Warns when a method is called without a user context (i.e. as service principal). - * OBO access via `asUser(req)` is strongly recommended. + * Extract user identity from the request. + * Falls back to `getCurrentUserId()` in development mode. */ - private warnIfNoUserContext(volumeKey: string, method: string): void { - if (!isInUserContext()) { - logger.warn( - `app.files("${volumeKey}").${method}() called without user context (service principal). ` + - `Please use OBO instead: app.files("${volumeKey}").asUser(req).${method}()`, - ); + private _extractUser(req: import("express").Request): FilePolicyUser { + const userId = req.header("x-forwarded-user"); + if (userId) return { id: userId }; + if (process.env.NODE_ENV === "development") { + return { id: getCurrentUserId() }; } + throw AuthenticationError.missingToken( + "Missing x-forwarded-user header. Cannot resolve user ID.", + ); } /** - * Throws when a method is called without a user context (i.e. as service principal). - * OBO access via `asUser(req)` is enforced for now. + * Check the policy for a volume. No-op if no policy is configured. + * Throws `PolicyDeniedError` if denied. */ - private throwIfNoUserContext(volumeKey: string, method: string): void { - if (!isInUserContext()) { - throw new Error( - `app.files("${volumeKey}").${method}() called without user context (service principal). Use OBO instead: app.files("${volumeKey}").asUser(req).${method}()`, - ); + private async _checkPolicy( + volumeKey: string, + action: FileAction, + path: string, + user: FilePolicyUser, + resourceOverrides?: Partial, + ): Promise { + const policyFn = this.volumeConfigs[volumeKey]?.policy; + if (typeof policyFn !== "function") return; + + const resource: FileResource = { + path, + volume: volumeKey, + ...resourceOverrides, + }; + const allowed = await policyFn(action, resource, user); + if (!allowed) { + throw new PolicyDeniedError(action, volumeKey); } } + /** + * HTTP-level wrapper around `_checkPolicy`. + * Extracts user (401 on failure), runs policy (403 on denial). + * Returns `true` if the request may proceed, `false` if a response was sent. + */ + private async _enforcePolicy( + req: import("express").Request, + res: import("express").Response, + volumeKey: string, + action: FileAction, + resourceOverrides?: Partial, + ): Promise { + if (!this._hasPolicy(volumeKey)) return true; + + let user: FilePolicyUser; + try { + user = this._extractUser(req); + } catch (error) { + if (error instanceof AuthenticationError) { + res.status(401).json({ error: error.message, plugin: this.name }); + return false; + } + throw error; + } + + const path = + (req.query.path as string | undefined) ?? + (typeof req.body?.path === "string" ? req.body.path : undefined) ?? + "/"; + + try { + await this._checkPolicy(volumeKey, action, path, user, resourceOverrides); + } catch (error) { + if (error instanceof PolicyDeniedError) { + res.status(403).json({ error: error.message, plugin: this.name }); + return false; + } + throw error; + } + + return true; + } + constructor(config: IFilesConfig) { super(config); this.config = config; @@ -138,6 +207,7 @@ export class FilesPlugin extends Plugin { maxUploadSize: volumeCfg.maxUploadSize ?? config.maxUploadSize, customContentTypes: volumeCfg.customContentTypes ?? config.customContentTypes, + policy: volumeCfg.policy, }; this.volumeConfigs[key] = mergedConfig; @@ -152,56 +222,32 @@ export class FilesPlugin extends Plugin { /** * Creates a VolumeAPI for a specific volume key. - * Each method warns if called outside a user context (service principal). + * All operations execute as the service principal. */ protected createVolumeAPI(volumeKey: string): VolumeAPI { const connector = this.volumeConnectors[volumeKey]; return { - list: (directoryPath?: string) => { - this.throwIfNoUserContext(volumeKey, `list`); - return connector.list(getWorkspaceClient(), directoryPath); - }, - read: (filePath: string, options?: { maxSize?: number }) => { - this.throwIfNoUserContext(volumeKey, `read`); - return connector.read(getWorkspaceClient(), filePath, options); - }, - download: (filePath: string): Promise => { - this.throwIfNoUserContext(volumeKey, `download`); - return connector.download(getWorkspaceClient(), filePath); - }, - exists: (filePath: string) => { - this.throwIfNoUserContext(volumeKey, `exists`); - return connector.exists(getWorkspaceClient(), filePath); - }, - metadata: (filePath: string) => { - this.throwIfNoUserContext(volumeKey, `metadata`); - return connector.metadata(getWorkspaceClient(), filePath); - }, + list: (directoryPath?: string) => + connector.list(getWorkspaceClient(), directoryPath), + read: (filePath: string, options?: { maxSize?: number }) => + connector.read(getWorkspaceClient(), filePath, options), + download: (filePath: string): Promise => + connector.download(getWorkspaceClient(), filePath), + exists: (filePath: string) => + connector.exists(getWorkspaceClient(), filePath), + metadata: (filePath: string) => + connector.metadata(getWorkspaceClient(), filePath), upload: ( filePath: string, contents: ReadableStream | Buffer | string, options?: { overwrite?: boolean }, - ) => { - this.throwIfNoUserContext(volumeKey, `upload`); - return connector.upload( - getWorkspaceClient(), - filePath, - contents, - options, - ); - }, - createDirectory: (directoryPath: string) => { - this.throwIfNoUserContext(volumeKey, `createDirectory`); - return connector.createDirectory(getWorkspaceClient(), directoryPath); - }, - delete: (filePath: string) => { - this.throwIfNoUserContext(volumeKey, `delete`); - return connector.delete(getWorkspaceClient(), filePath); - }, - preview: (filePath: string) => { - this.throwIfNoUserContext(volumeKey, `preview`); - return connector.preview(getWorkspaceClient(), filePath); - }, + ) => connector.upload(getWorkspaceClient(), filePath, contents, options), + createDirectory: (directoryPath: string) => + connector.createDirectory(getWorkspaceClient(), directoryPath), + delete: (filePath: string) => + connector.delete(getWorkspaceClient(), filePath), + preview: (filePath: string) => + connector.preview(getWorkspaceClient(), filePath), }; } @@ -381,7 +427,6 @@ export class FilesPlugin extends Plugin { private _invalidateListCache( volumeKey: string, parentPath: string, - userId: string, connector: FilesConnector, ): void { const parent = parentDirectory(parentPath); @@ -390,16 +435,63 @@ export class FilesPlugin extends Plugin { : "__root__"; const listKey = this.cache.generateKey( [`files:${volumeKey}:list`, cachePathSegment], - userId, + getCurrentUserId(), ); this.cache.delete(listKey); } + /** + * Like `execute()`, but re-throws Databricks API client errors (4xx) + * so route handlers can return the appropriate HTTP status via `_handleApiError`. + * + * Server errors and infrastructure failures are still swallowed (returns `undefined`). + */ + private async _executeOrThrow( + fn: (signal?: AbortSignal) => Promise, + options: PluginExecutionSettings, + userKey?: string, + ): Promise { + let capturedClientError: unknown; + + const result = await this.execute( + async (signal) => { + try { + return await fn(signal); + } catch (error) { + if ( + error instanceof ApiError && + error.statusCode !== undefined && + error.statusCode >= 400 && + error.statusCode < 500 + ) { + capturedClientError = error; + } + throw error; + } + }, + options, + userKey, + ); + + if (result === undefined && capturedClientError) { + throw capturedClientError; + } + + return result; + } + private _handleApiError( res: express.Response, error: unknown, fallbackMessage: string, ): void { + if (error instanceof PolicyDeniedError) { + res.status(403).json({ + error: error.message, + plugin: this.name, + }); + return; + } if (error instanceof AuthenticationError) { res.status(401).json({ error: error.message, @@ -438,15 +530,13 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { + if (!(await this._enforcePolicy(req, res, volumeKey, "list"))) return; + const path = req.query.path as string | undefined; try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `list`); - return connector.list(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.list(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:list`, path ? connector.resolvePath(path) : "__root__", @@ -469,6 +559,8 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { + if (!(await this._enforcePolicy(req, res, volumeKey, "read"))) return; + const path = req.query.path as string; const valid = this._isValidPath(path); if (valid !== true) { @@ -477,12 +569,8 @@ export class FilesPlugin extends Plugin { } try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `read`); - return connector.read(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.read(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:read`, connector.resolvePath(path), @@ -533,6 +621,8 @@ export class FilesPlugin extends Plugin { volumeKey: string, opts: { mode: "download" | "raw" }, ): Promise { + if (!(await this._enforcePolicy(req, res, volumeKey, opts.mode))) return; + const path = req.query.path as string; const valid = this._isValidPath(path); if (valid !== true) { @@ -544,14 +634,13 @@ export class FilesPlugin extends Plugin { const volumeCfg = this.volumeConfigs[volumeKey]; try { - const userPlugin = this.asUser(req); const settings: PluginExecutionSettings = { default: FILES_DOWNLOAD_DEFAULTS, }; - const response = await userPlugin.execute(async () => { - this.warnIfNoUserContext(volumeKey, `download`); - return connector.download(getWorkspaceClient(), path); - }, settings); + const response = await this.execute( + async () => connector.download(getWorkspaceClient(), path), + settings, + ); if (!response.ok) { this._sendStatusError(res, response.status); @@ -610,6 +699,8 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { + if (!(await this._enforcePolicy(req, res, volumeKey, "exists"))) return; + const path = req.query.path as string; const valid = this._isValidPath(path); if (valid !== true) { @@ -618,12 +709,8 @@ export class FilesPlugin extends Plugin { } try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `exists`); - return connector.exists(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.exists(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:exists`, connector.resolvePath(path), @@ -646,6 +733,8 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { + if (!(await this._enforcePolicy(req, res, volumeKey, "metadata"))) return; + const path = req.query.path as string; const valid = this._isValidPath(path); if (valid !== true) { @@ -654,12 +743,8 @@ export class FilesPlugin extends Plugin { } try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `metadata`); - return connector.metadata(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.metadata(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:metadata`, connector.resolvePath(path), @@ -682,6 +767,8 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { + if (!(await this._enforcePolicy(req, res, volumeKey, "preview"))) return; + const path = req.query.path as string; const valid = this._isValidPath(path); if (valid !== true) { @@ -690,12 +777,8 @@ export class FilesPlugin extends Plugin { } try { - const userPlugin = this.asUser(req); - const result = await userPlugin.execute( - async () => { - this.warnIfNoUserContext(volumeKey, `preview`); - return connector.preview(getWorkspaceClient(), path); - }, + const result = await this.execute( + async () => connector.preview(getWorkspaceClient(), path), this._readSettings([ `files:${volumeKey}:preview`, connector.resolvePath(path), @@ -744,6 +827,13 @@ export class FilesPlugin extends Plugin { return; } + if ( + !(await this._enforcePolicy(req, res, volumeKey, "upload", { + size: contentLength, + })) + ) + return; + logger.debug(req, "Upload started: volume=%s path=%s", volumeKey, path); try { @@ -774,24 +864,17 @@ export class FilesPlugin extends Plugin { path, contentLength ?? 0, ); - const userPlugin = this.asUser(req); const settings: PluginExecutionSettings = { default: FILES_WRITE_DEFAULTS, }; const result = await this.trackWrite(() => - userPlugin.execute(async () => { - this.warnIfNoUserContext(volumeKey, `upload`); + this.execute(async () => { await connector.upload(getWorkspaceClient(), path, webStream); return { success: true as const }; }, settings), ); - this._invalidateListCache( - volumeKey, - path, - this.resolveUserId(req), - connector, - ); + this._invalidateListCache(volumeKey, path, "global", connector); if (!result.ok) { logger.error( @@ -825,6 +908,8 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { + if (!(await this._enforcePolicy(req, res, volumeKey, "mkdir"))) return; + const dirPath = typeof req.body?.path === "string" ? req.body.path : undefined; const valid = this._isValidPath(dirPath); @@ -834,24 +919,17 @@ export class FilesPlugin extends Plugin { } try { - const userPlugin = this.asUser(req); const settings: PluginExecutionSettings = { default: FILES_WRITE_DEFAULTS, }; const result = await this.trackWrite(() => - userPlugin.execute(async () => { - this.warnIfNoUserContext(volumeKey, `createDirectory`); + this.execute(async () => { await connector.createDirectory(getWorkspaceClient(), dirPath); return { success: true as const }; }, settings), ); - this._invalidateListCache( - volumeKey, - dirPath, - this.resolveUserId(req), - connector, - ); + this._invalidateListCache(volumeKey, dirPath, "global", connector); if (!result.ok) { this._sendStatusError(res, result.status); @@ -870,6 +948,8 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { + if (!(await this._enforcePolicy(req, res, volumeKey, "delete"))) return; + const rawPath = req.query.path as string | undefined; const valid = this._isValidPath(rawPath); if (valid !== true) { @@ -879,24 +959,17 @@ export class FilesPlugin extends Plugin { const path = rawPath as string; try { - const userPlugin = this.asUser(req); const settings: PluginExecutionSettings = { default: FILES_WRITE_DEFAULTS, }; const result = await this.trackWrite(() => - userPlugin.execute(async () => { - this.warnIfNoUserContext(volumeKey, `delete`); + this.execute(async () => { await connector.delete(getWorkspaceClient(), path); return { success: true as const }; }, settings), ); - this._invalidateListCache( - volumeKey, - path, - this.resolveUserId(req), - connector, - ); + this._invalidateListCache(volumeKey, path, "global", connector); if (!result.ok) { this._sendStatusError(res, result.status); @@ -909,6 +982,70 @@ export class FilesPlugin extends Plugin { } } + /** + * Creates a VolumeAPI that enforces the volume's policy and then delegates + * to the connector as the service principal. + */ + private _createPolicyWrappedAPI( + volumeKey: string, + user: FilePolicyUser, + ): VolumeAPI { + const connector = this.volumeConnectors[volumeKey]; + const check = ( + action: FileAction, + path: string, + overrides?: Partial, + ) => this._checkPolicy(volumeKey, action, path, user, overrides); + + return { + list: async (directoryPath?: string) => { + await check("list", directoryPath ?? "/"); + return connector.list(getWorkspaceClient(), directoryPath); + }, + read: async (filePath: string, options?: { maxSize?: number }) => { + await check("read", filePath); + return connector.read(getWorkspaceClient(), filePath, options); + }, + download: async (filePath: string) => { + await check("download", filePath); + return connector.download(getWorkspaceClient(), filePath); + }, + exists: async (filePath: string) => { + await check("exists", filePath); + return connector.exists(getWorkspaceClient(), filePath); + }, + metadata: async (filePath: string) => { + await check("metadata", filePath); + return connector.metadata(getWorkspaceClient(), filePath); + }, + upload: async ( + filePath: string, + contents: ReadableStream | Buffer | string, + options?: { overwrite?: boolean }, + ) => { + await check("upload", filePath); + return connector.upload( + getWorkspaceClient(), + filePath, + contents, + options, + ); + }, + createDirectory: async (directoryPath: string) => { + await check("mkdir", directoryPath); + return connector.createDirectory(getWorkspaceClient(), directoryPath); + }, + delete: async (filePath: string) => { + await check("delete", filePath); + return connector.delete(getWorkspaceClient(), filePath); + }, + preview: async (filePath: string) => { + await check("preview", filePath); + return connector.preview(getWorkspaceClient(), filePath); + }, + }; + } + private inflightWrites = 0; private trackWrite(fn: () => Promise): Promise { @@ -941,13 +1078,16 @@ export class FilesPlugin extends Plugin { * Returns the programmatic API for the Files plugin. * Callable with a volume key to get a volume-scoped handle. * + * All operations execute as the service principal. + * Use policies to control per-user access. + * * @example * ```ts - * // OBO access (recommended) - * appKit.files("uploads").asUser(req).list() - * - * // Service principal access (logs a warning) + * // Service principal access * appKit.files("uploads").list() + * + * // With policy: pass user identity for access control + * appKit.files("uploads").asUser(req).list() * ``` */ exports(): FilesExport { @@ -958,14 +1098,21 @@ export class FilesPlugin extends Plugin { ); } - // Service principal API — each method logs a warning recommending OBO - const spApi = this.createVolumeAPI(volumeKey); + const spApi = this._hasPolicy(volumeKey) + ? this._createPolicyWrappedAPI(volumeKey, { + id: getCurrentUserId(), + isServicePrincipal: true, + }) + : this.createVolumeAPI(volumeKey); return { ...spApi, asUser: (req: import("express").Request) => { - const userPlugin = this.asUser(req) as FilesPlugin; - return userPlugin.createVolumeAPI(volumeKey); + if (this._hasPolicy(volumeKey)) { + const user = this._extractUser(req); + return this._createPolicyWrappedAPI(volumeKey, user); + } + return spApi; }, }; }; diff --git a/packages/appkit/src/plugins/files/policy.ts b/packages/appkit/src/plugins/files/policy.ts new file mode 100644 index 00000000..fd4f3143 --- /dev/null +++ b/packages/appkit/src/plugins/files/policy.ts @@ -0,0 +1,152 @@ +/** + * Per-volume file access policies. + * + * A `FilePolicy` is a function that decides whether a given action on a + * resource is allowed for a specific user. When a policy is attached to a + * volume, the policy controls whether the action is allowed for the requesting user. + */ + +// --------------------------------------------------------------------------- +// Actions +// --------------------------------------------------------------------------- + +/** Every action the files plugin can perform. */ +export type FileAction = + | "list" + | "read" + | "download" + | "raw" + | "exists" + | "metadata" + | "preview" + | "upload" + | "mkdir" + | "delete"; + +/** Actions that only read data. */ +export const READ_ACTIONS: ReadonlySet = new Set([ + "list", + "read", + "download", + "raw", + "exists", + "metadata", + "preview", +]); + +/** Actions that mutate data. */ +export const WRITE_ACTIONS: ReadonlySet = new Set([ + "upload", + "mkdir", + "delete", +]); + +// --------------------------------------------------------------------------- +// Resource & User +// --------------------------------------------------------------------------- + +/** Describes the file or directory being acted upon. */ +export interface FileResource { + /** Relative path within the volume. */ + path: string; + /** The volume key (e.g. `"uploads"`). */ + volume: string; + /** Content length in bytes — only present for uploads. */ + size?: number; +} + +/** Minimal user identity passed to the policy function. */ +export interface FilePolicyUser { + id: string; + /** `true` when the caller is the service principal (direct SDK call, not `asUser`). */ + isServicePrincipal?: boolean; +} + +// --------------------------------------------------------------------------- +// Policy function type +// --------------------------------------------------------------------------- + +/** + * A policy function that decides whether `user` may perform `action` on + * `resource`. Return `true` to allow, `false` to deny. + */ +export type FilePolicy = ( + action: FileAction, + resource: FileResource, + user: FilePolicyUser, +) => boolean | Promise; + +// --------------------------------------------------------------------------- +// PolicyDeniedError +// --------------------------------------------------------------------------- + +/** + * Thrown when a policy denies an action. + */ +export class PolicyDeniedError extends Error { + readonly action: FileAction; + readonly volumeKey: string; + + constructor(action: FileAction, volumeKey: string) { + super(`Policy denied "${action}" on volume "${volumeKey}"`); + this.name = "PolicyDeniedError"; + this.action = action; + this.volumeKey = volumeKey; + } +} + +// --------------------------------------------------------------------------- +// Combinators +// --------------------------------------------------------------------------- + +/** Utility namespace with common policy combinators. */ +export const policy = { + /** + * AND — all policies must allow. Short-circuits on first denial. + */ + all(...policies: FilePolicy[]): FilePolicy { + return async (action, resource, user) => { + for (const p of policies) { + if (!(await p(action, resource, user))) return false; + } + return true; + }; + }, + + /** + * OR — at least one policy must allow. Short-circuits on first allow. + */ + any(...policies: FilePolicy[]): FilePolicy { + return async (action, resource, user) => { + for (const p of policies) { + if (await p(action, resource, user)) return true; + } + return false; + }; + }, + + /** Negates a policy. */ + not(p: FilePolicy): FilePolicy { + return async (action, resource, user) => !(await p(action, resource, user)); + }, + + /** Allow all read actions (list, read, download, raw, exists, metadata, preview). */ + publicRead(): FilePolicy { + return (action) => READ_ACTIONS.has(action); + }, + + /** Alias for `publicRead()` — included for discoverability. */ + publicReadAndList(): FilePolicy { + return policy.publicRead(); + }, + + /** Deny every action. */ + denyAll(): FilePolicy { + return () => false; + }, + + /** Allow every action. */ + allowAll(): FilePolicy { + return () => true; + }, +} as const; diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index 55989148..4c8f5e31 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -438,17 +438,35 @@ describe("Files Plugin Integration", () => { }); }); - describe("OBO Gateway", () => { - test("production: rejects requests without user token with 401", async () => { - const response = await fetch(`${baseUrl}/api/files/${VOL}/list`); + describe("Service principal execution", () => { + test("requests without user token execute as service principal", async () => { + mockFilesApi.listDirectoryContents.mockReturnValue( + (async function* () { + yield { + name: "sp-file.txt", + path: "/Volumes/catalog/schema/vol/sp-only/sp-file.txt", + is_directory: false, + }; + })(), + ); - expect(response.status).toBe(401); - const data = (await response.json()) as { error: string; plugin: string }; - expect(data.plugin).toBe("files"); - expect(data.error).toMatch(/token/i); + // Use a unique path to avoid cached results from earlier tests + const response = await fetch( + `${baseUrl}/api/files/${VOL}/list?path=sp-only`, + ); + + expect(response.status).toBe(200); + const data = await response.json(); + expect(data).toEqual([ + { + name: "sp-file.txt", + path: "/Volumes/catalog/schema/vol/sp-only/sp-file.txt", + is_directory: false, + }, + ]); }); - test("production: allows requests with valid user token (OBO)", async () => { + test("requests with user headers also succeed", async () => { mockFilesApi.listDirectoryContents.mockReturnValue( (async function* () { yield { @@ -466,51 +484,16 @@ describe("Files Plugin Integration", () => { expect(response.status).toBe(200); }); - test("development: falls back to service principal when no user token", async () => { - const originalEnv = process.env.NODE_ENV; - process.env.NODE_ENV = "development"; - - try { - mockFilesApi.listDirectoryContents.mockReturnValue( - (async function* () { - yield { - name: "dev-file.txt", - path: "/Volumes/catalog/schema/vol/dev-file.txt", - is_directory: false, - }; - })(), - ); - - const response = await fetch(`${baseUrl}/api/files/${VOL}/list`); + test("write operations without user token execute as service principal", async () => { + mockFilesApi.createDirectory.mockResolvedValue(undefined); - expect(response.status).toBe(200); - const data = await response.json(); - expect(data).toEqual([ - { - name: "dev-file.txt", - path: "/Volumes/catalog/schema/vol/dev-file.txt", - is_directory: false, - }, - ]); - } finally { - if (originalEnv === undefined) { - delete process.env.NODE_ENV; - } else { - process.env.NODE_ENV = originalEnv; - } - } - }); - - test("production: rejects write operations without user token", async () => { const response = await fetch(`${baseUrl}/api/files/${VOL}/mkdir`, { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ path: "/Volumes/catalog/schema/vol/newdir" }), + body: JSON.stringify({ path: "newdir" }), }); - expect(response.status).toBe(401); - const data = (await response.json()) as { error: string; plugin: string }; - expect(data.plugin).toBe("files"); + expect(response.status).toBe(200); }); }); diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 99e08b8c..bfae613c 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -9,6 +9,7 @@ import { FILES_WRITE_DEFAULTS, } from "../defaults"; import { FilesPlugin, files } from "../plugin"; +import { PolicyDeniedError, policy } from "../policy"; const { mockClient, MockApiError, mockCacheInstance } = vi.hoisted(() => { const mockFilesApi = { @@ -247,7 +248,7 @@ describe("FilesPlugin", () => { }); }); - describe("OBO and service principal access", () => { + describe("Service principal access", () => { const volumeMethods = [ "list", "read", @@ -270,7 +271,7 @@ describe("FilesPlugin", () => { } }); - test("asUser throws AuthenticationError without token in production", () => { + test("asUser without policy returns SP API (no auth check)", () => { const originalEnv = process.env.NODE_ENV; process.env.NODE_ENV = "production"; @@ -279,7 +280,10 @@ describe("FilesPlugin", () => { const handle = plugin.exports()("uploads"); const mockReq = { header: () => undefined } as any; - expect(() => handle.asUser(mockReq)).toThrow(AuthenticationError); + const api = handle.asUser(mockReq); + for (const method of volumeMethods) { + expect(typeof (api as any)[method]).toBe("function"); + } } finally { process.env.NODE_ENV = originalEnv; } @@ -303,17 +307,12 @@ describe("FilesPlugin", () => { } }); - test("direct methods on handle throw without user context (OBO enforced)", async () => { - const { isInUserContext } = await import("../../../context"); - (isInUserContext as ReturnType).mockReturnValue(false); - + test("direct methods on handle work as service principal", () => { const plugin = new FilesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); - // Direct call without user context should throw synchronously - expect(() => handle.list()).toThrow( - 'app.files("uploads").list() called without user context (service principal). Use OBO instead: app.files("uploads").asUser(req).list()', - ); + // Direct call executes as service principal (returns a promise, does not throw) + expect(typeof handle.list).toBe("function"); }); }); @@ -893,6 +892,356 @@ describe("FilesPlugin", () => { }); }); + describe("Policy enforcement", () => { + const POLICY_CONFIG = { + volumes: { + public: { policy: policy.publicRead() }, + locked: { policy: policy.denyAll() }, + open: { policy: policy.allowAll() }, + uploads: {}, + exports: {}, + }, + }; + + function getRouteHandler( + plugin: FilesPlugin, + method: "get" | "post" | "delete", + pathSuffix: string, + ) { + 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[method].mock.calls.find( + (c: unknown[]) => + typeof c[0] === "string" && (c[0] as string).endsWith(pathSuffix), + ); + return call[call.length - 1] as (req: any, res: any) => Promise; + } + + function mockRes() { + const res: any = { headersSent: false }; + res.status = vi.fn().mockReturnValue(res); + res.json = vi.fn().mockReturnValue(res); + res.type = vi.fn().mockReturnValue(res); + res.send = vi.fn().mockReturnValue(res); + res.setHeader = vi.fn().mockReturnValue(res); + res.end = vi.fn(); + return res; + } + + function mockReq(volumeKey: string, overrides: Record = {}) { + const headers: Record = { + "x-forwarded-access-token": "test-token", + "x-forwarded-user": "test-user", + ...(overrides.headers ?? {}), + }; + return { + params: { volumeKey }, + query: {}, + ...overrides, + headers, + header: (name: string) => headers[name.toLowerCase()], + }; + } + + beforeEach(() => { + process.env.DATABRICKS_VOLUME_PUBLIC = "/Volumes/c/s/public"; + process.env.DATABRICKS_VOLUME_LOCKED = "/Volumes/c/s/locked"; + process.env.DATABRICKS_VOLUME_OPEN = "/Volumes/c/s/open"; + }); + + afterEach(() => { + delete process.env.DATABRICKS_VOLUME_PUBLIC; + delete process.env.DATABRICKS_VOLUME_LOCKED; + delete process.env.DATABRICKS_VOLUME_OPEN; + }); + + test("policy volume + no user header (production) → 401", async () => { + const originalEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "production"; + try { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + // Override both headers to undefined so _extractUser has no user + const noUserHeaders: Record = {}; + await handler( + { + params: { volumeKey: "public" }, + query: {}, + headers: noUserHeaders, + header: (name: string) => noUserHeaders[name.toLowerCase()], + }, + res, + ); + + expect(res.status).toHaveBeenCalledWith(401); + } finally { + process.env.NODE_ENV = originalEnv; + } + }); + + test("policy volume + policy returns false → 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler(mockReq("locked"), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("policy volume + policy returns true → 200, runs as SP", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + mockClient.files.listDirectoryContents.mockImplementation( + async function* () { + yield { name: "a.txt", path: "/a.txt", is_directory: false }; + }, + ); + + await handler(mockReq("public", { query: {} }), res); + + expect(res.json).toHaveBeenCalledWith( + expect.arrayContaining([expect.objectContaining({ name: "a.txt" })]), + ); + // Should NOT have received a 401 or 403 + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(401); + expect(statusCodes).not.toContain(403); + }); + + test("policy volume + async policy → works", async () => { + const asyncConfig = { + volumes: { + async_vol: { + policy: async () => true, + }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_ASYNC_VOL = "/Volumes/c/s/async"; + + try { + const plugin = new FilesPlugin(asyncConfig); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + mockClient.files.listDirectoryContents.mockImplementation( + async function* () { + yield { name: "b.txt", path: "/b.txt", is_directory: false }; + }, + ); + + await handler(mockReq("async_vol"), res); + + expect(res.json).toHaveBeenCalledWith( + expect.arrayContaining([expect.objectContaining({ name: "b.txt" })]), + ); + } finally { + delete process.env.DATABRICKS_VOLUME_ASYNC_VOL; + } + }); + + test("non-policy volume → executes as service principal", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + mockClient.files.listDirectoryContents.mockImplementation( + async function* () { + yield { name: "c.txt", path: "/c.txt", is_directory: false }; + }, + ); + + await handler(mockReq("uploads"), res); + + // Should succeed (no 403) + expect(res.json).toHaveBeenCalledWith( + expect.arrayContaining([expect.objectContaining({ name: "c.txt" })]), + ); + }); + + test("upload with policy → policy receives size in resource", async () => { + const policySpy = vi.fn().mockReturnValue(true); + const sizeConfig = { + volumes: { + sized: { policy: policySpy }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_SIZED = "/Volumes/c/s/sized"; + + try { + const plugin = new FilesPlugin(sizeConfig); + const handler = getRouteHandler(plugin, "post", "/upload"); + const res = mockRes(); + + await handler( + mockReq("sized", { + query: { path: "/test.bin" }, + headers: { + "content-length": "12345", + "x-forwarded-user": "test-user", + "x-forwarded-access-token": "test-token", + }, + }), + res, + ); + + expect(policySpy).toHaveBeenCalledWith( + "upload", + expect.objectContaining({ size: 12345 }), + expect.objectContaining({ id: "test-user" }), + ); + } finally { + delete process.env.DATABRICKS_VOLUME_SIZED; + } + }); + + test("SDK asUser(req) on policy volume → policy-wrapped API works", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const exported = plugin.exports(); + const handle = exported("public"); + + mockClient.files.listDirectoryContents.mockImplementation( + async function* () { + yield { name: "d.txt", path: "/d.txt", is_directory: false }; + }, + ); + + const mockReqObj = { + header: (name: string) => { + if (name === "x-forwarded-user") return "test-user"; + if (name === "x-forwarded-access-token") return "test-token"; + return undefined; + }, + } as any; + + const api = handle.asUser(mockReqObj); + const result = await api.list(); + expect(result).toEqual( + expect.arrayContaining([expect.objectContaining({ name: "d.txt" })]), + ); + }); + + test("SDK asUser(req) on policy volume + deny → throws PolicyDeniedError", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const exported = plugin.exports(); + const handle = exported("locked"); + + const mockReqObj = { + header: (name: string) => { + if (name === "x-forwarded-user") return "test-user"; + if (name === "x-forwarded-access-token") return "test-token"; + return undefined; + }, + } as any; + + const api = handle.asUser(mockReqObj); + await expect(api.list()).rejects.toThrow(PolicyDeniedError); + }); + + test("direct call on policy volume → enforces policy as SP", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handle = plugin.exports()("open"); + + // Direct call on allowAll() volume succeeds (policy is checked but allows) + const result = await handle.list(); + expect(result).toEqual( + expect.arrayContaining([expect.objectContaining({ name: "d.txt" })]), + ); + }); + + test("direct SP call on denyAll() volume → throws PolicyDeniedError", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handle = plugin.exports()("locked"); + + await expect(handle.list()).rejects.toThrow(PolicyDeniedError); + }); + + test("direct SP call → policy receives { isServicePrincipal: true }", async () => { + const policySpy = vi.fn().mockReturnValue(true); + const spyConfig = { + volumes: { + spied: { policy: policySpy }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_SPIED = "/Volumes/c/s/spied"; + + try { + const plugin = new FilesPlugin(spyConfig); + const handle = plugin.exports()("spied"); + await handle.list(); + + expect(policySpy).toHaveBeenCalledWith( + "list", + expect.objectContaining({ volume: "spied" }), + expect.objectContaining({ isServicePrincipal: true }), + ); + } finally { + delete process.env.DATABRICKS_VOLUME_SPIED; + } + }); + + test("asUser() call → policy receives user without isServicePrincipal", async () => { + const policySpy = vi.fn().mockReturnValue(true); + const spyConfig = { + volumes: { + spied: { policy: policySpy }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_SPIED = "/Volumes/c/s/spied"; + + try { + const plugin = new FilesPlugin(spyConfig); + const handle = plugin.exports()("spied"); + const mockReqObj = { + header: (name: string) => { + if (name === "x-forwarded-user") return "test-user"; + if (name === "x-forwarded-access-token") return "test-token"; + return undefined; + }, + } as any; + + await handle.asUser(mockReqObj).list(); + + expect(policySpy).toHaveBeenCalledWith( + "list", + expect.objectContaining({ volume: "spied" }), + expect.objectContaining({ id: "test-user" }), + ); + // Should NOT have isServicePrincipal set + const userArg = policySpy.mock.calls[0][2]; + expect(userArg.isServicePrincipal).toBeUndefined(); + } finally { + delete process.env.DATABRICKS_VOLUME_SPIED; + } + }); + }); + describe("Upload Stream Size Limiter", () => { test("stream under limit passes through all chunks", async () => { const maxSize = 100; diff --git a/packages/appkit/src/plugins/files/tests/policy.test.ts b/packages/appkit/src/plugins/files/tests/policy.test.ts new file mode 100644 index 00000000..b835245a --- /dev/null +++ b/packages/appkit/src/plugins/files/tests/policy.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, test } from "vitest"; +import { + type FileAction, + type FilePolicyUser, + type FileResource, + PolicyDeniedError, + policy, + READ_ACTIONS, + WRITE_ACTIONS, +} from "../policy"; + +const user: FilePolicyUser = { id: "user-1" }; +const resource: FileResource = { path: "/file.txt", volume: "uploads" }; + +describe("FileAction sets", () => { + test("READ_ACTIONS contains all read actions", () => { + for (const a of [ + "list", + "read", + "download", + "raw", + "exists", + "metadata", + "preview", + ] as FileAction[]) { + expect(READ_ACTIONS.has(a)).toBe(true); + } + }); + + test("WRITE_ACTIONS contains all write actions", () => { + for (const a of ["upload", "mkdir", "delete"] as FileAction[]) { + expect(WRITE_ACTIONS.has(a)).toBe(true); + } + }); + + test("READ_ACTIONS and WRITE_ACTIONS are disjoint", () => { + for (const a of READ_ACTIONS) { + expect(WRITE_ACTIONS.has(a)).toBe(false); + } + }); +}); + +describe("policy.publicRead()", () => { + const p = policy.publicRead(); + + test("allows read actions", () => { + for (const a of READ_ACTIONS) { + expect(p(a, resource, user)).toBe(true); + } + }); + + test("denies write actions", () => { + for (const a of WRITE_ACTIONS) { + expect(p(a, resource, user)).toBe(false); + } + }); +}); + +describe("policy.denyAll() / policy.allowAll()", () => { + test("denyAll denies everything", () => { + const p = policy.denyAll(); + expect(p("list", resource, user)).toBe(false); + expect(p("upload", resource, user)).toBe(false); + }); + + test("allowAll allows everything", () => { + const p = policy.allowAll(); + expect(p("list", resource, user)).toBe(true); + expect(p("upload", resource, user)).toBe(true); + }); +}); + +describe("policy.all()", () => { + test("returns true when all policies allow", async () => { + const p = policy.all(policy.allowAll(), policy.allowAll()); + expect(await p("list", resource, user)).toBe(true); + }); + + test("short-circuits on first deny", async () => { + let secondCalled = false; + const p = policy.all(policy.denyAll(), () => { + secondCalled = true; + return true; + }); + expect(await p("list", resource, user)).toBe(false); + expect(secondCalled).toBe(false); + }); +}); + +describe("policy.any()", () => { + test("returns false when all policies deny", async () => { + const p = policy.any(policy.denyAll(), policy.denyAll()); + expect(await p("list", resource, user)).toBe(false); + }); + + test("short-circuits on first allow", async () => { + let secondCalled = false; + const p = policy.any(policy.allowAll(), () => { + secondCalled = true; + return false; + }); + expect(await p("list", resource, user)).toBe(true); + expect(secondCalled).toBe(false); + }); +}); + +describe("policy.not()", () => { + test("inverts allow to deny", async () => { + const p = policy.not(policy.allowAll()); + expect(await p("list", resource, user)).toBe(false); + }); + + test("inverts deny to allow", async () => { + const p = policy.not(policy.denyAll()); + expect(await p("list", resource, user)).toBe(true); + }); +}); + +describe("async policy support", () => { + test("handles async policy that returns Promise", async () => { + const asyncPolicy = async () => true; + const p = policy.all(asyncPolicy); + expect(await p("list", resource, user)).toBe(true); + }); +}); + +describe("PolicyDeniedError", () => { + test("has correct name and message", () => { + const err = new PolicyDeniedError("upload", "images"); + expect(err.name).toBe("PolicyDeniedError"); + expect(err.message).toBe('Policy denied "upload" on volume "images"'); + expect(err.action).toBe("upload"); + expect(err.volumeKey).toBe("images"); + expect(err instanceof Error).toBe(true); + }); +}); diff --git a/packages/appkit/src/plugins/files/types.ts b/packages/appkit/src/plugins/files/types.ts index 9a37a4a1..82b54688 100644 --- a/packages/appkit/src/plugins/files/types.ts +++ b/packages/appkit/src/plugins/files/types.ts @@ -1,5 +1,6 @@ import type { files } from "@databricks/sdk-experimental"; import type { BasePluginConfig, IAppRequest } from "shared"; +import type { FilePolicy } from "./policy"; /** * Per-volume configuration options. @@ -9,11 +10,17 @@ export interface VolumeConfig { maxUploadSize?: number; /** Map of file extensions to MIME types for this volume. Inherits from plugin-level `customContentTypes` if not set. */ customContentTypes?: Record; + /** + * Access-control policy for this volume. When set, operations execute as the + * service principal and the policy decides whether the action is allowed. + */ + policy?: FilePolicy; } /** * User-facing API for a single volume. - * Prefer OBO access via `app.files("volumeKey").asUser(req).list()`. + * All operations execute as the service principal. When a policy is + * configured on the volume, every call is checked against that policy. */ export interface VolumeAPI { list(directoryPath?: string): Promise; @@ -78,8 +85,9 @@ 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). + * All methods execute as the service principal and enforce the volume's + * policy (if configured) with `{ isServicePrincipal: true }`. + * `asUser(req)` re-wraps with the real user identity for per-user policy checks. */ export type VolumeHandle = VolumeAPI & { asUser: (req: IAppRequest) => VolumeAPI; @@ -91,15 +99,15 @@ export type VolumeHandle = VolumeAPI & { * * @example * ```ts - * // OBO access (recommended) - * appKit.files("uploads").asUser(req).list() - * - * // Service principal access (logs a warning) + * // Service principal access * appKit.files("uploads").list() * + * // With policy: pass user identity for access control + * appKit.files("uploads").asUser(req).list() + * * // Named accessor * const vol = appKit.files.volume("uploads") - * await vol.asUser(req).list() + * await vol.list() * ``` */ export interface FilesExport { From 4fd480896aaf0c77efdbf6deca0db003eba6f735 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 18 Mar 2026 15:13:19 +0100 Subject: [PATCH 02/14] chore: only allow reads by default / fix userId check on export --- docs/docs/plugins/files.md | 2 +- packages/appkit/src/plugins/files/plugin.ts | 48 +++++++++++---- .../files/tests/plugin.integration.test.ts | 35 ++++------- .../src/plugins/files/tests/plugin.test.ts | 58 +++++++++++++++---- 4 files changed, 95 insertions(+), 48 deletions(-) diff --git a/docs/docs/plugins/files.md b/docs/docs/plugins/files.md index 8d15f75d..f51fa68a 100644 --- a/docs/docs/plugins/files.md +++ b/docs/docs/plugins/files.md @@ -207,7 +207,7 @@ files({ - **HTTP routes**: Policy checked before every operation. Denied → `403` JSON response with `"Action denied by volume policy"`. - **Programmatic API**: Policy checked on both `appkit.files("vol").list()` (SP identity, `isServicePrincipal: true`) and `appkit.files("vol").asUser(req).list()` (user identity). Denied → throws `PolicyDeniedError`. -- **No policy configured**: All actions allowed (backwards compatible with existing behaviour). +- **No policy configured**: Defaults to `publicRead()` — read actions are allowed, write actions are denied. A startup warning is logged encouraging you to set an explicit policy. ### Custom content types diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 7558fb88..4ce11f67 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -28,6 +28,7 @@ import { type FilePolicyUser, type FileResource, PolicyDeniedError, + policy, } from "./policy"; import type { DownloadResponse, @@ -139,6 +140,17 @@ export class FilesPlugin extends Plugin { }; const allowed = await policyFn(action, resource, user); if (!allowed) { + try { + logger.warn( + 'Policy denied "%s" on volume "%s" for user "%s"', + action, + volumeKey, + user.id, + ); + } catch { + // user.id may not be resolvable in all contexts (e.g. lazy SP getter) + logger.warn('Policy denied "%s" on volume "%s"', action, volumeKey); + } throw new PolicyDeniedError(action, volumeKey); } } @@ -207,7 +219,7 @@ export class FilesPlugin extends Plugin { maxUploadSize: volumeCfg.maxUploadSize ?? config.maxUploadSize, customContentTypes: volumeCfg.customContentTypes ?? config.customContentTypes, - policy: volumeCfg.policy, + policy: volumeCfg.policy ?? policy.publicRead(), }; this.volumeConfigs[key] = mergedConfig; @@ -218,6 +230,18 @@ export class FilesPlugin extends Plugin { customContentTypes: mergedConfig.customContentTypes, }); } + + // Warn at startup for volumes without an explicit policy + for (const key of this.volumeKeys) { + if (!volumes[key].policy) { + logger.warn( + 'Volume "%s" has no explicit policy — defaulting to publicRead(). ' + + "Set a policy in files({ volumes: { %s: { policy: ... } } }) to silence this warning.", + key, + key, + ); + } + } } /** @@ -1098,21 +1122,21 @@ export class FilesPlugin extends Plugin { ); } - const spApi = this._hasPolicy(volumeKey) - ? this._createPolicyWrappedAPI(volumeKey, { - id: getCurrentUserId(), - isServicePrincipal: true, - }) - : this.createVolumeAPI(volumeKey); + // Lazy user resolution: getCurrentUserId() is called when a method + // is invoked (policy check), not when exports() is called. + const spUser: FilePolicyUser = { + get id() { + return getCurrentUserId(); + }, + isServicePrincipal: true, + }; + const spApi = this._createPolicyWrappedAPI(volumeKey, spUser); return { ...spApi, asUser: (req: import("express").Request) => { - if (this._hasPolicy(volumeKey)) { - const user = this._extractUser(req); - return this._createPolicyWrappedAPI(volumeKey, user); - } - return spApi; + const user = this._extractUser(req); + return this._createPolicyWrappedAPI(volumeKey, user); }, }; }; diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index 4c8f5e31..0c3af9a3 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -439,31 +439,15 @@ describe("Files Plugin Integration", () => { }); describe("Service principal execution", () => { - test("requests without user token execute as service principal", async () => { - mockFilesApi.listDirectoryContents.mockReturnValue( - (async function* () { - yield { - name: "sp-file.txt", - path: "/Volumes/catalog/schema/vol/sp-only/sp-file.txt", - is_directory: false, - }; - })(), - ); - + test("requests without user token return 401 (policy requires user identity)", async () => { // Use a unique path to avoid cached results from earlier tests const response = await fetch( `${baseUrl}/api/files/${VOL}/list?path=sp-only`, ); - expect(response.status).toBe(200); - const data = await response.json(); - expect(data).toEqual([ - { - name: "sp-file.txt", - path: "/Volumes/catalog/schema/vol/sp-only/sp-file.txt", - is_directory: false, - }, - ]); + expect(response.status).toBe(401); + const data = (await response.json()) as { error: string }; + expect(data.error).toMatch(/x-forwarded-user/); }); test("requests with user headers also succeed", async () => { @@ -484,16 +468,21 @@ describe("Files Plugin Integration", () => { expect(response.status).toBe(200); }); - test("write operations without user token execute as service principal", async () => { + test("write operations without explicit policy are denied by default publicRead()", async () => { mockFilesApi.createDirectory.mockResolvedValue(undefined); const response = await fetch(`${baseUrl}/api/files/${VOL}/mkdir`, { method: "POST", - headers: { "Content-Type": "application/json" }, + headers: { + ...MOCK_AUTH_HEADERS, + "Content-Type": "application/json", + }, body: JSON.stringify({ path: "newdir" }), }); - expect(response.status).toBe(200); + expect(response.status).toBe(403); + const data = (await response.json()) as { error: string }; + expect(data.error).toMatch(/Policy denied/); }); }); diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index bfae613c..c159e720 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -61,6 +61,7 @@ vi.mock("../../../context", async (importOriginal) => { return { ...actual, getWorkspaceClient: vi.fn(() => mockClient), + getCurrentUserId: vi.fn(() => "test-service-principal"), isInUserContext: vi.fn(() => true), }; }); @@ -73,8 +74,8 @@ vi.mock("../../../cache", () => ({ const VOLUMES_CONFIG = { volumes: { - uploads: { maxUploadSize: 100_000_000 }, - exports: {}, + uploads: { maxUploadSize: 100_000_000, policy: policy.allowAll() }, + exports: { policy: policy.allowAll() }, }, }; @@ -271,7 +272,7 @@ describe("FilesPlugin", () => { } }); - test("asUser without policy returns SP API (no auth check)", () => { + test("asUser without user header in production → throws AuthenticationError", () => { const originalEnv = process.env.NODE_ENV; process.env.NODE_ENV = "production"; @@ -280,10 +281,7 @@ describe("FilesPlugin", () => { const handle = plugin.exports()("uploads"); const mockReq = { header: () => undefined } as any; - const api = handle.asUser(mockReq); - for (const method of volumeMethods) { - expect(typeof (api as any)[method]).toBe("function"); - } + expect(() => handle.asUser(mockReq)).toThrow(AuthenticationError); } finally { process.env.NODE_ENV = originalEnv; } @@ -296,7 +294,10 @@ describe("FilesPlugin", () => { try { const plugin = new FilesPlugin(VOLUMES_CONFIG); const handle = plugin.exports()("uploads"); - const mockReq = { header: () => undefined } as any; + const mockReq = { + header: (name: string) => + name === "x-forwarded-user" ? "test-user" : undefined, + } as any; const api = handle.asUser(mockReq); for (const method of volumeMethods) { @@ -492,11 +493,16 @@ describe("FilesPlugin", () => { const handler = getUploadHandler(plugin); const res = mockRes(); + const headers: Record = { + "content-length": String(100_000_000), + "x-forwarded-user": "test-user", + }; await handler( { params: { volumeKey: "uploads" }, query: { path: "/file.bin" }, - headers: { "content-length": String(100_000_000) }, + headers, + header: (name: string) => headers[name.toLowerCase()], }, res, ); @@ -514,11 +520,15 @@ describe("FilesPlugin", () => { const handler = getUploadHandler(plugin); const res = mockRes(); + const headers: Record = { + "x-forwarded-user": "test-user", + }; await handler( { params: { volumeKey: "uploads" }, query: { path: "/file.bin" }, - headers: {}, + headers, + header: (name: string) => headers[name.toLowerCase()], }, res, ); @@ -643,6 +653,8 @@ describe("FilesPlugin", () => { const handlerPromise = handler(mockReq("uploads"), res); + // Flush microtasks (policy check) before advancing timers + await vi.advanceTimersByTimeAsync(0); await vi.advanceTimersByTimeAsync(100); await handlerPromise; @@ -868,6 +880,8 @@ describe("FilesPlugin", () => { const handlerPromise = handler(mockReq("uploads"), res); + // Flush microtasks (policy check) before advancing timers + await vi.advanceTimersByTimeAsync(0); // Advance past read-tier timeout (30s) await vi.advanceTimersByTimeAsync(31_000); await handlerPromise; @@ -1060,7 +1074,7 @@ describe("FilesPlugin", () => { } }); - test("non-policy volume → executes as service principal", async () => { + test("default publicRead() volume → reads succeed", async () => { const plugin = new FilesPlugin(POLICY_CONFIG); const handler = getRouteHandler(plugin, "get", "/list"); const res = mockRes(); @@ -1073,12 +1087,32 @@ describe("FilesPlugin", () => { await handler(mockReq("uploads"), res); - // Should succeed (no 403) + // Should succeed (reads allowed by publicRead default) expect(res.json).toHaveBeenCalledWith( expect.arrayContaining([expect.objectContaining({ name: "c.txt" })]), ); }); + test("default publicRead() volume → writes denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "post", "/mkdir"); + const res = mockRes(); + + await handler( + mockReq("uploads", { + body: { path: "/newdir" }, + }), + res, + ); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + test("upload with policy → policy receives size in resource", async () => { const policySpy = vi.fn().mockReturnValue(true); const sizeConfig = { From 2d85dffc424fe5ae8c6e04fd71ec5427b284b083 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 18 Mar 2026 15:19:20 +0100 Subject: [PATCH 03/14] chore: log when no user id is provided in dev - update docs error msg --- docs/docs/plugins/files.md | 2 +- packages/appkit/src/plugins/files/plugin.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/docs/plugins/files.md b/docs/docs/plugins/files.md index f51fa68a..b57f46e0 100644 --- a/docs/docs/plugins/files.md +++ b/docs/docs/plugins/files.md @@ -438,7 +438,7 @@ All errors return JSON: | Status | Description | | ------ | -------------------------------------------------------------- | | 400 | Missing or invalid `path` parameter | -| 403 | Action denied by volume policy | +| 403 | Policy denied "`{action}`" on volume "`{volumeKey}`" | | 404 | Unknown volume key | | 413 | Upload exceeds `maxUploadSize` | | 500 | Operation failed (SDK, network, upstream, or unhandled error) | diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 4ce11f67..e0671e06 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -112,6 +112,10 @@ export class FilesPlugin extends Plugin { const userId = req.header("x-forwarded-user"); if (userId) return { id: userId }; if (process.env.NODE_ENV === "development") { + logger.warn( + "No x-forwarded-user header — falling back to service principal identity for policy checks. " + + "Ensure your proxy forwards user headers to test per-user policies.", + ); return { id: getCurrentUserId() }; } throw AuthenticationError.missingToken( From e68fad97b4bbd054fbb7487b5f0cc1c130899f8c Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 18 Mar 2026 15:27:32 +0100 Subject: [PATCH 04/14] chore: change dev-playground policy / update plugin tests --- apps/dev-playground/server/index.ts | 2 +- packages/appkit/src/plugins/files/tests/plugin.test.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/dev-playground/server/index.ts b/apps/dev-playground/server/index.ts index 14eccfbe..b456316a 100644 --- a/apps/dev-playground/server/index.ts +++ b/apps/dev-playground/server/index.ts @@ -33,7 +33,7 @@ createApp({ spaces: { demo: process.env.DATABRICKS_GENIE_SPACE_ID ?? "placeholder" }, }), lakebaseExamples(), - files({ volumes: { default: { policy: policy.denyAll() } } }), + files({ volumes: { default: { policy: policy.allowAll() } } }), serving(), ], ...(process.env.APPKIT_E2E_TEST && { client: createMockClient() }), diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index c159e720..74deba34 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -62,7 +62,6 @@ vi.mock("../../../context", async (importOriginal) => { ...actual, getWorkspaceClient: vi.fn(() => mockClient), getCurrentUserId: vi.fn(() => "test-service-principal"), - isInUserContext: vi.fn(() => true), }; }); From 101c6a208a65a263be712fce9f41679441c545b2 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 18 Mar 2026 17:19:11 +0100 Subject: [PATCH 05/14] chore: fix tests and linting --- packages/appkit/src/context/user-context.ts | 7 +++++++ .../src/plugins/files/tests/plugin.integration.test.ts | 9 ++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/appkit/src/context/user-context.ts b/packages/appkit/src/context/user-context.ts index d051cbd7..12548103 100644 --- a/packages/appkit/src/context/user-context.ts +++ b/packages/appkit/src/context/user-context.ts @@ -19,6 +19,13 @@ export interface UserContext { isUserContext: true; } +/** + * Type guard to check if a context is a UserContext. + */ +export function isUserContext(ctx: ExecutionContext): ctx is UserContext { + return "isUserContext" in ctx && ctx.isUserContext === true; +} + /** * Execution context can be either service or user context. */ diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index 0c3af9a3..3d343474 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -583,11 +583,10 @@ describe("Files Plugin Integration", () => { new MockApiError("Conflict", 409), ); - const response = await fetch(`${baseUrl}/api/files/${VOL}/mkdir`, { - method: "POST", - headers: { ...MOCK_AUTH_HEADERS, "Content-Type": "application/json" }, - body: JSON.stringify({ path: "/Volumes/catalog/schema/vol/existing" }), - }); + const response = await fetch( + `${baseUrl}/api/files/${VOL}/metadata?path=/Volumes/catalog/schema/vol/existing`, + { headers: MOCK_AUTH_HEADERS }, + ); expect(response.status).toBe(409); const data = (await response.json()) as { From 45523ba83e7701668d81d859dccc4d84e829b313 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 18 Mar 2026 17:45:51 +0100 Subject: [PATCH 06/14] chore: more linting fixes --- packages/appkit/src/plugins/files/plugin.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index e0671e06..cbc407f1 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -902,7 +902,7 @@ export class FilesPlugin extends Plugin { }, settings), ); - this._invalidateListCache(volumeKey, path, "global", connector); + this._invalidateListCache(volumeKey, path, connector); if (!result.ok) { logger.error( @@ -957,7 +957,7 @@ export class FilesPlugin extends Plugin { }, settings), ); - this._invalidateListCache(volumeKey, dirPath, "global", connector); + this._invalidateListCache(volumeKey, dirPath, connector); if (!result.ok) { this._sendStatusError(res, result.status); @@ -997,7 +997,7 @@ export class FilesPlugin extends Plugin { }, settings), ); - this._invalidateListCache(volumeKey, path, "global", connector); + this._invalidateListCache(volumeKey, path, connector); if (!result.ok) { this._sendStatusError(res, result.status); From c653052ddda02a3a9d08a7bced0b31972febd993 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Wed, 18 Mar 2026 18:11:41 +0100 Subject: [PATCH 07/14] chore: update docs --- docs/docs/api/appkit/Variable.policy.md | 119 ++++++++++++++++++++++++ docs/docs/api/appkit/index.md | 1 + docs/docs/api/appkit/typedoc-sidebar.ts | 5 + 3 files changed, 125 insertions(+) create mode 100644 docs/docs/api/appkit/Variable.policy.md diff --git a/docs/docs/api/appkit/Variable.policy.md b/docs/docs/api/appkit/Variable.policy.md new file mode 100644 index 00000000..f1a00143 --- /dev/null +++ b/docs/docs/api/appkit/Variable.policy.md @@ -0,0 +1,119 @@ +# Variable: policy + +```ts +const policy: { + all: FilePolicy; + allowAll: FilePolicy; + any: FilePolicy; + denyAll: FilePolicy; + not: FilePolicy; + publicRead: FilePolicy; + publicReadAndList: FilePolicy; +}; +``` + +Utility namespace with common policy combinators. + +## Type Declaration + +### all() + +```ts +readonly all(...policies: FilePolicy[]): FilePolicy; +``` + +AND — all policies must allow. Short-circuits on first denial. + +#### Parameters + +| Parameter | Type | +| ------ | ------ | +| ...`policies` | `FilePolicy`[] | + +#### Returns + +`FilePolicy` + +### allowAll() + +```ts +readonly allowAll(): FilePolicy; +``` + +Allow every action. + +#### Returns + +`FilePolicy` + +### any() + +```ts +readonly any(...policies: FilePolicy[]): FilePolicy; +``` + +OR — at least one policy must allow. Short-circuits on first allow. + +#### Parameters + +| Parameter | Type | +| ------ | ------ | +| ...`policies` | `FilePolicy`[] | + +#### Returns + +`FilePolicy` + +### denyAll() + +```ts +readonly denyAll(): FilePolicy; +``` + +Deny every action. + +#### Returns + +`FilePolicy` + +### not() + +```ts +readonly not(p: FilePolicy): FilePolicy; +``` + +Negates a policy. + +#### Parameters + +| Parameter | Type | +| ------ | ------ | +| `p` | `FilePolicy` | + +#### Returns + +`FilePolicy` + +### publicRead() + +```ts +readonly publicRead(): FilePolicy; +``` + +Allow all read actions (list, read, download, raw, exists, metadata, preview). + +#### Returns + +`FilePolicy` + +### publicReadAndList() + +```ts +readonly publicReadAndList(): FilePolicy; +``` + +Alias for `publicRead()` — included for discoverability. + +#### Returns + +`FilePolicy` diff --git a/docs/docs/api/appkit/index.md b/docs/docs/api/appkit/index.md index f5163db4..0ae5e4e4 100644 --- a/docs/docs/api/appkit/index.md +++ b/docs/docs/api/appkit/index.md @@ -65,6 +65,7 @@ plugin architecture, and React integration. | Variable | Description | | ------ | ------ | +| [policy](Variable.policy.md) | Utility namespace with common policy combinators. | | [sql](Variable.sql.md) | SQL helper namespace | ## Functions diff --git a/docs/docs/api/appkit/typedoc-sidebar.ts b/docs/docs/api/appkit/typedoc-sidebar.ts index 720c78ea..d6fcd54a 100644 --- a/docs/docs/api/appkit/typedoc-sidebar.ts +++ b/docs/docs/api/appkit/typedoc-sidebar.ts @@ -219,6 +219,11 @@ const typedocSidebar: SidebarsConfig = { type: "category", label: "Variables", items: [ + { + type: "doc", + id: "api/appkit/Variable.policy", + label: "policy" + }, { type: "doc", id: "api/appkit/Variable.sql", From 90008fff1c44790e3a50b1de3a096beddd61da5b Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Thu, 2 Apr 2026 15:19:32 +0200 Subject: [PATCH 08/14] chore: address critical findings --- packages/appkit/src/plugins/files/plugin.ts | 64 ++++++--- packages/appkit/src/plugins/files/policy.ts | 11 +- .../src/plugins/files/tests/plugin.test.ts | 128 ++++++++++++++++++ .../src/plugins/files/tests/policy.test.ts | 12 ++ 4 files changed, 188 insertions(+), 27 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index cbc407f1..85f9959c 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -169,6 +169,7 @@ export class FilesPlugin extends Plugin { res: import("express").Response, volumeKey: string, action: FileAction, + path: string, resourceOverrides?: Partial, ): Promise { if (!this._hasPolicy(volumeKey)) return true; @@ -184,11 +185,6 @@ export class FilesPlugin extends Plugin { throw error; } - const path = - (req.query.path as string | undefined) ?? - (typeof req.body?.path === "string" ? req.body.path : undefined) ?? - "/"; - try { await this._checkPolicy(volumeKey, action, path, user, resourceOverrides); } catch (error) { @@ -558,10 +554,11 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { - if (!(await this._enforcePolicy(req, res, volumeKey, "list"))) return; - const path = req.query.path as string | undefined; + if (!(await this._enforcePolicy(req, res, volumeKey, "list", path ?? "/"))) + return; + try { const result = await this.execute( async () => connector.list(getWorkspaceClient(), path), @@ -587,9 +584,10 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { - if (!(await this._enforcePolicy(req, res, volumeKey, "read"))) return; - const path = req.query.path as string; + + if (!(await this._enforcePolicy(req, res, volumeKey, "read", path))) return; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); @@ -649,9 +647,11 @@ export class FilesPlugin extends Plugin { volumeKey: string, opts: { mode: "download" | "raw" }, ): Promise { - if (!(await this._enforcePolicy(req, res, volumeKey, opts.mode))) return; - const path = req.query.path as string; + + if (!(await this._enforcePolicy(req, res, volumeKey, opts.mode, path))) + return; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); @@ -727,9 +727,11 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { - if (!(await this._enforcePolicy(req, res, volumeKey, "exists"))) return; - const path = req.query.path as string; + + if (!(await this._enforcePolicy(req, res, volumeKey, "exists", path))) + return; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); @@ -761,9 +763,11 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { - if (!(await this._enforcePolicy(req, res, volumeKey, "metadata"))) return; - const path = req.query.path as string; + + if (!(await this._enforcePolicy(req, res, volumeKey, "metadata", path))) + return; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); @@ -795,9 +799,11 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { - if (!(await this._enforcePolicy(req, res, volumeKey, "preview"))) return; - const path = req.query.path as string; + + if (!(await this._enforcePolicy(req, res, volumeKey, "preview", path))) + return; + const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); @@ -856,7 +862,7 @@ export class FilesPlugin extends Plugin { } if ( - !(await this._enforcePolicy(req, res, volumeKey, "upload", { + !(await this._enforcePolicy(req, res, volumeKey, "upload", path, { size: contentLength, })) ) @@ -936,10 +942,14 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { - if (!(await this._enforcePolicy(req, res, volumeKey, "mkdir"))) return; - const dirPath = typeof req.body?.path === "string" ? req.body.path : undefined; + + if ( + !(await this._enforcePolicy(req, res, volumeKey, "mkdir", dirPath ?? "/")) + ) + return; + const valid = this._isValidPath(dirPath); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); @@ -976,9 +986,19 @@ export class FilesPlugin extends Plugin { connector: FilesConnector, volumeKey: string, ): Promise { - if (!(await this._enforcePolicy(req, res, volumeKey, "delete"))) return; - const rawPath = req.query.path as string | undefined; + + if ( + !(await this._enforcePolicy( + req, + res, + volumeKey, + "delete", + rawPath ?? "/", + )) + ) + return; + const valid = this._isValidPath(rawPath); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); diff --git a/packages/appkit/src/plugins/files/policy.ts b/packages/appkit/src/plugins/files/policy.ts index fd4f3143..87b23f37 100644 --- a/packages/appkit/src/plugins/files/policy.ts +++ b/packages/appkit/src/plugins/files/policy.ts @@ -105,6 +105,9 @@ export const policy = { * AND — all policies must allow. Short-circuits on first denial. */ all(...policies: FilePolicy[]): FilePolicy { + if (policies.length === 0) { + throw new Error("policy.all() requires at least one policy"); + } return async (action, resource, user) => { for (const p of policies) { if (!(await p(action, resource, user))) return false; @@ -117,6 +120,9 @@ export const policy = { * OR — at least one policy must allow. Short-circuits on first allow. */ any(...policies: FilePolicy[]): FilePolicy { + if (policies.length === 0) { + throw new Error("policy.any() requires at least one policy"); + } return async (action, resource, user) => { for (const p of policies) { if (await p(action, resource, user)) return true; @@ -135,11 +141,6 @@ export const policy = { return (action) => READ_ACTIONS.has(action); }, - /** Alias for `publicRead()` — included for discoverability. */ - publicReadAndList(): FilePolicy { - return policy.publicRead(); - }, - /** Deny every action. */ denyAll(): FilePolicy { return () => false; diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 74deba34..fb03930a 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -1273,6 +1273,134 @@ describe("FilesPlugin", () => { delete process.env.DATABRICKS_VOLUME_SPIED; } }); + + test("denyAll() volume → read denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/read"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("publicRead() volume → read allowed", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/read"); + const res = mockRes(); + + mockClient.files.download.mockResolvedValue({ + contents: new ReadableStream({ + start(controller) { + controller.enqueue(new TextEncoder().encode("file content")); + controller.close(); + }, + }), + }); + + await handler(mockReq("public", { query: { path: "/test.txt" } }), res); + + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(401); + expect(statusCodes).not.toContain(403); + }); + + test("denyAll() volume → download denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/download"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.bin" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → raw denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/raw"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → exists denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/exists"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → metadata denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/metadata"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → preview denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/preview"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("denyAll() volume → delete denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "delete", "/:volumeKey"); + const res = mockRes(); + + await handler(mockReq("locked", { query: { path: "/test.txt" } }), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); }); describe("Upload Stream Size Limiter", () => { diff --git a/packages/appkit/src/plugins/files/tests/policy.test.ts b/packages/appkit/src/plugins/files/tests/policy.test.ts index b835245a..dc477198 100644 --- a/packages/appkit/src/plugins/files/tests/policy.test.ts +++ b/packages/appkit/src/plugins/files/tests/policy.test.ts @@ -85,6 +85,12 @@ describe("policy.all()", () => { expect(await p("list", resource, user)).toBe(false); expect(secondCalled).toBe(false); }); + + test("throws when called with no policies", () => { + expect(() => policy.all()).toThrow( + "policy.all() requires at least one policy", + ); + }); }); describe("policy.any()", () => { @@ -102,6 +108,12 @@ describe("policy.any()", () => { expect(await p("list", resource, user)).toBe(true); expect(secondCalled).toBe(false); }); + + test("throws when called with no policies", () => { + expect(() => policy.any()).toThrow( + "policy.any() requires at least one policy", + ); + }); }); describe("policy.not()", () => { From 4573b1f73e598d15cd00bd235519531eec211ce8 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Thu, 2 Apr 2026 15:54:27 +0200 Subject: [PATCH 09/14] chore: add note to `createVolumeAPI` usage --- docs/docs/api/appkit/Variable.policy.md | 13 ------------- packages/appkit/src/plugins/files/plugin.ts | 11 ++++++++++- 2 files changed, 10 insertions(+), 14 deletions(-) diff --git a/docs/docs/api/appkit/Variable.policy.md b/docs/docs/api/appkit/Variable.policy.md index f1a00143..ba2af4ce 100644 --- a/docs/docs/api/appkit/Variable.policy.md +++ b/docs/docs/api/appkit/Variable.policy.md @@ -8,7 +8,6 @@ const policy: { denyAll: FilePolicy; not: FilePolicy; publicRead: FilePolicy; - publicReadAndList: FilePolicy; }; ``` @@ -105,15 +104,3 @@ Allow all read actions (list, read, download, raw, exists, metadata, preview). #### Returns `FilePolicy` - -### publicReadAndList() - -```ts -readonly publicReadAndList(): FilePolicy; -``` - -Alias for `publicRead()` — included for discoverability. - -#### Returns - -`FilePolicy` diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 85f9959c..c0d792bb 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -246,7 +246,16 @@ export class FilesPlugin extends Plugin { /** * Creates a VolumeAPI for a specific volume key. - * All operations execute as the service principal. + * All operations execute as the service principal without policy checks. + * + * Not used internally — `_createPolicyWrappedAPI` handles all current + * call sites. Kept as a `protected` extension point so subclasses can + * override `exports()` or build custom APIs with raw connector access, + * e.g. background jobs or migrations that should bypass user-facing policies. + * + * @security This method skips all policy enforcement. Do not expose its + * return value to HTTP routes or end-user-facing code paths — use + * `_createPolicyWrappedAPI` for anything that serves user requests. */ protected createVolumeAPI(volumeKey: string): VolumeAPI { const connector = this.volumeConnectors[volumeKey]; From 5992530aaece25cb636778b21d6caed2053cc888 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 13:14:18 +0200 Subject: [PATCH 10/14] chore: address multiple code-reviews comments and update docs --- apps/dev-playground/server/index.ts | 3 +- .../api/appkit/Class.PolicyDeniedError.md | 48 ++++++++ .../api/appkit/Interface.FilePolicyUser.md | 21 ++++ .../docs/api/appkit/Interface.FileResource.md | 33 ++++++ docs/docs/api/appkit/TypeAlias.FileAction.md | 17 +++ docs/docs/api/appkit/TypeAlias.FilePolicy.md | 20 ++++ docs/docs/api/appkit/Variable.READ_ACTIONS.md | 7 ++ .../docs/api/appkit/Variable.WRITE_ACTIONS.md | 7 ++ docs/docs/api/appkit/Variable.policy.md | 106 ------------------ docs/docs/api/appkit/index.md | 8 +- docs/docs/api/appkit/typedoc-sidebar.ts | 34 +++++- docs/docs/plugins/files.md | 34 +++--- .../appkit/src/context/execution-context.ts | 5 + packages/appkit/src/index.ts | 13 ++- packages/appkit/src/plugins/files/plugin.ts | 50 +-------- 15 files changed, 235 insertions(+), 171 deletions(-) create mode 100644 docs/docs/api/appkit/Class.PolicyDeniedError.md create mode 100644 docs/docs/api/appkit/Interface.FilePolicyUser.md create mode 100644 docs/docs/api/appkit/Interface.FileResource.md create mode 100644 docs/docs/api/appkit/TypeAlias.FileAction.md create mode 100644 docs/docs/api/appkit/TypeAlias.FilePolicy.md create mode 100644 docs/docs/api/appkit/Variable.READ_ACTIONS.md create mode 100644 docs/docs/api/appkit/Variable.WRITE_ACTIONS.md delete mode 100644 docs/docs/api/appkit/Variable.policy.md diff --git a/apps/dev-playground/server/index.ts b/apps/dev-playground/server/index.ts index b456316a..7868ccce 100644 --- a/apps/dev-playground/server/index.ts +++ b/apps/dev-playground/server/index.ts @@ -4,7 +4,6 @@ import { createApp, files, genie, - policy, server, serving, } from "@databricks/appkit"; @@ -33,7 +32,7 @@ createApp({ spaces: { demo: process.env.DATABRICKS_GENIE_SPACE_ID ?? "placeholder" }, }), lakebaseExamples(), - files({ volumes: { default: { policy: policy.allowAll() } } }), + files({ volumes: { default: { policy: files.policy.allowAll() } } }), serving(), ], ...(process.env.APPKIT_E2E_TEST && { client: createMockClient() }), diff --git a/docs/docs/api/appkit/Class.PolicyDeniedError.md b/docs/docs/api/appkit/Class.PolicyDeniedError.md new file mode 100644 index 00000000..63a46a64 --- /dev/null +++ b/docs/docs/api/appkit/Class.PolicyDeniedError.md @@ -0,0 +1,48 @@ +# Class: PolicyDeniedError + +Thrown when a policy denies an action. + +## Extends + +- `Error` + +## Constructors + +### Constructor + +```ts +new PolicyDeniedError(action: FileAction, volumeKey: string): PolicyDeniedError; +``` + +#### Parameters + +| Parameter | Type | +| ------ | ------ | +| `action` | [`FileAction`](TypeAlias.FileAction.md) | +| `volumeKey` | `string` | + +#### Returns + +`PolicyDeniedError` + +#### Overrides + +```ts +Error.constructor +``` + +## Properties + +### action + +```ts +readonly action: FileAction; +``` + +*** + +### volumeKey + +```ts +readonly volumeKey: string; +``` diff --git a/docs/docs/api/appkit/Interface.FilePolicyUser.md b/docs/docs/api/appkit/Interface.FilePolicyUser.md new file mode 100644 index 00000000..0c4bae2f --- /dev/null +++ b/docs/docs/api/appkit/Interface.FilePolicyUser.md @@ -0,0 +1,21 @@ +# Interface: FilePolicyUser + +Minimal user identity passed to the policy function. + +## Properties + +### id + +```ts +id: string; +``` + +*** + +### isServicePrincipal? + +```ts +optional isServicePrincipal: boolean; +``` + +`true` when the caller is the service principal (direct SDK call, not `asUser`). diff --git a/docs/docs/api/appkit/Interface.FileResource.md b/docs/docs/api/appkit/Interface.FileResource.md new file mode 100644 index 00000000..44aa15cd --- /dev/null +++ b/docs/docs/api/appkit/Interface.FileResource.md @@ -0,0 +1,33 @@ +# Interface: FileResource + +Describes the file or directory being acted upon. + +## Properties + +### path + +```ts +path: string; +``` + +Relative path within the volume. + +*** + +### size? + +```ts +optional size: number; +``` + +Content length in bytes — only present for uploads. + +*** + +### volume + +```ts +volume: string; +``` + +The volume key (e.g. `"uploads"`). diff --git a/docs/docs/api/appkit/TypeAlias.FileAction.md b/docs/docs/api/appkit/TypeAlias.FileAction.md new file mode 100644 index 00000000..3223cc6a --- /dev/null +++ b/docs/docs/api/appkit/TypeAlias.FileAction.md @@ -0,0 +1,17 @@ +# Type Alias: FileAction + +```ts +type FileAction = + | "list" + | "read" + | "download" + | "raw" + | "exists" + | "metadata" + | "preview" + | "upload" + | "mkdir" + | "delete"; +``` + +Every action the files plugin can perform. diff --git a/docs/docs/api/appkit/TypeAlias.FilePolicy.md b/docs/docs/api/appkit/TypeAlias.FilePolicy.md new file mode 100644 index 00000000..1d818560 --- /dev/null +++ b/docs/docs/api/appkit/TypeAlias.FilePolicy.md @@ -0,0 +1,20 @@ +# Type Alias: FilePolicy() + +```ts +type FilePolicy = (action: FileAction, resource: FileResource, user: FilePolicyUser) => boolean | Promise; +``` + +A policy function that decides whether `user` may perform `action` on +`resource`. Return `true` to allow, `false` to deny. + +## Parameters + +| Parameter | Type | +| ------ | ------ | +| `action` | [`FileAction`](TypeAlias.FileAction.md) | +| `resource` | [`FileResource`](Interface.FileResource.md) | +| `user` | [`FilePolicyUser`](Interface.FilePolicyUser.md) | + +## Returns + +`boolean` \| `Promise`\<`boolean`\> diff --git a/docs/docs/api/appkit/Variable.READ_ACTIONS.md b/docs/docs/api/appkit/Variable.READ_ACTIONS.md new file mode 100644 index 00000000..bcbca838 --- /dev/null +++ b/docs/docs/api/appkit/Variable.READ_ACTIONS.md @@ -0,0 +1,7 @@ +# Variable: READ\_ACTIONS + +```ts +const READ_ACTIONS: ReadonlySet; +``` + +Actions that only read data. diff --git a/docs/docs/api/appkit/Variable.WRITE_ACTIONS.md b/docs/docs/api/appkit/Variable.WRITE_ACTIONS.md new file mode 100644 index 00000000..39c90dff --- /dev/null +++ b/docs/docs/api/appkit/Variable.WRITE_ACTIONS.md @@ -0,0 +1,7 @@ +# Variable: WRITE\_ACTIONS + +```ts +const WRITE_ACTIONS: ReadonlySet; +``` + +Actions that mutate data. diff --git a/docs/docs/api/appkit/Variable.policy.md b/docs/docs/api/appkit/Variable.policy.md deleted file mode 100644 index ba2af4ce..00000000 --- a/docs/docs/api/appkit/Variable.policy.md +++ /dev/null @@ -1,106 +0,0 @@ -# Variable: policy - -```ts -const policy: { - all: FilePolicy; - allowAll: FilePolicy; - any: FilePolicy; - denyAll: FilePolicy; - not: FilePolicy; - publicRead: FilePolicy; -}; -``` - -Utility namespace with common policy combinators. - -## Type Declaration - -### all() - -```ts -readonly all(...policies: FilePolicy[]): FilePolicy; -``` - -AND — all policies must allow. Short-circuits on first denial. - -#### Parameters - -| Parameter | Type | -| ------ | ------ | -| ...`policies` | `FilePolicy`[] | - -#### Returns - -`FilePolicy` - -### allowAll() - -```ts -readonly allowAll(): FilePolicy; -``` - -Allow every action. - -#### Returns - -`FilePolicy` - -### any() - -```ts -readonly any(...policies: FilePolicy[]): FilePolicy; -``` - -OR — at least one policy must allow. Short-circuits on first allow. - -#### Parameters - -| Parameter | Type | -| ------ | ------ | -| ...`policies` | `FilePolicy`[] | - -#### Returns - -`FilePolicy` - -### denyAll() - -```ts -readonly denyAll(): FilePolicy; -``` - -Deny every action. - -#### Returns - -`FilePolicy` - -### not() - -```ts -readonly not(p: FilePolicy): FilePolicy; -``` - -Negates a policy. - -#### Parameters - -| Parameter | Type | -| ------ | ------ | -| `p` | `FilePolicy` | - -#### Returns - -`FilePolicy` - -### publicRead() - -```ts -readonly publicRead(): FilePolicy; -``` - -Allow all read actions (list, read, download, raw, exists, metadata, preview). - -#### Returns - -`FilePolicy` diff --git a/docs/docs/api/appkit/index.md b/docs/docs/api/appkit/index.md index 0ae5e4e4..c0d06a9b 100644 --- a/docs/docs/api/appkit/index.md +++ b/docs/docs/api/appkit/index.md @@ -21,6 +21,7 @@ plugin architecture, and React integration. | [ExecutionError](Class.ExecutionError.md) | Error thrown when an operation execution fails. Use for statement failures, canceled operations, or unexpected states. | | [InitializationError](Class.InitializationError.md) | Error thrown when a service or component is not properly initialized. Use when accessing services before they are ready. | | [Plugin](Class.Plugin.md) | Base abstract class for creating AppKit plugins. | +| [PolicyDeniedError](Class.PolicyDeniedError.md) | Thrown when a policy denies an action. | | [ResourceRegistry](Class.ResourceRegistry.md) | Central registry for tracking plugin resource requirements. Deduplication uses type + resourceKey (machine-stable); alias is for display only. | | [ServerError](Class.ServerError.md) | Error thrown when server lifecycle operations fail. Use for server start/stop issues, configuration conflicts, etc. | | [TunnelError](Class.TunnelError.md) | Error thrown when remote tunnel operations fail. Use for tunnel connection issues, message parsing failures, etc. | @@ -34,6 +35,8 @@ plugin architecture, and React integration. | [CacheConfig](Interface.CacheConfig.md) | Configuration for the CacheInterceptor. Controls TTL, size limits, storage backend, and probabilistic cleanup. | | [DatabaseCredential](Interface.DatabaseCredential.md) | Database credentials with OAuth token for Postgres connection | | [EndpointConfig](Interface.EndpointConfig.md) | - | +| [FilePolicyUser](Interface.FilePolicyUser.md) | Minimal user identity passed to the policy function. | +| [FileResource](Interface.FileResource.md) | Describes the file or directory being acted upon. | | [GenerateDatabaseCredentialRequest](Interface.GenerateDatabaseCredentialRequest.md) | Request parameters for generating database OAuth credentials | | [ITelemetry](Interface.ITelemetry.md) | Plugin-facing interface for OpenTelemetry instrumentation. Provides a thin abstraction over OpenTelemetry APIs for plugins. | | [LakebasePoolConfig](Interface.LakebasePoolConfig.md) | Configuration for creating a Lakebase connection pool | @@ -55,6 +58,8 @@ plugin architecture, and React integration. | ------ | ------ | | [ConfigSchema](TypeAlias.ConfigSchema.md) | Configuration schema definition for plugin config. Re-exported from the standard JSON Schema Draft 7 types. | | [ExecutionResult](TypeAlias.ExecutionResult.md) | Discriminated union for plugin execution results. | +| [FileAction](TypeAlias.FileAction.md) | Every action the files plugin can perform. | +| [FilePolicy](TypeAlias.FilePolicy.md) | A policy function that decides whether `user` may perform `action` on `resource`. Return `true` to allow, `false` to deny. | | [IAppRouter](TypeAlias.IAppRouter.md) | Express router type for plugin route registration | | [PluginData](TypeAlias.PluginData.md) | Tuple of plugin class, config, and name. Created by `toPlugin()` and passed to `createApp()`. | | [ResourcePermission](TypeAlias.ResourcePermission.md) | Union of all possible permission levels across all resource types. | @@ -65,8 +70,9 @@ plugin architecture, and React integration. | Variable | Description | | ------ | ------ | -| [policy](Variable.policy.md) | Utility namespace with common policy combinators. | +| [READ\_ACTIONS](Variable.READ_ACTIONS.md) | Actions that only read data. | | [sql](Variable.sql.md) | SQL helper namespace | +| [WRITE\_ACTIONS](Variable.WRITE_ACTIONS.md) | Actions that mutate data. | ## Functions diff --git a/docs/docs/api/appkit/typedoc-sidebar.ts b/docs/docs/api/appkit/typedoc-sidebar.ts index d6fcd54a..1df5652a 100644 --- a/docs/docs/api/appkit/typedoc-sidebar.ts +++ b/docs/docs/api/appkit/typedoc-sidebar.ts @@ -56,6 +56,11 @@ const typedocSidebar: SidebarsConfig = { id: "api/appkit/Class.Plugin", label: "Plugin" }, + { + type: "doc", + id: "api/appkit/Class.PolicyDeniedError", + label: "PolicyDeniedError" + }, { type: "doc", id: "api/appkit/Class.ResourceRegistry", @@ -102,6 +107,16 @@ const typedocSidebar: SidebarsConfig = { id: "api/appkit/Interface.EndpointConfig", label: "EndpointConfig" }, + { + type: "doc", + id: "api/appkit/Interface.FilePolicyUser", + label: "FilePolicyUser" + }, + { + type: "doc", + id: "api/appkit/Interface.FileResource", + label: "FileResource" + }, { type: "doc", id: "api/appkit/Interface.GenerateDatabaseCredentialRequest", @@ -188,6 +203,16 @@ const typedocSidebar: SidebarsConfig = { id: "api/appkit/TypeAlias.ExecutionResult", label: "ExecutionResult" }, + { + type: "doc", + id: "api/appkit/TypeAlias.FileAction", + label: "FileAction" + }, + { + type: "doc", + id: "api/appkit/TypeAlias.FilePolicy", + label: "FilePolicy" + }, { type: "doc", id: "api/appkit/TypeAlias.IAppRouter", @@ -221,13 +246,18 @@ const typedocSidebar: SidebarsConfig = { items: [ { type: "doc", - id: "api/appkit/Variable.policy", - label: "policy" + id: "api/appkit/Variable.READ_ACTIONS", + label: "READ_ACTIONS" }, { type: "doc", id: "api/appkit/Variable.sql", label: "sql" + }, + { + type: "doc", + id: "api/appkit/Variable.WRITE_ACTIONS", + label: "WRITE_ACTIONS" } ] }, diff --git a/docs/docs/plugins/files.md b/docs/docs/plugins/files.md index b57f46e0..c823a581 100644 --- a/docs/docs/plugins/files.md +++ b/docs/docs/plugins/files.md @@ -129,16 +129,22 @@ Since HTTP routes always execute as the service principal, removing a user's UC ::: +:::info New in v0.21.0 + +File policies are new. Volumes without an explicit policy now default to `publicRead()`, which **denies all write operations** (`upload`, `mkdir`, `delete`). If your app relies on write access, set an explicit policy — for example `files.policy.allowAll()` — on each volume that needs it. + +::: + #### Access policies Attach a policy to a volume to control which actions are allowed: ```ts -import { files, policy } from "@databricks/appkit"; +import { files } from "@databricks/appkit"; files({ volumes: { - uploads: { policy: policy.publicRead() }, + uploads: { policy: files.policy.publicRead() }, }, }); ``` @@ -156,26 +162,26 @@ Policies receive an action string. The full list, split by category: | Helper | Allows | Denies | |--------|--------|--------| -| `policy.publicRead()` | all read actions | all write actions | -| `policy.allowAll()` | everything | nothing | -| `policy.denyAll()` | nothing | everything | +| `files.policy.publicRead()` | all read actions | all write actions | +| `files.policy.allowAll()` | everything | nothing | +| `files.policy.denyAll()` | nothing | everything | #### Composing policies Combine built-in and custom policies with three combinators: -- **`policy.all(a, b)`** — AND: all policies must allow. Short-circuits on first denial. -- **`policy.any(a, b)`** — OR: at least one policy must allow. Short-circuits on first allow. -- **`policy.not(p)`** — Inverts a policy. For example, `not(publicRead())` yields a write-only policy (useful for ingestion/drop-box volumes). +- **`files.policy.all(a, b)`** — AND: all policies must allow. Short-circuits on first denial. +- **`files.policy.any(a, b)`** — OR: at least one policy must allow. Short-circuits on first allow. +- **`files.policy.not(p)`** — Inverts a policy. For example, `not(publicRead())` yields a write-only policy (useful for ingestion/drop-box volumes). ```ts // Read-only for regular users, full access for the service principal files({ volumes: { shared: { - policy: policy.any( + policy: files.policy.any( (_action, _resource, user) => !!user.isServicePrincipal, - policy.publicRead(), + files.policy.publicRead(), ), }, }, @@ -205,9 +211,9 @@ files({ #### Enforcement -- **HTTP routes**: Policy checked before every operation. Denied → `403` JSON response with `"Action denied by volume policy"`. +- **HTTP routes**: Policy checked before every operation. Denied → `403` JSON response with `Policy denied "{action}" on volume "{volumeKey}"`. - **Programmatic API**: Policy checked on both `appkit.files("vol").list()` (SP identity, `isServicePrincipal: true`) and `appkit.files("vol").asUser(req).list()` (user identity). Denied → throws `PolicyDeniedError`. -- **No policy configured**: Defaults to `publicRead()` — read actions are allowed, write actions are denied. A startup warning is logged encouraging you to set an explicit policy. +- **No policy configured**: Defaults to `files.policy.publicRead()` — read actions are allowed, write actions are denied. A startup warning is logged encouraging you to set an explicit policy. ### Custom content types @@ -227,7 +233,7 @@ Dangerous MIME types (`text/html`, `text/javascript`, `application/javascript`, ## HTTP routes -Routes are mounted at `/api/files/*`. All routes except `/volumes` execute in user context via `asUser(req)`. +Routes are mounted at `/api/files/*`. All routes execute as the service principal. Policy enforcement checks user identity (from the `x-forwarded-user` header) before allowing operations — see [Access policies](#access-policies). | Method | Path | Query / Body | Response | | ------ | -------------------------- | ---------------------------- | ------------------------------------------------- | @@ -414,7 +420,7 @@ Built-in extensions: `.png`, `.jpg`, `.jpeg`, `.gif`, `.webp`, `.svg`, `.bmp`, ` ## User context -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. +HTTP routes always execute as the **service principal** — the SP's Databricks credentials are used for all API calls. User identity is extracted from the `x-forwarded-user` header and passed to the volume's [access policy](#access-policies) for authorization. This means UC grants on the SP (not individual users) determine what operations are possible, while policies control what each user is allowed to do through the app. 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. diff --git a/packages/appkit/src/context/execution-context.ts b/packages/appkit/src/context/execution-context.ts index dbc4788e..9588558e 100644 --- a/packages/appkit/src/context/execution-context.ts +++ b/packages/appkit/src/context/execution-context.ts @@ -81,3 +81,8 @@ export function getWarehouseId(): Promise { export function getWorkspaceId(): Promise { return getExecutionContext().workspaceId; } + +function isInUserContext(): boolean { + const ctx = executionContextStorage.getStore(); + return ctx !== undefined; +} diff --git a/packages/appkit/src/index.ts b/packages/appkit/src/index.ts index 90f7fad1..77ae4785 100644 --- a/packages/appkit/src/index.ts +++ b/packages/appkit/src/index.ts @@ -54,7 +54,18 @@ export { toPlugin, } from "./plugin"; export { analytics, files, genie, lakebase, server, serving } from "./plugins"; -export { policy } from "./plugins/files"; +// Files plugin types (for custom policy authoring) +export type { + FileAction, + FilePolicy, + FilePolicyUser, + FileResource, +} from "./plugins/files"; +export { + PolicyDeniedError, + READ_ACTIONS, + WRITE_ACTIONS, +} from "./plugins/files"; export type { EndpointConfig, ServingEndpointEntry, diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index c0d792bb..e0ff24c0 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -108,7 +108,7 @@ export class FilesPlugin extends Plugin { * Extract user identity from the request. * Falls back to `getCurrentUserId()` in development mode. */ - private _extractUser(req: import("express").Request): FilePolicyUser { + private _extractUser(req: express.Request): FilePolicyUser { const userId = req.header("x-forwarded-user"); if (userId) return { id: userId }; if (process.env.NODE_ENV === "development") { @@ -165,8 +165,8 @@ export class FilesPlugin extends Plugin { * Returns `true` if the request may proceed, `false` if a response was sent. */ private async _enforcePolicy( - req: import("express").Request, - res: import("express").Response, + req: express.Request, + res: express.Response, volumeKey: string, action: FileAction, path: string, @@ -473,46 +473,6 @@ export class FilesPlugin extends Plugin { this.cache.delete(listKey); } - /** - * Like `execute()`, but re-throws Databricks API client errors (4xx) - * so route handlers can return the appropriate HTTP status via `_handleApiError`. - * - * Server errors and infrastructure failures are still swallowed (returns `undefined`). - */ - private async _executeOrThrow( - fn: (signal?: AbortSignal) => Promise, - options: PluginExecutionSettings, - userKey?: string, - ): Promise { - let capturedClientError: unknown; - - const result = await this.execute( - async (signal) => { - try { - return await fn(signal); - } catch (error) { - if ( - error instanceof ApiError && - error.statusCode !== undefined && - error.statusCode >= 400 && - error.statusCode < 500 - ) { - capturedClientError = error; - } - throw error; - } - }, - options, - userKey, - ); - - if (result === undefined && capturedClientError) { - throw capturedClientError; - } - - return result; - } - private _handleApiError( res: express.Response, error: unknown, @@ -1167,7 +1127,7 @@ export class FilesPlugin extends Plugin { return { ...spApi, - asUser: (req: import("express").Request) => { + asUser: (req: express.Request) => { const user = this._extractUser(req); return this._createPolicyWrappedAPI(volumeKey, user); }, @@ -1189,4 +1149,4 @@ export class FilesPlugin extends Plugin { /** * @internal */ -export const files = toPlugin(FilesPlugin); +export const files = Object.assign(toPlugin(FilesPlugin), { policy }); From 8c45004587e4a05027df66536d59350fd1a59606 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 14:19:39 +0200 Subject: [PATCH 11/14] chore: reorder policy enforcement, add more tests --- packages/appkit/src/plugins/files/plugin.ts | 26 ++++++---- .../files/tests/plugin.integration.test.ts | 8 ++-- .../src/plugins/files/tests/plugin.test.ts | 48 ++++++++++++++++++- 3 files changed, 68 insertions(+), 14 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index e0ff24c0..14c49a94 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -151,8 +151,12 @@ export class FilesPlugin extends Plugin { volumeKey, user.id, ); - } catch { - // user.id may not be resolvable in all contexts (e.g. lazy SP getter) + } catch (err) { + if (!(err instanceof TypeError)) throw err; + logger.debug( + "Could not resolve user ID for policy denial log: %O", + err, + ); logger.warn('Policy denied "%s" on volume "%s"', action, volumeKey); } throw new PolicyDeniedError(action, volumeKey); @@ -258,6 +262,10 @@ export class FilesPlugin extends Plugin { * `_createPolicyWrappedAPI` for anything that serves user requests. */ protected createVolumeAPI(volumeKey: string): VolumeAPI { + logger.debug( + 'createVolumeAPI("%s") — bypassing policy enforcement', + volumeKey, + ); const connector = this.volumeConnectors[volumeKey]; return { list: (directoryPath?: string) => @@ -818,6 +826,13 @@ export class FilesPlugin extends Plugin { ? parseInt(rawContentLength, 10) : undefined; + if ( + !(await this._enforcePolicy(req, res, volumeKey, "upload", path, { + size: contentLength, + })) + ) + return; + if ( contentLength !== undefined && !Number.isNaN(contentLength) && @@ -830,13 +845,6 @@ export class FilesPlugin extends Plugin { return; } - if ( - !(await this._enforcePolicy(req, res, volumeKey, "upload", path, { - size: contentLength, - })) - ) - return; - logger.debug(req, "Upload started: volume=%s path=%s", volumeKey, path); try { diff --git a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts index 3d343474..5c97ce96 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.integration.test.ts @@ -510,7 +510,9 @@ describe("Files Plugin Integration", () => { }); } - test(`POST /api/files/${VOL}/upload with content-length over limit returns 413`, async () => { + test(`POST /api/files/${VOL}/upload with content-length over limit returns 403 (policy checked before size)`, async () => { + // Default publicRead() policy denies uploads. Policy enforcement runs + // before the content-length size check, so the response is 403 (not 413). const res = await rawPost( `/api/files/${VOL}/upload?path=/Volumes/catalog/schema/vol/large.bin`, { @@ -519,10 +521,10 @@ describe("Files Plugin Integration", () => { }, ); - expect(res.status).toBe(413); + expect(res.status).toBe(403); const data = JSON.parse(res.body) as { error: string; plugin: string }; expect(data.plugin).toBe("files"); - expect(data.error).toMatch(/exceeds maximum allowed size/); + expect(data.error).toMatch(/Policy denied/); }); }); diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index fb03930a..08fcd6b8 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -445,11 +445,16 @@ describe("FilesPlugin", () => { const res = mockRes(); // uploads has maxUploadSize: 100_000_000 + const headers: Record = { + "content-length": String(200_000_000), + "x-forwarded-user": "test-user", + }; await handler( { params: { volumeKey: "uploads" }, query: { path: "/large.bin" }, - headers: { "content-length": String(200_000_000) }, + headers, + header: (name: string) => headers[name.toLowerCase()], }, res, ); @@ -469,11 +474,16 @@ describe("FilesPlugin", () => { const res = mockRes(); // exports has no maxUploadSize, uses default 5GB + const headers: Record = { + "content-length": String(6 * 1024 * 1024 * 1024), + "x-forwarded-user": "test-user", + }; await handler( { params: { volumeKey: "exports" }, query: { path: "/large.bin" }, - headers: { "content-length": String(6 * 1024 * 1024 * 1024) }, + headers, + header: (name: string) => headers[name.toLowerCase()], }, res, ); @@ -911,6 +921,7 @@ describe("FilesPlugin", () => { public: { policy: policy.publicRead() }, locked: { policy: policy.denyAll() }, open: { policy: policy.allowAll() }, + writeonly: { policy: policy.not(policy.publicRead()) }, uploads: {}, exports: {}, }, @@ -967,12 +978,14 @@ describe("FilesPlugin", () => { process.env.DATABRICKS_VOLUME_PUBLIC = "/Volumes/c/s/public"; process.env.DATABRICKS_VOLUME_LOCKED = "/Volumes/c/s/locked"; process.env.DATABRICKS_VOLUME_OPEN = "/Volumes/c/s/open"; + process.env.DATABRICKS_VOLUME_WRITEONLY = "/Volumes/c/s/writeonly"; }); afterEach(() => { delete process.env.DATABRICKS_VOLUME_PUBLIC; delete process.env.DATABRICKS_VOLUME_LOCKED; delete process.env.DATABRICKS_VOLUME_OPEN; + delete process.env.DATABRICKS_VOLUME_WRITEONLY; }); test("policy volume + no user header (production) → 401", async () => { @@ -1401,6 +1414,37 @@ describe("FilesPlugin", () => { }), ); }); + + test("not(publicRead()) volume → read denied with 403", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler(mockReq("writeonly"), res); + + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.stringContaining("Policy denied"), + }), + ); + }); + + test("not(publicRead()) volume → write allowed", async () => { + const plugin = new FilesPlugin(POLICY_CONFIG); + const handler = getRouteHandler(plugin, "post", "/mkdir"); + const res = mockRes(); + + mockClient.files.createDirectory.mockResolvedValue(undefined); + + await handler(mockReq("writeonly", { body: { path: "/dropbox" } }), res); + + // Should not receive 403 + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(403); + }); }); describe("Upload Stream Size Limiter", () => { From 25ddb6ab089970b038982c7de3866bd36814b734 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 14:57:02 +0200 Subject: [PATCH 12/14] chore: simplify try/catch for checking policy / more tests --- packages/appkit/src/plugins/files/plugin.ts | 30 +++--- .../src/plugins/files/tests/plugin.test.ts | 102 ++++++++++++++++++ 2 files changed, 116 insertions(+), 16 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 14c49a94..480ed0b7 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -144,21 +144,13 @@ export class FilesPlugin extends Plugin { }; const allowed = await policyFn(action, resource, user); if (!allowed) { - try { - logger.warn( - 'Policy denied "%s" on volume "%s" for user "%s"', - action, - volumeKey, - user.id, - ); - } catch (err) { - if (!(err instanceof TypeError)) throw err; - logger.debug( - "Could not resolve user ID for policy denial log: %O", - err, - ); - logger.warn('Policy denied "%s" on volume "%s"', action, volumeKey); - } + const userId = user.isServicePrincipal ? "" : user.id; + logger.warn( + 'Policy denied "%s" on volume "%s" for user "%s"', + action, + volumeKey, + userId, + ); throw new PolicyDeniedError(action, volumeKey); } } @@ -196,7 +188,13 @@ export class FilesPlugin extends Plugin { res.status(403).json({ error: error.message, plugin: this.name }); return false; } - throw error; + // A crashing policy is treated as a server error (fail closed). + logger.error("Policy function threw on volume %s: %O", volumeKey, error); + res.status(500).json({ + error: "Policy evaluation failed", + plugin: this.name, + }); + return false; } return true; diff --git a/packages/appkit/src/plugins/files/tests/plugin.test.ts b/packages/appkit/src/plugins/files/tests/plugin.test.ts index 08fcd6b8..3edd7a65 100644 --- a/packages/appkit/src/plugins/files/tests/plugin.test.ts +++ b/packages/appkit/src/plugins/files/tests/plugin.test.ts @@ -1445,6 +1445,108 @@ describe("FilesPlugin", () => { ); expect(statusCodes).not.toContain(403); }); + + test("policy that throws → HTTP route returns 500 (fail closed)", async () => { + const brokenConfig = { + volumes: { + broken: { + policy: () => { + throw new Error("policy crashed"); + }, + }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; + + try { + const plugin = new FilesPlugin(brokenConfig); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler(mockReq("broken"), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Policy evaluation failed", + plugin: "files", + }), + ); + // Must NOT be 403 — a broken policy is not a denial, it's a server error + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(403); + } finally { + delete process.env.DATABRICKS_VOLUME_BROKEN; + } + }); + + test("policy that throws → programmatic API propagates the error", async () => { + const brokenConfig = { + volumes: { + broken: { + policy: () => { + throw new Error("policy crashed"); + }, + }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; + + try { + const plugin = new FilesPlugin(brokenConfig); + const handle = plugin.exports()("broken"); + + await expect(handle.list()).rejects.toThrow("policy crashed"); + await expect(handle.list()).rejects.not.toBeInstanceOf( + PolicyDeniedError, + ); + } finally { + delete process.env.DATABRICKS_VOLUME_BROKEN; + } + }); + + test("async policy that rejects → HTTP route returns 500 (fail closed)", async () => { + const brokenConfig = { + volumes: { + broken: { + policy: async () => { + throw new Error("async policy crashed"); + }, + }, + uploads: {}, + exports: {}, + }, + }; + process.env.DATABRICKS_VOLUME_BROKEN = "/Volumes/c/s/broken"; + + try { + const plugin = new FilesPlugin(brokenConfig); + const handler = getRouteHandler(plugin, "get", "/list"); + const res = mockRes(); + + await handler(mockReq("broken"), res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + error: "Policy evaluation failed", + plugin: "files", + }), + ); + const statusCodes = (res.status.mock.calls as number[][]).map( + (c) => c[0], + ); + expect(statusCodes).not.toContain(403); + } finally { + delete process.env.DATABRICKS_VOLUME_BROKEN; + } + }); }); describe("Upload Stream Size Limiter", () => { From 1c102a5df4de95cb001b4c6bea7ceba107ee3424 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 14:59:08 +0200 Subject: [PATCH 13/14] chore: update docs --- docs/docs/plugins/execution-context.md | 1 - 1 file changed, 1 deletion(-) diff --git a/docs/docs/plugins/execution-context.md b/docs/docs/plugins/execution-context.md index 6c7f8960..68dbb795 100644 --- a/docs/docs/plugins/execution-context.md +++ b/docs/docs/plugins/execution-context.md @@ -41,7 +41,6 @@ Exported from `@databricks/appkit`: - `getWorkspaceClient()`: Returns the appropriate WorkspaceClient for current context - `getWarehouseId()`: `Promise` (from `DATABRICKS_WAREHOUSE_ID` or auto-selected in dev) - `getWorkspaceId()`: `Promise` (from `DATABRICKS_WORKSPACE_ID` or fetched) -- `isInUserContext()`: Returns `true` if currently executing in user context ## Development mode behavior From 6ef1ade0c56ce8542c7dc8a45b85193e46595553 Mon Sep 17 00:00:00 2001 From: Atila Fassina Date: Fri, 10 Apr 2026 15:45:35 +0200 Subject: [PATCH 14/14] chore: path validation before policy check --- packages/appkit/src/plugins/files/plugin.ts | 59 ++++++++------------- 1 file changed, 21 insertions(+), 38 deletions(-) diff --git a/packages/appkit/src/plugins/files/plugin.ts b/packages/appkit/src/plugins/files/plugin.ts index 480ed0b7..6ef3b2a3 100644 --- a/packages/appkit/src/plugins/files/plugin.ts +++ b/packages/appkit/src/plugins/files/plugin.ts @@ -99,17 +99,12 @@ export class FilesPlugin extends Plugin { })); } - /** Whether a volume has a policy attached. */ - private _hasPolicy(volumeKey: string): boolean { - return typeof this.volumeConfigs[volumeKey]?.policy === "function"; - } - /** * Extract user identity from the request. * Falls back to `getCurrentUserId()` in development mode. */ private _extractUser(req: express.Request): FilePolicyUser { - const userId = req.header("x-forwarded-user"); + const userId = req.header("x-forwarded-user")?.trim(); if (userId) return { id: userId }; if (process.env.NODE_ENV === "development") { logger.warn( @@ -168,8 +163,6 @@ export class FilesPlugin extends Plugin { path: string, resourceOverrides?: Partial, ): Promise { - if (!this._hasPolicy(volumeKey)) return true; - let user: FilePolicyUser; try { user = this._extractUser(req); @@ -561,14 +554,14 @@ export class FilesPlugin extends Plugin { ): Promise { const path = req.query.path as string; - if (!(await this._enforcePolicy(req, res, volumeKey, "read", path))) return; - const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "read", path))) return; + try { const result = await this.execute( async () => connector.read(getWorkspaceClient(), path), @@ -624,15 +617,15 @@ export class FilesPlugin extends Plugin { ): Promise { const path = req.query.path as string; - if (!(await this._enforcePolicy(req, res, volumeKey, opts.mode, path))) - return; - const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, opts.mode, path))) + return; + const label = opts.mode === "download" ? "Download" : "Raw fetch"; const volumeCfg = this.volumeConfigs[volumeKey]; @@ -704,15 +697,15 @@ export class FilesPlugin extends Plugin { ): Promise { const path = req.query.path as string; - if (!(await this._enforcePolicy(req, res, volumeKey, "exists", path))) - return; - const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "exists", path))) + return; + try { const result = await this.execute( async () => connector.exists(getWorkspaceClient(), path), @@ -740,15 +733,15 @@ export class FilesPlugin extends Plugin { ): Promise { const path = req.query.path as string; - if (!(await this._enforcePolicy(req, res, volumeKey, "metadata", path))) - return; - const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "metadata", path))) + return; + try { const result = await this.execute( async () => connector.metadata(getWorkspaceClient(), path), @@ -776,15 +769,15 @@ export class FilesPlugin extends Plugin { ): Promise { const path = req.query.path as string; - if (!(await this._enforcePolicy(req, res, volumeKey, "preview", path))) - return; - const valid = this._isValidPath(path); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "preview", path))) + return; + try { const result = await this.execute( async () => connector.preview(getWorkspaceClient(), path), @@ -920,17 +913,15 @@ export class FilesPlugin extends Plugin { const dirPath = typeof req.body?.path === "string" ? req.body.path : undefined; - if ( - !(await this._enforcePolicy(req, res, volumeKey, "mkdir", dirPath ?? "/")) - ) - return; - const valid = this._isValidPath(dirPath); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); return; } + if (!(await this._enforcePolicy(req, res, volumeKey, "mkdir", dirPath))) + return; + try { const settings: PluginExecutionSettings = { default: FILES_WRITE_DEFAULTS, @@ -963,17 +954,6 @@ export class FilesPlugin extends Plugin { ): Promise { const rawPath = req.query.path as string | undefined; - if ( - !(await this._enforcePolicy( - req, - res, - volumeKey, - "delete", - rawPath ?? "/", - )) - ) - return; - const valid = this._isValidPath(rawPath); if (valid !== true) { res.status(400).json({ error: valid, plugin: this.name }); @@ -981,6 +961,9 @@ export class FilesPlugin extends Plugin { } const path = rawPath as string; + if (!(await this._enforcePolicy(req, res, volumeKey, "delete", path))) + return; + try { const settings: PluginExecutionSettings = { default: FILES_WRITE_DEFAULTS,