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

Allow an option to choose the hashing algorithm #374

Closed
Kpovoc opened this issue Jun 4, 2024 · 9 comments
Closed

Allow an option to choose the hashing algorithm #374

Kpovoc opened this issue Jun 4, 2024 · 9 comments

Comments

@Kpovoc
Copy link

Kpovoc commented Jun 4, 2024

Hi! First of all, thank you for all of your hard work! This module is super useful, and makes file-upload a breeze.

The Issue

My team is using this module in an app running in a restricted environment. FIPS compliant algorithms are strictly enforced, and this is causing an error when express-fileupload uses MD5 to hash the temporary files.

The Suggestion

I think a good solution would be adding a hashing-algorithm option to the setup, to be used instead of a hard-coded 'md5'.

I don't mind doing the work and putting in a PR, but I wanted to make an issue first to see if this solution was a desired one.

Thank you for your time!

@RomanBurunkov
Copy link
Collaborator

Hi, sure, go ahead.

@barucoh
Copy link

barucoh commented Jun 23, 2024

+1

@Kpovoc Is there an update on this? I'm facing the same issue.
Also, there seems to be an existing PR for that - #245

@Kpovoc
Copy link
Author

Kpovoc commented Jun 24, 2024

@barucoh Sorry for my delay. I just go back in from a 2 week vacation. Kind of bad timing on my part for opening the issue, but I didn't know how responsive the author/maintainer would be. 😇

I'm going to work it today, and try to have something up soon.

I am curious why that PR you found from 4 years ago didn't make the cut.

Kpovoc added a commit to Kpovoc/express-fileupload that referenced this issue Jun 25, 2024
- hashAlgorithm key added to DEFAULT_OPTIONS
- hashAlgorithm key can be overriden by value given in options argument
- buildOptions function will ensure a default a hashAlgorithm exists,
  and throw an error if the provided option is not supported by the
  system.
  - throws early error during app setup, rather than waiting
    until an upload is attempted
- memHandler and tempFileHandler use the hashAlgorithm option now
- Updated tests for buildOptions function in utilities.spec.js

ref: richardgirges#374
@Kpovoc
Copy link
Author

Kpovoc commented Jun 25, 2024

@RomanBurunkov PR is up for review. Let me know if something isn't to your liking. All tests are passing.

@RomanBurunkov
Copy link
Collaborator

RomanBurunkov commented Jun 26, 2024

Hi @Kpovoc

I checked a PR, please check my comments on it.
Along with changes in code we also need new option description in readme and some clarification that factory field with md5 props is not always md5.

Leaving the md5 prop name, I would consider as a tradeoff between new feature and backward compatibility.

@Kpovoc
Copy link
Author

Kpovoc commented Jun 26, 2024

@RomanBurunkov I have addressed your comments as I understood them, and updated the README with your suggestions here. I agree with leaving the md5 prop name for backward compatibility.

Also, I iterated the version number on the package.json. I wasn't sure if this way something I needed to do, or if it was something that would automatically happen in the build pipeline.

@Kpovoc
Copy link
Author

Kpovoc commented Jul 10, 2024

@RomanBurunkov Gentle reminder that I am awaiting feedback or a merge in my PR. Thank you for your time and attention. Hope all is well.

@RomanBurunkov
Copy link
Collaborator

PR #375 merged and new version published.

@barucoh
Copy link

barucoh commented Aug 29, 2024

For whom it may concern, I added this to DefinitelyTyped to be supported in TypeScript

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

No branches or pull requests

3 participants