From 11139a80aa7b7aaf156bec93e480e7c88ccacdab Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Wed, 7 Oct 2026 11:51:22 -0400 Subject: [PATCH 1/3] fix(server): every lookup request carries the output cap The server sent the spec's output cap (policy.max_tokens: 1,200 for a lookup, 4,000 for a respelling) only when OpenRouter's catalogue said the model takes it. A model the catalogue doesn't know, such as an LLM_MODEL on a paid custom endpoint, got no cap at all, so a model that kept writing could bill the owner's key up to the provider's own limit (security review D-03, Linear SCR-761). The cap now goes on every request, as the extension already does. Its field is the maxTokensField of the spec/providers.json preset on LLM_URL's host (max_completion_tokens for OpenAI), max_tokens otherwise; the server now embeds providers.json with the rest of the spec. Four new tests in llm_test.exs (custom endpoint, OpenRouter with an unknown model, OpenAI's field, a respelling) failed before the fix. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ayla Croft --- CHANGELOG.md | 6 ++++ server/lib/kotiko/llm/client.ex | 31 +++++++++++++++-- server/lib/kotiko/spec.ex | 10 +++++- server/test/kotiko/llm_test.exs | 44 +++++++++++++++++++++++-- slices/10-llm-client-resilience/SPEC.md | 15 ++++++--- spec/README.md | 2 +- 6 files changed, 96 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61cc9ca..37fc530 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ Conventional Commits by release-please. ## Unreleased +- Security: a Kotiko server set to a model of its own (`LLM_MODEL`), for example on a paid + provider, sent no limit on how long an answer may be, so a model that kept writing could + cost you up to the provider's own limit for one word. Every lookup now asks for at most + 1,200 tokens of answer (4,000 for refreshing pronunciations), like the extension does, + in the field OpenAI expects when `LLM_URL` is OpenAI's. + - Security: a web page could read your word list. It could hide a long list of words on the page, where you'd never see them, and read back which ones Kotiko swapped. Kotiko now swaps only words you can see: text a page hides isn't touched, and text below the diff --git a/server/lib/kotiko/llm/client.ex b/server/lib/kotiko/llm/client.ex index 0ed747e..e726fff 100644 --- a/server/lib/kotiko/llm/client.ex +++ b/server/lib/kotiko/llm/client.ex @@ -18,7 +18,8 @@ defmodule Kotiko.LLM.Client do @doc """ Asks `model` once. `caps` is the model's `%{json_mode, reasoning_toggle, max_tokens}`; `opts`: `:timeout_ms` (required), `:json` (send `response_format`, default - `caps.json_mode`), `:max_tokens` (sent when `caps.max_tokens`). + `caps.json_mode`), `:max_tokens` (the output cap, sent on every request in the field + `max_tokens_field/0` names). """ def attempt(model, caps, messages, opts) do timeout = Keyword.fetch!(opts, :timeout_ms) @@ -29,7 +30,10 @@ defmodule Kotiko.LLM.Client do |> put_if(json?, :response_format, %{type: "json_object"}) # Thinking takes 5-20 s longer and doesn't get these lookups more right. |> put_if(openrouter?() and caps.reasoning_toggle, :reasoning, %{enabled: false}) - |> put_if(caps.max_tokens and is_integer(opts[:max_tokens]), :max_tokens, opts[:max_tokens]) + # The spec's output cap on every request, whatever the catalog says, so a model on a + # paid endpoint that keeps writing can't bill the key up to the provider's own limit + # (security review D-03). OpenRouter ignores it for a model that doesn't take it. + |> put_if(is_integer(opts[:max_tokens]), max_tokens_field(), opts[:max_tokens]) request = fn -> Req.post( @@ -72,6 +76,29 @@ defmodule Kotiko.LLM.Client do @doc "Whether LLM_URL is OpenRouter (its catalog, quota and app headers)." def openrouter?, do: Kotiko.Config.openrouter?(Application.fetch_env!(:kotiko, :llm_url)) + @doc """ + The request field for the output cap: the `maxTokensField` of the spec/providers.json + preset on LLM_URL's host (OpenAI's reasoning models take only `max_completion_tokens`), + `max_tokens` otherwise. + """ + def max_tokens_field do + host = host(Application.fetch_env!(:kotiko, :llm_url)) + + field = + Enum.find_value(Spec.providers(), "max_tokens", fn p -> + host && is_binary(p["baseUrl"]) && host(p["baseUrl"]) == host && p["maxTokensField"] + end) + + if field == "max_completion_tokens", do: :max_completion_tokens, else: :max_tokens + end + + defp host(url) do + case URI.parse(url).host do + host when is_binary(host) and host != "" -> String.downcase(host) + _ -> nil + end + end + @doc "Whether an API key is configured." def key?, do: Application.get_env(:kotiko, :llm_api_key) not in [nil, ""] diff --git a/server/lib/kotiko/spec.ex b/server/lib/kotiko/spec.ex index da32f09..3cda6bc 100644 --- a/server/lib/kotiko/spec.ex +++ b/server/lib/kotiko/spec.ex @@ -12,7 +12,7 @@ defmodule Kotiko.Spec do @root Path.expand("../../../spec", __DIR__) - @json_files ~w(languages lang-aliases rules pronunciation models wiktionary) + @json_files ~w(languages lang-aliases rules pronunciation models wiktionary providers) for name <- @json_files, do: @external_resource(Path.join(@root, name <> ".json")) @external_resource Path.join(@root, "prompt.md") @external_resource Path.join(@root, "VERSION") @@ -73,6 +73,7 @@ defmodule Kotiko.Spec do @pronunciation read_json.(Path.join(@root, "pronunciation.json")) @models read_json.(Path.join(@root, "models.json")) @wiktionary read_json.(Path.join(@root, "wiktionary.json")) + @providers read_json.(Path.join(@root, "providers.json")) @prompt_text File.read!(Path.join(@root, "prompt.md")) @version @root |> Path.join("VERSION") |> File.read!() |> String.trim() @lang_data lang_data @@ -107,6 +108,13 @@ defmodule Kotiko.Spec do @doc "spec/wiktionary.json: where pronunciations come from (slice 49 section 4a)." def wiktionary, do: Map.delete(@wiktionary, "_comment") + @doc """ + spec/providers.json (slice 11 section 4): the lookup presets. The server has no preset + setting, only LLM_URL; it uses a preset's request details (such as `maxTokensField`) + when LLM_URL is on that preset's host. + """ + def providers, do: @providers["providers"] + @doc "spec/prompt.md, unparsed." def prompt_text, do: @prompt_text diff --git a/server/test/kotiko/llm_test.exs b/server/test/kotiko/llm_test.exs index 17465cd..f18bbc5 100644 --- a/server/test/kotiko/llm_test.exs +++ b/server/test/kotiko/llm_test.exs @@ -49,10 +49,8 @@ defmodule Kotiko.LLMTest do assert system =~ "\"add a word\" box" assert body["temperature"] == 0.2 assert body["response_format"] == %{"type" => "json_object"} - # Not OpenRouter: no reasoning switch, no app attribution, no max_tokens for a - # model the catalog doesn't know. + # Not OpenRouter: no reasoning switch, no app attribution. refute Map.has_key?(body, "reasoning") - refute Map.has_key?(body, "max_tokens") refute List.keymember?(headers, "http-referer", 0) assert {"authorization", "Bearer test-llm-key"} in headers assert {"x-title", "Kotiko"} in headers @@ -82,6 +80,39 @@ defmodule Kotiko.LLMTest do assert {"x-title", "Kotiko"} in headers end + # Security review D-03: the spec's output cap goes on every request, so a model on a + # paid endpoint that keeps writing can't bill the key up to the provider's own limit. + test "an LLM_MODEL model on a custom endpoint gets the output cap" do + LLMStub.stub(fn _, conn -> LLMStub.answer(conn, @da) end) + interpret() + assert_received {:llm_request, "m1", body, _headers} + assert body["max_tokens"] == 1200 + refute Map.has_key?(body, "max_completion_tokens") + end + + test "on OpenRouter, a model the catalog doesn't know gets the output cap" do + openrouter() + LLMStub.stub(fn _, conn -> LLMStub.answer(conn, @da) end) + interpret() + assert_received {:llm_request, "m1", body, _headers} + assert body["max_tokens"] == 1200 + end + + test "OpenAI gets the cap in its own field (spec/providers.json maxTokensField)" do + for {url, text} <- [ + {"https://api.openai.com/v1", "da"}, + {"https://API.OpenAI.com/v1", "da?"} + ] do + put_app_env(:llm_url, url) + LLMStub.stub(fn _, conn -> LLMStub.answer(conn, @da) end) + # A different text each time, so the second isn't answered from the cache. + interpret(text) + assert_received {:llm_request, "m1", body, _headers} + assert body["max_completion_tokens"] == 1200 + refute Map.has_key?(body, "max_tokens") + end + end + test "JSON wrapped in prose or a think block is still read" do LLMStub.stub(fn "m1", conn -> LLMStub.answer( @@ -812,6 +843,13 @@ defmodule Kotiko.LLMTest do assert {{:error, %{code: "quota_exhausted"}}, _} = with_log(fn -> LLM.respell(@items) end) end + test "asks for more tokens than a lookup, even for a model the catalog doesn't know" do + LLMStub.stub(fn _, conn -> LLMStub.answer(conn, %{items: []}) end) + with_log(fn -> LLM.respell(@items) end) + assert_received {:llm_request, "m1", body, _} + assert body["max_tokens"] == 4000 + end + test "asks for more tokens than a lookup when the model takes max_tokens" do LLMStub.stub(fn _, conn -> LLMStub.answer(conn, %{items: []}) end) diff --git a/slices/10-llm-client-resilience/SPEC.md b/slices/10-llm-client-resilience/SPEC.md index da3c309..7e8b4ec 100644 --- a/slices/10-llm-client-resilience/SPEC.md +++ b/slices/10-llm-client-resilience/SPEC.md @@ -130,7 +130,7 @@ is enforced with a monotonic clock; an attempt is not started with less than 4 s **Request per attempt**: slice 09's messages; `temperature: 0.2`; `response_format: {type: "json_object"}` only if the model's `json_mode`; `reasoning: {enabled: false}` -only on OpenRouter and if `reasoning_toggle`; `max_tokens: 1200` if supported; headers +only on OpenRouter and if `reasoning_toggle`; `max_tokens: 1200` on every request; headers `X-Title: Kotiko` and `HTTP-Referer: https://github.com/ScriptKittyOS/kotiko` on OpenRouter (app attribution, as slice 11 specifies); `Req` with `retry: false`. @@ -345,10 +345,15 @@ failing models; when every model is skipped, none is. string `error` for 0.2 extensions and adds `code` and `details` beside it, which the current popup reads. The plain `message` is English: the server's locales (25, 50) aren't built. -- *Request fields.* `max_tokens` and `reasoning` are sent only when the catalog lists them - for the model; a model the catalog doesn't know gets `response_format` (with the - 400-without-it retry) and neither of the others. A respelling asks for 4,000 tokens, since - 20 items times their bases don't fit the lookup's 1,200 (`policy.max_tokens`). +- *Request fields.* `reasoning` is sent only when the catalog lists it for the model; a + model the catalog doesn't know gets `response_format` (with the 400-without-it retry) and + no `reasoning`. The output cap (`policy.max_tokens`) goes on every request, whatever the + catalog says, so a model on a paid endpoint (an `LLM_MODEL` the catalog doesn't know) + can't write up to the provider's own limit on the owner's key (security review D-03). Its + field is the `maxTokensField` of the `spec/providers.json` preset on `LLM_URL`'s host + (`max_completion_tokens` for OpenAI), `max_tokens` otherwise, as the extension does. A + respelling asks for 4,000 tokens, since 20 items times their bases don't fit the lookup's + 1,200. - *Quota.* The gate applies only when every model in the chain is free (OpenRouter's daily count is for free models). The refresh right after a 429 runs inside the lookup, bounded by what's left of the deadline, because its answer decides between `quota_exhausted` and a diff --git a/spec/README.md b/spec/README.md index 5a60c94..4040014 100644 --- a/spec/README.md +++ b/spec/README.md @@ -16,7 +16,7 @@ the server (`Kotiko.Spec`, compiled into the release) and the extension (its cop | `rules.json` | Every number the validator uses (caps, minimum lengths), and the content scripts' `max_page_*` caps (how many words one page view may show; security review A-01), which `sync-extension.mjs` also writes to `extension/spec/page.js`. | | `pronunciation.json` | Per target language: romanization scheme, stress kind, tone range, vocalization marks. | | `models.json` | Slice 10: the free models to prefer for lookups and respellings (from `eval/RESULTS.md`), the ones to deny, the shipped fallback list, and the lookup client's budgets (deadlines, attempts, health, quota reserve, cache). | -| `providers.json` | Slice 11: the lookup provider presets (OpenRouter by default, OpenAI, Anthropic, Google Gemini, Groq, Ollama, LM Studio, Custom): base URL, whether a key is needed, JSON mode, extra headers, where the model list comes from. Never a key. | +| `providers.json` | Slice 11: the lookup provider presets (OpenRouter by default, OpenAI, Anthropic, Google Gemini, Groq, Ollama, LM Studio, Custom): base URL, whether a key is needed, JSON mode, extra headers, where the model list comes from, and the field for the output cap (`maxTokensField`, which the server also uses when `LLM_URL` is on a preset's host). Never a key. | | `languages.json`, `lang-aliases.json` | Language tags (slice 08), generated from Unicode CLDR. | | `lang/` | Per base language: stopwords, stem rules, spelling variants, the respelling key. See `lang/README.md`. | | `fixtures/` | Shared fixtures both runtimes must pass (below). | From 2e8bc14417502f64dacbe9bdf714a0896bd067f2 Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Wed, 7 Oct 2026 11:52:28 -0400 Subject: [PATCH 2/3] fix(server): warn about plain HTTP however the address is written The warning for an API key sent over plain HTTP to another machine matched "http://" case-sensitively, so LLM_URL=HTTP://203.0.113.7/v1 with a key gave no warning, while the request still went out in the clear (security review D-04). The check now takes the scheme from URI.parse/1, which lowercases it. Checking the other URL comparisons in config.ex and exposure.ex: the URL validator already compared URI.new/1's lowercased scheme and Exposure.network/1 already lowercases the host, but openrouter?/1 compared the host as written, so https://OpenRouter.ai/... wasn't treated as OpenRouter; it now lowercases the host too. The new tests in config_test.exs (HTTP://, Http://, hTtP://, HTTP:// for the transcriber, and an OpenRouter host in capitals) failed before the fix. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ayla Croft --- CHANGELOG.md | 5 ++++ server/lib/kotiko/config.ex | 18 +++++++++----- server/test/kotiko/config_test.exs | 38 ++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37fc530..f879b96 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ Conventional Commits by release-please. ## Unreleased +- Security: the server warns when it would send an API key over plain HTTP to another + machine, but missed an address written in capitals, such as `HTTP://203.0.113.7/v1`. It + now warns however the address is written. An OpenRouter address in capitals is also + recognised as OpenRouter. + - Security: a Kotiko server set to a model of its own (`LLM_MODEL`), for example on a paid provider, sent no limit on how long an answer may be, so a model that kept writing could cost you up to the provider's own limit for one word. Every lookup now asks for at most diff --git a/server/lib/kotiko/config.ex b/server/lib/kotiko/config.ex index fda9d2b..7f39567 100644 --- a/server/lib/kotiko/config.ex +++ b/server/lib/kotiko/config.ex @@ -550,10 +550,14 @@ defmodule Kotiko.Config do @doc "Whether the model API is OpenRouter (it has the built-in free models)." def openrouter?(url) do + # URI.parse/1 lowercases the scheme but not the host, which is case-insensitive too. case URI.parse(url).host do - "openrouter.ai" -> true - host when is_binary(host) -> String.ends_with?(host, ".openrouter.ai") - _ -> false + host when is_binary(host) -> + host = String.downcase(host) + host == "openrouter.ai" or String.ends_with?(host, ".openrouter.ai") + + _ -> + false end end @@ -648,10 +652,12 @@ defmodule Kotiko.Config do # between can read it. This computer and Tailscale (encrypted) are fine. defp plain_http(_key_var, nil, _url_var, _url), do: [] - defp plain_http(key_var, _key, url_var, {:ok, "http://" <> _ = url}) do - host = URI.parse(url).host + # The scheme from URI.parse/1, which lowercases it: a scheme is case-insensitive, so + # HTTP://... goes out in the clear just the same (security review D-04). + defp plain_http(key_var, _key, url_var, {:ok, url}) when is_binary(url) do + %URI{scheme: scheme, host: host} = URI.parse(url) - if Kotiko.Exposure.network(host) in [:loopback, :tailscale] do + if scheme != "http" or Kotiko.Exposure.network(host) in [:loopback, :tailscale] do [] else [ diff --git a/server/test/kotiko/config_test.exs b/server/test/kotiko/config_test.exs index 9c7647a..cb84ae2 100644 --- a/server/test/kotiko/config_test.exs +++ b/server/test/kotiko/config_test.exs @@ -529,6 +529,9 @@ defmodule Kotiko.ConfigTest do assert Config.openrouter?("https://eu.openrouter.ai/api/v1") refute Config.openrouter?("http://localhost:11434/v1") refute Config.openrouter?("https://notopenrouter.ai/v1") + # A host name is case-insensitive too (security review D-04's neighbour). + assert Config.openrouter?("HTTPS://OpenRouter.ai/api/v1") + assert Config.openrouter?("https://EU.OPENROUTER.AI/api/v1") end # B-09 (slice 54): any 24 characters were accepted, even 24 a's. @@ -615,6 +618,41 @@ defmodule Kotiko.ConfigTest do assert warning =~ "TRANSCRIBE_API_KEY is sent over plain HTTP to whisper.lan" end + # D-04 (slice 54): a scheme is case-insensitive (RFC 3986 section 3.1) and Req sends + # HTTP://... in the clear, but only a lowercase http:// got the warning. + test "whatever the scheme's case" do + for url <- ~w(HTTP://203.0.113.7/v1 Http://203.0.113.7/v1 hTtP://203.0.113.7/v1) do + assert [warning] = + plain_http_warnings(%{ + "LLM_URL" => url, + "LLM_MODEL" => "m", + "LLM_API_KEY" => "k" + }), + url + + assert warning =~ "LLM_API_KEY is sent over plain HTTP to 203.0.113.7" + end + + assert [_] = + plain_http_warnings(%{ + "TRANSCRIBE_URL" => "HTTP://whisper.lan:8178/inference", + "TRANSCRIBE_API_KEY" => "k", + "LLM_API_KEY" => "k" + }) + + assert plain_http_warnings(%{ + "LLM_URL" => "HTTPS://203.0.113.7/v1", + "LLM_MODEL" => "m", + "LLM_API_KEY" => "k" + }) == [] + + assert plain_http_warnings(%{ + "LLM_URL" => "HTTP://LOCALHOST:11434/v1", + "LLM_MODEL" => "m", + "LLM_API_KEY" => "k" + }) == [] + end + test "not to this machine, over Tailscale, over HTTPS, or without a key" do for url <- ~w(http://127.0.0.1:11434/v1 http://localhost:11434/v1 http://[::1]:1/v1 http://ollama.localhost/v1 http://100.101.102.103/v1 From 34177fcb5f3ac572c1e68f880e1114ce54c3bbda Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Wed, 7 Oct 2026 11:55:56 -0400 Subject: [PATCH 3/3] fix(release): the security gate reads lines as a reader sees them The store gate counts the lines above a review's "## Appendix" heading that read as a gate line and needs exactly one, the exact closed line. Its pattern was ASCII-only, so an open gate line written with a Cyrillic letter, behind a zero-width space or with a fullwidth colon wasn't counted: beside the closed line, a reader saw "Gate: open" while the check read the gate as closed (security review D-05). Each line is now NFKC-normalised and stripped of invisible characters (format and default-ignorable code points) and combining marks before the check. A line then reads as a gate line when its first four characters after any leading markup are g, a, t, e (any case) or any character outside ASCII, with no letter or digit after them. Letting any non-ASCII character stand in for a letter catches lookalikes from every script, and colon lookalikes NFKC keeps, without a confusables table; it errs on the side of keeping the gate open. The closing line must also be plain ASCII. When the gate stays open, the command now says why for each report and names the lines. The new tests in security-gate.test.mjs cover the reviewer's three lines (Cyrillic a, a leading U+200B, a fullwidth colon) and seven more disguises, before and after the closed line, plus the exact closed line and ordinary prose still closing it. Against the old script, all three of the reviewer's reports read as closed. Co-Authored-By: Claude Opus 5.5 Signed-off-by: Ayla Croft --- CHANGELOG.md | 5 + docs/stores.md | 5 +- scripts/check-security-gate.mjs | 94 ++++++++++++++++--- slices/54-pre-release-security-review/SPEC.md | 4 +- test/unit/security-gate.test.mjs | 42 ++++++++- 5 files changed, 134 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f879b96..c655e04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ Conventional Commits by release-please. ## Unreleased +- Release process: the check that holds store uploads until the security review is closed + could miss a "Gate: open" line written with a lookalike letter from another alphabet, an + invisible character or a fullwidth colon, and read the review as closed. It now reads + each line as a person would, keeps the gate open for any such line, and says which line. + - Security: the server warns when it would send an API key over plain HTTP to another machine, but missed an address written in capitals, such as `HTTP://203.0.113.7/v1`. It now warns however the address is written. An OpenRouter address in capitals is also diff --git a/docs/stores.md b/docs/stores.md index f74f4eb..6244d7a 100644 --- a/docs/stores.md +++ b/docs/stores.md @@ -262,7 +262,10 @@ Gate: closed 2026-11-02 by , fixes confirmed on v1.0.0-rc.2 The candidate it names is one of the reviewed version's. A second `Gate:` line (for example `Gate: open`) above the appendix keeps the gate open, and lines under the appendix count for -nothing. That blocks every store upload until the first review is done. When a review is +nothing. Lines are read as a reader sees them: a gate line written with a lookalike letter +from another alphabet, an invisible character or a fullwidth colon still counts as a second +gate line, and the closing line itself must be plain ASCII (security review D-05). When the +gate stays open, the job says why for each report. That blocks every store upload until the first review is done. When a review is opened for the version being released (a `review-vX.Y.Z-rc.N.md` for that version), that review must be the closed one. Whether a later release needs a review at all (it touches the areas above) is enforced by the checklist box, not by the workflow, because no script diff --git a/scripts/check-security-gate.mjs b/scripts/check-security-gate.mjs index 91778c1..b58dd49 100755 --- a/scripts/check-security-gate.mjs +++ b/scripts/check-security-gate.mjs @@ -14,6 +14,18 @@ // below the appendix count for nothing: a reviewer's report may quote the phrase (security // review C-05). // +// "One gate line" is judged as a reader sees the text, not byte by byte (security review +// D-05: an open line in a Cyrillic letter, behind a zero-width space or with a fullwidth +// colon sat unseen beside the closed one). Each line is NFKC-normalised, stripped of +// invisible characters (format and default-ignorable code points) and of combining marks; +// then a line whose first four characters, after any leading markup, are g, a, t, e (any +// case) or a character outside ASCII, not followed by a letter or digit, reads as a gate +// line. Any non-ASCII character stands in for any letter, so lookalikes from every +// script count without a confusables table, and a colon-like character after the word +// counts too. That is stricter than a reader (a stray "Gâté:" line keeps the gate open, +// with a message saying which line), which is the safe side. The closed line itself must +// still be the exact ASCII form. +// // This check passes when the tagged tree has a closed report. With --version X.Y.Z (the // release workflow passes the tag's), a review opened for that version must itself be // closed; a version without its own review is gated by the earlier ones. The release @@ -32,21 +44,52 @@ const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), ".."); export const REPORT_RE = /^review-v(\d+\.\d+\.\d+)(?:-rc\.\d+)?\.md$/; export const CLOSED_RE = /^Gate: closed (\d{4}-\d{2}-\d{2}) by (.+), fixes confirmed on (v\d+\.\d+\.\d+-rc\.\d+)$/; export const APPENDIX_RE = /^## Appendix\b/; -// Any line that reads as a gate line, however it is dressed up, so that only one may exist. -const ANY_GATE_RE = /^[\s>*_\-+#]*gate[*_\s]*:/i; +// Any line that reads as a gate line, however it is dressed up, so that only one may exist: +// after `visible`, leading markup or punctuation, then four characters that are each the +// letter of "gate" or not ASCII, and no letter or digit right after (D-05). +const ANY_GATE_RE = /^[\s\p{P}>+]*[g\P{ASCII}][a\P{ASCII}][t\P{ASCII}][e\P{ASCII}](?![\p{L}\p{N}])/iu; + +/** A line as a reader sees it: NFKC, without invisible characters or combining marks. */ +const visible = (line) => + line + .normalize("NFKC") + .replace(/[\p{Cf}\p{DI}]/gu, "") + .normalize("NFD") + .replace(/\p{M}/gu, "") + .normalize("NFC"); +const isGateLine = (line) => ANY_GATE_RE.test(visible(line)); const realDate = (d) => !Number.isNaN(Date.parse(`${d}T00:00:00Z`)) && new Date(`${d}T00:00:00Z`).toISOString().startsWith(d); -/** The lead's closing line in a report's text, or null while the gate is open. */ -export function gateLine(text) { +/** `{ gate }` (date, lead, candidate) when the report's gate is closed, else `{ problem }`. */ +function readGate(text) { const lines = text.split(/\r?\n/); const end = lines.findIndex((l) => APPENDIX_RE.test(l)); - if (end < 0) return null; - const gates = lines.slice(0, end).filter((l) => ANY_GATE_RE.test(l)); - if (gates.length !== 1) return null; - const m = gates[0].match(CLOSED_RE); - if (!m || !realDate(m[1])) return null; - return { date: m[1], by: m[2], candidate: m[3] }; + if (end < 0) return { problem: 'no "## Appendix" heading' }; + const gates = lines.slice(0, end).flatMap((l, i) => (isGateLine(l) ? [i + 1] : [])); + if (gates.length === 0) return { problem: 'no "Gate:" line above the appendix' }; + if (gates.length > 1) { + return { problem: `${gates.length} lines above the appendix read as a gate line (lines ${gates.join(", ")}); keep only the one` }; + } + const [n] = gates; + const line = lines[n - 1]; + if (/[^\x20-\x7E]/.test(line)) { + return { problem: `line ${n} reads as a gate line but has characters outside plain ASCII (a lookalike letter, an invisible or fullwidth character); write it in plain ASCII` }; + } + const m = line.match(CLOSED_RE); + if (!m) return { problem: `line ${n} is not the exact closed line` }; + if (!realDate(m[1])) return { problem: `line ${n}: ${m[1]} is not a date` }; + return { gate: { date: m[1], by: m[2], candidate: m[3] } }; +} + +/** The lead's closing line in a report's text, or null while the gate is open. */ +export function gateLine(text) { + return readGate(text).gate ?? null; +} + +/** Why a report's gate is open (one sentence), or null when its closing line is in order. */ +export function gateProblem(text) { + return readGate(text).problem ?? null; } const core = (tag) => tag.replace(/^v/, "").replace(/-rc\.\d+$/, ""); @@ -60,16 +103,40 @@ export function closedReviews(dir, { version } = {}) { if (e.code === "ENOENT") return []; throw e; } + return reviews(dir, names, version) + .filter((r) => !r.problem) + .map((r) => r.name); +} + +/** Every report the release would read, each with why its gate is open (or no problem). */ +function reviews(dir, names, version) { let reports = names.filter((n) => REPORT_RE.test(n)).sort(); if (version && reports.some((n) => n.match(REPORT_RE)[1] === version)) { reports = reports.filter((n) => n.match(REPORT_RE)[1] === version); } - return reports.filter((n) => { - const line = gateLine(readFileSync(join(dir, n), "utf8")); - return line !== null && core(line.candidate) === n.match(REPORT_RE)[1]; + return reports.map((name) => { + const { gate, problem } = readGate(readFileSync(join(dir, name), "utf8")); + const reviewed = name.match(REPORT_RE)[1]; + if (problem) return { name, problem }; + if (core(gate.candidate) !== reviewed) return { name, problem: `${gate.candidate} is not a candidate of ${reviewed}` }; + return { name, problem: null }; }); } +/** Why each report's gate is open, as "file: reason" lines. */ +export function openReasons(dir, { version } = {}) { + let names; + try { + names = readdirSync(dir); + } catch (e) { + if (e.code === "ENOENT") return []; + throw e; + } + return reviews(dir, names, version) + .filter((r) => r.problem) + .map((r) => `${r.name}: ${r.problem}`); +} + if (process.argv[1] && resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { const arg = (name) => { const i = process.argv.indexOf(name); @@ -91,6 +158,7 @@ if (process.argv[1] && resolve(process.argv[1]) === fileURLToPath(import.meta.ur "\"Gate: closed YYYY-MM-DD by , fixes confirmed on vX.Y.Z-rc.N\"" + (version ? `, and if ${core(version)} has its own review, that one closed.` : "."), ); + for (const reason of openReasons(dir, { version: version && core(version) })) console.error(` ${reason}`); process.exitCode = 1; } } diff --git a/slices/54-pre-release-security-review/SPEC.md b/slices/54-pre-release-security-review/SPEC.md index d367ae1..577119a 100644 --- a/slices/54-pre-release-security-review/SPEC.md +++ b/slices/54-pre-release-security-review/SPEC.md @@ -165,7 +165,9 @@ are summarised without exploit steps (SECURITY.md's disclosure rules, 03). `Gate: closed YYYY-MM-DD by , fixes confirmed on vX.Y.Z-rc.N` (for example `Gate: closed 2026-11-02 by Ayla Croft, fixes confirmed on v1.0.0-rc.2`), naming a candidate of the reviewed version. Until then the report says `Gate: open`; the part - above the appendix has only one line starting `Gate:`. 30's release workflow checks for + above the appendix has only one line starting `Gate:`, read as a reader sees it (after + NFKC, without invisible characters, with any non-ASCII character standing in for a letter + of "Gate"; security review D-05), and the closed line is plain ASCII. 30's release workflow checks for the closed line before any store upload (`scripts/check-security-gate.mjs`): anything else, including the phrase inside an appended report, keeps the gate open (security review C-05). diff --git a/test/unit/security-gate.test.mjs b/test/unit/security-gate.test.mjs index ecb6f88..872f0cd 100644 --- a/test/unit/security-gate.test.mjs +++ b/test/unit/security-gate.test.mjs @@ -12,7 +12,7 @@ import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; -import { closedReviews, gateLine } from "../../scripts/check-security-gate.mjs"; +import { closedReviews, gateLine, gateProblem } from "../../scripts/check-security-gate.mjs"; const ROOT = join(dirname(fileURLToPath(import.meta.url)), "../.."); const CLOSED = "Gate: closed 2026-11-02 by Ayla Croft, fixes confirmed on v1.0.0-rc.2"; @@ -60,6 +60,41 @@ test("only the exact closed line, above the appendix, closes the gate", (t) => { assert.deepEqual(closedReviews(gateDir(t, { "review-v1.1.0-rc.1.md": report(CLOSED) })), []); }); +// Security review D-05: an open gate line the script couldn't read (a lookalike letter, an +// invisible character, a fullwidth colon) sat beside the exact closed line, so a reader saw +// "Gate: open" while the gate read closed. +test("D-05: a gate line in lookalike, invisible or fullwidth characters keeps the gate open", (t) => { + const disguised = [ + "G\u0430te: open (fixes pending)", // Cyrillic а + "\u200BGate: open (fixes pending)", // a leading zero-width space + "Gate\uFF1A open (fixes pending)", // a fullwidth colon + "Gat\u0435: open", // Cyrillic е + "\u0262ate: open", // a small-capital G + "Gate\uA789 open", // a modifier-letter colon NFKC leaves alone + "Ga\u00ADte: open", // a soft hyphen inside the word + "\u3164Gate: open", // a Hangul filler, an invisible letter + "\u2022 Gate: open", // a non-ASCII bullet + "G\u0301ate: open", // a combining accent + ]; + for (const sneaky of disguised) { + for (const lead of [`${sneaky}\n\n${CLOSED}`, `${CLOSED}\n\n${sneaky}`]) { + const text = report(lead); + assert.equal(gateLine(text), null, JSON.stringify(sneaky)); + assert.deepEqual(closedReviews(gateDir(t, { "review-v1.0.0-rc.1.md": text })), [], JSON.stringify(sneaky)); + assert.match(gateProblem(text), /2 lines above the appendix read as a gate line/, JSON.stringify(sneaky)); + } + } + // The closing line itself written with a lookalike doesn't close it either, and says why. + const lookalike = report(CLOSED.replace("closed", "cl\u043Esed")); + assert.equal(gateLine(lookalike), null); + assert.match(gateProblem(lookalike), /characters outside plain ASCII/); + // The exact closed line alone still closes it; prose that merely starts with "Gate" isn't a gate line. + const fine = report(`Gatekeeping notes: none.\nGates were checked.\n- \u0441\u043F\u0430\u0441\u0438\u0431\u043E: swapped as expected.\n${CLOSED}`); + assert.deepEqual(gateLine(fine), { date: "2026-11-02", by: "Ayla Croft", candidate: "v1.0.0-rc.2" }); + assert.equal(gateProblem(fine), null); + assert.deepEqual(closedReviews(gateDir(t, { "review-v1.0.0-rc.1.md": report(CLOSED) })), ["review-v1.0.0-rc.1.md"]); +}); + test("gateLine reads the date, the lead and the candidate", () => { assert.deepEqual(gateLine(report(CLOSED)), { date: "2026-11-02", by: "Ayla Croft", candidate: "v1.0.0-rc.2" }); assert.equal(gateLine(report("Gate: open")), null); @@ -83,6 +118,11 @@ test("the command line: --version, and the exit code", (t) => { let r = gate("--version", "1.0.0"); assert.equal(r.status, 1); assert.match(r.stderr, /no closed security review/); + assert.match(r.stderr, /review-v1\.0\.0-rc\.1\.md: line 3 is not the exact closed line/, "says why, per report"); + writeFileSync(join(dir, "review-v1.0.0-rc.1.md"), report(`G\u0430te: open\n${CLOSED}`)); + r = gate("--version", "1.0.0"); + assert.equal(r.status, 1); + assert.match(r.stderr, /2 lines above the appendix read as a gate line \(lines 3, 4\)/); writeFileSync(join(dir, "review-v1.0.0-rc.1.md"), report(CLOSED)); r = gate("--version", "1.0.0"); assert.equal(r.status, 0, r.stderr);