Merge pull request #5243 from springfall2008/fix/apps-page-reveal-secret-on-edit

fix(web): let /apps Edit reveal a masked credential instead of offering "xxx"

Author: springfall2008

apps/predbat/tests/test_web_apps_edit.py

@@ -67,6 +67,14 @@ async def post(self):
         return self.postdata
 
 
+class FakeGetRequest:
+    """A minimal aiohttp-request stand-in exposing only the query string the handler reads."""
+
+    def __init__(self, query):
+        """Store the query parameters this request will hand back."""
+        self.query = query
+
+
 def _load_yaml(path="apps.yaml"):
     """Load a YAML file from the temporary working directory."""
     yaml = YAML()
@@ -502,6 +510,111 @@ def run_web_apps_edit_tests(my_predbat):
             print("  ERROR: the rotated credential was not written")
             failed += 1
 
+        # ---------------------------------------------------------------------
+        # The page serving the mask meant clicking Edit on a credential filled the input with
+        # "xxx": the real key could not be seen or amended, only retyped from scratch. The page
+        # stays masked; Edit fetches the one real value through /apps_value instead.
+        print("Test: credential rows are flagged for reveal-on-edit and still served masked")
+        text = _render_apps_page(my_predbat, {"octopus_api_key": "sk-top-level-credential-0000", "chat": {"providers": {"openrouter": {"api_key": "sk-live-nested-credential-1234", "model": "some-model"}}}})
+        if "sk-top-level-credential-0000" in text or "sk-live-nested-credential-1234" in text:
+            print("  ERROR: a credential reached the rendered page in the clear")
+            failed += 1
+        if not re.search(r'<tr id="row_\d+" data-arg-name="octopus_api_key" data-original-value="xxx" data-secret="1">', text):
+            print("  ERROR: the top-level credential row should be masked and flagged data-secret")
+            failed += 1
+        if not re.search(r"data-nested-path='chat\.providers\.openrouter\.api_key' data-nested-original='xxx' data-secret='1'", text):
+            print("  ERROR: the nested credential row should be masked and flagged data-secret")
+            failed += 1
+        if re.search(r"data-nested-path='chat\.providers\.openrouter\.model'[^>]*data-secret", text):
+            print("  ERROR: a non-credential sibling must not be flagged data-secret")
+            failed += 1
+
+        print("Test: /apps_value hands back the real value of the credential being edited")
+        web_interface = _reset_fixture(my_predbat)
+        for path, expected in (("chat.providers.openrouter.api_key", "sk-live-nested-credential-1234"), ("forecast_solar[0].api_key", "fs-live-credential-5678"), ("forecast_solar[0].declination", "30")):
+            result = json.loads(asyncio.run(web_interface.html_apps_value(FakeGetRequest({"path": path}))).text)
+            if not result.get("success") or result.get("value") != expected:
+                print("  ERROR: expected /apps_value for {} to return {}, got: {}".format(path, expected, result))
+                failed += 1
+
+        print("Test: /apps_value refuses a missing path or a whole structure")
+        for query in ({}, {"path": "chat.providers.nope"}, {"path": "chat.providers"}):
+            result = json.loads(asyncio.run(web_interface.html_apps_value(FakeGetRequest(query))).text)
+            if result.get("success") or "value" in result:
+                print("  ERROR: expected /apps_value to refuse {}, got: {}".format(query, result))
+                failed += 1
+
+        print("Test: Edit reveals a flagged credential before opening the input")
+        for function, attribute in (("editValue", "originalValue"), ("editNestedValue", "nestedOriginal")):
+            edit_src = apps_js[apps_js.index("function {}(".format(function)) :]
+            edit_src = edit_src[: edit_src.index("\n}\n")]
+            if "row.dataset.secret === '1'" not in edit_src or "revealSecretValue(row, '{}'".format(attribute) not in edit_src:
+                print("  ERROR: expected {} to fetch the real value of a data-secret row, got:\n{}".format(function, edit_src))
+                failed += 1
+        reveal_src = apps_js[apps_js.index("async function revealSecretValue(") :]
+        reveal_src = reveal_src[: reveal_src.index("\n}\n")]
+        if "fetch('./apps_value?path=' + encodeURIComponent(path))" not in reveal_src or "delete row.dataset.secret" not in reveal_src:
+            print("  ERROR: expected revealSecretValue to fetch /apps_value and replace the masked original, got:\n{}".format(reveal_src))
+            failed += 1
+
+        # A revealed credential is the first value that routinely holds quotes, braces or angle
+        # brackets to reach the editor - the mask "xxx" never did (#5243 review)
+        print("Test: a credential holding quote, brace and angle-bracket characters round-trips through reveal and save")
+        awkward = 'pa"ss{word<x>&{0}'
+        web_interface = _reset_fixture(my_predbat)
+        web_interface.args["chat"]["providers"]["openrouter"]["api_key"] = awkward
+        path = "chat.providers.openrouter.api_key"
+        result = json.loads(asyncio.run(web_interface.html_apps_value(FakeGetRequest({"path": path}))).text)
+        if not result.get("success") or result.get("value") != awkward:
+            print("  ERROR: expected /apps_value to return the brace-bearing credential verbatim, got: {}".format(result))
+            failed += 1
+        else:
+            result = _post_changes(web_interface, {path: {"rowId": 1001, "originalValue": awkward, "newValue": awkward + "2", "type": "string", "isNested": True, "path": path}})
+            if not result.get("success") or _load_yaml()["pred_bat"]["chat"]["providers"]["openrouter"]["api_key"] != awkward + "2":
+                print("  ERROR: the edited quote-bearing credential was not written intact, got: {}".format(result))
+                failed += 1
+
+        print("Test: a digit-only credential saved as a revealed row keeps its leading zeros")
+        web_interface = _reset_fixture(my_predbat)
+        path = "forecast_solar[0].api_key"
+        result = _post_changes(web_interface, {path: {"rowId": 1001, "originalValue": "0042", "newValue": "00430", "type": "string", "isNested": True, "path": path}})
+        if not result.get("success") or str(_load_yaml()["pred_bat"]["forecast_solar"][0]["api_key"]) != "00430":
+            print("  ERROR: expected the credential to be written as the string 00430, got: {} / {}".format(result, _load_yaml()["pred_bat"]["forecast_solar"][0]["api_key"]))
+            failed += 1
+
+        print("Test: /apps_value refuses a negative index into an empty list rather than raising")
+        web_interface = _reset_fixture(my_predbat)
+        web_interface.args["forecast_solar"] = []
+        result = json.loads(asyncio.run(web_interface.html_apps_value(FakeGetRequest({"path": "forecast_solar[-1].api_key"}))).text)
+        if result.get("success") or "value" in result:
+            print("  ERROR: expected a refusal for an index into an empty list, got: {}".format(result))
+            failed += 1
+
+        print("Test: the editor sets a revealed value as a property and treats it as plain text")
+        for function in ("editValue", "editNestedValue"):
+            edit_src = apps_js[apps_js.index("function {}(".format(function)) :]
+            edit_src = edit_src[: edit_src.index("\n}\n")]
+            if 'value="${currentValue}"' in edit_src or "input.value = currentValue" not in edit_src:
+                print("  ERROR: expected {} to assign input.value rather than interpolate the value into markup, got:\n{}".format(function, edit_src))
+                failed += 1
+            if "row.dataset.revealed !== '1' && currentValue" not in edit_src:
+                print("  ERROR: expected {} to skip the entity dropdown for a revealed credential, got:\n{}".format(function, edit_src))
+                failed += 1
+        for function in ("saveValue", "saveNestedValue"):
+            save_src = apps_js[apps_js.index("function {}(".format(function)) :]
+            save_src = save_src[: save_src.index("\n}\n")]
+            if "row.dataset.revealed === '1' ? 'string'" not in save_src:
+                print("  ERROR: expected {} to save a revealed credential as a string, got:\n{}".format(function, save_src))
+                failed += 1
+        if "row.dataset.revealed = '1'" not in reveal_src:
+            print("  ERROR: expected revealSecretValue to mark the row as revealed")
+            failed += 1
+        display_src = apps_js[apps_js.index("function getDisplayValueEntity(") :]
+        display_src = display_src[: display_src.index("\n}\n")]
+        if display_src.count("escapeHtml(") != 2:
+            print("  ERROR: expected getDisplayValueEntity to escape both of its returns, got:\n{}".format(display_src))
+            failed += 1
+
     finally:
         os.chdir(original_dir)
         shutil.rmtree(temp_dir, ignore_errors=True)

apps/predbat/web.py

@@ -91,7 +91,7 @@
     predbat_log_file_prev,
     is_secret_key,
 )
-from utils import is_data_numerical, ROOT_YAML_KEY, SECRET_MASK, YAML_DUMP_WIDTH, parse_yaml_path, update_nested_yaml_value  # noqa: F401 - re-exported: moved to utils.py, agent_tools.py/chat_tools.py must not import from web.py
+from utils import is_data_numerical, ROOT_YAML_KEY, SECRET_MASK, YAML_DUMP_WIDTH, parse_yaml_path, resolve_nested_yaml_value, update_nested_yaml_value  # noqa: F401 - re-exported: moved to utils.py, agent_tools.py/chat_tools.py must not import from web.py
 from const import TIME_FORMAT, TIME_FORMAT_DAILY, TIME_FORMAT_HA, MANUAL_RATE_MAX_MINUTES, MANUAL_TIME_MAX_MINUTES
 from predbat import THIS_VERSION_DISPLAY
 from component_base import ComponentBase
@@ -514,6 +514,7 @@ async def start(self):
         app.router.add_get("/log", self.html_log)
         app.router.add_get("/apps", self.html_apps)
         app.router.add_post("/apps", self.html_apps_post)
+        app.router.add_get("/apps_value", self.html_apps_value)
         app.router.add_get("/charts", self.html_charts)
         app.router.add_get("/config", self.html_config)
         app.router.add_get("/entity", self.html_entity)
@@ -2950,9 +2951,10 @@ def render_type(self, arg, value, parent_path="", row_counter=None):
                     actions_cell += self.render_delete_button(nested_row_id)
 
                 raw_value = self.resolve_value_raw(key, nested_value)
+                secret_attr = self.secret_row_attr(key, nested_value, "'")
 
                 if nested_row_id is not None:
-                    text += f"<tr id='nested_row_{nested_row_id}' data-nested-path='{nested_path_attr}' data-nested-original='{html_module.escape(str(raw_value))}'><td><b>{key}: </b></td><td id='nested_value_{nested_row_id}'>{self.render_type(key, nested_value, nested_path, row_counter)}</td><td>{actions_cell}</td></tr>\n"
+                    text += f"<tr id='nested_row_{nested_row_id}' data-nested-path='{nested_path_attr}' data-nested-original='{html_module.escape(str(raw_value))}'{secret_attr}><td><b>{key}: </b></td><td id='nested_value_{nested_row_id}'>{self.render_type(key, nested_value, nested_path, row_counter)}</td><td>{actions_cell}</td></tr>\n"
                 else:
                     text += "<tr><td><b>{}: </b></td><td colspan='2'>{}</td></tr>\n".format(key, self.render_type(key, nested_value, nested_path, row_counter))
             text += self.render_add_row("addDictKey", [dict_path], "Add setting", row_counter)
@@ -3752,6 +3754,47 @@ def is_editable_value(self, value):
             return len(value) > 0 and any(self.is_editable_value(item) for item in value)
         return False
 
+    def secret_row_attr(self, key, value, quote):
+        """
+        Return the data-secret attribute for a row whose value html_apps() served masked, else "".
+
+        The page holds SECRET_MASK for a credential rather than the credential itself, so the
+        browser's Edit has to fetch the real value (/apps_value) before it can offer it - this flag
+        is how it knows to. mask_secret_args() replaces a secret key's whole value, list or dict
+        included, so a credential container (e.g. redact_strings) is also one masked row and is
+        flagged too; /apps_value refuses to hand a container back, so its Edit ends in that
+        refusal rather than letting the editor overwrite the whole list with a single string.
+        """
+        if value == SECRET_MASK and is_secret_key(key):
+            return " data-secret={}1{}".format(quote, quote)
+        return ""
+
+    async def html_apps_value(self, request):
+        """
+        Return the real value of one apps.yaml setting, for the /apps editor's Edit on a credential.
+
+        html_apps() serves credentials masked, so without this Edit could only offer "xxx" - the
+        user could neither see nor amend the key, and saving it back unchanged is refused. Only
+        the one path asked for is returned, and only when it is a single value, so the page itself
+        still never carries credentials and a whole container cannot be pulled in the clear. The
+        unmasked /debug_apps download already exposes the same values behind the same access.
+
+        The value is returned as stored, not passed through resolve_value_raw(): a credential is
+        literal text, and one holding a brace ("abc{def", "{0}") makes str.format() in
+        resolve_arg() raise ValueError/IndexError, which would surface as a 500 here.
+        """
+        path = request.query.get("path", "")
+        if not path:
+            return web.json_response({"success": False, "message": "No path given"})
+        try:
+            value = resolve_nested_yaml_value(self.args, path)
+        except (KeyError, TypeError, ValueError, IndexError) as e:
+            # IndexError: a negative index into an empty list gets past the lookup's range check
+            return web.json_response({"success": False, "message": f"Path {path} not found: {str(e)}"})
+        if not self.is_editable_value(value) or isinstance(value, list):
+            return web.json_response({"success": False, "message": f"{path} is not a single value that can be edited here - edit apps.yaml directly"})
+        return web.json_response({"success": True, "value": str(value)})
+
     def resolve_value_raw(self, arg, value):
         if isinstance(value, str) and "{" in value:
             text = self.base.resolve_arg(arg, value, indirect=False, quiet=True)
@@ -3870,8 +3913,8 @@ async def html_apps(self, request):
                         # For numerical values, show edit button
                         actions_cell = f'<button class="edit-button" onclick="editValue({row_id})">Edit</button>'
 
-                text += '<tr id="row_{}" data-arg-name="{}" data-original-value="{}"><td>{}</td><td id="value_{}">{}</td><td>{}</td></tr>\n'.format(
-                    row_id, arg_attr, html_module.escape(str(raw_value)), arg, row_id, self.render_type(arg, value, "", row_counter), actions_cell
+                text += '<tr id="row_{}" data-arg-name="{}" data-original-value="{}"{}><td>{}</td><td id="value_{}">{}</td><td>{}</td></tr>\n'.format(
+                    row_id, arg_attr, html_module.escape(str(raw_value)), self.secret_row_attr(arg, value, '"'), arg, row_id, self.render_type(arg, value, "", row_counter), actions_cell
                 )
             row_id += 1