# i18n web commits — audit notes Scope: the three commits that introduced Fluent i18n on the web crate: - `2dc0d94` — `i18n on the web` - `8f1bfdb` — `i18n on the web - deepseek translations` - `967335d` — `i18n on the web - deepseek translations2` Baseline used for "what changed": `git diff 2dc0d94^ HEAD`. Everything below has been fixed in the working tree. `cargo check -p web` and `cargo test -p web` both pass (172 tests). Where the fix could regress silently, there is now a test rather than a note. ## What the tests found that the reading did not `cargo test -p web` had **13 failures at `HEAD`** — the audit's first pass never ran it. Three separate causes, all now fixed: - **Fluent bidi isolation.** Every interpolated value was wrapped in U+2068 / U+2069, so a table name rendered as `⁨invoice⁩`. Invisible on screen, but they are in the copied text and in every `contains` assertion. All three languages are left-to-right, so the loader now sets `use_isolating(false)` ([web/src/i18n/mod.rs](web/src/i18n/mod.rs)). 7 failures. - **`td-type-to-confirm` lost half its sentence.** The English message had been reduced to the single word `Type`, with the table name concatenated by the template — so the delete confirmation read "Type `invoice`" with no "to confirm", in all three languages. The message is now one whole sentence with `{ $table }` inside it, which also lets sk/cs put the verb where it belongs. 2 failures. - **A hardcoded limit that was wrong.** `schema-err-table-name-too-long` said "63 characters"; `MAX_TABLE_NAME_LENGTH` is **38**. It and `error-identifier-too-long` now take `{ $limit }` from the constant, so the message cannot drift from the rule again. 1 failure. The remaining 3 were a stale expectation: Askama escapes `&` as `&`, not `&`. ## Fixed — security ### 1. `POST /login` had no cross-site check at all `change_password` directly above it had one; `login` did not. That is login CSRF: another site can post its own credentials and move the victim's browser into an account the attacker controls, and everything done there afterwards. `SameSite=Strict` does not prevent it — Strict governs *sending* a cookie cross-site, not *setting* one from a cross-site response. `register`, `logout`, and the two analytics endpoints were also uncovered. All now check. The helper's doc comment claimed it "covers every state-changing endpoint the same way", which was false when written; it is true now, and [web/src/lib.rs](web/src/lib.rs) has `a_cross_site_post_is_refused_before_any_backend_call` walking all 22 POST routes to keep it true. ### 2. The check ran *after* the backend call in four handlers `add_logic`, `add_validation` (×3), `import`, and `export` loaded the page — a gRPC round trip — and only then asked whether the post was cross-site. A refusal that happens after the work is not a refusal. All moved to the first line of the handler. That same test pins the ordering rather than just the presence: the test router points at a dead port, so a handler that called the backend first could not answer `403`. ### 3. Three copies of the check, plus one inline `services::reject_cross_site` existed alongside private `cross_site` helpers in `add_validation`, `import`, and `export`, and an inline copy in `add_logic`. Four codepaths to keep in agreement. Deleted; everything calls the shared one. ### 4. `|safe` on messages that interpolate values Three sites render a translation as raw HTML with a value substituted into it. Neither was exploitable — role names are validated server-side to `[a-z][a-z0-9_-]*` (`server/src/auth/handlers/roles/store.rs:43`), and `$type` only renders behind `catalog.is_compound()` — but the guard sat in another crate across a gRPC boundary, and the web crate re-rendered the result as trusted markup without rechecking. `Nav::tr_args_html` now escapes the argument values while leaving the message's own markup alone, and the three sites use it. The invariant no longer depends on a validator in a different process. ### 5. The bigger `|safe` exposure: the catalogues themselves The audit's original framing missed this. There are 11 `|safe` sites, and the `sk`/`cs` catalogues were **machine-translated**. Every one of those messages is raw HTML written by something nobody reviewed tag-by-tag. A malformed tag is a broken page; a hostile one is XSS. [web/src/i18n/catalogue.rs](web/src/i18n/catalogue.rs) now checks, for all three locales on every test run: - the `|safe` keys — **read out of the templates**, so the list cannot drift — contain only ``, ``, ``, ``, balanced; - an `` carries nothing but a same-app `href="/…"` (no `javascript:`, no off-site redirect); - every *other* message contains no markup at all, so a message cannot arrive at a `|safe` site later with tags already in it; - key parity and no duplicates, which the first pass had checked by hand. Writing these three tests immediately turned up `` inside `notice-log-in-first` and a literal `<1s` in `ecb-less-than-1s` — neither a bug, both things nobody had looked at. ## Fixed — correctness 6. **A malformed `q=` counted as `1.0`** ([web/src/i18n/mod.rs](web/src/i18n/mod.rs)). `q=banana` outranked every honest preference in the header. Unparseable and out-of-range qvalues are now `0.0` per RFC 9110 §12.4.2; a *missing* `q=` is still `1.0`. Three tests. 7. **`⟪key⟫` shipped to production.** The sentinel had no build-mode gate. It is now `debug_assertions`-only; a release build degrades a missing key to its own words rather than putting debug output on a user's screen. 8. **Runtime-built keys.** `format!("td-money-{mode}")` in two places, which no compiler can check. Both now call the existing `MoneyMode::display_label`, which matches on the enum. 9. **Misleading `#[allow(dead_code)]`** on `language`, `tr`, `tr_args`, `tr_count`, `lookup_args`, and `Nav::locale` — all of them used. Removed; the crate is still warning-free, so they were suppressing nothing but future signal. 10. **Untranslated strings reaching the UI**: the dialog's `aria-label="close modal"`, and the two hardcoded `"Invalid redirect"` bodies. 11. **`&headers` where the parameter was already `&HeaderMap`** in `add_logic`, plus the repeated `Locale::from_headers` in the same function — hoisted to one `let locale`. Clippy is quiet on `web/src` now apart from five pre-existing warnings unrelated to i18n. ## Left alone, deliberately - **`&tr!(...)` temporaries.** The first pass called this "fragile" and implied a use-after-free risk. That was overstated: the failure mode is a compile error, which is the safe direction. Not worth touching. - **`tr_args` / `tr_count` split.** A single `FluentValue`-taking method would be nicer. It is an ergonomics wart, not a defect. - **`tr!` exporting a `$crate::i18n::…` path from a private module.** Fine in-crate; only matters if this ever becomes a library. - **ECB duration units (`d`/`h`/`m`/`s`).** Latin abbreviations, read the same in sk/cs. Worth an explicit decision someday, not a bug today. - **The `cz` alias.** Accepted on input, never emitted. Correct as-is.