Skip to content

Commit

Permalink
fix(s3): Add validations for S3 bucket names (#2256)
Browse files Browse the repository at this point in the history
Bucket names are verified to conform with rules published by S3 -
https://docs.aws.amazon.com/AmazonS3/latest/dev/BucketRestrictions.html

Fixes #1308
  • Loading branch information
nija-at authored and rix0rrr committed Apr 16, 2019
1 parent ec367c8 commit f810265
Show file tree
Hide file tree
Showing 2 changed files with 175 additions and 0 deletions.
38 changes: 38 additions & 0 deletions packages/@aws-cdk/aws-s3/lib/bucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -669,6 +670,9 @@ export class Bucket extends BucketBase {
super(scope, id);

const { bucketEncryption, encryptionKey } = this.parseEncryption(props);
if (props.bucketName && !cdk.Token.unresolved(props.bucketName)) {
this.validateBucketName(props.bucketName);
}

const resource = new CfnBucket(this, 'Resource', {
bucketName: props && props.bucketName,
Expand Down Expand Up @@ -776,6 +780,40 @@ 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');
}
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))) {
errors.push('Bucket name must start and end with a lowercase character or number '
+ '(offset: 0)');
}
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');
}

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.
Expand Down
137 changes: 137 additions & 0 deletions packages/@aws-cdk/aws-s3/test/test.bucket.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -72,6 +73,142 @@ export = {
test.done();
},

'valid bucket names'(test: Test) {
const stack = new cdk.Stack();

test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket1', {
bucketName: 'abc.xyz-34ab'
}));

test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket2', {
bucketName: '124.pp--33'
}));

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 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();

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'
}), /offset: 1/);

test.throws(() => new s3.Bucket(stack, 'MyBucket2', {
bucketName: 'bucKet'
}), /offset: 3/);

test.throws(() => new s3.Bucket(stack, 'MyBucket3', {
bucketName: 'bučket'
}), /offset: 2/);

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'
}), /offset: 0/);

test.throws(() => new s3.Bucket(stack, 'MyBucket2', {
bucketName: 'bucke.'
}), /offset: 5/);

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'
}), /offset: 3/);

test.throws(() => new s3.Bucket(stack, 'MyBucket2', {
bucketName: 'buck.-et'
}), /offset: 4/);

test.throws(() => new s3.Bucket(stack, 'MyBucket3', {
bucketName: 'b-.ucket'
}), /offset: 1/);

test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket4', {
bucketName: 'bu--cket'
}));

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.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket2', {
bucketName: '1.2.3'
}));

test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket3', {
bucketName: '1.2.3.a'
}));

test.doesNotThrow(() => new s3.Bucket(stack, 'MyBucket4', {
bucketName: '1000.2.3.4'
}));

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');
Expand Down

0 comments on commit f810265

Please sign in to comment.