[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.

[guillem-bartrina-sonarsource]

LGTM from my side, after addressing the other review comments

[gitar-bot]

Code Review ✅ Approved 3 closed / 3 findings

🟡 Medium risk · Introducing shared IP-candidate extraction with URL, endpoint, and CIDR heuristics could select unintended address text for consumers to classify.

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.

✅ 3 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.

Closed: 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".

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}

[sonarqube-next]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube