Mirror okhttp parity in approov-service-httpsurlconn#9
Mirror okhttp parity in approov-service-httpsurlconn#9charlesoj6205 wants to merge 17 commits intomainfrom
Conversation
…or implementation.
…termediates using getPeerCertificates. If peer chain doesn't match, new code calls getTrustAnchorCertifcate to pull trusted root to verify
There was a problem hiding this comment.
Pull request overview
This PR brings the HttpsURLConnection service layer closer to the OkHttp service’s feature set by introducing a mutator-driven request preparation flow, optional TraceID propagation, query-parameter substitution support, and message signing (including HTTP Structured Field Values support).
Changes:
- Add
ApproovServiceMutatordecision points throughout request preparation (token fetch, substitution handling, pinning, processed-request callback). - Implement message signing for
HttpsURLConnectionrequests (including request-body buffering when URL substitution changes the effective connection). - Add SFV parsing/serialization utilities plus updated docs and build/test configuration.
Reviewed changes
Copilot reviewed 40 out of 42 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovService.java | Mutator-driven request prep, TraceID support, query substitution, pinning enhancements, addApproov now returns possibly-wrapped connection. |
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovServiceMutator.java | New callback interface defining default decision behavior. |
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovDefaultMessageSigning.java | Adds signing implementation + signature parameter factory. |
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovBufferedHttpsURLConnection.java | Wrapper connection to buffer body + apply processed-request callback late when URL changes. |
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovRequestMutations.java | Tracks request mutations (token header, substituted headers/query params, TraceID header). |
| approov-service/src/main/java/io/approov/util/sig/* | Signature base building + signature parameter parsing/building utilities. |
| approov-service/src/main/java/io/approov/util/http/sfv/* | Adds SFV parser/model classes used to construct signature headers. |
| approov-service/src/test/java/io/approov/service/httpsurlconn/ApproovServiceTest.java | New unit tests covering addApproov behavior (signing + query substitution + status-as-token). |
| approov-service/build.gradle | Updates SDK levels, adds unit test config and dependencies (Mockito/JUnit/BouncyCastle). |
| README.md / USAGE.md / REFERENCE.md / CHANGELOG.md | Adds/updates documentation for the new APIs and behavior. |
| .wrangler/cache/wrangler-account.json | Newly added Wrangler cache content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mockAndroidBase64.when(() -> android.util.Base64.encodeToString(Mockito.any(byte[].class), Mockito.anyInt())) | ||
| .thenAnswer(invocation -> | ||
| Base64.getEncoder().withoutPadding().encodeToString(invocation.getArgument(0, byte[].class))); |
There was a problem hiding this comment.
The mocked android.util.Base64.encodeToString(..., flags) uses Base64.getEncoder().withoutPadding(), but Android's Base64.NO_WRAP does not strip padding. This makes the unit test base64 behavior diverge from production and can hide/introduce failures around signature/header serialization. Consider honoring the passed flags (only remove padding when NO_PADDING is set) or defaulting to Base64.getEncoder() for parity with NO_WRAP.
There was a problem hiding this comment.
Fixed in b927668. The mock now checks the NO_PADDING flag and only calls withoutPadding() when that flag is set, so NO_WRAP (the flag used in production) correctly produces padded output matching Android's behavior.
| public boolean containsComponentIdentifier(StringItem identifier) { | ||
| String value = identifier.get(); | ||
| Parameters params = identifier.getParams(); | ||
| for (StringItem item : componentIdentifiers) { | ||
| if (value.equals(identifier.get()) | ||
| && params.equals(identifier.getParams())) { | ||
| return true; | ||
| } | ||
| } |
There was a problem hiding this comment.
containsComponentIdentifier(StringItem identifier) compares value/params against identifier inside the loop instead of the iterated item, so it will return true for any non-empty componentIdentifiers list regardless of contents. This breaks component-matching logic; compare against item.get()/item.getParams() inside the loop.
There was a problem hiding this comment.
Fixed in b927668. Changed the comparisons inside the loop from identifier.get()/identifier.getParams() to item.get()/item.getParams() so each iterated element is properly compared against the input identifier.
| // Calculate the signature and message descriptor headers. | ||
| String sigHeader = Dictionary.valueOf(Map.of( | ||
| sigId, ByteSequenceItem.valueOf(signature))).serialize(); | ||
| String sigInputHeader = Dictionary.valueOf(Map.of( | ||
| sigId, params.toComponentValue())).serialize(); | ||
|
|
There was a problem hiding this comment.
This uses Map.of(...) to build the Dictionary payload. Map.of is a Java 9 API and is not available on Android API levels below 24 (unless core library desugaring is enabled), so this can cause NoSuchMethodError/NoClassDefFoundError at runtime. Use a LinkedHashMap/HashMap builder instead (or enable coreLibraryDesugaring and ensure minSdk/tooling support).
There was a problem hiding this comment.
Fixed in c1de490. All four Map.of(...) calls in ApproovDefaultMessageSigning.java have been replaced with LinkedHashMap construction (also added the required ListElement import). This avoids the Java 9 API that is unavailable on Android API levels below 24.
| android { | ||
| namespace = 'io.approov.service.httpsurlconn' | ||
| compileSdkVersion 30 | ||
|
|
||
| defaultConfig { | ||
| minSdkVersion 21 | ||
| targetSdkVersion 28 | ||
| minSdkVersion 23 | ||
| targetSdkVersion 34 | ||
| } | ||
|
|
||
| buildTypes { | ||
| release { | ||
| minifyEnabled false | ||
| proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro' | ||
| } | ||
| } | ||
| compileOptions { | ||
| sourceCompatibility JavaVersion.VERSION_1_8 | ||
| targetCompatibility JavaVersion.VERSION_1_8 | ||
| } | ||
| testOptions { | ||
| unitTests.returnDefaultValues = true | ||
| } |
There was a problem hiding this comment.
The module now sets minSdkVersion 23, but the new SFV implementation and signing code reference Java 8+ library types like java.util.function.* and Java 9 APIs like Map.of(...). These are not available on Android API 23 without core library desugaring, so this will fail at runtime on supported devices. Either enable coreLibraryDesugaring (and add the appropriate desugar_jdk_libs dependency) or raise minSdkVersion to a level that guarantees these APIs are present.
There was a problem hiding this comment.
Fixed in c1de490. Added coreLibraryDesugaringEnabled true to compileOptions and coreLibraryDesugaring 'com.android.tools:desugar_jdk_libs:2.1.4' to the dependencies block. This makes java.util.function.* (used in Parameters.java) available at runtime on Android API 23+. The Map.of() calls were separately replaced with LinkedHashMap since those are Java 9 APIs.
…ApproovService.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ApproovService.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…usage Agent-Logs-Url: https://github.com/approov/approov-service-httpsurlconn/sessions/6f62bb9d-7f9c-46a9-844d-5b66e7639bb8 Co-authored-by: charlesoj6205 <272465118+charlesoj6205@users.noreply.github.com>
…ApproovDefaultMessageSigning.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ApproovDefaultMessageSigning.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ApproovService.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… in test mock Agent-Logs-Url: https://github.com/approov/approov-service-httpsurlconn/sessions/e2a57bf5-0213-4e61-8976-b568b6de3a75 Co-authored-by: charlesoj6205 <272465118+charlesoj6205@users.noreply.github.com>
…for Android API 23 compat Agent-Logs-Url: https://github.com/approov/approov-service-httpsurlconn/sessions/d7e54aea-dd27-482b-9ef2-1512591e1d37 Co-authored-by: charlesoj6205 <272465118+charlesoj6205@users.noreply.github.com>
adriantuk
left a comment
There was a problem hiding this comment.
I've made few changes
6bf3f41 Allow buffered mutator request edits
c9fcb55 Buffer body-capable requests before signing
b129b7f Preserve legacy addApproov API
4fa033a Update Mockito for JDK 21 tests
Could you please do few sanity checks before we merge it, and perhaps few test cases focused on approov service mutator?
Thanks!
No description provided.