Update JsThemis tests #620

ilammy · 2020-04-08T16:52:35Z

This PR updates JsThemis tests and fixes a bunch of issues and inconsistencies that I've discovered while working on this update.

Updated tests

Existing JsThemis tests are haphazard and incomplete. I've mostly reused WasmThemis tests, adapted them to JsThemis API, and salvaged some JsThemis-specific tests.

API changes: Secure Cell

It is now possible to use null values when the associated context is omitted:

let cell = new themis.SecureCellSeal(key)

let encrypted = cell.encrypt(message)
let decrypted = cell.decrypt(encrypted, null)

This is consistent with WasmThemis.

API changes: Secure Message

It is now possible to use null values for omitted keys to initialize SecureMessage in sign/verify mode:

let verifier = new themis.SecureMessage(null, publicKey)

This is consistent with other null usage in the API.

WasmThemis has a different API for sign/verify mode of Secure Message. Its encryption API does not allow null values.

API changes: Secure Session

SecureSession will now validate private key during construction, not on the first use. This is consistent with WasmThemis (and other wrappers).

It is also possible to safely throw JavaScript exceptions from Secure Session public key callback. An exception will be reported as a failure to retrieve public key, instead of aborting the process due to Node.js error checks.

Checklist

Change is covered by automated tests
The coding guidelines are followed
Public API has proper documentation
~~Example projects and code samples are up-to-date~~ (no need)
Changelog is updated

Update Secure Cell test suite with the tests used in WasmThemis. There are some minor tweaks here and there to make them compatible, but it's all the same tests otherwise.

Instead of throwing a generic error with INVALID_PARAMETER status code, throw a proper TypeError when we notice an issue with input types. This commit updates all cryptosystems, not only Secure Cell.

In addition to allowing empty buffers and omitted arguments, accept explicit "null" values when the context is not used. This improves API compatibility with WasmThemis.

In JsThemis there is no SecureMessageSign and SecureMessageVerify objects, empty keys should be used instead.

Allow omitted keys to be specified with "null" values in addition to empty buffers. This improves compatibility with WasmThemis API and is consistent with other places where we allow optional values.

Check that the user has provided either public or private key, do not allow both keys to be omitted.

Note that JsThemis uses a bit different naming and does not have a "destroy()" method. The Secure Comparator object is freed when Node.js runtime collects the garbage.

API is slightly different in JsThemis and WasmThemis, the names are not the same and JsThemis does not have a dedicated negotiation method, using just unwrap() for that.

WasmThemis verifies the private key during Secure Session construction. Don't defer this to usage in JsThemis either.

We should check the call result as it may be empty if the callback throws an exception. Failure to check for this leads to process abort: FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal. 1: 0x8fa090 node::Abort() [node] 2: 0x8fa0dc [node] 3: 0xb0030a v8::Utils::ReportApiFailure(char const*, char const*) [node] 4: 0x7ff9182b1785 jsthemis::SecureSession::get_public_key_for_id_callback(void const*, unsigned long, void*, unsigned long, void*) [...] 9: 0xb8e96f [node] 10: 0xb8f4d9 v8::internal::Builtin_HandleApiCall(int, v8::internal::Object**, v8::internal::Isolate*) [node] 11: 0x1762fd45be1d Aborted (core dumped) We don't want this to happen.

Since we're publishing test code on npm now, we should properly attribute this file. The tests were added in 944b9e0 ("Add JSTHEMIS test", 2016-01-17).

vixentael · 2020-04-08T17:01:01Z

src/wrappers/themis/jsthemis/secure_session.cpp

@@ -36,8 +37,11 @@ int SecureSession::get_public_key_for_id_callback(
        return THEMIS_BUFFER_TOO_SMALL;
    }
    v8::Local<v8::Value> argv[1] = {Nan::CopyBuffer((char*)id, id_length).ToLocalChecked()};
-    v8::Local<v8::Value> a =
-        Nan::Call(((SecureSession*)(user_data))->id_to_pub_key_callback_, 1, argv).ToLocalChecked();
+    Nan::MaybeLocal<v8::Value> result = Nan::Call(reinterpret_cast<SecureSession*>(user_data)->id_to_pub_key_callback_, 1, argv);


looks like magic to me ✨

Previously we have asserted that no error can possibly happen here and unwrapped the MaybeLocal reference unconditionally (it's still there, just as a temporary value). Well, in fact, an error is possible here if the callback throws. Now we check for that before unwrapping the value to avoid triggering an assertion failure.

vixentael · 2020-04-08T17:06:13Z

src/wrappers/themis/jsthemis/test/test.js

+            it('leaves signed data in plaintext', function() {
+                let signer = new addon.SecureMessage(privateKeyA, null)
+                let signed = signer.sign(testInput)
+                // TODO: there has to be more idiomatic way for this check...


don't they have a substring or similar?

don't they have a substring or similar?

Vanilla JS certainly does not. It's copied from WasmThemis where we have to deal with that.

However, here we know that we return Node.js' Buffer which should have includes method. So... thanks! I'll try updating this check.

happy to help 😎

vixentael · 2020-04-08T17:09:05Z

CHANGELOG.md

@@ -287,6 +287,9 @@ _Code:_

  - New class `SymmetricKey` can be used to generate symmetric keys for Secure Cell ([#562](https://github.com/cossacklabs/themis/pull/562)).
  - New makefile target `make jsthemis` can be used to build JsThemis from source ([#618](https://github.com/cossacklabs/themis/pull/618)).
+  - `SecureCell` now allows `null` to explicitly specify omitted encryption context ([#620](https://github.com/cossacklabs/themis/pull/620)).
+  - `SecureMessage` now allows `null` for omitted keys in sign/verify mode ([#620](https://github.com/cossacklabs/themis/pull/620)).
+  - Fixed a crash when an exception is thrown from `SecureSession` callback ([#620](https://github.com/cossacklabs/themis/pull/620)).


shouldn't we add that we've changed the type of thrown errors from NaN::Error to NaN::TypeError? I'm not sure how js devs are used to catch errors (if they do catch errors), but it might be useful for them to know that error type has changed.

shouldn't we add that we've changed the type of thrown errors?

I don't think it's necessary. It is a programming error to pass an invalid type so it should be caught during development, not at run-time. If it's absolutely necessary to catch it at runtime then any existing catch clause for Error (which we have been throwing before) will catch TypeError too because it's a subclass.

okay, makes sense

vixentael · 2020-04-08T17:10:21Z

src/wrappers/themis/jsthemis/test/test.js

+                if (peer == server) {
+                    peer = client
+                } else {
+                    peer = server


this is a very smart and hacky way to establish async communication in sync piece of code 😂

We know that JsThemis returns results as instances of Node.js Buffer so we can verify that signed message includes the plaintext by using the "includes" method. We don't need to write it ourselves.

ilammy added 11 commits April 8, 2020 19:23

Secure Cell: reuse WasmThemis tests

239f8ed

Update Secure Cell test suite with the tests used in WasmThemis. There are some minor tweaks here and there to make them compatible, but it's all the same tests otherwise.

Secure Cell: report TypeError correctly

47f1ca2

Instead of throwing a generic error with INVALID_PARAMETER status code, throw a proper TypeError when we notice an issue with input types. This commit updates all cryptosystems, not only Secure Cell.

Secure Cell: allow "null" for context values

39a910e

In addition to allowing empty buffers and omitted arguments, accept explicit "null" values when the context is not used. This improves API compatibility with WasmThemis.

Secure Message: reuse WasmThemis tests

ac3bb87

In JsThemis there is no SecureMessageSign and SecureMessageVerify objects, empty keys should be used instead.

Secure Message: allow "null" keys for sign/verify

ef25982

Allow omitted keys to be specified with "null" values in addition to empty buffers. This improves compatibility with WasmThemis API and is consistent with other places where we allow optional values.

Secure Message: disallow both empty keys

37a9cc9

Check that the user has provided either public or private key, do not allow both keys to be omitted.

Secure Comparator: reuse WasmThemis tests

c9359a0

Note that JsThemis uses a bit different naming and does not have a "destroy()" method. The Secure Comparator object is freed when Node.js runtime collects the garbage.

Secure Session: reuse WasmThemis tests

b316e55

API is slightly different in JsThemis and WasmThemis, the names are not the same and JsThemis does not have a dedicated negotiation method, using just unwrap() for that.

Secure Session: check private key validity

86d5696

WasmThemis verifies the private key during Secure Session construction. Don't defer this to usage in JsThemis either.

Retroactively apply copyright to tests

6d8f4af

Since we're publishing test code on npm now, we should properly attribute this file. The tests were added in 944b9e0 ("Add JSTHEMIS test", 2016-01-17).

ilammy added tests Themis test suite W-JsThemis 🍭 Wrapper: JsThemis for Node.js, JavaScript API, npm packages labels Apr 8, 2020

ilammy requested review from Lagovas and vixentael as code owners April 8, 2020 16:52

vixentael approved these changes Apr 8, 2020

View reviewed changes

ilammy added 2 commits April 9, 2020 11:36

Simplify sign/verify test case

10d26da

We know that JsThemis returns results as instances of Node.js Buffer so we can verify that signed message includes the plaintext by using the "includes" method. We don't need to write it ourselves.

Reformat code with "make fmt"

c618c01

ilammy merged commit 49d8389 into cossacklabs:master Apr 9, 2020

ilammy deleted the jsthemis-tests branch April 9, 2020 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update JsThemis tests #620

Update JsThemis tests #620

ilammy commented Apr 8, 2020 •

edited

Loading

vixentael Apr 8, 2020

ilammy Apr 8, 2020

vixentael Apr 8, 2020

ilammy Apr 8, 2020

vixentael Apr 8, 2020

vixentael Apr 8, 2020

ilammy Apr 8, 2020

vixentael Apr 8, 2020

vixentael Apr 8, 2020

Update JsThemis tests #620

Update JsThemis tests #620

Conversation

ilammy commented Apr 8, 2020 • edited Loading

Updated tests

API changes: Secure Cell

API changes: Secure Message

API changes: Secure Session

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilammy commented Apr 8, 2020 •

edited

Loading