Home Original page

[dotnet] [bidi] Adjust proxy configuration for new sessions by nvborisenko · Pull Request #15914 · SeleniumHQ/selenium

Conversation

@nvborisenko

@nvborisenko nvborisenko commented

Jun 19, 2025

•

edited by qodo-code-review bot

Loading

User description

https://w3c.github.io/webdriver-bidi/#ref-for-cddl-type-sessionmanualproxyconfiguration

  • Removed FTP
  • Added separate type to represent Socks
  • Added no proxy support

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Enhancement

Description

• Remove FTP proxy support from BiDi proxy configuration
• Add no proxy list support for bypassing proxy
• Extract SOCKS proxy configuration into separate interface
• Change SOCKS version type from long to int

Changes walkthrough 📝

Relevant files
Enhancement
ProxyConfiguration.cs
Refactor proxy configuration structure                                     

dotnet/src/webdriver/BiDi/Session/ProxyConfiguration.cs

• Removed FtpProxy property from ManualProxyConfiguration
• Added
NoProxy property for proxy bypass list
• Created
ISocksProxyConfiguration interface for SOCKS settings
• Changed
SocksVersion type from long to int

 +12/-4   
Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.

    • @qodo-code-review

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review
    Interface Design

    The ISocksProxyConfiguration interface has mutable properties with setters, which is unusual for interfaces and may not align with the immutable record pattern used elsewhere in the codebase.

     
    public interface ISocksProxyConfiguration
{
    public string? SocksProxy { get; set; }

    public int? SocksVersion { get; set; } // 0..255
}
    Breaking Change

    Removing FtpProxy property and changing SocksVersion from long to int are breaking changes that could affect existing code using these properties.

     
    public record ManualProxyConfiguration : ProxyConfiguration, ISocksProxyConfiguration
{
    public string? HttpProxy { get; set; }

    public string? SslProxy { get; set; }

    public string? SocksProxy { get; set; }

    public int? SocksVersion { get; set; }

    public IEnumerable<string>? NoProxy { get; set; }
}

    @qodo-code-review

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add range validation for SocksVersion

    Add validation to ensure SocksVersion stays within the documented range of
    0-255. Consider using a property with validation logic or constraints to prevent
    invalid values.

    dotnet/src/webdriver/BiDi/Session/ProxyConfiguration.cs [58]

    -public int? SocksVersion { get; set; } // 0..255
+private int? _socksVersion;
+public int? SocksVersion 
+{ 
+    get => _socksVersion;
+    set => _socksVersion = value is >= 0 and <= 255 ? value : throw new ArgumentOutOfRangeException(nameof(value), "SocksVersion must be between 0 and 255");
+}
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that the SocksVersion property, which has a documented range of 0-255, lacks validation. Adding a check in the setter of the implementing class (ManualProxyConfiguration) would improve the code's robustness by preventing invalid values from being assigned.

    		Medium
    • More

    @nvborisenko

    CI is failing not related to this PR.

    Labels

    2 participants

    @nvborisenko @selenium-ci