Don't let Uri to escape query

[alexeyzimarev]

User description

Description

Fixes #2207

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)

---

PR Type

Bug fix

---

Description

• Fix pipe character encoding in query parameters with Encode=false

• Implement proper query string parsing to preserve unencoded values

• Add DangerousDisablePathAndQueryCanonicalization for .NET 6+ URI handling

• Refactor URI building to prevent automatic re-encoding of query strings

---

Diagram Walkthrough

flowchart LR
  A["Query Parameter<br/>with encode=false"] -->|"AddQueryParameter"| B["RestRequest"]
  B -->|"BuildUri"| C["UriExtensions"]
  C -->|"Parse & Preserve"| D["Parsers.ParseQueryString"]
  D -->|"Disable Canonicalization<br/>.NET 6+"| E["Uri with<br/>DangerousDisablePathAndQueryCanonicalization"]
  E -->|"Result"| F["Unencoded Pipe<br/>ids=in:001|116"]

Loading

File Walkthrough

	Relevant files
Enhancement	2 files Parsers.cs New query string parser preserving encoded values                +60/-0    BuildUriExtensions.cs Replace ToLowerInvariant with StringComparison                      +1/-1
Bug fix	2 files UriExtensions.cs Disable URI canonicalization for query preservation            +44/-8    RestRequest.cs Use new parser for query string extraction                              +24/-27
Tests	5 files UrlBuilderTests.cs Add test for pipe character preservation                                  +124/-191 RestRequestTests.cs Update test for query parameter parsing                                    +13/-4    ParametersTests.cs Fix assertion comparison order                                                      +1/-1      RestClientTests.cs Remove duplicate URI building tests                                            +0/-32    ResourceStringParametersTests.cs Add URI building assertion to test                                              +2/-0

---

[cloudflare-workers-and-pages]

Deploying restsharp with    Cloudflare Pages

Latest commit:	04699ff
Status:	✅  Deploy successful!
Preview URL:	https://597b0e9e.restsharp.pages.dev
Branch Preview URL:	https://dont-encode-query.restsharp.pages.dev

View logs

[github-actions]

Test Results

   32 files     32 suites   15m 0s ⏱️
  437 tests   436 ✅ 0 💤 1 ❌
2 543 runs  2 542 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 04699ff.

[qodo-free-for-open-source-projects]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	URI canonicalization disabled Description: Using DangerousDisablePathAndQueryCanonicalization allows unencoded characters in URIs, potentially enabling path traversal attacks or injection vulnerabilities if user input is not properly validated before being added to URIs. UriExtensions.cs [22-22] Referred Code internal static UriCreationOptions UriOptions = new() { DangerousDisablePathAndQueryCanonicalization = true }; #endif
	Obsolete Uri constructor Description: Obsolete Uri constructor with dontEscape parameter is used for .NET Framework, which is deprecated due to security concerns and may not properly handle malicious input. UriExtensions.cs [40-40] Referred Code ? new Uri(assembled, true) #pragma warning restore CS0618 // Type or member is obsolete
Ticket Compliance
🟡	🎫 #2207 🟢 Prevent encoding of pipe character (|) when AddQueryParameter is called with encode=false Fix the issue where pipe character is encoded as %7C even with encode=false option Ensure that query parameter 'ids=in:001|116' is transmitted without encoding the pipe character ⚪ Maintain existing behavior for other characters like slash (/) which already work correctly
Codebase Duplication Compliance
🟢	No codebase code duplication found New Components Detected (Top 5): - Parsers - ParseQueryString - Should_build_with_passing_link_as_Uri - Should_build_with_passing_link_as_Uri_with_set_BaseUrl - Should_encode_resource
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Unclear variable names: Variables like startIndex1, startIndex2, and num lack descriptive names that convey their purpose in query string parsing Referred Code var startIndex1 = query[0] == '?' ? 1 : 0; if (length == startIndex1) yield break; while (startIndex1 <= length) { var startIndex2 = -1; var num = -1; for (var index = startIndex1; index < length; ++index) { if (startIndex2 == -1 && query[index] == '=') startIndex2 = index + 1; else if (query[index] == '&') { num = index; break; } } string? name; if (startIndex2 == -1) { ... (clipped 10 lines) Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Missing edge case handling: The ParseQueryString method lacks explicit handling for malformed query strings or invalid encoding scenarios Referred Code public static IEnumerable<KeyValuePair<string, string?>> ParseQueryString(string query, Encoding encoding) { Ensure.NotNull(query, nameof(query)); Ensure.NotNull(encoding, nameof(encoding)); var length = query.Length; var startIndex1 = query[0] == '?' ? 1 : 0; if (length == startIndex1) yield break; while (startIndex1 <= length) { var startIndex2 = -1; var num = -1; for (var index = startIndex1; index < length; ++index) { if (startIndex2 == -1 && query[index] == '=') startIndex2 = index + 1; else if (query[index] == '&') { num = index; break; } } ... (clipped 17 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Potential injection vulnerability: The query string parsing uses HttpUtility.UrlDecode without additional validation, which may allow injection attacks if malicious query strings are processed Referred Code name = HttpUtility.UrlDecode(query.Substring(startIndex1, startIndex2 - startIndex1 - 1), encoding); if (num < 0) num = query.Length; startIndex1 = num + 1; var str = HttpUtility.UrlDecode(query.Substring(startIndex2, num - startIndex2), encoding); Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[alexeyzimarev]

@copilot fix the merge conflicts

[qodo-free-for-open-source-projects]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix incorrect parsing of valueless query parameters Fix the ParseQueryString method to correctly handle query parameters that do not have an explicit value. The current logic incorrectly swaps the key and value in such cases. src/RestSharp/Request/Parsers.cs [22-59] public static IEnumerable<KeyValuePair<string, string?>> ParseQueryString(string query, Encoding encoding) { Ensure.NotNull(query, nameof(query)); Ensure.NotNull(encoding, nameof(encoding)); var length = query.Length; var startIndex1 = query[0] == '?' ? 1 : 0; if (length == startIndex1) yield break; while (startIndex1 <= length) { var startIndex2 = -1; var num = -1; for (var index = startIndex1; index < length; ++index) { if (startIndex2 == -1 && query[index] == '=') startIndex2 = index + 1; else if (query[index] == '&') { num = index; break; } } + if (num < 0) + num = length; + string? name; + string? value; if (startIndex2 == -1) { - name = null; - startIndex2 = startIndex1; + name = HttpUtility.UrlDecode(query.Substring(startIndex1, num - startIndex1), encoding); + value = ""; } - else - name = HttpUtility.UrlDecode(query.Substring(startIndex1, startIndex2 - startIndex1 - 1), encoding); + else { + name = HttpUtility.UrlDecode(query.Substring(startIndex1, startIndex2 - startIndex1 - 1), encoding); + value = HttpUtility.UrlDecode(query.Substring(startIndex2, num - startIndex2), encoding); + } - if (num < 0) - num = query.Length; startIndex1 = num + 1; - var str = HttpUtility.UrlDecode(query.Substring(startIndex2, num - startIndex2), encoding); - yield return new KeyValuePair<string, string?>(name ?? "", str); + yield return new KeyValuePair<string, string?>(name, value); } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a bug in the new ParseQueryString method where query parameters without a value (e.g., ?key) are parsed incorrectly as key="", value="key". The proposed fix correctly parses them as key="key", value="", which is a critical correction for query string parsing logic.	Medium
More

[Copilot]

@alexeyzimarev I've opened a new pull request, #2344, to work on those changes. Once the pull request is ready, I'll request review from you.