mike-jumper commented on a change in pull request #398: Modify Regex for
X-Forwarded-for to parse IP:Port
URL: https://github.com/apache/guacamole-client/pull/398#discussion_r279010990
##########
File path:
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java
##########
@@ -93,12 +93,12 @@
/**
* Regular expression which matches any IPv4 address.
*/
- private static final String IPV4_ADDRESS_REGEX =
"([0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3})";
+ private static final String IPV4_ADDRESS_REGEX =
"([0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3}\\.[0-9]{1,3})(?::[0-9]{1,4})?";
/**
* Regular expression which matches any IPv6 address.
*/
- private static final String IPV6_ADDRESS_REGEX =
"([0-9a-fA-F]*(:[0-9a-fA-F]*){0,7})";
+ private static final String IPV6_ADDRESS_REGEX =
"([0-9a-fA-F]*(:[0-9a-fA-F]*){0,7})(?::[0-9]{1,4})?";
Review comment:
Both `IPV4_ADDRESS_REGEX` and `IPV6_ADDRESS_REGEX` are documented here as
matching IP addresses. Altering them such that they also accept port numbers
will mean that the documentation becomes incorrect. If the change proposed here
is correct, then that documentation needs to be updated to match. However:
* Duplicating the same pattern across both `IPV4_ADDRESS_REGEX` and
`IPV6_ADDRESS_REGEX` is suboptimal. There are other patterns which would be
better homes for this change and avoid duplication, but again: modifying
something that is essentially named "IP address" and documented as matching IP
addresses such that it also matches port numbers isn't complete in itself. That
change would need to be followed through such that the documentation and naming
are correct.
* The de facto `X-Forwarded-For` header is defined as a list of IP
addresses, not a list of IP addresses with optional port numbers:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For
https://en.wikipedia.org/wiki/X-Forwarded-For
If there are real world cases where a port number is included, please
provide some background information when you open the corresponding issue in
JIRA so the reasoning for your proposed change can be understood.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
