fix: debugging and fixing three issues 0528 (#163)

CodingAleCR · cursoragent · web-flow · commit ecbada10f2dc · 2025-07-18T10:49:43.000-06:00
* Fix retry logic, URL parameter security, and timeout handling

Co-authored-by: me &lt;me@codingale.dev&gt;

* fix: improve retry policies, url parsing and documentation

* fix: remove unneeded markdown files

* style: dart formatting

* fix: handle missing edge cases on retry request fn

---------

Co-authored-by: Cursor Agent &lt;cursoragent@cursor.com&gt;
diff --git a/README.md b/README.md
@@ -227,11 +227,25 @@ Sometimes you need to retry a request due to different circumstances, an expired
 
 ```dart
 class ExpiredTokenRetryPolicy extends RetryPolicy {
+  @override
+  int get maxRetryAttempts => 2;
+
+  @override
+  bool shouldAttemptRetryOnException(Exception reason, BaseRequest request) {
+    // Log the exception for debugging
+    print('Request failed: ${reason.toString()}');
+    print('Request URL: ${request.url}');
+    
+    // Retry on network exceptions, but not on client errors
+    return reason is SocketException || reason is TimeoutException;
+  }
+
   @override
   Future<bool> shouldAttemptRetryOnResponse(BaseResponse response) async {
     if (response.statusCode == 401) {
       // Perform your token refresh here.
-
+      print('Token expired, refreshing...');
+      
       return true;
     }
 
@@ -242,13 +256,56 @@ class ExpiredTokenRetryPolicy extends RetryPolicy {
 
 You can also set the maximum amount of retry attempts with `maxRetryAttempts` property or override the `shouldAttemptRetryOnException` if you want to retry the request after it failed with an exception.
 
+### RetryPolicy Interface
+
+The `RetryPolicy` abstract class provides the following methods that you can override:
+
+- **`shouldAttemptRetryOnException(Exception reason, BaseRequest request)`**: Called when an exception occurs during the request. Return `true` to retry, `false` to fail immediately.
+- **`shouldAttemptRetryOnResponse(BaseResponse response)`**: Called after receiving a response. Return `true` to retry, `false` to accept the response.
+- **`maxRetryAttempts`**: The maximum number of retry attempts (default: 1).
+- **`delayRetryAttemptOnException({required int retryAttempt})`**: Delay before retrying after an exception (default: no delay).
+- **`delayRetryAttemptOnResponse({required int retryAttempt})`**: Delay before retrying after a response (default: no delay).
+
+### Using Retry Policies
+
+To use a retry policy, pass it to the `InterceptedClient` or `InterceptedHttp`:
+
+```dart
+final client = InterceptedClient.build(
+  interceptors: [WeatherApiInterceptor()],
+  retryPolicy: ExpiredTokenRetryPolicy(),
+);
+```
+
 Sometimes it is helpful to have a cool-down phase between multiple requests. This delay could for example also differ between the first and the second retry attempt as shown in the following example.
 
 ```dart
 class ExpiredTokenRetryPolicy extends RetryPolicy {
+  @override
+  int get maxRetryAttempts => 3;
+
+  @override
+  bool shouldAttemptRetryOnException(Exception reason, BaseRequest request) {
+    // Only retry on network-related exceptions
+    return reason is SocketException || reason is TimeoutException;
+  }
+
+  @override
+  Future<bool> shouldAttemptRetryOnResponse(BaseResponse response) async {
+    // Retry on server errors (5xx) and authentication errors (401)
+    return response.statusCode >= 500 || response.statusCode == 401;
+  }
+
+  @override
+  Duration delayRetryAttemptOnException({required int retryAttempt}) {
+    // Exponential backoff for exceptions
+    return Duration(milliseconds: (250 * math.pow(2.0, retryAttempt - 1)).round());
+  }
+
   @override
   Duration delayRetryAttemptOnResponse({required int retryAttempt}) {
-    return const Duration(milliseconds: 250) * math.pow(2.0, retryAttempt);
+    // Exponential backoff for response-based retries
+    return Duration(milliseconds: (250 * math.pow(2.0, retryAttempt - 1)).round());
   }
 }
 ```
diff --git a/lib/http/intercepted_client.dart b/lib/http/intercepted_client.dart
@@ -274,16 +274,89 @@ class InterceptedClient extends BaseClient {
   /// of the response
   Future<BaseResponse> _attemptRequest(BaseRequest request,
       {bool isStream = false}) async {
+    _retryCount = 0; // Reset retry count for each new request
+    return _attemptRequestWithRetries(request, isStream: isStream);
+  }
+
+  /// Internal method that handles the actual request with retry logic
+  Future<BaseResponse> _attemptRequestWithRetries(BaseRequest request,
+      {bool isStream = false}) async {
     BaseResponse response;
     try {
       // Intercept request
       final interceptedRequest = await _interceptRequest(request);
 
-      var stream = requestTimeout == null
-          ? await _inner.send(interceptedRequest)
-          : await _inner
-              .send(interceptedRequest)
-              .timeout(requestTimeout!, onTimeout: onRequestTimeout);
+      StreamedResponse stream;
+      if (requestTimeout == null) {
+        stream = await _inner.send(interceptedRequest);
+      } else {
+        // Use a completer to properly handle timeout and cancellation
+        final completer = Completer<StreamedResponse>();
+        final Future<StreamedResponse> requestFuture =
+            _inner.send(interceptedRequest);
+
+        // Set up timeout with proper cleanup
+        Timer? timeoutTimer;
+        bool isCompleted = false;
+
+        timeoutTimer = Timer(requestTimeout!, () {
+          if (!isCompleted) {
+            isCompleted = true;
+            if (onRequestTimeout != null) {
+              // If timeout callback is provided, use it
+              try {
+                final timeoutResponse = onRequestTimeout!();
+                if (timeoutResponse is Future<StreamedResponse>) {
+                  timeoutResponse.then((response) {
+                    if (!completer.isCompleted) {
+                      completer.complete(response);
+                    }
+                  }).catchError((error) {
+                    if (!completer.isCompleted) {
+                      completer.completeError(error);
+                    }
+                  });
+                } else {
+                  if (!completer.isCompleted) {
+                    completer.complete(timeoutResponse);
+                  }
+                }
+              } catch (error) {
+                if (!completer.isCompleted) {
+                  completer.completeError(error);
+                }
+              }
+            } else {
+              // Default timeout behavior
+              if (!completer.isCompleted) {
+                completer.completeError(Exception(
+                    'Request timeout after ${requestTimeout!.inMilliseconds}ms'));
+              }
+            }
+          }
+        });
+
+        // Handle the actual request completion
+        requestFuture.then((streamResponse) {
+          timeoutTimer?.cancel();
+          if (!isCompleted) {
+            isCompleted = true;
+            if (!completer.isCompleted) {
+              completer.complete(streamResponse);
+            }
+          }
+        }).catchError((error) {
+          timeoutTimer?.cancel();
+          if (!isCompleted) {
+            isCompleted = true;
+            if (!completer.isCompleted) {
+              completer.completeError(error);
+            }
+          }
+        });
+
+        stream = await completer.future;
+      }
 
       response = isStream ? stream : await Response.fromStream(stream);
 
@@ -293,7 +366,7 @@ class InterceptedClient extends BaseClient {
         _retryCount += 1;
         await Future.delayed(retryPolicy!
             .delayRetryAttemptOnResponse(retryAttempt: _retryCount));
-        return _attemptRequest(request, isStream: isStream);
+        return _attemptRequestWithRetries(request, isStream: isStream);
       }
     } on Exception catch (error) {
       if (retryPolicy != null &&
@@ -302,13 +375,12 @@ class InterceptedClient extends BaseClient {
         _retryCount += 1;
         await Future.delayed(retryPolicy!
             .delayRetryAttemptOnException(retryAttempt: _retryCount));
-        return _attemptRequest(request, isStream: isStream);
+        return _attemptRequestWithRetries(request, isStream: isStream);
       } else {
         rethrow;
       }
     }
 
-    _retryCount = 0;
     return response;
   }
 
diff --git a/lib/models/retry_policy.dart b/lib/models/retry_policy.dart
@@ -12,8 +12,9 @@ import 'package:http/http.dart';
 ///   int get maxRetryAttempts => 2;
 ///
 ///   @override
-///   bool shouldAttemptRetryOnException(Exception reason) {
+///   bool shouldAttemptRetryOnException(Exception reason, BaseRequest request) {
 ///     log(reason.toString());
+///     log("Request URL: ${request.url}");
 ///
 ///     return false;
 ///   }
@@ -36,20 +37,45 @@ import 'package:http/http.dart';
 abstract class RetryPolicy {
   /// Defines whether the request should be retried when an Exception occurs
   /// while making said request to the server.
+  ///
+  /// [reason] - The exception that occurred during the request
+  /// [request] - The original request that failed
+  ///
+  /// Returns `true` if the request should be retried, `false` otherwise.
   FutureOr<bool> shouldAttemptRetryOnException(
           Exception reason, BaseRequest request) =>
       false;
 
   /// Defines whether the request should be retried after the request has
   /// received `response` from the server.
+  ///
+  /// [response] - The response received from the server
+  ///
+  /// Returns `true` if the request should be retried, `false` otherwise.
+  /// Common use cases include retrying on 401 (Unauthorized) or 500 (Server Error).
   FutureOr<bool> shouldAttemptRetryOnResponse(BaseResponse response) => false;
 
   /// Number of maximum request attempts that can be retried.
+  ///
+  /// Default is 1, meaning the original request plus 1 retry attempt.
+  /// Set to 0 to disable retries, or higher values for more retry attempts.
   int get maxRetryAttempts => 1;
 
+  /// Delay before retrying when an exception occurs.
+  ///
+  /// [retryAttempt] - The current retry attempt number (1-based)
+  ///
+  /// Returns the delay duration. Default is no delay (Duration.zero).
+  /// Consider implementing exponential backoff for production use.
   Duration delayRetryAttemptOnException({required int retryAttempt}) =>
       Duration.zero;
 
+  /// Delay before retrying when a response indicates retry is needed.
+  ///
+  /// [retryAttempt] - The current retry attempt number (1-based)
+  ///
+  /// Returns the delay duration. Default is no delay (Duration.zero).
+  /// Consider implementing exponential backoff for production use.
   Duration delayRetryAttemptOnResponse({required int retryAttempt}) =>
       Duration.zero;
 }
diff --git a/lib/utils/query_parameters.dart b/lib/utils/query_parameters.dart
@@ -1,35 +1,63 @@
 /// Takes a string and appends [parameters] as query parameters of [url].
 ///
-/// It does not check if [url] is valid, it just appends the parameters.
+/// It validates the URL structure and properly encodes both keys and values
+/// to prevent URL injection attacks.
 String buildUrlString(String url, Map<String, dynamic>? parameters) {
   // Avoids unnecessary processing.
   if (parameters == null) return url;
 
   // Check if there are parameters to add.
   if (parameters.isNotEmpty) {
+    // Validate URL structure to prevent injection
+    // First check if it looks like a valid HTTP/HTTPS URL
+    if (!url.startsWith('http://') && !url.startsWith('https://')) {
+      throw ArgumentError(
+        'Invalid URL structure: $url - must be a valid HTTP/HTTPS URL',
+      );
+    }
+
+    try {
+      final uri = Uri.parse(url);
+      // Additional validation: ensure it has a host
+      if (uri.host.isEmpty) {
+        throw ArgumentError(
+          'Invalid URL structure: $url - must have a valid host',
+        );
+      }
+    } catch (e) {
+      if (e is ArgumentError) {
+        rethrow;
+      }
+      throw ArgumentError('Invalid URL structure: $url');
+    }
+
     // Checks if the string url already has parameters.
     if (url.contains("?")) {
       url += "&";
     } else {
       url += "?";
     }
 
-    // Concat every parameter to the string url.
+    // Concat every parameter to the string url with proper encoding
     parameters.forEach((key, value) {
+      // Encode the key to prevent injection
+      final encodedKey = Uri.encodeQueryComponent(key);
+
       if (value is List) {
         if (value is List<String>) {
           for (String singleValue in value) {
-            url += "$key=${Uri.encodeQueryComponent(singleValue)}&";
+            url += "$encodedKey=${Uri.encodeQueryComponent(singleValue)}&";
           }
         } else {
           for (dynamic singleValue in value) {
-            url += "$key=${Uri.encodeQueryComponent(singleValue.toString())}&";
+            url +=
+                "$encodedKey=${Uri.encodeQueryComponent(singleValue.toString())}&";
           }
         }
       } else if (value is String) {
-        url += "$key=${Uri.encodeQueryComponent(value)}&";
+        url += "$encodedKey=${Uri.encodeQueryComponent(value)}&";
       } else {
-        url += "$key=${Uri.encodeQueryComponent(value.toString())}&";
+        url += "$encodedKey=${Uri.encodeQueryComponent(value.toString())}&";
       }
     });
 
diff --git a/test/utils/utils_test.dart b/test/utils/utils_test.dart
