From 4c0a16e881d66c28ceb534995ad2b079f83ab2ec Mon Sep 17 00:00:00 2001
From: James Norton <james.norton@nasa.gov>
Date: Tue, 27 Aug 2024 14:07:15 -0400
Subject: [PATCH] HARMONY-1848: Add additional validation for `bucketPath`
 parameter NOTE: This is a breaking change as the allowd values are now more
 restricted

---
 .../app/frontends/staging-bucket-policy.ts    |  4 ---
 services/harmony/app/markdown/user-bucket.md  |  2 ++
 .../app/middleware/parameter-validation.ts    | 28 +++++++++++++++++
 .../harmony/test/staging-bucket-policy.ts     | 30 ++++++++++++++-----
 4 files changed, 52 insertions(+), 12 deletions(-)

diff --git a/services/harmony/app/frontends/staging-bucket-policy.ts b/services/harmony/app/frontends/staging-bucket-policy.ts
index bea080ccb..c27ae1e1b 100644
--- a/services/harmony/app/frontends/staging-bucket-policy.ts
+++ b/services/harmony/app/frontends/staging-bucket-policy.ts
@@ -46,10 +46,6 @@ export function bucketKeyFromBucketPath(bucketPath: string): [string, string] {
     key = matches[4];
   }
 
-  if (key?.includes('//')) {
-    throw new RequestValidationError(`'${bucketPath}' is not a valid bucket name with optional path`);
-  }
-
   // strip off trailing /
   if (key?.endsWith('/')) {
     key = key.slice(0, -1);
diff --git a/services/harmony/app/markdown/user-bucket.md b/services/harmony/app/markdown/user-bucket.md
index 20e199cfc..b53517dfa 100644
--- a/services/harmony/app/markdown/user-bucket.md
+++ b/services/harmony/app/markdown/user-bucket.md
@@ -33,6 +33,8 @@ The `bucketPath` parameter can be one of the following
 
 The third option is compatible with the `destinationUrl` parameter for requests.
 
+Bucket names must conform to the [AWS bucket naming rules](https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html). The path must conform to the [AWS object key naming guidelines](https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html). Characters listed under "Characters to Avoid" are not supported.
+
 A sample policy generated by the endpoint is shown below:
 
 
diff --git a/services/harmony/app/middleware/parameter-validation.ts b/services/harmony/app/middleware/parameter-validation.ts
index 96da21faa..5b029539e 100644
--- a/services/harmony/app/middleware/parameter-validation.ts
+++ b/services/harmony/app/middleware/parameter-validation.ts
@@ -127,6 +127,33 @@ async function validateDestinationUrlParameter(req: HarmonyRequest): Promise<voi
   }
 }
 
+/**
+ * Validate that the `bucketPath` parameter is a valid S3 url or bucket name with optional path
+ *
+ * @param req - The client request
+ */
+function validateBucketPathParameter(req: HarmonyRequest): void {
+  const bucketPathRegex = /^(?:(?:(?:s|S)?3:\/\/)?([a-z0-9][a-z0-9.-]{1,61}[a-z0-9])(?:(?:\.s3\.amazonaws\.com)?)(?:\/((?:[0-9a-zA-Z.!\-_*'()\/?,+:;=@$& ])*)?)?)$/;
+  const keys = keysToLowerCase(req.query);
+  const bucketPath = keys.bucketpath;
+
+  if (!bucketPath) return;
+
+  const matches = bucketPath.match(bucketPathRegex);
+
+  if (!matches) {
+    throw new RequestValidationError('bucketPath parameter value contains unsupported characters');
+  }
+
+  // check for the case of repeated forward slashes
+  if (matches.length == 3) {
+    const bucketPlusPath = matches[1] + '/' + matches[2];
+    if (bucketPlusPath?.includes('//')) {
+      throw new RequestValidationError('bucketPath parameter value contains repeated forward slashes (//)');
+    }
+  }
+}
+
 /**
  * Validate that the parameter names are correct.
  *  (Performs case insensitive comparison.)
@@ -191,6 +218,7 @@ export default async function parameterValidation(
     validateLinkTypeParameter(req);
     validateNoConflictingGridParameters(req.query);
     await validateDestinationUrlParameter(req);
+    validateBucketPathParameter(req);
     if (req.url.match(RANGESET_ROUTE_REGEX)) {
       validateCoverageRangesetParameterNames(req);
     }
diff --git a/services/harmony/test/staging-bucket-policy.ts b/services/harmony/test/staging-bucket-policy.ts
index 1cad5e8b7..16eaa4948 100644
--- a/services/harmony/test/staging-bucket-policy.ts
+++ b/services/harmony/test/staging-bucket-policy.ts
@@ -161,14 +161,28 @@ describe('staging-bucket-policy route', function () {
     });
 
     describe('and the user provides an invalid bucket path', async function () {
-      hookStagingBucketPolicy({ bucketPath: 'my-bucket//foo' });
-      it('returns a 400 error', function () {
-        expect(this.res.statusCode).to.equal(400);
-      });
-
-      it('returns an appropriate error message', function () {
-        const error = JSON.parse(this.res.text);
-        expect(error.description).to.eql('Error: \'my-bucket//foo\' is not a valid bucket name with optional path');
+      describe('due to repeated forward slashes', async function () {
+        hookStagingBucketPolicy({ bucketPath: 'my-bucket//foo' });
+        it('returns a 400 error', function () {
+          expect(this.res.statusCode).to.equal(400);
+        });
+
+        it('returns an appropriate error message', function () {
+          const error = JSON.parse(this.res.text);
+          expect(error.description).to.eql('Error: bucketPath parameter value contains repeated forward slashes (//)');
+        });
+      });
+
+      describe('due to unsupported characters', async function () {
+        hookStagingBucketPolicy({ bucketPath: '\'"/></script><script>function(){qxss6sxG94mr};</script>' });
+        it('returns a 400 error', function () {
+          expect(this.res.statusCode).to.equal(400);
+        });
+
+        it('returns an appropriate error message', function () {
+          const error = JSON.parse(this.res.text);
+          expect(error.description).to.eql('Error: bucketPath parameter value contains unsupported characters');
+        });
       });
     });
   });