From d1b4f20d67e447a665b7bba95f12c83bc797ffbb Mon Sep 17 00:00:00 2001 From: JeffreyChen Date: Sun, 26 Apr 2026 00:11:40 +0800 Subject: [PATCH] Fix action result key collisions and secret leak in substitute mode execute_action's result dict keyed entries on f"execute: {action}", so two identical actions in one batch would collide and the first result would be overwritten. With substitute=True, the substituted action (with ${env:...} already expanded) was also written into the log line and the result key, exposing secrets pulled in via ${env:...}. - Key results as f"execute[{index}]: {display}" so duplicate actions keep both results, matching execute_action_parallel's existing format. - Track the original (un-substituted) action separately and use it for the result key, the success log line, and the dry_run payload log; only the executed callable receives the substituted payload. - Update docs (en / zh-TW / zh-CN) and the http server test for the new key format. - Add regression tests for both fixes in test_action_executor. --- automation_file/core/action_executor.py | 36 ++++++++++++++----------- docs/source.zh-CN/usage.rst | 2 +- docs/source.zh-TW/usage.rst | 2 +- docs/source/usage.rst | 2 +- tests/test_action_executor.py | 32 ++++++++++++++++++++++ tests/test_http_server.py | 2 +- 6 files changed, 57 insertions(+), 19 deletions(-) diff --git a/automation_file/core/action_executor.py b/automation_file/core/action_executor.py index b6e753d..446882f 100644 --- a/automation_file/core/action_executor.py +++ b/automation_file/core/action_executor.py @@ -103,23 +103,26 @@ def execute_action( validate_first: bool = False, substitute: bool = False, ) -> dict[str, Any]: - """Execute every action; return ``{"execute: ": result|repr(error)}``. + """Execute every action; return ``{"execute[]: ": result|repr(error)}``. ``dry_run=True`` logs and records the resolved name without invoking the command. ``validate_first=True`` runs :meth:`validate` before touching any action so a typo aborts the whole batch up-front. ``substitute=True`` expands ``${env:...}`` / ``${date:...}`` / ``${uuid}`` / ``${cwd}`` - placeholders inside every string in the payload. + placeholders inside every string in the payload — the original + (un-substituted) action is used for log lines and result keys so + secrets pulled in via ``${env:...}`` never reach logs. """ actions = self._coerce(action_list) - if substitute: - actions = substitute_payload(actions) # type: ignore[assignment] + executed: list = ( + substitute_payload(actions) if substitute else actions # type: ignore[assignment] + ) if validate_first: - self.validate(actions) + self.validate(executed) results: dict[str, Any] = {} - for action in actions: - key = f"execute: {action}" - results[key] = self._run_one(action, dry_run=dry_run) + for index, (display, action) in enumerate(zip(actions, executed, strict=True)): + key = f"execute[{index}]: {display}" + results[key] = self._run_one(action, dry_run=dry_run, display=display) return results def execute_action_parallel( @@ -154,15 +157,16 @@ def add_command_to_executor(self, command_dict: Mapping[str, Any]) -> None: self.registry.register_many(command_dict) # Internals --------------------------------------------------------- - def _run_one(self, action: list, dry_run: bool) -> Any: + def _run_one(self, action: list, dry_run: bool, display: list | None = None) -> Any: + display_action = action if display is None else display name = _safe_action_name(action) if dry_run: - return self._run_dry(action) + return self._run_dry(action, display=display_action) started = time.monotonic() ok = False try: value = self._execute_event(action) - file_automation_logger.info("execute_action: %s", action) + file_automation_logger.info("execute_action: %s", display_action) ok = True return value except ExecuteActionException as error: @@ -174,19 +178,21 @@ def _run_one(self, action: list, dry_run: bool) -> Any: finally: record_action(name, time.monotonic() - started, ok) - def _run_dry(self, action: list) -> Any: + def _run_dry(self, action: list, display: list | None = None) -> Any: + display_action = action if display is None else display try: - name, kind, payload = self._parse_action(action) + name, _, _ = self._parse_action(action) if self.registry.resolve(name) is None: raise ExecuteActionException(f"unknown action: {name!r}") except ExecuteActionException as error: file_automation_logger.error("execute_action malformed: %r", error) return repr(error) + _, display_kind, display_payload = self._parse_action(display_action) file_automation_logger.info( "dry_run: %s kind=%s payload=%r", name, - kind, - payload, + display_kind, + display_payload, ) return f"dry_run:{name}" diff --git a/docs/source.zh-CN/usage.rst b/docs/source.zh-CN/usage.rst index 4cac7ae..7f80669 100644 --- a/docs/source.zh-CN/usage.rst +++ b/docs/source.zh-CN/usage.rst @@ -13,7 +13,7 @@ JSON 动作列表 ["FA_name", ["positional", "args"]] 动作列表是动作的数组。执行器按顺序执行并返回 -``"execute: " -> result | repr(error)`` 的映射表。 +``"execute[]: " -> result | repr(error)`` 的映射表。 .. code-block:: python diff --git a/docs/source.zh-TW/usage.rst b/docs/source.zh-TW/usage.rst index 6b5e80d..9f7215f 100644 --- a/docs/source.zh-TW/usage.rst +++ b/docs/source.zh-TW/usage.rst @@ -13,7 +13,7 @@ JSON 動作清單 ["FA_name", ["positional", "args"]] 動作清單是動作的陣列。執行器依序執行並回傳 -``"execute: " -> result | repr(error)`` 的對應表。 +``"execute[]: " -> result | repr(error)`` 的對應表。 .. code-block:: python diff --git a/docs/source/usage.rst b/docs/source/usage.rst index abc8148..016527c 100644 --- a/docs/source/usage.rst +++ b/docs/source/usage.rst @@ -13,7 +13,7 @@ An action is one of three shapes: ["FA_name", ["positional", "args"]] An action list is an array of actions. The executor runs them in order and -returns a mapping of ``"execute: " -> result | repr(error)``. +returns a mapping of ``"execute[]: " -> result | repr(error)``. .. code-block:: python diff --git a/tests/test_action_executor.py b/tests/test_action_executor.py index e717017..9695f5d 100644 --- a/tests/test_action_executor.py +++ b/tests/test_action_executor.py @@ -97,3 +97,35 @@ def test_json_store_roundtrip(tmp_path: Path) -> None: path = tmp_path / "payload.json" write_action_json(str(path), [["a", 1]]) assert read_action_json(str(path)) == [["a", 1]] + + +def test_duplicate_actions_do_not_collide() -> None: + """Two identical actions in one batch must keep both results.""" + executor = _fresh_executor() + results = executor.execute_action( + [ + ["echo", {"value": "first"}], + ["echo", {"value": "first"}], + ] + ) + assert len(results) == 2 + assert list(results.values()) == ["first", "first"] + + +def test_substitute_does_not_leak_into_result_key() -> None: + """``substitute=True`` must keep the un-expanded literal in result keys.""" + import os + + os.environ["FA_EXEC_SECRET"] = "TOP_SECRET" + try: + executor = _fresh_executor() + results = executor.execute_action( + [["echo", {"value": "${env:FA_EXEC_SECRET}"}]], + substitute=True, + ) + [(key, value)] = results.items() + assert "TOP_SECRET" not in key + assert "${env:FA_EXEC_SECRET}" in key + assert value == "TOP_SECRET" + finally: + os.environ.pop("FA_EXEC_SECRET", None) diff --git a/tests/test_http_server.py b/tests/test_http_server.py index ab759e2..eea7245 100644 --- a/tests/test_http_server.py +++ b/tests/test_http_server.py @@ -39,7 +39,7 @@ def test_http_server_executes_action() -> None: url = insecure_url("http", f"{host}:{port}/actions") status, body = _post(url, [["test_http_echo", {"value": "hi"}]]) assert status == 200 - assert json.loads(body) == {"execute: ['test_http_echo', {'value': 'hi'}]": "hi"} + assert json.loads(body) == {"execute[0]: ['test_http_echo', {'value': 'hi'}]": "hi"} finally: server.shutdown()