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

Bisstocuz · 2021-08-19T18:00:19Z

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.

Quick Fix #2814

codecov · 2021-08-20T00:37:45Z

Codecov Report

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

@@           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.

Bisstocuz · 2021-08-21T19:24:12Z

@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

menduo

Great works.

Bisstocuz · 2021-09-20T10:08:06Z

@thinkerou @appleboy code review plz

gin.go

appleboy · 2021-09-21T07:17:10Z

@Bisstocuz I gave the comment.

directly initialization

Bisstocuz · 2021-09-22T08:27:28Z

@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

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

Bisstocuz · 2021-09-27T15:33:36Z

@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:

it's not backward-compatible

is doesn't update related documents

#2692 (comment) by @leafduo

appleboy · 2021-09-28T01:11:32Z

@thinkerou need your approval.

thinkerou

lgtm

zihengCat

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

zihengCat · 2021-09-29T03:34:51Z

@Bisstocuz Could you make all CI unit tests passed?

Bisstocuz · 2021-09-29T07:52:37Z

@Bisstocuz Could you make all CI unit tests passed?

@zihengCat I'm checking it.

This seems to have something to do with #2872

Bisstocuz · 2021-09-29T08:25:38Z

@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

…ng r.Run() to run http server (#2832)

…ng r.Run() to run http server (gin-gonic#2832) (cherry picked from commit 6d75aba)

…ng r.Run() to run http server (#2832)

…ng r.Run() to run http server (gin-gonic#2832) (cherry picked from commit 6d75aba)

…ng r.Run() to run http server (#2832)

…ng r.Run() to run http server (gin-gonic#2832)

Bisstocuz added 2 commits August 20, 2021 01:37

Quick Fix #2814

27fabb1

Merge pull request #1 from Bisstocuz/Fix-#2814

bd77f6a

Quick Fix #2814

thinkerou added this to the v1.7.5 milestone Aug 20, 2021

thinkerou added break-backward enhancement labels Aug 20, 2021

thinkerou requested review from a team, thinkerou, appleboy and javierprovecho and removed request for a team August 20, 2021 02:17

thinkerou mentioned this pull request Aug 20, 2021

Make TrustedProxies feature backward-compatible #2819

Closed

ghost reviewed Aug 26, 2021

View reviewed changes

gin.go Outdated Show resolved Hide resolved

Resolve review

c2e6c7e

menduo previously approved these changes Sep 13, 2021

View reviewed changes

appleboy reviewed Sep 21, 2021

View reviewed changes

gin.go Outdated Show resolved Hide resolved

initialize trustedCIDRs directly

c7e3b72

directly initialization

Bisstocuz dismissed menduo’s stale review via c7e3b72 September 22, 2021 08:23

initialize trustedCIDRs directly

25af02a

directly initialization

Bisstocuz requested a review from appleboy September 22, 2021 09:49

ghost reviewed Sep 22, 2021

View reviewed changes

gin.go Show resolved Hide resolved

ghost previously approved these changes Sep 27, 2021

View reviewed changes

appleboy previously approved these changes Sep 28, 2021

View reviewed changes

appleboy removed the break-backward label Sep 28, 2021

thinkerou previously approved these changes Sep 28, 2021

View reviewed changes

zihengCat previously approved these changes Sep 29, 2021

View reviewed changes

Merge branch 'gin-gonic:master' into master

7f40bb7

Fixed #2871 #2872

265d06a

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

appleboy approved these changes Sep 29, 2021

View reviewed changes

appleboy merged commit 6d75aba into gin-gonic:master Sep 29, 2021

Bisstocuz mentioned this pull request Sep 29, 2021

Tidy: Complete TrustedProxies feature #2887

Merged

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 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

menduo mentioned this pull request Oct 20, 2021

Would you please make a release which fix #2697 c.ClientIP() bug in #2832 #2909

Closed

Bisstocuz mentioned this pull request Nov 19, 2021

update version to v1.7.5 #2943

Closed

thinkerou pushed a commit that referenced this pull request Nov 20, 2021

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

3783ce0

…ng r.Run() to run http server (#2832)

Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 22, 2021

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

b989a6e

…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

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

77644e5

…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

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

2c5e5c6

…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

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

3356f1c

…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

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

a408ded

…ng r.Run() to run http server (#2832)

Bisstocuz added a commit to Bisstocuz/gin that referenced this pull request Nov 23, 2021

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

33daaee

…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

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

76939ae

…ng r.Run() to run http server (#2832)

thinkerou pushed a commit that referenced this pull request Nov 24, 2021

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

7d20914

…ng r.Run() to run http server (#2832)

daheige pushed a commit to daheige/gin that referenced this pull request Apr 18, 2022

Quick Fix c.ClientIP() mistakely parsing to 127.0.0.1 for who not usi…

6d9e16e

…ng r.Run() to run http server (gin-gonic#2832)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Bisstocuz commented Aug 19, 2021 •

edited

Loading

codecov bot commented Aug 20, 2021 •

edited

Loading

Bisstocuz commented Aug 21, 2021

menduo left a comment

Bisstocuz commented Sep 20, 2021

appleboy commented Sep 21, 2021

Bisstocuz commented Sep 22, 2021 •

edited

Loading

Bisstocuz commented Sep 27, 2021

appleboy commented Sep 28, 2021

thinkerou left a comment

zihengCat left a comment

zihengCat commented Sep 29, 2021

Bisstocuz commented Sep 29, 2021 •

edited

Loading

Bisstocuz commented Sep 29, 2021

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

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

Conversation

Bisstocuz commented Aug 19, 2021 • edited Loading

codecov bot commented Aug 20, 2021 • edited Loading

Codecov Report

Bisstocuz commented Aug 21, 2021

menduo left a comment

Choose a reason for hiding this comment

Bisstocuz commented Sep 20, 2021

appleboy commented Sep 21, 2021

Bisstocuz commented Sep 22, 2021 • edited Loading

Bisstocuz commented Sep 27, 2021

appleboy commented Sep 28, 2021

thinkerou left a comment

Choose a reason for hiding this comment

zihengCat left a comment

Choose a reason for hiding this comment

zihengCat commented Sep 29, 2021

Bisstocuz commented Sep 29, 2021 • edited Loading

Bisstocuz commented Sep 29, 2021

Bisstocuz commented Aug 19, 2021 •

edited

Loading

codecov bot commented Aug 20, 2021 •

edited

Loading

Bisstocuz commented Sep 22, 2021 •

edited

Loading

Bisstocuz commented Sep 29, 2021 •

edited

Loading