[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #398: Modify Regex for X-Forwarded-for to parse IP:Port

[GitBox <gi...@apache.org>]

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 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:
users@infra.apache.org


With regards,
Apache Git Services