Enhance IP validation #256

smaye81 · 2025-04-01T16:02:17Z

This beefs up the validation for the following:

IP address (v4 and v6)
IP prefix (v4 and v6)
Hostname / Port

smaye81 · 2025-04-01T18:45:10Z

@pkwarren reverted back to JDK 8.

pkwarren · 2025-04-02T14:55:13Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+        return !portRequired && isIP(str.substring(1, end), 6);
+      } else if (endPlus == splitIdx) { // port
+        return isIP(str.substring(1, end), 6) && isPort(str.substring(splitIdx + 1));
+      } else { // malformed


nit: General code style but would prefer we remove the else here and just return as before.

pkwarren · 2025-04-02T14:56:59Z

src/main/java/build/buf/protovalidate/CustomOverload.java

-  private static boolean validatePort(String value) {
+  // Returns true if the string is a valid port for isHostAndPort.
+  private static boolean isPort(String str) {
+    if (str.length() == 0) {


Suggested change

if (str.length() == 0) {

if (str.isEmpty()) {

pkwarren · 2025-04-02T14:57:58Z

src/main/java/build/buf/protovalidate/CustomOverload.java

-      int portNum = Integer.parseInt(value);
-      return portNum >= 0 && portNum <= 65535;
+      int val = Integer.parseInt(str);
+


nit: Can remove the extra space here and below before the return.

pkwarren · 2025-04-02T14:58:07Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+      return val <= 65535;
+
    } catch (NumberFormatException nfe) {
+      // Error converting to number


Can remove this comment.

pkwarren · 2025-04-02T15:04:53Z

src/main/java/build/buf/protovalidate/CustomOverload.java

-      return true;
+  private static boolean isIP(String addr, long ver) {
+    if (ver == 6L) {
+      return new Ipv6(addr).address();


This is not very idiomatic Java here - having to create an empty object then calling a method on it to determine if it is valid.

I think we should make these functions like isIPv4 and isIPv6 that minimize allocations and state if possible.

OK, this is where the nuance of trying to stay consistent comes into play. This allocation is done across impls, so should we refactor those also? cc @timostamm

pkwarren · 2025-04-02T15:09:40Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+  private boolean prefixLength() {
+    int start = this.index;
+
+    while (true) {


The condition on line 756 should be moved into the while here.

pkwarren · 2025-04-02T15:09:55Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+    }
+
+    String str = this.str.substring(start, this.index);
+    if (str.length() == 0) {


Suggested change

if (str.length() == 0) {

if (str.isEmpty()) {

pkwarren · 2025-04-02T15:10:24Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+      return true;
+
+    } catch (NumberFormatException nfe) {
+      // Error converting to number


Can remove this.

pkwarren · 2025-04-02T15:10:43Z

src/main/java/build/buf/protovalidate/CustomOverload.java

-      addr = str.toAddress();
-    } catch (Exception e) {
+      int val = Integer.parseInt(str);
+


Lots of extra whitespace in here that isn't improving readability - I'd remove the majority of this.

pkwarren · 2025-04-02T15:12:00Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+   */
+  private boolean digit() {
+    char c = this.str.charAt(this.index);
+    if ('0' <= c && c <= '9') {


We've got lots of places in here that could benefit from a static boolean isDigit(char c) helper to simplify code.

timostamm · 2025-04-02T13:45:22Z

src/main/java/build/buf/protovalidate/CustomOverload.java

            return BoolT.False;
          }
-          return Types.boolOf(validateIP(address, rhs.intValue()));
+          return Types.boolOf(isIP(address, rhs.intValue()));


Is the check for address.isEmpty() still necessary?

Same for other places.

timostamm · 2025-04-02T13:51:06Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+   * <p>The host can be one of: - An IPv4 address in dotted decimal format, for example
+   * "192.168.0.1". - An IPv6 address enclosed in square brackets, for example "[::1]". - A
+   * hostname, for example "example.com".


Should use <ul> for the list:

Suggested change

* <p>The host can be one of: - An IPv4 address in dotted decimal format, for example

* "192.168.0.1". - An IPv6 address enclosed in square brackets, for example "[::1]". - A

* hostname, for example "example.com".

* <p>The host can be one of:

* <ul>

* <li>An IPv4 address in dotted decimal format, for example "192.168.0.1".</li>

* <li>An IPv6 address enclosed in square brackets, for example "[::1]".</li>

* <li>A hostname, for example "example.com".</li>

* </ul>

timostamm · 2025-04-02T13:51:27Z

src/main/java/build/buf/protovalidate/CustomOverload.java

   *
-   * @param host The input string to validate as a hostname.
-   * @return {@code true} if the input string is a valid hostname, {@code false} otherwise.
+   * <p>A valid hostname follows the rules below: - The name consists of one or more labels,


Needs a <ul>

timostamm · 2025-04-02T13:53:04Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+   * <p>Returns 0 if no address was parsed successfully.
+   */
+  int getBits() {
+    if (this.octets.size() != 4) {
+      return -1;


Documented to return 0, but returns -1.

timostamm · 2025-04-02T13:55:40Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+   *
+   * <p>Method parses the rule:
+   *
+   * <p>DIGIT = %x30-39 ; 0-9


Can we use <pre> for ABNF rules?

Suggested change

* <p>DIGIT = %x30-39 ; 0-9

* <pre>DIGIT = %x30-39 ; 0-9</pre>

timostamm · 2025-04-02T13:59:38Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+  // dotted notation successfully parsed as IPv4
+  @Nullable private Ipv4 dottedAddr;
+  private boolean zoneIDFound;
+  // 0 -128


Suggested change

// 0 -128

// 0 - 128

timostamm · 2025-04-02T14:01:12Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+  @Nullable private Ipv4 dottedAddr;
+  private boolean zoneIDFound;
+  // 0 -128
+  private long prefixLen;


Why a long if we're only ever assign values between 0 and 128?

timostamm · 2025-04-02T14:28:31Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+   * <p>The same principle applies to IPv4 addresses. "192.168.1.0/24" designates the first 24 bits
+   * of the 32-bit IPv4 as the network prefix.
+   */
+  static boolean isIPPrefix(String str, long version, boolean strict) {


Should be private like the other members.

timostamm · 2025-04-02T14:36:41Z

src/main/java/build/buf/protovalidate/CustomOverload.java

+   * <p>Note Java does not support unsigned numeric types, so to handle unsigned 32-bit values, we
+   * need to use a 64-bit long type instead of the 32-bit (signed) Integer type.


Documented to use long, but returns an int.

smaye81 · 2025-04-02T20:20:57Z

build.gradle.kts

    api(libs.protobuf.java)
    implementation(enforcedPlatform(libs.cel))
    implementation(libs.cel.core)
    implementation(libs.guava)


We actually can't remove Guava. It's used in a bunch of places after looking more in-depth.

smaye81 · 2025-04-02T20:21:27Z

@timostamm @pkwarren think i addressed most comments. lmk if I missed anything.

Steve Ayers added 5 commits April 1, 2025 10:57

Merge

52f012b

Fix

2f06269

Remove test logging

756f4a0

Cleanup

9cc805f

Revert

ce80f4c

smaye81 changed the base branch from main to sayers/validation_upgrades April 1, 2025 16:03

smaye81 requested review from pkwarren and timostamm April 1, 2025 16:05

Steve Ayers added 3 commits April 1, 2025 14:40

Revert back to JDK 8

69b15dc

Remove ip address

9666630

Remove ip address

edae74f

pkwarren reviewed Apr 2, 2025

View reviewed changes

timostamm reviewed Apr 2, 2025

View reviewed changes

Steve Ayers added 8 commits April 2, 2025 13:03

Feedback

7c1deef

URI

847a663

Fix

2cda9db

Fix

bd1628c

Feedback

80d4168

Updates

0f7a79a

Update

f050ad7

Cleanup

01971b0

smaye81 commented Apr 2, 2025

View reviewed changes

smaye81 requested review from pkwarren and timostamm April 2, 2025 20:21

Remove unsigned shift to 0

8ecbe58

pkwarren approved these changes Apr 2, 2025

View reviewed changes

smaye81 merged commit 5a6c681 into sayers/validation_upgrades Apr 3, 2025
3 checks passed

smaye81 deleted the sayers/is_ip branch April 3, 2025 19:31

	* <p>DIGIT = %x30-39 ; 0-9
	* <pre>DIGIT = %x30-39 ; 0-9</pre>

		* <p>Note Java does not support unsigned numeric types, so to handle unsigned 32-bit values, we
		* need to use a 64-bit long type instead of the 32-bit (signed) Integer type.

Enhance IP validation #256

Enhance IP validation #256

Uh oh!

Conversation

smaye81 commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smaye81 commented Apr 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smaye81 commented Apr 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

smaye81 commented Apr 1, 2025 •

edited

Loading