Type-check the exemplar in the EnumMap server field serializer by Nexory · Pull Request #10392 · gwtproject/gwt

Nexory · 2026-08-11T17:59:22Z

EnumMap_ServerCustomFieldSerializer overrode the type-checking server
instantiateInstance but threw the type information away and called the untyped
client instantiate, which reads the exemplar with a bare readObject(). Every
other collection serializer reads what it instantiates from through the typed
path; TreeMap/TreeSet are the direct precedent
(readObject(Comparator.class, resolvedTypes)).

Because the exemplar was read untyped, a request could substitute another type
for it, and new EnumMap(nonEnumClass) throws an uncaught
NullPointerException (keyUniverse is null) instead of a clean serialization
error.

This adds a server-side instantiate(streamReader, expectedParameterTypes, resolvedTypes) that reads the exemplar against expectedParameterTypes[0], the
declared key type. A non-enum is rejected with SerializedTypeViolationException
the same way the other serializers reject mismatched types, and so is an enum
other than the declared one. A legitimate exemplar still passes and the resulting
getClass() is unchanged, so behaviour for valid EnumMaps is identical.

Tests

Four cases in RPCTypeCheckTest, all going through RPC.decodeRequest:

testEnumMapSpoofingClass substitutes an Integer exemplar on the existing raw
EnumMap parameter. Before this change it fails with an uncaught
NullPointerException; after it, the request is rejected with
IncompatibleRemoteServiceException wrapping SerializedTypeViolationException,
matching the existing spoofing tests for the other collections.
testEnumMapSpoofingEnum substitutes an exemplar of a different enum on a
parameter declared EnumMap<NEnum, Integer> and expects the same rejection.
This is the case that separates expectedParameterTypes[0] from Enum.class:
reverting only that argument leaves the test failing at its fail() call,
because Enum.class accepts any enum.
testEnumMapTypedValid decodes a request whose exemplar is the declared key
type and asserts the resulting EnumMap, so the tighter check is shown not to
reject the legitimate case.
testEnumMapValid decodes an EnumMap with an NEnum exemplar and one with an
OEnum exemplar on the raw parameter, which declares no key type, and asserts
both maps. A key type pinned to one enum passes every other case and fails this
one, on the OEnum request.

The raw parameter is kept alongside the typed one, so both paths stay covered.
RPCTypeCheckTest passes with 29 tests, and the rest of
com.google.gwt.user.server.rpc is unchanged.

Diff

EnumMap_ServerCustomFieldSerializer.java (+16/-1): the typed instantiate.
RPCTypeCheckTest.java (+158): two enums, a typed test service method, the
four tests and their request generators.
RPCTypeCheckFactory.java (+46): helpers to write an enum constant and an
EnumMap message with a chosen exemplar, plus a javadoc that had ended up
separated from its method.

Note

The failure is reachable through the normal servlet path, not only through
RPC.decodeRequest in a test. processCall guards the decode against
IncompatibleRemoteServiceException only, so the NullPointerException
propagates to doPost, which catches Throwable and answers through
RPCServletUtils.writeResponseForUnexpectedFailure, that is with status 500. I
also confirmed it with an embedded RemoteServiceServlet and a strict
serialization policy that allow-lists only the service's own types.

niloc132

Given that the only bug being fixed here is that bad (malicious?) input results in a slightly less useful error message, this seems low priority.

niloc132 · 2026-09-10T01:24:47Z

   * This checks that a Map generated by Collections.emptyMap correctly reports
   * that it is an incorrect type.
   */
+  /**


looks like you separated the javadoc above from its method below

Fixed in 1d9eaf2. The javadoc belonged to testEmptyMapSpoofingClass and my
test went in between it and its method. While I was there I dropped the attack
framing from my own test comment, since what it exercises is the type check.

niloc132 · 2026-09-10T01:26:52Z

+     * which throws an uncaught {@link NullPointerException} rather than a clean
+     * serialization error.
+     */
+    @SuppressWarnings("unused")


Suggested change

@SuppressWarnings("unused")

This method is used, you are calling it below.

Removed in a7360f1. It was there because expectedParameterTypes went unused
inside the method, not because the method is unused, and using it below made
the annotation unnecessary anyway. TreeMap_ServerCustomFieldSerializer
ignores the same parameter without one, so it should never have been added.

Correction to what I wrote here: TreeMap_ServerCustomFieldSerializer does carry
@SuppressWarnings("unused") on its instantiate, and so does
TreeSet_ServerCustomFieldSerializer. I had it backwards.

Removing the annotation was still right, since expectedParameterTypes[0] is used now,
but the reason I gave for it was not. The commit message of a7360f1 carries the same
wrong claim.

Thanks - commit message isn't important, we'll squash the commits and use the PR description instead (so be sure that is all accurate and ready for merge).

niloc132 · 2026-09-10T01:29:06Z

+    public static EnumMap instantiate(ServerSerializationStreamReader streamReader,
+        Type[] expectedParameterTypes, DequeMap<TypeVariable< ? >, Type> resolvedTypes)
+        throws SerializationException {
+      Object exemplar = streamReader.readObject(Enum.class, resolvedTypes);


Shouldn't this really be expectedParameterTypes[0]? We don't need to allow any enum here, we can be specific to the actual enum being used.

Tests appear to pass with this additional change, but you might have a more specific use case to consider than the raw test below.

Applied in a7360f1, and you are right about the effect, but it is not
observable in this PR as it stands.

expectedParameterTypes[0] here is the type variable K, not a class. I
printed it: len=2 [0]=K. With the raw EnumMap in the test service method,
K resolves to its bound, so readObject sees Enum and behaves exactly as
the old Enum.class did.

Measured on this branch: reverting just that one line back to Enum.class
leaves RPCTypeCheckTest at 26/26 green. Nothing currently covers the change,
which is the same thing you point out below.

niloc132 · 2026-09-10T01:30:32Z

+    @SuppressWarnings({"unused", "rawtypes"})
+    public static void testEnumMap(java.util.EnumMap arg1) {
+    }
+


While not technically illegal, using raw generics does limit rpc from doing more accurate typechecking, so we're missing out on testing that. As an example, if expectedParameterTypes[0] were used above, we should be able to tell the difference between the right and wrong enum type, even if both are enums - but only if the correct type is actually specified.

Agreed, and it is the reason the change above is untestable right now: with the
raw declaration K erases to Enum, so right and wrong enum are
indistinguishable.

Parameterising it needs an enum in that test's type universe, and there is not
one anywhere under user/test/com/google/gwt/user/server/rpc/. Adding one means
introducing it into the RPC allowlist for ClassesParamTestClass, which is more
than a test tweak.

Do you want me to add one, or leave the tighter check in with the raw test and
handle the coverage separately?

Yes, the new behavior should be tested. Please ensure there is a positive test as well.

Done in a3429a1, rebased onto current main. The service method now declares EnumMap<NEnum, Integer>, so
expectedParameterTypes[0] is a class rather than the type variable, and two enums
make the right and the wrong one distinguishable.

Negative: testEnumMapSpoofingEnum substitutes an OEnum exemplar and expects the
type violation. Positive: testEnumMapTypedValid decodes a request whose exemplar is
the declared key type and asserts the resulting map.

Measured both ways out of the same build. As it stands, RPCTypeCheckTest is 28/28.
Reverting only the readObject argument back to Enum.class:

Tests run: 28, Failures: 1, Errors: 0 1) testEnumMapSpoofingEnum(...RPCTypeCheckTest)junit.framework.AssertionFailedError: Expected IncompatibleRemoteServiceException from testEnumMapSpoofingEnum

It fails at the fail() call and not as an error, so decodeRequest returned
normally: Enum.class does accept the other enum. testEnumMapTypedValid is green in
both runs, which is the point of having it.

Two details in case they look arbitrary. I measured both rather than assuming them. If
OEnum does not implement IsSerializable, the reverted run still fails, but on the
serialization policy: expected:<SerializedTypeViolationException> but was:<SerializationException>, which is red for a reason unrelated to the line. If the
negative case carries a map entry instead of an empty body, the reverted run is green:
the entry's key is checked against the declared key type and raises the violation by
itself, so the test would be measuring the map key check rather than the exemplar read.
The empty body is what isolates the exemplar.

The raw testEnumMap and its Integer-exemplar test are untouched, so that path stays
covered. Production code is unchanged in this round; both commits are test only.

Separately, d27fb93 moves writeEnumMapWithSpoofedExemplar in RPCTypeCheckFactory:
I had inserted it between the javadoc of write(int) and the method, the same thing you
spotted in the test file.

niloc132

Thanks for the changes, some feedback below.

niloc132 · 2026-09-16T18:23:09Z

+    Object deserializedArg = decoded.getParameters()[0];
+    assertEquals(EnumMap.class, deserializedArg.getClass());
+
+    EnumMap<NEnum, Integer> expected = new EnumMap<NEnum, Integer>(NEnum.class);


Suggested change

EnumMap<NEnum, Integer> expected = new EnumMap<NEnum, Integer>(NEnum.class);

EnumMap<NEnum, Integer> expected = new EnumMap<>(NEnum.class);

Applied in 088c386.

niloc132 · 2026-09-16T18:23:37Z

+   * This checks that an EnumMap whose exemplar is the declared key type is
+   * accepted, and that it is built with that key type.
+   */
+  public void testEnumMapTypedValid() throws Exception {


Suggested change

public void testEnumMapTypedValid() throws Exception {

public void testEnumMapTypedValid() {

Applied in 088c386.

niloc132 · 2026-09-16T18:36:02Z

+   */
+  public void writeEnumMapWithEntry(Enum<?> exemplar, Enum<?> key, Integer value)
+      throws SerializationException {
+    writeStringFromTable(generateSerializedClassString(java.util.EnumMap.class));


import this so it doesnt need to be qualified

Suggested change

writeStringFromTable(generateSerializedClassString(java.util.EnumMap.class));

writeStringFromTable(generateSerializedClassString(EnumMap.class));

Applied in 088c386, at all three uses in RPCTypeCheckFactory.

niloc132 · 2026-09-16T19:05:04Z

+  private static String generateEnumMapSpoofingClass() {
+    try {
+      RPCTypeCheckFactory strFactory =
+          new RPCTypeCheckFactory(ClassesParamTestClass.class, "testEnumMap");


looks like we still need a postive test for this, probably something that exercises both NEnum and OEnum?

Added in 6637070 as testEnumMapValid, on the raw testEnumMap(EnumMap) parameter
this generator targets. It decodes two requests there, one with an NEnum exemplar and
key, one with an OEnum exemplar and key, and for each asserts an EnumMap equal to the
expected map. Each map carries an entry, and EnumMap only treats maps with entries as
equal when their key types match, so a map built for the wrong enum would not pass.

To check that the OEnum half earns its place, I pinned the serializer's key type to
NEnum, a deliberately too-strict variant, and ran RPCTypeCheckTest against it:

Tests run: 29, Failures: 0, Errors: 1 testEnumMapValid: IncompatibleRemoteServiceException: Attempt to deserialize an object of type class ...RPCTypeCheckTest$OEnum when an object of type class ...RPCTypeCheckTest$NEnum is expected

The error comes from the OEnum request. The NEnum half and every other test in the
class pass under that variant, so the second request is what catches a key type pinned
to one enum. With the serializer restored, RPCTypeCheckTest is 29/29.

I also updated the PR description for the fourth test, and rebased onto current main so
the tests ran against it.

EnumMap_ServerCustomFieldSerializer overrode the type-checking server instantiateInstance but discarded the type information and called the untyped client instantiate, which reads the exemplar with a bare readObject(). Every other collection serializer reads what it instantiates from through the typed path; TreeMap and TreeSet are the direct precedent. Because the exemplar was read untyped, a request could substitute a non-enum type for it, and new EnumMap(nonEnumClass) throws an uncaught NullPointerException instead of a clean serialization error. Add a server-side instantiate that reads the exemplar with readObject(Enum.class, resolvedTypes), so a non-enum is rejected with SerializedTypeViolationException like the other serializers. A valid enum constant still passes and getClass() is unchanged, so behaviour for valid EnumMaps is identical.

The type-check test family covers every collection serializer for exemplar and element substitution except EnumMap. Add testEnumMapSpoofingClass, its request generator, and an EnumMap method on the existing ClassesParamTestClass service. Before the serializer change the test fails with an uncaught NullPointerException; after it, the request is rejected with IncompatibleRemoteServiceException wrapping SerializedTypeViolationException, matching the other spoofing tests. RPCTypeCheckTest passes with 26 tests.

@SuppressWarnings

Read the exemplar with expectedParameterTypes[0] rather than Enum.class, so a different enum is rejected too and not only a non-enum. The parameter was already being passed in and ignored. That also removes the reason for the @SuppressWarnings("unused") on the method: it was there because the parameter went unused, not because the method is unused. TreeMap's server serializer ignores the same parameter without an annotation, so there was no need for one here either.

The new test was inserted between that javadoc and the method it describes, leaving it attached to the wrong one. Move it back and drop the framing in the new test's own comment, which described the substitution as an attack rather than as the type check it is.

TypeVariable<?> rather than TypeVariable< ? > on the signature this pull request adds. The other three occurrences in this file keep the spaced form they had before, so the file is now mixed; say the word and I will normalise all four.

Commit 23ad7ea inserted writeEnumMapWithSpoofedExemplar between the javadoc of write(int) and the method itself, so the javadoc documented the wrong member. Move the method down to the other writers, where it also sits in alphabetical order.

The existing testEnumMap declares a raw EnumMap, so expectedParameterTypes[0] is the type variable K, which resolves to its bound Enum. The exemplar check therefore behaved exactly as the earlier Enum.class did, and no test could tell the two apart. Add a service method declaring EnumMap<NEnum, Integer> and two enums, so the expected type is a class rather than a type variable. testEnumMapSpoofingEnum substitutes an OEnum exemplar and expects a type violation; reverting the read back to Enum.class makes it fail at its fail() call, because Enum.class accepts any enum. testEnumMapTypedValid decodes a request whose exemplar is the declared key type and asserts the resulting EnumMap, so the tighter check is shown not to reject the legitimate case. Both enums implement IsSerializable, and the negative case carries an empty map body, so that the reverted run reaches the fail() call rather than stopping earlier on the serialization policy or on EnumMap.put.

Use the diamond operator for the expected map, drop the throws clause that testEnumMapTypedValid does not need, and import EnumMap in RPCTypeCheckFactory instead of qualifying it at each use.

The raw EnumMap parameter declares no key type, so an EnumMap of any enum has to decode. testEnumMapValid decodes one with an NEnum exemplar and one with an OEnum exemplar on the same parameter and asserts both maps, including their key type.

writeEnum writes an enum constant the way RPC puts it on the wire, as its ordinal, so the Error Prone EnumOrdinal warning does not apply there. Suppress it for the method.

niloc132 reviewed Sep 10, 2026

View reviewed changes

Nexory force-pushed the enummap-server-typecheck branch from 7f28843 to a3429a1 Compare September 10, 2026 17:22

niloc132 reviewed Sep 16, 2026

View reviewed changes

Nexory force-pushed the enummap-server-typecheck branch from a3429a1 to 6637070 Compare September 16, 2026 23:24

niloc132 reviewed Sep 23, 2026

View reviewed changes

Comment thread user/test/com/google/gwt/user/server/rpc/RPCTypeCheckFactory.java

niloc132 added the ready This PR has been reviewed by a maintainer and is ready for a CI run. label Sep 23, 2026

Nexory added 10 commits September 24, 2026 00:04

Put back the javadoc of write(int)

c275cf3

Commit 23ad7ea inserted writeEnumMapWithSpoofedExemplar between the javadoc of write(int) and the method itself, so the javadoc documented the wrong member. Move the method down to the other writers, where it also sits in alphabetical order.

Apply review suggestions to the EnumMap tests

eab923f

Use the diamond operator for the expected map, drop the throws clause that testEnumMapTypedValid does not need, and import EnumMap in RPCTypeCheckFactory instead of qualifying it at each use.

Suppress EnumOrdinal on writeEnum in RPCTypeCheckFactory

36f3acb

writeEnum writes an enum constant the way RPC puts it on the wire, as its ordinal, so the Error Prone EnumOrdinal warning does not apply there. Suppress it for the method.

Nexory force-pushed the enummap-server-typecheck branch from 6637070 to 36f3acb Compare September 23, 2026 22:15

niloc132 approved these changes Oct 7, 2026

View reviewed changes

zbynek merged commit 9d7f963 into gwtproject:main Oct 7, 2026
6 checks passed

Nexory deleted the enummap-server-typecheck branch October 10, 2026 12:18

	EnumMap<NEnum, Integer> expected = new EnumMap<NEnum, Integer>(NEnum.class);
	EnumMap<NEnum, Integer> expected = new EnumMap<>(NEnum.class);

	public void testEnumMapTypedValid() throws Exception {
	public void testEnumMapTypedValid() {

	writeStringFromTable(generateSerializedClassString(java.util.EnumMap.class));
	writeStringFromTable(generateSerializedClassString(EnumMap.class));

Conversation

Nexory commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Diff

Note

Uh oh!

niloc132 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niloc132 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Nexory commented Aug 11, 2026 •

edited

Loading