go-resty / resty

Simple HTTP and REST client library for Go
MIT License
10.23k stars 715 forks source link

Encoded QueryStringParam shows !A(MISSING) #446

Closed RafPe closed 1 year ago

RafPe commented 3 years ago

Hi ,

When working with resty & querystring params I noticed that when showing debug output and having special characters in queryParamString it shows them as !A(MISSING) altough query is properly formated and sent

2021/08/20 11:53:53.437251 DEBUG RESTY 
==============================================================================
GET  /api?paramSpecial=a+aa%!A(MISSING)sss&extended=false  HTTP/1.1

Its not a show stopper - but confuses end users when debugging.

moorereason commented 3 years ago

I think this line is the problem. debugLog should be escaped before passing to Debugf.

jeevatkm commented 3 years ago

@RafPe Do you mind helping with the tiny test cases or test request data? It will be helpful to replicate at my end and addressing an issue.

RafPe commented 3 years ago

@jeevatkm would this work https://github.com/apiheat/akamai-cli-netlist/issues/30#issuecomment-901991146 ? Its part of the refferenced issue here.

In short it happens when we have special characters in query string.

jeevatkm commented 3 years ago

@RafPe I have read the issue you have referred. It seems this param accountSwitchKey has a special character. Actually, I was trying to find out those special characters as a sample.

Right here Request URI is grabbed and composed for debug log https://github.com/go-resty/resty/blob/79cc4d44d50815b89104fec0cc8f363e7e8c7586/middleware.go#L277

I think applying Escape for the overall debug log might have side effects. So it's better to address at the URI processing end.

can you help to find out those special characters as best you can? So that it will be helpful.

jeevatkm commented 3 years ago

@RafPe Also after reading your referred issue. I think param accountSwitchKey value had a character set something like %A and then package fmt was unable to recognize, so it logged as %!A(MISSING). https://pkg.go.dev/fmt#hdr-Format_errors

fmt.Printf("%s hello %A", "welcome")

// output
// welcome hello %!A(MISSING)

fmt.Printf("accountSwitchKey=F-AC-XXXXXXX%AY-YYYY\n")

// output
// accountSwitchKey=F-AC-XXXXXXX%!A(MISSING)Y-YYYY

So, if apply the url.QueryEscape method, the value would be

F-AC-XXXXXXX%25AY-YYYY

Personally, I feel Resty will get confused more because the query string has an incorrect value that gets logged.

jeevatkm commented 3 years ago

@moorereason your thoughts

RafPe commented 3 years ago

@jeevatkm The param query string accountSwitchKey in deed was the one causing problem. The situation was caused by using the following characters F-AC-XXXXXXX:Y-YYYY

In my opinion the debug URI should reflect what is being actually sent to the end server as request ( imho )

jeevatkm commented 3 years ago

@RafPe Thanks for the info. Do you mean : caused this? I will look further at my end too.

RafPe commented 3 years ago

@jeevatkm I believe it did. Based on my tests there thats what I got

GET /network-list/v2/network-lists?accountSwitchKey=F-AC-XXXXXXX%!A(MISSING)Y-YYYY

moorereason commented 3 years ago

Somewhere along the way, the : is probably being uri-encoded as %3A. This would give the %!A(MISSING) message seen.

However, I'm unable to reproduce this issue with a simple test:

package main

import "github.com/go-resty/resty/v2"

func main() {
        _, err := resty.New().SetDebug(true).R().Get("http://httpbingo.org/get?foo=XX:YY")
        if err != nil {
                panic(err)
        }
}

I don't see anywhere in resty where we're misusing a printf-style function which would cause this problem.

@RafPe, can you make sure you're using the latest resty release? How exactly is your project using resty?

RafPe commented 3 years ago

@moorereason Resty is being used as http client within my apis base client which then in response is used in specific multiple service implementations.

The behaviour happens when I use the functionality of setting query string param - so not directly via URL.

For the current version I will have to check - but I'm sure I was updating it locally while debugging the referenced issue

moorereason commented 3 years ago

It looks to me like you're using v2.0.0. v2.0.0 had a misuse of log.Debugf, which was fixed and shipped with v2.3.0.

RafPe commented 3 years ago

@moorereason I will double check that tomorrow and confirm to you what is the precise version I get this behaviour

jeevatkm commented 3 years ago

@RafPe Yeah, it seems upstream lib go-edgegrid using Resty v2.0.0 https://github.com/apiheat/go-edgegrid/blob/4fd7bca6dc93d2e9376a44291f6a2a14a06f4b2c/go.mod#L8

Can you try the latest version and share feedback?

jeevatkm commented 1 year ago

Upgrading to Resty v2.3.0 or later would be appropriate to overcome this debug log issue.