Skip to content

fix: add proper CSRF for bulk upload#1036

Merged
carddev81 merged 1 commit intomainfrom
CK-7vn/id-504
Nov 11, 2025
Merged

fix: add proper CSRF for bulk upload#1036
carddev81 merged 1 commit intomainfrom
CK-7vn/id-504

Conversation

@CK-7vn
Copy link
Copy Markdown
Member

@CK-7vn CK-7vn commented Nov 4, 2025

Description of the change

PR Fixes CSRF token missing error in bulk user upload feature. In particular this is a refactoring of the API client to support FormData/file uploads.

Screenshot(s)

screenshot-2025-11-03_21-55-36

Additional context

Root issue: BulkUploadModal component directly used a fetch call to upload CSV files, this bypassed the central API client by default excluding the X-CSRF-Token header that the backend now requires. Rather than adding an entirely separate uploadFile() method I decided upon refactoring the existing fetchWithHandling() method in the API client to automatically detect and handle FormData.

X

I looked through the codebase for other CSRF vulnerabilities or potential issues, I found 10 other direct fetch() calls in the frontend, 9 of 10 are GET requests which don't require CSRF tokens, the other 1 was the BulkUploadModal which was fixed in this PR. So, if there any other CSRF issues, I definitely missed them/it.

Other benefits of this implementation include the fact that now API.put() and API.patch() automatically now support file uploads without any additional code. Now there is a single source of truth for all HTTP request handling.

@CK-7vn CK-7vn requested a review from a team as a code owner November 4, 2025 03:18
@CK-7vn CK-7vn requested review from carddev81 and removed request for a team November 4, 2025 03:18
Copy link
Copy Markdown
Contributor

@calisio calisio left a comment

Choose a reason for hiding this comment

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

looks good and tested good. Thanks!

Copy link
Copy Markdown
Contributor

@carddev81 carddev81 left a comment

Choose a reason for hiding this comment

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

Looks and tested great.

@carddev81 carddev81 merged commit 6c98e20 into main Nov 11, 2025
9 checks passed
@carddev81 carddev81 deleted the CK-7vn/id-504 branch November 11, 2025 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants