Skip to content

Commit ae6b354

Browse files
Implement custom redirect handling to fix lost Set-Cookie on redirects (#2360)
* Implement custom redirect handling to fix lost Set-Cookie on redirects Fixes #2077 and #2059. Previously RestSharp delegated redirects to HttpClient (AllowAutoRedirect=true) but set UseCookies=false, so Set-Cookie headers from intermediate redirect responses were silently lost. This replaces HttpClient's redirect handling with a custom loop in ExecuteRequestAsync that processes Set-Cookie at each hop. Adds RedirectOptions class with fine-grained control over redirect behavior: FollowRedirectsToInsecure, ForwardHeaders, ForwardAuthorization, ForwardCookies, ForwardBody, ForwardQuery, MaxRedirects, and RedirectStatusCodes. Existing FollowRedirects/MaxRedirects properties delegate to RedirectOptions for backward compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Address PR review feedback: reduce complexity, fix disposal, fix duplicate cookies - Extract redirect loop into smaller focused methods (SendWithRedirectsAsync, ShouldFollowRedirect, ResolveRedirectUrl, CreateRedirectMessage, ParseResponseCookies, AddPendingCookies) to reduce cognitive complexity - Fix double-dispose warning (S3966) by using previousMessage pattern and try/finally for message disposal in SendWithRedirectsAsync - Fix duplicate Cookie header bug in AddCookieHeaders (remove existing parameter before adding merged cookies) - Add Host/CacheControl headers to redirect request messages - Add comments for intentional cert validation bypass in HTTPS tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Reduce test code duplication flagged by SonarCloud - Move shared test endpoints (set-cookie-and-redirect, echo-cookies, redirect-no-query, redirect-custom-status) into WireMockTestServer - Switch CookieRedirectTests to use IClassFixture<WireMockTestServer> instead of standalone WireMockServer, eliminating cross-file duplication - Parameterize verb change tests with [Theory]/[InlineData] (5 tests → 1) - Parameterize header, auth, query, and HTTPS tests with [Theory] - Extract CreateClient helper to reduce setup boilerplate - CookieRedirectTests: 616 → 336 lines (45% reduction) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Strip Authorization header on cross-origin and HTTPS-to-HTTP redirects Address security concern from PR review: ForwardAuthorization could leak credentials to unintended hosts on redirect. - Compare full authority (host+port) against original request URL, matching browser same-origin policy - Always strip Authorization on HTTPS→HTTP redirects (defense-in-depth) - Add ForwardAuthorizationToExternalHost option (default false) for explicit opt-in to cross-origin auth forwarding - Add tests for cross-host auth stripping and explicit opt-in Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Consolidate cross-host auth tests into parameterized Theory to reduce duplication Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Reduce code duplication: consolidate ForwardBody tests and reuse shared EchoRequest - Merge ForwardBody_False and ForwardBody_True into a single parameterized Theory - Replace inline echo-request callback with shared WireMockTestServer.EchoRequest - Make EchoRequest public for cross-project reuse Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 91dfd97 commit ae6b354

File tree

8 files changed

+817
-50
lines changed

8 files changed

+817
-50
lines changed
Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
// Copyright (c) .NET Foundation and Contributors
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
//
15+
16+
namespace RestSharp;
17+
18+
/// <summary>
19+
/// Options for controlling redirect behavior when RestSharp handles redirects.
20+
/// </summary>
21+
public class RedirectOptions {
22+
/// <summary>
23+
/// Whether to follow redirects. Default is true.
24+
/// </summary>
25+
public bool FollowRedirects { get; set; } = true;
26+
27+
/// <summary>
28+
/// Whether to follow redirects from HTTPS to HTTP (insecure). Default is false.
29+
/// </summary>
30+
public bool FollowRedirectsToInsecure { get; set; }
31+
32+
/// <summary>
33+
/// Whether to forward request headers on redirect. Default is true.
34+
/// </summary>
35+
public bool ForwardHeaders { get; set; } = true;
36+
37+
/// <summary>
38+
/// Whether to forward the Authorization header on same-host redirects. Default is false.
39+
/// Even when enabled, Authorization is stripped on cross-host redirects unless
40+
/// <see cref="ForwardAuthorizationToExternalHost"/> is also set to true.
41+
/// </summary>
42+
public bool ForwardAuthorization { get; set; }
43+
44+
/// <summary>
45+
/// Whether to forward the Authorization header when redirecting to a different host. Default is false.
46+
/// Only applies when <see cref="ForwardAuthorization"/> is true. Enabling this can expose credentials
47+
/// to unintended hosts if a redirect points to a third-party server.
48+
/// </summary>
49+
public bool ForwardAuthorizationToExternalHost { get; set; }
50+
51+
/// <summary>
52+
/// Whether to forward cookies on redirect. Default is true.
53+
/// Cookies from Set-Cookie headers are always stored in the CookieContainer regardless of this setting.
54+
/// </summary>
55+
public bool ForwardCookies { get; set; } = true;
56+
57+
/// <summary>
58+
/// Whether to forward the request body on redirect when the HTTP verb is preserved. Default is true.
59+
/// Body is always dropped when the verb changes to GET.
60+
/// </summary>
61+
public bool ForwardBody { get; set; } = true;
62+
63+
/// <summary>
64+
/// Whether to forward original query string parameters on redirect. Default is true.
65+
/// </summary>
66+
public bool ForwardQuery { get; set; } = true;
67+
68+
/// <summary>
69+
/// Maximum number of redirects to follow. Default is 50.
70+
/// </summary>
71+
public int MaxRedirects { get; set; } = 50;
72+
73+
/// <summary>
74+
/// HTTP status codes that are considered redirects.
75+
/// </summary>
76+
public IReadOnlyList<HttpStatusCode> RedirectStatusCodes { get; set; } = [
77+
HttpStatusCode.MovedPermanently, // 301
78+
HttpStatusCode.Found, // 302
79+
HttpStatusCode.SeeOther, // 303
80+
HttpStatusCode.TemporaryRedirect, // 307
81+
(HttpStatusCode)308, // 308 Permanent Redirect
82+
];
83+
}

src/RestSharp/Options/RestClientOptions.cs

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,19 @@ public RestClientOptions(string baseUrl) : this(new Uri(Ensure.NotEmptyString(ba
108108
#endif
109109

110110
/// <summary>
111-
/// Set the maximum number of redirects to follow
111+
/// Set the maximum number of redirects to follow.
112+
/// This is a convenience property that delegates to <see cref="RedirectOptions"/>.MaxRedirects.
112113
/// </summary>
113114
#if NET
114115
[UnsupportedOSPlatform("browser")]
115116
#endif
116-
public int? MaxRedirects { get; set; }
117+
[Exclude]
118+
public int? MaxRedirects {
119+
get => RedirectOptions.MaxRedirects;
120+
set {
121+
if (value.HasValue) RedirectOptions.MaxRedirects = value.Value;
122+
}
123+
}
117124

118125
/// <summary>
119126
/// X509CertificateCollection to be sent with request
@@ -141,8 +148,18 @@ public RestClientOptions(string baseUrl) : this(new Uri(Ensure.NotEmptyString(ba
141148

142149
/// <summary>
143150
/// Instruct the client to follow redirects. Default is true.
151+
/// This is a convenience property that delegates to <see cref="RedirectOptions"/>.FollowRedirects.
152+
/// </summary>
153+
[Exclude]
154+
public bool FollowRedirects {
155+
get => RedirectOptions.FollowRedirects;
156+
set => RedirectOptions.FollowRedirects = value;
157+
}
158+
159+
/// <summary>
160+
/// Options for controlling redirect behavior.
144161
/// </summary>
145-
public bool FollowRedirects { get; set; } = true;
162+
public RedirectOptions RedirectOptions { get; set; } = new();
146163

147164
/// <summary>
148165
/// Gets or sets a value that indicates if the <see langword="Expect" /> header for an HTTP request contains Continue.

src/RestSharp/Request/RequestHeaders.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,11 @@ public RequestHeaders AddAcceptHeader(string[] acceptedContentTypes) {
3535
return this;
3636
}
3737

38+
public RequestHeaders RemoveHeader(string name) {
39+
Parameters.RemoveAll(p => string.Equals(p.Name, name, StringComparison.InvariantCultureIgnoreCase));
40+
return this;
41+
}
42+
3843
// Add Cookie header from the cookie container
3944
public RequestHeaders AddCookieHeaders(Uri uri, CookieContainer? cookieContainer) {
4045
if (cookieContainer == null) return this;
@@ -48,6 +53,7 @@ public RequestHeaders AddCookieHeaders(Uri uri, CookieContainer? cookieContainer
4853

4954
if (existing?.Value != null) {
5055
newCookies = newCookies.Union(SplitHeader(existing.Value!));
56+
Parameters.Remove(existing);
5157
}
5258

5359
Parameters.Add(new(KnownHeaders.Cookie, string.Join("; ", newCookies)));

0 commit comments

Comments
 (0)