Conversation
- [Android/iOS] AIA caIssuers chain completion for servers with incomplete chains (mattermost-mobile#9153, mattermost-mobile#6643, mattermost-mobile#9623, mattermost-mobile#6062) - [Android] Trust user-installed and MDM root CAs via AndroidCAStore (mattermost-mobile#9337) - [Android] Replace okhttp-tls HandshakeCertificates with javax.net.ssl APIs - [Android/iOS] Include cert subject, issuer and expiry in SSL error events (mattermost-mobile#8461) - [Android/iOS] Deduplicate CLIENT_ERROR events on retried cert failures (mattermost-mobile#7658) - [Android] Set explicit Content-Type on POST bodies to avoid WAF rejections - [iOS] Fix Session extension property leak on session invalidation
…ndler as is no longer required
📝 WalkthroughWalkthroughRefactors TLS/SSL certificate handling (AIA-based chain completion, TrustManager/KeyManager and client P12 support), rewrites retry interceptor control flow and moves tests to a JVM runner, modernizes Android/iOS example projects and build/tooling, updates TypeScript schemas and configs, replaces DocumentPicker in examples, and migrates to an ESLint flat config (deleted .eslintignore). ChangesTLS/SSL: Android and iOS certificate chain & trust wiring
Build, dependencies and platform configs
Example app modernization and UI/tooling updates
Retry logic, streaming reads and test migration
Linting, TypeScript schemas and tooling
Sequence Diagram(s)sequenceDiagram
participant Client as App/JS
participant Network as NetworkClient (Kotlin/Swift)
participant Trust as TrustManager / SecTrust
participant Chain as CertificateChainHelper / AiaCertHelper
participant JS as JS Event Emitter
Client->>Network: Make HTTPS request
Network->>Trust: Validate server certificate
alt Trust validation succeeds
Trust-->>Network: Validation OK
Network->>Client: Proceed with response
else Trust validation fails (recoverable)
Trust->>Chain: Fetch AIA caIssuers & attempt chain completion
Chain-->>Trust: Augmented chain or nil
alt Augmented chain valid
Trust-->>Network: Validation succeeded with augmented chain
Network->>Client: Proceed with response
else Augmentation failed
Network->>JS: emitInvalidCertificateError(chain, cause)
JS-->>Network: (event handled)
Network->>Client: Fail request with certificate error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
example/android/app/src/main/java/com/example/reactnativenetworkclient/MainApplication.kt (1)
24-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
OkHttpClientProvider.setOkHttpClientFactory()beforeloadReactNative(this).
OkHttpClientProviderlazily initializes its client on the first network request.NetworkingModule(created on first JS network access) requests the client fromOkHttpClientProviderduring construction. IfloadReactNative(this)is called first and triggers any network activity during React Native initialization, the default client will be created before your factory is registered, bypassing SSL/Brotli/cookie customizations. Setting the factory beforehand guarantees it's in place before anyNetworkingModuleinstance can be created.Suggested fix
override fun onCreate() { super.onCreate() - loadReactNative(this) - // Tells React Native to use our RCTOkHttpClientFactory which builds an OKHttpClient // with a cookie jar defined in APIClientModule and an interceptor to intercept all // requests that originate from React Native's OKHttpClient OkHttpClientProvider.setOkHttpClientFactory(RCTOkHttpClientFactory()) + loadReactNative(this) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/android/app/src/main/java/com/example/reactnativenetworkclient/MainApplication.kt` around lines 24 - 31, Move the OkHttpClientProvider factory registration so it runs before React Native initializes: call OkHttpClientProvider.setOkHttpClientFactory(RCTOkHttpClientFactory()) prior to invoking loadReactNative(this) in onCreate(), ensuring the custom factory is registered before any NetworkingModule or first network request can lazily create the default client and bypass your SSL/Brotli/cookie customizations.android/src/main/java/com/mattermost/networkclient/NetworkClient.kt (1)
403-436:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the bodyless-POST fallback even when
optionsisnull.The new empty JSON body is only added inside
if (options != null). A call likepost(..., null)still builds a POST withbody == null, so it misses the WAF workaround and can still trip OkHttp's method/body validation.💡 Proposed fix
private fun prepareRequestBody(method: String, options: ReadableMap?): RequestBody? { var requestBody: RequestBody? = null if (options != null) { if (options.hasKey("body")) { when (options.getType("body")) { @@ - } else if (method.uppercase(Locale.ENGLISH) == "POST") { - // Explicit Content-Type ensures WAFs don't reject bodyless POSTs (`#9689`). - // Caller can override by including Content-Type in the request headers. - requestBody = "".toRequestBody(MEDIA_TYPE_JSON) - } + } + } + + if (requestBody == null && method.uppercase(Locale.ENGLISH) == "POST") { + // Explicit Content-Type ensures WAFs don't reject bodyless POSTs (`#9689`). + // Caller can override by including Content-Type in the request headers. + requestBody = "".toRequestBody(MEDIA_TYPE_JSON) } return requestBody }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt` around lines 403 - 436, In prepareRequestBody, the POST-empty-body fallback is only applied when options != null so post(..., null) can produce a null body; update prepareRequestBody to ensure when method.uppercase(Locale.ENGLISH) == "POST" and requestBody is still null (or options is null) you assign an empty JSON body (e.g., "".toRequestBody(MEDIA_TYPE_JSON)) so the WAF workaround always runs; locate prepareRequestBody and the existing options null branch and move or add the POST fallback after the options handling (or check for options == null) so the empty-body assignment executes whenever no body was produced.ios/ApiClient/ApiClient.mm (1)
238-241:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
requestAdapterConfigurationis built and then discarded.In the new-architecture path,
bearerAuthTokenResponseHeadernever reacheswrapperbecauseadapterDictionaryis never stored indict. That breaks request-adapter configuration for codegen clients.💡 Proposed fix
if (config.requestAdapterConfiguration().has_value()) { NSMutableDictionary *adapterDictionary = [[NSMutableDictionary alloc] init]; adapterDictionary[@"bearerAuthTokenResponseHeader"] = config.requestAdapterConfiguration().value().bearerAuthTokenResponseHeader(); + dict[@"requestAdapterConfiguration"] = adapterDictionary; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/ApiClient/ApiClient.mm` around lines 238 - 241, The code creates adapterDictionary in the requestAdapterConfiguration() branch but never attaches it to the outer dict/wrapper, so the bearerAuthTokenResponseHeader never flows to wrapper; update the branch that builds adapterDictionary (referencing requestAdapterConfiguration(), adapterDictionary) to insert it into the main dict/wrapper (e.g., set dict[@"requestAdapterConfiguration"] = adapterDictionary or otherwise merge adapterDictionary into dict before using wrapper) so the request-adapter configuration reaches wrapper.
🧹 Nitpick comments (3)
example/android/gradlew (1)
117-117: ⚡ Quick winSimplify wrapper launch by removing unused classpath plumbing.
Both
example/android/gradlewandexample/android/gradlew.batset aCLASSPATHvariable and pass it via-classpathflag, even though the wrapper is invoked with-jar. This unnecessary plumbing—settingCLASSPATHto an empty value, converting it for Cygwin/MSYS, and passing it as an argument—adds avoidable complexity to the shell path handling without functional benefit. Remove theCLASSPATHdeclaration (line 117), its conversion (line 175), and the-classpathargument (line 215) to align with the standard Gradle 9 wrapper pattern.Suggested diff
-CLASSPATH="\\\"\\\"" @@ if "$cygwin" || "$msys" ; then APP_HOME=$( cygpath --path --mixed "$APP_HOME" ) - CLASSPATH=$( cygpath --path --mixed "$CLASSPATH" ) @@ set -- \ "-Dorg.gradle.appname=$APP_BASE_NAME" \ - -classpath "$CLASSPATH" \ -jar "$APP_HOME/gradle/wrapper/gradle-wrapper.jar" \ "$@"Apply the same cleanup to
example/android/gradlew.bat(remove line 78 and the-classpath "%CLASSPATH%"argument from line 82).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/android/gradlew` at line 117, Remove the unused CLASSPATH plumbing: delete the CLASSPATH declaration that sets CLASSPATH="\"\"" in the shell wrapper, remove the block that converts CLASSPATH for Cygwin/MSYS, and remove the -classpath argument passed to the java invocation that launches the wrapper JAR; do the same in the Windows wrapper by deleting the CLASSPATH variable and removing the -classpath "%CLASSPATH%" argument so the wrapper launches with -jar only (locate the CLASSPATH assignment, the cygpath conversion block, and the java invocation that includes -classpath to modify).example/android/app/build.gradle (1)
73-73: ⚡ Quick winPin the JSC artefact to an exact version.
Using
2026004.+makes the example build non-reproducible and can start pulling a different JSC patch release on the next install. Given the rest of this PR is explicitly pinning OkHttp/Kotlin, this one should be locked to the exact version you validated too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/android/app/build.gradle` at line 73, The jscFlavor variable is using a wildcard version ('io.github.react-native-community:jsc-android:2026004.+') which makes builds non-reproducible; change the jscFlavor assignment to pin the exact validated artifact version (replace the '2026004.+' range with the specific patch version you tested) so the build pulls an immutable dependency (update the jscFlavor definition accordingly).eslint.config.js (1)
9-16: ⚡ Quick win
example/**is fully excluded from linting.This ignore means the upgraded sample app won't be linted at all, even though this PR changes its Gradle, package, and iOS bootstrap files. If that isn't intentional, trim the ignore or add a dedicated flat-config entry for
example/so regressions in the RN 0.83 migration are still caught.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@eslint.config.js` around lines 9 - 16, The current ESLint ignores array contains "example/**" which fully excludes the sample app from linting; remove or narrow that pattern (or add a dedicated flat-config entry for example/) so changes to the sample app (the RN 0.83 migration files) are linted; edit the ignores array (the ignores symbol) to either delete "example/**" or replace it with a more specific pattern (e.g., exclude generated/artifacts only) or add a separate flat-config entry targeting "example/" to ensure the example project is still validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@android/build.gradle`:
- Around line 61-67: The forced Kotlin stdlib version in configurations.all
(resolutionStrategy forcing "org.jetbrains.kotlin:kotlin-stdlib:2.2.21") is
hard-coded and can diverge from the plugin/dependency Kotlin version; change the
force call to use the project's Kotlin version accessor (e.g., call
getKotlinVersion() or reference rootProject.ext.kotlinVersion) so the forced
artifact matches the Kotlin version used by the gradle-plugin and implementation
dependencies in this build.
In `@android/src/main/java/com/mattermost/networkclient/Extensions.kt`:
- Around line 83-115: The code calls source.request(Long.MAX_VALUE) which forces
the entire response into memory before you check bufferSize, so remove or
replace that call: either use responseBody.contentLength() if available or call
source.request(MAX_IN_MEMORY_SIZE + 1) to probe whether the body exceeds the
threshold without loading everything. Then use source (not
source.buffer.clone()) to stream-read into readBufferInChunks when the probe
indicates a large body, and only call buffer.readUtf8() for the small-body path;
update references to source.request, buffer.clone(), bufferSize,
MAX_IN_MEMORY_SIZE, and readBufferInChunks accordingly.
- Around line 31-45: readBufferInChunks currently decodes each fixed-size byte
chunk independently which can split multibyte UTF-8 characters and produce
U+FFFD; fix by switching to a streaming decode using a CharsetDecoder so partial
multibyte sequences are preserved across iterations. In readBufferInChunks,
create a CharsetDecoder from StandardCharsets.UTF_8.newDecoder(), on each loop
read the chunk bytes into a ByteBuffer, call decoder.decode(byteBuffer,
charBuffer, false) appending decoded chars to the StringBuilder and retain any
remaining undecoded bytes between iterations (by compacting or slicing the
ByteBuffer), and after the loop call decoder.decode(..., true) and
decoder.flush(...) to finish decoding; reference readBufferInChunks, CHUNK_SIZE,
buffer.read... and StandardCharsets.UTF_8 in your changes.
In
`@android/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.kt`:
- Around line 112-120: The fetchCertificate function currently reads the entire
response via readBytes(), risking OOM from a malicious AIA endpoint; change it
to stream and enforce a hard byte limit (e.g., MAX_AIA_BYTES ~ a few MB) when
reading the InputStream instead of using readBytes(), reading in a buffered loop
into a ByteArrayOutputStream and aborting (throwing/logging and closing the
connection) if the limit is exceeded, then pass the collected byte array to
CertificateFactory.generateCertificate; keep existing timeouts and ensure all
streams are closed on both success and error.
- Around line 91-103: Validate DER lengths before slicing and return null on
malformed data: in the AIA parsing block (where CA_ISSUERS_OID is compared)
guard the copyOfRange and String construction by checking oidLen and uriLen
against remaining bounds (use seqEnd and der.size and current pos) before
calling lengthAt/lengthSize results; ensure pos + oidLen and pos + uriLen are <=
seqEnd and <= der.size and that pos is valid when testing tags (der[pos])—if any
check fails, bail out/return null instead of performing the slice or
constructing the String.
In
`@android/src/main/java/com/mattermost/networkclient/helpers/KeyStoreHelper.kt`:
- Around line 87-94: getClientCertKeyStore currently assumes the PKCS12 file
exists and doesn't close the InputStream; update getClientCertKeyStore to safely
handle missing files by catching FileNotFoundException from
ApiClientModuleImpl.context.openFileInput(p12Alias), returning null if missing,
and ensure the opened InputStream is closed in a finally/use block;
additionally, update deleteClientCertificates to also clear the stored password
via ApiClientModuleImpl.retrieveValue/remove method so the persisted secret and
file can't get out of sync.
In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt`:
- Around line 422-424: The code currently maps ReadableType.Null to
RequestBody.EMPTY which forces bodyless verbs like GET/HEAD to have a body and
breaks Request.Builder.method(...); change the ReadableType.Null branch to set
requestBody = null (make the requestBody variable nullable if needed) so that
callers of the surrounding function (the logic building the Request via
Request.Builder.method(...)) can apply their verb-specific fallback behavior;
keep the reference to RequestBody.EMPTY only for places that truly require an
empty body, and ensure the surrounding method (where requestBody is used)
handles a null body appropriately.
- Around line 620-626: The SSLContext is being initialized with the original
trustManager inside TrustManagerHelper.buildSslSocketFactory so the
later-wrapped trust manager returned by NetworkClient.buildSslConfig via
getTrustManager(trustManager) is not used; fix by ensuring the SSLContext is
initialized with the wrapped trust manager: either wrap the trustManager before
calling TrustManagerHelper.buildSslSocketFactory (i.e., call
getTrustManager(...) in NetworkClient.buildSslConfig and pass that wrapped
manager into buildSslSocketFactory), or modify
TrustManagerHelper.buildSslSocketFactory so it accepts/creates the wrapped trust
manager and calls sslContext.init(keyManagers, arrayOf(wrappedTrustManager),
SecureRandom()); update references to TrustManagerHelper.buildSslSocketFactory,
NetworkClient.buildSslConfig, getTrustManager, and the sslContext.init call
accordingly.
In `@example/android/gradle/wrapper/gradle-wrapper.properties`:
- Line 3: The Gradle wrapper was bumped to 9.0.0 but the project AGP is still on
a 7.x line; confirm the Android Gradle Plugin version used by both the root
android/build.gradle and example/android/build.gradle (look for the
com.android.tools.build:gradle dependency or plugin declaration) and resolve the
incompatibility by either setting distributionUrl in
example/android/gradle/wrapper/gradle-wrapper.properties back to a compatible
Gradle 8.x release or updating the AGP declaration(s) to 8.0+ (and adjust any
AGP-related build config if required) so Gradle 9.0.0 is supported.
In `@example/src/components/FilePickerButtonGroup.tsx`:
- Around line 36-50: The code currently reads result.fileCopyUri which doesn't
exist on keepLocalCopy's response; update the flow in the pick/keepLocalCopy
sequence to check the discriminant (status) on the keepLocalCopy response (the
returned result) and, when status indicates success, read the success field's
localUri and use that as the NativeFile.uri (instead of result.fileCopyUri);
reference the functions/variables pick, keepLocalCopy, selectedFile, result and
the constructed file: NativeFile to locate and change the logic so it only
accesses localUri on the successful branch.
- Line 2: Update the import to include errorCodes from
"@react-native-documents/picker" and change the catch logic that currently uses
isErrorWithCode to swallow all coded errors: use isErrorWithCode(err,
errorCodes.OPERATION_CANCELED) (or compare err.code ===
errorCodes.OPERATION_CANCELED) so only OPERATION_CANCELED is swallowed and all
other errors (e.g., IN_PROGRESS, NULL_PRESENTER, UNABLE_TO_OPEN_FILE_TYPE) are
re-thrown or propagated; reference the existing
pick/keepLocalCopy/isErrorWithCode usage to locate where to adjust the catch
handling.
In `@example/src/components/P12Inputs.tsx`:
- Around line 48-63: The code is reading a non-existent result.fileCopyUri and
swallowing picker errors; update the keepLocalCopy handling to inspect the
discriminant (result.status), throw or propagate when status !== "success", and
on success use the returned localUri (e.g. copyResult.localUri) to call
props.onSelectP12. In the error handler, avoid silently ignoring errors matched
by isErrorWithCode(err); rethrow or surface picker/keepLocalCopy failures so the
caller can show feedback. Locate the logic around pick(), keepLocalCopy(...) and
props.onSelectP12 and replace the fileCopyUri usage with the union-safe status
check and localUri property.
- Line 7: Update the error handling in P12Inputs and FilePickerButtonGroup where
isErrorWithCode(err) is used: import errorCodes from
"@react-native-documents/picker" and only suppress errors when err.code ===
errorCodes.OPERATION_CANCELED; for any other isErrorWithCode(err) values (e.g.,
IN_PROGRESS, UNABLE_TO_OPEN_FILE_TYPE) re-throw the error so real failures are
not silently ignored. Locate the checks around the picker calls (functions using
pick/keepLocalCopy and the isErrorWithCode branch) and replace the blanket
suppression with an explicit err.code === errorCodes.OPERATION_CANCELED check
before swallowing the error.
In `@example/src/components/ResponseSuccessOverlay.tsx`:
- Around line 5-6: The component imports Text from "react-native" but uses the
react-native-elements-specific prop h4 in ResponseSuccessOverlay (several Text
usages), causing runtime errors; change the import to pull Text from
"react-native-elements" (or replace h4 usages with appropriate react-native
styles) so the Text component matches the h4 prop expected by the code—update
the import statement that currently imports Text from "react-native" and ensure
only the react-native-elements Text is used by functions/components like
ResponseSuccessOverlay and any local Text usages.
In `@example/src/screens/APIClientRequestScreen.tsx`:
- Line 99: The console.log('RESPONSE', response) in APIClientRequestScreen.tsx
must be removed or gated to development and redact sensitive fields; locate the
logging inside the APIClientRequestScreen component (or the function handling
the network response) and either delete the console.log or wrap it with a
development-only guard (e.g., __DEV__ or NODE_ENV==='development') and ensure
any logged object strips tokens/PII (redact Authorization, token, user
identifiers) before logging.
In `@ios/Extensions/Session`+Extensions.swift:
- Around line 15-16: The two global dictionaries certErrorEmitted_FILEPRIVATE
and lastServerCertSummary_FILEPRIVATE are mutated from multiple threads
(delegate callbacks and invalidation) and must be made thread-safe: create a
dedicated serial DispatchQueue or an NSLock (e.g. certStateLockQueue or
certStateLock) and use it to synchronise all reads and writes to
certErrorEmitted_FILEPRIVATE and lastServerCertSummary_FILEPRIVATE (including
places that clear them during invalidation and any delegate-path accesses);
alternatively move these dictionaries onto a thread-safe owner object and gate
access through that owner. Ensure every site that touches these symbols
(including the locations around the other mentioned block using the same
globals) is updated to use the chosen lock/queue.
In `@ios/patches/apply_patches.rb`:
- Line 12: The current directory_arg assignment can return nil or pick the wrong
Pods dir; replace the one-liner that uses Dir.glob(...).first.sub(...) with
deterministic logic: collect all matches from
Dir.glob(Pathname(repo_root).join("**/**/Pods")), fail fast with a clear
error/log and exit if empty, and if multiple matches exist prefer in order:
"#{repo_root}/ios/Pods", "#{repo_root}/Pods", otherwise choose the shortest
relative path (closest to repo_root) and log a warning about multiple
candidates; then set directory_arg to the chosen path with the "#{repo_root}/"
prefix removed. Update the code around variable directory_arg and use
repo_root/Pathname references accordingly.
In `@ios/Utilities/AiaCertHelper.swift`:
- Around line 154-164: The fetchCertificate(from:) function starts a URLSession
dataTask but doesn't retain the task or cancel it on semaphore timeout, leaving
background requests running; update fetchCertificate(from:) to store the data
task in a local variable (e.g., let task = URLSession.shared.dataTask(...)),
call task.resume(), wait on the semaphore, and if the wait returns .timedOut
call task.cancel() before returning nil; also ensure the completion handler
always signals the semaphore so the wait unblocks and use the retained task
variable in the cancel call.
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/RetryInterceptorTest.kt`:
- Around line 135-136: The test uses verify(retryInterceptor,
times(0)).getWaitInterval(0) and verify(...).waitForMilliseconds(0) which only
assert those methods were not called with the literal 0; change these to
verify(retryInterceptor, never()).getWaitInterval(<any-int-matcher>) and
verify(retryInterceptor, never()).waitForMilliseconds(<any-int-matcher>) so the
test asserts no invocations with any argument; update the three other
occurrences mentioned (the assertions at lines 153-154 and 232-233) accordingly
and import/use the appropriate Mockito/Kotlin any-int matcher and never()
matcher used elsewhere in RetryInterceptorTest.
---
Outside diff comments:
In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt`:
- Around line 403-436: In prepareRequestBody, the POST-empty-body fallback is
only applied when options != null so post(..., null) can produce a null body;
update prepareRequestBody to ensure when method.uppercase(Locale.ENGLISH) ==
"POST" and requestBody is still null (or options is null) you assign an empty
JSON body (e.g., "".toRequestBody(MEDIA_TYPE_JSON)) so the WAF workaround always
runs; locate prepareRequestBody and the existing options null branch and move or
add the POST fallback after the options handling (or check for options == null)
so the empty-body assignment executes whenever no body was produced.
In
`@example/android/app/src/main/java/com/example/reactnativenetworkclient/MainApplication.kt`:
- Around line 24-31: Move the OkHttpClientProvider factory registration so it
runs before React Native initializes: call
OkHttpClientProvider.setOkHttpClientFactory(RCTOkHttpClientFactory()) prior to
invoking loadReactNative(this) in onCreate(), ensuring the custom factory is
registered before any NetworkingModule or first network request can lazily
create the default client and bypass your SSL/Brotli/cookie customizations.
In `@ios/ApiClient/ApiClient.mm`:
- Around line 238-241: The code creates adapterDictionary in the
requestAdapterConfiguration() branch but never attaches it to the outer
dict/wrapper, so the bearerAuthTokenResponseHeader never flows to wrapper;
update the branch that builds adapterDictionary (referencing
requestAdapterConfiguration(), adapterDictionary) to insert it into the main
dict/wrapper (e.g., set dict[@"requestAdapterConfiguration"] = adapterDictionary
or otherwise merge adapterDictionary into dict before using wrapper) so the
request-adapter configuration reaches wrapper.
---
Nitpick comments:
In `@eslint.config.js`:
- Around line 9-16: The current ESLint ignores array contains "example/**" which
fully excludes the sample app from linting; remove or narrow that pattern (or
add a dedicated flat-config entry for example/) so changes to the sample app
(the RN 0.83 migration files) are linted; edit the ignores array (the ignores
symbol) to either delete "example/**" or replace it with a more specific pattern
(e.g., exclude generated/artifacts only) or add a separate flat-config entry
targeting "example/" to ensure the example project is still validated.
In `@example/android/app/build.gradle`:
- Line 73: The jscFlavor variable is using a wildcard version
('io.github.react-native-community:jsc-android:2026004.+') which makes builds
non-reproducible; change the jscFlavor assignment to pin the exact validated
artifact version (replace the '2026004.+' range with the specific patch version
you tested) so the build pulls an immutable dependency (update the jscFlavor
definition accordingly).
In `@example/android/gradlew`:
- Line 117: Remove the unused CLASSPATH plumbing: delete the CLASSPATH
declaration that sets CLASSPATH="\"\"" in the shell wrapper, remove the block
that converts CLASSPATH for Cygwin/MSYS, and remove the -classpath argument
passed to the java invocation that launches the wrapper JAR; do the same in the
Windows wrapper by deleting the CLASSPATH variable and removing the -classpath
"%CLASSPATH%" argument so the wrapper launches with -jar only (locate the
CLASSPATH assignment, the cygpath conversion block, and the java invocation that
includes -classpath to modify).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb070022-06d7-45e0-843f-c48377c66219
⛔ Files ignored due to path filters (5)
example/Gemfile.lockis excluded by!**/*.lockexample/android/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jarexample/ios/Podfile.lockis excluded by!**/*.lockexample/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (72)
.eslintignore.eslintrc.json.gitignoreREADME.mdandroid/build.gradleandroid/src/main/java/com/mattermost/networkclient/ApiClientModuleImpl.ktandroid/src/main/java/com/mattermost/networkclient/Extensions.ktandroid/src/main/java/com/mattermost/networkclient/NetworkClient.ktandroid/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.ktandroid/src/main/java/com/mattermost/networkclient/helpers/KeyStoreHelper.ktandroid/src/main/java/com/mattermost/networkclient/helpers/TrustManagerHelper.ktandroid/src/main/java/com/mattermost/networkclient/interfaces/RetryInterceptor.ktandroid/src/test/java/com/mattermost/networkclient/ExponentialRetryInterceptorTest.ktandroid/src/test/java/com/mattermost/networkclient/WebSocketClientModuleImplTest.ktbabel.config.jseslint.config.jsexample/Gemfileexample/android/app/build.gradleexample/android/app/src/main/java/com/example/reactnativenetworkclient/MainApplication.ktexample/android/build.gradleexample/android/gradle.propertiesexample/android/gradle/wrapper/gradle-wrapper.propertiesexample/android/gradlewexample/android/gradlew.batexample/babel.config.jsexample/ios/NetworkClientExample-Bridging-Header.hexample/ios/NetworkClientExample.xcodeproj/project.pbxprojexample/ios/NetworkClientExample/AppDelegate.hexample/ios/NetworkClientExample/AppDelegate.mmexample/ios/NetworkClientExample/AppDelegate.swiftexample/ios/NetworkClientExample/Info.plistexample/ios/NetworkClientExample/SDWebImageDownloaderOperation+Swizzle.swiftexample/ios/NetworkClientExample/main.mexample/ios/Podfileexample/metro.config.jsexample/package.jsonexample/patches/react-native-image-picker+7.2.3.patchexample/patches/react-native-markdown-renderer+3.2.8.patchexample/src/App.tsxexample/src/components/FilePickerButtonGroup.tsxexample/src/components/P12Inputs.tsxexample/src/components/ResponseSuccessOverlay.tsxexample/src/screens/APIClientDownloadScreen.tsxexample/src/screens/APIClientFastImageScreen.tsxexample/src/screens/APIClientImportP12Screen.tsxexample/src/screens/APIClientRequestScreen.tsxexample/src/screens/APIClientScreen.tsxexample/src/screens/APIClientUploadScreen.tsxexample/src/screens/ClientListScreen.tsxexample/src/screens/CreateAPIClientScreen.tsxexample/src/screens/CreateWebSocketClientScreen.tsxexample/src/screens/GenericClientRequestScreen.tsxexample/src/screens/MattermostClientUploadScreen.tsxexample/src/screens/WebSocketClientScreen.tsxios/ApiClient/ApiClient.hios/ApiClient/ApiClient.mmios/ApiClient/ApiClientWrapper.swiftios/Delegates/ApiClientSessionDelegate.swiftios/Extensions/Session+Extensions.swiftios/NetworkClient.xcodeproj/project.pbxprojios/SessionManager.swiftios/Utilities/AiaCertHelper.swiftios/patches/Alamofire+5.11.2.diffios/patches/apply_patches.rbpackage.jsonreact-native-network-client.podspecsrc/APIClient/NativeApiClient.tssrc/schemas.tsxtest-runner/build.gradle.ktstest-runner/src/test/kotlin/com/mattermost/networkclient/RetryInterceptorTest.kttsconfig.build.jsontsconfig.json
💤 Files with no reviewable changes (11)
- example/ios/NetworkClientExample-Bridging-Header.h
- example/ios/NetworkClientExample/AppDelegate.h
- example/ios/NetworkClientExample/main.m
- ios/NetworkClient.xcodeproj/project.pbxproj
- example/patches/react-native-markdown-renderer+3.2.8.patch
- .eslintignore
- example/ios/NetworkClientExample/AppDelegate.mm
- .eslintrc.json
- android/src/test/java/com/mattermost/networkclient/WebSocketClientModuleImplTest.kt
- android/src/test/java/com/mattermost/networkclient/ExponentialRetryInterceptorTest.kt
- example/patches/react-native-image-picker+7.2.3.patch
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
android/src/main/java/com/mattermost/networkclient/NetworkClient.kt (2)
400-433:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the bodyless POST fallback even when
optionsisnull.The empty JSON body is only added inside
if (options != null). A reachable call likerequest("POST", ..., null, ...)still leavesbody == null, so the WAF workaround added in this PR is skipped on that path.Suggested fix
private fun prepareRequestBody(method: String, options: ReadableMap?): RequestBody? { var requestBody: RequestBody? = null if (options != null) { if (options.hasKey("body")) { @@ - } else if (method.uppercase(Locale.ENGLISH) == "POST") { - // Explicit Content-Type ensures WAFs don't reject bodyless POSTs (`#9689`). - // Caller can override by including Content-Type in the request headers. - requestBody = "".toRequestBody(MEDIA_TYPE_JSON) } } + + if (requestBody == null && method.uppercase(Locale.ENGLISH) == "POST") { + // Explicit Content-Type ensures WAFs don't reject bodyless POSTs (`#9689`). + // Caller can override by including Content-Type in the request headers. + requestBody = "".toRequestBody(MEDIA_TYPE_JSON) + } return requestBody }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt` around lines 400 - 433, The POST body fallback is only applied when options != null, so a call like request("POST", ..., null, ...) skips the empty JSON body; update prepareRequestBody to ensure that when method.uppercase(Locale.ENGLISH) == "POST" and no body was created (i.e., options is null or options.hasKey("body") is false), you still set requestBody = "".toRequestBody(MEDIA_TYPE_JSON) so the empty JSON body is applied; locate this logic in prepareRequestBody and move or add the POST fallback check outside/after the options != null branch (or check for requestBody == null) while preserving the existing comment about WAFs and allowing header overrides.
199-210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the configured hostname policy when rebuilding after P12 import.
This path always reinstalls the default verifier. If
trustSelfSignedServerCertificatewas enabled on the original client, importing a P12 at runtime flips the client back to strict hostname checks and breaks the self-signed flow only on the rebuild path.Suggested fix
if (baseUrl != null) { val (sslSocketFactory, trustManager) = buildSslConfig() - okHttpClient = okHttpClient.newBuilder() - .sslSocketFactory(sslSocketFactory, trustManager) - .hostnameVerifier { hostname, session -> - val hv = HttpsURLConnection.getDefaultHostnameVerifier() - val result = hv.verify(hostname, session) - if (!result) { - emitInvalidCertificateError() - } - result - } - .build() + val rebuilt = okHttpClient.newBuilder() + .sslSocketFactory(sslSocketFactory, trustManager) + + if (trustSelfSignedServerCertificate) { + rebuilt.hostnameVerifier { _, _ -> true } + } else { + rebuilt.hostnameVerifier { hostname, session -> + val hv = HttpsURLConnection.getDefaultHostnameVerifier() + val result = hv.verify(hostname, session) + if (!result) { + emitInvalidCertificateError() + } + result + } + } + + okHttpClient = rebuilt.build() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt` around lines 199 - 210, When rebuilding okHttpClient after importing a P12 (in the branch where baseUrl != null and buildSslConfig() is called), preserve and reuse the existing hostname verifier instead of always installing the default one; capture the current client's hostnameVerifier (from the existing okHttpClient instance) and only replace it when necessary (e.g., when a new explicit policy is intended), otherwise reapply that verifier in the newBuilder() call so trustSelfSignedServerCertificate behavior isn’t lost; ensure emitInvalidCertificateError() is still invoked on verification failure as before.
♻️ Duplicate comments (1)
android/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.kt (1)
84-106:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject zero-length DER lengths before continuing the AIA scan.
lengthAt()returns0on malformed/truncated input. In this loop, that can makeseqEnd == pos, so thecontinuepath re-enters with the same offset and spins during the TLS handshake instead of bailing out cleanly. Treat non-positiveseqLen/oidLen/uriLenas invalid and returnnullbefore reusingpos.Suggested fix
val seqLen = lengthAt(der, pos) val seqLenSize = lengthSize(der, pos) + if (seqLen <= 0 || pos + seqLenSize > der.size) return null pos += seqLenSize val seqEnd = pos + seqLen if (seqEnd > der.size) return null @@ pos++ val oidLen = lengthAt(der, pos) - pos += lengthSize(der, pos) - if (oidLen > seqEnd - pos) return null + val oidLenSize = lengthSize(der, pos) + if (oidLen <= 0 || pos + oidLenSize > seqEnd || pos + oidLenSize > der.size) return null + pos += oidLenSize + if (pos + oidLen > seqEnd || pos + oidLen > der.size) return null val oidBytes = der.copyOfRange(pos, pos + oidLen) pos += oidLen @@ if (pos < seqEnd && der[pos].toInt() and 0xFF == 0x86) { pos++ val uriLen = lengthAt(der, pos) - pos += lengthSize(der, pos) - if (uriLen > seqEnd - pos) return null + val uriLenSize = lengthSize(der, pos) + if (uriLen <= 0 || pos + uriLenSize > seqEnd || pos + uriLenSize > der.size) return null + pos += uriLenSize + if (pos + uriLen > seqEnd || pos + uriLen > der.size) return null return String(der, pos, uriLen, Charsets.US_ASCII) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.kt` around lines 84 - 106, The loop in CertificateChainHelper that parses AIA must reject zero or negative lengths from lengthAt() to avoid infinite loops; update the parsing logic around lengthAt/lengthSize usage (for seqLen, oidLen and uriLen) so that after computing seqLen and seqLenSize you return null if seqLen <= 0 (and likewise return null if oidLen <= 0 or uriLen <= 0) before advancing pos or continuing; ensure these checks occur immediately after computing each length (references: lengthAt, lengthSize, CA_ISSUERS_OID and the AIA parsing block) so malformed/truncated DER causes an early null return instead of reusing the same pos.
🧹 Nitpick comments (1)
test-runner/src/test/kotlin/com/mattermost/networkclient/RetryInterceptorTest.kt (1)
113-113: 💤 Low valueConsider closing the Response to avoid resource warnings.
The
Responseobject returned byexecute()should be closed to release connection resources. While MockWebServer handles server-side cleanup, leaving Response objects unclosed can generate warnings and leak connections in more complex test scenarios.♻️ Suggested fix
- client.newCall(request).execute() + client.newCall(request).execute().close()Same pattern applies to lines 136 and 154 where the response is not inspected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test-runner/src/test/kotlin/com/mattermost/networkclient/RetryInterceptorTest.kt` at line 113, The test calls client.newCall(request).execute() without closing the returned Response; update each invocation (including the occurrences at client.newCall(request).execute() around lines noted and the similar calls at lines 136 and 154) to close the Response—preferably by using Kotlin's use { ... } on the Response (e.g., client.newCall(request).execute().use { /* optionally assert or ignore */ }) or by calling response.close()—so the connection resources are properly released in RetryInterceptorTest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@android/build.gradle`:
- Line 2: The default Kotlin version returned by
getKotlinVersion()/rootProject.ext ('kotlinVersion') is set to "2.2.21", which
is incompatible with the pinned Android Gradle Plugin (AGP) 7.3.1; either change
the default Kotlin version to one supported by AGP 7.3.1 or upgrade the
AGP/Gradle versions together. Locate the getKotlinVersion()/kotlinVersion
default and either lower the default string to a Kotlin 1.x/1.8.x version
compatible with AGP 7.3.1, or update the AGP plugin dependency (the classpath
artifact that currently pins 7.3.1) and the Gradle wrapper to the matching
versions required for Kotlin 2.2.21 (AGP 8.10+), ensuring both changes are
coherent across places that call getKotlinVersion().
In
`@android/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.kt`:
- Around line 120-125: In fetchCertificate, validate the URL scheme before
calling URL(url).openConnection(): parse the input string (e.g., via URL or URI)
and only allow "http" or "https"; if the scheme is anything else, reject it
(return null or throw) to prevent local/resource schemes like file: or jar:
being used for AIA fetches. Apply this check at the top of the fetchCertificate
function so conn is only opened for http(s) URLs.
In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt`:
- Around line 604-620: Before attempting to apply a new clientP12Configuration,
clear any previously stored client certificate/state so a failed import does not
leave a stale identity in place; specifically, in the block that reads
clientP12Configuration and calls importClientP12(path, password) (and in the
importClientP12 implementation if needed), reset the p12Alias/any keystore
entries used by configureSsl() (or set the variable that configureSsl() reads to
null/empty) before calling importClientP12, and ensure that on Exception (or if
importClientP12 no-ops) you leave that alias cleared so configureSsl() will not
load the old certificate.
In `@ios/Utilities/AiaCertHelper.swift`:
- Around line 209-220: The lengthAt(_:_: ) function can crash when the DER
length byte is 0x80 because numBytes becomes 0 and the for loop uses
1...numBytes; update lengthAt to explicitly guard for numBytes == 0
(indefinite-length) before constructing the range and treat it as a
malformed-length error—e.g., return a safe error tuple (such as (0,1) or (0,1 +
numBytes)) or otherwise bail out—also ensure you validate that pos + numBytes
does not exceed der.count before reading bytes; reference lengthAt, first, and
numBytes when making the checks.
---
Outside diff comments:
In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt`:
- Around line 400-433: The POST body fallback is only applied when options !=
null, so a call like request("POST", ..., null, ...) skips the empty JSON body;
update prepareRequestBody to ensure that when method.uppercase(Locale.ENGLISH)
== "POST" and no body was created (i.e., options is null or
options.hasKey("body") is false), you still set requestBody =
"".toRequestBody(MEDIA_TYPE_JSON) so the empty JSON body is applied; locate this
logic in prepareRequestBody and move or add the POST fallback check
outside/after the options != null branch (or check for requestBody == null)
while preserving the existing comment about WAFs and allowing header overrides.
- Around line 199-210: When rebuilding okHttpClient after importing a P12 (in
the branch where baseUrl != null and buildSslConfig() is called), preserve and
reuse the existing hostname verifier instead of always installing the default
one; capture the current client's hostnameVerifier (from the existing
okHttpClient instance) and only replace it when necessary (e.g., when a new
explicit policy is intended), otherwise reapply that verifier in the
newBuilder() call so trustSelfSignedServerCertificate behavior isn’t lost;
ensure emitInvalidCertificateError() is still invoked on verification failure as
before.
---
Duplicate comments:
In
`@android/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.kt`:
- Around line 84-106: The loop in CertificateChainHelper that parses AIA must
reject zero or negative lengths from lengthAt() to avoid infinite loops; update
the parsing logic around lengthAt/lengthSize usage (for seqLen, oidLen and
uriLen) so that after computing seqLen and seqLenSize you return null if seqLen
<= 0 (and likewise return null if oidLen <= 0 or uriLen <= 0) before advancing
pos or continuing; ensure these checks occur immediately after computing each
length (references: lengthAt, lengthSize, CA_ISSUERS_OID and the AIA parsing
block) so malformed/truncated DER causes an early null return instead of reusing
the same pos.
---
Nitpick comments:
In
`@test-runner/src/test/kotlin/com/mattermost/networkclient/RetryInterceptorTest.kt`:
- Line 113: The test calls client.newCall(request).execute() without closing the
returned Response; update each invocation (including the occurrences at
client.newCall(request).execute() around lines noted and the similar calls at
lines 136 and 154) to close the Response—preferably by using Kotlin's use { ...
} on the Response (e.g., client.newCall(request).execute().use { /* optionally
assert or ignore */ }) or by calling response.close()—so the connection
resources are properly released in RetryInterceptorTest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f53c262-291b-4d40-b269-a1ce2c1b3630
📒 Files selected for processing (13)
android/build.gradleandroid/src/main/java/com/mattermost/networkclient/Extensions.ktandroid/src/main/java/com/mattermost/networkclient/NetworkClient.ktandroid/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.ktandroid/src/main/java/com/mattermost/networkclient/helpers/KeyStoreHelper.ktandroid/src/main/java/com/mattermost/networkclient/helpers/TrustManagerHelper.ktexample/src/components/FilePickerButtonGroup.tsxexample/src/components/P12Inputs.tsxexample/src/components/ResponseSuccessOverlay.tsxexample/src/screens/APIClientRequestScreen.tsxios/Extensions/Session+Extensions.swiftios/Utilities/AiaCertHelper.swifttest-runner/src/test/kotlin/com/mattermost/networkclient/RetryInterceptorTest.kt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
android/src/main/java/com/mattermost/networkclient/NetworkClient.kt (3)
597-600:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep hostname verification enabled for
trustSelfSignedServerCertificate.
getTrustManager()already handles the self-signed opt-in during trust evaluation. Returningtruehere additionally accepts certificates for the wrong host, which is a much broader MITM bypass than the option name implies.Suggested fix
if (trustSelf) { trustSelfSignedServerCertificate = true - builder.hostnameVerifier { _, _ -> true } - } else { - buildHostnameVerifier() } + buildHostnameVerifier()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt` around lines 597 - 600, The current branch sets builder.hostnameVerifier { _, _ -> true } when trustSelf is true, which disables hostname verification and creates a broad MITM risk; instead remove the hostnameVerifier override and rely on the existing trustSelfSignedServerCertificate flag and getTrustManager() logic to handle self-signed certificates during trust evaluation. Locate the block that sets trustSelfSignedServerCertificate and remove the builder.hostnameVerifier lines, ensuring getTrustManager() remains responsible for evaluating self-signed opt-in behavior and the TLS hostname checks are preserved.
196-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear the previous client identity before importing a replacement.
If
importClientP12()throws—or just no-ops because the URI cannot be resolved—buildSslConfig()will keep loading the oldp12Alias.applyClientSslConfiguration()already clears the alias first; this path should do the same so a failed replacement does not keep presenting a stale client cert.Suggested fix
fun importClientP12AndRebuildClient(p12FilePath: String, password: String) { + KeyStoreHelper.deleteClientCertificates(p12Alias) importClientP12(p12FilePath, password) if (baseUrl != null) { val (sslSocketFactory, trustManager) = buildSslConfig() okHttpClient = okHttpClient.newBuilder()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt` around lines 196 - 203, The importClientP12AndRebuildClient path must clear the existing client identity before attempting a replacement so a failed import doesn't leave the old p12Alias in use; modify importClientP12AndRebuildClient to clear/reset the stored client identity (e.g. set p12Alias/nullify the alias or call the same clear logic used by applyClientSslConfiguration) before calling importClientP12(p12FilePath, password), and only set the alias back after a successful import so buildSslConfig() cannot load a stale client cert.
419-432:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the POST fallback after the body switch.
Line 419 correctly keeps an explicit
nullbody asnull, but Lines 429-432 only add the empty JSON body when thebodykey is absent.{"body": null}andoptions == nullstill skip the workaround and leave POSTs bodyless.Suggested fix
private fun prepareRequestBody(method: String, options: ReadableMap?): RequestBody? { var requestBody: RequestBody? = null - if (options != null) { - if (options.hasKey("body")) { + if (options?.hasKey("body") == true) { when (options.getType("body")) { ReadableType.Array -> { val jsonBody = JSONArray(options.getArray("body")!!.toArrayList()) requestBody = jsonBody.toString().toRequestBody(MEDIA_TYPE_JSON) } @@ ReadableType.Number -> { requestBody = options.getDouble("body").toString().toRequestBody(MEDIA_TYPE_JSON) } } - } else if (method.uppercase(Locale.ENGLISH) == "POST") { - // Explicit Content-Type ensures WAFs don't reject bodyless POSTs (`#9689`). - // Caller can override by including Content-Type in the request headers. - requestBody = "".toRequestBody(MEDIA_TYPE_JSON) - } + } + + if (requestBody == null && method.uppercase(Locale.ENGLISH) == "POST") { + // Explicit Content-Type ensures WAFs don't reject bodyless POSTs (`#9689`). + // Caller can override by including Content-Type in the request headers. + requestBody = "".toRequestBody(MEDIA_TYPE_JSON) } return requestBody }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt` around lines 419 - 432, The POST empty-body workaround should run after the body-parsing switch and must also handle options == null; move the existing POST fallback so it executes after you parse options into requestBody and apply it only when method.uppercase(Locale.ENGLISH) == "POST" && (options == null || !options.hasKey("body")), leaving cases where the caller explicitly provided body: null (handled by ReadableType.Null setting requestBody = null) untouched; update code around the requestBody construction in NetworkClient.kt (the requestBody variable, the body switch, MEDIA_TYPE_JSON, and the POST fallback) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/Utilities/AiaCertHelper.swift`:
- Around line 126-136: The DER length parser lengthAt should return (0, 0) on
parse error and callers must validate the returned length-size before advancing
or slicing; update lengthAt (the function performing the early break at the old
line 225) to detect insufficient bytes and return (0,0) on error, and then in
all call sites (the blocks using octetLenSize, seqLenSize, descLenSize,
oidLenSize and variables pos) check lenSize > 0 immediately after the call and
bail out (or set pos = descEnd / break) if it is zero so you never advance pos
or slice with insufficient bytes—apply this to the sequences at the uses of
octetLenSize, seqLenSize, descLenSize and oidLenSize to fail fast on
malformed/truncated DER.
---
Duplicate comments:
In `@android/src/main/java/com/mattermost/networkclient/NetworkClient.kt`:
- Around line 597-600: The current branch sets builder.hostnameVerifier { _, _
-> true } when trustSelf is true, which disables hostname verification and
creates a broad MITM risk; instead remove the hostnameVerifier override and rely
on the existing trustSelfSignedServerCertificate flag and getTrustManager()
logic to handle self-signed certificates during trust evaluation. Locate the
block that sets trustSelfSignedServerCertificate and remove the
builder.hostnameVerifier lines, ensuring getTrustManager() remains responsible
for evaluating self-signed opt-in behavior and the TLS hostname checks are
preserved.
- Around line 196-203: The importClientP12AndRebuildClient path must clear the
existing client identity before attempting a replacement so a failed import
doesn't leave the old p12Alias in use; modify importClientP12AndRebuildClient to
clear/reset the stored client identity (e.g. set p12Alias/nullify the alias or
call the same clear logic used by applyClientSslConfiguration) before calling
importClientP12(p12FilePath, password), and only set the alias back after a
successful import so buildSslConfig() cannot load a stale client cert.
- Around line 419-432: The POST empty-body workaround should run after the
body-parsing switch and must also handle options == null; move the existing POST
fallback so it executes after you parse options into requestBody and apply it
only when method.uppercase(Locale.ENGLISH) == "POST" && (options == null ||
!options.hasKey("body")), leaving cases where the caller explicitly provided
body: null (handled by ReadableType.Null setting requestBody = null) untouched;
update code around the requestBody construction in NetworkClient.kt (the
requestBody variable, the body switch, MEDIA_TYPE_JSON, and the POST fallback)
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e7179c7-0ffa-4ca2-9d6e-61202f126fc3
📒 Files selected for processing (3)
android/src/main/java/com/mattermost/networkclient/NetworkClient.ktandroid/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.ktios/Utilities/AiaCertHelper.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- android/src/main/java/com/mattermost/networkclient/helpers/CertificateChainHelper.kt
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ios/Utilities/AiaCertHelper.swift (2)
22-22: 💤 Low valueRemove unused
hostparameter or document its intended purpose.The
hostparameter is declared but never referenced in the function body. The SSL policies (including hostname verification) are already copied from the originalserverTrustat line 45, so the host appears unnecessary.Suggested fix
- static func completingChain(for serverTrust: SecTrust, host: String) -> SecTrust? { + static func completingChain(for serverTrust: SecTrust) -> SecTrust? {Also update the call site(s) accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Utilities/AiaCertHelper.swift` at line 22, The parameter host on completingChain(for: host:) is unused; either remove the host parameter from the static function signature (completingChain(for:)) and update all call sites to pass only the SecTrust, or if host is intended for future hostname policy logic, add a brief doc comment explaining its purpose and why it is unused now. Locate the function named completingChain(for: host:) and adjust its signature and any callers accordingly (or add documentation above the function) so there are no unused-parameter warnings.
42-42: 💤 Low valueCache original certificate count to avoid redundant call.
existingCertificates(from:)is called twice with the same trust object. Store the count at line 23 for a minor efficiency improvement.Suggested fix
static func completingChain(for serverTrust: SecTrust, host: String) -> SecTrust? { - var certs = existingCertificates(from: serverTrust) + let originalCerts = existingCertificates(from: serverTrust) + var certs = originalCerts guard let leaf = certs.first else { return nil } // ... - guard certs.count > existingCertificates(from: serverTrust).count else { return nil } + guard certs.count > originalCerts.count else { return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ios/Utilities/AiaCertHelper.swift` at line 42, The guard calls existingCertificates(from:) twice on the same serverTrust causing redundant work; cache the original count in a local variable (e.g., let originalCount = existingCertificates(from: serverTrust).count) near where certs is computed, then change the guard to compare certs.count > originalCount; reference existingCertificates(from:), certs, serverTrust, and the guard statement to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ios/Utilities/AiaCertHelper.swift`:
- Line 22: The parameter host on completingChain(for: host:) is unused; either
remove the host parameter from the static function signature
(completingChain(for:)) and update all call sites to pass only the SecTrust, or
if host is intended for future hostname policy logic, add a brief doc comment
explaining its purpose and why it is unused now. Locate the function named
completingChain(for: host:) and adjust its signature and any callers accordingly
(or add documentation above the function) so there are no unused-parameter
warnings.
- Line 42: The guard calls existingCertificates(from:) twice on the same
serverTrust causing redundant work; cache the original count in a local variable
(e.g., let originalCount = existingCertificates(from: serverTrust).count) near
where certs is computed, then change the guard to compare certs.count >
originalCount; reference existingCertificates(from:), certs, serverTrust, and
the guard statement to locate and update the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72ae4433-5d78-4e0f-8a48-89df5c2c245a
📒 Files selected for processing (1)
ios/Utilities/AiaCertHelper.swift
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughRefactors TLS/SSL certificate handling (AIA-based chain completion, TrustManager/KeyManager and client P12 support), rewrites retry interceptor control flow and moves tests to a JVM runner, modernizes Android/iOS example projects and build/tooling, updates TypeScript schemas and configs, replaces DocumentPicker in examples, and migrates to an ESLint flat config (deleted .eslintignore). ChangesTLS/SSL: Android and iOS certificate chain & trust wiring
Build, dependencies and platform configs
Example app modernization and UI/tooling updates
Retry logic, streaming reads and test migration
Linting, TypeScript schemas and tooling
Sequence Diagram(s)sequenceDiagram
participant Client as App/JS
participant Network as NetworkClient (Kotlin/Swift)
participant Trust as TrustManager / SecTrust
participant Chain as CertificateChainHelper / AiaCertHelper
participant JS as JS Event Emitter
Client->>Network: Make HTTPS request
Network->>Trust: Validate server certificate
alt Trust validation succeeds
Trust-->>Network: Validation OK
Network->>Client: Proceed with response
else Trust validation fails (recoverable)
Trust->>Chain: Fetch AIA caIssuers & attempt chain completion
Chain-->>Trust: Augmented chain or nil
alt Augmented chain valid
Trust-->>Network: Validation succeeded with augmented chain
Network->>Client: Proceed with response
else Augmentation failed
Network->>JS: emitInvalidCertificateError(chain, cause)
JS-->>Network: (event handled)
Network->>Client: Fail request with certificate error
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
larkox
approved these changes
May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
toWritableMap, removal ofReactApplicationContextfromForwardingCookieHandlerSSL / Certificate Changes (mattermost-mobile issues)
AndroidCAStoreon API 24+ (mattermost-mobile#9337)okhttp-tlsHandshakeCertificateswith standardjavax.net.sslAPIs; removesokhttp-tlsdependencyCLIENT_ERRORevents are deduplicated — only one event fires per session regardless of retries (mattermost-mobile#7658)Content-Typeset on POST request bodies to prevent WAF rejectionsSessionextension properties on session invalidation