From 3ac4f53935464ae3c39eec4fbae70db4d7e473b9 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 12 Apr 2019 11:24:13 +0100 Subject: [PATCH 1/6] fix(s3): Add validations for S3 bucket names Bucket names are verified to conform with rules published by S3 - https://docs.aws.amazon.com/AmazonS3/latest/dev/BucketRestrictions.html fixes #1308 --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 29 ++++++ packages/@aws-cdk/aws-s3/test/test.bucket.ts | 100 +++++++++++++++++++ 2 files changed, 129 insertions(+) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 5357dbe754dc4..124a655a70cd4 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -3,6 +3,7 @@ import iam = require('@aws-cdk/aws-iam'); import kms = require('@aws-cdk/aws-kms'); import { IBucketNotificationDestination } from '@aws-cdk/aws-s3-notifications'; import cdk = require('@aws-cdk/cdk'); +import { EOL } from 'os'; import { BucketPolicy } from './bucket-policy'; import { BucketNotifications } from './notifications-resource'; import perms = require('./perms'); @@ -669,6 +670,9 @@ export class Bucket extends BucketBase { super(scope, id); const { bucketEncryption, encryptionKey } = this.parseEncryption(props); + if (props.bucketName) { + this.validateBucketName(props.bucketName); + } const resource = new CfnBucket(this, 'Resource', { bucketName: props && props.bucketName, @@ -776,6 +780,31 @@ export class Bucket extends BucketBase { return this.onEvent(EventType.ObjectRemoved, dest, ...filters); } + private validateBucketName(bucketName: string) { + const errors: string[] = []; + + // Rules codified from https://docs.aws.amazon.com/AmazonS3/latest/dev/BucketRestrictions.html + if (bucketName.length < 3 || bucketName.length > 63) { + errors.push('Bucket name must be at least 3 and no more than 63 characters'); + } + if (!/^[a-z0-9.-]+$/.test(bucketName)) { + errors.push('Bucket name must only contain lowercase characters and the symbols, period (.) and dash (-)'); + } + if (!/[a-z0-9]/.test(bucketName.charAt(0)) || !/[a-z0-9]/.test(bucketName.charAt(bucketName.length - 1))) { + errors.push('Bucket name must start and end with a lowercase character or number'); + } + if (/\.-|-\.|\.\./.test(bucketName)) { + errors.push('Bucket name must not have dash next to period, or period next to dash, or consecutive periods'); + } + if (/\d+\.\d+\.\d+\.\d+/.test(bucketName)) { + errors.push('Bucket name must not resemble an IP address'); + } + + if (errors.length > 0) { + throw new Error(`Invalid S3 bucket name (value: ${bucketName})${EOL}${errors.join(EOL)}`); + } + } + /** * Set up key properties and return the Bucket encryption property from the * user's configuration. diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 8789d97ceb91e..21e7f91d22623 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -72,6 +72,106 @@ export = { test.done(); }, + 'valid bucket names'(test: Test) { + const stack = new cdk.Stack(); + + test.ok(() => new s3.Bucket(stack, 'MyBucket1', { + bucketName: 'abc.xyz-34ab' + })); + + test.ok(() => new s3.Bucket(stack, 'MyBucket1', { + bucketName: '124.pp--33' + })); + + test.done(); + }, + + 'fails if bucket name has less than 3 or more than 63 characters'(test: Test) { + const stack = new cdk.Stack(); + + test.throws(() => new s3.Bucket(stack, 'MyBucket1', { + bucketName: 'a' + }), /at least 3/); + + test.throws(() => new s3.Bucket(stack, 'MyBucket2', { + bucketName: new Array(65).join('x') + }), /no more than 63/); + + test.done(); + }, + + 'fails if bucket name has invalid characters'(test: Test) { + const stack = new cdk.Stack(); + + test.throws(() => new s3.Bucket(stack, 'MyBucket1', { + bucketName: 'b@cket' + }), /only contain lowercase characters and the symbols/); + + test.throws(() => new s3.Bucket(stack, 'MyBucket2', { + bucketName: 'bUcket' + }), /only contain lowercase characters and the symbols/); + + test.throws(() => new s3.Bucket(stack, 'MyBucket3', { + bucketName: 'bücket' + }), /only contain lowercase characters and the symbols/); + + test.done(); + }, + + 'fails if bucket name does not start or end with lowercase character or number'(test: Test) { + const stack = new cdk.Stack(); + + test.throws(() => new s3.Bucket(stack, 'MyBucket1', { + bucketName: '-ucket' + }), /must start and end with a lowercase character or number/); + + test.throws(() => new s3.Bucket(stack, 'MyBucket2', { + bucketName: 'bucke.' + }), /must start and end with a lowercase character or number/); + + test.done(); + }, + + 'fails only if bucket name has the consecutive symbols (..), (.-), (-.)'(test: Test) { + const stack = new cdk.Stack(); + + test.throws(() => new s3.Bucket(stack, 'MyBucket1', { + bucketName: 'buc..ket' + }), /must not have dash next to period, or period next to dash, or consecutive periods/); + + test.throws(() => new s3.Bucket(stack, 'MyBucket2', { + bucketName: 'buc.-ket' + }), /must not have dash next to period, or period next to dash, or consecutive periods/); + + test.throws(() => new s3.Bucket(stack, 'MyBucket3', { + bucketName: 'buc-.ket' + }), /must not have dash next to period, or period next to dash, or consecutive periods/); + + test.ok(() => new s3.Bucket(stack, 'MyBucket4', { + bucketName: 'buc--ket' + })); + + test.done(); + }, + + 'fails only if bucket name resembles IP address'(test: Test) { + const stack = new cdk.Stack(); + + test.throws(() => new s3.Bucket(stack, 'MyBucket1', { + bucketName: '1.2.3.4' + }), /must not resemble an IP address/); + + test.ok(() => new s3.Bucket(stack, 'MyBucket2', { + bucketName: '1.2.3' + })); + + test.ok(() => new s3.Bucket(stack, 'MyBucket3', { + bucketName: '1.2.3.a' + })); + + test.done(); + }, + 'fails if encryption key is used with managed encryption'(test: Test) { const stack = new cdk.Stack(); const myKey = new kms.EncryptionKey(stack, 'MyKey'); From 8e74077cec9adf9dc811146c17b3f09846907351 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 12 Apr 2019 16:32:10 +0100 Subject: [PATCH 2/6] Replaced `ok` assertions with `doesNotThrow`. Misused the wrong assertion method previously. `ok` assertions don't fail when the conditional throws an exception. --- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 21e7f91d22623..3853328eb92c9 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -75,11 +75,11 @@ export = { 'valid bucket names'(test: Test) { const stack = new cdk.Stack(); - test.ok(() => new s3.Bucket(stack, 'MyBucket1', { + test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket1', { bucketName: 'abc.xyz-34ab' })); - test.ok(() => new s3.Bucket(stack, 'MyBucket1', { + test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket2', { bucketName: '124.pp--33' })); @@ -147,7 +147,7 @@ export = { bucketName: 'buc-.ket' }), /must not have dash next to period, or period next to dash, or consecutive periods/); - test.ok(() => new s3.Bucket(stack, 'MyBucket4', { + test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket4', { bucketName: 'buc--ket' })); @@ -161,11 +161,11 @@ export = { bucketName: '1.2.3.4' }), /must not resemble an IP address/); - test.ok(() => new s3.Bucket(stack, 'MyBucket2', { + test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket2', { bucketName: '1.2.3' })); - test.ok(() => new s3.Bucket(stack, 'MyBucket3', { + test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket3', { bucketName: '1.2.3.a' })); From de5c283229349564d646a13d5a36d1c109b93041 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 12 Apr 2019 16:33:55 +0100 Subject: [PATCH 3/6] Skip bucket name validation checks for CDK tokens --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 2 +- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 124a655a70cd4..66b484f51ffce 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -670,7 +670,7 @@ export class Bucket extends BucketBase { super(scope, id); const { bucketEncryption, encryptionKey } = this.parseEncryption(props); - if (props.bucketName) { + if (props.bucketName && !cdk.Token.unresolved(props.bucketName)) { this.validateBucketName(props.bucketName); } diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 3853328eb92c9..6bd1e7e4466d7 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -86,6 +86,16 @@ export = { test.done(); }, + 'bucket validation skips tokenized values'(test: Test) { + const stack = new cdk.Stack(); + + test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket', { + bucketName: new cdk.Token(() => '_BUCKET').toString() + })); + + test.done(); + }, + 'fails if bucket name has less than 3 or more than 63 characters'(test: Test) { const stack = new cdk.Stack(); From e19b859ee0b0aefbeff056c26b414a1920ea4075 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 12 Apr 2019 16:41:46 +0100 Subject: [PATCH 4/6] Fixed regexp for IP address to match entire string --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 2 +- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 66b484f51ffce..e2f5a249f1e82 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -796,7 +796,7 @@ export class Bucket extends BucketBase { if (/\.-|-\.|\.\./.test(bucketName)) { errors.push('Bucket name must not have dash next to period, or period next to dash, or consecutive periods'); } - if (/\d+\.\d+\.\d+\.\d+/.test(bucketName)) { + if (/^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(bucketName)) { errors.push('Bucket name must not resemble an IP address'); } diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 6bd1e7e4466d7..1b96c21c79897 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -179,6 +179,10 @@ export = { bucketName: '1.2.3.a' })); + test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket4', { + bucketName: '1000.2.3.4' + })); + test.done(); }, From c9cdd27a1ca1f47ec514da1ebb70683265ab0412 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 12 Apr 2019 19:30:18 +0100 Subject: [PATCH 5/6] Error messages now include offending offset Each class of failed bucket name validation shows the offset of the offending character. --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 21 +++++++++++----- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 26 ++++++++++---------- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index e2f5a249f1e82..ed071d793276b 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -787,14 +787,23 @@ export class Bucket extends BucketBase { if (bucketName.length < 3 || bucketName.length > 63) { errors.push('Bucket name must be at least 3 and no more than 63 characters'); } - if (!/^[a-z0-9.-]+$/.test(bucketName)) { - errors.push('Bucket name must only contain lowercase characters and the symbols, period (.) and dash (-)'); + const charsetMatch = bucketName.match(/[^a-z0-9.-]/); + if (charsetMatch) { + errors.push('Bucket name must only contain lowercase characters and the symbols, period (.) and dash (-) ' + + `(offset: ${charsetMatch.index})`); } - if (!/[a-z0-9]/.test(bucketName.charAt(0)) || !/[a-z0-9]/.test(bucketName.charAt(bucketName.length - 1))) { - errors.push('Bucket name must start and end with a lowercase character or number'); + if (!/[a-z0-9]/.test(bucketName.charAt(0))) { + errors.push('Bucket name must start and end with a lowercase character or number ' + + '(offset: 0)'); } - if (/\.-|-\.|\.\./.test(bucketName)) { - errors.push('Bucket name must not have dash next to period, or period next to dash, or consecutive periods'); + if (!/[a-z0-9]/.test(bucketName.charAt(bucketName.length - 1))) { + errors.push('Bucket name must start and end with a lowercase character or number ' + + `(offset: ${bucketName.length - 1})`); + } + const consecSymbolMatch = bucketName.match(/\.-|-\.|\.\./); + if (consecSymbolMatch) { + errors.push('Bucket name must not have dash next to period, or period next to dash, or consecutive periods ' + + `(offset: ${consecSymbolMatch.index})`); } if (/^\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.test(bucketName)) { errors.push('Bucket name must not resemble an IP address'); diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 1b96c21c79897..0f2728e8e1392 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -115,15 +115,15 @@ export = { test.throws(() => new s3.Bucket(stack, 'MyBucket1', { bucketName: 'b@cket' - }), /only contain lowercase characters and the symbols/); + }), /offset: 1/); test.throws(() => new s3.Bucket(stack, 'MyBucket2', { - bucketName: 'bUcket' - }), /only contain lowercase characters and the symbols/); + bucketName: 'bucKet' + }), /offset: 3/); test.throws(() => new s3.Bucket(stack, 'MyBucket3', { - bucketName: 'bücket' - }), /only contain lowercase characters and the symbols/); + bucketName: 'bučket' + }), /offset: 2/); test.done(); }, @@ -133,11 +133,11 @@ export = { test.throws(() => new s3.Bucket(stack, 'MyBucket1', { bucketName: '-ucket' - }), /must start and end with a lowercase character or number/); + }), /offset: 0/); test.throws(() => new s3.Bucket(stack, 'MyBucket2', { bucketName: 'bucke.' - }), /must start and end with a lowercase character or number/); + }), /offset: 5/); test.done(); }, @@ -147,18 +147,18 @@ export = { test.throws(() => new s3.Bucket(stack, 'MyBucket1', { bucketName: 'buc..ket' - }), /must not have dash next to period, or period next to dash, or consecutive periods/); + }), /offset: 3/); test.throws(() => new s3.Bucket(stack, 'MyBucket2', { - bucketName: 'buc.-ket' - }), /must not have dash next to period, or period next to dash, or consecutive periods/); + bucketName: 'buck.-et' + }), /offset: 4/); test.throws(() => new s3.Bucket(stack, 'MyBucket3', { - bucketName: 'buc-.ket' - }), /must not have dash next to period, or period next to dash, or consecutive periods/); + bucketName: 'b-.ucket' + }), /offset: 1/); test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket4', { - bucketName: 'buc--ket' + bucketName: 'bu--cket' })); test.done(); From a040f09253d54bb3ab9ee5e052cb210bca917901 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Sun, 14 Apr 2019 18:18:47 +0100 Subject: [PATCH 6/6] Additional test that validates the entire error message. As suggested on the PR review, current set of tests only validate that the offsets are computed correctly. This test verifies that the message is generated as expected. --- packages/@aws-cdk/aws-s3/test/test.bucket.ts | 23 ++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/@aws-cdk/aws-s3/test/test.bucket.ts b/packages/@aws-cdk/aws-s3/test/test.bucket.ts index 0f2728e8e1392..eb23be105fc90 100644 --- a/packages/@aws-cdk/aws-s3/test/test.bucket.ts +++ b/packages/@aws-cdk/aws-s3/test/test.bucket.ts @@ -3,6 +3,7 @@ import iam = require('@aws-cdk/aws-iam'); import kms = require('@aws-cdk/aws-kms'); import cdk = require('@aws-cdk/cdk'); import { Test } from 'nodeunit'; +import { EOL } from 'os'; import s3 = require('../lib'); // to make it easy to copy & paste from output: @@ -96,6 +97,28 @@ export = { test.done(); }, + 'fails with message on invalid bucket names'(test: Test) { + const stack = new cdk.Stack(); + const bucket = `-buckEt.-${new Array(65).join('$')}`; + const expectedErrors = [ + `Invalid S3 bucket name (value: ${bucket})`, + 'Bucket name must be at least 3 and no more than 63 characters', + 'Bucket name must only contain lowercase characters and the symbols, period (.) and dash (-) (offset: 5)', + 'Bucket name must start and end with a lowercase character or number (offset: 0)', + `Bucket name must start and end with a lowercase character or number (offset: ${bucket.length - 1})`, + 'Bucket name must not have dash next to period, or period next to dash, or consecutive periods (offset: 7)', + ].join(EOL); + + test.throws(() => new s3.Bucket(stack, 'MyBucket', { + bucketName: bucket + // tslint:disable-next-line:only-arrow-functions + }), function(err: Error) { + return expectedErrors === err.message; + }); + + test.done(); + }, + 'fails if bucket name has less than 3 or more than 63 characters'(test: Test) { const stack = new cdk.Stack();