Skip to content

Feature/ios optional approov plist - #32

Open
ivolz wants to merge 14 commits into
mainfrom
feature/ios-optional-approov-plist
Open

Feature/ios optional approov plist#32
ivolz wants to merge 14 commits into
mainfrom
feature/ios-optional-approov-plist

Conversation

@ivolz

@ivolz ivolz commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Ships as 3.5.16 (last published: 3.5.15). Related changes to the React Native service layer, on both Android and iOS:

  1. feat — select the Approov request policy from JavaScript (setServiceMutatorType), no native mutator code.
  2. fix — the custom service mutator now resets on re-initialization (Android + iOS).
  3. fix — iOS approov.plist is now optional (no launch crash with a config-only native integration).
  4. hardening + test-gap fixes — from review; see "Review hardening" below.

Feature — ApproovService.setServiceMutatorType(mask, { sign })

  • New JS API + ApproovService.ReturnDecision (one bit per Approov failure status) + ApproovService.MutatorPreset presets (DEFAULT, ALWAYS_PROCEED, PROCEED_IF_UNAVAILABLE, PROCEED_DEV_CLEARTEXT).
  • The mask lists which failure statuses may proceed; a not-in-mask failure status blocks the request. SUCCESS / UNKNOWN_URL / UNPROTECTED_URL always proceed and are not maskable.
  • HTTP Message Signing preserved by default; { sign: false } opts out.
  • Backed by a native PolicyMutator on each platform (Android Java, iOS Swift), composing the default message signer. Behaviour is identical across platforms — a blocked status is a hard fetch() failure on both (Android IOException, iOS NSError).
  • No native app code needed for the common policy cases (previously required hand-writing an ApproovServiceMutator).
  • ⚠️ Security: putting MITM_DETECTED or REJECTED in the mask disables the protection those statuses provide — use presets by default; reach for the raw bitmask deliberately.
  • PROCEED_DEV_CLEARTEXT is the supported dev-only opt-in for the Metro cleartext BAD_URL symptom (BAD_URL token status crashes network requests instead of being handled gracefully #30).

Fix — service mutator reset on re-initialization (Android + iOS)

initialize() cleared the runtime config on a config change but not the active service mutator, so a mutator installed via setServiceMutator/setServiceMutatorType persisted across an initialization boundary. It now resets to the built-in message-signing default on both platforms (root TESTING_REQUIREMENTS §2, "Service Mutator Reset").

Every successful initialization is a reset boundary, including same-config re-initialization. Same-config re-init is forwarded to the native SDK, treated as success, and then resets runtime configuration and the active service mutator — it no longer preserves state. A warning is logged whenever a custom mutator is discarded this way. Consumers must re-apply setServiceMutatorType (and any substitution headers, exclusions, or token-header settings) after each successful initialize(). In ApproovProvider, apply the policy in an effect keyed on approovReady rather than in onInit, which runs before initialize() and would be wiped by the reset.

Fix — iOS approov.plist optional

With native approov.config init, a missing bundled approov.plist no longer raises ApproovPropsNotFound at launch — it logs and runs with defaults, matching Android's optional approov.props. A present-but-unreadable plist still raises. (Reported by PropertyGuru.)

Review hardening

Mask validation (security-relevant). The proceed bitmask is bridged from JS as a double and is now rejected on both platforms unless it is a finite, integral 32-bit value and sets no bit outside the defined flags (bits 0-10, exposed as PolicyMutator.ALL_BITS), with MutatorPreset.DEFAULT (-1) excepted. Previously 1 << 11 validated fine and installed a policy granting proceed to nothing — a silent block-everything policy indistinguishable from a deliberate one. Rejecting undefined bits is a user-visible contract change, so REFERENCE.md and USAGE.md are updated.

Stale mutator across re-init (Android, security-relevant). ApproovService.serviceMutator is now volatile. The reset wrote it under the instance monitor, but the interceptor reads it through the static unsynchronized getServiceMutator(), so that lock ordered nothing for those reads.

iOS Swift was never compiled against the real SDK. Every Swift suite compiled against a hand-written ApproovStub, so none could catch an ApproovTokenFetchStatus case name absent from the shipping SDK — and since PolicyMutator.bitFor/decideFor both end in default:, such a name would compile and then silently degrade to "no bit → BLOCK" at runtime. New tests/ios/run_real_sdk_typecheck.sh compiles the same sources against the real Approov.xcframework and runs as part of npm run test:ios. Existing status handling verified correct against SDK 3.5.3 (0 errors), including the importer renames (.mitmDetected, .badURL). Gate verified to fail on an injected wrong case name.

Mutator reset tests strengthened. The Android reset test installed ApproovServiceMutator.DEFAULT and asserted instanceof ApproovDefaultMessageSigning, which PolicyMutator also satisfies — so it could not fail. It now installs a real PolicyMutator and asserts the negative. Same-config counter-tests added on both platforms, and the iOS bridge stub now records resetToDefault instead of swallowing it, so the iOS initialize() reset path is asserted rather than assumed.

Versioning

Patch 3.5.16 — all changes ship together (last published 3.5.15). package.json, podspec (s.version = package["version"]) and Package.swift (git-tag) are consistent; the release tag will be 3.5.16.

Testing

All run locally on this branch:

  • Android: npm run test:android59 tests, 0 failures.
  • iOS: npm run test:iosexit 0, all five suites: mini-SDK native, legacy native, Swift message-signing, Swift PolicyMutator, real-SDK typecheck.
  • JS: npm run test:js — 15/15.
  • The worker-backed run_mini_sdk_native_tests.sh has now been run locally against the real Approov SDK (3.5.3(7361)), resolving the earlier caveat in this description. It requires core-service-layers-testing present as a sibling directory, which CI provides.

Related

ivolz added 4 commits July 22, 2026 11:58
The iOS native module no longer raises ApproovPropsNotFound at launch when a
bundled approov.plist is absent. With native approov.config-based init this
made the properties file mandatory on iOS while Android treats approov.props
as optional. It now logs and runs with default properties (leaving _props nil,
which is safe: all consumers nil-guard and -valueForKey: on nil returns nil).
A present-but-unreadable plist still raises (genuine misconfiguration). Also
fixes a nil-pointer log when the file is absent.

Reported by PropertyGuru (approov.config-only native integration crashed
without an empty placeholder approov.plist).
initialize() reset the token/trace headers, substitution maps, exclusion regexes and other runtime state when the config changed, but never reset the custom service mutator. A mutator installed via setServiceMutator (or the JS setServiceMutatorType wrapper) therefore persisted across an initialization boundary — violating root TESTING_REQUIREMENTS.md section 2 "Service Mutator Reset" and diverging from approov-service-okhttp, which resets the mutator to the default on init.

Reset the mutator to a fresh ApproovDefaultMessageSigning (the React Native default, which performs HTTP message signing) inside the config-changed reset block, rather than the no-signing ApproovServiceMutator.DEFAULT that would silently disable default signing on every re-init. Same-config re-inits (React remounts / StrictMode / Fast Refresh) intentionally preserve runtime state and are unaffected.

Adds a regression test (initializeWithDifferentConfigResetsCustomServiceMutator). Pre-existing defect, independent of the prebuilt-mutator feature work. iOS needs the equivalent reset when the Android changes are ported.
The ObjC initialize() reset block cleared token/trace headers, substitution maps and exclusion regexes on a config change but never reset the active service mutator, so a custom mutator installed via ApproovServiceMutatorBridge persisted across an initialization boundary — violating root TESTING_REQUIREMENTS.md section 2 "Service Mutator Reset". This is the iOS counterpart of the Android fix (commit 48ee5b6).

Adds ApproovServiceMutatorBridge.resetToDefault() (an @objc helper, since the Swift-typed serviceMutator cannot be assigned from Objective-C) and calls it in the config-changed reset block, restoring a fresh ApproovDefaultMessageSigning (the RN default, which performs message signing). Same-config re-inits intentionally preserve state and are unaffected.
Add ApproovService.setServiceMutatorType(mask, { sign }) plus the ApproovService.ReturnDecision status flags and ApproovService.MutatorPreset presets, so an app can choose the per-status proceed/block policy — and whether the outbound request is HTTP Message Signed — entirely from JavaScript, with no native mutator code.

Android: PolicyMutator (bitmask-driven, composes ApproovDefaultMessageSigning) + setServiceMutatorType @ReactMethod. iOS: Swift PolicyMutator + ApproovServiceMutatorBridge.setPolicyMutator + RCT_EXPORT_METHOD setServiceMutatorType, behaviour identical to Android — a not-in-mask failure status blocks the request via ApproovServiceError.permanentError, the same hard failure as Android's IOException. JS: ReturnDecision flags + MutatorPreset presets (DEFAULT, ALWAYS_PROCEED, PROCEED_IF_UNAVAILABLE, PROCEED_DEV_CLEARTEXT). Message signing preserved by default; { sign: false } opts out.

Docs: USAGE.md, REFERENCE.md, CHANGELOG (3.5.16), including the security caveat that proceeding on MITM_DETECTED / REJECTED disables that protection. Tests: Android PolicyMutatorTest (JVM); iOS PolicyMutatorTests (Swift) + run_policy_mutator_tests.sh. PROCEED_DEV_CLEARTEXT is the supported opt-in for the Metro dev BAD_URL symptom (#30).

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR (targeting release 3.5.16) extends the React Native Approov service layer with a JS-selectable request policy mutator (bitmask + presets), fixes service-mutator reset behavior across config-change re-initializations, and makes the iOS bundled approov.plist optional to avoid launch crashes for config-only native integrations.

Changes:

  • Add ApproovService.setServiceMutatorType(mask, { sign }) plus ReturnDecision bit flags and MutatorPreset presets, backed by new native PolicyMutator implementations on Android and iOS.
  • Reset the active service mutator back to the built-in message-signing default when initialize() is called with a genuinely different config (Android + iOS).
  • Make iOS approov.plist optional (missing file logs + defaults; unreadable file still raises), and add/extend native/unit test coverage and documentation.

Reviewed changes

Copilot reviewed 20 out of 22 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
USAGE.md Documents JS-driven policy selection (presets, raw bitmask, signing option) and re-init reset semantics.
REFERENCE.md Adds API reference for setServiceMutatorType, ReturnDecision, and MutatorPreset.
CHANGELOG.md Adds 3.5.16 release notes for the new policy API and the two fixes.
package.json Bumps package version to 3.5.16.
package-lock.json Updates lockfile version fields to 3.5.16.
index.js Adds Proxy wrapper for setServiceMutatorType and exports ReturnDecision/MutatorPreset.
index.d.ts Types the new JS API and the added constants.
android/src/main/java/io/approov/reactnative/ApproovService.java Adds native setServiceMutatorType method and resets mutator on config-change re-init.
android/src/main/java/io/approov/reactnative/PolicyMutator.java New Android native mutator implementing proceed/forward/block policy + optional signing.
android/src/test/java/io/approov/reactnative/PolicyMutatorTest.java New unit tests for Android PolicyMutator decision logic and signing behavior.
android/src/test/java/io/approov/reactnative/ApproovServiceRegressionTest.java Adds regression test to ensure mutator resets on config-change re-init.
ios/ApproovProps.m Makes missing approov.plist optional (log + defaults) instead of raising at launch.
ios/ApproovService.m Resets mutator on config-change re-init and adds exported setServiceMutatorType bridge method.
ios/ApproovServiceMutatorBridge.swift Adds ObjC-callable helpers to install PolicyMutator and reset to default signer.
ios/ApproovURLSession/PolicyMutator.swift New Swift PolicyMutator implementing bitmask decision policy + optional signing via composition.
tests/ios/swift/TestSupport/ApproovStub.swift Extends Swift stub statuses to cover DISABLED and INTERNAL_ERROR.
tests/ios/swift/PolicyMutatorTests.swift New Swift test runner validating bit assignments, blocking semantics, signing delegation, and bridge behavior.
tests/ios/run_policy_mutator_tests.sh New script compiling/running the Swift PolicyMutator test runner via swiftc.
tests/ios/run_message_signing_tests.sh Includes PolicyMutator.swift in the message signing swiftc compilation set.
tests/ios/run_native_tests.sh Adds the Swift PolicyMutator suite to the iOS native test runner.
tests/ios/native/TestSupport/ApproovServiceMutatorBridgeStub.m Adds no-op ObjC stub selectors for new Swift bridge helpers to keep native suites linking.
tests/ios/native/TestSupport/approov_service_react_native-Swift.h Adds stub header declarations for setPolicyMutator and resetToDefault.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread android/src/main/java/io/approov/reactnative/ApproovService.java
Comment thread ios/ApproovService.m
The proceed bitmask is bridged from JS as a double and was narrowed to
int (Android) / NSInteger (iOS) with an unchecked cast. A fractional,
NaN/Infinity, or out-of-32-bit-range value was silently truncated or
coerced and could install a proceed policy other than the caller's
intent (security-relevant: the mask decides which failure statuses may
proceed). Reject such masks before applying the mutator on both
platforms. Adds Android and iOS native regression tests.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 24 changed files in this pull request and generated 1 comment.

Comment thread ios/ApproovURLSession/PolicyMutator.swift
PolicyMutator's block error interpolated the Swift enum case via
\(status); Approov.string(from:) yields the canonical MITM_DETECTED-style
name, matching the rest of the iOS layer and Android's enum toString().
Also reword the MITM_DETECTED/REJECTED mask security warning in
REFERENCE.md, USAGE.md and CHANGELOG.md to state the request proceeds
without proof of attestation (no valid Approov token). (Copilot review,
PR #32.)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 24 changed files in this pull request and generated 2 comments.

Comment thread ios/ApproovProps.m
Comment thread index.js
index.js: options = {} only defaults on undefined; an explicit null
(setServiceMutatorType(mask, null)) threw on options.sign. Treat null
like no options via (options || {}), so sign defaults to true.

ios/ApproovProps.m: correct the optional-props comment - the accessor
is -objectForKey: sent to the nil _props dictionary, not -valueForKey:.

(Copilot review, PR #32.)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 24 changed files in this pull request and generated 1 comment.

Comment on lines +323 to +347
public void initializeWithDifferentConfigResetsCustomServiceMutator() {
ApproovService service = newService();

Promise firstInit = mock(Promise.class);
service.initialize("config-one", null, firstInit);
verify(firstInit, timeout(2000)).resolve(null);

// Install a custom, non-signing mutator (as an app would via setServiceMutator,
// or the JS setServiceMutatorType wrapper).
ApproovServiceMutator custom = ApproovServiceMutator.DEFAULT;
ApproovService.setServiceMutator(custom);
assertSame(custom, ApproovService.getServiceMutator());

// A genuinely different config must reset the mutator so a custom override does
// not persist across an initialization boundary (root TESTING_REQUIREMENTS.md
// section 2, "Service Mutator Reset").
Promise secondInit = mock(Promise.class);
service.initialize("config-two", null, secondInit);
verify(secondInit, timeout(2000)).resolve(null);

ApproovServiceMutator afterReset = ApproovService.getServiceMutator();
assertNotSame("custom mutator must not persist across a config change", custom, afterReset);
assertTrue("re-init must restore the default message-signing mutator",
afterReset instanceof ApproovDefaultMessageSigning);
}
Prevent custom mutator installed by one test leaking into later tests
if an assertion fails before the re-init step resets it. Flagged by
Copilot review on PR #32.
@ivolz
ivolz requested a review from charlesoj6205 July 27, 2026 14:03

@charlesoj6205 charlesoj6205 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking — no iOS Swift is compiled against the real Approov SDK, so the new PolicyMutator is unverified. CI being green doesn't cover this. I went through all four suites run_native_tests.sh invokes:

run_mini_sdk_native_tests.sh — ObjC only: mini-SDK Approov.m plus ApproovServiceMutatorBridgeStub.m. No Swift at all.
run_legacy_native_tests.sh — ObjC + stubs.
run_message_signing_tests.sh and run_policy_mutator_tests.sh — swiftc against tests/ios/swift/TestSupport/ApproovStub.swift, which this PR extends (+.disabled, +.internalError) to make the suite build.
npm run build:ios wouldn't help either (and CI doesn't run it): ios/Approov.xcodeproj lists zero .swift files and has no file-system-synchronized group, so it builds no Swift — pre-existing, but it means the podspec glob is the only thing that ever compiles these files.

Net effect: every ApproovTokenFetchStatus case name in bitFor/decideFor is unchecked against the real module, and both switches end in default:, so a wrong-but-plausible spelling compiles against the stub and silently degrades to "no bit → BLOCK" at runtime. approov-ios-sdk 3.5.3 is public, so a swiftc -typecheck with -F against the real xcframework clears this in minutes — or pod lib lint for the thorough version. Worth adding to CI while you're there.

Four cheap asks, all on lines you've already touched:

serviceMutator (ApproovService.java:204) needs to be volatile. The non-volatile field is pre-existing, but the new reset writes it under synchronized (this) with a comment claiming that lock covers the interceptor's reads — it doesn't, they go through the static unsynchronized getServiceMutator(). A stale mutator can survive a config-change re-init.
initializeWithDifferentConfigResetsCustomServiceMutator can't fail: PolicyMutator extends ApproovDefaultMessageSigning, so the instanceof assertion passes even if the custom mutator survived. It also installs ApproovServiceMutator.DEFAULT rather than a PolicyMutator, so the CHANGELOG's scenario is untested.
No iOS test that initialize() does the reset — the ObjC suite stubs resetToDefault as a no-op, so ApproovService.m:436 is uncovered while Android has a test. Needs the stub to record the call rather than swallow it.
setServiceMutatorType(1 << 11) validates fine and installs a silent block-everything policy. Worth rejecting bits outside 0..10 now that you validate the mask's representation.

Address the blocking review on PR #32.

setServiceMutatorType now rejects masks with undefined bits on both
platforms. Only bits 0-10 name an Approov status, so a mask such as
1 << 11 granted proceed to nothing and installed a silent
block-everything policy that looked deliberate. The permitted set is
exposed as PolicyMutator.ALL_BITS.

ApproovService.serviceMutator is now volatile. The re-initialization
reset wrote it under the instance monitor, but the interceptor reads it
through the static unsynchronized getServiceMutator(), so that lock
ordered nothing for those reads and a stale custom mutator could survive
a config change.

Add tests/ios/run_real_sdk_typecheck.sh and wire it into the iOS runner.
Every Swift suite compiled against ApproovStub.swift, so none could
catch an ApproovTokenFetchStatus case name absent from the shipping SDK;
since bitFor/decideFor both end in default:, such a name would compile
and then degrade to "no bit -> BLOCK" at runtime. The gate compiles the
same sources against the real Approov.xcframework. Existing status
handling verified correct against SDK 3.5.3.

Fix the mutator reset tests. The Android test installed
ApproovServiceMutator.DEFAULT and asserted instanceof
ApproovDefaultMessageSigning, which PolicyMutator also satisfies, so it
could not fail; it now installs a PolicyMutator and asserts the negative.
Add same-config counter-tests on both platforms, and make the iOS bridge
stub record resetToDefault instead of swallowing it so the iOS
initialize() reset path is asserted rather than assumed.
@ivolz

ivolz commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @charlesoj6205 — all five addressed in 47bdc4a. Taking them in your order.

1. Blocking: no iOS Swift compiled against the real Approov SDK

You were right that nothing verified this, and your reading of the four suites matches what I found. Added tests/ios/run_real_sdk_typecheck.sh, wired into run_native_tests.sh (so it runs under npm run test:ios, and therefore in CI).

It resolves a real Approov.xcframework in three steps — $APPROOV_XCFRAMEWORK, then the local CocoaPods cache, then downloading the public release zip — picks the simulator slice, and runs swiftc -typecheck over the same Swift sources the other suites compile. ApproovStub.swift is deliberately not compiled, so import Approov resolves to the real framework module. RawStructuredFieldValuesStub stays, since it stands in for a genuine third-party dependency rather than for Approov.

Result: 0 errors against SDK 3.5.3. Every ApproovTokenFetchStatus case name in bitFor/decideFor is correct, including the importer renames that worried me most (ApproovTokenFetchStatusMITMDetected.mitmDetected, ...BadURL.badURL). So the process gap was real but there was no live bug behind it.

I verified the gate actually catches the failure mode rather than trusting that it would. Renaming a case consistently across both ApproovStub.swift and PolicyMutator.swift — the scenario where stub and production code agree on a name the SDK does not have — produces:

PolicyMutator.swift:156:15: error: type 'ApproovTokenFetchStatus' has no member 'mitmDetectedX'

I did not add pod lib lint to CI. It needs a full pod install and is much slower; the typecheck closes the specific hole for a few seconds of wall clock. Happy to add it too if you want the thorough version.

2. serviceMutator needs to be volatile

Correct, and your diagnosis of why is exactly right — the reset writes under synchronized (this), but the interceptor reads through the static unsynchronized getServiceMutator(), so that monitor orders nothing for those reads. Now volatile, with a comment recording the reason so it does not get dropped again.

3. initializeWithDifferentConfigResetsCustomServiceMutator can't fail

Both halves were fair. The test now installs a real PolicyMutator — the type the JS API actually installs, so the CHANGELOG scenario is the one under test — and asserts the negative explicitly, since PolicyMutator extends ApproovDefaultMessageSigning makes the instanceof assertion alone unfalsifiable:

assertFalse("re-init must not leave a PolicyMutator installed",
    afterReset instanceof PolicyMutator);

Also added initializeWithSameConfigPreservesCustomServiceMutator as a counter-test, so a reset that fired unconditionally would fail rather than pass both.

4. No iOS test that initialize() performs the reset

The stub now records resetToDefault and setPolicyMutator instead of swallowing them, exposed through ApproovMutatorBridgeResetToDefaultCount() and friends (all cleared by the existing ApproovMutatorBridgeReset()). New TestInitializeResetsServiceMutatorOnConfigChange in ApproovNativeTests.m asserts the mask and sign flag reach the bridge, that a same-config re-init does not reset, and that a different-config re-init does — so ApproovService.m:436 is covered on both sides of the branch.

5. setServiceMutatorType(1 << 11) installs a silent block-everything policy

Fixed on both platforms. Added PolicyMutator.ALL_BITS (Java and Swift) as the permitted set, and both bridges now reject any mask with bits outside it, MutatorPreset.DEFAULT (-1) excepted. iOS duplicates the constant locally rather than reading it from the Swift type, so the ObjC suites that link the stub bridge still compile — the comment says so.

Rejecting undefined bits is a user-visible contract change, so REFERENCE.md, USAGE.md and CHANGELOG.md are updated too.

Verification

Everything below was run locally on this branch, including the mini-SDK suite against the real SDK (3.5.3(7361)), which needed core-service-layers-testing present as a sibling:

  • npm run test:ios — exit 0, all five suites: mini-SDK native, legacy native, Swift message-signing, Swift PolicyMutator, real-SDK typecheck
  • npm run test:android — 59 tests, 0 failures (was 57)
  • npm run test:js — 15/15
  • CI green on 47bdc4a

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

tests/ios/run_real_sdk_typecheck.sh:105

  • run_real_sdk_typecheck.sh hard-codes TARGET="arm64-apple-ios13.0-simulator". This can break developers/CI running on Intel macOS (x86_64), where swiftc may not be able to typecheck for an arm64 simulator target. Consider selecting the simulator target dynamically based on host architecture so the gate is portable across macOS runners.
SDK_PATH="$(xcrun --sdk iphonesimulator --show-sdk-path)"
TARGET="arm64-apple-ios13.0-simulator"

@ivolz

ivolz commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Re the suppressed comment on tests/ios/run_real_sdk_typecheck.sh:105 (hardcoded TARGET="arm64-apple-ios13.0-simulator"): not acting on this one, because the stated mechanism does not hold.

The comment argues that on Intel macOS swiftc "may not be able to typecheck for an arm64 simulator target". That conflates host architecture with target architecture. Swift's -target selection is independent of the host — that is how an arm64 simulator slice gets built on an Intel machine in the first place — and -typecheck does no codegen or linking at all, so there is nothing arch-specific to fail. Verified both directions resolve from this host:

$ xcrun swiftc -typecheck -target x86_64-apple-ios13.0-simulator ... # OK
$ xcrun swiftc -typecheck -target arm64-apple-ios13.0-simulator ... # OK

Two further reasons it is not worth the branch:

  • The xcframework slice the script selects, ios-arm64_x86_64-simulator, is universal, so slice resolution is arch-independent too.
  • GitHub's macos-latest has been arm64 since macos-14, so there is no Intel runner in this pipeline to be portable to.

Adding host-arch detection would mean more moving parts in a gate whose whole value is that it is simple and fast. Happy to revisit if an Intel runner ever enters the picture.

ivolz added 2 commits July 30, 2026 10:48
Strict fail-closed signing errors (unsupported algorithm, required body
digest) escaped the OkHttp chain as unchecked IllegalStateException,
crashing instead of surfacing as a network error. Also correct the
USAGE.md tip that pointed custom mutators at the no-signing
ApproovServiceMutator.DEFAULT.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 34 out of 36 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

ios/ApproovService.m:405

  • PR description says same-config re-initializations “still preserve state”, but the implementation now treats same-config re-init as an initialization boundary that resets runtime configuration and the active service mutator after native success. Please update the PR description (and any release notes text) to match this behavior so consumers don’t rely on state preservation across same-config re-init.
    // SDK succeeded (or bypass) — now commit a fresh service-layer state.
    // Same-config re-initialization is an initialization boundary too: it is
    // forwarded to the SDK first, then runtime configuration and custom mutators
    // are reset only after native success.

ios/ApproovService.m:745

  • @synchronized(approovTokenPrefix) is locking on the value object, but that reference is reassigned inside the critical section. If another thread reads/locks while the prefix changes, reads/writes can be synchronized on different objects (or become effectively unlocked), which defeats the intended thread-safety. Consider introducing a dedicated lock object (e.g. approovTokenPrefixLock) and using it consistently for both reads and writes.
  @synchronized(approovTokenHeader) {
    approovTokenHeader = header;
  }
  @synchronized(approovTokenPrefix) {
    approovTokenPrefix = prefix ?: @"";
  }

@ivolz

ivolz commented Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Addressing the two suppressed comments from the latest Copilot pass.

1. ios/ApproovService.m:405 — PR description contradicted the implementation. Fixed.

Correct, and worth catching: the description still claimed "Same-config re-inits still preserve state", which 86beb27 inverted. The description has been updated to state the actual contract — every successful initialization is a reset boundary, same-config re-init included, so runtime configuration and the active service mutator are reset after native success. It also now spells out the consumer-facing consequence: re-apply setServiceMutatorType after each successful initialize(), and in ApproovProvider do that in an effect keyed on approovReady rather than in onInit, which runs before initialize() and would be wiped by the reset. USAGE.md and REFERENCE.md already documented the reset; a warning is now logged whenever a custom mutator is discarded this way.

2. ios/ApproovService.m:745@synchronized on a reassigned value object. Real, but pre-existing and out of scope here.

The mechanism is right, and it is worse than the comment suggests: because @synchronized locks on the object identity, reassigning the ivar inside its own critical section means a reader can hold a different lock than the writer, and interned string literals make collisions arbitrary. @synchronized(nil) is also a silent no-op, so any site whose ivar is currently nil is entirely unsynchronized.

It is not something this PR introduced or can scope-contain:

  • 13 sites use this pattern (approovTokenHeader, approovTokenPrefix, approovTraceIDHeader, bindingHeader), and the count is identical on main and on this branchgit diff main...HEAD -- ios/ApproovService.m adds zero @synchronized lines.
  • The only change on the flagged line is the prefix ?: @"" coalesce, which narrows the defect rather than widening it: before it, a nil prefix meant @synchronized(nil) and therefore no locking at all.

A correct fix is a dedicated lock object (or a single state lock) applied consistently across all 13 read and write sites, which is a self-contained change worth reviewing on its own rather than folding into this branch. Filing it separately.

3. tests/ios/run_real_sdk_typecheck.sh:105 (previous pass)

Already answered above — the stated Intel-macOS mechanism does not hold, and CI runs macos-latest (arm64). No change.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 34 out of 36 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

ios/ApproovService.m:422

  • The new re-initialization warning says to re-apply setServiceMutatorType, but a custom service mutator may also have been installed natively (via ApproovServiceMutatorBridge.serviceMutator). This makes the warning misleading for native integrations and doesn’t match Android’s wording (which mentions both paths). Consider updating the message to include the native-mutator case.
    if (![[ApproovServiceMutatorBridge shared] isDefaultMutator])
      ApproovLogW(@"initialization is discarding a custom service mutator - re-apply "
                   "setServiceMutatorType after initialize if a custom policy is still required");

The native SDK treats the comment as part of the already-initialized
match, so a same-config re-initialization is only accepted when the
comment is unchanged or starts with reinit. Verified on device against
Approov SDK 3.5.3 on Android and iOS: swapping null for an empty string
is reported as a different configuration. Also widen the iOS mutator
discard warning to name the native ApproovServiceMutatorBridge path,
matching the Android wording.

@charlesoj6205 charlesoj6205 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

setServiceMutatorType can be silently lost — the documented mitigation doesn't hold

USAGE.md:314 tells consumers to apply the policy in an effect "keyed on approovReady, which re-runs after every successful initialization (including React StrictMode double-mounts and Fast Refresh)". It doesn't. In approov-provider.js approovReady is a boolean latch — it goes false → true on the first success and never changes again — so useEffect(..., [approovReady]) will not re-run when a later initialize() resets the mutator.

That matters specifically because this PR makes same-config re-init a reset boundary. The cases that break:

  • an explicit re-initialize() after the app is ready (account switch, config refresh, the documented reinit: flow) — policy silently reverts to the built-in default
  • the headless ApproovService.initialize() path in Option 2, which has no approovReady at all

StrictMode double-mount happens to survive, but by timing — both effect setups run while approovReady is still false, and native module calls are serialized in dispatch order — not by the mechanism the doc describes.

It fails closed (the default is the stricter posture), so this is a functional bug rather than a security hole: requests that the app expected to proceed will start blocking, with only a native log line to go on.

Suggested fix — give the provider a signal that actually changes per initialization:

approov-provider.js

  const [status, setStatus] = useState({
    approovReady: false,
    approovError: null,
    approovInitCount: 0,
  })
        // success path
        setStatus((previous) => ({
          approovReady: true,
          approovError: null,
          approovInitCount: previous.approovInitCount + 1,
        }))
        // error path
        setStatus((previous) => ({
          approovReady: false,
          approovError: error,
          approovInitCount: previous.approovInitCount,
        }))

The updater form matters — two in-flight initializeApproov() closures can otherwise clobber the count.

index.d.ts:209

export declare function useApproov(): {
  approovReady: boolean;
  approovError: any;
  approovInitCount: number;
};

USAGE.md:314 — replace the paragraph and example with:

Do not set the policy in ApproovProvider's onInit — that callback runs before initialize, so the policy is immediately wiped by the reset. Apply it afterwards, keyed on approovInitCount:

const { approovReady, approovInitCount } = useApproov();
useEffect(() => {
    if (approovReady) {
        ApproovService.setServiceMutatorType(ApproovService.MutatorPreset.PROCEED_IF_UNAVAILABLE);
    }
}, [approovReady, approovInitCount]);

approovReady alone is not enough: it latches true on the first success and never changes again, so an effect keyed only on it will not re-run when a later re-initialization resets the mutator. approovInitCount increments on every successful initialization, so the effect re-runs and the policy is re-applied.

If you call ApproovService.initialize() yourself rather than through ApproovProvider (Option 2 above), the counter does not apply — re-apply setServiceMutatorType after each of those calls.

Adding a field to useApproov() is a public-contract addition, so it wants a 3.5.16 CHANGELOG bullet too. approov-monitor.js:26 destructures only approovReady/approovError, so nothing else breaks.


Non-blocking items from the same pass, for a follow-up commit on this branch:

  1. The re-init reset only warns about the mutator. It also clears bindingHeader, substitution headers and query params, exclusion regexes, token header/prefix and useApproovStatusIfNoToken, with no log at all. Token binding silently ceasing to apply is the security-relevant one — worth the same warning the mutator gets.
  2. CHANGELOG gaps. Nothing covers 14ced30 (under a PolicyMutator, a blocked status now fails the request during secure-string substitution — a real behavior change) or setServiceMutator(null) now restoring the signing default instead of the no-op ApproovServiceMutator.DEFAULT, which is a public-API change for native integrators. grep -n "substitut" CHANGELOG.md returns nothing.
  3. Fail-open vs fail-closed on signing — question rather than a change request. 86beb27 moved ASN.1 DER decode failure (both platforms) and the iOS HMAC path from throw to proceedUnsigned, so the request goes out unsigned; 07262e5 then argues 20 minutes later that strict signing failures must fail closed. Signature unavailability proceeding unsigned matches the established contract, but a DER decode failure is a corruption/integrity failure — should that one stay fail-closed as an IOException? Either way the rule is worth stating explicitly in the code or CHANGELOG so the next change doesn't have to re-derive it.

Verified correct while looking at this, noting it so it doesn't get re-flagged: the truefalse flip for noNetwork/poorNetwork/mitmDetected in ios/ApproovURLSession/ApproovServiceMutator.swift is a genuine parity fix — Android's default at ApproovServiceMutator.java:272 already returned false, the documented contract is "true if substitution should proceed", and it's behavior-neutral at the call site because ios/ApproovService.m guards on status == ApproovTokenFetchStatusSuccess before substituting. Likewise 07262e5's IllegalStateExceptionIOException wrapping is right, and the genuinely strict paths (unsupported algorithm, required body digest) still throw.

approovReady latches true on the first successful initialization and never
changes again, so an effect keyed only on it cannot observe a later
re-initialization. Since every successful initialization is a reset
boundary, a policy applied that way was silently reverted to the built-in
default. ApproovProvider now publishes approovInitCount, incremented with
the updater form so concurrent initialization closures cannot clobber it,
and USAGE.md keys the documented effect on it.

Also warn when a re-initialization discards the binding header or the
substitution and exclusion configuration, state the message signing
fail-open/fail-closed rule on both platforms, and record the previously
undocumented setServiceMutator(null) and secure-string substitution
behaviour changes in the changelog.
@ivolz

ivolz commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the blocking finding is correct and is fixed in edeabbe, along with all three follow-ups.

Blocking: setServiceMutatorType silently lost

Confirmed exactly as described. approov-provider.js has useEffect(..., []) and approovReady only ever goes false → true, so an effect keyed on it cannot observe a later initialization — and since this PR makes every successful initialization a reset boundary, the policy reverted to the default with only a native log line to show for it. My USAGE.md guidance was wrong.

Implemented your suggested fix as written: approovInitCount in provider state, incremented with the updater form so two in-flight initializeApproov() closures cannot clobber the count, added to the useApproov() return type in index.d.ts, and USAGE.md re-keyed onto [approovReady, approovInitCount] with your explanatory paragraph and the Option 2 caveat. CHANGELOG bullet added for the public-contract addition.

One limitation worth stating explicitly, since the counter is easy to over-trust: it only advances when ApproovProvider itself initializes. An app calling ApproovService.initialize() directly — your account-switch and config-refresh cases, if driven outside the provider — still gets no signal, because the provider's state never changes. That is what the Option 2 paragraph now says, but it means the counter narrows the gap rather than closing it. If you want it closed properly, the provider would need to re-initialize on config/comment change, or the native layer would need to emit an initialization event JS can subscribe to. Happy to do either as a follow-up — it felt like a larger design decision than this PR should absorb.

Test coverage added in __tests__/approov-provider.test.js: an effect keyed on [approovReady, approovInitCount] is asserted to fire with {false, 0} then {true, 1}. I first wrote it to mount two providers and assert [1, 1], which reads like proof of an increment but is nothing of the sort — two separate instances each reaching 1. Replaced with the honest version. The two existing exact-shape assertions were updated for the new field.

Follow-up 1 — reset now warns about the rest of the discarded state

Agreed, and the token-binding case is the one that mattered. Both platforms now warn separately: one line when bindingHeader is discarded, calling out that tokens will no longer be bound to that header value, and one covering the substitution headers, query params and exclusion regexes together. Kept as two messages rather than one so the security-relevant case doesn't get lost in a list.

Follow-up 2 — CHANGELOG gaps

Both added, plus a third you didn't flag:

  • 14ced30 — policy mutators now govern secure-string substitution, so a masked status no longer blocks the request during substitution. This is what made PROCEED_IF_UNAVAILABLE behave as documented for apps using addSubstitutionHeader/addSubstitutionQueryParam.
  • setServiceMutator(null) now restoring the signing default instead of the no-op pass-through, flagged as a public API change for native integrators.
  • The fail-open/fail-closed rule itself (see below).

Follow-up 3 — signing fail-open vs fail-closed

Good question to force, and the answer is that DER decode should stay fail-open. The contract in root TESTING_REQUIREMENTS.md is that signing is fail-open except an unsupported algorithm and a required body digest that cannot be produced. The distinction is what the failure tells you: unsupported-algorithm and required-digest mean the caller asked for a guarantee that cannot be honoured, so proceeding would silently weaken the request; a DER decode failure means the signature material could not be obtained, which is the same class as unavailability or a base64 failure. So 86beb27 was right and 07262e5 is consistent with it — 07262e5 only ensured the two genuinely strict paths surface as a clean IOException rather than an unchecked crash.

Rather than leave that to be re-derived, the rule is now stated on the proceedUnsigned helper in both ApproovDefaultMessageSigning.java and ApproovDefaultMessageSigning.swift, and recorded in the CHANGELOG.

Verification

npx jest --runInBand — 16/16. Android ./gradlew test — 14 suites, 0 failures. All four iOS suites green (mini-sdk, native, message-signing, PolicyMutator), including the real-SDK typecheck.

Separately, the re-initialization behaviour this PR depends on has now been verified on device against the real Approov SDK 3.5.3 rather than the mini-SDK model: a genuine React Native hot restart (native process preserved, JS VM restarted) on Android and iOS, plus a native probe confirming Objective-C receives (NO, nil) for a same-config call while Swift receives a _GenericObjCError throw from the same call. One thing that surfaced and is now documented in REFERENCE.md: the SDK treats the comment as part of the already-initialized identity, so a same-config re-init with a changed comment — including swapping null for "" — is rejected as a different configuration.

@charlesoj6205 charlesoj6205 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants