From b61c83152891e89040893e21696b698ec364608e Mon Sep 17 00:00:00 2001 From: Kevin Foong <55353265+kevin9foong@users.noreply.github.com> Date: Tue, 10 Dec 2024 14:26:43 +0800 Subject: [PATCH 1/4] feat: add logs for uploads to s3 (#7975) * feat: add logs for uploads to s3 * feat: add logs including formid for virus scanning * feat: remove attachmentmetadata from upload log * feat: update error message precision * feat: cast id to string * feat: add attachment metadata to logs to link s3 object to form * feat: remove upload attachment log --- .../encrypt-submission.middleware.ts | 18 +++++- .../multirespondent-submission.middleware.ts | 15 ++++- .../modules/submission/submission.service.ts | 64 +++++++++++++------ 3 files changed, 71 insertions(+), 26 deletions(-) diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.middleware.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.middleware.ts index 4f61409015..584b588851 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.middleware.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.middleware.ts @@ -166,6 +166,7 @@ export const validateStorageSubmissionParams = celebrate({ */ const asyncVirusScanning = ( responses: ParsedClearFormFieldResponse[], + formId: string, ): ResultAsync< ParsedClearFormFieldResponse, | VirusScanFailedError @@ -176,6 +177,7 @@ const asyncVirusScanning = ( if (isQuarantinedAttachmentResponse(response)) { return SubmissionService.triggerVirusScanThenDownloadCleanFileChain( response, + formId, ) } @@ -191,6 +193,7 @@ const asyncVirusScanning = ( */ const devModeSyncVirusScanning = async ( responses: ParsedClearFormFieldResponse[], + formId: string, ): Promise< Result< ParsedClearFormFieldResponse, @@ -209,6 +212,7 @@ const devModeSyncVirusScanning = async ( const attachmentResponse = await SubmissionService.triggerVirusScanThenDownloadCleanFileChain( response, + formId, ) results.push(attachmentResponse) if (attachmentResponse.isErr()) break @@ -246,8 +250,18 @@ export const scanAndRetrieveAttachments = async ( // run the virus scanning asynchronously for better performance (lower latency). // Note on .combine: if any scans or downloads error out, it will short circuit and return the first error. isDev - ? Result.combine(await devModeSyncVirusScanning(req.body.responses)) - : await ResultAsync.combine(asyncVirusScanning(req.body.responses)) + ? Result.combine( + await devModeSyncVirusScanning( + req.body.responses, + req.formsg.formDef._id.toString(), + ), + ) + : await ResultAsync.combine( + asyncVirusScanning( + req.body.responses, + req.formsg.formDef._id.toString(), + ), + ) if (scanAndRetrieveFilesResult.isErr()) { logger.error({ diff --git a/src/app/modules/submission/multirespondent-submission/multirespondent-submission.middleware.ts b/src/app/modules/submission/multirespondent-submission/multirespondent-submission.middleware.ts index bc1f243a13..590b3f44d0 100644 --- a/src/app/modules/submission/multirespondent-submission/multirespondent-submission.middleware.ts +++ b/src/app/modules/submission/multirespondent-submission/multirespondent-submission.middleware.ts @@ -184,6 +184,7 @@ type IdTaggedParsedClearAttachmentResponseV3 = */ const asyncVirusScanning = ( responses: IdTaggedParsedClearAttachmentResponseV3[], + formId: string, ): ResultAsync< IdTaggedParsedClearAttachmentResponseV3, | VirusScanFailedError @@ -191,7 +192,7 @@ const asyncVirusScanning = ( | MaliciousFileDetectedError >[] => responses.map((response) => - triggerVirusScanThenDownloadCleanFileChain(response.answer).map( + triggerVirusScanThenDownloadCleanFileChain(response.answer, formId).map( (attachmentResponse) => ({ ...response, answer: attachmentResponse }), ), ) @@ -203,6 +204,7 @@ const asyncVirusScanning = ( */ const devModeSyncVirusScanning = async ( responses: IdTaggedParsedClearAttachmentResponseV3[], + formId: string, ): Promise< Result< IdTaggedParsedClearAttachmentResponseV3, @@ -216,6 +218,7 @@ const devModeSyncVirusScanning = async ( // await to pause for...of loop until the virus scanning and downloading of clean file is completed. const attachmentResponse = await triggerVirusScanThenDownloadCleanFileChain( response.answer, + formId, ) if (attachmentResponse.isErr()) { results.push(err(attachmentResponse.error)) @@ -267,10 +270,16 @@ export const scanAndRetrieveAttachments = async ( // Note on .combine: if any scans or downloads error out, it will short circuit and return the first error. isDev ? Result.combine( - await devModeSyncVirusScanning(attachmentResponsesToRetrieve), + await devModeSyncVirusScanning( + attachmentResponsesToRetrieve, + req.formsg.formDef._id.toString(), + ), ) : await ResultAsync.combine( - asyncVirusScanning(attachmentResponsesToRetrieve), + asyncVirusScanning( + attachmentResponsesToRetrieve, + req.formsg.formDef._id.toString(), + ), ) if (scanAndRetrieveFilesResult.isErr()) { diff --git a/src/app/modules/submission/submission.service.ts b/src/app/modules/submission/submission.service.ts index f573d1bb53..3f4a7e722f 100644 --- a/src/app/modules/submission/submission.service.ts +++ b/src/app/modules/submission/submission.service.ts @@ -432,33 +432,55 @@ export const triggerVirusScanThenDownloadCleanFileChain = < | ParsedClearAttachmentFieldResponseV3, >( response: T, + formId: string, ): ResultAsync< T, | VirusScanFailedError | DownloadCleanFileFailedError | MaliciousFileDetectedError -> => +> => { + const logMeta = { + action: 'triggerVirusScanThenDownloadCleanFileChain', + formId, + quarantineFileKey: response.answer, + } // Step 3: Trigger lambda to scan attachments. - triggerVirusScanning(response.answer) - .mapErr((error) => { - if (error instanceof MaliciousFileDetectedError) - return new MaliciousFileDetectedError(response.filename) - return error - }) - .map((lambdaOutput) => lambdaOutput.body) - // Step 4: Retrieve attachments from the clean bucket. - .andThen((cleanAttachment) => - // Retrieve attachment from clean bucket. - downloadCleanFile( - cleanAttachment.cleanFileKey, - cleanAttachment.destinationVersionId, - ).map((attachmentBuffer) => ({ - ...response, - // Replace content with attachmentBuffer and answer with filename. - content: attachmentBuffer, - answer: response.filename, - })), - ) + return ( + triggerVirusScanning(response.answer) + .mapErr((error) => { + if (error instanceof MaliciousFileDetectedError) { + logger.error({ + message: 'Malicious file detected during lambda virus scan', + meta: logMeta, + error, + }) + return new MaliciousFileDetectedError(response.filename) + } + return error + }) + .map((lambdaOutput) => { + logger.info({ + message: + 'Successfully retrieved clean file from virus scanning lambda', + meta: { ...logMeta, cleanFileKey: lambdaOutput.body.cleanFileKey }, + }) + return lambdaOutput.body + }) + // Step 4: Retrieve attachments from the clean bucket. + .andThen((cleanAttachment) => + // Retrieve attachment from clean bucket. + downloadCleanFile( + cleanAttachment.cleanFileKey, + cleanAttachment.destinationVersionId, + ).map((attachmentBuffer) => ({ + ...response, + // Replace content with attachmentBuffer and answer with filename. + content: attachmentBuffer, + answer: response.filename, + })), + ) + ) +} type AttachmentReducerData = { attachmentMetadata: AttachmentMetadata // type alias for Map From 518796bc14bdc3df45a5a3f32b3c6dfe051757d2 Mon Sep 17 00:00:00 2001 From: Ken Date: Tue, 10 Dec 2024 22:47:21 +0800 Subject: [PATCH 2/4] enable myinfo child on storage admin create page --- .../FieldListDrawer/field-panels/MyInfoPanel.tsx | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/MyInfoPanel.tsx b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/MyInfoPanel.tsx index ed86c2db7c..1080a918a7 100644 --- a/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/MyInfoPanel.tsx +++ b/frontend/src/features/admin-form/create/builder-and-design/BuilderAndDesignDrawer/FieldListDrawer/field-panels/MyInfoPanel.tsx @@ -244,8 +244,7 @@ export const MyInfoFieldPanel = ({ searchValue }: { searchValue: string }) => { )} - {user?.betaFlags?.children && - form?.responseMode === FormResponseMode.Email ? ( + {user?.betaFlags?.children ? ( {(provided) => ( From 4523b11025ffd8fc0f4f9d93e11f94efb16857b5 Mon Sep 17 00:00:00 2001 From: Ken Date: Wed, 11 Dec 2024 22:29:32 +0800 Subject: [PATCH 3/4] fix: remove childname in _id --- src/app/modules/myinfo/myinfo.util.ts | 2 +- src/app/modules/submission/submission.utils.ts | 11 +++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/app/modules/myinfo/myinfo.util.ts b/src/app/modules/myinfo/myinfo.util.ts index e8c15fddd8..ce847be7c2 100644 --- a/src/app/modules/myinfo/myinfo.util.ts +++ b/src/app/modules/myinfo/myinfo.util.ts @@ -521,7 +521,7 @@ export const handleMyInfoChildHashResponse = ( // Validate each answer (child) childAnswer.forEach((attrAnswer, subFieldIndex) => { const key = getMyInfoChildHashKey( - field._id as string, + field._id, subFields[subFieldIndex], childIndex, childName, diff --git a/src/app/modules/submission/submission.utils.ts b/src/app/modules/submission/submission.utils.ts index 495fc5f91d..e54346799f 100644 --- a/src/app/modules/submission/submission.utils.ts +++ b/src/app/modules/submission/submission.utils.ts @@ -752,17 +752,12 @@ export const getAnswersForChild = ( return [] } return response.answerArray.flatMap((arr, childIdx) => { - // First array element is always child name - const childName = arr[0] return arr.map((answer, idx) => { const subfield = subFields[idx] return { - _id: getMyInfoChildHashKey( - response._id, - subFields[idx], - childIdx, - childName, - ), + // Recreates the individual _id of the child field based on the parent field's _id and the subfield + // e.g., childrenbirthrecords.67585515e1ced6d790a91e14.childname.0 + _id: `${MyInfoAttribute.ChildrenBirthRecords}.${response._id}.${subFields[idx]}.${childIdx}`, fieldType: response.fieldType, // qnChildIdx represents the index of the MyInfo field // childIdx represents the index of the child in this MyInfo field From 1156f7a7001bbe7e4776d0c20b9d3b6f64c8291e Mon Sep 17 00:00:00 2001 From: Ken Date: Wed, 11 Dec 2024 22:47:48 +0800 Subject: [PATCH 4/4] chore: remove unused imports --- src/app/modules/submission/submission.utils.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/modules/submission/submission.utils.ts b/src/app/modules/submission/submission.utils.ts index e54346799f..d02d2308c5 100644 --- a/src/app/modules/submission/submission.utils.ts +++ b/src/app/modules/submission/submission.utils.ts @@ -91,7 +91,6 @@ import { MyInfoMissingLoginCookieError, } from '../myinfo/myinfo.errors' import { MyInfoKey } from '../myinfo/myinfo.types' -import { getMyInfoChildHashKey } from '../myinfo/myinfo.util' import { InvalidPaymentProductsError, PaymentNotFoundError,