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

Bugfix/1834 Fix X-Real-IP bug #2007

Merged
merged 5 commits into from Mar 1, 2022

Conversation

yeyisan
Copy link
Contributor

@yeyisan yeyisan commented Oct 10, 2021

This PR Fixes #1834

  • The ExtractIPFromRealIPHeader function returns realIP value only when realIP is not empty and is a valid and trusted IP.
  • Otherwise returns the directIP value.
  • Test functions now have a ipForInternalRealIP value for comparing internal XRealIP test cases.

See #1834/comment

@yeyisan yeyisan changed the title Bugfix/1834 fix x real ip bug Bugfix/1834 Fix X-Real-IP bug Oct 10, 2021
@codecov
Copy link

codecov bot commented Oct 15, 2021

Codecov Report

Merging #2007 (09e518a) into master (27b404b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2007      +/-   ##
==========================================
- Coverage   92.21%   92.21%   -0.01%     
==========================================
  Files          37       37              
  Lines        3019     3018       -1     
==========================================
- Hits         2784     2783       -1     
  Misses        148      148              
  Partials       87       87              
Impacted Files Coverage Δ
echo.go 95.14% <ø> (ø)
ip.go 100.00% <100.00%> (ø)

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 27b404b...09e518a. Read the comment docs.

@lammel
Copy link
Contributor

lammel commented Dec 14, 2021

Going to look into this soon

@yeyisan
Copy link
Contributor Author

yeyisan commented Dec 14, 2021

Thank you @lammel

@aldas aldas force-pushed the bugfix/1834-fix-x-real-ip-bug branch from fb12a8f to 635adb9 Compare February 24, 2022 15:15
@aldas
Copy link
Contributor

aldas commented Feb 24, 2022

@lammel please review. I have reworked all tests to be more "readable", added comments. Fixed couple of headers to be in canonical format.

Tests are way longer now but these older tests were so unreadable with their double loops and constants. For readability using text and copying it here and there is better than code that takes serious IDE line-hopping to understand what is actually used as inputs

@lammel
Copy link
Contributor

lammel commented Feb 28, 2022

@lammel please review. I have reworked all tests to be more "readable", added comments. Fixed couple of headers to be in canonical format.

Tests are way longer now but these older tests were so unreadable with their double loops and constants. For readability using text and copying it here and there is better than code that takes serious IDE line-hopping to understand what is actually used as inputs

Awesome, that was what I meant with my comments, thanks for doing the work I wanted to do @aldas !
Will take a look into it tomorrow.

Copy link
Contributor

@lammel lammel left a comment

Choose a reason for hiding this comment

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

Looks very good to me.

ip_test.go Outdated Show resolved Hide resolved
ip_test.go Outdated Show resolved Hide resolved
ip_test.go Outdated Show resolved Hide resolved
TrustLinkLocal(false),
TrustPrivateNet(false),
// this is private IPv6 ip
// CIDR Notation: 2001:0db8:0000:0000:0000:0000:0000:0000/48
Copy link
Contributor

Choose a reason for hiding this comment

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

For better readability for different tab sizes better uses spaces for aligning the values here (github also displays with tabsize of 4).
Not really important though.

Copy link
Contributor

Choose a reason for hiding this comment

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

seems that Echo .editorconfig sets indent_size = 2

ip_test.go Outdated
// Address: 8.8.8.8
// Range start: 8.8.8.0
// Range end: 8.8.8.255
givenRange: "8.8.8.8/24",
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually a trust subnet would be specified using the base address with subnet mask, in that case 8.8.8.0/24. Unless this was intentional, maybe we should change that.
(also not really important)

@aldas aldas requested a review from lammel March 1, 2022 06:06
@lammel lammel merged commit 124825e into labstack:master Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExtractIPFromRealIPHeader returns RemoteAddr instead of x-real-ip
3 participants