Sentry: a path inside a non-string logentry.params element is not scrubbed (Uri, nested collection, object property) #373

Activity

1. tylerkron commented on Sep 18, 2026

   

   tylerkron

   on Sep 18, 2026

   ContributorAuthor

   More actions

   Picked up by the avalonia loop as a builder target. Outcome: premise re-confirmed on main, consequence confirmed unreachable. No PR. Details below so the next person doesn't have to redo it.

   Premise: reproduced on main (8f5c943, after #371 merged)

   A throwaway [Theory] inside SentryAccountNameRedactionTests, using the same shipped ConfigureSentryOptions → SentryClient → RecordingWorker harness, reading logentry.params back out of the serialised envelope. Not committed.

   element	logentry.params[0] in the envelope	account in raw envelope
   new[] { ProfilePath }	["/Users/octocat/Library/…/DAQifiProfilesConfiguration.xml"]	True
   object with a string Location property	{"Location":"/Users/octocat/Library/…"}	True
   new Uri("file:///Users/octocat/Library/uri.xml")	"file:///Users/octocat/Library/uri.xml"	True
   ProfilePath (string, the control)	"/Users/<account>/Library/…"	False

   The string control is what shows the harness can see a redaction happen, so the three leaks are real and the table in this issue still holds on main.

   Consequence: nothing in the app can put a non-string element into Params today

   I enumerated every way a SentryMessage.Params could get populated in this app and found none.

   The app's own code. git grep over every production .cs (tests excluded) for Params, CaptureMessage, CaptureEvent, new SentryEvent, SentryEvent(, target-typed Message = new(, IEventProcessor/AddEventProcessor, Sentry.Extensions, SentryLog/EnableLogs. The only hit is the scrubber itself, AppLogger.cs:503 (Params = RedactAccountNames(message.Params)), which doubles as the positive control for that search. The app talks to Sentry in exactly these places: CaptureException ×2, AddBreadcrumb ×2, ConfigureScope ×2, FlushAsync, all in AppLogger.cs. None of them builds a logentry.

   Logging integrations. The only Sentry package referenced is Sentry 6.9.0 (Daqifi.Avalonia.csproj:96). Every packages.lock.json lists just Sentry plus the native Sentry.Bindings.Android/.Cocoa/Android.AssemblyReader. There's no Sentry.Extensions.Logging, no Sentry.NLog and no Sentry.Serilog, and those are the integrations that fill Params from a structured-log template. SentryTarget is deliberately not wired (AppLogger.cs:183).

   The SDK itself. I decompiled Sentry.dll 6.9.0 (net10.0) with ilspycmd and searched it for Params. There are three hits, all in SentryMessage.cs: the property, WriteTo, and FromJson. FromJson only runs when EnvelopeItem deserialises an event that has already been serialised, so it went through BeforeSend first. It isn't a capture path. The SDK builds a SentryMessage in two places. One is the implicit string → SentryMessage conversion, which sets Message only. The other is BlockingMonitor, which assigns a constant string and needs CaptureBlockingCalls, which this app doesn't set. Positive control for this search: the same grep finds the Formatted = and .Message = writers, so it would have found a Params = writer if one existed.

   Native crashes from the Android/Cocoa SDKs never pass through the managed SentryMessage at all.

   Why no PR

   What I found matches the Severity section above: the gap is real, and today nothing can reach it. That leaves two options, and neither is worth a PR:

   • The leaf-widening fix proposed here (redact any element whose serialised form is a JSON string, e.g. Uri) is small and changes no payload shape. But it would guard a field nothing writes, and it still leaves the nested-collection and object-property rows open. Those need recursion, which does change what gets serialised, the over-broad direction this issue already argues against.
   • A full fix means choosing which element kinds to recurse into, and that is only a sound choice once there's a real caller to design against.

   Suggested trigger for reopening the work: the first PR that adds CaptureMessage/CaptureEvent, a Sentry.Extensions.Logging or Sentry.NLog reference, or wires SentryTarget. That PR creates the reachable path, so it should also carry the Params fix, tested at the envelope against the element kinds its caller actually passes. I'm leaving this open as the record of that boundary rather than closing it; closing is a maintainer call.
2. tylerkron commented on Sep 21, 2026

   

   tylerkron

   on Sep 21, 2026

   ContributorAuthor

   More actions

   Re-picked up by the loop. I did not take the previous pass on trust — I re-derived both halves independently on today's main (4716ce1, five merges later than the 8f5c943 that pass measured). Both halves hold. Still no PR. One thing is new, and it is bigger than the thing this issue is about; it is at the bottom.

   Premise: re-measured on 4716ce1

   Own probe, own harness — a self-contained SentryEvent → shipped ConfigureSentryOptions → SentryClient → recording worker, reading logentry.params[0] out of the serialised envelope. Inert .invalid DSN, no transport constructed, nothing left the machine. Not committed.

   element	logentry.params[0] on the wire	account in raw envelope
   string (control)	"/Users/<account>/Library/…/DAQifiProfilesConfiguration.xml"	False
   string[]	["/Users/octocat/Library/…/DAQifiProfilesConfiguration.xml"]	True
   object with a public string Location	{"Location":"/Users/octocat/Library/…"}	True
   Uri	"file:///Users/octocat/Library/uri.xml"	True
   object whose path is only in ToString()	{}	False — the serializer reflects over properties and never calls it

   The string control redacting is what shows the harness can see a redaction happen at all, so the three True rows are real. This issue's table reproduces exactly.

   Consequence: still nothing in the app can put a non-string element into Params

   Re-derived from scratch, not copied:

   • App code. One git grep over every production .cs for CaptureMessage|CaptureEvent|SentryEvent|SentryMessage|IEventProcessor|AddEventProcessor|SentryTarget|EnableLogs|SentryLog|Params returns hits in exactly one file, AppLogger.cs, and every one of them is the scrubber or its own documentation. Params = RedactAccountNames(message.Params) at AppLogger.cs:503 is the positive control that the search works. The app's whole Sentry surface is CaptureException ×2, AddBreadcrumb ×2, ConfigureScope ×2, FlushAsync — none builds a logentry.
   • Packages. Every packages.lock.json in the repo lists Sentry 6.9.0 and the native Sentry.Bindings.Android/.Cocoa/Android.AssemblyReader, and nothing else. No Sentry.Extensions.Logging, no .NLog, no .Serilog — those are the integrations that fill Params from a structured-log template. SentryTarget is still deliberately unwired (AppLogger.cs:183).
   • The SDK. Decompiled Sentry.dll 6.9.0 (net10.0). Params has three occurrences, all in SentryMessage: the property, WriteTo, and FromJson. FromJson is a deserialiser — it runs on an event that has already been through BeforeSend — so it is not a capture path. The grep does find a Params = writer (that one), which is its own positive control.
   • Open PRs. New this pass: I checked the changed-file list of all 13 open PRs. None touches AppLogger.cs or SentryAccountNameRedactionTests.cs, and none adds a capture path. The only Sentry-adjacent one is dependabot chore(deps): Bump the minor-and-patch group with 17 updates #363, which bumps Sentry 6.9.0 → 6.11.0 and adds no integration package.

   Why still no PR

   Unchanged from the last pass, and I agree with it. The leaf-widening refinement this issue proposes (redact any element whose serialised form is a JSON string, which would cover Uri) is small and costs no payload compatibility — but it would guard a field nothing writes, and it would still leave the string[] and object-property rows open. Closing those needs recursion, which is the over-broad direction this issue already argues against. Choosing which element kinds to recurse into is only a sound decision once there is a real caller to design against.

   New: there is a second, larger gap in the same wall

   While reading the SDK to check the Params claim I found something this issue does not cover. Sentry 6.9.0 ships structured logs (SentryOptions.EnableLogs, [Experimental("SENTRY0001")]), and that path does not pass through BeforeSend at all:

   • DefaultSentryStructuredLogger.CaptureLog calls BeforeSendLogInternal — i.e. only a SetBeforeSendLog callback — and then goes straight to the batch processor, which ships its own envelope item via CaptureEnvelope. SetBeforeSend is applied by CaptureEvent, which is never on this path.
   • So if EnableLogs were ever switched on, AppLogger's account-name scrubber would not run on a log's message, template, parameters or attributes. That is strictly worse than this issue: it swallows plain string parameters too, which BeforeSend handles correctly today.

   Not a defect today, and not an SDK bug. EnableLogs defaults to false — SentryStructuredLogger.Create returns DisabledSentryStructuredLogger — and ConfigureSentryOptions never sets it. The SDK offers the right hook (SetBeforeSendLog); we simply have no reason to set one yet. Recorded here so it is not rediscovered. (This is a code read of the shipped 6.9.0 assembly, not a measured envelope — I did not enable the feature to probe it, because doing so means SentrySdk.Init and global state in the test process.)

   Reopening trigger, widened

   The first PR that does any of these creates the reachable path and should carry the corresponding fix, tested at the envelope against the element kinds its caller actually passes:

   1. adds CaptureMessage / CaptureEvent, or
   2. references Sentry.Extensions.Logging / Sentry.NLog / Sentry.Serilog, or wires SentryTarget, or
   3. sets options.EnableLogs — which needs a SetBeforeSendLog scrub, not a Params one.

   Leaving this open as the record of the boundary, per the Severity section above; closing remains a maintainer call. Also worth a re-check when #363 lands, since 6.11.0 is a different SDK than the one measured here.