Use md5 node module #1308

belcherj · 2019-06-04T17:10:00Z

Fix

This uses a node module that only does md5 not a host of other hashing functions. This is only used in the sharing modal to pull down a users Gravatar

Test

Go to https://app.simplenote.com/new
Click share icon in top right add a user to share with
In Chrome dev tools get the link to the image
Build this branch
Open app
Click share
Enter email
Get image link
Compare hash from production, this branch, and another md5 hash generator
They should all match

Review

Only one developer is required to review these changes, but anyone can perform the review.

Release

RELEASE-NOTES.txt was updated in d3adb3ef with:

md5 hashing module was replaced

Closes: #1294

mirka

Works as described 👍

mirka · 2019-06-06T20:50:25Z

lib/dialogs/share/index.jsx

-    hash.update(email.trim().toLowerCase());
-    let digest = hash
-      .digest()
+    let hash = md5(email.trim().toLowerCase())


const? (I didn't realize we don't have the prefer const rule enabled in eslint!)

I need to add that! I think I'll merge and then add that rule and the fixes in another pr

dmsnell · 2019-09-20T19:01:07Z

What was the purpose of this PR? Were we having trouble with the create-hash library? Was it unmaintained?

belcherj · 2019-09-21T01:11:02Z

We were including create-hash that does a lot of things besides creating an md5 hash. If I remember correctly I did away the middleman and just included the md5 library which just creates an md5 hash.

dmsnell · 2019-09-23T02:17:49Z

Ah. I just noticed now that this merged months ago. I guess @belcherj we're saying that the purpose was to eliminate superfluous dependencies? Did you catch a before-and-after count of dependencies in the node_modules directory?

I'm not sure if I did this properly, and with npm it's always hard to do testing from code that existed in the past (because the repository is always changing) but I found that this change added three new directories to node_modules after applying it. That seems counter-intuitive, but with the way dependencies work it wouldn't surprise me if this is right.

	Before applying patch	After applying patch
Directories in `node_modules`	1037	1040
Unique packages in `npm list`	1071	1074

For counting unique packages in npm list I used the following command to try and get the total number of unique package names. I'm not sure if it's important to count unique versions or if those are even installed. I discovered that counting total number of dependencies is hard.

npm list | sed -e 's/[├┬│─└]//g' | sed -e 's/^[[:space:]]*//' | sed -e 's/@.*//g' | sort | uniq | wc -l

Either way it looks like this change probably increased our dependency count and did so in a mostly insignificant way.

belcherj · 2019-09-24T18:59:11Z

I did not catch the before and after but: https://www.npmjs.com/package/md5 has three dependencies which in turn have zero dependencies.

https://www.npmjs.com/package/create-hash has 5 dependencies which in turn have more dependencies. Not sure how the number of dependencies could have gone up.

dmsnell · 2019-09-24T20:20:58Z

Not sure how the number of dependencies could have gone up.

npm is funny sometimes. one way it could have gone up is that if create-hash were already a transitive dependency and already pulled in md5 then our introduction of md5 as a new top-level dependency at a specific version may have introduced a second copy of md5 (as another version) while also not eliminating the cause for the create-hash sub-dependency, thus leaving it in there.

I wish I were better at navigating the dependency tree and finding answers to these kinds of questions.

From some additional basic investigation…

many packages depend on create-hash: browserify-, public-encrypt, pbkdf2, parse-asn1, create-hmac. they are pulling in ^1.1.0
create-hash@1.20 is in the tree and it depends on md5.js@^1.3.4
md5.js@1.3.4 is required by evp_bytestokey
md5@2.2.1 is a top-level dependency

So I think what happened is we added a new top-level dependency we didn't already have in our dependency tree before but we also didn't remove any of the dependencies for the package we were presumably replacing. In effect, this didn't do anything but introduce a new dependency.

Had we used md5.js instead of md5 we may have eliminated the top-level dependency of create-hash without introducing new dependencies, but the net effect would be that we didn't change the total number of dependencies. At best, possibly by using md5.js we would leave open the possibility that if several of our dependencies also remove their need for create-hash that we could eventually get rid of it too because we wouldn't directly depend on it.

If we continue with the numbers from earlier then by switching md5 to md5.js the new directory count is 1037, matching the pre-patch count, and the unique packages count is 1857, so I have no idea how that happened - presumably because it updated the package-lock.json based on today when I changed md5 to md5.js vs. what it made when this merged.

See #1308 When we added a new direct dependency on `md5` we inadvertendly created three new dependencies to the application. We intended to _remove_ a depdendency on `create-hash` but that library was already transitively pulled in via several other packages. Those packages and `create-hash` depend on `md5.js` and when we added `md5` as a requirement it added something we didn't already have. In this patch we're replacing our use of `md5` with `md5.js` and thus removing the added dependencies, since `md5.js` is already in the project. An alternative approach to remove the excess dependencies would be to revert #1308 and rely directly on `create-hash` again. Before applying this patch there are 1160 directories in `node_modules`. After applying this patch there are 1157 directories in `node_modules`. This count may or may not be relevant.

belcherj added the [status] needs code review label Jun 4, 2019

belcherj requested review from bummytime and mirka June 4, 2019 17:10

belcherj self-assigned this Jun 4, 2019

belcherj mentioned this pull request Jun 4, 2019

Update dependency create-hash to v1.2.0 - autoclosed #1285

Closed

1 task

belcherj force-pushed the update/use-better-dependency branch from ac1d26b to 8db9b91 Compare June 4, 2019 18:31

mirka approved these changes Jun 6, 2019

View reviewed changes

mirka added [status] Ready to Merge and removed [status] needs code review labels Jun 6, 2019

belcherj added 2 commits June 7, 2019 09:21

Use md5 node module

db0a044

Push package-lock

c598726

belcherj force-pushed the update/use-better-dependency branch from 8db9b91 to c598726 Compare June 7, 2019 13:23

belcherj merged commit 9f0c7ea into master Jun 7, 2019

belcherj deleted the update/use-better-dependency branch June 7, 2019 13:24

bummytime added this to the 1.6.0 ❄️ milestone Jun 17, 2019

dmsnell mentioned this pull request Sep 24, 2019

Deps: Remove redundant md5 library #1591

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use md5 node module #1308

Use md5 node module #1308

belcherj commented Jun 4, 2019 •

edited

mirka left a comment

mirka Jun 6, 2019

belcherj Jun 7, 2019

dmsnell commented Sep 20, 2019

belcherj commented Sep 21, 2019

dmsnell commented Sep 23, 2019 •

edited

belcherj commented Sep 24, 2019

dmsnell commented Sep 24, 2019

Use md5 node module #1308

Use md5 node module #1308

Conversation

belcherj commented Jun 4, 2019 • edited

Fix

Test

Review

Release

mirka left a comment

Choose a reason for hiding this comment

mirka Jun 6, 2019

Choose a reason for hiding this comment

belcherj Jun 7, 2019

Choose a reason for hiding this comment

dmsnell commented Sep 20, 2019

belcherj commented Sep 21, 2019

dmsnell commented Sep 23, 2019 • edited

belcherj commented Sep 24, 2019

dmsnell commented Sep 24, 2019

belcherj commented Jun 4, 2019 •

edited

dmsnell commented Sep 23, 2019 •

edited