Skip to content

Fix log redaction, device registration race, and incomplete reset - #13

Merged
mohn93 merged 1 commit into
mainfrom
fix/logging-pii-registration-race-reset
Aug 4, 2026
Merged

Fix log redaction, device registration race, and incomplete reset#13
mohn93 merged 1 commit into
mainfrom
fix/logging-pii-registration-race-reset

Conversation

@mohn93

@mohn93 mohn93 commented Aug 4, 2026

Copy link
Copy Markdown
Member

Closes #9, closes #10, closes #11.

Replaces #12, which GitHub auto-closed when #8 merged and its base branch was
deleted. Same branch, same commit, now based on main.

#9 — credentials and PII in the logs

Applies only when enableLogging: true, so a debug-mode leak rather than a
default one.

Three sites wrote secrets verbatim:

  • device_service.dart:76 logged Device.toJson(), which carries the full FCM
    token. An FCM token is a send capability, and device logs are read by other
    tooling and swept up by crash collectors.
  • subscriber_service.dart:50 logged the login payload: name, email, phone and
    the whole metadata map.
  • tag_service.dart:17 and :62 logged tag values, which routinely hold an
    email, plan or region.

Every request body also reached logApiRequest verbatim, repeating the same
values at debug level.

Adds PushFireLogger.redact, applied to device info and to request and response
bodies. The token keeps the existing first/last-ten mask so it stays
correlatable with the server; everything else becomes <redacted>.
Identifiers — externalId, deviceId, subscriberId, tagId — are left
alone: they are what a support ticket is traced by.

Beyond what the issue asked: response bodies are redacted too. register-device
returns the device row and login-subscriber the subscriber row, so redacting
only requests would have reopened the same leak from the other direction. A body
that is not a JSON object — an HTML gateway page — passes through unchanged.

The API key was never affected; the Authorization header is not passed to a
logger.

#10 — registerDevice had no concurrency guard

registerDevice reads the stored device id, makes a network round trip, then
writes the id back. Two callers entering before either wrote both saw no id,
both POSTed, and created two device rows for one device. The second write won
locally, so the first row was orphaned server-side while still holding the same
FCM token.

registerDevice is now single-flight: concurrent callers join the in-flight
registration. The guard is released on failure too, so a failed attempt — a cold
iOS start before APNS answers — does not wedge every later caller.

The permission check is coalesced the same way. The old _isCheckingPermission
flag guarded only the resume path, so a token refresh arriving during a resume
ran two overlapping checks. Both syncNotificationPermission and the
token-refresh handler now go through the shared guard. Callers that join receive
null rather than being dropped, so one permission change still emits exactly one
onDeviceRegistered.

#11 — reset() could leave state behind

Two failure modes:

  • A stored subscriber blob whose id is null does not count as logged in, so
    the gated logout skipped it and the blob — name, email, phone — survived a
    call documented as clearing all local state. On a shared device the next
    user's session started holding the previous user's details.
  • logoutSubscriber clears locally then rethrows, so a failed logout request
    propagated out of reset() and clearDeviceData() never ran, leaving the
    device id, FCM token, permission status and preference behind.

Both clears are now unconditional and run after a logout that cannot escape.
clearSubscriberData is public for that. The teardown is extracted as
clearAllLocalState so it can be tested without a live SDK singleton, which
needs Firebase.

Tests

419 pass, up from 391. Each of the 28 new tests was checked to fail with its own
fix reverted:

  • test/utils/logger_redaction_test.dart — the redaction helpers, plus the
    emitted log records for logDeviceInfo, logApiRequest and logApiResponse.
  • test/services/service_logging_pii_test.dart — what loginSubscriber,
    addTag and updateTag actually write while doing real work.
  • test/services/device_service_concurrent_registration_test.dart — 2 and 5
    concurrent callers produce one POST; a rotated token still reaches the server;
    a failed registration releases the guard; joiners see the failure.
  • test/pushfire_sdk_reset_test.dart — the null-id blob, the failed logout, an
    unexpected throw, and a clean install.

No parity divergence introduced: the native Swift SDK marks bodies
privacy: .private, coalesces registration and the permission check, and clears
both stores unconditionally after a try? logout.

Fixes #9, #10 and #11. The three touch overlapping files, so they land
together.

#9 - credentials and PII in the logs (enableLogging: true only)

Three sites wrote secrets verbatim: device_service.dart logged
Device.toJson(), which carries the full FCM token; subscriber_service
logged the login payload - name, email, phone, metadata; tag_service
logged tag values, which routinely hold an email, plan or region. Every
request body also reached logApiRequest verbatim, repeating the same
values at debug level.

Adds PushFireLogger.redact, applied to device info and to request and
response bodies. The token keeps the existing first/last-ten mask so it
stays correlatable with the server; everything else becomes <redacted>.
Identifiers - externalId, deviceId, subscriberId, tagId - are left
alone: they are what a support ticket is traced by. Response bodies are
redacted too, since register-device returns the device row and
login-subscriber the subscriber row; a body that is not a JSON object
passes through unchanged.

#10 - registerDevice had no concurrency guard

registerDevice reads the stored device id, makes a network round trip,
then writes the id back. Two callers entering before either wrote both
saw no id, both POSTed, and created two device rows for one device. The
second write won locally, so the first row was orphaned server-side
while still holding the same FCM token. Reachable from auto-registration
at init, the token-refresh handler, the foreground permission check and
requestNotificationPermission.

registerDevice is now single-flight: concurrent callers join the
in-flight registration. The guard is released on failure too, so a
failed attempt does not wedge later callers.

The permission check is coalesced the same way. The old
_isCheckingPermission flag guarded only the resume path, so a token
refresh arriving during a resume ran two overlapping checks; both
syncNotificationPermission and the token-refresh handler now go through
it. Callers that join receive null, so one permission change still emits
exactly one onDeviceRegistered.

#11 - reset() could leave state behind

Two failure modes. A stored subscriber blob whose id is null does not
count as logged in, so the gated logout skipped it and the blob - name,
email, phone - survived a call documented as clearing all local state;
on a shared device the next user's session started holding the previous
user's details. And logoutSubscriber clears locally then rethrows, so a
failed logout request propagated out of reset() and clearDeviceData()
never ran, leaving the device id, FCM token and permission state behind.

Both clears are now unconditional and run after a logout that cannot
escape. clearSubscriberData is public for that; the teardown is
extracted as clearAllLocalState so it can be tested without a live SDK
singleton, which needs Firebase.

28 regression tests. Each was checked to fail with its fix reverted.
@mohn93
mohn93 merged commit acd6364 into main Aug 4, 2026
3 checks passed
@mohn93
mohn93 deleted the fix/logging-pii-registration-race-reset branch August 4, 2026 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant