Release 3.5.7 — init hardening, message-signing fail-open, §9 dependency isolation, body-digest API, remove automated query substitution (Issue #14)#15
Open
ivolz wants to merge 24 commits into
Conversation
…README restructuring for httpsurlconn (v3.5.5)
Refactors initialize() to match okhttp/retrofit/volley pattern: - null config now throws IllegalArgumentException (no silent bypass coercion) - SDK is called first before touching any service-layer state - State is only reset and committed after SDK confirms success - Removes the old service-layer 'already initialized' guard; SDK itself detects and throws on conflicting configs Per TESTING_REQUIREMENTS §17-18 (core-service-layers-testing).
Updated the security policy to clarify supported versions and reporting process.
… consumer R8 rules (TESTING_REQUIREMENTS §9)
… 3.5.7 java.net.URL is immutable once the connection is opened, and the automated query-substitution path broke the request-mutation tracking message signing relies on. Removed the public API (addSubstitutionQueryParam, removeSubstitutionQueryParam, getSubstitutionQueryParams, substituteQueryParams, substituteQueryParam) and the automated substitution in the addApproov flow. Secure-string query values are now fetched manually via fetchSecureString() and built into the URL before openConnection(). USAGE.md/REFERENCE.md updated. BREAKING CHANGE: query-parameter substitution APIs removed.
…token; required-digest -> ApproovException - Add ApproovServiceMiniSdkTest / ApproovNativeSdkTest (run against the mini-sdk). - NO_APPROOV_SERVICE now proceeds emitting an empty Approov-Token (+ trace) per root TESTING_REQUIREMENTS §2, instead of omitting the headers. - A required body digest that cannot be generated now fails closed as ApproovException (the addApproov contract) rather than a raw IllegalStateException. - Adapt the same-config/different-config init test: the layer surfaces the platform SDK's IllegalStateException (§1), not a service-layer guard message.
…rop stale query-substitution mention
Contributor
There was a problem hiding this comment.
Pull request overview
This PR prepares the approov-service-httpsurlconn Android service layer for the 3.5.7 release by aligning behavior with shared testing requirements: hardened initialization/bypass behavior, message-signing/body-digest support with fail-open/closed semantics, removal of automated query substitution, and dependency isolation (shaded BouncyCastle), backed by new integration tests and updated docs/CI.
Changes:
- Harden initialization and public API gating (explicit bypass mode, state commit only after successful SDK init).
- Update message signing behavior (fail-open in specific decode paths) and add an
addApproov(connection, byte[])overload to supportContent-Digest. - Remove automated query substitution and add dependency isolation (shaded/relocated BouncyCastle), integration tests, and documentation/CI release checks.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| USAGE.md | Updates usage guidance for manual query secure-string usage and body-digest signing overload. |
| settings.gradle | Adds optional inclusion of mini-SDK projects for integration tests via local.properties. |
| SECURITY.md | Adds a security policy document and supported versions table. |
| REFERENCE.md | Updates API reference for new initialization overloads and removal of query substitution APIs. |
| README.md | Replaces the README with a fuller integration guide (init/bypass example, badges, links). |
| CHANGELOG.md | Adds 3.5.7 release entry describing the behavioral and API changes. |
| approov-service/src/test/java/io/approov/service/httpsurlconn/ApproovServiceTest.java | Updates unit tests for new init semantics and removed query-substitution behavior. |
| approov-service/src/test/java/io/approov/service/httpsurlconn/ApproovServiceMiniSdkTest.java | Adds a large mini-SDK integration test suite aligned to TESTING_REQUIREMENTS scenarios. |
| approov-service/src/test/java/io/approov/service/httpsurlconn/ApproovNativeSdkTest.java | Adds a Robolectric test for native SDK init behavior (currently needs assertions). |
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovServiceMutator.java | Adjusts missing-artifacts fallback behavior for NO_APPROOV_SERVICE. |
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovService.java | Adds init/bypass state tracking, new addApproov(..., byte[]) overload, and removes query substitution APIs. |
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovRequestMutations.java | Adds body-bytes plumbing used by message signing digest computation. |
| approov-service/src/main/java/io/approov/service/httpsurlconn/ApproovDefaultMessageSigning.java | Switches to relocated BouncyCastle packages and updates message-signing fail-open/closed behavior + body digest input. |
| approov-service/consumer-rules.pro | Adds consumer ProGuard/R8 rules to preserve Approov SDK classes/native methods. |
| approov-service/build.gradle | Adds Shadow plugin-based shading/relocation of BouncyCastle and conditional mini-SDK test wiring. |
| .github/workflows/build_and_publish.yml | Adds a tag-vs-CHANGELOG top entry validation step to fail fast on release drift. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1105,7 +1125,22 @@ static synchronized QuerySubstitutionResult substituteQueryParamsDetailed(URL ur | |||
| * secure strings | |||
Contributor
There was a problem hiding this comment.
Fixed in the latest commit — the addApproov(HttpsURLConnection) Javadoc no longer mentions query substitutions changing the effective URL; it now only references deferred body-aware processing as the reason to prefer addApproovToConnection.
| @@ -1127,124 +1162,50 @@ public static synchronized void addApproov(HttpsURLConnection request) throws Ap | |||
| * secure strings | |||
Contributor
There was a problem hiding this comment.
Fixed in the latest commit — the addApproovToConnection Javadoc now describes only the buffered-wrapper/deferred body-aware processing case, with no mention of query substitution changing the target URL.
…fix, bypass verify, empty trace Pre-merge fixes from the full TESTING_REQUIREMENTS audit of 3.5.7: - §1: an empty config after a valid one is now ignored (guard) instead of silently downgrading a protected layer to bypass. + regression test. - §2: setApproovHeader(h, null) coerces the prefix to "" (no literal "null"). - §4: bypass-mode PinningHostnameVerifier delegates to the default verifier rather than returning true, so OS trust validation is never skipped. - §2: empty trace ID emitted as an empty header rather than omitted. - Mini-sdk empty-config/bypass tests reset to a clean state before the empty init (they were relying on the removed downgrade behavior).
… dead query-substitution plumbing - Tests: assert the Signature header is the byte-sequence form install=:<base64>: / account=:<base64>: (was only startsWith), guarding against a quoted-string regression. - REFERENCE.md: document initialize() throws IllegalArgumentException (null config) and IllegalStateException (different-config reinit). - Remove now-unreachable query-substitution plumbing: ApproovRequestMutations query fields + accessors, ApproovServiceMutator.handleInterceptorQueryParamSubstitutionResult, QuerySubstitutionResult.hasEffectiveUrlChange (+ unused fields), and the dead buffered- connection block; simplify shouldUseBufferedConnection.
Wrap signature-base construction and Signature / Signature-Input serialization so a failure logs at error level and proceeds unsigned rather than propagating and aborting the request, for full conformance with the cross-layer message-signing fail-open policy (core-project-approov#564). Required body digest and unsupported algorithm remain fail-closed.
Previously setUserProperty reported a bare, version-less string
("approov-service-httpsurlconn"), and the published Maven artifact
carried no version at all — unlike the other Android layers.
- build.gradle: enable BuildConfig and bake APPROOV_SERVICE_VERSION from
-PapproovServiceVersion (default "dev").
- ApproovService: setUserProperty now appends "/" + BuildConfig.APPROOV_SERVICE_VERSION.
- build_and_publish.yml: build the AAR with -PapproovServiceVersion=<tag>
so the published artifact reports the real release version.
Verified: the generated BuildConfig bakes the injected version
(APPROOV_SERVICE_VERSION = "<tag>"); local/dev builds report "dev".
Add a standalone tag-release workflow (main push): the top CHANGELOG entry drives a matching git tag, which triggers the Maven publish workflow (build + CHANGELOG-vs-tag gate + publish). This repo has no main build/test workflow, so tagging is standalone; the build and gate run in build_and_publish.yml on the resulting tag. Skipped if the tag already exists.
Replace the standalone tag-release workflow with a complete build_and_test workflow modelled on the other Android layers: on push to main and on PRs it checks out the core-service-layers-testing mini-SDK, builds the AAR, and runs the unit + mini-SDK integration tests (with a JUnit summary). The tag-release job now lives here as `needs: build-and-test`, so a release is only tagged after the tests pass; the tag then triggers the Maven publish workflow. Removed tag-release.yml (superseded). settings.gradle resolves the mini-SDK at '../../core-service-layers-testing' for the nested local layout; CI checks the testing repo out as a sibling, so a step writes an absolute miniSdkAndroidRoot into local.properties. Verified locally (Java 21, mini-SDK wired): assembleRelease + testDebugUnitTest pass — 57 tests, 0 failures (ApproovServiceMiniSdkTest 41, ApproovNativeSdkTest 1, ApproovServiceTest 15).
ivolz
force-pushed
the
feature/initialize-guard-service-layers
branch
from
7a1167a to
3441ef4
Compare
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
charlesoj6205
left a comment
charlesoj6205
left a comment
Contributor
There was a problem hiding this comment.
Sense checked Copilot suggestions and applied all which were valid. In summary,
- Aligned the documented minimum Android SDK with
minSdkVersion 23. - Improved the wording in
SECURITY.md. - Defensively copy request body bytes used for message signing.
- Replaced diagnostic output in the native SDK test with assertions.
- Corrected OkHttp references to HttpsURLConnection.
- Replaced reflection-based test state resetting with
ApproovService.reset(). - Removed the unused
HttpURLConnectionimport.
| @@ -1105,7 +1125,22 @@ static synchronized QuerySubstitutionResult substituteQueryParamsDetailed(URL ur | |||
| * secure strings | |||
| @@ -1127,124 +1162,50 @@ public static synchronized void addApproov(HttpsURLConnection request) throws Ap | |||
| * secure strings | |||
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
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.
Summary
Brings
approov-service-httpsurlconninto full compliance with the sharedcore-service-layers-testingTESTING_REQUIREMENTS and ships it as the 3.5.7 production release.Changes
Initialization (TESTING_REQUIREMENTS §1)
nullconfig now throwsIllegalArgumentException(pass""for bypass) instead of silent coercion.isInitialized()/isApproovEnabled(); guards on every public API so calls in bypass/uninitialized mode no longer hit the SDK and crash.initialize(context, config, comment)overload.Message signing (core issue #564 — fail-open)
ApproovException(the documentedaddApproovcontract).Body digest (TESTING_REQUIREMENTS supp §4)
addApproov(HttpsURLConnection, byte[])overload computes theContent-Digestover the supplied repeatable body bytes; the legacyaddApproov(connection)gracefully skips the digest when no body is available.Packaging & dependency isolation (TESTING_REQUIREMENTS §9)
io.approov.internal.httpsurlconn.bouncycastleand removed as an exposed/transitive dependency (no more clashes with an app's own copy). Bumped tobcprov-jdk15to18:1.84.consumer-rules.pro) preservingcom.criticalblue.approovsdkand its native methods.Missing-artifacts fallback (TESTING_REQUIREMENTS §2)
NO_APPROOV_SERVICE, the request proceeds emitting an emptyApproov-Tokenheader (and a trace ID if available) as evidence of Approov processing, instead of omitting the headers. (UNKNOWN_URL/UNPROTECTED_URLstill send no headers.)Removed (Issue #14 — BREAKING)
addSubstitutionQueryParam,removeSubstitutionQueryParam,getSubstitutionQueryParams,substituteQueryParams,substituteQueryParam.java.net.URLis immutable once the connection is opened, and the automated path broke the request-mutation tracking message signing relies on. Fetch secure-string query values withfetchSecureString()and build the URL beforeopenConnection()(documented in USAGE.md / REFERENCE.md).Docs, security & CI
initialize()try/catch example (logs device ID + an app session/correlation id on success; on failure continues unprotected via empty-config bypass).SECURITY.md(supported-versions table corrected to< 3.5).## [x.y.z]CHANGELOG entry doesn't match the release tag.Testing
ApproovServiceMiniSdkTest,ApproovNativeSdkTest) wired againstcore-service-layers-testing/mini-sdk../gradlew :approov-service:testDebugUnitTest→ 56 tests pass, 2 skipped (pinning/Robolectric-limited). Build/tests run on JDK 21.Follow-ups (separate)