Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix issue #5371 to correctly handle Request IP Addresses #5372

Merged
merged 4 commits into from
Nov 15, 2022

Conversation

davidjrh
Copy link
Contributor

@davidjrh davidjrh commented Nov 3, 2022

Fixes issue #5371.

Summary

Added code to correctly parse the request IP Address when original IP Address comes in the request header "X-Forwarded-For" (i.e. when using an ARR like in an Azure App Service) by removing the port.

@bdukes bdukes added this to the 9.11.1 milestone Nov 3, 2022
Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Hmm, not sure if it's an issue or not but what about IPv6 addresses that use : as a separator ?

@bdukes
Copy link
Contributor

bdukes commented Nov 4, 2022

That's a great point @valadas. I think we need to try calling this.ValidateIP(userIPAddress, ipFamily) and only if it fails do we try cutting off after the colon.

@davidjrh
Copy link
Contributor Author

davidjrh commented Nov 5, 2022

Good point @valadas. I've moved the check to the this.ValidateIP(userIPAddress, ipFamily) methods as @bdukes suggested. Need to to it beforeIPAddress.TryParse(ipString, out address) because that method fails if contains an IPv4 with colons.

@davidjrh
Copy link
Contributor Author

davidjrh commented Nov 6, 2022

Finnally brought back the port removal outside the ValidateIP method since it must return the IP without the port (returning the IP with the port is not for what the method is currently used)

@valadas
Copy link
Contributor

valadas commented Nov 7, 2022

That looks better, it only leaves the possibility of an IPv6 with a port which would look like
http://[1fff:0:a88:85a3::ac1f]:8001/index.html which is a bit more of a pickle because the [] are optional and only needed if a port is specified.
So I guess if it's IPv6 we could check if there is a ], if so we split on it and remove the first character [ or something like that.

@valadas
Copy link
Contributor

valadas commented Nov 8, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidjrh
Copy link
Contributor Author

Hi @valadas, I've added code to handle the same situation for IPv6 addresses, following your thoughts.

Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

That looks awesome

@valadas valadas merged commit 9cd7e26 into dnnsoftware:develop Nov 15, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants