Skip to content

fix(security): redact email PII from auth-flow logs #4254

@realmeylisdev

Description

@realmeylisdev

Background

Reviewer @rabble flagged a PII-in-log concern on PR #3784 (review thread point 4):

PII in warning log. PendingVerificationService.load() logs the email address when discarding pre-fix pending verification. Safer to omit or redact the email unless auth email logging is already explicitly accepted.

A repo-wide audit found this is the established convention across the auth flow — 7 call sites log email at info/warning level today. Redacting only the one line rabble flagged would create inconsistency without security benefit; the systematic fix was deferred from #3784's emergency-security scope to its own follow-up.

This issue tracks the systematic redaction.

Sites to redact (audit, on origin/main)

File Line Level Current shape
lib/blocs/divine_auth/divine_auth_cubit.dart 275 Log.info 'Email verification required for $email'
lib/blocs/divine_auth/divine_auth_cubit.dart 388 Log.info 'Sending password reset email to $email'
lib/blocs/email_verification/email_verification_cubit.dart 75 Log.info 'startPolling called for $email ...'
lib/services/pending_verification_service.dart 68 Log.info 'Saved pending verification for $email'
lib/services/pending_verification_service.dart 139 Log.info 'Pending verification for $email has expired, clearing'
lib/services/pending_verification_service.dart 148 Log.info 'Loaded pending verification for $email'

Plus one site on the unmerged #3784 branch:

File Line Level Current shape
lib/services/pending_verification_service.dart 118 (post-#3784) Log.warning 'Discarding pre-fix pending verification for $email ...'

The Crashlytics path through Reportable.toString() strips npub/nsec but not email — broadening the sanitizer is part of this issue.

Open decision: redaction strategy

Option Output for user@example.com Trade-off
A — opaque [REDACTED] Matches existing redactedSensitiveLogPlaceholder convention; loses triage value
B — partial (recommended) u***@example.com Preserves domain for debugging; lets ops correlate same-account log entries; GDPR-friendly

Affected files

File Action Description
lib/utils/sensitive_uri_for_logs.dart Modify Add redactEmailForLogs(String) helper
lib/observability/reportable_error.dart Modify Extend sanitizeForCrashReport to strip email patterns
lib/blocs/divine_auth/divine_auth_cubit.dart Modify Redact 2 call sites (275, 388)
lib/blocs/email_verification/email_verification_cubit.dart Modify Redact 1 call site (75)
lib/services/pending_verification_service.dart Modify Redact 3 call sites (68, 139, 148) on main; +1 site (118) after #3784 merges
test/utils/sensitive_uri_for_logs_test.dart Modify (or Create) Unit tests for the new helper
test/observability/reportable_error_test.dart Modify (or Create) Tests for email stripping in Crashlytics path

Implementation steps (bottom-up)

  1. Util — add redactEmailForLogs in lib/utils/sensitive_uri_for_logs.dart. Partial-redactor (Option B) that returns redactedSensitiveLogPlaceholder for empty / malformed input. Single regex pass, no allocation on empty input.
  2. Observability — broaden Crashlytics sanitizer in lib/observability/reportable_error.dart. Add an _emailPattern regex parallel to _npubPattern / _nsecPattern; replace matches with the partial-redacted form. Defense-in-depth even if a call site forgets to redact.
  3. BLoCs — redact 3 call sites: divine_auth_cubit.dart:275, divine_auth_cubit.dart:388, email_verification_cubit.dart:75. Pattern: '… for ${redactEmailForLogs(email)}'.
  4. Service — redact 3 call sites in pending_verification_service.dart (68, 139, 148). Same pattern.
  5. Tests — new helper-level tests + Crashlytics sanitizer tests. Verify existing behavioural tests in the 7-site files still pass.

Testing

Layer Test File What to test
Util test/utils/sensitive_uri_for_logs_test.dart redactEmailForLogs across valid, malformed, unicode, single-char local-part
Observability test/observability/reportable_error_test.dart sanitizeForCrashReport strips email alongside npub/nsec
Regression existing tests in pending_verification_service_test.dart, email_verification_cubit_test.dart, divine_auth_cubit_test.dart Confirm behaviour-level tests still pass

Risks

  • Log correlation loss — ops may rely on email-in-logs for incident triage. Mitigation: Option B preserves enough to correlate.
  • Missed future sites — new code may add email logs that bypass the helper. Mitigation: rely on review; optional follow-up to add a CI guard script (mirror of grep_for_nsec_payload.sh).
  • Scope creep — other PII (phone, names) may exist in logs. Out of scope for this issue; file separately if discovered.

Coordination with #3784

This issue's PR is independent of #3784 — they touch overlapping code in pending_verification_service.dart but neither blocks the other. Whichever PR merges second receives a small rebase to also redact line 118 (the migration-warning site #3784 added).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No fields configured for Task.

    Projects

    Status
    Testing

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions