[tylerkron]

closes #369

Not merging — for review.

Two pieces of work with very different justifications, kept apart below.

---

1. logentry.params was passing through verbatim — hardening, not a live leak

ScrubAccountNames(SentryEvent) rebuilt the message with Message and Formatted redacted and copied Params across untouched. It serialises as logentry.params.

I re-proved the ticket's reachability claim rather than taking it, and it holds: this is NOT reachable today. SentrySdk is called from exactly four places in this repo — Init, CaptureException twice, AddBreadcrumb twice, ConfigureScope twice — and CaptureMessage and CaptureEvent from nowhere. SentryTarget is deliberately not wired into NLog (AppLogger.cs:183), so no logging integration builds one either. Nothing in the app produces an event carrying a logentry at all, so nothing leaks today and no user data has gone anywhere. Treat this as defensive hardening of a privacy path: it is the same shape #366 was — a redactor that is correct and simply is not on the path — caught one step before anyone walks it. The first CaptureMessage("… {0}", path) added would leak silently.

The fix reuses the existing redactor and the existing hook. One new private overload beside the string one, and one changed line at the call site.

Params is IEnumerable<object>, so it needed a decision the string fields did not. Only string elements go through the redactor; everything else is passed on as it arrived:

element	behaviour	why
string	redacted	the only shape a path actually arrives in
number, bool, any other object	untouched, keeps its JSON type	stringifying to redact would send 42 as "42" — a payload change for every event with a parameter list, well past a scrub
null	untouched	nothing to match
nested collection	untouched, not recursed	same boundary the Extra loop already draws

Disclosure, added while shepherding — the boundary above has a consequence the table does not spell out. A path carried inside a non-string element is not scrubbed. Measured on ea38b2c, through the shipped options and read back out of the serialised envelope:

element	serialises as	path redacted?
new[] { "/Users/octocat/…/DAQifiProfilesConfiguration.xml" }	["/Users/octocat/…"]	no — not recursed into
object with a public string property holding a path	{"Location":"/Users/octocat/…"}	no
new Uri("file:///Users/octocat/Library/uri.xml")	"file:///Users/octocat/Library/uri.xml"	no
object whose path is only in an overridden ToString()	{}	n/a — no leak; the serializer reflects over properties and never calls ToString()

The Uri row is the one that strains the rationale: it is not a string element, so the is string test skips it, but its serialised JSON type is a string, so redacting it would cost no payload compatibility at all. Same for any type with a System.Text.Json converter that renders to a JSON string — Uri and Version being the two that can carry a filesystem path.

Not fixed here, deliberately: this PR is settled and a change to where the leaf boundary sits is its own decision. Filed as #373. It is not reachable today for the same reason the rest of this PR is not — nothing builds a logentry at all.

This is the same rule ScrubAccountNames' Extra loop follows, deliberately, so there is one rule to remember rather than two. The projection is materialised with ToList() so the serializer walks a caller's enumerable once rather than twice.

2. Three scrubbed fields had no test — the higher-value half

logentry, breadcrumb data and server_name were all scrubbed by #367 and pinned by nothing. #366 existed precisely because a correct redactor was not on the path; an untested scrub is that failure waiting to recur, and a future refactor could silently unwire any of the three.

Tests are at the boundary, not at the helper — the lesson #367 established, and the whole reason #366 was invisible. Every new test assembles an event, pushes it through ConfigureSentryOptions (the configuration the app ships), and reads the field back out of the serialised envelope. They extend #367's existing suite rather than starting a parallel one. Nothing leaves the machine: the same unroutable .invalid DSN and the same in-memory RecordingWorker.

server_name gets both halves the issue asks for: an event that carries one has it redacted, and the shipped options never populate the field at all (SendDefaultPii = false), so the default envelope has no server_name. Without the first assertion the scrub line is unpinned; without the second, the belt-and-braces claim in the remarks is.

---

Evidence

Against unmodified main — the two figures are different in kind, so they are split.

The whole file compiles against main (every member it names already exists there), so all 10 new cases genuinely ran there. That is not true of #367's suite, which could not compile against its own base — worth saying, because "does not compile" is not "0 failed".

	result on unmodified main
logentry.params (1 fact + 2 of the 5 table rows)	3 failed — the defect, reproduced
logentry, breadcrumb data, server_name	passed — they characterise behaviour #367 already shipped
type-preservation + the 3 pass-through table rows	passed

26 passed / 3 failed of 29 in the class. The three passing field tests are therefore uninformative as regression evidence on their own, so each was mutation-tested instead: the scrub for one field unwired at a time on otherwise-unmodified main, rebuilt, re-run. Each mutation was killed by exactly the test claiming that field and by nothing else:

mutation	killed by	collateral
Message/Formatted stop being redacted	A_path_in_a_log_message_is_redacted	none
breadcrumb data values stop being redacted	A_path_in_breadcrumb_data_is_redacted	none
@event.ServerName = … removed	A_path_in_the_server_name_is_redacted_and_the_field_is_absent_by_default	none

A fourth mutation was run against this branch, on the new code, to show the over-redaction guard is not vacuous: replacing the string test with RedactAccountNames(parameter?.ToString()) — the over-eager version of this same fix — is killed by Log_message_parameters_that_are_not_strings_keep_their_serialised_type, and by nothing else.

Every mutation was applied by rewriting the source in place, never by a backup-file rename, so no run could have been served a stale assembly.

Over-redaction is the failure mode of this class of fix and #367 shipped a real instance of it (a profile of /Users/octocat turned a sibling's /Users/octocat2/… into ~2/…). The new parameter path is table-driven over the adjacency cases: a strict-prefix sibling account, the account name mid-path under a non-home root, an account name that is an ordinary English word sitting in prose beside a real path, and a string with no path at all that must arrive byte-identical.

Gate

Measured on this branch and on its own merge-base (origin/main), both sides force-rebuilt with --no-incremental — an incremental build reports zero warnings, which reads exactly like a clean result.

	merge-base	this branch	delta
Desktop warnings (all-project)	175	175	0
Desktop warnings (first-party, after the 20 vendored OxyPlot)	155	155	0
iOS Release/iossimulator-arm64 (all-project)	176	176	0
dotnet test	896 passed	906 passed, 0 failed	+10

Both denominators are labelled because they differ. The first-party figure is validated by the identity rather than asserted: the vendored third_party/oxyplot-avalonia warnings count 20 at each end (21 log lines carrying both "warning" and "oxyplot", one of which is that project's own summary line, not a warning), and 155 + 20 = 175 = the all-project total. Filtering on AVP1002 alone would have undercounted — only 13 of the 20 are that code.

No warning in any of the four build logs cites AppLogger.cs. Both builds judged by process exit code, not by grepping for a literal — -tl:on writes ANSI escapes between "Build" and "succeeded", which is also why the per-code tally above had to be done on paths rather than on warning CSxxxx. Neither build was a re-run following an XAML-step failure, so neither could be reporting a bogus success.

git status --short -- '**/packages.lock.json' was empty after the no-RID restore; no lock-file drift to revert.

origin/main was re-fetched after the measurements and is still c45fe32, so these merge-base numbers are against the same base the PR targets.

The Android head does not build on this machine, so I did not claim it locally — CI has since built it green (Android head, 3m21s), along with Library + Desktop head + capture harness.

Final diff, re-derived from git diff --numstat origin/main...HEAD after the last commit: 2 files, +225 / −10 — SentryAccountNameRedactionTests.cs +199/−5, AppLogger.cs +26/−5.

Scope

Legitimately additive, and mostly tests. No second scrubber, no strategy interface, no config knob — the existing redactor and the existing two hooks do all of it; the production change is one new 4-line private overload and one changed line. tags remain unscrubbed by design (a stated decision in AppLogger's remarks), and server-side scrubbing is #100 — neither is touched here. No Sentry event was emitted and the live project's settings were not touched.

Checked against the other open PRs: #368 (LoggedSeriesLegendItem.cs, LegendFormattingTests.cs) and #370 (SummaryFlyout.axaml and its two test files) share no file with this branch.

🤖 Generated with Claude Code

[tylerkron]

Requesting a review on head ea38b2c. Two things worth a reviewer's attention specifically: (1) the choice to redact only the string elements of Params and leave every other element's serialised type alone — the alternative, stringifying to redact, is a payload change for every event with a parameter list; and (2) whether the three field tests earn their place given they pass on unmodified main — the mutation results in the body are the argument that they do.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Scrub account names from Sentry log message parameters

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Redacts string-valued Sentry log parameters before events leave the process.
• Preserves non-string parameter values and their serialized JSON types.
• Adds envelope-level coverage for log entries, breadcrumbs, server names, and parameters.

Diagram

sequenceDiagram
    actor Producer as Event Producer
    participant SDK as Sentry SDK
    participant Hook as BeforeSend Hook
    participant Redactor as Account Redactor
    participant Envelope as Envelope Serializer
    Producer->>SDK: Capture event
    SDK->>Hook: Apply event scrub
    Hook->>Redactor: Scrub message fields
    Redactor-->>Hook: Redacted strings
    Hook-->>SDK: Return typed parameters
    SDK->>Envelope: Serialize event
    Envelope-->>SDK: Safe payload

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Recursively scrub parameter object graphs

• ➕ Could redact paths nested inside collections or custom parameter objects.
• ➕ Would provide broader protection if structured parameters become common.
• ➖ Risks changing arbitrary object serialization and JSON types.
• ➖ Introduces cycle handling, reflection, and collection reconstruction complexity.
• ➖ Exceeds the existing string-only boundary used for Sentry extra values.

Recommendation: Keep the PR's string-only projection through the existing BeforeSend hook. It closes the known redaction gap without changing non-string payload semantics; recursive object-graph scrubbing should only be considered separately if nested path-bearing parameters become reachable.

Files changed (2) +225 / -10

Bug fix (1) +26 / -5

AppLogger.csRedact string values in Sentry log message parameters +26/-5

Redact string values in Sentry log message parameters

• Adds a parameter-list redaction overload that materializes the result and scrubs only string elements. The BeforeSend event rebuild now applies it to SentryMessage.Params while preserving numbers, booleans, nulls, collections, and other objects unchanged.

Daqifi.Avalonia/Daqifi.Desktop.Common/Loggers/AppLogger.cs

Tests (1) +199 / -5

SentryAccountNameRedactionTests.csAdd serialized-envelope coverage for all Sentry scrub regions +199/-5

Add serialized-envelope coverage for all Sentry scrub regions

• Adds boundary tests for log message text, breadcrumb data, server names, and log message parameters. The tests verify account-name removal, preservation of non-string JSON types, default server-name omission, and resistance to over-redaction. The envelope inspection helper now exposes these fields without coupling tests to in-memory event objects.

Daqifi.Avalonia.Tests/Loggers/SentryAccountNameRedactionTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit ea38b2c ⚖️ Balanced

Results up to commit ea38b2c ⚖️ Balanced

No changes from previous review

[tylerkron]

Shepherd re-review on the same head ea38b2c — an unchanged tree, deliberately, because a second independent pass on this diff is the point rather than a response to any change.

Three things worth a reviewer's attention, all in AppLogger.RedactAccountNames(IEnumerable<object>?) and its tests:

1. The string-only projection. Non-string elements keep their serialised JSON type, which is the right call for payload compatibility — but it means a path reaching Sentry inside a non-string element is not redacted. The type-preservation test pins a nested string[], so that shape is known to pass through. Worth a second opinion on whether that boundary is right.
2. Over-redaction on the new parameter path. fix(#366): redact account names where every Sentry event passes, not only on breadcrumbs #367 shipped a real instance of this class of bug (/Users/octocat2/… → ~2/… for an account named octocat). The new theory covers strict-prefix siblings, a mid-path occurrence under a non-home root, prose beside a real path, and an empty string.
3. .ToList() materialisation of the projection, so the serializer cannot re-run a caller's lazy enumerable on its way onto the wire.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit ea38b2c

[tylerkron]

Qodo-clean, CI green — ready for review.

Two graded rounds on ea38b2c, both ⚖️ Balanced, both 0 bugs / 0 rule violations / 0 requirement gaps, 0 review threads. My shepherd round fired on the unchanged head rather than being refused: the review comment's updated_at advanced 07:59:42Z → 08:11:20Z, past my 08:10:10Z request, and re-pinned to ea38b2c. Round 1's grade was genuinely unobservable until that second round created the "Previous reviews" block — it reads ⚖️ Balanced too. Re-checked both surfaces after a >5min settle: unchanged.

Verification is CI's own run on ea38b2c (35197276337), not a local run standing in for it — all six jobs pass including Android head and iOS head, and I read the job logs rather than the reported numbers: CI's log reads Passed: 906, Total: 906, identical to my local run. Not a stale green: the run was created after main's newest merge (c45fe32, 07:26:43Z) and a pull_request run builds the merge commit, so it already covers today's tip. main has not moved since.

The Params path is NOT reachable today — I re-derived this rather than taking it. No SentrySdk.CaptureMessage/CaptureEvent anywhere, SentryTarget appears only in the comment saying it is intentionally omitted, and the only Sentry package is base Sentry 6.9.0 — no NLog or MEL integration to build a logentry. The same searches returned live hits for CaptureException, AddBreadcrumb and ConfigureScope, so this is not a proof-by-absence over a mistyped path. The body's "defensive hardening, not a live leak" framing is correct and does not need correcting upward.

Re-derived independently, all exact: diffstat 2 files +225/−10; merge-base = main tip c45fe32; 906/906; the class is 29 tests; 26 passed / 3 failed of 29 with the Params scrub reverted to main's behaviour, the three failures being precisely the logentry.params fact plus 2 of the 5 theory rows; and two mutation controls re-run — unwiring server_name is killed by exactly A_path_in_the_server_name_is_redacted_and_the_field_is_absent_by_default and the stringify-everything variant by exactly Log_message_parameters_that_are_not_strings_keep_their_serialised_type, both 1 failed / 28 passed with zero collateral. Mutations were applied by whole-file rewrite with an explicit mtime bump, so no run was served a stale assembly. I did not re-derive the warning counts (175/175, 155 first-party, 176/176) — a zero-delta claim CI already covers, called out rather than implied.

One finding, disclosed not patched. The string-only projection leaves a path inside a non-string element unscrubbed — measured: a nested string[], an object with a public string property, and notably Uri, which a built-in converter renders as a JSON string while not being a string element, so the is string test skips it at no payload-compatibility saving. (An object whose path is only in ToString() is harmless — the serializer reflects over properties.) Body amended in place and filed as #373; not fixed here, since this PR is settled and where the leaf boundary sits is its own decision.

Flag for a sweep, not a blocker: #371 has not been built with #368 or #370. Its CI builds Merge ea38b2c into main, and both are still open; the five-way 919/919 merge covered them and not this. Residual risk is low on the shapes a file-overlap check cannot see — no file overlap, #371 adds no new top-level type (one private overload inside an existing class), and it removes and renames nothing — and no repo-wide guard landed on main after this PR's CI run.

Not merging.