Improve error reporting in JsThemis

[ilammy]

The users are confused with non-descriptive errors like Secure Message failed decrypting, let's tell them more about what's wrong:

• [Attempt at producing] unified error messages
• Early checks for argument types (mismatch could crash Node)
• Early checks for argument domains (generally we do not accept empty inputs)

And we get this:

-Secure Message failed decrypting
+Secure Message failed to decrypt: invalid parameter: private key is empty

[ilammy]

You can consider this WIP, but the new GitHub feature does not seem to work for us for some reason.

I plan to add more error checks to other places as well (Secure Cell and Session also do not like empty inputs, as I recall).

The error messages are changed from gerund (failed decrypting) to infinitive (failed to decrypt) so that they are more closely related to method names. For example, the method is called decrypt and you're more likely to associate this exact word in the error message with the code that you wrote.

[Lagovas]

what about to move these strings with error description to error.h and store them as constants accessible from client's code? it helps to:

• use 1 constant name and value for 1+ usage as a description
• change text in one place for all usages
• compare error string from client's code and to allow distinguish one error from other (THEMIS_INVALID_PARAMETER was returned for public or private key)

[ilammy]

Mmm... I'm not really a fan of storing developer-visible human-readable strings in separate files far from their actual usage because as a developer I'd rather see "missing private and public key" in the code immediately where the error is reported rather than some cryptic constant THEMIS_SECURE_MESSAGE_ARGUMENT_COUNT_KEYS. (Error codes are a different story.)

If I have an error message then grepping for its substrings will lead directly to the source code rather than to the errors.hpp where I'd have to do another search for the intermediate constant usage.

I also don't imagine our users handling these errors based on human-readable strings. That's a bad practice. If anything, I'd export all those themis_status_t constants and added a proper exception type with some status property which then can be programmatically compared to error code constants.

[Lagovas]

I don't like to distinguish error types by string literals too but it's okay to compare some constant values if environment doesn't have any way to use explicit user's exception. as I understand now we return general Exception with some error message - https://github.com/cossacklabs/themis/pull/384/files#diff-55a946083bac69602385c43c0a35e717R83
How user can check in runtime that this error related to incorrect/empty private key and create custom message for user without pass error from our library as is? for example

try {
// use secure message with invalid private key
} catch (err) {
  if err.message == jsthemis.INVALID_PRIVATE_KEY {
     alert('you uploaded empty file for private key')
  } 
} 

instead

try {
// use secure message with invalid private key
} catch (err) {
  alert(err.message); // message == 'secure message: invalid parameter'
} 

?

[ilammy]

Uh... well... currently this is impossible to do consistently with any wrapper in any language. The core library (at best) reports THEMIS_INVALID_PARAMETER and then the user code has to check itself which parameter is invalid, why it is so, and maybe show an appropriate message to the end user in their preferred language.

If machine-readable errors are important I'd suggest to plan a more broad improvement starting from the core: introduce new error codes like THEMIS_INVALID_PRIVATE_KEY, start using them in the core code instead of generic THEMIS_INVALID_PARAMETER, and then update the error reporting across language wrappers. That way we'd be consistent with error reporting.

There's a number of an open questions there. For example, do we need more detailed codes for various reasons why the private key can be invalid? It can be empty, malformed, valid but of unexpected type, does not match the public key, etc.

[ilammy]

As for this PR, I'd rather not export any new API unless we're sure that it's the right thing and going to support it.

Currently the users of JsThemis do not have an official blessed way to distinguish between Themis errors. However, as developers they may read the documentation, read the error messages they observe during testing, and do something about that on their application level. This PR does not change that fundamentally, but it improves the error messages which makes it a bit easier for developers to guess what's wrong with the way they use Themis.

I think that comparing human-readable error messages is not a good API and we should not start supporting it since 0.11. I'd suggest to keep the current status quo, and maybe introduce a better support for error handling in 0.12+

[vixentael]

I support @Lagovas – it's important to improve text messages, but it would be nice to allow users to distinguish errors by checking error code / error name as well.

From what I see, it's typical for NaN to throw errors as strings only:
https://github.com/nodejs/nan/blob/master/test/cpp/error.cpp

However can we propagate error number as well?

Maybe smth similar to

  static NAN_INLINE(void NanThrowError(
      const char *msg,
      const int errorNumber)) {
    NanThrowError(NanError(msg, errorNumber));
  }

(inspired by http://res.wisedu.com/FS/mockserver/node_modules/nan/nan.h and https://git.visteon.com/external/nan/commit/c1ab008e7d9151a133ad7cd84a8443409e471dcb#7de2fb2c0f5228eb2fbf733d63d9d3203ec08371_350_357)

[ilammy]

Yeah, it seems that we can use this approach. Then the exception will have an additional property code which will contain Themis error code. We should also export error code constants from our module. This should make it possible to handle errors like this:

try {
  // use Themis
}
catch (e) {
  if (e.code == jsthemis.INVALID_PARAMETER) {
    // handle error
  }
}

[vixentael]

I support @Lagovas – it's important to improve text messages, but it would be nice to allow users to distinguish errors by checking error code / error name as well.

From what I see, it's typical for NaN to throw errors as strings only:
https://github.com/nodejs/nan/blob/master/test/cpp/error.cpp

However can we propagate error number as well?

Maybe smth similar to

  static NAN_INLINE(void NanThrowError(
      const char *msg,
      const int errorNumber)) {
    NanThrowError(NanError(msg, errorNumber));
  }

(inspired by http://res.wisedu.com/FS/mockserver/node_modules/nan/nan.h and https://git.visteon.com/external/nan/commit/c1ab008e7d9151a133ad7cd84a8443409e471dcb#7de2fb2c0f5228eb2fbf733d63d9d3203ec08371_350_357)

[vixentael]

I really love new wording and these checks!

Suggested few improvements here and there

[ilammy]

@vixentael, I've updated some error messages to include more instructions and information.

@Lagovas, @vixentael, error objects now contain code property with Themis status code. Status codes are exported as constants. They can be checked like in that comment.

Please review the list and naming:

themis/src/wrappers/themis/jsthemis/errors.cpp

Lines 30 to 43 in 27fe0dc

	void Errors::Init(v8::Handle<v8::Object> exports) {
	ExportStatusCode(exports, "SUCCESS", THEMIS_SUCCESS);
	ExportStatusCode(exports, "FAIL", THEMIS_FAIL);
	ExportStatusCode(exports, "INVALID_PARAMETER", THEMIS_INVALID_PARAMETER);
	ExportStatusCode(exports, "NO_MEMORY", THEMIS_NO_MEMORY);
	ExportStatusCode(exports, "BUFFER_TOO_SMALL", THEMIS_BUFFER_TOO_SMALL);
	ExportStatusCode(exports, "DATA_CORRUPT", THEMIS_DATA_CORRUPT);
	ExportStatusCode(exports, "INVALID_SIGNATURE", THEMIS_INVALID_SIGNATURE);
	ExportStatusCode(exports, "NOT_SUPPORTED", THEMIS_NOT_SUPPORTED);
	ExportStatusCode(exports, "SSESSION_KA_NOT_FINISHED", THEMIS_SSESSION_KA_NOT_FINISHED);
	ExportStatusCode(exports, "SSESSION_TRANSPORT_ERROR", THEMIS_SSESSION_TRANSPORT_ERROR);
	ExportStatusCode(exports, "SSESSION_GET_PUB_FOR_ID_CALLBACK_ERROR", THEMIS_SSESSION_GET_PUB_FOR_ID_CALLBACK_ERROR);
	ExportStatusCode(exports, "SCOMPARE_NOT_READY", THEMIS_SCOMPARE_NOT_READY);
	}

This is new API surface and we will have to live with these names later. I did not export all errors, only the ones which are likely to be of some use (e.g., THEMIS_SSESSION_SEND_OUTPUT_TO_PEER is not exported as it should not be visible to the users).

[vixentael]

Great job!
I really like how useful error messages became

[ilammy]

This should be it. I have added the same checks to Secure Session and Secure Comparator API where appropriate. Now (I hope) all JsThemis functions validate their argument count, types, and values.

It was a surprise, but key pair construction required checks as well. It was possible to construct a key pair with empty private or public keys, but the key pairs are useful only for Secure Messages where both components have to be non-empty. Thus constructing a key pair with an empty key is considered an error now.

[ilammy]

Amazing news. Recently released mocha 6.0.0 does not seem to be compatible with Node version on our CI. And it breaks our JavaScript tests. I guess I'll pin the version to 5.2.0 that we have been using until now.

[ilammy]

During a dependency downgrade I have noticed that JsThemis triggers a deprecation warning during native addon compilation. I've replaced the deprecated method call with its recommended replacement. That also focused my attention on Secure Session callback which was missing type and length checks. Now it has checks and does not cause Node crashes.

Please let it be a green build 🤞

[Lagovas]

This is new API surface and we will have to live with these names later. I did not export all errors, only the ones which are likely to be of some use (e.g., THEMIS_SSESSION_SEND_OUTPUT_TO_PEER is not exported as it should not be visible to the users).

As I remember THEMIS_SSESSION_SEND_OUTPUT_TO_PEER used as correct status for Secure Session handshake and tell a user that it needs to be sent to another peer

[ilammy]

@Lagovas, JsThemis makes Secure Session more convenient for the user:

themis/tests/jsthemis/test.js

Lines 68 to 76 in 6a6b540

	data = client_session.connectRequest();
	data = server_session.unwrap(data);
	data = client_session.unwrap(data);
	data = server_session.unwrap(data);
	assert.equal(client_session.isEstablished(), false);
	data = client_session.unwrap(data);
	assert.equal(data,undefined);
	assert.equal(server_session.isEstablished(), true);
	assert.equal(client_session.isEstablished(), true);

The user is expected to send the data to the peer if the data is not undefined (of if the session is not established). JsThemis checks for THEMIS_SSESSION_SEND_OUTPUT_TO_PEER internally, this code should not be necessary for JsThemis users.

[ilammy]

Yay! Thank you, @vixentael @Lagovas, for very valuable input on this PR.