Skip to content

Fix initialization regressions: same-config wipe, reset race, ungated mutation log#27

Merged
ivolz merged 3 commits into
feature/additional-loggingfrom
feature/additional-logging-adrian-Edits
Jun 9, 2026
Merged

Fix initialization regressions: same-config wipe, reset race, ungated mutation log#27
ivolz merged 3 commits into
feature/additional-loggingfrom
feature/additional-logging-adrian-Edits

Conversation

@adriantuk

@adriantuk adriantuk commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Follow-up review fixes on top of #26 (feature/additional-logging). Targets the initialization rewrite, where the real risks were found. Each fix is a separate commit describing the issue and the fix.

Fixes

1. Preserve runtime configuration on same-config re-initialization (HIGH — Android + iOS)

The initialize() rewrite reset all service-layer state on every successful call; the only guard was the "already enabled + empty config" case. Every other path — including re-initializing with the same config already in force — fell through to the unconditional reset.

In React Native this is reached routinely: ApproovProvider calls initialize() from a useEffect, and React StrictMode double-invokes effects, Fast Refresh re-runs them, and apps may mount more than one provider. A second initialize() with the same config then silently discarded any configuration applied after the first call — dropping request mutations and, more seriously, exclusion URL regexes, which are security relevant.

Fix: only reset runtime configuration when the config actually changes. Same-config re-init is still forwarded to the platform SDK but preserves service-layer state. Applied identically to ApproovService.java and ApproovService.m.

2. Make the Android initialize() state transition atomic (MEDIUM)

initialize() is not synchronized, yet it performs a multi-field reset that transiently sets isInitialized=false/initialConfig=null on non-volatile static fields, while the interceptor reads them through synchronized getters on OkHttp network threads. A request racing a runtime re-init could observe an inconsistent state and forward a request that should be protected without an Approov token, or re-arm the startup sync gate and stall concurrent requests.

Fix: perform the reset+commit inside synchronized(this) (the SDK network call stays outside the lock so it never blocks the getters). Mirrors iOS, which already does the equivalent inside @synchronized(initializerLock).

3. Gate the Android request-mutation log by log level (LOW)

The new request mutation log used android.util.Log.i directly, so it couldn't be suppressed via setLogLevel and logged the URL + token length on every request. The iOS task mutation log it mirrors uses ApproovLogI, which is gated below INFO.

Fix: add a package-private, level-gated logInfo() on ApproovService and route the interceptor's mutation log through it.

Testing

All run locally on both platforms.

  • Android (ApproovServiceRegressionTest): added same-config-preserves and different-config-resets tests. Full suite 45 tests, 0 failures. The same-config test fails without Fix 1.
  • iOS (ApproovNativeTests): added same-config-preserves test. All native tests pass; the 3 new assertions fail without the iOS fix.

Not changed (out of scope)

  • PR Feature/additional logging #26's own changes verified compile-safe (iOS withErrorCode: mock methods exist; Approov.initialize returns boolean in SDK 3.5.3).
  • Minor sessionMetadataCollectionEnabled cross-platform reset divergence and JS-layer observations (index.js Log assignment on the Proxy, approov-provider.js using raw NativeModules) are pre-existing and left untouched.

Related: approov/core-project-approov#562

adriantuk added 3 commits June 9, 2026 13:52
The initialize() rewrite reset ALL service-layer state (substitution headers,
exclusion URL regexes, token/binding header settings, status flags) on every
successful call. The only guard was "already enabled with a valid config +
empty config" — every other path, including re-initializing with the SAME
config already in force, fell through to the unconditional reset.

In React Native this is reached routinely: ApproovProvider calls initialize()
from a useEffect, and React StrictMode double-invokes effects, Fast Refresh
re-runs them, and apps may mount more than one provider. A second initialize()
with the same config then silently discarded any configuration the app applied
after the first call — dropping request mutations and, more seriously,
exclusion URL regexes, which are security relevant.

Fix: only reset the runtime configuration when the config actually changes.
A same-config re-initialization (with or without a reinit/options comment) is
still forwarded to the platform SDK but preserves the service-layer state.
Applied identically to Android (ApproovService.java) and iOS (ApproovService.m).

Tests:
- Android ApproovServiceRegressionTest: same-config re-init preserves
  substitution/exclusion/token/binding config; different-config still resets.
- iOS ApproovNativeTests: same-config re-init preserves substitution header,
  exclusion regex and token header. Both new tests fail without the fix.
…t unprotected requests

initialize() is not a synchronized method, yet it performs a multi-field state
reset that transiently sets isInitialized=false and initialConfig=null before
re-committing them. The interceptor reads isInitialized, isApproovEnabled and the
header/substitution/exclusion state through synchronized getters on OkHttp network
threads.

Because the reset wrote those non-volatile static fields with no lock (and thus no
happens-before relationship with the synchronized getters), a request racing a
runtime re-initialization could observe an inconsistent state — either the transient
isInitialized=false/initialConfig=null window, or a re-ordered commit where
isInitialized=true is visible before initialConfig is set — causing isApproovEnabled()
to return false and a request that should be protected to be forwarded without an
Approov token. The same window could also re-arm the startup sync gate and stall
concurrent requests for up to STARTUP_SYNC_TIME_WINDOW.

Fix: perform the reset and commit inside synchronized(this) so the transition is
atomic relative to the synchronized getters. The platform SDK call is deliberately
left outside the lock so its network work never blocks those getters. This mirrors
iOS, which already performs the equivalent reset inside @synchronized(initializerLock).

Verified by the existing Android unit suite (45 tests, 0 failures).
The new "request mutation" INFO log in ApproovInterceptor was emitted via
android.util.Log.i directly, so it could not be suppressed through setLogLevel and
logged the request URL plus token length on every single request regardless of the
configured level. The iOS "task mutation" log it mirrors uses ApproovLogI, which is
gated and suppressed below INFO.

Fix: add a package-private, level-gated logInfo() entry point on ApproovService and
route the interceptor's mutation log through it, so the log honours setLogLevel and
matches the iOS behaviour instead of writing unconditionally.

Verified by the existing Android unit suite (45 tests, 0 failures).
@adriantuk adriantuk self-assigned this Jun 9, 2026
@adriantuk adriantuk requested a review from ivolz June 9, 2026 13:06
@adriantuk adriantuk added the bug Something isn't working label Jun 9, 2026
@ivolz ivolz merged commit 4bcf50c into feature/additional-logging Jun 9, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants