Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(virus-scanner): frontend orchestration #6767

Merged
merged 37 commits into from
Oct 5, 2023
Merged

Conversation

LinHuiqing
Copy link
Contributor

@LinHuiqing LinHuiqing commented Oct 3, 2023

Problem

This is the 4th step of a 4-step effort (omg finally) to integrate the virus scanner into storage submission workflow after the encryption boundary shift (FRM-1292). Specifically, this PR implements the frontend orchestration required for the virus scanning flow, as specified in the design document by @tshuli.

Closes FRM-1303, closes FRM-1292

Another problem that this seeks to solve will be the small percentage of users still being directed to our old /submission/encrypt endpoint. The hypothesis is that this is due to the delay in instantiating growthbook before the endpoint is decided.

Closes FRM-1431

Solution

The implementation is explained under Features below.

Note: if anything on the frontend integration fails, we will fallback to storage submissions using fetch.

Breaking Changes

  • No - this PR is backwards compatible

Features:

  • PublicFormProvider.tsx > handleSubmitForm only triggers the storage submission flow with virus scanning if both encryption boundary shift and virus scanner have been enabled on the FE.
  • mutations.ts > usePublicFormMutations > submitStorageModeClearFormWithVirusScanningMutation orchestrates the whole flow with the following steps:
    1. Get presigned post data for all attachment fields.
    2. Upload attachments to quarantine bucket asynchronously.
    3. Submit form with keys to quarantine bucket attachments.
  • createSubmission.ts > createClearSubmissionWithVirusScanningFormData is implemented to:
    1. Populate answer in attachment responses with the quarantine bucket keys.
    2. Strip the file content of attachment fields.

Improvements:

  • In PublicFormProvider.tsx > handleSubmitForm: Move the use of enableVirusScanner straight to when the point where which mutation is used is decided. This is to address the possibility that the old storage submission endpoint is used because at the point at which the mutation was previously determined, growthbook might not have be instantiated yet, leading to the feature value defaulting to null.
  • In local development environments, our lambda cannot handle multiple invocations at once. As such, syncVirusScanning was implemented (in encrypt-submission.middleware.ts) to perform invocations synchronously in dev mode.
  • Maintain storage submission versions in shared/constants/form.ts.
  • Use useFeatureValue to set the default feature value of the encryption boundary shift flag to true.

Bug fixes:

  • CSP header for quarantine bucket to use url (virusScannerQuarantineS3BucketUrl) instead of bucket name.

Before & After Screenshots

BEFORE:

Refer to https://linear.app/ogp/issue/FRM-1303/fe-orchestration-for-virus-scanning#comment-f8959aa7

AFTER:

Note: eicar.com.txt.zip is a zipped test malicious file from eicar.

Screen.Recording.2023-10-04.at.10.00.50.AM.mov

Tests

Preperation

Test that virus scanner works

  • Create a storage mode form with > 1 attachment field, with <= 1 of these attachment fields to be required.
  • Go to the respondent page. Open the network panel. Upload the test malicious file and submit the form.
    • The submission should fail with an error prompting the respondent to create and upload the document again.
    • In the network panel, check that the status code of the request is 400.
    • Check that the submission did not go through to the admin portal.
  • Go to the respondent page. Upload 1 non-malicious file.
    • The submission should succeed.
    • Check that the submission went through to the admin portal. Download and check that the file name and content is correct.
  • Go to the respondent page. Upload 2 non-malicious files.
    • The submission should succeed.
    • Check that the submission went through to the admin portal. Download and check that the file name and content for both files is correct.
  • Go to the respondent page. Open the network panel. Upload the malicious file for 1 field and non-malicious for the other.
    • The submission should fail with an error prompting the respondent to create and upload the document again.
    • In the network panel, check that the status code of the request is 400.
    • Check that the submission did not go through to the admin portal.

Test that BE feature flag works

  • In the DB featureflags collection, set the encryption-boundary-shift-virus-scanner flag as { enabled: false }.
    • This means that the virus scanner should be enabled on the FE but not BE.
  • Go to the respondent page. Upload any file and submit the form.
    • The submission should fail with an error saying that the feature is disabled.
    • Check that the submission did not go through to the admin portal.
  • In the DB featureflags collection, restore the encryption-boundary-shift-virus-scanner flag as { enabled: true }.

Test that the FE feature flag works

Test that FE falls back to submission without virus scan if virus scanning fails

  • Change the function name in the env var (VIRUS_SCANNER_LAMBDA_FUNCTION_NAME) to an invalid function name and redeploy the app.
  • Go to the respondent page. Open the network panel. Upload the test malicious file and submit the form.
    • Based on the network panel, the request sent should have version 2.
    • The submission should succeed.
    • Check that the submission went through to the admin portal. Download and check that the file name and content for both files is correct.
  • Restore the function name to the appropriate function name:
  • (optional, if we want to test the virus scanner more) Redeploy the app.

Test that /submission/storage will be used if growthbook can't be instantiated

  • Change the growthbook client key in the env var (GROWTHBOOK_CLIENT_KEY) to an invalid one.
  • Go to the respondent page of any storage form. Open the network panel.
  • Requests to retrieve growthbook feature definitions should be observed to be failing. They're the ones with sdk-...
  • Submit the form. The submission should still go to the /submissions/storage endpoint rather than /submissions/encrypt.
  • Restore the growthbook client key in the env var (GROWTHBOOK_CLIENT_KEY) to the appropriate one.

Regression tests

  • Submit a storage mode form with logic.
  • Submit a email mode form with logic.

Deploy Notes

  • In the DB featureflags collection, set the encryption-boundary-shift-virus-scanner flag as { enabled: true }.

New GrowthBook flags:

  • Make sure that the encryption-boundary-shift-virus-scanner flag is disabled for prod before deployment. Rollout to 1% after deployment.

  • encryption-boundary-shift-virus-scanner: FE flag for virus scanner feature.

New AWS configs:

  • Make sure that the prod quarantine S3 bucket has the right CORS policy as described below.

  • Quarantine bucket CORS policy:

    • prod.virus.scanner.quarantine
      [
          {
              "AllowedHeaders": [],
              "AllowedMethods": [
                  "POST"
              ],
              "AllowedOrigins": [
                  "https://form.gov.sg"
              ],
              "ExposeHeaders": []
          }
      ]
    • staging.virus.scanner.quarantine
      [
          {
              "AllowedHeaders": [],
              "AllowedMethods": [
                  "POST"
              ],
              "AllowedOrigins": [
                  "https://staging.form.gov.sg",
                  "https://staging-alt.form.gov.sg",
                  "https://staging-alt2.form.gov.sg"
              ],
              "ExposeHeaders": []
          }
      ]
    • uat.virus.scanner.quarantine
      [
          {
              "AllowedHeaders": [],
              "AllowedMethods": [
                  "POST"
              ],
              "AllowedOrigins": [
                  "https://uat.form.gov.sg"
              ],
              "ExposeHeaders": []
          }
      ]

@LinHuiqing LinHuiqing changed the title Feat/virus scan fe feat(virus-scanner): placeholder Oct 3, 2023
@LinHuiqing LinHuiqing changed the title feat(virus-scanner): placeholder feat(virus-scanner): frontend orchestration Oct 4, 2023
@linear
Copy link

linear bot commented Oct 4, 2023

FRM-1303 FE orchestration for virus scanning

  1. Get presigned urls required for the number and size of attachments
    1. To further improve the security of the presigned URLs, we should include file specs when generating the presigned URL, namely file type and the MD5 hash
  2. Upload attachments using presigned url
  3. Call submission BE endpoint when upload is completed

FRM-1292 Set up Frontend / Backend Integration for Virus Scanner

Refer to virus scanning design doc here: https://www.notion.so/opengov/Virus-Scanner-Design-Doc-5464ffd2af88420495209baa6a55e1bf?pvs=4

- chore: rm unused URL for clean bucket
- fix: broken tests due to CSP change
@LinHuiqing LinHuiqing temporarily deployed to staging-alt2 October 4, 2023 02:25 — with GitHub Actions Inactive
@LinHuiqing LinHuiqing marked this pull request as ready for review October 4, 2023 03:57
@LinHuiqing LinHuiqing temporarily deployed to staging-alt2 October 4, 2023 05:56 — with GitHub Actions Inactive
@linear
Copy link

linear bot commented Oct 4, 2023

FRM-1431 Change default behaviour for encryption boundary shift to `/storage`

Description

Problem

Some submissions are still being served to /encrypt endpoint and our suspicion is that GrowthBook is not instantiated.

Solution

If my guess regarding the issue is right, swapping default behaviour over to /storage should be good!

src/app/config/config.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@tshuli tshuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @LinHuiqing ! lgtm other than some minor comments
will approve after we go through the tests 😃

@LinHuiqing LinHuiqing temporarily deployed to staging-alt2 October 4, 2023 09:07 — with GitHub Actions Inactive
@LinHuiqing
Copy link
Contributor Author

TODO:

  • add monitor for BE feature flag block errors (in case we messed up)

@LinHuiqing LinHuiqing temporarily deployed to staging-alt2 October 5, 2023 02:27 — with GitHub Actions Inactive
Copy link
Contributor

@tshuli tshuli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thanks @LinHuiqing

@LinHuiqing LinHuiqing enabled auto-merge (squash) October 5, 2023 03:01
@LinHuiqing LinHuiqing merged commit d8f900d into develop Oct 5, 2023
14 of 15 checks passed
@LinHuiqing LinHuiqing deleted the feat/virus-scan-fe branch October 5, 2023 06:21
@LinHuiqing LinHuiqing mentioned this pull request Oct 5, 2023
51 tasks
@LinHuiqing
Copy link
Contributor Author

Note: In this PR, the virus scanner does not block submissions! Instead submissions are retried with the storage submission endpoint with fetch.

The correct behaviour for when malicious files are part of a submission:

  • The submission should fail with an error prompting the respondent to create and upload the document again.
  • In the network panel, check that the status code of the request is 400.
  • Check that the submission did not go through to the admin portal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants