diff --git a/__tests__/e2e/encrypt-submission.spec.ts b/__tests__/e2e/encrypt-submission.spec.ts index e1516a2b59..4d42ce557d 100644 --- a/__tests__/e2e/encrypt-submission.spec.ts +++ b/__tests__/e2e/encrypt-submission.spec.ts @@ -1,7 +1,8 @@ import mongoose from 'mongoose' +import { featureFlags } from 'shared/constants/feature-flags' import { BasicField, FormResponseMode } from 'shared/types' -import { IFormModel } from 'src/types' +import { IFeatureFlagModel, IFormModel } from 'src/types' import { ALL_FIELDS as _ALL_FIELDS, @@ -36,12 +37,20 @@ const runEncryptSubmissionTest = createSubmissionTestRunnerForResponseMode( let db: mongoose.Connection let Form: IFormModel +let FeatureFlags: IFeatureFlagModel test.describe('Storage form submission', () => { test.beforeAll(async () => { // Create models db = await makeMongooseFixtures() Form = makeModel(db, 'form.server.model', 'Form') + // TODO(FRM-1232): Remove this once old storage submission endpoint (/submissions/encrypt) is removed + // Add feature flag model and set encryption boundary shift to true + FeatureFlags = makeModel(db, 'feature_flag.server.model', 'FeatureFlags') + await FeatureFlags.create({ + name: featureFlags.encryptionBoundaryShift, + enabled: true, + }) }) test.afterAll(async () => { // Clean up db diff --git a/frontend/src/features/public-form/PublicFormProvider.tsx b/frontend/src/features/public-form/PublicFormProvider.tsx index 68eeb73e69..82fd8ca277 100644 --- a/frontend/src/features/public-form/PublicFormProvider.tsx +++ b/frontend/src/features/public-form/PublicFormProvider.tsx @@ -11,7 +11,11 @@ import { SubmitHandler } from 'react-hook-form' import { useNavigate } from 'react-router-dom' import { useDisclosure } from '@chakra-ui/react' import { datadogLogs } from '@datadog/browser-logs' -import { useFeatureIsOn, useGrowthBook } from '@growthbook/growthbook-react' +import { + useFeatureIsOn, + useFeatureValue, + useGrowthBook, +} from '@growthbook/growthbook-react' import { differenceInMilliseconds, isPast } from 'date-fns' import get from 'lodash/get' import simplur from 'simplur' @@ -143,8 +147,9 @@ export const PublicFormProvider = ({ } }, [growthbook, formId]) - const routeToNewStorageModeSubmission = useFeatureIsOn( + const enableEncryptionBoundaryShift = useFeatureValue( featureFlags.encryptionBoundaryShift, + true, ) // Scroll to top of page when user has finished their submission. @@ -264,6 +269,10 @@ export const PublicFormProvider = ({ } }, [data?.form.form_fields, toast, vfnToastIdRef]) + const enableVirusScanner = useFeatureIsOn( + featureFlags.encryptionBoundaryShiftVirusScanner, + ) + const { submitEmailModeFormMutation, submitStorageModeFormMutation, @@ -271,12 +280,9 @@ export const PublicFormProvider = ({ submitStorageModeFormFetchMutation, submitStorageModeClearFormMutation, submitStorageModeClearFormFetchMutation, + submitStorageModeClearFormWithVirusScanningMutation, } = usePublicFormMutations(formId, submissionData?.id ?? '') - const submitStorageModeVnMutation = routeToNewStorageModeSubmission - ? submitStorageModeClearFormMutation - : submitStorageModeFormMutation - const { handleLogoutMutation } = usePublicAuthMutations(formId) const navigate = useNavigate() @@ -478,7 +484,9 @@ export const PublicFormProvider = ({ : {}), } - const submitStorageFormWithFetch = function () { + const submitStorageFormWithFetch = function ( + routeToNewStorageModeSubmission: boolean, + ) { datadogLogs.logger.info(`handleSubmitForm: submitting via fetch`, { meta: { ...logMeta, @@ -487,12 +495,11 @@ export const PublicFormProvider = ({ }, }) - const submitStorageModeVnFetchMutation = + return ( routeToNewStorageModeSubmission ? submitStorageModeClearFormFetchMutation : submitStorageModeFormFetchMutation - - return submitStorageModeVnFetchMutation + ) .mutateAsync( { ...formData, @@ -539,7 +546,7 @@ export const PublicFormProvider = ({ // TODO (#5826): Toggle to use fetch for submissions instead of axios. If enabled, this is used for testing and to use fetch instead of axios by default if testing shows fetch is more stable. Remove once network error is resolved if (useFetchForSubmissions) { - return submitStorageFormWithFetch() + return submitStorageFormWithFetch(enableEncryptionBoundaryShift) } datadogLogs.logger.info(`handleSubmitForm: submitting via axios`, { meta: { @@ -549,8 +556,58 @@ export const PublicFormProvider = ({ }, }) + // TODO (FRM-1413): Move to main return statement once virus scanner has been fully rolled out + if (enableEncryptionBoundaryShift && enableVirusScanner) { + return submitStorageModeClearFormWithVirusScanningMutation.mutateAsync( + formData, + { + onSuccess: ({ + submissionId, + timestamp, + // payment forms will have non-empty paymentData field + paymentData, + }) => { + trackSubmitForm(form) + + if (paymentData) { + navigate(getPaymentPageUrl(formId, paymentData.paymentId)) + storePaymentMemory(paymentData.paymentId) + return + } + setSubmissionData({ + id: submissionId, + timestamp, + }) + }, + onError: (error) => { + // TODO(#5826): Remove when we have resolved the Network Error + datadogLogs.logger.warn( + `handleSubmitForm: submit with virus scan`, + { + meta: { + ...logMeta, + responseMode: 'storage', + method: 'axios', + error, + }, + }, + ) + + // defaults to the safest option of storage submission without virus scanning + return submitStorageFormWithFetch( + enableEncryptionBoundaryShift, + ) + }, + }, + ) + } + return ( - submitStorageModeVnMutation + ( + enableEncryptionBoundaryShift + ? submitStorageModeClearFormMutation + : submitStorageModeFormMutation + ) .mutateAsync( { ...formData, @@ -595,7 +652,9 @@ export const PublicFormProvider = ({ if (/Network Error/i.test(error.message)) { axiosDebugFlow() - return submitStorageFormWithFetch() + return submitStorageFormWithFetch( + enableEncryptionBoundaryShift, + ) } showErrorToast(error, form) }) @@ -616,8 +675,7 @@ export const PublicFormProvider = ({ getCaptchaResponse, submitEmailModeFormFetchMutation, submitEmailModeFormMutation, - submitStorageModeVnMutation, - routeToNewStorageModeSubmission, + enableEncryptionBoundaryShift, submitStorageModeClearFormFetchMutation, submitStorageModeFormFetchMutation, navigate, diff --git a/frontend/src/features/public-form/PublicFormService.ts b/frontend/src/features/public-form/PublicFormService.ts index 76e198e076..1ae78819d0 100644 --- a/frontend/src/features/public-form/PublicFormService.ts +++ b/frontend/src/features/public-form/PublicFormService.ts @@ -1,5 +1,17 @@ +import { PresignedPost } from 'aws-sdk/clients/s3' +import axios from 'axios' + +import { + ENCRYPTION_BOUNDARY_SHIFT_SUBMISSION_VERSION, + VIRUS_SCANNER_SUBMISSION_VERSION, +} from '~shared/constants' import { SubmitFormIssueBodyDto, SuccessMessageDto } from '~shared/types' -import { FormFieldDto, PaymentFieldsDto } from '~shared/types/field' +import { + AttachmentPresignedPostDataMapType, + AttachmentSizeMapType, + FormFieldDto, + PaymentFieldsDto, +} from '~shared/types/field' import { ProductItem, PublicFormAuthLogoutDto, @@ -26,16 +38,14 @@ import { FormFieldValues } from '~templates/Field' import { createClearSubmissionFormData, + createClearSubmissionWithVirusScanningFormData, createEncryptedSubmissionData, + getAttachmentsMap, } from './utils/createSubmission' import { filterHiddenInputs } from './utils/filterHiddenInputs' export const PUBLIC_FORMS_ENDPOINT = '/forms' -// Encryption boundary shift RFC: https://docs.google.com/document/d/1VmNXS_xYY2Yg30AwVqzdndBp5yRJGSDsyjBnH51ktyc/edit?usp=sharing -// Encryption boundary shift implementation PR: https://github.com/opengovsg/FormSG/pull/6587 -const ENCRYPTION_BOUNDARY_SHIFT_ENCRYPTION_VERSION = 2 - /** * Gets public view of form, along with any * identify information obtained from Singpass/Corppass/MyInfo. @@ -103,6 +113,16 @@ export type SubmitStorageFormClearArgs = SubmitEmailFormArgs & { payments?: PaymentFieldsDto } +export type FieldIdToQuarantineKeyType = { + fieldId: string + quarantineBucketKey: string +} + +export type SubmitStorageFormWithVirusScanningArgs = + SubmitStorageFormClearArgs & { + fieldIdToQuarantineKeyMap: FieldIdToQuarantineKeyType[] + } + export const submitEmailModeForm = async ({ formFields, formLogics, @@ -199,7 +219,7 @@ export const submitStorageModeClearForm = async ({ paymentReceiptEmail, paymentProducts, payments, - version: ENCRYPTION_BOUNDARY_SHIFT_ENCRYPTION_VERSION, + version: ENCRYPTION_BOUNDARY_SHIFT_SUBMISSION_VERSION, }) return ApiService.post( @@ -240,7 +260,7 @@ export const submitStorageModeClearFormWithFetch = async ({ paymentReceiptEmail, paymentProducts, payments, - version: ENCRYPTION_BOUNDARY_SHIFT_ENCRYPTION_VERSION, + version: ENCRYPTION_BOUNDARY_SHIFT_SUBMISSION_VERSION, }) // Add captcha response to query string @@ -263,6 +283,51 @@ export const submitStorageModeClearFormWithFetch = async ({ return processFetchResponse(response) } +// Submit storage mode form with virus scanning (storage v2.1+) +export const submitStorageModeClearFormWithVirusScanning = async ({ + formFields, + formLogics, + formInputs, + formId, + captchaResponse = null, + captchaType = '', + paymentReceiptEmail, + responseMetadata, + paymentProducts, + payments, + fieldIdToQuarantineKeyMap, +}: SubmitStorageFormWithVirusScanningArgs) => { + const filteredInputs = filterHiddenInputs({ + formFields, + formInputs, + formLogics, + }) + + const formData = createClearSubmissionWithVirusScanningFormData( + { + formFields, + formInputs: filteredInputs, + responseMetadata, + paymentReceiptEmail, + paymentProducts, + payments, + version: VIRUS_SCANNER_SUBMISSION_VERSION, + }, + fieldIdToQuarantineKeyMap, + ) + + return ApiService.post( + `${PUBLIC_FORMS_ENDPOINT}/${formId}/submissions/storage`, + formData, + { + params: { + captchaResponse: String(captchaResponse), + captchaType: captchaType, + }, + }, + ).then(({ data }) => data) +} + // TODO (#5826): Fallback mutation using Fetch. Remove once network error is resolved export const submitEmailModeFormWithFetch = async ({ formFields, @@ -387,3 +452,48 @@ export const submitFormIssue = async ( issueToPost, ).then(({ data }) => data) } + +export const getAttachmentSizes = async ({ + formFields, + formInputs, +}: { + formFields: FormFieldDto[] + formInputs: FormFieldValues +}) => { + const attachmentsMap = getAttachmentsMap(formFields, formInputs) + const attachmentSizes: AttachmentSizeMapType[] = [] + for (const id in attachmentsMap) { + // Check if id is a valid ObjectId. mongoose.isValidaObjectId cannot be used as it will throw a Reference Error. + const isValidObjectId = new RegExp(/^[0-9a-fA-F]{24}$/).test(id) + if (!isValidObjectId) throw new Error(`Invalid attachment id: ${id}`) + attachmentSizes.push({ id, size: attachmentsMap[id].size }) + } + return attachmentSizes +} + +/** + * Get presigned post data for attachments. + * @returns presigned post data for attachments. + */ +export const getAttachmentPresignedPostData = async ({ + attachmentSizes, + formId, +}: { + attachmentSizes: AttachmentSizeMapType[] + formId: string +}) => { + return ApiService.post( + `${PUBLIC_FORMS_ENDPOINT}/${formId}/submissions/storage/get-s3-presigned-post-data`, + attachmentSizes, + ).then(({ data }) => data) +} + +export const uploadAttachmentToQuarantine = async ( + presignedPost: PresignedPost, + file: File, +) => { + return await axios.postForm(presignedPost.url, { + ...presignedPost.fields, + file, + }) +} diff --git a/frontend/src/features/public-form/mutations.ts b/frontend/src/features/public-form/mutations.ts index f8d91a7d45..1975644ce7 100644 --- a/frontend/src/features/public-form/mutations.ts +++ b/frontend/src/features/public-form/mutations.ts @@ -10,6 +10,9 @@ import { useToast } from '~hooks/useToast' import { useStorePrefillQuery } from './hooks/useStorePrefillQuery' import { + FieldIdToQuarantineKeyType, + getAttachmentPresignedPostData, + getAttachmentSizes, getPublicFormAuthRedirectUrl, logoutPublicForm, SubmitEmailFormArgs, @@ -18,10 +21,13 @@ import { submitFormFeedback, submitFormIssue, SubmitStorageFormArgs, + SubmitStorageFormClearArgs, submitStorageModeClearForm, submitStorageModeClearFormWithFetch, + submitStorageModeClearFormWithVirusScanning, submitStorageModeForm, submitStorageModeFormWithFetch, + uploadAttachmentToQuarantine, } from './PublicFormService' export const usePublicAuthMutations = (formId: string) => { @@ -83,7 +89,7 @@ export const usePublicFormMutations = ( ) const submitStorageModeClearFormMutation = useMutation( - (args: Omit) => { + (args: Omit) => { return submitStorageModeClearForm({ ...args, formId }) }, ) @@ -102,7 +108,7 @@ export const usePublicFormMutations = ( ) const submitStorageModeClearFormFetchMutation = useMutation( - (args: Omit) => { + (args: Omit) => { return submitStorageModeClearFormWithFetch({ ...args, formId }) }, ) @@ -117,6 +123,75 @@ export const usePublicFormMutations = ( }, ) + const submitStorageModeClearFormWithVirusScanningMutation = useMutation( + async (args: Omit) => { + const attachmentSizes = await getAttachmentSizes(args) + // If there are no attachments, submit form without virus scanning by passing in empty list + if (attachmentSizes.length === 0) { + return submitStorageModeClearFormWithVirusScanning({ + ...args, + fieldIdToQuarantineKeyMap: [], + formId, + }) + } + // Step 1: Get presigned post data for all attachment fields + return ( + getAttachmentPresignedPostData({ ...args, formId, attachmentSizes }) + .then( + // Step 2: Upload attachments to quarantine bucket asynchronously + (fieldToPresignedPostDataMap) => + Promise.all( + fieldToPresignedPostDataMap.map( + async (fieldToPresignedPostData) => { + const attachmentFile = + args.formInputs[fieldToPresignedPostData.id] + + // Check if response is a File object (from an attachment field) + if (!(attachmentFile instanceof File)) + throw new Error('Field is not attachment') + + const uploadResponse = await uploadAttachmentToQuarantine( + fieldToPresignedPostData.presignedPostData, + attachmentFile, + ) + + // If status code is not 200-299, throw error + if ( + uploadResponse.status < 200 || + uploadResponse.status > 299 + ) + throw new Error( + `Attachment upload failed - ${uploadResponse.statusText}`, + ) + + const quarantineBucketKey = + fieldToPresignedPostData.presignedPostData.fields.key + + if (!quarantineBucketKey) + throw new Error( + 'key is not defined in presigned post data', + ) + + return { + fieldId: fieldToPresignedPostData.id, + quarantineBucketKey, + } as FieldIdToQuarantineKeyType + }, + ), + ), + ) + // Step 3: Submit form with keys to quarantine bucket attachments + .then((fieldIdToQuarantineKeyMap) => { + return submitStorageModeClearFormWithVirusScanning({ + ...args, + fieldIdToQuarantineKeyMap, + formId, + }) + }) + ) + }, + ) + return { submitEmailModeFormMutation, submitStorageModeFormMutation, @@ -125,6 +200,7 @@ export const usePublicFormMutations = ( submitEmailModeFormFetchMutation, submitStorageModeClearFormMutation, submitStorageModeClearFormFetchMutation, + submitStorageModeClearFormWithVirusScanningMutation, } } diff --git a/frontend/src/features/public-form/utils/createSubmission.ts b/frontend/src/features/public-form/utils/createSubmission.ts index c24b723e58..502ec2ce2e 100644 --- a/frontend/src/features/public-form/utils/createSubmission.ts +++ b/frontend/src/features/public-form/utils/createSubmission.ts @@ -2,6 +2,7 @@ import { datadogLogs } from '@datadog/browser-logs' import { encode as encodeBase64 } from '@stablelib/base64' import { chain, forOwn, isEmpty, keyBy, omit, pick } from 'lodash' +import { E2EE_SUBMISSION_VERSION } from '~shared/constants' import { ProductItem } from '~shared/types' import { BasicField, FormFieldDto, PaymentFieldsDto } from '~shared/types/field' import { @@ -21,14 +22,11 @@ import fileArrayBuffer from '~/utils/fileArrayBuffer' import formsgSdk from '~utils/formSdk' import { AttachmentFieldSchema, FormFieldValues } from '~templates/Field' +import { FieldIdToQuarantineKeyType } from '../PublicFormService' + import { transformInputsToOutputs } from './inputTransformation' import { validateResponses } from './validateResponses' -// The current encrypt version to assign to the encrypted submission. -// This is needed if we ever break backwards compatibility with -// end-to-end encryption -const ENCRYPT_VERSION = 1 - /** * @returns StorageModeSubmissionContentDto * @throw Error if form inputs are invalid. @@ -72,7 +70,7 @@ export const createEncryptedSubmissionData = async ({ paymentReceiptEmail, paymentProducts, payments, - version: ENCRYPT_VERSION, + version: E2EE_SUBMISSION_VERSION, responseMetadata, } } @@ -125,6 +123,63 @@ export const createClearSubmissionFormData = ( return formData } +/** + * Used for Storage mode submissions v2.1+ (after virus scanning). + * @returns formData containing form responses and attachments. + * @throws Error if form inputs are invalid or contain malicious attachment(s). + */ +export const createClearSubmissionWithVirusScanningFormData = ( + formDataArgs: + | CreateEmailSubmissionFormDataArgs + | CreateStorageSubmissionFormDataArgs, + fieldIdToQuarantineKeyMap: FieldIdToQuarantineKeyType[], +) => { + const { formFields, formInputs, ...formDataArgsRest } = formDataArgs + const responses = createResponsesArray(formFields, formInputs).map( + (response) => { + if (response.fieldType === BasicField.Attachment && response.answer) { + // for each attachment response, find the corresponding quarantine bucket key + const fieldIdToQuarantineKeyEntry = fieldIdToQuarantineKeyMap.find( + (v) => v.fieldId === response._id, + ) + if (!fieldIdToQuarantineKeyEntry) + throw new Error( + `Attachment response with fieldId ${response._id} not found among attachments uploaded to quarantine bucket`, + ) + // set response.answer as the quarantine bucket key + response.answer = fieldIdToQuarantineKeyEntry.quarantineBucketKey + } + return response + }, + ) + const attachments = getAttachmentsMap(formFields, formInputs) + + // Convert content to FormData object. + const formData = new FormData() + formData.append( + 'body', + JSON.stringify({ + responses, + ...formDataArgsRest, + }), + ) + + if (!isEmpty(attachments)) { + forOwn(attachments, (attachment, fieldId) => { + if (attachment) { + formData.append( + attachment.name, + // Set content as empty array buffer. + new File([], attachment.name, { type: attachment.type }), + fieldId, + ) + } + }) + } + + return formData +} + const createResponsesArray = ( formFields: FormFieldDto[], formInputs: FormFieldValues, @@ -156,7 +211,7 @@ const getEncryptedAttachmentsMap = async ( ) } -const getAttachmentsMap = ( +export const getAttachmentsMap = ( formFields: FormFieldDto[], formInputs: FormFieldValues, ): Record => { diff --git a/shared/constants/form.ts b/shared/constants/form.ts index ca1e5883c1..a6a26085e1 100644 --- a/shared/constants/form.ts +++ b/shared/constants/form.ts @@ -61,3 +61,12 @@ export const PAYMENT_CONTACT_FIELD_ID = 'payment_contact_field' export const PAYMENT_PRODUCT_FIELD_ID = 'payment_products' export const PAYMENT_VARIABLE_INPUT_AMOUNT_FIELD_ID = 'payment_variable_input_amount_field_id' + +// The current encrypt version to assign to the encrypted submission. +// This is needed if we ever break backwards compatibility with +// end-to-end encryption +export const E2EE_SUBMISSION_VERSION = 1 +// Encryption boundary shift RFC: https://docs.google.com/document/d/1VmNXS_xYY2Yg30AwVqzdndBp5yRJGSDsyjBnH51ktyc/edit?usp=sharing +// Encryption boundary shift implementation PR: https://github.com/opengovsg/FormSG/pull/6587 +export const ENCRYPTION_BOUNDARY_SHIFT_SUBMISSION_VERSION = 2 +export const VIRUS_SCANNER_SUBMISSION_VERSION = 2.1 diff --git a/shared/types/field/attachmentField.ts b/shared/types/field/attachmentField.ts index 629f7cb141..383cd20769 100644 --- a/shared/types/field/attachmentField.ts +++ b/shared/types/field/attachmentField.ts @@ -1,4 +1,5 @@ import { BasicField, FieldBase } from './base' +import { PresignedPost } from 'aws-sdk/clients/s3' export enum AttachmentSize { OneMb = '1', @@ -14,3 +15,13 @@ export interface AttachmentFieldBase extends FieldBase { fieldType: BasicField.Attachment attachmentSize: AttachmentSize } + +export type AttachmentSizeMapType = { + id: string + size: number +} + +export type AttachmentPresignedPostDataMapType = { + id: string + presignedPostData: PresignedPost +} diff --git a/src/app/config/config.ts b/src/app/config/config.ts index 4c7bc502ac..f9cc3d1573 100644 --- a/src/app/config/config.ts +++ b/src/app/config/config.ts @@ -76,7 +76,6 @@ const s3BucketUrlVars = convict(s3BucketUrlSchema) // NOTE THE TRAILING / AT THE END OF THIS URL! This is only for attachments! attachmentBucketUrl: `${awsEndpoint}/${basicVars.awsConfig.attachmentS3Bucket}/`, virusScannerQuarantineS3BucketUrl: `${awsEndpoint}/${basicVars.awsConfig.virusScannerQuarantineS3Bucket}`, - virusScannerCleanS3BucketUrl: `${awsEndpoint}/${basicVars.awsConfig.virusScannerCleanS3Bucket}`, paymentProofS3BucketUrl: `${awsEndpoint}/${basicVars.awsConfig.paymentProofS3Bucket}`, }) .validate({ allowed: 'strict' }) diff --git a/src/app/config/schema.ts b/src/app/config/schema.ts index 590bce121b..5f8cde55c5 100644 --- a/src/app/config/schema.ts +++ b/src/app/config/schema.ts @@ -491,12 +491,6 @@ export const loadS3BucketUrlSchema = ({ validateS3BucketUrl(val, { isDev, hasTrailingSlash: false, region }), default: null, }, - virusScannerCleanS3BucketUrl: { - doc: 'Url of virus scanner clean S3 bucket.', - format: (val) => - validateS3BucketUrl(val, { isDev, hasTrailingSlash: false, region }), - default: null, - }, paymentProofS3BucketUrl: { doc: 'Url of payment proof S3 bucket.', format: (val) => diff --git a/src/app/loaders/express/__tests__/helmet.spec.ts b/src/app/loaders/express/__tests__/helmet.spec.ts index 06d2a448b0..898b8919f7 100644 --- a/src/app/loaders/express/__tests__/helmet.spec.ts +++ b/src/app/loaders/express/__tests__/helmet.spec.ts @@ -52,7 +52,7 @@ describe('helmetMiddlewares', () => { config.aws.attachmentBucketUrl, config.aws.imageBucketUrl, config.aws.logoBucketUrl, - config.aws.virusScannerQuarantineS3Bucket, + config.aws.virusScannerQuarantineS3BucketUrl, 'https://*.google-analytics.com', 'https://*.analytics.google.com', 'https://*.googletagmanager.com', diff --git a/src/app/loaders/express/helmet.ts b/src/app/loaders/express/helmet.ts index f76daca6e6..361bd36345 100644 --- a/src/app/loaders/express/helmet.ts +++ b/src/app/loaders/express/helmet.ts @@ -65,7 +65,7 @@ const helmetMiddlewares = () => { config.aws.attachmentBucketUrl, // Attachment downloads config.aws.imageBucketUrl, // Image field config.aws.logoBucketUrl, // Form logo - config.aws.virusScannerQuarantineS3Bucket, // Virus scanning + config.aws.virusScannerQuarantineS3BucketUrl, // Virus scanning 'https://*.google-analytics.com', // GA4 https://developers.google.com/tag-platform/tag-manager/web/csp 'https://*.analytics.google.com', 'https://*.googletagmanager.com', diff --git a/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.controller.spec.ts b/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.controller.spec.ts index e1d21cca4e..550d330025 100644 --- a/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.controller.spec.ts +++ b/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.controller.spec.ts @@ -24,6 +24,8 @@ import { } from 'src/types' import { + AttachmentPresignedPostDataMapType, + AttachmentSizeMapType, FormResponseMode, StorageModeSubmissionMetadata, SubmissionId, @@ -40,10 +42,6 @@ import { streamEncryptedResponses, } from '../encrypt-submission.controller' import * as EncryptSubmissionService from '../encrypt-submission.service' -import { - AttachmentPresignedPostDataMapType, - AttachmentSizeMapType, -} from '../encrypt-submission.types' jest.mock( 'src/app/modules/submission/encrypt-submission/encrypt-submission.service', diff --git a/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.service.spec.ts b/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.service.spec.ts index b420b52de3..4d0a6a9f3f 100644 --- a/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.service.spec.ts +++ b/src/app/modules/submission/encrypt-submission/__tests__/encrypt-submission.service.spec.ts @@ -34,6 +34,7 @@ import { DownloadCleanFileFailedError, InvalidFieldIdError, InvalidFileKeyError, + ParseVirusScannerLambdaPayloadError, VirusScanFailedError, } from '../encrypt-submission.errors' import { @@ -1035,8 +1036,8 @@ describe('encrypt-submission.service', () => { }) describe('getQuarantinePresignedPostData', () => { - const fieldId1 = new mongoose.Types.ObjectId() - const fieldId2 = new mongoose.Types.ObjectId() + const fieldId1 = new mongoose.Types.ObjectId().toHexString() + const fieldId2 = new mongoose.Types.ObjectId().toHexString() const MOCK_ATTACHMENT_SIZES = [ { id: fieldId1, size: 1 }, { id: fieldId2, size: 2 }, @@ -1131,7 +1132,7 @@ describe('encrypt-submission.service', () => { // Act const actualResult = await getQuarantinePresignedPostData([ - { id: 'test_file_1' as unknown as ObjectId, size: 1 }, + { id: 'test_file_1', size: 1 }, ]) // Assert @@ -1213,7 +1214,7 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) @@ -1232,7 +1233,7 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) @@ -1280,7 +1281,7 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) @@ -1305,7 +1306,7 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) @@ -1330,7 +1331,7 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) @@ -1355,7 +1356,7 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) @@ -1383,7 +1384,7 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) @@ -1411,14 +1412,14 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) it('should return errAsync if lambda returns an errored response (e.g. file not found)', async () => { // Arrange const failurePayload = { - statusCode: 200, + statusCode: 404, body: JSON.stringify({ message: 'File not found', }), @@ -1465,7 +1466,7 @@ describe('encrypt-submission.service', () => { expect(awsSpy).toHaveBeenCalledOnce() expect(actualResult.isErr()).toEqual(true) expect(actualResult._unsafeUnwrapErr()).toEqual( - new VirusScanFailedError(), + new ParseVirusScannerLambdaPayloadError(), ) }) }) diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts index fb892b4d5d..1e5c77bdc2 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.controller.ts @@ -9,6 +9,8 @@ import Stripe from 'stripe' import { featureFlags } from '../../../../../shared/constants' import { + AttachmentPresignedPostDataMapType, + AttachmentSizeMapType, ErrorDto, FormAuthType, FormResponseMode, @@ -75,8 +77,6 @@ import { uploadAttachments, } from './encrypt-submission.service' import { - AttachmentPresignedPostDataMapType, - AttachmentSizeMapType, SubmitEncryptModeFormHandlerRequest, SubmitEncryptModeFormHandlerType, } from './encrypt-submission.types' diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.errors.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.errors.ts index a3e54e6fd2..e5f58116ce 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.errors.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.errors.ts @@ -75,3 +75,19 @@ export class DownloadCleanFileFailedError extends ApplicationError { super(message) } } + +export class ParseVirusScannerLambdaPayloadError extends ApplicationError { + constructor(message = 'Unexpected payload from virus scanning lambda.') { + super(message) + } +} + +export class MaliciousFileDetectedError extends ApplicationError { + constructor(filename?: string) { + super( + `Your ${ + filename ? `file "${filename}"` : 'attachments(s)' + } has failed our virus scan. Try to create and upload it again.`, + ) + } +} 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 dc0302b6b7..48a3e97ea5 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.middleware.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.middleware.ts @@ -3,7 +3,7 @@ import { celebrate, Joi, Segments } from 'celebrate' import { NextFunction } from 'express' import { StatusCodes } from 'http-status-codes' import { chain, omit } from 'lodash' -import { okAsync, Result, ResultAsync } from 'neverthrow' +import { ok, okAsync, Result, ResultAsync } from 'neverthrow' import { featureFlags } from '../../../../../shared/constants' import { @@ -15,8 +15,10 @@ import { EncryptAttachmentResponse, EncryptFormFieldResponse, FormLoadedDto, + ParsedClearAttachmentResponse, ParsedClearFormFieldResponse, } from '../../../../types/api' +import { isDev } from '../../../config/config' import { paymentConfig } from '../../../config/features/payment.config' import formsgSdk from '../../../config/formsg-sdk' import { createLoggerWithLabel } from '../../../config/logger' @@ -36,6 +38,7 @@ import { DownloadCleanFileFailedError, EncryptedPayloadExistsError, FormsgReqBodyExistsError, + MaliciousFileDetectedError, VirusScanFailedError, } from './encrypt-submission.errors' import { @@ -188,7 +191,98 @@ export const checkNewBoundaryEnabled = async ( } /** - * !! Do NOT enable `encryption-boundary-shift-virus-scanner` feature flag until this has been completely implemented. !! + * Helper function to trigger virus scanning and download clean file. + * @param response quarantined attachment response from storage submissions v2.1+. + * @returns modified response with content replaced with clean file buffer and answer replaced with filename. + */ +const triggerVirusScanThenDownloadCleanFileChain = ( + response: ParsedClearAttachmentResponse, +): ResultAsync< + ParsedClearAttachmentResponse, + | VirusScanFailedError + | DownloadCleanFileFailedError + | MaliciousFileDetectedError +> => + // 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, + })), + ) + +/** + * Asynchronous virus scanning for storage submissions v2.1+. This is used for non-dev environments. + * @param responses all responses in the storage submissions v2.1+ request. + * @returns all responses with clean attachments and their filename populated for any attachment fields. + */ +const asyncVirusScanning = ( + responses: ParsedClearFormFieldResponse[], +): ResultAsync< + ParsedClearFormFieldResponse, + | VirusScanFailedError + | DownloadCleanFileFailedError + | MaliciousFileDetectedError +>[] => { + return responses.map((response) => { + if (isQuarantinedAttachmentResponse(response)) { + return triggerVirusScanThenDownloadCleanFileChain(response) + } + + // If field is not an attachment, return original response. + return okAsync(response) + }) +} + +/** + * Synchronous virus scanning for storage submissions v2.1+. This is used for dev environment. + * @param responses all responses in the storage submissions v2.1+ request. + * @returns all responses with clean attachments and their filename populated for any attachment fields. + */ +const devModeSyncVirusScanning = async ( + responses: ParsedClearFormFieldResponse[], +): Promise< + Result< + ParsedClearFormFieldResponse, + | VirusScanFailedError + | DownloadCleanFileFailedError + | MaliciousFileDetectedError + >[] +> => { + const results: Result< + ParsedClearFormFieldResponse, + VirusScanFailedError | DownloadCleanFileFailedError + >[] = [] + for (const response of responses) { + if (isQuarantinedAttachmentResponse(response)) { + // await to pause for...of loop until the virus scanning and downloading of clean file is completed. + const attachmentResponse = + await triggerVirusScanThenDownloadCleanFileChain(response) + results.push(attachmentResponse) + if (attachmentResponse.isErr()) break + } else { + // If field is not an attachment, return original response. + results.push(ok(response)) + } + } + return results +} + +/** * Scan attachments on quarantine bucket and retrieve attachments from the clean bucket. * Note: Downloading of attachments from the clean bucket is not implemented yet. See Step 4. */ @@ -231,35 +325,18 @@ export const scanAndRetrieveAttachments = async ( // Step 3 + 4: For each attachment, trigger lambda to scan and if it succeeds, retrieve attachment from clean bucket. Do this asynchronously. const scanAndRetrieveFilesResult: Result< ParsedClearFormFieldResponse[], // true for attachment fields, false for non-attachment fields. - VirusScanFailedError | DownloadCleanFileFailedError - > = await ResultAsync.combine( - // If any scans or downloads error out, it will short circuit and return the first error. - req.body.responses.map((response) => { - if (isQuarantinedAttachmentResponse(response)) { - // Step 3: Trigger lambda to scan attachments. - return ( - triggerVirusScanning(response.answer) - .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, - })), - ) - ) - } - - // If field is not an attachment, return original response. - return okAsync(response) - }), - ) + | VirusScanFailedError + | DownloadCleanFileFailedError + | MaliciousFileDetectedError + > = + // On the local development environment, there is only 1 lambda and the virus scanning service WILL CRASH if multiple lambda invocations are + // attempted at the same time. Reference: https://www.notion.so/opengov/Encryption-Boundary-Shift-the-journey-so-far-dfc6e15fc65f45eba3dd6a9af48eebea?pvs=4#d0944ba61aad45ce988ed0474f131e59 + // As such, in dev mode, we want to run the virus scanning synchronously. In non-dev mode, as we'll be using the lambdas on AWS, we should + // 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)) if (scanAndRetrieveFilesResult.isErr()) { logger.error({ diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.service.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.service.ts index 80931ccc66..e486d3852f 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.service.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.service.ts @@ -11,6 +11,8 @@ import { Transform, Writable } from 'stream' import { validate } from 'uuid' import { + AttachmentPresignedPostDataMapType, + AttachmentSizeMapType, FormResponseMode, StorageModeSubmissionMetadata, StorageModeSubmissionMetadataList, @@ -67,12 +69,12 @@ import { InvalidFieldIdError, InvalidFileKeyError, JsonParseFailedError, + MaliciousFileDetectedError, + ParseVirusScannerLambdaPayloadError, VirusScanFailedError, } from './encrypt-submission.errors' import { AttachmentMetadata, - AttachmentPresignedPostDataMapType, - AttachmentSizeMapType, bodyIsExpectedErrStructure, bodyIsExpectedOkStructure, ParseVirusScannerLambdaPayloadErrType, @@ -570,13 +572,14 @@ export const getQuarantinePresignedPostData = ( const totalAttachmentSizeLimit = fileSizeLimitBytes(FormResponseMode.Encrypt) const totalAttachmentSize = attachmentSizes .map(({ size }) => size) - .reduce((prev, next) => prev + next) + .reduce((prev, next) => prev + next, 0) if (totalAttachmentSize > totalAttachmentSizeLimit) return errAsync(new AttachmentSizeLimitExceededError()) // Step 2: Create presigned post data for each attachment return ResultAsync.combine( attachmentSizes.map(({ id, size }) => { + // Check if id is a valid ObjectId if (!mongoose.isValidObjectId(id)) return errAsync(new InvalidFieldIdError()) @@ -592,14 +595,6 @@ export const getQuarantinePresignedPostData = ( ) } -/** - * Catch all error for parsing the payload from the virus scanning lambda. - */ -const parseVirusScannerLambdaPayloadError = { - statusCode: StatusCodes.INTERNAL_SERVER_ERROR, - body: { message: 'Unexpected payload from virus scanning lambda' }, -} - const safeJSONParse = Result.fromThrowable(JSON.parse, (error) => { logger.error({ message: 'Error while calling JSON.parse on MyInfo relay state', @@ -638,7 +633,7 @@ const parseVirusScannerLambdaPayload = ( meta: logMeta, }) - return err(parseVirusScannerLambdaPayloadError) + return err(new ParseVirusScannerLambdaPayloadError()) } const parsedPayload = payloadIsExpectedStructure(parsedPayloadResult.value) @@ -656,7 +651,7 @@ const parseVirusScannerLambdaPayload = ( meta: logMeta, }) - return err(parseVirusScannerLambdaPayloadError) + return err(new ParseVirusScannerLambdaPayloadError()) } // Step 4a: If statusCode is 200, check if the body is of the correct type @@ -686,7 +681,7 @@ const parseVirusScannerLambdaPayload = ( meta: logMeta, }) - return err(parseVirusScannerLambdaPayloadError) + return err(new ParseVirusScannerLambdaPayloadError()) } // Step 4b: If statusCode is not 200, check if the body is of the correct type (errored message) @@ -698,6 +693,7 @@ const parseVirusScannerLambdaPayload = ( meta: logMeta, }) + // Only place where error from virus scanning lambda is returned return err({ statusCode: parsedPayload.statusCode, body: parsedBody, @@ -710,7 +706,7 @@ const parseVirusScannerLambdaPayload = ( meta: logMeta, }) - return err(parseVirusScannerLambdaPayloadError) + return err(new ParseVirusScannerLambdaPayloadError()) } // Step 2b: Return error if statusCode and body (unparsed) are of the wrong types @@ -721,7 +717,7 @@ const parseVirusScannerLambdaPayload = ( meta: logMeta, }) - return err(parseVirusScannerLambdaPayloadError) + return err(new ParseVirusScannerLambdaPayloadError()) } /** @@ -732,7 +728,10 @@ const parseVirusScannerLambdaPayload = ( */ export const triggerVirusScanning = ( quarantineFileKey: string, -): ResultAsync => { +): ResultAsync< + ParseVirusScannerLambdaPayloadOkType, + VirusScanFailedError | MaliciousFileDetectedError +> => { const logMeta = { action: 'triggerVirusScanning', quarantineFileKey, @@ -768,23 +767,23 @@ export const triggerVirusScanning = ( message: 'Error returned from virus scanning lambda or parsing lambda output', meta: logMeta, - error, + error: error, }) - return new VirusScanFailedError() + if (error instanceof ParseVirusScannerLambdaPayloadError) return error + else if (error.statusCode !== StatusCodes.BAD_REQUEST) + return new VirusScanFailedError() + + return new MaliciousFileDetectedError() }) // if data or data.Payload is undefined logger.error({ message: 'data or data.Payload from virus scanner lambda is undefined', meta: logMeta, - error: { - statusCode: parseVirusScannerLambdaPayloadError.statusCode, - message: parseVirusScannerLambdaPayloadError.body.message, - }, }) - return errAsync(new VirusScanFailedError()) + return errAsync(new ParseVirusScannerLambdaPayloadError()) }) } diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.types.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.types.ts index 9641e9a4ff..d5f37d0664 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.types.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.types.ts @@ -1,6 +1,4 @@ -import { PresignedPost } from 'aws-sdk/clients/s3' import { StatusCodes } from 'http-status-codes' -import { ObjectId } from 'mongodb' import { SubmissionErrorDto, @@ -17,6 +15,8 @@ import { } from '../../../../types/api' import { ControllerHandler } from '../../core/core.types' +import { ParseVirusScannerLambdaPayloadError } from './encrypt-submission.errors' + export type AttachmentMetadata = Map export type SaveEncryptSubmissionParams = { @@ -77,16 +77,6 @@ export type SubmitEncryptModeFormHandlerType = ControllerHandler< export type SubmitEncryptModeFormHandlerRequest = Parameters[0] & { formsg: FormCompleteDto } -export type AttachmentSizeMapType = { - id: ObjectId - size: number -} - -export type AttachmentPresignedPostDataMapType = { - id: ObjectId - presignedPostData: PresignedPost -} - export type ParseVirusScannerLambdaPayloadBeforeBodyIsParsed = { statusCode: number body: string @@ -106,10 +96,12 @@ export type ParseVirusScannerLambdaPayloadErrBody = { message: string } -export type ParseVirusScannerLambdaPayloadErrType = { - statusCode: number // custom status codes might be sent by the lambda - body: ParseVirusScannerLambdaPayloadErrBody -} +export type ParseVirusScannerLambdaPayloadErrType = + | { + statusCode: number // custom status codes might be sent by the lambda + body: ParseVirusScannerLambdaPayloadErrBody + } + | ParseVirusScannerLambdaPayloadError // Helper function to check if the payload is of the expected structure export const payloadIsExpectedStructure = ( diff --git a/src/app/modules/submission/encrypt-submission/encrypt-submission.utils.ts b/src/app/modules/submission/encrypt-submission/encrypt-submission.utils.ts index 9ece480f6d..631d04e0d9 100644 --- a/src/app/modules/submission/encrypt-submission/encrypt-submission.utils.ts +++ b/src/app/modules/submission/encrypt-submission/encrypt-submission.utils.ts @@ -78,6 +78,7 @@ import { FeatureDisabledError, InvalidFieldIdError, InvalidFileKeyError, + MaliciousFileDetectedError, SubmissionFailedError, VirusScanFailedError, } from './encrypt-submission.errors' @@ -236,6 +237,7 @@ const errorMapper: MapRouteError = ( case InvalidFieldIdError: case AttachmentSizeLimitExceededError: case InvalidFileKeyError: + case MaliciousFileDetectedError: return { statusCode: StatusCodes.BAD_REQUEST, errorMessage: error.message, diff --git a/src/types/config.ts b/src/types/config.ts index 7a90f16ef9..0295655d79 100644 --- a/src/types/config.ts +++ b/src/types/config.ts @@ -38,6 +38,7 @@ export type AwsConfig = { imageBucketUrl: string attachmentBucketUrl: string staticAssetsBucketUrl: string + virusScannerQuarantineS3BucketUrl: string virusScannerQuarantineS3Bucket: string virusScannerCleanS3Bucket: string s3: aws.S3 @@ -204,7 +205,6 @@ export interface IBucketUrlSchema { imageBucketUrl: string staticAssetsBucketUrl: string virusScannerQuarantineS3BucketUrl: string - virusScannerCleanS3BucketUrl: string paymentProofS3BucketUrl: string endPoint: string } diff --git a/src/types/index.ts b/src/types/index.ts index 7a35623330..c69189181f 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -22,3 +22,4 @@ export * from './email_mode_data' export * from './twilio' export * from './payment' export * from './admin_feedback' +export * from './feature_flag'