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

Fix issue with AES 256 breaking when running in Electron #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

superical
Copy link

The name used for the AES 256 algorithm will break Cryptex when used in Electron 73. It seems like the fix to this issue is by changing the algorithm name to aes-256-cbc for the crypto package to work. Otherwise, an "Unknown Cipher" error message will be thrown.

This PR attempts to detect whether the environment is Electron or not. If it is, we will use aes-256-cbc as the algorithm name. Otherwise, we will use aes256 in a regular Node environment.

@superical superical changed the title Fixes an issue with AES 256 breaking when running in Electron Fix issue with AES 256 breaking when running in Electron Oct 31, 2019
@superical
Copy link
Author

@TomFrost It would be great if I could have this PR reviewed and merged because this will help many Electron projects to continue using Cryptex. Thanks!

@TomFrost
Copy link
Owner

TomFrost commented Nov 4, 2019

Hi @superical, thanks for this! It looks like the only thing necessary to fix the build is npm run format. I can run that when I'm home, but if you'd like to add that then I can merge and release now.

@TomFrost
Copy link
Owner

TomFrost commented Nov 4, 2019

Actually, circling back, is the issue that electron doesn't include/blocks the aes256 cipher? I'm curious why that doesn't work if Node, ultimately, is what's supplying the ciphers.

If possible, it might make sense to use https://nodejs.org/api/crypto.html#crypto_crypto_getciphers to make the determination, rather than sniffing for signs of electron.

@superical
Copy link
Author

@TomFrost I agree that it makes more sense to use that. I will check on that and probably update the PR again very soon.

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