fix: 🐛 Resolve translations against every locale, not a hardcoded "en" - #294
Conversation
#287 made the hover show every locale that defines a key, but go-to-definition and diagnostics kept resolving against a hardcoded "en" across all three key shapes. A key defined only in `de` therefore rendered in the hover, refused to navigate, and was still reported missing — and the comment claiming "navigation, diagnostics, and hover all agree on where a key lives" was simply false. All three now resolve against translation_lookup::available_locales, a single shared helper that returns every locale which could define the key, ordered with the project's APP_LOCALE first and the rest alphabetically. Sharing the ordering matters as much as sharing the set: when several locales define a key, goto lands on the same one the hover leads with. Resolution is now by reading the key rather than probing for the file. The dotted and text branches previously accepted a lang file's existence as proof the key existed, so a file that had been created but never populated made diagnostics report a missing key as present and sent goto to a file that never contained it. resources/lang is wired into the resolver itself (resolve_dotted, resolve_namespaced, resolve_text_key) rather than only into discovery. Diagnostics had always checked both lang roots while the resolver checked only lang/, so on a Laravel-8-style project hover could find nothing while diagnostics resolved happily — the same divergence in the opposite direction. APP_LOCALE is read through config::read_env_value, extracted from DatabaseConfigResolver::resolve_env. That reader is deliberately horizontal to stop one variable's value swallowing the next line, a past credential-leak bug; a second hand-rolled regex would risk reintroducing it, so there is now one reader and database.rs delegates to it. The multi-locale card is rebuilt on the HoverContent template with a structured per-locale field instead of a hand-built markdown string. Source links render inline on each locale's line rather than as separate paragraphs, and when exactly one locale *resolves* the key the card collapses to the dense single-block form — most projects ship one locale and a lone value under a heading costs a line to say nothing. A distinct trailer states the key was found in no locale at all, leaving the single-locale card's "default locale" wording intact where it is still accurate. get_all_translation_keys deliberately keeps its first-wins directory selection: completion asks "what keys exist at all", which any single locale answers, and unioning every catalogue per request would read the whole project to produce the same list. Per-hover I/O is still outside Salsa and is now multiplied by locale count; that trade is tracked in #293. Fixes: #288
`available_locales` enumerated the unpublished vendor lang directory without a containment check. That directory is built from a `loadTranslationsFrom` argument in project/vendor source — untrusted input that can point anywhere — so a namespaced key could `read_dir` a tree outside the project and render whatever subdirectories it found there as that key's locale list. `resolve_namespaced_in_dir` has fenced its own read since issue #248; this enumeration is a read site too and was the one that missed the guard. It now takes the same fail-closed `path_within_root` check. Verified by removing the guard: out-of-root `ja`/`ko` directories leak straight into the returned locales. Also corrects the module header, which this branch made false: all three key shapes now resolve under both `lang/` and `resources/lang/`, and the claim that the resolver "only checks the published path" had been untrue since the vendor_map fallback landed. Drops a leftover bare block in `create_translation_location_from_salsa`.
|
Resumed this branch after the previous run stopped before handoff. The implementation was complete against the acceptance criteria; I audited it end to end and found one defect the branch introduced, plus doc drift it made false. Defect — out-of-root directory enumeration. Fixed with the same fail-closed Doc drift. The Cleanup. Removed a leftover bare block in Full suite, |
![mr-sherlock-holmes[bot]](https://avatars.githubusercontent.com/in/4012378?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔄 Changes Requested
Issues Found
Path traversal via unsanitized namespace — resolve_namespaced and available_locales's published-path branch (laravel-lsp/src/translation_lookup.rs:127-149 and :222-225)
Both build a filesystem path (or directory listing) by joining the namespace segment of a translation key — vendor::key, extracted verbatim from parsed PHP/Blade source via split_namespace — with no path_within_root containment check:
resolve_namespaced(127-149) joinsnamespaceintolang/vendor/{namespace}/{locale}/{file}.phpand reads it viaread_php_value→std::fs::read_to_string, unconditionally.available_locales's namespaced branch (222-225) pusheslang.join("vendor").join(namespace)into the dirs itread_dirs, then surfaces whatever subdirectory names it finds as "locales" for the key.
This is the exact class of bug issue #248 already fixed once in this file — the structurally identical sibling resolve_namespaced_in_dir (155-178) guards the same shape of untrusted input with path_within_root, citing #248 by name, and available_locales's own vendor_map branch three lines below (232-236) does the same. The guard just never made it to these two call sites, both of which this PR modified/added.
Reachability is real, not theoretical: salsa_impl.rs:6934-6940 confirms the indexer walks and indexes vendor/*.php/*.blade.php (Composer package source), not just first-party app code, and resolve_translation_detailed tries resolve_namespaced first and unconditionally — ahead of the already-guarded vendor_map fallback. A key like __('../../../../etc::passwd.x') (or an absolute-path namespace) planted in any indexed PHP file — including a compromised dependency — reaches the unguarded read/enumeration with no sanitization anywhere in the call chain. I adversarially verified this end-to-end (red-team built the concrete exploit chain, blue-team found no mitigating check anywhere in the tree, auditor upheld both independently): disclosure is narrowed somewhat by the .php extension requirement and resolve_in_source's "must parse as a return [...] array" filter, but the traversal/read primitive itself is real, and a hit surfaces in the hover tooltip and as a goto-definition navigation target opening the out-of-root file.
Fix: add the same path_within_root(&path, root) guard to both resolve_namespaced and available_locales's namespaced-published-path branch, mirroring resolve_namespaced_in_dir's existing pattern. The existing containment tests (available_locales_refuses_an_out_of_root_vendor_dir, namespaced_dir_outside_root_is_refused) only cover the vendor_map escape path — please add a regression test for this namespace-in-published-path shape too, since that's exactly the gap that shipped untested here.
That's the sole blocker — everything else in this PR is clean.
What's Good
- All 14 acceptance-criteria items are met, including two genuine improvements beyond the minimum bar: AC 11 does a real shared
.env-reader extraction (not just duplicated-but-matching logic), and AC 13's guard on the vendor-dir enumeration actually closes a pre-existing containment gap in the old hover-only discovery loop that predates this PR. available_localesis a clean single source of truth — hover, goto-definition, and diagnostics now genuinely agree on both locale set and ordering, proven by dedicated three-way integration tests (translation_locale_consistency.rs), not just superficially.- Test coverage is thorough and honest: enumeration,
.json-stem extraction, vendor exclusion, missing/empty-dir fallback, vendor-dir union vs. dedup as distinct cases,resources/langdiscovery/resolution parity, both not-found trailers pinned by exact-string tests, full-sequence ordering assertions (not just "first element"), and the single-resolved-locale collapse tested directly against the shared primitive. - CI green across the board:
cargo fmt --check,cargo clippy --all-targets, and the full suite (2417 + 489 + 80 tests) all pass clean.
📋 Non-blocking follow-ups
- None. Two minor test-rigor observations (containment-test fixture shape; a 2-element vs. literal 1-element list in the collapse test) were raised during review and adversarially checked — both refuted: the underlying guard and behavior are already correctly proven, just via a slightly different fixture shape than the reviewer expected. Nothing to track.
Please add the containment guard to both call sites plus a regression test, then re-request review.
The `vendor::` namespace of a translation key is lifted verbatim out of
parsed PHP/Blade source — including from indexed `vendor/**.php`, so a
dependency can choose it. Two sites joined it into a path with no
containment check: `resolve_namespaced` read
`lang/vendor/{namespace}/{locale}/{file}.php`, and `available_locales`
enumerated `lang/vendor/{namespace}`. An absolute or `../`-bearing
namespace escaped the tree, and a hit surfaced in the hover tooltip and
as a goto-definition target.
Rather than patch the two reported sites, route every file read in the
module through one `read_in_root` choke point that fail-closes on a path
it cannot prove inside the root. That covers the dotted-key and JSON
text-key reads too, which had the same shape, and leaves one place a
future read site can be forgotten. `resolve_namespaced_in_dir` drops its
own inline guard, now redundant. Enumeration takes the same guard, moved
to the `read_dir` loop so it applies to every directory rather than only
the `vendor_map` one.
Five tests pin the guard, each verified to redden when it is removed:
absolute and traversing namespaces in the published path, dotted-key and
text-key reads through a symlink that escapes the root, and the
enumeration twin of the published-path case.
|
Blocker fixed in The finding. Correct on both counts. The fix. Every file read in
Tests — 5 new, each mutation-verified to redden when its guard is removed (guards mutated one at a time, so neither test group passes on the other's guard):
Each carries a fixture-wiring assertion that the unguarded path really does reach the planted secret, so none can rot into a test that passes because the join went nowhere. Docs updated where the guard's location moved: the module header now states the invariant for the whole module, and Local: 2422 + 489 + 80 tests green, |
There was a problem hiding this comment.
✅ Approved
Review Summary
Four blind lenses (AC-conformance, correctness, security, test-honesty) ran independently over the current head (1e6108e) — all four came back clean.
- AC conformance: 13/13 met. Every hardcoded
"en"is gone from goto-definition and diagnostics, both now resolve the actual key (not just file existence), hover/goto/diagnostics share oneavailable_localesordering, two new de-only integration tests (dotted + namespaced) prove all three agree,resources/langis wired into the resolver with its own fixture test, the not-found trailer is split into two exact-string-tested constants,translation_card_localesrenders line-per-locale through structuredHoverContentfields (not a hand-built string), the single-resolution collapse is tested directly on the primitive, and APP_LOCALE-led ordering is proven with a full 4-locale sequence plus unset/not-in-set fallback tests.get_all_translation_keys's first-wins divergence fromavailable_localesis the AC's own sanctioned outcome — explicitly documented in-code, not a silent leftover. CI (fmt, clippy, full suite: 2422+489+80 tests) green. - Security — the path-traversal blocker from my last review is genuinely closed. I had the lens independently re-verify (not just trust the commit message): every disk-touching site in
translation_lookup.rsnow routes throughread_in_root/path_within_root, which canonicalizes before comparing — catching..traversal, symlink escapes, and the absolute-namespace-collapses-the-join case alike. The lens reverted the guards in a scratch copy and confirmed all 5 new tests plus the 2 pre-existing regression tests go red without them — real fixtures (a plantedLEAKEDsecret file, not a bareResult::is_err()check). - Correctness and test-honesty: no findings. Both lenses independently mutation-verified the security tests themselves and traced the hover/goto/diagnostics trio for consistency bugs — none found.
Nothing needed adversarial re-verification — every lens returned clean, so there's nothing on the blocker list to check.
📋 Non-blocking follow-ups
- None.
Ready for @mikebronner to merge.
translation_card_locales took each locale as (locale, Option<value>, Option<link>), which admitted a locale that resolved a value but carried no link. That state cannot arise: a value exists only because a file was read to produce it, and that file's path is exactly what the link is built from — source_link returns a String unconditionally, falling back to a backtick-quoted display path when the URL conversion fails. Pair them as (locale, Option<(value, link)>) so the shape says so. The compiler now rejects the impossible combination instead of a test pretending to cover it. Drops the fixture that exercised the valued-but-linkless case, which was documenting a scenario the resolver has no way to produce.
`ensure_external_php_source_loaded` read file metadata and content straight from disk and registered the result as a `SourceFile` that `handle_blade_backing_class_resolution` then emits as a goto-definition target — with no containment check of its own. Every caller pre-vets its paths today, so nothing escapes; the hazard is that the guard lives in the callers, and a new read site does not inherit the guard its neighbours carry. That shape has already become a security fix three times here (#294, #348 rounds 1 and 2). The guard splits by branch, because the branches ask different questions: - The client-ownership fast path reads no disk but emits the path, so it takes `path_within_root_emit_safe` — refuses out-of-root on the lexical pre-gate with no stat probe (#145), still admits a genuinely-absent in-root buffer (#361). - The disk branch reads real bytes, so it takes the fail-closed `canonical_within_root_registration`, and both filesystem calls go through the verified canonical path it returns rather than re-deriving one that a swapped symlink could redirect. Root unknown short-circuits first, before any state is read or mutated. Three test harnesses registered the backend's root but never the actor's `config_root`; `register_project_files` does not set it. Production always registers config first, so this was fixture drift, not a behaviour change — corrected in all three. Fixes #364
…inment guard (#366) * chore: start work on #364 Watson-Branch: #364 * refactor: ♻️ give the actor a constructor its tests can reach `SalsaActor`'s struct literal lived inside the `std::thread::spawn` closure, so no test could ever hold an actor and drive `&mut self` methods against it. Lift it into `SalsaActor::new`; `spawn` keeps the threading, `new` owns the fields. No behaviour change. Prerequisite for #364, whose acceptance criteria require assertions on `files` and `external_php_text` — neither of which any `SalsaHandle` message exposes. * fix: 🔒 put the containment guard on the read, not on its callers `ensure_external_php_source_loaded` read file metadata and content straight from disk and registered the result as a `SourceFile` that `handle_blade_backing_class_resolution` then emits as a goto-definition target — with no containment check of its own. Every caller pre-vets its paths today, so nothing escapes; the hazard is that the guard lives in the callers, and a new read site does not inherit the guard its neighbours carry. That shape has already become a security fix three times here (#294, #348 rounds 1 and 2). The guard splits by branch, because the branches ask different questions: - The client-ownership fast path reads no disk but emits the path, so it takes `path_within_root_emit_safe` — refuses out-of-root on the lexical pre-gate with no stat probe (#145), still admits a genuinely-absent in-root buffer (#361). - The disk branch reads real bytes, so it takes the fail-closed `canonical_within_root_registration`, and both filesystem calls go through the verified canonical path it returns rather than re-deriving one that a swapped symlink could redirect. Root unknown short-circuits first, before any state is read or mutated. Three test harnesses registered the backend's root but never the actor's `config_root`; `register_project_files` does not set it. Production always registers config first, so this was fixture drift, not a behaviour change — corrected in all three. Fixes #364 * fix: 🔒 gate the loader on the module that owns the path Round-1 review of my own guard: gating `ensure_external_php_source_loaded` against `config_root` alone silently dropped every backing class inside a module symlinked in from a composer path repository. `expand_module_dirs` admits that layout on purpose (`config.rs:1180`) and `livewire_namespaces::contained_class_path` gates its registrations against the owning module for exactly this reason (`livewire_namespaces.rs:205`) — so the paths were minted legally and then refused at the read. There is no "component not found" diagnostic, so the only symptom was goto and hover quietly doing nothing. Both branches now gate against `config::owning_module(&self.module_dirs, path)`, falling back to the root. This is not a relaxation: for a module path it TIGHTENS the guard, because a candidate lexically under a module must canonicalize inside THAT module — one reaching into a sibling module or into bare `app/` is refused despite being in-root. `owning_module` collapses `..` before its prefix test, so a traversing path cannot elect itself a laxer gate. With no modules configured the gate is the root and behaviour is unchanged. Five regression tests, each verified to fail under the mutation it pins: the symlinked module loading, the escape out of an elected module gate, the reach back into `app/`, the traversal, and the no-modules case.
1 participant
Fixes #288.
#287 made the hover show every locale that defines a key. Go-to-definition and diagnostics kept resolving against a hardcoded
"en", so a key defined only inderendered in the hover, refused to navigate, and was still reported missing — and the comment claiming "navigation, diagnostics, and hover all agree on where a key lives" was false.What changed
One shared locale set.
translation_lookup::available_locales(root, key, vendor_map)returns every locale that could define the key, ordered with the project'sAPP_LOCALEfirst and the rest alphabetically. Hover, goto and diagnostics all use it — sharing the ordering matters as much as the set, so when several locales define a key, goto lands on the one the hover leads with.Resolution by reading the key, not probing for the file. The dotted and text branches previously accepted a lang file's existence as proof the key existed — so a created-but-unpopulated file made diagnostics report a missing key as present and sent goto to a file that never held it.
resources/langwired into the resolver, not just discovery. Diagnostics had always checked both lang roots while the resolver checked onlylang/, so on a Laravel-8-style project hover found nothing while diagnostics resolved happily — the same divergence in the other direction.One
.envreader.APP_LOCALEgoes throughconfig::read_env_value, extracted fromDatabaseConfigResolver::resolve_env. That regex is deliberately horizontal to stop one variable's value swallowing the next line — a past credential-leak bug — so a second hand-rolled copy was not an option.database.rsnow delegates.The card rebuilt on the template.
translation_card_localesuses a structured per-localeHoverContentfield instead of a hand-built markdown string; links render inline per line rather than as separate paragraphs; and when exactly one locale resolves the key it collapses to the densetranslation_cardform. A distinct trailer states the key was found in no locale at all, leaving the single-locale card's "default locale" wording intact where it is still accurate.Every read in
translation_lookupfenced inside the project root. The paths this module builds join segments taken verbatim from a translation key in parsed PHP/Blade source — including source undervendor/, which the indexer walks, so a dependency can choose them. Two sites joined thevendor::namespace with no containment check:resolve_namespacedreadlang/vendor/{namespace}/{locale}/{file}.php, andavailable_localesenumeratedlang/vendor/{namespace}. An absolute namespace is the sharper shape —Path::joindiscards everything to its left, so the published path collapses to{namespace}/{locale}/{file}.phpwith no../needed.Rather than patch those two sites, every file read in the module now routes through one
read_in_rootchoke point that fail-closes on a path it cannot prove inside the root. That covers the dotted-key and JSON text-key reads, which had the same shape, and leaves a single place a future read site can be forgotten;resolve_namespaced_in_dirdrops its now-redundant inline guard. Enumeration keeps its own guard (it isread_dir, not a file read), moved to theread_dirloop so it applies to every directory rather than only thevendor_mapone.Deliberate decisions
get_all_translation_keyskeeps first-wins and now says why in a comment. It asks "what keys exist at all", which any single locale answers; unioning every catalogue per completion request would read the whole project to produce the same list, and would surface a key from a partially-translated locale as though it were project-wide.Per-hover I/O is still outside Salsa and is now multiplied by locale count. Scoped out of this PR as a separate architectural change; tracked in #293.
Acceptance criteria
check_translation_file→ no"en"; goto → no"en"/en.json/join("en")a_locale_file_without_the_key_is_not_treated_as_a_definitionavailable_locales' set and itsAPP_LOCALE-led orderingde-only dotted key and ade-only namespaced keyavailable_localesadded and used by hover, goto and diagnostics;get_all_translation_keys' divergence documented as an intentional exception.jsonstems,vendorexclusion, missing dir →["en"], empty dir →["en"], vendor-dir union, vendor-dir deduperesources/langsupported in the resolver, with discovery/resolution parity tests for both PHP and JSON shapestranslation_card_localesdirectlyAPP_LOCALEordering via the shared reader, with full-order, unset, and not-in-set testsHoverContentfields, not a pre-built stringtranslation_lookupcarries a fail-closed containment guard — reads via the sharedread_in_rootchoke point, enumeration inavailable_locales(Respect loadTranslationsFrom registrations in app and vendor service providers #248)cargo test,cargo clippy --all-targets,cargo fmt --checkcleanTest plan
Every new test mutation-verified — each goes red when its target breaks:
"en""en"a_locale_file_without_the_key…failsAPP_LOCALEreorderingavailable_locales_leads_with_app_locale…failsread_in_rootcontainment guardnamespaced_dir_outside_root_is_refused,absolute_namespace_in_the_published_path_is_refused,traversing_namespace_in_the_published_path_is_refused,dotted_key_read_through_an_escaping_symlink_is_refused,text_key_read_through_an_escaping_symlink_is_refusedavailable_locales_refuses_an_out_of_root_vendor_dirandavailable_locales_refuses_an_escaping_published_namespacefail — out-of-root locales leak inEach containment fixture carries a wiring assertion that the unguarded path really does reach the planted secret file, so none can rot into a test that passes because the join went nowhere. The two guards were mutated one at a time, so neither test group passes on the other's guard.
The ordering fixture uses
APP_LOCALE=fragainst[en, de, fr, es]deliberately —fris not alphabetically first, so an implementation that ignoredAPP_LOCALEand merely sorted would not pass. An earlier draft usedde, which sorts first anyway and could not discriminate.