search

richardgirges / express-fileupload

Simple express file upload middleware that wraps around busboy
MIT License
1.53k stars 260 forks source link

Allow an option to choose the hashing algorithm #374

Closed Kpovoc closed 1 month ago

Kpovoc commented 4 months ago

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 commented 3 months ago

Hi, sure, go ahead.

barucoh commented 3 months ago

+1

@Kpovoc Is there an update on this? I'm facing the same issue. Also, there seems to be an existing PR for that - https://github.com/richardgirges/express-fileupload/pull/245

Kpovoc commented 3 months ago

@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 commented 3 months ago

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

RomanBurunkov commented 3 months ago

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 commented 3 months ago

@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 commented 2 months ago

@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 commented 2 months ago

PR #375 merged and new version published.

barucoh commented 1 month ago

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