Skip to content

security: expand autoType deny list + implement addAutoTypeDeny (5 commits, closes 5 VULNs, tests included) #7846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DenceChen wants to merge 5 commits into
base: main
Choose a base branch
from
Open

security: expand autoType deny list + implement addAutoTypeDeny (5 commits, closes 5 VULNs, tests included) #7846

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1c21cbd
security: expand isAutoTypeDenyClass with hardcoded FQCN list (PR-1 o…
DenceChen Sep 2, 2026
6ad74fa
security: defence-in-depth in ObjectReaderImplClass (PR-2 of 3)
DenceChen Sep 2, 2026
11088ae
security: implement addAutoTypeDeny(String) + integrate programmatic …
DenceChen Sep 2, 2026
c00e3fd
fix(security): restore correct method bodies in ObjectReaderProvider.…
DenceChen Sep 3, 2026
e057e26
test(security): add unit tests covering all three PRs
DenceChen Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions core/src/main/java/com/alibaba/fastjson2/reader/ObjectReaderImplClass.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.alibaba.fastjson2.JSONException;
import com.alibaba.fastjson2.JSONReader;
import com.alibaba.fastjson2.util.Fnv;
import com.alibaba.fastjson2.util.JDKUtils;
import com.alibaba.fastjson2.util.TypeUtils;

import java.lang.reflect.Type;
Expand Down Expand Up @@ -47,6 +48,19 @@ public Object readObject(JSONReader jsonReader, Type fieldType, Object fieldName
}

String className = jsonReader.getString();

// Defence-in-depth: refuse to load any class whose name matches the FQCN deny table before

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-9: [certifies-falsely] [regression] both checks added to readObject are unconditional — they run before the SupportClassForName gate, ignore the provider's accept list, and veto a class checkAutoType has already returned for an explicit full-name accept.

That inverts the precedence the three pre-existing isAutoTypeDenyClass call sites encode (ObjectReaderProvider.java:952 and :986 both guard on i + 1 < typeNameLength) and the @return contract this diff itself keeps at JDKUtils.java:606 — "true if the class must not be resolved through a whitelist prefix match". An app that relied on SupportClassForName to carry JDK type names in a config bean loses it with no escape hatch, while the sibling @type path still honours the same accept entry — so one FQCN is hard-denied on one path and accepted on another. Apps that never enabled SupportClassForName also lose the actionable diagnostic and get an autoType error for a config that is already enabled.

Witness (A/B against released fastjson2-2.0.65):

{"clazz":"java.net.URL"} + SupportClassForName      BASE: OK    HEAD: THROW autoType is not support : java.net.URL
{"clazz":"java.lang.Thread"}                        BASE: OK    HEAD: THROW
{"clazz":"java.io.File"}                            BASE: OK    HEAD: THROW
after addAutoTypeAccept("java.net.URL")             BASE: OK    HEAD: THROW   <- explicit accept ignored
addAutoTypeAccept("java.lang.Thread") + @type       BASE: OK    HEAD: OK      <- sibling path honours it
"java.lang.Runtime" -> Class.class, SupportAutoType only
  BASE: THROW not support ClassForName : java.lang.Runtime, you can config 'JSONReader.Feature.SupportClassForName'
  HEAD: THROW autoType is not support : java.lang.Runtime
same shape, non-denied "com.acme.Gadget"            BASE == HEAD (unchanged)  <- loss is specific to deny-listed names

Make both checks conditional on the same policy the rest of the library uses: drop the early string loop and rely on the post-resolve check, or keep an early reject but place it after the classForName gate and skip it when the provider has an accept entry naming the type in full. For the post-resolve check at :84-86, restrict it to the prefix-match case so an explicit full-name accept stays an opt-in.

Please do not simply delete the :84-86 check: measured, it is the only gate that stops a full-name-accepted ClassLoader / DataSource / RowSet subtype end-to-end, because ObjectReaderProvider.java:951-954 skips its veto when i + 1 == typeNameLength.

Constraint: the post-resolve check only runs when provider.checkAutoType(...) at :76 returns non-null, so removing the early loop must not also remove that call; and TypeUtils.getMapping(className) at :71-74 returns before :84, so a name check placed only before loadClass leaves the mapping branch uncovered. AutoTypeValidationTest.java:279-289 (testExactAcceptNameAllowsClassLoader) is the existing test encoding that a full-name accept is an explicit opt-in.

Acceptance: a test asserting {"clazz":"java.net.URL"} into a Class<?> field succeeds after addAutoTypeAccept("java.net.URL") (red today), a mirror case asserting a full-name-accepted test-local ClassLoader subclass still throws, and {"clazz":"...TemplatesImpl"} still throws so the fix is not read as "delete the list".

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

// invoking TypeUtils.loadClass. Without this check, an attacker controlling JSON input
// could send {"@type":"java.lang.Class","val":"com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl"}
// and rely on the outer checkAutoType to silently permit the resolution because the inner
// Class.forName runs in a privileged context. This makes the policy explicit and matches
// the same safety check applied to the fastjson 1.x MiscCodec branch.
for (int i = 0; i < JDKUtils.AUTO_TYPE_DENY_FQCN.length; i++) {

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-1: core main sources do not compile — JDKUtils.AUTO_TYPE_DENY_FQCN is package-private in com.alibaba.fastjson2.util but is dereferenced from com.alibaba.fastjson2.reader.

No jar is produced, so core cannot be built or tested, fastjson1-compatible / extension-* / kotlin cannot be built against it, and every CI matrix job fails (29 of them are already red on this commit). Because nothing builds, none of the five vulnerabilities this PR says it closes changes for any consumer, and the verification command in the PR description cannot have been run. The break went unnoticed because the one test that touches the array, JDKUtilsDenyListTest, sits in the same package as JDKUtils.

Witness:

[ERROR] ObjectReaderImplClass.java:[58,37] AUTO_TYPE_DENY_FQCN is not public in com.alibaba.fastjson2.util.JDKUtils; cannot be accessed from outside package
[ERROR] ObjectReaderImplClass.java:[59,42] AUTO_TYPE_DENY_FQCN is not public in com.alibaba.fastjson2.util.JDKUtils; cannot be accessed from outside package
[INFO] BUILD FAILURE

Please do not fix this by adding public to the array — a public static final String[] is element-writable, so any caller could do JDKUtils.AUTO_TYPE_DENY_FQCN[0] = "" and silently disarm a security deny list (see the comment on JDKUtils.java:79). Expose the decision instead:

// JDKUtils
private static final Set<String> AUTO_TYPE_DENY_FQCN_SET =
        Collections.unmodifiableSet(new HashSet<>(Arrays.asList(AUTO_TYPE_DENY_FQCN)));

public static boolean isAutoTypeDenyName(String name) {
    return name != null && AUTO_TYPE_DENY_FQCN_SET.contains(name);
}
// ObjectReaderImplClass.readObject, replacing the open-coded loop
if (JDKUtils.isAutoTypeDenyName(className)) {
    throw new JSONException(jsonReader.info("autoType is not support : " + className));
}

This also removes the duplicated 80-iteration linear scan and brings isAutoTypeDenyClass back under the JIT inlining threshold.

Constraint: JDKUtilsDenyListTest.java:6 does import static com.alibaba.fastjson2.util.JDKUtils.AUTO_TYPE_DENY_FQCN;, which compiles today only because that test sits in com.alibaba.fastjson2.util — whatever shape replaces the array must keep that static import valid, or update the test in the same commit.

Acceptance: mvn -pl core compile goes green. Note that no test pins the code this restores — with the loop deleted, ObjectReaderImplClassTest still passes 2/2 (see the comment on that file), so if the loop is kept it needs a test whose payload is not already denied at ObjectReaderProvider.java:1024.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

if (className.equals(JDKUtils.AUTO_TYPE_DENY_FQCN[i])) {
throw new JSONException(jsonReader.info("autoType is not support : " + className));
}
}

boolean classForName = ((context.getFeatures() | features) & JSONReader.Feature.SupportClassForName.mask) != 0;
if (!classForName) {
String msg = jsonReader.info("not support ClassForName : " + className + ", you can config 'JSONReader.Feature.SupportClassForName'");
Expand All @@ -63,6 +77,13 @@ public Object readObject(JSONReader jsonReader, Type fieldType, Object fieldName
if (resolvedClass == null) {
throw new JSONException(jsonReader.info("class not found " + className));
}
// Defence-in-depth: even after the outer checkAutoType gate has run, re-check the resolved
// class against the FQCN deny table. This closes the residual surface where
// SupportClassForName is opted in and the outer checkAutoType would otherwise have to
// assume the resolved class is safe.
if (JDKUtils.isAutoTypeDenyClass(resolvedClass)) {
throw new JSONException(jsonReader.info("autoType is not support : " + className));
}
return resolvedClass;
}
}
74 changes: 70 additions & 4 deletions core/src/main/java/com/alibaba/fastjson2/reader/ObjectReaderProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,15 @@ public ObjectReaderCachePair(long hashCode, ObjectReader reader) {
*/
private volatile Set<String> acceptNameSet = Collections.emptySet();

/**
* Programmatic deny list registered via {@link #addAutoTypeDeny(String)}. Independent from the
* hardcoded FQCN list in {@link JDKUtils#AUTO_TYPE_DENY_FQCN}; both lists are consulted on every
* {@link #checkAutoType} call. Names are normalized the same way as {@link #acceptNameSet} so a
* {@code $} ↔ {@code .} rewrite cannot smuggle a denied entry past the rolling-hash scan.
*/
private volatile long[] denyHashCodes = new long[0];
private volatile Set<String> denyNameSet = Collections.emptySet();

private AutoTypeBeforeHandler autoTypeBeforeHandler = DEFAULT_AUTO_TYPE_BEFORE_HANDLER;
private Consumer<Class> autoTypeHandler = DEFAULT_AUTO_TYPE_HANDLER;
PropertyNamingStrategy namingStrategy;
Expand All @@ -240,6 +249,22 @@ public ObjectReaderCachePair(long hashCode, ObjectReader reader) {
acceptNameSet = Collections.unmodifiableSet(names);
acceptHashCodes = hashCodes;

// Seed the programmatic deny list from the JVM-wide fastjson2.parser.deny system property
// so that setting it at JVM start actually works in fastjson2 (this was previously only
// honoured by fastjson 1.x's ParserConfig; the 2.x Provider silently ignored it).
String denyProp = System.getProperty("fastjson2.parser.deny");

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-15: this instance initializer re-reads fastjson2.parser.deny through two hardcoded string literals, duplicating a read this class already performs into static final String[] DENYS (:70, :91-100) via the statically-imported constant PROPERTY_DENY_PROPERTY (JSONFactory.java:64) — and leaves DENYS still unread, so the class now holds two independent parses of one property, one of them dead.

DENYS was already dead at the base commit and stays dead; a repo-wide grep finds no reader. The comment above this block ("the 2.x Provider silently ignored it") is also imprecise about mechanism: the provider did read the property, it never enforced it. The concrete cost is two sources of truth 160 lines apart — a future rename or aliasing of the property updates the constant and the dead parse and silently leaves these literals behind, and the failure mode is a system property that stops having any effect with no compile error and no test that would notice.

Witness:

java -Dfastjson2.parser.deny='com.a.Foo, com.b.Bar'   (note the space after the comma)
  static DENYS         = [com.a.Foo,  com.b.Bar]   <- split(",") keeps the leading space
  instance denyNameSet = [com.a.Foo], [com.b.Bar]  <- the new loop trims
DENYS readers: grep over the whole repo -> 3 hits, all in ObjectReaderProvider (:70 decl, :97/:99 putstatic), 0 reads
base jar, all classes: "Field DENYS:" = 2 refs, both putstatic in <clinit>, 0 getstatic  <- already dead at the merge base

Use the constant for both reads and then pick one owner: either delete the now-doubly-dead DENYS, or seed from it and delete this second property read. Do not leave both.

Constraint: static final String[] DENYS; is assigned in a static {} block, so it snapshots the property once per classloader — while testSystemPropertyDenySeed sets the property at runtime and relies on this per-instance initializer seeing it. A DENYS-based consolidation therefore breaks that test; either keep the per-instance read or the test must seed the property before class initialization.

Acceptance: ObjectReaderProviderTest.testSystemPropertyDenySeed stays green whichever side is removed. (The whitespace divergence between the two parses is covered by the trim fix on addAutoTypeDeny, so it is not asked for twice here.)

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

if (denyProp == null || denyProp.isEmpty()) {
denyProp = JSONFactory.Conf.getProperty("fastjson2.parser.deny");

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-23: this seeding reads only the v2 spelling fastjson2.parser.deny. The v1 spelling fastjson.parser.deny — the one a migrating app actually has set, and the audience the new addAutoTypeDeny javadoc addresses — is honoured nowhere in the repo, so the parity this diff claims is only half-wired.

ParserConfig.DENY_PROPERTY = "fastjson.parser.deny" has exactly one consumer, configFromPropety(Properties) (ParserConfig.java:134-138), and that method has no production caller: the only call site repo-wide is ParserConfigTest.java:24, passing an empty Properties. The same file already shows the intended v1/v2 shape 90 lines above — the SAFE_MODE block at :162-171 reads both spellings of the analogous pair, and a repo-wide grep for "fastjson.parser.<x>" in core/src/main/java returns exactly two hits, both fastjson.parser.safeMode, both in that initializer. So the "core shouldn't read v1-namespaced properties" defence does not hold here.

Witness:

ARM A (PR head, -Dfastjson.parser.deny=probe.Target)
  checkAutoType(...) = RESOLVED -> probe.Target
  JSON.parseObject(...) = INSTANTIATED probe.Target -> {"id":7}
  provider.denyNameSet = []
ARM B (PR head, -Dfastjson2.parser.deny=probe.Target)
  checkAutoType(...) = DENIED (JSONException: autoType is not support. probe.Target)
  provider.denyNameSet = [probe.Target]
ARM C (PR head, no property) = identical to ARM A   <- the v1 property has literally zero effect
BASE 2.0.65, -Dfastjson.parser.deny=probe.Target -> RESOLVED / INSTANTIATED (same as head: nothing removed)
FIX APPLIED (both spellings, both channels), rebuilt:
  ARM A-fixed -> DENIED, denyNameSet = [probe.Target]      ARM C-fixed -> unchanged (fix does not over-deny)
  ARM D-fixed (v1 spelling via fastjson2.properties on TCCL, NO -D) -> denyNameSet = [probe.Target]

The migrating operator is real, not hypothetical: javap -c on the released fastjson-1.2.83.jar shows ParserConfig.<clinit> reading fastjson.parser.deny into DENYS, so that protection genuinely existed on 1.x and silently stops working after migration. Read both spellings through both channels, mirroring :162-171. If the omission is deliberate, drop the 1.x-parity framing from the comment above this block and from the addAutoTypeDeny javadoc instead, so the operator is not told the legacy path is covered.

Constraint: JSONFactory.Conf.DEFAULT_PROPERTIES is populated once, only from fastjson2.properties on the thread-context classloader, and does not merge system properties (JSONFactory.java:36-53) — so each spelling needs its own System.getProperty + Conf.getProperty pair; one read per spelling is not enough.

Acceptance: a new case that sets only fastjson.parser.deny=com.acme.Z, builds new ObjectReaderProvider(), and asserts checkAutoType("com.acme.Z", null, SupportAutoType.mask) throws — measured red before the two extra reads and green after, on both the -D and the fastjson2.properties channel. It must save and clear both property names in @BeforeEach/@AfterEach, since the existing pair handles only the v2 name.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

}
if (denyProp != null && !denyProp.isEmpty()) {
for (String item : denyProp.split(",")) {
String trimmed = item.trim();
if (!trimmed.isEmpty()) {
addAutoTypeDeny(trimmed);
}
}
}

hashCache.put(ObjectArrayReader.TYPE_HASH_CODE, ObjectArrayReader.INSTANCE);
final long STRING_CLASS_NAME_HASH = -4834614249632438472L; // Fnv.hashCode64(String.class.getName());
hashCache.put(STRING_CLASS_NAME_HASH, ObjectReaderImplString.INSTANCE);
Expand Down Expand Up @@ -299,8 +324,40 @@ public synchronized void addAutoTypeAccept(String name) {
}
}

@Deprecated
public void addAutoTypeDeny(String name) {
/**
* Adds a type name to the programmatic deny list. Types on this list are rejected by
* {@link #checkAutoType} regardless of {@code SupportAutoType} or any explicit accept entry.
* Previously this method was a no-op {@code @Deprecated} stub; this restores parity with
* {@code com.alibaba.fastjson.parser.ParserConfig#addDeny} so that downstream apps that
* migrate from fastjson 1.x and rely on the legacy deny API still get the protection they
* expect.
*
* @param name the type name to add (matched after {@code $} ↔ {@code .} normalization,
* exactly the same way {@link #addAutoTypeAccept} treats names)
*/
public synchronized void addAutoTypeDeny(String name) {
if (name == null || name.isEmpty()) {
return;
}
String denyName = normalizeAcceptName(name);

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-7: [certifies-falsely] [new-surface] a deny entry is stored and matched as ONE exact $→.-normalized string, while the rest of the library resolves a single type from many names and the sibling accept machinery 15 lines below matches prefixes — so four whole classes of entry silently deny nothing.

The javadoc this diff adds promises "restores parity with com.alibaba.fastjson.parser.ParserConfig#addDeny" and "matched ... exactly the same way addAutoTypeAccept treats names". Measured against the real 1.2.83 artifact, neither holds for the entry shapes below. This is one root decision — the deny key is the raw registered string, never the identity of the type it names — so it is filed once rather than as four comments.

  1. Prefix. addAutoTypeDeny("com.evil"), or -Dfastjson2.parser.deny=com.evil, or ParserConfig.getGlobalInstance().addDeny("com.evil") from fastjson1-compatible (which forwards straight here at ParserConfig.java:97-103): checkAutoType("com.evil.Gadget", ...) hashes the full name, misses, and resolves the gadget. No exception, no log, and denyNameSet still shows the entry — so an operator auditing "what did I deny" sees it. A trailing-dot spelling matches nothing at all, since normalizeAcceptName does not strip a trailing dot.
  2. Descriptor and array spellings. hasIllegalTypeNameChars rejects only : and ! (TypeUtils.java:2843-2845) while TypeUtils.loadClass strips L…; at :3027-3029 and resolves [X / X[] at :3031-3037; the component recursion at :908-911 fires only for a leading [, so it never sees a suffix-form component. The denied component is resolved and linked through the thread-context classloader — precisely what the comment at :918-920 says cannot happen.
  3. Alias spellings. TYPE_MAPPINGS holds 132 alias/short keys beside 84 dot-shaped ones and normalizeAcceptName performs no alias resolution, so denying java.util.HashMap does not block "HashMap".
  4. Whitespace. addAutoTypeDeny does not trim while the property-seeding loop this same PR adds at :258-266 does, and the only in-repo consumer of the restored API feeds it untrimmed items (splitItemsFormProperty is property.split(",") with no trim, ParserConfig.java:169-174). So fastjson.parser.deny=com.a.Evil, com.b.Evil registers a leading-space key that can never match. The first item works, which is what makes the failure invisible: the control is half-live. Note the asymmetry the accept side does not share — an untrimmed accept entry fails closed, an untrimmed deny entry fails open.

Witness:

# (1) real fastjson 1.2.83 — the parity authority the javadoc cites — vs this PR:
1.2.83 addDeny(com.evil)       -> checkAutoType(com.evil.Gadget) => THROW
1.2.83 addDeny(com.sun.rowset) -> checkAutoType(com.sun.rowset.JdbcRowSetImpl) => THROW
HEAD   deny(com.evil)          -> OK: class com.evil.Gadget   (BASE identical)
HEAD   deny(com.evil.)         -> OK: class com.evil.Gadget
HEAD   accept(com.evil.) CONTROL -> OK: class com.evil.Gadget  (accept side IS prefix-based)
HEAD   -Dfastjson2.parser.deny=com.evil -> OK: class com.evil.Gadget

# (2) sweep over every spelling TypeUtils.loadClass resolves — 4 of 5 bypass:
HEAD checkAutoType("java.util.Currency")     -> THREW
HEAD checkAutoType("java.util.Currency[]")   -> class [Ljava.util.Currency;
HEAD checkAutoType("Ljava.util.Currency;")   -> class java.util.Currency
HEAD checkAutoType("[Ljava.util.Currency;")  -> class [Ljava.util.Currency;
HEAD checkAutoType("[[Ljava.util.Currency;") -> class [[Ljava.util.Currency;
HEAD (app class in no mapping table) TCCL loadClass() requests observed: [probe.ProbeArrayDeny2$Gadget]
HEAD deny(com.acme.Gadget) @type "com.acme.Gadget"   -> THREW
HEAD deny(com.acme.Gadget) @type "Lcom.acme.Gadget;" -> OK: Gadget{pwn}  (instantiated AND populated)
# same hole on the hardcoded table: checkAutoType("java.net.InetSocketAddress[]") -> the array class
# and on a Class<?> field: clazz=...InetSocketAddress THROWS, clazz=...InetSocketAddress[] RESOLVES

# (3) alias, measured on the Class-field route (the @type-side alias row was not run)
HEAD {"clazz":"HashMap"} with addAutoTypeDeny("java.util.HashMap") -> RESOLVED class java.util.HashMap

# (4) whitespace, through the real fastjson1-compatible bridge:
BASE property "com.a.A, probe.Payload"  -> INSTANTIATED    BASE "com.a.A, probe.Payload" -> INSTANTIATED
HEAD property "com.a.A,probe.Payload"   -> THREW           HEAD "com.a.A, probe.Payload" -> INSTANTIATED
HEAD addAutoTypeDeny(" probe.Payload ") -> stored [" probe.Payload "], checkAutoType -> NO-THROW -> resolved
HEAD addAutoTypeDeny("probe.Payload")   -> stored ["probe.Payload"],   checkAutoType -> THREW  (control live)

Decide what a deny key is and enforce it once, at registration and at lookup alike: (a) trim here — String denyName = normalizeAcceptName(name.trim()); plus an isEmpty() return; (b) either implement prefix matching by mirroring the accept rolling-hash scan, or keep exact-name-only and delete the 1.x parity claim, saying explicitly that a package deny must list each FQCN and rejecting or logging a prefix-shaped argument at registration so the no-op is not silent; (c) normalize the descriptor and array spellings before hashing (strip leading [s, a surrounding L…;, and trailing []) and extend the :908 recursion with an endsWith("[]") arm — measured, the recursion arm closes both the programmatic list and the hardcoded table for the suffix family, while an unwrap at the deny-hash site alone closes only the programmatic list, and the bare L…; spelling still needs its own unwrap. Comparing the resolved class name after loadClass/getMapping would cover aliases too.

Three constraints, all measured. Fnv.hashCode64(String) (Fnv.java:135-158) takes a packed-little-endian fast path for name.length() <= 8 that is not the FNV-1a rolling value — "com.evil" is exactly 8 chars, on the boundary — so a prefix scan must hash deny entries with the rolling recurrence used at :941-942 or short entries silently stop matching. ObjectReaderProvider.java:211-214 — a hash match is only honoured when the matched prefix text is in the name set, so a prefix deny must keep the denyNameSet.contains(prefix) verification or a hash collision starts denying unrelated types. And the trim must go here, not in normalizeAcceptName: TypeUtils.java:2858-2860 has 7 main-source call sites (:238, :305, :342, :921, :945, :980 and ContextAutoTypeBeforeHandler:226, :270), so trimming there would re-key every existing accept entry and widen autoType acceptance.

Acceptance: four assertions, each red today — addAutoTypeDeny("com.acme") then checkAutoType("com.acme.Gadget", null, SupportAutoType.mask) throws; addAutoTypeDeny("java.util.Currency") then the same for "java.util.Currency[]", "Ljava.util.Currency;", "[Ljava.util.Currency;" and "[[Ljava.util.Currency;"; addAutoTypeDeny(" com.acme.Padded ") then checkAutoType("com.acme.Padded", ...) throws; and a compat-side regression using new ParserConfig() with DENY_PROPERTY="com.a.A, com.b.B" asserting both names throw. Please put these on a private provider — the existing test class mutates the JVM-global default provider.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)


// publish the name before the hash, so that a reader seeing the new hash array is

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-16: addAutoTypeDeny is a near-verbatim copy of addAutoTypeAccept immediately above it (:303-326) — same normalize, same copy-on-write set publish, same grow-copy-sort, down to this comment, which appears word-for-word in both. The copy is wrong for a deny list in two ways.

First, the sorted long[] is redundant state here. The accept list needs its long[] because it is scanned inside a per-character rolling-hash loop (:934-963, :966-995) where a Set lookup per character would be expensive; the deny list's single consumer at :921-925 already holds the full normalized name and does Arrays.binarySearch(denyHashCodes, denyHash) >= 0 && denyNameSet.contains(normalizedDenyName) — an && in which the HashSet.contains is authoritative and O(1), so the array only pre-filters a lookup that needs no pre-filter. Second, this comment names the accept-side risk. For an accept list, publishing name-before-hash fails closed (a reader with new hashes and an old name set transiently rejects). For a deny list the same intermediate state makes binarySearch miss, the conjunction short-circuits, and the just-denied type transiently resolves — fail-open. A maintainer who reads this as "the ordering avoids spurious rejections" treats it as a nicety and can merge the two volatile writes, opening a window whose real consequence the comment never states. That window is reasoned rather than demonstrated — observing a nanosecond-scale gap between two volatile writes needs a deliberately widened mutant, which would measure the edit rather than this PR — but note that reversing to hash-first does not close it either; only collapsing both fields into one immutable holder behind a single volatile reference does.

Witness:

SWEEP — every reference to denyHashCodes/denyNameSet in core/src/main/java:
  :223 :224 declarations | :346 :347 :349 :353 :359 writes inside addAutoTypeDeny
  :923 :924 — ONE read site, and it is the conjunction
identical comment text at :308-309 (addAutoTypeAccept) and :344-345 (addAutoTypeDeny)
test coverage: accept side = 12 refs in ObjectReaderProviderBugFixTest
  (testAddAutoTypeAccept_concurrent 8 threads x 100, testAddAutoTypeAccept_idempotent)
deny side = 2 single-threaded assertions on com.acme.* and NO concurrent/idempotency equivalent anywhere

Delete denyHashCodes and gate solely on denyNameSet.contains(normalizeAcceptName(typeName)) at :921-925, dropping the hash computation and the array rebuild — one volatile field, one publication, no ordering window, no sort. If the two-field shape is kept for future prefix matching, extract shared private static helpers so both methods carry one copy of the publication invariant, and reword this comment for the deny direction.

Constraint: the accept-side ordering at :307-308 is load-bearing, so a single-field collapse must be applied to the deny pair only, not to acceptHashCodes/acceptNameSet.

Acceptance: testAddAutoTypeDenyEnforced and testDenyNormalisationDollarToDot stay green after the array is removed (no test pins denyHashCodes); for the extraction route, the existing ObjectReaderProviderBugFixTest.testAddAutoTypeAccept_idempotent / testAddAutoTypeAccept_concurrent must also stay green.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

// guaranteed to see the name it verifies against rather than transiently rejecting
if (!this.denyNameSet.contains(denyName)) {
Set<String> names = new HashSet<>(this.denyNameSet);
names.add(denyName);
this.denyNameSet = Collections.unmodifiableSet(names);
}

long hash = Fnv.hashCode64(denyName);
long[] current = this.denyHashCodes;
if (Arrays.binarySearch(current, hash) < 0) {
long[] hashCodes = new long[current.length + 1];
hashCodes[hashCodes.length - 1] = hash;
System.arraycopy(current, 0, hashCodes, 0, current.length);
Arrays.sort(hashCodes);
this.denyHashCodes = hashCodes;
}
}

/**
Expand Down Expand Up @@ -858,6 +915,16 @@ public Class<?> checkAutoType(String typeName, Class<?> expectClass, long featur
return expectClass;
}

// Programmatic deny list check. Run BEFORE any allow-list rolling-hash scan or loadClass

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-6: [certifies-falsely] [new-surface] the programmatic deny list is read at exactly ONE place in all of core/src/main/java — this block — and the name-form policy has no owner, so four separate routes resolve a denied type without ever consulting it. The guarantee the javadoc 200 lines above adds ("rejected by checkAutoType regardless of SupportAutoType or any explicit accept entry") and this comment's "a denied type never triggers a Class.forName on attacker input" are both false.

This is a class-level finding rather than four separate ones, because the surface is not closable entrance by entrance — five successive audit passes each found another route. The root is that the policy is one inline block in one method instead of one owner consulted at every route that turns a name into a Class.

The four measured entrances:

  1. Handler / filter routes. checkAutoType returns the handler's class at :884-890, 34 lines above this block; JSONReader.Context.getObjectReaderAutoType (JSONReader.java:5664-5675, :5693-5705) hands that class straight to provider.getObjectReader and never calls checkAutoType; ObjectReaderBean.checkAutoType0:177-193 resolves through the context filter and reaches the provider only in the else; JSONReaderJSONB.checkAutoTypeWithHandler:1332-1343 has the same shape; and the shipped ContextAutoTypeBeforeHandler.apply consults only the hardcoded table (:290), never denyNameSet. So the standard way to enable autoType is also the standard way to bypass this list.
  2. The filter caches a class it then rejects. ContextAutoTypeBeforeHandler.apply(String,…) calls putCacheIfAbsent(typeNameHash, clazz) at :279 before the rejection at :290-291, and its hash-keyed sibling apply(long,…) at :237-248 is a pure predicate-free cache read. ObjectReaderImplClass:39 is the only caller of that hash entry point in the whole repo's src/main, and its :45 return filterClass; exits before both checks this PR adds.
  3. The mapping exit. ObjectReaderImplClass.readObject returns at TypeUtils.getMapping(className) (:70-73) before checkAutoType is ever called. This one needs no handler and no filter — it fires on a default-configured provider.
  4. Enforcement altitude. The pre-load name check exists only in ObjectReaderImplClass, so every other checkAutoType caller still resolves a hardcoded-denied name through ClassLoader.loadClass before rejecting it at :1024.

Witness:

# (1) the deny entry fires on the direct path and is invisible on the filter path:
r16-direct  BASE: OK: class com.acme.Gadget   HEAD: THROW autoType is not support. com.acme.Gadget
r16-filter  BASE: OK: Gadget{pwn}             HEAD: OK: Gadget{pwn}   <- denied class instantiated AND populated
r16-handler BASE: OK: Gadget{pwn}             HEAD: OK: Gadget{pwn}
r16-hardcoded-filter (autoTypeFilter("java.net.") + @type java.net.URL)
            BASE: THROW read URL error        HEAD: THROW autoType is not support. java.net.URL
# -> one PR ships two deny lists with different reachability

# (2) request 1 and request 2 are the SAME payload — the rejecting call poisons the cache itself:
HEAD apply("java.net.InetSocketAddress", null, 0)             = null
HEAD apply(Fnv.hashCode64("java.net.InetSocketAddress"), ...) = class java.net.InetSocketAddress
HEAD request1 -> THREW autoType is not support : java.net.InetSocketAddress
HEAD request2 -> bean.clazz = class java.net.InetSocketAddress

# (3) no handler, no filter, default provider:
A {"@type":"java.util.HashMap"} , SupportAutoType    : THREW autoType is not support. java.util.HashMap
B {"clazz":"java.util.HashMap"}, SupportClassForName : RESOLVED -> Bean{clazz=class java.util.HashMap}

# (4) recording context ClassLoader, same provider, same JVM:
hardcoded deny java.io.File      -> THREW | contextClassLoader.loadClass calls: [java.io.File]
hardcoded deny TemplatesImpl     -> THREW | loadClass calls: [com.sun...TemplatesImpl]
programmatic deny com.acme.Gadget -> THREW | loadClass calls: []

Close it structurally rather than route by route: add boolean isAutoTypeDenyName(String typeName) on ObjectReaderProvider (mirroring :921-923) and a name-based public static boolean isAutoTypeDenyName(String) in JDKUtils for the hardcoded table, then consult them at the top of checkAutoType (before the autoTypeBeforeHandler early return), in both JSONReader.Context.getObjectReaderAutoType overloads before honouring a handler result, in ObjectReaderBean.checkAutoType0, in JSONReaderJSONB.checkAutoTypeWithHandler, and in ObjectReaderImplClass.readObject before both the typeFilter exit at :45-47 and the getMapping exit at :70-73. Separately, move putCacheIfAbsent below the rejection in ContextAutoTypeBeforeHandler.apply(String,…) so only the return clazz branch caches; and call the JDKUtils name predicate before clazz = loadClass(typeName) at :1021 so the pre-load guarantee holds for every caller.

Two constraints that were measured, so please do not skip them. First, do not reorder autoTypeBeforeHandler against SAFE_MODE, and do not add a blanket deny check inside ContextAutoTypeBeforeHandler.apply(long,…): safemode-test/.../Issue1503S.java:18-23 pins that an explicit filter resolving an accepted type (including a $ nested subclass written by WriteClassName) must work — a trusted handler running before safeMode is a design contract here, not a bug. Measured, moving the cache publish below the rejection leaves all three accept contracts byte-identical, while a blanket deny check inside apply(long,…) flattens them (it returned null for an accept entry naming the type in full, because it has no i + 1 < typeNameLength gate and cannot see one). Second, the getMapping fix must not reorder getMapping behind checkAutoType, which would break the non-FQCN alias keys ("HashMap", "JO10", "[O", "[Object", "StackTraceElement" — TypeUtils.java:1792, :1797-1803).

Acceptance: three tests, each red today — (a) addAutoTypeDeny("com.acme.Gadget") + JSONReader.autoTypeFilter("com.acme.") + SupportAutoType must throw rather than return an instance; (b) ContextAutoTypeBeforeHandler f = new ContextAutoTypeBeforeHandler("java.net.") → assertNull(f.apply("java.net.InetSocketAddress", null, 0)) then assertNull(f.apply(Fnv.hashCode64("java.net.InetSocketAddress"), null, 0)), where the second assertion is the one that reds; (c) on a fresh provider, addAutoTypeDeny("java.util.HashMap") then {"clazz":"java.util.HashMap"} into a Class<?> field under SupportClassForName must throw, with a control asserting the same payload resolves when the deny is absent.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

// call so a denied type never triggers a Class.forName on attacker input. The name is
// normalized the same way as acceptNameSet so $ ↔ . rewrites don't smuggle past.
String normalizedDenyName = normalizeAcceptName(typeName);

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-18: this block runs unconditionally on every checkAutoType call, but for every user who never calls addAutoTypeDeny and never sets fastjson2.parser.deny — the default — denyHashCodes is new long[0] (:223) and the branch can never fire, so all the work it does to reach that conclusion is waste.

It sits above if (!autoTypeSupport) { return null; } at :1002, so it runs for every @type in untrusted input even with autoType off, and array type names recurse at :908-911 and pay it twice. Per call: normalizeAcceptName is an O(len) scan plus an allocation and a second O(len) copy whenever the name contains $ (the normal form for every nested class), then Fnv.hashCode64 is another O(len) pass with a 64-bit multiply per char, then a volatile read and a binarySearch that returns -1 immediately. When the list is non-empty the hash is computed twice for the same characters, since it equals the value the accept rolling loop below reaches after its final iteration.

Witness:

checkAutoType microbenchmark, autoType OFF (the default), denyHashCodes empty, 100k warmup + median of 7:
  name                              INTACT   BLOCK REVERTED   delta
  com.example.service.OrderDto       53.4        35.6        +17.8 ns/op (+50%)
  com.example.service.OrderDto$Key   83.8        50.8        +33.0 ns/op (+65%)
arm proofs (denyHashCodes refs in checkAutoType): intact=1, reverted=0, restored=1
CANDIDATE FIX, RUN not hypothesised (one volatile read into a local + a length guard):
  53.4 -> 38.8   and   83.8 -> 44.8 ns/op
behaviour preserved: both normalization directions still DENIED, control still DENIED,
  ObjectReaderProviderTest Tests run: 3, Failures: 0
on the polymorphic parse path, isolated by a third arm: +12.0 ns/object (+4.4%)

Read the volatile once into a local and short-circuit before touching the name:

long[] denies = this.denyHashCodes; // one volatile read
if (denies.length != 0) {
    String normalizedDenyName = normalizeAcceptName(typeName);
    if (Arrays.binarySearch(denies, Fnv.hashCode64(normalizedDenyName)) >= 0
            && denyNameSet.contains(normalizedDenyName)) {
        throw new JSONException("autoType is not support. " + typeName);
    }
}

Keeping the check ahead of the accept scan and of loadClass is required — only the empty-list fast path changes. As a side effect, snapshotting the volatile also makes the binarySearch and the contains operate against one consistent array, which narrows the publication window noted on :344.

Constraint: denyHashCodes is volatile and republished as a whole new sorted array by the synchronized addAutoTypeDeny (:337-360), so the guard must snapshot it into a local rather than re-reading the field twice. Note acceptHashCodes is not empty by default (the initializer at :245-249 always seeds ANTI_COLLISION_HASH_MAP_HASH), so the same fast path is not available on the accept side.

Acceptance: testAddAutoTypeDenyEnforced and testDenyNormalisationDollarToDot must still throw — they go red if the guard is inverted or the snapshot is taken before addAutoTypeDeny publishes. Nothing pins the empty-list fast path itself, so please add an assertion that checkAutoType still returns null (not throws) for an arbitrary name on a fresh provider.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

long denyHash = Fnv.hashCode64(normalizedDenyName);
if (Arrays.binarySearch(denyHashCodes, denyHash) >= 0
&& denyNameSet.contains(normalizedDenyName)) {
throw new JSONException("autoType is not support. " + typeName);
}

boolean autoTypeSupport = (features & JSONReader.Feature.SupportAutoType.mask) != 0;

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-22: the deny block sits above this autoTypeSupport computation, so a denied @type throws out of the provider even when autoType resolution is disabled and the caller has explicitly configured @type to be ignored.

That contradicts the standing policy this same method documents at :901-906 — "treat it as unresolvable rather than an error, the same as a type name that fails to load: JSON-LD uses @type for an IRI, and reporting that as unresolved leaves the ErrorOnNotSupportAutoType feature in charge of whether the caller sees an exception" — and it is asymmetric with the hardcoded veto at :1024, which is unreachable when autoType is off because :1002 returns null first. So only the new list behaves this way. The sharper measured cost is a deny-list membership oracle: a denied name throws while every other name is absorbed silently, so a remote attacker can enumerate an operator's deny list one name at a time on an endpoint that never enabled autoType.

Witness (same probe compiled once, run unchanged on both arms, deny registered, autoType OFF):

                                          BASE 2.0.65                  PR head
E3 parseObject(str, Object.class)         JSONObject {"@type":...}     THREW autoType is not support. com.acme.Legacy
E5 bean with Object field                 Holder {...}                 THREW
E6 JSONReader.readAny()                   JSONObject {...}             THREW
E7 getObjectReader(name,null,0)           null                         THREW
E8 checkAutoType(name,null,0)             null                         THREW
E9 ErrorOnNotSupportAutoType              THREW "autoType not support : ... offset 28"  THREW (caller pre-empted)
E10 "[com.acme.Legacy" component          JSONObject                   THREW
E1/E2 parseObject(str) / parse(str)       JSONObject                   JSONObject  (identical — does NOT reproduce)
R8 checkAutoType("java.lang.Runtime",null,0)  returned null            returned null  (hardcoded veto silent, autoType off)
FIX APPLIED (hoist autoTypeSupport, return null when off), rebuilt:
  E3/E5/E6/E10 back to BASE byte-for-byte, E7/E8 back to null, E9 back to the caller's own message and offset,
  while every shape ObjectReaderProviderTest pins (SupportAutoType.mask at :38, :54, :70) still throws

Compute autoTypeSupport before the deny block and make the autoType-off case unresolvable rather than fatal, matching :901-906: when the deny matches and !autoTypeSupport, return null and let ErrorOnNotSupportAutoType decide, exactly as with any unresolved name; keep the throw for the autoType-on case.

Two corrections so this is reproducible: plain JSON.parseObject(str) / JSON.parse(str) does not throw (the JSONObject-target path keeps @type as an ordinary key and never consults the provider) — the throwing entry points are parseObject(str, Object.class), a bean with an Object-typed field, readAny() and the two direct provider calls; and the :901-906 policy comment is pre-existing (verbatim in the released 2.0.65 sources jar), not added by this diff. The addAutoTypeDeny javadoc does say "regardless of SupportAutoType", so if throwing here is deliberate, please say so in the javadoc and accept the oracle — but then the hardcoded table 100 lines below should behave the same way, and it does not.

Constraint: ObjectReaderImplObject.java:76-80 (if (!supportAutoType && autoTypeObjectReader == null && context.isEnabled(JSONReader.Feature.ErrorOnNotSupportAutoType))) is the caller that owns this policy, so the fix must return null rather than move the decision into the provider.

Acceptance: a new case asserting checkAutoType("com.acme.Gadget", null, 0) returns null (not throws) on a fresh provider after addAutoTypeDeny("com.acme.Gadget"), plus an end-to-end parse of {"@type":"com.acme.Gadget","id":1} to Object.class asserting the id survives. Both go red if the deny block moves back above this line — nothing pins the autoType-off behaviour today, since all three existing tests pass SupportAutoType.mask.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

Class<?> clazz;

Expand Down Expand Up @@ -1535,9 +1602,8 @@ public PropertyNamingStrategy getNamingStrategy() {
* Sets the property naming strategy used by this provider.
*
* @param namingStrategy the property naming strategy to set
* @since 2.0.52
*/
public void setNamingStrategy(PropertyNamingStrategy namingStrategy) {
this.namingStrategy = namingStrategy;
}
}
}
Comment on lines 1607 to +1609

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-3: four files in this PR do not end with a newline, and this hunk removed the trailing LF ObjectReaderProvider.java previously had — which fails the project's own checkstyle gate, so mvn validate fails before anything compiles.

src/checkstyle/fastjson2-checks.xml:7-9 declares NewlineAtEndOfFile with lineSeparator=lf at Checker level, and pom.xml:575-584 binds checkstyle:check to the validate phase with failOnViolation=true and includeTestSourceDirectory=true. None of the four paths is in the plugin's <excludes> list. AGENTS.md names mvn validate as a required gate. Because the failure is reported by the style plugin rather than the compiler, an engineer sees "checkstyle violation" on a security PR and has to work out that a missing byte at EOF broke the build.

Witness:

[ERROR] src/main/java/com/alibaba/fastjson2/reader/ObjectReaderProvider.java:[1] (misc) NewlineAtEndOfFile: File does not end with a newline.
[ERROR] src/test/java/com/alibaba/fastjson2/reader/ObjectReaderProviderTest.java:[1] (misc) NewlineAtEndOfFile: File does not end with a newline.
[ERROR] src/test/java/com/alibaba/fastjson2/reader/ObjectReaderImplClassTest.java:[1] (misc) NewlineAtEndOfFile: File does not end with a newline.
[ERROR] src/test/java/com/alibaba/fastjson2/util/JDKUtilsDenyListTest.java:[1] (misc) NewlineAtEndOfFile: File does not end with a newline.
[ERROR] You have 4 Checkstyle violations.
# -Dcheckstyle.skip=true does NOT help: pom.xml:581 hardcodes <skip>false</skip> (measured, identical failure)
# after appending the four newlines:  mvn -o -pl core validate -> BUILD SUCCESS, "You have 0 Checkstyle violations"

Append a single LF to the end of all four files. JDKUtils.java and ObjectReaderImplClass.java already end correctly.

Constraint: the separator must be LF, not CRLF (fastjson2-checks.xml:8 pins lineSeparator=lf, and :10-13 separately reject any \r), and :20-23 rejects \n\n\Z — so each file must end with exactly one LF after }, not a trailing blank line.

Acceptance: mvn validate goes from 4 violations to 0; no unit test can pin a byte.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

146 changes: 141 additions & 5 deletions core/src/main/java/com/alibaba/fastjson2/util/JDKUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,118 @@ public class JDKUtils {
static volatile Throwable reflectErrorLast;
static final AtomicInteger reflectErrorCount = new AtomicInteger();

/**
* Hardcoded FQCN-based deny list for {@link #isAutoTypeDenyClass(Class)}. These classes are
* inherently unsafe when reachable via {@code @type} because their static initializers /
* setters / class-loading behaviour can lead to RCE, SSRF, or LFD even when no further
* gadget-chain setter is involved. Each {@code Class.forName} probe is wrapped in try/catch so
* an unloaded or absent module degrades to a silent skip rather than failing the whole class
* init of {@code JDKUtils}.
*/
static final String[] AUTO_TYPE_DENY_FQCN = new String[] {

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-21: the security policy table is published as a mutable String[]. static final fixes the reference only, so every element is writable — and both consumers compare with name.equals(entry), which is false for null, so an overwritten or nulled entry silently removes that class from both gates with no exception, no log, and a length that still reads 80.

Two routes, both real. (1) On a classpath (non-JPMS) build any jar may declare a class in com.alibaba.fastjson2.util and gain write access; the new JDKUtilsDenyListTest already statically imports the raw array and iterates it. (2) The compile error in ObjectReaderImplClass forces either widening this field to public or adding an accessor — and if it is widened, the entire classpath gets compile-time element write access. Either way the deny gate is defeatable at runtime with no diagnostic, and the table's integrity is unenforceable. This is why "just add public" is the wrong resolution of the build break.

Witness:

probe against JDKUtils compiled EXACTLY as submitted (field modifiers = static final),
from a class placed in com.alibaba.fastjson2.util (classpath split package):
  --- gate BEFORE mutation ---            --- AFTER AUTO_TYPE_DENY_FQCN[0]=null; [8]=null (no exception, no log) ---
  isAutoTypeDenyClass(Runtime) = true     isAutoTypeDenyClass(Runtime) = false
  isAutoTypeDenyClass(File)    = true     isAutoTypeDenyClass(File)    = false
  bare string -> Class target = THREW     bare string -> Class target = RESOLVED class java.lang.Runtime
  bean Class<?> field         = THREW     bean Class<?> field         = RESOLVED ...Holder@e2d56bf
  --- AFTER restore ---  isAutoTypeDenyClass(Runtime) = true ; bare string -> THREW   (probe flips both ways)
minimal resolution of the build break -> probe reports `field modifiers = public static final`
core/src/main/moditect/module-info.java: `open module com.alibaba.fastjson2` + `exports com.alibaba.fastjson2.util`

Keep the array private static final and expose the decision, not the data — a public static boolean isAutoTypeDenyName(String) backed by a precomputed immutable Set<String> (or a sorted FNV long[] plus a name set, the shape this PR already uses for denyHashCodes). That fixes the cross-package build break and the linear scan in the same move, and leaves nothing element-writable.

This stays a Suggestion rather than higher because the writer must already be executing code in the JVM (a split-package jar, or reflection via the open directive), an actor this table's threat model — untrusted JSON — does not cover. The actionable defect is that a security table's integrity is unenforceable and its violation is invisible.

Constraint: JDKUtilsDenyListTest.java:6 static-imports the raw array and :20 iterates it, so a predicate/Set replacement must update that test in the same change. Note that test's self-reference is part of the problem — its only in-loop assertion is assertTrue, so emptying the array makes it pass vacuously.

Acceptance: an assertion that the published table is immutable (e.g. assertThrows(UnsupportedOperationException.class, () -> JDKUtils.autoTypeDenyFqcn().set(0, null)) against an unmodifiable List view), plus the existing testIsAutoTypeDenyClass_ExactFQCN re-pointed at the accessor so it still pins "every entry is denied, Object/String are not".

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

"java.lang.Runtime",
"java.lang.Process",
"java.lang.ProcessBuilder",
"java.lang.System",
"java.lang.Thread",
"java.lang.ClassLoader",
"java.lang.Shutdown",
"java.lang.Class",

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-5: the table contains ordinary supported value types — java.lang.Class here, plus java.io.File, java.net.URL, java.net.URI, java.lang.Thread, java.lang.System, java.net.InetAddress, java.net.InetSocketAddress, javax.imageio.ImageIO — and isAutoTypeDenyClass is consulted by an unconditional veto at ObjectReaderProvider.java:1024, unlike the two sibling sites at :952/:986 which guard on i + 1 < typeNameLength. So these names stop resolving through autoType altogether, and an existing untouched test breaks.

AutoTypeTest15_noneStringKey.test_1 writes a LinkedHashMap keyed by String.class with WriteClassName and reads it back with SupportAutoType | FieldBased | SupportClassForName — i.e. any user payload whose map keys or field values are Class objects, written by fastjson2 itself, can no longer be read back after this change. File / URL / URI also have dedicated built-in readers (ObjectReaderBaseModule.java:1671/1682/1693) and writers, so a WriteClassName payload carrying them as @type now fails to deserialize with autoType is not support and no hint that a new hardcoded list is the cause. On the accept-prefix path the same widening degrades silently instead of throwing: :952/:986 and ContextAutoTypeBeforeHandler.java:290 continue past the match, so an app running JSONReader.autoTypeFilter("java.net.") gets a JSONObject where it used to get a java.net.URL.

Witness:

[ERROR] AutoTypeTest15_noneStringKey.test_1:41 » JSON autoType is not support. java.lang.Class
# causality measured, not assumed: deleting ONLY the "java.lang.Class" entry, nothing else
-> AutoTypeTest15_noneStringKey 3/3 GREEN    (net-new from this diff; reproduced on JDK 8, 17 and 21)

BASE isAutoTypeDenyClass(Class.class) = false   /   HEAD = true
BASE Map<Class,String> JSONB round-trip = OK -> {class java.lang.String=S}
HEAD Map<Class,String> JSONB round-trip = THROWN JSONException: autoType is not support. java.lang.Class
BASE JSON.toJSONString(mapWithThread, WriteClassName) round-trip = OK   /   HEAD = THROWN
BASE {"clazz":"java.net.URL"} + SupportClassForName = OK                /   HEAD = THROWN
# the failing file is NOT touched by this diff

java.lang.Class in particular is not a gadget sink by itself: resolving it is already gated by SupportClassForName plus the checks in ObjectReaderImplClass, and the genuinely dangerous part is the val it carries. Please restrict the table to names that are never legitimate deserialization targets — gadget entry points and code loaders (Runtime, ProcessBuilder, TemplatesImpl, JdbcRowSetImpl, InvokerTransformer, URLClassLoader, XMLDecoder, InitialContext, ScriptEngineManager, ...) — and drop the plain value types. If blocking java.lang.Class as a @type target is deliberate, it needs an exemption for the WriteClassName / SupportClassForName round-trip this test pins, and ObjectWriterImplClass must stop emitting a type name the reader refuses — otherwise previously-writable data becomes unreadable.

Constraint: AutoTypeValidationTest.java:264-289 requires isAutoTypeDenyClass to stay true for ClassLoader / DataSource subtypes so a prefix accept does not resolve them, while a full-name accept does — any narrowing must not weaken the ClassLoader.class.isAssignableFrom(type) || isSQLDataSourceOrRowSet(type) operand at :592. And ObjectReaderBaseModule.java:1682 / :1693 register first-class readers for File and URL, so their @type names must stay resolvable.

Acceptance: AutoTypeTest15_noneStringKey.test_1 goes green (measured red with the entry present, green with it removed), plus a round-trip test for a Map holding a Thread / File / URL serialized with WriteClassName and parsed back with SupportAutoType.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

"java.io.File",
"java.io.FileInputStream",
"java.io.FileOutputStream",
"java.io.ObjectInputStream",
"java.io.ObjectOutputStream",
"java.io.RandomAccessFile",
"java.net.URL",
"java.net.URI",
"java.net.URLClassLoader",
"java.net.InetAddress",

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] R1-8: [certifies-falsely] [new-surface] this is a class-level finding about the table's shape, not about any one entry. The @type entrance space is every class name loadable from the consuming application's classpath — unbounded — so a hand-enumerated exact-FQCN table over it has no last corner: each gadget, each subclass and each renamed third-party class needs another source edit and another library release. Meanwhile the two families this very method already handles structurally (ClassLoader.class.isAssignableFrom, isSQLDataSourceOrRowSet, :592) are closable.

The javadoc below states the gap outright — "The match is done by exact FQCN — subclasses of these types are NOT auto-banned" — and that is a real argument against isAssignableFrom. What it does not answer is that several entries are inert while the usable spelling sails through.

Witness:

r10-inet       @type java.net.InetAddress   BASE: THROW create object error  HEAD: THROW autoType is not support
r10-inet4      @type java.net.Inet4Address  BASE: OK: /0.0.0.0 (Inet4Address) HEAD: OK: /0.0.0.0 (Inet4Address)
r10-initialctx @type javax.naming.InitialContext           BASE: OK   HEAD: THROW
r10-dirctx     @type javax.naming.directory.InitialDirContext  BASE: OK   HEAD: OK (constructed)
r10-runtime-arr-suffix checkAutoType("java.lang.Runtime[]")   BASE: OK: class [Ljava.lang.Runtime;  HEAD: OK (same)
r10-runtime-arr-prefix checkAutoType("[Ljava.lang.Runtime;")  BASE: OK   HEAD: THROW
# hierarchy mutation: swapping the string loop for isAssignableFrom over AUTO_TYPE_DENY_CLASSES leaves
# JDKUtilsDenyListTest UNCHANGED while isAutoTypeDenyClass(java.net.Inet4Address) flips false -> true

java.net.InetAddress is abstract with no public constructor — base's own failure on it is create object error, so it was never a usable payload — while the resolvable forms Inet4Address / Inet6Address are instantiated on head. That is not a hypothetical spelling: this repo already treats it as a gadget payload at fastjson1-compatible/src/test/java/com/alibaba/fastjson/v2issues/Issue530.java:12 — {"@type":"java.net.Inet4Address","val":"dnslog"}. Same family: InitialDirContext is constructed while InitialContext is denied; org.apache.commons.collections4.functors.InvokerTransformer is listed while its chain partners InstantiateTransformer and TransformingComparator are not; and one type has two spellings with opposite verdicts (java.lang.Runtime[] resolves, [Ljava.lang.Runtime; throws). To be explicit about what is not claimed: no weaponised outcome — val is ignored on the Inet4Address path (any value yields /0.0.0.0) and the InitialDirContext constructed is the no-arg one. The demonstrated cost is a false guarantee plus a one-token payload rename that steps around the filter.

Close the class, not the corner: (1) match by hierarchy where the supertype is the danger and is itself unusable in application code — the shape already at :592 — adding javax.naming.Context, java.lang.Process and java.net.InetAddress as supertype tests rather than leaf names; (2) normalize array/descriptor spellings to the component type before comparing (Class<?> t = type; while (t.isArray()) { t = t.getComponentType(); }), which keeps exact-FQCN semantics — an array type is not a subclass — and closes the two-spelling inconsistency at all five call sites; (3) keep the fail-closed default (no accept entry ⇒ no resolution) as the real gate for everything else instead of growing the leaf list; (4) move the third-party gadget names out of library source into the channel this same PR wires up (fastjson2.parser.deny / addAutoTypeDeny), which an operator can update without waiting for a release; (5) drop entries the deserializer cannot instantiate at all and that therefore carry only compatibility cost — java.lang.Process, java.lang.System, java.lang.Shutdown, java.lang.ClassLoader (already covered by isAssignableFrom), javax.imageio.ImageIO, org.apache.commons.io.IOUtils, freemarker.cache.TemplateLoader (an interface), javassist.CtClass, and the abstract java.net.InetAddress.

Constraint: a hierarchy rule must be scoped to families where the supertype is itself unusable in application code (InetAddress, Context, Process), not applied to entries like java.io.File or java.lang.Thread whose subclasses are ordinary application types. And two existing tests bound it — Issue530.java:12-13 asserts {"@type":"java.net.Inet4Address","val":"dnslog"} degrades to JSONObject under fastjson1-compatible defaults, and AutoTypeValidationTest.java:264-289 pins the prefix-vs-full-name accept distinction.

Acceptance: assertTrue(JDKUtils.isAutoTypeDenyClass(java.net.Inet4Address.class)) and the InitialDirContext analogue, plus assertTrue(JDKUtils.isAutoTypeDenyClass(Runtime[].class)), plus an end-to-end assertThrows on the Inet4Address dnslog payload — all red today.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

"java.net.InetSocketAddress",
"java.net.Socket",
"java.net.ServerSocket",
"java.net.DatagramSocket",
"java.nio.channels.SocketChannel",
"java.nio.channels.ServerSocketChannel",
"java.rmi.server.UnicastRemoteObject",
"java.rmi.activation.Activator",
"java.beans.XMLDecoder",
"javax.naming.InitialContext",
"javax.script.ScriptEngineManager",
"javax.management.remote.rmi.RMIConnector",
"javax.management.remote.JMXServiceURL",
"javax.imageio.ImageIO",
"javax.activation.MimeType",
"javax.sound.sampled.AudioSystem",
"javax.sound.midi.MidiSystem",
"sun.print.PrintServiceLookup",
"com.sun.rowset.JdbcRowSetImpl",
"com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl",
"com.sun.org.apache.xpath.internal.jaxp.XPathFactory",
"com.sun.org.apache.bcel.internal.util.ClassLoader",
"com.sun.org.apache.jndi.ldap.LdapCtx",

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-24: three of the 80 entries name classes that do not exist on any JDK, so those gates can never fire. Both consumers are pure string gates — className.equals(...) on raw JSON text and name.equals(...) on type.getName() — and a name no class has can never equal either side.

This one is the LDAP JNDI context: the package com.sun.org.apache.jndi has never existed; the real class is com.sun.jndi.ldap.LdapCtx. The other two are com.sun.org.apache.xpath.internal.jaxp.XPathFactory (the real class in that package is XPathFactoryImpl) at :118 and sun.print.PrintServiceLookup (the real class is javax.print.PrintServiceLookup) at :115. The cost is a security table that asserts coverage it does not have, invisibly — an auditor reading it cannot tell. Secondarily, each of the three throws and fills a ClassNotFoundException stack inside the eager <clinit> sweep on every JVM start, buying nothing.

Witness:

arm proof: grep -c 'com.sun.org.apache.jndi.ldap.LdapCtx' JDKUtils.class -> PR=1, BASE=0
BASE com.sun.jndi.ldap.LdapCtx checkAutoType RETURNED | isAutoTypeDenyClass=false
PR   com.sun.jndi.ldap.LdapCtx checkAutoType RETURNED | isAutoTypeDenyClass=false   <- byte-identical to base
PR   controls (comparator live): TemplatesImpl THREW, java.lang.Runtime THREW, javax.naming.InitialContext THREW
FIXED arm (3 strings renamed): all three THREW, isAutoTypeDenyClass(...) = true — same flip on JDK 17.0.19 and 25.0.3
authority, Class.forName on FIVE runtimes (Zulu 8u492, 11.0.31, 17.0.19, 21.0.11, 25.0.3):
  all three submitted spellings -> ClassNotFoundException on all five
  all three corrected spellings -> RESOLVED on all five   (positive control: java.lang.Object = 1 hit in all five)
runtime images: rt.jar holds 0 entries under com/sun/org/apache/jndi/;
  com/sun/org/apache/xpath/internal/jaxp/ is exactly {JAXPExtensionsProvider, JAXPPrefixResolver, JAXPVariableStack,
  XPathExpressionImpl, XPathFactoryImpl, XPathImpl}; sun/print/ has only PrintServiceLookupProvider
  while javax/print/PrintServiceLookup.class is present
WHOLE-TABLE SWEEP (80 entries parsed out of the source by regex, never retyped; 80 unique, 0 duplicates):
  41 of 80 absent from every measured JDK image -> 32 verified to exist in a real ~/.m2 artifact
  (oracle = zip central directory of 2,773 jars, 198,225 class names), 6 unverifiable locally (no artifact at all),
  and EXACTLY 3 JDK-internal names no measured JDK ever shipped. No fourth typo.

Not claimed: a weaponised outcome. LdapCtx has no no-arg constructor (javap shows only LdapCtx(String, String, int, Hashtable, boolean) and a package-private copy constructor), PrintServiceLookup is abstract, and the actual JNDI entry point javax.naming.InitialContext is refused on head.

Suggested change
"com.sun.org.apache.jndi.ldap.LdapCtx",
"com.sun.jndi.ldap.LdapCtx",

and likewise "com.sun.org.apache.xpath.internal.jaxp.XPathFactoryImpl" and "javax.print.PrintServiceLookup" — or drop the entries you cannot name a threat for. Name-only correction is sufficient and does not depend on the AUTO_TYPE_DENY_CLASSES sweep, since both gates compare strings.

Constraint, measured: the obvious witness does not work as first written. A platform-prefix "must resolve" assertion excluding only Activator and MimeType stays RED after the rename on every JDK — the exclusion set must be 6 entries, adding the three javax.faces.context.* names (Java EE/Jakarta EE API classes no JDK ever shipped) plus com.sun.org.apache.bcel.internal.util.ClassLoader on JDK 11+ and java.rmi.activation.Activator on JDK 17+. Note Activator was not removed in JDK 11 — measured present and resolvable in 11.0.31, absent by 17.0.19, so removal is bounded to (11, 17]. The assertion must also tolerate LinkageError, not just ClassNotFoundException.

Acceptance: that platform-prefix assertion, red with the submitted spellings and green with the three corrected — and it shares its edit to JDKUtilsDenyListTest with the comment on :20, so please land them together.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

"com.sun.net.httpserver.HttpServer",
"org.apache.xbean.propertyeditor.JndiConverter",
"org.apache.catalina.startup.Tomcat",
"org.apache.commons.collections.Transformer",
"org.apache.commons.collections.functors.InvokerTransformer",
"org.apache.commons.collections.functors.ChainedTransformer",
"org.apache.commons.collections4.functors.InvokerTransformer",
"org.apache.commons.beanutils.BeanComparator",
"org.apache.commons.io.FileUtils",
"org.apache.commons.io.IOUtils",
"org.apache.commons.lang.SerializationUtils",
"org.springframework.beans.factory.config.PropertyPathFactoryBean",
"org.springframework.context.support.ClassPathXmlApplicationContext",
"org.springframework.context.support.FileSystemXmlApplicationContext",
"org.springframework.aop.aspectj.AspectJExpressionPointcut",
"org.springframework.aop.aspectj.annotation.AnnotationAwareAspectJAutoProxyCreator",
"org.springframework.jndi.JndiTemplate",
"org.springframework.jndi.JndiObjectTargetSource",
"org.springframework.transaction.jta.JtaTransactionManager",
"org.springframework.web.client.RestTemplate",
"org.hibernate.engine.spi.TypedValue",
"org.hibernate.jpa.HibernatePersistenceProvider",
"ch.qos.logback.core.db.JNDIConnectionSource",
"com.mchange.v2.c3p0.JndiRefForwardingDataSource",
"com.mchange.v2.c3p0.WrapperConnectionPoolDataSource",
"freemarker.template.utility.Execute",
"freemarker.cache.TemplateLoader",
"ognl.OgnlContext",
"javax.faces.context.FacesContext",
"javax.faces.context.ExternalContext",
"javax.faces.context.ResponseStream",
"javassist.ClassPool",
"javassist.CtClass",
"bsh.Interpreter",
"groovy.lang.GroovyShell",
"org.python.core.PyObject",
"org.codehaus.groovy.runtime.ConvertedClosure",
"org.codehaus.groovy.runtime.MethodClosure",
"kafka.utils.VerifiableProperties"
};

/**
* Lazily-loaded Class objects corresponding to {@link #AUTO_TYPE_DENY_FQCN}. A null entry means
* the class was either absent (e.g. optional module not on classpath) or failed to load during
* the static init sweep; the FQCN string itself is the authoritative gate so that a runtime
* {@code Class.forName(name)} on attacker input can be cross-referenced even when this array
* does not have the class loaded.
*/
static final Class<?>[] AUTO_TYPE_DENY_CLASSES;

static {
Class<?>[] deny = new Class<?>[AUTO_TYPE_DENY_FQCN.length];
for (int i = 0; i < AUTO_TYPE_DENY_FQCN.length; i++) {
try {
deny[i] = Class.forName(AUTO_TYPE_DENY_FQCN[i]);

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-10: AUTO_TYPE_DENY_CLASSES is written and read nowhere in the repository, yet building it runs the initializing Class.forName(String) over all 80 entries inside JDKUtils.<clinit> — on the first fastjson2 API call in every JVM — and the block is placed before the pre-existing static { Unsafe ... } block. The javadoc above calls the array "Lazily-loaded"; it is eager, and it is dead.

A repo-wide grep finds only its own javadoc (:163), its declaration (:169) and its assignment (:180); both live gates compare FQCN strings (:599-601 and ObjectReaderImplClass.java:58-59). So the whole cost buys nothing. And the cost is not only milliseconds: javax.imageio.ImageIO builds IIORegistry and issues five thread-context-classloader getResources lookups for META-INF/services/javax.imageio.spi.*, so whichever thread first touches fastjson2 — in a servlet container, a request thread whose TCCL is the webapp loader — makes a JSON library's class initializer enumerate every jar on that classpath and run the providers' constructors. It is also the only entry of the 80 that starts a thread, and that thread is non-daemon, so a short-lived process pays for it at exit.

Witness:

JDKUtils.<clinit> via Class.forName("com.alibaba.fastjson2.util.JDKUtils"), 3 runs per arm, fresh JVM:
  BASE 22.82 / 22.98 / 22.87 ms      HEAD 122.42 / 134.73 / 134.33 ms      (+~108 ms, 3/3 deterministic split)
classes loaded during that one call (-verbose:class):
  BASE 654                           HEAD 996                             (+342)
first public API call, JSON.toJSONString(new int[]{1,2,3}):
  BASE 87.1 / 81.4 / 81.2 ms         HEAD 207.1 / 199.9 / 193.2 ms        (+~115 ms)
attribution mutant (deny[i] = null; only): 22.78 / 22.82 / 23.30 ms       <- the whole delta is the sweep
PROBE FLIPS: Class.forName(name, false, loader) -> 26.26 / 25.97 / 25.86 ms, 666 classes,
             deny behaviour unchanged (programmatic deny still THROWS, InetAddress still THROWS)
per-entry thread attribution across all 80: only 'javax.imageio.ImageIO' starts a thread
             (AWT-Shutdown daemon=false group=system, AWT-AppKit daemon=true); the other 79 start none
             AWT-Shutdown still live and still non-daemon (TIMED_WAITING) 1.5 s later
JVM exit: empty main 0.048 / 0.061 / 0.059 s; Class.forName("javax.imageio.ImageIO") only
             2.217 / 1.934 / 1.944 / 2.695 / 1.942 s   (~40x)
             with -Djava.awt.headless=true 0.466 / 0.456 s   (AWT-Shutdown not started; toolkit still initializes)
ImageIO isolated: 145.3 ms of a 172.7 ms sweep; 5 TCCL getResources lookups; 41 of 80 entries throw CNFE

So the concrete victim is a short-lived process — CLI tool, batch job, cron entry, FaaS invocation, a Maven/Gradle/surefire fork — that parses one document and pays ~1.9 s of exit latency (~0.4 s headless) plus two AWT threads and the platform toolkit. On a real application classpath the remaining third-party entries (Spring, Tomcat, Hibernate, logback, c3p0, Groovy, javassist, OGNL, FreeMarker, Kafka) are initialized too, which means a JSON library decides when the app's logging and container classes initialize.

Delete AUTO_TYPE_DENY_CLASSES and its static block (:163-182) — the FQCN strings are the only gate, as this array's own javadoc says. If a Class-identity fast path is genuinely wanted later, build it lazily on first use and with Class.forName(name, false, JDKUtils.class.getClassLoader()) so no foreign static initializer, no SPI scan and no AWT thread results.

Three further hazards are reasoned rather than demonstrated, so please treat them as risk and not as measured fact: catch (Throwable ignored) swallowing an ExceptionInInitializerError leaves the class permanently erroneous, so the app's own later legitimate use fails with NoClassDefFoundError: Could not initialize class X; the AB-BA class-init lock-order window; and on headless Linux without -Djava.awt.headless=true an X11 connection failure poisoning sun.awt.X11.XToolkit. None was reproduced against a real list entry.

Constraint: JDKUtilsDenyListTest.java:24-27 deliberately tolerates a per-entry ClassNotFoundException because "the FQCN string itself is the authoritative gate", so any replacement must keep the FQCN strings as the matching key. Whatever replaces the sweep must keep catching Throwable, not ClassNotFoundException — some entries throw NoClassDefFoundError / UnsupportedClassVersionError. And do not touch AUTO_TYPE_DENY_FQCN's contents or visibility to chase this.

Acceptance: N/A for the deletion — no test references the array, and JDKUtilsDenyListTest staying green is the proof it was not load-bearing. If initialize=false is kept instead, pin it with a lifecycle assertion guarded by Assumptions.assumeFalse(GraphicsEnvironment.isHeadless()): force JDKUtils initialization, then assert no live thread whose name starts with AWT- — red as submitted, green with initialize=false.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

} catch (Throwable ignored) {
deny[i] = null;
}
}
AUTO_TYPE_DENY_CLASSES = deny;
}

static {
Unsafe unsafe;
try {
Expand Down Expand Up @@ -456,16 +568,40 @@ public static boolean isSQLDataSourceOrRowSet(Class<?> type) {
}

/**
* Tests whether a class is a well known deserialization gadget entry point, namely a
* {@link ClassLoader} subclass or a JDK SQL {@code DataSource}/{@code RowSet} implementation.
* Such types must not be resolved by matching an autoType whitelist prefix; only an accept
* entry naming the type in full is treated as an explicit opt-in.
* Tests whether a class is unsafe to expose via the autoType mechanism. Three categories are
* blocked:
* <ol>
* <li>Subclasses of {@link ClassLoader} — they can load arbitrary code.</li>
* <li>Subclasses of {@code javax.sql.DataSource} / {@code javax.sql.RowSet} — classic JNDI
* gadget sinks.</li>
* <li>The hardcoded FQCN list in {@link #AUTO_TYPE_DENY_FQCN} — classes whose presence on the
* classpath or whose static initializer / constructor / setter chain is itself enough to
* reach RCE / SSRF / LFD (e.g. {@code java.lang.Runtime}, {@code TemplatesImpl},
* {@code InvokerTransformer}, {@code URLClassLoader}). The match is done by exact
* FQCN — subclasses of these types are NOT auto-banned so that legitimate app classes
* with the same supertype are not collateral damage.</li>
* </ol>
* Subclasses of ClassLoader / DataSource / RowSet are blocked because the existing allow-list
* semantics in {@link com.alibaba.fastjson2.reader.ObjectReaderProvider#checkAutoType} treat a
* single explicit accept as opening the entire type hierarchy.
*
* @param type the class to test
* @return true if the class must not be resolved through a whitelist prefix match
*/
public static boolean isAutoTypeDenyClass(Class<?> type) {
return ClassLoader.class.isAssignableFrom(type) || isSQLDataSourceOrRowSet(type);
if (ClassLoader.class.isAssignableFrom(type) || isSQLDataSourceOrRowSet(type)) {
return true;
}
if (type == null) {

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-11: this null guard is unreachable. ClassLoader.class.isAssignableFrom(type) on the line above already dereferences type, and Class.isAssignableFrom(null) throws NPE — so isAutoTypeDenyClass(null) never reaches the branch, and the method advertises null-tolerance it does not have.

All five current call sites happen to pre-guard (ObjectReaderProvider.java:952, :986, :1024 inside clazz != null; ContextAutoTypeBeforeHandler.java:290 likewise; ObjectReaderImplClass.java:84 after the resolvedClass == null throw), so nothing crashes today. The cost is the next call site: this is a public static predicate on a public utility class, and a reader who sees the guard passes a possibly-null resolution result straight in — the natural thing to do with a TypeUtils.loadClass result, which returns null for unresolvable names — and gets an NPE out of a security check instead of false. The sibling helper in the same package puts the test first: BeanUtils.ignore(Class) (BeanUtils.java:1261-1264).

Witness:

r13-null JDKUtils.isAutoTypeDenyClass(null)
  BASE: THROW NullPointerException: null
  HEAD: THROW NullPointerException: null
bytecode: 3: invokevirtual Class.isAssignableFrom  ...  18: aload_0 / 19: ifnonnull 24 / 22: iconst_0 / 23: ireturn

Hoist the guard to be the first statement of the method, or delete it and record the non-null contract on the @param:

public static boolean isAutoTypeDenyClass(Class<?> type) {
    if (type == null) {
        return false;
    }
    if (ClassLoader.class.isAssignableFrom(type) || isSQLDataSourceOrRowSet(type)) {
        return true;
    }

Constraint: isSQLDataSourceOrRowSet(null) (:565-568) does CLASS_SQL_DATASOURCE.isAssignableFrom(type) with no null check either, so the guard must go above both predicates — placing it between them leaves the second dereference exposed.

Acceptance: assertFalse(JDKUtils.isAutoTypeDenyClass(null)) in JDKUtilsDenyListTest — it throws NPE with the guard where it is now and passes once hoisted, so reverting the reorder must red it. (Removing the dead guard also takes the method from 61 to 55 bytes, which is still over the 35-byte inlining gate, so it does not by itself address the size finding on :600.)

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

return false;
}
String name = type.getName();
for (int i = 0; i < AUTO_TYPE_DENY_FQCN.length; i++) {
if (name.equals(AUTO_TYPE_DENY_FQCN[i])) {

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-17: isAutoTypeDenyClass previously returned one boolean expression; it now appends a linear scan over 80 literals which, for the overwhelmingly common non-denied class, runs all 80 iterations — and the same scan is open-coded a second time in ObjectReaderImplClass.readObject before the SupportClassForName gate, so on the default configuration all 80 comparisons are wasted work on a path that throws two lines later.

AGENTS.md makes bytecode size an explicit project rule rather than a taste call: "Watch method bytecode size (codeSize) — keep hot-path methods under the two JIT inlining thresholds: 35 bytes (C1, -XX:MaxInlineSize) and 325 bytes (C2, -XX:FreqInlineSize) so they can be inlined", and "Prefer the implementation with smaller codeSize when choosing between equivalent approaches". This method crossed the first threshold, and the JVM was observed declining to inline it. The codebase's own convention for exactly this job is a precomputed sorted long[] of Fnv.hashCode64(name) probed with Arrays.binarySearch (BeanUtils.IGNORE_CLASS_HASH_CODES + BeanUtils.ignore(Class), BeanUtils.java:47 and 1261-1270) — which this PR itself reuses for denyHashCodes 20 lines away.

Witness:

codeSize (javap; both arms built by the project's own Maven at maven.compiler.source=8):
  isAutoTypeDenyClass  BASE 22 -> HEAD 61     (MaxInlineSize 35 | FreqInlineSize 325)
  checkAutoType        BASE 849 -> HEAD 902   (both far over 325 -> no gate crossed; NEGATIVE result)
-XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining, polymorphic JSONB parse workload (1.5M parses):
  BASE JDKUtils::isAutoTypeDenyClass (22 bytes) inline (hot)   declines: 0
  HEAD JDKUtils::isAutoTypeDenyClass (61 bytes) callee is too large x3   inline (hot) x1
throughput, 3 interleaved rounds, NO diagnostic flags, ranges non-overlapping:
  BASE (2.0.65 jar)                              253.4 / 250.6 / 252.3 ns/object
  ARM D (deny block reverted, callee LEFT at 61) 273.7 / 276.4 / 274.2
  HEAD (intact)                                  289.7 / 285.5 / 286.2 ns/object
  => +21.9 ns/object (+8.7%) attributable to THIS finding; +12.0 (+4.4%) to the unconditional-block finding
deny table size measured, not retyped: AUTO_TYPE_DENY_FQCN.length = 80, 80 distinct

Build the lookup once at class init and use it at both call sites — this is the same helper the cross-package build break needs:

private static final Set<String> AUTO_TYPE_DENY_FQCN_SET =
        Collections.unmodifiableSet(new HashSet<>(Arrays.asList(AUTO_TYPE_DENY_FQCN)));
// isAutoTypeDenyClass:  return AUTO_TYPE_DENY_FQCN_SET.contains(type.getName());
// ObjectReaderImplClass: if (JDKUtils.isAutoTypeDenyName(className)) { throw ... }

Class.getName() returns the same cached String instance on every call, so that string's lazily computed hash field is filled once and reused — one hash+probe per call instead of 80 comparisons, and the callee comes back near the 35-byte gate. Please do not reach for -XX:FreqInlineSize or CompileCommand=inline; those are runtime knobs the PR cannot ship and they pay at every other call site.

One correction for accuracy: only 2 of the 5 call sites are genuinely per-@type (ObjectReaderProvider:1024 and the two in ObjectReaderImplClass) — :952, :986 and ContextAutoTypeBeforeHandler:290 sit inside accept-prefix branches guarded by i + 1 < typeNameLength. Base already had the same three invocations, so this diff grows the callee, not the call count. And checkAutoType crossing no gate is recorded so it is not re-claimed later.

Constraint: matching must stay exact-FQCN per the javadoc this diff adds at :580-584 — a Set.contains(name) preserves that, an isAssignableFrom-style or prefix match would not and would break Issue530.java:12. Keep the array reachable for JDKUtilsDenyListTest.java:6's static import.

Acceptance: JDKUtilsDenyListTest.testIsAutoTypeDenyClass_ExactFQCN pins both directions (every resolvable FQCN returns true; Object.class and String.class return false), so a broken HashSet seed, a lost entry or a widened match goes red there. Runtime confirmation of the inlining half is -XX:+PrintInlining over an autoType-heavy parse: callee is too large must disappear.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

return true;
}
}
return false;
}

public static void setReflectErrorLast(Throwable error) {
Expand Down
47 changes: 47 additions & 0 deletions core/src/test/java/com/alibaba/fastjson2/reader/ObjectReaderImplClassTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package com.alibaba.fastjson2.reader;

import com.alibaba.fastjson2.JSON;
import com.alibaba.fastjson2.JSONException;
import com.alibaba.fastjson2.JSONReader;
import com.alibaba.fastjson2.util.JDKUtils;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertThrows;

public class ObjectReaderImplClassTest {
/**
* Verifies PR-2: ObjectReaderImplClass refuses to resolve a class whose FQCN is on the
* {@link JDKUtils#AUTO_TYPE_DENY_FQCN} deny list, even when both SupportAutoType and
* SupportClassForName are opted into. Without the defence-in-depth added in PR-2, this test
* would successfully call Class.forName("com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl")
* and load the TemplatesImpl class.
*/
@Test
public void testRefusesDangerousVal() {
JSONReader.Feature[] features = new JSONReader.Feature[]{
JSONReader.Feature.SupportAutoType,
JSONReader.Feature.SupportClassForName
};
String payload = "{\"@type\":\"java.lang.Class\",\"val\":\"com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl\"}";

@wenshao wenshao Sep 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-12: both tests in this file pass with the two guards they were written to witness deleted, and the javadoc above states a counterfactual that is measurably false.

The payload shape never reaches the guards. ObjectReaderImplClass.readObject begins with jsonReader.readValueHashCode(), and every text reader returns -1 immediately at a non-quote char without setting nameBegin/nameEnd (JSONReaderASCII.java:528-530, JSONReaderUTF8.java:2759-2761, JSONReaderUTF16.java:1619-1621), so the following getString() returns new String(chars, 0, 0) — the empty string. "" matches no FQCN, getMapping("") is null, checkAutoType("") returns null at its isEmpty() guard, and the pre-existing throw ... ("class not found " + className) at :77-79 fires, identically before and after this PR. So PR-2's ~21 added lines ship with nothing that would fail if a later refactor dropped them, and the javadoc's claim that without PR-2 this test "would successfully call Class.forName(...TemplatesImpl) and load the TemplatesImpl class" will mislead the next maintainer into believing this path is covered. It is true of the bare-string shape, which the test does not use.

Witness:

A/B, the tests' own payloads (the empty slot after "class not found " is className == ""):
  BASE(2.0.65) THROWN JSONException : class not found , offset 1, character {
  HEAD(PR)     THROWN JSONException : class not found , offset 1, character {
  control, harmless val (java.util.ArrayList): THROWN class not found , offset 1   (BASE == HEAD)
MUTATION, both new guards deleted (arm proof: 0 deny refs in the compiled class):
  ObjectReaderImplClassTest: Tests run: 2, Failures: 0, Errors: 0   <- STILL GREEN
The shape that DOES discriminate:
  BASE "com.sun...TemplatesImpl" -> RESULT class com.sun...TemplatesImpl   (base really loads it)
  HEAD "com.sun...TemplatesImpl" -> THROWN autoType is not support : com.sun...TemplatesImpl

Drive the reader with the input shape it actually parses — the bare class-name string — and assert on the message so the test cannot be satisfied by the unrelated class not found path again:

String payload = "\"com.sun.org.apache.xalan.internal.xsltc.trax.TemplatesImpl\"";
JSONException e = assertThrows(JSONException.class,
        () -> JSON.parseObject(payload, Class.class, JSONReader.Feature.SupportClassForName));
assertTrue(e.getMessage().contains("autoType is not support"), e.getMessage());

Then correct the javadoc to describe the measured pre-change behaviour. It is also worth covering the only configuration in which the post-checkAutoType block at :84-86 is load-bearing — a deny class outside the FQCN table, e.g. a test-local ClassLoader or javax.sql.DataSource implementation reached through a full-name accept.

Constraint: ObjectReaderImplClass.java:38-49 returns early when a context AutoTypeBeforeHandler resolves the name, so the replacement test must not install a filter; and readValueHashCode() only yields a class name for a quoted value. Note the two guards use distinguishable messages — ObjectReaderImplClass throws "autoType is not support : " (space-colon) while checkAutoType throws "autoType is not support. " (period) — so a message assertion can pin which guard fired.

Acceptance: the rewritten testRefusesDangerousVal is its own witness — measured red on the pre-PR jar (which returns class ...TemplatesImpl) and green with the deny loop. It must go red when the loop at ObjectReaderImplClass.java:58-62 is deleted; today's version does not.

— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)

assertThrows(
JSONException.class,
() -> JSON.parseObject(payload, Class.class, features)
);
}

/**
* Verifies PR-2 also rejects java.lang.Runtime (a 1.x-era gadget) via the same path.
*/
@Test
public void testRefusesRuntimeVal() {
JSONReader.Feature[] features = new JSONReader.Feature[]{
JSONReader.Feature.SupportAutoType,
JSONReader.Feature.SupportClassForName
};
String payload = "{\"@type\":\"java.lang.Class\",\"val\":\"java.lang.Runtime\"}";
assertThrows(
JSONException.class,
() -> JSON.parseObject(payload, Class.class, features)
);
}
}
Loading
Loading