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

Abierto Apto para principiantes
  • ¿Algo no está bien?
#70,342 1 comentario 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
2/5
Tiempo estimado
1-3 horas
Aptitud para principiantes
69/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
csharp
Área
api, backend

Línea de trabajo

Empieza con src/Grpc/JsonTranscoding/src/Shared/Legacy.cs y compara la validación de GetDurationText con las reglas de signo para duraciones descritas en el issue. Revisa las pruebas existentes para el formato JSON de las duraciones y añade cobertura para una duración negativa inferior a un segundo, como seconds = 0, nanos = -500000000. El trabajo estará terminado cuando las duraciones negativas válidas se serialicen correctamente y los valores de duración no válidos se sigan rechazando.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

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

Lenguaje dominante
C#
Estrellas
38.5k
Forks
11.6k
Merge medio
3 d 1 h
PR fusionados (30 d)
289

Preparar el entorno

Abrir en Codespaces

Inicia el contenedor de desarrollo del proyecto en tu navegador, con tu propia cuenta de GitHub.

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de dotnet/aspnetcore

Todos los issues de dotnet/aspnetcore

Issues similares

Más issues de C#

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.