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

Quick Fix c.ClientIP() Parse Error for TrustedProxies feature & NoMethod annotation #2832

Merged
merged 7 commits into from Sep 29, 2021

Conversation

Bisstocuz
Copy link
Contributor

@Bisstocuz Bisstocuz commented Aug 19, 2021

Related issues: #2791 #2697 #2814

Someone who not using r.Run() to run http server may meet this issue: c.ClientIP() always returns the same IP for all clients because of calling of prepareTrustedCIDRs() of TrustedProxies feature.
For example, c.ClientIP() always returns 127.0.0.1 while using local reverse proxy.

In New(): there is one definition: TrustedProxies: []string{"0.0.0.0/0"}
We can see this variable has default value, but trustedCIDRs doesn't have.

So we just need to give a default value to trustedCIDRs.

@codecov
Copy link

codecov bot commented Aug 20, 2021

Codecov Report

Merging #2832 (265d06a) into master (ef16867) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2832   +/-   ##
=======================================
  Coverage   98.75%   98.75%           
=======================================
  Files          41       41           
  Lines        3062     3063    +1     
=======================================
+ Hits         3024     3025    +1     
  Misses         26       26           
  Partials       12       12           
Flag Coverage Δ
go-1.13 98.75% <100.00%> (+<0.01%) ⬆️
go-1.14 98.59% <100.00%> (+<0.01%) ⬆️
go-1.15 98.59% <100.00%> (+0.02%) ⬆️
go-1.16 98.59% <100.00%> (+0.01%) ⬆️
go-1.17 98.49% <100.00%> (+<0.01%) ⬆️
macos-latest 98.75% <100.00%> (+<0.01%) ⬆️
nomsgpack 98.74% <100.00%> (+<0.01%) ⬆️
ubuntu-latest 98.75% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
context.go 97.98% <ø> (ø)
gin.go 99.10% <100.00%> (+<0.01%) ⬆️

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 97b3c0d...265d06a. Read the comment docs.

@thinkerou thinkerou added this to the v1.7.5 milestone Aug 20, 2021
@thinkerou thinkerou requested review from a team, thinkerou, appleboy and javierprovecho and removed request for a team August 20, 2021 02:17
@Bisstocuz
Copy link
Contributor Author

@menduo @zihengCat Please leave your review here to help fix this as soon as possible.

This change will parse TrustedProxies and trust all proxies by default.

gin.go Outdated Show resolved Hide resolved
menduo
menduo previously approved these changes Sep 13, 2021
Copy link

@menduo menduo left a comment

Choose a reason for hiding this comment

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

Great works.

@Bisstocuz
Copy link
Contributor Author

@thinkerou @appleboy code review plz

gin.go Outdated Show resolved Hide resolved
@appleboy
Copy link
Member

@Bisstocuz I gave the comment.

directly initialization
@Bisstocuz
Copy link
Contributor Author

Bisstocuz commented Sep 22, 2021

@junikaii @menduo I think that directly giving it a absolute default value is more consistent.

@xpunch mentioned this in #2692 :

Your code is used to set custom TrustedProxies when don't use gin.Run.
I think the simplest way is set default value for trustedCIDRs.

func New() *Engine {
	...
        engine.trustedCIDRs = []*net.IPNet{{IP: net.IP{0x0, 0x0, 0x0, 0x0}, Mask: net.IPMask{0x0, 0x0, 0x0, 0x0}}}
        ...
}

Default value of trustedCIDRs is missing when gin.New() called, TrustedProxies is set to []string{"0.0.0.0/0"}, which means trustedCIDRs should also set a default value matchs TrustedProxies.

This value means trust all IPs by default.

directly initialization
gin.go Show resolved Hide resolved
sveatlo added a commit to moderntv/cadre that referenced this pull request Sep 24, 2021
gin.ListenAndServe doesn't support trusted proxies and ClientIP doesn't work
bump gin down to 1.6.2 to mitigate temporarily
will update gin once gin-gonic/gin#2832 is merged
ghost
ghost previously approved these changes Sep 27, 2021
@Bisstocuz
Copy link
Contributor Author

@appleboy Hi, code review plz.

After this PR merged, I will do some works and create a new PR to perfect TrustedProxies feature according to this:

This fix has 2 problems:

  1. it's not backward-compatible
  2. is doesn't update related documents

#2692 (comment) by @leafduo

appleboy
appleboy previously approved these changes Sep 28, 2021
@appleboy
Copy link
Member

@thinkerou need your approval.

thinkerou
thinkerou previously approved these changes Sep 28, 2021
Copy link
Member

@thinkerou thinkerou left a comment

Choose a reason for hiding this comment

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

lgtm

zihengCat
zihengCat previously approved these changes Sep 29, 2021
Copy link
Contributor

@zihengCat zihengCat left a comment

Choose a reason for hiding this comment

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

Nice solution, make trustedCIDRs pass all IP addresses by changing its default value.

@zihengCat
Copy link
Contributor

@Bisstocuz Could you make all CI unit tests passed?

@Bisstocuz
Copy link
Contributor Author

Bisstocuz commented Sep 29, 2021

@Bisstocuz Could you make all CI unit tests passed?

@zihengCat I'm checking it.

This seems to have something to do with #2872

@Bisstocuz Bisstocuz dismissed stale reviews from zihengCat, thinkerou, appleboy, and ghost via 265d06a September 29, 2021 08:19
@Bisstocuz
Copy link
Contributor Author

@appleboy @zihengCat @thinkerou @junikaii
Hi, code review plz again, #2872 broke unit tests, and I fixed it now.

Test result: https://github.com/Bisstocuz/gin/actions/runs/1285966832

@appleboy appleboy merged commit 6d75aba into gin-gonic:master Sep 29, 2021
24 checks passed
@Bisstocuz Bisstocuz changed the title Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not using r.Run() to run http server Quick Fix c.ClientIP() Parse Error for TrustedProxies feature & NoMethod docs Oct 9, 2021
@Bisstocuz Bisstocuz changed the title Quick Fix c.ClientIP() Parse Error for TrustedProxies feature & NoMethod docs Quick Fix c.ClientIP() Parse Error for TrustedProxies feature & NoMethod annotation Oct 9, 2021
thinkerou pushed a commit that referenced this pull request Nov 20, 2021
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
…ng r.Run() to run http server (gin-gonic#2832)

(cherry picked from commit 6d75aba)
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
…ng r.Run() to run http server (gin-gonic#2832)

(cherry picked from commit 6d75aba)
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
…ng r.Run() to run http server (gin-gonic#2832)

(cherry picked from commit 6d75aba)
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021
…ng r.Run() to run http server (gin-gonic#2832)

(cherry picked from commit 6d75aba)
thinkerou pushed a commit that referenced this pull request Nov 23, 2021
Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021
…ng r.Run() to run http server (gin-gonic#2832)

(cherry picked from commit 6d75aba)
thinkerou pushed a commit that referenced this pull request Nov 24, 2021
thinkerou pushed a commit that referenced this pull request Nov 24, 2021
daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022
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

5 participants