Avoid IPv6 regex on IPv4:port in ProxyForwardedParser.normalizeIp#319
Avoid IPv6 regex on IPv4:port in ProxyForwardedParser.normalizeIp#319Mishenevd wants to merge 1 commit into
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! |
Mishenevd
force-pushed
the
fix/proxy-forwarded-ipv6-regex-perf
branch
from
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?)
Mishenevd
force-pushed
the
fix/proxy-forwarded-ipv6-regex-perf
branch
2 times, most recently
from
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%).
Mishenevd
force-pushed
the
fix/proxy-forwarded-ipv6-regex-perf
branch
from
6e7a84b to
5f9b47b
Compare
| ip = ip.trim(); | ||
|
|
||
| if (IPValidator.isIP(ip)) { | ||
| return ip; |
There was a problem hiding this comment.
moved this to the bottom so an IPv4:port value gets handled before we ever run the slow IPv6 regex on it.
This is the only difference from the similar algorithm in the node version. There it's fast because its regex engine is way faster, so that first check is basically free
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.