java/CookieHandler: escape cookie values when required

[Delta456]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

CookieHandler now escapes cookie name and value when required

Motivation and Context

It has listed in the TODO comment

Types of changes

• Bug fix (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 change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

enhancement

---

Description

• Added a new method escapeCookieValue to handle the escaping of special characters in cookie names and values.
• Updated the addCookie method to utilize the new escaping function, ensuring that cookie values are properly sanitized before being added to the response.
• This change addresses a TODO comment and enhances the security and correctness of cookie handling.

---

Changes walkthrough 📝

	Relevant files
Enhancement	CookieHandler.java Implement cookie value escaping in CookieHandler                  java/test/org/openqa/selenium/environment/webserver/CookieHandler.java Introduced escapeCookieValue method to sanitize cookie names and values. Updated addCookie method to use escapeCookieValue for cookie names and values. Ensured special characters in cookies are properly escaped. +15/-3

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Possible Bug The escapeCookieValue method doesn't handle all special characters that might need escaping in cookie values, such as spaces or equals signs. Performance Concern The escapeCookieValue method creates multiple intermediate String objects, which could be inefficient for large cookie values.

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Security	Use a standard URL encoding method for more comprehensive and reliable cookie value escaping Consider using a more robust URL encoding method for cookie values instead of manual character replacement. Java's URLEncoder.encode() method can handle a wider range of special characters and ensure proper encoding. java/test/org/openqa/selenium/environment/webserver/CookieHandler.java [199-204] -return value.replace("\\", "\\\\") - .replace("\"", "\\\"") - .replace(";", "\\;") - .replace(",", "\\,") - .replace("\r", "") - .replace("\n", ""); +return URLEncoder.encode(value, StandardCharsets.UTF_8) + .replace("+", "%20") + .replace("%3B", ";") + .replace("%2C", ","); Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion improves security by using a standard method for encoding, which is more robust and reliable than manual replacements. It addresses potential security vulnerabilities in handling special characters.	9
	Validate cookie names to ensure compliance with RFC 6265 standards Consider adding validation for the cookie name in the addCookie method to ensure it complies with RFC 6265 standards, which specify allowed characters for cookie names. java/test/org/openqa/selenium/environment/webserver/CookieHandler.java [119-121] -String name = escapeCookieValue(cook.getName()); +String name = cook.getName(); +if (!isValidCookieName(name)) { + throw new IllegalArgumentException("Invalid cookie name: " + name); +} String value = escapeCookieValue(cook.getValue()); cookie.append(name).append("=").append(value).append("; "); +// Add this method to the class: +// private boolean isValidCookieName(String name) { +// return name != null && !name.isEmpty() && name.matches("^[!#$%&'*+\\-.0-9A-Z^_`a-z|~]+$"); +// } + Apply this suggestion Suggestion importance[1-10]: 8 Why: This suggestion enhances security by ensuring cookie names comply with standards, preventing potential issues with invalid characters. It's a significant improvement for robustness.	8
Performance	✅ Add a check for empty string to avoid unnecessary processing of empty values Suggestion Impact: The suggestion to add a check for empty strings was directly implemented in the commit, enhancing performance by avoiding unnecessary processing for empty values. code diff: - if (value == null) { + if (value == null || value.isEmpty()) { return ""; Add a check for empty string in the escapeCookieValue method to avoid unnecessary processing and improve performance for empty values. java/test/org/openqa/selenium/environment/webserver/CookieHandler.java [195-197] -if (value == null) { +if (value == null || value.isEmpty()) { return ""; } Apply this suggestion Suggestion importance[1-10]: 7 Why: This suggestion enhances performance by avoiding unnecessary processing for empty strings, which is a minor but useful optimization.	7
	✅ Use StringBuilder for more efficient string manipulation when escaping cookie values Suggestion Impact: The commit implemented the suggestion by replacing multiple String.replace() calls with a StringBuilder for more efficient string manipulation. code diff: + StringBuilder cookieValue = new StringBuilder(); + + for (char c : value.toCharArray()) { + switch (c) { + case '\\': + cookieValue.append("\\\\"); + break; + case '"': + cookieValue.append("\\\""); + break; + case ';': + cookieValue.append("\\;"); + break; + case ',': + cookieValue.append("\\,"); + break; + case '\r': + case '\n': + // Skip carriage return and newline characters + break; + case '<': + cookieValue.append("&lt;"); + break; + case '>': + cookieValue.append("&gt;"); + break; + case '&': + cookieValue.append("&amp;"); + break; + default: + cookieValue.append(c); // Append safe characters as they are + } + } + return cookieValue.toString(); Consider using a StringBuilder instead of multiple String.replace() calls to improve performance, especially for large cookie values. java/test/org/openqa/selenium/environment/webserver/CookieHandler.java [199-204] -return value.replace("\\", "\\\\") - .replace("\"", "\\\"") - .replace(";", "\\;") - .replace(",", "\\,") - .replace("\r", "") - .replace("\n", ""); +StringBuilder sb = new StringBuilder(value.length()); +for (char c : value.toCharArray()) { + switch (c) { + case '\\': sb.append("\\\\"); break; + case '"': sb.append("\\\""); break; + case ';': sb.append("\\;"); break; + case ',': sb.append("\\,"); break; + case '\r': case '\n': break; + default: sb.append(c); + } +} +return sb.toString(); Apply this suggestion Suggestion importance[1-10]: 6 Why: While using StringBuilder can improve performance for large strings, the current implementation with replace() is sufficient for typical use cases. The suggestion is valid but not crucial.	6

[pujagani]

Please fix the formatting by running the script https://github.com/SeleniumHQ/selenium/blob/trunk/scripts/format.sh