From bd464e54cf592ddba06ebdf1bc63868b2a6bab14 Mon Sep 17 00:00:00 2001 From: Tyler Hillery Date: Sat, 3 May 2025 14:21:38 -0700 Subject: [PATCH 1/6] feat: add bucket name length constraint --- ...0037-add-bucket-name-length-constraint.sql | 6 ++++++ src/internal/database/migrations/types.ts | 1 + src/storage/database/knex.ts | 7 +++++++ src/storage/limits.ts | 5 ++++- src/test/bucket.test.ts | 19 +++++++++++++++++++ 5 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 migrations/tenant/0037-add-bucket-name-length-constraint.sql diff --git a/migrations/tenant/0037-add-bucket-name-length-constraint.sql b/migrations/tenant/0037-add-bucket-name-length-constraint.sql new file mode 100644 index 00000000..48c75352 --- /dev/null +++ b/migrations/tenant/0037-add-bucket-name-length-constraint.sql @@ -0,0 +1,6 @@ +alter table + "storage"."buckets" +add constraint + "buckets_name_length_check" +check + (length(name) > 0 AND length(name) < 64); \ No newline at end of file diff --git a/src/internal/database/migrations/types.ts b/src/internal/database/migrations/types.ts index 224a6f28..86800454 100644 --- a/src/internal/database/migrations/types.ts +++ b/src/internal/database/migrations/types.ts @@ -35,4 +35,5 @@ export const DBMigration = { 'optimize-search-function-v1': 34, 'add-insert-trigger-prefixes': 35, 'optimise-existing-functions': 36, + 'add-bucket-name-length-constraint': 37, } diff --git a/src/storage/database/knex.ts b/src/storage/database/knex.ts index eb511b4c..255ae6a4 100644 --- a/src/storage/database/knex.ts +++ b/src/storage/database/knex.ts @@ -901,6 +901,13 @@ export class DBError extends StorageBackendError implements RenderableError { query, code: pgError.code, }) + case '23514': { + const bucketName = pgError.detail ? pgError.detail.split(' ')[4]?.slice(0, -1) : 'unknown' + return ERRORS.InvalidBucketName(bucketName, pgError).withMetadata({ + query, + code: pgError.code, + }) + } default: return ERRORS.DatabaseError(pgError.message, pgError).withMetadata({ query, diff --git a/src/storage/limits.ts b/src/storage/limits.ts index cff8627d..a3dca091 100644 --- a/src/storage/limits.ts +++ b/src/storage/limits.ts @@ -59,9 +59,12 @@ export function isValidKey(key: string): boolean { export function isValidBucketName(bucketName: string): boolean { // only allow s3 safe characters and characters which require special handling for now // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html + // https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html // excluding / for bucketName return ( - bucketName.length > 0 && /^(\w|!|-|\.|\*|'|\(|\)| |&|\$|@|=|;|:|\+|,|\?)*$/.test(bucketName) + bucketName.length > 0 && + bucketName.length < 64 && + /^(\w|!|-|\.|\*|'|\(|\)| |&|\$|@|=|;|:|\+|,|\?)*$/.test(bucketName) ) } diff --git a/src/test/bucket.test.ts b/src/test/bucket.test.ts index e8111c63..84b74649 100644 --- a/src/test/bucket.test.ts +++ b/src/test/bucket.test.ts @@ -211,6 +211,25 @@ describe('testing POST bucket', () => { }) expect(response.statusCode).toBe(400) }) + + test('user is not able to create a bucket with a name longer than 63 characters', async () => { + const longBucketName = 'a'.repeat(64) + const response = await app().inject({ + method: 'POST', + url: `/bucket`, + headers: { + authorization: `Bearer ${process.env.AUTHENTICATED_KEY}`, + }, + payload: { + name: longBucketName, + }, + }) + expect(response.json()).toEqual({ + error: 'Invalid Input', + message: 'Bucket name invalid', + statusCode: '400', + }) + }) }) /* From 0303f32de23fd38b5385348101f833ccf39ae395 Mon Sep 17 00:00:00 2001 From: Tyler Hillery Date: Tue, 6 May 2025 18:29:31 -0700 Subject: [PATCH 2/6] fix: remove DBError for postgres check constraint --- src/storage/database/knex.ts | 7 ------- src/storage/limits.ts | 3 ++- src/test/bucket.test.ts | 6 +----- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/src/storage/database/knex.ts b/src/storage/database/knex.ts index 255ae6a4..eb511b4c 100644 --- a/src/storage/database/knex.ts +++ b/src/storage/database/knex.ts @@ -901,13 +901,6 @@ export class DBError extends StorageBackendError implements RenderableError { query, code: pgError.code, }) - case '23514': { - const bucketName = pgError.detail ? pgError.detail.split(' ')[4]?.slice(0, -1) : 'unknown' - return ERRORS.InvalidBucketName(bucketName, pgError).withMetadata({ - query, - code: pgError.code, - }) - } default: return ERRORS.DatabaseError(pgError.message, pgError).withMetadata({ query, diff --git a/src/storage/limits.ts b/src/storage/limits.ts index a3dca091..18c8e06e 100644 --- a/src/storage/limits.ts +++ b/src/storage/limits.ts @@ -58,9 +58,10 @@ export function isValidKey(key: string): boolean { */ export function isValidBucketName(bucketName: string): boolean { // only allow s3 safe characters and characters which require special handling for now + // the length (and slash) restriction come from bucket naming rules + // and the rest of the validation rules are based on S3 object key validation. // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html // https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html - // excluding / for bucketName return ( bucketName.length > 0 && bucketName.length < 64 && diff --git a/src/test/bucket.test.ts b/src/test/bucket.test.ts index 84b74649..fdc4c0c4 100644 --- a/src/test/bucket.test.ts +++ b/src/test/bucket.test.ts @@ -224,11 +224,7 @@ describe('testing POST bucket', () => { name: longBucketName, }, }) - expect(response.json()).toEqual({ - error: 'Invalid Input', - message: 'Bucket name invalid', - statusCode: '400', - }) + expect(response.statusCode).toBe(400) }) }) From cc5f9c7966d7b47deabffa258c41913ce3e39566 Mon Sep 17 00:00:00 2001 From: Tyler Hillery Date: Wed, 7 May 2025 13:56:03 -0700 Subject: [PATCH 3/6] refactor: change bucket name max length to 100 --- migrations/tenant/0037-add-bucket-name-length-constraint.sql | 2 +- src/storage/limits.ts | 4 ++-- src/test/bucket.test.ts | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/migrations/tenant/0037-add-bucket-name-length-constraint.sql b/migrations/tenant/0037-add-bucket-name-length-constraint.sql index 48c75352..9971d835 100644 --- a/migrations/tenant/0037-add-bucket-name-length-constraint.sql +++ b/migrations/tenant/0037-add-bucket-name-length-constraint.sql @@ -3,4 +3,4 @@ alter table add constraint "buckets_name_length_check" check - (length(name) > 0 AND length(name) < 64); \ No newline at end of file + (length(name) > 0 AND length(name) < 101); \ No newline at end of file diff --git a/src/storage/limits.ts b/src/storage/limits.ts index 18c8e06e..5f1157c7 100644 --- a/src/storage/limits.ts +++ b/src/storage/limits.ts @@ -58,13 +58,13 @@ export function isValidKey(key: string): boolean { */ export function isValidBucketName(bucketName: string): boolean { // only allow s3 safe characters and characters which require special handling for now - // the length (and slash) restriction come from bucket naming rules + // the slash restriction come from bucket naming rules // and the rest of the validation rules are based on S3 object key validation. // https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html // https://docs.aws.amazon.com/AmazonS3/latest/userguide/bucketnamingrules.html return ( bucketName.length > 0 && - bucketName.length < 64 && + bucketName.length < 101 && /^(\w|!|-|\.|\*|'|\(|\)| |&|\$|@|=|;|:|\+|,|\?)*$/.test(bucketName) ) } diff --git a/src/test/bucket.test.ts b/src/test/bucket.test.ts index fdc4c0c4..2235dab0 100644 --- a/src/test/bucket.test.ts +++ b/src/test/bucket.test.ts @@ -212,8 +212,8 @@ describe('testing POST bucket', () => { expect(response.statusCode).toBe(400) }) - test('user is not able to create a bucket with a name longer than 63 characters', async () => { - const longBucketName = 'a'.repeat(64) + test('user is not able to create a bucket with a name longer than 101 characters', async () => { + const longBucketName = 'a'.repeat(101) const response = await app().inject({ method: 'POST', url: `/bucket`, From dd17df916234abc033ade42b184de031cb7625c6 Mon Sep 17 00:00:00 2001 From: Tyler Hillery Date: Sun, 11 May 2025 12:24:54 -0700 Subject: [PATCH 4/6] refactor: add bucket name length trigger --- .../0037-add-bucket-name-length-constraint.sql | 6 ------ .../0037-add-bucket-name-length-trigger.sql | 15 +++++++++++++++ src/internal/database/migrations/types.ts | 6 +++--- 3 files changed, 18 insertions(+), 9 deletions(-) delete mode 100644 migrations/tenant/0037-add-bucket-name-length-constraint.sql create mode 100644 migrations/tenant/0037-add-bucket-name-length-trigger.sql diff --git a/migrations/tenant/0037-add-bucket-name-length-constraint.sql b/migrations/tenant/0037-add-bucket-name-length-constraint.sql deleted file mode 100644 index 9971d835..00000000 --- a/migrations/tenant/0037-add-bucket-name-length-constraint.sql +++ /dev/null @@ -1,6 +0,0 @@ -alter table - "storage"."buckets" -add constraint - "buckets_name_length_check" -check - (length(name) > 0 AND length(name) < 101); \ No newline at end of file diff --git a/migrations/tenant/0037-add-bucket-name-length-trigger.sql b/migrations/tenant/0037-add-bucket-name-length-trigger.sql new file mode 100644 index 00000000..95e1cc08 --- /dev/null +++ b/migrations/tenant/0037-add-bucket-name-length-trigger.sql @@ -0,0 +1,15 @@ +create or replace function storage.enforce_bucket_name_length() +returns trigger as $$ +begin + if length(new.name) > 100 then + raise exception 'bucket name "%" is too long (% characters). Max is 100.', new.name, length(new.name); + end if; + return new; +end; +$$ language plpgsql; + + +drop trigger if exists enforce_bucket_name_length_trigger on storage.buckets; +create trigger enforce_bucket_name_length_trigger +before insert or update of name on storage.buckets +for each row execute function storage.enforce_bucket_name_length(); diff --git a/src/internal/database/migrations/types.ts b/src/internal/database/migrations/types.ts index 86800454..88cfdeab 100644 --- a/src/internal/database/migrations/types.ts +++ b/src/internal/database/migrations/types.ts @@ -1,5 +1,5 @@ export const DBMigration = { - initialmigration: 1, + 'initialmigration': 1, 'search-files-search-function': 2, 'storage-schema': 3, 'pathtoken-column': 4, @@ -16,7 +16,7 @@ export const DBMigration = { 'add-can-insert-object-function': 15, 'add-version': 16, 'drop-owner-foreign-key': 17, - add_owner_id_column_deprecate_owner: 18, + 'add_owner_id_column_deprecate_owner': 18, 'alter-default-value-objects-id': 19, 'list-objects-with-delimiter': 20, 's3-multipart-uploads': 21, @@ -35,5 +35,5 @@ export const DBMigration = { 'optimize-search-function-v1': 34, 'add-insert-trigger-prefixes': 35, 'optimise-existing-functions': 36, - 'add-bucket-name-length-constraint': 37, + 'add-bucket-name-length-trigger': 37, } From c6f02221792d80febe6dedd0116cc808dfa60c34 Mon Sep 17 00:00:00 2001 From: Tyler Hillery Date: Sun, 11 May 2025 12:26:33 -0700 Subject: [PATCH 5/6] fix: update bucket name length test to reflect max length of 100 characters --- src/test/bucket.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/bucket.test.ts b/src/test/bucket.test.ts index 2235dab0..f49feef0 100644 --- a/src/test/bucket.test.ts +++ b/src/test/bucket.test.ts @@ -212,7 +212,7 @@ describe('testing POST bucket', () => { expect(response.statusCode).toBe(400) }) - test('user is not able to create a bucket with a name longer than 101 characters', async () => { + test('user is not able to create a bucket with a name longer than 100 characters', async () => { const longBucketName = 'a'.repeat(101) const response = await app().inject({ method: 'POST', From a9d85f794591050c396695cf0241073adb9d374f Mon Sep 17 00:00:00 2001 From: Tyler Hillery Date: Mon, 12 May 2025 07:00:20 -0700 Subject: [PATCH 6/6] revert: db migration types --- src/internal/database/migrations/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/internal/database/migrations/types.ts b/src/internal/database/migrations/types.ts index 88cfdeab..c109e5be 100644 --- a/src/internal/database/migrations/types.ts +++ b/src/internal/database/migrations/types.ts @@ -1,5 +1,5 @@ export const DBMigration = { - 'initialmigration': 1, + initialmigration: 1, 'search-files-search-function': 2, 'storage-schema': 3, 'pathtoken-column': 4, @@ -16,7 +16,7 @@ export const DBMigration = { 'add-can-insert-object-function': 15, 'add-version': 16, 'drop-owner-foreign-key': 17, - 'add_owner_id_column_deprecate_owner': 18, + add_owner_id_column_deprecate_owner: 18, 'alter-default-value-objects-id': 19, 'list-objects-with-delimiter': 20, 's3-multipart-uploads': 21,