[VBLOCKS-6683] fix secops 24657

[afalls-twilio]

Fixed some issues that were reported by a sec-opts ticket, and verified a bunch of dependabot issues. These required some minor code changes to refelect modenerizaiton.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

• I acknowledge that all my contributions will be made under the project's license.

[ramapal]

Comprehensive PR Review

Automated multi-pass review covering bug analysis and test coverage. Findings are ranked by severity; each includes a concrete failure scenario.

---

Bug Analysis (3 Independent Passes)

🔴 Critical

1. Two FirebaseMessagingService subclasses registered for the same intent action — one will silently never fire
exampleVideoInvite/src/main/AndroidManifest.xml

After the migration, both NotifyFirebaseMessagingService (message handler) and NotifyFirebaseInstanceIDService (token-refresh handler) extend FirebaseMessagingService, and both are declared in the manifest with the same intent filter:

<service android:name=".notify.fcm.NotifyFirebaseMessagingService">
  <intent-filter><action android:name="com.google.firebase.MESSAGING_EVENT"/></intent-filter>
</service>
<service android:name=".notify.fcm.NotifyFirebaseInstanceIDService">
  <intent-filter><action android:name="com.google.firebase.MESSAGING_EVENT"/></intent-filter>
</service>

Firebase Messaging routes an event to only one service (the first resolved). Depending on manifest order, either onMessageReceived or onNewToken will silently stop working. In the legacy API, FirebaseInstanceIdService listened for INSTANCE_ID_EVENT — a different action — so they coexisted. That is no longer the case.

Fix: Delete NotifyFirebaseInstanceIDService and move its onNewToken override into NotifyFirebaseMessagingService (and remove the @SuppressLint("MissingFirebaseInstanceTokenRefresh") — it will no longer be needed).

Failure scenario: New device install → onNewToken fires but the registered service (NotifyFirebaseMessagingService) has no override → token is never sent to the server → user never receives video invites. Or the reverse: NotifyFirebaseInstanceIDService wins routing → incoming push messages are dropped.

---

2. bind() deletes the null-guard on the Firebase token — NPE on cold-start races
RegistrationIntentService.java:96–99

The previous implementation had an explicit null check:

if (newAddress == null) {
    Log.w(TAG, "The Firebase token is not available yet.");
    return;
}

That guard has been removed. Tasks.await(FirebaseMessaging.getInstance().getToken()) can complete successfully with a null result on a fresh install (Firebase Installations not yet provisioned). The very next line newAddress.equals(address) will then NPE and crash the service.

Fix: Re-add the null check (or Objects.equals) before the comparison, and treat null as a retriable state (return early, log, wait for onNewToken).

---

🟠 High

3. Tasks.await(...) without a timeout — can hang the IntentService indefinitely
RegistrationIntentService.java:93–95

String installationId = Tasks.await(FirebaseInstallations.getInstance().getId());
newAddress = Tasks.await(FirebaseMessaging.getInstance().getToken());

If the Firebase task never resolves (no network, GMS bug, GCM channel down), Tasks.await blocks the IntentService worker thread forever. The service will pile up queued intents behind it and eventually be ANR'd or killed by the framework when the process is trimmed.

Fix: Use the overload with a timeout: Tasks.await(task, 30, TimeUnit.SECONDS). Also handle TimeoutException distinctly from other failures so the user gets a retry rather than a permanent failure.

---

4. catch (Exception e) swallows InterruptedException without restoring the interrupt flag
RegistrationIntentService.java:97–101

Tasks.await throws InterruptedException. Catching it in a broad catch (Exception e) and not calling Thread.currentThread().interrupt() violates Java's interruption contract and can prevent the IntentService's worker from exiting cleanly on shutdown.

Fix:

} catch (InterruptedException e) {
    Thread.currentThread().interrupt();
    // handle...
} catch (ExecutionException | TimeoutException e) { ... }

---

5. jackson-databind pinned to 2.7.9.7 — still has known CVEs
exampleVideoInvite/build.gradle:59–62

The comment says "mitigate CWE-502," but jackson-databind:2.7.9.7 (released 2018) has known deserialization CVEs of its own (e.g., CVE-2019-14540, CVE-2020-36518). If this is going through a SecOps review, use a currently-supported line (2.15.x or 2.17.x). Retrofit 2.3.0 is Jackson-agnostic and works fine with modern Jackson.

Also: retrofit:2.3.0 itself is from 2017. Consider bumping to 2.11.x while you're touching this — most of the security tickets you're chasing chain through transitive deps of the retrofit line.

---

6. response.body().identity — NPE if the server returns 200 with an empty body
RegistrationIntentService.java:65

response.isSuccessful() only means HTTP 2xx. It does not guarantee body() is non-null (a 204, or a truncated response, or a Jackson deserialization producing an empty object). This existed before the PR, but the surrounding catch (Exception e) now hides the NPE inside a generic "Fetching token failed" message that loses the stack trace — making it harder to diagnose in the field.

Fix: Explicitly check response.body() != null before dereferencing, and log the underlying exception (Log.e(TAG, message, e)).

---

🟡 Medium

7. identity from SharedPreferences can be null — passed straight into fetchToken(identity)
RegistrationIntentService.java:63

String identity = sharedPreferences.getString(IDENTITY, null);
try {
    Response<Token> response = TwilioSDKStarterAPI.fetchToken(identity).execute();

If the shared-preference key is absent (first launch), identity is null. Depending on Retrofit's converter and the server, this may produce a URL like /token/null or a 400. It's not new to this PR, but the exception-swallow makes the null-flow much less debuggable now.

---

8. Semantic drift: FirebaseInstanceId.getId() → FirebaseInstallations.getId()
RegistrationIntentService.java:94

The endpoint format is identity + "@" + installationId. The old Instance ID and the new Installation ID are not the same value. Any user whose ENDPOINT was stored on-device before this change will see the newEndpoint.equals(endpoint) comparison fail on first upgrade, forcing a rebind. For a quickstart this is fine; if this pattern is copied into production code, note that server-side binding history keyed on the old ID becomes orphaned.

---

9. androidx.core:core:1.7.0 and firebase-installations:17.1.3 hardcoded and outdated
exampleVideoInvite/build.gradle:54, 56

These bypass the top-level versions map. androidx.core:1.7.0 is from 2021; current is 1.13+. firebase-installations:17.1.3 is from 2022. Since the goal of this PR is remediating dependabot alerts, hardcoding old versions here undoes some of that work. Move to the versions block and bump.

---

10. Unused import java.io.IOException;
RegistrationIntentService.java:25

Left over from before the catch (Exception e) collapse.

---

🔵 Low / Nit

11. NotifyFirebaseInstanceIDService.onNewToken(String token) receives the token but discards it

@Override
public void onNewToken(String token) {
    Log.d(TAG, "Refreshed token: " + token);
    sendRegistrationToServer();  // param stripped, then fetched again later
}

Passing it through would save a redundant Tasks.await(FirebaseMessaging.getInstance().getToken()) round-trip in bind(). (Moot if you consolidate the two services per finding #1.)

12. Logging tokens at DEBUG level

Log.d(TAG, "Refreshed token: " + token) prints the FCM registration token. Not a hard secret, but SecOps typically flags it. Consider Log.d(TAG, "Refreshed token received (len=" + token.length() + ")").

13. README instruction risks silent breakage

The README now tells users to hand-patch ServerApp.java in sdk-starter-java to add a VideoGrant. If the upstream file changes shape, the instruction becomes lossy. Consider linking to a forked/pinned commit of sdk-starter-java or including a patch file.

---

Test Coverage Analysis

This is an example / quickstart repo — there are no existing unit or instrumentation tests for the notify package. The PR adds no tests, which is consistent with the repo's convention. That said, the changes touch two areas that historically break silently on Firebase upgrades:

Method / Behavior	Coverage	Recommendation
RegistrationIntentService.bind() — endpoint derivation from InstallationId	None	If any tests are added, this is the highest-value target: it's the piece that determines whether push routing works at all. A unit test around a small helper (String makeEndpoint(String identity, String installationId)) would catch the semantic drift called out in finding #8.
RegistrationIntentService.register() — retrofit synchronous execute path	None	Robolectric + MockWebServer (OkHttp) would let you assert 2xx / 4xx / body-null branches without live network.
NotifyFirebaseInstanceIDService.onNewToken()	None	Untestable in current form (no injection of the intent target); refactor to accept an Intent factory if you want to test.
Tasks.await(...) timeout/cancellation semantics	None	Not worth unit testing directly, but if #3 is fixed with a timeout, a smoke test that the service exits on Firebase failure within N seconds would be valuable — this is the failure mode most likely to hit production users.

Manual verification the PR description does not mention but should cover before merge:

• Cold-install: verify onNewToken fires and the server binding is created (this is the case that will regress from finding Fix issue with early initialization on Android M #1).
• Warm-launch: send a test push via Notify and confirm onMessageReceived fires.
• Airplane-mode launch: confirm Tasks.await doesn't hang the IntentService (currently it will — finding Added README. #3).
• Upgrade from prior version with existing ENDPOINT/ADDRESS in SharedPreferences: confirm rebind path executes without crash (finding Updated README, Moved local camera view to picture-in-picture #8).

---

Summary

Must-fix before merge:

1. Consolidate the two FirebaseMessagingService subclasses (Critical Fix issue with early initialization on Android M #1) — this almost certainly breaks either push receipt or token refresh right now.
2. Restore the null-guard on the Firebase token (Critical Adding option 2 for download token #2) — deterministic crash on some cold-start races.
3. Add a timeout to Tasks.await (High Added README. #3) — ANR risk.

Should-fix before merge:
4. Bump jackson-databind off 2.7.x (High #5) — this PR is a SecOps remediation; 2.7.9.7 still has open CVEs.
5. Preserve interrupt flag in the catch block (High #4).
6. Null-check response.body() (High #6).

Nice-to-have: move the two hardcoded lib versions into versions{}, remove the unused import, drop the token from the debug log.

🤖 Generated by /review-pr — 3 bug-finding passes + 1 test-coverage pass, findings deduplicated.

[ramapal]

Follow-up: Does this actually close SECOPS-24657?

Pulled the ticket to check remediation criteria against the PR. The ticket is a Snyk auto-generated stub — the description, severity, CVE list, and remediation criteria fields are all empty (only the standard triage boilerplate). The only comment is Jira automation pinging the assignee to transition it to In-Progress.

• Title: [SNYK] twilio/video-quickstart-android(master) - Vulnerable and Outdated Components of Critical Severity with Mature Exploits
• Status: Open (never moved to In-Progress) · Priority: unset · Resolution: unset
• Labels: snyk, snyk-no-auto-owners
• Related: VBLOCKS-6683 (Android security PR roll-up, in REVIEW)

Implications for merging:

1. No documented CVE list to verify against. The PR is written from the underlying Snyk report; the Jira ticket itself doesn't record what "fixed" looks like. Anyone re-verifying later has no baseline.
2. The Snyk bucket ("Vulnerable and Outdated Components with Mature Exploits") is CWE-1104-class — a rollup of vulnerable deps, not a single CVE. This aligns with the PR's approach (bump firebase, retrofit, pin jackson-databind) but means closure depends on the next Snyk scan showing zero criticals.
3. jackson-databind:2.7.9.7 will almost certainly get re-flagged. The 2.7 line has been EOL since 2018 and Snyk's DB carries multiple Critical CVEs with mature exploits against it (CVE-2020-36518 and the 2020-3617x series among them). If SECOPS-24657's implicit closure criterion is "Snyk shows no critical/high on master," this PR won't clear it.
4. No SecOps sign-off in the ticket comments — no one has explicitly whitelisted 2.7.9.7.

Suggested path before merge (pick one):

• (a) Update SECOPS-24657 with the actual Snyk vulnerability list and confirm each item is closed by the specific pins in this PR. Get a SecOps 👍 on the ticket. Belt-and-suspenders for the audit trail either way.
• (b) [Recommended] Bump jackson-databind to a supported line (2.15.x or 2.17.x). Retrofit's Jackson converter is version-agnostic — the exclude+pin pattern works unchanged. This makes the next Snyk scan clear the bucket without needing a whitelist conversation.

This is separate from the correctness findings in my earlier review (duplicate FirebaseMessagingService, dropped null guard, unbounded Tasks.await, etc.) — those need to be fixed regardless of how the SecOps question is resolved.

🤖 Follow-up from /review-pr after checking the linked SECOPS ticket.

[afalls-twilio]

Explicit whitelist of the previous patch version is whitelisted according to the secops ticket
Confirm the Upgrade jackson-databind to version 2.9.9.1,2.8.11.4, 2.7.9.6