Skip to content

harden: managed executor, resource cleanup, Zip Slip guard, TS types, CI updates#356

Open
plrthink wants to merge 45 commits into
masterfrom
harden-native-modules
Open

harden: managed executor, resource cleanup, Zip Slip guard, TS types, CI updates#356
plrthink wants to merge 45 commits into
masterfrom
harden-native-modules

Conversation

@plrthink

@plrthink plrthink commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Addresses the critical + high severity findings from a recent native-module review:

  • Android: Replace raw "Thread" usage with a managed "ExecutorService", close all zip/stream handles with try-with-resources, and add a Zip Slip guard to "unzip".
  • Android: Extract Zip Slip validation into a testable "ZipSecurity" utility with JUnit regression tests.
  • Android: Emit progress events on the main thread and flush output streams in "StreamUtil.copy".
  • TypeScript: Fix invalid default-parameter syntax in "index.d.ts" and move the import to the top level.
  • iOS: Add nullability annotations to "RNZipArchive.h".
  • Build: Align Android "minSdkVersion" fallback with documented API 23 minimum.
  • CI: Update publish workflow to modern action versions, Node 20, and a tagged npm-publish-action release; fix E2E workflow job names.

Verification

  • "npm test" passes (27/27)
  • "npm run lint" passes
  • Android library compiles successfully (":react-native-zip-archive:compileReleaseJavaWithJavac")
  • Android unit tests pass (":react-native-zip-archive:testReleaseUnitTest")
  • TypeScript strict typecheck against "index.d.ts" passes
  • iOS header + .mm syntax check passes

Notes

  • The iOS "getUncompressedSize" contract is preserved: it still resolves "-1" on error to maintain backward compatibility.
  • "pod lib lint" still fails in this environment due to React header resolution, which is a pre-existing lint-environment issue unrelated to these changes.

Open in Devin Review

plrthink added 2 commits June 22, 2026 18:43
… CI updates

- Replace raw Thread usage with ExecutorService in Android module
- Close ZipFile/stream handles with try-with-resources on all paths
- Extract Zip Slip validation into ZipSecurity utility and add JUnit tests
- Emit progress events on the main thread
- Fix TypeScript declarations (optional params, import placement)
- Add iOS nullability annotations
- Align Android minSdkVersion fallback with docs (23)
- Update CI workflows to modern action versions and Node 20

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 3 potential issues.

View 1 additional finding in Devin Review.

Open in Devin Review

Comment thread android/src/main/java/com/rnziparchive/RNZipArchiveModule.java
Comment thread android/src/main/java/com/rnziparchive/RNZipArchiveModule.java
Comment thread android/src/main/java/com/rnziparchive/RNZipArchiveModule.java
plrthink added 27 commits June 22, 2026 19:02
…ndroid

- Re-add Expo matrix entries to e2e.yml for iOS and Android
- Rename e2e jobs to E2E iOS/Android (rn|expo) to match branch protection
- Add Android build workflow for RN and Expo
- Update iOS build workflow to build both RN and Expo
- Disambiguate artifact names by app in matrix jobs
Expo Modules Core uses @MainActor/Sendable which require Swift 5.5+
and fail under Xcode 16 strict concurrency when compiled as Swift 5.0.
Apply SWIFT_VERSION=5.5 and SWIFT_STRICT_CONCURRENCY=minimal to the
iOS build commands so both RN and Expo simulator builds pass.
SWIFT_VERSION=5.5 was rejected/ignored by Xcode 16, leaving ExpoModulesCore
in Swift 5 mode where @mainactor is unknown. Use 6.0 (matching the podspec)
along with SWIFT_STRICT_CONCURRENCY=minimal to keep RN builds relaxed.
Swift 5 mode on the Xcode 16 / Swift 6 compiler supports @mainactor
but treats strict concurrency diagnostics as warnings, avoiding the
hard errors seen in react-native-screens and expo-modules-core under
Swift 6 complete concurrency checking.
macos-latest uses Xcode 16.4 / Swift 6, which enforces strict concurrency
as errors for Expo's dependencies. macos-14 defaults to Xcode 15.4 with
Swift 5.10, avoiding those hard errors while keeping @mainactor support
via SWIFT_VERSION=5.5.
- Keep iOS jobs on macos-latest (Xcode 16 / Swift 6)
- Update react-native-screens 4.25.1 -> 4.25.2 in playground-expo
With react-native-screens bumped to 4.25.2, retry Swift 6.0 language
mode and SWIFT_STRICT_CONCURRENCY=minimal to see if the RNScreens
strict-concurrency errors are resolved.
react-native-screens 4.25.2 still fails under Swift 6 on macos-latest.
Use macos-14 with Xcode 15.4 / Swift 5.10 as the pragmatic workaround
until upstream dependencies are Swift-6 clean.
React Native 0.83.9 requires Xcode >= 16.1, so we must stay on
macos-latest. Use a Podfile post-install hook to pin:
- ExpoModulesCore -> Swift 6.0 (needs @mainactor support)
- RNScreens -> Swift 5.0 (still has Swift 6 strict-concurrency issues)

Remove the global SWIFT_VERSION/SWIFT_STRICT_CONCURRENCY xcodebuild
overrides so the per-target settings take effect.
The per-target Swift version fix belongs on Xcode 16 / macos-latest,
not macos-14. Restore the runner.
@plrthink plrthink force-pushed the harden-native-modules branch from c37c7ab to 5446876 Compare June 23, 2026 17:17
@plrthink plrthink force-pushed the harden-native-modules branch from 6e0862d to 9ce1c80 Compare June 23, 2026 22:30
plrthink added 5 commits June 30, 2026 08:54
The GitHub Actions Android emulator (API 29, software GPU) sometimes shows
a system-level "System UI isn't responding" dialog shortly after launch.
The Expo dev client is heavy enough to trigger it; the RN Android E2E does
not. The dialog is unrelated to react-native-zip-archive logic.

A one-shot Maestro conditional can miss the dialog because it appears after
the check. Replace it with an Android-only polling loop that taps "Wait"
while the playground screen is not yet visible.

Also give the emulator 4 cores and a 1024 MB heap to reduce UI strain.
cursoragent and others added 4 commits June 30, 2026 03:25
The E2E Android (expo) job failed because a system-level "System UI
isn't responding" dialog blocked Maestro from seeing the home screen.
The one-shot dismiss handler never fired for two reasons:

1. The YAML used a curly apostrophe (U+2019) but Android renders a
   straight apostrophe (U+0027), so the visibility check was skipped.
2. The dialog often appears mid-wait, after the single pre-check runs.

Replace the one-shot handler with a repeat-while loop that taps Wait
whenever the ANR dialog is visible, and re-add adb settings to suppress
ANR prompts on the CI emulator.

Co-authored-by: Perry <plrthink@gmail.com>
fix(e2e): dismiss Android ANR dialog blocking Expo E2E on CI
Replace non-standard adb settings keys (show_anr_messages,
anr_dialogs_disabled, anr_show_background) that may be no-ops on the
API 29 emulator with Settings.Global.HIDE_ERROR_DIALOGS, the AOSP key
used by CTS ANR tests to suppress system error dialogs.

Addresses Devin review on PR #358.

Co-authored-by: Perry <plrthink@gmail.com>
fix(e2e): use documented hide_error_dialogs ADB setting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants