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

Message of UriFormatException does not include offending URI #30925

Open
andrau opened this issue Sep 23, 2019 · 6 comments
Open

Message of UriFormatException does not include offending URI #30925

andrau opened this issue Sep 23, 2019 · 6 comments
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@andrau
Copy link

andrau commented Sep 23, 2019

When reading an URI or even several URIs from configuration files as strings and instantiating them using new Uri(string), it would be helpful if the exception for invalid URIs (e.g. because of typos) would include the offending Uri (especially if the exception is in a logfile of a production system where debugging is not an easily available option).

This would save the need to do a try-catch around every new Uri() and pretty-print the log message and allow safeguarding an entire initialization section with a more general try - catch(Exception) block without losing any information.

Example Code:

    static void UriFormatExceptionWithoutOffendingUri()
    {
      try
      {
        var uri1 = new Uri("http://www.valid.com");
        var uri2 = new Uri("foo//invalid");
        var uri3 = new Uri("https://www.secure.org");
      }
      catch (UriFormatException ex)
      {
        Console.WriteLine(ex.Message);
      }
    }

yields: "Invalid URI: The format of the URI could not be determined."

So we cannot easily determine which URI was invalid

desired: "Invalid URI: The format of the URI 'foo//invalid' could not be determined."

Would that be safe or do you think this would disclose too much information?

@Wraith2
Copy link
Contributor

Wraith2 commented Sep 23, 2019

Is this not what Uri.TryCreate is for?

@andrau
Copy link
Author

andrau commented Sep 23, 2019

Well, it's a little better than using try-catch but still would require plastering the code with explicit checks while the intention is only to be able to see what went wrong in the logs if somebody breaks the configuration.

Of course you could argue that proper defensive programming does require explicit checks but even then the exception would be more helpful with the broken URI in the message when someone decides against the check (or simply forgets to implement one).

@davidsh
Copy link
Contributor

davidsh commented Sep 23, 2019

desired: "Invalid URI: The format of the URI 'foo//invalid' could not be determined."
Would that be safe or do you think this would disclose too much information?

I don't think there are any issues about 'safety' or disclosure here. We already have error message text that shows things like "invalid header X" for our Http APIs (where 'X' represents the invalid header itself). I think this would be an ok enhancement to our error message text.

Feel free to submit a PR for a change like this.

@scalablecory
Copy link
Contributor

This could result in some of those old http://user:password@foo.com URLs getting logged, but that is probably a rare case.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@Anderman
Copy link

Anderman commented Sep 5, 2024

Can I still create a PR for this issue

@julealgon
Copy link

What about exposing potentially sensitive querystring parameters?

There's been a recent set of changes across many libraries including OpenTelemetry ones to redact query string values by default as that was considered a security vulnerability.

There is also this:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

7 participants