Repository navigation
fix(flags): treat an unparsable feature flag payload as no payload - #341
Merged
Merged
Conversation
`FeatureFlag.CreateFromFlagsApi` and `CreateFromLocalEvaluation` both called `JsonDocument.Parse` unguarded, so a malformed, empty, or whitespace-only serialized payload threw during flag construction. On the local path that discarded the flag's correctly evaluated value and forced a remote fallback; on the remote path it discarded the entire `/flags` response. Both factories now decode through a shared guard that logs a warning and yields `Payload = null` — the same representation the SDK already uses for a flag with no payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 0b375c97-1a50-41a9-bbb4-a7d134e77862
Contributor
posthog-dotnet Compliance ReportDate: 2026-10-09T06:10:34.236760+00:00 ✅ All Tests Passed!47/47 tests passed Capture Tests✅ 30/30 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
ioannisj
approved these changes
Oct 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Motivation and Context
Contract:
get-feature-flag-payload— "Malformed serialized payloads return no payload" — and its bulk counterpart inget-feature-flags-and-payloads("Bulk payload decoding agrees with single-flag APIs"). The same rule is referenced byevaluate-flagsandget-feature-flag-result.The spec requires that a serialized payload be validated as JSON before public access: malformed, empty, or whitespace-only input must be logged and treated as an absent payload, never surfaced raw and never thrown to the caller — and "a payload decoding failure MUST NOT discard the associated flag value, healthy sibling payloads, or the combined result."
The sdk-specs compliance matrix for this SDK records this as a single shared root behind four non-compliant rows (Evaluate Flags, Get Feature Flag Payload, Get Feature Flag Result, Get Feature Flags And Payloads):
📝 What changed
FeatureFlag.CreateFromFlagsApiandFeatureFlag.CreateFromLocalEvaluationnow decode payloads through a shared privateParsePayloadOrNullguard instead of callingJsonDocument.Parsedirectly. Anullpayload staysnullas before; empty/whitespace-only input and aJsonExceptionboth log a warning and yieldPayload = null.ILogger?, threaded from the existing loggers at the three call sites (FlagsApiResultExtensions.ToFlagsResult,LocalEvaluator.EvaluateAllFlags,PostHogClient.GetFeatureFlagAsync), so the failure is attributable to a flag key in SDK diagnostics.Before this change, one bad payload byte in a
/flagsresponse threw out ofFlagsApiResult.ToFlagsResult's projection andEvaluateFlagsAsync's catch discarded the whole response — every healthy flag value and sibling payload with it — and nothing in the logs distinguished it from a transport failure. On the local-evaluation path the per-flag catch swallowed it, lost the flag's correctly evaluated value, and forced an unnecessary remote fallback.✅ Why this is backwards-compatible
Behavior changes only for payloads that are already unusable. A payload that parses today parses identically; a payload that threw today now resolves to
Payload = null, which is the SDK's existing representation for a flag with no payload, and which callers already have to handle. The raw malformed string is never exposed (JsonDocument?makes that impossible). No public API surface changes — both factories areinternal, the new parameters are optional, andPublicAPI.*.txtis untouched.💚 How did you test it?
{broken,"", and" "each yieldPayload = nullwhileKey,IsEnabled, andVariantKeyare preserved.EvaluateFlagsAsynctests covering both evaluation paths with the same three inputs, mirroring the spec's scenario outlines: a malformed remote payload leaves the sibling flag's value and payload intact, and a malformed locally-evaluated payload no longer triggers a remote/flagsfallback.main'sFeatureFlag.csand pass with the fix.dotnet restore --locked-mode,bin/fmt --check,dotnet build --configuration Release,dotnet test --configuration Release— green on bothnet8.0andnetcoreapp3.1(1235 / 1225 passed, 2 skipped, 0 failed).➡️ Follow-up work (not in this PR)
The compliance notes list two further gaps on these rows that are deliberately out of scope here, to keep this change to the single contract:
GetFeatureFlagPayloadAsync(key, distinctId, matchValue?)API — the only routes to a payload still require an already-evaluated flag or snapshot./flagsresponse, so such keys re-probe on every call.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
Produced by the scheduled sdk-specs compliance run (Claude Code). The run reads the compliance matrices in
PostHog/sdk-specs, ranks the open 🟡/❌ cells by safety, impact and scope, and implements exactly one per run. This cell was picked because its backwards-compatibility verdict is "Backward-compatible", it is user-facing (feature flag payload reads), and one guard resolves four matrix rows at a shared root. Breaking-verdict and Unknown cells are left for humans.Scope decisions: the parse guard only, not the two follow-ups listed above, which are separate contracts/API additions. The logger is passed as an optional parameter rather than a static or ambient logger, to match how
LocalEvaluatorandPostHogClientalready threadILogger. Empty/whitespace input is reported with a distinct message from a parse failure so the two causes stay separable in logs.Agent-authored — please review rather than rubber-stamp. No manual/sample-app testing was performed beyond the automated suite described above.
Created with PostHog Desktop
🤖 Generated with Claude Code