Add support for nested maven properties

Extends the Maven parsing logic to support nested properties

Fixes #13713

Author: yeikel

maven/lib/dependabot/maven/file_parser/property_value_finder.rb

@@ -20,6 +20,8 @@ class PropertyValueFinder
         require_relative "pom_fetcher"
 
         DOT_SEPARATOR_REGEX = %r{\.(?!\d+([.\/_\-]|$)+)}
+        # Matches ${...}
+        MAVEN_PROPERTY_REGEX = /\$\{.+?/
 
         sig do
           params(
@@ -37,10 +39,15 @@ def initialize(dependency_files:, credentials: [])
           )
         end
 
+        # rubocop:disable Metrics/PerceivedComplexity
         sig do
-          params(property_name: String, callsite_pom: DependencyFile).returns(T.nilable(T::Hash[Symbol, T.untyped]))
+          params(
+            property_name: String,
+            callsite_pom: DependencyFile,
+            seen_properties: T::Set[String]
+          ).returns(T.nilable(T::Hash[Symbol, T.untyped]))
         end
-        def property_details(property_name:, callsite_pom:)
+        def property_details(property_name:, callsite_pom:, seen_properties: Set.new)
           pom = callsite_pom
           doc = Nokogiri::XML(pom.content)
           doc.remove_namespaces!
@@ -63,53 +70,66 @@ def property_details(property_name:, callsite_pom:)
               raise DependencyFileNotEvaluatable, e.message
             end
 
-          # and value is an expression
-          if node && /\$\{(?<expression>.+)\}/.match(node.content.strip)
-            return extract_value_from_expression(
-              expression: node.content.strip,
+          if node.nil? && parent_pom(pom)
+            return property_details(
               property_name: property_name,
-              callsite_pom: callsite_pom
+              callsite_pom: T.must(parent_pom(pom)),
+              seen_properties: seen_properties
             )
           end
+          return nil unless node
 
-          # If we found a property, return it
-          return { file: pom.name, node: node, value: node.content.strip } if node
+          content = node.content.strip
 
-          # Otherwise, look for a value in this pom's parent
-          return unless (parent = parent_pom(pom))
+          # Detect infinite recursion such as ${property1} where property1=${property1}
+          if seen_properties.include?(property_name)
+            raise Dependabot::DependencyFileNotParseable.new(
+              callsite_pom.name,
+              "Error trying to resolve recursive expression '${#{property_name}}'."
+            )
+          end
 
-          property_details(
-            property_name: property_name,
-            callsite_pom: parent
-          )
+          seen_properties << property_name
+
+          # If the content has no placeholders, return it as-is
+          return { file: pom.name, node: node, value: content } unless content.match?(MAVEN_PROPERTY_REGEX)
+
+          resolve_property_placeholder(content, callsite_pom, pom, node, seen_properties)
         end
+        # rubocop:enable Metrics/PerceivedComplexity
 
         private
 
-        sig { returns(T::Array[DependencyFile]) }
-        attr_reader :dependency_files
-
+        # Extract property placeholders from a string and resolve them
+        # These properties can be simple properties such as ${project.version}
+        # or more complex such as ${my.property.${other.property}} or constant.${property}
+        # See https://maven.apache.org/pom.html#properties
         sig do
           params(
-            expression: String,
-            property_name: String,
-            callsite_pom: DependencyFile
-          )
-            .returns(T.nilable(T::Hash[Symbol, String]))
+            content: String,
+            callsite_pom: DependencyFile,
+            pom: DependencyFile,
+            node: T.untyped,
+            seen_properties: T::Set[String]
+          ).returns(T.nilable(T::Hash[Symbol, T.untyped]))
         end
-        def extract_value_from_expression(expression:, property_name:, callsite_pom:)
-          # and the expression is pointing to self then raise the error
-          if expression.eql?("${#{property_name}}")
-            raise Dependabot::DependencyFileNotParseable.new(
-              callsite_pom.name,
-              "Error trying to resolve recursive expression '#{expression}'."
+        def resolve_property_placeholder(content, callsite_pom, pom, node, seen_properties)
+          resolved_value = content.gsub(/\$\{(.+?)}/) do
+            inner_name = Regexp.last_match(1)
+            resolved = property_details(
+              property_name: T.must(inner_name),
+              callsite_pom: callsite_pom,
+              seen_properties: seen_properties
             )
+            T.must(resolved)[:value]
           end
 
-          # and the expression is pointing to another tag, then get the value of that tag
-          property_details(property_name: T.must(expression.slice(2..-2)), callsite_pom: callsite_pom)
+          { file: pom.name, node: node, value: resolved_value }
         end
 
+        sig { returns(T::Array[DependencyFile]) }
+        attr_reader :dependency_files
+
         sig { params(property_name: String).returns(String) }
         def sanitize_property_name(property_name)
           property_name.sub(/^pom\./, "").sub(/^project\./, "")

maven/spec/dependabot/maven/file_parser/property_value_finder_spec.rb

@@ -39,6 +39,20 @@
         its([:value]) { is_expected.to eq("0.0.2-RELEASE") }
       end
 
+      context "when the property is an nested property using project definition properties" do
+        let(:base_pom_fixture_name) { "pom_with_nested_properties.xml" }
+        let(:property_name) { "channels.maven.groupId" }
+
+        its([:value]) { is_expected.to eq("org.reproducer.channels") }
+      end
+
+      context "when the property is nested multiple times" do
+        let(:base_pom_fixture_name) { "pom_with_nested_properties.xml" }
+        let(:property_name) { "channels.maven.groupId.nested" }
+
+        its([:value]) { is_expected.to eq("org.reproducer.channels2") }
+      end
+
       context "when the property name starts with 'project' but not an attribute of the project" do
         let(:base_pom_fixture_name) { "property_name_starts_with_project_pom.xml" }
         let(:property_name) { "project.dependency.spring-boot.version" }

maven/spec/fixtures/poms/pom_with_nested_properties.xml

@@ -0,0 +1,30 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- This pom tests that properties can reference other properties that reference other properties
+    This is used in some complex projects like WildFly to manage variants of their projects dynamically
+    For more context see https://github.com/dependabot/dependabot-core/issues/13713
+-->
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+  <modelVersion>4.0.0</modelVersion>
+
+  <groupId>org.reproducer</groupId>
+  <artifactId>test</artifactId>
+  <version>1.0.0-SNAPSHOT</version>
+
+  <properties>
+    <channels>channels2</channels>
+    <channels.maven.groupId.nested>${project.groupId}.${channels}</channels.maven.groupId.nested>
+    <channels.maven.groupId>${project.groupId}.channels</channels.maven.groupId>
+  </properties>
+
+  <dependencyManagement>
+    <dependencies>
+
+      <dependency>
+        <groupId>${channels.maven.groupId}</groupId>
+        <artifactId>wildfly-preview</artifactId>
+        <version>19.0.0.Final</version>
+      </dependency>
+
+    </dependencies>
+  </dependencyManagement>
+</project>