optimize: limit allowlist cache growth - #8120
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 2.x #8120 +/- ##
============================================
- Coverage 73.04% 73.03% -0.02%
Complexity 1141 1141
============================================
Files 1151 1151
Lines 42275 42290 +15
Branches 5045 5048 +3
============================================
+ Hits 30881 30885 +4
- Misses 8919 8926 +7
- Partials 2475 2479 +4
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
slievrly
left a comment
There was a problem hiding this comment.
中文 / Chinese
总体判断
这是一个实打实的安全加固,值得合并。原来的 cache.computeIfAbsent(className, this::doCheck) 把拒绝结果也缓存了,攻击者只要不断发不同的伪造类名,就能把 cache 撑到无界 → 内存型 DoS。这个 PR 的四点改动都对症:
- 超长类名(>1024)在 allowlist 检查前就拒 —— 挡住"超长垃圾类名"这类输入
- 只缓存 allowed,不缓存 rejected —— 攻击者无法用垃圾类名撑大 cache
- cache 上限 4096 —— 即使 allowed 类很多也有界
- 异常消息里截断类名(>256)—— 避免超长类名污染日志 / 日志注入
正确性我看下来没问题:cacheAllowedClass 用 synchronized(cache) 保护 size 检查 + put,不会超过上限;isAllowed 里 cache.get 走 ConcurrentHashMap 读是安全的;check-then-act 因为 doCheck(className) 对同一输入幂等,竞争最多重复计算一次,结果一致。测试也覆盖了拒绝不缓存、超长拒绝、cache 上限、消息截断这几条主路径。
建议(均非阻塞)
1. 满了之后是"硬停",没有淘汰策略
cache 到 4096 后,后续新的 allowed 类永远不再缓存,每次都走 doCheck。如果某个应用的热点类恰好在 4096 个冷类之后才第一次出现,热点类会一直命不中缓存、永久降级为每次检查。正常应用几乎碰不到,但如果想更稳,可以考虑用 access-order 的 LinkedHashMap(LRU)或有界 cache(如 Caffeine maximumSize)替代"硬停"。当前实现作为默认足够,LRU 可作为 follow-up。
2. MAX_CACHE_SIZE / MAX_CLASS_NAME_LENGTH 硬编码
这两个是安全相关的旋钮。4096 对绝大多数应用够用,但代码生成密集(大量 proxy / DTO)的场景可能超。建议至少把 MAX_CACHE_SIZE 做成可配置(seata.json.allowlist.cache-size 之类),或在类上加注释说明选值依据,方便大部署调优。1024 的类名长度上限很宽松,保持硬编码没问题。
3. 拒绝结果不再缓存的 CPU 权衡
现在每次拒绝都会重跑 doCheck。这是"用 CPU 换内存"的正确取舍,但如果存在"同一个被拒类在热路径里反复检查"的场景(比如某种重试循环),会比之前慢。绝大多数情况可忽略,提一句让 reviewer 知道这是有意为之。
4. 测试脆弱性
cacheSize()用反射读私有cache字段 —— 这里为了断言 size 是合理的,但耦合实现。testAllowedClassCacheIsBounded断言"恰好 4096"依赖起始 cache 为空。请确认clearUserAllowlist()确实会清cache(而不只是清 user allowlist),否则 builtin 类若在别的用例里先被缓存过,这个精确断言会 flaky。testAllowedClassCacheIsBounded里 5000 个org.apache.seata.generated.AllowedClass{i}断言isTrue(),前提是 builtin allowlist 里有org.apache.seata前缀。建议在测试里显式加 user prefix 或注释说明这个前提,避免哪天 builtin 前缀调整后测试莫名失败。
小结
方向正确、实现正确、测试到位的一个安全加固,approve 方向。上面 1–4 都是 nice-to-have,其中第 4 点的"精确 4096 断言依赖清空"值得确认一下再合并。
English
Overall
This is a real security hardening and worth merging. The original cache.computeIfAbsent(className, this::doCheck) cached rejected results too, so an attacker sending an unbounded stream of distinct fake class names could grow the cache without limit → a memory DoS. All four changes here are on-target:
- Reject overlong class names (>1024) before the allowlist check — blocks "overlong garbage classname" inputs
- Cache only allowed, never rejected — an attacker can't grow the cache with garbage
- Cap the cache at 4096 — bounded even with many allowed classes
- Truncate the class name in the exception message (>256) — avoids polluting logs / log injection via long names
Correctness looks fine: cacheAllowedClass guards the size-check + put under synchronized(cache), so it never exceeds the cap; the cache.get read in isAllowed is safe on a ConcurrentHashMap; the check-then-act is idempotent since doCheck(className) is deterministic per input, so a race just recomputes once with a consistent result. Tests cover the main paths: rejected-not-cached, overlong-rejected, cache bound, message truncation.
Suggestions (all non-blocking)
1. "Hard stop" once full, no eviction policy
Once the cache hits 4096, subsequently-seen allowed classes are never cached and go through doCheck every time. If an app's hot classes happen to first appear after 4096 cold ones, the hot classes perpetually miss and degrade to check-every-time. Normal apps won't hit this, but for robustness consider an access-order LinkedHashMap (LRU) or a bounded cache (e.g. Caffeine maximumSize) instead of a hard stop. The current impl is fine as a default; LRU can be a follow-up.
2. MAX_CACHE_SIZE / MAX_CLASS_NAME_LENGTH hardcoded
These are security-relevant knobs. 4096 is enough for the vast majority, but codegen-heavy deployments (lots of proxies/DTOs) could exceed it. Suggest making at least MAX_CACHE_SIZE configurable (seata.json.allowlist.cache-size or similar), or add a comment explaining the chosen value so large deployments can tune. The 1024 name-length cap is generous; hardcoding it is fine.
3. CPU trade-off of not caching rejects
Every rejection now re-runs doCheck. This is a correct "trade CPU for memory" decision, but if there's a scenario where the same rejected class is checked repeatedly on a hot path (e.g. a retry loop), it's slower than before. Negligible in almost all cases — just flagging that it's intentional.
4. Test fragility
cacheSize()reflects into the privatecachefield — reasonable here to assert size, but couples to implementation.testAllowedClassCacheIsBoundedasserting "exactly 4096" depends on starting from an empty cache. Please confirmclearUserAllowlist()actually clearscache(not just the user allowlist); otherwise, if a builtin class was cached by another test first, this exact assertion becomes flaky.- In
testAllowedClassCacheIsBounded, the 5000org.apache.seata.generated.AllowedClass{i}are assertedisTrue(), which assumes the builtin allowlist includes anorg.apache.seataprefix. Consider adding a user prefix explicitly in the test or commenting the assumption, so the test doesn't mysteriously fail if the builtin prefixes change.
Summary
A correct, well-tested security hardening — approve direction. Items 1–4 are nice-to-haves; the "exact 4096 assertion depends on a cleared cache" point in #4 is worth confirming before merge.
Ⅰ. Describe what this PR did
Limit the growth of the JSON deserialization allowlist cache.
This PR updates
JsonAllowlistManagerto:It also adds unit tests covering rejected class names, overlong class names, cache hits, and cache size limits.
Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)?
Added unit tests in
JsonAllowlistManagerTest.Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews