From eebb3b2d189d6b0ed086a5ea1603971d4cbd8ceb Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Mon, 16 Oct 2023 09:02:14 -0700 Subject: [PATCH] Validate Input to rehydrateDetachedContainerFromSnapshot (#17706) Validates that the string passed to rehydrateDetachedContainerFromSnapshot is of the correct format, rather than assuming via cast which can lead to random failures. Also add a unit test to verify the validation --------- Co-authored-by: Tony Murphy --- .../loader/container-loader/src/container.ts | 15 +- .../container-loader/src/test/loader.spec.ts | 130 ++++++++++++++++++ .../api-report/driver-utils.api.md | 2 +- .../driver-utils/src/summaryForCreateNew.ts | 3 +- 4 files changed, 144 insertions(+), 6 deletions(-) create mode 100644 packages/loader/container-loader/src/test/loader.spec.ts diff --git a/packages/loader/container-loader/src/container.ts b/packages/loader/container-loader/src/container.ts index e8d28b9c202f..9b2a38b8818e 100644 --- a/packages/loader/container-loader/src/container.ts +++ b/packages/loader/container-loader/src/container.ts @@ -134,6 +134,8 @@ const savedContainerEvent = "saved"; const packageNotFactoryError = "Code package does not implement IRuntimeFactory"; +const hasBlobsSummaryTree = ".hasAttachmentBlobs"; + /** * @internal */ @@ -473,7 +475,11 @@ export class Container container.mc.logger, { eventName: "RehydrateDetachedFromSnapshot" }, async (_event) => { - const deserializedSummary = JSON.parse(snapshot) as ISummaryTree; + const deserializedSummary = JSON.parse(snapshot); + if (!isCombinedAppAndProtocolSummary(deserializedSummary, hasBlobsSummaryTree)) { + throw new UsageError("Cannot rehydrate detached container. Incorrect format"); + } + await container.rehydrateDetachedFromSnapshot(deserializedSummary); return container; }, @@ -1178,7 +1184,7 @@ export class Container const combinedSummary = combineAppAndProtocolSummary(appSummary, protocolSummary); if (this.detachedBlobStorage && this.detachedBlobStorage.size > 0) { - combinedSummary.tree[".hasAttachmentBlobs"] = { + combinedSummary.tree[hasBlobsSummaryTree] = { type: SummaryType.Blob, content: "true", }; @@ -1815,12 +1821,13 @@ export class Container } private async rehydrateDetachedFromSnapshot(detachedContainerSnapshot: ISummaryTree) { - if (detachedContainerSnapshot.tree[".hasAttachmentBlobs"] !== undefined) { + if (detachedContainerSnapshot.tree[hasBlobsSummaryTree] !== undefined) { assert( !!this.detachedBlobStorage && this.detachedBlobStorage.size > 0, 0x250 /* "serialized container with attachment blobs must be rehydrated with detached blob storage" */, ); - delete detachedContainerSnapshot.tree[".hasAttachmentBlobs"]; + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete detachedContainerSnapshot.tree[hasBlobsSummaryTree]; } const snapshotTree = getSnapshotTreeFromSerializedContainer(detachedContainerSnapshot); diff --git a/packages/loader/container-loader/src/test/loader.spec.ts b/packages/loader/container-loader/src/test/loader.spec.ts new file mode 100644 index 000000000000..8174b32c2cab --- /dev/null +++ b/packages/loader/container-loader/src/test/loader.spec.ts @@ -0,0 +1,130 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +import assert from "assert"; +import { v4 as uuid } from "uuid"; +import { isFluidError } from "@fluidframework/telemetry-utils"; +import { FluidErrorTypes } from "@fluidframework/core-interfaces"; +import { ICreateBlobResponse, SummaryType } from "@fluidframework/protocol-definitions"; +import { IRuntime } from "@fluidframework/container-definitions"; +import { stringToBuffer } from "@fluid-internal/client-utils"; +import { IDetachedBlobStorage, Loader } from "../loader"; + +const failProxy = () => { + const proxy = new Proxy({} as any as T, { + get: (_, p) => { + throw Error(`${p.toString()} not implemented`); + }, + }); + return proxy; +}; + +const failSometimeProxy = (handler: Partial) => { + const proxy = new Proxy(handler as T, { + get: (t, p, r) => { + if (p in handler) { + return Reflect.get(t, p, r); + } + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return failProxy(); + }, + }); + return proxy; +}; + +const codeLoader = { + load: async () => { + return { + details: { + package: "none", + }, + module: { + fluidExport: { + IRuntimeFactory: { + get IRuntimeFactory() { + return this; + }, + async instantiateRuntime(context, existing) { + return failSometimeProxy({ + createSummary: () => ({ + tree: {}, + type: SummaryType.Tree, + }), + }); + }, + }, + }, + }, + }; + }, +}; + +describe("loader unit test", () => { + it("rehydrateDetachedContainerFromSnapshot with invalid format", async () => { + const loader = new Loader({ + codeLoader: failProxy(), + documentServiceFactory: failProxy(), + urlResolver: failProxy(), + }); + + try { + await loader.rehydrateDetachedContainerFromSnapshot(`{"foo":"bar"}`); + assert.fail("should fail"); + } catch (e) { + assert.strict(isFluidError(e), `should be a Fluid error: ${e}`); + assert.strictEqual(e.errorType, FluidErrorTypes.usageError, "should be a usage error"); + } + }); + + it("rehydrateDetachedContainerFromSnapshot with valid format", async () => { + const loader = new Loader({ + codeLoader, + documentServiceFactory: failProxy(), + urlResolver: failProxy(), + }); + const detached = await loader.createDetachedContainer({ package: "none" }); + const summary = detached.serialize(); + assert.strictEqual( + summary, + '{"type":1,"tree":{".protocol":{"tree":{"attributes":{"content":"{\\"minimumSequenceNumber\\":0,\\"sequenceNumber\\":0}","type":2},"quorumMembers":{"content":"[]","type":2},"quorumProposals":{"content":"[]","type":2},"quorumValues":{"content":"[[\\"code\\",{\\"key\\":\\"code\\",\\"value\\":{\\"package\\":\\"none\\"},\\"approvalSequenceNumber\\":0,\\"commitSequenceNumber\\":0,\\"sequenceNumber\\":0}]]","type":2}},"type":1},".app":{"tree":{},"type":1}}}', + "summary does not match expected format", + ); + await loader.rehydrateDetachedContainerFromSnapshot(summary); + }); + + it("rehydrateDetachedContainerFromSnapshot with valid format and attachment blobs", async () => { + const blobs = new Map(); + const detachedBlobStorage: IDetachedBlobStorage = { + createBlob: async (file) => { + const response: ICreateBlobResponse = { + id: uuid(), + }; + blobs.set(response.id, file); + return response; + }, + getBlobIds: () => [...blobs.keys()], + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + readBlob: async (id) => blobs.get(id)!, + get size() { + return blobs.size; + }, + }; + const loader = new Loader({ + codeLoader, + documentServiceFactory: failProxy(), + urlResolver: failProxy(), + detachedBlobStorage, + }); + const detached = await loader.createDetachedContainer({ package: "none" }); + await detachedBlobStorage.createBlob(stringToBuffer("whatever", "utf8")); + const summary = detached.serialize(); + assert.strictEqual( + summary, + '{"type":1,"tree":{".protocol":{"tree":{"attributes":{"content":"{\\"minimumSequenceNumber\\":0,\\"sequenceNumber\\":0}","type":2},"quorumMembers":{"content":"[]","type":2},"quorumProposals":{"content":"[]","type":2},"quorumValues":{"content":"[[\\"code\\",{\\"key\\":\\"code\\",\\"value\\":{\\"package\\":\\"none\\"},\\"approvalSequenceNumber\\":0,\\"commitSequenceNumber\\":0,\\"sequenceNumber\\":0}]]","type":2}},"type":1},".app":{"tree":{},"type":1},".hasAttachmentBlobs":{"type":2,"content":"true"}}}', + "summary does not match expected format", + ); + await loader.rehydrateDetachedContainerFromSnapshot(summary); + }); +}); diff --git a/packages/loader/driver-utils/api-report/driver-utils.api.md b/packages/loader/driver-utils/api-report/driver-utils.api.md index 6178ae59fc7a..e5a5e26ce428 100644 --- a/packages/loader/driver-utils/api-report/driver-utils.api.md +++ b/packages/loader/driver-utils/api-report/driver-utils.api.md @@ -220,7 +220,7 @@ export interface IProgress { } // @internal -export function isCombinedAppAndProtocolSummary(summary: ISummaryTree | undefined): summary is CombinedAppAndProtocolSummary; +export function isCombinedAppAndProtocolSummary(summary: ISummaryTree | undefined, ...optionalRootTrees: string[]): summary is CombinedAppAndProtocolSummary; // @public (undocumented) export function isOnline(): OnlineStatus; diff --git a/packages/loader/driver-utils/src/summaryForCreateNew.ts b/packages/loader/driver-utils/src/summaryForCreateNew.ts index ba39aefe11d7..f100c924e39a 100644 --- a/packages/loader/driver-utils/src/summaryForCreateNew.ts +++ b/packages/loader/driver-utils/src/summaryForCreateNew.ts @@ -30,6 +30,7 @@ export interface CombinedAppAndProtocolSummary extends ISummaryTree { */ export function isCombinedAppAndProtocolSummary( summary: ISummaryTree | undefined, + ...optionalRootTrees: string[] ): summary is CombinedAppAndProtocolSummary { if ( summary?.tree === undefined || @@ -38,7 +39,7 @@ export function isCombinedAppAndProtocolSummary( ) { return false; } - const treeKeys = Object.keys(summary.tree); + const treeKeys = Object.keys(summary.tree).filter((t) => !optionalRootTrees.includes(t)); if (treeKeys.length !== 2) { return false; }