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

Fixed #15 and #22 #24

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Fixed #15 and #22 #24

wants to merge 14 commits into from

Conversation

konsultaner
Copy link
Contributor

@softvar I forgot to pull your changes before. You might need to update the builds again. I did not update the version, because you might want to fix things first before you release.

@coveralls
Copy link

coveralls commented Jan 18, 2019

Coverage Status

Coverage decreased (-4.9%) to 81.584% when pulling 8f48849 on konsultaner:master into 679c12d on softvar:master.

@konsultaner
Copy link
Contributor Author

@softvar since this is a breaking change you might want to update the version to 1.3.0

@konsultaner
Copy link
Contributor Author

konsultaner commented Jan 20, 2019

It sure why the KDF was removed as it helps strengthen the provided key. The guts of the CryptoJSWordArray.random() should have been modified to use Crypto.getRandomValues(). The current random function isn’t generating a true random number.

@jas- I completely removed the default password

  • because a password should be provided by a trusted endpoint, not the browser
  • because the generated password was random an needed to be saved into the localstorage CryptoJSWordArray.random() was only used to generate an inital password if non was provided

test/functional.spec.js Show resolved Hide resolved
@@ -17,12 +15,6 @@ let utils = {
reason = reason ? reason : constants.WarningEnum.DEFAULT_TEXT;
console.warn(constants.WarningTypes[reason]);
},
generateSecretKey: function () {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you please explain why are these lines removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to the issues a predefined password does not work with common security standards. You saved the password in the local storege. This was not secure. Thats why I removed it. The user should use a trusted endpoint to deliver the password. In the most cases this would be the logged in user or a trusted third party.

if (!this._isBase64 && typeof this.config.encryptionNamespace === 'undefined') {
this.utils.warn(this.WarningEnum.ENCRYPTION_NAMESPACE_NOT_PROVIDED);
}
if (
Copy link
Owner

Choose a reason for hiding this comment

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

some inline comments would help understanding the code

@softvar
Copy link
Owner

softvar commented Jan 28, 2019

@konsultaner Could you please help in maintaining the coverage as well by adding test cases for the new code?

@konsultaner
Copy link
Contributor Author

@konsultaner Could you please help in maintaining the coverage as well by adding test cases for the new code?

@softvar I mostly removed code. I added tests for the code I added. This was mainly the console warnings for this.WarningEnum.ENCRYPTION_NAMESPACE_NOT_PROVIDED and this.WarningEnum.INSECURE_PASSWORD

If you find untested code I added let me know. I'll provide test for it then.

@konsultaner
Copy link
Contributor Author

@softvar I also changed some tests for the metaKey

@konsultaner
Copy link
Contributor Author

@softvar is there anything needed for you to accept the PR?

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.

None yet

4 participants