[paulb777]

Summary

Realtime Database no longer crashes the app when a server protocol message is
missing a field or is otherwise malformed. Missing fields now get safe
fallbacks, and any other exception while processing a server message closes the
connection, which then reconnects, instead of crashing the app.

Problem

This comes from a crash-risk audit of server-controlled payloads, prompted by
firebase/firebase-ios-sdk#16728.

WebsocketConnection parses server messages, Connection.onMessage dispatches
them, and PersistentConnectionImpl handles them synchronously on the database
run loop. Several fields were dereferenced without null checks, and
Connection only caught ClassCastException. Any other exception reached
DefaultRunLoop, and the AndroidPlatform run loop rethrows it on the main
thread, which crashes the app. A backend change that drops or nulls one of these
fields would crash every client, typically on every connect (that is, at
launch):

Server message	Previous result
Handshake (h) without ts, with ts: null, or without a payload	NPE in Connection.onHandshake
Response with r but no b, for example if the backend stops answering the stats request that clients send on their first connect	NPE in the request callback
Response body without s	NPE on status.equals("ok") in every request callback
Listen acknowledgement without d, or with w: null	NPE while reading listen warnings
Kill (s) control message without a reason	NPE in PersistentConnectionImpl.onKill
Push (a) without b, or a data, merge, range merge, or listen cancellation push without p	NPE in onDataPush

Nothing is persisted, so the crashes stop once the server is fixed, but until
then every app that uses Realtime Database crashes.

Fix

• Safety net at the dispatch boundary. Connection.onMessage, and
  onControlMessage, which has its own catch, now catch RuntimeException
  instead of only ClassCastException, log a warning, and close the connection.
  This is the right boundary because all processing of a server message,
  including the handling and request callbacks in PersistentConnectionImpl,
  runs synchronously under it. Closing the connection makes
  PersistentConnectionImpl reconnect with its existing exponential backoff (up
  to 30 seconds). Errors, such as failed hardAsserts, are not caught, and
  the run loop exception policy in AndroidPlatform is unchanged, so exceptions
  from elsewhere still surface as before.
• Targeted, null-safe handling of the known triggers:
  • Handshake: any Number is accepted for ts. If it is missing or not a
    number, the client logs a warning and uses the local clock (a server time
    offset of zero), so the connection still works. A handshake without a
    payload is treated as empty, because all of its fields are optional.
  • Responses: a missing or non-map b is treated as an empty body. A missing
    or non-string s fails the request with the status invalid_response,
    which maps to DatabaseError.UNKNOWN_ERROR. The status passed on must not be
    null: Repo treats a null error code as success, and
    DatabaseError.fromStatus(null, ...) throws. Error messages are read with a
    type check, and status comparisons are constant-first
    ("ok".equals(status)).
  • Listen acknowledgements: a missing body or warnings list means no warnings.
  • Kill: INVALID_APP_CHECK_TOKEN.equals(reason), so a missing reason keeps
    the existing behavior (stop reconnecting and log the reason).
  • Pushes without an action, a body, or (for pushes about a location) a path
    are ignored with a warning.
• Warnings never include message payloads, which can contain user data. Existing
  debug logs are unchanged.

Alternatives considered:

• Only adding the catch-all: it prevents the crash, but a dropped stats
  response or a handshake without ts would then reset every connection, so
  the client would never stay connected. The known triggers get targeted
  handling instead.
• Treating a push without a path as a push to the root: rejected, because it
  could overwrite cached data.
• Changing the run loop exception policy in AndroidPlatform: rejected, because
  it would also hide unrelated bugs.

Testing

New JVM tests (Robolectric, Mockito, and Truth, all existing test dependencies)
under firebase-database/src/test/java/com/google/firebase/database/connection/.
This package had no unit tests before.

• ConnectionTest (12 tests): handshakes with a missing, null, non-numeric,
  or integer ts, a handshake without a payload, a kill without a reason, a
  data message without a payload, a RuntimeException from the delegate for
  data and control messages (the connection is closed and the delegate is
  notified of the disconnect), and an Error that still propagates.
• PersistentConnectionImplTest (20 tests): connects with a mocked
  Connection (through Mockito.mockConstruction, so no WebSocket is opened
  and no test-only production code is needed), then delivers responses without
  a body or status for the stats, put, listen, get, onDisconnect, auth, and App
  Check requests, listen acknowledgements without data or with w: null, a kill
  without a reason, and pushes without a body or path. Well-formed responses
  and pushes are tested too, to show that existing behavior is unchanged.

Before the fix, 22 of the 32 new tests failed: with the NPEs listed above, with
the exception escaping Connection.onMessage, or, for non-numeric and integer
handshake timestamps, with the connection being closed instead of becoming
ready. After the fix, all of them pass.

Commands:

./gradlew --max-workers=4 :firebase-database:spotlessApply
./gradlew --max-workers=4 :firebase-database:testDebugUnitTest --tests "com.google.firebase.database.connection.*"
./gradlew --max-workers=4 :firebase-database:testDebugUnitTest

Results: spotlessApply succeeded. The connection tests ran 32 tests with 0
failures. The full module run ran 435 tests with 0 failures and 2 skipped (both
already ignored before this change).

CI note: on Firebase Test Lab, Instrumentation Tests (:firebase-database)
reported 11 failed and 476 passed for this PR. The same job ran at about the same time on
#8758, an unrelated PR that doesn't touch the database code, and reported 12
failed and 475 passed. So these failures were already there before this
change.

[gemini-code-assist]

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

[github-actions]

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

[paulb777]

/gemini review

[gemini-code-assist]

Code Review

This pull request improves the robustness of the Firebase Realtime Database connection layer by adding validation for server messages and handshakes, catching runtime exceptions during message processing to prevent crashes, and ignoring malformed server pushes. It also adds comprehensive unit tests for these connection behaviors. The review feedback highlights two improvement opportunities in PersistentConnectionImpl.java: providing a fallback error message when a query fails with a null error body, and extending the push validation to verify that the body is a list for range merge actions to prevent potential class cast exceptions.

[paulb777]

/gemini review

[gemini-code-assist]

Code Review

This pull request improves the robustness of the Firebase Realtime Database connection handling by safely processing missing or malformed server messages instead of crashing. Specifically, it catches general runtime exceptions instead of only class cast exceptions, introduces validation helpers like isWellFormedPush and getResponseStatus to verify server payloads, and adds comprehensive unit tests. The reviewer recommended expanding the validation in isWellFormedPush to thoroughly check list elements for range merges and other push actions to prevent potential class cast exceptions and connection loops.

[paulb777]

/gemini review

[gemini-code-assist]

Code Review

This pull request improves the robustness of the Realtime Database connection handling by safely processing missing or malformed server messages, preventing crashes, and falling back to safe defaults (such as local time or empty maps) when fields are invalid. It replaces specific exception catching with broader runtime exception handling, introduces payload validation helpers, and adds comprehensive unit tests to verify these scenarios. A review comment suggests adding a defensive null check to the getString helper method to prevent potential null pointer exceptions in future usages.

[paulb777]

/gemini review

[gemini-code-assist]

Code Review

This pull request improves the robustness of the Realtime Database connection handling by safely processing missing or malformed server messages instead of crashing. It replaces direct casts with safe type checks, introduces validation for server pushes, and adds comprehensive unit tests to verify these recovery behaviors. The reviewer suggested making the retrieval of host and session ID fields in the handshake null-safe and type-safe to prevent potential class cast exceptions and unnecessary reconnect loops.