[lijun-chen-sonarsource]

Add IpAddressExtractor so PHP, Go, Kotlin, and other analyzers can reuse consistent extraction behavior for bare addresses, endpoints, and URLs.

The implementation is extracted from sonar-php PR #1848, with these additions:

• Return Optional<ParsedIp>, allowing callers to query classification directly. ParsedIp.literal() preserves the extracted spelling for analyzer-specific filters.
• Recognize standard URI schemes, including +, -, and ., and skip optional URL user information such as user:pass@.
• Fix: Treat numeric slash suffixes after endpoint ports as paths. For example, 1.2.3.4:8080/33 yields 1.2.3.4, while bare CIDRs retain their prefix for validation and classification.
• Add dedicated extraction tests and documentation covering accepted forms and malformed inputs.

---

_{Stack created with GitHub Stacks CLI • Give Feedback 💬}

---

Summary by Gitar

• Refactor:
  • Renamed IpAddressExtractor to IpAddressCandidateExtractor and removed its dependency on IpAddressClassifier
• API:
  • extract returns Optional<String> candidates instead of Optional<ParsedIp>, leaving validation to IpAddressClassifier.parse
• Tests:
  • Added parameterized tests for URL hosts, endpoints, CIDR suffixes, and malformed endpoints

_{This will update automatically on new commits.}

[hashicorp-vault-sonar-prod]

ACOMMONS-217

[guillem-bartrina-sonarsource]

I believe it is a good idea to standardize the extraction of IP addresses from literals. However, I am not convinced of the coupling with the IpAddressClassifier. I believe there should be a separation of concerns.

Note that ParsedIp is an internal class of the latter, produced by its parse functionality. The extractor is concerned with extracting the slice corresponding to an IP address from a more complex structure. It just so happens to wrap its final result with a call to parse. Notice that you had to add the literal field in ParsedIp just to be able to retrieve the result of the extractor.

To me, it would be clearer if the extractor just extracted the IP address from the literal; the users of the API could then call the classifier if they want to classify it. It is a simple composition on the user's side, and you could remove the literal field from ParsedIp.

WDYT?

[lijun-chen-sonarsource]

@guillem-bartrina-sonarsource

Thanks for the thorough review.

I believe it is a good idea to standardize the extraction of IP addresses from literals. However, I am not convinced of the coupling with the IpAddressClassifier. I believe there should be a separation of concerns.

I agree, I didn’t like the coupling either.

Given the name IpAddressExtractor, I wanted it to guarantee valid IP addresses. However, the extraction logic before the final parse call can produce strings that aren’t IP addresses. For example, https://example.com/path produces example.com.

To avoid duplicating IP address validation logic, I reused IpAddressClassifier.parse.

One option would be to extract the validation and parsing logic, along with the parsed representation, into a separate class shared by IpAddressExtractor and IpAddressClassifier. This would preserve the validity guarantee without depending on a classifier-owned type.

A simpler alternative would be to name it IpAddressCandidateExtractor and explicitly define its responsibility as extracting text for subsequent IP address parsing from URLs, endpoints, etc. The result could be a hostname or malformed IP address and callers would compose extraction with IpAddressClassifier.parse when they need validation or classification.

I'm inclined to go with the latter, though it feels uglier.

WDYT?

[guillem-bartrina-sonarsource]

I'm inclined to go with the latter, though it feels uglier.

I'd go with the latter as well, even if it is a bit ugly. Maybe @teemu-rytilahti-sonarsource has some input.

[teemu-rytilahti-sonarsource]

Just a couple of nits from a very quick review, but it is looking good already, thanks @lijun-chen-sonarsource!

The PR title and the desc need an update after the refactoring & renaming.

[teemu-rytilahti-sonarsource]

It is probably worth to limit the maximum number of matching characters for the scheme part. https://www.iana.org/assignments/uri-schemes contains the available, IANA-registered schemes, so maybe picking up the longest one and adding some extra characters (+10 or so) would make sense?

If this is only used for IP addresses, the part after // could also be stricter, if domain names are also wanted, the matching could be done by restricting the match length between // and the next non-dot and non-dash word boundary.

[lijun-chen-sonarsource]

We support matching also user information (e.g., http://user@1.2.3.4/path), for which there is AFAIK no official hard limit. So, in general we can't bound the regex match before reaching to a IP address host (unless we want to put some arbitrary custom limit).
Given this, I find bounding the scheme also not that useful. The scan is O(n) in time and O(1) in memory. I don't see strong reason to bound it.

WDYT?

[teemu-rytilahti-sonarsource]

1.2.3.4 is located in a reserved address space, it is better to use something non-routable (e.g., from the documentation networks) for both v4 and v6 examples.

[lijun-chen-sonarsource]

Addressed in 4f9e405

[teemu-rytilahti-sonarsource]

2.5/16 is an assigned range, see my earlier comment on using example/doc ranges.

[lijun-chen-sonarsource]

After 1d63e23, that part of IpAddressClassifier (literal()) is no more necessary.

Removed in 409e03a, which indirectly addressed your comment.

[teemu-rytilahti-sonarsource]

CIDR should be up to max 2 (for v4) and 3 (v6) digits.

[lijun-chen-sonarsource]

ASCII_DIGITS is used both both CIDR (2 or 3 digits) and port (5 digits).
I kept it simple and did length checking before doing the regex matching.
See 135abfd

[teemu-rytilahti-sonarsource]

It would be a good idea to have have few test cases for query strings containing ampersands, semi-colons, and similar characters that are found in URLs in the wild.

[lijun-chen-sonarsource]

Addressed in 36c5f6f

[guillem-bartrina-sonarsource]

LGTM from my side, after addressing the other review comments

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0 Dependency risks
99.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[gitar-bot]

💡 Quality: Javadoc omits the new five-character port limit

isPort now rejects any port string longer than 5 characters before parsing it, so 192.0.2.1:000080 returns empty even though its value, 80, is in range. The new test at line 161 checks for this. The Javadoc for extract still says ports are accepted when they are "ASCII decimal ports from 0 through 65535", which tells callers that zero-padded ports like 000080 will be stripped. Callers in PHP, Go and Kotlin analyzers that rely on the documented contract will get Optional.empty() for these inputs. The fix is to state the limit in the doc, e.g. "ASCII decimal ports of at most five digits with values from 0 through 65535".

Document the five-digit bound on ports:

*   <li>Removes a single enclosing pair of brackets from IPv6 addresses, and ASCII decimal ports of at most five digits
*     with values from 0 through 65535.

• Apply fix

_{Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎}

[gitar-bot]

Code Review 👍 Approved with suggestions 2 closed / 3 findings

🟡 Medium risk · The shared heuristic extractor selects URL hosts and first-list candidates, so ambiguous inputs could cause analyzers to miss or misclassify IP findings.

Adds IpAddressCandidateExtractor to pull bare addresses, endpoints, and URLs out of strings for reuse across analyzers, with validation left to IpAddressClassifier.parse. Endpoint numeric path suffixes and unlisted delimiters are now handled correctly.

One open minor finding: the extract Javadoc does not mention the new five-character port limit, so zero-padded ports such as 000080 return empty despite being in range. Updating the doc to state the limit is suggested.

💡 Quality: Javadoc omits the new five-character port limit

📄 commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressCandidateExtractor.java:47 📄 commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressCandidateExtractor.java:131-136 📄 commons/src/test/java/org/sonarsource/analyzer/commons/appsec/IpAddressCandidateExtractorTest.java:161

isPort now rejects any port string longer than 5 characters before parsing it, so 192.0.2.1:000080 returns empty even though its value, 80, is in range. The new test at line 161 checks for this. The Javadoc for extract still says ports are accepted when they are "ASCII decimal ports from 0 through 65535", which tells callers that zero-padded ports like 000080 will be stripped. Callers in PHP, Go and Kotlin analyzers that rely on the documented contract will get Optional.empty() for these inputs. The fix is to state the limit in the doc, e.g. "ASCII decimal ports of at most five digits with values from 0 through 65535".

Document the five-digit bound on ports

*   <li>Removes a single enclosing pair of brackets from IPv6 addresses, and ASCII decimal ports of at most five digits
*     with values from 0 through 65535.

✅ 2 closed

✅ Edge Case: Endpoint with port treats a numeric path as a CIDR suffix

📄 commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressExtractor.java:40 📄 commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressExtractor.java:68-82
The slash suffix is split off and kept as cidr (lines 69-77) before ports are handled, both the bracketed port check (lines 80-90) and the IPv4 port strip (lines 97-103). So bare endpoints with a port and a numeric path get the path reattached as a CIDR prefix: 1.2.3.4:8080/0 returns 1.2.3.4/0 and [2003::1]:8080/64 returns 2003::1/64. The Javadoc says CIDR suffixes are kept only on bare addresses, and an address with a port is an endpoint, where the slash starts a path, just as with URL hosts. With /0 the result is passed to IpAddressClassifier as an unrestricted CIDR (the S6321 case). Fix: drop cidr whenever a port was stripped, either from the bracket suffix or from the IPv4 colon.

✅ Edge Case: Unlisted delimiters cut off malformed candidates, so 1.2.3.4~foo is accepted

📄 commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressExtractor.java:29 📄 commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressExtractor.java:46 📄 commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressExtractor.java:64-69
ADDRESS_TOKEN ([\w.:\[\]%+/-]+) is matched with lookingAt(), so the token ends at the first character outside that class. That set is much wider than the Javadoc's "query, fragment, comma, or whitespace". I traced these inputs:

• "1.2.3.4~foo", "1.2.3.4!x", "1.2.3.4;x", "1.2.3.4'x", "1.2.3.4$x" and "1.2.3.4=x" all become the token 1.2.3.4, which then parses and is returned.
• The same happens for URL hosts such as http://1.2.3.4~x/path. There ~ (unreserved) and !$&'()*;= (sub-delims) are legal reg-name characters, so the real host is a name, not an IP address.

This contradicts the code comment "Keep malformed candidates intact: 1.2.3.4-foo must not become 1.2.3.4", which is enforced only for -, + and _. The fix is to stop only at the documented delimiters and reject when the token is followed by anything else. Alternatively, update the Javadoc to list every accepted stop character.

🤖 Prompt for agents

Code Review: Adds `IpAddressCandidateExtractor` to pull bare addresses, endpoints, and URLs out of strings for reuse across analyzers, with validation left to `IpAddressClassifier.parse`. Endpoint numeric path suffixes and unlisted delimiters are now handled correctly.
  
  One open minor finding: the `extract` Javadoc does not mention the new five-character port limit, so zero-padded ports such as `000080` return empty despite being in range. Updating the doc to state the limit is suggested.

1. 💡 Quality: Javadoc omits the new five-character port limit
   Files: commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressCandidateExtractor.java:47, commons/src/main/java/org/sonarsource/analyzer/commons/appsec/IpAddressCandidateExtractor.java:131-136, commons/src/test/java/org/sonarsource/analyzer/commons/appsec/IpAddressCandidateExtractorTest.java:161

   `isPort` now rejects any port string longer than 5 characters before parsing it, so `192.0.2.1:000080` returns empty even though its value, 80, is in range. The new test at line 161 checks for this. The Javadoc for `extract` still says ports are accepted when they are "ASCII decimal ports from 0 through 65535", which tells callers that zero-padded ports like `000080` will be stripped. Callers in PHP, Go and Kotlin analyzers that rely on the documented contract will get `Optional.empty()` for these inputs. The fix is to state the limit in the doc, e.g. "ASCII decimal ports of at most five digits with values from 0 through 65535".

   Fix (Document the five-digit bound on ports):
   *   <li>Removes a single enclosing pair of brackets from IPv6 addresses, and ASCII decimal ports of at most five digits
   *     with values from 0 through 65535.

Verification Rules 2/2 met Repos 12 inspected · PRs 2 inspected

Functional validation No results

Rules 2 passed 1 not applicable

✅ Internal artifact registries
✅ Repository license compliance

Broader impact Repos 12 inspected PRs 2 inspected

Cross-repo 12 repositories inspected 0 open impacts

SonarSource/sonar-php Inspected
SonarSource/sonar-kotlin Inspected
SonarSource/sonar-python Inspected
SonarSource/SonarJS Inspected
SonarSource/sonar-dotnet Inspected

7 more repositories

Cross-PR 2 PRs inspected 0 open impacts

PR 1848: SONARPHP-1927 S1313 Adopt IpAddressClassifier for hardcoded-IP filtering in sonar-php Inspected
PR 571: ACOMMONS-164 Improve IP classification and allow querying without reparsing Inspected

Auto-approval Not enabled

Set up

_{For more verification details, comment gitar display:verbose.}

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}