Skip to content

Commit

Permalink
Merge branch 'main' into main
Browse files Browse the repository at this point in the history
  • Loading branch information
frankinspace authored Aug 30, 2024
2 parents ca3a58c + 1d80b2b commit 8f6cff7
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 12 deletions.
4 changes: 0 additions & 4 deletions services/harmony/app/frontends/staging-bucket-policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions services/harmony/app/markdown/user-bucket.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:


Expand Down
28 changes: 28 additions & 0 deletions services/harmony/app/middleware/parameter-validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.)
Expand Down Expand Up @@ -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);
}
Expand Down
30 changes: 22 additions & 8 deletions services/harmony/test/staging-bucket-policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
});
});
});
Expand Down

0 comments on commit 8f6cff7

Please sign in to comment.