opentelemetry-configuration: substitute env vars after parsing (#5406)

.changelog/5407.fixed

@@ -0,0 +1 @@
+`opentelemetry-configuration`: perform environment variable substitution on scalar values after parsing the configuration file, so `${VAR}` references inside comments and mapping keys are no longer substituted and undefined references in comments no longer abort loading

opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py

@@ -8,9 +8,24 @@
 
 
 def substitute_env_vars(text: str) -> str:
-    """Substitute environment variables in configuration text.
+    """Substitute environment variables within a configuration value.
+
+    A configuration value is a single value from the parsed configuration file
+    (the value of one key or one list item), never a key, a comment, or a whole
+    mapping/list. Substitution is applied per configuration value after the
+    file has been parsed, so comments and mapping keys are never touched.
+
+    For example, given the YAML::
+
+        service_name: ${SERVICE_NAME}
+        endpoint: http://${HOST}:${PORT}
+
+    the configuration values are ``${SERVICE_NAME}`` and
+    ``http://${HOST}:${PORT}``; this function is called once with each, and
+    never with the keys ``service_name`` or ``endpoint``.
 
     Supports the following syntax:
+
     - ${VAR}: Substitute with environment variable VAR, or an empty value if
       VAR is not set.
     - ${VAR:-default}: Substitute with VAR if set, otherwise use default value.
@@ -23,19 +38,19 @@ def substitute_env_vars(text: str) -> str:
     files be shared across languages.
 
     Args:
-        text: Configuration text with potential ${VAR} placeholders.
+        text: A configuration value with potential ${VAR} placeholders.
 
     Returns:
-        Text with environment variables substituted.
+        The configuration value with environment variables substituted.
 
     Examples:
         >>> os.environ["SERVICE_NAME"] = "my-service"
-        >>> substitute_env_vars("name: ${SERVICE_NAME}")
-        'name: my-service'
-        >>> substitute_env_vars("name: ${MISSING:-default}")
-        'name: default'
-        >>> substitute_env_vars("price: $$100")
-        'price: $100'
+        >>> substitute_env_vars("${SERVICE_NAME}")
+        'my-service'
+        >>> substitute_env_vars("${MISSING:-default}")
+        'default'
+        >>> substitute_env_vars("$$100")
+        '$100'
     """
     # Pattern matches $$ (escape sequence) or ${VAR_NAME} / ${VAR_NAME:-default_value}
     # Handling both in a single pass ensures $$ followed by ${VAR} works correctly
@@ -57,23 +72,11 @@ def replace_var(match) -> str:
             # provided, otherwise with an empty value, per the spec.
             return default_value or ""
 
-        # Per spec: "It MUST NOT be possible to inject YAML structures by
-        # environment variables." Newlines are the primary injection vector —
-        # a value like "legit\nmalicious_key: val" would create extra YAML
-        # keys if substituted verbatim. Wrap such values in a YAML
-        # double-quoted scalar so the newline is treated as literal text.
-        # Simple values (no newlines) are returned as-is so that YAML type
-        # coercion still applies per spec ("Node types MUST be interpreted
-        # after environment variable substitution takes place").
-        if "\n" in value or "\r" in value:
-            escaped = (
-                value.replace("\\", "\\\\")
-                .replace('"', '\\"')
-                .replace("\n", "\\n")
-                .replace("\r", "\\r")
-                .replace("\t", "\\t")
-            )
-            return f'"{escaped}"'
+        # Substitution runs on an already-parsed configuration value, so the
+        # result cannot inject new YAML structure regardless of its contents
+        # (a newline in the value stays a literal character within this one
+        # value). Type interpretation for standalone references is handled by
+        # the loader, which re-resolves the node tag after substitution.
         return value
 
     return re.sub(pattern, replace_var, text)

opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py

@@ -7,6 +7,7 @@
 import json
 import logging
 import os
+import re
 from pathlib import Path
 from typing import Any
 
@@ -60,14 +61,167 @@ def _get_schema() -> dict:
 
 _logger = logging.getLogger(__name__)
 
+_YAML_STR_TAG = "tag:yaml.org,2002:str"
+
+# A configuration value whose entire content is a single ``${VAR}`` /
+# ``${VAR:-default}`` reference. Only such standalone references (when
+# unquoted) have their type re-interpreted after substitution; embedded or
+# multiple references resolve to a string per the configuration spec. A
+# leading ``$$`` escape does not match, so ``$${VAR}`` is treated as an
+# embedded (string) value.
+_STANDALONE_ENV_REF = re.compile(r"\A\$\{[A-Za-z_][A-Za-z0-9_]*(?::-[^}]*)?\}\Z")
+
+
+def _substitute_env_in_yaml_node(
+    node: yaml.Node,
+    loader: yaml.SafeLoader,
+    visited: set[int] | None = None,
+):
+    """Apply env-var substitution to string configuration values in a node tree.
+
+    Substitution runs after parsing on configuration values only, so comments
+    and mapping keys are never candidates (per the configuration spec). For an
+    unquoted standalone ``${VAR}`` reference the node's type tag is re-resolved
+    from the substituted value so YAML type coercion still applies (e.g.
+    ``${LIMIT}`` -> int); quoted or embedded references stay strings.
+
+    Args:
+        node: Root of the node tree to walk. Substitution happens in place.
+        loader: The loader that composed ``node``; used to re-resolve tags.
+        visited: Ids of nodes already processed in this traversal. Callers pass
+            nothing; the walker creates and threads one set through the whole
+            tree.
+    """
+    # Every anchor/alias pair shares one composed node, so the same node is
+    # reached once per reference to it. Each node must be substituted exactly
+    # once for two reasons:
+    #
+    #   1. Substituting twice re-reads the output of the first pass. An
+    #      anchored ``$${TOKEN}`` becomes the escaped literal ``${TOKEN}`` on
+    #      the first visit, which the second visit then resolves as a real
+    #      variable -- silently defeating the ``$$`` escape.
+    #   2. A cyclic alias (``a: &anchor {self: *anchor}``) makes the node tree
+    #      a graph with a loop, so an unguarded walk recurses until it raises
+    #      ``RecursionError``.
+    #
+    # Tracking ``id(node)`` is safe here because ``root_node`` keeps every node
+    # in the tree alive for the whole traversal, so no id can be reused.
+    if visited is None:
+        visited = set()
+    if id(node) in visited:
+        return
+    visited.add(id(node))
+    # Worked example: a node value that makes all three checks below true.
+    #
+    # YAML (with environment LIMIT=100):
+    #     attribute_limits:
+    #       attribute_count_limit: ${LIMIT}
+    #
+    # The parser hands this function the value node of attribute_count_limit,
+    # which has value="${LIMIT}", tag=str, style=None (plain/unquoted). Then:
+    #
+    #   1. isinstance(node, yaml.ScalarNode) -> True: it is a leaf value.
+    #   2. node.tag == _YAML_STR_TAG -> True: "${LIMIT}" is not a number or
+    #      bool, so the parser tagged it str. We substitute in place, giving
+    #      node.value = "100".
+    #   3. node.style is None and _STANDALONE_ENV_REF.match(raw) -> True:
+    #      it was unquoted and the whole value is a single reference. We
+    #      re-resolve the tag: resolve("100") -> int, and construct_document()
+    #      later builds the integer 100 (not the string "100").
+    #
+    # Counter-cases that reach check 3 but stop there: "${LIMIT}" quoted fails
+    # the style test; x${LIMIT} fails the regex. Both keep the str tag.
+    if isinstance(node, yaml.ScalarNode):
+        # A ``ScalarNode`` is a leaf -- one configuration value. Only a
+        # ``str``-tagged one can hold a ``${VAR}`` reference (nodes the parser
+        # already typed as int, float, bool or null cannot), so those are the
+        # only substitution candidates.
+        if node.tag == _YAML_STR_TAG:
+            # ``node.value`` is the raw text of this one value (never a key or
+            # comment). Replace any ${VAR} / ${VAR:-default} / $$ in it.
+            raw = node.value
+            node.value = substitute_env_vars(raw)
+            # The spec says node types are interpreted *after* substitution,
+            # but only for a value that is exactly one reference and written
+            # unquoted (``style is None``). Example: ``count: ${N}`` with
+            # ``N=42`` must yield the int 42, not the string "42". In that case
+            # re-run YAML's implicit type resolution on the substituted text
+            # and retag the node. A quoted (``"${N}"``) or embedded
+            # (``foo-${N}``) value keeps its ``str`` tag and stays a string.
+            if node.style is None and _STANDALONE_ENV_REF.match(raw):
+                # ``resolve`` returns the implicit tag (int/float/bool/null/
+                # str) YAML would assign to ``node.value``; the ``(True, False)``
+                # implicit-flag pair marks it as a plain scalar so that
+                # type-guessing applies, exactly as during normal parsing.
+                node.tag = loader.resolve(yaml.ScalarNode, node.value, (True, False))
+    elif isinstance(node, yaml.SequenceNode):
+        # A sequence (list): each item is itself a value node -- recurse.
+        for item in node.value:
+            _substitute_env_in_yaml_node(item, loader, visited)
+    elif isinstance(node, yaml.MappingNode):
+        # A mapping (dict): recurse into values only; keys are not
+        # substitution candidates per the spec. A merge key (``<<: *base``)
+        # appears here as an ordinary pair whose value node is the anchored
+        # mapping itself, so ``visited`` also covers the merged-in values.
+        for _key_node, value_node in node.value:
+            _substitute_env_in_yaml_node(value_node, loader, visited)
+
+
+def _substitute_env_in_json_value(value: Any) -> Any:
+    """Recursively apply env-var substitution to string values in JSON data.
+
+    JSON has explicit types and no comments, so substitution applies only to
+    string values (the result stays a string) and mapping keys are left as-is.
+    """
+    if isinstance(value, str):
+        return substitute_env_vars(value)
+    if isinstance(value, list):
+        return [_substitute_env_in_json_value(item) for item in value]
+    if isinstance(value, dict):
+        return {key: _substitute_env_in_json_value(val) for key, val in value.items()}
+    return value
+
+
+def _parse_config_content(content: str, suffix: str, file_path: str | os.PathLike[str]) -> Any:
+    """Parse configuration text and substitute environment variables.
+
+    Parsing happens first, so ``${VAR}`` references in comments and mapping
+    keys are never substitution candidates; substitution then runs on
+    configuration values, with YAML node types re-resolved for standalone
+    references.
+
+    Raises:
+        ConfigurationError: If the content cannot be parsed or substitution of
+            a required environment variable fails.
+    """
+    try:
+        if suffix == ".json":
+            return _substitute_env_in_json_value(json.loads(content))
+        yaml_loader = yaml.SafeLoader(content)
+        try:
+            root_node = yaml_loader.get_single_node()
+            if root_node is None:
+                return None
+            _substitute_env_in_yaml_node(root_node, yaml_loader)
+            return yaml_loader.construct_document(root_node)
+        finally:
+            yaml_loader.dispose()
+    except yaml.YAMLError as exc:
+        _logger.exception("Failed to parse YAML from %s", file_path)
+        raise ConfigurationError(f"Failed to parse YAML: {exc}") from exc
+    except json.JSONDecodeError as exc:
+        _logger.exception("Failed to parse JSON from %s", file_path)
+        raise ConfigurationError(f"Failed to parse JSON: {exc}") from exc
+
 
 def load_config_file(
     file_path: str | os.PathLike[str],
 ) -> OpenTelemetryConfiguration:
     """Load and parse an OpenTelemetry configuration file.
 
-    Supports YAML and JSON formats. Performs environment variable substitution
-    before parsing.
+    Supports YAML and JSON formats. Environment variable substitution is
+    performed after parsing, on configuration values only, so ``${VAR}``
+    references in comments or mapping keys are left untouched.
 
     Args:
         file_path: Path to the configuration file (.yaml, .yml, or .json).
@@ -99,28 +253,15 @@ def load_config_file(
         _logger.exception("Failed to read configuration file: %s", file_path)
         raise ConfigurationError(f"Failed to read configuration file: {file_path}") from exc
 
-    # Perform environment variable substitution
-    try:
-        content = substitute_env_vars(content)
-    except Exception as exc:
-        raise ConfigurationError(f"Environment variable substitution failed: {exc}") from exc
-
-    # Parse based on file extension
+    # Parse the file, then substitute env vars in configuration values only.
+    # Parsing first means comments and mapping keys are never substitution
+    # candidates, and node types are still resolved after substitution.
     suffix = path.suffix.lower()
-    try:
-        if suffix in (".yaml", ".yml"):
-            data = yaml.safe_load(content)
-        elif suffix == ".json":
-            data = json.loads(content)
-        else:
-            _logger.error("Unsupported file format: %s", suffix)
-            raise ConfigurationError(f"Unsupported file format: {suffix}. Use .yaml, .yml, or .json")
-    except yaml.YAMLError as exc:
-        _logger.exception("Failed to parse YAML from %s", file_path)
-        raise ConfigurationError(f"Failed to parse YAML: {exc}") from exc
-    except json.JSONDecodeError as exc:
-        _logger.exception("Failed to parse JSON from %s", file_path)
-        raise ConfigurationError(f"Failed to parse JSON: {exc}") from exc
+    if suffix not in (".yaml", ".yml", ".json"):
+        _logger.error("Unsupported file format: %s", suffix)
+        raise ConfigurationError(f"Unsupported file format: {suffix}. Use .yaml, .yml, or .json")
+
+    data = _parse_config_content(content, suffix, file_path)
 
     if data is None:
         _logger.error("Configuration file is empty: %s", file_path)

opentelemetry-configuration/tests/file/test_env_substitution.py

@@ -109,35 +109,22 @@ def test_only_dollar_signs(self):
         result = substitute_env_vars("$$$$")
         self.assertEqual(result, "$$")
 
-    def test_newline_in_value_prevents_yaml_injection(self):
-        """Values containing newlines must not inject YAML structure.
+    def test_newline_in_value_returned_verbatim(self):
+        """A newline in a value is returned as-is.
 
-        Per spec: "It MUST NOT be possible to inject YAML structures by
-        environment variables." A value like "legit\\nmalicious_key: val"
-        must be emitted as a quoted scalar, not raw YAML.
+        Substitution runs per configuration value after parsing, so it does
+        not escape or quote newlines; YAML injection is prevented structurally
+        (see the loader tests), not by rewriting the value here.
         """
-        with patch.dict(
-            os.environ,
-            {"SERVICE_NAME": "legit-service\nmalicious_key: injected_value"},
-        ):
-            result = substitute_env_vars("file_format: '1.0'\nservice_name: ${SERVICE_NAME}")
-        parsed = yaml.safe_load(result)
-        self.assertNotIn("malicious_key", parsed)
-        self.assertIn("legit-service", parsed["service_name"])
-
-    def test_newline_in_value_preserved_as_literal(self):
-        """Newline within a value is preserved as a literal newline character."""
         with patch.dict(os.environ, {"MULTI": "line1\nline2"}):
-            result = substitute_env_vars("key: ${MULTI}")
-        parsed = yaml.safe_load(result)
-        self.assertEqual(parsed["key"], "line1\nline2")
+            result = substitute_env_vars("${MULTI}")
+        self.assertEqual(result, "line1\nline2")
 
-    def test_carriage_return_in_value_is_escaped(self):
-        """Carriage return in value is escaped, not injected."""
+    def test_carriage_return_in_value_returned_verbatim(self):
+        """A carriage return in a value is returned as-is, not escaped."""
         with patch.dict(os.environ, {"VAL": "text\r\nmore"}):
-            result = substitute_env_vars("key: ${VAL}")
-        parsed = yaml.safe_load(result)
-        self.assertIsInstance(parsed["key"], str)
+            result = substitute_env_vars("${VAL}")
+        self.assertEqual(result, "text\r\nmore")
 
     def test_type_coercion_preserved_for_simple_values(self):
         """Simple values without newlines still undergo YAML type coercion per spec."""