From 797243b83757eddda3ee7523036f4a437c819668 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Wed, 19 Apr 2023 11:08:44 -0400 Subject: [PATCH 1/2] Apply an excessively permissive large string length limit by default This allows us to bridge the difference between jackson 2.14 and 2.15 such that we are able to safely upgrade, even if the upgrade occurs before CJR is prepared to take the upgrade itself. We will ratchet down the default over time. --- .../java/serialization/ObjectMappers.java | 4 +- .../ReflectiveStreamReadConstraints.java | 76 +++++++++++++++++++ .../java/serialization/ObjectMappersTest.java | 9 +++ 3 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ReflectiveStreamReadConstraints.java diff --git a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java index e6f7361ab..371df336d 100644 --- a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java +++ b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ObjectMappers.java @@ -243,8 +243,8 @@ public static CBORFactory cborFactory() { /** Configures provided JsonFactory with Conjure default settings. */ private static > B withDefaults(B builder) { - return builder + return ReflectiveStreamReadConstraints.withDefaultConstraints(builder // Interning introduces excessive contention https://github.com/FasterXML/jackson-core/issues/946 - .disable(JsonFactory.Feature.INTERN_FIELD_NAMES); + .disable(JsonFactory.Feature.INTERN_FIELD_NAMES)); } } diff --git a/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ReflectiveStreamReadConstraints.java b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ReflectiveStreamReadConstraints.java new file mode 100644 index 000000000..f7fc25feb --- /dev/null +++ b/conjure-java-jackson-serialization/src/main/java/com/palantir/conjure/java/serialization/ReflectiveStreamReadConstraints.java @@ -0,0 +1,76 @@ +/* + * (c) Copyright 2023 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.serialization; + +import com.fasterxml.jackson.core.JsonFactory; +import com.fasterxml.jackson.core.TSFBuilder; +import com.palantir.logsafe.logger.SafeLogger; +import com.palantir.logsafe.logger.SafeLoggerFactory; +import java.lang.reflect.Method; + +/** + * This class exists to ensure default values match our expectations in cases where jackson is + * upgraded transitively prior to a CJR release which expects 2.15.0+. After this library upgrades, the + * reflection may be replaced by the following: + *
{@code
+ * return builder.streamReadConstraints(StreamReadConstraints.builder()
+ *         .maxStringLength(MAX_STRING_LENGTH)
+ *         .build());
+ * }
+ */ +final class ReflectiveStreamReadConstraints { + private static final SafeLogger log = SafeLoggerFactory.get(ReflectiveStreamReadConstraints.class); + + // 50mb up from the default 5mb as a more permissive value to begin with, which we can ratchet down over time. + // This allows us to decouple the initial risk of adopting string length limits from the risk introduced by taking + // a dependency upgrade. + private static final int MAX_STRING_LENGTH = 50_000_000; + + @SuppressWarnings("unchecked") + static > B withDefaultConstraints(B builder) { + try { + // Use the same classloader which loaded the TSFBuilder + ClassLoader classLoader = builder.getClass().getClassLoader(); + Class streamReadConstraintsClass = + Class.forName("com.fasterxml.jackson.core.StreamReadConstraints", true, classLoader); + Object constraints = createConjureDefaultStreamReadConstraints(streamReadConstraintsClass); + return (B) builder.getClass() + .getMethod("streamReadConstraints", streamReadConstraintsClass) + .invoke(builder, constraints); + } catch (ClassNotFoundException cnfe) { + // Log at debug in the expected case using jackson 2.14 which does not support limits. + log.debug("StreamReadConstraints class does not exist, nothing to do", cnfe); + } catch (ReflectiveOperationException e) { + log.warn("Failed to update StreamReadConstraints, upstream default values will be used", e); + } + return builder; + } + + private static Object createConjureDefaultStreamReadConstraints(Class streamReadConstraintsClass) + throws ReflectiveOperationException { + Method builderMethod = streamReadConstraintsClass.getMethod("builder"); + Object streamReadConstraintsBuilder = builderMethod.invoke(null); + Class streamReadConstraintsBuilderClass = streamReadConstraintsBuilder.getClass(); + // Default to a max size of 50mb (up from the 5mb default) + streamReadConstraintsBuilderClass + .getMethod("maxStringLength", int.class) + .invoke(streamReadConstraintsBuilder, MAX_STRING_LENGTH); + return streamReadConstraintsBuilderClass.getMethod("build").invoke(streamReadConstraintsBuilder); + } + + private ReflectiveStreamReadConstraints() {} +} diff --git a/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java b/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java index 71924b392..d3b5bea2a 100644 --- a/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java +++ b/conjure-java-jackson-serialization/src/test/java/com/palantir/conjure/java/serialization/ObjectMappersTest.java @@ -497,6 +497,15 @@ private void testMapKeysAreNotInterned(ObjectMapper mapper) throws IOException { .isEmpty(); } + @Test + public void testExtraordinarilyLargeStrings() throws IOException { + // Value must sit between StreamReadConstraints.DEFAULT_MAX_STRING_LEN and + // ReflectiveStreamReadConstraints.MAX_STRING_LENGTH. + int size = 10_000_000; + String parsed = ObjectMappers.newServerJsonMapper().readValue('"' + "a".repeat(size) + '"', String.class); + assertThat(parsed).hasSize(size); + } + private static String ser(Object object) throws IOException { return MAPPER.writeValueAsString(object); } From d1e447db6dd4c6d686983b2b5df3bf6c58955beb Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Wed, 19 Apr 2023 15:15:35 +0000 Subject: [PATCH 2/2] Add generated changelog entries --- changelog/@unreleased/pr-2601.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-2601.v2.yml diff --git a/changelog/@unreleased/pr-2601.v2.yml b/changelog/@unreleased/pr-2601.v2.yml new file mode 100644 index 000000000..c5f09477d --- /dev/null +++ b/changelog/@unreleased/pr-2601.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Apply a large StreamReadConstraints maxStringLength to reduce friction + in preparation for jackson 2.15 adoption + links: + - https://github.com/palantir/conjure-java-runtime/pull/2601