fix(gitrest): Handle FileSystem Errors in HTTP Responses

server/gitrest/packages/gitrest-base/src/routes/summaries.ts

@@ -35,7 +35,8 @@ import {
 	logAndThrowApiError,
 	persistLatestFullSummaryInStorage,
 	retrieveLatestFullSummaryFromStorage,
-	SystemErrors,
+	isFilesystemError,
+	throwFileSystemErrorAsNetworkError,
 } from "../utils";
 
 function getFullSummaryDirectory(repoManager: IRepositoryManager, documentId: string): string {
@@ -79,14 +80,11 @@ async function getSummary(
 				error,
 			);
 			if (enforceStrictPersistedFullSummaryReads) {
-				if (isNetworkError(error) && error.code === 413) {
+				if (isNetworkError(error)) {
 					throw error;
 				}
-				if (
-					typeof (error as any).code === "string" &&
-					(error as any).code === SystemErrors.EFBIG.code
-				) {
-					throw new NetworkError(413, "Full summary too large.");
+				if (isFilesystemError(error)) {
+					throwFileSystemErrorAsNetworkError(error);
 				}
 			}
 		}

server/gitrest/packages/gitrest-base/src/utils/fileSystemBase.ts

@@ -9,7 +9,7 @@ import type { Stream } from "node:stream";
 import type { Abortable } from "node:events";
 import sizeof from "object-sizeof";
 import { type IFileSystemPromises } from "./definitions";
-import { FilesystemError, SystemErrors } from "./fileSystemHelper";
+import { filepathToString, FilesystemError, SystemErrors } from "./fileSystemHelper";
 
 export abstract class FsPromisesBase implements IFileSystemPromises {
 	public readonly promises: IFileSystemPromises;
@@ -88,8 +88,12 @@ export abstract class FsPromisesBase implements IFileSystemPromises {
 			this.maxFileSizeBytes > 0 &&
 			sizeof(data) > this.maxFileSizeBytes
 		) {
-			// eslint-disable-next-line @typescript-eslint/no-base-to-string
-			throw new FilesystemError(SystemErrors.EFBIG, filepath.toString());
+			throw new FilesystemError(
+				SystemErrors.EFBIG,
+				`Attempted write size (${sizeof(data)} bytes) to ${filepathToString(
+					filepath,
+				)} exceeds limit (${this.maxFileSizeBytes} bytes).`,
+			);
 		}
 		return this.writeFileCore(filepath, data, options);
 	}

server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts

@@ -3,7 +3,9 @@
  * Licensed under the MIT License.
  */
 
-import fs from "fs";
+import { NetworkError } from "@fluidframework/server-services-client";
+import fs, { type PathLike } from "fs";
+import type { FileHandle } from "fs/promises";
 
 export const packedRefsFileName = "packed-refs";
 
@@ -12,43 +14,54 @@ export type FsEntityType = "file" | "directory" | "symlink";
 export interface ISystemError {
 	code: string;
 	description: string;
+	httpStatusCode: number;
 }
 
 export const SystemErrors: Record<string, ISystemError> = {
 	EEXIST: {
 		code: "EEXIST",
 		description: "File already exists",
+		httpStatusCode: 409,
 	},
 	EINVAL: {
 		code: "EINVAL",
 		description: "Invalid argument",
+		httpStatusCode: 400,
 	},
 	EISDIR: {
 		code: "EISDIR",
 		description: "Illegal operation on a directory",
+		httpStatusCode: 405,
 	},
 	ENOENT: {
 		code: "ENOENT",
 		description: "No such file or directory",
+		httpStatusCode: 404,
 	},
 	ENOTDIR: {
 		code: "ENOTDIR",
 		description: "Not a directory",
+		httpStatusCode: 406,
 	},
 	ENOTEMPTY: {
 		code: "ENOTEMPTY",
 		description: "Directory not empty",
+		httpStatusCode: 409,
 	},
 	EFBIG: {
 		code: "EFBIG",
 		description: "File too large",
+		httpStatusCode: 413,
 	},
 	UNKNOWN: {
 		code: "UNKNOWN",
 		description: "Unknown error",
+		httpStatusCode: 500,
 	},
 };
 
+const KnownSystemErrorCodes = new Set(Object.keys(SystemErrors));
+
 export class FilesystemError extends Error {
 	public get code() {
 		return this.err.code;
@@ -63,6 +76,63 @@ export class FilesystemError extends Error {
 	}
 }
 
+/**
+ * Check if an error is a recognized FilesystemError (or RedisFsError).
+ *
+ * @param err - An unknown error object
+ * @returns Whether the error object is a FilesystemError (or RedisFsError)
+ */
+export function isFilesystemError(err: unknown): err is FilesystemError {
+	// This also works for RedisFsError which exposes a compatible code property.
+	return (
+		typeof err === "object" &&
+		err !== null &&
+		"code" in err &&
+		typeof err.code === "string" &&
+		KnownSystemErrorCodes.has(err.code)
+	);
+}
+
+/**
+ * If the error is a FilesystemError, throw it as a NetworkError with the appropriate status code.
+ * Otherwise, rethrow the error as-is.
+ *
+ * @param err - An unknown error object
+ */
+export function throwFileSystemErrorAsNetworkError(err: FilesystemError): never {
+	const systemError = SystemErrors[err.code] ?? SystemErrors.UNKNOWN;
+	const error = new NetworkError(
+		systemError.httpStatusCode,
+		// Only use SystemError.description, not the message, to protect against leaking sensitive information.
+		systemError.description,
+		systemError.httpStatusCode === 500,
+		undefined /* isFatal */,
+		undefined /* retryAfterMs */,
+		"Gitrest filesystem error",
+	);
+	throw error;
+}
+
+function isFileHandle(filepath: PathLike | FileHandle): filepath is FileHandle {
+	return typeof filepath !== "string" && !Buffer.isBuffer(filepath) && "fd" in filepath;
+}
+
+/**
+ * Convert a PathLike or FileHandle to a string path.
+ * @remarks
+ * This is useful for logging and debugging.
+ * If the input is a FileHandle, the path is unknown and a generic message is returned, rather than using readLink.
+ *
+ * @param filepath - A PathLike or FileHandle
+ * @returns The string representation of the path
+ */
+export function filepathToString(filepath: PathLike | FileHandle): string {
+	if (isFileHandle(filepath)) {
+		return "Unknown file handle path";
+	}
+	return filepath.toString();
+}
+
 /**
  * Creates an `fs.Stats` object using the information retrieved through the filesystem APIs.
  * GitRest and isomorphic-git expect `fs.Stats` objects, but we don't use `fs` to obtain them.

server/gitrest/packages/gitrest-base/src/utils/helpers.ts

@@ -30,6 +30,7 @@ import {
 	BaseGitRestTelemetryProperties,
 	GitRestLumberEventName,
 } from "./gitrestTelemetryDefinitions";
+import { isFilesystemError, throwFileSystemErrorAsNetworkError } from "./fileSystemHelper";
 
 /**
  * Validates that the input encoding is valid
@@ -139,11 +140,14 @@ export async function persistLatestFullSummaryInStorage(
 		persistLatestFullSummaryInStorageMetric.success(
 			"Successfully persisted latest full summary in storage",
 		);
-	} catch (error: any) {
+	} catch (error: unknown) {
 		persistLatestFullSummaryInStorageMetric.error(
 			"Failed to persist latest full summary in storage",
 			error,
 		);
+		if (isFilesystemError(error)) {
+			throwFileSystemErrorAsNetworkError(error);
+		}
 		throw error;
 	}
 }
@@ -261,6 +265,9 @@ export function logAndThrowApiError(
 	if (isNetworkError(error)) {
 		throw error;
 	}
+	if (isFilesystemError(error)) {
+		throwFileSystemErrorAsNetworkError(error);
+	}
 	// TODO: some APIs might expect 400 responses by default, like GetRef in GitManager. Since `handleResponse` uses
 	// 400 by default, using something different here would override the expected behavior and cause issues. Because
 	// of that, for now, we use 400 here. But ideally, we would revisit every RepoManager API and make sure that API

server/gitrest/packages/gitrest-base/src/utils/index.ts

@@ -19,7 +19,12 @@ export {
 	IStorageRoutingId,
 } from "./definitions";
 export { FsPromisesBase } from "./fileSystemBase";
-export { SystemErrors } from "./fileSystemHelper";
+export {
+	SystemErrors,
+	isFilesystemError,
+	throwFileSystemErrorAsNetworkError,
+	filepathToString,
+} from "./fileSystemHelper";
 export { MemFsManagerFactory, NodeFsManagerFactory, RedisFsManagerFactory } from "./filesystems";
 export {
 	BaseGitRestTelemetryProperties,