fix: avoid nil pointer error when debugging requests with no body #176
Conversation
| h.Set("X-Auth-Token", "**REDACTED**") | ||
| authToken, hasAuth := request.Header["X-Auth-Token"] | ||
| if hasAuth { | ||
| request.Header.Set("X-Auth-Token", "**REDACTED**") |
There was a problem hiding this comment.
I believe this change can create race conditions when one request object is being used to make multiple API requests, as in a go func loop, especially where execution timing could impact the request. Some requests may be submitted with the literal **REDACTED** token.
There was a problem hiding this comment.
Here's a ChatGPT generated example where the race is triggered in most cases (an actual remote request should reduce the occurrence)
https://gist.github.com/displague/2bda2d2176e255da968478455d0da96d
There was a problem hiding this comment.
By redacting a debug-only copy, we previously avoided that case.
We could test the Request ContentLength or nil'ness of the Body to avoid triggering the nil pointer exception.
There was a problem hiding this comment.
This SDK makes a single request per request object, and that can't be changed without significant refactoring to the templates: https://github.com/equinix-labs/metal-go/blob/main/templates/api.mustache#L355-L360
There was a problem hiding this comment.
I'm mostly concerned from a best practice perspective; I agree the likelihood of hitting the race would be near 0.
There was a problem hiding this comment.
I'm not clear if using a debug-only copy is a best practice; it seems more like a judgement call. For example, go-containerregistry also redacts the header in-place and then restores it: https://github.com/google/go-containerregistry/blob/5a53a12f09d0868a601d5e67838d39fe95e6b441/pkg/v1/remote/transport/logger.go#L48-L62
Another alternative I considered is redacting the dump directly, something like this: https://git.sr.ht/~jamesponddotco/xstd-go/commit/22489c0e7382e7bb0079a4489513386aad0d5601
Further, it's possible reusing a request is, in itself, not a best practice based on the caveats in this comment: golang/go#19653 (comment)
|
Can confirm this change avoids the nil pointer error I was getting in CAPP. |
|
This PR is included in version 0.28.0 🎉 |
@cprivitere reported that cluster-api-provider-packet experiences nil pointer errors when debugging is enabled and a request has no body. This behavior seems to have started in v0.25.1, which introduced automatic auth token redaction in debug output.
In order to redact the auth token without impacting the original request, we were cloning the request object. In order to clone the request body, we had to call
request.GetBody(), but that function causes a nil pointer error if the request body is nil.This updates the auth token redaction code to temporarily overwrite the auth token in the original request, rather than cloning the request & its potentially-nil body.