fix(proactive-refresh): redact account email through maskEmail in log calls

[ndycode]

Summary

Three log calls in lib/proactive-refresh.ts emitted email: account.email as a raw string:

• lib/proactive-refresh.ts:113 — "Proactively refreshing token"
• lib/proactive-refresh.ts:127 — "Proactive refresh succeeded"
• lib/proactive-refresh.ts:134 — "Proactive refresh failed"

Why this matters

lib/AGENTS.md (lib/** section) is explicit:

focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

The shared logger in lib/logger.ts does auto-redact any key whose normalized name is in SENSITIVE_KEYS (email is on that list), but it routes through maskToken, which produces partial-leak output for emails longer than 12 chars:

Input	Auto-mask via maskToken	Email-aware mask via maskEmail
user.example@longdomain.org	user.e...n.org	us***@***.org
firstname.lastname@gmail.com	firstn...l.com	fi***@***.com

The token-style mask still exposes most of the local part and most of the domain, which makes correlation across logs trivial. The email-aware mask (already used in lib/audit.ts and the embedded EMAIL_PATTERN in maskString) produces a much smaller fingerprint.

Changes

• lib/proactive-refresh.ts — import maskEmail from the same logger module; rewrite the three log calls to spread { email: maskEmail(account.email) } only when the field is defined (avoids emitting email: undefined).
• test/proactive-refresh.test.ts — two new regression tests:
  • redacts the account email through maskEmail in every log path spies on logger.maskEmail and asserts it is called with the raw email across success and failure paths.
  • omits the email field entirely when account has no email guards against accidentally emitting email: undefined.

Test plan

• npm run typecheck
• npx eslint
• npx vitest run test/proactive-refresh.test.ts — 31/31 pass (29 existing + 2 new)
• Full suite — 3746 / 3746 pass

Notes

This was found via a deeper PII-redaction sweep of lib/ after #443. I scanned all log.{info,debug,warn,error} calls in lib/ for raw email/token interpolation. These three were the only sites still using raw account.email; everywhere else either uses accountId, an account fingerprint helper, a tokenSuffix, or already routes through maskEmail.

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

fixes raw account.email leaking into three log calls in lib/proactive-refresh.ts by importing maskEmail and emitting the field as emailMasked (conditional spread) — the key rename also sidesteps the sanitizeValue double-mask path since "emailmasked" is not in SENSITIVE_KEYS.

Confidence Score: 5/5

safe to merge — production code change is correct and the single p2 test gap doesn't affect runtime behavior

all three log sites are fixed consistently, key rename correctly avoids double-masking, conditional spread prevents undefined emission; only finding is a vacuous spy in the omit-email test which doesn't affect production correctness

test/proactive-refresh.test.ts — the console.info spy in the "omits email" test should be console.log

Important Files Changed

Filename	Overview
lib/proactive-refresh.ts	renames email key to emailMasked and wraps with maskEmail() — correctly avoids double-masking via sanitizeValue and prevents emitting emailMasked: undefined via conditional spread
test/proactive-refresh.test.ts	adds two new regression tests; the maskEmail spy and the no-call assertion are sound, but the payload-content assertions in the "omits email" test are vacuously true because console.info is spied instead of console.log

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[proactiveRefreshAccount called] --> B[log.info: Proactively refreshing token]
    B --> C{account.email defined?}
    C -- yes --> D["maskEmail(account.email) → emailMasked: us***@***.org"]
    C -- no --> E[key omitted entirely]
    D --> F[sanitizeValue: key emailMasked not in SENSITIVE_KEYS]
    E --> F
    F --> G[maskString passes masked value unchanged — no double-mask]
    G --> H{queuedRefresh result}
    H -- success --> I[log.info: Proactive refresh succeeded + emailMasked]
    H -- failed --> J[log.warn: Proactive refresh failed + emailMasked]

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: test/proactive-refresh.test.ts
Line: 252-272

Comment:
**`console.info` spy never fires for info-level log calls**

`logToConsole` in `logger.ts` routes info-level messages through `console.log`, not `console.info` (line 197: `else console.log(...)`). spying on `console.info` here means `infoSpy.mock.calls` is always empty, so the loop that asserts absence of `email`/`emailMasked` keys never executes and the payload-content checks pass vacuously. swap to `console.log` to actually exercise the assertion:

```suggestion
			const infoSpy = vi
				.spyOn(console, "log")
				.mockImplementation(() => undefined);
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (3): Last reviewed commit: "test(proactive-refresh): assert email ke..." | Re-trigger Greptile}

Context used:

• Context used - lib/AGENTS.md (source)

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

📝 Walkthrough

Walkthrough

this change redacts account email from proactive refresh logging by importing maskEmail and conditionally logging a masked email field instead of raw email values across three log statements.

Changes

Cohort / File(s)	Summary
Email Redaction in Proactive Refresh lib/proactive-refresh.ts, test/proactive-refresh.test.ts	Updated logging statements to conditionally emit emailMasked via maskEmail(account.email) instead of raw account.email. Test coverage verifies maskEmail is invoked for both success and failure cases, and never invoked when email is absent.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Notes for reviewer

• verify maskEmail implementation handles edge cases (null, undefined, empty string) consistently across all call sites. the conditional check when account.email is present in lib/proactive-refresh.ts:line needs verification that it matches the test assertions.

• scan codebase for other places emitting account.email in logs (outside this diff) — this fix may need to be applied elsewhere in the same pattern.

• test coverage in test/proactive-refresh.test.ts is solid: it asserts the spy call count with a "lower-bound" check, which is sensible given multiple log statements per flow. confirm the exact line counts match actual log statements (refreshing token, succeeded, failed = 3 statements minimum).

• no concurrency or windows edge cases introduced here.

Suggested labels

bug

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	title exceeds 72-character limit (75 chars) despite following conventional-commits format and covering the main change accurately.	shorten to 72 chars or less; try 'fix(proactive-refresh): redact account email in logs' (56 chars) or similar.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	pr description comprehensively documents the security fix, masking strategy trade-offs, test coverage, and greptile feedback. all required template sections are addressed except validation checklist marks remain unchecked.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/proactive-refresh-redact-emails

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch fix/proactive-refresh-redact-emails

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/proactive-refresh.test.ts`:
- Line 241: The test "omits the email field entirely when account has no email"
currently only checks that maskEmail is not called (maskEmail), which doesn't
prove the payload lacks an email key; update the test to assert the key is
absent (e.g., expect(payload).not.toHaveProperty('emailMasked') or equivalent)
OR rename the test to "does not call maskEmail when account has no email" to
match the existing assertion, and adjust assertions around where the payload is
produced so the code paths that could produce emailMasked: undefined are
rejected.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 136950d6-f736-4cc7-bb81-5f573f75917c

📥 Commits

Reviewing files that changed from the base of the PR and between 2e2211a and 1ea5beb.

📒 Files selected for processing (2)

• lib/proactive-refresh.ts
• test/proactive-refresh.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (2)

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

• test/proactive-refresh.test.ts

lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

• lib/proactive-refresh.ts

🔇 Additional comments (2)

lib/proactive-refresh.ts (1)

14-14: good pii redaction wiring.

lib/proactive-refresh.ts:113, lib/proactive-refresh.ts:127, and lib/proactive-refresh.ts:134 now avoid raw email logging and only include masked email when present. that aligns with the added vitest coverage in test/proactive-refresh.test.ts:207 and test/proactive-refresh.test.ts:241.

As per coding guidelines, "check for logging that leaks tokens or emails."

Also applies to: 113-113, 127-127, 134-134

test/proactive-refresh.test.ts (1)

12-12: good deterministic regression coverage for mask paths.

test/proactive-refresh.test.ts:207 covers both success and failure log flows, and the logger module import at test/proactive-refresh.test.ts:12 enables direct maskEmail spying with vitest.

As per coding guidelines, "test/**: tests must stay deterministic and use vitest."

Also applies to: 207-207, 214-239