chore: Refactor SSLPinning.mm #6663
Open
Conversation
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
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughImplements host-aware PKCS#12 client-certificate provisioning and per-file SSL logging, converts a Challenge method to an instance method, adds a hex utility, reads per-host config from MMKV, swizzles SRWebSocket to apply client SSL options, and adds defensive logging and fallbacks. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant URLSession as URLSession
participant Handler as Challenge (instance)
participant MMKV as MMKV
participant Sec as Security (PKCS#12)
participant Cred as NSURLCredential
App->>URLSession: Request to host
URLSession-->>Handler: didReceiveAuthenticationChallenge (ClientCertificate)
Handler->>Handler: stringToHex(host)
Handler->>MMKV: Read clientSSL JSON by hex(host)
alt Config found
Handler->>Handler: Parse {path,password}
Handler->>Sec: SecPKCS12Import(p12, password)
alt Import success
Sec-->>Handler: identity + cert chain
Handler->>Cred: Create credential(identity, certs)
Handler-->>URLSession: useCredential(Cred)
else Import fails
Handler-->>URLSession: performDefaultHandling
end
else No config
Handler-->>URLSession: performDefaultHandling
end
sequenceDiagram
autonumber
participant WS as SRWebSocket
participant Cat as SRWebSocket(Challenge)
participant MMKV as MMKV
participant Sec as Security (PKCS#12)
participant Stream as CFStream (SSL options)
WS->>Cat: _updateSecureStreamOptions (swizzled)
Cat->>Cat: stringToHex(host)
Cat->>MMKV: Fetch clientSSL JSON by hex(host)
alt Config present
Cat->>Sec: SecPKCS12Import(p12, password)
alt Success
Cat->>Stream: Set kCFStreamSSLCertificates
Cat-->>WS: Continue normal update
else Failure
Cat-->>WS: Skip clientSSL, continue normal update
end
else Missing/invalid
Cat-->>WS: Continue normal update
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/SSLPinning.mm (2)
200-221: CF ownership leaks and weak validation in setClientSSL.keyref is never released; cert copy isn’t balanced; also check import result/count and prefer chain.
- NSDictionary* certOptions = @{ (id)kSecImportExportPassphrase:password }; - CFArrayRef keyref = NULL; - OSStatus sanityChesk = SecPKCS12Import((__bridge CFDataRef)pkcs12data, - (__bridge CFDictionaryRef)certOptions, - &keyref); - if (sanityChesk == noErr) { - CFDictionaryRef identityDict = (CFDictionaryRef)CFArrayGetValueAtIndex(keyref, 0); - SecIdentityRef identityRef = (SecIdentityRef)CFDictionaryGetValue(identityDict, kSecImportItemIdentity); - SecCertificateRef cert = NULL; - OSStatus status = SecIdentityCopyCertificate(identityRef, &cert); - if (!status) { - NSArray *certificates = [[NSArray alloc] initWithObjects:(__bridge id)identityRef, (__bridge id)cert, nil]; - [options setObject:certificates forKey:(NSString *)kCFStreamSSLCertificates]; - } - } + NSDictionary* certOptions = @{ (id)kSecImportExportPassphrase:password }; + CFArrayRef keyref = NULL; + OSStatus importStatus = SecPKCS12Import((__bridge CFDataRef)pkcs12data, + (__bridge CFDictionaryRef)certOptions, + &keyref); + if (importStatus == errSecSuccess && keyref != NULL && CFArrayGetCount(keyref) > 0) { + CFDictionaryRef identityDict = (CFDictionaryRef)CFArrayGetValueAtIndex(keyref, 0); + SecIdentityRef identityRef = (SecIdentityRef)CFDictionaryGetValue(identityDict, kSecImportItemIdentity); + CFArrayRef chainRef = (CFArrayRef)CFDictionaryGetValue(identityDict, kSecImportItemCertChain); + NSArray *certificates = nil; + if (chainRef) { + certificates = [@[ (__bridge id)identityRef ] arrayByAddingObjectsFromArray:(__bridge NSArray *)chainRef]; + } else { + SecCertificateRef cert = NULL; + OSStatus status = SecIdentityCopyCertificate(identityRef, &cert); + if (status == errSecSuccess && cert != NULL) { + certificates = @[ (__bridge id)identityRef, (__bridge id)cert ]; + CFRelease(cert); // balance SecIdentityCopyCertificate + } + } + if (certificates.count > 0) { + [options setObject:certificates forKey:(NSString *)kCFStreamSSLCertificates]; + } + } + if (keyref) { CFRelease(keyref); }
223-231: Swizzle defensively; guard missing selector.If SRWebSocket changes, method_exchangeImplementations(NULL, …) can crash.
- method_exchangeImplementations(original, swizzled); + if (original && swizzled) { + method_exchangeImplementations(original, swizzled); + } else { + os_log_error(SSLLog(), "SRWebSocket swizzle failed: original=%{public}p swizzled=%{public}p", original, swizzled); + }
🧹 Nitpick comments (5)
ios/SSLPinning.mm (5)
28-35: Guard nil and simplify hex encoder.Avoid potential NULL deref on [string UTF8String]; pre-size buffer; avoid extra format.
-+(NSString *)stringToHex:(NSString *)string -{ - char *utf8 = (char *)[string UTF8String]; - NSMutableString *hex = [NSMutableString string]; - while (*utf8) [hex appendFormat:@"%02X", *utf8++ & 0x00FF]; - - return [[NSString stringWithFormat:@"%@", hex] lowercaseString]; -} ++(NSString *)stringToHex:(NSString *)string +{ + if (string.length == 0) { return @""; } + const unsigned char *utf8 = (const unsigned char *)[string UTF8String]; + if (!utf8) { return @""; } + NSMutableString *hex = [NSMutableString stringWithCapacity:string.length * 2]; + while (*utf8) { [hex appendFormat:@"%02x", *utf8++ & 0xFF]; } + return hex; +}
64-71: Improve OSStatus diagnostics.Optional: include SecCopyErrorMessageString for human‑readable errors.
NSString *msg = (__bridge_transfer NSString *)SecCopyErrorMessageString(status, NULL); os_log_error(sslLog, "SecPKCS12Import failed: %d %{public}@", (int)status, msg ?: @"");
79-106: Prefer full certificate chain when available.NSURLCredential supports a chain array; use kSecImportItemCertChain and avoid leaking the copied leaf when using the chain.
SecCertificateRef certificate = NULL; OSStatus certStatus = SecIdentityCopyCertificate(identity, &certificate); if (certStatus != errSecSuccess || certificate == NULL) { os_log_error(sslLog, "SecIdentityCopyCertificate failed: %d", (int)certStatus); if (certificate) CFRelease(certificate); return nil; } - // Build NSArray of certificates (the array should contain certificates, not the identity). - // Some APIs accept an array starting with identity, but NSURLCredential expects - // an array of SecCertificateRef objects (chain). We'll pass the certificate we copied. - id certObj = (__bridge_transfer id)certificate; // certificate will be released by ARC - NSArray *certs = certObj ? @[certObj] : @[]; + // Build chain if provided by SecPKCS12Import; otherwise fall back to the leaf. + NSArray *chain = firstItem[(id)kSecImportItemCertChain]; + NSArray *certs = nil; + if ([chain isKindOfClass:[NSArray class]] && chain.count > 0) { + certs = chain; // contains the leaf already + if (certificate) CFRelease(certificate); + } else { + id certObj = (__bridge_transfer id)certificate; // ARC releases this + certs = certObj ? @[certObj] : @[]; + }
117-118: Non‑atomic static counter used across delegate threads.Minor: seq++ isn’t thread‑safe. Either remove it or make it atomic.
237-261: Harden KVC access and only set SSL settings when populated.Avoid KVC exceptions, NULL/nil mix, unnecessary __block, and no‑op property sets.
- // Read the clientSSL info from MMKV - NSMutableDictionary<NSString *, id> *SSLOptions = [NSMutableDictionary new]; - __block NSString *clientSSL; + // Read the clientSSL info from MMKV + os_log_t sslLog = SSLLog(); + NSMutableDictionary<NSString *, id> *SSLOptions = [NSMutableDictionary new]; + NSString *clientSSL = nil; @@ - if (key != NULL) { + if (key != nil) { NSData *cryptKey = [key dataUsingEncoding:NSUTF8StringEncoding]; MMKV *mmkv = [MMKV mmkvWithID:@"default" cryptKey:cryptKey mode:MMKVMultiProcess]; - NSURLRequest *_urlRequest = [self valueForKey:@"_urlRequest"]; - - clientSSL = [mmkv getStringForKey:_urlRequest.URL.host]; + NSURLRequest *_urlRequest = nil; + @try { _urlRequest = [self valueForKey:@"_urlRequest"]; } @catch (__unused NSException *e) {} + NSString *host = _urlRequest.URL.host; + if (!host.length) { return; } + clientSSL = [mmkv getStringForKey:host]; if (clientSSL) { NSData *data = [clientSSL dataUsingEncoding:NSUTF8StringEncoding]; - id dict = [NSJSONSerialization JSONObjectWithData:data options:0 error:nil]; - NSString *path = [dict objectForKey:@"path"]; - NSString *password = [dict objectForKey:@"password"]; + NSDictionary *dict = [NSJSONSerialization JSONObjectWithData:data options:0 error:nil]; + NSString *path = dict[@"path"]; + NSString *password = dict[@"password"]; [self setClientSSL:path password:password options:SSLOptions]; - if (SSLOptions) { + if (SSLOptions.count > 0) { id _outputStream = [self valueForKey:@"_outputStream"]; [_outputStream setProperty:SSLOptions forKey:(__bridge id)kCFStreamPropertySSLSettings]; + os_log_info(sslLog, "Applied client SSL settings to websocket for host %{public}@", host); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ios/SSLPinning.mm(1 hunks)
🔇 Additional comments (4)
ios/SSLPinning.mm (4)
17-24: os_log helper is fine; ensure header is imported.If not already included via SSLPinning.h, explicitly import os/log.h to avoid build errors on some configs.
Apply this diff near the imports:
#import <objc/runtime.h> #import "SSLPinning.h" +#import <os/log.h> #import <MMKV/MMKV.h>
37-37: Summary mismatch: method remains a class method.AI summary says getUrlCredential became an instance method; in code it’s still a class method. Confirm intent; current call sites rely on class method.
108-110: Credential creation looks good.Correct API usage; persistence None is reasonable for one‑off client auth.
172-174: Default handling fallback is appropriate.Good defensive behavior for unsupported auth methods.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/SSLPinning.mm (2)
206-221: Fix CF leaks and incorrect bridging in SRWebSocket setClientSSL.keyref is never released; cert is created and not released; OSStatus checks are brittle. Use errSecSuccess, CFBridgingRelease, and always CFRelease(keyref).
Apply this patch:
- CFArrayRef keyref = NULL; - OSStatus sanityChesk = SecPKCS12Import((__bridge CFDataRef)pkcs12data, - (__bridge CFDictionaryRef)certOptions, - &keyref); - if (sanityChesk == noErr) { - CFDictionaryRef identityDict = (CFDictionaryRef)CFArrayGetValueAtIndex(keyref, 0); - SecIdentityRef identityRef = (SecIdentityRef)CFDictionaryGetValue(identityDict, kSecImportItemIdentity); - SecCertificateRef cert = NULL; - OSStatus status = SecIdentityCopyCertificate(identityRef, &cert); - if (!status) { - NSArray *certificates = [[NSArray alloc] initWithObjects:(__bridge id)identityRef, (__bridge id)cert, nil]; - [options setObject:certificates forKey:(NSString *)kCFStreamSSLCertificates]; - } - } + CFArrayRef keyref = NULL; + OSStatus status = SecPKCS12Import((__bridge CFDataRef)pkcs12data, + (__bridge CFDictionaryRef)certOptions, + &keyref); + if (status == errSecSuccess && keyref && CFArrayGetCount(keyref) > 0) { + CFDictionaryRef identityDict = (CFDictionaryRef)CFArrayGetValueAtIndex(keyref, 0); + SecIdentityRef identityRef = (SecIdentityRef)CFDictionaryGetValue(identityDict, kSecImportItemIdentity); + SecCertificateRef cert = NULL; + OSStatus certStatus = SecIdentityCopyCertificate(identityRef, &cert); + if (certStatus == errSecSuccess && cert) { + NSArray *certificates = @[ (__bridge id)identityRef, CFBridgingRelease(cert) ]; + [options setObject:certificates forKey:(__bridge NSString *)kCFStreamSSLCertificates]; + } else if (cert) { + CFRelease(cert); + } + } + if (keyref) { CFRelease(keyref); }
257-260: Apply SSL options only when populated and to both streams (not just output).Avoid setting an empty dict (can override defaults) and keep input/output in sync.
- if (SSLOptions) { - id _outputStream = [self valueForKey:@"_outputStream"]; - [_outputStream setProperty:SSLOptions forKey:(__bridge id)kCFStreamPropertySSLSettings]; - } + id certs = SSLOptions[(__bridge NSString *)kCFStreamSSLCertificates]; + if (certs) { + id _outputStream = [self valueForKey:@"_outputStream"]; + id _inputStream = [self valueForKey:@"_inputStream"]; + [_outputStream setProperty:SSLOptions forKey:(__bridge id)kCFStreamPropertySSLSettings]; + [_inputStream setProperty:SSLOptions forKey:(__bridge id)kCFStreamPropertySSLSettings]; + }
♻️ Duplicate comments (1)
ios/SSLPinning.mm (1)
51-54: Thanks for redacting the client-cert path in logs.The private specifier avoids leaking filesystem paths.
🧹 Nitpick comments (8)
ios/SSLPinning.mm (8)
102-107: Consider ForSession credential persistence.If reuse within a session is desired, use NSURLCredentialPersistenceForSession.
Apply this small change:
- persistence:NSURLCredentialPersistenceNone]; + persistence:NSURLCredentialPersistenceForSession];
117-121: Sequence counter isn’t thread-safe.runChallenge can be called concurrently; either drop the counter or make it atomic.
154-158: Improve log context for missing path/password.Include host to aid debugging.
- os_log_error(sslLog, "clientSSL entry missing path/password"); + os_log_error(sslLog, "clientSSL entry missing path/password for host %{public}@", host);
223-231: Guard the SRWebSocket swizzle.Exchange implementations only if both methods exist; log failures to avoid crashes on API drift.
+(void)load { - Method original, swizzled; - - original = class_getInstanceMethod(objc_getClass("SRWebSocket"), @selector(_updateSecureStreamOptions)); - swizzled = class_getInstanceMethod(self, @selector(xxx_updateSecureStreamOptions)); - method_exchangeImplementations(original, swizzled); + Method original = class_getInstanceMethod(objc_getClass("SRWebSocket"), @selector(_updateSecureStreamOptions)); + Method swizzled = class_getInstanceMethod(self, @selector(xxx_updateSecureStreamOptions)); + if (original && swizzled) { + method_exchangeImplementations(original, swizzled); + } else { + os_log_error(SSLLog(), "SRWebSocket swizzle failed: original=%p swizzled=%p", original, swizzled); + } }
237-263: Optional: de-duplicate MMKV clientSSL lookup.The MMKV-by-host retrieval appears in both runChallenge and SRWebSocket; consider extracting a small helper to keep behavior consistent.
125-136: Minor: treat empty key as absent.Check key.length > 0 (not just non-NULL) to avoid creating MMKV with an empty cryptKey.
- if (key == nil) { + if (key.length == 0) { os_log_info(sslLog, "No secure storage key -> default handling"); completionHandler(NSURLSessionAuthChallengePerformDefaultHandling, nil); return; }
245-263: Minor: same empty-key guard in SRWebSocket path.Align with runChallenge and bail if key.length == 0.
- if (key != NULL) { + if (key.length > 0) {
198-221: Optional: add os_log diagnostics in setClientSSL failure paths.Helps triage PKCS#12 import/password issues at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ios/SSLPinning.mm(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (2)
ios/SSLPinning.mm (2)
17-24: LGTM: centralized os_log helper.Good use of dispatch_once and a dedicated subsystem/category.
198-221: Verify SRWebSocket private ivars/methods existEnsure KVC keys _urlRequest, _outputStream, _inputStream and method _updateSecureStreamOptions match your SRWebSocket implementation; local search returned no matches.
Applies to: ios/SSLPinning.mm lines 198–221 and 237–263.#!/bin/bash set -euo pipefail # Run in repo root rg -n --hidden -S -C3 '(@interface\s+SRWebSocket\b|SRWebSocket\b|_urlRequest|_outputStream|_inputStream|_updateSecureStreamOptions)' -g '*.h' -g '*.m' -g '*.mm' || true
Comment on lines
+28
to
+35
| +(NSString *)stringToHex:(NSString *)string | ||
| { | ||
| NSString *authMethod = [[challenge protectionSpace] authenticationMethod]; | ||
| SecTrustRef serverTrust = challenge.protectionSpace.serverTrust; | ||
| char *utf8 = (char *)[string UTF8String]; | ||
| NSMutableString *hex = [NSMutableString string]; | ||
| while (*utf8) [hex appendFormat:@"%02X", *utf8++ & 0x00FF]; | ||
|
|
||
| if ([authMethod isEqualToString:NSURLAuthenticationMethodServerTrust] || path == nil || password == nil) { | ||
| return [NSURLCredential credentialForTrust:challenge.protectionSpace.serverTrust]; | ||
| } else if (path && password) { | ||
| NSMutableArray *policies = [NSMutableArray array]; | ||
| [policies addObject:(__bridge_transfer id)SecPolicyCreateSSL(true, (__bridge CFStringRef)challenge.protectionSpace.host)]; | ||
| SecTrustSetPolicies(serverTrust, (__bridge CFArrayRef)policies); | ||
| return [[NSString stringWithFormat:@"%@", hex] lowercaseString]; | ||
| } |
There was a problem hiding this comment.
Fix nil-safety in stringToHex to avoid NULL deref.
If string is nil, while(*utf8) dereferences NULL. Use NSData bytes and guard.
Apply this diff:
-+(NSString *)stringToHex:(NSString *)string
-{
- char *utf8 = (char *)[string UTF8String];
- NSMutableString *hex = [NSMutableString string];
- while (*utf8) [hex appendFormat:@"%02X", *utf8++ & 0x00FF];
-
- return [[NSString stringWithFormat:@"%@", hex] lowercaseString];
-}
++(NSString *)stringToHex:(NSString *)string
+{
+ if (string.length == 0) { return @""; }
+ NSData *data = [string dataUsingEncoding:NSUTF8StringEncoding];
+ const unsigned char *bytes = (const unsigned char *)data.bytes;
+ NSMutableString *hex = [NSMutableString stringWithCapacity:data.length * 2];
+ for (NSUInteger i = 0; i < data.length; i++) {
+ [hex appendFormat:@"%02x", bytes[i]];
+ }
+ return hex;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| +(NSString *)stringToHex:(NSString *)string | |
| { | |
| NSString *authMethod = [[challenge protectionSpace] authenticationMethod]; | |
| SecTrustRef serverTrust = challenge.protectionSpace.serverTrust; | |
| char *utf8 = (char *)[string UTF8String]; | |
| NSMutableString *hex = [NSMutableString string]; | |
| while (*utf8) [hex appendFormat:@"%02X", *utf8++ & 0x00FF]; | |
| if ([authMethod isEqualToString:NSURLAuthenticationMethodServerTrust] || path == nil || password == nil) { | |
| return [NSURLCredential credentialForTrust:challenge.protectionSpace.serverTrust]; | |
| } else if (path && password) { | |
| NSMutableArray *policies = [NSMutableArray array]; | |
| [policies addObject:(__bridge_transfer id)SecPolicyCreateSSL(true, (__bridge CFStringRef)challenge.protectionSpace.host)]; | |
| SecTrustSetPolicies(serverTrust, (__bridge CFArrayRef)policies); | |
| return [[NSString stringWithFormat:@"%@", hex] lowercaseString]; | |
| } | |
| (NSString *)stringToHex:(NSString *)string | |
| { | |
| if (string.length == 0) { return @""; } | |
| NSData *data = [string dataUsingEncoding:NSUTF8StringEncoding]; | |
| const unsigned char *bytes = (const unsigned char *)data.bytes; | |
| NSMutableString *hex = [NSMutableString stringWithCapacity:data.length * 2]; | |
| for (NSUInteger i = 0; i < data.length; i++) { | |
| [hex appendFormat:@"%02x", bytes[i]]; | |
| } | |
| return hex; | |
| } |
🤖 Prompt for AI Agents
In ios/SSLPinning.mm around lines 28-35, stringToHex currently dereferences a
NULL when string is nil by using UTF8String and while(*utf8); change to guard
against nil and use NSData bytes: if string is nil return @"" (or nil per
project convention), obtain NSData *data = [string
dataUsingEncoding:NSUTF8StringEncoding], get const unsigned char *bytes =
data.bytes and iterate for (NSUInteger i=0;i<data.length;i++) appending each
byte with %02X, then return the lowercased hex string; ensure you handle empty
data and avoid dereferencing NULL bytes.
Comment on lines
+73
to
101
| NSArray *items = (__bridge_transfer NSArray *)rawItems; | ||
| if (items.count == 0) { | ||
| os_log_error(sslLog, "PKCS12 import returned zero items"); | ||
| return nil; | ||
| } | ||
|
|
||
| [SDWebImageDownloader sharedDownloader].config.urlCredential = [NSURLCredential credentialWithIdentity:identity certificates:certificates persistence:NSURLCredentialPersistenceNone]; | ||
| NSDictionary *firstItem = items[0]; | ||
| id identityObj = firstItem[(id)kSecImportItemIdentity]; | ||
| if (!identityObj) { | ||
| os_log_error(sslLog, "No identity found in PKCS12"); | ||
| return nil; | ||
| } | ||
|
|
||
| return [NSURLCredential credentialWithIdentity:identity certificates:certificates persistence:NSURLCredentialPersistenceNone]; | ||
| SecIdentityRef identity = (__bridge SecIdentityRef)identityObj; | ||
| // Copy certificate from identity | ||
| SecCertificateRef certificate = NULL; | ||
| OSStatus certStatus = SecIdentityCopyCertificate(identity, &certificate); | ||
| if (certStatus != errSecSuccess || certificate == NULL) { | ||
| os_log_error(sslLog, "SecIdentityCopyCertificate failed: %d", (int)certStatus); | ||
| if (certificate) CFRelease(certificate); | ||
| return nil; | ||
| } | ||
|
|
||
| return [NSURLCredential credentialForTrust:challenge.protectionSpace.serverTrust]; | ||
| } | ||
| // Build NSArray of certificates (the array should contain certificates, not the identity). | ||
| // Some APIs accept an array starting with identity, but NSURLCredential expects | ||
| // an array of SecCertificateRef objects (chain). We'll pass the certificate we copied. | ||
| id certObj = (__bridge_transfer id)certificate; // certificate will be released by ARC | ||
| NSArray *certs = certObj ? @[certObj] : @[]; | ||
|
|
There was a problem hiding this comment.
Use PKCS#12-provided cert chain (kSecImportItemCertChain); avoid manual SecIdentityCopyCertificate.
NSURLCredential’s certificates parameter should include the chain from the import; copying only the leaf can break client-auth on servers expecting intermediates.
Apply this refactor:
- SecIdentityRef identity = (__bridge SecIdentityRef)identityObj;
- // Copy certificate from identity
- SecCertificateRef certificate = NULL;
- OSStatus certStatus = SecIdentityCopyCertificate(identity, &certificate);
- if (certStatus != errSecSuccess || certificate == NULL) {
- os_log_error(sslLog, "SecIdentityCopyCertificate failed: %d", (int)certStatus);
- if (certificate) CFRelease(certificate);
- return nil;
- }
-
- // Build NSArray of certificates (the array should contain certificates, not the identity).
- // Some APIs accept an array starting with identity, but NSURLCredential expects
- // an array of SecCertificateRef objects (chain). We'll pass the certificate we copied.
- id certObj = (__bridge_transfer id)certificate; // certificate will be released by ARC
- NSArray *certs = certObj ? @[certObj] : @[];
+ SecIdentityRef identity = (__bridge SecIdentityRef)identityObj;
+ // Prefer the full chain provided by SecPKCS12Import (excluding the identity).
+ id chainObj = firstItem[(id)kSecImportItemCertChain];
+ NSArray *certs = ([chainObj isKindOfClass:[NSArray class]] && [chainObj count] > 0) ? (NSArray *)chainObj : nil;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| NSArray *items = (__bridge_transfer NSArray *)rawItems; | |
| if (items.count == 0) { | |
| os_log_error(sslLog, "PKCS12 import returned zero items"); | |
| return nil; | |
| } | |
| [SDWebImageDownloader sharedDownloader].config.urlCredential = [NSURLCredential credentialWithIdentity:identity certificates:certificates persistence:NSURLCredentialPersistenceNone]; | |
| NSDictionary *firstItem = items[0]; | |
| id identityObj = firstItem[(id)kSecImportItemIdentity]; | |
| if (!identityObj) { | |
| os_log_error(sslLog, "No identity found in PKCS12"); | |
| return nil; | |
| } | |
| return [NSURLCredential credentialWithIdentity:identity certificates:certificates persistence:NSURLCredentialPersistenceNone]; | |
| SecIdentityRef identity = (__bridge SecIdentityRef)identityObj; | |
| // Copy certificate from identity | |
| SecCertificateRef certificate = NULL; | |
| OSStatus certStatus = SecIdentityCopyCertificate(identity, &certificate); | |
| if (certStatus != errSecSuccess || certificate == NULL) { | |
| os_log_error(sslLog, "SecIdentityCopyCertificate failed: %d", (int)certStatus); | |
| if (certificate) CFRelease(certificate); | |
| return nil; | |
| } | |
| return [NSURLCredential credentialForTrust:challenge.protectionSpace.serverTrust]; | |
| } | |
| // Build NSArray of certificates (the array should contain certificates, not the identity). | |
| // Some APIs accept an array starting with identity, but NSURLCredential expects | |
| // an array of SecCertificateRef objects (chain). We'll pass the certificate we copied. | |
| id certObj = (__bridge_transfer id)certificate; // certificate will be released by ARC | |
| NSArray *certs = certObj ? @[certObj] : @[]; | |
| NSArray *items = (__bridge_transfer NSArray *)rawItems; | |
| if (items.count == 0) { | |
| os_log_error(sslLog, "PKCS12 import returned zero items"); | |
| return nil; | |
| } | |
| NSDictionary *firstItem = items[0]; | |
| id identityObj = firstItem[(id)kSecImportItemIdentity]; | |
| if (!identityObj) { | |
| os_log_error(sslLog, "No identity found in PKCS12"); | |
| return nil; | |
| } | |
| SecIdentityRef identity = (__bridge SecIdentityRef)identityObj; | |
| // Prefer the full chain provided by SecPKCS12Import (excluding the identity). | |
| id chainObj = firstItem[(id)kSecImportItemCertChain]; | |
| NSArray *certs = ([chainObj isKindOfClass:[NSArray class]] && [chainObj count] > 0) ? (NSArray *)chainObj : nil; |
🤖 Prompt for AI Agents
In ios/SSLPinning.mm around lines 73 to 101, the code copies only the leaf
certificate via SecIdentityCopyCertificate and builds certs with a single cert,
which drops the PKCS#12-provided chain; replace that logic to read
kSecImportItemCertChain from firstItem, bridge it to an NSArray/CFArrayRef of
SecCertificateRef objects and use that chain for the NSURLCredential
certificates parameter (do not call SecIdentityCopyCertificate), preserving
correct CF/ARC memory management when bridging the cert chain.
OtavioStasiak
requested changes
Sep 17, 2025
| #import "SecureStorage.h" | ||
| #import "SRWebSocket.h" | ||
| #import "EXSessionTaskDispatcher.h" | ||
|
|
There was a problem hiding this comment.
missing #include <os/log.h> import
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Proposed changes
This PR fixes critical issues in the iOS SSL pinning implementation by improving certificate handling, error management, and logging in the
SSLPinning.mmfile.📋 Key Changes
Enhanced Certificate Credential Creation
getUrlCredentialmethod: Added comprehensive error handling and validation for PKCS12 certificate processingNSURLCredentialcreationRobust Challenge Processing
runChallengemethod: Improved URL authentication challenge handling with better error reportingos_logstatements throughout the certificate processing flow for better debuggingTechnical Improvements
🔧 Specific Fixes
🚀 Benefits
🧪 Testing Focus
This change specifically affects:
The fix maintains full backward compatibility while providing more reliable SSL certificate handling for iOS users.
Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1023
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Improvements
Bug Fixes