fix(web): sanitize unparseable browser locale before engine bootstrap - #370
Conversation
An unparseable navigator.language (POSIX "C"/"POSIX", underscored "en_US", or empty) crashes the Flutter web engine's own bootstrap (EnginePlatformDispatcher_parseBrowserLanguages, called from CanvasKitRenderer.initialize) with an uncaught RangeError before runApp — an unrecoverable blank page with nothing logged. The crash is inside the engine's compiled JS, before Firebase.initializeApp or RustLib.init() ever run, so no app-level try/catch can guard it. Sanitize navigator.language/languages in web/index.html, before flutter_bootstrap.js loads: normalize underscored tags, fall back to en-US for anything Intl.DateTimeFormat still rejects. By the time the engine reads it, the browser only ever reports a valid tag. Add a CI regression case: web-build.yml reruns the release-bundle smoke test with the locale forced to "C" (smoke.mjs now honors SMOKE_LOCALE). Verified both directions locally against the real bundle — fails without the fix, passes with it. Fixes MostroP2P#227
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Catrya
left a comment
There was a problem hiding this comment.
Approved. I reproduced the crash end to end on the Flutter version CI pins, confirmed the fix removes it, and confirmed the new CI step genuinely fails without it. The diagnosis in the description is correct down to the frame.
End-to-end A/B (Flutter 3.38.2, Chromium 141)
Same built bundle in both runs — the only difference is whether the sanitizer script is present in index.html, stripped at serve time — with Playwright forcing locale: 'C':
| sanitizer present | sanitizer removed | |
|---|---|---|
navigator.languages |
"en-US" |
"C" |
flutter-view mounted |
yes | no |
RangeError: Incorrect locale information provided |
no | yes |
<body> children |
flt-semantics-placeholder, script, script, flt-announcement-host, flutter-view |
script, flt-announcement-host |
Two runs each, identical results. Without the fix the engine dies mid-bootstrap and the flutter-view element is never created — the page is not merely empty, the view never exists. That last row is also why the new CI step is not decorative: smoke.mjs asserts flutter-view mounts and that no page errors occurred, and both fail here.
Why this only reproduces on the pinnedWorth recording, because it nearly sent me pt showed no crash at all — I was buildingwith Flutter 3.35.7. The engine changed bet
- 3.35.7:
parseBrowserLanguagesdoes ldsui.Localein pure Dart.
ui.Locale('C')validates nothing and neve - 3.38.2 (
FLUTTER_VERSIONin all threomLocale(language), andDomLocaleis declared@js('Intl.Locale')(engine/src/gine/dom.dart:2448-2450). That is exactly thenew Locale ()` frame in you
Measured in Chromium 141: new Intl.Locale(orrect locale information provided — the
message verbatim. So this is a regression ie, not by this app, and it is invisible to
anyone still on 3.35.
One consequence for the CI guard: what makee change. A line in the workflow comment
saying so — the guard tests Intl.Locale, ince 3.38 — would keep it from quietly
becoming a no-op if that parser ever change
The validity oracle matches the engine'
The sanitizer validates with Intl.DateTimeFormat; the engine throws from Intl.Locale. I checked they agrsince a mismatch is how this fix could sile POSIX, en_US, en-US, c, zz, "", und, x, en-US-POSIX, both accept or both reject, all ten. The oracle is sound. ## Minor- **POSIX is not an invalid tag, and the * Measured: Intl.Locale('POSIX'),Intl.DateTimeFormat('POSIX') and Intl.gell succeed — it is a structurally validfive-letter language subtag. With Playwrighnavigator.languagesis still["POSIX"]after the script runs. That is the correct behaviour, since it never crashed anything, but the description says thesanitizer "falls back to en-US for anythi, empty)", which is wrong about POSIX.Issue #227 carries the same imprecision.- Two of the three acceptance-criteria ta Playwright, so the CI guard really onlycovers C. Measured:
locale "C" -> the browser reports "Cs intended)
locale "en_US" -> the browser reports "en-US" (Playwright normalizes before the page sees it)
locale "" -> the browser reports "e is used)
The footnote gets close for en_US but a; the simpler fact is that the underscoredtag never reaches the page at all. Worth stating so nobody reads those two criteria as guarded.
- **Neither
Object.definePropertycall issValidBcp47is; the writes are not. If abrowser ever refused to shadownavigator.language`, the inline script would throw — before the engine, and in
exactly the embedded/kiosk environments thit's "no page errors" assertion would fail on it. Cheap hardening for a script whose whole job is to run in hostile environments.
Also verified
- Script ordering in the built bundle, serviceworker
at offset 1426, sanitizer at 2060,flutter_bootstrap.jsat 3717. The sine script and the bootstrap isasync`, soit always runs first. - The repo's own web guard still passes.dart
→ 16/16, including the assertion that the isolation shim precedesflutter_bootstrap.js. - **Full suite on the merged tree, built wi **293 passed, 0 failed**;flutter analyze` → zero issues in hand-written code. Goldens included.
Not verified
- **Screenshots do not distinguish the two because my bundle has no Rust core(`web/pkg` needs `wasm-pack`, which I did n engine boots but `main()` then fails at`RustLib.init()` and paints nothing. The DOve is the evidence; a real side-by-sidepicture would need the wasm core.
- The repo's actual
smoke.mjscould nitsmostroBridgeReadyassertion needs theRust bridge. I used a targeted checker over the same bundle instead, serving it cross-origin isolated under/app/as the real one does. - The
""anden_UScases, per the Playwright limitation above.
…ening Catrya's review of MostroP2P#370. No behavior change to the sanitizer itself beyond never throwing. - Guard both Object.defineProperty writes with try/catch. A browser may expose navigator.language(s) as non-configurable; an uncaught error there would be the blank page this script exists to prevent, in exactly the locked-down environments it targets. Both writes share the block so a partial shadow cannot leave language and languages disagreeing. - Correct the POSIX claim. "POSIX" is a structurally valid five-letter language subtag: Intl accepts it and it passes through untouched, unlike the single-letter "C". Verified both oracles (Intl.DateTimeFormat, used here, and Intl.Locale, which the engine throws from) agree on all of C/POSIX/en_US/en-US/c/zz/""/und/x/en-US-POSIX. - Record that the CI guard only exercises "C": Playwright normalizes the locale before the page sees it, so "en_US" arrives as a valid "en-US" and "" falls back to the default. - Record that the crash only exists on Flutter >= 3.38, where parseBrowserLanguages moved from a pure-Dart ui.Locale that validated nothing to Intl.Locale — so the guard silently stops testing anything if FLUTTER_VERSION moves below it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011kR2ZS83zniZMXankK1Fry
|
Review round 1 — pushed in e90568a. Thanks for the measurements, they caught 1. try/catch around both 2. POSIX corrected. You're right, and I reproduced it: 3 & 4. Both recorded in the workflow comment: that Playwright normalizes Verified by handThe app was run in a real Chrome ( The broken-locale case was also reproduced by hand, though not in a browser Verified by scriptYou noted your bundle had no Rust core, so I ran the A/B you couldn't: a real
Full pass means all four assertions, Rust bridge call included. One red cell, Independently, the browser-side script was exercised against |
Catrya
left a comment
There was a problem hiding this comment.
Changes requested — light. The diagnosis is right, the fix is at the only layer that can work, and I reproduced the differential against a real bundle. What needs to change is the issue linkage and two lines in the sanitizer.
The root cause is confirmed at source level
Not just from the stack trace — this is Flutter 3.38.2's own SDK, flutter_web_sdk/lib/_engine/engine/platform_dispatcher.dart:916:
static List<ui.Locale> parseBrowserLanguages() {
final List<String>? languages = _browserLanguagesOverride ?? domWindow.navigator.languages;
if (languages == null || languages.isEmpty) { return const <ui.Locale>[_defaultLocale]; }
for (final String language in languages) {
final DomLocale domLocale = DomLocale(language); // ← throws hereand dom.dart:2443:
@JS('Intl.Locale')
extension type DomLocale._(JSObject _) implements JSObject {
external DomLocale(String tag, [DomLocaleOptions? options]);It runs from EnginePlatformDispatcher's field initializer, i.e. engine bootstrap, before runApp. Your conclusion that no app-level try/catch can reach it is correct, and the HTML shim is the only lever. Good call.
One thing that falls out of this and matters below: the engine reads navigator.languages only. It never touches navigator.language.
Reproduced against a real release bundle
I built the branch (build-web.sh --release + flutter build web --release --base-href "/app/" --pwa-strategy=none, Flutter 3.38.2) and ran your smoke.mjs four times, stripping only the sanitizer <script> from build/web/index.html between runs so both sides come from one build:
SMOKE_LOCALE=en-US |
SMOKE_LOCALE=C |
|
|---|---|---|
| with sanitizer | ✓ isolated, ✓ view mounted | ✓ isolated, ✓ view mounted |
| without sanitizer | ✓ isolated, ✓ view mounted | ✓ isolated, ✗ the Flutter view never mounted + RangeError: Incorrect locale information provided |
Exactly one of the four dies earlier than the others, and it is the cell your table predicts. The differential holds.
Caveat, stated plainly: my bundle had no web/pkg — building the wasm core here needs a rustup component I did not want to install on this machine — so smoke assertion 3 (the Rust bridge) fails in all four runs. It is a constant across the matrix and does not affect the locale comparison, but it means I could not independently confirm your "passes fully, Rust bridge call included". CI is green on your side, which covers it.
The Intl.DateTimeFormat proxy is sound
Worth recording, since the sanitizer validates with Intl.DateTimeFormat while the engine parses with Intl.Locale, and nothing guarantees those agree. I checked 33 tags — grandfathered (i-klingon, zh-min-nan, art-lojban, en-GB-oed, cel-gaulish, sgn-BE-FR), extensions (en-US-u-ca-gregory, ja-JP-u-ca-japanese), private use (qaa-Qaaa-QM-x-southern, x-private), malformed (en-, -en, en US, C.UTF-8, en_US@posix, *, 12, ab-CDE, en-a-bbb-a-ccc):
divergences between Intl.DateTimeFormat and Intl.Locale: 0
Both are gated by the same structural validity check in ECMA-402, so this is not a coincidence that could drift. No gap.
And the no-op path does what it claims — measured in Chromium: en-US, POSIX and es-419 come out byte-identical, no defineProperty attempted.
Blocking 1 — the atomicity comment is not true
From round 1:
// Both writes go together so a partial shadow can't leave language and
// languages disagreeing.It can. With navigator.languages exposed as non-configurable — the exact case the try/catch exists for:
before: { lang: 'C', langs: ['C'] }
after : { lang: 'en-US', langs: ['C'] }
warn : TypeError: Cannot redefine property: languages
The first defineProperty has already applied when the second throws, and the catch cannot undo it. So the comment states a guarantee the code does not provide.
The ordering makes it worse rather than harmless: languages is the one the engine reads, and it is attempted second. In the partial-failure case the fix does nothing and leaves the two properties disagreeing — the worst of both. Two options, either is fine:
- write
languagesfirst, thenlanguage— then a partial failure has already fixed the crash and only leaveslanguagestale; or - check
Object.getOwnPropertyDescriptor(navigator, ...).configurablefor both before writing either, and bail if either is locked.
Then the comment matches the code.
Blocking 2 — "Fixes #227" with two acceptance criteria unimplemented
The issue lists six. Two are not met on this branch:
- "No unhandled exception escapes
main(); a locale that cannot be used falls back to a default and is logged rather than thrown" — there is no top-level guard. This locale is normalized before it can throw, which is not the same thing. - "
Firebase.initializeAppcannot take down startup for any reason" —lib/core/app_bootstrap.dart:53is stillon UnsupportedError catch (e), unchanged. The AC asks for hardening, not for a verdict on whether Firebase caused this particular crash; the checkbox is ticked with reasoning that answers the second question.
The issue is explicit that this is the part that matters most — "2. Make startup survivable regardless of where it turns out to be. ... The second is the one that generalises." As it stands, merging auto-closes #227 with its generalising half undone.
I'd rather not see it folded in here: the sanitizer is a clean, self-contained change and the startup guard is a different concern. So: change the body to "Part of #227" and open a follow-up for the top-level guard + the Firebase catch, or say in the issue why those two are being dropped.
Suggestions
-
pages_bundle_test.dartdoesn't guard the script order. That file exists for exactly this class ("Every one of these, when wrong, yields a blank page — guarded statically"), and it already assertsshimAt < bootstrapAt. One more index comparison would cover the sanitizer, and unlike the CI step it cannot pass for the wrong reason. -
The CI locale step has no negative control. Your own comment admits it can go green for the wrong reason if Flutter changes.
selftest.mjsstates the principle already — "The healthy fixture is the control: it is what makes a failure in the other two mean 'the error was detected'". A second run against a copy ofbuild/webwith the<script>stripped, expecting exit 1, makes the guard self-validating. That is exactly the run I did by hand above, and it works. -
SMOKE_LOCALEusesprocess.env.SMOKE_LOCALE || 'en-US', soSMOKE_LOCALE=''silently becomesen-US— the empty-locale case can't be expressed through the knob its name advertises.??fixes it.
One thing you got right that's worth recording
The issue told you to add the case to selftest.mjs's CASES table. That would have been wrong — selftest.mjs runs against three static HTML fixtures with no Flutter engine in them, so a locale case there would prove nothing. Putting it in web-build.yml against the real bundle is the correct place. Worth a line in #227 so the next person doesn't "fix" it back.
What I verified
- The crash site in the Flutter 3.38.2 SDK sources, including that the engine reads
navigator.languagesand nevernavigator.language. - 4 smoke runs on a real release bundle, both locales × with/without the sanitizer.
- 33-tag agreement between
Intl.DateTimeFormatandIntl.Locale. - Sanitizer behaviour in Chromium for
C,c,POSIX,es-419,en-US, plus the non-configurable-languagescase. - That Playwright's
locale: 'C'genuinely reaches the page asnavigator.language === 'C'— so the CI step exercises the real broken state — and both of your caveats:en_USis normalized toen-USbefore the page sees it, and''falls back to the system locale. - The sanitizer survives
flutter build webintobuild/web/index.htmlverbatim, with script ordercoi-serviceworker→ sanitizer →flutter_bootstrap.js, so the "shim must stay first" constraint andpages_bundle_test.dartboth still hold. - All three CI checks green on
e90568a.
…ontrol Blocking 1: the sanitizer's try/catch was not the transaction its comment claimed. If the first defineProperty applied and the second threw, the first could not be undone — and since the engine reads navigator.languages only, a partial shadow both left the two properties disagreeing and failed to fix the crash. Check that neither is a non-configurable own property before writing either, and bail out with a warning if one is locked. Nothing can rescue that case, but the fix no longer half-applies. Suggestions: - web/index.html carries a <!-- locale-sanitizer --> marker and pages_bundle_test.dart asserts shim < sanitizer < flutter_bootstrap.js. Unlike the CI step, a static check cannot pass for the wrong reason; verified it fails when the script is moved after the bootstrap. - web-build.yml runs a negative control after the invalid-locale smoke test: the same bundle with the sanitizer stripped must fail under locale "C". Without it that step goes green whether or not the fix is present or still needed. The strip is asserted to have happened, so a no-op cannot turn the control into a rubber stamp. - smoke.mjs uses `??` instead of `||` for SMOKE_LOCALE, so the empty locale — one of the three broken tags — can actually be expressed instead of silently becoming en-US. Part of MostroP2P#227
grunch
left a comment
There was a problem hiding this comment.
Review — strict pass, verified on the merged tree
Verdict: ✅ Approve. No critical or high findings. Two medium findings on the new CI control step, one suggestion that would close the "CI only covers C" gap for real, two nits. None of them changes the sanitizer's behaviour, which is correct.
What I verified
- Merge with
main(0ce3d14) is clean; the four files are untouched onmain. test/web/pages_bundle_test.darton the merged tree: 17/17, including the new ordering guard (shim → sanitizer →flutter_bootstrap.js).- CI on
09b4859: all four checks green, including the new positive step and the negative control, so the control genuinely fails without the sanitizer today. - The app reads locales from the sanitized source only. The Dart side derives its language from
WidgetsBinding.instance.platformDispatcher.locales(settings_provider.dart:144), which the engine populates fromnavigator.languages. There is nofindSystemLocale()/Intl.systemLocalepath reading the rawnavigator.languagebehind the sanitizer's back — I grepped for it. - Round-2 changes hold up.
isLockedDownon both properties before either write closes the partial-shadow hole from round 1; the residualtry/catchis now reachable only for a non-extensiblenavigator(Object.preventExtensions), where the firstdefinePropertythrows before applying, so no partial write is possible there either. The comment above it could say that in one line instead of reading as belt-and-braces for the case it just excluded — nit, no change required. - No CSP in
web/index.html, so the inline script is not blocked; nothing in the change toucheslib/src/rust/or the bridge surface.
Findings (ranked)
| # | Severity | Where | Summary |
|---|---|---|---|
| 1 | web-build.yml:244 |
The control treats any non-zero exit as "died of the locale bug". Grep the log for the RangeError signature. |
|
| 2 | web-build.yml:244 |
The control burns the full 120 s TIMEOUT_MS on every PR (measured: 2 min 1 s vs ~1 s for the positive steps). Cap it with SMOKE_TIMEOUT_MS. |
|
| 3 | 💡 Suggestion | smoke.mjs:173 |
addInitScript can shadow navigator.languages before the sanitizer runs, covering en_US, "" and mixed lists in CI — the Playwright locale limitation is not a browser limitation. |
| 4 | 🔧 Nit | index.html:89 |
Object.freeze the shadow array to match the spec-shaped navigator.languages. |
Findings 1 and 2 are worth doing before merge because they are small and the control is the part of this PR most likely to rot silently; 3 can be a follow-up. I'm approving now so the choice is the author's.
Recorded for #227
Agree with the earlier round that "Part of #227" is the right linkage: the top-level main() guard and the app_bootstrap.dart:53 Firebase catch remain open there.
…hadow Three findings from grunch's review of 09b4859. None changes the sanitizer's behaviour on any locale. 1. The negative control accepted any failure as the expected one. It only checked the exit code, and smoke.mjs exits non-zero for five unrelated reasons — plus the ones where Chromium never launches or the port is taken — so the control could report "failed as expected" while proving nothing about this bug. It now captures the run's output and requires the RangeError signature in it, failing the job when the bundle dies of anything else. Verified against a real release bundle: the signature is present, so the control has been failing for the right reason; the risk was future rot, not a current hole. 2. That control burned the full 120s TIMEOUT_MS on every PR, waiting on a flutter-view element the crash guarantees will never appear. Capped at 20s for this step only — the positive steps keep the generous default, which they need to instantiate the wasm core on a cold runner. Measured: 121s -> 21s. 3. The shadow array is now frozen, matching the HTML spec's guarantee for navigator.languages. Both assignments, since the empty-result branch is the one the "C" case actually uses. grunch's third finding — shadowing navigator.languages via addInitScript so CI can cover en_US and "" as well, which Playwright's locale option cannot reach — is left as a follow-up, as he suggested. Verified: pages_bundle_test.dart 17/17; smoke.mjs against a real release bundle (build-web.sh --release + flutter build web --release, Flutter 3.38.2) passes fully under both en-US and C with the sanitizer, and the control fails with the RangeError without it.
Summary
Part of #227: on web, an unparseable browser locale (the POSIX
Clocale, an underscoreden_US, or empty) crashed the app beforerunAppwith a blank page and nothing logged.Root cause, pinned down with a full stack trace (the original issue narrowed it to "somewhere in early
main()" but not further):RangeError: Incorrect locale information provided
at new Locale ()
at Object.EnginePlatformDispatcher_parseBrowserLanguages (main.dart.js:2494:21)
at Object.EnginePlatformDispatcher$ (main.dart.js:2471:14)
at CanvasKitRenderer.initialize$0 (main.dart.js:73965:16)
This is inside the Flutter web engine's own bootstrap (
CanvasKitRenderer.initialize), not in this app's code — it fires beforeFirebase.initializeApporRustLib.init()ever run. That rules out both of the issue's other suspects and means no app-leveltry/catcharoundmain()can guard it; the fix has to happen before the engine reads the locale at all.Fix
Added a small inline script to
web/index.html, beforeflutter_bootstrap.jsloads (same pattern already used there forcoi-serviceworker.min.js): it validatesnavigator.language/navigator.languageswithIntl.DateTimeFormat(the same check that throws inside the engine), normalizes underscored tags (en_US→en-US), and falls back toen-USfor anything still invalid (C, empty). NotePOSIXis not one of those: unlike the single-letterC, it is a structurally valid five-letter language subtag, soIntlaccepts it and it passes through untouched — correctly, since it never crashed anything. If the shadowing itself is refused by the browser, the script warns and leaves the locale alone rather than throwing. By the time the engine reads it, the browser only ever reports a valid tag.Testing
Manual, against a real release bundle, both directions — with
navigator.language(s)shadowed toCbefore the sanitizer runs, since Chrome's DevTools rejectsCin its locale override field ("Locale must contain alphabetic characters"):RangeError: Incorrect locale information providedin the console,flutter-viewnever created.navigator.languagesreports["en-US"]and the app renders.""(empty locale).Cthemselves (they self-sanitize), which is why this only shows up in CI/headless/embedded environments — matches the "low severity" framing in the issue.Automated regression guard:
smoke.mjsnow reads the locale fromSMOKE_LOCALE(defaults toen-US, unchanged).web-build.ymlreruns the release-bundle smoke test withSMOKE_LOCALE=Cright after the existing one. Verified locally against a real bundle (build-web.sh --release+flutter build web --release, Flutter 3.38.2), stripping only the sanitizer<script>from the builtindex.htmlbetween runs so both sides come from the same build:en-USC"Passes fully" means all four smoke assertions, Rust bridge call included.
Acceptance criteria (from #227)
C,en_US†, and""— verified against a real release bundle; the CI step only exercisesC, since Playwright normalizes the other two before the page sees themmain()for a bad locale — it's normalized before the engine ever sees itFirebase.initializeAppwas already unaffected — the crash happens earlier, before that line runslocale: 'en-US'pin insmoke.mjskept for determinism, documented as no longer load-bearing for this bug†
en_USgets normalized toen-USby the sanitizer like any other case; a separateAccept-Language: en_USfetch failure I hit while testing that one specific tag turned out to be a pre-existing Playwright/Chromium quirk unrelated to this fix (reproduces identically with the sanitizer removed) — not covered by this PR.