pjfanning opened a new pull request, #3503:
URL: https://github.com/apache/pekko/pull/3503
### Motivation
Four issues in `JacksonSerializer`, all reachable from the manifest string
on the wire.
**The case object branch resolved the class in a way that initializes it,
before checking
the allow list.** When the manifest ends in `$`, `fromBinary` called
`system.dynamicAccess.getObjectFor(className)`, which reads the `MODULE$`
field through a
`VarHandle` and so runs the class's initializer; `checkAllowedClass` ran
only afterwards.
The only gate before that point is `checkAllowedClassName`, which consults
Jackson's gadget
deny list rather than `serialization-bindings` or `allowed-class-prefix`. So
a manifest
naming any Scala `object` on the classpath ran that object's body on the way
to being
rejected. The sibling branch already does the right thing — `getClassFor` is
`Class.forName(fqcn, false, cl)` and does not initialize. Same shape as
#3495.
**`LZ4Meta.get` checked for four remaining bytes and then read eight** — the
magic plus the
declared length. A 4-to-7 byte payload beginning with `0x87D96DF6` raised
`BufferUnderflowException` out of `decompress`.
**`parseManifest` called `toInt`** on whatever followed the last `#`, so
`Foo#abc` or a
trailing `#` raised `NumberFormatException` rather than a serialization
error.
**`isInAllowList` evaluated the throwing operand first.**
`isBoundToJacksonSerializer` calls
`serializerFor`, which raises `NotSerializableException` — stack trace and
all — for a class
that is not bound, and that is exactly the case for a class allowed only by
`allowed-class-prefix`. `checkAllowedClass` runs on every `fromBinary`, so
this built and
discarded one exception per message on that path.
### Modification
- Resolve the case object's class with `getClassFor`, run
`checkAllowedClass` on it, and read
the module field only after that.
- Require 8 remaining bytes before reading an LZ4 header.
- Parse the manifest version with `toIntOption` and report a non-numeric one
as
`NotSerializableException` naming the manifest.
- Test `hasAllowedClassPrefix` before `isBoundToJacksonSerializer`.
Applied identically to `serialization-jackson` and `serialization-jackson3`.
The reordering in `isInAllowList` is the only change with no behaviour
difference — both
operands are pure predicates and `isBoundToJacksonSerializer` already
swallows its exception —
so it is covered by the existing `allowed-class-prefix` tests rather than a
new one.
### Result
A rejected manifest no longer initializes the class it names; two malformed
manifests are
reported as serialization failures instead of unrelated runtime exceptions;
the allow list
check no longer constructs an exception per message on the prefix path. No
change for
manifests that were accepted before.
### Tests
New tests in `JacksonSerializerSpec` in both modules (so each runs under
both the JSON and
CBOR serializers):
- `not allow deserialization of a case object that is not in
serialization-bindings` — the
allow list still rejects it
- `not initialize a case object class it goes on to reject` — a test-only
object whose body
sets a flag; the flag must still be false after the rejection
- `reject a manifest whose version is not a number`
- `not underflow on a payload that is only as long as the LZ4 magic`
I checked these discriminate by reverting the production change and
re-running: the last
three fail without it (`true did not equal false`, `NumberFormatException
was thrown`, and
the underflow respectively). The first passes either way — it documents that
the allow list
decision itself is unchanged, which is the point.
- `sbt "serialization-jackson/testOnly
org.apache.pekko.serialization.jackson.*"` — 128 passed
- `sbt "serialization-jackson3/testOnly
org.apache.pekko.serialization.jackson3.*"` — 126 passed
- `sbt "serialization-jackson/mimaReportBinaryIssues"` — no issues
(`serialization-jackson3`
disables MimaPlugin)
- `sbt scalafmtAll headerCreateAll` — no changes
### References
The case object issue is the same shape as #3495, which stopped the
serializer resolving a
wire-supplied manifest class it would not use.
`ProtobufSerializer.isInAllowList` has the same throwing-operand-first
ordering, but it
caches its method handle after the check so it pays the cost once per class
rather than once
per message; it is in `pekko-remote` and is left for a separate change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
