Fix IndexOutOfRangeException for query segments without '=' in ChorusHubServerInfo#379
Merged
Merged
Conversation
imnasnainaec
marked this pull request as ready for review
June 22, 2026 18:59
imnasnainaec
marked this pull request as draft
June 23, 2026 10:32
imnasnainaec
marked this pull request as ready for review
June 23, 2026 17:41
rmunn
reviewed
Jul 2, 2026
imnasnainaec
added a commit
that referenced
this pull request
Jul 2, 2026
…g them Addresses review feedback on #379: a segment without '=' (e.g. 'flag') is now stored with an empty-string value, distinguishable from an absent parameter (which GetValue reports as "?"). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
imnasnainaec
force-pushed
the
fix/chorushub-query-parse
branch
from
July 2, 2026 13:03
988875b to
c536738
Compare
… query flags String.Split always returns at least one element, so the guard `parts.Length < 1` was unreachable. Any query segment without '=' would throw on `parts[1]`. Change the guard to `parts.Length < 2` so those segments are skipped safely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Splitting on all '=' would silently drop everything after the second '=' in a value like key=val=ue. Use Split(..., 2) so the key is parts[0] and the full remainder (including any embedded '=') is parts[1]. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The hand-rolled split loop had two bugs (unreachable guard for missing '=', truncated values containing '='). Replace it with HttpUtility.ParseQueryString, consistent with how UrlHelper parses query strings elsewhere in the library. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
HttpUtility.ParseQueryString URL-decodes values, so ToString() must URL-encode them to avoid silent corruption on a round-trip. Use Uri.EscapeDataString on all string fields. Also remove the now-unused System.Collections.Specialized import left over from the old manual parser. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
HttpUtility.ParseQueryString introduced a round-trip asymmetry (URL-decoding on parse but no encoding on serialization), and fixing that with Uri.EscapeDataString would encode IPv6 colons, breaking old clients. Since all values are machine-generated and will never contain '+' or '%', the manual parser is correct as-is with the two fixes: guard parts.Length < 2 to skip bare flags, and Split(..., 2) to preserve '=' in values. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e fix Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g them Addresses review feedback on #379: a segment without '=' (e.g. 'flag') is now stored with an empty-string value, distinguishable from an absent parameter (which GetValue reports as "?"). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
imnasnainaec
force-pushed
the
fix/chorushub-query-parse
branch
from
July 2, 2026 13:46
c536738 to
91ee5ad
Compare
imnasnainaec
commented
Jul 2, 2026
imnasnainaec
left a comment
imnasnainaec
left a comment
Contributor
Author
There was a problem hiding this comment.
@imnasnainaec reviewed all commit messages and resolved 1 discussion.
Reviewable status: 0 of 3 files reviewed, all discussions resolved.
Contributor
|
This is perfect now: fixes the bug without discarding valueless parameters, so that |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs in the manual query-string parser in `ChorusHubServerInfo.GetValue`:
Unreachable guard: `parts.Length < 1` was never true — `String.Split` always returns at least one element. A bare flag segment (no `=`) would throw `IndexOutOfRangeException` on `parts[1]`. Fix: change the guard to `parts.Length < 2`.
Truncated values: Splitting on all `=` characters turned `key=val=ue` into a 3-element array; only `parts[1]` was used, silently dropping everything after the second `=`. Fix: use `Split(new[] { '=' }, 2)` so the full remainder becomes `parts[1]`.
`HttpUtility.ParseQueryString` was considered but rejected: it URL-decodes values while `ToString()` does not URL-encode them, creating a round-trip asymmetry. Fixing that with `Uri.EscapeDataString` would encode colons in IPv6 addresses, breaking old clients. Since all values are machine-generated and will never contain `+` or `%`, the manual parser is correct with just these two fixes.
Fixes #372
Devin review: https://app.devin.ai/review/sillsdev/chorus/pull/379
This change is