Skip to content

feat: add @onekeyfe/react-native-sni-connect module (3.0.66)#68

Open
huhuanming wants to merge 2 commits into
mainfrom
feat/add-react-native-sni-connect
Open

feat: add @onekeyfe/react-native-sni-connect module (3.0.66)#68
huhuanming wants to merge 2 commits into
mainfrom
feat/add-react-native-sni-connect

Conversation

@huhuanming

@huhuanming huhuanming commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

What

Sync the react-native-sni-connect library from OneKeyHQ/react-native-sni-connect into native-modules/ as a workspace module.

Details

  • Version aligned to the shared native-modules version 3.0.66.
  • Adapted package.json to app-modules conventions (workspace devDependencies, RN 0.83.0 patch ref, no standalone workspaces/packageManager); dropped standalone repo scaffolding (example/, .yarn/, yarn.lock, .github/, lefthook, eslintrc, dotfiles).
  • EMASCurl pod bumped 1.4.1 → 1.5.5 in SniConnect.podspec — this baked in what was previously an app-monorepo patch (@onekeyfe+react-native-sni-connect+1.1.0.patch).
  • yarn install registers the workspace; yarn typecheck passes.

Related

  • app-monorepo: bumps the dependency to 3.0.66 and removes the now-obsolete patch.

Open in Devin Review

Sync react-native-sni-connect from OneKeyHQ/react-native-sni-connect into
native-modules, aligned to the shared 3.0.66 version. Bumps the EMASCurl
pod dependency to 1.5.5 (previously applied as an app-monorepo patch).
@socket-security

socket-security Bot commented Jun 12, 2026

Copy link
Copy Markdown

All alerts resolved. Learn more about Socket for GitHub.

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

View full report

@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 6 potential issues.

Open in Devin Review

Comment on lines +82 to +90
NSDictionary *configDict = @{
@"ip": config.ip(),
@"hostname": config.hostname(),
@"method": config.method(),
@"path": config.path(),
@"headers": config.headers(),
@"body": config.body() ?: [NSNull null],
@"timeout": @(config.timeout())
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 iOS TurboModule path drops requestId from request config, breaking cancellation

In the TurboModule (new architecture) code path, the requestId field from the codegen struct is not included when converting to NSDictionary. The dictionary at SniConnect.mm:82-90 maps ip, hostname, method, path, headers, body, and timeout but omits requestId. The Swift parser at SniConnect.swift:140 reads dictionary["requestId"] as? String which will always be nil, so cancelRequest() will never be able to find and cancel requests on iOS new architecture.

Suggested change
NSDictionary *configDict = @{
@"ip": config.ip(),
@"hostname": config.hostname(),
@"method": config.method(),
@"path": config.path(),
@"headers": config.headers(),
@"body": config.body() ?: [NSNull null],
@"timeout": @(config.timeout())
};
NSDictionary *configDict = @{
@"ip": config.ip(),
@"hostname": config.hostname(),
@"method": config.method(),
@"path": config.path(),
@"headers": config.headers(),
@"body": config.body() ?: [NSNull null],
@"timeout": @(config.timeout()),
@"requestId": config.requestId() ?: [NSNull null]
};
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +257 to +263
for key in expiredKeys {
remove(key)
// Also remove from hostnameToIP if this was the active mapping
if let entry = storage[key], hostnameToIP[entry.hostname] == entry.ip {
hostnameToIP.removeValue(forKey: entry.hostname)
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 cleanExpired() never removes stale hostnameToIP entries because entry is read after deletion

In cleanExpired(), remove(key) deletes the entry from storage (see SniConnectClient.swift:241-242), then immediately after, the code tries to read storage[key] which will always be nil. The hostnameToIP cleanup at line 260-262 is dead code — stale hostname-to-IP mappings will accumulate and never be cleaned up during expiration sweeps.

Suggested change
for key in expiredKeys {
remove(key)
// Also remove from hostnameToIP if this was the active mapping
if let entry = storage[key], hostnameToIP[entry.hostname] == entry.ip {
hostnameToIP.removeValue(forKey: entry.hostname)
}
}
for key in expiredKeys {
// Read entry BEFORE removing it from storage
if let entry = storage[key], hostnameToIP[entry.hostname] == entry.ip {
hostnameToIP.removeValue(forKey: entry.hostname)
}
remove(key)
}
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

unregisterTask(requestId: config.requestId)
}

DNSResolver.setIP(config.ip, for: config.hostname)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 iOS shared DNS resolver races when concurrent requests use same hostname with different IPs

The iOS DNS resolver uses a shared hostnameToIP dictionary that maps each hostname to a single IP (SniConnectClient.swift:195). When performRequest calls DNSResolver.setIP(config.ip, for: config.hostname) at line 353, it overwrites the mapping for that hostname. If two concurrent requests target the same hostname but different IPs (an explicit design goal per the comment at line 182: "accurate speed testing"), the second setIP call overwrites the first's mapping before EMASCurl resolves the domain for the first request. This causes the first request to connect to the wrong IP. The Android implementation handles this correctly by giving each (hostname, IP) pair its own OkHttpClient with an isolated Dns resolver.

Prompt for agents
The iOS DNS resolver (DNSResolver/DNSCache in SniConnectClient.swift) uses a shared hostnameToIP dictionary that only stores one IP per hostname. This fundamentally cannot support concurrent requests to the same hostname with different IPs — the second setIP call overwrites the first's mapping.

The Android implementation avoids this by creating a separate OkHttpClient per (hostname, IP) pair, each with its own Dns resolver pinned to the specific IP.

To fix this on iOS, the DNS resolver needs to support returning different IPs for the same hostname based on the request context. Possible approaches:
1. Use request-specific identifiers (e.g., encode the IP in the URL or use a request header) so the DNS resolver can determine which IP to return for each specific request.
2. Replace the hostname in the URL with the IP directly and use the Host header plus SNI configuration to maintain correct TLS validation, bypassing DNS resolution entirely.
3. Create separate URLSession instances per (hostname, IP) pair, each with its own EMASCurl configuration and DNS resolver.

The same issue applies to EMASCurlProtocol.setConnectTimeoutInterval at line 384, which is also global shared state that races across concurrent requests.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

}

// Configure connection timeout for EMASCurl
EMASCurlProtocol.setConnectTimeoutInterval(connectTimeoutSeconds)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Global setConnectTimeoutInterval races across concurrent requests with different timeouts

EMASCurlProtocol.setConnectTimeoutInterval(connectTimeoutSeconds) at line 384 sets a process-global connect timeout value immediately before each request. When concurrent requests specify different connect timeout values (via connectTimeout config or the computed effectiveConnectTimeout), they overwrite each other's values. A request with a 2-second connect timeout could end up using a 10-second timeout (or vice versa) set by another concurrent request.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +152 to +175
private static let urlSession: URLSession = {
let configuration = URLSessionConfiguration.default
configuration.requestCachePolicy = .reloadIgnoringLocalCacheData
configuration.urlCache = nil
configuration.httpCookieStorage = nil
configuration.httpShouldSetCookies = false
configuration.shouldUseExtendedBackgroundIdleMode = false

let curlConfig = EMASCurlConfiguration.default()
curlConfig.httpVersion = .HTTP2
curlConfig.connectTimeoutInterval = 2.5
curlConfig.enableBuiltInGzip = true
curlConfig.enableBuiltInRedirection = true
curlConfig.cacheEnabled = false

// Enable full certificate validation for security
// The certificate will be validated against the SNI hostname, not the IP
curlConfig.certificateValidationEnabled = true
curlConfig.domainNameVerificationEnabled = true
curlConfig.dnsResolver = DNSResolver.self

EMASCurlProtocol.install(into: configuration, with: curlConfig)
return URLSession(configuration: configuration)
}()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚩 iOS and Android DNS/client caching architectures are fundamentally different

The Android implementation creates a separate OkHttpClient per (hostname, IP) pair via clientCache (SniConnectModule.kt:53,201-221), with each client having its own pinned Dns resolver. This correctly isolates concurrent requests to the same hostname with different IPs.

The iOS implementation uses a single shared URLSession (SniConnectClient.swift:152-175) with a process-global DNS resolver that can only map one IP per hostname at a time (SniConnectClient.swift:208-212). This architectural difference means the two platforms have different concurrency guarantees — Android correctly supports parallel speed-testing of multiple IPs for the same hostname, while iOS does not.

This asymmetry should be documented or resolved to ensure consistent behavior across platforms.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +66 to +73
let task = Task { () -> SniConnectClient.Response in
return try await client.performRequest(config: config)
}

// Register task synchronously if requestId is provided
if let requestId = config.requestId {
client.registerTask(task, for: requestId)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🚩 iOS handleRequest has a subtle task registration race window

In SniConnect.swift:66-73, the Task is created (and may start executing immediately on a concurrent executor) before registerTask is called at line 72. If the task completes or fails extremely quickly (before registerTask runs), then unregisterTask in performRequest's defer block (SniConnectClient.swift:349-351) could run before registration, and the task would remain in activeTasks forever. In practice this is unlikely due to the network I/O involved, but it's a theoretical resource leak for error cases (e.g., invalid URL that throws synchronously).

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

…ogging via reflection

Security (boundary validation, both platforms):
- Validate ip as a public IP literal (reject loopback/private/link-local/
  metadata/CGNAT/multicast/reserved); reject hostname-as-ip (no DNS).
- Force https://hostname:443; reject absolute/scheme/authority paths
  (no scheme downgrade or host/port override).
- Method allowlist, strict hostname, reject CR/LF in header names/values.

Concurrency / correctness:
- iOS: per-request connect timeout via setConnectTimeoutIntervalFor(...)
  instead of the process-global setter; simplify DNS cache and fix the
  cleanExpired read-after-remove bug; fix block-based NotificationCenter
  observer leak (retain + remove token).
- iOS new-arch: forward requestId and null-guard the codegen->dict bridge.
- Android: bounded LRU OkHttpClient cache with shared dispatcher/pool;
  AtomicBoolean guard against double-settling the promise; validate timeout.

Logging (Q2): route native logs to OneKeyLog via reflection (SniConnectLog /
SniConnectLogger), mirroring BTLogger/SBLLogger, so this TurboModule does not
hard-link the nitro-based native-logger; remove the bespoke JS log event
channel (RCTEventEmitter / subscribeToLogs). Falls back to os/android log.

Cleanup: drop dev scripts/ (sudo pfctl/tcpdump + hardcoded token), fix
repository/podspec URLs, rewrite README to the real API, narrow Android
packagingOptions, trim dead code.

Example: add the module to the example app + Podfile (aliyun spec source,
EMASCurl modular_headers). Verified: Android compileDebugKotlin and iOS
xcodebuild of the SniConnect target both succeed; tsc + jest pass.
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.

1 participant