FlowCrypt / flowcrypt-ios

FlowCrypt iOS App
https://flowcrypt.com
Other
33 stars 10 forks source link

refactor Core response from string to string + byte array #508

Closed tomholub closed 3 years ago

tomholub commented 3 years ago

after https://github.com/FlowCrypt/flowcrypt-ios/issues/507

currently, when we call a method in Core, for example here: https://github.com/FlowCrypt/flowcrypt-ios/blob/6367703c35bfe3df860573e8b2b8b0393b64a545/FlowCrypt/Core/Core.swift#L47

    func version() throws -> CoreRes.Version {
        let r = try call("version", jsonDict: nil, data: nil)
        return try r.json.decodeJson(as: CoreRes.Version.self)
    }

it will call into javascript eg here:

  public version = async (): Promise<Buffers> => {
    return fmtRes({ app_version: VERSION });
  }

The fmtRes method formats a resulting JSON + optional data into a single string. After we dropped Android support and HTTP server support, there is no point in doing this. We can instead return an object like {json:string, data: Uint8Array} which Swift's JavaScriptCore can load directly instead of parsing it.

Here's the fmtRes implementation:

export const fmtRes = (response: {}, data?: Buf | Uint8Array): Buffers => {
  const buffers: Buffers = [];
  buffers.push(Buf.fromUtfStr(JSON.stringify(response)));
  buffers.push(Buf.fromUtfStr('\n'));
  if (data) {
    buffers.push(data);
  }
  return buffers;
}

I suppose it should change to something like this:

type EndpointRes = {json: strring, data: Buf | Uint8Array};

export const fmtRes = (response: {}, data?: Buf | Uint8Array): EndpointRes => {
  return {
    json: JSON.stringify(response)),
    data: data || new Uint8Array(0)
  }
}

Therefore the endpoints almost don't need to change:

  public version = async (): Promise<EndpointRes> => {
    return fmtRes({ app_version: VERSION });
  }

Then similarly, we should no longer try to parse it on Swift side. Currently done here: https://github.com/FlowCrypt/flowcrypt-ios/blob/6367703c35bfe3df860573e8b2b8b0393b64a545/FlowCrypt/Core/Core.swift#L198

Something like this:

    private func call(_ endpoint: String, jsonData: Data, data: Data) throws -> RawRes {
        try blockUntilReadyOrThrow()
        cb_last_value = nil
        jsEndpointListener!.call(withArguments: [endpoint, String(data: jsonData, encoding: .utf8)!, data.base64EncodedString(), cb_catcher!])
-       let b64response = cb_last_value![0] as! String
-       let rawResponse = Data(base64Encoded: b64response)!
-       let separatorIndex = rawResponse.firstIndex(of: 10)!
-       let resJsonData = Data(rawResponse[...(separatorIndex - 1)])
+       // now somehow access the object from Swift 
+       let resObject = cb_last_value![0] as! Dict<any> // pseudo code
+       let resJsonData = Data(resObject["json"] as! String) // pseudo code
+       let resBytesData = Data(resObject["data"] as! [Uint8]) // pseudo code
        let error = try? resJsonData.decodeJson(as: CoreRes.Error.self)
        if let error = error {
            let errMsg = "------ js err -------\nCore \(endpoint):\n\(error.error.message)\n\(error.error.stack ?? "no stack")\n------- end js err -----"
            logger.logError(errMsg)

            throw CoreError.init(coreError: error) ?? CoreError.exception(errMsg)
        }
-        return RawRes(json: resJsonData, data: Data(rawResponse[(separatorIndex + 1)...]))
+        return RawRes(json: resJsonData, data: Data(resBytesData)
    }
tomholub commented 3 years ago

@IvanPizhenko please have a look after #507

I recommend to only do the TypeScript parts + adjust TypeScript tests, and make a Draft PR where the iOS app remains broken. Then we'll leave fixing Swift to a Swift developer, to finish the PR. Thanks!

IvanPizhenko commented 3 years ago

yes, because I don't have Mac, I think I can only do TS part here and leave Swift part for another developer.

IvanPizhenko commented 3 years ago

@tomholub I have finished TS part, please look at PR #544. Now it is turn of the Swift developers.

tomholub commented 3 years ago

Left to do in this issue - pick any unit test that uses the JavaScriptCore and fix the test, which should fix all others. The fix will be likely here https://github.com/FlowCrypt/flowcrypt-ios/blob/6367703c35bfe3df860573e8b2b8b0393b64a545/FlowCrypt/Core/Core.swift#L198 and look roughly like this:

    private func call(_ endpoint: String, jsonData: Data, data: Data) throws -> RawRes {
        try blockUntilReadyOrThrow()
        cb_last_value = nil
        jsEndpointListener!.call(withArguments: [endpoint, String(data: jsonData, encoding: .utf8)!, data.base64EncodedString(), cb_catcher!])
-       let b64response = cb_last_value![0] as! String
-       let rawResponse = Data(base64Encoded: b64response)!
-       let separatorIndex = rawResponse.firstIndex(of: 10)!
-       let resJsonData = Data(rawResponse[...(separatorIndex - 1)])
+       // now somehow access the object from Swift 
+       let resObject = cb_last_value![0] as! Dict<any> // pseudo code
+       let resJsonData = Data(resObject["json"] as! String) // pseudo code
+       let resBytesData = Data(resObject["data"] as! [Uint8]) // pseudo code
        let error = try? resJsonData.decodeJson(as: CoreRes.Error.self)
        if let error = error {
            let errMsg = "------ js err -------\nCore \(endpoint):\n\(error.error.message)\n\(error.error.stack ?? "no stack")\n------- end js err -----"
            logger.logError(errMsg)

            throw CoreError.init(coreError: error) ?? CoreError.exception(errMsg)
        }
-        return RawRes(json: resJsonData, data: Data(rawResponse[(separatorIndex + 1)...]))
+        return RawRes(json: resJsonData, data: Data(resBytesData))
    }

A suitable test for this is testEndToEnd in FlowCryptAppTests/Core/FlowCryptCoreTests.swift

tomholub commented 3 years ago

It should be an easy update but if it gives you a lot of trouble, it's possible that an update would be needed on TypeScript side to make it work. If TypeScript is not your area, and you suspect this, tag me to have a look.

ivan-ushakov commented 3 years ago

Looks like JavaScriptCore does not support UInt8Array because it has type NSDictionary in JSValue. I tried to get NSArray because JavaScript should map to it but without success. Here is mapping for JavaScript and JSValue types.

ivan-ushakov commented 3 years ago

With this change data property has value NSArray, but each element inside array is a NSNumber so I guess this is not what you want.

export type EndpointRes = {json: string, data: Array<number>};

export const fmtRes = (response: {}, data?: Buf | Uint8Array): EndpointRes => {
  return {
    json: JSON.stringify(response),
    data: Array.from(data || new Uint8Array(0))
  };
}
IvanPizhenko commented 3 years ago

@tomholub @ivan-ushakov I suggest to change data into hex or base64 string, it seems like there is no better option here. However, this will require more changes than just changing EndpointRes, since JS tests will be affected. If @tomholub agree, I will do this today later.

tomholub commented 3 years ago

@IvanPizhenko sure as a backup, but not so fast :-) I want to make sure.

@ivan-ushakov could you try casting it straight from any to [Uint8] in Swift?

Because I was able to do it in CoreHost - have a look at decryptAesCfbNoPadding. This method is called from JavaScript. The method is defined in Swift as func decryptAesCfbNoPadding(_ ct: [UInt8], _ key: [UInt8], _ iv: [UInt8]) -> [UInt8] so it both receives [Uint8] from JS and then returns it back to JS. On JavaScript side, these values "show up" as Uint8Array, so it seems the JS-Swift boundary should be able to handle the conversion.

tomholub commented 3 years ago

Even better example (Uint8 array from JS to swift) is produceHashedIteratedS2k - the arguments, in particular prefix which is more obvious:

    func produceHashedIteratedS2k(_ algo: String, _ prefix: [UInt8], _ salt: [UInt8], _ passphrase: [UInt8], _ count: Int) -> [UInt8] {
        let dataGroupSize = 750 // performance optimisation
        let data = salt + passphrase
        let dataRepeatCount = Int((Float(count - prefix.count) / Float(max(data.count, 1))).rounded(.up))
        var dataGroup = Data()
        for _ in 0 ..< dataGroupSize { // takes 11 ms
            dataGroup += data
        }
        var isp = prefix + dataGroup
        for _ in 0 ... dataRepeatCount / dataGroupSize { // takes 75 ms, just adding data (16mb)
            isp += dataGroup
        }
        let subArr = isp[0 ..< prefix.count + count] // free
        let hashable = Data(subArr) // takes 18 ms just recreating data, could be shaved off by passing ArraySlice to hash
        return try! hashDigest(name: algo, data: hashable) // takes 30 ms for sha256 16mb
    }

And here excerpts from javascript:

  async function round(prefix, s2k) {
    const algorithm = _enums2.default.write(_enums2.default.hash, s2k.algorithm);

    switch (s2k.type) {
      // ...
      case 'iterated':
        {
          const count = s2k.get_count();
          return Uint8Array.from(coreHost.produceHashedIteratedS2k(s2k.algorithm, prefix, s2k.salt, passphrase, count));
        }
      // ...
    }
  }

The prefix that is used there (and sent straight to Swift as parameter call) is Uint8Array which can be visible just below:

  const arr = [];
  let rlength = 0;
  const prefix = new Uint8Array(numBytes); //    <--- here

  for (let i = 0; i < numBytes; i++) {
    prefix[i] = 0;
  }

  let i = 0;
  while (rlength < numBytes) {
    const result = await round(prefix.subarray(0, i), this);   //    <--- here
    arr.push(result);
    rlength += result.length;
    i++;
  }

If it works there, I suppose there should be a way to work it on this other context - both are JS/Swift boundaries, and both go from JS to Swift.

tomholub commented 3 years ago

Looks like JavaScriptCore does not support UInt8Array because it has type NSDictionary in JSValue. I tried to get NSArray because JavaScript should map to it but without success. Here is mapping for JavaScript and JSValue types.

For clarification - the return type as it currently is implemented is {json:string, data: Uint8Array} which would probably translate to NSDictionary<Any>? Is it not possible to the access the actual values of the NSDictionary and cast one of them to string and the other to [Uint8]?

ivan-ushakov commented 3 years ago

I tried this one:

export type EndpointRes = {json: string, data: Uint8Array};

export const fmtRes = (response: {}, data?: Buf | Uint8Array): EndpointRes => {
  return {
    json: JSON.stringify(response),
    data: new Uint8Array(data || new Uint8Array(0))
  };
}

but data property is still NSDictionary and it is not possible to cast to [UInt8] in Swift.

ivan-ushakov commented 3 years ago

This could be a limitation of JSValue. So when you invoke Swift methods from JS you could have Uint8Array to [UInt8] mapping.

tomholub commented 3 years ago

Hm. Maybe if we define the callback method signature more clearly in Swift, then it will be possible.

Eg here:

let cb_last_value_filler: @convention(block) ([NSObject]) -> Void = { values in self.cb_last_value = values }

Instead of [NSObject] could you try [[Uint8]] ?

And in FlowCrypt/Resources/flowcrypt-ios-prod.js.txt file, change this:

global.handleRequestFromHost = (endpointName, request, data, cb) => {
    try {
        const handler = endpoints[endpointName];
        if (!handler) {
            cb(formatBareOutput((0, format_output_1.fmtErr)(new Error(`Unknown endpoint: ${endpointName}`))));
        }
        else {
            handler(JSON.parse(request), [buf_1.Buf.fromBase64Str(data)])
                .then(res => cb(formatBareOutput(buf_1.Buf.concat(res))))
                .catch(err => cb(formatBareOutput((0, format_output_1.fmtErr)(err))));
        }
    }
    catch (err) {
        cb(formatBareOutput((0, format_output_1.fmtErr)(err)));
    }
};

To this:

global.handleRequestFromHost = (endpointName, request, data, cb) => {
    cb(new Uint8Array(0));
};

Try to find a more minimal example to debug on - like this above - see if you can make it actually work with the [Uint8]. Since at least sometimes, it can.

ivan-ushakov commented 3 years ago

I changed result processing to avoid using JSValue. Now result goes directly to the Swift method. Please check my commit. Some of tests are still failing.