Merge pull request #61 from Integration-Automation/fix/action-executor-keys

Fix action result key collisions and secret leak in substitute mode

Author: JE-Chen

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: <action>": result|repr(error)}``.
+        """Execute every action; return ``{"execute[<index>]: <action>": 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}"
 

docs/source.zh-CN/usage.rst

@@ -13,7 +13,7 @@ JSON 动作列表
    ["FA_name", ["positional", "args"]]
 
 动作列表是动作的数组。执行器按顺序执行并返回
-``"execute: <action>" -> result | repr(error)`` 的映射表。
+``"execute[<index>]: <action>" -> result | repr(error)`` 的映射表。
 
 .. code-block:: python
 

docs/source.zh-TW/usage.rst

@@ -13,7 +13,7 @@ JSON 動作清單
    ["FA_name", ["positional", "args"]]
 
 動作清單是動作的陣列。執行器依序執行並回傳
-``"execute: <action>" -> result | repr(error)`` 的對應表。
+``"execute[<index>]: <action>" -> result | repr(error)`` 的對應表。
 
 .. code-block:: python
 

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: <action>" -> result | repr(error)``.
+returns a mapping of ``"execute[<index>]: <action>" -> result | repr(error)``.
 
 .. code-block:: python
 

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)

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()