diff --git a/I18N_COMMIT_AUDIT.md b/I18N_COMMIT_AUDIT.md new file mode 100644 index 00000000..75079afb --- /dev/null +++ b/I18N_COMMIT_AUDIT.md @@ -0,0 +1,278 @@ +# 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 ``, 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 `{{ role }}` 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::().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 +```