Replace godebug with go-cmp#598
Conversation
| e.Resp.Request = nil // This brings in a bunch of TLS crap we don't need | ||
| e.Resp.TLS = nil // Same | ||
| return fmt.Sprintf("%s:\nRequest:\n%s\nResponse:\n%s", e.Err, prettyConf.Sprint(e.Req), prettyConf.Sprint(e.Resp)) | ||
| return fmt.Sprintf("%s:\nRequest:\n%s\nResponse:\n%s", e.Err, dumpRequest(e.Req), dumpResponse(e.Resp)) |
There was a problem hiding this comment.
This part needs some cleanup logic. Something like:
For dumpRequest:
- Log the URL host and path only (no query string values)
- Log HTTP method
- Log header names only (not values — Authorization header leaks tokens)
- Log GET/POST parameter names only (not values — body leaks secrets/assertions)
For dumpResponse:
- On error responses (non-2xx): log field names + values (safe, it's just an error description)
- On success responses: log field names only, never values
There was a problem hiding this comment.
I made the output more similar to how it worked before with godebug/pretty. I do believe the prior version with godebug/pretty could leak header tokens and secrets since there was no processing of those values, but that may have been intentional since it is a verbose log. I made it a bit more structured so it should be easier to decide what to print. Maybe you could help with that as that seems like there are a few edge cases and more like a separate PR, since the current code can leak these tokens.
There was a problem hiding this comment.
Agreed, let's keep it simple. Print minimal information only. From what I can see, app owner can choose to print full HTTP details on their side anyway.
| "github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/oauth/ops/accesstokens" | ||
| "github.com/AzureAD/microsoft-authentication-library-for-go/apps/internal/shared" | ||
| "github.com/kylelemons/godebug/pretty" | ||
| "github.com/google/go-cmp/cmp" |
There was a problem hiding this comment.
@chlowell - would appreciate your review on this PR.
| fmt.Fprintf(&b, " URL: {Scheme: %q,\n", req.URL.Scheme) | ||
| fmt.Fprintf(&b, " Host: %q,\n", req.URL.Host) | ||
| fmt.Fprintf(&b, " Path: %q,\n", req.URL.Path) | ||
| fmt.Fprintf(&b, " RawQuery: %q},\n", req.URL.RawQuery) |
There was a problem hiding this comment.
The rawQuery should not be printed. It may contain login hint, i.e. user emails.
| fmt.Fprintf(&b, " Path: %q,\n", req.URL.Path) | ||
| fmt.Fprintf(&b, " RawQuery: %q},\n", req.URL.RawQuery) | ||
| fmt.Fprintf(&b, " Proto: %q,\n", req.Proto) | ||
| fmt.Fprintf(&b, " ProtoMajor: %d,\n", req.ProtoMajor) |
There was a problem hiding this comment.
Yes! godebug/pretty would print it, so I added that to the prints. But now it will just print Proto.
| return b.String() | ||
| } | ||
|
|
||
| func formatHeaders(header http.Header, indent string) string { |
There was a problem hiding this comment.
We should not print header values here. It's ok to not print headers at all. We can decide to print specific headers in other layers of the library.
There was a problem hiding this comment.
Removed printing headers.
| fmt.Fprintf(&b, " Proto: %q,\n", resp.Proto) | ||
| fmt.Fprintf(&b, " ProtoMajor: %d,\n", resp.ProtoMajor) | ||
| fmt.Fprintf(&b, " ProtoMinor: %d,\n", resp.ProtoMinor) | ||
| fmt.Fprintf(&b, " Header: %s,\n", formatHeaders(resp.Header, " ")) |
There was a problem hiding this comment.
As above, let's not print headers.
| fmt.Fprintf(&b, " ProtoMajor: %d,\n", resp.ProtoMajor) | ||
| fmt.Fprintf(&b, " ProtoMinor: %d,\n", resp.ProtoMinor) | ||
| fmt.Fprintf(&b, " Header: %s,\n", formatHeaders(resp.Header, " ")) | ||
| if bodyStr == "" { |
There was a problem hiding this comment.
body can contain tokens, better not print it at all.
|
chlowell
left a comment
chlowell
left a comment
There was a problem hiding this comment.
Looks good overall, the blocker for me is the nil URL guard. Version upgrade policy is up to @bgavrilMS
| module github.com/AzureAD/microsoft-authentication-library-for-go | ||
|
|
||
| go 1.18 | ||
| go 1.21 |
There was a problem hiding this comment.
I doubt this will cause problems because Google stopped supporting 1.20 a couple years ago and upgrading to 1.21 should be easy, however I think it's important to note this is raising the library's minimum required version to enable a test change having no functional impact on the tests or customer applications. Telemetry, if there is any, would help evaluate this
There was a problem hiding this comment.
We don't have telemetry on Go.
| fmt.Fprintf(&b, " URL: {Scheme: %q,\n", req.URL.Scheme) | ||
| fmt.Fprintf(&b, " Host: %q,\n", req.URL.Host) | ||
| fmt.Fprintf(&b, " Path: %q},\n", req.URL.Path) |
There was a problem hiding this comment.
We should guard against req.URL == nil, if only to satisfy static analysis and LLMs
3 participants
This PR refactors the codebase to replace the use of the
github.com/kylelemons/godebugpackage withgithub.com/google/go-cmppackage. Godebug is no longer maintained and has not been updated in 6 years and go-cmp is actively maintained. This mainly just replacespretty.Comparewithcmp.Diff. And the errors.Verbose() function now uses the httputil library to dump the requests and responses.