-
Notifications
You must be signed in to change notification settings - Fork 82
Don't include the Request Headers in the UnexpectedHttpStatusCodeException message anymore #369
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
Don't include the Request Headers in the UnexpectedHttpStatusCodeException message anymore #369
Conversation
…HttpRequestMessage.ToString anymore, it includes the header values e.g. "Authorization". Now append all the request properties ourself, except for Headers.
|
I think it's super reasonable to don't include the headers, usually it's a best practice to redact headers from any message than can be logged, and an exception is usually something that is logged, so for me it's a 👍 |
|
So what now? Does everyone need to approve? How long do you think that will take? |
|
Hello? Anyone? |
|
Waiting for more than a month now for any further reaction. Sorry, now everyone's getting a ping. |
|
❤ |
|
Oh right. Still need a new release for the change to be available on nuget. |
@zidad Could you make one soon? Or do you usually want to wait to batch more changes? I hope not 😅 |
|
@floge07 sorry to make you wait, I'm publishing it now! |
|
@floge07 if you're a heavy user of easynetq.management, are you interested in becoming a maintainer? |
Interesting offer, but I don't think it would make sense for me. I mean I only ran into this "regression" because I finally updated from a 3 years old version and then I got such a quick initial response to the issue I opened, and then I was kind of stuck with a sunk cost fallacy to get this pr through. 😅 |
Resolves #368
I basically looked at the HttpRequestMessage.ToString and replicated it, except for the Headers appending part.
At first, I wanted to only redact/censor the individual values for sensitive Headers but that got too annoying. The DumpHeaders method is internal and replicating it seemed wrong. Then I tried cloning the RequestMessage object but with redacted headers and simply doing ToString on that object, but cloning the Content is really weird... Then I thought about a Regex Replace on the result string... 😑 yeah, nope.
So I just pushed this solution, that doesn't include any Headers at all. Maybe no one even cares about the Header values in general and I don't need to put more time into this.
Message before:
Now: