use hash-base #36

fanatid · 2016-04-14T14:43:49Z

dcousens · 2016-04-15T03:06:02Z

README.md


+  - SHA (SHA-0) -- **legacy, no not use in new systems**
+  - SHA-1 -- **legacy, no not use in new systems**


do not, not no not

Fixed, thanks.
I doubted about this phrase, but decided not change it when edited README.md

dcousens · 2016-04-18T01:11:26Z

lib/sha1.js

+var inherits = require('inherits')
+var SHA = require('./sha')
+
+var ARRAY80 = new Array(80)


Maybe just call this var W = new Array(80), and let it be super clear that it is a global used by _getExpandedMessage [by moving it to just above that declaration]

dcousens · 2016-04-18T01:19:11Z

utACK e6e054c,

fanatid · 2016-04-18T05:31:01Z

@dcousens updated

dcousens · 2016-11-10T01:48:30Z

Published as + [email protected]

dcousens · 2016-11-10T01:48:44Z

Why did you move the files to lib/?
Seems a bit unnecessary IMHO.

Also, I personally prefer functions before use, but hey, its still standard so whatever.

dcousens · 2016-11-10T01:49:46Z

lib/sha.js

+
+SHA.prototype._expandMessage = function (W) {
+  for (var i = 0; i < 16; ++i) W[i] = this._block.readInt32BE(i * 4)
+  for (; i < 80; ++i) W[i] = W[i - 3] ^ W[i - 8] ^ W[i - 14] ^ W[i - 16]


Where did this come from?

dcousens · 2016-11-10T01:50:18Z

sha.js

-  var e = this._e | 0
-
-  for (var i = 0; i < 16; ++i) W[i] = M.readInt32BE(i * 4)
-  for (; i < 80; ++i) W[i] = W[i - 3] ^ W[i - 8] ^ W[i - 14] ^ W[i - 16]


Ah, this. All good 👍

dcousens · 2016-11-10T01:52:02Z

bin.js

-  }
-} else {
-  usage()
-}


Ah, wait.
This is a breaking change.

Can you please re-add so I can release a fixed patch?

dcousens · 2016-11-10T01:52:29Z

Why did you remove .bin... it might be used by people.

dcousens · 2016-11-10T01:54:48Z

Re-added bin.js, released as v2.4.7
(unpublished v2.4.6)

dcousens · 2016-11-10T01:58:11Z

@fanatid please keep PRs atomic.

Don't remove things in the same PR resolving something as useful as #25.

dcousens · 2016-11-10T02:00:06Z

index.js

-exports.sha224 = require('./sha224')
-exports.sha256 = require('./sha256')
-exports.sha384 = require('./sha384')
-exports.sha512 = require('./sha512')


dcousens · 2016-11-10T02:00:26Z

@fanatid also please mark things as major bump if they are.

I spent most of my review time looking over the math changes/refactors instead of focusing on the exports... apologies all (as they weren't flagged as changed).

Unpublished v2.4.7.

dcousens · 2016-11-10T02:02:28Z

@fanatid can you please re-add the tests, un-change the exports to avoid breaking backwards compatibility and then submit that PR, so we can release this as a non-breaking change?

fanatid · 2016-11-10T08:00:21Z

@dcousens please revert all change and unpublish, I can't add tests/refactor PR right now.
I mentioned in #39 that this PR need major bump.
Sorry for not atomic commits. We need merge such big PR carefully.

dcousens · 2016-11-10T10:10:16Z

@fanatid I already unpublished the changes.
The change need not be reverted, it just needs to be fixed to be non-breaking 👍

I mentioned in #39 that this PR need major bump.

I didn't see that until after.

fanatid · 2016-11-10T10:12:10Z

I think hash-base makes breaking changes anyway.
utf8 instead binary by default for example

dcousens · 2016-11-10T10:13:58Z

@fanatid not willing to be backwards compatible in hash-base?

fanatid · 2016-11-10T10:19:38Z

are you think that this necessary? 0.12 was last version of node who uses binary by default

gethinwebster · 2016-11-10T17:51:50Z

Just to note, as others may come across this as well, this update broke some of our projects which use this module, but don't use browserify. hash-base pulls in a new dependency of stream, which wasn't previously required, and although it's provided as a shim by default in browserify, other packagers vary.

fanatid · 2016-11-10T17:54:48Z

@gethinwebster sorry hear this, but is not 2.4.7 fix hash-base changes?

gethinwebster · 2016-11-10T18:06:17Z

It doesn't seem to - looks like 2.4.7 still uses hash-base unless I'm missing something?

fanatid · 2016-11-10T18:08:32Z

ah, I see :(
@dcousens what you think about publish 2.4.8 equal to old ~~2.4.6~~2.4.5 and unpublish 2.4.7?

gethinwebster · 2016-11-10T18:09:45Z

2.4.5 is the latest version that works correctly for me

dominictarr · 2016-11-10T22:37:01Z

Ah, so moving the files into lib was a breaking change.
I was using it like this:
https://github.com/dominictarr/sodium-browserify-tweetnacl/blob/master/index.js#L3

and so I have lots of stuff that is broken now. If it goes into lib it needs a major semver bump

dcousens · 2016-11-10T23:34:18Z

Agreed, I commonly use it like sha.js/sha256 as well.
Will need to revert lib/ as well.

Revert lib/
Revert export uppercasing
Revert bin.js removal

dcousens · 2016-11-10T23:43:33Z

Unpublish wasn't working as expected for v2.4.7.
Reverted all changes and published as v2.4.8.

@dominictarr @fanatid please verify.

Sorry for the screw-around @gethinwebster.

gethinwebster · 2016-11-11T08:49:16Z

Thanks for the really quick response on this, great work @dcousens @fanatid

dominictarr · 2016-11-11T09:48:28Z

yup, working again.

dcousens · 2016-11-11T13:12:18Z

@fanatid please submit atomic PRs for the changes 👍

dcousens · 2017-04-20T05:56:50Z

@fanatid what were the blocks here?
I think we could default to utf8 over binary using the same trick we did in pbkdf2?

larcho · 2023-06-26T03:28:46Z

I know this is a very very VERY old PR, but I'd like to double check something. I actually do like this version because it uses readable-stream from hash-base instead of standard Browser/Node Stream. React Native does not include Stream.
I'm aware of the rn-nodeify, but I'm updating a library that depends on the crypto library to not require "nodeification".
Is there any reason I should not use this version?

fanatid added 3 commits April 11, 2016 09:57

remove bin.js

f7c86a7

move shaX to lib directory

cf2ab1d

update tests

8522be9

fanatid added the feature label Apr 14, 2016

dcousens reviewed Apr 15, 2016
View reviewed changes

fanatid force-pushed the feature/hash-base branch from 3a2038a to e6e054c Compare April 15, 2016 04:37

fanatid mentioned this pull request Apr 15, 2016

[WIP] update dependencies browserify/createHash#13

Closed

dcousens reviewed Apr 18, 2016
View reviewed changes

fanatid force-pushed the feature/hash-base branch from e6e054c to 7ba67d6 Compare April 18, 2016 05:27

update implementations

aba27f9

fanatid force-pushed the feature/hash-base branch from 7ba67d6 to 7d049ec Compare April 18, 2016 05:30

update README.md

8eec0fb

fanatid force-pushed the feature/hash-base branch from 7d049ec to 8eec0fb Compare May 4, 2016 10:42

fanatid mentioned this pull request May 19, 2016

Bug in sha implementation (likely due to international characters) #37

Closed

fanatid mentioned this pull request Nov 9, 2016

Fixes for situation when data size in bits is a great than max number for type Int32 #39

Closed

dcousens merged commit 21e2873 into master Nov 10, 2016

dcousens deleted the feature/hash-base branch November 10, 2016 01:46

dcousens reviewed Nov 10, 2016

View reviewed changes

bin.js

}

} else {

usage()

}

Copy link

Member

dcousens Nov 10, 2016

Choose a reason for hiding this comment

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

Ah, wait.
This is a breaking change.

Can you please re-add so I can release a fixed patch?

dcousens reviewed Nov 10, 2016

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use hash-base #36

use hash-base #36

fanatid commented Apr 14, 2016

dcousens Apr 15, 2016

fanatid Apr 15, 2016

dcousens Apr 18, 2016 •

edited

Loading

dcousens commented Apr 18, 2016

fanatid commented Apr 18, 2016

dcousens commented Nov 10, 2016

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens Nov 10, 2016 •

edited

Loading

dcousens Nov 10, 2016

dcousens Nov 10, 2016

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens Nov 10, 2016

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016

fanatid commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

fanatid commented Nov 10, 2016

dcousens commented Nov 10, 2016

fanatid commented Nov 10, 2016

gethinwebster commented Nov 10, 2016

fanatid commented Nov 10, 2016

gethinwebster commented Nov 10, 2016 •

edited

Loading

fanatid commented Nov 10, 2016 •

edited

Loading

gethinwebster commented Nov 10, 2016

dominictarr commented Nov 10, 2016

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

gethinwebster commented Nov 11, 2016

dominictarr commented Nov 11, 2016

dcousens commented Nov 11, 2016

dcousens commented Apr 20, 2017

larcho commented Jun 26, 2023


		- SHA (SHA-0) -- legacy, no not use in new systems
		- SHA-1 -- legacy, no not use in new systems

use hash-base #36

use hash-base #36

Conversation

fanatid commented Apr 14, 2016

dcousens Apr 15, 2016

Choose a reason for hiding this comment

fanatid Apr 15, 2016

Choose a reason for hiding this comment

dcousens Apr 18, 2016 • edited Loading

Choose a reason for hiding this comment

dcousens commented Apr 18, 2016

fanatid commented Apr 18, 2016

dcousens commented Nov 10, 2016

dcousens commented Nov 10, 2016 • edited Loading

dcousens Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

dcousens Nov 10, 2016

Choose a reason for hiding this comment

dcousens Nov 10, 2016

Choose a reason for hiding this comment

dcousens commented Nov 10, 2016 • edited Loading

dcousens commented Nov 10, 2016 • edited Loading

dcousens commented Nov 10, 2016 • edited Loading

dcousens Nov 10, 2016

Choose a reason for hiding this comment

dcousens commented Nov 10, 2016 • edited Loading

dcousens commented Nov 10, 2016

fanatid commented Nov 10, 2016 • edited Loading

dcousens commented Nov 10, 2016 • edited Loading

fanatid commented Nov 10, 2016

dcousens commented Nov 10, 2016

fanatid commented Nov 10, 2016

gethinwebster commented Nov 10, 2016

fanatid commented Nov 10, 2016

gethinwebster commented Nov 10, 2016 • edited Loading

fanatid commented Nov 10, 2016 • edited Loading

gethinwebster commented Nov 10, 2016

dominictarr commented Nov 10, 2016

dcousens commented Nov 10, 2016 • edited Loading

dcousens commented Nov 10, 2016 • edited Loading

gethinwebster commented Nov 11, 2016

dominictarr commented Nov 11, 2016

dcousens commented Nov 11, 2016

dcousens commented Apr 20, 2017

larcho commented Jun 26, 2023

dcousens Apr 18, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

fanatid commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

gethinwebster commented Nov 10, 2016 •

edited

Loading

fanatid commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading

dcousens commented Nov 10, 2016 •

edited

Loading