Skip to content

Commit 4d1d69d

Browse files
atilafassinaatilafassina
committed
chore: improve security and cleanup code a bit more
1 parent 47a201f commit 4d1d69d

7 files changed

Lines changed: 235 additions & 188 deletions

File tree

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

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,25 @@ describe("CacheManager", () => {
555555
expect((cache as any).inFlightRequests.has(cacheKey)).toBe(false);
556556
});
557557

558+
test("should re-throw ApiError without wrapping", async () => {
559+
const { ApiError } = await import("@databricks/sdk-experimental");
560+
const cache = await CacheManager.getInstance({
561+
storage: createMockStorage(),
562+
});
563+
const apiError = new ApiError(
564+
"Not Found",
565+
"NOT_FOUND",
566+
404,
567+
undefined,
568+
[],
569+
);
570+
const fn = vi.fn().mockRejectedValue(apiError);
571+
572+
await expect(cache.getOrExecute(["key"], fn, "user1")).rejects.toThrow(
573+
apiError,
574+
);
575+
});
576+
558577
test("should allow retry after error", async () => {
559578
const cache = await CacheManager.getInstance({
560579
storage: createMockStorage(),

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

Lines changed: 98 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,8 @@ export class FilesConnector {
6060
}
6161

6262
resolvePath(filePath: string): string {
63-
if (filePath.includes("..")) {
63+
const segments = filePath.split("/");
64+
if (segments.some((s) => s === "..")) {
6465
throw new Error('Path traversal ("../") is not allowed.');
6566
}
6667
if (filePath.startsWith("/")) {
@@ -150,83 +151,71 @@ export class FilesConnector {
150151
}
151152

152153
async read(client: WorkspaceClient, filePath: string): Promise<string> {
153-
return this.traced(
154-
"read",
155-
{ "files.path": this.resolvePath(filePath) },
156-
async () => {
157-
const response = await this.download(client, filePath);
158-
if (!response.contents) {
159-
return "";
160-
}
161-
const reader = response.contents.getReader();
162-
const decoder = new TextDecoder();
163-
let result = "";
164-
while (true) {
165-
const { done, value } = await reader.read();
166-
if (done) break;
167-
result += decoder.decode(value, { stream: true });
168-
}
169-
result += decoder.decode();
170-
return result;
171-
},
172-
);
154+
const resolvedPath = this.resolvePath(filePath);
155+
return this.traced("read", { "files.path": resolvedPath }, async () => {
156+
const response = await this.download(client, filePath);
157+
if (!response.contents) {
158+
return "";
159+
}
160+
const reader = response.contents.getReader();
161+
const decoder = new TextDecoder();
162+
let result = "";
163+
while (true) {
164+
const { done, value } = await reader.read();
165+
if (done) break;
166+
result += decoder.decode(value, { stream: true });
167+
}
168+
result += decoder.decode();
169+
return result;
170+
});
173171
}
174172

175173
async download(
176174
client: WorkspaceClient,
177175
filePath: string,
178176
): Promise<DownloadResponse> {
179-
return this.traced(
180-
"download",
181-
{ "files.path": this.resolvePath(filePath) },
182-
async () => {
183-
return client.files.download({
184-
file_path: this.resolvePath(filePath),
185-
});
186-
},
187-
);
177+
const resolvedPath = this.resolvePath(filePath);
178+
return this.traced("download", { "files.path": resolvedPath }, async () => {
179+
return client.files.download({
180+
file_path: resolvedPath,
181+
});
182+
});
188183
}
189184

190185
async exists(client: WorkspaceClient, filePath: string): Promise<boolean> {
191-
return this.traced(
192-
"exists",
193-
{ "files.path": this.resolvePath(filePath) },
194-
async () => {
195-
try {
196-
await this.metadata(client, filePath);
197-
return true;
198-
} catch (error) {
199-
if (error instanceof ApiError && error.statusCode === 404) {
200-
return false;
201-
}
202-
throw error;
186+
const resolvedPath = this.resolvePath(filePath);
187+
return this.traced("exists", { "files.path": resolvedPath }, async () => {
188+
try {
189+
await this.metadata(client, filePath);
190+
return true;
191+
} catch (error) {
192+
if (error instanceof ApiError && error.statusCode === 404) {
193+
return false;
203194
}
204-
},
205-
);
195+
throw error;
196+
}
197+
});
206198
}
207199

208200
async metadata(
209201
client: WorkspaceClient,
210202
filePath: string,
211203
): Promise<FileMetadata> {
212-
return this.traced(
213-
"metadata",
214-
{ "files.path": this.resolvePath(filePath) },
215-
async () => {
216-
const response = await client.files.getMetadata({
217-
file_path: this.resolvePath(filePath),
218-
});
219-
return {
220-
contentLength: response["content-length"],
221-
contentType: contentTypeFromPath(
222-
filePath,
223-
response["content-type"],
224-
this.customContentTypes,
225-
),
226-
lastModified: response["last-modified"],
227-
};
228-
},
229-
);
204+
const resolvedPath = this.resolvePath(filePath);
205+
return this.traced("metadata", { "files.path": resolvedPath }, async () => {
206+
const response = await client.files.getMetadata({
207+
file_path: resolvedPath,
208+
});
209+
return {
210+
contentLength: response["content-length"],
211+
contentType: contentTypeFromPath(
212+
filePath,
213+
response["content-type"],
214+
this.customContentTypes,
215+
),
216+
lastModified: response["last-modified"],
217+
};
218+
});
230219
}
231220

232221
async upload(
@@ -274,7 +263,14 @@ export class FilesConnector {
274263
if (!res.ok) {
275264
const text = await res.text();
276265
logger.error(`Upload failed (${res.status}): ${text}`);
277-
throw new Error(`Upload failed (${res.status}): ${text}`);
266+
const safeMessage = text.length > 200 ? `${text.slice(0, 200)}…` : text;
267+
throw new ApiError(
268+
`Upload failed: ${safeMessage}`,
269+
"UPLOAD_FAILED",
270+
res.status,
271+
undefined,
272+
[],
273+
);
278274
}
279275
});
280276
}
@@ -283,72 +279,67 @@ export class FilesConnector {
283279
client: WorkspaceClient,
284280
directoryPath: string,
285281
): Promise<void> {
282+
const resolvedPath = this.resolvePath(directoryPath);
286283
return this.traced(
287284
"createDirectory",
288-
{ "files.path": this.resolvePath(directoryPath) },
285+
{ "files.path": resolvedPath },
289286
async () => {
290287
await client.files.createDirectory({
291-
directory_path: this.resolvePath(directoryPath),
288+
directory_path: resolvedPath,
292289
});
293290
},
294291
);
295292
}
296293

297294
async delete(client: WorkspaceClient, filePath: string): Promise<void> {
298-
return this.traced(
299-
"delete",
300-
{ "files.path": this.resolvePath(filePath) },
301-
async () => {
302-
await client.files.delete({
303-
file_path: this.resolvePath(filePath),
304-
});
305-
},
306-
);
295+
const resolvedPath = this.resolvePath(filePath);
296+
return this.traced("delete", { "files.path": resolvedPath }, async () => {
297+
await client.files.delete({
298+
file_path: resolvedPath,
299+
});
300+
});
307301
}
308302

309303
async preview(
310304
client: WorkspaceClient,
311305
filePath: string,
312306
options?: { maxChars?: number },
313307
): Promise<FilePreview> {
314-
return this.traced(
315-
"preview",
316-
{ "files.path": this.resolvePath(filePath) },
317-
async () => {
318-
const meta = await this.metadata(client, filePath);
319-
const isText = isTextContentType(meta.contentType);
320-
const isImage = meta.contentType?.startsWith("image/") || false;
308+
const resolvedPath = this.resolvePath(filePath);
309+
return this.traced("preview", { "files.path": resolvedPath }, async () => {
310+
const meta = await this.metadata(client, filePath);
311+
const isText = isTextContentType(meta.contentType);
312+
const isImage = meta.contentType?.startsWith("image/") || false;
321313

322-
if (!isText) {
323-
return { ...meta, textPreview: null, isText: false, isImage };
324-
}
314+
if (!isText) {
315+
return { ...meta, textPreview: null, isText: false, isImage };
316+
}
325317

326-
const response = await client.files.download({
327-
file_path: this.resolvePath(filePath),
328-
});
329-
if (!response.contents) {
330-
return { ...meta, textPreview: "", isText: true, isImage: false };
331-
}
318+
const response = await client.files.download({
319+
file_path: resolvedPath,
320+
});
321+
if (!response.contents) {
322+
return { ...meta, textPreview: "", isText: true, isImage: false };
323+
}
332324

333-
const reader = response.contents.getReader();
334-
const decoder = new TextDecoder();
335-
let preview = "";
336-
const maxChars = options?.maxChars ?? 1024;
325+
const reader = response.contents.getReader();
326+
const decoder = new TextDecoder();
327+
let preview = "";
328+
const maxChars = options?.maxChars ?? 1024;
337329

338-
while (preview.length < maxChars) {
339-
const { done, value } = await reader.read();
340-
if (done) break;
341-
preview += decoder.decode(value, { stream: true });
342-
}
343-
preview += decoder.decode();
344-
await reader.cancel();
330+
while (preview.length < maxChars) {
331+
const { done, value } = await reader.read();
332+
if (done) break;
333+
preview += decoder.decode(value, { stream: true });
334+
}
335+
preview += decoder.decode();
336+
await reader.cancel();
345337

346-
if (preview.length > maxChars) {
347-
preview = preview.slice(0, maxChars);
348-
}
338+
if (preview.length > maxChars) {
339+
preview = preview.slice(0, maxChars);
340+
}
349341

350-
return { ...meta, textPreview: preview, isText: true, isImage: false };
351-
},
352-
);
342+
return { ...meta, textPreview: preview, isText: true, isImage: false };
343+
});
353344
}
354345
}

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

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,18 @@ const { mockFilesApi, mockConfig, mockClient, MockApiError } = vi.hoisted(
2626
} as unknown as WorkspaceClient;
2727

2828
class MockApiError extends Error {
29+
errorCode: string;
2930
statusCode: number;
30-
constructor(message: string, statusCode: number) {
31+
constructor(
32+
message: string,
33+
errorCode: string,
34+
statusCode: number,
35+
_response?: any,
36+
_details?: any[],
37+
) {
3138
super(message);
3239
this.name = "ApiError";
40+
this.errorCode = errorCode;
3341
this.statusCode = statusCode;
3442
}
3543
}
@@ -356,7 +364,7 @@ describe("FilesConnector", () => {
356364

357365
test("returns false on 404 ApiError", async () => {
358366
mockFilesApi.getMetadata.mockRejectedValue(
359-
new MockApiError("Not found", 404),
367+
new MockApiError("Not found", "NOT_FOUND", 404),
360368
);
361369

362370
const result = await connector.exists(mockClient, "/missing.txt");
@@ -366,7 +374,7 @@ describe("FilesConnector", () => {
366374

367375
test("rethrows non-404 ApiError", async () => {
368376
mockFilesApi.getMetadata.mockRejectedValue(
369-
new MockApiError("Server error", 500),
377+
new MockApiError("Server error", "SERVER_ERROR", 500),
370378
);
371379

372380
await expect(connector.exists(mockClient, "/file.txt")).rejects.toThrow(
@@ -538,7 +546,7 @@ describe("FilesConnector", () => {
538546
);
539547
});
540548

541-
test("throws on non-ok response", async () => {
549+
test("throws ApiError on non-ok response", async () => {
542550
fetchSpy.mockResolvedValue({
543551
ok: false,
544552
status: 403,
@@ -547,7 +555,14 @@ describe("FilesConnector", () => {
547555

548556
await expect(
549557
connector.upload(mockClient, "file.txt", "data"),
550-
).rejects.toThrow("Upload failed (403): Forbidden");
558+
).rejects.toThrow("Upload failed: Forbidden");
559+
560+
try {
561+
await connector.upload(mockClient, "file.txt", "data");
562+
} catch (error) {
563+
expect(error).toBeInstanceOf(MockApiError);
564+
expect((error as any).statusCode).toBe(403);
565+
}
551566
});
552567

553568
test("resolves absolute paths directly", async () => {

‎packages/appkit/src/plugin/plugin.ts‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -364,7 +364,15 @@ export abstract class Plugin<
364364
await this.streamManager.stream(res, asyncWrapperFn, streamConfig);
365365
}
366366

367-
// single sync execution with interceptors
367+
/**
368+
* Execute a function with the plugin's interceptor chain.
369+
*
370+
* **Breaking change:** `ApiError` instances (and duck-typed errors with a
371+
* numeric `statusCode`) are now re-thrown instead of being swallowed.
372+
* This allows route handlers to map Databricks API status codes to proper
373+
* HTTP responses. Custom plugins that relied on `execute()` returning
374+
* `undefined` for API errors must update their error handling.
375+
*/
368376
protected async execute<T>(
369377
fn: (signal?: AbortSignal) => Promise<T>,
370378
options: PluginExecutionSettings,

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,5 +27,6 @@ export function parentDirectory(path: string): string {
2727
* HTTP header injection if upstream constraints ever change.
2828
*/
2929
export function sanitizeFilename(raw: string): string {
30-
return raw.replace(/["\\]/g, "\\$&").replace(/[\r\n]/g, "");
30+
// biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally stripping control chars for security
31+
return raw.replace(/["\\]/g, "\\$&").replace(/[\x00-\x1f]/g, "");
3132
}

0 commit comments

Comments
 (0)