From dac93cddf086d12b0c2c1ab126a189896abd57b3 Mon Sep 17 00:00:00 2001 From: Zach Newton Date: Tue, 5 Nov 2024 12:19:43 -0800 Subject: [PATCH 1/3] Handle FileSystem Errors in HTTP Responses --- .../gitrest-base/src/routes/summaries.ts | 12 ++--- .../gitrest-base/src/utils/fileSystemBase.ts | 12 +++-- .../src/utils/fileSystemHelper.ts | 53 ++++++++++++++++++- .../gitrest-base/src/utils/helpers.ts | 11 +++- .../packages/gitrest-base/src/utils/index.ts | 7 ++- 5 files changed, 82 insertions(+), 13 deletions(-) diff --git a/server/gitrest/packages/gitrest-base/src/routes/summaries.ts b/server/gitrest/packages/gitrest-base/src/routes/summaries.ts index d9fbc1f130a5..728fb057d10d 100644 --- a/server/gitrest/packages/gitrest-base/src/routes/summaries.ts +++ b/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); } } } diff --git a/server/gitrest/packages/gitrest-base/src/utils/fileSystemBase.ts b/server/gitrest/packages/gitrest-base/src/utils/fileSystemBase.ts index 97c0084b96c6..71824573b2ce 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/fileSystemBase.ts +++ b/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,14 @@ 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); } diff --git a/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts b/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts index 12ef3eec18e1..5a49e31d5ed9 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts +++ b/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,40 +14,49 @@ export type FsEntityType = "file" | "directory" | "symlink"; export interface ISystemError { code: string; description: string; + httpStatusCode: number; } export const SystemErrors: Record = { 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, }, }; @@ -63,6 +74,46 @@ export class FilesystemError extends Error { } } +export function isFilesystemError(err: unknown): err is FilesystemError { + return ( + typeof err === "object" && + err !== null && + "code" in err && + typeof err.code === "string" && + "name" in err && + err.name === "FilesystemError" + ); +} + +export function throwFileSystemErrorAsNetworkError(err: unknown): never { + if (isFilesystemError(err)) { + 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; + } + throw err; +} + +function isFileHandle(filepath: PathLike | FileHandle): filepath is FileHandle { + return typeof filepath !== "string" && !Buffer.isBuffer(filepath) && "fd" in filepath; +} + +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. diff --git a/server/gitrest/packages/gitrest-base/src/utils/helpers.ts b/server/gitrest/packages/gitrest-base/src/utils/helpers.ts index 3bf8acc44bda..fd40ba171902 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/helpers.ts +++ b/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,16 @@ 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 +267,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 diff --git a/server/gitrest/packages/gitrest-base/src/utils/index.ts b/server/gitrest/packages/gitrest-base/src/utils/index.ts index e729ce444242..6260b38babbc 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/index.ts +++ b/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, From 5fbcfcbaa7e021a76168065d00ec9fb771415470 Mon Sep 17 00:00:00 2001 From: Zach Newton Date: Tue, 5 Nov 2024 12:28:19 -0800 Subject: [PATCH 2/3] Add some doc comments --- .../src/utils/fileSystemHelper.ts | 54 +++++++++++++------ 1 file changed, 37 insertions(+), 17 deletions(-) diff --git a/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts b/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts index 5a49e31d5ed9..fbb8a9eb341f 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts @@ -60,6 +60,8 @@ export const SystemErrors: Record = { }, }; +const KnownSystemErrorCodes = new Set(Object.keys(SystemErrors)); + export class FilesystemError extends Error { public get code() { return this.err.code; @@ -74,38 +76,56 @@ 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" && - "name" in err && - err.name === "FilesystemError" + KnownSystemErrorCodes.has(err.code) ); } -export function throwFileSystemErrorAsNetworkError(err: unknown): never { - if (isFilesystemError(err)) { - 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; - } - throw err; +/** + * 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"; From c8d9ae2007a512be33e6dacdcf4500b3da45fae3 Mon Sep 17 00:00:00 2001 From: Zach Newton Date: Tue, 5 Nov 2024 12:28:32 -0800 Subject: [PATCH 3/3] formatter --- .../packages/gitrest-base/src/utils/fileSystemBase.ts | 8 +++----- .../packages/gitrest-base/src/utils/fileSystemHelper.ts | 1 - server/gitrest/packages/gitrest-base/src/utils/helpers.ts | 4 +--- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/server/gitrest/packages/gitrest-base/src/utils/fileSystemBase.ts b/server/gitrest/packages/gitrest-base/src/utils/fileSystemBase.ts index 71824573b2ce..56a0882512e2 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/fileSystemBase.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/fileSystemBase.ts @@ -90,11 +90,9 @@ export abstract class FsPromisesBase implements IFileSystemPromises { ) { throw new FilesystemError( SystemErrors.EFBIG, - `Attempted write size (${sizeof( - data, - )} bytes) to ${filepathToString(filepath)} exceeds limit (${ - this.maxFileSizeBytes - } bytes).`, + `Attempted write size (${sizeof(data)} bytes) to ${filepathToString( + filepath, + )} exceeds limit (${this.maxFileSizeBytes} bytes).`, ); } return this.writeFileCore(filepath, data, options); diff --git a/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts b/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts index fbb8a9eb341f..b4a77b49bfa9 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/fileSystemHelper.ts @@ -131,7 +131,6 @@ export function filepathToString(filepath: PathLike | FileHandle): string { return "Unknown file handle path"; } return filepath.toString(); - } /** diff --git a/server/gitrest/packages/gitrest-base/src/utils/helpers.ts b/server/gitrest/packages/gitrest-base/src/utils/helpers.ts index fd40ba171902..dc38c4e12c3b 100644 --- a/server/gitrest/packages/gitrest-base/src/utils/helpers.ts +++ b/server/gitrest/packages/gitrest-base/src/utils/helpers.ts @@ -145,9 +145,7 @@ export async function persistLatestFullSummaryInStorage( "Failed to persist latest full summary in storage", error, ); - if ( - isFilesystemError(error) - ) { + if (isFilesystemError(error)) { throwFileSystemErrorAsNetworkError(error); } throw error;