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

## Verification status

- `cargo check -p web --message-format=short` **passes at HEAD** (0 errors).
  The "20 errors" in the review note describe the state *before* `967335d`;
  the fix-up commit compiles.
- Catalogues: `en`, `sk`, `cs` each contain **677 keys**, and the key sets are
  identical across the three files.
- Duplicate keys: none in any of the three catalogues.
- I did **not** run `cargo test -p web` (per repo agreement). When you run it,
  the i18n unit test worth watching first is
  `browser_language_is_selected_by_quality` in
  [web/src/i18n/mod.rs](/home/priec/Documents/programming/komp_ac/web/src/i18n/mod.rs:137).

## Resolved blockers from the review note

These were the blockers in the review text; their current state at `HEAD`:

1. **`cargo check -p web` 20 errors** — resolved by `967335d`.
2. **`tr` must be imported from crate root** — current code imports
   `use crate::{i18n::Locale, tr}` or uses `crate::tr!`; no
   `crate::i18n::tr` imports remain.
3. **Headers borrowed after move** — no longer a compile error. Locale is
   sometimes precomputed with `let locale = ...` (for example
   [web/src/pages/permissions/users/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/permissions/users/logic.rs:41)),
   sometimes recomputed inline.
4. **`load_error_response` missing `&headers`** — every call now passes a
   `&HeaderMap` (or a value already of type `&HeaderMap`).
5. **Askama passes counts as references vs `tr_count` expecting `i64`** —
   resolved by changing the signature to `count: &i64` in
   [web/src/ui/mod.rs](/home/priec/Documents/programming/komp_ac/web/src/ui/mod.rs:75);
   templates cast with `as i64`.
6. **Askama dynamic keys (`nav.tr("td-money-" ~ ...)`)** — no such `~`
   concatenation remains in the templates.
7. **`|safe` on Fluent messages with variables** — **not resolved**; still
   present (see High section below).
8. **Key parity and plural definitions** — verified, see appendix.

## High — review these before merge

### 1. `|safe` on translated HTML that interpolates attacker-influenced values

Fluent messages contain literal HTML and are rendered with `|safe`, and two of
them interpolate values inside that HTML:

- `grants-inherits-parent` / `grants-inherits-none` interpolate `$role` and
  `$parent`, rendered `|safe` in
  [web/templates/pages/permissions/grants/grants.html](/home/priec/Documents/programming/komp_ac/web/templates/pages/permissions/grants/grants.html:58).
- `builder-reserved-hint` interpolates `$type` inside `<code>`, rendered
  `|safe` in
  [web/templates/pages/add_table/builder.html](/home/priec/Documents/programming/komp_ac/web/templates/pages/add_table/builder.html:267).

Why it matters: `|safe` disables Askama's escaping, so any markup in `$role`,
`$parent`, or `$type` survives. Role/parent values come from the backend and
are not re-validated here; `pending_compound_name()` comes from the posted
`type_input`. The compound branch only renders for a *recognised* compound
type, which limits `$type` today, but the pattern is one small validation
change away from a stored-XSS sink.

Recommended: keep the HTML in the template and translate only the text, e.g.
render `<strong>{{ role }}</strong>` and a separate
`grants-inherits-parent-text` message, then drop `|safe` from these calls.

### 2. Dynamic translation keys are still built at runtime in Rust

The Askama dynamic-key problem was removed, but the Rust side still builds keys
with `format!`:

- [web/src/schema/mod.rs](/home/priec/Documents/programming/komp_ac/web/src/schema/mod.rs:453)
  `crate::tr!(*locale, &format!("td-money-{mode}"))`
- [web/src/pages/add_table/draft.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/add_table/draft.rs:474)
  `tr!(*locale, &format!("td-money-{mode}"))`

`mode` is constrained to `exact` / `half-up` today, so it works, but the
compiler can never prove the key exists and a rename of the enum label becomes
an invisible `⟪td-money-...⟫` on the page. A `match mode` returning the two
fixed keys would be safer and matches the review note's recommendation.

### 3. Accept-Language q-value parsing treats a malformed `q=` as `1.0`

In [web/src/i18n/mod.rs](/home/priec/Documents/programming/komp_ac/web/src/i18n/mod.rs:45):

```rust
let quality = parts.find_map(|parameter| {
    parameter
        .trim()
        .strip_prefix("q=")
        .and_then(|value| value.parse::<f32>().ok())
}).unwrap_or(1.0);
```

`q=` present but unparseable (`q=banana`) falls back to `1.0`, so a broken
header value can outrank a valid preference. Per RFC 9110, an unparseable
qvalue should be treated as not acceptable (`0.0`). It is a small edge case,
but it changes which language a request gets.

Related: `q=0` entries are filtered by `(quality > 0.0)` on line 51, which is
correct, but only after the malformed-q bug above.

### 4. Missing-key fallback leaks raw keys in production

[web/src/i18n/mod.rs](/home/priec/Documents/programming/komp_ac/web/src/i18n/mod.rs:89)
always renders `⟪key⟫` for an unknown key. The comment says it is "visible
during development", but there is no build-mode or env switch; a missing key in
production ships as visible `⟪...⟫` text. Consider a `debug_assertions`-gated
sentinel and a production fallback (English, or the key as a last resort but
not the sentinel).

## Medium — correctness and consistency

### 5. Cross-site check runs after an expensive backend load in several handlers

`reject_cross_site` exists precisely to refuse cross-site POSTs before doing
work, and `add_table` / `table_definition` / `permissions` call it first.
But these handlers load the page (and therefore call the backend) *before*
checking `sec-fetch-site`:

- [web/src/pages/add_logic/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/add_logic/logic.rs:33)
  loads at line 33, checks at line 36.
- [web/src/pages/add_validation/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/add_validation/logic.rs:36)
  loads at line 36, checks at line 39 (same in the rule/set handlers).
- [web/src/pages/import_export/import/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/import_export/import/logic.rs:47)
  loads at line 47, checks at line 51.
- [web/src/pages/import_export/export/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/import_export/export/logic.rs:47)
  loads at line 47, checks at line 51.

Move the check to the top of each handler so a forged cross-site POST cannot
trigger backend work, and so every state-changing handler behaves identically.

### 6. Cross-site logic is duplicated in two shapes

There is the shared helper
[web/src/services/mod.rs](/home/priec/Documents/programming/komp_ac/web/src/services/mod.rs:32)
and two local copies:

- [web/src/pages/add_validation/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/add_validation/logic.rs:293)
- [web/src/pages/import_export/import/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/import_export/import/logic.rs:345)

plus an inline check in `add_logic`. The duplicated version can drift from the
shared one (message, status, or header name). Consolidate on
`services::reject_cross_site`.

Adjacent observation (pre-existing, not introduced by these commits): the
state-changing `POST /login` and `POST /register` handlers do not call
`reject_cross_site` at all, so the helper's doc comment "covers every
state-changing endpoint the same way" is not accurate. Worth a separate look.

### 7. Locale is recomputed dozens of times per request

Most handlers call `Locale::from_headers(&headers)` once per branch rather
than once per request. Examples:

- [web/src/pages/import_export/import/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/import_export/import/logic.rs:61)
- [web/src/pages/import_export/export/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/import_export/export/logic.rs:79)
- [web/src/pages/add_validation/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/add_validation/logic.rs:43)

The review note's suggested shape — `let locale = Locale::from_headers(&headers);`
once, before anything moves — is applied in some files
([web/src/pages/permissions/users/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/permissions/users/logic.rs:41))
but not in others. This is currently correct but noisy, and it makes the
"compute before move" invariant easy to break later.

### 8. Inconsistent double-reference in `add_logic`

[web/src/pages/add_logic/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/add_logic/logic.rs:88)
passes `&headers` to `Locale::from_headers` even though the function parameter
is already `headers: &HeaderMap`:

```rust
Locale::from_headers(&headers)
```

It compiles via deref coercion (`&&HeaderMap` -> `&HeaderMap`) but is a
copy-paste sloppiness. The analogous functions in `add_validation` and
`table_definition` pass `headers` without the extra `&`.

### 9. `&tr!(...)` temporary-reference pattern is fragile

Many UI calls borrow the temporary `String` returned by `tr!`:

```rust
&tr!(Locale::from_headers(&headers), "add-logic-err-permission")
```

It compiles only because the temporary lives long enough for the immediate
render/struct construction. It is correct today but relies on temporary
lifetime extension in expression position; any refactor that stores one of
those `&str` fields will turn into a use-after-free compile error (or worse if
ever `unsafe`). Prefer `let message = tr!(...); ... &message`.

### 10. `tr_count`/`tr_args` numeric and string split is easy to misuse

`tr_args` only accepts `(&str, String)`, while `tr_count` is the only plural
path and takes `&i64`. Templates therefore stringify several numeric values
that are never pluralised, e.g.:

- [web/templates/pages/permissions/users/users.html](/home/priec/Documents/programming/komp_ac/web/templates/pages/permissions/users/users.html:22)
  `.to_string()` for two counts.

This is fine functionally, but a template author has to know in advance whether
a message is plural. A single `tr_args` accepting `FluentValue` would remove the
two-method trap. Low urgency.

### 11. Untranslated bits still reach the UI

Smaller leftovers found while sweeping:

- [web/templates/ui/dialog.html](/home/priec/Documents/programming/komp_ac/web/templates/ui/dialog.html:34)
  hardcoded `aria-label="close modal"` (the footer button is translated, the
  close button is not).
- [web/src/pages/admin/ecb/state.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/admin/ecb/state.rs:36)
  duration units `d`/`h`/`m`/`s` are not localised (`{days}d {hours}h`, ...).
  They are Latin abbreviations, so probably acceptable, but worth an explicit
  decision for `sk`/`cs`.
- `"Invalid redirect"` remains hardcoded in
  [web/src/pages/admin/table_definition/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/admin/table_definition/logic.rs:456)
  and
  [web/src/pages/permissions/common/logic.rs](/home/priec/Documents/programming/komp_ac/web/src/pages/permissions/common/logic.rs:45).
  These are near-impossible to reach (header value parse failure), but they are
  user-visible if they do.

## Low — cleanup and style

### 12. Misleading `#[allow(dead_code)]` markers

`language`, `tr`, `tr_args`, `tr_count`, and `lookup_args` are all used but
carry `#[allow(dead_code)]`:

- [web/src/ui/mod.rs](/home/priec/Documents/programming/komp_ac/web/src/ui/mod.rs:50)
- [web/src/i18n/mod.rs](/home/priec/Documents/programming/komp_ac/web/src/i18n/mod.rs:95)

The attributes suppress real "this is no longer called" signals once templates
stop using these methods.

### 13. `tr!` macro exports a path that is only crate-visible

The macro is `#[macro_export]` but expands to
`$crate::i18n::fluent_value::FluentValue`, while `i18n` is a private module.
Inside this crate that is fine; if the macro were ever used from another crate
it would fail. Not a current bug, just a mismatch to be aware of.

### 14. Czech `cz` alias is accepted but invisible

`from_language_tag` accepts `cz`, but `Locale::code()` always emits `cs`, which
is correct for `lang=` and the catalogue directory. The comment already says
this; leaving it here for completeness.

## Appendix — what actually looks good

- Key parity: all three catalogues have 677 keys with identical key sets.
- Fluent plural forms are correct for `sk`/`cs` (`one`/`few`/`other`), and
  English uses `one`/`other`. No `zero`/`many` gaps for integer counts.
- The locale is threaded per-request through `Nav` and `Locale`, not a global.
- The moved-locale-before-header-move pattern is done correctly where it was
  fixed (permissions handlers, analytics load error).
- The Askama dynamic-key `~` problem is gone from templates.

## Suggested next commands (you run them)

```text
cargo check -p web
cargo test -p web
```

If you want a focused test first:

```text
cargo test -p web browser_language_is_selected_by_quality
```