chore: improve security and cleanup code a bit more

Author: atilafassina

packages/appkit/src/cache/tests/cache-manager.test.ts

@@ -557,6 +557,25 @@ describe("CacheManager", () => {
       expect((cache as any).inFlightRequests.has(cacheKey)).toBe(false);
     });
 
+    test("should re-throw ApiError without wrapping", async () => {
+      const { ApiError } = await import("@databricks/sdk-experimental");
+      const cache = await CacheManager.getInstance({
+        storage: createMockStorage(),
+      });
+      const apiError = new ApiError(
+        "Not Found",
+        "NOT_FOUND",
+        404,
+        undefined,
+        [],
+      );
+      const fn = vi.fn().mockRejectedValue(apiError);
+
+      await expect(cache.getOrExecute(["key"], fn, "user1")).rejects.toThrow(
+        apiError,
+      );
+    });
+
     test("should allow retry after error", async () => {
       const cache = await CacheManager.getInstance({
         storage: createMockStorage(),

packages/appkit/src/connectors/files/client.ts

@@ -60,7 +60,8 @@ export class FilesConnector {
   }
 
   resolvePath(filePath: string): string {
-    if (filePath.includes("..")) {
+    const segments = filePath.split("/");
+    if (segments.some((s) => s === "..")) {
       throw new Error('Path traversal ("../") is not allowed.');
     }
     if (filePath.startsWith("/")) {
@@ -150,83 +151,71 @@ export class FilesConnector {
   }
 
   async read(client: WorkspaceClient, filePath: string): Promise<string> {
-    return this.traced(
-      "read",
-      { "files.path": this.resolvePath(filePath) },
-      async () => {
-        const response = await this.download(client, filePath);
-        if (!response.contents) {
-          return "";
-        }
-        const reader = response.contents.getReader();
-        const decoder = new TextDecoder();
-        let result = "";
-        while (true) {
-          const { done, value } = await reader.read();
-          if (done) break;
-          result += decoder.decode(value, { stream: true });
-        }
-        result += decoder.decode();
-        return result;
-      },
-    );
+    const resolvedPath = this.resolvePath(filePath);
+    return this.traced("read", { "files.path": resolvedPath }, async () => {
+      const response = await this.download(client, filePath);
+      if (!response.contents) {
+        return "";
+      }
+      const reader = response.contents.getReader();
+      const decoder = new TextDecoder();
+      let result = "";
+      while (true) {
+        const { done, value } = await reader.read();
+        if (done) break;
+        result += decoder.decode(value, { stream: true });
+      }
+      result += decoder.decode();
+      return result;
+    });
   }
 
   async download(
     client: WorkspaceClient,
     filePath: string,
   ): Promise<DownloadResponse> {
-    return this.traced(
-      "download",
-      { "files.path": this.resolvePath(filePath) },
-      async () => {
-        return client.files.download({
-          file_path: this.resolvePath(filePath),
-        });
-      },
-    );
+    const resolvedPath = this.resolvePath(filePath);
+    return this.traced("download", { "files.path": resolvedPath }, async () => {
+      return client.files.download({
+        file_path: resolvedPath,
+      });
+    });
   }
 
   async exists(client: WorkspaceClient, filePath: string): Promise<boolean> {
-    return this.traced(
-      "exists",
-      { "files.path": this.resolvePath(filePath) },
-      async () => {
-        try {
-          await this.metadata(client, filePath);
-          return true;
-        } catch (error) {
-          if (error instanceof ApiError && error.statusCode === 404) {
-            return false;
-          }
-          throw error;
+    const resolvedPath = this.resolvePath(filePath);
+    return this.traced("exists", { "files.path": resolvedPath }, async () => {
+      try {
+        await this.metadata(client, filePath);
+        return true;
+      } catch (error) {
+        if (error instanceof ApiError && error.statusCode === 404) {
+          return false;
         }
-      },
-    );
+        throw error;
+      }
+    });
   }
 
   async metadata(
     client: WorkspaceClient,
     filePath: string,
   ): Promise<FileMetadata> {
-    return this.traced(
-      "metadata",
-      { "files.path": this.resolvePath(filePath) },
-      async () => {
-        const response = await client.files.getMetadata({
-          file_path: this.resolvePath(filePath),
-        });
-        return {
-          contentLength: response["content-length"],
-          contentType: contentTypeFromPath(
-            filePath,
-            response["content-type"],
-            this.customContentTypes,
-          ),
-          lastModified: response["last-modified"],
-        };
-      },
-    );
+    const resolvedPath = this.resolvePath(filePath);
+    return this.traced("metadata", { "files.path": resolvedPath }, async () => {
+      const response = await client.files.getMetadata({
+        file_path: resolvedPath,
+      });
+      return {
+        contentLength: response["content-length"],
+        contentType: contentTypeFromPath(
+          filePath,
+          response["content-type"],
+          this.customContentTypes,
+        ),
+        lastModified: response["last-modified"],
+      };
+    });
   }
 
   async upload(
@@ -274,7 +263,14 @@ export class FilesConnector {
       if (!res.ok) {
         const text = await res.text();
         logger.error(`Upload failed (${res.status}): ${text}`);
-        throw new Error(`Upload failed (${res.status}): ${text}`);
+        const safeMessage = text.length > 200 ? `${text.slice(0, 200)}…` : text;
+        throw new ApiError(
+          `Upload failed: ${safeMessage}`,
+          "UPLOAD_FAILED",
+          res.status,
+          undefined,
+          [],
+        );
       }
     });
   }
@@ -283,72 +279,67 @@ export class FilesConnector {
     client: WorkspaceClient,
     directoryPath: string,
   ): Promise<void> {
+    const resolvedPath = this.resolvePath(directoryPath);
     return this.traced(
       "createDirectory",
-      { "files.path": this.resolvePath(directoryPath) },
+      { "files.path": resolvedPath },
       async () => {
         await client.files.createDirectory({
-          directory_path: this.resolvePath(directoryPath),
+          directory_path: resolvedPath,
         });
       },
     );
   }
 
   async delete(client: WorkspaceClient, filePath: string): Promise<void> {
-    return this.traced(
-      "delete",
-      { "files.path": this.resolvePath(filePath) },
-      async () => {
-        await client.files.delete({
-          file_path: this.resolvePath(filePath),
-        });
-      },
-    );
+    const resolvedPath = this.resolvePath(filePath);
+    return this.traced("delete", { "files.path": resolvedPath }, async () => {
+      await client.files.delete({
+        file_path: resolvedPath,
+      });
+    });
   }
 
   async preview(
     client: WorkspaceClient,
     filePath: string,
     options?: { maxChars?: number },
   ): Promise<FilePreview> {
-    return this.traced(
-      "preview",
-      { "files.path": this.resolvePath(filePath) },
-      async () => {
-        const meta = await this.metadata(client, filePath);
-        const isText = isTextContentType(meta.contentType);
-        const isImage = meta.contentType?.startsWith("image/") || false;
+    const resolvedPath = this.resolvePath(filePath);
+    return this.traced("preview", { "files.path": resolvedPath }, async () => {
+      const meta = await this.metadata(client, filePath);
+      const isText = isTextContentType(meta.contentType);
+      const isImage = meta.contentType?.startsWith("image/") || false;
 
-        if (!isText) {
-          return { ...meta, textPreview: null, isText: false, isImage };
-        }
+      if (!isText) {
+        return { ...meta, textPreview: null, isText: false, isImage };
+      }
 
-        const response = await client.files.download({
-          file_path: this.resolvePath(filePath),
-        });
-        if (!response.contents) {
-          return { ...meta, textPreview: "", isText: true, isImage: false };
-        }
+      const response = await client.files.download({
+        file_path: resolvedPath,
+      });
+      if (!response.contents) {
+        return { ...meta, textPreview: "", isText: true, isImage: false };
+      }
 
-        const reader = response.contents.getReader();
-        const decoder = new TextDecoder();
-        let preview = "";
-        const maxChars = options?.maxChars ?? 1024;
+      const reader = response.contents.getReader();
+      const decoder = new TextDecoder();
+      let preview = "";
+      const maxChars = options?.maxChars ?? 1024;
 
-        while (preview.length < maxChars) {
-          const { done, value } = await reader.read();
-          if (done) break;
-          preview += decoder.decode(value, { stream: true });
-        }
-        preview += decoder.decode();
-        await reader.cancel();
+      while (preview.length < maxChars) {
+        const { done, value } = await reader.read();
+        if (done) break;
+        preview += decoder.decode(value, { stream: true });
+      }
+      preview += decoder.decode();
+      await reader.cancel();
 
-        if (preview.length > maxChars) {
-          preview = preview.slice(0, maxChars);
-        }
+      if (preview.length > maxChars) {
+        preview = preview.slice(0, maxChars);
+      }
 
-        return { ...meta, textPreview: preview, isText: true, isImage: false };
-      },
-    );
+      return { ...meta, textPreview: preview, isText: true, isImage: false };
+    });
   }
 }

packages/appkit/src/connectors/files/tests/client.test.ts

@@ -26,10 +26,18 @@ const { mockFilesApi, mockConfig, mockClient, MockApiError } = vi.hoisted(
     } as unknown as WorkspaceClient;
 
     class MockApiError extends Error {
+      errorCode: string;
       statusCode: number;
-      constructor(message: string, statusCode: number) {
+      constructor(
+        message: string,
+        errorCode: string,
+        statusCode: number,
+        _response?: any,
+        _details?: any[],
+      ) {
         super(message);
         this.name = "ApiError";
+        this.errorCode = errorCode;
         this.statusCode = statusCode;
       }
     }
@@ -356,7 +364,7 @@ describe("FilesConnector", () => {
 
     test("returns false on 404 ApiError", async () => {
       mockFilesApi.getMetadata.mockRejectedValue(
-        new MockApiError("Not found", 404),
+        new MockApiError("Not found", "NOT_FOUND", 404),
       );
 
       const result = await connector.exists(mockClient, "/missing.txt");
@@ -366,7 +374,7 @@ describe("FilesConnector", () => {
 
     test("rethrows non-404 ApiError", async () => {
       mockFilesApi.getMetadata.mockRejectedValue(
-        new MockApiError("Server error", 500),
+        new MockApiError("Server error", "SERVER_ERROR", 500),
       );
 
       await expect(connector.exists(mockClient, "/file.txt")).rejects.toThrow(
@@ -538,7 +546,7 @@ describe("FilesConnector", () => {
       );
     });
 
-    test("throws on non-ok response", async () => {
+    test("throws ApiError on non-ok response", async () => {
       fetchSpy.mockResolvedValue({
         ok: false,
         status: 403,
@@ -547,7 +555,14 @@ describe("FilesConnector", () => {
 
       await expect(
         connector.upload(mockClient, "file.txt", "data"),
-      ).rejects.toThrow("Upload failed (403): Forbidden");
+      ).rejects.toThrow("Upload failed: Forbidden");
+
+      try {
+        await connector.upload(mockClient, "file.txt", "data");
+      } catch (error) {
+        expect(error).toBeInstanceOf(MockApiError);
+        expect((error as any).statusCode).toBe(403);
+      }
     });
 
     test("resolves absolute paths directly", async () => {

packages/appkit/src/plugin/plugin.ts

@@ -344,7 +344,15 @@ export abstract class Plugin<
     await this.streamManager.stream(res, asyncWrapperFn, streamConfig);
   }
 
-  // single sync execution with interceptors
+  /**
+   * Execute a function with the plugin's interceptor chain.
+   *
+   * **Breaking change:** `ApiError` instances (and duck-typed errors with a
+   * numeric `statusCode`) are now re-thrown instead of being swallowed.
+   * This allows route handlers to map Databricks API status codes to proper
+   * HTTP responses. Custom plugins that relied on `execute()` returning
+   * `undefined` for API errors must update their error handling.
+   */
   protected async execute<T>(
     fn: (signal?: AbortSignal) => Promise<T>,
     options: PluginExecutionSettings,

packages/appkit/src/plugins/files/helpers.ts

@@ -27,5 +27,6 @@ export function parentDirectory(path: string): string {
  * HTTP header injection if upstream constraints ever change.
  */
 export function sanitizeFilename(raw: string): string {
-  return raw.replace(/["\\]/g, "\\$&").replace(/[\r\n]/g, "");
+  // biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally stripping control chars for security
+  return raw.replace(/["\\]/g, "\\$&").replace(/[\x00-\x1f]/g, "");
 }