redesigned the framework to make it less vulnerable to native exceptions. #951

radetsky · 2022-09-23T14:41:23Z

Made changes to the code that should catch any exceptions
Updated and moved to different directory previous react-native-example
Written the 'app of 70 tests of react-native-themis' and well tested it on debug, release, and TestFlight modes.
Fixed found issues after running the app

Checklist

Change is covered by automated tests
The [coding guidelines] are followed
Example projects and code samples are up-to-date (in case of API changes)

src/wrappers/themis/react-native-themis/ios/RCTThemis.m

...ers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java

Lagovas · 2022-09-23T15:51:29Z

...ers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java

+            if (privateKey == null || privateKey.size() == 0) {
+                secureMessage = new SecureMessage(pubKey);
+            } else {
+                bkey = dataDeserialize(privateKey);


do we need private key here? as I remember and what our docs says - https://docs.cossacklabs.com/themis/crypto-theory/cryptosystems/secure-message/#implementation-details
for verifying we need only public key. So, if it is legacy signature of method, we can leave it but ignore private key and don't validate.

plus we don't use it anywhere below.

Removed privateKey from verify method. Also removed a public key from the sign method.
Fixed the tests.

src/wrappers/themis/react-native-themis/src/index.js

Lagovas · 2022-09-23T15:55:35Z

docs/examples/react-native/react-native-example/yarn.lock

-  integrity sha512-q5M7Jq41FYw0BlfZDWdfo2c1/eljVp3QrCkDDevBgMwWo77HQvZ8vXJIkRsdaCmWj0CHjZUzOFW6FWx3hY9Y7w==
+react-native-themis@/Users/rad/Downloads/react-native-themis-0.14.10.tgz:
+  version "0.14.10"
+  resolved "/Users/rad/Downloads/react-native-themis-0.14.10.tgz#fc1db86d4b1a3c9a82ce32a8795e7057a4f33cf2"


/Users/rad/Downloads...

is it correct value?

Nope. I will update '[email protected]' after the package is published.

ilammy · 2022-09-24T01:14:37Z

194 files changed, 14685 insertions(+), 1554 deletions(-)

Is this representative of React Native changes? 😞

I really think boilerplate should be trimmed down. Looks like we're one step away from checking binaries into the repo, and then compiled examples as well because why not, users might be not experienced enough to compile them.

EDIT: Oh, those are examples moving around. I think this better be done on master instead, since that's where people look for examples.

^{And maybe don't compile them? 🥺}

ilammy · 2022-09-24T01:19:07Z

src/wrappers/themis/react-native-themis/ios/RCTThemis.m

 - (instancetype)init
 {
-  self = [super init];
-  cmprs = [[NSMutableDictionary alloc] init];
-  return self;
+    self = [super init];
+    cmprs = [[NSMutableDictionary alloc] init];
+    return self;
 }


Grrr, formatting changes mixed with substantial changes.

(puts on nazi maintainer hat)

Please, use the following action plan:

Do not reformat code on maintenance branch. Live with whatever formatting is there.

Configure auto-formatter for the code, share the configuration.

Reformat everything as a separate PR. That way resulting commit can be easily ignored.

Get CI to check auto-formatting in PRs to master from now on.

(takes off nazi maintainer hat)

Thank you for pointing that out to me. It's just that my eyesight has deteriorated and I became comfortable working with 4 spaces. It seems to me that no one else is working with this particular code now.
Next time I will definitely follow your comments.

my eyesight has deteriorated and I became comfortable working with 4 spaces.

@radetsky i'm very sorry for your eyesight. however, mixing formatting and real value PRs in one PR is a no go.

we don't mind against changing formatting, but we do mind against mixing formatting changes and real changes in the same PR, and changing formatting in the maintenance branch as a hotfix.

please follow @ilammy recommendations.

src/wrappers/themis/react-native-themis/ios/RCTThemis.m

ilammy · 2022-09-24T02:09:25Z

src/wrappers/themis/react-native-themis/src/index.js

+    if (!isBase64(symmetricKey64)) {
+        throw new Error("Parameter symmetricKey64 is not base64 encoded");
+    }
    const symmetricKey = Array.from(Buffer.from(symmetricKey64, 'base64'));


If you're wondering why this check is necessary, Buffer.from('💩💩💩', 'base64') – in typical Node.js & JS fashion – simply skips all characters that it can't recognize.

I believe if we announced that parameters should be base64 encoded, we have to validate them. Of course, I know about it behavior of Javascipt.

That comment was for other reviewers, who might not be aware why exactly you have to validate before calling the base64 conversion.

src/wrappers/themis/react-native-themis/src/index.js

Lagovas · 2022-09-27T14:08:48Z

...ers/themis/react-native-themis/android/src/main/java/com/reactnativethemis/ThemisModule.java

@@ -388,7 +390,6 @@ public void secureMessageSign(String message,

    @ReactMethod
    public void secureMessageVerify(ReadableArray message,
-                                    ReadableArray privateKey,


does it break backward compatibility? do we know that these methods are used anywhere or by someone?

A little. Verify method now has only two required parameters. I deleted the middle one - an optional private key. The sign method is compatible.
I think we should describe the changes in our documentation for react-native-themis.
The GitHub search tool returns only 8 code results and all results contain in our repositories.

does it break backward compatibility?

A little.

No such thing. Either applications are going to break after update, or they will not.

Aren't these methods semi-private? As in, they are an implementation detail of ReactNative Themis, and the users will not be calling them directly, but rather through JS code?

Though, I see JS API being changed as well, so it's a genuine breaking change.

Please, don't make breaking changes in patch releases, that's irresponsible.

In fact, better avoid making breaking changes at all until the next major version. Deprecations are okay though.

Or else, be honest, slap a prominent red banner in ReactNative documentation that the package is currently under development, is unstable, anything can be changed at any time, and it will be totally the user's fault if their application stops working after an update.

Fixed. I returned the middle param privateKey in the public API function. But mark it as unused and optional.

radetsky · 2022-09-27T17:09:33Z

194 files changed, 14685 insertions(+), 1554 deletions(-)
EDIT: Oh, those are examples moving around. I think this better be done on master instead since that's where people look for examples.
And maybe don't compile them? 🥺

It is only the source code and the assets for two mobile OS of two examples. I checked that twice. No compiled files.

src/wrappers/themis/react-native-themis/ios/RCTThemis.m

src/wrappers/themis/react-native-themis/src/index.d.ts

src/wrappers/themis/react-native-themis/src/index.js

ilammy · 2022-10-01T04:34:18Z

It is only the source code and the assets for two mobile OS of two examples. I checked that twice. No compiled files.

I'd consider all code-generated crap to be "compiled files", but that could be agreeable, since those files are needed for the examples to build and there doesn't seem to be a way to generate all this stuff in a reproducible manner.

So that's okay.

What's not okay is that all this is mixed with formatting changes and substantial changes. Which makes the PR borderline unreviewable. At the very least, all changes under docs/... should have been made into a separate PR and they have nothing to do with redesigning the framework to make it less vulnerable to native exceptions.

ilammy · 2022-10-01T04:48:40Z

src/wrappers/themis/react-native-themis/src/index.d.ts

@@ -9,11 +11,10 @@ export declare function secureCellTokenProtectEncrypt64(symmetricKey64: String,
 export declare function secureCellTokenProtectDecrypt64(symmetricKey64: String, encrypted64: String, token64: String, context?: String): Promise<string>;
 export declare function secureCellContextImprintEncrypt64(symmetricKey64: String, plaintext: String, context: String): Promise<string>;
 export declare function secureCellContextImprintDecrypt64(symmetricKey64: String, encrypted64: String, context: String): Promise<string>;
-export declare function secureMessageSign64(plaintext: String, privateKey64: String, publicKey64: String): Promise<string>;
-export declare function secureMessageVerify64(signed64: String, privateKey64: String, publicKey64: String): Promise<string>;
+export declare function secureMessageSign64(plaintext: String, privateKey64: String): Promise<string>;


publicKey64 parameter should be reinstated here to avoid breaking changes.

ilammy · 2022-10-01T04:51:47Z

src/wrappers/themis/react-native-themis/src/index.tsx

 export function secureMessageVerify64(
-  signed64: String,
-  privateKey64: String,
-  publicKey64: String): Promise<string> {
-
-  if (signed64 === "" || signed64 === undefined || signed64 === null) {
-    throw new Error("Parameter signed64 can not be empty");
-  }
-  if (publicKey64 === "" || publicKey64 === undefined || publicKey64 === null) {
-    throw new Error("Parameter publicKey64 can not be empty");
-  }
-
-  const signed = Array.from(Buffer.from(signed64, 'base64'));
-  const privateKey = privateKey64 !== null && privateKey64 !== "" ?
-    Array.from(Buffer.from(privateKey64, 'base64')) : null;
-  const publicKey = Array.from(Buffer.from(publicKey64, 'base64'));
-
-  return new Promise((resolve, reject) => {
-    Themis.secureMessageVerify(signed, privateKey, publicKey, (verified: any) => {
-      resolve(Buffer.from(new Uint8Array(verified)).toString())
-    }, (error: any) => {
-      reject(error)
+    signed64: String,
+    _privateKey64: String = "",
+    publicKey64: String): Promise<string> {


Do optional parameters actually work like that in TypeScript? You can make a parameter in the middle of the list optional? 🤔

To make it back compatible I had to return the unused privateKey64 parameter to the middle of the list of parameters. But typescript wants to use the required parameter in the function body. So, to omit the error I had to add undersign character. That indicates the parameter is maybe unused in the future.

Nah, I'm just surprised it allows to put default values in the middle of the list. Usually it's allowed only at the end, as that makes it easy to disambiguate which parameters should be defaulted. However, maybe the compiler is smart enough, since if there is only one optional parameter, it's unambiguous. But I'm not so sure about vanilla JS.

Could you please add a test to ensure that both 2-argument and 3-argument calls keep working?

ilammy · 2022-10-01T04:53:41Z

Good progress on eliminating exceptions, nevertheless!

radetsky · 2022-10-02T09:32:45Z

I'll try to remove docs/ changes to make it in a separate PR.

ilammy · 2022-10-02T21:08:56Z

I'll try to remove docs/ changes to make it in a separate PR.

Your latest commit broke GitHub 😂

You know, with 9-million-loc diff... Just shoot the horse already, put him out of his misery.

(Take useful changes, apply them on a separate branch, throw this one away.)

vixentael · 2022-10-20T11:46:07Z

as i understand, we are closing this PR in favor of #955

radetsky added 12 commits September 2, 2022 12:31

improve the code with the help of new try-catch

300e0fa

integrate isBase64 into code

8d2eb52

review Obj-C code to make all try-catch and error checks

a47fad8

fix an errors

9a27632

review Java code

241e0ac

review Obj-C code

9907cb5

review Javascript code

ecb3457

moved old example and add ThemisTest for RN app

0d9de20

new Android Studio support file

9c8377f

failed/passed fixes in order

90a0401

comparator can return nil without an error

c4eecd6

update example app

6ecbec4

radetsky requested review from vixentael, julepka and ilammy as code owners September 23, 2022 14:41

radetsky added 2 commits September 23, 2022 17:43

Merge branch 'release/0.14' into rad-dev

19496ce

changelog update

e3a0322

Lagovas reviewed Sep 23, 2022

View reviewed changes

ilammy requested changes Sep 24, 2022

View reviewed changes

radetsky added 4 commits September 26, 2022 13:49

check int value before casting to unsigned char

84218b2

fix the compare procedure; var naming due the suggestion;

cdd443f

remove privateKey from secureMessageVerify functions

480702a

remove publicKey from secureMessageSign64

99342d1

Lagovas reviewed Sep 27, 2022

View reviewed changes

radetsky added 2 commits September 29, 2022 09:48

rewrite Obj-C code to use NSError instead of try/catch

297c93d

fix the tests. The issue was in incorrect display of error

cc5c3bb

ilammy requested changes Sep 29, 2022

View reviewed changes

src/wrappers/themis/react-native-themis/ios/RCTThemis.m Outdated Show resolved Hide resolved

src/wrappers/themis/react-native-themis/ios/RCTThemis.m Outdated Show resolved Hide resolved

updated the code according to the comments

aa72af2

ilammy reviewed Oct 1, 2022

View reviewed changes

updated the code according to the comments

35ec1e9

move examples to the previous state

913fec3

vixentael mentioned this pull request Oct 9, 2022

Redesigned the framework to make it less vulnerable to native exceptions. #955

Merged

vixentael closed this Oct 20, 2022

radetsky deleted the rad-dev branch November 2, 2022 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redesigned the framework to make it less vulnerable to native exceptions. #951

redesigned the framework to make it less vulnerable to native exceptions. #951

radetsky commented Sep 23, 2022

Lagovas Sep 23, 2022

radetsky Sep 26, 2022

Lagovas Sep 23, 2022

radetsky Sep 26, 2022

ilammy commented Sep 24, 2022 •

edited

Loading

ilammy Sep 24, 2022

radetsky Sep 27, 2022

vixentael Oct 9, 2022

ilammy Sep 24, 2022

radetsky Sep 28, 2022

ilammy Sep 29, 2022

Lagovas Sep 27, 2022

radetsky Sep 27, 2022

ilammy Sep 29, 2022

radetsky Sep 29, 2022

radetsky commented Sep 27, 2022 •

edited

Loading

ilammy commented Oct 1, 2022

ilammy Oct 1, 2022

radetsky Oct 2, 2022

ilammy Oct 1, 2022

radetsky Oct 2, 2022

ilammy Oct 2, 2022

ilammy commented Oct 1, 2022

radetsky commented Oct 2, 2022

ilammy commented Oct 2, 2022

vixentael commented Oct 20, 2022

redesigned the framework to make it less vulnerable to native exceptions. #951

redesigned the framework to make it less vulnerable to native exceptions. #951

Conversation

radetsky commented Sep 23, 2022

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

ilammy commented Sep 24, 2022 • edited Loading

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

Choose a reason for hiding this comment

radetsky commented Sep 27, 2022 • edited Loading

ilammy commented Oct 1, 2022

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 Oct 1, 2022

radetsky commented Oct 2, 2022

ilammy commented Oct 2, 2022

vixentael commented Oct 20, 2022

ilammy commented Sep 24, 2022 •

edited

Loading

radetsky commented Sep 27, 2022 •

edited

Loading