translations fixed to be safe
This commit is contained in:
Priec
@@ -8,271 +8,141 @@ Scope: the three commits that introduced Fluent i18n on the web crate:
|
||||
|
||||
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
|
||||
```
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user