From 6b8c77da5a1808a413dfc396136c30657c0b030b Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sun, 12 Jul 2026 13:42:31 -0500 Subject: [PATCH 01/10] opentelemetry-configuration: substitute env vars after parsing (#5406) Environment variable substitution ran on the raw configuration file text before parsing, so a ${VAR} reference inside a YAML comment was treated as a real substitution -- an undefined variable in a comment crashed loading, and documenting the substitution mechanism was impossible. Parse the file first, then substitute only within scalar values (never in comments or mapping keys), matching the configuration spec and the Java and Node.js implementations. For an unquoted standalone ${VAR} reference the YAML node's type tag is re-resolved from the substituted value so type coercion still applies (e.g. ${LIMIT} -> int); quoted or embedded references resolve to strings. JSON string values are substituted in place. --- .changelog/5406.fixed | 1 + .../configuration/file/_env_substitution.py | 33 ++--- .../configuration/file/_loader.py | 125 +++++++++++++---- .../tests/file/test_env_substitution.py | 35 ++--- .../tests/file/test_loader.py | 126 ++++++++++++++++++ 5 files changed, 252 insertions(+), 68 deletions(-) create mode 100644 .changelog/5406.fixed diff --git a/.changelog/5406.fixed b/.changelog/5406.fixed new file mode 100644 index 00000000000..e6c5f221467 --- /dev/null +++ b/.changelog/5406.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 diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py index 8b5488233d8..9a83ce6b3a7 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py @@ -8,9 +8,12 @@ def substitute_env_vars(text: str) -> str: - """Substitute environment variables in configuration text. + """Substitute environment variables within a configuration scalar value. + + Substitution is applied per scalar value after the configuration file has + been parsed, so comments and mapping keys are never touched. Supports the + following syntax: - 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,10 +26,10 @@ def substitute_env_vars(text: str) -> str: files be shared across languages. Args: - text: Configuration text with potential ${VAR} placeholders. + text: A scalar value with potential ${VAR} placeholders. Returns: - Text with environment variables substituted. + The value with environment variables substituted. Examples: >>> os.environ["SERVICE_NAME"] = "my-service" @@ -57,23 +60,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 scalar 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 + # scalar). 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) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py index fd1d96ffda0..d81a6f044a3 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py +++ b/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,103 @@ def _get_schema() -> dict: _logger = logging.getLogger(__name__) +_YAML_STR_TAG = "tag:yaml.org,2002:str" + +# A scalar whose entire value 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): + """Apply env-var substitution to string scalar values in a YAML node tree. + + Substitution runs after parsing on scalar *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. + """ + if isinstance(node, yaml.ScalarNode): + if node.tag == _YAML_STR_TAG: + raw = node.value + node.value = substitute_env_vars(raw) + if node.style is None and _STANDALONE_ENV_REF.match(raw): + node.tag = loader.resolve( + yaml.ScalarNode, node.value, (True, False) + ) + elif isinstance(node, yaml.SequenceNode): + for item in node.value: + _substitute_env_in_yaml_node(item, loader) + elif isinstance(node, yaml.MappingNode): + # Recurse into values only; keys are not substitution candidates. + for _key_node, value_node in node.value: + _substitute_env_in_yaml_node(value_node, loader) + + +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 scalar + 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 scalar 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 +189,17 @@ 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 environment variables in scalar values. + # 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) diff --git a/opentelemetry-configuration/tests/file/test_env_substitution.py b/opentelemetry-configuration/tests/file/test_env_substitution.py index ff52b9d704c..6cce731b4aa 100644 --- a/opentelemetry-configuration/tests/file/test_env_substitution.py +++ b/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 scalar 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.""" diff --git a/opentelemetry-configuration/tests/file/test_loader.py b/opentelemetry-configuration/tests/file/test_loader.py index 9b13b371b65..a16cf619333 100644 --- a/opentelemetry-configuration/tests/file/test_loader.py +++ b/opentelemetry-configuration/tests/file/test_loader.py @@ -7,6 +7,8 @@ from pathlib import Path from unittest.mock import patch +import yaml + from opentelemetry.configuration._tracer_provider import ( create_tracer_provider, ) @@ -17,6 +19,8 @@ from opentelemetry.configuration.file._loader import ( _SUPPORTED_SCHEMA_MAJOR, _SUPPORTED_SCHEMA_MINOR, + _substitute_env_in_json_value, + _substitute_env_in_yaml_node, ) from opentelemetry.configuration.models import ( BatchSpanProcessor as BatchSpanProcessorConfig, @@ -383,3 +387,125 @@ def test_malformed_version_is_rejected(self): with self.assertRaises(ConfigurationError) as ctx: self._load("not-a-version") self.assertIn("file_format", str(ctx.exception)) + + +class TestEnvVarSubstitutionScope(unittest.TestCase): + """Substitution applies only to scalar values, per the config spec. + + These exercise the YAML node walker directly so the spec's type and + scope rules can be asserted without the JSON schema constraining shape. + """ + + @staticmethod + def _substitute_yaml(text: str): + loader = yaml.SafeLoader(text) + try: + node = loader.get_single_node() + _substitute_env_in_yaml_node(node, loader) + return loader.construct_document(node) + finally: + loader.dispose() + + def test_unquoted_standalone_reference_is_type_coerced(self): + with patch.dict(os.environ, {"N": "42", "FLAG": "true"}): + result = self._substitute_yaml("count: ${N}\nflag: ${FLAG}") + self.assertEqual(result["count"], 42) + self.assertIsInstance(result["count"], int) + self.assertIs(result["flag"], True) + + def test_quoted_reference_stays_string(self): + with patch.dict(os.environ, {"N": "42"}): + result = self._substitute_yaml('count: "${N}"') + self.assertEqual(result["count"], "42") + + def test_embedded_reference_resolves_to_string(self): + with patch.dict(os.environ, {"N": "42"}): + result = self._substitute_yaml("name: svc-${N}") + self.assertEqual(result["name"], "svc-42") + + def test_mapping_key_is_not_substituted(self): + # A ${VAR} in a key position is left verbatim and triggers no lookup, + # so an undefined variable there does not raise. + with patch.dict(os.environ, {}, clear=True): + result = self._substitute_yaml("${UNDEFINED_KEY}: value") + self.assertEqual(result, {"${UNDEFINED_KEY}": "value"}) + + def test_escape_sequence_is_not_a_reference(self): + with patch.dict(os.environ, {}, clear=True): + result = self._substitute_yaml("literal: $${NOT_A_VAR}") + self.assertEqual(result["literal"], "${NOT_A_VAR}") + + def test_value_newline_cannot_inject_mapping_keys(self): + with patch.dict(os.environ, {"VAL": "legit\nmalicious_key: injected"}): + result = self._substitute_yaml("service_name: ${VAL}") + self.assertEqual(list(result), ["service_name"]) + self.assertEqual( + result["service_name"], "legit\nmalicious_key: injected" + ) + + +class TestJsonEnvVarSubstitution(unittest.TestCase): + """JSON substitution touches only string values, not keys or non-strings.""" + + def test_string_values_substituted_keys_untouched(self): + with patch.dict(os.environ, {"V": "resolved"}): + result = _substitute_env_in_json_value( + {"${KEY}": "${V}", "nested": ["${V}", 1, True, None]} + ) + self.assertEqual( + result, + {"${KEY}": "resolved", "nested": ["resolved", 1, True, None]}, + ) + + +class TestEnvVarSubstitutionEndToEnd(unittest.TestCase): + """End-to-end loader behavior for issue #5406.""" + + @staticmethod + def _load_yaml(text: str) -> OpenTelemetryConfiguration: + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yaml", delete=False + ) as fh: + fh.write(text) + path = fh.name + try: + return load_config_file(path) + finally: + os.unlink(path) + + def test_undefined_variable_in_comment_does_not_crash(self): + # The reported bug: a ${VAR} inside a comment must be ignored, so an + # undefined variable there no longer aborts loading. + text = ( + "file_format: '1.0'\n" + "# documented default uses ${UNDEFINED_VAR} - not substituted\n" + "disabled: false\n" + ) + with patch.dict(os.environ, {}, clear=True): + config = self._load_yaml(text) + self.assertEqual(config.file_format, "1.0") + self.assertIs(config.disabled, False) + + def test_standalone_reference_coerces_type_for_schema(self): + # An integer field populated from ${VAR} must be an int so it passes + # JSON-schema validation. + text = ( + "file_format: '1.0'\n" + "attribute_limits:\n" + " attribute_count_limit: ${LIMIT}\n" + ) + with patch.dict(os.environ, {"LIMIT": "100"}): + config = self._load_yaml(text) + self.assertEqual(config.attribute_limits.attribute_count_limit, 100) + + def test_quoted_reference_for_int_field_fails_schema(self): + # Quoting forces a string, which is invalid for an integer field. + text = ( + "file_format: '1.0'\n" + "attribute_limits:\n" + ' attribute_count_limit: "${LIMIT}"\n' + ) + with patch.dict(os.environ, {"LIMIT": "100"}): + with self.assertRaises(ConfigurationError) as ctx: + self._load_yaml(text) + self.assertIn("schema", str(ctx.exception).lower()) From 6b2c62587d076f122f4013886e4bffcf427f4784 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sun, 12 Jul 2026 15:11:38 -0500 Subject: [PATCH 02/10] Rename changelog fragment to PR number --- .changelog/{5406.fixed => 5407.fixed} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{5406.fixed => 5407.fixed} (100%) diff --git a/.changelog/5406.fixed b/.changelog/5407.fixed similarity index 100% rename from .changelog/5406.fixed rename to .changelog/5407.fixed From 9ea37d8e4cf6f1bcd03fbb7eca3b19dba9639bc9 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sun, 12 Jul 2026 15:20:47 -0500 Subject: [PATCH 03/10] Rename substitute_env_vars parameter to configuration_value and clarify docstring --- .../configuration/file/_env_substitution.py | 43 ++++++++++++------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py index 9a83ce6b3a7..29fffa7c5b9 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py @@ -7,12 +7,24 @@ import re -def substitute_env_vars(text: str) -> str: - """Substitute environment variables within a configuration scalar value. +def substitute_env_vars(configuration_value: str) -> str: + """Substitute environment variables within a configuration value. - Substitution is applied per scalar value after the configuration file has - been parsed, so comments and mapping keys are never touched. Supports the - following syntax: + 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. @@ -26,19 +38,20 @@ def substitute_env_vars(text: str) -> str: files be shared across languages. Args: - text: A scalar value with potential ${VAR} placeholders. + configuration_value: A configuration value with potential ${VAR} + placeholders. Returns: - The value 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' + >>> os.environ['SERVICE_NAME'] = 'my-service' + >>> 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 @@ -67,4 +80,4 @@ def replace_var(match) -> str: # the loader, which re-resolves the node tag after substitution. return value - return re.sub(pattern, replace_var, text) + return re.sub(pattern, replace_var, configuration_value) From 953ea3cb6091344c628ed16343573dc0007df664 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sun, 12 Jul 2026 15:24:10 -0500 Subject: [PATCH 04/10] Use 'configuration value' instead of 'scalar value' in docs and comments --- .../configuration/file/_env_substitution.py | 8 +++--- .../configuration/file/_loader.py | 28 ++++++++++--------- .../tests/file/test_env_substitution.py | 4 +-- .../tests/file/test_loader.py | 2 +- 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py index 29fffa7c5b9..0eafd614fee 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py @@ -73,10 +73,10 @@ def replace_var(match) -> str: # provided, otherwise with an empty value, per the spec. return default_value or "" - # Substitution runs on an already-parsed scalar 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 - # scalar). Type interpretation for standalone references is handled by + # 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 diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py index d81a6f044a3..c62c1a46f5d 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py @@ -63,21 +63,22 @@ def _get_schema() -> dict: _YAML_STR_TAG = "tag:yaml.org,2002:str" -# A scalar whose entire value 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. +# 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): - """Apply env-var substitution to string scalar values in a YAML node tree. + """Apply env-var substitution to string configuration values in a node tree. - Substitution runs after parsing on scalar *values* only, so comments and - mapping keys are never candidates (per the configuration spec). For an + 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. @@ -123,8 +124,9 @@ def _parse_config_content( """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 scalar - values, with YAML node types re-resolved for standalone references. + 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 @@ -156,8 +158,8 @@ def load_config_file( """Load and parse an OpenTelemetry configuration file. Supports YAML and JSON formats. Environment variable substitution is - performed after parsing, on scalar values only, so ``${VAR}`` references in - comments or mapping keys are left untouched. + 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). @@ -189,7 +191,7 @@ 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 - # Parse the file, then substitute environment variables in scalar values. + # 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() diff --git a/opentelemetry-configuration/tests/file/test_env_substitution.py b/opentelemetry-configuration/tests/file/test_env_substitution.py index 6cce731b4aa..2714d085435 100644 --- a/opentelemetry-configuration/tests/file/test_env_substitution.py +++ b/opentelemetry-configuration/tests/file/test_env_substitution.py @@ -112,8 +112,8 @@ def test_only_dollar_signs(self): def test_newline_in_value_returned_verbatim(self): """A newline in a value is returned as-is. - Substitution runs per scalar value after parsing, so it does not - escape or quote newlines; YAML injection is prevented structurally + 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, {"MULTI": "line1\nline2"}): diff --git a/opentelemetry-configuration/tests/file/test_loader.py b/opentelemetry-configuration/tests/file/test_loader.py index a16cf619333..fffa3444608 100644 --- a/opentelemetry-configuration/tests/file/test_loader.py +++ b/opentelemetry-configuration/tests/file/test_loader.py @@ -390,7 +390,7 @@ def test_malformed_version_is_rejected(self): class TestEnvVarSubstitutionScope(unittest.TestCase): - """Substitution applies only to scalar values, per the config spec. + """Substitution applies only to configuration values, per the config spec. These exercise the YAML node walker directly so the spec's type and scope rules can be asserted without the JSON schema constraining shape. From b930baaaf969c66cf728f46a284b9afc5fe6d412 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sun, 12 Jul 2026 15:39:13 -0500 Subject: [PATCH 05/10] Explain the YAML node substitution and type re-resolution logic --- .../configuration/file/_loader.py | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py index c62c1a46f5d..8c42bc1129a 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py @@ -84,18 +84,37 @@ def _substitute_env_in_yaml_node(node: yaml.Node, loader: yaml.SafeLoader): ``${LIMIT}`` -> int); quoted or embedded references stay strings. """ 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) elif isinstance(node, yaml.MappingNode): - # Recurse into values only; keys are not substitution candidates. + # A mapping (dict): recurse into values only; keys are not + # substitution candidates per the spec. for _key_node, value_node in node.value: _substitute_env_in_yaml_node(value_node, loader) From 120747f1865958db96089b25d7fb2e47dbeb0c29 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Sun, 12 Jul 2026 15:44:15 -0500 Subject: [PATCH 06/10] Add worked example tracing an unquoted standalone reference through the node walker --- .../configuration/file/_loader.py | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py index 8c42bc1129a..5ca099929a6 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py @@ -83,6 +83,26 @@ def _substitute_env_in_yaml_node(node: yaml.Node, loader: yaml.SafeLoader): from the substituted value so YAML type coercion still applies (e.g. ``${LIMIT}`` -> int); quoted or embedded references stay strings. """ + # 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 From b1a0248a88068ae7a844772b977a66ee6ed899e1 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 15 Jul 2026 18:43:19 -0500 Subject: [PATCH 07/10] Keep public parameter name of substitute_env_vars substitute_env_vars is re-exported in opentelemetry.configuration.file.__all__, so renaming its parameter is a breaking public API change. Restore the parameter name to text to satisfy the public-symbols-check while keeping the docstring that explains the configuration-value semantics. --- .../opentelemetry/configuration/file/_env_substitution.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py index 0eafd614fee..42f7b7b5c52 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py @@ -7,7 +7,7 @@ import re -def substitute_env_vars(configuration_value: str) -> str: +def substitute_env_vars(text: str) -> str: """Substitute environment variables within a configuration value. A configuration value is a single value from the parsed configuration file @@ -38,8 +38,7 @@ def substitute_env_vars(configuration_value: str) -> str: files be shared across languages. Args: - configuration_value: A configuration value with potential ${VAR} - placeholders. + text: A configuration value with potential ${VAR} placeholders. Returns: The configuration value with environment variables substituted. @@ -80,4 +79,4 @@ def replace_var(match) -> str: # the loader, which re-resolves the node tag after substitution. return value - return re.sub(pattern, replace_var, configuration_value) + return re.sub(pattern, replace_var, text) From f9366b19aa8be5c64bc959d74196a826f5be597b Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Thu, 6 Aug 2026 19:31:30 -0500 Subject: [PATCH 08/10] Fix CI: ruff format docstring quotes and rename shadowing test parameter --- .../configuration/file/_env_substitution.py | 8 ++++---- opentelemetry-configuration/tests/file/test_loader.py | 10 +++++++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py index 42f7b7b5c52..82327ce2db7 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_env_substitution.py @@ -44,12 +44,12 @@ def substitute_env_vars(text: str) -> str: The configuration value with environment variables substituted. Examples: - >>> os.environ['SERVICE_NAME'] = 'my-service' - >>> substitute_env_vars('${SERVICE_NAME}') + >>> os.environ["SERVICE_NAME"] = "my-service" + >>> substitute_env_vars("${SERVICE_NAME}") 'my-service' - >>> substitute_env_vars('${MISSING:-default}') + >>> substitute_env_vars("${MISSING:-default}") 'default' - >>> substitute_env_vars('$$100') + >>> substitute_env_vars("$$100") '$100' """ # Pattern matches $$ (escape sequence) or ${VAR_NAME} / ${VAR_NAME:-default_value} diff --git a/opentelemetry-configuration/tests/file/test_loader.py b/opentelemetry-configuration/tests/file/test_loader.py index fffa3444608..a4d5b0e9b74 100644 --- a/opentelemetry-configuration/tests/file/test_loader.py +++ b/opentelemetry-configuration/tests/file/test_loader.py @@ -265,9 +265,13 @@ class TestConfigLoaderEndToEnd(unittest.TestCase): trace_id_ratio_based: {ratio: 0.5} """ - def _load(self, yaml: str | None = None) -> OpenTelemetryConfiguration: - with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as fh: - fh.write(self._YAML if yaml is None else yaml) + def _load( + self, yaml_content: str | None = None + ) -> OpenTelemetryConfiguration: + with tempfile.NamedTemporaryFile( + mode="w", suffix=".yaml", delete=False + ) as fh: + fh.write(self._YAML if yaml_content is None else yaml_content) path = fh.name try: return load_config_file(path) From 36d345246ce822300ba52071659e83553e2e1b1f Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Fri, 7 Aug 2026 16:12:00 -0500 Subject: [PATCH 09/10] Reformat with ruff at line-length 120 --- .../configuration/file/_loader.py | 21 +++------- .../tests/file/test_loader.py | 38 ++++--------------- 2 files changed, 13 insertions(+), 46 deletions(-) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py index 5ca099929a6..5c9143c0213 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py @@ -69,9 +69,7 @@ def _get_schema() -> dict: # 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" -) +_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): @@ -125,9 +123,7 @@ def _substitute_env_in_yaml_node(node: yaml.Node, loader: yaml.SafeLoader): # 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) - ) + 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: @@ -150,16 +146,11 @@ def _substitute_env_in_json_value(value: Any) -> Any: 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 {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: +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 @@ -236,9 +227,7 @@ def load_config_file( suffix = path.suffix.lower() 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" - ) + raise ConfigurationError(f"Unsupported file format: {suffix}. Use .yaml, .yml, or .json") data = _parse_config_content(content, suffix, file_path) diff --git a/opentelemetry-configuration/tests/file/test_loader.py b/opentelemetry-configuration/tests/file/test_loader.py index a4d5b0e9b74..117eead6d4e 100644 --- a/opentelemetry-configuration/tests/file/test_loader.py +++ b/opentelemetry-configuration/tests/file/test_loader.py @@ -265,12 +265,8 @@ class TestConfigLoaderEndToEnd(unittest.TestCase): trace_id_ratio_based: {ratio: 0.5} """ - def _load( - self, yaml_content: str | None = None - ) -> OpenTelemetryConfiguration: - with tempfile.NamedTemporaryFile( - mode="w", suffix=".yaml", delete=False - ) as fh: + def _load(self, yaml_content: str | None = None) -> OpenTelemetryConfiguration: + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as fh: fh.write(self._YAML if yaml_content is None else yaml_content) path = fh.name try: @@ -443,9 +439,7 @@ def test_value_newline_cannot_inject_mapping_keys(self): with patch.dict(os.environ, {"VAL": "legit\nmalicious_key: injected"}): result = self._substitute_yaml("service_name: ${VAL}") self.assertEqual(list(result), ["service_name"]) - self.assertEqual( - result["service_name"], "legit\nmalicious_key: injected" - ) + self.assertEqual(result["service_name"], "legit\nmalicious_key: injected") class TestJsonEnvVarSubstitution(unittest.TestCase): @@ -453,9 +447,7 @@ class TestJsonEnvVarSubstitution(unittest.TestCase): def test_string_values_substituted_keys_untouched(self): with patch.dict(os.environ, {"V": "resolved"}): - result = _substitute_env_in_json_value( - {"${KEY}": "${V}", "nested": ["${V}", 1, True, None]} - ) + result = _substitute_env_in_json_value({"${KEY}": "${V}", "nested": ["${V}", 1, True, None]}) self.assertEqual( result, {"${KEY}": "resolved", "nested": ["resolved", 1, True, None]}, @@ -467,9 +459,7 @@ class TestEnvVarSubstitutionEndToEnd(unittest.TestCase): @staticmethod def _load_yaml(text: str) -> OpenTelemetryConfiguration: - with tempfile.NamedTemporaryFile( - mode="w", suffix=".yaml", delete=False - ) as fh: + with tempfile.NamedTemporaryFile(mode="w", suffix=".yaml", delete=False) as fh: fh.write(text) path = fh.name try: @@ -480,11 +470,7 @@ def _load_yaml(text: str) -> OpenTelemetryConfiguration: def test_undefined_variable_in_comment_does_not_crash(self): # The reported bug: a ${VAR} inside a comment must be ignored, so an # undefined variable there no longer aborts loading. - text = ( - "file_format: '1.0'\n" - "# documented default uses ${UNDEFINED_VAR} - not substituted\n" - "disabled: false\n" - ) + text = "file_format: '1.0'\n# documented default uses ${UNDEFINED_VAR} - not substituted\ndisabled: false\n" with patch.dict(os.environ, {}, clear=True): config = self._load_yaml(text) self.assertEqual(config.file_format, "1.0") @@ -493,22 +479,14 @@ def test_undefined_variable_in_comment_does_not_crash(self): def test_standalone_reference_coerces_type_for_schema(self): # An integer field populated from ${VAR} must be an int so it passes # JSON-schema validation. - text = ( - "file_format: '1.0'\n" - "attribute_limits:\n" - " attribute_count_limit: ${LIMIT}\n" - ) + text = "file_format: '1.0'\nattribute_limits:\n attribute_count_limit: ${LIMIT}\n" with patch.dict(os.environ, {"LIMIT": "100"}): config = self._load_yaml(text) self.assertEqual(config.attribute_limits.attribute_count_limit, 100) def test_quoted_reference_for_int_field_fails_schema(self): # Quoting forces a string, which is invalid for an integer field. - text = ( - "file_format: '1.0'\n" - "attribute_limits:\n" - ' attribute_count_limit: "${LIMIT}"\n' - ) + text = "file_format: '1.0'\nattribute_limits:\n attribute_count_limit: \"${LIMIT}\"\n" with patch.dict(os.environ, {"LIMIT": "100"}): with self.assertRaises(ConfigurationError) as ctx: self._load_yaml(text) From 64dbc50a9b70c91b427a72d4216b6b0d8ef4e0d2 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Mon, 24 Aug 2026 08:49:59 -0500 Subject: [PATCH 10/10] Substitute each YAML node once when anchors and aliases share it An alias resolves to the same composed node as its anchor, so the walker reached one node once per reference to it. That substituted the node repeatedly, which re-read the previous pass's output: an anchored $${TOKEN} became the literal ${TOKEN} on the first visit, and the second visit resolved it as a real variable, defeating the escape and exposing the value. Merge keys took the same path, since a merge key's value node is the anchored mapping itself. A cyclic alias made the node tree a graph with a loop, so the walk recursed until it raised RecursionError. Thread a set of visited node ids through the traversal and return early on a node already seen. --- .../configuration/file/_loader.py | 40 +++++++- .../tests/file/test_loader.py | 94 +++++++++++++++---- 2 files changed, 114 insertions(+), 20 deletions(-) diff --git a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py index 5c9143c0213..f3a29aa7986 100644 --- a/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py +++ b/opentelemetry-configuration/src/opentelemetry/configuration/file/_loader.py @@ -72,7 +72,11 @@ def _get_schema() -> dict: _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): +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 @@ -80,7 +84,33 @@ def _substitute_env_in_yaml_node(node: yaml.Node, loader: yaml.SafeLoader): 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): @@ -127,12 +157,14 @@ def _substitute_env_in_yaml_node(node: yaml.Node, loader: yaml.SafeLoader): 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) + _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. + # 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) + _substitute_env_in_yaml_node(value_node, loader, visited) def _substitute_env_in_json_value(value: Any) -> Any: diff --git a/opentelemetry-configuration/tests/file/test_loader.py b/opentelemetry-configuration/tests/file/test_loader.py index 117eead6d4e..4dc4629de66 100644 --- a/opentelemetry-configuration/tests/file/test_loader.py +++ b/opentelemetry-configuration/tests/file/test_loader.py @@ -389,6 +389,21 @@ def test_malformed_version_is_rejected(self): self.assertIn("file_format", str(ctx.exception)) +def _substitute_yaml(text: str): + """Parse ``text``, run the node walker over it, and build the document. + + This is what ``_parse_config_content`` does for YAML, without the schema + validation, so walker behavior can be asserted on any document shape. + """ + loader = yaml.SafeLoader(text) + try: + node = loader.get_single_node() + _substitute_env_in_yaml_node(node, loader) + return loader.construct_document(node) + finally: + loader.dispose() + + class TestEnvVarSubstitutionScope(unittest.TestCase): """Substitution applies only to configuration values, per the config spec. @@ -396,52 +411,99 @@ class TestEnvVarSubstitutionScope(unittest.TestCase): scope rules can be asserted without the JSON schema constraining shape. """ - @staticmethod - def _substitute_yaml(text: str): - loader = yaml.SafeLoader(text) - try: - node = loader.get_single_node() - _substitute_env_in_yaml_node(node, loader) - return loader.construct_document(node) - finally: - loader.dispose() - def test_unquoted_standalone_reference_is_type_coerced(self): with patch.dict(os.environ, {"N": "42", "FLAG": "true"}): - result = self._substitute_yaml("count: ${N}\nflag: ${FLAG}") + result = _substitute_yaml("count: ${N}\nflag: ${FLAG}") self.assertEqual(result["count"], 42) self.assertIsInstance(result["count"], int) self.assertIs(result["flag"], True) def test_quoted_reference_stays_string(self): with patch.dict(os.environ, {"N": "42"}): - result = self._substitute_yaml('count: "${N}"') + result = _substitute_yaml('count: "${N}"') self.assertEqual(result["count"], "42") def test_embedded_reference_resolves_to_string(self): with patch.dict(os.environ, {"N": "42"}): - result = self._substitute_yaml("name: svc-${N}") + result = _substitute_yaml("name: svc-${N}") self.assertEqual(result["name"], "svc-42") def test_mapping_key_is_not_substituted(self): # A ${VAR} in a key position is left verbatim and triggers no lookup, # so an undefined variable there does not raise. with patch.dict(os.environ, {}, clear=True): - result = self._substitute_yaml("${UNDEFINED_KEY}: value") + result = _substitute_yaml("${UNDEFINED_KEY}: value") self.assertEqual(result, {"${UNDEFINED_KEY}": "value"}) def test_escape_sequence_is_not_a_reference(self): with patch.dict(os.environ, {}, clear=True): - result = self._substitute_yaml("literal: $${NOT_A_VAR}") + result = _substitute_yaml("literal: $${NOT_A_VAR}") self.assertEqual(result["literal"], "${NOT_A_VAR}") def test_value_newline_cannot_inject_mapping_keys(self): with patch.dict(os.environ, {"VAL": "legit\nmalicious_key: injected"}): - result = self._substitute_yaml("service_name: ${VAL}") + result = _substitute_yaml("service_name: ${VAL}") self.assertEqual(list(result), ["service_name"]) self.assertEqual(result["service_name"], "legit\nmalicious_key: injected") +class TestEnvVarSubstitutionWithAliases(unittest.TestCase): + """An anchored value is substituted once, however many aliases reach it. + + An alias resolves to the very same composed node as its anchor, so the + walker reaches one node once per reference. Substituting it more than once + would re-read the first pass's output and defeat the ``$$`` escape, and a + cyclic alias would recurse without end. + """ + + def test_aliased_reference_resolves_for_every_alias(self): + with patch.dict(os.environ, {"TOKEN": "resolved"}): + result = _substitute_yaml("a: &anchor ${TOKEN}\nb: *anchor\nc: *anchor") + self.assertEqual(result, {"a": "resolved", "b": "resolved", "c": "resolved"}) + + def test_aliased_escape_stays_literal(self): + # Without a visited set the first visit turns $${TOKEN} into the + # literal ${TOKEN} and the second resolves it, leaking TOKEN's value. + with patch.dict(os.environ, {"TOKEN": "leaked"}): + result = _substitute_yaml("a: &anchor $${TOKEN}\nb: *anchor\nc: *anchor") + self.assertEqual(result, {"a": "${TOKEN}", "b": "${TOKEN}", "c": "${TOKEN}"}) + + def test_aliased_reference_is_type_coerced_once(self): + with patch.dict(os.environ, {"N": "42"}): + result = _substitute_yaml("a: &anchor ${N}\nb: *anchor") + self.assertEqual(result["a"], 42) + self.assertIsInstance(result["a"], int) + self.assertEqual(result["b"], 42) + + def test_merge_key_escape_stays_literal(self): + # A merge key's value node is the anchored mapping itself, so the + # merged-in values are reached twice as well. + text = "base: &base\n x: $${TOKEN}\nderived:\n <<: *base\n y: 1\n" + with patch.dict(os.environ, {"TOKEN": "leaked"}): + result = _substitute_yaml(text) + self.assertEqual(result, {"base": {"x": "${TOKEN}"}, "derived": {"x": "${TOKEN}", "y": 1}}) + + def test_merge_key_resolves_reference(self): + text = "base: &base\n x: ${TOKEN}\nderived:\n <<: *base\n y: 1\n" + with patch.dict(os.environ, {"TOKEN": "resolved"}): + result = _substitute_yaml(text) + self.assertEqual(result, {"base": {"x": "resolved"}, "derived": {"x": "resolved", "y": 1}}) + + def test_cyclic_alias_terminates(self): + # A mapping that aliases itself makes the node tree a graph with a + # loop; the walk must end instead of raising RecursionError. + with patch.dict(os.environ, {"TOKEN": "resolved"}): + result = _substitute_yaml("a: &anchor\n self: *anchor\n value: ${TOKEN}") + self.assertIs(result["a"]["self"], result["a"]) + self.assertEqual(result["a"]["value"], "resolved") + + def test_cyclic_alias_in_sequence_terminates(self): + with patch.dict(os.environ, {"TOKEN": "resolved"}): + result = _substitute_yaml("a: &anchor\n - *anchor\n - ${TOKEN}") + self.assertIs(result["a"][0], result["a"]) + self.assertEqual(result["a"][1], "resolved") + + class TestJsonEnvVarSubstitution(unittest.TestCase): """JSON substitution touches only string values, not keys or non-strings."""