fix: correct errors revealed by testing a deployment

Author: pjhartzell

README.md

@@ -1172,7 +1172,9 @@ When asset proxying is enabled, two endpoints are available for accessing proxie
 
 ### IAM Permissions
 
-For the Asset Proxy feature to generate pre-signed URLs, the API and ingest Lambdas must be assigned permissions for the S3 buckets containing the assets. Add the following to the IAM role statements in your `serverless.yml` file, adjusting the resources as needed:
+For the Asset Proxy feature to generate pre-signed URLs, the API and ingest Lambdas must
+be assigned permissions for the S3 buckets containing the assets. Add the following to the
+IAM role statements in your `serverless.yml` file, adjusting the resources as needed:
 
 For the `LIST` mode, you can specify the buckets listed in `ASSET_PROXY_BUCKET_LIST`:
 
@@ -1186,6 +1188,7 @@ For the `LIST` mode, you can specify the buckets listed in `ASSET_PROXY_BUCKET_L
 - Effect: Allow
   Action:
     - s3:HeadBucket
+    - s3:ListBucket
   Resource:
     - "arn:aws:s3:::my-bucket-1"
     - "arn:aws:s3:::my-bucket-2"
@@ -1201,10 +1204,12 @@ For the `ALL` mode, use wildcards:
 - Effect: Allow
   Action:
     - s3:HeadBucket
+    - s3:ListBucket
   Resource: "arn:aws:s3:::*"
 ```
 
-When using `ALL_BUCKETS_IN_ACCOUNT` mode, the Lambda also needs permission to list buckets:
+When using `ALL_BUCKETS_IN_ACCOUNT` mode, the Lambda also needs permission to list the
+account buckets:
 
 ```yaml
 - Effect: Allow
@@ -1214,6 +1219,7 @@ When using `ALL_BUCKETS_IN_ACCOUNT` mode, the Lambda also needs permission to li
 - Effect: Allow
   Action:
     - s3:HeadBucket
+    - s3:ListBucket
   Resource: "arn:aws:s3:::*"
 - Effect: Allow
   Action:

src/lib/asset-buckets.js

@@ -5,7 +5,8 @@ import {
 import { s3 } from './aws-clients.js'
 import logger from './logger.js'
 
-const s3Client = s3()
+// Follow, rather than throw, HeadBucket redirects for buckets not in the client's region
+const s3Client = s3({ followRegionRedirects: true })
 
 export const BucketOptionEnum = Object.freeze({
   NONE: 'NONE',
@@ -56,9 +57,10 @@ export class AssetBuckets {
           )
         }
 
-        const count = Object.keys(this.bucketCache).length
+        const bucketNames = Object.keys(this.bucketCache)
         logger.info(
-          `Parsed ${count} buckets from ASSET_PROXY_BUCKET_LIST for asset proxy`
+          `Parsed ${bucketNames.length} buckets from ASSET_PROXY_BUCKET_LIST `
+          + `for asset proxy: ${bucketNames.join(', ')}`
         )
       } else {
         throw new Error(
@@ -80,9 +82,10 @@ export class AssetBuckets {
           .map(async (name) => { await this.getBucket(name) })
       )
 
-      const count = Object.keys(this.bucketCache).length
+      const bucketNames = Object.keys(this.bucketCache)
       logger.info(
-        `Fetched ${count} buckets from AWS account for asset proxy`
+        `Fetched ${bucketNames.length} buckets from AWS account `
+        + `for asset proxy: ${bucketNames.join(', ')}`
       )
       break
     }
@@ -99,29 +102,33 @@ export class AssetBuckets {
   async getBucket(bucketName) {
     if (!(bucketName in this.bucketCache)) {
       const command = new HeadBucketCommand({ Bucket: bucketName })
-      const response = await s3Client.send(command)
-      const statusCode = response.$metadata.httpStatusCode
       let name = null
       let region = null
 
-      switch (statusCode) {
-      case 200:
+      try {
+        const response = await s3Client.send(command)
         name = bucketName
         region = response.BucketRegion === 'EU'
           ? 'eu-west-1'
           : response.BucketRegion || 'us-east-1'
-        break
-      case 403:
-        logger.warn(`Access denied to bucket ${bucketName}`)
-        break
-      case 404:
-        logger.warn(`Bucket ${bucketName} does not exist`)
-        break
-      case 400:
-        logger.warn(`Bad request for bucket ${bucketName}`)
-        break
-      default:
-        logger.warn(`Unexpected status code ${statusCode} for bucket ${bucketName}`)
+      } catch (err) {
+        const error = /** @type {any} */ (err)
+        const statusCode = error.$metadata?.httpStatusCode
+
+        switch (statusCode) {
+        case 403:
+          logger.warn(`Access denied to bucket ${bucketName}`)
+          break
+        case 404:
+          logger.warn(`Bucket ${bucketName} does not exist`)
+          break
+        case 400:
+          logger.warn(`Bad request for bucket ${bucketName}`)
+          break
+        default:
+          logger.error(`Unexpected error for bucket ${bucketName}:`, error)
+          throw error
+        }
       }
 
       this.bucketCache[bucketName] = { name, region }

tests/unit/test-asset-buckets.js

@@ -50,7 +50,8 @@ test('AssetBuckets - LIST mode throws if bucket list is null', async (t) => {
 })
 
 test('AssetBuckets - LIST mode throws if bucket is inaccessible', async (t) => {
-  s3Mock.on(HeadBucketCommand).resolves({
+  s3Mock.on(HeadBucketCommand).rejects({
+    name: '403',
     $metadata: { httpStatusCode: 403 }
   })
 
@@ -130,7 +131,8 @@ test('AssetBuckets - shouldProxyBucket with ALL_BUCKETS_IN_ACCOUNT mode only pro
 
 // Using serial to prevent HeadBucketCommand mock interference between tests
 test.serial('AssetBuckets - getBucket handles 403 access denied', async (t) => {
-  s3Mock.on(HeadBucketCommand).resolves({
+  s3Mock.on(HeadBucketCommand).rejects({
+    name: '403',
     $metadata: { httpStatusCode: 403 }
   })
 
@@ -143,7 +145,8 @@ test.serial('AssetBuckets - getBucket handles 403 access denied', async (t) => {
 
 // Using serial to prevent HeadBucketCommand mock interference between tests
 test.serial('AssetBuckets - getBucket handles 404 not found', async (t) => {
-  s3Mock.on(HeadBucketCommand).resolves({
+  s3Mock.on(HeadBucketCommand).rejects({
+    name: '404',
     $metadata: { httpStatusCode: 404 }
   })