From 8eb3e6f7bc255818374dbb0ad3415af4166c66a0 Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 7 Sep 2026 07:55:53 -0600 Subject: [PATCH 01/11] Keep the user's API keys in one per-user file, not in per-channel settings (BL-16820) A key the user entered in one Bloom was invisible in the next. Both the Pixabay key and the OpenRouter key lived in .NET user settings, whose file is %LocalAppData%\SIL\\\user.config. The product name carries the release channel (Bloom, BloomAlpha, BloomBeta) and the folder carries the build version, so Bloom Beta, Bloom Alpha and a dev build each read a different file, and an upgrade could lose the key too. Settings.Upgrade() cannot bridge them, because it only copies a value forward within one channel. This adds UserKeyStore, the one place Bloom keeps a key that belongs to the user rather than to a book, a collection, or a copy of Bloom. It writes %LocalAppData%\SIL\Bloom\UserKeys.json, a path with no channel and no version in it, so every Bloom the user runs reads the same keys. Each value is encrypted with the Windows user login (DPAPI, CurrentUser scope), so a copy of the file in a backup, a cloud sync, or a support log is useless to anyone else. The keys do not travel to another computer or another Windows account; there Get reports the key as absent and the feature asks for it again. Each key records how it is encrypted, and the file carries a plain-English "about" note saying what that means, so a later Bloom can read the field, convert the keys it wants to convert, and leave alone anything a newer Bloom wrote. Keeping a key with the user's Bloom Library account, which the card asks for later, is such a change. The store knows nothing about any service: a caller picks a name and owns its meaning, so a new service needs no change to the store. The image gallery keeps one key per provider (imageGallery.pixabay) through a new imageGallery/providerKeys endpoint, so Bloom needs no change when the gallery gains a provider. "Edit with AI" uses the name openRouter. The two old settings and the OpenRouter-only store they used are removed. There is no migration of stored values: both features are new in Bloom 6.5. Co-Authored-By: Claude Opus 5 (1M context) --- .../image-gallery/ImageGalleryDialog.tsx | 28 +- src/BloomExe/Properties/Settings.Designer.cs | 26 -- src/BloomExe/Properties/Settings.settings | 6 - src/BloomExe/Utils/UserKeyStore.cs | 329 ++++++++++++++++++ .../web/controllers/AiImageEditorApi.cs | 7 +- .../web/controllers/ImageGalleryApi.cs | 61 ++++ .../controllers/OpenRouterCredentialStore.cs | 103 ------ src/BloomTests/Utils/UserKeyStoreTests.cs | 287 +++++++++++++++ .../OpenRouterCredentialStoreTests.cs | 93 ----- 9 files changed, 694 insertions(+), 246 deletions(-) create mode 100644 src/BloomExe/Utils/UserKeyStore.cs delete mode 100644 src/BloomExe/web/controllers/OpenRouterCredentialStore.cs create mode 100644 src/BloomTests/Utils/UserKeyStoreTests.cs delete mode 100644 src/BloomTests/web/controllers/OpenRouterCredentialStoreTests.cs diff --git a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx index 8031d8805591..c5dd19c23bc9 100644 --- a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx +++ b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx @@ -14,6 +14,7 @@ import { getBloomApiPrefix, getAsync, postJsonAsync, + postString, postDataWithConfigAsync, trackEvent, } from "../../utils/bloomApi"; @@ -38,7 +39,7 @@ const ImageGalleryDialog: React.FunctionComponent<{ searchLang: string; }> = (props) => { const [open, setOpen] = useState(true); - // Keys are loaded from durable Bloom settings before the gallery is rendered, + // Keys are loaded from the per-user key store before the gallery is rendered, // so providers (e.g. Pixabay) receive their initial API key in their constructor. const [providerKeys, setProviderKeys] = useState< IProviderKeysV1 | undefined @@ -65,17 +66,14 @@ const ImageGalleryDialog: React.FunctionComponent<{ // so the component can render before the network round-trip completes. // There are no dependencies to react to; [] is correct. useEffect(() => { - getAsync("app/userSetting?settingName=ImageGalleryProviderKeys") + getAsync("imageGallery/providerKeys") .then((r) => { - const json = r?.data?.settingValue as string; - if (json) { - try { - const keys = JSON.parse(json) as IProviderKeysV1; - setProviderKeys(keys); - pixabayKeyPresentRef.current = !!keys.pixabay; - } catch { - // ignore malformed stored value - } + const keys = r?.data as IProviderKeysV1; + // Bloom replies with the format version plus one property per provider the + // user has a key for, so anything past the version means there is a key. + if (keys && Object.keys(keys).length > 1) { + setProviderKeys(keys); + pixabayKeyPresentRef.current = !!keys.pixabay; } }) .finally(() => setKeysLoaded(true)); @@ -259,10 +257,10 @@ const ImageGalleryDialog: React.FunctionComponent<{ // key supplied while the chooser is open is reflected in what this // visit reports. pixabayKeyPresentRef.current = !!keys.pixabay; - postJsonAsync("app/userSetting", { - settingName: "ImageGalleryProviderKeys", - settingValue: JSON.stringify(keys), - }); + postString( + "imageGallery/providerKeys", + JSON.stringify(keys), + ); }} onLanguageChange={(lang) => postJsonAsync("app/userSetting", { diff --git a/src/BloomExe/Properties/Settings.Designer.cs b/src/BloomExe/Properties/Settings.Designer.cs index 0886cba900d6..2086590c3d7c 100644 --- a/src/BloomExe/Properties/Settings.Designer.cs +++ b/src/BloomExe/Properties/Settings.Designer.cs @@ -126,19 +126,6 @@ public string ImageHandler { } } - [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] - [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] - [global::System.Configuration.DefaultSettingValueAttribute("")] - public string ImageGalleryProviderKeys { - get { - return ((string)(this["ImageGalleryProviderKeys"])); - } - set { - this["ImageGalleryProviderKeys"] = value; - } - } - [global::System.Configuration.UserScopedSettingAttribute()] [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] @@ -513,19 +500,6 @@ public string ExportImportFileFolder { } } - [global::System.Configuration.UserScopedSettingAttribute()] - [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] - [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] - [global::System.Configuration.DefaultSettingValueAttribute("")] - public string OpenRouterApiKey { - get { - return ((string)(this["OpenRouterApiKey"])); - } - set { - this["OpenRouterApiKey"] = value; - } - } - [global::System.Configuration.UserScopedSettingAttribute()] [global::System.Configuration.SettingsProviderAttribute(typeof(SIL.Settings.CrossPlatformSettingsProvider))] [global::System.Diagnostics.DebuggerNonUserCodeAttribute()] diff --git a/src/BloomExe/Properties/Settings.settings b/src/BloomExe/Properties/Settings.settings index a66fc53c6621..9d3ca1c23eff 100644 --- a/src/BloomExe/Properties/Settings.settings +++ b/src/BloomExe/Properties/Settings.settings @@ -26,9 +26,6 @@ http - - - False @@ -122,9 +119,6 @@ - - - diff --git a/src/BloomExe/Utils/UserKeyStore.cs b/src/BloomExe/Utils/UserKeyStore.cs new file mode 100644 index 000000000000..b9e353a8f49b --- /dev/null +++ b/src/BloomExe/Utils/UserKeyStore.cs @@ -0,0 +1,329 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Security.Cryptography; +using System.Text; +using Newtonsoft.Json; +using SIL.IO; +using SIL.Reporting; + +namespace Bloom.Utils +{ + /// + /// The one place Bloom keeps a key that belongs to the user rather than to a book, a + /// collection, or a copy of Bloom: an API key the user fetched from a service's own site + /// (OpenRouter, Pixabay, ElevenLabs, a translation service, and whatever comes next). + /// + /// Two rules shape it. + /// + /// It is per Windows user, and independent of the release channel and the build version. + /// That rules out , whose file is + /// %LocalAppData%\SIL\<product>\<version>\user.config: the product name carries + /// the channel (Bloom, BloomAlpha, BloomBeta) and the folder carries the version, so a key + /// entered in one channel is invisible in the next, and Settings.Upgrade() cannot help + /// because it only copies a value forward within one channel. This store lives in + /// , which is %LocalAppData%\SIL\Bloom + /// whatever the channel or version, so every Bloom the user runs reads the same file. + /// + /// It is encrypted with the Windows user login. Each value is protected with DPAPI in + /// CurrentUser scope, so the file opens only for that Windows account on that computer. + /// What that buys: a file that is copied, backed up, synced to the cloud, or picked up in a + /// support log is useless to anyone else. What it does not buy: protection from a program + /// running as that same Windows user, which can call Unprotect exactly as we do. What it + /// costs: the keys do not travel to a new computer. Get cannot decrypt a value written + /// by another account or another machine, and reports it as absent, so the user is asked + /// for the key again. A caller that has anything better to say than silence should say it. + /// + /// Nothing here knows what any key is for. A caller picks a name and owns its meaning, + /// so a new service needs no change to this class. + /// + public static class UserKeyStore + { + private const string kFileName = "UserKeys.json"; + private const int kCurrentFormatVersion = 1; + + /// + /// What a key's "protection" field says when Windows DPAPI encrypted its value in + /// CurrentUser scope. The name states the scope as well as the method, because DPAPI + /// also has a LocalMachine scope that decrypts for any account on the computer, and a + /// reader must be able to tell which one it is holding. + /// + /// Every method Bloom ever uses gets its own name here, and the name is recorded on + /// each key rather than once for the file. That is what makes a later change of method + /// a migration rather than a loss: a future Bloom reads the field, keeps reading the + /// keys it recognizes, converts the ones it wants to move, and leaves alone anything + /// written by a version newer than itself. reports + /// the field without decrypting, so such a pass can see what it is dealing with. + /// + private const string kDpapiCurrentUserProtection = "windows-dpapi-currentuser"; + + /// + /// The name under which the user's OpenRouter API key is stored (the "Edit with AI" + /// feature). + /// + public const string kOpenRouterName = "openRouter"; + + /// + /// The start of the name of every image gallery provider key, for example + /// "imageGallery.pixabay". The rest of the name is the gallery's own provider id, so + /// Bloom needs no change when the gallery gains a provider. + /// + public const string kImageGalleryNamePrefix = "imageGallery."; + + /// + /// Serializes this process's read-modify-write cycles. Two instances of Bloom are not + /// serialized against each other, so a key written by one while the other is + /// writing a different one can be lost. That is acceptable here: a user enters a key + /// once, by hand, in one window. + /// + private static readonly object s_lock = new object(); + + /// + /// Set by a test so that it works on a folder of its own. A test must never run + /// against the real file: it holds the developer's own keys, and a test that wrote + /// there would destroy them. + /// + internal static string FolderForTests; + + /// The folder holding the file. See . + private static string Folder => FolderForTests ?? ProjectContext.GetBloomAppDataFolder(); + + /// The one file, shared by every channel and version. + public static string FilePath => Path.Combine(Folder, kFileName); + + /// + /// Returns the secret stored under this name, or null if there is none, or if the + /// stored value cannot be decrypted for this Windows account on this computer. + /// + public static string Get(string name) + { + lock (s_lock) + { + var store = Load(); + if (!store.Keys.TryGetValue(name, out var storedKey)) + return null; + if (string.IsNullOrEmpty(storedKey?.Value)) + return null; + if (storedKey.Protection != kDpapiCurrentUserProtection) + { + // Most likely a file written by a newer Bloom that protects keys some + // other way. Guessing at the bytes would be worse than asking the user + // again, and this version must not overwrite what it cannot read. + Logger.WriteEvent( + $"UserKeyStore: the key '{name}' says it is protected by '{storedKey.Protection}', which this version of Bloom does not know how to read. Treating it as absent." + ); + return null; + } + return Unprotect(storedKey.Value); + } + } + + /// + /// Reports how the key of this name is protected, without decrypting it, or null when + /// there is no such key. This is the seam a later change of method needs: it can list + /// the names, ask each how it is protected, and convert only what it means to convert. + /// + public static string GetProtectionMethod(string name) + { + lock (s_lock) + { + return Load().Keys.TryGetValue(name, out var storedKey) + ? storedKey?.Protection + : null; + } + } + + /// + /// Stores a secret under this name, replacing any previous one. A null or empty secret + /// removes the key, which is how a caller handles the user clearing one. + /// + public static void Set(string name, string secret) + { + lock (s_lock) + { + var store = Load(); + if (string.IsNullOrEmpty(secret)) + { + if (!store.Keys.Remove(name)) + return; // nothing there, so nothing to write + } + else + { + var protectedSecret = Protect(secret); + if (protectedSecret == null) + return; // Protect already reported why; better to forget the key than to store it in the clear + store.Keys[name] = new StoredKey + { + Value = protectedSecret, + Protection = kDpapiCurrentUserProtection, + }; + } + Save(store); + } + } + + /// + /// The names of the keys on file, optionally only those starting with a prefix. + /// A caller that keeps a family of keys (one per provider, say) uses this to + /// find them all without knowing in advance which providers the user has keys for. + /// The names are returned whether or not their values can still be decrypted. + /// + public static IEnumerable GetNames(string namePrefix = null) + { + lock (s_lock) + { + return Load() + .Keys.Keys.Where(name => + string.IsNullOrEmpty(namePrefix) + || name.StartsWith(namePrefix, StringComparison.Ordinal) + ) + .OrderBy(name => name, StringComparer.Ordinal) + .ToList(); + } + } + + /// + /// Encrypts a string with the Windows user login (DPAPI, CurrentUser scope) and returns + /// it as base64, or null if this platform or account cannot do that. Public so that a + /// test can prove the round trip. + /// + public static string Protect(string plaintext) + { + try + { + var encrypted = ProtectedData.Protect( + Encoding.UTF8.GetBytes(plaintext), + null, + DataProtectionScope.CurrentUser + ); + return Convert.ToBase64String(encrypted); + } + catch (Exception error) + { + // Bloom targets net8.0-windows, so this is not expected. It becomes real on the + // day Bloom runs somewhere without DPAPI, and storing the secret in the clear + // instead would be a nasty surprise to a user who was told it was encrypted. + Logger.WriteError( + "UserKeyStore could not encrypt a key, so it was not saved", + error + ); + return null; + } + } + + /// + /// Reverses . Returns null when the value cannot be decrypted for + /// this Windows account on this computer, which is the expected outcome for a file + /// brought from another computer, another account, or a reinstalled Windows. Public so + /// that a test can prove the round trip. + /// + public static string Unprotect(string protectedBase64) + { + try + { + var bytes = ProtectedData.Unprotect( + Convert.FromBase64String(protectedBase64), + null, + DataProtectionScope.CurrentUser + ); + return Encoding.UTF8.GetString(bytes); + } + catch (Exception error) + when (error is CryptographicException || error is FormatException) + { + Logger.WriteEvent( + $"UserKeyStore: a stored key could not be decrypted on this computer and account ({error.Message}). The user must enter it again." + ); + return null; + } + } + + /// One key as it sits in the file. + private class StoredKey + { + [JsonProperty("value")] + public string Value; + + [JsonProperty("protection")] + public string Protection; + } + + /// The whole file. + private class StoreFile + { + [JsonProperty("version")] + public int Version = kCurrentFormatVersion; + + /// + /// Written on every save and ignored on read: it is there so that whoever opens + /// this file, a person or a later program, can see how the values were encrypted + /// without having to find the Bloom source that wrote them. + /// + [JsonProperty("about")] + public string About; + + [JsonProperty("keys")] + public Dictionary Keys = new Dictionary(); + } + + /// + /// The text of the file's "about" property. It names the protection method Bloom + /// writes today and says that the authority is each key's own "protection" field, so a + /// file holding keys written by two different versions cannot be misread. + /// + private static string AboutText => + "Each key's \"protection\" field says how that key's value is encrypted; " + + $"\"{kDpapiCurrentUserProtection}\" means Windows DPAPI in CurrentUser scope, " + + "which only the Windows account that wrote it, on the computer that wrote it, " + + "can decrypt. Keys do not move to another computer or another account."; + + /// + /// Reads the file, or reports an empty store when there is none yet. Damaged content is + /// reported and treated as empty rather than thrown, because losing a saved key is a + /// smaller harm than a feature that cannot open. Callers hold s_lock. + /// + private static StoreFile Load() + { + if (!RobustFile.Exists(FilePath)) + return new StoreFile(); + try + { + var store = JsonConvert.DeserializeObject( + RobustFile.ReadAllText(FilePath) + ); + if (store?.Keys == null) + return new StoreFile(); + return store; + } + catch (Exception error) + { + Logger.WriteError( + "UserKeyStore could not read " + FilePath + "; treating it as empty", + error + ); + return new StoreFile(); + } + } + + /// Callers hold s_lock. + private static void Save(StoreFile store) + { + // A failure here costs the user only the memory of a key they can enter again, so + // report it and carry on rather than stopping whatever they were doing. + try + { + store.Version = kCurrentFormatVersion; + store.About = AboutText; + RobustFile.WriteAllText( + FilePath, + JsonConvert.SerializeObject(store, Formatting.Indented) + ); + } + catch (Exception error) + { + Logger.WriteError("UserKeyStore could not write " + FilePath, error); + } + } + } +} diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index bb5866435df6..dcf0ba2d3ea8 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -10,6 +10,7 @@ using Bloom.Edit; using Bloom.ImageProcessing; using Bloom.SafeXml; +using Bloom.Utils; using L10NSharp; using Newtonsoft.Json; using SIL.Core.ClearShare; @@ -497,7 +498,7 @@ private void HandleLaunch(ApiRequest request) // Bloom owns the OpenRouter key: supply the per-user stored key so the AI // image editor doesn't have to ask for it again. It hands any newly // obtained key back via aiImageEditor/saveCredentials. - apiKey = OpenRouterCredentialStore.GetApiKey(), + apiKey = UserKeyStore.Get(UserKeyStore.kOpenRouterName), // In a Playground template book all features are unlocked for // "try it out", so the AI image editor opens — but it's a shared demo // context, so it must not let the user set/save an OpenRouter API key. @@ -564,7 +565,7 @@ private class SaveCredentialsRequest /// /// Receives the user's OpenRouter API key from the AI image editor (manual key entry) - /// and persists it per-user via . A null/empty + /// and persists it per Windows user via . A null/empty /// apiKey clears the stored key (sign-out). Session-gated so a stray frame can't /// overwrite the user's stored key. /// @@ -593,7 +594,7 @@ private void HandleSaveCredentials(ApiRequest request) return; } - OpenRouterCredentialStore.Save(payload.apiKey); + UserKeyStore.Set(UserKeyStore.kOpenRouterName, payload.apiKey); request.PostSucceeded(); } diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index c22c7ef0434c..973f9a96afd8 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -14,6 +14,8 @@ using Bloom.ImageProcessing; using Bloom.MiscUI; using Bloom.Utils; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; using SIL.Core.ClearShare; using SIL.IO; using SIL.Reporting; @@ -96,6 +98,11 @@ public class ImageGalleryApi : IDisposable "ImageCollections" ); + /// + /// The start of the name of every key stored for a gallery provider. + /// + private const string kGalleryKeyPrefix = UserKeyStore.kImageGalleryNamePrefix; + public void RegisterWithApiHandler(BloomApiHandler apiHandler) { apiHandler.RegisterAsyncEndpointHandler( @@ -136,6 +143,60 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) HandleLocalCollectionImage, false ); + apiHandler.RegisterEndpointHandler( + "imageGallery/providerKeys", + HandleProviderKeys, + false + ); + } + + /// + /// Gets or sets the API keys the user has for the gallery's search providers, such as + /// the key they fetched from Pixabay's site. The gallery's shape for these is one JSON + /// object of provider id to key, plus a "version" property; Bloom keeps each key as + /// its own entry, named with the gallery's provider id, so neither side needs a + /// change when the gallery gains a provider. + /// + /// These are per Windows user, not per collection and not per copy of Bloom. See + /// , which explains why they cannot be Bloom settings. + /// + private void HandleProviderKeys(ApiRequest request) + { + if (request.HttpMethod == HttpMethods.Get) + { + // One flat object, the shape the gallery expects: the format version plus + // one property per provider that has a key. + var keys = new Dictionary { ["version"] = 1 }; + foreach (var name in UserKeyStore.GetNames(kGalleryKeyPrefix)) + { + // Null when the key cannot be decrypted on this computer, which the + // gallery reads the same way as never having had a key: it asks for one. + var key = UserKeyStore.Get(name); + if (!string.IsNullOrEmpty(key)) + keys[name.Substring(kGalleryKeyPrefix.Length)] = key; + } + request.ReplyWithJson(JsonConvert.SerializeObject(keys)); + } + else + { + var posted = JObject.Parse(request.RequiredPostString()); + var providerIds = new HashSet(); + foreach (var property in posted.Properties()) + { + if (property.Name == "version") + continue; + providerIds.Add(property.Name); + UserKeyStore.Set(kGalleryKeyPrefix + property.Name, (string)property.Value); + } + // The gallery sends every key it has, so a provider missing from the post is a + // key the user removed. + foreach (var name in UserKeyStore.GetNames(kGalleryKeyPrefix)) + { + if (!providerIds.Contains(name.Substring(kGalleryKeyPrefix.Length))) + UserKeyStore.Set(name, null); + } + request.PostSucceeded(); + } } /// diff --git a/src/BloomExe/web/controllers/OpenRouterCredentialStore.cs b/src/BloomExe/web/controllers/OpenRouterCredentialStore.cs deleted file mode 100644 index 7080390ed4f8..000000000000 --- a/src/BloomExe/web/controllers/OpenRouterCredentialStore.cs +++ /dev/null @@ -1,103 +0,0 @@ -using System; -using System.Diagnostics; -using System.Security.Cryptography; -using System.Text; - -namespace Bloom.web.controllers -{ - /// - /// Per-user storage for the OpenRouter credentials used by the "Edit with AI…" feature. - /// - /// The key is the user's, tied to their OpenRouter billing account — not to a book or - /// collection — so it is stored per-user in - /// (%LocalAppData%\SIL\Bloom\user.config) rather than travelling with a book or - /// collection that gets shared or uploaded. Bloom is the single source of truth: the - /// editor library no longer persists the key itself; it hands a newly obtained key up to - /// Bloom (via aiImageEditor/saveCredentials) and Bloom supplies it back in the launch - /// payload on each launch. - /// - /// The API key is encrypted at rest with Windows DPAPI (CurrentUser scope) so that a - /// copied or cloud-synced user.config is useless on another machine or account. This is - /// cheap insurance, not a guarantee: it does not defend against malware running as the - /// same Windows user. Bloom currently targets net8.0-windows; when it becomes an Electron - /// app on Mac/Linux this encryption step will need a per-platform equivalent. - /// - public static class OpenRouterCredentialStore - { - /// - /// Saves (or, when is null/empty, clears) the user's - /// OpenRouter API key. The key is DPAPI-encrypted before being written. - /// - public static void Save(string apiKey) - { - if (string.IsNullOrEmpty(apiKey)) - { - Clear(); - return; - } - - var settings = Properties.Settings.Default; - settings.OpenRouterApiKey = Protect(apiKey); - settings.Save(); - } - - /// Clears the stored OpenRouter API key (e.g. on sign-out). - public static void Clear() - { - var settings = Properties.Settings.Default; - settings.OpenRouterApiKey = ""; - settings.Save(); - } - - /// - /// Returns the decrypted OpenRouter API key, or null if none is stored or the stored - /// blob cannot be decrypted on this machine/account (e.g. a config copied from - /// elsewhere). A non-decryptable blob is treated as "no key" — the user simply signs - /// in again — rather than an error, because that is the expected outcome of DPAPI's - /// machine/account binding. - /// - public static string GetApiKey() - { - var stored = Properties.Settings.Default.OpenRouterApiKey; - if (string.IsNullOrEmpty(stored)) - return null; - return Unprotect(stored); - } - - /// - /// Encrypts a string with Windows DPAPI (CurrentUser scope) and returns it as base64. - /// Public for round-trip unit testing. - /// - public static string Protect(string plaintext) - { - var bytes = Encoding.UTF8.GetBytes(plaintext); - var encrypted = ProtectedData.Protect(bytes, null, DataProtectionScope.CurrentUser); - return Convert.ToBase64String(encrypted); - } - - /// - /// Reverses . Returns null if the base64/DPAPI blob can't be - /// decrypted on this machine/account. Public for round-trip unit testing. - /// - public static string Unprotect(string protectedBase64) - { - try - { - var encrypted = Convert.FromBase64String(protectedBase64); - var bytes = ProtectedData.Unprotect( - encrypted, - null, - DataProtectionScope.CurrentUser - ); - return Encoding.UTF8.GetString(bytes); - } - catch (Exception ex) when (ex is CryptographicException || ex is FormatException) - { - Debug.WriteLine( - $"OpenRouterCredentialStore: stored key could not be decrypted ({ex.Message}); treating as absent." - ); - return null; - } - } - } -} diff --git a/src/BloomTests/Utils/UserKeyStoreTests.cs b/src/BloomTests/Utils/UserKeyStoreTests.cs new file mode 100644 index 000000000000..45de7fad9a14 --- /dev/null +++ b/src/BloomTests/Utils/UserKeyStoreTests.cs @@ -0,0 +1,287 @@ +using System; +using System.IO; +using System.Linq; +using Bloom.Utils; +using NUnit.Framework; +using SIL.IO; +using SIL.TestUtilities; + +namespace BloomTests.Utils +{ + /// + /// Tests for . + /// + /// Every test works on a temporary folder, set through + /// . The real file holds the developer's + /// own API keys, so a test that wrote there would destroy them. + /// + /// The behavior that matters most here is what happens to a key that cannot be + /// decrypted, which is what a user gets on a new computer: Get reports it as absent rather + /// than throwing, so the feature asks for the key again. + /// + [TestFixture] + public class UserKeyStoreTests + { + private TemporaryFolder _folder; + + [SetUp] + public void Setup() + { + _folder = new TemporaryFolder("UserKeyStoreTests"); + UserKeyStore.FolderForTests = _folder.Path; + } + + [TearDown] + public void TearDown() + { + UserKeyStore.FolderForTests = null; + _folder.Dispose(); + } + + [Test] + public void Get_NothingStored_ReturnsNull() + { + Assert.That(UserKeyStore.Get("someService"), Is.Null); + } + + [Test] + public void SetThenGet_ReturnsTheSecret() + { + const string secret = "sk-or-v1-EXAMPLE-key_0123456789"; + + UserKeyStore.Set("someService", secret); + + // Sanity: the file exists and does not contain the secret in the clear, so the + // successful read below proves decryption rather than a plain-text round trip. + Assert.That( + RobustFile.Exists(UserKeyStore.FilePath), + Is.True, + "setup: Set should have written the file" + ); + Assert.That( + RobustFile.ReadAllText(UserKeyStore.FilePath), + Does.Not.Contain(secret), + "setup: the secret must not be stored in the clear" + ); + + Assert.That(UserKeyStore.Get("someService"), Is.EqualTo(secret)); + } + + [Test] + public void SetThenGet_UnicodeSecret_RoundTrips() + { + // Keys are ASCII, but the encryption is UTF-8 based, so prove non-ASCII survives. + const string secret = "clé-secrète-日本語-😀"; + + UserKeyStore.Set("someService", secret); + + Assert.That(UserKeyStore.Get("someService"), Is.EqualTo(secret)); + } + + [Test] + public void Set_SecondValue_ReplacesTheFirst() + { + UserKeyStore.Set("someService", "first"); + Assert.That( + UserKeyStore.Get("someService"), + Is.EqualTo("first"), + "setup: the first value should be readable before we replace it" + ); + + UserKeyStore.Set("someService", "second"); + + Assert.That(UserKeyStore.Get("someService"), Is.EqualTo("second")); + } + + [Test] + public void Set_EmptySecret_RemovesTheKey() + { + UserKeyStore.Set("someService", "a key"); + Assert.That( + UserKeyStore.GetNames().ToList(), + Has.Count.EqualTo(1), + "setup: the key should be on file before we clear it" + ); + + UserKeyStore.Set("someService", ""); + + Assert.That(UserKeyStore.Get("someService"), Is.Null); + Assert.That(UserKeyStore.GetNames(), Is.Empty); + } + + [Test] + public void Set_TwoKeys_KeepsBoth() + { + UserKeyStore.Set("serviceOne", "one"); + UserKeyStore.Set("serviceTwo", "two"); + + Assert.That(UserKeyStore.Get("serviceOne"), Is.EqualTo("one")); + Assert.That(UserKeyStore.Get("serviceTwo"), Is.EqualTo("two")); + } + + [Test] + public void GetNames_WithPrefix_ReturnsOnlyTheMatchingOnes() + { + UserKeyStore.Set("imageGallery.pixabay", "one"); + UserKeyStore.Set("imageGallery.somethingElse", "two"); + UserKeyStore.Set("openRouter", "three"); + + var names = UserKeyStore.GetNames("imageGallery.").ToList(); + + Assert.That( + names, + Is.EqualTo(new[] { "imageGallery.pixabay", "imageGallery.somethingElse" }) + ); + } + + [Test] + public void Get_ValueThatCannotBeDecrypted_ReturnsNull() + { + // The stand-in for a file brought from another computer or another Windows account. + // Base64 that decodes but is not a DPAPI blob for this user. + var notADpapiBlob = Convert.ToBase64String(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 }); + WriteRawFile( + $"{{'version':1,'keys':{{'someService':{{'value':'{notADpapiBlob}','protection':'windows-dpapi-currentuser'}}}}}}" + ); + + Assert.That( + UserKeyStore.Get("someService"), + Is.Null, + "a key that cannot be decrypted here must read as absent, not throw" + ); + } + + [Test] + public void Get_ValueThatIsNotEvenBase64_ReturnsNull() + { + WriteRawFile( + "{'version':1,'keys':{'someService':{'value':'not base64 !!!','protection':'windows-dpapi-currentuser'}}}" + ); + + Assert.That(UserKeyStore.Get("someService"), Is.Null); + } + + [Test] + public void Get_UnknownProtectionMethod_ReturnsNull() + { + // What a file written by a future Bloom, or by hand, could look like. Reading the + // value as if we knew how it was protected would be worse than asking again. + WriteRawFile( + "{'version':1,'keys':{'someService':{'value':'anything','protection':'somethingElse'}}}" + ); + + Assert.That(UserKeyStore.Get("someService"), Is.Null); + } + + [Test] + public void Get_DamagedFile_ReturnsNullRatherThanThrowing() + { + WriteRawFile("this is not JSON at all"); + + Assert.That(UserKeyStore.Get("someService"), Is.Null); + } + + [Test] + public void Set_AfterDamagedFile_StillStoresTheSecret() + { + WriteRawFile("this is not JSON at all"); + + UserKeyStore.Set("someService", "a key"); + + Assert.That(UserKeyStore.Get("someService"), Is.EqualTo("a key")); + } + + [Test] + public void Set_TheFileSaysHowTheValueIsEncrypted() + { + // A future Bloom, or a person looking at the file, must be able to tell how each + // value was encrypted without reading the Bloom source that wrote it. That is what + // makes a change of method a migration rather than a loss. + UserKeyStore.Set("someService", "a key"); + + var fileText = RobustFile.ReadAllText(UserKeyStore.FilePath); + + Assert.That( + fileText, + Does.Contain("windows-dpapi-currentuser"), + "each key must name its own protection method" + ); + Assert.That( + fileText, + Does.Contain("about"), + "the file must carry a note explaining what that method means" + ); + } + + [Test] + public void GetProtectionMethod_ReportsTheMethodWithoutDecrypting() + { + UserKeyStore.Set("someService", "a key"); + + Assert.That( + UserKeyStore.GetProtectionMethod("someService"), + Is.EqualTo("windows-dpapi-currentuser") + ); + } + + [Test] + public void GetProtectionMethod_MethodThisVersionCannotRead_StillReportsIt() + { + // What a migration pass needs: Get refuses the value, but the method is still + // legible, so the pass can see what it is dealing with and leave it alone. + WriteRawFile( + "{'version':1,'keys':{'someService':{'value':'anything','protection':'some-future-method'}}}" + ); + + Assert.That( + UserKeyStore.Get("someService"), + Is.Null, + "setup: this version must refuse a method it does not know" + ); + Assert.That( + UserKeyStore.GetProtectionMethod("someService"), + Is.EqualTo("some-future-method") + ); + } + + [Test] + public void GetProtectionMethod_NoSuchKey_ReturnsNull() + { + Assert.That(UserKeyStore.GetProtectionMethod("someService"), Is.Null); + } + + [Test] + public void ProtectThenUnprotect_RoundTripsThePlaintext() + { + const string original = "sk-or-v1-EXAMPLE-key_0123456789"; + + var protectedText = UserKeyStore.Protect(original); + + // Sanity: encryption actually transformed the value, so the round trip below is + // meaningful and is not just echoing the plaintext back. + Assert.That( + protectedText, + Is.Not.EqualTo(original), + "setup: Protect should not return the plaintext unchanged" + ); + Assert.DoesNotThrow( + () => Convert.FromBase64String(protectedText), + "setup: Protect must produce base64, because that is what we store" + ); + + Assert.That(UserKeyStore.Unprotect(protectedText), Is.EqualTo(original)); + } + + /// + /// Writes the file as given, so a test can set up content Bloom itself would not + /// write. Single quotes stand in for double quotes, to keep the test strings readable. + /// + private void WriteRawFile(string contentWithSingleQuotes) + { + RobustFile.WriteAllText( + UserKeyStore.FilePath, + contentWithSingleQuotes.Replace('\'', '"') + ); + } + } +} diff --git a/src/BloomTests/web/controllers/OpenRouterCredentialStoreTests.cs b/src/BloomTests/web/controllers/OpenRouterCredentialStoreTests.cs deleted file mode 100644 index 285e4a44d0ae..000000000000 --- a/src/BloomTests/web/controllers/OpenRouterCredentialStoreTests.cs +++ /dev/null @@ -1,93 +0,0 @@ -using System; -using Bloom.web.controllers; -using NUnit.Framework; - -namespace BloomTests.web.controllers -{ - /// - /// Tests for the DPAPI encrypt/decrypt core of . - /// These exercise the two public helpers directly; the Save/Clear/Get methods are not - /// tested here because they read and write the real per-user Properties.Settings singleton - /// (and thus the machine's user.config), which we don't want a unit test to mutate. - /// - /// The important behavior these lock down is the one E5 in the manual test plan depends on: - /// a stored blob that cannot be decrypted on this machine/account (e.g. a user.config copied - /// from another computer) is treated as "no key" — Unprotect returns null — rather than - /// throwing, so the user is simply asked to sign in again. - /// - [TestFixture] - public class OpenRouterCredentialStoreTests - { - [Test] - public void ProtectThenUnprotect_RoundTripsThePlaintext() - { - const string original = "sk-or-v1-EXAMPLE-key_0123456789"; - - var protectedText = OpenRouterCredentialStore.Protect(original); - - // Sanity: encryption actually transformed the value, so a successful round-trip - // below is meaningful and isn't just echoing the plaintext back. - Assert.That( - protectedText, - Is.Not.EqualTo(original), - "setup: Protect should not return the plaintext unchanged" - ); - - var recovered = OpenRouterCredentialStore.Unprotect(protectedText); - - Assert.That(recovered, Is.EqualTo(original)); - } - - [Test] - public void ProtectThenUnprotect_RoundTripsUnicode() - { - // Keys are ASCII, but the encryption is UTF-8 based, so prove non-ASCII survives too. - const string original = "clé-secrète-日本語-😀"; - - var recovered = OpenRouterCredentialStore.Unprotect( - OpenRouterCredentialStore.Protect(original) - ); - - Assert.That(recovered, Is.EqualTo(original)); - } - - [Test] - public void Protect_ProducesValidBase64() - { - var protectedText = OpenRouterCredentialStore.Protect("anything"); - - // Should be storable as-is in user.config; Convert.FromBase64String must accept it. - Assert.DoesNotThrow(() => Convert.FromBase64String(protectedText)); - } - - [Test] - public void Unprotect_NonBase64Input_ReturnsNull() - { - // A stored value that isn't even base64 (FormatException path). - var result = OpenRouterCredentialStore.Unprotect("this is not base64 !!!"); - - Assert.That( - result, - Is.Null, - "a malformed (non-base64) stored blob must be treated as absent, not throw" - ); - } - - [Test] - public void Unprotect_ValidBase64ButNotADpapiBlob_ReturnsNull() - { - // Base64 that decodes fine but is not a DPAPI blob for this user - // (the CryptographicException path). This is the stand-in for a user.config - // copied from another machine/account. - var notADpapiBlob = Convert.ToBase64String(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 }); - - var result = OpenRouterCredentialStore.Unprotect(notADpapiBlob); - - Assert.That( - result, - Is.Null, - "a blob that can't be decrypted on this machine/account must be treated as absent" - ); - } - } -} From 9438377cf1c312caf1a0bd62469f2ce207edc249 Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 7 Sep 2026 08:06:35 -0600 Subject: [PATCH 02/11] Address the Devin review: keep a key intact and say when a save fails (BL-16820) Four fixes, from the review of the first commit. A key is stored exactly as the gallery sent it. The gallery's post now reads the body without the default unescape, which turned a "+" into a space and decoded a percent escape. A key that contains either character was stored altered, and the service then rejected it. A key this version of Bloom cannot read is no longer deleted. The gallery's post carries every key the user can see, so a key missing from it is one they cleared, and Bloom removed it. A key protected by a method only a newer Bloom understands never reaches the gallery, so its absence said nothing about what the user wanted. UserKeyStore.CanRead answers that question, and the removal pass now asks it. A failed write is no longer silent. UserKeyStore.Save logged the failure and returned, so the endpoint told the user the key was saved when the file had not been written, and they found out at the next launch. It now throws. The gallery dialog fetches its keys through useMountEffect, the helper src/BloomBrowserUI/AGENTS.md asks for in place of a bare useEffect with an empty dependency array. Co-Authored-By: Claude Opus 5 (1M context) --- .../image-gallery/ImageGalleryDialog.tsx | 10 ++--- src/BloomExe/Utils/UserKeyStore.cs | 39 +++++++++++-------- .../web/controllers/ImageGalleryApi.cs | 18 +++++++-- src/BloomTests/Utils/UserKeyStoreTests.cs | 32 +++++++++++++++ 4 files changed, 74 insertions(+), 25 deletions(-) diff --git a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx index c5dd19c23bc9..dd55272c1815 100644 --- a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx +++ b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx @@ -5,7 +5,7 @@ import type { IProviderKeysV1, ISearchReport, } from "bloom-image-gallery"; -import React, { useEffect, useRef, useState } from "react"; +import React, { useRef, useState } from "react"; import { BloomDialog, DialogTitle, @@ -18,6 +18,7 @@ import { postDataWithConfigAsync, trackEvent, } from "../../utils/bloomApi"; +import { useMountEffect } from "../../utils/useMountEffect"; import { kBloomBlue } from "../../bloomMaterialUITheme"; import BloomMessageBoxSupport from "../../utils/bloomMessageBoxSupport"; import { getEditablePageBundleExports } from "../../bookEdit/js/workspaceFrames"; @@ -62,10 +63,9 @@ const ImageGalleryDialog: React.FunctionComponent<{ // Exactly one "Image Chooser Closed" event per dialog session. const closeReportedRef = useRef(false); - // useEffect justified: this is a one-time async fetch that must run after mount + // A mount effect is justified: this is a one-time async fetch that must run after mount // so the component can render before the network round-trip completes. - // There are no dependencies to react to; [] is correct. - useEffect(() => { + useMountEffect(() => { getAsync("imageGallery/providerKeys") .then((r) => { const keys = r?.data as IProviderKeysV1; @@ -77,7 +77,7 @@ const ImageGalleryDialog: React.FunctionComponent<{ } }) .finally(() => setKeysLoaded(true)); - }, []); + }); // Searches are counted, not reported one by one: how many a visit took and which sources it // tried are what the close event needs, and a per-query event adds nothing on top of them. diff --git a/src/BloomExe/Utils/UserKeyStore.cs b/src/BloomExe/Utils/UserKeyStore.cs index b9e353a8f49b..a3912ce40283 100644 --- a/src/BloomExe/Utils/UserKeyStore.cs +++ b/src/BloomExe/Utils/UserKeyStore.cs @@ -134,6 +134,18 @@ public static string GetProtectionMethod(string name) } } + /// + /// True when this version of Bloom knows how to read the key of this name, and there is + /// such a key. A caller that removes the keys the user no longer wants asks this first: + /// a key protected by a method only a newer Bloom understands is invisible to + /// , so its absence from what the user is looking at means nothing, and + /// removing it would throw away what the newer Bloom stored. + /// + public static bool CanRead(string name) + { + return GetProtectionMethod(name) == kDpapiCurrentUserProtection; + } + /// /// Stores a secret under this name, replacing any previous one. A null or empty secret /// removes the key, which is how a caller handles the user clearing one. @@ -306,24 +318,19 @@ private static StoreFile Load() } } - /// Callers hold s_lock. + /// + /// Writes the file. A failure throws, on purpose: the caller has just told the user + /// their key is saved, so swallowing the error would leave them to discover next time + /// that it never was. Callers hold s_lock. + /// private static void Save(StoreFile store) { - // A failure here costs the user only the memory of a key they can enter again, so - // report it and carry on rather than stopping whatever they were doing. - try - { - store.Version = kCurrentFormatVersion; - store.About = AboutText; - RobustFile.WriteAllText( - FilePath, - JsonConvert.SerializeObject(store, Formatting.Indented) - ); - } - catch (Exception error) - { - Logger.WriteError("UserKeyStore could not write " + FilePath, error); - } + store.Version = kCurrentFormatVersion; + store.About = AboutText; + RobustFile.WriteAllText( + FilePath, + JsonConvert.SerializeObject(store, Formatting.Indented) + ); } } } diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index 973f9a96afd8..472e3de19366 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -179,7 +179,11 @@ private void HandleProviderKeys(ApiRequest request) } else { - var posted = JObject.Parse(request.RequiredPostString()); + // A key can hold any character a service cares to use, "+" and "%" among + // them, so read the body exactly as the gallery sent it. The default + // unescape would turn a "+" into a space and decode a percent escape, and + // Bloom would store a key the service then rejects. + var posted = JObject.Parse(request.RequiredPostString(unescape: false)); var providerIds = new HashSet(); foreach (var property in posted.Properties()) { @@ -189,11 +193,17 @@ private void HandleProviderKeys(ApiRequest request) UserKeyStore.Set(kGalleryKeyPrefix + property.Name, (string)property.Value); } // The gallery sends every key it has, so a provider missing from the post is a - // key the user removed. + // key the user removed. A key this version cannot read is a different case: it + // never reached the gallery, so its absence from the post says nothing about + // what the user wants, and deleting it would throw away a key a newer Bloom + // put there. foreach (var name in UserKeyStore.GetNames(kGalleryKeyPrefix)) { - if (!providerIds.Contains(name.Substring(kGalleryKeyPrefix.Length))) - UserKeyStore.Set(name, null); + if (providerIds.Contains(name.Substring(kGalleryKeyPrefix.Length))) + continue; + if (!UserKeyStore.CanRead(name)) + continue; + UserKeyStore.Set(name, null); } request.PostSucceeded(); } diff --git a/src/BloomTests/Utils/UserKeyStoreTests.cs b/src/BloomTests/Utils/UserKeyStoreTests.cs index 45de7fad9a14..db5e3cd6368e 100644 --- a/src/BloomTests/Utils/UserKeyStoreTests.cs +++ b/src/BloomTests/Utils/UserKeyStoreTests.cs @@ -250,6 +250,38 @@ public void GetProtectionMethod_NoSuchKey_ReturnsNull() Assert.That(UserKeyStore.GetProtectionMethod("someService"), Is.Null); } + [Test] + public void CanRead_KeyThisVersionWrote_IsTrue() + { + UserKeyStore.Set("someService", "a-secret"); + + Assert.That(UserKeyStore.CanRead("someService"), Is.True); + } + + [Test] + public void CanRead_NoSuchKey_IsFalse() + { + Assert.That(UserKeyStore.CanRead("someService"), Is.False); + } + + [Test] + public void CanRead_MethodThisVersionCannotRead_IsFalse() + { + // A caller that removes keys the user cleared asks this before removing one, so + // that a key a newer Bloom protected some other way survives. + WriteRawFile( + "{ 'version': 1, 'keys': { 'someService': { 'value': 'AAAA'," + + " 'protection': 'something-a-later-bloom-invented' } } }" + ); + + Assert.That(UserKeyStore.CanRead("someService"), Is.False); + Assert.That( + UserKeyStore.GetProtectionMethod("someService"), + Is.EqualTo("something-a-later-bloom-invented"), + "sanity: the key is on file, it is only unreadable" + ); + } + [Test] public void ProtectThenUnprotect_RoundTripsThePlaintext() { From 9cb3199dd96a91ab79135fba7f6da687d4bd7010 Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 7 Sep 2026 08:10:48 -0600 Subject: [PATCH 03/11] Keep a key this Windows account cannot decrypt (BL-16820) UserKeyStore.CanRead asked only whether the protection method was one this version knows. A key copied in from another computer or another Windows account carries a method we know and a value we cannot decrypt, so CanRead said yes, the key never reached the gallery, and the gallery's next post deleted it as though the user had cleared it. CanRead now reports whether the key can actually be read. Both kinds of unreadable key therefore survive: one protected by a method only a newer Bloom understands, and one that belongs to another account or another computer. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomExe/Utils/UserKeyStore.cs | 14 ++++++++------ src/BloomTests/Utils/UserKeyStoreTests.cs | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/BloomExe/Utils/UserKeyStore.cs b/src/BloomExe/Utils/UserKeyStore.cs index a3912ce40283..bcc91d03deb7 100644 --- a/src/BloomExe/Utils/UserKeyStore.cs +++ b/src/BloomExe/Utils/UserKeyStore.cs @@ -135,15 +135,17 @@ public static string GetProtectionMethod(string name) } /// - /// True when this version of Bloom knows how to read the key of this name, and there is - /// such a key. A caller that removes the keys the user no longer wants asks this first: - /// a key protected by a method only a newer Bloom understands is invisible to - /// , so its absence from what the user is looking at means nothing, and - /// removing it would throw away what the newer Bloom stored. + /// True when there is a key of this name and can actually read it. + /// A caller that removes the keys the user no longer wants asks this first, because a + /// key it cannot read never reached the user: its absence from what they are looking at + /// means nothing, and removing it would throw away a key that is not theirs to lose. + /// Two kinds of key are unreadable, and both must survive: one protected by a method + /// only a newer Bloom understands, and one written by another Windows account or on + /// another computer, which this account cannot decrypt. /// public static bool CanRead(string name) { - return GetProtectionMethod(name) == kDpapiCurrentUserProtection; + return Get(name) != null; } /// diff --git a/src/BloomTests/Utils/UserKeyStoreTests.cs b/src/BloomTests/Utils/UserKeyStoreTests.cs index db5e3cd6368e..47cb29b67c1e 100644 --- a/src/BloomTests/Utils/UserKeyStoreTests.cs +++ b/src/BloomTests/Utils/UserKeyStoreTests.cs @@ -282,6 +282,25 @@ public void CanRead_MethodThisVersionCannotRead_IsFalse() ); } + [Test] + public void CanRead_ValueThisAccountCannotDecrypt_IsFalse() + { + // What a file copied from another computer or another Windows account looks like: + // the protection method is one this Bloom knows, but the value will not decrypt. + // Such a key must survive, so a caller that removes cleared keys leaves it alone. + WriteRawFile( + "{ 'version': 1, 'keys': { 'someService': { 'value': 'bm90LWEtcHJvdGVjdGVkLWJsb2I='," + + " 'protection': 'windows-dpapi-currentuser' } } }" + ); + Assert.That( + UserKeyStore.GetProtectionMethod("someService"), + Is.EqualTo("windows-dpapi-currentuser"), + "sanity: the key is on file with a protection method this version knows" + ); + + Assert.That(UserKeyStore.CanRead("someService"), Is.False); + } + [Test] public void ProtectThenUnprotect_RoundTripsThePlaintext() { From 353af0d14ac83e661bff9f1dfb70d0743a09a93c Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 7 Sep 2026 08:28:52 -0600 Subject: [PATCH 04/11] Make the user key file inconspicuous, and add DPAPI entropy DPAPI CurrentUser does nothing against malware running as the user, so the store leaned on protection it does not really have. It cannot be made to protect against a targeted attacker -- Bloom is open source, so any scheme that had to stay secret was never on offer -- but it can be made to survive the one attacker it can reach: the untargeted credential stealer that sweeps a profile for filenames and words like key, token and api and calls CryptUnprotectData on any blob it finds. Two changes aim at exactly that reader. The file no longer says anything about itself. It is called services.bloom rather than UserKeys.json, its properties are "services", "value" and "method", the method is a bare number instead of "windows-dpapi-currentuser", and the plain-English "about" note explaining that Windows can decrypt the values is gone. What the number means now lives in UserKeyStore's class comment, where it does a maintainer good and a scavenger none. DPAPI is also given fixed app-specific entropy, so unprotecting a found blob takes knowing about Bloom. The bytes are not a secret -- they are in the source -- and they must never change, because every stored value would become unreadable. Method "1" is therefore defined as DPAPI, CurrentUser scope, and exactly those bytes; a later change to any of the three is a new method number written alongside a reader for the old one, which is what the per-value method code exists for. No migration from the old file: nothing has shipped with it. Two new tests: one asserts the file's own vocabulary carries none of key, token, secret, password, api or dpapi (with the base64 values stripped first, since a random DPAPI blob contains such a word often enough to make the test flaky), and one proves the entropy is load-bearing by protecting a blob without it, checking that DPAPI itself can still read that blob, and then watching UserKeyStore.Unprotect refuse it. --- src/BloomExe/Utils/UserKeyStore.cs | 107 ++++++++++++++------- src/BloomTests/Utils/UserKeyStoreTests.cs | 108 +++++++++++++++++----- 2 files changed, 162 insertions(+), 53 deletions(-) diff --git a/src/BloomExe/Utils/UserKeyStore.cs b/src/BloomExe/Utils/UserKeyStore.cs index bcc91d03deb7..580b52acd3d0 100644 --- a/src/BloomExe/Utils/UserKeyStore.cs +++ b/src/BloomExe/Utils/UserKeyStore.cs @@ -35,28 +35,93 @@ namespace Bloom.Utils /// by another account or another machine, and reports it as absent, so the user is asked /// for the key again. A caller that has anything better to say than silence should say it. /// + /// Since DPAPI stops nothing that runs as the user, two cheap measures aim at the one + /// attacker they can reach: the untargeted credential stealer that sweeps a profile for + /// filenames and words like key, token and api, and that calls CryptUnprotectData on any + /// blob it finds. First, the file says nothing about itself. It is called services.bloom, its + /// properties are dull ("services", "value", "method"), the method is a bare number, and + /// there is no note explaining the format -- that explanation lives here, in the source, + /// where it does a maintainer good and a scavenger none. Second, DPAPI is given + /// , so unprotecting a found blob takes knowing about Bloom. + /// Anyone who reads this file still wins, and that is accepted: Bloom is open source, so + /// a scheme that had to stay secret was never on offer. + /// + /// The format is + /// { "version": 1, "services": { "openRouter": { "value": "<base64>", "method": "1" } } }. + /// /// Nothing here knows what any key is for. A caller picks a name and owns its meaning, /// so a new service needs no change to this class. /// public static class UserKeyStore { - private const string kFileName = "UserKeys.json"; + private const string kFileName = "services.bloom"; private const int kCurrentFormatVersion = 1; /// - /// What a key's "protection" field says when Windows DPAPI encrypted its value in - /// CurrentUser scope. The name states the scope as well as the method, because DPAPI - /// also has a LocalMachine scope that decrypts for any account on the computer, and a - /// reader must be able to tell which one it is holding. + /// What a key's "method" field says when its value was encrypted the way this version + /// of Bloom encrypts: Windows DPAPI, CurrentUser scope (not LocalMachine, which any + /// account on the computer could decrypt), with exactly the bytes in + /// as the optional entropy. Those three facts together are what + /// "1" means, and none of them can change without a new number. /// - /// Every method Bloom ever uses gets its own name here, and the name is recorded on + /// Every method Bloom ever uses gets its own number here, and the number is recorded on /// each key rather than once for the file. That is what makes a later change of method /// a migration rather than a loss: a future Bloom reads the field, keeps reading the /// keys it recognizes, converts the ones it wants to move, and leaves alone anything /// written by a version newer than itself. reports /// the field without decrypting, so such a pass can see what it is dealing with. /// - private const string kDpapiCurrentUserProtection = "windows-dpapi-currentuser"; + private const string kDpapiCurrentUserProtection = "1"; + + /// + /// The optional entropy handed to DPAPI along with each value. It is not a secret: it + /// is right here in the source of an open-source program, and anyone who reads this + /// file can use it. What it buys is narrow and worth having anyway. A credential + /// stealer that sweeps a profile and calls CryptUnprotectData on every blob it finds + /// gets nothing from this file without knowing about Bloom, because DPAPI refuses to + /// unprotect a value unless it is given the same entropy that protected it. + /// + /// These bytes must never change. Every value on every user's disk becomes unreadable + /// if they do, and Bloom would have no way to tell that from a file belonging to + /// another Windows account. A later change of entropy is therefore a new method number + /// (see ) written alongside a reader for the + /// old one, not an edit to this array. + /// + private static readonly byte[] kEntropy = + { + 0xC4, + 0xA6, + 0x55, + 0x5E, + 0x6F, + 0x3F, + 0x70, + 0xBA, + 0xFE, + 0x36, + 0x0E, + 0x97, + 0xD3, + 0x13, + 0xA3, + 0xC1, + 0x7A, + 0xBB, + 0xDE, + 0xB3, + 0x46, + 0x0F, + 0x74, + 0x9A, + 0x47, + 0x3F, + 0xB3, + 0x8A, + 0xD5, + 0xAD, + 0x18, + 0x05, + }; /// /// The name under which the user's OpenRouter API key is stored (the "Edit with AI" @@ -208,7 +273,7 @@ public static string Protect(string plaintext) { var encrypted = ProtectedData.Protect( Encoding.UTF8.GetBytes(plaintext), - null, + kEntropy, DataProtectionScope.CurrentUser ); return Convert.ToBase64String(encrypted); @@ -238,7 +303,7 @@ public static string Unprotect(string protectedBase64) { var bytes = ProtectedData.Unprotect( Convert.FromBase64String(protectedBase64), - null, + kEntropy, DataProtectionScope.CurrentUser ); return Encoding.UTF8.GetString(bytes); @@ -259,7 +324,7 @@ private class StoredKey [JsonProperty("value")] public string Value; - [JsonProperty("protection")] + [JsonProperty("method")] public string Protection; } @@ -269,29 +334,10 @@ private class StoreFile [JsonProperty("version")] public int Version = kCurrentFormatVersion; - /// - /// Written on every save and ignored on read: it is there so that whoever opens - /// this file, a person or a later program, can see how the values were encrypted - /// without having to find the Bloom source that wrote them. - /// - [JsonProperty("about")] - public string About; - - [JsonProperty("keys")] + [JsonProperty("services")] public Dictionary Keys = new Dictionary(); } - /// - /// The text of the file's "about" property. It names the protection method Bloom - /// writes today and says that the authority is each key's own "protection" field, so a - /// file holding keys written by two different versions cannot be misread. - /// - private static string AboutText => - "Each key's \"protection\" field says how that key's value is encrypted; " - + $"\"{kDpapiCurrentUserProtection}\" means Windows DPAPI in CurrentUser scope, " - + "which only the Windows account that wrote it, on the computer that wrote it, " - + "can decrypt. Keys do not move to another computer or another account."; - /// /// Reads the file, or reports an empty store when there is none yet. Damaged content is /// reported and treated as empty rather than thrown, because losing a saved key is a @@ -328,7 +374,6 @@ private static StoreFile Load() private static void Save(StoreFile store) { store.Version = kCurrentFormatVersion; - store.About = AboutText; RobustFile.WriteAllText( FilePath, JsonConvert.SerializeObject(store, Formatting.Indented) diff --git a/src/BloomTests/Utils/UserKeyStoreTests.cs b/src/BloomTests/Utils/UserKeyStoreTests.cs index 47cb29b67c1e..5740fd59890d 100644 --- a/src/BloomTests/Utils/UserKeyStoreTests.cs +++ b/src/BloomTests/Utils/UserKeyStoreTests.cs @@ -1,6 +1,7 @@ using System; using System.IO; using System.Linq; +using System.Security.Cryptography; using Bloom.Utils; using NUnit.Framework; using SIL.IO; @@ -141,7 +142,7 @@ public void Get_ValueThatCannotBeDecrypted_ReturnsNull() // Base64 that decodes but is not a DPAPI blob for this user. var notADpapiBlob = Convert.ToBase64String(new byte[] { 1, 2, 3, 4, 5, 6, 7, 8 }); WriteRawFile( - $"{{'version':1,'keys':{{'someService':{{'value':'{notADpapiBlob}','protection':'windows-dpapi-currentuser'}}}}}}" + $"{{'version':1,'services':{{'someService':{{'value':'{notADpapiBlob}','method':'1'}}}}}}" ); Assert.That( @@ -155,7 +156,7 @@ public void Get_ValueThatCannotBeDecrypted_ReturnsNull() public void Get_ValueThatIsNotEvenBase64_ReturnsNull() { WriteRawFile( - "{'version':1,'keys':{'someService':{'value':'not base64 !!!','protection':'windows-dpapi-currentuser'}}}" + "{'version':1,'services':{'someService':{'value':'not base64 !!!','method':'1'}}}" ); Assert.That(UserKeyStore.Get("someService"), Is.Null); @@ -167,7 +168,7 @@ public void Get_UnknownProtectionMethod_ReturnsNull() // What a file written by a future Bloom, or by hand, could look like. Reading the // value as if we knew how it was protected would be worse than asking again. WriteRawFile( - "{'version':1,'keys':{'someService':{'value':'anything','protection':'somethingElse'}}}" + "{'version':1,'services':{'someService':{'value':'anything','method':'somethingElse'}}}" ); Assert.That(UserKeyStore.Get("someService"), Is.Null); @@ -192,24 +193,59 @@ public void Set_AfterDamagedFile_StillStoresTheSecret() } [Test] - public void Set_TheFileSaysHowTheValueIsEncrypted() + public void Set_TheFileRecordsTheMethodAndNothingChatty() { - // A future Bloom, or a person looking at the file, must be able to tell how each - // value was encrypted without reading the Bloom source that wrote it. That is what - // makes a change of method a migration rather than a loss. + // Each value carries the code for how it was encrypted, so that a change of method + // is a migration rather than a loss. What the code means is written in the Bloom + // source and deliberately not in the file: an explanation there would tell a + // scavenger what it had found and a maintainer nothing they cannot read in + // UserKeyStore. UserKeyStore.Set("someService", "a key"); var fileText = RobustFile.ReadAllText(UserKeyStore.FilePath); Assert.That( fileText, - Does.Contain("windows-dpapi-currentuser"), - "each key must name its own protection method" + Does.Contain("\"method\": \"1\""), + "each value must record the method it was encrypted with" ); + Assert.That(fileText, Does.Not.Contain("about"), "the file must not explain itself"); + } + + [Test] + public void Set_TheFileUsesNoGiveawayWords() + { + // The one thing obscurity buys here: an untargeted credential stealer sweeping the + // profile for files whose names or contents say key, token or api passes this one + // over. Anyone who reads Bloom's source still finds it, and that is accepted. + UserKeyStore.Set("someService", "a key"); Assert.That( - fileText, - Does.Contain("about"), - "the file must carry a note explaining what that method means" + UserKeyStore.Get("someService"), + Is.EqualTo("a key"), + "setup: the key must really be stored, or this proves nothing" + ); + + // The encrypted values are base64 of random-looking bytes, so any of these words + // can turn up inside one by chance. They are not what a scavenger reads, so strip + // them before looking at the words the file itself chose. + var fileText = System.Text.RegularExpressions.Regex.Replace( + RobustFile.ReadAllText(UserKeyStore.FilePath), + "\"value\": \"[^\"]*\"", + "\"value\": \"\"" + ); + + foreach (var giveaway in new[] { "key", "token", "secret", "password", "api", "dpapi" }) + { + Assert.That( + fileText.ToLowerInvariant(), + Does.Not.Contain(giveaway), + $"the file's own words must not include '{giveaway}'" + ); + } + Assert.That( + Path.GetFileName(UserKeyStore.FilePath).ToLowerInvariant(), + Does.Not.Contain("key"), + "nor must its name" ); } @@ -218,10 +254,7 @@ public void GetProtectionMethod_ReportsTheMethodWithoutDecrypting() { UserKeyStore.Set("someService", "a key"); - Assert.That( - UserKeyStore.GetProtectionMethod("someService"), - Is.EqualTo("windows-dpapi-currentuser") - ); + Assert.That(UserKeyStore.GetProtectionMethod("someService"), Is.EqualTo("1")); } [Test] @@ -230,7 +263,7 @@ public void GetProtectionMethod_MethodThisVersionCannotRead_StillReportsIt() // What a migration pass needs: Get refuses the value, but the method is still // legible, so the pass can see what it is dealing with and leave it alone. WriteRawFile( - "{'version':1,'keys':{'someService':{'value':'anything','protection':'some-future-method'}}}" + "{'version':1,'services':{'someService':{'value':'anything','method':'some-future-method'}}}" ); Assert.That( @@ -270,8 +303,8 @@ public void CanRead_MethodThisVersionCannotRead_IsFalse() // A caller that removes keys the user cleared asks this before removing one, so // that a key a newer Bloom protected some other way survives. WriteRawFile( - "{ 'version': 1, 'keys': { 'someService': { 'value': 'AAAA'," - + " 'protection': 'something-a-later-bloom-invented' } } }" + "{ 'version': 1, 'services': { 'someService': { 'value': 'AAAA'," + + " 'method': 'something-a-later-bloom-invented' } } }" ); Assert.That(UserKeyStore.CanRead("someService"), Is.False); @@ -289,12 +322,12 @@ public void CanRead_ValueThisAccountCannotDecrypt_IsFalse() // the protection method is one this Bloom knows, but the value will not decrypt. // Such a key must survive, so a caller that removes cleared keys leaves it alone. WriteRawFile( - "{ 'version': 1, 'keys': { 'someService': { 'value': 'bm90LWEtcHJvdGVjdGVkLWJsb2I='," - + " 'protection': 'windows-dpapi-currentuser' } } }" + "{ 'version': 1, 'services': { 'someService': { 'value': 'bm90LWEtcHJvdGVjdGVkLWJsb2I='," + + " 'method': '1' } } }" ); Assert.That( UserKeyStore.GetProtectionMethod("someService"), - Is.EqualTo("windows-dpapi-currentuser"), + Is.EqualTo("1"), "sanity: the key is on file with a protection method this version knows" ); @@ -323,6 +356,37 @@ public void ProtectThenUnprotect_RoundTripsThePlaintext() Assert.That(UserKeyStore.Unprotect(protectedText), Is.EqualTo(original)); } + [Test] + public void Unprotect_BlobMadeWithoutBloomEntropy_ReturnsNull() + { + // Bloom hands DPAPI a fixed extra input, so a tool that finds an encrypted value and + // calls CryptUnprotectData on it the obvious way gets nothing. This test is what + // proves that extra input is actually in play. + const string original = "sk-or-v1-EXAMPLE-key_0123456789"; + var bytes = System.Text.Encoding.UTF8.GetBytes(original); + var blobWithNoEntropy = ProtectedData.Protect( + bytes, + null, + DataProtectionScope.CurrentUser + ); + + // Sanity: this blob is perfectly good DPAPI for this user, so the refusal below is + // about the entropy and not about a blob Windows could never have read. + Assert.That( + System.Text.Encoding.UTF8.GetString( + ProtectedData.Unprotect( + blobWithNoEntropy, + null, + DataProtectionScope.CurrentUser + ) + ), + Is.EqualTo(original), + "setup: DPAPI itself must be able to read this blob" + ); + + Assert.That(UserKeyStore.Unprotect(Convert.ToBase64String(blobWithNoEntropy)), Is.Null); + } + /// /// Writes the file as given, so a test can set up content Bloom itself would not /// write. Single quotes stand in for double quotes, to keep the test strings readable. From f8406aa1ec27ee8aecd4c39ec215a3f28ff9aa1c Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 7 Sep 2026 08:41:39 -0600 Subject: [PATCH 05/11] Let a failure to encrypt a key throw instead of losing the key Set called Protect, and on a null return quietly did nothing. Both callers had already told the user their key was saved, so the user would have found out only at the next launch, when the key was gone again -- and would have kept finding out, every launch, with nothing anywhere saying why. Protect now lets the exception out, which is what Save already does and what the Fail Fast rule in AGENTS.md asks for. Bloom targets net8.0-windows, so this cannot happen today; the day Bloom runs somewhere without DPAPI we want to hear about it. Unprotect is deliberately not symmetrical: a value it cannot read is an ordinary thing to find in the file -- a key from another computer or another Windows account -- so it still returns null. Found by Devin. --- src/BloomExe/Utils/UserKeyStore.cs | 41 +++++++++++------------------- 1 file changed, 15 insertions(+), 26 deletions(-) diff --git a/src/BloomExe/Utils/UserKeyStore.cs b/src/BloomExe/Utils/UserKeyStore.cs index 580b52acd3d0..be478d84767f 100644 --- a/src/BloomExe/Utils/UserKeyStore.cs +++ b/src/BloomExe/Utils/UserKeyStore.cs @@ -229,12 +229,9 @@ public static void Set(string name, string secret) } else { - var protectedSecret = Protect(secret); - if (protectedSecret == null) - return; // Protect already reported why; better to forget the key than to store it in the clear store.Keys[name] = new StoredKey { - Value = protectedSecret, + Value = Protect(secret), Protection = kDpapiCurrentUserProtection, }; } @@ -264,31 +261,23 @@ public static IEnumerable GetNames(string namePrefix = null) /// /// Encrypts a string with the Windows user login (DPAPI, CurrentUser scope) and returns - /// it as base64, or null if this platform or account cannot do that. Public so that a - /// test can prove the round trip. + /// it as base64. Public so that a test can prove the round trip. + /// + /// A failure throws, for the same reason does: the caller is in the + /// middle of telling the user their key is saved. Bloom targets net8.0-windows, so this + /// is not expected at all; it becomes real on the day Bloom runs somewhere without + /// DPAPI, and on that day we want to hear about it rather than have every user quietly + /// re-enter their key at each launch. Note that is different: a + /// value it cannot read is an ordinary thing to find in the file, so it returns null. /// public static string Protect(string plaintext) { - try - { - var encrypted = ProtectedData.Protect( - Encoding.UTF8.GetBytes(plaintext), - kEntropy, - DataProtectionScope.CurrentUser - ); - return Convert.ToBase64String(encrypted); - } - catch (Exception error) - { - // Bloom targets net8.0-windows, so this is not expected. It becomes real on the - // day Bloom runs somewhere without DPAPI, and storing the secret in the clear - // instead would be a nasty surprise to a user who was told it was encrypted. - Logger.WriteError( - "UserKeyStore could not encrypt a key, so it was not saved", - error - ); - return null; - } + var encrypted = ProtectedData.Protect( + Encoding.UTF8.GetBytes(plaintext), + kEntropy, + DataProtectionScope.CurrentUser + ); + return Convert.ToBase64String(encrypted); } /// From 1522396d478cc843c06503660a44602d84d849bd Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 7 Sep 2026 09:28:23 -0600 Subject: [PATCH 06/11] Name the key file services.blm, not services.bloom .bloom is already taken: it is a book inside a Team Collection, and FolderTeamCollection and TeamCollection enumerate *.bloom in a dozen places. Putting an unrelated file with that extension in the user's profile invites a collision for no gain, since the extension was only ever chosen to be dull. .blm is used nowhere in the repo. Nothing has shipped with either name, so there is nothing to migrate. --- src/BloomExe/Utils/UserKeyStore.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/BloomExe/Utils/UserKeyStore.cs b/src/BloomExe/Utils/UserKeyStore.cs index be478d84767f..d5896582e342 100644 --- a/src/BloomExe/Utils/UserKeyStore.cs +++ b/src/BloomExe/Utils/UserKeyStore.cs @@ -38,7 +38,7 @@ namespace Bloom.Utils /// Since DPAPI stops nothing that runs as the user, two cheap measures aim at the one /// attacker they can reach: the untargeted credential stealer that sweeps a profile for /// filenames and words like key, token and api, and that calls CryptUnprotectData on any - /// blob it finds. First, the file says nothing about itself. It is called services.bloom, its + /// blob it finds. First, the file says nothing about itself. It is called services.blm, its /// properties are dull ("services", "value", "method"), the method is a bare number, and /// there is no note explaining the format -- that explanation lives here, in the source, /// where it does a maintainer good and a scavenger none. Second, DPAPI is given @@ -54,7 +54,7 @@ namespace Bloom.Utils /// public static class UserKeyStore { - private const string kFileName = "services.bloom"; + private const string kFileName = "services.blm"; private const int kCurrentFormatVersion = 1; /// From 548ace78c525ae539a81a204caad11b11e2c96c3 Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 7 Sep 2026 09:59:31 -0600 Subject: [PATCH 07/11] Give the service keys their own general API, and rename the store The store was already general -- a caller picks a name, and nothing in it knows what any key is for -- but its only HTTP door was inside the image gallery's controller, and the OpenRouter key had a second, separate door of its own in the AI image editor's controller. Neither placement follows from what a key is: it belongs to the Windows user and outlives any collection. So there is now one ServiceKeysApi, registered application-wide in ApplicationContainer rather than per collection, with two endpoints for the two shapes of caller: serviceKeys/key?name= one key, its value the bare string. serviceKeys/keys?prefix= a whole namespace at once, as one flat JSON object of short name to key plus a "version" property. The image gallery uses the second (prefix imageGallery.), so it still deals only in its own provider ids; "Edit with AI" uses the first. Both old endpoints are gone, imageGallery/providerKeys and aiImageEditor/saveCredentials alike. No migration: a key already on file is read by name exactly as before, since only the door moved, not the store. Dropping aiImageEditor/saveCredentials drops two things it did that a general endpoint cannot know about: the per-launch session token check, and a refusal to persist a key during a Playground session. The editor still hides its credential UI in a Playground book (demoOnly in the launch payload), and every other endpoint that can reach a book keeps its session gate. The namespace endpoint now reads its body with RequiredPostJson, and the gallery dialog posts with postJson, instead of sending JSON as text/plain -- GetPostJson already preserves the payload exactly, which is what the old handler's unescape: false was for. Renamed UserKeyStore to ServiceKeyStore and UserKeysApi to ServiceKeysApi, so the names match the file the keys live in, services.blm. ServiceKeysApiTests covers the real HTTP surface: an absent key replies null, a key holding "+", a space and "%2B" round-trips unchanged, an empty body removes a key, GET strips the prefix, and a namespace POST stores what it carries, removes what it omits, and leaves keys outside the namespace alone. Co-Authored-By: Claude Opus 5 (1M context) --- .../aiImageEditor/aiImageEditorOverlay.ts | 12 +- .../image-gallery/ImageGalleryDialog.tsx | 17 ++- src/BloomExe/ApplicationContainer.cs | 6 +- .../{UserKeyStore.cs => ServiceKeyStore.cs} | 8 +- .../web/controllers/AiImageEditorApi.cs | 58 +------ .../web/controllers/ImageGalleryApi.cs | 73 +-------- .../web/controllers/ServiceKeysApi.cs | 122 +++++++++++++++ ...yStoreTests.cs => ServiceKeyStoreTests.cs} | 118 +++++++-------- src/BloomTests/web/ServiceKeysApiTests.cs | 142 ++++++++++++++++++ 9 files changed, 352 insertions(+), 204 deletions(-) rename src/BloomExe/Utils/{UserKeyStore.cs => ServiceKeyStore.cs} (97%) create mode 100644 src/BloomExe/web/controllers/ServiceKeysApi.cs rename src/BloomTests/Utils/{UserKeyStoreTests.cs => ServiceKeyStoreTests.cs} (74%) create mode 100644 src/BloomTests/web/ServiceKeysApiTests.cs diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts index 818f2158547a..d205584a2cfe 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts @@ -1,4 +1,4 @@ -// The AI Image Editor overlay and session — the TOP-WINDOW half of the feature. +// The AI Image Editor overlay and session — the TOP-WINDOW half of the feature. // // This runs in the workspace root, not the page iframe, for the same reason the image // gallery and the copyright/license dialog do (see the comments on those commands in @@ -32,6 +32,7 @@ import { post, postJson, + postString, trackChangePicture, trackEvent, } from "../../utils/bloomApi"; @@ -634,12 +635,9 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { // Bloom owns the OpenRouter API key. A key the user pastes into the // AI Image Editor is handed up here so Bloom persists it per-user (and // supplies it on the next launch). A null apiKey clears the stored key. - postJson( - "aiImageEditor/saveCredentials?session=" + - encodeURIComponent(launchData.sessionToken), - { - apiKey: data.payload?.apiKey ?? null, - }, + postString( + "serviceKeys/key?name=openRouter", + data.payload?.apiKey ?? "", ); break; } diff --git a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx index dd55272c1815..009be7ff4bac 100644 --- a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx +++ b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx @@ -1,4 +1,4 @@ -import { css } from "@emotion/react"; +import { css } from "@emotion/react"; import { ImageGallery } from "bloom-image-gallery"; import type { IImage, @@ -13,8 +13,8 @@ import { import { getBloomApiPrefix, getAsync, + postJson, postJsonAsync, - postString, postDataWithConfigAsync, trackEvent, } from "../../utils/bloomApi"; @@ -40,8 +40,9 @@ const ImageGalleryDialog: React.FunctionComponent<{ searchLang: string; }> = (props) => { const [open, setOpen] = useState(true); - // Keys are loaded from the per-user key store before the gallery is rendered, - // so providers (e.g. Pixabay) receive their initial API key in their constructor. + // Keys are loaded from Bloom's per-user service key store before the gallery is + // rendered, so providers (e.g. Pixabay) receive their initial API key in their + // constructor. const [providerKeys, setProviderKeys] = useState< IProviderKeysV1 | undefined >(undefined); @@ -66,7 +67,7 @@ const ImageGalleryDialog: React.FunctionComponent<{ // A mount effect is justified: this is a one-time async fetch that must run after mount // so the component can render before the network round-trip completes. useMountEffect(() => { - getAsync("imageGallery/providerKeys") + getAsync("serviceKeys/keys?prefix=imageGallery.") .then((r) => { const keys = r?.data as IProviderKeysV1; // Bloom replies with the format version plus one property per provider the @@ -257,9 +258,9 @@ const ImageGalleryDialog: React.FunctionComponent<{ // key supplied while the chooser is open is reflected in what this // visit reports. pixabayKeyPresentRef.current = !!keys.pixabay; - postString( - "imageGallery/providerKeys", - JSON.stringify(keys), + postJson( + "serviceKeys/keys?prefix=imageGallery.", + keys, ); }} onLanguageChange={(lang) => diff --git a/src/BloomExe/ApplicationContainer.cs b/src/BloomExe/ApplicationContainer.cs index 87bd05ade4a6..03518b90d372 100644 --- a/src/BloomExe/ApplicationContainer.cs +++ b/src/BloomExe/ApplicationContainer.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Linq; using System.Reflection; using System.Windows.Forms; @@ -83,6 +83,7 @@ public ApplicationContainer() typeof(CollectionChooserApi), typeof(I18NApi), typeof(ProgressDialogApi), + typeof(ServiceKeysApi), }.Contains(t) ); @@ -131,6 +132,9 @@ public ApplicationContainer() _container .Resolve() .RegisterWithApiHandler(server.ApiHandler); + // The user's service API keys belong to the Windows user, not to a collection, so + // these endpoints live here rather than in ProjectContext. + _container.Resolve().RegisterWithApiHandler(server.ApiHandler); server.ApiHandler.RecordApplicationLevelHandlers(); } diff --git a/src/BloomExe/Utils/UserKeyStore.cs b/src/BloomExe/Utils/ServiceKeyStore.cs similarity index 97% rename from src/BloomExe/Utils/UserKeyStore.cs rename to src/BloomExe/Utils/ServiceKeyStore.cs index d5896582e342..64bb0dd949f6 100644 --- a/src/BloomExe/Utils/UserKeyStore.cs +++ b/src/BloomExe/Utils/ServiceKeyStore.cs @@ -52,7 +52,7 @@ namespace Bloom.Utils /// Nothing here knows what any key is for. A caller picks a name and owns its meaning, /// so a new service needs no change to this class. /// - public static class UserKeyStore + public static class ServiceKeyStore { private const string kFileName = "services.blm"; private const int kCurrentFormatVersion = 1; @@ -176,7 +176,7 @@ public static string Get(string name) // other way. Guessing at the bytes would be worse than asking the user // again, and this version must not overwrite what it cannot read. Logger.WriteEvent( - $"UserKeyStore: the key '{name}' says it is protected by '{storedKey.Protection}', which this version of Bloom does not know how to read. Treating it as absent." + $"ServiceKeyStore: the key '{name}' says it is protected by '{storedKey.Protection}', which this version of Bloom does not know how to read. Treating it as absent." ); return null; } @@ -301,7 +301,7 @@ public static string Unprotect(string protectedBase64) when (error is CryptographicException || error is FormatException) { Logger.WriteEvent( - $"UserKeyStore: a stored key could not be decrypted on this computer and account ({error.Message}). The user must enter it again." + $"ServiceKeyStore: a stored key could not be decrypted on this computer and account ({error.Message}). The user must enter it again." ); return null; } @@ -348,7 +348,7 @@ private static StoreFile Load() catch (Exception error) { Logger.WriteError( - "UserKeyStore could not read " + FilePath + "; treating it as empty", + "ServiceKeyStore could not read " + FilePath + "; treating it as empty", error ); return new StoreFile(); diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index dcf0ba2d3ea8..eaab967a84bf 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Diagnostics; using System.IO; @@ -62,7 +62,6 @@ namespace Bloom.web.controllers /// images + history, return the launch payload. /// aiImageEditor/file GET/POST/DELETE files under .ai-image-editor/. /// aiImageEditor/commit apply the chosen replacements to the book. - /// aiImageEditor/saveCredentials persist the user's OpenRouter API key. /// 2. window.postMessage on channel "bloom-ai-image-tools", between the overlay JS /// (aiImageEditorOverlay.ts, in the TOP window) and the AI image editor's iframe: ready / /// init / commit / cancel / log / ack. The overlay JS — NOT this class — sends @@ -75,7 +74,7 @@ namespace Bloom.web.controllers /// source of truth. /// /// SECURITY - /// A per-launch session token (query param) gates /file, /commit, /saveCredentials. + /// A per-launch session token (query param) gates /file and /commit. /// File names are allow-listed; page/result ids are charset-restricted; reused /// source URLs must resolve inside the book folder (no path traversal). /// @@ -196,12 +195,6 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) handleOnUiThread: true, requiresSync: true ); - apiHandler.RegisterEndpointHandler( - "aiImageEditor/saveCredentials", - HandleSaveCredentials, - handleOnUiThread: false, - requiresSync: false - ); } /// @@ -497,13 +490,12 @@ private void HandleLaunch(ApiRequest request) references = Array.Empty(), // Bloom owns the OpenRouter key: supply the per-user stored key so the AI // image editor doesn't have to ask for it again. It hands any newly - // obtained key back via aiImageEditor/saveCredentials. - apiKey = UserKeyStore.Get(UserKeyStore.kOpenRouterName), + // obtained key back to Bloom via serviceKeys/key (see ServiceKeysApi). + apiKey = ServiceKeyStore.Get(ServiceKeyStore.kOpenRouterName), // In a Playground template book all features are unlocked for // "try it out", so the AI image editor opens — but it's a shared demo // context, so it must not let the user set/save an OpenRouter API key. - // The AI image editor disables its credential UI when this is true; - // HandleSaveCredentials also refuses to persist. + // The AI image editor disables its credential UI when this is true. demoOnly = book.IsPlayground, // Let the AI image editor reveal its developer/tester tools (e.g. the // "Local Dummy (No AI)" model, for cost-free testing). The AI image @@ -558,46 +550,6 @@ internal static bool ShouldShowDeveloperTools() && kTesterToolsOnValues.Contains(optIn, StringComparer.OrdinalIgnoreCase); } - private class SaveCredentialsRequest - { - public string apiKey { get; set; } - } - - /// - /// Receives the user's OpenRouter API key from the AI image editor (manual key entry) - /// and persists it per Windows user via . A null/empty - /// apiKey clears the stored key (sign-out). Session-gated so a stray frame can't - /// overwrite the user's stored key. - /// - private void HandleSaveCredentials(ApiRequest request) - { - if (!HasValidSession(request)) - return; - - // Defense in depth for the Playground "demo" case (see HandleLaunch): never - // persist a key obtained during a Playground session, even if a stray frame - // posts here despite the AI image editor's disabled credential UI. - if (_bookSelection.CurrentSelection?.IsPlayground == true) - { - request.PostSucceeded(); - return; - } - - SaveCredentialsRequest payload; - try - { - payload = request.RequiredPostObject(); - } - catch (Exception) - { - request.Failed(HttpStatusCode.BadRequest, "Invalid credentials payload"); - return; - } - - UserKeyStore.Set(UserKeyStore.kOpenRouterName, payload.apiKey); - request.PostSucceeded(); - } - // Invalidates the current session. Called at the start of each launch to tear down // any prior session; the overlay itself is created and removed by the overlay JS. private void EndSession() diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index 472e3de19366..57990f2649f5 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.IO; using System.Linq; @@ -14,8 +14,6 @@ using Bloom.ImageProcessing; using Bloom.MiscUI; using Bloom.Utils; -using Newtonsoft.Json; -using Newtonsoft.Json.Linq; using SIL.Core.ClearShare; using SIL.IO; using SIL.Reporting; @@ -98,11 +96,6 @@ public class ImageGalleryApi : IDisposable "ImageCollections" ); - /// - /// The start of the name of every key stored for a gallery provider. - /// - private const string kGalleryKeyPrefix = UserKeyStore.kImageGalleryNamePrefix; - public void RegisterWithApiHandler(BloomApiHandler apiHandler) { apiHandler.RegisterAsyncEndpointHandler( @@ -143,70 +136,6 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) HandleLocalCollectionImage, false ); - apiHandler.RegisterEndpointHandler( - "imageGallery/providerKeys", - HandleProviderKeys, - false - ); - } - - /// - /// Gets or sets the API keys the user has for the gallery's search providers, such as - /// the key they fetched from Pixabay's site. The gallery's shape for these is one JSON - /// object of provider id to key, plus a "version" property; Bloom keeps each key as - /// its own entry, named with the gallery's provider id, so neither side needs a - /// change when the gallery gains a provider. - /// - /// These are per Windows user, not per collection and not per copy of Bloom. See - /// , which explains why they cannot be Bloom settings. - /// - private void HandleProviderKeys(ApiRequest request) - { - if (request.HttpMethod == HttpMethods.Get) - { - // One flat object, the shape the gallery expects: the format version plus - // one property per provider that has a key. - var keys = new Dictionary { ["version"] = 1 }; - foreach (var name in UserKeyStore.GetNames(kGalleryKeyPrefix)) - { - // Null when the key cannot be decrypted on this computer, which the - // gallery reads the same way as never having had a key: it asks for one. - var key = UserKeyStore.Get(name); - if (!string.IsNullOrEmpty(key)) - keys[name.Substring(kGalleryKeyPrefix.Length)] = key; - } - request.ReplyWithJson(JsonConvert.SerializeObject(keys)); - } - else - { - // A key can hold any character a service cares to use, "+" and "%" among - // them, so read the body exactly as the gallery sent it. The default - // unescape would turn a "+" into a space and decode a percent escape, and - // Bloom would store a key the service then rejects. - var posted = JObject.Parse(request.RequiredPostString(unescape: false)); - var providerIds = new HashSet(); - foreach (var property in posted.Properties()) - { - if (property.Name == "version") - continue; - providerIds.Add(property.Name); - UserKeyStore.Set(kGalleryKeyPrefix + property.Name, (string)property.Value); - } - // The gallery sends every key it has, so a provider missing from the post is a - // key the user removed. A key this version cannot read is a different case: it - // never reached the gallery, so its absence from the post says nothing about - // what the user wants, and deleting it would throw away a key a newer Bloom - // put there. - foreach (var name in UserKeyStore.GetNames(kGalleryKeyPrefix)) - { - if (providerIds.Contains(name.Substring(kGalleryKeyPrefix.Length))) - continue; - if (!UserKeyStore.CanRead(name)) - continue; - UserKeyStore.Set(name, null); - } - request.PostSucceeded(); - } } /// diff --git a/src/BloomExe/web/controllers/ServiceKeysApi.cs b/src/BloomExe/web/controllers/ServiceKeysApi.cs new file mode 100644 index 000000000000..299cd5f5b84d --- /dev/null +++ b/src/BloomExe/web/controllers/ServiceKeysApi.cs @@ -0,0 +1,122 @@ +using System.Collections.Generic; +using Bloom.Api; +using Bloom.Utils; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; + +namespace Bloom.web.controllers +{ + /// + /// The front end's door to : the API keys the user fetched from + /// some service's own website, such as Pixabay for picture search or OpenRouter for + /// "Edit with AI". Like the store, this knows nothing about any particular service -- a + /// caller picks a name -- so a new service needs no change here. + /// + /// Two endpoints, because there are two shapes of caller: + /// + /// serviceKeys/key handles ONE key by its full name, and its value is the bare string. That + /// suits a feature with a single key, such as "Edit with AI" (name "openRouter"). + /// + /// serviceKeys/keys handles a whole NAMESPACE at once, named by a prefix, as one flat JSON + /// object of short name to key plus a "version" property. That suits the image gallery, + /// which has one key per search provider and hands Bloom all of them together. The prefix + /// is stripped on the way out and added on the way in, so the gallery deals only in its + /// own provider ids and neither side changes when it gains a provider. + /// + /// These endpoints are registered application-wide rather than per collection, because a + /// key belongs to the Windows user and outlives any collection. They are not + /// authenticated, which is true of Bloom's whole localhost API; anything that can reach + /// the server can read the user's keys, and the server is only reachable from this + /// computer. + /// + public class ServiceKeysApi + { + /// + /// The property the namespace shape carries alongside the keys, so that the front end + /// can tell a Bloom that speaks this shape from a later one that does not. + /// + private const int kNamespaceFormatVersion = 1; + + private const string kVersionPropertyName = "version"; + + /// Wires up the endpoints. See the class comment. + public void RegisterWithApiHandler(BloomApiHandler apiHandler) + { + apiHandler.RegisterEndpointHandler("serviceKeys/key", HandleKey, false); + apiHandler.RegisterEndpointHandler("serviceKeys/keys", HandleNamespace, false); + } + + /// + /// GET returns the one key named by the "name" parameter as a JSON string, or null + /// when there is none, or when there is one this computer cannot decrypt -- which the + /// caller should treat the same way, by asking the user for the key again. + /// POST stores the posted body as that key's value; an empty body removes it. + /// + private void HandleKey(ApiRequest request) + { + var name = request.RequiredParam("name"); + if (request.HttpMethod == HttpMethods.Get) + { + request.ReplyWithJson(JsonConvert.SerializeObject(ServiceKeyStore.Get(name))); + return; + } + // A key can hold any character a service cares to use, "+" and "%" among them, so + // take the body exactly as it was posted. The default unescape would turn a "+" + // into a space and decode a percent escape, and Bloom would store a key the + // service then rejects. + ServiceKeyStore.Set(name, request.RequiredPostString(unescape: false)); + request.PostSucceeded(); + } + + /// + /// GET returns every readable key whose name starts with the "prefix" parameter, as + /// one flat object of the rest-of-the-name to the key, plus the format version. + /// POST replaces that namespace with the JSON that is posted, in the same shape. + /// + private void HandleNamespace(ApiRequest request) + { + var prefix = request.RequiredParam("prefix"); + if (request.HttpMethod == HttpMethods.Get) + { + var keys = new Dictionary + { + [kVersionPropertyName] = kNamespaceFormatVersion, + }; + foreach (var name in ServiceKeyStore.GetNames(prefix)) + { + // Null when the key cannot be decrypted on this computer, which a caller + // reads the same way as never having had a key: it asks for one. + var key = ServiceKeyStore.Get(name); + if (!string.IsNullOrEmpty(key)) + keys[name.Substring(prefix.Length)] = key; + } + request.ReplyWithJson(JsonConvert.SerializeObject(keys)); + return; + } + + var posted = JObject.Parse(request.RequiredPostJson()); + var postedShortNames = new HashSet(); + foreach (var property in posted.Properties()) + { + if (property.Name == kVersionPropertyName) + continue; + postedShortNames.Add(property.Name); + ServiceKeyStore.Set(prefix + property.Name, (string)property.Value); + } + + // The caller sends the whole namespace, so a name missing from the post is a key + // the user removed. A key this version cannot read is a different case: it never + // reached the caller, so its absence from the post says nothing about what the + // user wants, and deleting it would throw away a key a newer Bloom put there. + foreach (var name in ServiceKeyStore.GetNames(prefix)) + { + if (postedShortNames.Contains(name.Substring(prefix.Length))) + continue; + if (!ServiceKeyStore.CanRead(name)) + continue; + ServiceKeyStore.Set(name, null); + } + request.PostSucceeded(); + } + } +} diff --git a/src/BloomTests/Utils/UserKeyStoreTests.cs b/src/BloomTests/Utils/ServiceKeyStoreTests.cs similarity index 74% rename from src/BloomTests/Utils/UserKeyStoreTests.cs rename to src/BloomTests/Utils/ServiceKeyStoreTests.cs index 5740fd59890d..f6789fd788b7 100644 --- a/src/BloomTests/Utils/UserKeyStoreTests.cs +++ b/src/BloomTests/Utils/ServiceKeyStoreTests.cs @@ -10,10 +10,10 @@ namespace BloomTests.Utils { /// - /// Tests for . + /// Tests for . /// /// Every test works on a temporary folder, set through - /// . The real file holds the developer's + /// . The real file holds the developer's /// own API keys, so a test that wrote there would destroy them. /// /// The behavior that matters most here is what happens to a key that cannot be @@ -21,28 +21,28 @@ namespace BloomTests.Utils /// than throwing, so the feature asks for the key again. /// [TestFixture] - public class UserKeyStoreTests + public class ServiceKeyStoreTests { private TemporaryFolder _folder; [SetUp] public void Setup() { - _folder = new TemporaryFolder("UserKeyStoreTests"); - UserKeyStore.FolderForTests = _folder.Path; + _folder = new TemporaryFolder("ServiceKeyStoreTests"); + ServiceKeyStore.FolderForTests = _folder.Path; } [TearDown] public void TearDown() { - UserKeyStore.FolderForTests = null; + ServiceKeyStore.FolderForTests = null; _folder.Dispose(); } [Test] public void Get_NothingStored_ReturnsNull() { - Assert.That(UserKeyStore.Get("someService"), Is.Null); + Assert.That(ServiceKeyStore.Get("someService"), Is.Null); } [Test] @@ -50,22 +50,22 @@ public void SetThenGet_ReturnsTheSecret() { const string secret = "sk-or-v1-EXAMPLE-key_0123456789"; - UserKeyStore.Set("someService", secret); + ServiceKeyStore.Set("someService", secret); // Sanity: the file exists and does not contain the secret in the clear, so the // successful read below proves decryption rather than a plain-text round trip. Assert.That( - RobustFile.Exists(UserKeyStore.FilePath), + RobustFile.Exists(ServiceKeyStore.FilePath), Is.True, "setup: Set should have written the file" ); Assert.That( - RobustFile.ReadAllText(UserKeyStore.FilePath), + RobustFile.ReadAllText(ServiceKeyStore.FilePath), Does.Not.Contain(secret), "setup: the secret must not be stored in the clear" ); - Assert.That(UserKeyStore.Get("someService"), Is.EqualTo(secret)); + Assert.That(ServiceKeyStore.Get("someService"), Is.EqualTo(secret)); } [Test] @@ -74,60 +74,60 @@ public void SetThenGet_UnicodeSecret_RoundTrips() // Keys are ASCII, but the encryption is UTF-8 based, so prove non-ASCII survives. const string secret = "clé-secrète-日本語-😀"; - UserKeyStore.Set("someService", secret); + ServiceKeyStore.Set("someService", secret); - Assert.That(UserKeyStore.Get("someService"), Is.EqualTo(secret)); + Assert.That(ServiceKeyStore.Get("someService"), Is.EqualTo(secret)); } [Test] public void Set_SecondValue_ReplacesTheFirst() { - UserKeyStore.Set("someService", "first"); + ServiceKeyStore.Set("someService", "first"); Assert.That( - UserKeyStore.Get("someService"), + ServiceKeyStore.Get("someService"), Is.EqualTo("first"), "setup: the first value should be readable before we replace it" ); - UserKeyStore.Set("someService", "second"); + ServiceKeyStore.Set("someService", "second"); - Assert.That(UserKeyStore.Get("someService"), Is.EqualTo("second")); + Assert.That(ServiceKeyStore.Get("someService"), Is.EqualTo("second")); } [Test] public void Set_EmptySecret_RemovesTheKey() { - UserKeyStore.Set("someService", "a key"); + ServiceKeyStore.Set("someService", "a key"); Assert.That( - UserKeyStore.GetNames().ToList(), + ServiceKeyStore.GetNames().ToList(), Has.Count.EqualTo(1), "setup: the key should be on file before we clear it" ); - UserKeyStore.Set("someService", ""); + ServiceKeyStore.Set("someService", ""); - Assert.That(UserKeyStore.Get("someService"), Is.Null); - Assert.That(UserKeyStore.GetNames(), Is.Empty); + Assert.That(ServiceKeyStore.Get("someService"), Is.Null); + Assert.That(ServiceKeyStore.GetNames(), Is.Empty); } [Test] public void Set_TwoKeys_KeepsBoth() { - UserKeyStore.Set("serviceOne", "one"); - UserKeyStore.Set("serviceTwo", "two"); + ServiceKeyStore.Set("serviceOne", "one"); + ServiceKeyStore.Set("serviceTwo", "two"); - Assert.That(UserKeyStore.Get("serviceOne"), Is.EqualTo("one")); - Assert.That(UserKeyStore.Get("serviceTwo"), Is.EqualTo("two")); + Assert.That(ServiceKeyStore.Get("serviceOne"), Is.EqualTo("one")); + Assert.That(ServiceKeyStore.Get("serviceTwo"), Is.EqualTo("two")); } [Test] public void GetNames_WithPrefix_ReturnsOnlyTheMatchingOnes() { - UserKeyStore.Set("imageGallery.pixabay", "one"); - UserKeyStore.Set("imageGallery.somethingElse", "two"); - UserKeyStore.Set("openRouter", "three"); + ServiceKeyStore.Set("imageGallery.pixabay", "one"); + ServiceKeyStore.Set("imageGallery.somethingElse", "two"); + ServiceKeyStore.Set("openRouter", "three"); - var names = UserKeyStore.GetNames("imageGallery.").ToList(); + var names = ServiceKeyStore.GetNames("imageGallery.").ToList(); Assert.That( names, @@ -146,7 +146,7 @@ public void Get_ValueThatCannotBeDecrypted_ReturnsNull() ); Assert.That( - UserKeyStore.Get("someService"), + ServiceKeyStore.Get("someService"), Is.Null, "a key that cannot be decrypted here must read as absent, not throw" ); @@ -159,7 +159,7 @@ public void Get_ValueThatIsNotEvenBase64_ReturnsNull() "{'version':1,'services':{'someService':{'value':'not base64 !!!','method':'1'}}}" ); - Assert.That(UserKeyStore.Get("someService"), Is.Null); + Assert.That(ServiceKeyStore.Get("someService"), Is.Null); } [Test] @@ -171,7 +171,7 @@ public void Get_UnknownProtectionMethod_ReturnsNull() "{'version':1,'services':{'someService':{'value':'anything','method':'somethingElse'}}}" ); - Assert.That(UserKeyStore.Get("someService"), Is.Null); + Assert.That(ServiceKeyStore.Get("someService"), Is.Null); } [Test] @@ -179,7 +179,7 @@ public void Get_DamagedFile_ReturnsNullRatherThanThrowing() { WriteRawFile("this is not JSON at all"); - Assert.That(UserKeyStore.Get("someService"), Is.Null); + Assert.That(ServiceKeyStore.Get("someService"), Is.Null); } [Test] @@ -187,9 +187,9 @@ public void Set_AfterDamagedFile_StillStoresTheSecret() { WriteRawFile("this is not JSON at all"); - UserKeyStore.Set("someService", "a key"); + ServiceKeyStore.Set("someService", "a key"); - Assert.That(UserKeyStore.Get("someService"), Is.EqualTo("a key")); + Assert.That(ServiceKeyStore.Get("someService"), Is.EqualTo("a key")); } [Test] @@ -199,10 +199,10 @@ public void Set_TheFileRecordsTheMethodAndNothingChatty() // is a migration rather than a loss. What the code means is written in the Bloom // source and deliberately not in the file: an explanation there would tell a // scavenger what it had found and a maintainer nothing they cannot read in - // UserKeyStore. - UserKeyStore.Set("someService", "a key"); + // ServiceKeyStore. + ServiceKeyStore.Set("someService", "a key"); - var fileText = RobustFile.ReadAllText(UserKeyStore.FilePath); + var fileText = RobustFile.ReadAllText(ServiceKeyStore.FilePath); Assert.That( fileText, @@ -218,9 +218,9 @@ public void Set_TheFileUsesNoGiveawayWords() // The one thing obscurity buys here: an untargeted credential stealer sweeping the // profile for files whose names or contents say key, token or api passes this one // over. Anyone who reads Bloom's source still finds it, and that is accepted. - UserKeyStore.Set("someService", "a key"); + ServiceKeyStore.Set("someService", "a key"); Assert.That( - UserKeyStore.Get("someService"), + ServiceKeyStore.Get("someService"), Is.EqualTo("a key"), "setup: the key must really be stored, or this proves nothing" ); @@ -229,7 +229,7 @@ public void Set_TheFileUsesNoGiveawayWords() // can turn up inside one by chance. They are not what a scavenger reads, so strip // them before looking at the words the file itself chose. var fileText = System.Text.RegularExpressions.Regex.Replace( - RobustFile.ReadAllText(UserKeyStore.FilePath), + RobustFile.ReadAllText(ServiceKeyStore.FilePath), "\"value\": \"[^\"]*\"", "\"value\": \"\"" ); @@ -243,7 +243,7 @@ public void Set_TheFileUsesNoGiveawayWords() ); } Assert.That( - Path.GetFileName(UserKeyStore.FilePath).ToLowerInvariant(), + Path.GetFileName(ServiceKeyStore.FilePath).ToLowerInvariant(), Does.Not.Contain("key"), "nor must its name" ); @@ -252,9 +252,9 @@ public void Set_TheFileUsesNoGiveawayWords() [Test] public void GetProtectionMethod_ReportsTheMethodWithoutDecrypting() { - UserKeyStore.Set("someService", "a key"); + ServiceKeyStore.Set("someService", "a key"); - Assert.That(UserKeyStore.GetProtectionMethod("someService"), Is.EqualTo("1")); + Assert.That(ServiceKeyStore.GetProtectionMethod("someService"), Is.EqualTo("1")); } [Test] @@ -267,12 +267,12 @@ public void GetProtectionMethod_MethodThisVersionCannotRead_StillReportsIt() ); Assert.That( - UserKeyStore.Get("someService"), + ServiceKeyStore.Get("someService"), Is.Null, "setup: this version must refuse a method it does not know" ); Assert.That( - UserKeyStore.GetProtectionMethod("someService"), + ServiceKeyStore.GetProtectionMethod("someService"), Is.EqualTo("some-future-method") ); } @@ -280,21 +280,21 @@ public void GetProtectionMethod_MethodThisVersionCannotRead_StillReportsIt() [Test] public void GetProtectionMethod_NoSuchKey_ReturnsNull() { - Assert.That(UserKeyStore.GetProtectionMethod("someService"), Is.Null); + Assert.That(ServiceKeyStore.GetProtectionMethod("someService"), Is.Null); } [Test] public void CanRead_KeyThisVersionWrote_IsTrue() { - UserKeyStore.Set("someService", "a-secret"); + ServiceKeyStore.Set("someService", "a-secret"); - Assert.That(UserKeyStore.CanRead("someService"), Is.True); + Assert.That(ServiceKeyStore.CanRead("someService"), Is.True); } [Test] public void CanRead_NoSuchKey_IsFalse() { - Assert.That(UserKeyStore.CanRead("someService"), Is.False); + Assert.That(ServiceKeyStore.CanRead("someService"), Is.False); } [Test] @@ -307,9 +307,9 @@ public void CanRead_MethodThisVersionCannotRead_IsFalse() + " 'method': 'something-a-later-bloom-invented' } } }" ); - Assert.That(UserKeyStore.CanRead("someService"), Is.False); + Assert.That(ServiceKeyStore.CanRead("someService"), Is.False); Assert.That( - UserKeyStore.GetProtectionMethod("someService"), + ServiceKeyStore.GetProtectionMethod("someService"), Is.EqualTo("something-a-later-bloom-invented"), "sanity: the key is on file, it is only unreadable" ); @@ -326,12 +326,12 @@ public void CanRead_ValueThisAccountCannotDecrypt_IsFalse() + " 'method': '1' } } }" ); Assert.That( - UserKeyStore.GetProtectionMethod("someService"), + ServiceKeyStore.GetProtectionMethod("someService"), Is.EqualTo("1"), "sanity: the key is on file with a protection method this version knows" ); - Assert.That(UserKeyStore.CanRead("someService"), Is.False); + Assert.That(ServiceKeyStore.CanRead("someService"), Is.False); } [Test] @@ -339,7 +339,7 @@ public void ProtectThenUnprotect_RoundTripsThePlaintext() { const string original = "sk-or-v1-EXAMPLE-key_0123456789"; - var protectedText = UserKeyStore.Protect(original); + var protectedText = ServiceKeyStore.Protect(original); // Sanity: encryption actually transformed the value, so the round trip below is // meaningful and is not just echoing the plaintext back. @@ -353,7 +353,7 @@ public void ProtectThenUnprotect_RoundTripsThePlaintext() "setup: Protect must produce base64, because that is what we store" ); - Assert.That(UserKeyStore.Unprotect(protectedText), Is.EqualTo(original)); + Assert.That(ServiceKeyStore.Unprotect(protectedText), Is.EqualTo(original)); } [Test] @@ -384,7 +384,7 @@ public void Unprotect_BlobMadeWithoutBloomEntropy_ReturnsNull() "setup: DPAPI itself must be able to read this blob" ); - Assert.That(UserKeyStore.Unprotect(Convert.ToBase64String(blobWithNoEntropy)), Is.Null); + Assert.That(ServiceKeyStore.Unprotect(Convert.ToBase64String(blobWithNoEntropy)), Is.Null); } /// @@ -394,7 +394,7 @@ public void Unprotect_BlobMadeWithoutBloomEntropy_ReturnsNull() private void WriteRawFile(string contentWithSingleQuotes) { RobustFile.WriteAllText( - UserKeyStore.FilePath, + ServiceKeyStore.FilePath, contentWithSingleQuotes.Replace('\'', '"') ); } diff --git a/src/BloomTests/web/ServiceKeysApiTests.cs b/src/BloomTests/web/ServiceKeysApiTests.cs new file mode 100644 index 000000000000..01cdc538a40b --- /dev/null +++ b/src/BloomTests/web/ServiceKeysApiTests.cs @@ -0,0 +1,142 @@ +using System.Linq; +using System.Threading; +using Bloom.Api; +using Bloom.Book; +using Bloom.Utils; +using Bloom.web.controllers; +using Newtonsoft.Json.Linq; +using NUnit.Framework; +using SIL.TestUtilities; + +namespace BloomTests.web +{ + /// + /// Integration tests over the real HTTP surface of : the two + /// endpoints the front end uses to read and write the user's service API keys. + /// + /// points the store at a TemporaryFolder, so + /// nothing here can read or overwrite the developer's own keys. + /// + [TestFixture] + public class ServiceKeysApiTests + { + private BloomServer _server; + private TemporaryFolder _folder; + + [SetUp] + public void Setup() + { + // Share the same monitor as the other server tests so we never run two servers on + // the fixed test port at once. + Monitor.Enter(EndpointHandlerTests._portMonitor); + + _folder = new TemporaryFolder("ServiceKeysApiTests"); + ServiceKeyStore.FolderForTests = _folder.Path; + _server = new BloomServer(new BookSelection()); + new ServiceKeysApi().RegisterWithApiHandler(_server.ApiHandler); + } + + [TearDown] + public void TearDown() + { + RetiredTestServers.Retire(_server); + _server = null; + ServiceKeyStore.FolderForTests = null; + _folder.Dispose(); + + Monitor.Exit(EndpointHandlerTests._portMonitor); + } + + [Test] + public void GetKey_WhenThereIsNone_RepliesNull() + { + Assert.That( + ApiTest.GetString(_server, "serviceKeys/key", "name=openRouter"), + Is.EqualTo("null") + ); + } + + [Test] + public void PostKey_ThenGet_RoundTripsCharactersUrlEscapingWouldChange() + { + // A real key can hold any of these, and each is something the default unescaping + // would rewrite: "+" would become a space, and "%2B" a "+". + const string key = "sk-a+b%2Bc d/e="; + + ApiTest.PostString( + _server, + "serviceKeys/key?name=openRouter", + key, + ApiTest.ContentType.Text + ); + + Assert.That( + ServiceKeyStore.Get("openRouter"), + Is.EqualTo(key), + "the store must hold exactly what was posted" + ); + Assert.That( + ApiTest.GetString(_server, "serviceKeys/key", "name=openRouter"), + Is.EqualTo(JToken.FromObject(key).ToString(Newtonsoft.Json.Formatting.None)) + ); + } + + [Test] + public void PostKey_EmptyBody_RemovesTheKey() + { + ServiceKeyStore.Set("openRouter", "a key"); + Assert.That( + ServiceKeyStore.Get("openRouter"), + Is.EqualTo("a key"), + "setup: the key must really be there, or this proves nothing" + ); + + ApiTest.PostString( + _server, + "serviceKeys/key?name=openRouter", + "", + ApiTest.ContentType.Text + ); + + Assert.That(ServiceKeyStore.Get("openRouter"), Is.Null); + } + + [Test] + public void GetNamespace_ReturnsTheVersionAndTheNamesWithThePrefixStripped() + { + ServiceKeyStore.Set("imageGallery.pixabay", "pix"); + ServiceKeyStore.Set("imageGallery.other", "oth"); + // Outside the namespace, so it must not appear. + ServiceKeyStore.Set("openRouter", "or"); + + var reply = JObject.Parse( + ApiTest.GetString(_server, "serviceKeys/keys", "prefix=imageGallery.") + ); + + Assert.That((int)reply["version"], Is.EqualTo(1)); + Assert.That((string)reply["pixabay"], Is.EqualTo("pix")); + Assert.That((string)reply["other"], Is.EqualTo("oth")); + Assert.That(reply.Properties().Count(), Is.EqualTo(3), reply.ToString()); + } + + [Test] + public void PostNamespace_StoresWhatIsPostedAndRemovesWhatIsNot() + { + ServiceKeyStore.Set("imageGallery.pixabay", "old"); + ServiceKeyStore.Set("imageGallery.goneNow", "bye"); + // Outside the namespace: the post says nothing about it, so it must survive. + ServiceKeyStore.Set("openRouter", "or"); + + ApiTest.PostString( + _server, + "serviceKeys/keys?prefix=imageGallery.", + "{\"version\":1,\"pixabay\":\"new\"}", + ApiTest.ContentType.JSON + ); + + Assert.That(ServiceKeyStore.Get("imageGallery.pixabay"), Is.EqualTo("new")); + Assert.That(ServiceKeyStore.Get("imageGallery.goneNow"), Is.Null); + Assert.That(ServiceKeyStore.Get("openRouter"), Is.EqualTo("or")); + } + } +} From 9a2bebe7583a294b1fa01fb71b51b229d3bf5874 Mon Sep 17 00:00:00 2001 From: Hatton Date: Mon, 7 Sep 2026 10:20:53 -0600 Subject: [PATCH 08/11] Read the key file's format version, and keep what a newer Bloom put there Two ways an older Bloom could damage a file a newer one wrote, both found by walking through what happens the day we add a second protection method: a beta migrates the user's keys to method 2, then the user opens the release. The release could not lose the key itself -- Get treats an unknown method as absent, and the namespace endpoint skips any name CanRead refuses, so the entry survives and the beta still reads it. But two things around the key were being thrown away, because every write rewrites the whole file: The format version was written and never read. Save stamped kCurrentFormatVersion unconditionally, so an older Bloom silently told a newer one that its file was an older format. Load now reads the number and logs when the file is from a version it does not know, and Save never lowers it. Reading such a file still goes ahead: what decides whether a value can be decrypted is the method on the value itself, and a newer Bloom has to leave the methods it inherited readable. If a later format ever changes the shape of the file rather than adding to it, that log line is what tells us an older Bloom was looking at it -- and the check is the place a hard refusal to write would go. Properties this version has no field for were dropped. Newtonsoft discards them on deserialize, so an older Bloom storing one key would strip whatever a newer one had added, and the newer Bloom would find its own data gone with nothing to say why. StoreFile and StoredKey now both carry JsonExtensionData, so an unknown top-level property and an unknown field on a key both survive the trip. Three tests: a method-1 key in a version-2 file still reads, so the file version does not gate decryption; storing a key in a version-7 file leaves it saying 7; and storing a key preserves both an unknown top-level property and an unknown field on another key, with its value, leaving that key's method untouched. Also strips a UTF-8 BOM that an editing script had added to nine files that had none. Co-Authored-By: Claude Opus 5 (1M context) --- .../aiImageEditor/aiImageEditorOverlay.ts | 2 +- .../image-gallery/ImageGalleryDialog.tsx | 2 +- src/BloomExe/ApplicationContainer.cs | 2 +- src/BloomExe/Utils/ServiceKeyStore.cs | 32 +++++++- .../web/controllers/AiImageEditorApi.cs | 2 +- .../web/controllers/ImageGalleryApi.cs | 2 +- .../web/controllers/ServiceKeysApi.cs | 2 +- src/BloomTests/Utils/ServiceKeyStoreTests.cs | 73 ++++++++++++++++++- src/BloomTests/web/ServiceKeysApiTests.cs | 2 +- 9 files changed, 110 insertions(+), 9 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts index d205584a2cfe..0d297d692aa8 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts @@ -1,4 +1,4 @@ -// The AI Image Editor overlay and session — the TOP-WINDOW half of the feature. +// The AI Image Editor overlay and session — the TOP-WINDOW half of the feature. // // This runs in the workspace root, not the page iframe, for the same reason the image // gallery and the copyright/license dialog do (see the comments on those commands in diff --git a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx index 009be7ff4bac..87c15bb5f297 100644 --- a/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx +++ b/src/BloomBrowserUI/react_components/image-gallery/ImageGalleryDialog.tsx @@ -1,4 +1,4 @@ -import { css } from "@emotion/react"; +import { css } from "@emotion/react"; import { ImageGallery } from "bloom-image-gallery"; import type { IImage, diff --git a/src/BloomExe/ApplicationContainer.cs b/src/BloomExe/ApplicationContainer.cs index 03518b90d372..8057a82d9183 100644 --- a/src/BloomExe/ApplicationContainer.cs +++ b/src/BloomExe/ApplicationContainer.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Linq; using System.Reflection; using System.Windows.Forms; diff --git a/src/BloomExe/Utils/ServiceKeyStore.cs b/src/BloomExe/Utils/ServiceKeyStore.cs index 64bb0dd949f6..e530da174942 100644 --- a/src/BloomExe/Utils/ServiceKeyStore.cs +++ b/src/BloomExe/Utils/ServiceKeyStore.cs @@ -5,6 +5,7 @@ using System.Security.Cryptography; using System.Text; using Newtonsoft.Json; +using Newtonsoft.Json.Linq; using SIL.IO; using SIL.Reporting; @@ -315,6 +316,10 @@ private class StoredKey [JsonProperty("method")] public string Protection; + + /// See . + [JsonExtensionData] + public IDictionary Extra; } /// The whole file. @@ -325,6 +330,16 @@ private class StoreFile [JsonProperty("services")] public Dictionary Keys = new Dictionary(); + + /// + /// Anything in the file that this version of Bloom has no field for, kept so that + /// writing the file back does not throw it away. Every write is a whole-file + /// rewrite, so without this an older Bloom storing one key would quietly strip + /// whatever a newer one had added -- and the newer Bloom would find its own data + /// gone with nothing to say why. + /// + [JsonExtensionData] + public IDictionary Extra; } /// @@ -343,6 +358,18 @@ private static StoreFile Load() ); if (store?.Keys == null) return new StoreFile(); + if (store.Version > kCurrentFormatVersion) + { + // A newer Bloom wrote this. Reading goes ahead anyway, because what + // decides whether a value can be decrypted is the "method" on the value + // itself, and a newer Bloom has to leave the methods it inherited + // readable. Say so in the log, though: if a later format ever changes the + // shape of the file rather than adding to it, this line is what tells us + // an older Bloom was looking at it. + Logger.WriteEvent( + $"ServiceKeyStore: {FilePath} says it is format version {store.Version}, and this version of Bloom knows version {kCurrentFormatVersion}. Reading it anyway; each key's own method decides whether it can be read." + ); + } return store; } catch (Exception error) @@ -362,7 +389,10 @@ private static StoreFile Load() /// private static void Save(StoreFile store) { - store.Version = kCurrentFormatVersion; + // Never lower it: a file a newer Bloom wrote keeps saying so, because the number + // is how that Bloom will know its own format when it next reads the file. Raise a + // file with no version, or one from before the field existed, to ours. + store.Version = Math.Max(store.Version, kCurrentFormatVersion); RobustFile.WriteAllText( FilePath, JsonConvert.SerializeObject(store, Formatting.Indented) diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index eaab967a84bf..a13310f4554c 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Diagnostics; using System.IO; diff --git a/src/BloomExe/web/controllers/ImageGalleryApi.cs b/src/BloomExe/web/controllers/ImageGalleryApi.cs index 57990f2649f5..c22c7ef0434c 100644 --- a/src/BloomExe/web/controllers/ImageGalleryApi.cs +++ b/src/BloomExe/web/controllers/ImageGalleryApi.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.IO; using System.Linq; diff --git a/src/BloomExe/web/controllers/ServiceKeysApi.cs b/src/BloomExe/web/controllers/ServiceKeysApi.cs index 299cd5f5b84d..06c1165b22ab 100644 --- a/src/BloomExe/web/controllers/ServiceKeysApi.cs +++ b/src/BloomExe/web/controllers/ServiceKeysApi.cs @@ -1,4 +1,4 @@ -using System.Collections.Generic; +using System.Collections.Generic; using Bloom.Api; using Bloom.Utils; using Newtonsoft.Json; diff --git a/src/BloomTests/Utils/ServiceKeyStoreTests.cs b/src/BloomTests/Utils/ServiceKeyStoreTests.cs index f6789fd788b7..5390b80f2523 100644 --- a/src/BloomTests/Utils/ServiceKeyStoreTests.cs +++ b/src/BloomTests/Utils/ServiceKeyStoreTests.cs @@ -384,13 +384,84 @@ public void Unprotect_BlobMadeWithoutBloomEntropy_ReturnsNull() "setup: DPAPI itself must be able to read this blob" ); - Assert.That(ServiceKeyStore.Unprotect(Convert.ToBase64String(blobWithNoEntropy)), Is.Null); + Assert.That( + ServiceKeyStore.Unprotect(Convert.ToBase64String(blobWithNoEntropy)), + Is.Null + ); } /// /// Writes the file as given, so a test can set up content Bloom itself would not /// write. Single quotes stand in for double quotes, to keep the test strings readable. /// + [Test] + public void Get_FileFromANewerFormatVersion_StillReadsAKeyWhoseMethodIsKnown() + { + // What a Bloom that has moved the file on to format 2 leaves behind. The format + // version describes the file; whether a value can be decrypted is what the value's + // own "method" says, so a key this version wrote is still this version's to read. + var secret = "the-key"; + ServiceKeyStore.Set("someService", secret); + var written = RobustFile.ReadAllText(ServiceKeyStore.FilePath); + Assert.That( + written, + Does.Contain("\"version\": 1"), + "setup: the file should say version 1 before we age it forward" + ); + RobustFile.WriteAllText( + ServiceKeyStore.FilePath, + written.Replace("\"version\": 1", "\"version\": 2") + ); + + Assert.That(ServiceKeyStore.Get("someService"), Is.EqualTo(secret)); + } + + [Test] + public void Set_FileFromANewerFormatVersion_DoesNotLowerTheVersion() + { + WriteRawFile("{'version':7,'services':{}}"); + + ServiceKeyStore.Set("someService", "a key"); + + Assert.That( + RobustFile.ReadAllText(ServiceKeyStore.FilePath), + Does.Contain("\"version\": 7"), + "writing a key must not tell a newer Bloom its file is an older format" + ); + Assert.That( + ServiceKeyStore.Get("someService"), + Is.EqualTo("a key"), + "and the key must really have been stored" + ); + } + + [Test] + public void Set_KeepsWhatThisVersionHasNoFieldFor() + { + // Every write rewrites the whole file, so anything a newer Bloom added -- a + // property of its own, or a field on a key -- has to survive the trip. + WriteRawFile( + "{'version':2,'services':{'otherService':{'value':'abc','method':'2'," + + "'expires':'2027-01-01'}},'somethingNew':{'a':1}}" + ); + + ServiceKeyStore.Set("someService", "a key"); + + var fileText = RobustFile.ReadAllText(ServiceKeyStore.FilePath); + Assert.That(fileText, Does.Contain("somethingNew"), "a whole property went missing"); + Assert.That(fileText, Does.Contain("expires"), "a field on a key went missing"); + Assert.That( + fileText, + Does.Contain("2027-01-01"), + "the field survived in name only, without its value" + ); + Assert.That( + ServiceKeyStore.GetProtectionMethod("otherService"), + Is.EqualTo("2"), + "the untouched key's own method must be exactly as it was" + ); + } + private void WriteRawFile(string contentWithSingleQuotes) { RobustFile.WriteAllText( diff --git a/src/BloomTests/web/ServiceKeysApiTests.cs b/src/BloomTests/web/ServiceKeysApiTests.cs index 01cdc538a40b..22064a2b79e2 100644 --- a/src/BloomTests/web/ServiceKeysApiTests.cs +++ b/src/BloomTests/web/ServiceKeysApiTests.cs @@ -1,4 +1,4 @@ -using System.Linq; +using System.Linq; using System.Threading; using Bloom.Api; using Bloom.Book; From 6267a9c4b604386d5680a4c36fb51a35c924667c Mon Sep 17 00:00:00 2001 From: Hatton Date: Wed, 9 Sep 2026 17:11:38 -0600 Subject: [PATCH 09/11] some review fixes --- .../bookEdit/aiImageEditor/aiImageEditorOverlay.ts | 3 ++- src/BloomExe/Utils/ServiceKeyStore.cs | 7 ++++--- src/BloomExe/web/controllers/ServiceKeysApi.cs | 13 +++++++------ src/BloomTests/web/ServiceKeysApiTests.cs | 10 ++++------ 4 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts index 0d297d692aa8..51e1169aee0b 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts @@ -635,8 +635,9 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { // Bloom owns the OpenRouter API key. A key the user pastes into the // AI Image Editor is handed up here so Bloom persists it per-user (and // supplies it on the next launch). A null apiKey clears the stored key. + // The name must match ServiceKeyStore.kOpenRouterName. postString( - "serviceKeys/key?name=openRouter", + "serviceKeys/key?name=OR", data.payload?.apiKey ?? "", ); break; diff --git a/src/BloomExe/Utils/ServiceKeyStore.cs b/src/BloomExe/Utils/ServiceKeyStore.cs index e530da174942..f36914da980d 100644 --- a/src/BloomExe/Utils/ServiceKeyStore.cs +++ b/src/BloomExe/Utils/ServiceKeyStore.cs @@ -48,7 +48,7 @@ namespace Bloom.Utils /// a scheme that had to stay secret was never on offer. /// /// The format is - /// { "version": 1, "services": { "openRouter": { "value": "<base64>", "method": "1" } } }. + /// { "version": 1, "services": { "OR": { "value": "<base64>", "method": "1" } } }. /// /// Nothing here knows what any key is for. A caller picks a name and owns its meaning, /// so a new service needs no change to this class. @@ -126,9 +126,10 @@ public static class ServiceKeyStore /// /// The name under which the user's OpenRouter API key is stored (the "Edit with AI" - /// feature). + /// feature). It is deliberately terse: the name goes into the file, where the less it + /// says the better (see the class comment). /// - public const string kOpenRouterName = "openRouter"; + public const string kOpenRouterName = "OR"; /// /// The start of the name of every image gallery provider key, for example diff --git a/src/BloomExe/web/controllers/ServiceKeysApi.cs b/src/BloomExe/web/controllers/ServiceKeysApi.cs index 06c1165b22ab..643e320949d3 100644 --- a/src/BloomExe/web/controllers/ServiceKeysApi.cs +++ b/src/BloomExe/web/controllers/ServiceKeysApi.cs @@ -14,8 +14,8 @@ namespace Bloom.web.controllers /// /// Two endpoints, because there are two shapes of caller: /// - /// serviceKeys/key handles ONE key by its full name, and its value is the bare string. That - /// suits a feature with a single key, such as "Edit with AI" (name "openRouter"). + /// serviceKeys/key handles ONE key by its full name, and its value is the bare string in + /// both directions. That suits a feature with a single key, such as "Edit with AI". /// /// serviceKeys/keys handles a whole NAMESPACE at once, named by a prefix, as one flat JSON /// object of short name to key plus a "version" property. That suits the image gallery, @@ -47,9 +47,10 @@ public void RegisterWithApiHandler(BloomApiHandler apiHandler) } /// - /// GET returns the one key named by the "name" parameter as a JSON string, or null - /// when there is none, or when there is one this computer cannot decrypt -- which the - /// caller should treat the same way, by asking the user for the key again. + /// GET returns the one key named by the "name" parameter as plain text, the same bare + /// string a POST sends. The body is empty when there is no such key, and also when + /// there is one this computer cannot decrypt -- which the caller should treat the same + /// way, by asking the user for the key again. /// POST stores the posted body as that key's value; an empty body removes it. /// private void HandleKey(ApiRequest request) @@ -57,7 +58,7 @@ private void HandleKey(ApiRequest request) var name = request.RequiredParam("name"); if (request.HttpMethod == HttpMethods.Get) { - request.ReplyWithJson(JsonConvert.SerializeObject(ServiceKeyStore.Get(name))); + request.ReplyWithText(ServiceKeyStore.Get(name) ?? ""); return; } // A key can hold any character a service cares to use, "+" and "%" among them, so diff --git a/src/BloomTests/web/ServiceKeysApiTests.cs b/src/BloomTests/web/ServiceKeysApiTests.cs index 22064a2b79e2..62b0559794ed 100644 --- a/src/BloomTests/web/ServiceKeysApiTests.cs +++ b/src/BloomTests/web/ServiceKeysApiTests.cs @@ -48,12 +48,9 @@ public void TearDown() } [Test] - public void GetKey_WhenThereIsNone_RepliesNull() + public void GetKey_WhenThereIsNone_RepliesWithNothing() { - Assert.That( - ApiTest.GetString(_server, "serviceKeys/key", "name=openRouter"), - Is.EqualTo("null") - ); + Assert.That(ApiTest.GetString(_server, "serviceKeys/key", "name=openRouter"), Is.Empty); } [Test] @@ -77,7 +74,8 @@ public void PostKey_ThenGet_RoundTripsCharactersUrlEscapingWouldChange() ); Assert.That( ApiTest.GetString(_server, "serviceKeys/key", "name=openRouter"), - Is.EqualTo(JToken.FromObject(key).ToString(Newtonsoft.Json.Formatting.None)) + Is.EqualTo(key), + "the GET must give back the bare string that was posted" ); } From 976405874eada00b38c93c7c6007b9dcfe79402c Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 10 Sep 2026 15:13:47 -0600 Subject: [PATCH 10/11] Lock the AI image editor to look-around mode without a subscription (BL-16820) The AI image editor now opens in look-around mode whenever the collection's subscription does not cover AI image editing, or the book is based on the Playground template. In that mode Bloom withholds the stored OpenRouter key altogether and tells the editor (playgroundMode in the launch payload) to disable every tool run, AI or not, so an expired subscription stops the spending rather than only stopping the entry of a new subscription code. Editor package moves to dist-v0.1.13, which carries the look-around dialog ("This tool is in "look-around" mode..."), the disabling of every run including the browser-only PDF to Images and Remove Background, and no offer to connect an OpenRouter account. Co-Authored-By: Claude Opus 5 (1M context) --- .../aiImageEditor/aiImageEditorOverlay.ts | 9 ++++--- src/BloomBrowserUI/package.json | 2 +- src/BloomBrowserUI/pnpm-lock.yaml | 14 +++++----- .../web/controllers/AiImageEditorApi.cs | 27 ++++++++++++++----- 4 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts index 51e1169aee0b..a1f959b8204c 100644 --- a/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts +++ b/src/BloomBrowserUI/bookEdit/aiImageEditor/aiImageEditorOverlay.ts @@ -117,10 +117,11 @@ export function openAiImageEditor(target: IAiImageEditorTarget): void { metadata?: Record | null; }>; apiKey?: string | null; - // Playground/demo context: the AI Image Editor must disable its - // "set OpenRouter API key" UI. Rides through the `...launchData` - // spread below into the AI Image Editor's init payload. - demoOnly?: boolean; + // Set when the subscription does not cover AI image editing: the AI + // Image Editor opens to be looked at, with every run that would reach + // OpenRouter disabled. Rides through the `...launchData` spread below + // into the AI Image Editor's init payload. + playgroundMode?: boolean; }; const hostWindow = window as Window & { __bloomAiImageEditorCleanup?: () => void; diff --git a/src/BloomBrowserUI/package.json b/src/BloomBrowserUI/package.json index a54eac31f5f4..db03e6d31f9e 100644 --- a/src/BloomBrowserUI/package.json +++ b/src/BloomBrowserUI/package.json @@ -145,7 +145,7 @@ "@types/react-transition-group": "4.4.1", "@use-it/event-listener": "0.1.7", "axios": "0.21.1", - "bloom-ai-image-tools": "github:BloomBooks/bloom-ai-image-tools#dist-v0.1.7", + "bloom-ai-image-tools": "github:BloomBooks/bloom-ai-image-tools#dist-v0.1.13", "bloom-image-gallery": "github:BloomBooks/bloom-image-gallery#e376463bcd21b1558750570b63269a2e133b46c0", "bloom-player": "2.20.2", "calculate-aspect-ratio": "0.1.3", diff --git a/src/BloomBrowserUI/pnpm-lock.yaml b/src/BloomBrowserUI/pnpm-lock.yaml index a6da3b485317..14333d287fd9 100644 --- a/src/BloomBrowserUI/pnpm-lock.yaml +++ b/src/BloomBrowserUI/pnpm-lock.yaml @@ -85,8 +85,8 @@ importers: specifier: 0.21.1 version: 0.21.1 bloom-ai-image-tools: - specifier: github:BloomBooks/bloom-ai-image-tools#dist-v0.1.7 - version: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/a24e636bd05a943b1b909811210c9803be091494 + specifier: github:BloomBooks/bloom-ai-image-tools#dist-v0.1.13 + version: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/f4e150be295d908b54debc74c0e21e21cfa2c813 bloom-image-gallery: specifier: github:BloomBooks/bloom-image-gallery#e376463bcd21b1558750570b63269a2e133b46c0 version: https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/e376463bcd21b1558750570b63269a2e133b46c0(@types/react@18.3.31)(supports-color@5.5.0) @@ -4049,14 +4049,14 @@ packages: integrity: sha512-p2q/t/mhvuOj/UeLlV6566GD/guowlr0hHxClI0W9m7MWYkL1F0hLo+0Aexs9HSPCtR1SXQ0TD3MMKrXZajbiQ==, } - bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/a24e636bd05a943b1b909811210c9803be091494: + bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/f4e150be295d908b54debc74c0e21e21cfa2c813: resolution: { gitHosted: true, - integrity: sha512-F67/eh54qujL/f1XtFVfJSwiMeiVr2OsRZHpnlyfhh49g7drMaXGs0cjTBhhDYVMiA+O4UFaACN58wrFMTGyCA==, - tarball: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/a24e636bd05a943b1b909811210c9803be091494, + integrity: sha512-oA/ESzkmYjrYqty3uxAUcxMBKyUW4f3PFkSeZVJkXwhKdCripsQfwIGnZ9o1/lzU1/fF4yoqeEQcd36vyp7eDg==, + tarball: https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/f4e150be295d908b54debc74c0e21e21cfa2c813, } - version: 0.1.7 + version: 0.1.13 bloom-image-gallery@https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/e376463bcd21b1558750570b63269a2e133b46c0: resolution: @@ -13532,7 +13532,7 @@ snapshots: file-uri-to-path: 1.0.0 optional: true - bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/a24e636bd05a943b1b909811210c9803be091494: + bloom-ai-image-tools@https://codeload.github.com/BloomBooks/bloom-ai-image-tools/tar.gz/f4e150be295d908b54debc74c0e21e21cfa2c813: {} bloom-image-gallery@https://codeload.github.com/BloomBooks/bloom-image-gallery/tar.gz/e376463bcd21b1558750570b63269a2e133b46c0(@types/react@18.3.31)(supports-color@5.5.0): diff --git a/src/BloomExe/web/controllers/AiImageEditorApi.cs b/src/BloomExe/web/controllers/AiImageEditorApi.cs index a13310f4554c..3b9d8304b1bd 100644 --- a/src/BloomExe/web/controllers/AiImageEditorApi.cs +++ b/src/BloomExe/web/controllers/AiImageEditorApi.cs @@ -10,6 +10,7 @@ using Bloom.Edit; using Bloom.ImageProcessing; using Bloom.SafeXml; +using Bloom.SubscriptionAndFeatures; using Bloom.Utils; using L10NSharp; using Newtonsoft.Json; @@ -472,6 +473,17 @@ private void HandleLaunch(ApiRequest request) var httpBase = $"{BloomServer.ServerUrlWithBloomPrefixEndingInSlash}api/aiImageEditor"; + // Whether this collection's subscription actually covers AI image editing. The + // book is deliberately left out of the question: a Playground book counts as + // Enterprise for every feature, which is what opens the editor there at all. + var subscriptionCoversAiImageEditing = FeatureStatus + .GetFeatureStatus(book.CollectionSettings.Subscription, FeatureName.AiImageEditing) + .Enabled; + + // A Playground book is a place to look around, and so is a collection whose + // subscription does not cover AI image editing. + var playgroundMode = !subscriptionCoversAiImageEditing || book.IsPlayground; + // Return the data the JS needs to create the iframe overlay. The AI image editor // runs in iframe mode and gets its `init` from the overlay JS (which builds it // from this reply and posts it to the iframe), so the whole-book image list must @@ -491,12 +503,15 @@ private void HandleLaunch(ApiRequest request) // Bloom owns the OpenRouter key: supply the per-user stored key so the AI // image editor doesn't have to ask for it again. It hands any newly // obtained key back to Bloom via serviceKeys/key (see ServiceKeysApi). - apiKey = ServiceKeyStore.Get(ServiceKeyStore.kOpenRouterName), - // In a Playground template book all features are unlocked for - // "try it out", so the AI image editor opens — but it's a shared demo - // context, so it must not let the user set/save an OpenRouter API key. - // The AI image editor disables its credential UI when this is true. - demoOnly = book.IsPlayground, + // Nothing that costs money can be run in playground mode, so there the + // key stays here. + apiKey = playgroundMode + ? null + : ServiceKeyStore.Get(ServiceKeyStore.kOpenRouterName), + // In playground mode the editor shows its tools but disables every one + // whose run would reach OpenRouter, and offers no way to connect an + // account. + playgroundMode, // Let the AI image editor reveal its developer/tester tools (e.g. the // "Local Dummy (No AI)" model, for cost-free testing). The AI image // editor hides those tools unless the host opts in, so ordinary From 0ddd42c9e36849f47686679ecf378b3e76bb2c03 Mon Sep 17 00:00:00 2001 From: Hatton Date: Thu, 10 Sep 2026 15:37:06 -0600 Subject: [PATCH 11/11] Put the raw-file helper's comment back on the helper The summary describing WriteRawFile -- that it writes the file as given so a test can set up content Bloom itself would not write -- had drifted above the newer-format-version test, which left the helper undocumented and described the test as something it is not. Co-Authored-By: Claude Opus 5 (1M context) --- src/BloomTests/Utils/ServiceKeyStoreTests.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/BloomTests/Utils/ServiceKeyStoreTests.cs b/src/BloomTests/Utils/ServiceKeyStoreTests.cs index 5390b80f2523..f9c674d5e260 100644 --- a/src/BloomTests/Utils/ServiceKeyStoreTests.cs +++ b/src/BloomTests/Utils/ServiceKeyStoreTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.IO; using System.Linq; using System.Security.Cryptography; @@ -390,10 +390,6 @@ public void Unprotect_BlobMadeWithoutBloomEntropy_ReturnsNull() ); } - /// - /// Writes the file as given, so a test can set up content Bloom itself would not - /// write. Single quotes stand in for double quotes, to keep the test strings readable. - /// [Test] public void Get_FileFromANewerFormatVersion_StillReadsAKeyWhoseMethodIsKnown() { @@ -462,6 +458,10 @@ public void Set_KeepsWhatThisVersionHasNoFieldFor() ); } + /// + /// Writes the file as given, so a test can set up content Bloom itself would not + /// write. Single quotes stand in for double quotes, to keep the test strings readable. + /// private void WriteRawFile(string contentWithSingleQuotes) { RobustFile.WriteAllText(