diff --git a/packages/common_client/lib/src/data_sources/polling_data_source.dart b/packages/common_client/lib/src/data_sources/polling_data_source.dart index e218a48..291689e 100644 --- a/packages/common_client/lib/src/data_sources/polling_data_source.dart +++ b/packages/common_client/lib/src/data_sources/polling_data_source.dart @@ -95,8 +95,11 @@ final class PollingDataSource implements DataSource { switch (event) { case null: - // No change. - return; + // No change (e.g. 304 Not Modified). Fall through to schedule + // the next poll -- returning here would leave the timer + // unset and silently stop polling after the first unchanged + // response. + break; case DataEvent(): _eventController.sink.add(event); case StatusEvent(): diff --git a/packages/common_client/lib/src/data_sources/requestor.dart b/packages/common_client/lib/src/data_sources/requestor.dart index d6c66f7..4500b77 100644 --- a/packages/common_client/lib/src/data_sources/requestor.dart +++ b/packages/common_client/lib/src/data_sources/requestor.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:http/http.dart' as http; import 'package:launchdarkly_dart_common/launchdarkly_dart_common.dart'; @@ -57,25 +59,42 @@ final class Requestor { Future requestAllFlags() async { try { _logger.debug( - 'Making polling request, method: $_method, uri: $_uri, etag: $_lastEtag'); + 'Making polling request, method: $_method, hasEtag: ${_lastEtag != null}'); + final additionalHeaders = switch (_lastEtag) { + final etag? => {'if-none-match': etag}, + null => null, + }; final res = await _client.request(_method, _uri, - additionalHeaders: _lastEtag != null ? {'etag': _lastEtag!} : null, + additionalHeaders: additionalHeaders, body: _method != RequestMethod.get ? _contextString : null); return await _handleResponse(res); } catch (err) { - return StatusEvent(ErrorKind.networkError, null, err.toString()); + // Don't echo the raw exception. `http.ClientException`'s + // `toString()` formats as `'ClientException: , uri='`, + // and the URL embeds the base64url-encoded context in GET mode. + // Categorize into a fixed message and use that everywhere. + final sanitized = _describeError(err); + _logger.warn('Polling request failed: $sanitized'); + return StatusEvent(ErrorKind.networkError, null, sanitized); } } Future _handleResponse(http.Response res) async { - if (res.statusCode == 200 || res.statusCode == 304) { + if (res.statusCode == 304) { + // Server confirmed our cached payload is still current. + return null; + } + if (res.statusCode == 200) { + // Reject empty-string ETags: an unquoted empty token is invalid + // per RFC 7232 ยง2.1, and sending `if-none-match: ` on the next + // request could be interpreted by some servers as "match anything" + // and pin the SDK to a permanent 304. Also preserve the previously + // stored value when the response simply omits the header rather + // than clearing it. final etag = res.headers['etag']; - if (etag != null && etag == _lastEtag) { - // The response has not changed, so we don't need to do the work of - // updating the store, calculating changes, or persisting the payload. - return null; + if (etag != null && etag.isNotEmpty) { + _lastEtag = etag; } - _lastEtag = etag; var environmentId = getEnvironmentId(res.headers); @@ -88,15 +107,25 @@ final class Requestor { } return DataEvent('put', res.body, environmentId: environmentId); - } else { - if (isHttpGloballyRecoverable(res.statusCode)) { - return StatusEvent(ErrorKind.errorResponse, res.statusCode, - 'Received unexpected status code: ${res.statusCode}'); - } else { - return StatusEvent(ErrorKind.errorResponse, res.statusCode, - 'Received unexpected status code: ${res.statusCode}', - shutdown: true); - } } + if (isHttpGloballyRecoverable(res.statusCode)) { + return StatusEvent(ErrorKind.errorResponse, res.statusCode, + 'Received unexpected status code: ${res.statusCode}'); + } + return StatusEvent(ErrorKind.errorResponse, res.statusCode, + 'Received unexpected status code: ${res.statusCode}', + shutdown: true); + } + + String _describeError(Object err) { + if (err is TimeoutException) return 'Polling request timed out'; + if (err is http.ClientException) { + return 'Network error during polling request'; + } + final type = err.runtimeType.toString(); + if (type.contains('Tls') || type.contains('Handshake')) { + return 'TLS error during polling request'; + } + return 'Polling request failed'; } } diff --git a/packages/common_client/test/data_sources/polling_data_source_test.dart b/packages/common_client/test/data_sources/polling_data_source_test.dart index 2fb3af1..e57b023 100644 --- a/packages/common_client/test/data_sources/polling_data_source_test.dart +++ b/packages/common_client/test/data_sources/polling_data_source_test.dart @@ -12,21 +12,25 @@ import 'package:launchdarkly_common_client/src/flag_manager/flag_manager.dart'; import 'package:launchdarkly_dart_common/launchdarkly_dart_common.dart' as ld_common; import 'package:http/http.dart' as http; +import 'package:mocktail/mocktail.dart'; import 'package:test/test.dart'; +class MockLogAdapter extends Mock implements LDLogAdapter {} + (PollingDataSource, FlagManager, DataSourceStatusManager) makeDataSourceForTest( MockClient innerClient, {LDContext? inContext, HttpProperties? inProperties, bool useReport = false, bool withReasons = false, - Duration? testingInterval}) { + Duration? testingInterval, + LDLogger? inLogger}) { final context = inContext ?? LDContextBuilder().kind('user', 'test').build(); // We are not testing the data source status manager here, so we just want a // fixed time to make events easy to get. final statusManager = DataSourceStatusManager(stamper: () => DateTime(1)); - final logger = LDLogger(); + final logger = inLogger ?? LDLogger(); final httpProperties = inProperties ?? HttpProperties(); const sdkKey = 'dummy-key'; final flagManager = @@ -511,4 +515,207 @@ void main() { polling.stop(); }); + + group('conditional requests', () { + setUpAll(() { + registerFallbackValue(LDLogRecord( + level: LDLogLevel.debug, + message: '', + time: DateTime.now(), + logTag: '')); + }); + + test( + 'sends if-none-match on the next request after receiving a 200 with etag', + () async { + var requestNumber = 0; + Map? secondRequestHeaders; + final innerClient = MockClient((request) async { + requestNumber++; + if (requestNumber == 1) { + return http.Response('{}', 200, headers: {'etag': 'abc-123'}); + } + secondRequestHeaders = request.headers; + return http.Response('{}', 200); + }); + + final (polling, _, _) = makeDataSourceForTest(innerClient, + testingInterval: const Duration(milliseconds: 5)); + polling.start(); + + // Drain to the second request before asserting. + while (requestNumber < 2) { + await Future.delayed(const Duration(milliseconds: 5)); + } + polling.stop(); + + expect(secondRequestHeaders, containsPair('if-none-match', 'abc-123')); + // The wrong header name (`etag`) must not be sent. + expect(secondRequestHeaders!.containsKey('etag'), isFalse); + }); + + test('empty-string etag on a 200 response is not stored', () async { + var requestNumber = 0; + Map? secondRequestHeaders; + final innerClient = MockClient((request) async { + requestNumber++; + if (requestNumber == 1) { + return http.Response('{}', 200, headers: {'etag': ''}); + } + secondRequestHeaders = request.headers; + return http.Response('{}', 200); + }); + + final (polling, _, _) = makeDataSourceForTest(innerClient, + testingInterval: const Duration(milliseconds: 5)); + polling.start(); + + while (requestNumber < 2) { + await Future.delayed(const Duration(milliseconds: 5)); + } + polling.stop(); + + // An unquoted empty token is invalid per RFC 7232 and sending + // `if-none-match: ` could pin the SDK to permanent 304s on lenient + // servers. The empty value must not round-trip. + expect(secondRequestHeaders!.containsKey('if-none-match'), isFalse); + }); + + test('200 without an etag header preserves the previously stored value', + () async { + var requestNumber = 0; + Map? thirdRequestHeaders; + final innerClient = MockClient((request) async { + requestNumber++; + if (requestNumber == 1) { + return http.Response('{}', 200, headers: {'etag': 'first'}); + } + if (requestNumber == 2) { + // 200 without an etag header. Old code would clear `_lastEtag` + // to null here; the fix preserves "first". + return http.Response('{}', 200); + } + thirdRequestHeaders = request.headers; + return http.Response('{}', 200); + }); + + final (polling, _, _) = makeDataSourceForTest(innerClient, + testingInterval: const Duration(milliseconds: 5)); + polling.start(); + + while (requestNumber < 3) { + await Future.delayed(const Duration(milliseconds: 5)); + } + polling.stop(); + + expect(thirdRequestHeaders, containsPair('if-none-match', 'first')); + }); + + test('304 response is treated as no-change, not as malformed data', + () async { + // The pre-fix behavior fell through 304 to DataEvent('put', '', ...), + // which downstream parsed as empty JSON and surfaced as + // `ErrorKind.invalidData`. The fix returns null on 304, so the + // status manager should never see an `invalidData` error. + var requestCount = 0; + final innerClient = MockClient((request) async { + requestCount++; + return http.Response('', 304); + }); + + final (polling, flagManager, statusManager) = makeDataSourceForTest( + innerClient, + testingInterval: const Duration(milliseconds: 5)); + polling.start(); + + while (requestCount < 1) { + await Future.delayed(const Duration(milliseconds: 5)); + } + // Give the status pipeline a chance to surface anything. + await Future.delayed(const Duration(milliseconds: 20)); + polling.stop(); + + final lastError = statusManager.status.lastError; + expect(lastError?.kind, isNot(equals(ErrorKind.invalidData))); + // FlagManager should remain in its initial empty state -- no + // attempt was made to parse the empty body. + expect(flagManager.environmentId, isNull); + }); + + test('a 304 response does not stop polling', () async { + // Regression: a previous version of _doPoll early-returned on a + // null event without scheduling the next poll, so the first 304 + // permanently halted the loop. This pins multiple 304s in a row + // resulting in multiple requests. + var requestCount = 0; + final innerClient = MockClient((request) async { + requestCount++; + return http.Response('', 304); + }); + + final (polling, _, _) = makeDataSourceForTest(innerClient, + testingInterval: const Duration(milliseconds: 5)); + polling.start(); + + while (requestCount < 3) { + await Future.delayed(const Duration(milliseconds: 5)); + } + polling.stop(); + + expect(requestCount, greaterThanOrEqualTo(3)); + }); + }); + + group('network error log content', () { + setUpAll(() { + registerFallbackValue(LDLogRecord( + level: LDLogLevel.debug, + message: '', + time: DateTime.now(), + logTag: '')); + }); + + test( + 'warn-level log on a network error does not contain the encoded context', + () async { + // http.ClientException's toString embeds the request URL, which + // in turn embeds the base64url-encoded context. The requestor + // must categorize the exception into a fixed string and log + // only that. + final adapter = MockLogAdapter(); + when(() => adapter.log(any())).thenReturn(null); + final logger = LDLogger(adapter: adapter, level: LDLogLevel.debug); + + final secretContext = + LDContextBuilder().kind('user', 'secret-key-shibboleth').build(); + final innerClient = MockClient((request) async { + throw http.ClientException('Connection refused', request.url); + }); + + final (polling, _, statusManager) = makeDataSourceForTest(innerClient, + inContext: secretContext, + inLogger: logger, + testingInterval: const Duration(milliseconds: 1)); + polling.start(); + await Future.delayed(const Duration(milliseconds: 30)); + polling.stop(); + + final encodedContext = base64UrlEncode(utf8.encode(jsonEncode( + ld_common.LDContextSerialization.toJson(secretContext, + isEvent: false)))); + final records = verify(() => adapter.log(captureAny())).captured; + for (final record in records) { + expect( + (record as LDLogRecord).message, isNot(contains(encodedContext))); + expect(record.message, isNot(contains('secret-key-shibboleth'))); + } + + // The user-visible status surface must also not echo the URL. + final lastError = statusManager.status.lastError; + if (lastError != null) { + expect(lastError.message, isNot(contains(encodedContext))); + expect(lastError.message, isNot(contains('secret-key-shibboleth'))); + } + }); + }); }