[thomasluizon]

Removes email PII from the Sentry-captured (Warning-level) auth log templates and makes the Logout action's anonymous access explicit.

Changes

• ResendContactsService — LogContactAdded / LogContactAddFailed drop {Email} and the raw response {Body} (which can echo submitted PII). The failure log keeps the HTTP status code; the now-orphaned response-body read is removed.
• AuthController — LogFailedToSendCode / LogCodeVerificationFailed (both Warning, captured by Sentry) drop {Email}, keeping {Error} and the existing RequestId correlation.
• GoogleAuthCommand — LogWelcomeEmailFailed logs {UserId} (the user's id) instead of {Email}.
• AuthController.Logout — adds the explicit [AllowAnonymous] the csharp-authz rule wants (auth endpoints are exempt per CLAUDE.md).

Scope note

Deliberately scoped to the Sentry-captured Warning-level templates per the task. The two Information-level email logs in AuthController (LogVerificationCodeSent, LogUserLoggedInViaCode) are out of this change's scope and left untouched to avoid conflicting with other in-flight #243 work on this file.

Tests

• New reflection test Logout_IsAnnotatedAllowAnonymous_SoItIsReachableWithoutAuth asserts [AllowAnonymous] is present and no [Authorize] overrides it.
• Full Orbit.Infrastructure.Tests (1508) + Orbit.Application.Tests (2617) green locally; the existing ResendContactsServiceTests failure-path test still exercises the trimmed error log.

No API contract change (internal logging + authz attribute only), so no packages/shared/consumer edits.

Refs thomasluizon/orbit-ui-mobile#243

[claude]

Review Complete

Scope: PR #325 in thomasluizon/orbit-api — fix(api): strip email PII from auth log templates + explicit AllowAnonymous on logout
Recommendation: APPROVE

Severity	Count
Critical (incl. old-client breaks)	0
High	0
Medium	0
Low / Info	0

---

Code Review: PR #325 (thomasluizon/orbit-api)

Scope: PR #325, fix/api-pii-logs-logout-authz to main
Recommendation: APPROVE

Summary

This PR strips email PII from four Sentry-captured Warning-level (and one Information-level) log templates in AuthController, GoogleAuthCommand, and ResendContactsService, replacing {Email} with {UserId} or nothing, and makes AuthController.Logout's anonymous access explicit via [AllowAnonymous]. A new reflection-based test asserts the attribute is present. No DTOs, routes, or contracts changed. This is the first review pass on this PR - no prior comments, reviews, or resolved threads exist.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

None (signal gate: Low/Info are not posted per rubric).

Subagents

Agent	Verdict
security-reviewer	PASS
contract-aligner	N/A - no DTO, route, or shared-type surface touched by this diff (confirmed by PR body: "No API contract change")

Validation

Check	Result
Build (dotnet)	N/A - CI job, separate required check handles this
Tests (dotnet)	N/A - CI job, separate required check handles this

Verification detail

Coverage contract: all 4 changed files given a verdict, ranked worst-first:

1. src/Orbit.Api/Controllers/AuthController.cs (trust boundary, authz surface) - reviewed in full; confirmed [AllowAnonymous] on Logout is a lint/CI-hook-satisfying change, not a runtime behavior change - AuthController has no class-level [Authorize] and there's no global fallback policy in Program.cs/Extensions, so SendCode/VerifyCode/GoogleAuth/Refresh (unchanged) are already just as anonymously reachable today. LogoutSessionCommand uses the refresh token itself as the authorization credential (same pattern as /api/auth/refresh), so anonymous reachability is not a new risk.
2. src/Orbit.Application/Auth/Commands/GoogleAuthCommand.cs - SendWelcomeEmailInBackground correctly threads the new userId param through to the log call while email remains used for its real purpose (SendWelcomeEmailAsync); no dead parameter.
3. src/Orbit.Infrastructure/Services/ResendContactsService.cs - the now-orphaned response.Content.ReadAsStringAsync body read was correctly deleted alongside {Body}/{Email} removal from the log templates; email remains used in the outbound request payload.
4. tests/Orbit.Infrastructure.Tests/Controllers/AuthControllerTests.cs - new reflection test correctly asserts [AllowAnonymous] present and [Authorize] absent on Logout.

Adversarial pass: no Critical/High findings were raised, so no skeptic challenge was required. Cross-model second opinion: not triggered (no Critical survivors). Completeness pass: one pass over the changed surface - nothing missed; the diff is self-contained (logging + one authz attribute + one test file).

Deferred - N/A dimensions & files not verdicted

• DESIGN.md / AI-slop: N/A - no apps/* UI files in this diff (backend-only repo).
• Parity: N/A - frontend-only dimension, orbit-ui-mobile not checked out; doesn't apply to a pure orbit-api diff.
• i18n: N/A - frontend-only dimension; doesn't apply here.
• Contract drift + backward-compat guard: N/A - diff contains no packages/shared or DTO hunks to compare (sibling repo not checked out, but the gate itself never fires since no DTO/route/contract field changed in this diff).
• FEATURES.md parity: N/A - no user-facing feature surface changed; this is an internal logging + authz-annotation change only.
• All 4 changed files received a verdict; nothing left un-reviewed.

What's good

• PII minimization is precise and complete - every touched log call site was checked for leftover dead parameters (none found) and for whether the referenced field is still needed elsewhere (it is, where kept).
• The [AllowAnonymous] addition is correctly reasoned as a no-op relative to actual runtime authorization, and is backed by a durable reflection test rather than a one-off manual check, guarding against future regression if someone later adds a stray [Authorize].
• PR body explicitly scopes out the two Information-level email logs in AuthController to avoid conflicting with in-flight work (#243) - good hygiene, avoids scope creep into a file another change is actively touching.
• RequestId correlation is preserved on every trimmed log line, so the debugging value lost by dropping {Email} is offset by request-level traceability.

Recommendation

No blocking issues. Approve and merge.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[claude]

Review Complete

Scope: PR #325 in thomasluizon/orbit-api — "fix(api): strip email PII from auth log templates + explicit AllowAnonymous on logout"
Recommendation: APPROVE

Severity	Count
Critical (incl. old-client breaks)	0
High	0
Medium	0
Low / Info	1

---

Code Review: PR #325 (thomasluizon/orbit-api)

Scope: fix/api-pii-logs-logout-authz → main (5 files: AuthController.cs, GoogleAuthCommand.cs, ResendContactsService.cs, AuthControllerTests.cs, ResendContactsServiceTests.cs)

Summary

This PR strips {Email} from Warning-level (Sentry-captured) auth failure logs and one Error-level Resend log, replaces one welcome-email failure log's {Email} with {UserId}, and adds explicit [AllowAnonymous] to AuthController.Logout. Prior review: this repo's own bot already reviewed and APPROVED commit 24afb65 (the substantive diff) with zero findings. Since that approval, one incremental commit 5020e82 was pushed — test-only, fixing a SonarCloud coverage-gate failure (66.7% < 80% required) by swapping a NullLogger for an ILogger substitute with IsEnabled stubbed true, so the previously-untested success-path LogContactAdded branch is now exercised. I independently re-verified the full diff (not just the delta) rather than trust the prior pass blindly, and reached the same conclusion.

Findings

Critical

None.

High

None.

Medium

None.

Low / Info

1. src/Orbit.Api/Controllers/AuthController.cs:353,359 — two Information-level logs still emit raw {Email} (LogVerificationCodeSent, LogUserLoggedInViaCode), on every successful send-code/verify-code call — higher volume than the Warning-level ones this PR fixed. Not a regression and not introduced by this diff; the PR body explicitly discloses and scopes this out ("out of this change's scope... to avoid conflicting with other in-flight #243 work on this file"). Flagged here for traceability only — does not block.

Independent re-verification detail

• [AllowAnonymous] on Logout (AuthController.cs:259) — verified myself, not just trusted the PR description: AuthController has no class-level [Authorize] (AuthController.cs:16), and ServiceCollectionExtensions.cs:164-168's AddAuthorization registers only AdminPolicy — no FallbackPolicy/RequireAuthenticatedUser default. Under ASP.NET Core defaults an action with neither attribute is already anonymous-reachable, identical to the unchanged Refresh action (AuthController.cs:206, still no attribute). This addition is behavior-preserving (satisfies the csharp-authz lint intent, not a runtime change); [DistributedRateLimit("auth")] remains intact on Logout.
• PII removal correctness — checked every touched call site for orphaned params/dead code: ResendContactsService.cs's now-unused response.Content.ReadAsStringAsync() was correctly deleted alongside {Body}; GoogleAuthCommand.cs's email param is still legitimately used for SendWelcomeEmailAsync (not logged) while the log call now takes userId. No leftover PII, no dead parameters.
• New commit 5020e82 (test-only) — read the full ResendContactsServiceTests.cs. Swapping NullLoggerFactory for an NSubstitute ILogger with IsEnabled(Arg.Any<LogLevel>()) → true correctly makes the success-path LogContactAdded call (guarded by logger.IsEnabled(LogLevel.Information)) actually execute under test, which is exactly what the coverage gate was missing. No assertions in the other two tests depend on IsEnabled being false, so no collateral breakage.
• Security-reviewer subagent dispatched independently on the three changed source files plus the AddAuthorization config — confirms the same conclusions above; its one MEDIUM candidate (the two untouched Info-level email logs) is the same pre-existing, PR-disclosed item captured as Low/Info above, not a new Medium.

Subagents

Agent	Verdict
security-reviewer	PASS (one pre-existing/disclosed Info-level observation, not a diff regression)
contract-aligner	N/A — no DTO, route, or packages/shared surface touched; PR body confirms "No API contract change"

Validation

Check	Result
Build (dotnet)	N/A — CI job, separate required check
Tests (dotnet)	N/A — CI job, separate required check

Prior review / thread check

One prior review exists from this bot (claude, state APPROVED, submitted against commit 24afb65) with a full detailed pass reaching the same no-findings conclusion this pass reaches. No requested-changes reviews or open threads to re-address. The only new activity since that approval is the coverage-fix commit 5020e82, reviewed fresh here.

Deferred — N/A dimensions & files not verdicted

• DESIGN.md / AI-slop: N/A — no apps/* UI files (backend-only repo).
• Parity / i18n: N/A — frontend-only dimensions, not verifiable from orbit-api (orbit-ui-mobile not checked out per task scope).
• Contract drift + backward-compat guard: N/A — no DTO/route/shared-type field changed.
• FEATURES.md parity: N/A — internal logging + authz-annotation change only, no user-facing feature surface.
• All 5 changed files received a verdict (including the coverage-fix commit's sole file); nothing left un-reviewed.

What's good

• PII minimization is precise: every touched log call site was checked for orphaned parameters and for whether the removed field is still needed elsewhere (it is, where kept) — no dead code introduced.
• [AllowAnonymous] addition backed by a durable reflection test (Logout_IsAnnotatedAllowAnonymous_SoItIsReachableWithoutAuth) rather than a one-off manual check, guarding against a future regression.
• PR body proactively discloses the Info-level email logs left out of scope and why (avoiding conflict with in-flight #243) — good hygiene, not scope creep.
• The coverage-fix commit is minimal and targeted: it doesn't touch production code, only makes the existing test actually exercise the branch it already claimed to cover.

Recommendation

No blocking issues. Approve and merge. Optionally track the two Information-level {Email} logs (AuthController.cs:353,359) as a fast-follow once #243 lands, since the PR itself flags this as intentionally deferred.