diff --git a/CLAUDE.md b/CLAUDE.md index 11e90634..67337db8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,6 +46,30 @@ There is also **one shared Gradle wrapper at the repo root** (`gradlew` + `gradl Both jars are referenced via `../libs/*.jar`. **Always use the repo-root `libs/` jars and the repo-root Gradle wrapper — never bundle per-plugin copies.** A plugin that ships its own `libs/plugin-api.jar` / `libs/gradle-plugin.jar` (e.g. copied from another plugin) can drift out of sync with the rest of the repo; point `build.gradle.kts` (`compileOnly`) and `settings.gradle.kts` (buildscript `classpath`) at `../libs/*.jar` and delete any local `libs/`. The root `plugin-api.jar` already carries the full API surface (including `IdeTemplateService`/`CgtTemplateBuilder`), so newer sub-APIs do not justify a local copy. **A plugin folder is not standalone in isolation** — copy the root `libs/` along if you move one elsewhere. When CoGo's API changes, refresh via the script above or the **Update libs from CodeOnTheGo** GitHub Action (which also commits the refreshed jars, cuts a release, and deploys `.cgp` files to the website). +### Credentials: use the host's `KeystoreSecretStore`, never your own crypto + +A plugin that stores a credential encrypts it with `com.itsaky.androidide.plugins.security.KeystoreSecretStore` +from `plugin-api.jar` (**26.35+** — set `plugin.min_ide_version` accordingly). It is `compileOnly` +like the rest of the API, so there is one implementation in the IDE's process rather than a copy +compiled into each `.cgp`. Do not re-implement AES/GCM in a plugin; three AI plugins each grew a +copy that started to diverge, which is what ADFA-5255 removed. + +Construct it with **this plugin's own alias** (`KeystoreSecretStore(ALIAS)`) as a single +top-level `val` in a `SecureApiKeyStore.kt`/`SecureTokenStore.kt` that holds nothing but the alias; +callers use that instance directly. The store logs under its own name — it takes no log tag, and +the second constructor parameter is a `SecretKeySource` override that plugins do not pass. +`ai-agent-mcp`, `ai-agent-gemini` and `ai-agent-openai` are the reference shape. Do **not** wrap +it in an object of forwarding methods — that is just a second copy of the store's contract to keep +in step. The alias must be unique per +plugin (all plugins share the host's UID and Keystore, so a shared alias lets one plugin's +invalidated-key recovery delete another's secret) and must never change across releases. + +`readAndMigrate` returns a three-way `Stored` (`Absent` / `Value` / `Unreadable`) rather than a +nullable String on purpose: "never saved" and "saved but this device's Keystore can no longer open +it" need opposite advice, and a plugin that collapses them tells a user their credential was +refused when it was never sent. Collapse it only where the caller genuinely has one answer for +both, and say so in a comment. + ### Plugin shape A plugin is an Android *application* module (despite installing as a library) with: diff --git a/ai-agent-gemini/README.md b/ai-agent-gemini/README.md index 4be7e9d3..128acc06 100644 --- a/ai-agent-gemini/README.md +++ b/ai-agent-gemini/README.md @@ -27,10 +27,14 @@ The key is entered in **AI Core → Agent settings**, not here. It is stored encrypted (AES/GCM under a hardware-backed Android Keystore secret) and sent as an `x-goog-api-key` **header**, never in a URL query string. -`security/SecureApiKeyStore.kt` is the only copy of the crypto — this plugin owns -both the write and the read, so there are no constants to keep in sync with -another plugin. A key written under an earlier plugin id is adopted once by -`preferences/GeminiPreferences.kt` and re-encrypted here. +`security/SecureApiKeyStore.kt` holds only this plugin's Keystore alias +(`cotg_ai_gemini_key_v1`); the AES/GCM itself is the IDE's `KeystoreSecretStore` +(`plugin-api`, since **26.35** — hence this plugin's `min_ide_version`), so there +is one implementation in the process rather than a copy per plugin. The alias +stays per plugin: they all share the host's Keystore, so a shared alias would let +one plugin's invalidated-key recovery delete another's secret. A key written under +an earlier plugin id is adopted once by `preferences/GeminiPreferences.kt` and +re-encrypted here. ## Installation @@ -57,7 +61,7 @@ root of `com/itsaky/androidide/plugins/aiagentgemini/`. - `plugin/GeminiPlugin.kt` — plugin entry point; registers the backend with ai-core - `backend/GeminiBackend.kt` — the REST transport, streaming (SSE) and model catalog - `errors/GeminiErrorFormatter.kt` — turns an API failure into one translated sentence -- `security/SecureApiKeyStore.kt` — AES/GCM at rest +- `security/SecureApiKeyStore.kt` — this plugin's Keystore alias, over the IDE's `KeystoreSecretStore` - `preferences/GeminiPreferences.kt` — this plugin's settings store, plus the one-time adoption of settings written under earlier plugin ids - `prompt/GeminiSystemPrompt.kt` — the system prompt this cloud model is given diff --git a/ai-agent-gemini/ai-agent-gemini.html b/ai-agent-gemini/ai-agent-gemini.html index 9a228040..e73c45ab 100644 --- a/ai-agent-gemini/ai-agent-gemini.html +++ b/ai-agent-gemini/ai-agent-gemini.html @@ -82,9 +82,9 @@

Technical architecture

GeminiErrorFormatterClassifies a failure (retired model, quota, refused key, outage, unreachable) so it can be reported as one translated sentence. - SecureApiKeyStoreAES/GCM encryption of the API - key under a hardware-backed Android Keystore secret owned by this - plugin. + SecureApiKeyStoreBinds this plugin's Keystore + alias to the IDE's KeystoreSecretStore, which AES/GCM-encrypts + the API key under a hardware-backed Android Keystore secret. GeminiSettingsFragmentThe settings pane AI Core mounts: key entry and verification, visibility toggle, and the model picker driven by the live catalog. diff --git a/ai-agent-gemini/build.gradle.kts b/ai-agent-gemini/build.gradle.kts index d3ecedf3..ab95ea6c 100644 --- a/ai-agent-gemini/build.gradle.kts +++ b/ai-agent-gemini/build.gradle.kts @@ -71,8 +71,8 @@ dependencies { testImplementation("org.json:json:20231013") } -// SecureApiKeyStore is no longer duplicated: this plugin holds the only copy, so there is nothing -// left to drift against. The parity check that guarded the ai-assistant copy went with that plugin. +// No SecureApiKeyStore parity check any more: the AES/GCM core is the host's KeystoreSecretStore +// (plugin-api), so there is one implementation in the process rather than copies to keep in step. // AAR metadata checks are disabled by convention for these application-as-library plugins. tasks.matching { diff --git a/ai-agent-gemini/src/main/AndroidManifest.xml b/ai-agent-gemini/src/main/AndroidManifest.xml index 5c721c7b..74ba6b64 100644 --- a/ai-agent-gemini/src/main/AndroidManifest.xml +++ b/ai-agent-gemini/src/main/AndroidManifest.xml @@ -33,7 +33,7 @@ and pairs with ai-core, which requires the same release. --> + android:value="26.35" /> stored.plain.trim().takeIf { it.isNotBlank() } + KeystoreSecretStore.Stored.Absent -> null + // Reported here rather than passed on as "no key": generation fails either way, but a + // lost Keystore entry needs the key entering again, and the log is all that says so. + KeystoreSecretStore.Stored.Unreadable -> { + context.logger.warn( + "GeminiBackend: the saved API key cannot be decrypted on this device; " + + "it has to be entered again in settings" + ) + null + } + // Transient, so it returns without caching: the key is very likely intact, and caching + // this answer would freeze "no key" until the stored value itself changed. + KeystoreSecretStore.Stored.Unavailable -> { + context.logger.warn( + "GeminiBackend: the keystore could not be reached to read the saved API key; " + + "retrying on the next read" + ) + return null + } + } val raw = prefs?.getString(GeminiPreferences.KEY_API_KEY, null) keyCache = raw?.let { it to plain } return plain diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt index a23b01ff..c59d33ee 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/preferences/GeminiPreferences.kt @@ -59,7 +59,7 @@ internal object GeminiPreferences { * Copies this backend's settings out of every store in [LEGACY_FILES], once. * * The API key moves as ciphertext and stays readable: it is encrypted under a Keystore alias - * (see [SecureApiKeyStore]) rather than under anything plugin-specific, and every plugin runs + * (see [secureApiKeyStore]) rather than under anything plugin-specific, and every plugin runs * in the host's process and UID. Copies rather than moves, so downgrading still finds the old * values. Call before anything reads a setting. * diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt index 198b65e0..6daa4ca2 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/security/SecureApiKeyStore.kt @@ -1,146 +1,17 @@ package com.itsaky.androidide.plugins.aiagentgemini.security -import android.content.SharedPreferences -import android.security.keystore.KeyGenParameterSpec -import android.security.keystore.KeyPermanentlyInvalidatedException -import android.security.keystore.KeyProperties -import android.util.Base64 -import android.util.Log -import com.itsaky.androidide.plugins.aiagentgemini.logging.LOG_PREFIX -import java.security.GeneralSecurityException -import java.security.KeyStore -import javax.crypto.Cipher -import javax.crypto.KeyGenerator -import javax.crypto.SecretKey -import javax.crypto.spec.GCMParameterSpec +import com.itsaky.androidide.plugins.security.KeystoreSecretStore + +/** Unique to this plugin and fixed across releases; see [KeystoreSecretStore] for why both matter. */ +private const val ALIAS = "cotg_ai_gemini_key_v1" /** - * AES/GCM encryption for sensitive settings (currently the Gemini API key), - * keyed by a hardware-backed Android Keystore secret. Only ciphertext is - * written to SharedPreferences, so a copied prefs file (root, `adb backup`, - * forensic dump) is useless without this device's Keystore. + * This plugin's binding of [KeystoreSecretStore]: its API key, encrypted under this plugin's own + * Keystore alias. * - * The [ALIAS] must stay stable across releases: a key encrypted under one - * alias cannot be read under another, so changing it silently invalidates - * every stored key. It is also what lets a key written before the AI plugins - * were reorganised still decrypt today — every plugin runs in the host app's - * process and UID, so they all share one Android Keystore. + * The store is the IDE's, from plugin-api, and callers use it directly. A forwarding object per + * method would only be a second copy of its contract to keep in step — and one that had to pick a + * single answer for "absent" and "no longer decryptable", which callers here do not share. The + * thing this file owns is the alias. */ -object SecureApiKeyStore { - private const val TAG = "$LOG_PREFIX.SecureApiKeyStore" - private const val KEYSTORE = "AndroidKeyStore" - private const val ALIAS = "cotg_ai_gemini_key_v1" - private const val TRANSFORM = "AES/GCM/NoPadding" - private const val IV_LEN = 12 - private const val TAG_BITS = 128 - - /** Marks a stored value as ciphertext; anything without it is treated as legacy plaintext. */ - const val ENC_PREFIX = "enc:v1:" - - private fun getOrCreateKey(): SecretKey { - val ks = KeyStore.getInstance(KEYSTORE).apply { load(null) } - (ks.getEntry(ALIAS, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } - val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE) - generator.init( - KeyGenParameterSpec.Builder( - ALIAS, - KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT - ) - .setBlockModes(KeyProperties.BLOCK_MODE_GCM) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) - .build() - ) - return generator.generateKey() - } - - private fun deleteKey() { - try { - KeyStore.getInstance(KEYSTORE).apply { load(null) }.deleteEntry(ALIAS) - } catch (e: Exception) { - Log.w(TAG, "Failed to delete Keystore alias $ALIAS", e) - } - } - - private fun encryptWith(key: SecretKey, plain: String): String { - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.ENCRYPT_MODE, key) - val iv = cipher.iv - val ciphertext = cipher.doFinal(plain.toByteArray(Charsets.UTF_8)) - val combined = ByteArray(iv.size + ciphertext.size) - System.arraycopy(iv, 0, combined, 0, iv.size) - System.arraycopy(ciphertext, 0, combined, iv.size, ciphertext.size) - return ENC_PREFIX + Base64.encodeToString(combined, Base64.NO_WRAP) - } - - /** - * Encrypt [plain] into a self-describing string: [ENC_PREFIX] + base64(iv | ciphertext). - * - * The key is not auth-bound, so a credential change does not invalidate it; an alias an - * OEM Keystore drops anyway is regenerated once before retrying. - * - * @param plain the value to encrypt - * @throws GeneralSecurityException on any other Keystore/cipher failure, so the caller can - * inform the user instead of crashing the IDE on Save - */ - @Throws(GeneralSecurityException::class) - fun encrypt(plain: String): String { - return try { - encryptWith(getOrCreateKey(), plain) - } catch (e: KeyPermanentlyInvalidatedException) { - Log.w(TAG, "Keystore key invalidated; regenerating and retrying encrypt", e) - deleteKey() - encryptWith(getOrCreateKey(), plain) - } - } - - /** - * Return the plaintext for a stored value, handling both formats transparently: - * an [ENC_PREFIX] value is decrypted; anything else is returned unchanged as - * legacy plaintext (use [readAndMigrate] to upgrade it in place). Returns - * null if a ciphertext value can't be decrypted — e.g. the Keystore key was - * lost or invalidated — in which case the user must re-enter the key. - */ - fun decrypt(stored: String?): String? { - if (stored == null) return null - if (!stored.startsWith(ENC_PREFIX)) return stored - return try { - val combined = Base64.decode(stored.removePrefix(ENC_PREFIX), Base64.NO_WRAP) - val iv = combined.copyOfRange(0, IV_LEN) - val ciphertext = combined.copyOfRange(IV_LEN, combined.size) - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.DECRYPT_MODE, getOrCreateKey(), GCMParameterSpec(TAG_BITS, iv)) - String(cipher.doFinal(ciphertext), Charsets.UTF_8) - } catch (e: Exception) { - Log.w(TAG, "Failed to decrypt stored API key", e) - null - } - } - - /** - * Read [key] from [prefs], upgrading a legacy plaintext value to ciphertext in place. - * - * Keys written before this store existed are still plaintext on disk, and [decrypt] alone - * hands them back unchanged forever — so an install that configured its key earlier would - * never actually gain encryption. Re-encrypting on the first read closes that gap without - * making the user re-enter the key. - * - * The value is trimmed on migration, so the stored, displayed and sent forms all agree. - * - * Keystore IPC + AES/GCM, so call this off the main thread. - * - * @return the trimmed plaintext value, or null when nothing is stored or decryption failed. - */ - fun readAndMigrate(prefs: SharedPreferences?, key: String): String? { - val stored = prefs?.getString(key, null) ?: return null - if (stored.startsWith(ENC_PREFIX)) return decrypt(stored) - val plain = stored.trim() - if (plain.isEmpty()) return plain - try { - prefs.edit().putString(key, encrypt(plain)).apply() - Log.i(TAG, "Upgraded legacy plaintext value for '$key' to ciphertext") - } catch (e: Exception) { - Log.w(TAG, "Could not upgrade legacy plaintext value for '$key' to ciphertext", e) - } - return plain - } -} +val secureApiKeyStore = KeystoreSecretStore(ALIAS) diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt index bec2f7bd..c9dbea4b 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsFragment.kt @@ -31,6 +31,7 @@ import com.itsaky.androidide.plugins.PluginContext import com.itsaky.androidide.plugins.aiagentgemini.plugin.GeminiPlugin import com.itsaky.androidide.plugins.aiagentgemini.R import com.itsaky.androidide.plugins.base.PluginFragmentHelper +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import com.itsaky.androidide.plugins.services.IdeTooltipService import kotlinx.coroutines.launch import java.text.SimpleDateFormat @@ -189,20 +190,30 @@ class GeminiSettingsFragment : Fragment() { } viewLifecycleOwner.lifecycleScope.launch { - val savedApiKey = viewModel.getGeminiApiKey() + val stored = viewModel.getGeminiApiKey() + val savedApiKey = (stored as? KeystoreSecretStore.Stored.Value)?.plain val hasKey = !savedApiKey.isNullOrBlank() updateUiState(isEditing = !hasKey) if (hasKey) { statusTextView.text = savedApiKeyStatusText() } else { apiKeyInput.setText("") - // A stored-but-undecryptable key also reads as null; warn as the Edit path does. - if (viewModel.hasStoredGeminiApiKey()) { + // Only for a key that is there and will not decrypt; an empty box alone looks like + // data loss. Nothing stored at all is the ordinary first run and says nothing. + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() + } else if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from the above: the key is still there and intact, so this + // must not send the user off to find and type it again. + Toast.makeText( + requireContext(), + getString(R.string.msg_api_key_unavailable), + Toast.LENGTH_LONG + ).show() } } } @@ -383,20 +394,30 @@ class GeminiSettingsFragment : Fragment() { editButton.setOnClickListener { editButton.isEnabled = false viewLifecycleOwner.lifecycleScope.launch { - val apiKey = try { + val stored = try { viewModel.getGeminiApiKey() } finally { editButton.isEnabled = true } - // null = a key IS stored but won't decrypt; an empty box alone looks like data loss. - if (apiKey == null) { + // A key that is stored and will not decrypt; an empty box alone looks like data + // loss. Told apart from "nothing stored" here, which this button rarely sees but + // must not report as a lost Keystore entry when it does. + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() + } else if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from the above: the key is still there and intact, so this + // must not send the user off to find and type it again. + Toast.makeText( + requireContext(), + getString(R.string.msg_api_key_unavailable), + Toast.LENGTH_LONG + ).show() } - revealEditMode(apiKey.orEmpty()) + revealEditMode((stored as? KeystoreSecretStore.Stored.Value)?.plain.orEmpty()) } } diff --git a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt index 12060510..3e47a891 100644 --- a/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt +++ b/ai-agent-gemini/src/main/kotlin/com/itsaky/androidide/plugins/aiagentgemini/settings/GeminiSettingsViewModel.kt @@ -10,7 +10,8 @@ import com.itsaky.androidide.plugins.PluginLogger import com.itsaky.androidide.plugins.aiagentgemini.backend.GeminiBackend import com.itsaky.androidide.plugins.aiagentgemini.logging.LOG_PREFIX import com.itsaky.androidide.plugins.aiagentgemini.preferences.GeminiPreferences -import com.itsaky.androidide.plugins.aiagentgemini.security.SecureApiKeyStore +import com.itsaky.androidide.plugins.aiagentgemini.security.secureApiKeyStore +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers @@ -112,7 +113,7 @@ class GeminiSettingsViewModel( } /** - * Encrypts [apiKey] via [SecureApiKeyStore] and persists only the ciphertext to private prefs, + * Encrypts [apiKey] via [secureApiKeyStore] and persists only the ciphertext to private prefs, * off the main thread. Nothing is written on failure. Kept separate from [verifyGeminiKey]: a * rejected key never reaches here, and an unverifiable one only after the user says so. * @@ -130,7 +131,7 @@ class GeminiSettingsViewModel( return@withContext false } val encrypted = try { - SecureApiKeyStore.encrypt(apiKey.trim()) + secureApiKeyStore.encrypt(apiKey.trim()) } catch (e: Exception) { logger?.error("$TAG: failed to encrypt Gemini API key", e) return@withContext false @@ -156,19 +157,15 @@ class GeminiSettingsViewModel( * Decrypt the stored key off the main thread (Keystore IPC + AES/GCM), upgrading a * pre-encryption plaintext key to ciphertext in passing so existing installs actually * end up encrypted rather than waiting for the user to re-enter the key. + * + * @return what is on disk: nothing, the key, a key this device's Keystore can no longer open, + * or one it would not open just now. Those are not the same — a lost Keystore entry has to be + * entered again, a keystore that did not answer only retried — so the caller says which. */ - suspend fun getGeminiApiKey(): String? = withContext(ioDispatcher) { - SecureApiKeyStore.readAndMigrate(prefs(), KEY_API_KEY) + suspend fun getGeminiApiKey(): KeystoreSecretStore.Stored = withContext(ioDispatcher) { + secureApiKeyStore.readAndMigrate(prefs(), KEY_API_KEY) } - /** - * True when a key is present on disk, whether or not it can still be decrypted. Lets the UI - * tell "nothing was saved" from "the Keystore entry is gone" — [getGeminiApiKey] is null for - * both. Raw pref only, so no Keystore IPC and safe on the main thread. - */ - fun hasStoredGeminiApiKey(): Boolean = - !prefs()?.getString(KEY_API_KEY, null).isNullOrBlank() - fun getGeminiApiKeySaveTimestamp(): Long = prefs()?.getLong(KEY_API_KEY_TIMESTAMP, 0L) ?: 0L fun clearGeminiApiKey() { @@ -199,9 +196,11 @@ class GeminiSettingsViewModel( _geminiModelsLoading.postValue(true) try { - val apiKey = getGeminiApiKey()?.trim() + // The fallback list is the answer to any key it cannot read, whatever the reason, + // so this is one of the few callers that has no use for the difference. + val apiKey = (getGeminiApiKey() as? KeystoreSecretStore.Stored.Value)?.plain?.trim() if (apiKey.isNullOrBlank()) { - logger?.warn("$TAG: no Gemini API key saved; showing fallback models") + logger?.warn("$TAG: no usable Gemini API key saved; showing fallback models") _geminiModels.postValue(GeminiModelOptions(FALLBACK_MODELS, isLive = false)) return@launch } diff --git a/ai-agent-gemini/src/main/res/values/strings.xml b/ai-agent-gemini/src/main/res/values/strings.xml index 255b1997..d5345f3e 100644 --- a/ai-agent-gemini/src/main/res/values/strings.xml +++ b/ai-agent-gemini/src/main/res/values/strings.xml @@ -30,6 +30,7 @@ API Key saved on: %s API Key saved and verified on: %s The stored API key could not be read on this device. Please enter it again. + The device keystore could not be reached, so the stored API key could not be read. It is still saved — please try again in a moment. Couldn\'t save the API key on this device. Please try again. Checking this key with Google… Verified, your API key works diff --git a/ai-agent-mcp/ai-agent-mcp.html b/ai-agent-mcp/ai-agent-mcp.html index 121feb82..d45b4742 100644 --- a/ai-agent-mcp/ai-agent-mcp.html +++ b/ai-agent-mcp/ai-agent-mcp.html @@ -107,8 +107,9 @@

Technical architecture

McpToolTextSanitises server-supplied names and descriptions — untrusted remote text that would otherwise land verbatim in a prompt assembled inside a third-party backend plugin. - SecureTokenStoreAES/GCM encryption under a - Keystore alias owned by this plugin, so only ciphertext reaches disk. + SecureTokenStoreBinds this plugin's own Keystore + alias to the IDE's KeystoreSecretStore, whose AES/GCM keeps + only ciphertext on disk.

The transport is MCP's Streamable HTTP revision over HttpURLConnection. Two dependencies were deliberately not taken: diff --git a/ai-agent-mcp/build.gradle.kts b/ai-agent-mcp/build.gradle.kts index cf5a3325..b082c4ce 100644 --- a/ai-agent-mcp/build.gradle.kts +++ b/ai-agent-mcp/build.gradle.kts @@ -75,6 +75,7 @@ dependencies { testImplementation(files("../libs/plugin-api.jar")) testImplementation("junit:junit:4.13.2") + testImplementation("io.mockk:mockk:1.13.8") testImplementation("org.json:json:20240303") testImplementation("org.jetbrains.kotlinx:kotlinx-coroutines-test:1.8.1") } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt index ace2ea23..1f11a8a6 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpConnections.kt @@ -2,10 +2,11 @@ package com.itsaky.androidide.plugins.aiagentmcp.client import android.util.Log import com.itsaky.androidide.plugins.aiagentmcp.logging.LOG_PREFIX -import com.itsaky.androidide.plugins.aiagentmcp.security.SecureTokenStore +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException import com.itsaky.androidide.plugins.aiagentmcp.settings.McpServer import com.itsaky.androidide.plugins.aiagentmcp.settings.McpServerStore +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import java.security.MessageDigest import java.util.concurrent.ConcurrentHashMap @@ -89,13 +90,21 @@ object McpConnections { * @return its token and headers; either may be empty. * @throws UnreadableSecretException when a stored credential cannot be decrypted here. Sending * the request without it would earn a 401 and tell the user their token was refused. + * @throws UnavailableSecretException when the keystore could not be reached to decrypt one, a + * failure to retry rather than to report as a lost credential. */ private fun credentialsFor(serverId: String): McpCredentials { val token = when (val stored = McpServerStore.token(serverId)) { - is SecureTokenStore.Stored.Value -> stored.plain - SecureTokenStore.Stored.Absent -> "" - SecureTokenStore.Stored.Unreadable -> + is KeystoreSecretStore.Stored.Value -> stored.plain + KeystoreSecretStore.Stored.Absent -> "" + KeystoreSecretStore.Stored.Unreadable -> throw UnreadableSecretException("The stored token for '$serverId' cannot be decrypted.") + // Not Unreadable: the token is very likely intact and the call is worth repeating, so + // the user is told to retry rather than to enter the token again. + KeystoreSecretStore.Stored.Unavailable -> + throw UnavailableSecretException( + "The stored token for '$serverId' could not be read just now." + ) } return McpCredentials(token, McpServerStore.headers(serverId)) } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatter.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatter.kt index 30bc3ddd..05081868 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatter.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/errors/McpErrorFormatter.kt @@ -3,6 +3,7 @@ package com.itsaky.androidide.plugins.aiagentmcp.errors import android.content.Context import com.itsaky.androidide.plugins.aiagentmcp.R import com.itsaky.androidide.plugins.aiagentmcp.client.McpProtocolException +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHttpException import com.itsaky.androidide.plugins.aiagentmcp.transport.McpRedirectException @@ -64,6 +65,9 @@ sealed interface McpFailure { /** A stored credential cannot be decrypted on this device, so nothing was sent. */ data object SecretUnreadable : McpFailure + /** The keystore would not open a stored credential just now, so nothing was sent — retry. */ + data object SecretUnavailable : McpFailure + /** The server redirected somewhere the request cannot be repeated with its credentials. */ data object RedirectRefused : McpFailure @@ -88,6 +92,7 @@ object McpErrorFormatter { fun classify(error: Throwable): McpFailure = when (error) { // Before the IOException branches below, which it is one of. is UnreadableSecretException -> McpFailure.SecretUnreadable + is UnavailableSecretException -> McpFailure.SecretUnavailable is McpRedirectException -> McpFailure.RedirectRefused is McpHttpException -> forStatus(error.statusCode) is McpProtocolException -> McpFailure.Rejected(error.message.orEmpty()) @@ -125,6 +130,8 @@ object McpErrorFormatter { McpFailure.Cancelled -> context.getString(R.string.mcp_error_cancelled, serverName) McpFailure.SecretUnreadable -> context.getString(R.string.mcp_error_secret_unreadable, serverName) + McpFailure.SecretUnavailable -> + context.getString(R.string.mcp_error_secret_unavailable, serverName) McpFailure.RedirectRefused -> context.getString(R.string.mcp_error_redirect_refused, serverName) is McpFailure.ServerError -> diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPlugin.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPlugin.kt index 2dc68346..79732e27 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPlugin.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPlugin.kt @@ -18,6 +18,7 @@ import com.itsaky.androidide.plugins.services.SharedServices import com.itsaky.androidide.plugins.services.ToolSourceRegistry import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job import kotlinx.coroutines.SupervisorJob import kotlinx.coroutines.cancel import kotlinx.coroutines.isActive @@ -38,15 +39,33 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { /** True once [toolSource] is registered with AI Core, so re-registration is idempotent. */ @Volatile private var registered = false + /** + * Serialises every swap of [scope]. + * + * Cancelling the old scope and installing a new one is one transition, not two: without this, + * two lifecycle calls landing together can each cancel the scope the other has already replaced, + * leaving a live refresh behind after [deactivate] or orphaning an activation's scope uncancelled. + * `@Volatile` alone would publish each write but still let the pair interleave. + */ + private val lifecycleLock = Any() + /** * Background work: listing tools is network work and never belongs on the main thread. * * Replaced on every [activate] and cancelled by [deactivate], so a refresh left running cannot * register sessions in [McpConnections] after `closeAll()` emptied the map. Volatile like - * [registered]: the host may drive the lifecycle from one thread and the next from another. + * [registered]: the host may drive the lifecycle from one thread and the next from another, + * and [scopeJob] reads it outside [lifecycleLock]. */ @Volatile private var scope = newScope() + /** + * The scope [stopScope] leaves behind: cancelled from birth, so a `launch` arriving after the + * lifecycle edge is the no-op it has always been. One per plugin, since cancellation is + * terminal and a cancelled scope carries no state a later stop could disturb. + */ + private val stoppedScope = newScope().apply { cancel() } + companion object { /** Must match `plugin.id` in AndroidManifest.xml; also this source's provider id. */ const val PLUGIN_ID = "com.itsaky.androidide.plugins.aiagentmcp" @@ -117,9 +136,10 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { } override fun activate(): Boolean = try { - // Cancelled first: a host that activates twice would otherwise orphan the running scope. - scope.cancel() - scope = newScope() + // Cancelled and replaced as one step: a host that activates twice would otherwise orphan + // the running scope, and the launch below has to use this activation's scope, not whatever + // a concurrent lifecycle call has since installed. + val active = swapScope(newScope()) toolSource = McpToolSource() McpServerStore.addChangeListener(settingsChanged) context.addPluginLifecycleListener(aiCoreLifecycle) @@ -130,7 +150,7 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { // Tool lists are answered from cache, so the cache has to be filled before the user opens // the Agent — otherwise the first cold-start session sees no MCP tools at all. - scope.launch { + active.launch { val refreshed = McpToolCatalog.refreshAll { isActive } if (refreshed > 0 && isActive) settingsChanged() } @@ -146,7 +166,7 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { unregisterToolSource() // Before the connections are closed: an in-flight refresh would otherwise repopulate the // catalogue and the session map straight after they were cleared. - scope.cancel() + stopScope() releaseConnections() true } catch (e: Exception) { @@ -158,7 +178,7 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { runCatching { context.removePluginLifecycleListener(aiCoreLifecycle) } McpServerStore.removeChangeListener(settingsChanged) unregisterToolSource() - scope.cancel() + stopScope() releaseConnections() pluginContext = null context.logger.info("McpPlugin: disposed") @@ -167,6 +187,41 @@ class McpPlugin : IPlugin, SettingsExtension, DocumentationExtension { /** A fresh scope for this activation; the previous one is cancelled, never reused. */ private fun newScope() = CoroutineScope(SupervisorJob() + Dispatchers.IO) + /** + * Installs [next] as the current scope and cancels whichever scope it displaced. + * + * @param next the scope to install. + * @return [next], so a caller can launch on the scope it installed rather than re-reading the + * field and handing its work to a later activation. + */ + private fun swapScope(next: CoroutineScope): CoroutineScope { + val previous = synchronized(lifecycleLock) { scope.also { scope = next } } + previous.cancel() + return next + } + + /** + * Ends the current activation's scope, leaving an already-cancelled one in the field. + * + * Installing [stoppedScope] rather than cancelling the field in place: an [activate] running + * alongside this has by then installed a scope of its own, and cancelling whatever the field + * happens to hold would either miss it or kill it. Swapping ends exactly the scope this call + * displaced. + */ + private fun stopScope() { + swapScope(stoppedScope) + } + + /** + * The current activation scope's job. + * + * A seam: the rule that a second [activate] orphans nothing and that [deactivate] leaves no + * refresh running is otherwise only observable on a device, where the symptom is a background + * `tools/list` repopulating a catalogue that was just cleared. + */ + internal val scopeJob: Job? + get() = scope.coroutineContext[Job] + /** * Registers this plugin's tools with AI Core, if the registry is reachable. * diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/SecureTokenStore.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/SecureTokenStore.kt index 40e44e13..38139600 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/SecureTokenStore.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/SecureTokenStore.kt @@ -1,199 +1,16 @@ package com.itsaky.androidide.plugins.aiagentmcp.security -import android.content.SharedPreferences -import android.security.keystore.KeyGenParameterSpec -import android.security.keystore.KeyPermanentlyInvalidatedException -import android.security.keystore.KeyProperties -import android.util.Base64 -import android.util.Log -import com.itsaky.androidide.plugins.aiagentmcp.logging.LOG_PREFIX -import java.security.GeneralSecurityException -import java.security.KeyStore -import javax.crypto.Cipher -import javax.crypto.KeyGenerator -import javax.crypto.SecretKey -import javax.crypto.spec.GCMParameterSpec +import com.itsaky.androidide.plugins.security.KeystoreSecretStore -private const val TAG = "$LOG_PREFIX.SecureTokenStore" +/** Unique to this plugin and fixed across releases; see [KeystoreSecretStore] for why both matter. */ +private const val ALIAS = "cotg_ai_mcp_token_v1" /** - * AES/GCM encryption for the bearer tokens of configured MCP servers, keyed by a hardware-backed - * Android Keystore secret. Only ciphertext is written to SharedPreferences, so a copied prefs file - * (root, `adb backup`, forensic dump) is useless without this device's Keystore. + * This plugin's binding of [KeystoreSecretStore]: the bearer tokens and extra headers of configured + * MCP servers, encrypted under this plugin's own Keystore alias. * - * The [ALIAS] must stay stable across releases — a token encrypted under one alias cannot be read - * under another — and is deliberately this plugin's own: every plugin runs in the host's process - * and UID and therefore shares one Keystore, so a shared alias would let this plugin's recovery - * path destroy a backend plugin's stored key as a side effect. + * The store is the IDE's, from plugin-api, and callers use it directly. A forwarding object per + * method would only be a second copy of its contract to keep in step — the thing this file owns is + * the alias. */ -object SecureTokenStore { - - /** - * What was found under a preference key. - * - * Three outcomes rather than a nullable String: "nothing stored" and "stored but no longer - * readable on this device" lead to opposite advice, and collapsing them is what tells a user - * their token was refused when it was never sent. - */ - sealed interface Stored { - - /** Nothing is stored under the key. */ - data object Absent : Stored - - /** The stored value, decrypted. */ - data class Value(val plain: String) : Stored - - /** Something is stored, but this device's Keystore can no longer open it. */ - data object Unreadable : Stored - } - - private const val KEYSTORE = "AndroidKeyStore" - private const val ALIAS = "cotg_ai_mcp_token_v1" - private const val TRANSFORM = "AES/GCM/NoPadding" - private const val IV_LEN = 12 - private const val TAG_BITS = 128 - - /** Marks a stored value as ciphertext; anything without it is treated as legacy plaintext. */ - const val ENC_PREFIX = "enc:v1:" - - /** - * Encrypts [plain] into a self-describing string: [ENC_PREFIX] + base64(iv | ciphertext). - * - * The key is not auth-bound, so a credential change does not invalidate it; an alias an OEM - * Keystore drops anyway is regenerated once before retrying. - * - * @param plain the value to encrypt. - * @return the ciphertext to store. - * @throws GeneralSecurityException on any other Keystore or cipher failure, so the caller can - * tell the user instead of crashing the IDE on Save. - */ - @Throws(GeneralSecurityException::class) - fun encrypt(plain: String): String = try { - encryptWith(getOrCreateKey(), plain) - } catch (e: KeyPermanentlyInvalidatedException) { - Log.w(TAG, "Keystore key invalidated; regenerating and retrying encrypt", e) - deleteKey() - encryptWith(getOrCreateKey(), plain) - } - - /** - * Reads a stored value back. - * @param stored the stored string, ciphertext or legacy plaintext. - * @return the plaintext, or null when a ciphertext value cannot be decrypted — the Keystore key - * was lost, and the user has to enter the token again. - */ - fun decrypt(stored: String?): String? { - if (stored == null) return null - if (!stored.startsWith(ENC_PREFIX)) return stored - return try { - val combined = Base64.decode(stored.removePrefix(ENC_PREFIX), Base64.NO_WRAP) - val iv = combined.copyOfRange(0, IV_LEN) - val ciphertext = combined.copyOfRange(IV_LEN, combined.size) - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.DECRYPT_MODE, getOrCreateKey(), GCMParameterSpec(TAG_BITS, iv)) - // Zeroed once the String is built; see the note in encryptWith about the String. - val plainBytes = cipher.doFinal(ciphertext) - try { - String(plainBytes, Charsets.UTF_8) - } finally { - plainBytes.fill(0) - } - } catch (e: Exception) { - Log.w(TAG, "Failed to decrypt a stored MCP token", e) - null - } - } - - /** - * Stores [plain] under [key], encrypted; an empty value removes the entry instead. - * - * Keystore IPC plus AES/GCM, so call this off the main thread. - * - * @param prefs where to store it. - * @param key the preference key. - * @param plain the token, or empty to forget it. - * @return true when the value was stored (or removed), false when encryption failed. - */ - fun write(prefs: SharedPreferences?, key: String, plain: String): Boolean { - val editor = prefs?.edit() ?: return false - if (plain.isBlank()) { - editor.remove(key).apply() - return true - } - return try { - editor.putString(key, encrypt(plain)).apply() - true - } catch (e: Exception) { - Log.e(TAG, "Could not encrypt a token for '$key'", e) - false - } - } - - /** - * Reads [key] from [prefs], upgrading a legacy plaintext value to ciphertext in place. - * - * @param prefs where the value lives. - * @param key the preference key. - * @return what was found: nothing, the plaintext, or a value that cannot be decrypted here. - */ - fun readAndMigrate(prefs: SharedPreferences?, key: String): Stored { - val stored = prefs?.getString(key, null) ?: return Stored.Absent - if (stored.startsWith(ENC_PREFIX)) { - // A lost Keystore alias — restore onto new hardware, an OEM reset, a re-enrolled screen - // lock — is not the same as an absent token, and must not be reported as one. - return decrypt(stored)?.let(Stored::Value) ?: Stored.Unreadable - } - val plain = stored.trim() - if (plain.isEmpty()) return Stored.Value(plain) - try { - prefs.edit().putString(key, encrypt(plain)).apply() - Log.i(TAG, "Upgraded a legacy plaintext token to ciphertext") - } catch (e: Exception) { - Log.w(TAG, "Could not upgrade a legacy plaintext token to ciphertext", e) - } - return Stored.Value(plain) - } - - private fun getOrCreateKey(): SecretKey { - val store = KeyStore.getInstance(KEYSTORE).apply { load(null) } - (store.getEntry(ALIAS, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } - val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE) - generator.init( - KeyGenParameterSpec.Builder( - ALIAS, - KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT - ) - .setBlockModes(KeyProperties.BLOCK_MODE_GCM) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) - .build() - ) - return generator.generateKey() - } - - private fun deleteKey() { - try { - KeyStore.getInstance(KEYSTORE).apply { load(null) }.deleteEntry(ALIAS) - } catch (e: Exception) { - Log.w(TAG, "Failed to delete Keystore alias $ALIAS", e) - } - } - - private fun encryptWith(key: SecretKey, plain: String): String { - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.ENCRYPT_MODE, key) - val iv = cipher.iv - // Zeroed straight after the cipher reads it. The String itself cannot be: every API this - // token passes through — SharedPreferences, JSONObject, setRequestProperty — takes one, so - // a CharArray here would only move the immutable copy one frame away. - val plainBytes = plain.toByteArray(Charsets.UTF_8) - val ciphertext = try { - cipher.doFinal(plainBytes) - } finally { - plainBytes.fill(0) - } - val combined = ByteArray(iv.size + ciphertext.size) - System.arraycopy(iv, 0, combined, 0, iv.size) - System.arraycopy(ciphertext, 0, combined, iv.size, ciphertext.size) - return ENC_PREFIX + Base64.encodeToString(combined, Base64.NO_WRAP) - } -} +val secureTokenStore = KeystoreSecretStore(ALIAS) diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/UnavailableSecretException.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/UnavailableSecretException.kt new file mode 100644 index 00000000..cb3355cd --- /dev/null +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/security/UnavailableSecretException.kt @@ -0,0 +1,14 @@ +package com.itsaky.androidide.plugins.aiagentmcp.security + +import java.io.IOException + +/** + * A stored credential this device's Keystore would not open just now. + * + * Its own type rather than an [UnreadableSecretException], because the two lead to opposite advice: + * the credential here is very likely intact — the Keystore was not ready, or a binder call failed — + * and the only useful thing to say is to try again, not to enter the credential over. + * + * @param detail what could not be read, for logcat; the user sees the formatted sentence instead. + */ +class UnavailableSecretException(detail: String) : IOException(detail) diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt index 75e60950..ff8cfcc8 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStore.kt @@ -4,9 +4,11 @@ import android.content.SharedPreferences import android.util.Log import com.itsaky.androidide.plugins.aiagentmcp.logging.LOG_PREFIX import com.itsaky.androidide.plugins.aiagentmcp.plugin.McpPlugin -import com.itsaky.androidide.plugins.aiagentmcp.security.SecureTokenStore +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException +import com.itsaky.androidide.plugins.aiagentmcp.security.secureTokenStore import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHeaders +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import java.util.UUID import java.util.concurrent.CopyOnWriteArrayList import org.json.JSONArray @@ -164,10 +166,11 @@ object McpServerStore { * * @param id the server the token belongs to. * @param token the token, or blank to remove it. - * @return true when it was stored. + * @return true once it is on disk, or removed for a blank one; false when encrypting it or the + * write itself failed, which is what the pane must not report as saved. */ fun setToken(id: String, token: String): Boolean { - val stored = SecureTokenStore.write(prefs(), KEY_TOKEN_PREFIX + id, token) + val stored = secureTokenStore.write(prefs(), KEY_TOKEN_PREFIX + id, token) // Like every other mutator: a new credential has to reach the agent, or it keeps calling // with the old one until something else happens to touch the store. fireChanged() @@ -180,10 +183,11 @@ object McpServerStore { * Keystore work, so call this off the main thread. * * @param id the server. - * @return what is stored: nothing, the token, or a token this device can no longer read. + * @return what is stored: nothing, the token, a token this device can no longer read, or one + * the keystore would not open just now. */ - fun token(id: String): SecureTokenStore.Stored = - SecureTokenStore.readAndMigrate(prefs(), KEY_TOKEN_PREFIX + id) + fun token(id: String): KeystoreSecretStore.Stored = + secureTokenStore.readAndMigrate(prefs(), KEY_TOKEN_PREFIX + id) /** True when a token is stored for [id], without decrypting it. */ fun hasToken(id: String): Boolean = prefs()?.contains(KEY_TOKEN_PREFIX + id) == true @@ -200,13 +204,18 @@ object McpServerStore { * @return the headers in the order they were entered; empty when there are none. */ fun headers(id: String): Map { - val stored = SecureTokenStore.readAndMigrate(prefs(), KEY_HEADERS_PREFIX + id) - if (stored is SecureTokenStore.Stored.Unreadable) { + val stored = secureTokenStore.readAndMigrate(prefs(), KEY_HEADERS_PREFIX + id) + if (stored is KeystoreSecretStore.Stored.Unreadable) { // Same failure as an unreadable token, and reported the same way: sending the request // without them would look like the server refusing a credential that is still correct. throw UnreadableSecretException("The stored headers for '$id' cannot be decrypted.") } - val raw = (stored as? SecureTokenStore.Stored.Value)?.plain ?: return emptyMap() + if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Kept apart from the above: these headers are very likely intact, so the caller is + // told to retry instead of asking the user to enter them again. + throw UnavailableSecretException("The stored headers for '$id' could not be read just now.") + } + val raw = (stored as? KeystoreSecretStore.Stored.Value)?.plain ?: return emptyMap() return try { val json = JSONObject(raw) val parsed = LinkedHashMap() @@ -225,7 +234,8 @@ object McpServerStore { * * @param id the server the headers belong to. * @param headers the headers to store; unusable pairs are dropped. - * @return true when they were stored. + * @return true once they are on disk, or removed for an empty map; false when encrypting them + * or the write itself failed. */ fun setHeaders(id: String, headers: Map): Boolean { val clean = McpHeaders.sanitize(headers) @@ -237,12 +247,12 @@ object McpServerStore { } val json = JSONObject() clean.forEach { (name, value) -> json.put(name, value) } - val stored = SecureTokenStore.write(prefs(), key, json.toString()) + val stored = secureTokenStore.write(prefs(), key, json.toString()) fireChanged() return stored } - /** How many extra headers are configured for [id], without decrypting them. */ + /** Whether any extra header is configured for [id], without decrypting them. */ fun hasHeaders(id: String): Boolean = prefs()?.contains(KEY_HEADERS_PREFIX + id) == true /** diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt index 281fe248..af9e1867 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsFragment.kt @@ -211,6 +211,9 @@ class McpSettingsFragment : Fragment() { if (form.secretsUnreadable) { tokenField.hint = getString(R.string.mcp_hint_token_unreadable) status.text = getString(R.string.mcp_secrets_unreadable) + } else if (form.secretsUnavailable) { + tokenField.hint = getString(R.string.mcp_hint_token_unavailable) + status.text = getString(R.string.mcp_secrets_unavailable) } else if (form.hasToken) { tokenField.hint = getString(R.string.mcp_hint_token_stored) } diff --git a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt index 6b0c41d5..09372cbb 100644 --- a/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt +++ b/ai-agent-mcp/src/main/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpSettingsViewModel.kt @@ -8,9 +8,10 @@ import com.itsaky.androidide.plugins.aiagentmcp.R import com.itsaky.androidide.plugins.aiagentmcp.client.McpConnections import com.itsaky.androidide.plugins.aiagentmcp.client.McpTool import com.itsaky.androidide.plugins.aiagentmcp.errors.McpErrorFormatter -import com.itsaky.androidide.plugins.aiagentmcp.security.SecureTokenStore +import com.itsaky.androidide.plugins.aiagentmcp.security.UnavailableSecretException import com.itsaky.androidide.plugins.aiagentmcp.security.UnreadableSecretException import com.itsaky.androidide.plugins.aiagentmcp.tools.McpToolCatalog +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -48,12 +49,15 @@ class McpSettingsViewModel( * unreadable" has to count as a stored credential or the control that clears it hides. * @property secretsUnreadable whether a stored token or header cannot be decrypted on this * device, which the field has to say aloud: it looks stored, but nothing can send it. + * @property secretsUnavailable whether the keystore merely would not answer this time, which + * the field says differently: the credential is intact and the read is worth repeating. * @property headers the extra headers configured for the server. */ data class FormState( val hasToken: Boolean, val hasHeaders: Boolean, val secretsUnreadable: Boolean, + val secretsUnavailable: Boolean, val headers: Map, ) @@ -81,16 +85,26 @@ class McpSettingsViewModel( viewModelScope.launch { val state = withContext(Dispatchers.IO) { val token = McpServerStore.token(id) + var headersUnreadable = false + var headersUnavailable = false val headers = try { McpServerStore.headers(id) } catch (e: UnreadableSecretException) { + headersUnreadable = true + null + } catch (e: UnavailableSecretException) { + headersUnavailable = true null } FormState( hasToken = McpServerStore.hasToken(id), hasHeaders = McpServerStore.hasHeaders(id), secretsUnreadable = - token is SecureTokenStore.Stored.Unreadable || headers == null, + token is KeystoreSecretStore.Stored.Unreadable || headersUnreadable, + // Both can be true; the dialog shows the unreadable message first, since a + // credential that has to be entered again is the worse news. + secretsUnavailable = + token is KeystoreSecretStore.Stored.Unavailable || headersUnavailable, headers = headers.orEmpty(), ) } diff --git a/ai-agent-mcp/src/main/res/values/strings.xml b/ai-agent-mcp/src/main/res/values/strings.xml index 39a48b28..e3067541 100644 --- a/ai-agent-mcp/src/main/res/values/strings.xml +++ b/ai-agent-mcp/src/main/res/values/strings.xml @@ -24,6 +24,7 @@ Leave empty if the server needs none Stored — type to replace it Stored but unreadable — type it again + Stored, but unreadable right now — try again Connect Save Cancel @@ -42,6 +43,7 @@ The stored token and headers for this server have been removed. Still reading this server\'s stored credentials — try again in a moment. The saved token or headers for this server can no longer be read on this device. Enter them again. + The saved token or headers for this server could not be read just now. They are still stored — close this and try again in a moment. Connecting… Connected to %1$s. It listed %2$d tools below — new ones start switched off. Connected to %1$s, which offers no tools. @@ -65,6 +67,7 @@ The call to %1$s was cancelled. %1$s redirected the request to another address, so it was not sent — it carries your token. Check the endpoint URL. %1$s\'s saved token can no longer be read on this device. Open MCP server settings and enter it again. + %1$s\'s saved token could not be read just now. It is still stored — try again in a moment. Could not reach %1$s. Could not reach %1$s: %2$s \'%1$s\' is no longer offered by any configured MCP server. diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpSessionLifecycleTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpSessionLifecycleTest.kt index bee9d7f7..01ecb8b7 100644 --- a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpSessionLifecycleTest.kt +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/client/McpSessionLifecycleTest.kt @@ -1,14 +1,18 @@ package com.itsaky.androidide.plugins.aiagentmcp.client import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHttpClient +import com.itsaky.androidide.plugins.aiagentmcp.transport.McpHttpException import java.net.HttpURLConnection +import org.json.JSONArray import org.json.JSONObject import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull import org.junit.Assert.assertTrue import org.junit.Test /** - * Whether `notifications/initialized` is sent, which the negotiated revision alone decides. + * What a session does across calls: whether `notifications/initialized` is sent, and whether the + * handshake it paid for is then kept alive rather than repeated or silently lost. * * Reading an absent `Mcp-Session-Id` as "stateless" left a conforming 2025-06-18 server without * the notification, so its next `tools/list` answered "not initialized" and the user saw no tools. @@ -18,15 +22,38 @@ class McpSessionLifecycleTest { private companion object { const val ENDPOINT = "https://example.test/mcp" const val NOTIFICATION = "notifications/initialized" + const val INITIALIZE = "initialize" + const val LIST_TOOLS = "tools/list" + const val SESSION = "s-1" + const val TOOL = "search" } - /** Answers `initialize` with [protocolVersion], recording the methods it was asked for. */ + /** + * Answers `initialize` with [protocolVersion], recording the methods it was asked for. + * + * @param protocolVersion the revision the handshake reports back. + * @param sessionId the session the server assigns, or null for one that keeps no state. + */ private class FakeHttpClient( private val protocolVersion: String, private val sessionId: String? = null, ) : McpHttpClient() { - val methods = mutableListOf() + /** One request the client made, with the session it carried. */ + data class Request(val method: String, val sessionId: String?) + + val requests = mutableListOf() + + /** Sessions the client ended with a DELETE. */ + val deletedSessions = mutableListOf() + + /** The method to answer `404` for, standing in for a session the server forgot. */ + var expiringMethod: String? = null + + /** How many more times [expiringMethod] answers `404` before it starts working. */ + var expiriesLeft = 0 + + val methods: List get() = requests.map { it.method } override fun post( url: String, @@ -38,15 +65,34 @@ class McpSessionLifecycleTest { onConnected: (HttpURLConnection) -> Unit, ): Response { val method = body.optString("method") - methods += method - if (method != "initialize") return Response(null, this.sessionId) - val result = JSONObject().put("protocolVersion", this.protocolVersion) + requests += Request(method, sessionId) + if (method == expiringMethod && expiriesLeft > 0) { + expiriesLeft-- + throw McpHttpException(HttpURLConnection.HTTP_NOT_FOUND, "session expired") + } + val result = when (method) { + INITIALIZE -> JSONObject().put("protocolVersion", this.protocolVersion) + LIST_TOOLS -> JSONObject().put( + "tools", + JSONArray().put(JSONObject().put("name", TOOL)) + ) + else -> return Response(null, this.sessionId) + } val document = JSONObject() .put("jsonrpc", "2.0") .put("id", body.opt("id")) .put("result", result) return Response(document.toString(), this.sessionId) } + + override fun deleteSession( + url: String, + token: String, + sessionId: String, + extraHeaders: Map, + ) { + deletedSessions += sessionId + } } private fun sessionOn(http: McpHttpClient) = @@ -58,16 +104,16 @@ class McpSessionLifecycleTest { sessionOn(http).initialize() - assertEquals(listOf("initialize", NOTIFICATION), http.methods) + assertEquals(listOf(INITIALIZE, NOTIFICATION), http.methods) } @Test fun givenAStatefulRevisionAndASessionHeader_whenInitializing_thenTheNotificationIsSent() { - val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = "s-1") + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION) sessionOn(http).initialize() - assertEquals(listOf("initialize", NOTIFICATION), http.methods) + assertEquals(listOf(INITIALIZE, NOTIFICATION), http.methods) } @Test @@ -76,7 +122,7 @@ class McpSessionLifecycleTest { sessionOn(http).initialize() - assertEquals(listOf("initialize"), http.methods) + assertEquals(listOf(INITIALIZE), http.methods) } @Test @@ -87,4 +133,60 @@ class McpSessionLifecycleTest { assertTrue("an unparseable revision must fail safe", NOTIFICATION in http.methods) } + + @Test + fun givenAnInitializedSession_whenMoreCallsFollow_thenTheHandshakeIsNotRepeated() { + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION) + val session = sessionOn(http) + + session.initialize() + session.listTools() + session.listTools() + + // The handshake is what a kept-alive session buys; paying it per call is the regression. + assertEquals(1, http.methods.count { it == INITIALIZE }) + assertEquals(2, http.methods.count { it == LIST_TOOLS }) + } + + @Test + fun givenAServerAssignedSession_whenACallFollows_thenItCarriesTheSessionHeader() { + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION) + + sessionOn(http).listTools() + + assertNull("the handshake itself has no session yet", http.requests.first().sessionId) + assertTrue( + "every later request must carry the assigned session", + http.requests.drop(1).all { it.sessionId == SESSION } + ) + } + + @Test + fun givenAnExpiredSession_whenTheServerAnswers404_thenItReInitializesAndRetriesOnce() { + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION).apply { + expiringMethod = LIST_TOOLS + expiriesLeft = 1 + } + + val tools = sessionOn(http).listTools() + + assertEquals( + listOf(INITIALIZE, NOTIFICATION, LIST_TOOLS, INITIALIZE, NOTIFICATION, LIST_TOOLS), + http.methods + ) + assertEquals(listOf(TOOL), tools.map { it.name }) + } + + @Test + fun givenAClosedSession_whenItIsUsedAgain_thenTheServerSessionIsEndedAndTheHandshakeRepeats() { + val http = FakeHttpClient(McpSession.PREFERRED_PROTOCOL_VERSION, sessionId = SESSION) + val session = sessionOn(http) + + session.listTools() + session.close() + session.listTools() + + assertEquals(listOf(SESSION), http.deletedSessions) + assertEquals(2, http.methods.count { it == INITIALIZE }) + } } diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt new file mode 100644 index 00000000..22aa9231 --- /dev/null +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/plugin/McpPluginScopeTest.kt @@ -0,0 +1,93 @@ +package com.itsaky.androidide.plugins.aiagentmcp.plugin + +import com.itsaky.androidide.plugins.PluginContext +import com.itsaky.androidide.plugins.aiagentmcp.testing.FakeSharedPreferences +import io.mockk.every +import io.mockk.mockk +import kotlinx.coroutines.Job +import org.junit.After +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotSame +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +/** + * Whether [McpPlugin] leaves a coroutine scope running past the lifecycle edge that ended it. + * + * The scope carries the cold-start `tools/list` refresh, which registers sessions in + * `McpConnections` and fills `McpToolCatalog`. Left running past `deactivate`, it repopulates both + * straight after they were cleared, leaving sockets nothing can reach — and a host that activates + * twice would orphan the first scope entirely. + */ +class McpPluginScopeTest { + + private val prefs = FakeSharedPreferences() + private lateinit var context: PluginContext + private lateinit var plugin: McpPlugin + + @Before + fun setUp() { + context = mockk(relaxed = true) + every { context.getPluginSharedPreferences(any()) } returns prefs + plugin = McpPlugin() + plugin.initialize(context) + } + + @After + fun tearDown() { + plugin.dispose() + } + + @Test + fun givenAnActivatedPlugin_whenActivatedAgain_thenThePreviousScopeIsCancelled() { + plugin.activate() + val first = requireNotNull(plugin.scopeJob) + + plugin.activate() + + val second = requireNotNull(plugin.scopeJob) + assertTrue("the orphaned scope must not outlive the activation", first.isCancelled) + assertNotSame("a second activation gets a scope of its own", first, second) + assertScopeUsable(second) + } + + @Test + fun givenAnActivatedPlugin_whenDeactivated_thenTheScopeIsCancelled() { + plugin.activate() + val job = requireNotNull(plugin.scopeJob) + + plugin.deactivate() + + assertTrue("an in-flight refresh must not survive deactivation", job.isCancelled) + } + + @Test + fun givenAnActivatedPlugin_whenDisposed_thenTheScopeIsCancelled() { + plugin.activate() + val job = requireNotNull(plugin.scopeJob) + + plugin.dispose() + + assertTrue("dispose must leave nothing running", job.isCancelled) + } + + @Test + fun givenADeactivatedPlugin_whenActivatedAgain_thenItGetsALiveScope() { + plugin.activate() + plugin.deactivate() + + plugin.activate() + + assertScopeUsable(requireNotNull(plugin.scopeJob)) + } + + /** + * Asserts a scope can still take work, which a cancelled one cannot. + * @param job the activation scope's job. + */ + private fun assertScopeUsable(job: Job) { + assertFalse("the current activation's scope must be live", job.isCancelled) + assertTrue("the current activation's scope must accept work", job.isActive) + } +} diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStoreLockTest.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStoreLockTest.kt new file mode 100644 index 00000000..8e7532f2 --- /dev/null +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/settings/McpServerStoreLockTest.kt @@ -0,0 +1,124 @@ +package com.itsaky.androidide.plugins.aiagentmcp.settings + +import com.itsaky.androidide.plugins.PluginContext +import com.itsaky.androidide.plugins.aiagentmcp.plugin.McpPlugin +import com.itsaky.androidide.plugins.aiagentmcp.testing.FakeSharedPreferences +import io.mockk.every +import io.mockk.mockk +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import org.junit.After +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test + +/** + * Whether [McpServerStore]'s lock actually serialises the read-modify-write of the server list. + * + * The list is one JSON blob, so every mutator reads it whole and puts it back whole. A refresh on + * `McpPlugin`'s scope and a toggle on the screen's dispatcher do interleave in practice, and the + * loser's write simply vanishes — the switch stays on while `enabledTools` on disk no longer holds + * it, which reads to the user as the Agent ignoring a tool they enabled. + */ +class McpServerStoreLockTest { + + private companion object { + /** Enough concurrent writers to lose one, few enough to stay fast. */ + const val WRITERS = 8 + + /** Widens the read-modify-write window; see [FakeSharedPreferences.readDelayMillis]. */ + const val READ_DELAY_MS = 2L + + const val JOIN_TIMEOUT_SECONDS = 30L + } + + private val prefs = FakeSharedPreferences() + private lateinit var plugin: McpPlugin + private lateinit var serverId: String + + @Before + fun setUp() { + // Through the plugin's own lifecycle rather than a hook on the store: `initialize` is how + // the host hands over the preferences [McpServerStore] then reads, so the test drives the + // same path the device does and the store keeps no test-only surface. + val context = mockk(relaxed = true) + every { context.getPluginSharedPreferences(any()) } returns prefs + plugin = McpPlugin() + plugin.initialize(context) + serverId = McpServerStore.saveDetails( + McpServerStore.newServer("Docs", "https://example.test/mcp") + ).id + } + + @After + fun tearDown() { + plugin.dispose() + } + + @Test + fun givenConcurrentToolToggles_whenTheyInterleave_thenNoWriteIsLost() { + val tools = (1..WRITERS).map { "tool_$it" } + McpServerStore.setKnownTools(serverId, tools) + prefs.readDelayMillis = READ_DELAY_MS + + runTogether(tools) { McpServerStore.setToolEnabled(serverId, it, true) } + + assertEquals(tools.toSet(), McpServerStore.server(serverId)?.enabledTools) + } + + @Test + fun givenAToggleAndAWholeServerSwitchAtOnce_whenTheyInterleave_thenNeitherIsLost() { + val tools = (1..WRITERS).map { "tool_$it" } + McpServerStore.setKnownTools(serverId, tools) + prefs.readDelayMillis = READ_DELAY_MS + + runTogether(tools + "disable") { work -> + if (work == "disable") { + McpServerStore.setEnabled(serverId, false) + } else { + McpServerStore.setToolEnabled(serverId, work, true) + } + } + + val stored = McpServerStore.server(serverId) + assertEquals(tools.toSet(), stored?.enabledTools) + assertFalse("the whole-server switch must survive the toggles", stored?.enabled ?: true) + } + + /** + * Runs [work] on one thread per item, all released at once. + * + * @param items one item per thread. + * @param work what each thread does with its item. + */ + private fun runTogether(items: List, work: (String) -> Unit) { + val start = CountDownLatch(1) + val done = CountDownLatch(items.size) + val threads = items.map { item -> + // Daemons, and joined in a `finally` below: a writer wedged on the store's lock must not + // outlive the assertion that noticed it and hold the Gradle test worker's JVM open. + Thread { + start.await() + try { + work(item) + } finally { + done.countDown() + } + }.apply { + isDaemon = true + start() + } + } + try { + start.countDown() + assertTrue( + "the writers did not finish", + done.await(JOIN_TIMEOUT_SECONDS, TimeUnit.SECONDS) + ) + } finally { + threads.forEach { it.join(TimeUnit.SECONDS.toMillis(JOIN_TIMEOUT_SECONDS)) } + } + } +} diff --git a/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt new file mode 100644 index 00000000..f8660738 --- /dev/null +++ b/ai-agent-mcp/src/test/kotlin/com/itsaky/androidide/plugins/aiagentmcp/testing/FakeSharedPreferences.kt @@ -0,0 +1,98 @@ +package com.itsaky.androidide.plugins.aiagentmcp.testing + +import android.content.SharedPreferences +import java.util.concurrent.ConcurrentHashMap + +/** + * An in-memory [SharedPreferences], so the settings store can be exercised off a device. + * + * The seam this plugs into exists for one reason — showing that `McpServerStore`'s lock actually + * serialises a read-modify-write — so [readDelayMillis] is here too: a real preferences read costs + * a lock and a file, and a race that needs microseconds of window is not a race a test would ever + * catch on an in-memory map. + */ +class FakeSharedPreferences : SharedPreferences { + + private val values = ConcurrentHashMap() + + /** How long a read blocks, widening the window a missing lock would lose a write in. */ + @Volatile + var readDelayMillis: Long = 0 + + override fun getAll(): MutableMap = HashMap(values) + + override fun getString(key: String, defValue: String?): String? { + if (readDelayMillis > 0) Thread.sleep(readDelayMillis) + return values[key] ?: defValue + } + + override fun getStringSet(key: String, defValues: MutableSet?): MutableSet? = + defValues + + override fun getInt(key: String, defValue: Int): Int = defValue + + override fun getLong(key: String, defValue: Long): Long = defValue + + override fun getFloat(key: String, defValue: Float): Float = defValue + + override fun getBoolean(key: String, defValue: Boolean): Boolean = defValue + + override fun contains(key: String): Boolean = values.containsKey(key) + + override fun edit(): SharedPreferences.Editor = FakeEditor() + + override fun registerOnSharedPreferenceChangeListener( + listener: SharedPreferences.OnSharedPreferenceChangeListener? + ) = Unit + + override fun unregisterOnSharedPreferenceChangeListener( + listener: SharedPreferences.OnSharedPreferenceChangeListener? + ) = Unit + + /** Batches edits and applies them at once, as the real editor does. */ + private inner class FakeEditor : SharedPreferences.Editor { + + private val puts = LinkedHashMap() + private val removals = LinkedHashSet() + private var cleared = false + + override fun putString(key: String, value: String?): SharedPreferences.Editor { + if (value == null) removals += key else puts[key] = value + return this + } + + override fun putStringSet( + key: String, + values: MutableSet? + ): SharedPreferences.Editor = this + + override fun putInt(key: String, value: Int): SharedPreferences.Editor = this + + override fun putLong(key: String, value: Long): SharedPreferences.Editor = this + + override fun putFloat(key: String, value: Float): SharedPreferences.Editor = this + + override fun putBoolean(key: String, value: Boolean): SharedPreferences.Editor = this + + override fun remove(key: String): SharedPreferences.Editor { + removals += key + return this + } + + override fun clear(): SharedPreferences.Editor { + cleared = true + return this + } + + override fun commit(): Boolean { + if (cleared) values.clear() + removals.forEach { values.remove(it) } + values.putAll(puts) + return true + } + + override fun apply() { + commit() + } + } +} diff --git a/ai-agent-openai/README.md b/ai-agent-openai/README.md index 86476f22..182ed5ca 100644 --- a/ai-agent-openai/README.md +++ b/ai-agent-openai/README.md @@ -97,14 +97,15 @@ after configuring OpenAI cannot put that bearer token on the network in the clea A key stored before the origin was recorded is still sent, since it cannot be shown to belong elsewhere. The connection test applies the same rule. -`security/SecureApiKeyStore.kt` is this plugin's **own copy**, under its own -Keystore alias (`cotg_ai_openai_key_v1`). It is deliberately not shared with -ai-agent-gemini's copy: every plugin runs in the host app's process and UID and -therefore shares one Keystore, so a shared alias would let one plugin's +`security/SecureApiKeyStore.kt` holds only this plugin's Keystore alias +(`cotg_ai_openai_key_v1`); the AES/GCM itself is the IDE's `KeystoreSecretStore` +(`plugin-api`, since **26.35** — hence this plugin's `min_ide_version`), so there +is one implementation in the process rather than a copy per plugin. The **alias** +is deliberately not shared: every plugin runs in the host app's process and UID +and therefore shares one Keystore, so a shared alias would let one plugin's invalidated-key recovery (`deleteEntry`) destroy the other backend's stored key. -The two never read each other's ciphertext, so they have no reason to share an -alias — and there is therefore nothing to keep in parity. Extracting the shared -*source* is tracked separately. +The plugins never read each other's ciphertext, so they have no reason to share +one. ## Installation @@ -135,7 +136,7 @@ root of `com/itsaky/androidide/plugins/aiagentopenai/`. - `backend/SseChunk.kt` — one line of the token stream (pure) - `backend/ChatModelFilter.kt` — keeps non-chat models out of the picker (pure) - `errors/OpenAiErrorFormatter.kt` — turns a failure into one translated sentence -- `security/SecureApiKeyStore.kt` — AES/GCM at rest +- `security/SecureApiKeyStore.kt` — this plugin's Keystore alias, over the IDE's `KeystoreSecretStore` - `preferences/OpenAiPreferences.kt` — this plugin's settings store - `prompt/OpenAiSystemPrompt.kt` — the system prompt this cloud model is given - `settings/BaseUrlPolicy.kt` — URL normalization and the cleartext rule (pure) diff --git a/ai-agent-openai/ai-agent-openai.html b/ai-agent-openai/ai-agent-openai.html index 4508ae41..8403d0f9 100644 --- a/ai-agent-openai/ai-agent-openai.html +++ b/ai-agent-openai/ai-agent-openai.html @@ -118,8 +118,9 @@

Technical architecture

OpenAiErrorFormatterClassifies a failure (unknown model, rate limit, spent balance, refused key, outage, server not running) so it can be reported as one translated sentence. - SecureApiKeyStoreAES/GCM encryption of the API - key, under this plugin's own Keystore alias. + SecureApiKeyStoreBinds this plugin's own + Keystore alias to the IDE's KeystoreSecretStore, which + AES/GCM-encrypts the API key.

No third-party HTTP SDK. Plugins run in the host IDE's classloader where okhttp3 resolves to the host's older OkHttp — a mismatch that diff --git a/ai-agent-openai/src/main/AndroidManifest.xml b/ai-agent-openai/src/main/AndroidManifest.xml index aefa3899..dd3d2814 100644 --- a/ai-agent-openai/src/main/AndroidManifest.xml +++ b/ai-agent-openai/src/main/AndroidManifest.xml @@ -38,7 +38,7 @@ carries it once that host version is known. --> + android:value="26.35" /> stored.plain.trim().takeIf { it.isNotBlank() } + KeystoreSecretStore.Stored.Absent -> null + // Reported here rather than passed on as "no key": generation fails either way, but a + // lost Keystore entry needs the key entering again, and the log is all that says so. + KeystoreSecretStore.Stored.Unreadable -> { + logger.warn( + "ApiKeyCache: the saved API key cannot be decrypted on this device; " + + "it has to be entered again in settings" + ) + null + } + // Transient, so it returns without caching: the key is very likely intact, and caching + // this answer would freeze "no key" until the stored value itself changed. + KeystoreSecretStore.Stored.Unavailable -> { + logger.warn( + "ApiKeyCache: the keystore could not be reached to read the saved API key; " + + "retrying on the next read" + ) + return null + } + } val raw = prefs?.getString(prefKey, null) cached = raw?.let { it to plain } return plain diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/security/SecureApiKeyStore.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/security/SecureApiKeyStore.kt index 5f383bd4..e6ae6667 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/security/SecureApiKeyStore.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/security/SecureApiKeyStore.kt @@ -1,143 +1,17 @@ package com.itsaky.androidide.plugins.aiagentopenai.security -import android.content.SharedPreferences -import android.security.keystore.KeyGenParameterSpec -import android.security.keystore.KeyPermanentlyInvalidatedException -import android.security.keystore.KeyProperties -import android.util.Base64 -import android.util.Log -import com.itsaky.androidide.plugins.aiagentopenai.logging.LOG_PREFIX -import java.security.GeneralSecurityException -import java.security.KeyStore -import javax.crypto.Cipher -import javax.crypto.KeyGenerator -import javax.crypto.SecretKey -import javax.crypto.spec.GCMParameterSpec +import com.itsaky.androidide.plugins.security.KeystoreSecretStore + +/** Unique to this plugin and fixed across releases; see [KeystoreSecretStore] for why both matter. */ +private const val ALIAS = "cotg_ai_openai_key_v1" /** - * AES/GCM encryption for this plugin's API key, keyed by a hardware-backed Android Keystore secret. - * Only ciphertext is written to SharedPreferences, so a copied prefs file (root, `adb backup`, - * forensic dump) is useless without this device's Keystore. - * - * The [ALIAS] must stay stable across releases: a key encrypted under one alias cannot be read - * under another, so changing it silently invalidates every stored key. + * This plugin's binding of [KeystoreSecretStore]: its API key, encrypted under this plugin's own + * Keystore alias. * - * It is also deliberately **this plugin's own** alias, not the one ai-agent-gemini uses. Every - * plugin runs in the host app's process and UID and therefore shares one Keystore, so a shared - * alias would let [deleteKey] — the recovery path for an invalidated key — destroy the other - * backend's stored key as a side effect. The two plugins never read each other's ciphertext, so - * they have no reason to share. + * The store is the IDE's, from plugin-api, and callers use it directly. A forwarding object per + * method would only be a second copy of its contract to keep in step — and one that had to pick a + * single answer for "absent" and "no longer decryptable", which callers here do not share. The + * thing this file owns is the alias. */ -object SecureApiKeyStore { - private const val TAG = "$LOG_PREFIX.SecureApiKeyStore" - private const val KEYSTORE = "AndroidKeyStore" - private const val ALIAS = "cotg_ai_openai_key_v1" - private const val TRANSFORM = "AES/GCM/NoPadding" - private const val IV_LEN = 12 - private const val TAG_BITS = 128 - - /** Marks a stored value as ciphertext; anything without it is treated as legacy plaintext. */ - const val ENC_PREFIX = "enc:v1:" - - private fun getOrCreateKey(): SecretKey { - val ks = KeyStore.getInstance(KEYSTORE).apply { load(null) } - (ks.getEntry(ALIAS, null) as? KeyStore.SecretKeyEntry)?.let { return it.secretKey } - val generator = KeyGenerator.getInstance(KeyProperties.KEY_ALGORITHM_AES, KEYSTORE) - generator.init( - KeyGenParameterSpec.Builder( - ALIAS, - KeyProperties.PURPOSE_ENCRYPT or KeyProperties.PURPOSE_DECRYPT - ) - .setBlockModes(KeyProperties.BLOCK_MODE_GCM) - .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) - .build() - ) - return generator.generateKey() - } - - private fun deleteKey() { - try { - KeyStore.getInstance(KEYSTORE).apply { load(null) }.deleteEntry(ALIAS) - } catch (e: Exception) { - Log.w(TAG, "Failed to delete Keystore alias $ALIAS", e) - } - } - - private fun encryptWith(key: SecretKey, plain: String): String { - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.ENCRYPT_MODE, key) - val iv = cipher.iv - val ciphertext = cipher.doFinal(plain.toByteArray(Charsets.UTF_8)) - val combined = ByteArray(iv.size + ciphertext.size) - System.arraycopy(iv, 0, combined, 0, iv.size) - System.arraycopy(ciphertext, 0, combined, iv.size, ciphertext.size) - return ENC_PREFIX + Base64.encodeToString(combined, Base64.NO_WRAP) - } - - /** - * Encrypt [plain] into a self-describing string: [ENC_PREFIX] + base64(iv | ciphertext). - * - * The key is not auth-bound, so a credential change does not invalidate it; an alias an - * OEM Keystore drops anyway is regenerated once before retrying. - * - * @param plain the value to encrypt - * @throws GeneralSecurityException on any other Keystore/cipher failure, so the caller can - * inform the user instead of crashing the IDE on Save - */ - @Throws(GeneralSecurityException::class) - fun encrypt(plain: String): String { - return try { - encryptWith(getOrCreateKey(), plain) - } catch (e: KeyPermanentlyInvalidatedException) { - Log.w(TAG, "Keystore key invalidated; regenerating and retrying encrypt", e) - deleteKey() - encryptWith(getOrCreateKey(), plain) - } - } - - /** - * Return the plaintext for a stored value, handling both formats transparently: - * an [ENC_PREFIX] value is decrypted; anything else is returned unchanged as - * legacy plaintext (use [readAndMigrate] to upgrade it in place). Returns - * null if a ciphertext value can't be decrypted — e.g. the Keystore key was - * lost or invalidated — in which case the user must re-enter the key. - */ - fun decrypt(stored: String?): String? { - if (stored == null) return null - if (!stored.startsWith(ENC_PREFIX)) return stored - return try { - val combined = Base64.decode(stored.removePrefix(ENC_PREFIX), Base64.NO_WRAP) - val iv = combined.copyOfRange(0, IV_LEN) - val ciphertext = combined.copyOfRange(IV_LEN, combined.size) - val cipher = Cipher.getInstance(TRANSFORM) - cipher.init(Cipher.DECRYPT_MODE, getOrCreateKey(), GCMParameterSpec(TAG_BITS, iv)) - String(cipher.doFinal(ciphertext), Charsets.UTF_8) - } catch (e: Exception) { - Log.w(TAG, "Failed to decrypt stored API key", e) - null - } - } - - /** - * Read [key] from [prefs], upgrading a legacy plaintext value to ciphertext in place. - * - * The value is trimmed on migration, so the stored, displayed and sent forms all agree. - * - * Keystore IPC + AES/GCM, so call this off the main thread. - * - * @return the trimmed plaintext value, or null when nothing is stored or decryption failed. - */ - fun readAndMigrate(prefs: SharedPreferences?, key: String): String? { - val stored = prefs?.getString(key, null) ?: return null - if (stored.startsWith(ENC_PREFIX)) return decrypt(stored) - val plain = stored.trim() - if (plain.isEmpty()) return plain - try { - prefs.edit().putString(key, encrypt(plain)).apply() - Log.i(TAG, "Upgraded legacy plaintext value for '$key' to ciphertext") - } catch (e: Exception) { - Log.w(TAG, "Could not upgrade legacy plaintext value for '$key' to ciphertext", e) - } - return plain - } -} +val secureApiKeyStore = KeystoreSecretStore(ALIAS) diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt index fab7f2f6..0fba45d2 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsFragment.kt @@ -33,6 +33,7 @@ import com.itsaky.androidide.plugins.PluginContext import com.itsaky.androidide.plugins.aiagentopenai.R import com.itsaky.androidide.plugins.aiagentopenai.plugin.OpenAiPlugin import com.itsaky.androidide.plugins.base.PluginFragmentHelper +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import com.itsaky.androidide.plugins.services.IdeTooltipService import kotlinx.coroutines.launch import java.text.SimpleDateFormat @@ -347,20 +348,30 @@ class OpenAiSettingsFragment : Fragment() { } viewLifecycleOwner.lifecycleScope.launch { - val savedApiKey = viewModel.getApiKey() + val stored = viewModel.getApiKey() + val savedApiKey = (stored as? KeystoreSecretStore.Stored.Value)?.plain val hasKey = !savedApiKey.isNullOrBlank() updateUiState(isEditing = !hasKey) if (hasKey) { statusTextView.text = savedApiKeyStatusText() } else { apiKeyInput.setText("") - // A stored-but-undecryptable key also reads as null; warn as the Edit path does. - if (viewModel.hasStoredApiKey()) { + // Only for a key that is there and will not decrypt; an empty box alone looks like + // data loss. Nothing stored at all is the ordinary first run and says nothing. + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() + } else if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from the above: the key is still there and intact, so this + // must not send the user off to find and type it again. + Toast.makeText( + requireContext(), + getString(R.string.msg_api_key_unavailable), + Toast.LENGTH_LONG + ).show() } } } @@ -566,20 +577,30 @@ class OpenAiSettingsFragment : Fragment() { editButton.setOnClickListener { editButton.isEnabled = false viewLifecycleOwner.lifecycleScope.launch { - val apiKey = try { + val stored = try { viewModel.getApiKey() } finally { editButton.isEnabled = true } - // null = a key IS stored but won't decrypt; an empty box alone looks like data loss. - if (apiKey == null) { + // A key that is stored and will not decrypt; an empty box alone looks like data + // loss. Told apart from "nothing stored" here, which this button rarely sees but + // must not report as a lost Keystore entry when it does. + if (stored is KeystoreSecretStore.Stored.Unreadable) { Toast.makeText( requireContext(), getString(R.string.msg_api_key_unreadable), Toast.LENGTH_LONG ).show() + } else if (stored is KeystoreSecretStore.Stored.Unavailable) { + // Said differently from the above: the key is still there and intact, so this + // must not send the user off to find and type it again. + Toast.makeText( + requireContext(), + getString(R.string.msg_api_key_unavailable), + Toast.LENGTH_LONG + ).show() } - revealEditMode(apiKey.orEmpty()) + revealEditMode((stored as? KeystoreSecretStore.Stored.Value)?.plain.orEmpty()) } } diff --git a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt index 22a810f8..85e4596b 100644 --- a/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt +++ b/ai-agent-openai/src/main/kotlin/com/itsaky/androidide/plugins/aiagentopenai/settings/OpenAiSettingsViewModel.kt @@ -10,7 +10,8 @@ import com.itsaky.androidide.plugins.PluginLogger import com.itsaky.androidide.plugins.aiagentopenai.backend.OpenAiBackend import com.itsaky.androidide.plugins.aiagentopenai.logging.LOG_PREFIX import com.itsaky.androidide.plugins.aiagentopenai.preferences.OpenAiPreferences -import com.itsaky.androidide.plugins.aiagentopenai.security.SecureApiKeyStore +import com.itsaky.androidide.plugins.aiagentopenai.security.secureApiKeyStore +import com.itsaky.androidide.plugins.security.KeystoreSecretStore import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.Dispatchers @@ -244,7 +245,7 @@ class OpenAiSettingsViewModel( } /** - * Encrypts [apiKey] via [SecureApiKeyStore] and persists only the ciphertext, off the main + * Encrypts [apiKey] via [secureApiKeyStore] and persists only the ciphertext, off the main * thread. Nothing is written on failure. * * @param apiKey the plaintext key to store (trimmed before encryption) @@ -261,7 +262,7 @@ class OpenAiSettingsViewModel( return@withContext false } val encrypted = try { - SecureApiKeyStore.encrypt(apiKey.trim()) + secureApiKeyStore.encrypt(apiKey.trim()) } catch (e: Exception) { logger?.error("$TAG: failed to encrypt API key", e) return@withContext false @@ -288,9 +289,13 @@ class OpenAiSettingsViewModel( /** * Decrypt the stored key off the main thread (Keystore IPC + AES/GCM), upgrading a plaintext * value to ciphertext in passing. + * + * @return what is on disk: nothing, the key, a key this device's Keystore can no longer open, + * or one it would not open just now. Those are not the same — a lost Keystore entry has to be + * entered again, a keystore that did not answer only retried — so the caller says which. */ - suspend fun getApiKey(): String? = withContext(ioDispatcher) { - SecureApiKeyStore.readAndMigrate(prefs(), OpenAiPreferences.KEY_API_KEY) + suspend fun getApiKey(): KeystoreSecretStore.Stored = withContext(ioDispatcher) { + secureApiKeyStore.readAndMigrate(prefs(), OpenAiPreferences.KEY_API_KEY) } /** @@ -300,18 +305,20 @@ class OpenAiSettingsViewModel( * entered for OpenAI. A key stored before the origin was recorded is returned, matching the * backend's own rule. * - * @return the plaintext key, or null when none is stored or it belongs to another server + * @return the plaintext key, or null when none is stored, it cannot be decrypted here, or it + * belongs to another server. The connection test has the same answer — send no key — for + * every one of them, and the pane has already said so on the read that opened it. */ suspend fun getApiKeyFor(baseUrl: String): String? { val savedFor = prefs()?.getString(OpenAiPreferences.KEY_API_KEY_URL, null) if (savedFor != null && !BaseUrlPolicy.sameOrigin(savedFor, baseUrl)) return null - return getApiKey() + return (getApiKey() as? KeystoreSecretStore.Stored.Value)?.plain } /** - * True when a key is present on disk, whether or not it can still be decrypted. Lets the UI - * tell "nothing was saved" from "the Keystore entry is gone" — [getApiKey] is null for both. - * Raw pref only, so no Keystore IPC and safe on the main thread. + * True when a key is present on disk, whether or not it can still be decrypted: what the key + * block is dressed from, which must not collapse the moment a Keystore entry is lost. Raw pref + * only, so no Keystore IPC and safe on the main thread — which [getApiKey] is not. */ fun hasStoredApiKey(): Boolean = !prefs()?.getString(OpenAiPreferences.KEY_API_KEY, null).isNullOrBlank() diff --git a/ai-agent-openai/src/main/res/values/strings.xml b/ai-agent-openai/src/main/res/values/strings.xml index f13a8291..8d954ac4 100644 --- a/ai-agent-openai/src/main/res/values/strings.xml +++ b/ai-agent-openai/src/main/res/values/strings.xml @@ -69,6 +69,7 @@ API Key saved on: %s API Key saved and verified on: %s The stored API key could not be read on this device. Please enter it again. + The device keystore could not be reached, so the stored API key could not be read. It is still saved — please try again in a moment. Couldn\'t save the API key on this device. Please try again. Checking this key with the server… Verified, your API key works diff --git a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt index eb63fa7e..59d6bcf3 100644 --- a/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt +++ b/ai-core/src/main/kotlin/com/itsaky/androidide/plugins/aicore/viewmodel/ChatViewModel.kt @@ -25,6 +25,7 @@ import com.itsaky.androidide.plugins.aicore.tool.ToolCall import com.itsaky.androidide.plugins.aicore.tool.ToolCallExtractor import com.itsaky.androidide.plugins.aicore.tool.ToolExecutionTracker import com.itsaky.androidide.plugins.aicore.tool.ToolHandler +import com.itsaky.androidide.plugins.aicore.tool.isTerminalToolName import com.itsaky.androidide.plugins.aicore.tool.sources.ToolSourceStore import com.itsaky.androidide.plugins.aicore.tool.handlers.AddDependencyHandler import com.itsaky.androidide.plugins.aicore.tool.handlers.CreateFileHandler @@ -1004,7 +1005,10 @@ class ChatViewModel( // Per-run flag (set by executeToolCalls), not a session-wide scan. val lastToolFailed = lastToolFailedThisRun - val realCalls = toolCalls.filterNot { it.name == RESPOND_TOOL } + // Matched loosely, as everywhere else: a backend that answers `Respond` would + // otherwise leave the terminal call in `realCalls`, so an identical repeat + // never matches and the duplicate-turn bubble is never dropped. + val realCalls = toolCalls.filterNot { isTerminalToolName(it.name, RESPOND_TOOL) } if (realCalls.isNotEmpty() && realCalls == lastSucceededCalls) { viewModelScope.launch(Dispatchers.Main) { _messages.value = _messages.value.filter { it.id != agentMessageId }