Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dotnet] Tolerate invalid UTF-16 strings in DevTools JSON response #14972

Merged
merged 21 commits into from
Jan 10, 2025

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Dec 28, 2024

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

This restores Newtonsoft behavior of tolerating invalid UTF-16 strings.

Motivation and Context

Fixes #14903

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.

Copy link
Contributor

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

Typo

The word "Recieved" is misspelled in the log message. Should be "Received"

LogTrace("Recieved Event {0}: {1}", method, loggableEventData.ToString());
Error Handling

The code assumes postData and request properties exist and can be accessed without null checks, which could lead to NullReferenceException

var loggableRequest = loggableEventData["request"]!;
loggableRequest["postData"] = "*RAW POST DATA REMOVED FROM LOGS*";
loggableRequest["postDataEntries"] = new JsonArray();

Copy link
Contributor

qodo-merge-pro bot commented Dec 28, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add defensive programming by validating JSON data before access to prevent runtime exceptions

Add null check for eventData before accessing it to prevent potential
NullReferenceException

dotnet/src/webdriver/DevTools/DevToolsSession.cs [603]

-if (eventData.AsObject().TryGetPropertyValue("request", out var requestNode)
+if (eventData != null && eventData.AsObject().TryGetPropertyValue("request", out var requestNode)
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: This is an important defensive programming suggestion that could prevent runtime crashes. The null check is crucial when dealing with JSON data that might be malformed or incomplete.

8
General
Correct spelling error in log message to maintain code quality and professionalism

Fix the typo in the log message where "Received" is misspelled as "Recieved"

dotnet/src/webdriver/DevTools/DevToolsSession.cs [610]

-LogTrace("Recieved Event {0}: {1}", method, loggableEventData.ToString());
+LogTrace("Received Event {0}: {1}", method, loggableEventData.ToString());
  • Apply this suggestion
Suggestion importance[1-10]: 3

Why: While this is a valid correction of a spelling mistake ("Recieved" to "Received"), it's a minor issue that only affects log messages and doesn't impact functionality.

3

@RenderMichael RenderMichael changed the title [dotnet] Avoid exceptions in cdp logging on file upload [dotnet] Tolerate invalid UTF-16 strings in DevTools JSON response Jan 8, 2025
@RenderMichael
Copy link
Contributor Author

Repro test:

[Test]
public void AAA()
{
    driver.Manage().Network.StartMonitoring().GetAwaiter().GetResult(); // this enables serialization
    driver.Navigate().GoToUrl("https://tus.io/demo");

    Thread.Sleep(10_000);
    driver.FindElement(By.Id("P0-0")).SendKeys("C:\\Users\\Nick\\Downloads\\Firefox Installer.exe");

    Thread.Sleep(60_000);
}

Passes both on Firefox and Chrome.

@RenderMichael
Copy link
Contributor Author

That's my bad for forgetting the file header. Fixed.

Copy link
Member

@nvborisenko nvborisenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RenderMichael !

@nvborisenko nvborisenko merged commit c1bb977 into SeleniumHQ:trunk Jan 10, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: IWebDriver.Manage().Network.StartMonitoring() causes file uploads to fail
2 participants