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.

Sign up for GitHub

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

Jump to bottom

Fix redirect request body mismatch with origin request body caused by buffer reused by mistake #568

Merged
merged 3 commits into from Mar 8, 2023
Merged

Fix redirect request body mismatch with origin request body caused by buffer reused by mistake #568

merged 3 commits into from Mar 8, 2023

Conversation

liguangbo
Copy link
Contributor

@liguangbo liguangbo commented Jul 26, 2022 •
edited by jeevatkm

As issue #567 described.

Copy the body buffer by buffer.Write to fix body mismatch with origin request when 307 or 308 redirect.

closes #567

@liguangbo liguangbo changed the title Fix redirect body reuse buffer by mistake Fix redirect request body mismatch origin request body caused by buffer reused by mistake Jul 26, 2022
@liguangbo liguangbo changed the title Fix redirect request body mismatch origin request body caused by buffer reused by mistake Fix redirect request body mismatch with origin request body caused by buffer reused by mistake Jul 26, 2022
@codecov
Copy link

codecov bot commented Jul 26, 2022 •
edited

Codecov Report

Merging #568 (2e0dbde) into master (0451c4c) will decrease coverage by 0.14%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #568      +/-   ##
==========================================
- Coverage   95.94%   95.80%   -0.14%     
==========================================
  Files          10       10              
  Lines        1357     1360       +3     
==========================================
+ Hits         1302     1303       +1     
- Misses         34       35       +1     
- Partials       21       22       +1
Impacted Files Coverage Δ
middleware.go 90.64% <50.00%> (-0.63%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@liguangbo
Copy link
Contributor Author

Anyone can help？
It's a serious problem in some cases, such as user A POST user B's data to server, and lost user A's data.

@jeevatkm

@thalesfsp
Copy link

@jeevatkm Sept 22

segevda
segevda suggested changes Sep 29, 2022
View reviewed changes
Copy link
Contributor

@segevda segevda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice catch!

client_test.go Outdated Show resolved Hide resolved
resty_test.go Outdated Show resolved Hide resolved
segevda
segevda reviewed Oct 1, 2022
View reviewed changes
middleware.go Outdated Show resolved Hide resolved
@liguangbo liguangbo force-pushed the fix/redirect-body-reuse-buffer-by-mistake branch from b996aaa to 4a0f1a3 Compare October 8, 2022 05:38
@liguangbo liguangbo requested a review from segevda October 8, 2022 05:41
@liguangbo
Copy link
Contributor Author

@segevda thanks for the review

segevda
segevda previously approved these changes Oct 31, 2022
View reviewed changes
@bbrodriges
Copy link

@segevda will this fix be merged? This bug affects our production code.

@segevda
Copy link
Contributor

segevda commented Mar 1, 2023

@bbrodriges unfortunately I don't have merge capabilities for this repo. Maybe @jeevatkm or @moorereason can take a look..?

jeevatkm
jeevatkm requested changes Mar 6, 2023
View reviewed changes
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liguangbo Thanks for you PR and contribution. I'm sorry for the delayed attention.
Can please resolve the review comment?

middleware.go Outdated Show resolved Hide resolved
@jeevatkm jeevatkm force-pushed the fix/redirect-body-reuse-buffer-by-mistake branch from 9c1deaf to 2e0dbde Compare March 8, 2023 04:28
jeevatkm
jeevatkm approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@jeevatkm jeevatkm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @liguangbo, I have updated the line.

@jeevatkm jeevatkm merged commit 5ebbbb1 into go-resty:master Mar 8, 2023
@jeevatkm jeevatkm added this to the v2.8.0 Milestone milestone Mar 8, 2023
@liguangbo
Copy link
Contributor Author

@jeevatkm Sorry for didn't see the notification. Happy to contribute to resty.

@liguangbo liguangbo deleted the fix/redirect-body-reuse-buffer-by-mistake branch March 16, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeevatkm jeevatkm jeevatkm approved these changes

@segevda segevda segevda left review comments

Assignees

@liguangbo liguangbo

Labels
bug enhancement
Milestone
v2.8.0 Milestone
Development

Successfully merging this pull request may close these issues.

Bug report: Request.bodyBuf resused by mistake
5 participants
@liguangbo @thalesfsp @bbrodriges @segevda @jeevatkm