fix(security): redact email PII from auth-flow logs (#4254)#4255
Merged
Conversation
Email addresses were logged in plaintext at 6 sites across the auth flow (3 in PendingVerificationService, 2 in DivineAuthCubit, 1 in EmailVerificationCubit). Flagged in #3784's review (point 4) but kept out of the emergency-security scope of that PR — addressed here as the systematic fix. Approach: - New `redactEmailForLogs(String)` helper in lib/utils/sensitive_uri_for_logs.dart. Partial-redaction (`user@example.com` → `u***@example.com`) preserves the domain so ops can correlate failure patterns per provider without identifying individual accounts. Empty / malformed input returns the existing `redactedSensitiveLogPlaceholder`. - Broadened `sanitizeForCrashReport` in lib/observability/reportable_error.dart to also strip emails before forwarding to Crashlytics — defense-in-depth so any future call site that forgets the helper still gets sanitized when its error flows through `Reportable.toString()`. - All 6 call sites on origin/main wrapped with the helper. The 7th site (the legacy-nsec migration warning added by #3784) is intentionally NOT included here — whichever of #3784 / this PR merges second receives a small rebase to wrap that site too. Tests: - 9 new cases for `redactEmailForLogs` (standard, single-char, long local-part, subdomains, empty, no-`@`, empty local-part, no-TLD domain, whitespace). - 4 new cases for `sanitizeForCrashReport` covering email stripping, multiple emails, mixed npub/nsec/email, and domain preservation. Closes #4254.
NotThatKindOfDrLiz
force-pushed
the
fix/4254-redact-email-from-auth-logs
branch
from
c580c3b to
87d53fa
Compare
NotThatKindOfDrLiz
approved these changes
May 11, 2026
Member
NotThatKindOfDrLiz
left a comment
NotThatKindOfDrLiz
left a comment
There was a problem hiding this comment.
Shared email redaction path is in place, the alignment regression test is covering the cleanup, and checks are green on the current head.
Contributor
Author
|
Thanks @NotThatKindOfDrLiz and @hm21 — both review threads addressed and resolved. The Crashlytics path now routes through the shared |
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
3 participants
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.
Description
Systematic redaction of email PII from auth-flow logs. Flagged in #3784's review (point 4) but kept out of the emergency-security scope of that PR; this PR is the dedicated follow-up.
Audit of
origin/mainfound 7 sites that log email at info/warning level acrossPendingVerificationService,DivineAuthCubit,EmailVerificationCubit, andEmailVerificationScreen. Redacting only the one line Rabble flagged would create inconsistency without security benefit; this PR addresses all 7 sites via a single helper plus a broadened Crashlytics sanitizer.Related Issue: Closes #4254
What's in this PR
mobile/lib/utils/sensitive_uri_for_logs.dartredactEmailForLogs(String)helper (Option B — partial-redact, preserves domain)mobile/lib/observability/reportable_error.dartsanitizeForCrashReportto strip emails alongside npub/nsecmobile/lib/services/pending_verification_service.dartmobile/lib/blocs/divine_auth/divine_auth_cubit.dartmobile/lib/blocs/email_verification/email_verification_cubit.dartmobile/lib/screens/auth/email_verification_screen.dartmobile/test/unit/utils/sensitive_uri_for_logs_test.dartmobile/test/observability/reportable_error_test.dartmobile/test/screens/auth/email_verification_screen_test.dartRedaction strategy
Chose Option B — partial over Option A — opaque (
[REDACTED]):Rationale: preserves the domain so ops can spot patterns like "all gmail.com users are failing" without identifying individual accounts. Local-part collapses to a fixed-width
x***mask regardless of length so original length isn't leaked.Empty / malformed input (no
@, empty local-part, domain without.) falls back to the existingredactedSensitiveLogPlaceholder([REDACTED]).Type of Change
Coordination with #3784
#3784 (still open at time of writing) adds one overlapping auth-log site on top of the 7 handled here:
pending_verification_service.dart:118(post-fix(security): strip BYOK nsec from Keycast OAuth wire (#3359) #3784) — the legacy-nsec migration warning.Whichever PR merges second will need a tiny rebase to wrap that site too. There is no functional dependency between the two PRs; they only overlap in the same service.
Test plan
flutter analyze lib test integration_test— no issuesflutter test test/unit/utils/sensitive_uri_for_logs_test.dart— 20/20 (9 new + 11 existing)flutter test test/observability/reportable_error_test.dart— 17/17 (4 new + 13 existing)flutter test test/blocs/divine_auth/ test/blocs/email_verification/ test/services/email_verification_listener_test.dart— 96/96 (regression on the originally wrapped auth-log sites)flutter test test/screens/auth/email_verification_screen_test.dart— 16/16 (regression on the persisted auto-login log path)Follow-ups (not in this PR)
grep_for_nsec_payload.shto flag any newLog\.(info|debug|warning|error).*emailpatterns that bypass the helper.