From ba102c0db7c3e31c52daa8d6cc2f9a3fc78976a3 Mon Sep 17 00:00:00 2001 From: Piyush Makhija Date: Sun, 9 Aug 2026 21:02:51 +0530 Subject: [PATCH] fix(config): do not inject unfilled placeholder values as credentials The shipped workflow-config.json carries "github": {"token": "YOUR_GITHUB_TOKEN"}. load_workflow_config injected that literal string into GITHUB_TOKEN, and one placeholder then defeated BOTH GitHub backends at once. The MCP client authenticated with it and got 401 Bad credentials. The gh CLI fallback also lost, because gh prefers an environment token over its own keyring and reported itself unauthenticated. So `gh auth login` appeared to do nothing: the machine was correctly authenticated, `gh auth status` said so, a standalone GitHubOperationRouter said so -- and the pipeline still fell back to "no GitHub backend available" on every run and could create neither an issue nor a branch. It was worth a day. The engine's whole SDLC arc was blocked by a string that was never filled in. The scope is wider than GitHub, which only became visible once the skip was announced: this config was injecting TEN placeholder values -- anthropic.api_key, figma.access_token, figma.team_id, github.token, jenkins.api_token, jenkins.url, jenkins.user, jira.api_token, jira.url, jira.user. Anyone enabling any of those integrations would have met the same failure, and the ones that take a URL rather than a secret would have failed even less legibly. The predicate is deliberately narrow, and half the tests exist to hold it there. A false positive rejects a working secret and breaks a setup that was fine, which is worse than the bug it guards against, so nothing is matched on length, entropy or character class -- only on shapes no real credential has: a YOUR_/your- prefix, angle brackets, CHANGEME, REPLACE_ME, TODO, xxx, or empty. ghp_/gho_/github_pat_ values pass, and so does "yourcompany-prod-key", which starts with "your" but is not a template. The skip is announced on stderr. A value silently ignored is indistinguishable from one that was never read, and the reader is someone wondering why their config has no effect -- which is exactly the position this bug put its author in. Proven against the real broken config rather than a fixture: the placeholder was restored to ~/.claude/workflow-config.json, and with it present GITHUB_TOKEN is no longer injected, MCP falls through to the gh keyring, PyGithub authenticates, and both backends initialise. The file is left in that shipped state, since it is now handled correctly and the notice tells the user which integrations are unconfigured. One test-harness note worth recording: _load_raw memoises into _CONFIG_CACHE on first read, after which the `path` argument is silently ignored. The first draft of these tests did not reset it, so three of them asserted against the developer's own ~/.claude/workflow-config.json instead of their fixtures -- a green suite proving nothing. The autouse fixture resets it. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 4 +- README.md | 10 +- langgraph_engine/core/config_loader.py | 65 +++++++++- tests/test_config_loader_placeholders.py | 156 +++++++++++++++++++++++ 4 files changed, 225 insertions(+), 10 deletions(-) create mode 100644 tests/test_config_loader_placeholders.py diff --git a/CLAUDE.md b/CLAUDE.md index 965fa7c1..6fa6ce26 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,8 +20,8 @@ Claude Workflow Engine is a 3-level LangGraph-based orchestration pipeline for a | **Status** | Active Development | | **Primary Location** | langgraph_engine/ | | **MCP Servers** | 13 servers -- all in separate repos under [techdeveloper-org](https://github.com/orgs/techdeveloper-org/repositories); 1 also keeps an in-engine copy in `src/mcp/` | -| **Total Python Files** | 248 (langgraph_engine/); 492 repo-wide | -| **Test Files** | 104 (95 unit, 5 integration, 3 e2e, 1 load) | +| **Total Python Files** | 248 (langgraph_engine/); 493 repo-wide | +| **Test Files** | 105 (96 unit, 5 integration, 3 e2e, 1 load) | | **Call Graph** | 578 classes, 3,985 methods, 4 languages (Python/Java/TS/Kotlin) | --- diff --git a/README.md b/README.md index 99bcdf92..9a0585b3 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ [![Python](https://img.shields.io/badge/Python-3.10%2B-green)](https://python.org) [![CI](https://github.com/techdeveloper-org/claude-workflow-engine/actions/workflows/ci.yml/badge.svg)](https://github.com/techdeveloper-org/claude-workflow-engine/actions/workflows/ci.yml) [![License](https://img.shields.io/badge/License-MIT-yellow)](LICENSE) -[![Tests](https://img.shields.io/badge/Tests-104%20files%20·%205%20integration-brightgreen)](tests/) +[![Tests](https://img.shields.io/badge/Tests-105%20files%20·%205%20integration-brightgreen)](tests/) [![Discussions](https://img.shields.io/badge/Discussions-GitHub-blueviolet)](../../discussions) --- @@ -469,8 +469,8 @@ claude-workflow-engine/ # 369 Python files total ├── policies/ # non-.md policy state only (failure-kb.json) — see note below │ └── 03-execution-system/failure-prevention/failure-kb.json │ -├── tests/ # 104 test_*.py files (125 total Python files) -│ ├── test_*.py # 95 unit tests +├── tests/ # 105 test_*.py files (126 total Python files) +│ ├── test_*.py # 96 unit tests │ ├── integration/ # 5 integration tests (GitHub, MCP, runtime verification) │ ├── e2e/ # 3 end-to-end scenario tests │ └── load/ # 1 concurrency / load test @@ -628,11 +628,11 @@ ENABLE_HEALTH_SERVER=1 ENABLE_METRICS=1 LOG_FORMAT=json \ ## Testing -104 test_*.py files (125 total Python files in `tests/` including conftest and `__init__` files): +105 test_*.py files (126 total Python files in `tests/` including conftest and `__init__` files): | Category | Files | Notes | |---|:---:|---| -| Unit tests | 95 | In `tests/test_*.py` — no external dependencies required | +| Unit tests | 96 | In `tests/test_*.py` — no external dependencies required | | Integration tests | 5 | `tests/integration/` — require live GitHub token or MCP servers | | E2E tests | 3 | `tests/e2e/` — require full pipeline environment | | Load tests | 1 | `tests/load/` — enabled with `RUN_LOAD_TESTS=1` | diff --git a/langgraph_engine/core/config_loader.py b/langgraph_engine/core/config_loader.py index 7fd63c2d..7e60aa73 100644 --- a/langgraph_engine/core/config_loader.py +++ b/langgraph_engine/core/config_loader.py @@ -100,12 +100,54 @@ def _flatten(data: dict, prefix: str = "") -> dict[str, str]: return out +# Values that mean "you were supposed to fill this in". The shipped +# workflow-config.json carries them, so an untouched config injects them exactly +# as if they were real credentials. +# +# This is deliberately narrow. A false positive here rejects a working secret and +# breaks a setup that was fine, which is worse than the bug it guards against, so +# nothing is matched on length, entropy or character class -- only on shapes no +# real credential has. A real GitHub token starts ghp_/gho_/github_pat_ and a real +# API key is not spelled CHANGEME. +_PLACEHOLDER_PREFIXES = ("your_", "your-", "my_", "replace_", "replaceme", "changeme", "change_me", "todo", "xxx") + + +def _is_placeholder(value: str) -> bool: + """Return True when a config value is an unfilled template, not a secret. + + Args: + value: The raw value read from workflow-config.json. + + Returns: + bool: True if the value should be treated as absent. + """ + if not isinstance(value, str): + return False + stripped = value.strip() + if not stripped: + return True + if stripped.startswith("<") and stripped.endswith(">"): + return True + return stripped.lower().startswith(_PLACEHOLDER_PREFIXES) + + def load_workflow_config(path: Path | None = None) -> dict[str, str]: """Load workflow-config.json and inject into os.environ (no-override). Covers any remaining os.environ.get() call sites that have not yet been migrated to use get_section() directly. + Unfilled template values are skipped rather than injected. The reason is + specific and was expensive: the shipped config carries + ``"github": {"token": "YOUR_GITHUB_TOKEN"}``, this function injected it into + GITHUB_TOKEN, and that one string then defeated BOTH GitHub backends at once + -- the MCP client authenticated with it and got 401 Bad credentials, and the + gh CLI fallback also lost, because gh prefers an environment token over its + own keyring and reported itself unauthenticated. `gh auth login` therefore + appeared to do nothing, and Step 2 fell back to "no GitHub backend available" + on every run. Injecting nothing leaves the keyring reachable, which is the + working path. + Returns a dict of env-var-name -> value pairs that were injected. """ raw = _load_raw(path) @@ -114,10 +156,27 @@ def load_workflow_config(path: Path | None = None) -> dict[str, str]: flat = _flatten(raw) injected: dict[str, str] = {} + skipped: list[str] = [] for json_key, env_key in _MAPPING.items(): - if json_key in flat and env_key not in os.environ: - os.environ[env_key] = flat[json_key] - injected[env_key] = flat[json_key] + if json_key not in flat or env_key in os.environ: + continue + value = flat[json_key] + if _is_placeholder(value): + skipped.append(json_key) + continue + os.environ[env_key] = value + injected[env_key] = value + + # Said out loud, because a value silently ignored is indistinguishable from a + # value that was never read -- and the reader is someone wondering why their + # config has no effect. + if skipped: + import sys # noqa: PLC0415 + + print( + "[config_loader] ignored %d unfilled placeholder value(s): %s" % (len(skipped), ", ".join(sorted(skipped))), + file=sys.stderr, + ) return injected diff --git a/tests/test_config_loader_placeholders.py b/tests/test_config_loader_placeholders.py new file mode 100644 index 00000000..d7e5eba9 --- /dev/null +++ b/tests/test_config_loader_placeholders.py @@ -0,0 +1,156 @@ +"""An unfilled config template must not be injected as if it were a credential. + +The shipped workflow-config.json carries `"github": {"token": "YOUR_GITHUB_TOKEN"}`. +load_workflow_config injected that string into GITHUB_TOKEN, and one placeholder +then defeated BOTH GitHub backends at once: the MCP client authenticated with it +and got 401 Bad credentials, and the gh CLI fallback also lost, because gh prefers +an environment token over its own keyring and reported itself unauthenticated. + +`gh auth login` therefore appeared to do nothing. Step 2 fell back to "no GitHub +backend available" on every run, and the pipeline could not create an issue or a +branch on a machine that was, by every other measure, correctly authenticated. +Injecting nothing leaves the keyring reachable, which is the working path. + +The predicate is deliberately narrow, and half of these tests exist to hold it +that way. A false positive rejects a real secret and breaks a setup that was +working -- worse than the bug it guards against -- so nothing is matched on +length, entropy or character class. +""" + +import json +import sys +from pathlib import Path + +import pytest + +_PROJECT_ROOT = Path(__file__).resolve().parent.parent +if str(_PROJECT_ROOT) not in sys.path: + sys.path.insert(0, str(_PROJECT_ROOT)) + +from langgraph_engine.core import config_loader # noqa: E402 +from langgraph_engine.core.config_loader import _is_placeholder, load_workflow_config # noqa: E402 + + +@pytest.fixture(autouse=True) +def _clean_env(monkeypatch): + """Reset the env vars and the module's config cache before each test. + + _load_raw memoises into _CONFIG_CACHE on first read, after which the `path` + argument is silently ignored. Without this reset the first test to run would + pin the real ~/.claude/workflow-config.json for every test after it, and the + suite would be asserting against the developer's own machine -- which is how + a green suite ends up proving nothing. + """ + for var in ("GITHUB_TOKEN", "GITHUB_DEFAULT_LABEL", "CLAUDE_GITHUB_OWNER", "JIRA_API_TOKEN"): + monkeypatch.delenv(var, raising=False) + monkeypatch.setattr(config_loader, "_CONFIG_CACHE", None) + + +def _write_config(tmp_path, payload): + """Write a workflow-config.json and return its path.""" + path = tmp_path / "workflow-config.json" + path.write_text(json.dumps(payload), encoding="utf-8") + return path + + +class TestThePlaceholderIsNotInjected: + """The exact failure that cost a day.""" + + def test_the_shipped_github_placeholder_is_skipped(self, tmp_path): + path = _write_config(tmp_path, {"github": {"token": "YOUR_GITHUB_TOKEN"}}) + injected = load_workflow_config(path) + assert "GITHUB_TOKEN" not in injected + assert "GITHUB_TOKEN" not in __import__("os").environ + + def test_a_real_looking_token_is_still_injected(self, tmp_path): + """The half that fails if the predicate became greedy. + + Without this, a predicate that rejected everything would satisfy the test + above -- and would break every correctly configured machine instead. + """ + path = _write_config(tmp_path, {"github": {"token": "ghp_16CharsOrMoreOfRealLookingSecret"}}) + injected = load_workflow_config(path) + assert injected.get("GITHUB_TOKEN") == "ghp_16CharsOrMoreOfRealLookingSecret" + + def test_other_real_values_survive_alongside_a_skipped_one(self, tmp_path): + """One placeholder must not suppress the rest of the file.""" + path = _write_config( + tmp_path, + {"github": {"token": "YOUR_GITHUB_TOKEN", "owner": "techdeveloper-org", "default_label": "feature"}}, + ) + injected = load_workflow_config(path) + assert "GITHUB_TOKEN" not in injected + assert injected.get("CLAUDE_GITHUB_OWNER") == "techdeveloper-org" + assert injected.get("GITHUB_DEFAULT_LABEL") == "feature" + + +class TestTheSkipIsAnnounced: + """A value silently ignored is indistinguishable from one never read.""" + + def test_the_skip_is_reported(self, tmp_path, capsys): + path = _write_config(tmp_path, {"github": {"token": "YOUR_GITHUB_TOKEN"}}) + load_workflow_config(path) + assert "github.token" in capsys.readouterr().err + + def test_nothing_is_reported_when_nothing_was_skipped(self, tmp_path, capsys): + """The half that fails if the notice were unconditional.""" + path = _write_config(tmp_path, {"github": {"owner": "techdeveloper-org"}}) + load_workflow_config(path) + assert "config_loader" not in capsys.readouterr().err + + +class TestThePredicateIsNarrow: + """A false positive breaks a working setup, so the shapes are enumerated.""" + + @pytest.mark.parametrize( + "value", + [ + "YOUR_GITHUB_TOKEN", + "your-api-key", + "", + "CHANGEME", + "change_me", + "REPLACE_ME", + "TODO", + "xxxxxxxx", + "", + " ", + ], + ) + def test_template_shapes_are_placeholders(self, value): + assert _is_placeholder(value) is True + + @pytest.mark.parametrize( + "value", + [ + "ghp_realLookingToken1234567890", + "gho_anotherRealShape", + "github_pat_11ABCDEF", + "techdeveloper-org", + "feature", + "sk-ant-api03-abcdef", + "yourcompany-prod-key", # starts with "your" but not "your_" or "your-" + "0123456789abcdef", + ], + ) + def test_real_values_are_not_placeholders(self, value): + assert _is_placeholder(value) is False + + def test_a_non_string_is_not_a_placeholder(self): + """Config values may be bools or numbers; those are not templates.""" + assert _is_placeholder(True) is False + assert _is_placeholder(0) is False + + +class TestExistingBehaviourIsUnchanged: + """The no-override rule and missing-file handling must not have moved.""" + + def test_an_existing_env_var_still_wins(self, tmp_path, monkeypatch): + monkeypatch.setenv("CLAUDE_GITHUB_OWNER", "already-set") + path = _write_config(tmp_path, {"github": {"owner": "from-config"}}) + injected = load_workflow_config(path) + assert "CLAUDE_GITHUB_OWNER" not in injected + assert __import__("os").environ["CLAUDE_GITHUB_OWNER"] == "already-set" + + def test_a_missing_file_returns_empty(self, tmp_path): + assert load_workflow_config(tmp_path / "absent.json") == {}