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). 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 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 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 <strong>, <code>, <em>, <a>, balanced;
an <a> 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 <a href="/login"> 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

A malformed q= counted as 1.0 (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.
⟪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.
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.
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.
Untranslated strings reaching the UI: the dialog's aria-label="close modal", and the two hardcoded "Invalid redirect" bodies.
&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.

7.1 KiB Raw Blame History Unescape Escape