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

proposal: net/http.Client should allow omitting URLs from error strings #44819

Open
theckman opened this issue Mar 5, 2021 · 14 comments
Open

proposal: net/http.Client should allow omitting URLs from error strings #44819

theckman opened this issue Mar 5, 2021 · 14 comments
Labels
Projects
Milestone

Comments

@theckman
Copy link
Contributor

@theckman theckman commented Mar 5, 2021

Context

When using a net/http.Client, especially with a context.Context that has a deadline, it’s not uncommon for the http.Client.Do() method to return an error that contains the full URL used when actioning the request. Unfortunately that URL may contain sensitive query parameters, or other things that make it unsafe for directly logging or forwarding elsewhere without first sanitizing the URL or removing it entirely.

There was a Gopher who recently ran into a case where they were almost writing sensitive URLs out to log files, but they managed to catch that in a pre-production environment. They could, at each http.Client.Do() call site, write logic to reformat the error value to not include the URL, but there are a few problems with that approach.

The first is that you’d run the risk of forgetting to add that protection somewhere, or a code change / addition in the future could omit it. In addition if you’re mutating the error value to remove the URL, you’ve made it so that callers cannot inspect that detail of the error if they wanted to use it to decide to handle the error in a specific way.

Proposal

Considering the above context, I’d like to propose that we extend the net/http.Client to optionally omit the URL from any and all errors that get generated when it tries to action a request. This would allow programmers to set the behavior on the client itself, mitigating the first risk of forgetting to filter the URL from the error at a specific call site. This would be accomplished by adding a bool field to the net/http.Client struct: OnErrorOmitURL. The zero value (false) for this field would result in the http.Client behaving like it does today, meaning this would not be a breaking change.

Because the errors returned from the http.Client.Do() method are of type net/url.Error, we will also need to extend that type to include a bool struct field, OmitURL, which results in the Error() method not including the URL in the error string it returns. This will address the second risk above as the URL will still be present in string form within the url.Error.URL field, but it won’t be implicitly included when the error value is printed or logged. Like the field on the net/http.Client, the zero value (false) will result in url.Error working like it does today, meaning this would also not be a breaking change.

@networkimprov
Copy link

@networkimprov networkimprov commented Mar 5, 2021

cc @ianlancetaylor as likely Proposal

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 5, 2021

This seems related to net/url.(*URL).Redacted. And, for that matter, net/http already tries to strip the password in a returned URL (https://go.googlesource.com/go/+/refs/heads/master/src/net/http/client.go#1003).

This seems like a reasonable idea to me if you want to write a proposal. Thanks.

@theckman
Copy link
Contributor Author

@theckman theckman commented Mar 5, 2021

@ianlancetaylor thank you for the quick reply; I can definitely work to reformat this issue into more of a proposal format. I've zero experience with that so I'll figure out how to do it right.

To your comment, yeah it's definitely similar. In this case it was a query parameter that wouldn't be caught by that method, and so I was thinking it might be better to have a way to turn this functionality off versus trying to complicate the stdlib with some sort of programmer-supplied filter specification.

I'll be sure to include that context in the reformat I do, to give others a concrete opportunity to challenge that assumption. Thanks again!

@dmitshur dmitshur changed the title net/url+net/http: Sensitive URLs should be able to be omitted from url.Error.String() output net/url, net/http: sensitive URLs should be able to be omitted from url.Error.String() output Mar 6, 2021
@theckman theckman changed the title net/url, net/http: sensitive URLs should be able to be omitted from url.Error.String() output proposal: net/http.Client should allow omitting URLs from error strings Mar 7, 2021
@gopherbot gopherbot added this to the Proposal milestone Mar 7, 2021
@theckman
Copy link
Contributor Author

@theckman theckman commented Mar 7, 2021

@ianlancetaylor alrighty, I've reformatted the issue to be more proposal-like. If this format doesn't fit the bill give me a shout.

@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Mar 8, 2021
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 8, 2021

@theckman
Copy link
Contributor Author

@theckman theckman commented Mar 8, 2021

One thing I considered when writing my proposal was to not have the bool on the net/url.Error be exported, and to instead move the error creation to a constructor function so that net/http could still specify it. That felt like a much larger change that I had a hard time justifying, but I could see it as being a way to avoid exposing what is effectively meant to be an internal API.

@neild
Copy link
Contributor

@neild neild commented Mar 8, 2021

This seems reasonable to me.

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Mar 9, 2021

You can do something like this to redact the URLs without requiring any changes to net/http (playground link - won't run on the playground because of #44822):

func RedactErrorURL(err error) {
	var ue *url.Error
	if errors.As(err, &ue) {
		ue.URL = "<redacted>"
	}
}

That works even when you don't have control over the Client struct (which is pretty common in my experience) and can be used wherever is appropriate. You could use it directly after a http request, or after a wrapper function is done, or all the way down at where the error is actually being logged.

@theckman
Copy link
Contributor Author

@theckman theckman commented Mar 9, 2021

@tmthrgd you are correct that is the solution you can pursue today. In the context section I mention the challenges of that approach:

The first is that you’d run the risk of forgetting to add that protection somewhere, or a code change / addition in the future could omit it. In addition if you’re mutating the error value to remove the URL, you’ve made it so that callers cannot inspect that detail of the error if they wanted to use it to decide to handle the error in a specific way.

@tmthrgd
Copy link
Contributor

@tmthrgd tmthrgd commented Mar 9, 2021

@tmthrgd you are correct that is the solution you can pursue today. In the context section I mention the challenges of that approach:

The first is that you’d run the risk of forgetting to add that protection somewhere, or a code change / addition in the future could omit it. In addition if you’re mutating the error value to remove the URL, you’ve made it so that callers cannot inspect that detail of the error if they wanted to use it to decide to handle the error in a specific way.

But surely that’s no different at all to how easy it would be to forget to set the OnErrorOmitURL field? Also you have to be aware of any calls to http.Get or similar, any code using a custom http.Client that you might have missed or not easily control, and any code that performs http requests as part of a library.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 12, 2021

What about net/http.Client.MutateErr func(error) error? So the user can specify the policy and we don't need a dozen policy bools in the future for all the possible variants people might want? (keep domain, keep URL path, only redact auth, only redact params, only redact certain params, ....)

Notably, http.Client is already a bag of policy funcs/interfaces, so it's somewhat fitting.

@theckman
Copy link
Contributor Author

@theckman theckman commented Mar 12, 2021

@bradfitz Considering the use case of wanting to retain the URL in the net/url.Error struct so that consumers can inspect it, but not have the URL be printed out when the .Error() method is invoked, I'm having a hard time visualizing how that would work with your suggestion. Or are you suggesting that we don't support that, and make the errors unusable by the caller?

@theckman
Copy link
Contributor Author

@theckman theckman commented Mar 12, 2021

I guess a second question comes to mind, would we want that function to take an error or a *url.Error? If we made it an error, we'd probably need to be real careful about how we change error handling inside of the net/http package going forward since folks would need to use type assertions to sanitize their URLs. Discoverability of all potential error types would likely need to be documented.

@theckman
Copy link
Contributor Author

@theckman theckman commented May 19, 2021

@bradfitz wanted to see if you'd had any spare cycles to noodle on the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants