From 849e8782c8472a771a3de11452262c7ee7bf24f8 Mon Sep 17 00:00:00 2001 From: Luke Policinski Date: Mon, 31 Aug 2026 12:12:42 -0400 Subject: [PATCH 1/2] bug: fix moving dir paths --- src/file-manager/file-manager.service.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/file-manager/file-manager.service.ts b/src/file-manager/file-manager.service.ts index 51da79964..d1450b395 100644 --- a/src/file-manager/file-manager.service.ts +++ b/src/file-manager/file-manager.service.ts @@ -62,6 +62,19 @@ export class FileManagerService { return `http://${nodeIP}:8585/file-operations/${endpoint}`; } + // The connector's validation errors come back as an array of strings, which + // Nest drops on the floor -- an HttpException built from a non-string reports + // itself as "Bad Request Exception" and the operator never learns why. + private connectorErrorMessage(error: { + message?: string | string[]; + }): string { + if (Array.isArray(error?.message)) { + return error.message.join(", "); + } + + return error?.message ?? ""; + } + private async requestNodeConnector( nodeIP: string, endpoint: string, @@ -81,7 +94,8 @@ export class FileManagerService { if (!response.ok) { const error = await response.json().catch(() => ({})); throw new BadRequestException( - error.message || `Node connector error: ${response.statusText}`, + this.connectorErrorMessage(error) || + `Node connector error: ${response.statusText}`, ); } @@ -257,7 +271,8 @@ export class FileManagerService { if (!response.ok) { const error = await response.json().catch(() => ({})); throw new BadRequestException( - error.message || `Upload failed: ${response.statusText}`, + this.connectorErrorMessage(error) || + `Upload failed: ${response.statusText}`, ); } From e2fbb502bb359b7b1edf4dad69a9f850466f37e7 Mon Sep 17 00:00:00 2001 From: Luke Policinski Date: Mon, 31 Aug 2026 12:37:48 -0400 Subject: [PATCH 2/2] wip --- src/file-manager/file-manager.service.spec.ts | 55 ++++++++++++ src/file-manager/file-manager.service.ts | 86 ++++++++++--------- src/game-plugins/game-plugins.service.ts | 8 +- 3 files changed, 103 insertions(+), 46 deletions(-) create mode 100644 src/file-manager/file-manager.service.spec.ts diff --git a/src/file-manager/file-manager.service.spec.ts b/src/file-manager/file-manager.service.spec.ts new file mode 100644 index 000000000..d658aa90c --- /dev/null +++ b/src/file-manager/file-manager.service.spec.ts @@ -0,0 +1,55 @@ +import { BadRequestException } from "@nestjs/common"; +import { FileManagerService } from "./file-manager.service"; + +// Nest builds an HttpException's message from the response only when it is a +// string; anything else falls back to the class name, so the operator is told +// "Bad Request Exception" and nothing about what they did wrong. +describe("FileManagerService.connectorErrorMessage", () => { + const message = (error: unknown) => + new BadRequestException(FileManagerService.connectorErrorMessage(error)) + .message; + + it("flattens the array the connector's ValidationPipe returns", () => { + expect( + FileManagerService.connectorErrorMessage({ + statusCode: 400, + message: [ + "destPath must be a string", + "sourcePath should not be empty", + ], + error: "Bad Request", + }), + ).toBe("destPath must be a string, sourcePath should not be empty"); + }); + + it("passes a plain string through", () => { + expect( + FileManagerService.connectorErrorMessage({ + message: "Destination already exists: addons", + }), + ).toBe("Destination already exists: addons"); + }); + + // A proxy in front of the connector answers in its own shape, and an object + // reaching BadRequestException reads as "Bad Request Exception". + it("flattens a message that is not a string at all", () => { + expect(message({ message: { error: "path traversal detected" } })).toBe( + '{"error":"path traversal detected"}', + ); + expect(message({ message: 400 })).toBe("400"); + }); + + it("gives an empty string when there is no message to report", () => { + expect(FileManagerService.connectorErrorMessage({})).toBe(""); + expect(FileManagerService.connectorErrorMessage(undefined)).toBe(""); + expect(FileManagerService.connectorErrorMessage({ message: null })).toBe( + "", + ); + }); + + it("survives the round trip into an exception the operator reads", () => { + expect(message({ message: ["destPath must be a string"] })).toBe( + "destPath must be a string", + ); + }); +}); diff --git a/src/file-manager/file-manager.service.ts b/src/file-manager/file-manager.service.ts index d1450b395..7a31def36 100644 --- a/src/file-manager/file-manager.service.ts +++ b/src/file-manager/file-manager.service.ts @@ -62,17 +62,28 @@ export class FileManagerService { return `http://${nodeIP}:8585/file-operations/${endpoint}`; } - // The connector's validation errors come back as an array of strings, which - // Nest drops on the floor -- an HttpException built from a non-string reports - // itself as "Bad Request Exception" and the operator never learns why. - private connectorErrorMessage(error: { - message?: string | string[]; - }): string { - if (Array.isArray(error?.message)) { - return error.message.join(", "); + // The connector's validation errors come back as an array of strings, and an + // HttpException built from anything but a string reports itself as "Bad + // Request Exception" -- the operator is told the request failed and nothing + // about why. The body is whatever came off the wire, so nothing here can + // assume the shape. Public because GamePluginsService reaches the same + // connector behind the same ValidationPipe. + public static connectorErrorMessage(error: unknown): string { + const message = (error as { message?: unknown })?.message; + + if (Array.isArray(message)) { + return message.join(", "); } - return error?.message ?? ""; + if (typeof message === "string") { + return message; + } + + if (message === undefined || message === null) { + return ""; + } + + return JSON.stringify(message); } private async requestNodeConnector( @@ -81,29 +92,37 @@ export class FileManagerService { options: RequestInit = {}, ): Promise { const url = this.getNodeConnectorURL(nodeIP, endpoint); + // fetch sets its own multipart Content-Type, boundary and all; naming it + // here would leave the connector unable to parse the upload. + const isFormData = options.body instanceof FormData; + + let response: Response; try { - const response = await fetch(url, { + response = await fetch(url, { ...options, headers: { - "Content-Type": "application/json", + ...(isFormData ? {} : { "Content-Type": "application/json" }), ...options.headers, }, }); - - if (!response.ok) { - const error = await response.json().catch(() => ({})); - throw new BadRequestException( - this.connectorErrorMessage(error) || - `Node connector error: ${response.statusText}`, - ); - } - - return await response.json(); } catch (error) { this.logger.error(`Error calling node connector at ${url}`, error); throw error; } + + // Thrown outside the catch above: a rejected file operation is the operator + // mistyping a path, not the node being unreachable, and logging it at error + // level with a stack buries the transport failures that are. + if (!response.ok) { + const error = await response.json().catch(() => ({})); + throw new BadRequestException( + FileManagerService.connectorErrorMessage(error) || + `Node connector error: ${response.statusText}`, + ); + } + + return await response.json(); } async listFiles( @@ -260,26 +279,9 @@ export class FileManagerService { formData.append("basePath", basePath); formData.append("filePath", filePath); - const url = this.getNodeConnectorURL(nodeIP, "upload"); - - try { - const response = await fetch(url, { - method: "POST", - body: formData, - }); - - if (!response.ok) { - const error = await response.json().catch(() => ({})); - throw new BadRequestException( - this.connectorErrorMessage(error) || - `Upload failed: ${response.statusText}`, - ); - } - - return await response.json(); - } catch (error) { - this.logger.error(`Error uploading file to ${url}`, error); - throw error; - } + return await this.requestNodeConnector(nodeIP, "upload", { + method: "POST", + body: formData, + }); } } diff --git a/src/game-plugins/game-plugins.service.ts b/src/game-plugins/game-plugins.service.ts index e10a6e0f0..a6ae294c6 100644 --- a/src/game-plugins/game-plugins.service.ts +++ b/src/game-plugins/game-plugins.service.ts @@ -9,6 +9,7 @@ import { InjectQueue } from "@nestjs/bullmq"; import { Job, Queue } from "bullmq"; import { HasuraService } from "../hasura/hasura.service"; import { PostgresService } from "../postgres/postgres.service"; +import { FileManagerService } from "../file-manager/file-manager.service"; import { PluginRuntimeService } from "../plugin-runtime/plugin-runtime.service"; import { CacheService } from "../cache/cache.service"; import { SystemSettingName } from "../system/enums/SystemSettingName"; @@ -1476,11 +1477,10 @@ export class GamePluginsService { }); if (!response.ok) { - const body = await response - .json() - .catch(() => ({}) as { message?: string }); + const body = await response.json().catch(() => ({})); throw new BadRequestException( - body.message || `node connector returned ${response.status}`, + FileManagerService.connectorErrorMessage(body) || + `node connector returned ${response.status}`, ); }