Skip to content

Conversation

@alexeyzimarev
Copy link
Member

Description

Fixes #2292

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Needs tests

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 11, 2025

Deploying restsharp with  Cloudflare Pages  Cloudflare Pages

Latest commit: 316baf6
Status: ✅  Deploy successful!
Preview URL: https://85e161bb.restsharp.pages.dev
Branch Preview URL: https://ignore-invalid-cookies.restsharp.pages.dev

View logs

@github-actions
Copy link

github-actions bot commented Sep 11, 2025

Test Results

   25 files     25 suites   10m 43s ⏱️
  446 tests   446 ✅ 0 💤 0 ❌
2 128 runs  2 128 ✅ 0 💤 0 ❌

Results for commit 316baf6.

♻️ This comment has been updated with latest results.

@alexeyzimarev
Copy link
Member Author

/review

@qodo-merge-for-open-source
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

2292 - PR Code Verified

Compliant requirements:

  • Added a boolean configuration option (IgnoreInvalidCookies) in RestClientOptions
  • Implemented logic to catch CookieException and conditionally ignore it based on the option
  • Allows accepting request data even when invalid cookies are present

Requires further human verification:

  • Verify that the implementation correctly handles the specific use case with domain mismatch cookies (api.new-domain.com returning .old-domain.com cookies)
  • Confirm that the response data is properly returned when invalid cookies are ignored
  • Test the behavior when IgnoreInvalidCookies is false (default) to ensure exceptions are still thrown as expected
⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Logic Error

The catch block at line 156 uses when (!Options.IgnoreInvalidCookies) which means it will only catch and re-throw when the option is false. However, when the option is true (ignore invalid cookies), the exception will propagate uncaught. The logic should be when (Options.IgnoreInvalidCookies) to catch and suppress the exception when ignoring is enabled, or restructure to catch unconditionally and only re-throw when not ignoring.

catch (CookieException) when (!Options.IgnoreInvalidCookies) {
    throw;
}
Incomplete Refactoring

Lines 162 and 169 appear to be moved outside their original context. Line 162 returns an HttpResponse in a generic catch block, but the original code shows this was inside the try-catch. Line 169 also appears misplaced. This suggests incomplete refactoring that may break the control flow and error handling logic.

            return new(null, url, null, ex, timeoutCts.Token);
        }

#pragma warning disable CS0618 // Type or member is obsolete
        if (request.OnAfterRequest != null) await request.OnAfterRequest(responseMessage).ConfigureAwait(false);
#pragma warning restore CS0618 // Type or member is obsolete
        await OnAfterHttpRequest(request, responseMessage, cancellationToken).ConfigureAwait(false);
        return new(responseMessage, url, cookieContainer, null, timeoutCts.Token);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

option to ignore response cookies

2 participants