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

Fix intercepting headers in middlewares #1271

Merged
merged 2 commits into from May 28, 2022
Merged

Conversation

tjamet
Copy link
Contributor

@tjamet tjamet commented Mar 7, 2018

As explained in the TestInterceptedHeader test, in case a middleware
filters out the headers, this middleware can be done inefficient in case
one following handler is using c.String or other methods writing to the
response body directly.

This commit fixes the issue by using c.Writer when writing the Status as
done in other c.Header, c.SetCookie and other response writers.

The bug has been originally discovered using
https://github.com/gin-contrib/gzip where a failing test has been added
here: https://github.com/tjamet/gzip/blob/header/gzip_test.go#L71

Signed-off-by: Thibault Jamet tjamet@users.noreply.github.com

@codecov
Copy link

codecov bot commented Mar 7, 2018

Codecov Report

Merging #1271 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1271   +/-   ##
=======================================
  Coverage   98.62%   98.62%           
=======================================
  Files          41       41           
  Lines        2112     2112           
=======================================
  Hits         2083     2083           
  Misses         18       18           
  Partials       11       11
Impacted Files Coverage Δ
context.go 98.37% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 805b2d4...520df29. Read the comment docs.

@tjamet
Copy link
Contributor Author

tjamet commented Mar 29, 2018

@manucorporat having a look at the history, you seems to have the most knowledge on context.go, may I ask you for help reviewing this change?
I am experiencing difficulties using the gzipping middleware together with the context Status helper, gzip fails to filter out the Content-Length header

@tjamet
Copy link
Contributor Author

tjamet commented May 23, 2018

@appleboy @thinkerou you both seem to be active lately, may I ask your help reviewing this change?

@thinkerou thinkerou added this to the 1.4 milestone Feb 28, 2019
@thinkerou thinkerou modified the milestones: 1.4, 1.5 Mar 1, 2019
tjamet added 2 commits Mar 4, 2019
As explained in the TestInterceptedHeader test, in case a middleware
filters out the headers, this middleware can be done inefficient in case
one following handler is using c.String or other methods writing to the
response body directly.

This commit fixes the issue by using c.Writer when writing the Status as
done in other c.Header, c.SetCookie and other response writers.

The bug has been originally discovered using
https://github.com/gin-contrib/gzip where a failing test has been added
here: https://github.com/tjamet/gzip/blob/header/gzip_test.go#L71

Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>
Signed-off-by: Thibault Jamet <tjamet@users.noreply.github.com>
@thinkerou thinkerou modified the milestones: 1.5, 1.6 Oct 31, 2019
@thinkerou thinkerou modified the milestones: 1.6, 1.7 Feb 28, 2020
@thinkerou thinkerou modified the milestones: 1.7, v1.8 Oct 27, 2020
@boindil
Copy link

boindil commented May 23, 2022

is there a reason for this not being closed?

the adjusted line

gin/context.go

Line 828 in f1e9428

c.Writer.WriteHeader(code)
was already changed in #1606

@thinkerou thinkerou merged commit aa60021 into gin-gonic:master May 28, 2022
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants