Merge pull request #10032 from weseek/imprv/type-safe-configuration-for-file-uploading

imprv: Type safe configuration for file uploading

apps/app/src/server/routes/apiv3/app-settings.js

@@ -1,4 +1,6 @@
-import { ConfigSource } from '@growi/core/dist/interfaces';
+import {
+  ConfigSource, toNonBlankString, toNonBlankStringOrUndefined,
+} from '@growi/core/dist/interfaces';
 import { ErrorV3 } from '@growi/core/dist/models';
 import { body } from 'express-validator';
 
@@ -367,6 +369,7 @@ module.exports = (crowi) => {
       body('gcsBucket').trim(),
       body('gcsUploadNamespace').trim(),
       body('gcsReferenceFileWithRelayMode').if(value => value != null).isBoolean(),
+      body('s3Bucket').trim(),
       body('s3Region')
         .trim()
         .if(value => value !== '')
@@ -387,7 +390,6 @@ module.exports = (crowi) => {
           }
           return true;
         }),
-      body('s3Bucket').trim(),
       body('s3AccessKeyId').trim().if(value => value !== '').matches(/^[\da-zA-Z]+$/),
       body('s3SecretAccessKey').trim(),
       body('s3ReferenceFileWithRelayMode').if(value => value != null).isBoolean(),
@@ -888,42 +890,88 @@ module.exports = (crowi) => {
   router.put('/file-upload-setting', loginRequiredStrictly, adminRequired, addActivity, validator.fileUploadSetting, apiV3FormValidator, async(req, res) => {
     const { fileUploadType } = req.body;
 
-    const requestParams = {
-      'app:fileUploadType': fileUploadType,
-    };
-
-    if (fileUploadType === 'gcs') {
-      requestParams['gcs:apiKeyJsonPath'] = req.body.gcsApiKeyJsonPath;
-      requestParams['gcs:bucket'] = req.body.gcsBucket;
-      requestParams['gcs:uploadNamespace'] = req.body.gcsUploadNamespace;
-      requestParams['gcs:referenceFileWithRelayMode'] = req.body.gcsReferenceFileWithRelayMode;
+    if (fileUploadType === 'aws') {
+      try {
+        try {
+          toNonBlankString(req.body.s3Bucket);
+        }
+        catch (err) {
+          throw new Error('S3 Bucket name is required');
+        }
+        try {
+          toNonBlankString(req.body.s3Region);
+        }
+        catch (err) {
+          throw new Error('S3 Region is required');
+        }
+        await configManager.updateConfigs({
+          'app:fileUploadType': fileUploadType,
+          'aws:s3Region': toNonBlankString(req.body.s3Region),
+          'aws:s3Bucket': toNonBlankString(req.body.s3Bucket),
+          'aws:referenceFileWithRelayMode': req.body.s3ReferenceFileWithRelayMode,
+        },
+        { skipPubsub: true });
+        await configManager.updateConfigs({
+          'app:s3CustomEndpoint': toNonBlankStringOrUndefined(req.body.s3CustomEndpoint),
+          'aws:s3AccessKeyId': toNonBlankStringOrUndefined(req.body.s3AccessKeyId),
+          'aws:s3SecretAccessKey': toNonBlankStringOrUndefined(req.body.s3SecretAccessKey),
+        },
+        {
+          skipPubsub: true,
+          removeIfUndefined: true,
+        });
+      }
+      catch (err) {
+        const msg = `Error occurred in updating AWS S3 settings: ${err.message}`;
+        logger.error('Error', err);
+        return res.apiv3Err(new ErrorV3(msg, 'update-fileUploadType-failed'));
+      }
     }
 
-    if (fileUploadType === 'aws') {
-      requestParams['aws:s3Region'] = req.body.s3Region;
-      requestParams['aws:s3CustomEndpoint'] = req.body.s3CustomEndpoint;
-      requestParams['aws:s3Bucket'] = req.body.s3Bucket;
-      requestParams['aws:s3AccessKeyId'] = req.body.s3AccessKeyId;
-      requestParams['aws:referenceFileWithRelayMode'] = req.body.s3ReferenceFileWithRelayMode;
+    if (fileUploadType === 'gcs') {
+      try {
+        await configManager.updateConfigs({
+          'app:fileUploadType': fileUploadType,
+          'gcs:referenceFileWithRelayMode': req.body.gcsReferenceFileWithRelayMode,
+        },
+        { skipPubsub: true });
+        await configManager.updateConfigs({
+          'gcs:apiKeyJsonPath': toNonBlankStringOrUndefined(req.body.gcsApiKeyJsonPath),
+          'gcs:bucket': toNonBlankStringOrUndefined(req.body.gcsBucket),
+          'gcs:uploadNamespace': toNonBlankStringOrUndefined(req.body.gcsUploadNamespace),
+        },
+        { skipPubsub: true, removeIfUndefined: true });
+      }
+      catch (err) {
+        const msg = `Error occurred in updating GCS settings: ${err.message}`;
+        logger.error('Error', err);
+        return res.apiv3Err(new ErrorV3(msg, 'update-fileUploadType-failed'));
+      }
     }
 
     if (fileUploadType === 'azure') {
-      requestParams['azure:tenantId'] = req.body.azureTenantId;
-      requestParams['azure:clientId'] = req.body.azureClientId;
-      requestParams['azure:clientSecret'] = req.body.azureClientSecret;
-      requestParams['azure:storageAccountName'] = req.body.azureStorageAccountName;
-      requestParams['azure:storageContainerName'] = req.body.azureStorageContainerName;
-      requestParams['azure:referenceFileWithRelayMode'] = req.body.azureReferenceFileWithRelayMode;
+      try {
+        await configManager.updateConfigs({
+          'app:fileUploadType': fileUploadType,
+          'azure:referenceFileWithRelayMode': req.body.azureReferenceFileWithRelayMode,
+        },
+        { skipPubsub: true });
+        await configManager.updateConfigs({
+          'azure:tenantId': toNonBlankStringOrUndefined(req.body.azureTenantId),
+          'azure:clientId': toNonBlankStringOrUndefined(req.body.azureClientId),
+          'azure:clientSecret': toNonBlankStringOrUndefined(req.body.azureClientSecret),
+          'azure:storageAccountName': toNonBlankStringOrUndefined(req.body.azureStorageAccountName),
+          'azure:storageContainerName': toNonBlankStringOrUndefined(req.body.azureStorageContainerName),
+        }, { skipPubsub: true, removeIfUndefined: true });
+      }
+      catch (err) {
+        const msg = `Error occurred in updating Azure settings: ${err.message}`;
+        logger.error('Error', err);
+        return res.apiv3Err(new ErrorV3(msg, 'update-fileUploadType-failed'));
+      }
     }
 
     try {
-      await configManager.updateConfigs(requestParams, { skipPubsub: true });
-
-      const s3SecretAccessKey = req.body.s3SecretAccessKey;
-      if (fileUploadType === 'aws' && s3SecretAccessKey != null && s3SecretAccessKey.trim() !== '') {
-        await configManager.updateConfigs({ 'aws:s3SecretAccessKey': s3SecretAccessKey }, { skipPubsub: true });
-      }
-
       await crowi.setUpFileUpload(true);
       crowi.fileUploaderSwitchService.publishUpdatedMessage();
 
@@ -959,7 +1007,7 @@ module.exports = (crowi) => {
       return res.apiv3({ responseParams });
     }
     catch (err) {
-      const msg = 'Error occurred in updating fileUploadType';
+      const msg = 'Error occurred in retrieving file upload configurations';
       logger.error('Error', err);
       return res.apiv3Err(new ErrorV3(msg, 'update-fileUploadType-failed'));
     }

apps/app/src/server/service/config-manager/config-definition.ts

@@ -1,6 +1,9 @@
 import { GrowiDeploymentType, GrowiServiceType } from '@growi/core/dist/consts';
-import type { ConfigDefinition, Lang } from '@growi/core/dist/interfaces';
-import { defineConfig } from '@growi/core/dist/interfaces';
+import type { ConfigDefinition, Lang, NonBlankString } from '@growi/core/dist/interfaces';
+import {
+  toNonBlankString,
+  defineConfig,
+} from '@growi/core/dist/interfaces';
 import type OpenAI from 'openai';
 
 import { ActionGroupSize } from '~/interfaces/activity';
@@ -819,28 +822,29 @@ export const CONFIG_DEFINITIONS = {
     envVarName: 'S3_OBJECT_ACL',
     defaultValue: undefined,
   }),
-  'aws:s3Bucket': defineConfig<string>({
-    defaultValue: 'growi',
+  'aws:s3Bucket': defineConfig<NonBlankString>({
+    defaultValue: toNonBlankString('growi'),
   }),
-  'aws:s3Region': defineConfig<string>({
-    defaultValue: 'ap-northeast-1',
+  'aws:s3Region': defineConfig<NonBlankString>({
+    defaultValue: toNonBlankString('ap-northeast-1'),
   }),
-  'aws:s3AccessKeyId': defineConfig<string | undefined>({
+  'aws:s3AccessKeyId': defineConfig<NonBlankString | undefined>({
     defaultValue: undefined,
   }),
-  'aws:s3SecretAccessKey': defineConfig<string | undefined>({
+  'aws:s3SecretAccessKey': defineConfig<NonBlankString | undefined>({
     defaultValue: undefined,
+    isSecret: true,
   }),
-  'aws:s3CustomEndpoint': defineConfig<string | undefined>({
+  'aws:s3CustomEndpoint': defineConfig<NonBlankString | undefined>({
     defaultValue: undefined,
   }),
 
   // GCS Settings
-  'gcs:apiKeyJsonPath': defineConfig<string | undefined>({
+  'gcs:apiKeyJsonPath': defineConfig<NonBlankString | undefined>({
     envVarName: 'GCS_API_KEY_JSON_PATH',
     defaultValue: undefined,
   }),
-  'gcs:bucket': defineConfig<string | undefined>({
+  'gcs:bucket': defineConfig<NonBlankString | undefined>({
     envVarName: 'GCS_BUCKET',
     defaultValue: undefined,
   }),
@@ -866,15 +870,15 @@ export const CONFIG_DEFINITIONS = {
     envVarName: 'AZURE_REFERENCE_FILE_WITH_RELAY_MODE',
     defaultValue: false,
   }),
-  'azure:tenantId': defineConfig<string | undefined>({
+  'azure:tenantId': defineConfig<NonBlankString | undefined>({
     envVarName: 'AZURE_TENANT_ID',
     defaultValue: undefined,
   }),
-  'azure:clientId': defineConfig<string | undefined>({
+  'azure:clientId': defineConfig<NonBlankString | undefined>({
     envVarName: 'AZURE_CLIENT_ID',
     defaultValue: undefined,
   }),
-  'azure:clientSecret': defineConfig<string | undefined>({
+  'azure:clientSecret': defineConfig<NonBlankString | undefined>({
     envVarName: 'AZURE_CLIENT_SECRET',
     defaultValue: undefined,
     isSecret: true,

apps/app/src/server/service/file-uploader/aws/index.ts

@@ -13,6 +13,8 @@ import {
   AbortMultipartUploadCommand,
 } from '@aws-sdk/client-s3';
 import { getSignedUrl } from '@aws-sdk/s3-request-presigner';
+import type { NonBlankString } from '@growi/core/dist/interfaces';
+import { toNonBlankStringOrUndefined } from '@growi/core/dist/interfaces';
 import urljoin from 'url-join';
 
 import type Crowi from '~/server/crowi';
@@ -79,14 +81,15 @@ const getS3PutObjectCannedAcl = (): ObjectCannedACL | undefined => {
   return undefined;
 };
 
-const getS3Bucket = (): string | undefined => {
-  return configManager.getConfig('aws:s3Bucket') ?? undefined; // return undefined when getConfig() returns null
+const getS3Bucket = (): NonBlankString | undefined => {
+  return toNonBlankStringOrUndefined(configManager.getConfig('aws:s3Bucket')); // Blank strings may remain in the DB, so convert with toNonBlankStringOrUndefined for safety
 };
 
 const S3Factory = (): S3Client => {
   const accessKeyId = configManager.getConfig('aws:s3AccessKeyId');
   const secretAccessKey = configManager.getConfig('aws:s3SecretAccessKey');
-  const s3CustomEndpoint = configManager.getConfig('aws:s3CustomEndpoint') || undefined;
+  const s3Region = toNonBlankStringOrUndefined(configManager.getConfig('aws:s3Region')); // Blank strings may remain in the DB, so convert with toNonBlankStringOrUndefined for safety
+  const s3CustomEndpoint = toNonBlankStringOrUndefined(configManager.getConfig('aws:s3CustomEndpoint'));
 
   return new S3Client({
     credentials: accessKeyId != null && secretAccessKey != null
@@ -95,9 +98,9 @@ const S3Factory = (): S3Client => {
         secretAccessKey,
       }
       : undefined,
-    region: configManager.getConfig('aws:s3Region'),
+    region: s3Region,
     endpoint: s3CustomEndpoint,
-    forcePathStyle: !!s3CustomEndpoint, // s3ForcePathStyle renamed to forcePathStyle in v3
+    forcePathStyle: s3CustomEndpoint != null, // s3ForcePathStyle renamed to forcePathStyle in v3
   });
 };
 

apps/app/src/server/service/file-uploader/azure.ts

@@ -17,6 +17,7 @@ import {
   type BlockBlobUploadResponse,
   type BlockBlobParallelUploadOptions,
 } from '@azure/storage-blob';
+import { toNonBlankStringOrUndefined } from '@growi/core/dist/interfaces';
 
 import type Crowi from '~/server/crowi';
 import { FilePathOnStoragePrefix, ResponseMode, type RespondOptions } from '~/server/interfaces/attachment';
@@ -60,9 +61,9 @@ function getAzureConfig(): AzureConfig {
 }
 
 function getCredential(): TokenCredential {
-  const tenantId = configManager.getConfig('azure:tenantId');
-  const clientId = configManager.getConfig('azure:clientId');
-  const clientSecret = configManager.getConfig('azure:clientSecret');
+  const tenantId = toNonBlankStringOrUndefined(configManager.getConfig('azure:tenantId'));
+  const clientId = toNonBlankStringOrUndefined(configManager.getConfig('azure:clientId'));
+  const clientSecret = toNonBlankStringOrUndefined(configManager.getConfig('azure:clientSecret'));
 
   if (tenantId == null || clientId == null || clientSecret == null) {
     throw new Error(`Azure Blob Storage missing required configuration: tenantId=${tenantId}, clientId=${clientId}, clientSecret=${clientSecret}`);

apps/app/src/server/service/file-uploader/gcs/index.ts

@@ -2,6 +2,7 @@ import type { Readable } from 'stream';
 import { pipeline } from 'stream/promises';
 
 import { Storage } from '@google-cloud/storage';
+import { toNonBlankStringOrUndefined } from '@growi/core/dist/interfaces';
 import axios from 'axios';
 import urljoin from 'url-join';
 
@@ -24,7 +25,7 @@ const logger = loggerFactory('growi:service:fileUploaderGcs');
 
 
 function getGcsBucket(): string {
-  const gcsBucket = configManager.getConfig('gcs:bucket');
+  const gcsBucket = toNonBlankStringOrUndefined(configManager.getConfig('gcs:bucket')); // Blank strings may remain in the DB, so convert with toNonBlankStringOrUndefined for safety
   if (gcsBucket == null) {
     throw new Error('GCS bucket is not configured.');
   }
@@ -34,7 +35,7 @@ function getGcsBucket(): string {
 let storage: Storage;
 function getGcsInstance() {
   if (storage == null) {
-    const keyFilename = configManager.getConfig('gcs:apiKeyJsonPath');
+    const keyFilename = toNonBlankStringOrUndefined(configManager.getConfig('gcs:apiKeyJsonPath')); // Blank strings may remain in the DB, so convert with toNonBlankStringOrUndefined for safety
     // see https://googleapis.dev/nodejs/storage/latest/Storage.html
     storage = keyFilename != null
       ? new Storage({ keyFilename }) // Create a client with explicit credentials