File upload improvements #2919

BCerki · 2025-02-26T20:35:57Z

This PR

set blob chunk size (I tried a couple options, didn't seem to make much of a difference)
remove check for same hash -- more efficient to just delete and recreate than read and check
the docs explain the file upload strategy
consolidated and renamed some of the ninja schemas
I'm having trouble with some mypy errors I hope the reviewer can weigh in on
I'm also having trouble with a bug where you can't edit an operation to be new entrant because an opted-in field is missing

andrea-williams · 2025-03-11T22:08:15Z

bciers/libs/components/src/form/widgets/FileWidget.tsx

+        <ul className="m-0 py-0 flex flex-col justify-start">
+          <li>
+            <Link
+              download={fileName}


To fix the problem, we need to ensure that the fileName is properly escaped before being used in the Link component. This can be achieved by using a library like he (HTML entities) to encode the fileName, which will prevent any HTML or script content from being interpreted by the browser.

Install the he library to handle HTML entity encoding.

Import the he library in the file.

Encode the fileName before using it in the Link component.

CodeQL has raised an interesting point here. I'm looking into this more to see if it can actually be exploited.

BCerki · 2025-03-10T15:25:08Z

bc_obps/registration/api/v2/_operations/_operation_id/_registration/new_entrant_application.py

 ) -> Tuple[Literal[200], Operation]:
+    payload = OperationNewEntrantApplicationInWithDocuments(
+        date_of_first_shipment=details.date_of_first_shipment,


Doing it like this instead of **details.dict() was to make mypy happy

BCerki · 2025-03-10T15:26:35Z

bc_obps/registration/schema/v2/operation.py

+    naics_code_id: Optional[int] = None
+    opt_in: Optional[bool] = False


These are optional and None so I'm not sure why I'm getting mypy errors

andrea-williams

I wasn't able to get past the first step of the Registration workflow. Getting this error in the backend:

  File "/Users/awilliam/Documents/cas-registration/bc_obps/service/document_service_v2.py", line 29, in create_or_replace_operation_document
    existing_document = cls.get_operation_document_by_type_if_authorized(user_guid, operation_id, document_type)
                                                                                    ^^^^^^^^^^^^
NameError: name 'operation_id' is not defined

---------------------------------------------ERROR END-------------------------------------------------
Bad Request: /api/registration/operations/df62d793-8cfe-4272-a93e-ea9c9139ff82/registration/operation
[11/Mar/2025 22:07:41] "PUT /api/registration/operations/df62d793-8cfe-4272-a93e-ea9c9139ff82/registration/operation HTTP/1.1" 400 49

bc_obps/registration/schema/operation.py

andrea-williams · 2025-03-11T00:38:57Z

bc_obps/registration/api/v2/_operations/_operation_id/documents/document_id.py

+@router.get(
+    "/operations/{uuid:operation_id}/documents/{uuid:document_type}",
+    response={200: DocumentOut, custom_codes_4xx: Message},
+    tags=OPERATOR_TAGS,


OPERATION_TAGS instead?

I ended up not using this endpoint because we can use a GCS download link instead

bc_obps/registration/api/v2/_operations/_operation_id/documents/document_id.py

andrea-williams · 2025-03-11T19:20:02Z

bc_obps/service/document_service.py

        """
-        existing_document = cls.get_operation_document_by_type_if_authorized(user_guid, operation_id, document_type)
-        # if there is an existing  document, check if the new one is different
+        existing_document = DocumentDataAccessService.get_operation_document_by_type(operation_id, type)


why did get_operation_document_by_type_if_authorized get replaced with this? We're no longer checking whether the user is authorized to access the document

bciers/apps/administration/tests/components/operations/OperationInformationForm.test.tsx

andrea-williams · 2025-03-11T20:52:56Z

docs/file_uploads.md

+- DocumentServiceV2.create_or_replace_operation_document
+- DocumentDataAccessServiceV2.create_document


Suggested change

- DocumentServiceV2.create_or_replace_operation_document

- DocumentDataAccessServiceV2.create_document

- DocumentService.create_or_replace_operation_document

- DocumentDataAccessService.create_document

andrea-williams · 2025-03-11T20:57:12Z

bciers/apps/registration/app/components/operations/registration/OperationInformationForm.tsx

+    // this removes the file keys if no new file has been uploaded
+    if (
+      (key === "boundary_map" ||
+        key === "process_flow_diagram" ||
+        key === "new_entrant_application") &&
+      typeof rjsfFormData[key] === "string"


please help this idiot understand - where does the check for "no new file has been uploaded" happen?

If a new file has been uploaded, the value is a File. If the value is a string, it means there's no new file. There's some info about this in the file_uploads.md (feel free to suggest improvements), and I'll update this comment to make it clearer too

andrea-williams · 2025-03-11T22:08:15Z

bciers/libs/components/src/form/widgets/FileWidget.tsx

+        <ul className="m-0 py-0 flex flex-col justify-start">
+          <li>
+            <Link
+              download={fileName}


CodeQL has raised an interesting point here. I'm looking into this more to see if it can actually be exploited.

BCerki · 2025-03-12T20:09:24Z

document_service_v2.py

I think this is on old version of the branch--the attachment filename is displaying as the id in the screenshot, and I fixed that in a later commit

Co-authored-by: Andrea Williams <[email protected]> Signed-off-by: Brianna Cerkiewicz <[email protected]>

BCerki force-pushed the 2123-remove-files-have-same-hash branch from e775467 to 43cca7d Compare February 27, 2025 19:38

BCerki force-pushed the 2123-remove-files-have-same-hash branch from 38918a4 to 0bdf9cc Compare March 6, 2025 23:14

github-advanced-security bot found potential problems Mar 6, 2025

View reviewed changes

BCerki mentioned this pull request Mar 7, 2025

Multiple operators are lost #2968

Closed

BCerki commented Mar 10, 2025

View reviewed changes

BCerki force-pushed the 2123-remove-files-have-same-hash branch 4 times, most recently from 06b749b to 6a71ad4 Compare March 10, 2025 20:33

andrea-williams reviewed Mar 11, 2025

View reviewed changes

BCerki and others added 13 commits March 13, 2025 08:13

chore: backend changes to file uploads

db05f16

chore: set blob chunk size

c7af017

test: backend file upload tests

761d7f8

chore: FE file upload refactor

d0de801

test: vitests for file upload refactor

bf9c800

docs: explain file upload strategy

e542ae0

chore: post-rebase updates

259c869

chore: fix comment

0a927c7

chore: apply suggestions from code review

3d8f389

Co-authored-by: Andrea Williams <[email protected]> Signed-off-by: Brianna Cerkiewicz <[email protected]>

chore: apply suggestions from code review

d992c69

Co-authored-by: Andrea Williams <[email protected]> Signed-off-by: Brianna Cerkiewicz <[email protected]>

chore: apply suggestions from code review

8fdc424

chore: fix new entrant form

c7bf777

chore: debugging new entrant

ad33f5c

BCerki force-pushed the 2123-remove-files-have-same-hash branch from 9c912b2 to 8baa162 Compare March 13, 2025 17:40

test: remove leftover code in vitest

a7f8c44

BCerki force-pushed the 2123-remove-files-have-same-hash branch from 8baa162 to a7f8c44 Compare March 13, 2025 22:27

pbastia force-pushed the develop branch from 6a0661a to 41ae97d Compare March 24, 2025 17:10

Sepehr-Sobhani force-pushed the develop branch from 06db73a to 4ba58ea Compare March 26, 2025 23:23

@@ -6,2 +6,3 @@
             import Link from "next/link";
+            import he from 'he';
@@ -99,3 +100,3 @@
                         <Link
-                          download={fileName}
+                          download={he.encode(fileName)}
                           href={downloadUrl}
@@ -104,3 +105,3 @@
                         >
-                          {fileName}
+                          {he.encode(fileName)}
                         </Link>

@@ -106,3 +106,4 @@
                 "vite-require": "^0.2.3",
-                "vitest": "^3.0.5"
+                "vitest": "^3.0.5",
+                "he": "^1.2.0"
               },

Package	Version	Security advisories
he (npm)	1.2.0	None

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

File upload improvements #2919

File upload improvements #2919

BCerki commented Feb 26, 2025 •

edited

Loading

andrea-williams Mar 11, 2025

BCerki Mar 10, 2025

BCerki Mar 10, 2025

andrea-williams left a comment

andrea-williams Mar 11, 2025

BCerki Mar 12, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

BCerki Mar 12, 2025

andrea-williams Mar 11, 2025

BCerki commented Mar 12, 2025

		naics_code_id: Optional[int] = None
		opt_in: Optional[bool] = False

		- DocumentServiceV2.create_or_replace_operation_document
		- DocumentDataAccessServiceV2.create_document

File upload improvements #2919

Are you sure you want to change the base?

File upload improvements #2919

Conversation

BCerki commented Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrea-williams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BCerki commented Mar 12, 2025

BCerki commented Feb 26, 2025 •

edited

Loading