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

improve keys checks for jsthemis in SecureMessage #465

Merged
merged 4 commits into from
May 7, 2019

Conversation

vixentael
Copy link
Contributor

@vixentael vixentael commented May 6, 2019

In new SecureMessage API we made a mistake when checking keys for SecureMessage. For sign mode we should check private key, for verify mode we should check public key.

This affects only error messages for user, Themis uses correct keys while actual signing/verifying.

After this merge, we should update jsthemis and push to npm again as 0.11.2.

Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

would be great to add some regression tests for these cases when we create secure message with empty keys and catch exceptions

@ilammy
Copy link
Collaborator

ilammy commented May 6, 2019

We have some tests here:

it("empty keys", function(){
encrypted_message = encrypter.encrypt(message);
signed_message = encrypter.sign(message);
assert.throws(function(){empty_secure_message.encrypt(message);}, expect_code(addon.INVALID_PARAMETER));
assert.throws(function(){empty_secure_message.decrypt(encrypted_message);}, expect_code(addon.INVALID_PARAMETER));
assert.throws(function(){empty_secure_message.sign(message);}, expect_code(addon.INVALID_PARAMETER));
assert.throws(function(){empty_secure_message.verify(signed_message);}, expect_code(addon.INVALID_PARAMETER));
})

But unfortunately they don't cover the case when only one of the keys is empty.

They also do not check the specific error message, only the error code. The assert.throws() function can accept a regex for the message (or, probably, it would be better to check both code and message properties).

@vixentael vixentael mentioned this pull request May 6, 2019
@vixentael
Copy link
Contributor Author

@Lagovas @ilammy i've extended test suite

Copy link
Collaborator

@Lagovas Lagovas left a comment

Choose a reason for hiding this comment

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

nice

Copy link
Collaborator

@ilammy ilammy left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I wonder whether we should invest more into code coverage analysis over wrappers to catch mistakes like this...

@vixentael vixentael merged commit a5c57f9 into master May 7, 2019
@vixentael vixentael deleted the vxtl/T1176-js-correct-messages branch May 7, 2019 09:02
@vixentael vixentael mentioned this pull request May 7, 2019
@ilammy ilammy added W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages and removed E-Node.js labels Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants