Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono] Possible fragile code in /src/mono/browser/runtime/globalization-locale.ts #112573

Closed
IDisposable opened this issue Feb 14, 2025 · 2 comments · Fixed by #112575
Closed
Assignees
Labels
arch-wasm WebAssembly architecture area-System.Globalization in-pr There is an active PR which will close this issue when it is merged

Comments

@IDisposable
Copy link
Contributor

IDisposable commented Feb 14, 2025

In the removal of HybridLocalization in PR #110567 the following code was introduced into globalization-locale.ts

locale = locale.toLocaleLowerCase();
if (locale.includes("zh")) {
    // browser does not recognize "zh-chs" and "zh-cht" as equivalents of "zh-HANS" "zh-HANT", we are helping, otherwise
    // it would throw on getCanonicalLocales with "RangeError: Incorrect locale information provided"
    locale = locale.replace("chs", "HANS").replace("cht", "HANT");
}

Firstly, the Chinese language check should be a check of startsWith( the prefix "zh-", not include( the more ambiguous "zh". This could lead to problems if someone was using the substring zh in any of the other parts of the locale string. To ensure this works with locales formatted with underscores, we need to move up that locale.replace("_", "-") to the first line.

Secondly, per RFC 5646 the script code should be upper-case initial letter and lower-case the rest, so the replacement constants should be "Hans" and "Hant" respectively. Also, since those replacement patterns for those subparts are supposed to be targeting the script indicator, it would be much safer to include the '-' prefix.

In short, this normalizeLocale function should probably be this:

function normalizeLocale (locale: string | null) {
    if (!locale)
        return undefined;
    try {
        locale = locale.toLocaleLowerCase().replace("_", "-");
        if (locale.startsWith("zh-")) {
            // browser does not recognize "zh-chs" and "zh-cht" as equivalents of "zh-Hans" "zh-Hant", we are helping, otherwise
            // it would throw on getCanonicalLocales with "RangeError: Incorrect locale information provided"
            locale = locale.replace("-chs", "-Hans").replace("-cht", "-Hant");
        }
        const canonicalLocales = (Intl as any).getCanonicalLocales(locale);
        return canonicalLocales.length > 0 ? canonicalLocales[0] : undefined;
    } catch {
        return undefined;
    }
}
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Feb 14, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

IDisposable added a commit to IDisposable/dotnet-runtime that referenced this issue Feb 14, 2025
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Feb 14, 2025
@tarekgh tarekgh added the arch-wasm WebAssembly architecture label Feb 14, 2025
Copy link
Contributor

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Feb 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arch-wasm WebAssembly architecture area-System.Globalization in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants