fix: Correct conditional-request header and sanitize network error log in FDv1 polling requestor#263
Merged
Merged
Conversation
…ssage in FDv1 polling requestor
Two fixes in `packages/common_client/lib/src/data_sources/requestor.dart`:
1. Conditional-request header
The requestor was sending the stored ETag back as `etag: <value>` (a
response header name), not `If-None-Match: <value>`. The server didn't
recognize it as a conditional request and never emitted 304s, so every
poll paid full bandwidth. If a 304 *did* arrive (non-compliant proxy),
the same code path fell through to `DataEvent('put', '', ...)` with an
empty body, which downstream then parsed as empty JSON and reported as
`ErrorKind.invalidData` -- the opposite of the intended "data is
current" semantics.
The fix sends `if-none-match` correctly, treats 304 as null (no
change to apply), and treats 200 as a fresh body that parses and
updates the stored ETag.
2. Network-error log message
The catch block was passing `err.toString()` into both the warn log
and the public `StatusEvent.message`. For `http.ClientException`,
`toString()` formats as `'ClientException: <msg>, uri=<full-url>'`,
so the request URL was ending up in both places -- and that URL
embeds the base64url-encoded context.
The fix categorizes the exception with minification-safe `is` checks
(TimeoutException, http.ClientException, plus a substring fallback
for dart:io's TlsException / HandshakeException) into a fixed
sanitized message and uses that for both the log line and the
StatusEvent.
Tests added: ETag round-trip (response then If-None-Match on the
next request, with the wrong `etag` header NOT sent), 304 does not
surface as `ErrorKind.invalidData`, and warn-level log records on a
network error do not contain the encoded context.
kinyoklion
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
|
Is there end to end test coverage in the flutter repo or via contract tests for this case? |
tanderson-ld
approved these changes
May 5, 2026
…on omitted header Match the FDv2 sibling: only update the stored ETag when the response carries a non-null, non-empty value. This protects against two edge cases: - Empty-string ETag: the previous code stored '' verbatim and would send `if-none-match: ` on the next request. An unquoted empty token is invalid per RFC 7232, and lenient servers can interpret it as "match anything" and pin the SDK to permanent 304s. - Missing ETag header on a 200: the previous code cleared `_lastEtag` to null. The fix preserves the previously stored value so the next request is still conditional. Tests pin both: an empty-string ETag is not stored (next request sends no `if-none-match`), and a 200 without an ETag header preserves the prior value (third request still uses the original).
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b632cd1. Configure here.
`_doPoll` early-returned on `case null:` without calling `_schedulePoll()`, so the first no-change response halted the loop. This was previously dead code -- the requestor's wrong conditional-request header meant 304s never arrived. Fixing the header (earlier in this PR) made 304s the expected response when flag data hasn't changed, which immediately exposed the latent bug: the SDK stopped receiving updates after the first unchanged-data poll. The fix is to fall through the null arm to `_schedulePoll()` so the next poll cycle is set up regardless of whether a value, no-change, or status was received. Test pins the regression: three 304 responses in a row must produce at least three requests.
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
2 participants
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.
Two FDv1 polling bugs surfaced during review of the FDv2 polling work. The FDv2 sibling already handles both correctly; this PR mirrors that pattern in
packages/common_client/lib/src/data_sources/requestor.dart.Jira: SDK-2322
1. Conditional-request header
The requestor was sending the stored ETag back as
etag: <value>. The conditional-request header per RFC 7232 isIf-None-Match, notetag(which is a response header). The server didn't recognize the request as conditional, so 304s were never emitted for our polling traffic and the SDK paid full bandwidth on every successful poll.If a 304 ever did arrive (non-compliant proxy), the same code path fell through to
DataEvent('put', '', environmentId)with an empty body, which downstream parsed as empty JSON and surfaced asErrorKind.invalidData— the opposite of the intended "data is current" semantics.Fix: send
if-none-match, treat 304 asnull(no change), treat 200 as fresh body.2. Network-error log message
The catch block passed
err.toString()into both the warn log and the publicStatusEvent.message. Forhttp.ClientException,toString()formats as'ClientException: <msg>, uri=<full-url>', so the request URL ended up in both places — and that URL contains the base64url-encoded context.Fix: categorize the exception via minification-safe
ischecks (TimeoutException,http.ClientException, plus a substring fallback fordart:io'sTlsException/HandshakeException) into a fixed sanitized message, and use that for both the log line and theStatusEvent.Testing
etag: abc-123results inif-none-match: abc-123on the next request, and the wrongetagheader is not sent.ErrorKind.invalidDataand leavesFlagManageruntouched.MockLogAdapter).dart format,dart analyze lib test, and the polling test suite all pass.Note
Medium Risk
Adjusts polling HTTP semantics (ETag/304 handling) and the polling loop scheduling, which could change request frequency and cache behavior if incorrect. Mitigated by added regression tests covering header behavior, 304 handling, and log sanitization.
Overview
Fixes FDv1 polling conditional requests by sending cached ETags as
if-none-match(instead ofetag), treating304 Not Modifiedas no change, and avoiding storing invalid/empty ETags (while preserving the previous ETag when the header is omitted).Hardens polling failure reporting by replacing raw exception
toString()with a sanitized, categorized message (timeout/network/TLS/unknown) for both logs andStatusEvents, preventing encoded context leakage. Adds regression tests for ETag round-tripping, 304 behavior (including continuing to poll), and sanitized network-error logging.Reviewed by Cursor Bugbot for commit ad22a25. Bugbot is set up for automated code reviews on this repo. Configure here.