Conversation
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
@SocketSecurity ignore npm/entities@4.5.0 |
ropable
left a comment
ropable
left a comment
There was a problem hiding this comment.
Recommendations:
- models.PrivateMediaStorage: for all media uploads, consider a using Azure blob storage (in conjunction with django-storages) as the storage method rather than local file storage in order to help make the system more portable/resilient. Note that this recommendation will have implications for your views (it's generally easier to serve blobs).
- models.PrivateMediaStorage: consider not extending the Django FileSystemStorage class at all, not using a separate local "private" media directory path, and instead manage all file access/authorisation within the model and views.
- models.PrivateMediaStorage: if you absolutely must use this class, consider removing the path.exists check in init as it might make bootstrapping a new project harder. Also consider moving the class out of models.py (e.g. storage.py).
- If you keep PrivateMediaStorage, give settings.PRIVATE_MEDIA_ROOT a default value (even an empty string).
- entrypoint.sh: remove the migrate command (run this manually during changes to avoid it being run simultaneously in production environments).
- entrypoint.sh: move the collectstatic command out and into the Dockerfile instead in order to guarantee the state of static assets in a runnng container, rather than running it dynamically on every start.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Hey @ropable, Thanks very much for your review and recommendations. I wanted to run this with you before merging to main but as far as I can see you're on leave until Wednesday. So, I'm writing this to update you on what has been addressed and what has not, with reasons behind.
I have replaced the storage backend to Azure Blob as you recommended. It is working nicely on UAT (with DEV container though, waiting on OIM to give me the UAT container).
I explicitly want to avoid using Django's MEDIA file system, although I am still managing the access from views / models. The files we are managing are official sensitive and personally identifiable. I don't want these files to be written and served from the "default public" MEDIA files.
I have moved that class to its own Python file, not checking / throwing error if the folder doesn't exist on initialisation as you recommended. I did create that directory in Django settings replicating the PRS application behaviour.
I want the application to fail explicitly if the path was not given by the environment in combination of using
Thank you for catching these 2 issues. I will definitely address them asap but on the main branch. |
| rootProps.onDragLeave = (e) => e.preventDefault(); | ||
| rootProps.onDragOver = (e) => e.preventDefault(); | ||
| } | ||
| // Calculate the maximum file size in MB for human readable display | ||
| const maxFilesizeMB = Math.round(clientConfig.upload_max_size / (1024 * 1024)); | ||
|
|
||
| return ( | ||
| <Box {...rootProps}> | ||
| <VisuallyHiddenInput {...inputProps} /> | ||
| {styling.icon} | ||
| <br /><br /> | ||
| {progress === null ? | ||
| styling.icon : | ||
| <LinearProgress variant="determinate" value={progress} color="success" className="w-full mt-8" /> | ||
| } | ||
|
|
||
| {filename ? ( |
There was a problem hiding this comment.
Bug: The file upload request in DropzoneDialogContent is not cancelled when the component unmounts, such as when a user navigates to another form step during an upload.
Severity: MEDIUM
Suggested Fix
Create an AbortController instance within the DropzoneDialogContent component. Pass its signal to the ApiManager.uploadAttachment function call. Implement a useEffect cleanup function that calls controller.abort() when the component unmounts. This will ensure that any in-progress upload is cancelled if the user navigates away.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: frontend/src/components/inputs/file.tsx#L174-L224
Potential issue: The refactored `DropzoneDialogContent` component no longer uses an
`AbortController` for file uploads. If a user starts an upload and then navigates to a
different form step, the component unmounts but the underlying `axios` request is not
cancelled. This continues the upload in the background, consuming server resources and
bandwidth. When the request eventually completes, its promise handlers (`.then()` and
`.finally()`) will attempt to update the state of the unmounted component, leading to
React warnings and the creation of orphaned attachment records on the server for an
upload the user effectively abandoned.
4 participants
Pull Request: File Attachments Feature
Working copy in UAT:
https://authorisations-uat-internal-oim03.dbca.wa.gov.au/my-applications
Please go with the "Care and Use of Animals" application and you will see 2 file attachment fields on the "Project Details" step:
A) 8. Attach your current CI competency form
F) 2. Please attach location map of the fieldwork
Overview
This PR implements a comprehensive file attachment system for applications, allowing users to upload, manage, and track files associated with their submissions. The feature includes frontend drag-and-drop functionality, backend validation, and a dedicated database model for attachment management.
Key Implementation Details
Frontend: Drag & Drop Upload
Files:
frontend/src/components/inputs/file.tsxclientConfig.upload_max_size(10MB limit)UPLOAD_MIME_TYPES(JPEG, PNG, PDF)Backend: File Attachment Model
File:
backend/applications/models.pyNew
ApplicationAttachmentmodel provides:Applicationis_deletedflag for data integrityAPI Endpoint: File Upload & Management
Files:
backend/api/views.py,backend/applications/serialisers.pyEndpoint:
GET, POST, PATCH, DEL /api/attachmentsAttachmentViewSethandles all attachment related API methodsAttachmentSerialiservalidates attachment metadata and uploaded fileIApplicationAttachmentwith attachment metadata to frontendServer-Side Validation
File:
backend/applications/serialisers.pyAttachmentSerialiser.validate_file()enforcesUPLOAD_MAX_SIZElimitFileTooLargeErrorexception returns appropriate HTTP 413 statusConfiguration
File:
backend/config/settings.pyUPLOAD_MAX_SIZE = 10 * 1024 * 1024(10MB)UPLOAD_MIME_TYPESwhitelist: JPEG, PNG, PDFbackend/api/serialisers.pyClientConfigSerialiserFrontend API Management
File:
frontend/src/context/ApiManager.tsxApiManager.uploadAttachment()method handles multipart form uploadApiManager.deleteAttachment()to handle attachment DEL methodApiManager.renameAttachment()to handle attachment rename PATCH methodAreas for Review
PrivateMediaStorageinbackend/applications/models.pyfor storage path integrationTesting Checklist