fix: reject leading-zero semver values in local evaluation#206
Conversation
Per semver 2.0.0 §2, numeric identifiers must not include leading zeros. Values like "1.07.3" are not valid semver and should not match targeting conditions. Both override values and flag values are validated; invalid inputs cause TryParse to return false so the condition does not match.
posthog-dotnet Compliance ReportDate: 2026-05-22 19:25:30 UTC
|
| Test | Status | Duration |
|---|---|---|
| Request Payload.Request With Person Properties Device Id | ❌ | 44ms |
| Request Payload.Flags Request Uses V2 Query Param | ❌ | 22ms |
| Request Payload.Flags Request Hits Flags Path Not Decide | ❌ | 5ms |
| Request Payload.Flags Request Omits Authorization Header | ❌ | 5ms |
| Request Payload.Token In Flags Body Matches Init | ❌ | 5ms |
| Request Payload.Groups Round Trip | ❌ | 5ms |
| Request Payload.Groups Default To Empty Object | ❌ | 5ms |
| Request Payload.Person Properties Distinct Id Auto Populated When Caller Omits It | ❌ | 4ms |
| Request Payload.Disable Geoip False Propagates As Geoip Disable False | ❌ | 4ms |
| Request Payload.Disable Geoip Omitted Defaults To False | ❌ | 4ms |
| Request Payload.Flag Keys To Evaluate Contains Only Requested Key | ❌ | 5ms |
| Request Lifecycle.No Flags Request On Init Alone | ✅ | 3ms |
| Request Lifecycle.No Flags Request On Normal Capture | ✅ | 180ms |
| Request Lifecycle.Two Flag Calls Produce Two Remote Requests | ❌ | 7ms |
| Request Lifecycle.Mock Response Value Is Returned To Caller | ❌ | 4ms |
| Side Effect Events.Get Feature Flag Captures Feature Flag Called Event | ❌ | 5ms |
Failures
request_payload.request_with_person_properties_device_id
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_uses_v2_query_param
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_hits_flags_path_not_decide
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flags_request_omits_authorization_header
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.token_in_flags_body_matches_init
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.groups_round_trip
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.groups_default_to_empty_object
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.person_properties_distinct_id_auto_populated_when_caller_omits_it
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.disable_geoip_false_propagates_as_geoip_disable_false
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.disable_geoip_omitted_defaults_to_false
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_payload.flag_keys_to_evaluate_contains_only_requested_key
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_lifecycle.two_flag_calls_produce_two_remote_requests
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
request_lifecycle.mock_response_value_is_returned_to_caller
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
side_effect_events.get_feature_flag_captures_feature_flag_called_event
404, message='Not Found', url='http://sdk-adapter:8080/get_feature_flag'
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/PostHog/Library/SemanticVersion.cs:149-156
The manual `foreach` digit-check followed by `int.TryParse` validates twice. More importantly, `char.IsDigit` returns `true` for non-ASCII Unicode decimal digits (e.g., Arabic-Indic `١٢٣`), so the loop is both redundant and slightly looser than intended. Using `int.TryParse` with `NumberStyles.None` and `CultureInfo.InvariantCulture` rejects signs, whitespace, and non-ASCII digits in a single pass — no separate loop needed.
```suggestion
return int.TryParse(part, System.Globalization.NumberStyles.None, System.Globalization.CultureInfo.InvariantCulture, out value);
```
### Issue 2 of 2
tests/UnitTests/Features/LocalEvaluatorTests.cs:2404-2437
**Duplicate test body violates OnceAndOnlyOnce**
`ThrowsInconclusiveMatchExceptionForLeadingZeroFlagValueAcrossOperators` has an identical method body to `ThrowsInconclusiveMatchExceptionForInvalidFilterVersion` — the only difference is that the new test also parametrises the `ComparisonOperator`. The existing `ThrowsInconclusiveMatchExceptionForInvalidFilterVersion` could be extended to accept an operator parameter (defaulting to `SemverEquals` for the current cases), exactly like `ThrowsInconclusiveMatchExceptionForInvalidOverrideVersion` already does. That would let all the leading-zero + multi-operator cases be added as `[InlineData]` rows to the existing test instead of duplicating the body in a new method.
Reviews (1): Last reviewed commit: "Reject semver values with leading zeros ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
dmarticus
changed the title
- TryParseSemverNumeric: replace the manual char.IsDigit loop with int.TryParse(part, NumberStyles.None, CultureInfo.InvariantCulture, ...), which is single-pass and correctly rejects non-ASCII decimal digits. - Merge ThrowsInconclusiveMatchExceptionForLeadingZeroFlagValueAcrossOperators into ThrowsInconclusiveMatchExceptionForInvalidFilterVersion by adding a ComparisonOperator parameter, matching the existing ThrowsInconclusiveMatchExceptionForInvalidOverrideVersion pattern.
|
missing changelog entry |
haacked
left a comment
haacked
left a comment
There was a problem hiding this comment.
Looks good to me. Have some non-blocking test case suggestions, but other than that, 🚢
| [InlineData("1.2.03")] // Leading zero in patch | ||
| [InlineData("01.2.3")] // Leading zero in major | ||
| [InlineData("00.0.0")] // Leading zero on a zero major | ||
| [InlineData("v01.2.3")] // Leading zero with v-prefix |
There was a problem hiding this comment.
suggestion: The new ReturnsFalseForInvalidInput rows cover bare leading-zero triples and "v01.2.3", but nothing pins the contract that the leading-zero check survives the upstream pre-release, build-metadata, and outer-whitespace strips. If a future change reorders TryParse so the leading-zero check runs before those strips, today's tests still pass while "01.2.3-alpha" silently goes back to being accepted as 1.2.3. A few rows lock the composition in:
[InlineData("01.2.3-alpha")] // Leading zero + pre-release
[InlineData("01.2.3+build")] // Leading zero + build metadata
[InlineData(" 01.2.3 ")] // Leading zero + outer whitespace| [InlineData("1.-2.3")] // Negative minor | ||
| [InlineData("-1.2.3")] // Negative major | ||
| [InlineData("1.2.-3")] // Negative patch | ||
| [InlineData("01.02.03")] // Leading zeros (all components) |
There was a problem hiding this comment.
suggestion: TryParseSemverNumeric relies on NumberStyles.None + InvariantCulture to reject signs, embedded whitespace, and non-ASCII digits, plus on int.TryParse returning false on overflow. The new leading-zero rows exercise the explicit part[0] == '0' guard; the rest of the helper's contract is only covered by the pre-existing -1.2.3/1.-2.3 sign cases. If someone later swaps in int.Parse(part) or relaxes the styles, every present test still passes. Three rows close the gap:
[InlineData("1. 2.3")] // Embedded whitespace in minor (NumberStyles.None)
[InlineData("١.2.3")] // Arabic-Indic digit in major (InvariantCulture)
[InlineData("99999999999999.0.0")] // Overflows int.MaxValue| [InlineData("not-a-version", ComparisonOperator.SemverEquals)] | ||
| [InlineData("", ComparisonOperator.SemverEquals)] | ||
| [InlineData("abc.def.ghi", ComparisonOperator.SemverEquals)] | ||
| [InlineData(".1.2.3", ComparisonOperator.SemverEquals)] |
There was a problem hiding this comment.
suggestion: The signature change to this test opens the door to running each invalid value under every operator, but the four pre-existing rows ("not-a-version", "", "abc.def.ghi", ".1.2.3") only get SemverEquals. The override-version twin test above already pairs "not-a-version" and "" with SemverGreaterThan. Mirror that here so the non-equals operators stay covered for non-leading-zero malformed input:
[InlineData("not-a-version", ComparisonOperator.SemverGreaterThan)]
[InlineData("", ComparisonOperator.SemverGreaterThan)]
[InlineData("abc.def.ghi", ComparisonOperator.SemverTilde)]
[InlineData(".1.2.3", ComparisonOperator.SemverCaret)]|
|
||
| version = new SemanticVersion(major, minor, patch); | ||
| return true; | ||
| // NumberStyles.None + InvariantCulture rejects signs, whitespace, and non-ASCII digits. |
There was a problem hiding this comment.
suggestion: The deletion of the explicit if (major < 0 || minor < 0 || patch < 0) check is safe only because NumberStyles.None rejects sign characters during parse. That coupling is load-bearing and invisible. The existing comment names the behavior; spelling out the dependency keeps the next person from "tightening" to NumberStyles.Integer and silently re-opening negative-component acceptance.
| // NumberStyles.None + InvariantCulture rejects signs, whitespace, and non-ASCII digits. | |
| // NumberStyles.None + InvariantCulture rejects signs, whitespace, and non-ASCII digits. | |
| // The sign rejection is what lets TryParse drop the explicit negative-component check above. |
| [InlineData("01.2.*")] // Leading zero in major (X.Y.*) | ||
| [InlineData("01")] // Leading zero in implicit major | ||
| [InlineData("01.2")] // Leading zero in implicit X.Y | ||
| [InlineData("1.02")] // Leading zero in implicit minor |
There was a problem hiding this comment.
suggestion: ReturnsFalseForInvalidPatterns covers leading zeros in major/minor and the implicit X/X.Y forms but is missing two cases that ReturnsFalseForInvalidInput does cover for TryParse: a v-prefixed leading zero and a multiple-leading-zero variant. Both paths go through TryParseSemverNumeric, so behavior should already be correct, but the symmetry protects against someone later short-circuiting the wildcard path or moving the strip-v logic:
[InlineData("v01.*")] // Leading zero with v-prefix
[InlineData("001.*")] // Multiple leading zeros| static bool TryParseSemverNumeric(string part, out int value) | ||
| { | ||
| value = 0; | ||
| if (string.IsNullOrEmpty(part) || (part.Length > 1 && part[0] == '0')) |
There was a problem hiding this comment.
nit: This guard packs two independent failure modes into one boolean: empty input and leading zero. A reader has to parse the precedence of && inside || and remember the Length > 1 exception exists for the literal "0" case. Splitting reads more like the spec rules in the <remarks> block:
if (string.IsNullOrEmpty(part)) return false;
// Semver 2.0.0 §2: literal "0" is allowed; "01", "00", "001" are not.
if (part.Length > 1 && part[0] == '0') return false;
return int.TryParse(part, NumberStyles.None, CultureInfo.InvariantCulture, out value);| [InlineData("001.002.003", 1, 2, 3)] | ||
| public void ParsesLeadingZeros(string input, int expectedMajor, int expectedMinor, int expectedPatch) | ||
| // Literal "0" components are valid per semver 2.0.0 | ||
| [InlineData("0.0.0", 0, 0, 0)] |
There was a problem hiding this comment.
nit: [InlineData("0.0.0", 0, 0, 0)] here is already covered verbatim by ParsesValidVersions higher up in the file. Drop this row or swap it for a case that isn't already pinned (e.g., [InlineData("0.0.1", 0, 0, 1)]). The other three new rows do pull their weight.
| } | ||
|
|
||
| // Semver 2.0.0 §2: numeric identifiers MUST NOT include leading zeros. | ||
| static bool TryParseSemverNumeric(string part, out int value) |
There was a problem hiding this comment.
nit: TryParseSemverNumeric reads like "parse a full semver string" but actually parses a single numeric identifier. TryParseSemverNumericIdentifier or TryParseSemverComponent matches the doc comment ("numeric identifiers MUST NOT include leading zeros") more directly. Stylistic.
Split combined empty/leading-zero guard into two ifs, rename helper to
TryParseSemverNumericIdentifier to match its doc-comment terminology, and
document that NumberStyles.None's sign rejection is what lets us drop the
explicit negative-component check.
Lock in helper-contract and composition cases the existing rows didn't
pin: leading zero combined with pre-release, build metadata, and outer
whitespace; embedded whitespace, Arabic-Indic digits, and int overflow
inputs; v-prefixed and multi-leading-zero wildcard rejection. Mirror
the override-version test by running non-leading-zero malformed inputs
under non-equals operators (SemverGreaterThan, SemverTilde, SemverCaret),
and swap the duplicate ParsesLiteralZeroComponents("0.0.0") row for "0.0.1".
3 participants
Summary
Per semver 2.0.0 §2, numeric identifiers MUST NOT include leading zeros. Previously,
SemanticVersion.TryParseandTryParseWildcardusedint.TryParsedirectly, which accepted strings like"01.02.03"and"1.07.3"and quietly converted them to1.2.3/1.7.3. That meant feature flag targeting conditions could match on invalid semver inputs, producing incorrect rollout results.This brings posthog-dotnet in line with the matching fixes in posthog-python (#601) and posthog-go (#200).
Changes
TryParseSemverNumerichelper inSemanticVersionthat rejects empty input, non-digit characters, and any value with a leading zero (except literal"0").TryParse(major/minor/patch) andTryParseWildcard(each numeric segment).TryParse— non-digits are rejected up front.semver_eq,semver_neq,semver_gt,semver_gte,semver_lt,semver_lte,semver_tilde,semver_caret,semver_wildcard) for both override values and flag values."0"components (e.g."0.1.0","1.0.0","0.0.0") remain valid.Invalid inputs cause
TryParseto returnfalse, which causes the higher-level evaluator to raiseInconclusiveMatchExceptionfor that condition — matching the existing behavior for any other unparseable semver value.Test plan
dotnet test -f net8.0— all previously passing tests still pass (2 pre-existing failures inPostHogClientTests.TheCaptureExceptionMethodare unrelated and reproduce onmain)SemanticVersionTestscases cover leading zeros in major/minor/patch, multi-zero, and v-prefixed inputsTryParseWildcardcases reject"01.*","1.02.*","01.2.*","01","01.2","1.02"TheSemverOperatorscases verify rejection across every semver operator for both override and flag values0.0.0,0.1.0,1.0.0,1.2.0) still parse and match