-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
notify/webhook: Fix crash on errors when url_file is used (#3798) #3800
Conversation
…#3798) When using url_file the conf.URL will be nil and when an error occurs we will panic. Given that the URL is considered a secret, let's just remove the custom details func. Signed-off-by: Holger Hans Peter Freyther <holger@freyther.de>
Good find that URL is not redacted. I understand that this change removes both the response and URL when the server returns HTTP 500. Something to keep in mind is that
It would be nice if we could either show |
My understanding was that we continue to include the response body (if present): Line 241 in 14cbe63
I think the remaining comes from the notify.RedactURL code that handles net/url.Error to remove the URL. What is the benefit of further modifying this? |
I thought it would be nice if we can make the error message consistent. For example, how it works right now is if the server returns a 5xx error the URL is logged. But if the error is a Your fix removes the URL in the case of 5xx error – but should we also remove |
Is there any indication of someone expecting the error to follow the url.Error.Error() format (link) I am torn on this. The formats are not the same today (one uses %s and the other %q) and then we don't have the URL readily available in the CustomDetailsFunc. Just printing At the same time modifying notify.RedactURL to change Am I missing something? |
It's OK – I realized I'm distracting the actual contribution which is fixing a panic. I'm going to test this now and let @gotjosh know. |
I've tested it and looks good to me! Thank you for your contribution! 👍 Here is the log output for the different cases I tested: HTTP 5xx
Connection refused
HTTP 5xx with "Hello, world!" in the response
|
LGTM |
When using url_file the conf.URL will be nil and when an error occurs we will panic. Given that the URL is considered a secret, let's just remove the custom details func.