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

Explicitly set default encoding in createHash() #5

Merged
merged 1 commit into from
Feb 23, 2017

Conversation

addaleax
Copy link
Contributor

Up until now, the default encoding for this is binary, but this is scheduled to change in Node 8. Since this module is currently always passing string input to the Hash object, it needs to account for that.

A better fix would likely involve dropping all strings handling and treating binary data using Buffer objects, but I kept this change minimal to avoid any breakage.

Refs: nodejs/node#8611

@malept Could you backport this change to the 1.x.x versions of sumchecker? This came up because a major dependent of sumchecker, electron-prebuilt, would be affected by the change in Node, and it still supports 0.10.x/0.12.x Node versions.

Up until now, the default encoding for this is `binary`, but
this is scheduled to change in Node 8. Since this module is
currently always passing string input to the `Hash` object,
it needs to account for that.

A better fix would likely involve dropping all strings handling
and treating binary data using Buffer objects, but I kept
this change minimal to avoid any breakage.

Refs: nodejs/node#8611
@addaleax
Copy link
Contributor Author

Also, fyi: This code is not optimal anyway. One of the reasons for switching the default encoding to UTF-8 is that it actually affects security – with binary, multiple characters would get mapped to the same hash, because binary effectively just takes the last 8 bits of the codepoint and throws the rest away.

@malept
Copy link
Owner

malept commented Feb 16, 2017

I'll try to look at this PR this weekend.

Also, fyi: This code is not optimal anyway. One of the reasons for switching the default encoding to UTF-8 is that it actually affects security – with binary, multiple characters would get mapped to the same hash, because binary effectively just takes the last 8 bits of the codepoint and throws the rest away.

It's like that because the checksums for Electron are generated with the -b flag of sha256sum. I am open to changing the default (and appropriately major version bumping this module), but I'll need to keep electron-download checking the Electron files using the binary encoding.

@addaleax
Copy link
Contributor Author

I'll try to look at this PR this weekend.

Thanks!

but I'll need to keep electron-download checking the Electron files using the binary encoding.

What I meant was that, while you can process binary data using that encoding, it would be a bit more customary to not apply any string encoding/decoding at all and just use Buffer objects. That should have behaviour equivalent to reading the file as binary and then passing it to the Hash object as binary again (i.e. what currently happens).

@addaleax
Copy link
Contributor Author

Bump @malept – This PR really only explicitly sets the defaultEncoding to what it currently is by default, it should be very low-risk.

@malept
Copy link
Owner

malept commented Feb 21, 2017

I didn't have time this weekend to look at it mostly because of the backporting part. However, I did just push a v1 branch that you can submit a PR against.

@addaleax
Copy link
Contributor Author

@malept thanks, done!

@malept malept self-requested a review February 21, 2017 22:33
@malept malept merged commit 0874486 into malept:master Feb 23, 2017
@addaleax addaleax deleted the fix-default-encoding branch February 23, 2017 14:48
wincent added a commit to wincent/corpus that referenced this pull request Aug 11, 2017
Fails to install using Node 8; see:

- electron-userland/electron-prebuilt#264
- malept/sumchecker#5

Tried this:

```
yarn upgrade electron electron-download sumchecker
```

But it blows up due to the checksum "mismatch".

Worked around that by downgrading to 7 temporarily:

```
brew install n
sudo n 7.10.1
yarn upgrade electron
sudo n latest
```
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

Successfully merging this pull request may close these issues.

2 participants