From 5eebc4cddce0982976031c6a8edb09047e99ea30 Mon Sep 17 00:00:00 2001 From: PJ Fanning Date: Thu, 3 Sep 2026 09:14:18 +0100 Subject: [PATCH] fix: don't resolve HOCON includes in config that arrived in a message (#3505) * fix: don't resolve HOCON includes in config that arrived in a message Motivation: Three sites parse HOCON that came off the wire with the default parse options: InternalClusterAction.InitJoin and InitJoinAck in ClusterMessageSerializer, and the Config payload in MiscMessageSerializer. HOCON include directives are resolved by the parser rather than by resolve(), so include file(...) and include classpath(...) read from the local filesystem and classpath and include url(...) performs an outbound request, all while deserializing a peer's message. InitJoin is accepted from a node that has not joined, in ClusterDaemon's uninitialized state. Modification: Add WireConfig (@InternalApi), which parses with a ConfigIncluder that resolves every include to an empty object, and route the three sites through it. The includer implements ConfigIncluderFile, ConfigIncluderURL and ConfigIncluderClasspath as well as ConfigIncluder: the parser falls back to its own handling, which does read the resource, for any of the typed forms the configured includer does not implement. Every serializer writes config with ConfigRenderOptions.concise, which renders JSON and cannot produce an include, so a well-behaved sender is unaffected. Result: Deserializing a message no longer reads local files or issues outbound requests on behalf of the sender. * Update WireConfigSpec.scala --- .../pekko/serialization/WireConfigSpec.scala | 93 +++++++++++++++++++ .../pekko/serialization/WireConfig.scala | 82 ++++++++++++++++ .../protobuf/ClusterMessageSerializer.scala | 4 +- .../ClusterMessageSerializerSpec.scala | 35 +++++++ .../serialization/MiscMessageSerializer.scala | 10 +- .../MiscMessageSerializerSpec.scala | 21 ++++- 6 files changed, 240 insertions(+), 5 deletions(-) create mode 100644 actor-tests/src/test/scala/org/apache/pekko/serialization/WireConfigSpec.scala create mode 100644 actor/src/main/scala/org/apache/pekko/serialization/WireConfig.scala diff --git a/actor-tests/src/test/scala/org/apache/pekko/serialization/WireConfigSpec.scala b/actor-tests/src/test/scala/org/apache/pekko/serialization/WireConfigSpec.scala new file mode 100644 index 00000000000..cf0ab9831cc --- /dev/null +++ b/actor-tests/src/test/scala/org/apache/pekko/serialization/WireConfigSpec.scala @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.pekko.serialization + +import java.nio.charset.StandardCharsets +import java.nio.file.{ Files, Path } + +import org.apache.pekko.util.ccompat.JavaConverters._ + +import org.apache.pekko.testkit.PekkoSpec + +import com.typesafe.config.{ ConfigFactory, ConfigRenderOptions } + +class WireConfigSpec extends PekkoSpec { + + // a file the parser must not read when a message asks it to + private val secretFile: Path = { + val f = Files.createTempFile("wire-config-spec", ".conf") + Files.write(f, "secret = leaked".getBytes(StandardCharsets.UTF_8)) + f + } + private val filePath = secretFile.toAbsolutePath.toString + private val fileUrl = secretFile.toUri.toString + + override def afterTermination(): Unit = Files.deleteIfExists(secretFile) + + "WireConfig" must { + + "parse ordinary HOCON" in { + val config = WireConfig.parseString("a = 1\nb { c = two }") + config.getInt("a") should ===(1) + config.getString("b.c") should ===("two") + } + + "parse what a serializer writes" in { + // every serializer renders config with ConfigRenderOptions.concise + val rendered = + ConfigFactory.parseString("pekko.cluster.roles = [a, b]").root.render(ConfigRenderOptions.concise()) + WireConfig.parseString(rendered).getStringList("pekko.cluster.roles").asScala.toList should ===(List("a", "b")) + } + + "not read a file named by an include" in { + // s"...\"..." is not valid Scala 2.12, hence the triple quotes + val config = WireConfig.parseString(s"""include file("$filePath") + |a = 1""".stripMargin) + config.hasPath("secret") should ===(false) + config.getInt("a") should ===(1) + } + + "not read a file named by a required include" in { + WireConfig + .parseString(s"""include required(file("$filePath")) + |a = 1""".stripMargin) + .hasPath("secret") should ===(false) + } + + "not fetch a URL named by an include" in { + // a file: URL stands in for an outbound request, so the test needs no network + val config = WireConfig.parseString(s"""include url("$fileUrl") + |a = 1""".stripMargin) + config.hasPath("secret") should ===(false) + config.getInt("a") should ===(1) + } + + "not read a resource named by a classpath include" in { + // reference.conf is on the test classpath, so the default includer would pull it in + WireConfig.parseString("include classpath(\"reference.conf\")\na = 1").hasPath("pekko.version") should ===(false) + } + + "differ from the default parser, which does resolve all three" in { + // guards the premise of the tests above: these directives really do resolve without the + // includer, so those tests are checking the change rather than an inert directive + ConfigFactory.parseString(s"""include file("$filePath")""").hasPath("secret") should ===(true) + ConfigFactory.parseString(s"""include url("$fileUrl")""").hasPath("secret") should ===(true) + ConfigFactory.parseString("include classpath(\"reference.conf\")").hasPath("pekko.version") should ===(true) + } + } +} diff --git a/actor/src/main/scala/org/apache/pekko/serialization/WireConfig.scala b/actor/src/main/scala/org/apache/pekko/serialization/WireConfig.scala new file mode 100644 index 00000000000..673f56df343 --- /dev/null +++ b/actor/src/main/scala/org/apache/pekko/serialization/WireConfig.scala @@ -0,0 +1,82 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.pekko.serialization + +import java.io.File +import java.net.URL + +import org.apache.pekko.annotation.InternalApi + +import com.typesafe.config.{ + Config, + ConfigFactory, + ConfigIncludeContext, + ConfigIncluder, + ConfigIncluderClasspath, + ConfigIncluderFile, + ConfigIncluderURL, + ConfigObject, + ConfigParseOptions +} + +/** + * INTERNAL API + * + * Parsing of HOCON that arrived in a message. + * + * HOCON `include` directives are resolved by the parser, not by `resolve()`, so parsing a + * string with the default includer reads whatever it names: `include file(...)` and + * `include classpath(...)` read from the local filesystem and classpath, and + * `include url(...)` performs an outbound request. None of that belongs on a path whose + * input came from a peer. + * + * Every serializer writes config with `ConfigRenderOptions.concise`, which renders JSON and + * cannot produce an `include`, so dropping them costs a well-behaved sender nothing. + */ +@InternalApi private[pekko] object WireConfig { + + /** + * Resolves every form of `include` to an empty object. + * + * All four interfaces have to be implemented: the parser dispatches `include file(...)`, + * `include url(...)` and `include classpath(...)` to the typed methods and falls back to + * its own default handling — which does read the resource — when the configured includer + * does not implement the matching interface. Only bare `include "..."` goes to `include`. + */ + private object NoIncludes + extends ConfigIncluder + with ConfigIncluderFile + with ConfigIncluderURL + with ConfigIncluderClasspath { + + private def empty: ConfigObject = ConfigFactory.empty().root() + + override def withFallback(fallback: ConfigIncluder): ConfigIncluder = this + override def include(context: ConfigIncludeContext, what: String): ConfigObject = empty + override def includeFile(context: ConfigIncludeContext, what: File): ConfigObject = empty + override def includeURL(context: ConfigIncludeContext, what: URL): ConfigObject = empty + override def includeResources(context: ConfigIncludeContext, what: String): ConfigObject = empty + } + + private val parseOptions: ConfigParseOptions = ConfigParseOptions.defaults().setIncluder(NoIncludes) + + /** + * Like `ConfigFactory.parseString`, but with `include` directives resolved to nothing. + */ + def parseString(hocon: String): Config = ConfigFactory.parseString(hocon, parseOptions) +} diff --git a/cluster/src/main/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializer.scala b/cluster/src/main/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializer.scala index 48c0d4a83d5..557f9a274b5 100644 --- a/cluster/src/main/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializer.scala +++ b/cluster/src/main/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializer.scala @@ -314,7 +314,7 @@ final class ClusterMessageSerializer(val system: ExtendedActorSystem) private def deserializeInitJoin(bytes: Array[Byte]): InternalClusterAction.InitJoin = { val m = cm.InitJoin.parseFrom(bytes) if (m.hasCurrentConfig) - InternalClusterAction.InitJoin(ConfigFactory.parseString(m.getCurrentConfig)) + InternalClusterAction.InitJoin(WireConfig.parseString(m.getCurrentConfig)) else InternalClusterAction.InitJoin(ConfigFactory.empty) } @@ -325,7 +325,7 @@ final class ClusterMessageSerializer(val system: ExtendedActorSystem) val configCheck = i.getConfigCheck.getType match { case cm.ConfigCheck.Type.CompatibleConfig => - CompatibleConfig(ConfigFactory.parseString(i.getConfigCheck.getClusterConfig)) + CompatibleConfig(WireConfig.parseString(i.getConfigCheck.getClusterConfig)) case cm.ConfigCheck.Type.IncompatibleConfig => IncompatibleConfig case cm.ConfigCheck.Type.UncheckedConfig => UncheckedConfig } diff --git a/cluster/src/test/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializerSpec.scala b/cluster/src/test/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializerSpec.scala index 7615de9585c..6bb4b07707b 100644 --- a/cluster/src/test/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializerSpec.scala +++ b/cluster/src/test/scala/org/apache/pekko/cluster/protobuf/ClusterMessageSerializerSpec.scala @@ -13,6 +13,9 @@ package org.apache.pekko.cluster.protobuf +import java.nio.charset.StandardCharsets +import java.nio.file.Files + import collection.immutable.SortedSet import scala.annotation.nowarn @@ -22,6 +25,7 @@ import org.apache.pekko import pekko.actor.{ Address, ExtendedActorSystem } import pekko.cluster._ import pekko.cluster.InternalClusterAction.CompatibleConfig +import pekko.cluster.protobuf.msg.{ ClusterMessages => cm } import pekko.cluster.routing.{ ClusterRouterPool, ClusterRouterPoolSettings } import pekko.routing.RoundRobinPool import pekko.testkit.PekkoSpec @@ -183,6 +187,37 @@ class ClusterMessageSerializerSpec extends PekkoSpec("pekko.actor.provider = clu env.gossip.members.tail.head.roles should be(Set("r1", ClusterSettings.DcRolePrefix + "foo")) } + "not resolve includes in the config of a join message" in { + // The joining node renders its config with ConfigRenderOptions.concise, which is JSON and + // cannot carry an include, so nothing legitimate is lost by refusing to resolve one. An + // include that did resolve would read a local file or issue an outbound request while + // deserializing a message from a node that has not joined yet. + val secretFile = Files.createTempFile("cluster-message-serializer-spec", ".conf") + try { + Files.write(secretFile, """secret = "leaked"""".getBytes(StandardCharsets.UTF_8)) + val hocon = s"""include file("${secretFile.toAbsolutePath}") + pekko.cluster.roles = []""" + + val initJoin = serializer + .fromBinary(cm.InitJoin.newBuilder().setCurrentConfig(hocon).build().toByteArray, "IJ") + .asInstanceOf[InternalClusterAction.InitJoin] + initJoin.configOfJoiningNode.hasPath("secret") should ===(false) + + val ackBytes = cm.InitJoinAck + .newBuilder() + .setAddress(serializer.addressToProto(Address("pekko", "system", "some.host.org", 4711))) + .setConfigCheck( + cm.ConfigCheck + .newBuilder() + .setType(cm.ConfigCheck.Type.CompatibleConfig) + .setClusterConfig(hocon)) + .build() + .toByteArray + val ack = serializer.fromBinary(ackBytes, "IJA").asInstanceOf[InternalClusterAction.InitJoinAck] + ack.configCheck.asInstanceOf[CompatibleConfig].clusterConfig.hasPath("secret") should ===(false) + } finally Files.deleteIfExists(secretFile) + } + "add a default data center role to internal join action if none is present" in { val join = roundtrip(InternalClusterAction.Join(a1.uniqueAddress, Set(), Version.Zero)) join.roles should be(Set(ClusterSettings.DcRolePrefix + "default")) diff --git a/remote/src/main/scala/org/apache/pekko/remote/serialization/MiscMessageSerializer.scala b/remote/src/main/scala/org/apache/pekko/remote/serialization/MiscMessageSerializer.scala index 70c6c925916..ad86f34b343 100644 --- a/remote/src/main/scala/org/apache/pekko/remote/serialization/MiscMessageSerializer.scala +++ b/remote/src/main/scala/org/apache/pekko/remote/serialization/MiscMessageSerializer.scala @@ -29,7 +29,13 @@ import pekko.remote._ import pekko.remote.WireFormats.AddressData import pekko.remote.routing.RemoteRouterConfig import pekko.routing._ -import pekko.serialization.{ BaseSerializer, Serialization, SerializationExtension, SerializerWithStringManifest } +import pekko.serialization.{ + BaseSerializer, + Serialization, + SerializationExtension, + SerializerWithStringManifest, + WireConfig +} import pekko.util.ccompat.JavaConverters._ class MiscMessageSerializer(val system: ExtendedActorSystem) extends SerializerWithStringManifest with BaseSerializer { @@ -544,7 +550,7 @@ class MiscMessageSerializer(val system: ExtendedActorSystem) extends SerializerW private def deserializeConfig(bytes: Array[Byte]): Config = { if (bytes.isEmpty) EmptyConfig - else ConfigFactory.parseString(new String(bytes, StandardCharsets.UTF_8)) + else WireConfig.parseString(new String(bytes, StandardCharsets.UTF_8)) } private def deserializeFromConfig(bytes: Array[Byte]): FromConfig = diff --git a/remote/src/test/scala/org/apache/pekko/remote/serialization/MiscMessageSerializerSpec.scala b/remote/src/test/scala/org/apache/pekko/remote/serialization/MiscMessageSerializerSpec.scala index 570e3f8d4c7..78b808a5e4d 100644 --- a/remote/src/test/scala/org/apache/pekko/remote/serialization/MiscMessageSerializerSpec.scala +++ b/remote/src/test/scala/org/apache/pekko/remote/serialization/MiscMessageSerializerSpec.scala @@ -14,6 +14,8 @@ package org.apache.pekko.remote.serialization import java.io.NotSerializableException +import java.nio.charset.StandardCharsets +import java.nio.file.Files import java.util.Optional import java.util.concurrent.TimeoutException @@ -21,7 +23,7 @@ import scala.annotation.nowarn import scala.concurrent.duration._ import scala.util.control.NoStackTrace -import com.typesafe.config.ConfigFactory +import com.typesafe.config.{ Config, ConfigFactory } import org.apache.pekko import pekko.{ Done, NotUsed } import pekko.actor._ @@ -157,6 +159,23 @@ class MiscMessageSerializerSpec extends PekkoSpec(MiscMessageSerializerSpec.test } } + "not resolve includes in a serialized Config" in { + // Config is written with ConfigRenderOptions.concise, which is JSON and cannot carry an + // include, so refusing to resolve one loses nothing. An include that did resolve would + // read a local file or issue an outbound request while deserializing a peer's message. + val secretFile = Files.createTempFile("misc-message-serializer-spec", ".conf") + try { + Files.write(secretFile, """secret = "leaked"""".getBytes(StandardCharsets.UTF_8)) + val serializer = new MiscMessageSerializer(system.asInstanceOf[ExtendedActorSystem]) + val hocon = s"""include file("${secretFile.toAbsolutePath}") + a = 1""" + + val config = serializer.fromBinary(hocon.getBytes(StandardCharsets.UTF_8), "CF").asInstanceOf[Config] + config.hasPath("secret") should ===(false) + config.getInt("a") should ===(1) + } finally Files.deleteIfExists(secretFile) + } + "reject invalid manifest" in { intercept[IllegalArgumentException] { val serializer = new MiscMessageSerializer(system.asInstanceOf[ExtendedActorSystem])