Escape directory names

storage/core/src/common/internal/Helpers.ts

@@ -77,6 +77,23 @@ export function assertRelativeDirectory(
     );
 }
 
+export function assertBaseDirectory(baseDirectory: string | undefined): void {
+  if (!baseDirectory) throw new Error("Base directory cannot be empty.");
+
+  const backslash = "\\";
+  if (baseDirectory.includes(backslash))
+    throw new Error("Base directory cannot contain backslashes.");
+
+  const separator = "/";
+  if (
+    baseDirectory[0] === separator ||
+    baseDirectory[baseDirectory.length - 1] === separator
+  )
+    throw new Error(
+      "Base directory cannot contain slashes at the beginning or the end of the string."
+    );
+}
+
 export function assertTransferConfig(transferConfig: TransferConfig): void {
   assertPrimitiveType(transferConfig, "transferConfig", "object");
   assertPrimitiveType(

storage/core/src/test/unit/common/Helpers.test.ts

@@ -7,6 +7,7 @@ import * as chaiAsPromised from "chai-as-promised";
 
 import { TransferConfig } from "../../../common";
 import {
+  assertBaseDirectory,
   assertRelativeDirectory,
   assertTransferConfig,
 } from "../../../common/internal";
@@ -124,4 +125,53 @@ describe("Helper functions", () => {
       }
     );
   });
+
+  describe(`${assertBaseDirectory.name}()`, () => {
+    [
+      {
+        baseDirectory: undefined,
+        expectedErrorMessage: "Base directory cannot be empty.",
+      },
+      {
+        baseDirectory: "",
+        expectedErrorMessage: "Base directory cannot be empty.",
+      },
+      {
+        baseDirectory: "\\foo",
+        expectedErrorMessage: "Base directory cannot contain backslashes.",
+      },
+      {
+        baseDirectory: "/foo",
+        expectedErrorMessage:
+          "Base directory cannot contain slashes at the beginning or the end of the string.",
+      },
+      {
+        baseDirectory: "foo/",
+        expectedErrorMessage:
+          "Base directory cannot contain slashes at the beginning or the end of the string.",
+      },
+      {
+        baseDirectory: "/foo/",
+        expectedErrorMessage:
+          "Base directory cannot contain slashes at the beginning or the end of the string.",
+      },
+    ].forEach((testCase) => {
+      it(`should throw if base directory is invalid (${testCase.baseDirectory})`, () => {
+        const testedFunction = () =>
+          assertBaseDirectory(testCase.baseDirectory);
+        expect(testedFunction)
+          .to.throw(Error)
+          .with.property("message", testCase.expectedErrorMessage);
+      });
+    });
+
+    ["foo", "foo/bar", "12345678-1234-1234-1234-123456789abc"].forEach(
+      (baseDirectory: string) => {
+        it(`should not throw if base directory is valid (${baseDirectory})`, () => {
+          const testedFunction = () => assertBaseDirectory(baseDirectory);
+          expect(testedFunction).to.not.throw();
+        });
+      }
+    );
+  });
 });

storage/google/src/frontend/GoogleFrontendStorage.ts

@@ -79,7 +79,9 @@ export class GoogleFrontendStorage extends FrontendStorage {
     assertRelativeDirectory(input.reference.relativeDirectory);
     const url = `https://storage.googleapis.com/upload/storage/v1/b/${
       input.transferConfig.bucketName
-    }/o?uploadType=media&name=${this.objectName(input.reference)}`;
+    }/o?uploadType=media&name=${encodeURIComponent(
+      this.objectName(input.reference)
+    )}`;
     return this._urlTransferClient.upload(url, input.data, "POST", {
       Authorization: input.transferConfig.authentication,
       "Content-Type": "application/octet-stream",
@@ -94,7 +96,9 @@ export class GoogleFrontendStorage extends FrontendStorage {
 
     const url = `https://storage.googleapis.com/upload/storage/v1/b/${
       input.transferConfig.bucketName
-    }/o?uploadType=media&name=${this.objectName(input.reference)}`;
+    }/o?uploadType=media&name=${encodeURIComponent(
+      this.objectName(input.reference)
+    )}`;
     const data = await streamToTransferTypeFrontend(input.data, "buffer");
 
     return this._urlTransferClient.upload(url, data, "POST", {

storage/google/src/server/GoogleServerStorage.ts

@@ -6,6 +6,7 @@
 import { Readable } from "stream";
 
 import {
+  assertBaseDirectory,
   assertRelativeDirectory,
   buildObjectDirectoryString,
 } from "@itwin/object-storage-core/lib/common/internal";
@@ -232,6 +233,7 @@ export class GoogleServerStorage extends ServerStorage {
     directory: ObjectDirectory,
     expiry?: ExpiryOptions
   ): Promise<GoogleTransferConfig> {
+    assertBaseDirectory(directory.baseDirectory);
     assertRelativeDirectory(directory.relativeDirectory);
     getExpiryDate(expiry);
 
@@ -243,6 +245,7 @@ export class GoogleServerStorage extends ServerStorage {
     directory: ObjectDirectory,
     expiry?: ExpiryOptions
   ): Promise<TransferConfig> {
+    assertBaseDirectory(directory.baseDirectory);
     assertRelativeDirectory(directory.relativeDirectory);
     getExpiryDate(expiry);
 
@@ -254,6 +257,7 @@ export class GoogleServerStorage extends ServerStorage {
     directory: ObjectDirectory,
     expiry?: ExpiryOptions
   ): Promise<TransferConfig> {
+    assertBaseDirectory(directory.baseDirectory);
     assertRelativeDirectory(directory.relativeDirectory);
     getExpiryDate(expiry);
 

storage/google/src/server/wrappers/StorageControlClientWrapper.ts

@@ -24,6 +24,10 @@ export function isGoogleError(error: unknown): error is GoogleError {
   return error instanceof Error && (error as GoogleError).code !== undefined;
 }
 
+export function escapeCelStringLiteral(value: string): string {
+  return value.replace(/\\/g, "\\\\").replace(/'/g, "\\'");
+}
+
 export class StorageControlClientWrapper {
   private readonly _client: StorageControlClient;
   constructor(private readonly _config: GoogleStorageConfig) {
@@ -117,6 +121,7 @@ export class StorageControlClientWrapper {
     action: GoogleStorageConfigType,
     folderName: string
   ): Promise<GoogleTransferConfig> {
+    const escapedFolderName = escapeCelStringLiteral(folderName);
     const cab = {
       accessBoundary: {
         accessBoundaryRules: [
@@ -125,9 +130,9 @@ export class StorageControlClientWrapper {
             availablePermissions: [roleFromConfigType(action)],
             availabilityCondition: {
               expression:
-                `resource.name.startsWith('${this.bucketPath}/objects/${folderName}') || ` +
+                `resource.name.startsWith('${this.bucketPath}/objects/${escapedFolderName}') || ` +
                 `api.getAttribute('storage.googleapis.com/objectListPrefix', '')` +
-                `.startsWith('${folderName}/')`,
+                `.startsWith('${escapedFolderName}/')`,
             },
           },
         ],

storage/google/src/test/unit/backend/EscapeCelStringLiteral.test.ts

@@ -0,0 +1,30 @@
+/*---------------------------------------------------------------------------------------------
+ * Copyright (c) Bentley Systems, Incorporated. All rights reserved.
+ * See LICENSE.md in the project root for license terms and full copyright notice.
+ *--------------------------------------------------------------------------------------------*/
+
+import { expect } from "chai";
+
+import { escapeCelStringLiteral } from "../../../server/wrappers/StorageControlClientWrapper";
+
+describe("escapeCelStringLiteral", () => {
+  [
+    { input: "folder", expected: "folder" },
+    { input: "folder/sub", expected: "folder/sub" },
+    { input: "foo bar", expected: "foo bar" },
+    { input: "foo`bar", expected: "foo`bar" },
+    { input: "foo'bar", expected: "foo\\'bar" },
+    {
+      input: "x') || resource.name.startsWith('",
+      expected: "x\\') || resource.name.startsWith(\\'",
+    },
+    { input: "back\\slash", expected: "back\\\\slash" },
+    { input: "both'\\end", expected: "both\\'\\\\end" },
+  ].forEach((testCase) => {
+    it(`should escape (${testCase.input})`, () => {
+      expect(escapeCelStringLiteral(testCase.input)).to.equal(
+        testCase.expected
+      );
+    });
+  });
+});

storage/oss/src/server/OssTransferConfigProvider.ts

@@ -4,9 +4,15 @@
  *--------------------------------------------------------------------------------------------*/
 import * as Core from "@alicloud/pop-core";
 
-import { buildObjectDirectoryString } from "@itwin/object-storage-core/lib/common/internal";
+import {
+  assertBaseDirectory,
+  buildObjectDirectoryString,
+} from "@itwin/object-storage-core/lib/common/internal";
 import { getRandomString } from "@itwin/object-storage-core/lib/server/internal";
-import { getExpiresInSeconds } from "@itwin/object-storage-s3/lib/server/internal";
+import {
+  assertPolicyResourceDirectory,
+  getExpiresInSeconds,
+} from "@itwin/object-storage-s3/lib/server/internal";
 
 import {
   ExpiryOptions,
@@ -35,17 +41,16 @@ export class OssTransferConfigProvider implements TransferConfigProvider {
     directory: ObjectDirectory,
     expiry?: ExpiryOptions
   ): Promise<S3TransferConfig> {
+    const directoryPath = buildObjectDirectoryString(directory);
+    assertBaseDirectory(directory.baseDirectory);
+    assertPolicyResourceDirectory(directoryPath);
     const policy = {
       Version: "1",
       Statement: [
         {
           Effect: "Allow",
           Action: ["oss:GetObject"],
-          Resource: [
-            `acs:oss:*:*:${this._config.bucket}/${buildObjectDirectoryString(
-              directory
-            )}/*`,
-          ],
+          Resource: [`acs:oss:*:*:${this._config.bucket}/${directoryPath}/*`],
         },
       ],
     };
@@ -81,17 +86,16 @@ export class OssTransferConfigProvider implements TransferConfigProvider {
     directory: ObjectDirectory,
     expiry?: ExpiryOptions
   ): Promise<S3TransferConfig> {
+    const directoryPath = buildObjectDirectoryString(directory);
+    assertBaseDirectory(directory.baseDirectory);
+    assertPolicyResourceDirectory(directoryPath);
     const policy = {
       Version: "1",
       Statement: [
         {
           Effect: "Allow",
           Action: ["oss:PutObject"],
-          Resource: [
-            `acs:oss:*:*:${this._config.bucket}/${buildObjectDirectoryString(
-              directory
-            )}/*`,
-          ],
+          Resource: [`acs:oss:*:*:${this._config.bucket}/${directoryPath}/*`],
         },
       ],
     };
@@ -128,16 +132,17 @@ export class OssTransferConfigProvider implements TransferConfigProvider {
     expiry?: ExpiryOptions
   ): Promise<S3TransferConfig> {
     const actions = getActions();
+    const directoryPath = buildObjectDirectoryString(directory);
+    assertBaseDirectory(directory.baseDirectory);
+    assertPolicyResourceDirectory(directoryPath);
     const policy = {
       Version: "1",
       Statement: [
         {
           Effect: "Allow",
           Action: actions,
           Resource: [
-            `acs:oss:*:*:${this._config.bucket}/${buildObjectDirectoryString(
-              directory
-            )}/*`,
+            `acs:oss:*:*:${this._config.bucket}/${directoryPath}/*`,
             `acs:oss:*:*:${this._config.bucket}`,
           ],
         },

storage/s3/src/server/S3TransferConfigProvider.ts

@@ -4,7 +4,10 @@
  *--------------------------------------------------------------------------------------------*/
 import { AssumeRoleCommand } from "@aws-sdk/client-sts";
 
-import { buildObjectDirectoryString } from "@itwin/object-storage-core/lib/common/internal";
+import {
+  assertBaseDirectory,
+  buildObjectDirectoryString,
+} from "@itwin/object-storage-core/lib/common/internal";
 import { getRandomString } from "@itwin/object-storage-core/lib/server/internal";
 
 import {
@@ -15,7 +18,11 @@ import {
 
 import { Constants, S3TransferConfig } from "../common";
 
-import { getActions, getExpiresInSeconds } from "./internal";
+import {
+  assertPolicyResourceDirectory,
+  getActions,
+  getExpiresInSeconds,
+} from "./internal";
 import { S3ServerStorageConfig } from "./S3ServerStorage";
 import { StsWrapper } from "./wrappers";
 
@@ -32,18 +39,17 @@ export class S3TransferConfigProvider implements TransferConfigProvider {
     directory: ObjectDirectory,
     options?: ExpiryOptions
   ): Promise<S3TransferConfig> {
+    const directoryPath = buildObjectDirectoryString(directory);
+    assertBaseDirectory(directory.baseDirectory);
+    assertPolicyResourceDirectory(directoryPath);
     /* eslint-disable @typescript-eslint/naming-convention */
     const policy = {
       Version: "2012-10-17",
       Statement: [
         {
           Effect: "Allow",
           Action: ["s3:GetObject"],
-          Resource: [
-            `arn:aws:s3:::${this._config.bucket}/${buildObjectDirectoryString(
-              directory
-            )}/*`,
-          ],
+          Resource: [`arn:aws:s3:::${this._config.bucket}/${directoryPath}/*`],
         },
       ],
     };
@@ -76,18 +82,17 @@ export class S3TransferConfigProvider implements TransferConfigProvider {
     directory: ObjectDirectory,
     options?: ExpiryOptions
   ): Promise<S3TransferConfig> {
+    const directoryPath = buildObjectDirectoryString(directory);
+    assertBaseDirectory(directory.baseDirectory);
+    assertPolicyResourceDirectory(directoryPath);
     /* eslint-disable @typescript-eslint/naming-convention */
     const policy = {
       Version: "2012-10-17",
       Statement: [
         {
           Effect: "Allow",
           Action: ["s3:PutObject"],
-          Resource: [
-            `arn:aws:s3:::${this._config.bucket}/${buildObjectDirectoryString(
-              directory
-            )}/*`,
-          ],
+          Resource: [`arn:aws:s3:::${this._config.bucket}/${directoryPath}/*`],
         },
       ],
     };
@@ -121,6 +126,9 @@ export class S3TransferConfigProvider implements TransferConfigProvider {
     options?: ExpiryOptions
   ): Promise<S3TransferConfig> {
     const actions = getActions();
+    const directoryPath = buildObjectDirectoryString(directory);
+    assertBaseDirectory(directory.baseDirectory);
+    assertPolicyResourceDirectory(directoryPath);
     /* eslint-disable @typescript-eslint/naming-convention */
     const policy = {
       Version: "2012-10-17",
@@ -129,9 +137,7 @@ export class S3TransferConfigProvider implements TransferConfigProvider {
           Effect: "Allow",
           Action: actions,
           Resource: [
-            `arn:aws:s3:::${this._config.bucket}/${buildObjectDirectoryString(
-              directory
-            )}/*`,
+            `arn:aws:s3:::${this._config.bucket}/${directoryPath}/*`,
             `arn:aws:s3:::${this._config.bucket}`,
           ],
         },

storage/s3/src/server/internal/Helpers.ts

@@ -47,3 +47,10 @@ export function getActions(): string[] {
 
   return actions;
 }
+
+export function assertPolicyResourceDirectory(directory: string): void {
+  if (directory.includes("*") || directory.includes("?"))
+    throw new Error(
+      "Directory cannot contain wildcard characters ('*' or '?')."
+    );
+}