Skip to content

Validate l10n ID is non-empty to prevent crash on empty string ID#146

Merged
imnasnainaec merged 8 commits into
masterfrom
fix/104-empty-id
Jun 29, 2026
Merged

Validate l10n ID is non-empty to prevent crash on empty string ID#146
imnasnainaec merged 8 commits into
masterfrom
fix/104-empty-id

Conversation

@imnasnainaec

@imnasnainaec imnasnainaec commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Closes #104

Passing a null, empty, or whitespace string as the id argument to GetString/GetDynamicString produced a malformed or zero-length .xlf file that crashed the application on the next launch.

The fix is in XliffTransUnitUpdater.Update: a guard (mirroring the existing LangId check) now exits early when id is null/empty/whitespace, preventing the corrupt entry from ever being written. The public API methods (GetDynamicString, GetDynamicStringOrEnglish, GetString) return the English fallback text when called with a blank ID rather than attempting a cache lookup or write, so this is a non-breaking fix.

Note on StrictInitializationMode: the early-return in GetString bypasses the StrictInitializationMode check inside GetStringFromAnyLocalizationManager. An app with StrictInitializationMode = true and no managers loaded will now get English fallback text instead of InvalidOperationException when a blank ID is passed. This seems like the right behavior — throwing about missing managers when the real problem is a blank ID would be the wrong signal — but it is an observable change in that scenario.


This change is Reviewable

Devin review: https://app.devin.ai/review/sillsdev/l10nsharp/pull/146

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

Test Results

    7 files  ±  0  124 suites  +20   37s ⏱️ +12s
185 tests +15  180 ✔️ +15    5 💤 ±0  0 ±0 
706 runs  +60  691 ✔️ +60  15 💤 ±0  0 ±0 

Results for commit c7fe0a2. ± Comparison against base commit 69c056e.

♻️ This comment has been updated with latest results.

@imnasnainaec imnasnainaec marked this pull request as ready for review June 5, 2026 15:14
@imnasnainaec imnasnainaec requested a review from andrew-polk June 5, 2026 15:14
@imnasnainaec imnasnainaec self-assigned this Jun 5, 2026
@imnasnainaec imnasnainaec removed the request for review from andrew-polk June 5, 2026 15:15
@imnasnainaec imnasnainaec marked this pull request as draft June 5, 2026 15:15
imnasnainaec and others added 4 commits June 26, 2026 11:36
…pdater (#104)

An empty string ID propagated silently through the system, producing a
malformed .xlf file that crashed the application on next launch. Added
ArgumentException at the GetDynamicString/GetDynamicStringOrEnglish
entry points and a defensive early-return in XliffTransUnitUpdater.Update
mirroring the existing LangId guard.
The CHANGELOG documented GetString as throwing ArgumentException on a null/empty ID, but the guard was only present on GetDynamicString and GetDynamicStringOrEnglish. Add the IsNullOrWhiteSpace guard to both terminal GetString overloads so behavior matches the documented contract.

Expand test coverage: parameterize the GetDynamic* empty-ID tests to cover null and whitespace, add GetString guard tests for both overloads, and add a test confirming UpdateLocalizedInfo with an empty ID adds no entry (covering the XliffTransUnitUpdater.Update guard).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@imnasnainaec imnasnainaec marked this pull request as ready for review June 26, 2026 17:59
@imnasnainaec imnasnainaec requested a review from andrew-polk June 26, 2026 17:59

@andrew-polk andrew-polk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're truly treating this as a breaking change, it needs semver:major in the commit message.

Initially, I was going to approve the general idea, but I'm getting more and more hesitant.
I wouldn't be at all surprised if Bloom (or some other program) is accidentally making use of the fallback behavior and sending null or empty sometimes.

Can we solve instead solve the underlying problem while maintaining the immediate behavior of falling back to english?

I'm not sure what can cause the corrupt file, but can we just exit early, returning the English or ID instead of throwing, for example?

@andrew-polk reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).


CHANGELOG.md line 35 at r1 (raw file):

- [L10NSharp.Windows.Forms] Removed emailForSubmissions parameter (8th parameter) from LocalizationManagerWinforms.Create. Since the localization dialog was jettisoned, it no longer makes sense to store this information on the localization manager.
- [L10NSharp] Replaced the .NET 8.0 target with .NET Standard 2.0 for broader compatibility.
- [L10NSharp] BREAKING CHANGE: `LocalizationManager.GetString`, `GetDynamicString`, and `GetDynamicStringOrEnglish` now throw `ArgumentException` when called with a null or empty string ID. Previously, empty IDs were silently accepted and could produce a malformed XLIFF file that crashed on next launch.

This says null or empty, but the actual check is whitespace.

@imnasnainaec imnasnainaec marked this pull request as draft June 26, 2026 18:15
imnasnainaec and others added 4 commits June 29, 2026 09:57
…back (#104)

Per reviewer feedback, drop the breaking-change throws from the public API.
The XLIFF corruption is prevented by the guard in XliffTransUnitUpdater.Update
(widened to IsNullOrWhiteSpace); callers passing a blank ID now receive the
English fallback text unchanged instead of an exception.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…overload; clean up test null-coalesce

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@imnasnainaec imnasnainaec left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Went for the English/empty fallback instead of the throw.

@imnasnainaec made 2 comments.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on andrew-polk).


CHANGELOG.md line 35 at r1 (raw file):

Previously, andrew-polk wrote…

This says null or empty, but the actual check is whitespace.

Done.

@imnasnainaec imnasnainaec marked this pull request as ready for review June 29, 2026 14:29
@imnasnainaec imnasnainaec requested a review from andrew-polk June 29, 2026 14:37

@andrew-polk andrew-polk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrew-polk reviewed 4 files and all commit messages, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).

@imnasnainaec imnasnainaec merged commit f008c92 into master Jun 29, 2026
6 checks passed
@imnasnainaec imnasnainaec deleted the fix/104-empty-id branch June 29, 2026 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Empty string for l10n ID causes crash

2 participants