Colm O hEigeartaigh created CXF-9264:
----------------------------------------
Summary: Replace the hand-written JSON parser
Key: CXF-9264
URL: https://issues.apache.org/jira/browse/CXF-9264
Project: CXF
Issue Type: Improvement
Reporter: Colm O hEigeartaigh
Assignee: Colm O hEigeartaigh
Fix For: 4.3.0
h2. Summary
Replace the hand-written JSON parser and writer in
\{{JsonMapObjectReaderWriter}} (cxf-rt-rs-json-basic) with Jakarta JSON-P
(Eclipse Parsson). Keep the public \{{JsonMapObject}} /
\{{JsonMapObjectReaderWriter}} API as a thin adapter so that JOSE, OAuth2 and
OIDC callers are unaffected in the first step.
h2. Background
{\{JsonMapObjectReaderWriter}} is a hand-written JSON parser and writer (about
640 LOC). It works on substrings: it finds the matching closing bracket for
each object or array, slices out that substring, and parses it recursively. It
sits on the security-critical path for:
* JOSE: JWS/JWE headers, JWT claims, JWK/JWK sets (\{{JoseHeaders}},
\{{JwtClaims}}, \{{JsonWebKey}}, \{{JsonWebKeys}}, \{{JwkReaderWriter}},
\{{JoseUtils}})
* OAuth2: token requests and responses (\{{OAuthJSONProvider}}),
\{{ClientRegistration}}, \{{ClientRegistrationResponse}},
\{{AuthorizationMetadata}}
* OIDC: \{{ClaimsRequest}}, \{{ClaimRequirement}}, \{{UserAddress}}, and the
IdP and RP services
* the \{{JsonMapObjectProvider}} JAX-RS provider
* \{{OpenApiParseUtils}} (cxf-rt-rs-description-openapi-v3)
About 40 non-test classes depend on it, and 10 classes extend
\{{JsonMapObject}}.
Much of its input comes from untrusted sources: JWT/JWS/JWE tokens, OAuth token
and registration requests, and JWK sets fetched from remote endpoints.
h2. Motivation
The parser has needed a steady stream of correctness and hardening fixes, for
example:
* PR 3137: NaN / infinite values in JSON / JWT
* PR 3140, PR 3167: escaped backslash and quote handling
* PR 3141: key parsing
* PR 3142: unicode characters
* PR 3145: escaping control characters
* PR 3149, PR 3152: depth limit on parsing and writing
* PR 3168: nested arrays
* PR 3172: default limits on the number of object keys and array elements
* CXF-9240 (PR 3367): \{{OAuthJSONProvider}} did not escape JSON string values
Every one of these bugs is already handled by a standard, widely used parser.
Keeping our own means we keep finding and fixing the same class of bugs, and
every fix lands on the token-processing path.
h2. Proposed approach
h3. Phase 1: swap the implementation behind the existing API
* Re-implement the read paths of \{{JsonMapObjectReaderWriter}} (\{{fromJson}},
\{{fromJsonToJsonObject}}, \{{fromJsonAsList}}) with the streaming
\{{jakarta.json.stream.JsonParser}}. Build the same \{{LinkedHashMap}} /
\{{List}} / \{{Long}} / \{{Double}} / \{{String}} / \{{Boolean}} / \{{null}}
structures as today.
* Re-implement the write paths (\{{toJson}}) with
\{{jakarta.json.stream.JsonGenerator}}.
* Do not change the public API in this phase. Subclasses such as
\{{JwkReaderWriter}} and all \{{JsonMapObject}} subclasses must keep working
without modification.
* Add the Jakarta JSON-P API and Parsson dependencies to cxf-rt-rs-json-basic.
Parsson is already managed in parent/pom.xml (\{{cxf.parsson.version}}).
h3. Phase 2: move direct callers off the old internals
* Move \{{OAuthJSONProvider}} and \{{JsonMapObjectProvider}} onto the new
implementation and remove any remaining hand-written escaping.
* Move \{{OpenApiParseUtils}} off json-basic (Jackson / swagger-core are
already available in that module).
* Remove the protected internals that existed only to support substring parsing
(\{{internalFromJsonAsList}}, the \{{Settable}} plumbing, \{{getClosingIndex}}
etc.), deprecating them first if they are part of the API.
h3. Phase 3 (optional, separate issue)
Decide whether to keep cxf-rt-rs-json-basic as a published module or fold it
into JOSE as an internal detail. Document that it is not intended as a
general-purpose parser for application payloads.
h2. Behaviour to preserve (must be covered by tests before the swap)
These are deliberate behaviours of the current implementation that callers rely
on:
# *Duplicate key detection for JOSE.* The parser fills objects through
\{{JsonMapObject.setProperty}}, which counts repeated keys in \{{updateCount}}.
\{{JwsCompactConsumer}} and \{{JweCompactConsumer}} check \{{getUpdateCount()}}
to reject headers with duplicate keys. The new reader must still route every
key through \{{setProperty}}, or detect duplicates in some other way. It must
not silently keep the first or the last value.
# *Number mapping.* Integers that fit are parsed as \{{Long}}, everything else
as \{{Double}}, and non-finite values are rejected. Claims code (\{{exp}},
\{{iat}}, \{{nbf}}) casts to these types, so the types must stay the same.
Decide and document what happens to integers that don't fit in a \{{long}}
(today they fall through to \{{Double}} and lose precision; consider rejecting
them).
# *Key order.* Objects are read into \{{LinkedHashMap}} and written in
insertion order.
# *Compact output format.* With \{{format=false}} the output must stay
byte-identical (no whitespace, same escaping), because existing tests compare
against RFC 7515 / 7516 / 7517 examples and against stored values. Signature
verification itself uses the raw base64url segments, but serialised headers and
claims become part of the signing input on the producing side.
# *Pretty-print mode.* \{{new JsonMapObjectReaderWriter(true)}} is used by
\{{JoseUtils}} for logging and debug output. The exact format may change, but
it must remain valid JSON.
# *\{{JsonObject}} marker values.* Values implementing \{{JsonObject}}
(\{{JwsJsonSignatureEntry}}, \{{JweJsonEncryptionEntry}}) are written unquoted
using their \{{toString()}}, which produces JSON. Keep this, or replace it with
an explicit serialisation hook.
# *Non-String scalar values.* Other objects are written as quoted
\{{toString()}}, and today they are *not* escaped (only \{{String}} values
are). The new writer must escape them as well. This is a deliberate fix, not a
behaviour change to preserve.
# *Limits.* Keep the current defaults and system properties:
#* nesting depth 500 (on read and write)
#* \{{org.apache.cxf.jaxrs.json.basic.maxObjectKeys}} (default 10,000)
#* \{{org.apache.cxf.jaxrs.json.basic.maxArrayElements}} (default 10,000)
Parsson's built-in limits (depth, big number scale and length) should be
checked for the version in use and set at least as strictly. Limits that
Parsson doesn't provide (key and element counts) must be enforced in the
adapter while streaming.
# *Error type.* Malformed input currently surfaces as \{{UncheckedIOException}}
/ \{{NumberFormatException}} / \{{IllegalArgumentException}}. Callers in JOSE
and OAuth translate these into protocol errors (for example \{{JwsException}},
\{{OAuthServiceException}}). Map Parsson's \{{JsonParsingException}} to the
same exceptions so error handling and HTTP status codes don't change.
# *Escape decoding.* Unicode escapes, surrogate pairs and escaped keys must
decode the same way. All the cases added in the PRs listed above must keep
passing.
h2. Test plan
* Before changing anything: add characterisation tests for every item in
"Behaviour to preserve", run them against the current implementation, and
commit them first.
* Keep all existing json-basic, JOSE, OAuth2 and OIDC unit tests passing
unchanged, plus the rs-security systests (jose, oauth2, oidc, jwt).
* Add tests for malformed and adversarial input: truncated documents, deep
nesting, huge key and array counts, very long numbers, invalid escapes, lone
surrogates, BOMs and trailing data. Check that each fails with the expected
exception and bounded resource use.
* Optional: run the RFC 7520 (JOSE cookbook) examples end to end.
* Optional: a microbenchmark comparing old and new parse/serialise for typical
JWT and JWK payloads, to confirm there's no regression. The current substring
approach is expected to be slower on large or deeply nested input.
h2. Out of scope
* Changing the public \{{JsonMapObject}} API or the class hierarchy of JOSE /
OAuth model classes.
* Replacing the JSON-B / JSON-P / Jackson providers used for application
payloads.
* Removing the Jettison-based \{{JSONProvider}} (tracked separately).
h2. Acceptance criteria
* \{{JsonMapObjectReaderWriter}} no longer contains any hand-written tokenizing
or escaping code. All parsing and generation goes through JSON-P.
* All characterisation, unit and systests pass, and compact serialisation of
existing JOSE test vectors is byte-identical.
* The duplicate-header rejection in JOSE still works (covered by a test).
* The existing limits and system properties still work and are documented.
* Release notes and the migration guide mention the new dependency
(jakarta.json-api and Parsson in cxf-rt-rs-json-basic), along with any
intentional behaviour changes (escaping of non-String scalars, handling of
integers that don't fit in a \{{long}}).
h2. Suggested sub-tasks
# Characterisation tests for current JsonMapObjectReaderWriter behaviour
# JSON-P based reader behind the existing API (including duplicate-key routing
and limits)
# JSON-P based writer behind the existing API (byte-identical compact output)
# Move OAuthJSONProvider and JsonMapObjectProvider onto the new implementation
# Move OpenApiParseUtils off json-basic
# Deprecate / remove the substring-parsing internals
# Documentation: release notes, migration guide, THREAT_MODEL.md note
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
