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

net/gclient: fix parameters containing equal signs were tampered #3643

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fghwett
Copy link

@fghwett fghwett commented Jun 14, 2024

Fixed the issue where parameters containing equal signs were tampered with when there were file parameters in the same form.

Example:

gclient.New().Post(context.Background(), "https://example.com", fmt.Sprintf("source=https://exmaple.com/rand?type=girls&image=@file:filename.png"))

// gclient_request.go:233 this will cause the parameters to be divided into three sections. [source, https://exmaple.com/rand?type, girls] 
for _, item := range strings.Split(params, "&") {
	array := strings.Split(item, "=")
	...
}

//  gclient_request.go:232 this will cause the third and subsequent parameters to be lost.
var (
	fieldName  = array[0]
	fieldValue = array[1]
)
if err = writer.WriteField(fieldName, fieldValue); err != nil {
	err = gerror.Wrapf(err, `write form field failed with "%s", "%s"`, fieldName, fieldValue)
	return nil, err
}

… with when there were file parameters in the same form.
@gqcn
Copy link
Member

gqcn commented Jun 18, 2024

@fghwett Good enhancement! Would you please add associated unit testing case for this update?

@gqcn gqcn added awesome It's awesome! We keep watching. missing unit testing cases Missing unit testing cases for this PR. labels Jun 18, 2024
…x parameters were tampered with in the form request parameters.
@fghwett
Copy link
Author

fghwett commented Jun 20, 2024

When I was writing the test case, I found a problem with more than just containing equal signs. After looking at the code, I decided it was the '@file:' parameter and noUrlEncode, so I made some adjustments.
Now, as long as the @file: parameter is included, it will be encoded and decoded in the corresponding processing place. This is the processing scheme I have come up with at present, I don't know whether it can be accepted.

@gqcn gqcn added wip and removed missing unit testing cases Missing unit testing cases for this PR. labels Jun 25, 2024
@gqcn
Copy link
Member

gqcn commented Jun 25, 2024

Fixed the issue where parameters containing equal signs were tampered with when there were file parameters in the same form.

Example:

gclient.New().Post(context.Background(), "https://example.com", fmt.Sprintf("source=https://exmaple.com/rand?type=girls&image=@file:filename.png"))

// gclient_request.go:233 this will cause the parameters to be divided into three sections. [source, https://exmaple.com/rand?type, girls] 
for _, item := range strings.Split(params, "&") {
	array := strings.Split(item, "=")
	...
}

//  gclient_request.go:232 this will cause the third and subsequent parameters to be lost.
var (
	fieldName  = array[0]
	fieldValue = array[1]
)
if err = writer.WriteField(fieldName, fieldValue); err != nil {
	err = gerror.Wrapf(err, `write form field failed with "%s", "%s"`, fieldName, fieldValue)
	return nil, err
}

@fghwett or you can urlencode your parameters? like:

g.Client().Post(
    context.Background(), 
    "https://example.com", 
    "source=https%3A%2F%2Fexmaple.com%2Frand%3Ftype%3Dgirls%26image%3D%40file%3Afilename.png",
)

@fghwett
Copy link
Author

fghwett commented Jun 26, 2024

Fixed the issue where parameters containing equal signs were tampered with when there were file parameters in the same form.
Example:

gclient.New().Post(context.Background(), "https://example.com", fmt.Sprintf("source=https://exmaple.com/rand?type=girls&image=@file:filename.png"))

// gclient_request.go:233 this will cause the parameters to be divided into three sections. [source, https://exmaple.com/rand?type, girls] 
for _, item := range strings.Split(params, "&") {
	array := strings.Split(item, "=")
	...
}

//  gclient_request.go:232 this will cause the third and subsequent parameters to be lost.
var (
	fieldName  = array[0]
	fieldValue = array[1]
)
if err = writer.WriteField(fieldName, fieldValue); err != nil {
	err = gerror.Wrapf(err, `write form field failed with "%s", "%s"`, fieldName, fieldValue)
	return nil, err
}

@fghwett or you can urlencode your parameters? like:

g.Client().Post(
    context.Background(), 
    "https://example.com", 
    "source=https%3A%2F%2Fexmaple.com%2Frand%3Ftype%3Dgirls%26image%3D%40file%3Afilename.png",
)

I tried the following two ways and got wrong results.

# Only the parameters other than the file are encoded, and the backend will receive the encoded URL. However, in terms of business logic, the backend does not perform URL de-encoding processing.
resp, err := gclient.New().
	Post(context.Background(), "https://exmaple.com",
	fmt.Sprintf("source=%s&image=@file:%s", gurl.Encode("https://exmaple.com/rand?type=girls"), "filepath.jpg"),
)

# Encode all parameters, gclient will think they are uploaded in json format.
resp, err := gclient.New().
	Post(context.Background(), "https://example.com",
	gurl.Encode("source=https://exmaple.com/rand?type=girls&image=@file:filename.jpg"),
)

In the business logic, I need to upload a photo along with the original URL of the photo.

@gqcn
Copy link
Member

gqcn commented Jun 26, 2024

@fghwett What you expect is the server side receiving the normal source parameter (https://exmaple.com/rand?type=girls) and the uploaded file, or the complete source parameter containing file parameter(https://exmaple.com/rand?type=girls&image=@file:filename.png)?

@fghwett
Copy link
Author

fghwett commented Jun 26, 2024

@fghwett What you expect is the server side receiving the normal source parameter (https://exmaple.com/rand?type=girls) and the uploaded file, or the complete source parameter containing file parameter(https://exmaple.com/rand?type=girls&image=@file:filename.png)?

I'm the first case.

@gqcn
Copy link
Member

gqcn commented Jul 4, 2024

I'm still handling on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awesome It's awesome! We keep watching. wip
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants