Skip to content

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Nov 29, 2025

User description

Dedicated serialization context for Browser module.

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Create dedicated BrowserJsonSerializerContext for Browser module

  • Remove Browser types from shared BiDiJsonSerializerContext

  • Update Browser module to use module-specific serialization context

  • Improve separation of concerns for JSON serialization


Diagram Walkthrough

flowchart LR
  A["BiDiJsonSerializerContext<br/>Shared context"] -->|"includes"| B["Browser types<br/>CloseCommand, etc."]
  C["BrowserJsonSerializerContext<br/>New dedicated context"] -->|"replaces"| B
  D["BrowserModule"] -->|"uses"| C
  A -->|"chains to"| C
Loading

File Walkthrough

Relevant files
Enhancement
BrowserModule.cs
Implement dedicated Browser JSON serialization context     

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs

  • Replace BiDiJsonSerializerContext with new
    BrowserJsonSerializerContext
  • Update Initialize method to chain BiDiJsonSerializerContext into
    Browser-specific context
  • Add BrowserJsonSerializerContext class with [JsonSerializable]
    attributes for all Browser types
  • Simplify property name references from
    _jsonContext.Browser_CloseCommand to _jsonContext.CloseCommand
+26/-3   
Refactoring
BiDiJsonSerializerContext.cs
Remove Browser types from shared context                                 

dotnet/src/webdriver/BiDi/Json/BiDiJsonSerializerContext.cs

  • Remove 12 [JsonSerializable] attributes for Browser module types
  • Remove Browser.CloseCommand and Browser.CloseResult with custom
    TypeInfoPropertyName
  • Remove Browser-related collection types from shared context
  • Reduce shared context responsibility to non-Browser BiDi types
+0/-16   

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Nov 29, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

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: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing auditing: The new Browser module methods invoke commands without adding or referencing any audit
logging of critical actions such as browser close or download behavior changes.

Referred Code
public async Task<CloseResult> CloseAsync(CloseOptions? options = null)
{
    return await Broker.ExecuteCommandAsync(new CloseCommand(), options, _jsonContext.CloseCommand, _jsonContext.CloseResult).ConfigureAwait(false);
}

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:
Unhandled failures: The new command executions delegate to Broker without visible error handling or
context-specific exceptions, making it unclear how failures or null/edge cases are
managed.

Referred Code
public async Task<CloseResult> CloseAsync(CloseOptions? options = null)
{
    return await Broker.ExecuteCommandAsync(new CloseCommand(), options, _jsonContext.CloseCommand, _jsonContext.CloseResult).ConfigureAwait(false);
}

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:
Error exposure unknown: The code adds no user-facing error handling and relies on Broker; without visibility into
Broker, it is unclear whether detailed internal errors could be surfaced to callers.

Referred Code
public async Task<CloseResult> CloseAsync(CloseOptions? options = null)
{
    return await Broker.ExecuteCommandAsync(new CloseCommand(), options, _jsonContext.CloseCommand, _jsonContext.CloseResult).ConfigureAwait(false);
}

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:
Input validation gap: Methods accept options and construct parameters passed to Broker without visible
validation or sanitization of external inputs such as proxies or behaviors in the new code
paths.

Referred Code
public async Task<SetDownloadBehaviorResult> SetDownloadBehaviorDeniedAsync(SetDownloadBehaviorOptions? options = null)
{
    var @params = new SetDownloadBehaviorParameters(new DownloadBehaviorDenied(), options?.UserContexts);

    return await Broker.ExecuteCommandAsync(new SetDownloadBehaviorCommand(@params), options, _jsonContext.SetDownloadBehaviorCommand, _jsonContext.SetDownloadBehaviorResult).ConfigureAwait(false);
}

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Nov 29, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Validate string input parameters

Validate destinationFolder (null/empty/whitespace) and normalize/encode or
reject invalid values before invoking the command to avoid invalid requests or
injection issues.

dotnet/src/webdriver/BiDi/Browser/BrowserModule.cs [62-67]

 public async Task<SetDownloadBehaviorResult> SetDownloadBehaviorAllowedAsync(string destinationFolder, SetDownloadBehaviorOptions? options = null)
-... (clipped 4 lines)
+{
+    if (string.IsNullOrWhiteSpace(destinationFolder))
+    {
+        throw new ArgumentException("Destination folder must be a non-empty path.", nameof(destinationFolder));
+    }
+    destinationFolder = destinationFolder.Trim();
+    // optionally: Path.GetFullPath(destinationFolder) or additional validation
+    ... (rest of method unchanged)
+}

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate and sanitize external inputs, and guard against null or whitespace strings before using them in API calls.

Low
  • Update

@nvborisenko nvborisenko changed the title [dotnet] [bidi] Dedicated json context for Browser module [dotnet] [bidi] Dedicated json context for all modules Nov 30, 2025
@nvborisenko
Copy link
Member Author

Yahoo, it is easier than I thought!

@nvborisenko nvborisenko merged commit d178006 into SeleniumHQ:trunk Nov 30, 2025
10 checks passed
@nvborisenko nvborisenko deleted the bidi-context-browser branch November 30, 2025 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants