fix(extension): 保护插件凭据引用与删除事务

2026-09-02 15:31:39 +08:00
parent 9e680a0239
commit 022c3226c7
14 changed files with 223 additions and 46 deletions
@@ -23,7 +23,7 @@ uv run uvicorn app.main:app --reload --host 127.0.0.1 --port 8000
 uv run pytest
 ```

-当前基线为 116 项测试通过。Provider API Key 可通过前端设置页写入，也可用 `OPENAI_API_KEY`、`DEEPSEEK_API_KEY` 或 `AINOTE_CREDENTIAL_<ID>` 注入；不要把真实密钥写入仓库。`plugin.*` 是 Plugin Settings 的保留凭据命名空间，通用 Provider 凭据接口不能读写。
+当前基线为 121 项测试通过。Provider API Key 可通过前端设置页写入，也可用 `OPENAI_API_KEY`、`DEEPSEEK_API_KEY` 或 `AINOTE_CREDENTIAL_<ID>` 注入；不要把真实密钥写入仓库。`plugin.*` 是 Plugin Settings 的保留凭据命名空间，通用 Provider 凭据接口不能读写。

 团队接口清单见 `../docs/contracts/后端接口契约-开发版.md`，机器可读契约以运行时的 `/openapi.json` 为准。

@@ -326,6 +326,7 @@ class PluginSettingsStore:
            secret_refs = entry.get("secret_refs", {})
            if not isinstance(stored_values, dict) or not isinstance(secret_refs, dict):
                raise self._storage_format_error(plugin_id)
+            validated_refs = self._validate_secret_refs(plugin_id, secret_refs)
            values = {
                field.key: field.default
                for field in definition.fields
@@ -349,7 +350,7 @@ class PluginSettingsStore:
            for field in definition.fields:
                if field.type != PluginSettingType.secret:
                    continue
-                reference = secret_refs.get(field.key)
+                reference = validated_refs.get(field.key)
                secrets[field.key] = PluginSecretState(
                    configured=isinstance(reference, str) and self._has_secret(reference)
                )
@@ -457,6 +458,7 @@ class PluginSettingsStore:
            refs = entry.get("secret_refs", {})
            if not isinstance(refs, dict):
                raise self._storage_format_error(plugin_id)
+            self._validate_secret_refs(plugin_id, refs)
            entry["secret_refs"] = refs
            try:
                previous = self.credentials.resolve(reference)
@@ -494,16 +496,28 @@ class PluginSettingsStore:
            refs = entry.get("secret_refs", {})
            if not isinstance(refs, dict):
                raise self._storage_format_error(plugin_id)
-            reference = refs.pop(key, None)
-            if reference:
-                try:
-                    self.credentials.delete(reference)
-                except CredentialStoreError as exc:
-                    raise ExtensionError(
-                        "PLUGIN_SECRET_STORE_ERROR", str(exc), status_code=500
-                    ) from exc
-            if plugin_id in data:
+            self._validate_secret_refs(plugin_id, refs)
+            reference = _secret_reference(plugin_id, key)
+            had_reference = refs.pop(key, None) is not None
+            if plugin_id in data and had_reference:
                self._write(data)
+            try:
+                self.credentials.delete(reference)
+            except CredentialStoreError as exc:
+                if had_reference:
+                    refs[key] = reference
+                    try:
+                        self._write(data)
+                    except ExtensionError as rollback_exc:
+                        raise ExtensionError(
+                            "PLUGIN_STORAGE_ERROR",
+                            "Plugin Secret deletion failed and its reference could not be restored.",
+                            status_code=500,
+                            details={"plugin_id": plugin_id, "key": key},
+                        ) from rollback_exc
+                raise ExtensionError(
+                    "PLUGIN_SECRET_STORE_ERROR", str(exc), status_code=500
+                ) from exc
        return PluginSecretStatus(plugin_id=plugin_id, key=key, configured=False)

    def resolve_secret(
@@ -515,7 +529,7 @@ class PluginSettingsStore:
            refs = entry.get("secret_refs", {})
            if not isinstance(refs, dict):
                raise self._storage_format_error(plugin_id)
-            reference = refs.get(key)
+            reference = self._validate_secret_refs(plugin_id, refs).get(key)
        try:
            return self.credentials.resolve(reference) if isinstance(reference, str) else None
        except CredentialStoreError as exc:
@@ -527,21 +541,48 @@ class PluginSettingsStore:
        with self._lock:
            data = self._read()
            entry = data.pop(plugin_id, None)
-            if isinstance(entry, dict):
+            references: list[str] = []
+            if entry is not None and not isinstance(entry, dict):
+                raise self._storage_format_error(plugin_id)
+            if entry is not None:
                refs = entry.get("secret_refs", {})
-                if isinstance(refs, dict):
-                    for reference in refs.values():
-                        if isinstance(reference, str):
-                            try:
-                                self.credentials.delete(reference)
-                            except CredentialStoreError as exc:
-                                raise ExtensionError(
-                                    "PLUGIN_SECRET_STORE_ERROR",
-                                    str(exc),
-                                    status_code=500,
-                                ) from exc
+                if not isinstance(refs, dict):
+                    raise self._storage_format_error(plugin_id)
+                references = list(self._validate_secret_refs(plugin_id, refs).values())
            if entry is not None:
                self._write(data)
+            try:
+                self.credentials.delete_many(references)
+            except CredentialStoreError as exc:
+                if entry is not None:
+                    data[plugin_id] = entry
+                    try:
+                        self._write(data)
+                    except ExtensionError as rollback_exc:
+                        raise ExtensionError(
+                            "PLUGIN_STORAGE_ERROR",
+                            "Plugin uninstall failed and its Settings namespace could not be restored.",
+                            status_code=500,
+                            details={"plugin_id": plugin_id},
+                        ) from rollback_exc
+                raise ExtensionError(
+                    "PLUGIN_SECRET_STORE_ERROR", str(exc), status_code=500
+                ) from exc
+
+    def _validate_secret_refs(
+        self, plugin_id: str, refs: dict[Any, Any]
+    ) -> dict[str, str]:
+        validated: dict[str, str] = {}
+        for key, reference in refs.items():
+            if (
+                not isinstance(key, str)
+                or not _SETTING_KEY.fullmatch(key)
+                or not isinstance(reference, str)
+                or reference != _secret_reference(plugin_id, key)
+            ):
+                raise self._storage_format_error(plugin_id)
+            validated[key] = reference
+        return validated

    def _has_secret(self, reference: str) -> bool:
        try:
@@ -599,15 +640,19 @@ class PluginSettingsStore:

    def _write(self, value: dict[str, dict[str, Any]]) -> None:
        path = self._path()
-        path.parent.mkdir(parents=True, exist_ok=True)
        temporary = path.with_suffix(".tmp")
        try:
+            path.parent.mkdir(parents=True, exist_ok=True)
            temporary.write_text(
                json.dumps(value, ensure_ascii=False, sort_keys=True),
                encoding="utf-8",
            )
            temporary.replace(path)
        except OSError as exc:
+            try:
+                temporary.unlink(missing_ok=True)
+            except OSError:
+                pass
            raise ExtensionError(
                "PLUGIN_STORAGE_ERROR",
                "Plugin settings storage cannot be written.",
@@ -119,17 +119,26 @@ class EncryptedCredentialStore:

    def _write_tokens(self, tokens: dict[str, str]) -> None:
        _, store_path = self._paths()
-        store_path.parent.mkdir(parents=True, exist_ok=True)
-        self._restrict(store_path.parent, 0o700)
        temporary = store_path.with_suffix(".tmp")
-        temporary.write_text(
-            json.dumps(tokens, ensure_ascii=True, sort_keys=True),
-            encoding="utf-8",
-        )
-        self._restrict(temporary, 0o600)
-        # 凭据表同样使用原子替换，确保并发读取只会看到完整 JSON。
-        temporary.replace(store_path)
-        self._restrict(store_path, 0o600)
+        try:
+            store_path.parent.mkdir(parents=True, exist_ok=True)
+            self._restrict(store_path.parent, 0o700)
+            temporary.write_text(
+                json.dumps(tokens, ensure_ascii=True, sort_keys=True),
+                encoding="utf-8",
+            )
+            self._restrict(temporary, 0o600)
+            # 凭据表同样使用原子替换，确保并发读取只会看到完整 JSON。
+            temporary.replace(store_path)
+            self._restrict(store_path, 0o600)
+        except OSError as exc:
+            try:
+                temporary.unlink(missing_ok=True)
+            except OSError:
+                pass
+            raise CredentialStoreError(
+                "Encrypted credential store cannot be written."
+            ) from exc

    def put(self, credential_id: str, secret: str) -> None:
        self._validate_id(credential_id)
@@ -168,6 +177,24 @@ class EncryptedCredentialStore:
                self._write_tokens(tokens)
            return removed

+    def delete_many(self, credential_ids: list[str]) -> set[str]:
+        """用一次原子替换删除多个凭据，避免插件卸载只删除部分 Secret。"""
+
+        for credential_id in credential_ids:
+            self._validate_id(credential_id)
+        with self._lock:
+            tokens = self._read_tokens()
+            removed = {
+                credential_id
+                for credential_id in credential_ids
+                if credential_id in tokens
+            }
+            if removed:
+                for credential_id in removed:
+                    del tokens[credential_id]
+                self._write_tokens(tokens)
+            return removed
+

 class ChainedCredentialResolver:
    def __init__(self, *resolvers: CredentialResolver) -> None:
@@ -1,4 +1,5 @@
 import asyncio
+from pathlib import Path

 import httpx
 import pytest
@@ -35,6 +36,33 @@ def test_encrypted_credential_store_round_trip_without_plaintext_on_disk() -> No
    assert store.resolve("deepseek") is None


+def test_encrypted_credential_store_deletes_multiple_credentials_atomically() -> None:
+    store = EncryptedCredentialStore()
+    store.put("plugin.first", "first")
+    store.put("plugin.second", "second")
+    store.put("openai", "keep")
+
+    removed = store.delete_many(["plugin.first", "plugin.second"])
+
+    assert removed == {"plugin.first", "plugin.second"}
+    assert store.resolve("plugin.first") is None
+    assert store.resolve("plugin.second") is None
+    assert store.resolve("openai") == "keep"
+
+
+def test_credential_write_os_error_uses_stable_store_error(monkeypatch) -> None:
+    store = EncryptedCredentialStore()
+    store.put("existing", "value")
+
+    def fail_replace(_path: Path, _target: Path) -> Path:
+        raise OSError("injected replace failure")
+
+    monkeypatch.setattr(Path, "replace", fail_replace)
+
+    with pytest.raises(CredentialStoreError, match="cannot be written"):
+        store.put("new", "value")
+
+
 def test_credential_api_never_returns_secret() -> None:
    written = asyncio.run(
        put_credential(
@@ -15,6 +15,7 @@ from app.contracts import (
 from app.extensions import ExtensionError, PluginRuntime
 from app.extensions.contributions import _secret_reference
 from app.extensions.runtime import DeclarativePluginHost
+from app.providers.credentials import CredentialStoreError

 TEXT_TOOLS = BACKEND_DIR / "extensions" / "plugins" / "text-tools"

@@ -296,6 +297,80 @@ def test_plugin_secret_reference_has_fixed_credential_safe_length() -> None:
    assert len(reference) <= 128


+def test_tampered_secret_reference_cannot_cross_credential_namespace() -> None:
+    container = build_container()
+    container.credentials.put("openai", "provider-private-secret")
+    settings_path = get_settings().data_dir / "plugins" / "settings.json"
+    settings_path.parent.mkdir(parents=True, exist_ok=True)
+    settings_path.write_text(
+        json.dumps(
+            {
+                "text-tools": {
+                    "schema_version": 1,
+                    "values": {},
+                    "secret_refs": {"api_key": "openai"},
+                }
+            }
+        ),
+        encoding="utf-8",
+    )
+
+    with pytest.raises(ExtensionError) as read_error:
+        container.plugins.get_settings("text-tools")
+    with pytest.raises(ExtensionError) as uninstall_error:
+        container.plugins.uninstall("text-tools")
+
+    assert read_error.value.code == "PLUGIN_STORAGE_ERROR"
+    assert uninstall_error.value.code == "PLUGIN_STORAGE_ERROR"
+    assert container.credentials.resolve("openai") == "provider-private-secret"
+
+
+def test_secret_delete_restores_reference_when_credential_delete_fails(
+    monkeypatch,
+) -> None:
+    container = build_container()
+    container.plugins.put_setting_secret("text-tools", "api_key", "keep-me")
+    settings_path = get_settings().data_dir / "plugins" / "settings.json"
+    original = settings_path.read_text(encoding="utf-8")
+    reference = _secret_reference("text-tools", "api_key")
+
+    def fail_delete(_credential_id: str) -> bool:
+        raise CredentialStoreError("injected delete failure")
+
+    monkeypatch.setattr(container.credentials, "delete", fail_delete)
+
+    with pytest.raises(ExtensionError) as exc:
+        container.plugins.delete_setting_secret("text-tools", "api_key")
+
+    assert exc.value.code == "PLUGIN_SECRET_STORE_ERROR"
+    assert settings_path.read_text(encoding="utf-8") == original
+    assert container.credentials.resolve(reference) == "keep-me"
+
+
+def test_uninstall_restores_settings_when_atomic_secret_delete_fails(
+    monkeypatch,
+) -> None:
+    container = build_container()
+    container.plugins.update_settings("text-tools", 1, {"result_limit": 12})
+    container.plugins.put_setting_secret("text-tools", "api_key", "keep-me")
+    settings_path = get_settings().data_dir / "plugins" / "settings.json"
+    original = settings_path.read_text(encoding="utf-8")
+    reference = _secret_reference("text-tools", "api_key")
+
+    def fail_delete_many(_credential_ids: list[str]) -> set[str]:
+        raise CredentialStoreError("injected batch delete failure")
+
+    monkeypatch.setattr(container.credentials, "delete_many", fail_delete_many)
+
+    with pytest.raises(ExtensionError) as exc:
+        container.plugins.uninstall("text-tools")
+
+    assert exc.value.code == "PLUGIN_SECRET_STORE_ERROR"
+    assert settings_path.read_text(encoding="utf-8") == original
+    assert container.credentials.resolve(reference) == "keep-me"
+    assert container.plugins.get("text-tools").manifest.plugin_id == "text-tools"
+
+
 def test_invalid_command_and_settings_manifest_are_rejected(tmp_path: Path) -> None:
    invalid_command = tmp_path / "invalid-command"
    invalid_command.mkdir()