Skip to content

Fix path traversal in BuildLocaleNames and uncaught parse exception in PhoneNumberTypeConverter - #428

Merged
twcclegg merged 1 commit into
mainfrom
fix/release-review-findings
Aug 27, 2026
Merged

Fix path traversal in BuildLocaleNames and uncaught parse exception in PhoneNumberTypeConverter#428
twcclegg merged 1 commit into
mainfrom
fix/release-review-findings

Conversation

@twcclegg

Copy link
Copy Markdown
Owner

Changes

Found during an aggressive re-review of the full diff from v9.0.37 to main (~113 files, everything since the last release — style cleanups, the metadata triage bot, benchmark CI changes, the LocaleData/LocaleNames refactor, new Extensions features).

  • csharp/PhoneNumbers.MetadataBuilder/Program.cs: BuildLocaleNames wrote its per-country output file at Path.Join(outputDir, country.Key) without the Path.GetFileName guard its two sibling call sites (BuildGeocoding, the metadata writer) already got elsewhere in this same range. country.Key comes from resources/locale/country_names.txt, which — unlike every other resource file — is hand-generated locally (lib/DumpLocale.java) rather than copied verbatim from upstream, so it's more exposed to a bad regeneration or manual edit writing outside outputDir. Confirmed the fix is a no-op for real data: still 249 files, same names (US, GB, CA, ...).
  • csharp/PhoneNumbers.Extensions/PhoneNumberTypeConverter.cs: ConvertFrom let PhoneNumbers.NumberParseException propagate straight out of a generic TypeConverter API on invalid input. Reproduced directly — ConvertFrom("not-a-number") threw the raw internal exception type. Standard TypeConverter convention, and this repo's own PhoneNumberAttribute (via PhoneNumber.TryParseValid), is to convert a failed parse into a predictable outcome rather than leak an implementation-specific exception type to callers (WPF/WinForms data binding, TypeDescriptor consumers, etc. won't be catching PhoneNumbers.NumberParseException). Now wraps it in a FormatException with the original as InnerException, plus a test for the invalid-input path that had no coverage before.

Test plan

  • dotnet build csharp — clean, 0 warnings.
  • dotnet test csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0 — 451/451 passing, including the new ConvertFrom_InvalidString_ThrowsFormatException test.
  • Manually inspected obj/locale output after the BuildLocaleNames fix — still exactly 249 files with unchanged names, confirming the Path.GetFileName wrap is behavior-preserving for legitimate data.

Generated by Claude Code

…n PhoneNumberTypeConverter

Found during an aggressive re-review of everything since v9.0.37:

- BuildLocaleNames wrote its per-country output file at
  Path.Join(outputDir, country.Key) without the Path.GetFileName guard its
  two sibling call sites in the same history got (BuildGeocoding,
  BuildMetadata). country.Key comes from resources/locale/country_names.txt,
  which - unlike the other resource files - is hand-generated locally rather
  than copied verbatim from upstream, so it's more exposed to a bad
  regeneration or manual edit writing outside outputDir. Verified the fix is
  a no-op for the real data (still 249 files, same names).

- PhoneNumberTypeConverter.ConvertFrom let PhoneNumbers.NumberParseException
  propagate straight out of a generic TypeConverter API on invalid input -
  reproduced directly (ConvertFrom("not-a-number") threw the raw internal
  exception type). Standard TypeConverter convention, and this repo's own
  PhoneNumberAttribute, is to convert a failed parse into a predictable
  exception rather than leak an implementation-specific type. Now wraps it
  in a FormatException with the original as InnerException, with a test
  covering the invalid-input path that had no coverage before.

Verified: dotnet build (0 warnings), full test suite (451/451 passing).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVGRJyQwtvBGXAmWswxyqW
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.26%. Comparing base (1f10349) to head (78dd2bc).

Files with missing lines Patch % Lines
...honeNumbers.Extensions/PhoneNumberTypeConverter.cs 66.66% 1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (66.66%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
- Coverage   87.27%   87.26%   -0.02%     
==========================================
  Files          41       41              
  Lines        3828     3831       +3     
  Branches      978      978              
==========================================
+ Hits         3341     3343       +2     
- Misses        283      284       +1     
  Partials      204      204              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

2 participants