fix(security): restore #178/#179 silently reverted by PR #181 + keep ipv4 perf (REV43/REV44) - #193
Luyicheng-Agent wants to merge 1 commit into
Conversation
… (REV43/REV44) Post-commit review of PR #181 (commit 99a6594, "perf(utils): optimize ipv4 parsing") found it silently and byte-exactly reverts two security fixes merged one day earlier, buried under a benign perf title: - REV43: re-adds `log.Printf(" INTERNAL_API_KEY generated (first 4 chars: %s...)", prefix)` in server/api/main.go, reverting #178 (commit 0f70d67, "移除日志中的 INTERNAL_API_KEY 输出"). Dev-only reachable; leaks ~23.8 bits of the 190-bit ephemeral key into logs. - REV44: reverts `shouldTrustAllCertificatesForCurrentBuild` to return `DebugSettingsStore.isSkipMqttCertValidationEnabled(context)` for debug builds and flips the test back to assertTrue, reverting #179 (commit e8352ce, "修复 TLS 证书验证绕过漏洞"). Release stays safe (4 gates); debug opt-in but re-enables an empty X509TrustManager (chain-validation + MQTT TLS pinning bypass). This corrective PR (branched from main, which already contains both fixes): - Keeps the legitimate, behavior-preserving ipv4 parse optimization (parseIpv4Octets rewrite) that was PR #181's actual goal. - Extracts dev-mode key gen into initDevModeInternalAPIKey() (no key logging) and adds TestInitDevModeInternalAPIKeyDoesNotLogKeyOrPrefix to lock in #178 (verified to fail when the leak line is re-added). - Adds a regression-guard comment to the existing shouldTrustAllCertificatesForCurrentBuild_debugBuildAlwaysFalse test to lock in #179. - Adds edge-case tests asserting parseIpv4Octets stays equivalent to the old split(".").map{it.toInt()} (guards SOCKS5 DNS-rebinding defense H4 / M1). - Documents REV43/REV44 in docs/ISSUES.md. PR #181 should not merge until its server/api/main.go and MqttConnectionManager .kt / TlsPolicyTest.kt reversions are dropped; the ipv4 perf portion can be superseded by this PR. Co-authored-by: 01luyicheng <172185967+01luyicheng@users.noreply.github.com>
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
📝 WalkthroughSummary by CodeRabbit
Walkthrough本次变更更新 IPv4 解析边界处理，修复开发模式 API key 日志脱敏，并补充 MQTT TLS 安全约束及问题记录。 ChangesIPv4 解析
开发模式 API key 日志
MQTT TLS 策略
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoRestore reverted security fixes and keep IPv4 parser optimization
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1. REV43/REV44 使用状态字段
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/ISSUES.md (1)
1503-1522: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win压缩 REV43 和 REV44 的历史叙述。
保留提交哈希、位置、触发条件、风险、修复动作和回归测试名称。
删除完整旧代码、熵计算、分支历史和重复的防护说明。
这些信息不改变后续修复或审查决策，并会增加文档上下文噪声。As per coding guidelines, “Documentation minimalism rule: only retain documentation that changes AI execution decisions. Delete outdated, duplicate, or idealized documentation to reduce context noise.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ISSUES.md` around lines 1503 - 1522, 压缩 docs/ISSUES.md 中 REV43 和 REV44 的条目，仅保留提交哈希、受影响位置、触发条件、风险、修复动作及回归测试名称；删除完整旧代码、熵计算、分支历史和重复防护说明，同时保持后续修复与审查决策所需的信息不变。Source: Coding guidelines
android/app/src/main/java/com/netproxy/gateway/utils/IpAddressUtils.kt (1)
31-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
parseIpv4Octets逐字符算法正确,建议拆分以降低复杂度。已逐条模拟符号处理、空段检测、溢出检测与
Int.MIN_VALUE/Int.MAX_VALUE边界情形,该实现精确复刻了负数累加防溢出算法(与 JVMInteger.parseInt一致),未发现功能性错误。静态分析将此代码块标记为高复杂度。建议将单个片段(符号解析 + 数字累加 + 溢出检测)提取为私有辅助函数,例如
parseSignedOctet(ip: String, start: Int, end: Int): AppResult<Int>,由外层循环按.分段调用。这样可以让parseIpv4Octets只负责按.切分,辅助函数只负责单段数值解析,符合单一职责原则,也便于单独测试该辅助函数。由于该算法的负数累加技巧不直观,建议同时添加一条简短注释说明其借鉴了
Integer.parseInt处理Int.MIN_VALUE的方式,方便后续维护者理解。♻️ 提取辅助函数示例
+ // 借鉴 JVM Integer.parseInt 的负数累加算法,以正确处理 Int.MIN_VALUE 而不溢出。 + private fun parseSignedOctet(ip: String, start: Int, end: Int): AppResult<Int> { + var isNegative = false + var j = start + if (ip[j] == '-') { + isNegative = true + j++ + if (j == end) return AppResult.error(IllegalArgumentException("Just minus")) + } else if (ip[j] == '+') { + j++ + if (j == end) return AppResult.error(IllegalArgumentException("Just plus")) + } + + val limit = if (isNegative) Int.MIN_VALUE else -Int.MAX_VALUE + val multmin = limit / 10 + var result = 0 + + while (j < end) { + val digit = ip[j] - '0' + if (digit !in 0..9) return AppResult.error(IllegalArgumentException("Not a digit")) + if (result < multmin) return AppResult.error(IllegalArgumentException("Overflow")) + result *= 10 + if (result < limit + digit) return AppResult.error(IllegalArgumentException("Overflow")) + result -= digit + j++ + } + return AppResult.success(if (isNegative) result else -result) + } + private fun parseIpv4Octets(ip: String): AppResult<List<Int>> { if (ip.isEmpty()) return AppResult.error(IllegalArgumentException("Empty string")) val octets = ArrayList<Int>(4) var start = 0 var i = 0 val len = ip.length while (i <= len) { if (i == len || ip[i] == '.') { if (start == i) return AppResult.error(IllegalArgumentException("Empty octet")) - - var isNegative = false - var j = start - if (ip[j] == '-') { - isNegative = true - j++ - if (j == i) return AppResult.error(IllegalArgumentException("Just minus")) - } else if (ip[j] == '+') { - j++ - if (j == i) return AppResult.error(IllegalArgumentException("Just plus")) - } - - val limit = if (isNegative) Int.MIN_VALUE else -Int.MAX_VALUE - val multmin = limit / 10 - var result = 0 - - while (j < i) { - val digit = ip[j] - '0' - if (digit !in 0..9) return AppResult.error(IllegalArgumentException("Not a digit")) - if (result < multmin) return AppResult.error(IllegalArgumentException("Overflow")) - result *= 10 - if (result < limit + digit) return AppResult.error(IllegalArgumentException("Overflow")) - result -= digit - j++ - } - val value = if (isNegative) result else -result - - octets.add(value) + val octetResult = parseSignedOctet(ip, start, i) + if (octetResult.isError()) return octetResult.map { emptyList() } + octets.add(octetResult.getOrNull()!!) start = i + 1 } i++ } return AppResult.success(octets) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android/app/src/main/java/com/netproxy/gateway/utils/IpAddressUtils.kt` around lines 31 - 73, Reduce complexity in parseIpv4Octets by extracting signed octet parsing—including sign handling, digit validation, and overflow checks—into a private helper such as parseSignedOctet, while keeping the outer method responsible only for dot-separated segment traversal. Preserve all existing AppResult errors and Int.MIN_VALUE/Int.MAX_VALUE behavior, and add a brief comment documenting the negative accumulation approach based on Integer.parseInt.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@android/app/src/main/java/com/netproxy/gateway/utils/IpAddressUtils.kt`:
- Around line 31-73: Reduce complexity in parseIpv4Octets by extracting signed
octet parsing—including sign handling, digit validation, and overflow
checks—into a private helper such as parseSignedOctet, while keeping the outer
method responsible only for dot-separated segment traversal. Preserve all
existing AppResult errors and Int.MIN_VALUE/Int.MAX_VALUE behavior, and add a
brief comment documenting the negative accumulation approach based on
Integer.parseInt.
In `@docs/ISSUES.md`:
- Around line 1503-1522: 压缩 docs/ISSUES.md 中 REV43 和 REV44
的条目，仅保留提交哈希、受影响位置、触发条件、风险、修复动作及回归测试名称；删除完整旧代码、熵计算、分支历史和重复防护说明，同时保持后续修复与审查决策所需的信息不变。
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c02ea6a4-9146-47e7-8726-acf956335bde
📒 Files selected for processing (6)
android/app/src/main/java/com/netproxy/gateway/utils/IpAddressUtils.ktandroid/app/src/test/java/com/netproxy/gateway/connection/MqttConnectionManagerTlsPolicyTest.ktandroid/app/src/test/java/com/netproxy/gateway/utils/IpAddressUtilsTest.ktdocs/ISSUES.mdserver/api/main.goserver/api/main_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Go Server Build (api)
- GitHub Check: Android Build & Test
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx,py,java,cs,go,rb,php}: Use clear, descriptive variable names in all code
Write comments for complex logic and edge cases
Keep functions focused on a single responsibility
Document public APIs and complex modules
Files:
server/api/main.goserver/api/main_test.go
**/*.{kt,go}
📄 CodeRabbit inference engine (CLAUDE.md)
Critical defect: connection pool cleanup race condition - connection state may change between read and write locks. See docs/ISSUES.md H5.
Files:
server/api/main.goandroid/app/src/main/java/com/netproxy/gateway/utils/IpAddressUtils.ktandroid/app/src/test/java/com/netproxy/gateway/utils/IpAddressUtilsTest.ktandroid/app/src/test/java/com/netproxy/gateway/connection/MqttConnectionManagerTlsPolicyTest.ktserver/api/main_test.go
server/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
server/**/*.go: Dependency risk:gorilla/websocketlibrary is archived and no longer maintained. Security vulnerabilities cannot be timely fixed. See docs/ISSUES.md N14.
Service discovery: services use hardcoded addresses for communication instead of service discovery. See docs/TECH_DEBT.md C10.
Single point of failure: Tunnel Gateway has single-point deployment with no horizontal scaling. See docs/TECH_DEBT.md C9.
Code organization: Go project structure is non-standard and not properly layered. See docs/TECH_DEBT.md C7.
Files:
server/api/main.goserver/api/main_test.go
android/app/src/main/java/com/netproxy/gateway/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
android/app/src/main/java/com/netproxy/gateway/**/*.kt: Source of truth for runtime behavior is Kotlin code underandroid/app/src/main/java/com/netproxy/gateway/. If documentation conflicts with code, code is authoritative and documentation must be updated.
WiFi connection path uses traditional API limited on Android 10+.
MQTT security uses TLS 1.2 and certificate pinning when MQTT_TLS_PUBLIC_KEY_PINS is configured; falls back to default CA validation when empty.
Network egress control: cannot force Socket to use WiFi or mobile data; currentprotect()bypasses VPN but traffic may be routed to unintended interfaces in vendor scenarios (Link Turbo, dual WiFi acceleration).
Missing native Android multi-network APIs: code does not useNetwork.bindSocket(),excludeRoute(), or other APIs for true network interface binding.
Multi-network handling is incomplete: code only identifies single network type and cannot properly handle multiple simultaneous networks (dual WiFi, Bluetooth PAN, OTG wired) or network switching scenarios. See docs/TECH_DEBT.md C1 and C3.
Dependency risk: Paho MQTT client maintenance is inactive. New features and bug fixes may be delayed. See docs/ISSUES.md N15.
Deprecated API usage: large amounts of deprecated API usage includingEncryptedSharedPreferences,WifiConfiguration,NioEventLoopGroup. See docs/ISSUES.md N13.
Test coverage gap: core business logic (processVpnTraffic,forwardViaSocks5) lacks test coverage. See docs/ISSUES.md N8.
Files:
android/app/src/main/java/com/netproxy/gateway/utils/IpAddressUtils.kt
android/**
📄 CodeRabbit inference engine (CLAUDE.md)
Build validation gate:
make android-buildmust pass.
Files:
android/app/src/main/java/com/netproxy/gateway/utils/IpAddressUtils.ktandroid/app/src/test/java/com/netproxy/gateway/utils/IpAddressUtilsTest.ktandroid/app/src/test/java/com/netproxy/gateway/connection/MqttConnectionManagerTlsPolicyTest.kt
android/app/src/test/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
android/app/src/test/**/*.kt: Tests are located inandroid/app/src/test/. Verify all changes with unit tests.
Unit test validation gate:make android-testmust pass.
Files:
android/app/src/test/java/com/netproxy/gateway/utils/IpAddressUtilsTest.ktandroid/app/src/test/java/com/netproxy/gateway/connection/MqttConnectionManagerTlsPolicyTest.kt
android/app/src/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Before starting work, verify
make android-testpasses. If main branch tests fail, address with explicit blocking issue statement.
Files:
android/app/src/test/java/com/netproxy/gateway/utils/IpAddressUtilsTest.ktandroid/app/src/test/java/com/netproxy/gateway/connection/MqttConnectionManagerTlsPolicyTest.kt
docs/**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Documentation minimalism rule: only retain documentation that changes AI execution decisions. Delete outdated, duplicate, or idealized documentation to reduce context noise.
Files:
docs/ISSUES.md
docs/{ISSUES,TECH_DEBT}.md
📄 CodeRabbit inference engine (CLAUDE.md)
If discovering unrecorded issues, log commit hash and problem description to docs/ISSUES.md or docs/TECH_DEBT.md.
Files:
docs/ISSUES.md
🔇 Additional comments (8)
android/app/src/test/java/com/netproxy/gateway/connection/MqttConnectionManagerTlsPolicyTest.kt (2)
68-74: LGTM!
68-74: 🎯 Functional Correctness请在合并前运行 Android 单元测试门禁。
当前测试断言与生产代码的 TLS 策略一致。请运行
make android-test并确认通过。As per path instructions，
android/app/src/test/**的变更必须通过make android-test。Source: Path instructions
server/api/main.go (1)
157-161: LGTM!Also applies to: 325-343
server/api/main_test.go (1)
650-658: 🩺 Stability & Availability无需修改测试隔离。
server/api中不存在并行测试；标准 logger 的修改不会和同包其他测试并发执行。android/app/src/main/java/com/netproxy/gateway/utils/IpAddressUtils.kt (1)
30-74: 📐 Maintainability & Code Quality验证
make android-build门禁。根据编码规范,
android/**路径要求make android-build必须通过。PR 描述中的验证记录只提到了server/api下的go vet ./...与go build ./...,未提及 android 构建结果。请确认make android-build已针对本次IpAddressUtils.kt改动执行并通过。As per coding guidelines, "Build validation gate:
make android-buildmust pass." forandroid/**.Source: Coding guidelines
android/app/src/test/java/com/netproxy/gateway/utils/IpAddressUtilsTest.kt (3)
100-121: LGTM!
112-112: 🎯 Functional Correctness | ⚡ Quick win加强对
+4用例的断言,校验解析结果而不仅是成功状态。第112行只断言
isSuccess(),未验证返回值本身是否为true。若解析结果错误地返回false(例如某次重构中符号处理被破坏,导致4被错误地解析为超出范围的值),该断言依然会通过,无法捕获此类回归。既然这是回归测试,应直接校验解析后的布尔值,以确保+4被正确解析为八位段4。✅ 加强断言
- assertTrue(IpAddressUtils.validateIpv4WithResult("1.2.3.+4").isSuccess()) + val plusSignResult = IpAddressUtils.validateIpv4WithResult("1.2.3.+4") + assertTrue(plusSignResult.isSuccess()) + assertTrue(plusSignResult.getOrNull()!!)
1-122: 📐 Maintainability & Code Quality验证
make android-test门禁。根据编码规范,
android/app/src/test/**/*.kt路径要求单元测试验证门禁make android-test必须通过。请确认新增的parseIpv4Octets_preservesToIntSemanticsAcrossEdgeCases测试已纳入make android-test并通过。As per coding guidelines, "Unit test validation gate:
make android-testmust pass." forandroid/app/src/test/**/*.kt.Source: Coding guidelines
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (1) 🔗 Fix PR: #194 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 1 fixed
|
提交后交叉审查（多 subagent）确认本 PR 的修复正确且完整：
一点准确性说明：IPv4 解析器重写并非 100% 等价于 合并建议：合并本 PR 前，PR #181 需先撤销 REV43/REV44 回退并删除 |
独立提交后复核结论：PR #193 通过（未发现新增高危缺陷）作为提交后正确性复核，对本 PR 5 个关注点逐一独立验证（读取实际文件 + git 历史 + commit (a)
|
1 participant
Summary
Post-commit correctness review of the past 24h of commits found that PR #181 commit
99a6594(titledperf(utils): optimize ipv4 parsing) silently and byte-exactly reverts two security fixes merged one day earlier, buried under a benign perf title. Two independent subagents + two cross-review subagents confirmed both via byte-exact diff evidence.This corrective PR is branched from
main(which already contains both fixes) and:parseIpv4Octetsrewrite) — PR ⚡ 优化 IPv4 解析性能 #181's actual goal, verified behavior-preserving vs the oldsplit(".").map { it.toInt() }.initDevModeInternalAPIKey()(no key logging) + a regression test that fails if the leak returns.shouldTrustAllCertificatesForCurrentBuild_debugBuildAlwaysFalsetest.docs/ISSUES.md.Confirmed regressions in PR #181 (commit
99a6594)REV43 — INTERNAL_API_KEY prefix leaked to logs (reverts #178)
server/api/main.gore-adds verbatim the 5 lines removed by0f70d67(PR #178, "移除日志中的 INTERNAL_API_KEY 输出"):INTERNAL_API_KEYunset ANDAPP_ENV=developmentANDENABLE_TLS != "true"(dev-only).REV44 — TLS cert-validation bypass re-enabled in debug (reverts #179)
MqttConnectionManager.ktshouldTrustAllCertificatesForCurrentBuildis changed fromreturn falseback toDebugSettingsStore.isSkipMqttCertValidationEnabled(context), and the test is flippedassertFalse→assertTrue(renamed..._debugBuildAlwaysFalse→..._debugBuildUsesSettingValue) so CI stays green. Whentrue,createDevSocketFactory()installs an emptyX509TrustManager.checkServerTrusted→ chain validation skipped +MQTT_TLS_PUBLIC_KEY_PINSpinning bypassed.BuildConfig.DEBUG,DebugSettingsStoreDEBUG gate,createSecureSocketFactoryIllegalStateException, GradlevalidateReleaseConfig).MQTT_TRUST_ALL_CERTShardcoded"false"), but when enabled any CA-trusted/self-signed + DNS-controlled hostname-matching cert can MITM MQTT.What this PR changes
IpAddressUtils.ktparseIpv4Octetsrewrite (the real perf win).IpAddressUtilsTest.kttoInt()semantics (guards H4 DNS-rebinding / M1).server/api/main.goinitDevModeInternalAPIKey()(banner only, no key logging); preserves #178.server/api/main_test.goTestInitDevModeInternalAPIKeyDoesNotLogKeyOrPrefix(verified to FAIL when the leak line is re-added).MqttConnectionManagerTlsPolicyTest.kt..._debugBuildAlwaysFalselocking in #179.docs/ISSUES.mdVerification
go vet ./...andgo build ./...pass inserver/api.go test -run TestInitDevModeInternalAPIKeyDoesNotLogKeyOrPrefixPASS in clean state; FAIL when thefirst 4 charsleak line is re-injected (guard proven).+/-signs, leading zeros, and Int.MIN/MAX overflow boundaries.Recommendation
Do not merge PR #181 as-is. Drop its
server/api/main.goAPI-key-logging hunk and itsMqttConnectionManager.kt+MqttConnectionManagerTlsPolicyTest.ktTLS hunk (or merge this PR instead, which delivers the ipv4 perf safely). If the debug TLS bypass is genuinely desired, it must come in a dedicated, security-justified PR with a docs update (note:docs/DECISIONS.md:189"debug 默认为 true" is already stale — code default isfalse).Other recently-updated PRs (#177 ipfilter, #184 stream regex, #189 CORS, #192 pairing button) were also reviewed and found to be behavior-preserving / no high-impact bugs introduced.