komp_ac/I18N_COMMIT_AUDIT.md

# 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 `<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

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.