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 rendered in the DOM. This can be achieved by using a library like he (HTML entities) to encode the file name, which will prevent any HTML or JavaScript from being executed.

Import the he library for encoding HTML entities.

Encode the fileName before rendering 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

andrea-williams · 2025-03-11T00:34:11Z

bc_obps/registration/schema/operation.py

+# Administration schemas
+
+
+class OperationAdminstrationIn(OperationRegistrationIn):


Suggested change

class OperationAdminstrationIn(OperationRegistrationIn):

class OperationAdministrationIn(OperationRegistrationIn):

andrea-williams · 2025-03-11T00:34:22Z

bc_obps/registration/schema/operation.py

+    operation_representatives: List[int]
+
+
+class OperationAdminstrationInWithDocuments(OperationRegistrationInWithDocuments):


Suggested change

class OperationAdminstrationInWithDocuments(OperationRegistrationInWithDocuments):

class OperationAdministrationInWithDocuments(OperationRegistrationInWithDocuments):

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?

andrea-williams · 2025-03-11T00:39:25Z

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

+    description="""Retrieves a document""",
+    auth=authorize("approved_authorized_roles"),
+)
+def get_operation_doucment(


Suggested change

def get_operation_doucment(

def get_operation_document(

I think this function name could be renamed to a more accurate descriptor - e.g., get_operation_document_by_type

andrea-williams · 2025-03-11T00:40:58Z

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

+    "/operations/{uuid:operation_id}/documents/{uuid:document_type}",
+    response={200: DocumentOut, custom_codes_4xx: Message},
+    tags=OPERATOR_TAGS,
+    description="""Retrieves a document""",


Suggested change

description="""Retrieves a document""",

description="""Retrieves the most recent version of a document for the specified operation and document type.""",

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

andrea-williams · 2025-03-11T20:36:13Z

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

@@ -897,7 +904,7 @@ describe("the OperationInformationForm component", () => {
    expect(screen.getByText(/Must not have fewer than 1 items/i)).toBeVisible();
  });

-  it(
+  it.only(


Suggested change

it.only(

it(

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?

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 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

Open

BCerki commented Mar 10, 2025

View reviewed changes

BCerki force-pushed the 2123-remove-files-have-same-hash branch 3 times, most recently from 9f2664b to 06b749b Compare March 10, 2025 16:21

BCerki added 7 commits March 10, 2025 10:03

chore: backend changes to file uploads

2a94bcd

chore: set blob chunk size

32a66a6

test: backend file upload tests

b4c35ad

chore: FE file upload refactor

8cd60aa

test: vitests for file upload refactor

f98bcad

docs: explain file upload strategy

62a8f4f

chore: post-rebase updates

6a71ad4

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

andrea-williams reviewed Mar 11, 2025

View reviewed changes

@@ -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

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

andrea-williams Mar 11, 2025

BCerki Mar 10, 2025

BCerki Mar 10, 2025

andrea-williams left a comment

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

andrea-williams Mar 11, 2025

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

		# Administration schemas


		class OperationAdminstrationIn(OperationRegistrationIn):

	class OperationAdminstrationIn(OperationRegistrationIn):
	class OperationAdministrationIn(OperationRegistrationIn):

		operation_representatives: List[int]


		class OperationAdminstrationInWithDocuments(OperationRegistrationInWithDocuments):

	class OperationAdminstrationInWithDocuments(OperationRegistrationInWithDocuments):
	class OperationAdministrationInWithDocuments(OperationRegistrationInWithDocuments):

	description="""Retrieves a document""",
	description="""Retrieves the most recent version of a document for the specified operation and document type.""",

		- 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

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 Feb 26, 2025 •

edited

Loading