`Grpc.JsonTranscoding` `GetDurationText` unreachable branch / incorrect validation via `IsNormalized` due `toTimestamp` validator reuse

Open Beginner friendly
  • Is something wrong?
#70,342 1 comment 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
69/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
csharp
Domain
api, backend

Research direction

Start with src/Grpc/JsonTranscoding/src/Shared/Legacy.cs and compare GetDurationText validation with the duration sign rules described in the issue. Check the existing tests for duration JSON formatting, then add coverage for a negative subsecond duration such as seconds = 0, nanos = -500000000. Done means valid negative durations serialize correctly and invalid duration values are still rejected.

Written by the indexing model from the issue text.

Description

area-grpc
Is there an existing issue for this?
  • I have searched the existing issues
Describe the bug
Describe the bug

In src/Grpc/JsonTranscoding/src/Shared/Legacy.cs, the GetDurationText(int nanos, long seconds) method erroneously reuses the IsNormalized method to validate a Duration.

However, IsNormalized in this file is a direct copy of the Timestamp validation logic from Google's TimestampPartial.cs, which strictly enforces nanoseconds >= 0. Reusing it inside GetDurationText breaks the serialization of valid negative durations and makes the subsequent nanos < 0 check inside GetDurationText completely unreachable (dead code).

Root Cause Analysis

In the official Google Protobuf C# runtime (protocolbuffers/protobuf), these two types are implemented using partial classes split across generated and manual files:

  1. TimestampPartial.cs enforces nanoseconds >= 0 because timestamps are absolute points in time.
  2. DurationPartial.cs (and JsonFormatter.cs) allows negative nanoseconds because durations can be negative (e.g., -0.5s is represented as seconds = 0, nanos = -500,000,000).

When porting this logic into ASP.NET Core's internal Legacy.cs utility class, the Timestamp version of IsNormalized was accidentally applied to GetDurationText.

As a result:

// Inside Legacy.cs
public static string GetDurationText(int nanos, long seconds)
{
    if (IsNormalized(seconds, nanos)) // <-- Rejects any negative nanos, returning false
    {
        // ...
        if (seconds < 0 || nanos < 0) // <-- This branch is now dead code
        {
            builder.Append('-');
        }
    }
}
Expected Behavior

GetDurationText should either:

  1. Not use IsNormalized and instead implement the correct sign-matching validation rule for Duration (similar to how Google's Duration.ToJson works).
  2. Or use a separate IsNormalizedForDuration validator that allows negative nanoseconds when seconds <= 0.
Steps To Reproduce

Attempting to format a negative duration under 1 second (e.g., -0.5s -> seconds = 0, nanos = -500000000) will fail validation or format incorrectly because IsNormalized will evaluate -500000000 >= 0 to false.

Exceptions (if any)

No response

.NET Version

No response

Anything else?

No response

Dominant language
C#
Stars
38.5k
Forks
11.6k
Avg merge
3d 1h
Merged PRs (30d)
289

Getting set up

Open in Codespaces

Starts the project's dev container in your browser, under your own GitHub account.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from dotnet/aspnetcore

All issues in dotnet/aspnetcore

Similar issues

More C# issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.