Keep the user's API keys in one per-user file, not in per-channel settings (BL-16820) - #8316
Keep the user's API keys in one per-user file, not in per-channel settings (BL-16820)#8316hatton wants to merge 12 commits into
Conversation
…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>
|
[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin on 2026-09-07 up to commit 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 No informational flags. CI ( |
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>
|
[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin again on 2026-09-07 up to commit One new bug, now fixed in A third pass over |
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.
|
[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin (job Six findings. One was new and real, and is fixed in The other five repeat findings already answered and resolved on this PR. Devin re-reviewed the whole diff rather than the new commit ( |
.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>
|
[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin (job Six findings, none of them new and none actionable. Five describe code this commit deleted or replaced, and they name it:
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 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 |
…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>
|
[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin (job 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:
Gates on this commit: C# suite 3354 passed / 13 skipped; front-end typecheck and eslint clean; CI |
hatton
left a comment
There was a problem hiding this comment.
@hatton partially reviewed 12 files and all commit messages.
Reviewable status: 0 of 13 files reviewed, 8 unresolved discussions.
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson partially reviewed 13 files and all commit messages, made 3 comments, and resolved 8 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on hatton).
src/BloomExe/web/controllers/AiImageEditorApi.cs line 498 at r5 (raw file):
// "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.
I'm not getting this. The credentials seem to be saved per user login per machine (and locked to Bloom by the fixed entropy), but no other context seems to be involved. Why shouldn't a user enter some real credentials and do some real image manipulation to help decide whether the capability is useful enough to subscribe for?
Of course that would let them make some real images which they might then be able to use in a real book, getting around our subscription requirement...but the comment does not read as if that's what we're trying to prevent.
src/BloomExe/web/controllers/ServiceKeysApi.cs line 62 at r5 (raw file):
request.ReplyWithJson(JsonConvert.SerializeObject(ServiceKeyStore.Get(name))); return; }
Seems strange that GET uses Json, while POST does not...and contradicts the claim in the class summary that this API uses plain strings.
src/BloomExe/Utils/ServiceKeyStore.cs line 131 at r5 (raw file):
/// feature). /// </summary> public const string kOpenRouterName = "openRouter";
openRouter sounds like the sort of string a credential-stealer might know about, and I think this is going to be stored (albeit encrypted) in the file, where we've tried to make other names unhelpful. Do we want something less recognizable?
…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) <noreply@anthropic.com>
hatton
left a comment
There was a problem hiding this comment.
@hatton+JHAI made 3 comments.
Reviewable status: 6 of 15 files reviewed, 3 unresolved discussions (waiting on JohnThomson).
src/BloomExe/web/controllers/AiImageEditorApi.cs line 498 at r5 (raw file):
[Claude Opus 5 from Hatton's machine during preflight] You read it right, and the comment was describing the wrong thing. What we actually want is what your last sentence names: no real image generation without a subscription. Preventing the entry of a new subscription code is not enough, because a key already on the machine keeps working after a subscription expires.
So the gate is now about that, and the flag is named for what it is. The editor opens in look-around mode when either the collection's subscription does not cover AI image editing, or the book is based on the Playground template:
var playgroundMode = !subscriptionCoversAiImageEditing || book.IsPlayground;In that mode Bloom does not send the stored key at all, and sets playgroundMode in the launch payload (it was demoOnly). The editor then shows everything but runs nothing: every tool's action button is disabled with "Not available in look-around mode" — including the browser-only "PDF to Images" and "Remove Background", since the point is "you can look but not do" rather than "you can't spend money" — and one dialog on opening says what it would take:
This tool is in "look-around" mode. To actually edit or create images, you will need a subscription and a book that is not based on the "Playground" template.
The offer to connect an OpenRouter account is left out entirely there, since the session could not use one.
The subscription check deliberately omits the book, because FeatureStatus promotes a Playground book to Enterprise for every feature — which is what opens the editor there at all. The editor side of this is bloom-ai-image-tools dist-v0.1.13.
src/BloomExe/web/controllers/ServiceKeysApi.cs line 62 at r5 (raw file):
Previously, JohnThomson (John Thomson) wrote…
Seems strange that GET uses Json, while POST does not...and contradicts the claim in the class summary that this API uses plain strings.
[Claude Opus 5 from Hatton's machine during preflight] Agreed, and fixed: GET serviceKeys/key now replies with ReplyWithText, so the value is the bare string in both directions, as the class summary claims. No caller had to change — nothing in the front end was GETting that endpoint. The tests cover it: an empty body when there is no key, and a round trip that asserts the bare key.
The namespace endpoint (serviceKeys/keys?prefix=) still uses JSON, which it has to — it carries a whole object of name-to-key plus the format version.
src/BloomExe/Utils/ServiceKeyStore.cs line 131 at r5 (raw file):
Previously, JohnThomson (John Thomson) wrote…
openRouter sounds like the sort of string a credential-stealer might know about, and I think this is going to be stored (albeit encrypted) in the file, where we've tried to make other names unhelpful. Do we want something less recognizable?
[Claude Opus 5 from Hatton's machine during preflight] Agreed, and done: the name is now OR (ServiceKeyStore.kOpenRouterName), with a doc line saying it is deliberately terse because the name goes into the file. The front end posts serviceKeys/key?name=OR, and the format example in the class comment was updated to match.
Worth knowing: any key stored under the old openRouter name while testing this branch is orphaned rather than migrated, so it stays in services.blm and the user is asked for the key once more.
There was a problem hiding this comment.
[Claude Opus 5 from Hatton's machine during devin-review] Devin (Bug): existing service keys disappear on upgrade. The keys the user had already entered were kept in Bloom's per-channel user settings (ImageGalleryProviderKeys and OpenRouterApiKey). This PR removes both settings and stores keys in the new per-user file instead, with no migration, so a user who upgrades finds their picture-search and OpenRouter keys gone and has to fetch and paste them again.
Agreed that this is what happens. Whether it is acceptable, or whether the old settings should be read once and copied into the new file, is a user-facing call, so it is going to the developer rather than being decided here. Leaving this thread open until it comes back.
|
[Claude Opus 5 from Hatton's machine during devin-review] Consulted Devin for the AI image editor / service key work, up to It raised 8 bugs and 8 things to investigate (no informational notes). Five of the bugs it had already marked resolved — the earlier rounds where a failed encryption reported a successful save, an unreadable key was deleted, an older Bloom deleted a newer one's keys, and a key with a Everything current has its own thread above. In short:
CI and the local gates are green. Devin re-reviewed the last commit from scratch and returned exactly the same set, so there is nothing further outstanding from it beyond the open thread above. |
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) <noreply@anthropic.com>
hatton
left a comment
There was a problem hiding this comment.
@hatton partially reviewed 11 files and all commit messages, and made 2 comments.
Reviewable status: 5 of 15 files reviewed, 11 unresolved discussions (waiting on JohnThomson).
src/BloomExe/web/controllers/AiImageEditorApi.cs line 498 at r5 (raw file):
Previously, hatton+JHAI (Agent JHAI for John Hatton) wrote…
[Claude Opus 5 from Hatton's machine during preflight] You read it right, and the comment was describing the wrong thing. What we actually want is what your last sentence names: no real image generation without a subscription. Preventing the entry of a new subscription code is not enough, because a key already on the machine keeps working after a subscription expires.
So the gate is now about that, and the flag is named for what it is. The editor opens in look-around mode when either the collection's subscription does not cover AI image editing, or the book is based on the Playground template:
var playgroundMode = !subscriptionCoversAiImageEditing || book.IsPlayground;In that mode Bloom does not send the stored key at all, and sets
playgroundModein the launch payload (it wasdemoOnly). The editor then shows everything but runs nothing: every tool's action button is disabled with "Not available in look-around mode" — including the browser-only "PDF to Images" and "Remove Background", since the point is "you can look but not do" rather than "you can't spend money" — and one dialog on opening says what it would take:This tool is in "look-around" mode. To actually edit or create images, you will need a subscription and a book that is not based on the "Playground" template.
The offer to connect an OpenRouter account is left out entirely there, since the session could not use one.
The subscription check deliberately omits the book, because
FeatureStatuspromotes a Playground book to Enterprise for every feature — which is what opens the editor there at all. The editor side of this isbloom-ai-image-toolsdist-v0.1.13.
Done.
src/BloomExe/Utils/ServiceKeyStore.cs line 131 at r5 (raw file):
Previously, hatton+JHAI (Agent JHAI for John Hatton) wrote…
[Claude Opus 5 from Hatton's machine during preflight] Agreed, and done: the name is now
OR(ServiceKeyStore.kOpenRouterName), with a doc line saying it is deliberately terse because the name goes into the file. The front end postsserviceKeys/key?name=OR, and the format example in the class comment was updated to match.Worth knowing: any key stored under the old
openRoutername while testing this branch is orphaned rather than migrated, so it stays inservices.blmand the user is asked for the key once more.
Done.
Problem
Pixabay keys were not remembered between channels, because each channel kept them in its own settings. And once a key was stored, an expired subscription did not stop it being used: Bloom refused a new subscription code but the AI image editor kept working with the key already on the machine.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16820
Fix
ServiceKeyStoreis 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, and the OpenRouter key is stored under the terse nameOR. Nothing in it knows what any key is for, so a new service needs no change to it.ServiceKeysApiis 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, as the bare string in both directions;serviceKeys/keys?prefix=handles a whole namespace at once, in the flat shape the image gallery already uses.imageGallery/providerKeysandaiImageEditor/saveCredentials. The gallery keeps one key per search provider under theimageGallery.prefix. No migration was needed — only the door moved, not the store.playgroundModein the launch payload; the editor then shows everything but runs nothing — every tool is disabled, including the browser-only "PDF to Images" and "Remove Background" — says why in one dialog, and offers no way to connect an account.aiImageEditor/saveCredentialsalso drops its session-token check and its refusal to persist a key during a Playground session; the look-around gate above is what covers that case now, and every endpoint that can reach a book keeps its session gate.dist-v0.1.13, which carries the look-around behavior above.Devin review
This change is