Feature/use mini sdk#18
Conversation
… GH Actions Node.js version warnings
There was a problem hiding this comment.
Pull request overview
Standardizes empty-config initialization/bypass behavior for the Retrofit service layer, adds fail-fast gating for direct SDK wrappers, introduces short-lived failure caching for token fetch failures, and integrates a shared mini-SDK integration test suite with CI support.
Changes:
- Add bypass-mode initialization semantics (empty config), plus exported state checks (
isInitialized,isApproovEnabled) and fail-fast gating on direct SDK APIs. - Add a thread-safe 500ms interceptor failure cache to reduce repeated blocking SDK calls during sustained failure states.
- Add/conditionally-wire mini-SDK integration tests and a GitHub Actions workflow to build and run tests (including worker endpoint verification).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle | Conditionally includes sibling mini-SDK/testing projects via local.properties/default path. |
| approov-service/src/main/java/io/approov/service/retrofit/ApproovService.java | Implements bypass semantics, exported state helpers, fail-fast gating, and failure caching in the interceptor path. |
| approov-service/src/main/AndroidManifest.xml | Removes manifest package attribute (namespace handled in Gradle). |
| approov-service/build.gradle | Conditionally excludes mini-SDK integration test when test-support module missing; adds Robolectric/testing deps. |
| approov-service/src/test/java/io/approov/service/retrofit/ApproovTestSupport.java | Resets new cache fields and initializes with non-empty config for unit tests. |
| approov-service/src/test/java/io/approov/service/retrofit/ApproovServiceContractTest.java | Ensures service initialization occurs before direct SDK wrapper tests. |
| approov-service/src/test/java/io/approov/service/retrofit/ApproovServiceMiniSdkTest.java | Adds integration tests against mini-SDK/testing framework. |
| REFERENCE.md | Documents bypass semantics and new exported state helpers; updates prefetch docs. |
| CHANGELOG.md | Adds 3.5.6 entry describing new behavior/caching/testing changes. |
| .github/workflows/build_and_test.yml | Adds CI workflow to build/test and verify/redeploy worker endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The direction makes sense, especially separating “service layer initialized” from “Approov actively enabled” and making the direct SDK APIs fail fast in bypass mode. I found a few behavior issues that should be addressed before merge. Blocking issues
That conflicts with the PR description that says a later valid initialization “enables Approov protection at runtime”. In practice, protection only applies to newly built Retrofit instances. This needs one of:
File:
The new failure cache stores a single static For example, a The cache should be keyed at least by host or URL, or limited only to statuses that are truly global across all requests. File:
Most direct SDK APIs now correctly fail fast when the service is initialized in bypass mode. However,
and
without checking In bypass mode these should behave consistently with |
Device-wide failure conditions (NO_NETWORK, MITM_DETECTED, etc.) apply equally to all URLs. The previous URL-scoped single-slot cache would evict on every different URL, defeating the cache for concurrent requests to different endpoints. This now matches the urlsession service layer.
Resolves high severity vulnerability (Dependabot #17). Updates both build.gradle and pom.xml to keep versions in sync.
Adds fetchApproovTokenWithGate() using a separate fetchGateLock to ensure only ONE thread calls the SDK when the failure cache is empty/expired. Other threads wait on the gate and re-check the cache. - Fast path: cached failure returned immediately (no lock) - Slow path: gate serializes SDK calls, caches failures - Happy path unaffected: success tokens are never cached - Separate lock avoids holding cache lock during ~1-3s SDK call
NO_NETWORK throws by default (proceedOnNetworkFail is false), so intercept() never returns a Response. Use assertThrows instead of .close() while preserving the SDK-called-once assertion.
- actions/checkout v4 → v5 - actions/setup-java v4 → v5 - actions/upload-artifact v4 → v7 Resolves Node.js 20 deprecation warnings. build_and_test.yml was already on the latest versions.
|
LGTM |
This PR focuses on standardizing the empty-config bypass behaviors, hardening the fail-fast logic across all direct SDK APIs, introducing thread-safe failure caching, and integrating the shared integration testing framework.
Initialization & Empty-Config Bypass
Bypass Semantics: Initializing the service with an empty configuration string ("") now successfully initializes the Retrofit service layer in a bypass mode. The underlying Retrofit client is configured and traffic passes through normally without Approov protection. A subsequent initialize call with a valid string seamlessly enables Approov protection at runtime.
Public State Exporters: Added public static boolean isInitialized() and exposed public static boolean isApproovEnabled() to safely distinguish between "service layer ready" and "native SDK actively protecting traffic".
Fail-Fast Native Helpers: Consistently gated all direct native wrappers (precheck(), fetchToken(), getDeviceID(), fetchSecureString(), getAccountMessageSignature(), etc.) on isApproovEnabled(). Calling these methods while bypassed now immediately throws an ApproovException rather than reaching down and crashing the uninitialized native SDK.
Initialization Resilience: Tightened the initialization guard to tolerate benign, cross-service-layer same-config initializations, while properly throwing on legitimate different-configuration failures.
prefetch() Obsoletion: Deprecated prefetch() as a no-op that emits a warning log, aligning it with current SDK architectural behavior where native prefetching is automatic.
Short-Lived Failure Caching
Thread-Safe Caching: Implemented a 500ms failure cache (getCachedFailure / cacheFailureIfNeeded). When the platform SDK returns a hard failure status (NO_NETWORK, POOR_NETWORK, MITM_DETECTED, NO_APPROOV_SERVICE), the result is cached. Subsequent requests within that tight window short-circuit the interceptor, avoiding redundant ~1s SDK blocking calls.
Testing & Infrastructure Standardization
Unified Integration Testing: Added comprehensive test coverage for core service flows using the cross-platform ApproovServiceMiniSdkTest.java from core-service-layers-testing.
Clean Checkout Resilience: Modified settings.gradle and build.gradle to conditionally exclude the ApproovServiceMiniSdkTest source set when the sibling test-support module is missing. This completely resolves compilation failures on clean checkouts, ensuring the repository builds perfectly in standalone or public CI environments.