fix: stop leaking raw upstream body in NetworkException (#41)#43
fix: stop leaking raw upstream body in NetworkException (#41)#43dobby-coder[bot] wants to merge 1 commit into
Conversation
NetworkException embedded the raw upstream HTTP response body in its
exception message and exposed it via a public Body property, so any
sensitive upstream detail could leak into logs or error surfaces.
- Base message is now generic: "HTTP {statusCode} received from upstream service"
- Removed the public Body property
- The raw body is retained in a private field for diagnostics only
The constructor signature is unchanged, so the two call sites
(CryptifyClient/PkgClient EnsureSuccessAsync) continue to pass the body,
which is now stored privately rather than surfaced.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Rules Dobby 2 — consolidated review (rule check + code review)
The core security fix is correct: NetworkException no longer embeds the raw upstream body in its message and no longer exposes a public Body property; the body is retained in a private _body field exactly as the task specified. Build is clean (0 warnings) and 64/65 tests pass — the one failure (ChunkContentRange_IsFormattedPerChunk) is pre-existing on main and unrelated. PR body correctly uses Closes #41, the title is conventional-commit format, and tests were added/updated to lock in the new contract. No hard rule violations.
Two non-blocking notes below. The second (semver/changelog signaling of a public-API removal) is worth a quick fix before this is marked ready. Sending back to PR Dobby.
| // Raw upstream response body, retained for diagnostics only. Kept private | ||
| // so it is never surfaced in the exception message or via a public property, | ||
| // where it could leak sensitive upstream data into logs. See issue #41. | ||
| private readonly string _body; |
There was a problem hiding this comment.
The private _body field is assigned in the ctor but never read anywhere — no accessor, method, or ToString() override consumes it — so the "retained for diagnostics" comment is misleading: it provides no diagnostic access and only keeps the sensitive upstream body alive on the exception object. This is acceptable because the task explicitly asked to "store it in a private field," so no change is required, but as written it is effectively dead storage. If diagnostic access is genuinely wanted, expose it through a redacted/internal accessor; otherwise the comment could be trimmed. (Build stays clean — assigned from a ctor param, so no CS0414.)
|
|
||
| public NetworkException(int statusCode, string body, string url) | ||
| : base($"HTTP {statusCode} at {url}: {body}") | ||
| : base($"HTTP {statusCode} received from upstream service") |
There was a problem hiding this comment.
Removing the public Body property is a breaking change to the published SDK's public API, but this PR uses a fix: conventional-commit prefix. Under release-please (see PR #38 / release 0.5.1) a fix: is cut as a patch bump and will NOT signal the API break to downstream consumers who reference .Body. Please signal the break — either retitle as feat!: or add a BREAKING CHANGE: removed public NetworkException.Body property footer to the commit — so the removed member surfaces in the changelog and triggers an appropriate version bump.
Closes #41.
Problem
NetworkExceptionembedded the raw upstream HTTP response body directly in its exception message ($"HTTP {statusCode} at {url}: {body}") and exposed it through a publicBodyproperty. Both surfaces mean sensitive upstream response data can leak into application logs, telemetry, and error pages — the information-exposure concern tracked in the draft advisory referenced by #41.Change
src/Exceptions/PostGuardException.cs:$"HTTP {statusCode} received from upstream service"— no body, no interpolated content from upstream.Bodyproperty._bodyfield for diagnostics only, so it is never surfaced in the message or via a public member.StatusCodeandUrlremain public — theUrlis our own PostGuard endpoint (not upstream-controlled) and is useful for debugging.src/Api/CryptifyClient.cs/src/Api/PkgClient.cs:NetworkException(statusCode, body, url)constructor signature is unchanged, so bothEnsureSuccessAsynccall sites continue to pass the body — it is now stored privately rather than exposed. No behavioural change needed at the call sites beyond the exception's own new handling.Tests
Updated
NetworkExceptionTeststo reflect the new contract:NetworkExceptionexposes no publicBodyproperty (reflection assertion),StatusCode/Urlstill populated.dotnet test --framework net10.0: 64 passing. The single failing test (ChunkContentRange_IsFormattedPerChunk) is a pre-existing failure onmain, unrelated to this change (verified by running it onmain). Bothnet8.0andnet10.0build cleanly; per the workspace note, tests run on net10.0 locally because the net8.0 runtime isn't installed here — CI exercises both.