Skip to content

fix: make national-prefix stripping's null handling provable - #459

Merged
twcclegg merged 3 commits into
mainfrom
fix/codeql-null-safety
Sep 6, 2026
Merged

fix: make national-prefix stripping's null handling provable#459
twcclegg merged 3 commits into
mainfrom
fix/codeql-null-safety

Conversation

@twcclegg

@twcclegg twcclegg commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Both null-related CodeQL alerts, which are the only two open alerts that looked like latent bugs rather than style.

1. cs/dereferenced-value-may-be-nullPhoneNumberUtil.cs:2614

MaybeStripNationalPrefixAndCarrierCode accepts the number in either form and folded both into one length:

var numberLength = numberString?.Length ?? number?.Length ?? 0;
if (numberLength == 0 || !metadata.HasNationalPrefixForParsing) return false;
numberString ??= number.ToString();   // flagged

It cannot actually throw today. Reaching the ToString() requires numberString == null, and in that case the length comes from number?.Length ?? 0, so a null number yields 0 and returns early. Every caller was checked — PhoneNumberUtil.cs:2500, the public overload at :2597, PhoneNumberUtil.cs:2942, and PhoneNumberMatcher.cs:748 — and none passes a null number with a non-null numberString.

It still warranted a change: the safety argument spans two statements and a ?? chain, while every other use of number in the method is null-conditional (number?.Remove, number is not null). The file's own convention says "number may be null here" and this one line dereferences it bare.

The method now branches on which form was supplied, so the dereference sits in the arm where number is provably non-null. Behavior is byte-identical, including the ordering that keeps metadata untouched for a null/empty number and keeps the string un-materialised until after the length and HasNationalPrefixForParsing checks — the hot-path allocation concern AGENTS.md calls out.

Since the change is behavior-preserving there is no test that fails before and passes after, and none is claimed. TestMaybeStripNationalPrefixLeavesEmptyNumberAlone was added to pin the zero-length early return (with and without a carrier code), which is the branch the restructure rewrote.

2. cs/null-argument-to-equalsTestMetadataFilter.cs:892

TestEquals_WhenNull_ReturnsFalse did new MetadataFilter(...).Equals(null). The contract is real and worth asserting — MetadataFilter.Equals is obj is not MetadataFilter other → false, and the test came from aba077bb "add missing coverage over equals method" — but a literal null argument reads as a compile-time constant comparison, which is what the query flags.

The null now goes through an object? local so it is a run-time reference comparison, and the test was widened to the rest of the override's contract: false for a non-MetadataFilter, true for a separately built filter with an equal blacklist, false for ForLiteBuild(). Renamed accordingly. The assertion was kept, not deleted.

Verification

  • dotnet build csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0 — 0 warnings, 0 errors (TreatWarningsAsErrors is on).
  • dotnet test csharp/PhoneNumbers.slnx -p:TargetFrameworks=net10.0 — PhoneNumbers.Test 441 passed (440 before, +1 new), PhoneNumbers.Extensions.Test 51 passed. 492 total, zero failures, nothing skipped.

No public API or parsing behavior changed.

@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 87.69%. Comparing base (c740fc9) to head (fc0b8db).

Files with missing lines Patch % Lines
csharp/PhoneNumbers/PhoneNumberUtil.cs 85.71% 0 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (85.71%) 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     #459      +/-   ##
==========================================
+ Coverage   87.68%   87.69%   +0.01%     
==========================================
  Files          43       43              
  Lines        3889     3893       +4     
  Branches      992      993       +1     
==========================================
+ Hits         3410     3414       +4     
- Misses        276      277       +1     
+ Partials      203      202       -1     

☔ 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.

Comment thread csharp/PhoneNumbers.Test/TestMetadataFilter.cs Fixed
twcclegg and others added 3 commits September 5, 2026 20:27
Two CodeQL correctness alerts, both in code whose null handling was
correct but not locally provable.

cs/dereferenced-value-may-be-null, PhoneNumberUtil.cs:2614. The internal
MaybeStripNationalPrefixAndCarrierCode overload accepts the number as a
StringBuilder, as a string, or as both, and folded the two into a single
`numberString?.Length ?? number?.Length ?? 0` before dereferencing
`number` in `numberString ??= number.ToString()`. The dereference is in
fact unreachable with a null `number` -- reaching it requires
`numberString` to be null, which makes the length come from `number`, so
a null `number` returns early -- but that argument spans two statements
and a `??` chain, and every other use of `number` in the method is
null-conditional, so the code reads as if the ToString() were the one
unguarded access. Branch on which form was supplied instead, so the
ToString() sits inside the arm where `number` is known to be non-null.
Behaviour and the allocation-free early return are unchanged: the string
is still only materialised after the length and national-prefix checks
pass, and a null or empty number still returns false without touching
`metadata`.

cs/null-argument-to-equals, TestMetadataFilter.cs:892. The test asserted
MetadataFilter.Equals(null) is false, which is a real contract worth
holding, but the literal null argument reads as a comparison the compiler
can decide. Pass the null through an `object?` local so the call is a
run-time reference comparison, and extend the test to the rest of the
contract the override implements: false for a non-MetadataFilter object,
true for a separately built filter with an equal blacklist, false for one
with a different blacklist.

Also add TestMaybeStripNationalPrefixLeavesEmptyNumberAlone, covering the
zero-length early return that the restructure touched, with and without a
carrier code requested.
The widened equality test passed a string literal straight to Equals, which
trips cs/equals-on-unrelated-types (error) - CodeQL sees a comparison
between statically incomparable types, which is exactly the smell that rule
exists to catch. Asserting that Equals rejects a foreign type is the one
place the comparison is deliberate, so hold it in an object local, the same
way the null case already does.
Both arms of the numberString/number branch repeated
!metadata.HasNationalPrefixForParsing. Check it once up front - it does not
depend on which form of the number the caller supplied - so each arm is left
with only its own zero-length test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K1NSXom6AbfgVknJDtfR9C
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