release: freeRASP 4.4.0 #139
Conversation
martinzigrai
left a comment
martinzigrai
left a comment
There was a problem hiding this comment.
Nice work! 🚀 The logic seems solid. I just found a few spots that need a bit of polish. Left some comments below. 👇
android/build.gradle
Outdated
| implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" | ||
| implementation "org.jetbrains.kotlinx:kotlinx-serialization-json:1.4.1" | ||
| implementation "com.aheaditec.talsec.security:TalsecSecurity-Community-ReactNative:17.0.1" | ||
| implementation "com.aheaditec.talsec.security:TalsecSecurity-Community-ReactNative:18.0.1" |
There was a problem hiding this comment.
We already have 18.0.2, could you please use that version?
There was a problem hiding this comment.
Yes, it was not ready then
| } | ||
| } | ||
|
|
||
| private fun flushCache(registeredListener: WrapperExecutionStateListener) { |
There was a problem hiding this comment.
Just a suggestion 👀: Invoking the listener inside the synchronized block can lead to deadlocks. It might be safer to copy the events to a local list, clear the cache, and then dispatch them outside the lock.
There was a problem hiding this comment.
Can you elaborate a bit more? This depends on the implementation of the listener, but now the listener implementation does not interact with cache. Synchronized block ensures we don't leave any threats in cache (in rare events), which copying to local list does not ensure
There was a problem hiding this comment.
I understand the concern about missing threats. However, the standard pattern avoids that by clearing the cache before releasing the lock. The flow would be:
-
Lock -> Copy items to a temp list -> Clear the original cache -> Unlock.
-
Iterate over the temp list and invoke the listener.
| } | ||
| } | ||
|
|
||
| private fun flushCache(registeredListener: WrapperThreatListener) { |
There was a problem hiding this comment.
Same as in ExecutionStateDispatcher.
android/src/main/java/com/freeraspreactnative/FreeraspReactNativeModule.kt
Show resolved
Hide resolved
| }; | ||
| eventsListener = eventEmitter.addListener(channel, listener); | ||
| eventsListener = eventEmitter.addListener(executionStateChannel, listener); | ||
| isInitializing = false; |
There was a problem hiding this comment.
If await removeRaspExecutionStateEventListener() or data fetching throws an error, isInitializing will remain true forever, blocking subsequent calls. I suggest wrapping the logic in a try { ... } finally { isInitializing = false; } block to ensure the lock is always released.
There was a problem hiding this comment.
Only case when these methods could fail is when the native method is not found. This case does not happen unless there is an issue with a bridge. Then I think it is better if we crash the app (and isInitializing is then irrelevant). try block would add unnecessary complexity in this case, in my opinion
There was a problem hiding this comment.
Okay, let's leave it as is.
| }; | ||
| eventsListener = eventEmitter.addListener(channel, listener); | ||
| eventsListener = eventEmitter.addListener(threatChannel, listener); | ||
| isInitializing = false; |
There was a problem hiding this comment.
Same as in raspExecutionState.ts
2 participants
What's new
Describe the PR shortly
PR checklist
In-app checks:
README.mdis updated (if applicable)expoplugin is updated (if applicable)exampleapp is workingPre-release checks:
CHANGELOG.mdis updatedpackage.jsonversion is increasedreleaselabel is applied on PRTo be checked by maintainers:
sdkVersionproperty in logs is correctsdkPlatformproperty in logs is correctResolved issues
list of issues resolved by this PR
closes #136