Keep the user's API keys in one per-user file, not in per-channel settings (BL-16820) by hatton · Pull Request #8316 · BloomBooks/BloomDesktop

hatton · 2026-09-07T13:57:07Z

Problem

Original problem: pixabay keys not remembered between channels. While solving this, we went ahead and started to have Windows encrypt these and other keys. Unfortunately not portable when upgrading machines, but it's a start.

Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16820

Fix

ServiceKeyStore is the one place a key the user fetched from a service's own site is kept: %LocalAppData%\SIL\Bloom\services.blm, which is channel- and version-independent, so a key entered in one channel is there in the next. Each value is DPAPI-protected in CurrentUser scope with our own entropy, so the file is useless to another account or machine (and a key does not travel to a new computer — the user is asked for it again). The file deliberately says nothing about itself: no giveaway filename or property names. Nothing in it knows what any key is for, so a new service needs no change to it.
ServiceKeysApi is the front end's one door to it, registered application-wide rather than per collection, because a key belongs to the Windows user and outlives any collection. serviceKeys/key?name= handles a single key; serviceKeys/keys?prefix= handles a whole namespace at once, in the flat shape the image gallery already uses.
The two old endpoints are gone: imageGallery/providerKeys and aiImageEditor/saveCredentials. The gallery keeps one key per search provider under the imageGallery. prefix; "Edit with AI" stores openRouter. No migration was needed — only the door moved, not the store.
Dropping aiImageEditor/saveCredentials also drops its session-token check and its 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 endpoint that can reach a book keeps its session gate.

Devin review

This change is

…tings (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\<product>\<version>\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) <noreply@anthropic.com>

… (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) <noreply@anthropic.com>

hatton · 2026-09-07T14:09:06Z

[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin on 2026-09-07 up to commit 8eb3e6f7bc.

It raised four bugs and two investigate flags, each mirrored above as its own thread with what we did about it. Four were real and are fixed in 9438377cf1: a key containing + or a percent escape was stored altered; a key this version of Bloom cannot read was deleted instead of left alone; a failed write reported success; and the gallery dialog now uses the useMountEffect helper the front-end guidance asks for. Two we assessed as not issues, with the reasoning on their threads: the localhost API server is not an authenticated boundary and this endpoint is no more open than the one it replaces, and the single-process lock is a documented, accepted limit.

No informational flags. CI (pr-automation) is green, and CodeRabbit auto-review is off for this repo by .coderabbit.yml. A re-review of 9438377cf1 is running.

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) <noreply@anthropic.com>

hatton · 2026-09-07T14:13:35Z

[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin again on 2026-09-07 up to commit 9cb3199dd9.

One new bug, now fixed in 9cb3199dd9 and recorded on its own thread: a key belonging to another Windows account or another computer was deleted as though the user had cleared it, because CanRead only checked that the protection method was one we recognize.

A third pass over 9cb3199dd9 produced no new findings. It re-reported all seven earlier items because it re-reviewed the whole diff from scratch rather than incrementally (incremental.detected: false); each was checked against the reviewed commit and is either already fixed there or already answered on its thread. CanRead, for instance, reads Get(name) != null at that very commit, which is the fix the repeat asks for.

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.

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.

hatton · 2026-09-07T14:47:23Z

{"body":"[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin (job pr-review-job-13787a48b30a) up to 353af0d14a, after the commit that renames the file to services.bloom and adds DPAPI entropy.\n\nSix findings. One was new and real, and is fixed in f8406aa1ec: Set returned silently when Protect failed, so an endpoint reported a save that had not happened — mirrored and answered at https://github.com/BloomBooks/BloomDesktop/pull/8316#discussion_r3950773792.\n\nThe other five repeat findings already answered and resolved on this PR. Devin re-reviewed the whole diff rather than the new commit (incremental.detected was not set), so it re-raised them against code that already carries the fix; each was screened against the file at 353af0d14a before being set aside. One of them still names UserKeys.json, a file that no longer exists on this branch, which is the clearest sign of the stale pass."}

.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.

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=<name> one key, its value the bare string. serviceKeys/keys?prefix=<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) <noreply@anthropic.com>

hatton · 2026-09-07T16:06:45Z

[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin (job pr-review-job-c2ff776b83de) up to 548ace78c5, the commit that moves the key endpoints into ServiceKeysApi and renames the store.

Six findings, none of them new and none actionable. Five describe code this commit deleted or replaced, and they name it: UserKeyStore.cs and UserKeys.json (renamed to ServiceKeyStore.cs / services.blm), and ImageGalleryApi.HandleProviderKeys (gone). Each was screened against the file at 548ace78c5:

Encryption failures report successful saves and Failed key saves report success — Protect and Save both throw now; neither has a try/catch. Fixed in f8406aa1ec.
Unreadable keys are deleted — CanRead is Get(name) != null, so a key this account cannot decrypt reads as unreadable and is left alone.
Older versions delete newer protected keys — the namespace POST skips any name CanRead says no to, for exactly this reason.
Valid API keys are altered — the namespace endpoint now reads its body with RequiredPostJson, which preserves the payload byte for byte, and the single-key endpoint reads with unescape: false. ServiceKeysApiTests round-trips a key holding +, a space and %2B.

The sixth, Web pages can steal API keys, is the localhost-API-boundary point already answered and resolved earlier on this PR. It is true of Bloom's whole localhost API and the class comment says so in as many words. Worth noting explicitly, though, because this commit does change it: removing aiImageEditor/saveCredentials removed a per-launch session token check that the general endpoint cannot reproduce, and that was a deliberate call recorded in the commit message.

Devin again re-reviewed the whole diff rather than the new commit, which is why every item is a repeat.

Gates on this commit: C# suite 3351 passed / 13 skipped; front-end typecheck, lint and Vitest green; the isolated production bundle builds; CI pr-automation green.

…here 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) <noreply@anthropic.com>

hatton · 2026-09-07T16:24:26Z

[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin (job pr-review-job-0d6f95ced167) up to 9a2bebe758, the commit that makes the key file's format version meaningful and keeps unknown properties through a rewrite.

The same six findings as the previous pass, word for word, and nothing new. Two of them are the very scenario this commit came from — an older Bloom meeting a file a newer Bloom wrote — so worth saying where that now stands:

Unreadable keys are deleted / Older versions delete newer protected keys — still not what happens. CanRead is Get(name) != null, so a key this account or this version cannot decrypt reads as unreadable, and the namespace POST skips any name CanRead refuses. Walking through it (beta migrates to a method 2, user opens the release) turned up two real gaps around the key rather than in it, and both are fixed here: Save no longer stamps the format version down to 1, and StoreFile/StoredKey carry [JsonExtensionData] so an unknown top-level property or an unknown field on someone else's key survives an older Bloom's whole-file rewrite. Three tests cover it.
The rest name files this branch renamed or deleted (UserKeyStore.cs, UserKeys.json, ImageGalleryApi.HandleProviderKeys), and were screened against the code at 9a2bebe758 as before. Web pages can steal API keys is the localhost-API-boundary point already answered and resolved on this PR.

Gates on this commit: C# suite 3354 passed / 13 skipped; front-end typecheck and eslint clean; CI pr-automation green.

hatton

@hatton partially reviewed 12 files and all commit messages.
Reviewable status: 0 of 13 files reviewed, 8 unresolved discussions.

hatton and others added 2 commits September 7, 2026 07:55