filipriec/komp_ac
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity
Files
6ef2694fefc5068f7ce5f4af2e30a48163c0e9a6
komp_ac/I18N_COMMIT_AUDIT.md
Priec 5792a1f533 audit of the i18n deepseek changes
2026-08-14 21:07:56 +02:00

13 KiB
Raw Blame History

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.

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), 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; 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:

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!:

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:

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 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:

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 and two local copies:

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:

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) 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 passes &headers to Locale::from_headers even though the function parameter is already headers: &HeaderMap:

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!:

&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.:

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:

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)]:

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)

cargo check -p web
cargo test -p web

If you want a focused test first:

cargo test -p web browser_language_is_selected_by_quality