Avoid IPv6 regex on IPv4:port in ProxyForwardedParser.normalizeIp#319
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
| int closingBracket = ip.indexOf(']'); | ||
| if (closingBracket > 0) { | ||
| String unwrappedIp = ip.substring(1, closingBracket); | ||
| if (IPValidator.isIP(unwrappedIp)) { | ||
| return unwrappedIp; |
There was a problem hiding this comment.
Bracket parsing accepts malformed inputs like "[::1]foo" because it only checks for ']' and ignores trailing content, yet still returns the inner IP.
Show fix
| int closingBracket = ip.indexOf(']'); | |
| if (closingBracket > 0) { | |
| String unwrappedIp = ip.substring(1, closingBracket); | |
| if (IPValidator.isIP(unwrappedIp)) { | |
| return unwrappedIp; | |
| int closingBracket = ip.indexOf(']'); | |
| // Verify nothing follows the bracket, or only :port follows | |
| if (closingBracket == ip.length() - 1 || ip.charAt(closingBracket + 1) == ':') { | |
| return unwrappedIp; | |
| } |
Details
✨ AI Reasoning
The updated bracket-handling branch now looks for any closing bracket and immediately validates the substring inside it. That makes inputs with invalid trailing characters after the bracket satisfy the branch and return a normalized IP. This contradicts the stated intent of only handling bracketed IP or bracketed IP with port, and it causes clearly wrong acceptance for malformed values.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
a087892 to
9807716
Compare
| int closingBracket = ip.indexOf(']'); | ||
| boolean validSuffix = closingBracket > 0 | ||
| && (closingBracket == ip.length() - 1 || ip.startsWith(":", closingBracket + 1)); | ||
| if (validSuffix) { |
There was a problem hiding this comment.
let's try to keep it more like the node.js logic: https://github.com/AikidoSec/firewall-node/blob/main/library/helpers/getIPAddressFromRequest.ts
(reduce diff a bit, or find maybe a smaller fix?)
a008d59 to
6e7a84b
Compare
normalizeIp() called the version-less IPValidator.isIP(ip) first, which runs the expensive IPv6 regex on every value that is not a bare IPv4 — including IPv4:port forms that proxies routinely send in X-Forwarded-For / the peer address. Since this runs in the per-request context constructor, it added CPU on proxied traffic (introduced in #307). Reorder normalizeIp to run the cheap structural checks before the version-less isIP, keeping the same checks (and branch bodies) as zen-node's getClientIpFromHeader: single-colon IPv4:port via isIP(host, "4"), then bracketed IPv6 ([ipv6] / [ipv6]:port), and only then the version-less isIP as the final fallback. node's isIP is native and cheap, ours is a regex, so the only deviation from node is running isIP last instead of first. Behavior is unchanged (existing ProxyForwardedParserTest suite passes; added cases for multi-colon IPv4, IPv4:port short-circuit, bracketed IPv4 and malformed "[::1]foo"). Verified with a JFR A/B on zen-demo-java under ip:port traffic: total agent CPU 4.90% -> 4.41% (baseline before the regression 4.53%).
6e7a84b to
5f9b47b
Compare
2 participants
Regressed in #307: normalizeIp runs the version-less isIP first, so every IPv4:port value (common in X-Forwarded-For / peer address) pays for the failing IPv6 regex. It runs per request and the IPv6 pattern is expensive on java.util.regex (~335ns here). Fix: check IPv4:port first and keep the version-less isIP as the fallback. Behavior unchanged; existing tests pass plus added cases for multi-colon IPv4, IPv4:port short-circuit, and bracketed IPv4.