Repository navigation
fix(extension): security review fixes for the background and settings - #31
Merged
Merged
Conversation
…epended on the date
Two findings from the pre-release review (slice 54, C-08).
validateWordsResponse turned the content type into a string with
String(), which throws for an object whose toString can't be called.
The property test found it ({contentType: {toString: false}}, seed
504574479). fetch always gives a string, so nobody could reach this,
but the check now takes only a string and treats anything else as no
content type. A unit test pins the counterexample.
The DOM test "a word on the never-swap list stays, on every page, until
it is taken off" failed the rc.1 release build. It wasn't timing: the
language a word shows in is drawn per page per day (slice 18), and
"thanks" has three. The test expected Russian, which the draw picks on
some days only (Mandarin on 7 October, the release day). The test now
uses the Russian words only, so it no longer depends on the date;
pinned to each day from 1 to 30 October, it fails on 18 of them before
and passes on all after.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Root cause of the welcome test that flaked under load (slice 54, C-08: test/dom/welcome.test.mjs got ['es', 'en'] where it expected ['es']). The projector, which writes the words pages read, read the learner's bases, then waited for the store's word list, then wrote the words with those bases. A change of bases while it waited (unticking a language on the welcome page) was written over with the old list, in the trusted copy as well as the pages' copy, until the next projection put it right 100 ms later. A slow IndexedDB read under load made that window wide enough for the test to see it; a worker that stopped in between would have kept the old list. - The trusted copy gets update(fn): fn reads and returns what to write, and no other write runs in between. The projector reads the bases, the list and the version inside one update. - A page's change of bases writes `ui` and the pages' `baseLangs` in one write, so the projector never sees one changed and not the other. The first install and the upgrade do the same. A test in test/bg/settings.test.mjs holds the store's list while a page changes the bases; it got ['es', 'en'] before and ['es'] after. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Slice 54, A-08. The `sync` message took `force: true` from content scripts too, which skips the 5 s wait between page-load syncs. Code in a content script's world could make the learner's server send its whole word list in a tight loop (the reviewer counted 20 syncs in 61 ms). `force` now counts only from Kotiko's own pages (the popup, the dashboard); a content script's sync waits like a page load's. A test sends 20 forced syncs from a content script: 20 requests before, none after, and the popup and an extension tab still sync at once. The other sync tests now send their forced syncs from the popup, as the real popup does. The `requested` helper in the same file waited at most about a second for the worker's first request, which the full suite on a busy machine can exceed; it now waits up to 10 s for the condition. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Slice 54, A-09. Both release zips shipped `globalThis.__kotiko`, the hook the tests drive the background through (`seed` writes the trusted settings, `toServer`, `getStore` and more). No page or content script can reach the worker's globals, but it is test code in the product. The hook now sits between `// test-only: start` and `// test-only: end` lines, and scripts/build-extension.mjs leaves every such block out of the zips. The unpacked extension keeps it, so the unit, e2e and screenshot tests work as before. The build stops on a block left open or a `__kotiko` outside one, so the hook can't slip back in. Tests: the real extension's zips have no `__kotiko` in background.js and keep the rest of it (failed before: the hook was there); a fixture checks the cut and both refusals. A real build with web-ext lint passes with no warnings. docs/reproducible-builds.md says what is left out. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
…kens Slice 54, A-07. Lookups sent `max_tokens` only when OpenRouter's catalogue said the model takes it, so lookups to OpenAI, Anthropic, Google Gemini, Groq, a local server or a custom address had no output cap. A model stuck repeating itself could write up to its own limit on every lookup, billed to the learner's key; the 25 s deadline cuts Kotiko off, but providers bill what they generate after a disconnect. Every request now carries the cap from spec/models.json (1,200 tokens for a lookup, 4,000 for a respelling). The field is `max_tokens`, which OpenRouter, Anthropic's and Gemini's OpenAI-compatible endpoints, Groq, Ollama and LM Studio read; the OpenAI preset says `max_completion_tokens` (new `maxTokensField` in spec/providers.json), because OpenAI's reasoning models refuse `max_tokens`. OpenRouter ignores a parameter a model doesn't take, so its free models get the cap too. A test per preset checks the field and both values; all eight failed before (no cap, except OpenRouter models that list it). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Slice 54, A-05. While a sign-in waited, any page could send the browser to https://kotiko.org/connect/?code=<junk>. Kotiko's content script there handed the code on, and the background deleted the stored verifier before trying it, so the learner's real code then failed with "This sign-in has expired". No key was at risk (PKCE held); the sign-in was simply spent. - Each sign-in gets a random 128-bit `state`, sent to OpenRouter's /auth, which returns it unchanged on the callback (OpenRouter's OAuth PKCE docs: "Returned unchanged as a state query parameter on the callback URL"). The return page's content script passes it on, and the background compares it with the waiting sign-in's before anything else. A code without it never reaches OpenRouter. - The verifier is deleted only once a code works, or when it has expired, so a code OpenRouter refuses no longer ends the sign-in. The kotiko.org/connect/ page needs no change (it has no script; the comment now names the state). Tests: in the background, a junk code with no state or a guessed one is refused unsent and the real code then works, and a refused code leaves the sign-in waiting (both failed before); the return page passes the state on; the PKCE pair and the OpenRouter address carry it; in Chromium, the reviewer's proof (a page opens the callback with a junk code, then the real code arrives) now connects. The existing e2e and welcome tests return the state as OpenRouter does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Slice 54, B-01 (the extension's half; the server now also listens on [::1], fix/sec-server). Browsers resolve `localhost` themselves and try [::1] first. Kotiko's server listened on 127.0.0.1 only, so another account on the computer could listen on [::1] at the same port and get the bearer token with the first sync (reviewer B's proof, in Chromium and Firefox). Ollama and LM Studio presets had the same shape, minus a key. - The default server is http://127.0.0.1:4747, in the settings, the popup, the dashboard and the welcome page, and in the hints and placeholders that show an example. The Ollama and LM Studio presets are 127.0.0.1 too. - ServerUrl.pinLoopback turns plain-http `localhost` into 127.0.0.1; normalizeServerUrl applies it, so a typed localhost is saved, bound and used as 127.0.0.1, and lookup addresses get the same through the client's endpoint and the route check. https:// keeps its name (its certificate names it), and so does *.localhost (a proxy's site). - An address bound before this version compares in today's form, so existing installs keep working at once; on update the saved server and lookup addresses and their bound routes are rewritten to 127.0.0.1, so the settings show where requests really go. Tests: in Chromium, a listener on [::1] next to a server typed as http://localhost:PORT heard the token in every request before and nothing after (test/e2e/loopback.spec.mjs, from reviewer B's b-extension-v6-squat.mjs); in the background, an install that saved localhost keeps syncing at 127.0.0.1 after the update with nothing sent to localhost, and a typed localhost is saved as 127.0.0.1 (both failed before); unit and property tests for the normalisation; a check that no default, example or preset in the extension names localhost. The privacy inventory lists 127.0.0.1; the site's guides use it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
…n clear
Slice 54, B-06 (the extension's half). The server's address field
warned about plain http:// to another machine (ServerUrl.sendsInClear),
but the address of "Another service", Ollama or LM Studio didn't,
though the learner's key and every word looked up go there.
Settings, Word lookups, and the welcome page's "Another service" now
show the same kind of warning under the address, in the lookup's own
words ("your key and the words you look up travel unencrypted"), as
soon as it is typed and when a saved one is shown. Lookups still work.
The field names the warning in aria-describedby. New English string
`lookup_url_unencrypted`; other locales fall back to English until
translated.
The jsdom tests now load lib/url.js, as both pages do. Two DOM tests
check the warning for plain http to another machine, loopback,
Tailscale and https; both failed before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Slice 54, C-07. A fresh install wrote the languages read from the browser's settings (`baseLangsDetected`), and the list made from them, to storage.sync before the learner had confirmed anything. The browser syncs that to the learner's Google or Mozilla account, while the privacy policy lists "Your browser's languages" under "What stays on your device" and the inventory says they never leave it. The behaviour now matches the documents: - storage.sync gets `ui` only once the learner has confirmed their languages (the first word, Skip, or a change in Settings, which all set `baseLangsConfirmed`), and never with `baseLangsDetected`. Until then the list is kept in the trusted copy and its mirror only. - A list synced from another browser keeps this browser's own detected languages. The policy's "Browser sync" section says when the languages go there (no version change here: the page-facing fixes bump it), the inventory says the detected ones never do, and the extension's copy is synced. Test (from reviewer C's sync-browser-langs.mjs): with Accept-Languages en-US, de-CH, ja, tr, an install and an unticked language write nothing to storage.sync, and the confirmed list arrives without the detected one. It failed before (storage.sync held ui with baseLangsDetected right after the install). The install, update and welcome tests now read the trusted copy's mirror instead of storage.sync. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
…nted Slice 54, A-06. "Delete everything" deleted the database, so the next store was set up like an update from 0.2, from storage.local, which content scripts can write. A 0.2-style `token` and `serverUrl` planted there while Kotiko was wiped became the trusted server, and the next word the learner typed went there (reviewer A's proof). The in-memory "wiped" state doesn't survive the worker stopping, so a later worker did the same with no learner action. - After deleting the database, the background creates it again with one row, `meta.bornFromWipe`. A store set up in a database with that mark reads nothing from storage.local, in this worker or a later one. It holds no words, keys or settings; slice 12's acceptance criterion and its implementation notes now say so instead of "does not exist". - A store set up from storage.local at a worker's start is kept only when onInstalled says it was an update from 0.1 or 0.2, the versions that kept the token there. On any other update or browser update it is dropped like a new install's planted keys (discardPlanted), after this start's set-up has run, so there is no race. Tests (test/bg/settings.test.mjs): after Delete everything and planted keys, the learner's next word stays in this browser, in the same worker and in a new worker over the same storage and database (both sent it to the planted server before); a token found at an update from 0.1 or 0.2 is kept and from 1.0.0 dropped (kept before). The slice 12 tests check the database holds only the mark. The server-install upgrade test now names the version it updates from, and the update/sync trigger test waits for the words to be written rather than a fixed time. The assurance case says when storage.local is trusted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Slice 54, B-01's defence in depth, against the route the server branch
added (fix/sec-server, POST /api/v1/proof). With 127.0.0.1 and the
server on both loopback families, a program at the address could still
answer while Kotiko's server is stopped, and the first sync used to
carry the token.
Before the first request that carries the token in a worker's life,
and again after the server couldn't be reached, the background sends a
fresh 32-byte random nonce (base64url, no token, credentials omitted)
and checks the answer, base64url(HMAC-SHA256(token, "kotiko-proof-v1:"
+ nonce)), with crypto.subtle.verify (constant time). Proofs are cached
per address and token and shared by concurrent requests.
- A proof made with another token: server_key_rejected, reason
wrong_proof ("didn't accept the access key", which covers a wrong
token and someone else's server alike).
- No route (401, 404, 405) or no proof in the answer: not_kotiko_server,
reason no_proof, with its own message ("update your server, or check
the address"). Anything can answer 404, so there's no fallback to
sending the token: a Kotiko server from before this route must be
updated with the extension. The CHANGELOG and the help page say so.
The test servers answer the proof (the fixture server, and the
background tests' stub, with the token the test holds). Tests: the
server's own test vector; listeners that answer 404, 401, no proof, or
a proof for another token or nonce never get the token, nor an add's
text (all five sent it before); one proof per worker life, again after
a failure; in Chromium, a listener on 127.0.0.1 standing in for a
stopped server hears the proof request and nothing with the token
(it got the token before). The privacy inventory counts the new call,
and the assurance case lists the check.
The full-stack e2e test runs the real server from this tree, so it
needs fix/sec-server's route: merge that branch first.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
`no-shadow` in background.js (pinLoopbackAddresses), the fake server's proof route and a B-01 background test. No behaviour change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Slice 54, B-01. The settings and the address hint now show http://127.0.0.1:4747. The server's proof request carries no token, so the upgrade test checks it apart from the requests that do. A wrong token typed during a slow sync is no longer sent at all: the server can't prove it holds that one; the test now checks the server never saw it, where it used to check the server did. Ran the whole Playwright suite. The full-stack tests pass against fix/sec-server's server (its /api/v1/proof); against this tree's server they need that branch merged first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
…kground Signed-off-by: Ayla Croft <dev@scriptkittyos.com> # Conflicts: # CHANGELOG.md # docs/security/assurance-case.md # site/src/content/docs/server/index.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extension background fixes from the pre-release security review (slice 54, SCR-476). Each fix has a regression test that failed before it. Merge after #29: the extension now asks the server to prove it holds the token, which needs #29's
/api/v1/proofroute, so the full-stack e2e fails until #29 is onmain.http://127.0.0.1:4747, Ollama and LM Studio default to127.0.0.1too, a typedhttp://localhostis asked at127.0.0.1, and saved addresses migrate on update. Before the first request that carries the token, and after an outage, the extension checks an HMAC proof from/api/v1/proof. A server that can't prove it holds the token never receives the token.test/e2e/loopback.spec.mjsreplays reviewer B's[::1]listener: it gets nothing now.state, which is checked in constant time. The verifier is kept until the exchange succeeds or the sign-in expires, so a junk code from a page no longer spoils a sign-in (e2e from reviewer A's proof).storage.localtoken or server. 0.2 settings are read only on a real update from 0.1/0.2.max_tokens, ormax_completion_tokensfor OpenAI).forceonsyncis honoured only from extension pages.__kotikotest hook is stripped from release zips, and the build refuses to finish if it remains.storage.sync; theuisettings sync only after the learner confirms them.validateWordsResponsetakes only a string content type. Two flaky tests are fixed at the root: the never-swap test's date dependence (also in test(extension): the never-swap test no longer depends on the date #30), and a race in welcome's base-language write, which is now one atomic update.Checks: lint; coverage 94.32 % lines and 85.17 % branches (2015 tests); perf within budget; Playwright 101 passed, with full-stack 3/3 against
fix/sec-server;npm test5 times plus once under UTC; spec/privacy sync; REUSE; DCO.🤖 Generated with Claude Code