perf: 优化获取客户端IP地址的逻辑 #358
perf: 优化获取客户端IP地址的逻辑 #358
Conversation
Summary of ChangesHello @jamebal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求主要聚焦于提升系统在获取客户端 IP 地址时的准确性和鲁棒性。通过调整 IP 获取的策略，优先从常见的代理服务器 HTTP 头中提取真实 IP，并新增对特定代理头（如 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| if (CharSequenceUtil.isNotBlank(ip)) { | ||
| return request.getRemoteHost(); | ||
| } | ||
| return ip; |
There was a problem hiding this comment.
这里的逻辑是错误的。如果通过请求头成功获取了客户端的真实IP地址（存储在 ip 变量中），代码却返回了 request.getRemoteHost() 的值。getRemoteHost() 返回的是直接连接到服务器的客户端（可能是代理服务器）的主机名，这会丢失真实的客户端IP。此外，如果所有请求头都没有IP地址，该方法将返回一个空字符串，而不是回退到 request.getRemoteAddr()。
建议修改逻辑为：如果从请求头中获取到IP，则返回该IP；否则，回退到使用 request.getRemoteAddr()。使用 getRemoteAddr() 而不是 getRemoteHost() 可以避免潜在的DNS查询，性能更好。
| if (CharSequenceUtil.isNotBlank(ip)) { | |
| return request.getRemoteHost(); | |
| } | |
| return ip; | |
| if (CharSequenceUtil.isNotBlank(ip)) { | |
| return ip; | |
| } | |
| return request.getRemoteAddr(); |
| if (CharSequenceUtil.isBlank(ip)) { | ||
| ip = request.getHeader("X-Real-IP"); | ||
| } |
There was a problem hiding this comment.
HttpServletRequest.getHeader() 方法对头名称不区分大小写。因此，在第164行已经检查了 X-real-ip 之后，这里再次检查 X-Real-IP 是多余的。建议删除这个多余的检查。
|
/gemini review |
| try { | ||
| ipSearcher.close(); | ||
| } catch (IOException e) { | ||
| log.error("failed to close searcher\n", e); |
There was a problem hiding this comment.
日志消息字符串末尾的 \n 是多余的。大多数日志框架会自动为每条日志消息附加换行符。移除它可以使日志输出更整洁，并与其他日志消息保持一致。
| log.error("failed to close searcher\n", e); | |
| log.error("failed to close searcher", e); |
| public class IPUtil { | ||
|
|
There was a problem hiding this comment.
对于只包含静态方法的工具类，最好将其声明为 final 并添加一个私有构造函数，以防止被继承和实例化。
| public class IPUtil { | |
| public final class IPUtil { | |
| private IPUtil() {} | |
| } | ||
|
|
||
| private static String extractFirstIp(String ip, String header) { | ||
| if (header.contains("Forwarded") && ip.contains(",")) { |
There was a problem hiding this comment.
在 extractFirstIp 方法中，使用 header.contains("Forwarded") 来判断是否为 X-Forwarded-For 头可能不够健壮。HTTP头是大小写不敏感的，而且 contains 可能会匹配到其他包含 "Forwarded" 的非标准头。建议使用 equalsIgnoreCase 进行精确匹配。
| if (header.contains("Forwarded") && ip.contains(",")) { | |
| if ("X-Forwarded-For".equalsIgnoreCase(header) && ip.contains(",")) { |
|
/gemini review |
There was a problem hiding this comment.
Code Review
本次PR将获取客户端IP的逻辑重构到了新的工具类 IPUtil 中，并优化了实现，整体结构更加清晰。同时，对 region2IpInfo 方法进行了优化，并添加了 @PreDestroy 方法来确保资源被正确释放，这些都是很好的改进。
我在代码中发现两个潜在的正确性问题，并给出了修复建议：
- 在
LogService.region2IpInfo方法中，split方法的limit参数使用不当，可能导致IP区域信息解析错误。 - 在
IPUtil.getClientIP方法中，处理包含前导逗号的IP列表（如', 1.2.3.4'）时存在缺陷，可能导致获取不到正确的IP地址。
请查看具体的审查评论。
| if (!Constants.REGION_DEFAULT.equals(operators)) { | ||
| ipInfo.setOperators(operators); | ||
|
|
||
| String[] parts = IPUtil.SPLIT_PATTERN.split(region, IPUtil.REGION_LENGTH); |
There was a problem hiding this comment.
此处 split 方法的 limit 参数使用不当。当 region 字符串包含的 | 分隔符超过预期（4个）时，split(region, 5) 会将第五个分隔符之后的所有内容都放入最后一个数组元素中，导致 parts.length 仍然为5，从而绕过了长度检查，但最后一个元素的内容是错误的。
例如，如果 region 是 "a|b|c|d|e|f"，parts 将是 ["a", "b", "c", "d", "e|f"]，这会导致后续的IP区域信息解析错误。
建议移除 limit 参数，让 split 方法分割所有部分，然后通过数组长度检查来保证数据的正确性，这与之前的逻辑行为一致。
| String[] parts = IPUtil.SPLIT_PATTERN.split(region, IPUtil.REGION_LENGTH); | |
| String[] parts = IPUtil.SPLIT_PATTERN.split(region); |
| public static String getClientIP(HttpServletRequest request) { | ||
| for (String header : IP_HEADERS) { | ||
| String ip = request.getHeader(header); | ||
| if (isValidIp(ip)) { | ||
| return extractFirstIp(ip); | ||
| } | ||
| } | ||
| return request.getRemoteAddr(); | ||
| } |
There was a problem hiding this comment.
当前的 getClientIP 方法在处理某些边缘情况时存在逻辑缺陷。当HTTP头中的IP地址字符串包含前导逗号（例如 X-Forwarded-For: , 1.2.3.4）时，isValidIp(ip) 会返回 true，但 extractFirstIp(ip) 会返回一个空字符串，导致此方法最终返回一个空字符串而不是正确的IP地址。
为了修复这个问题并使逻辑更健壮，建议调整为先提取第一个IP，然后再对提取出的IP进行有效性验证。这样可以正确处理上述边缘情况，并确保只有有效的IP地址被返回。
| public static String getClientIP(HttpServletRequest request) { | |
| for (String header : IP_HEADERS) { | |
| String ip = request.getHeader(header); | |
| if (isValidIp(ip)) { | |
| return extractFirstIp(ip); | |
| } | |
| } | |
| return request.getRemoteAddr(); | |
| } | |
| public static String getClientIP(HttpServletRequest request) { | |
| for (String header : IP_HEADERS) { | |
| String ip = request.getHeader(header); | |
| if (ip == null) { | |
| continue; | |
| } | |
| String firstIp = extractFirstIp(ip); | |
| if (isValidIp(firstIp)) { | |
| return firstIp; | |
| } | |
| } | |
| return request.getRemoteAddr(); | |
| } |
|
/gemini review |
| if (region == null || region.isEmpty()) { | ||
| return ipInfo; | ||
| } |
There was a problem hiding this comment.
为了使代码更健壮，建议使用 CharSequenceUtil.isBlank(region) 来代替 region == null || region.isEmpty()。isBlank 可以处理 null、空字符串以及只包含空白字符的字符串（例如 " "）。这个类中已经导入并使用了 cn.hutool.core.text.CharSequenceUtil，因此保持代码风格一致性也是一个好处。
| if (region == null || region.isEmpty()) { | |
| return ipInfo; | |
| } | |
| if (CharSequenceUtil.isBlank(region)) { | |
| return ipInfo; | |
| } |
|
/gemini review |
| private static boolean isValidIp(String ip) { | ||
| return ip != null | ||
| && !ip.isEmpty() | ||
| && !"unknown".equalsIgnoreCase(ip); | ||
| } |
There was a problem hiding this comment.
当前的IP验证逻辑 (isValidIp) 过于宽松，只检查了字符串是否为非空且不等于 'unknown'。这可能允许格式不正确的字符串（例如，恶意注入的文本）被当作有效的IP地址处理和记录，存在潜在的日志注入风险。
为了增强安全性和数据准确性，建议使用 hutool 库中的 NetUtil.isIP() 方法来进行更严格的IP地址格式验证。这将确保只接受有效的IPv4或IPv6地址。
为了方便阅读，建议在文件顶部添加 import cn.hutool.core.net.NetUtil;，然后在代码中使用 NetUtil.isIP(ip)。
private static boolean isValidIp(String ip) {
return ip != null
&& !ip.isEmpty()
&& !"unknown".equalsIgnoreCase(ip)
&& cn.hutool.core.net.NetUtil.isIP(ip);
}
No description provided.