Merge pull request #276 from Yubico/dennisdyallo/fixes-182

fix(otp): Handle case when last slot is deleted

Author: DennisDyallo

Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/OtpErrorTransform.cs

@@ -16,6 +16,7 @@
 using Microsoft.Extensions.Logging;
 using Yubico.Core.Iso7816;
 using Yubico.Core.Logging;
+using Yubico.YubiKey.Otp;
 using Yubico.YubiKey.Otp.Commands;
 
 namespace Yubico.YubiKey.Pipelines
@@ -37,45 +38,70 @@ public ResponseApdu Invoke(CommandApdu command, Type commandType, Type responseT
         {
             // If this is just a regular ReadStatusCommand, or it's a command that doesn't ask for a status response
             // in return, invoke the pipeline as usual.
-            if (commandType == typeof(ReadStatusCommand)
-                || responseType != typeof(ReadStatusResponse))
+            if (commandType == typeof(ReadStatusCommand) ||
+                responseType != typeof(ReadStatusResponse))
             {
                 return _nextTransform.Invoke(command, commandType, responseType);
             }
 
             // Otherwise we assume this to be a command that applies a config (and therefore looks for a status response).
             // In order to detect failures, we grab the status structure before applying said command so that we have a
             // sequence number to compare to.
-            int beforeSequence = new ReadStatusResponse(
-                _nextTransform.Invoke(
-                    new ReadStatusCommand().CreateCommandApdu(),
-                    typeof(ReadStatusCommand),
-                    typeof(ReadStatusResponse)))
-                .GetData()
-                .SequenceNumber;
+            var beforeStatus = GetCurrentStatus();
+            var responseApdu = _nextTransform.Invoke(command, commandType, responseType);
+            var afterStatus = ReadStatus(responseApdu);
 
             try
             {
-                var responseApdu = _nextTransform.Invoke(command, commandType, responseType);
-                int afterSequence = new ReadStatusResponse(responseApdu).GetData().SequenceNumber;
-                int expectedSequence = (beforeSequence + 1) % 0x100;
-
-                // If we see the sequence number change, we can assume that the configuration was applied successfully. Otherwise
-                // we just invent an error in the response.
-                return afterSequence != expectedSequence
-                    ? new ResponseApdu(responseApdu.Data.ToArray(), SWConstants.WarningNvmUnchanged)
-                    : responseApdu;
+                // If we see the sequence number change, we can assume that the configuration was applied successfully.
+                // Otherwise we just invent an error in the response.
+                return IsValidSequenceProgression(beforeStatus, afterStatus)
+                    ? responseApdu
+                    : CreateFailedApdu(responseApdu.Data.ToArray());
             }
             catch (KeyboardConnectionException e)
             {
                 _logger.LogWarning(e, "Handling keyboard connection exception. Translating to APDU response.");
 
-                return new ResponseApdu(Array.Empty<byte>(), SWConstants.WarningNvmUnchanged);
+                return CreateFailedApdu();
             }
         }
 
         public void Setup() => _nextTransform.Setup();
 
         public void Cleanup() => _nextTransform.Cleanup();
+
+        // Internal for testing
+        internal static bool IsValidSequenceProgression(OtpStatus beforeStatus, OtpStatus afterStatus)
+        {
+            byte before = beforeStatus.SequenceNumber;
+            byte after = afterStatus.SequenceNumber;
+
+            bool normalIncrement = after == before + 1;
+            bool validReset = before > 0 && after == 0 &&
+                afterStatus is { LongPressConfigured: false, ShortPressConfigured: false };
+
+            return normalIncrement || validReset;
+        }
+
+        private OtpStatus GetCurrentStatus()
+        {
+            var command = new ReadStatusCommand();
+            var responseApdu = _nextTransform.Invoke(
+                command.CreateCommandApdu(),
+                typeof(ReadStatusCommand),
+                typeof(ReadStatusResponse));
+
+            return ReadStatus(responseApdu);
+        }
+
+        private static OtpStatus ReadStatus(ResponseApdu responseApdu)
+        {
+            var readStatusResponse = new ReadStatusResponse(responseApdu);
+            var afterStatus = readStatusResponse.GetData();
+            return afterStatus;
+        }
+
+        private static ResponseApdu CreateFailedApdu(byte[]? data = null) => new(data ?? [], SWConstants.WarningNvmUnchanged);
     }
 }

Yubico.YubiKey/tests/unit/Yubico/YubiKey/Pipelines/OtpErrorTransformTests.cs

@@ -0,0 +1,85 @@
+using Xunit;
+using Yubico.YubiKey.Otp;
+
+namespace Yubico.YubiKey.Pipelines;
+
+public class OtpErrorTransformTests
+{
+    [Theory]
+    [InlineData(0, 1)] // First increment
+    [InlineData(5, 6)] // Normal increment  
+    [InlineData(254, 255)] // Near byte boundary
+    public void IsValidSequenceProgression_NormalIncrement_ReturnsTrue(
+        byte before,
+        byte after)
+    {
+        var beforeStatus = CreateOtpStatus(before, hasConfigs: true);
+        var afterStatus = CreateOtpStatus(after, hasConfigs: true);
+
+        var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus);
+
+        Assert.True(isValid);
+    }
+
+    [Theory]
+    [InlineData(0, 2)] // Skip increment
+    [InlineData(5, 7)] // Jump by 2
+    [InlineData(10, 8)] // Backwards
+    [InlineData(255, 0)] // Byte overflow (should be rejected as normal increment)
+    public void IsValidSequenceProgression_InvalidIncrement_ReturnsFalse(
+        byte before,
+        byte after)
+    {
+        var beforeStatus = CreateOtpStatus(before, hasConfigs: true);
+        var afterStatus = CreateOtpStatus(after, hasConfigs: true); // Still has configs
+
+        var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus);
+
+        Assert.False(isValid);
+    }
+
+    [Theory]
+    [InlineData(1)] // Delete from sequence 1
+    [InlineData(5)] // Delete from sequence 5  
+    [InlineData(255)] // Delete from sequence 255
+    public void IsValidSequenceProgression_ValidReset_ReturnsTrue(
+        byte before)
+    {
+        var beforeStatus = CreateOtpStatus(before, hasConfigs: true);
+        var afterStatus = CreateOtpStatus(0, hasConfigs: false); // No configs remain
+
+        var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus);
+
+        Assert.True(isValid);
+    }
+
+    [Theory]
+    [InlineData(0, false)] // Already zero
+    [InlineData(1, true)] // Reset but configs still exist
+    [InlineData(5, true)] // Reset but configs still exist
+    public void IsValidSequenceProgression_InvalidReset_ReturnsFalse(
+        byte before,
+        bool afterHasConfigs)
+    {
+        var beforeStatus = CreateOtpStatus(before, hasConfigs: true);
+        var afterStatus = CreateOtpStatus(0, hasConfigs: afterHasConfigs);
+
+        var isValid = OtpErrorTransform.IsValidSequenceProgression(beforeStatus, afterStatus);
+
+        Assert.False(isValid);
+    }
+
+    private static OtpStatus CreateOtpStatus(
+        byte sequenceNumber,
+        bool hasConfigs) => new()
+    {
+        FirmwareVersion = new FirmwareVersion(),
+        SequenceNumber = sequenceNumber,
+        TouchLevel = 0x00,
+        ShortPressConfigured = hasConfigs,
+        LongPressConfigured = false, // Only test one slot for simplicity
+        ShortPressRequiresTouch = false,
+        LongPressRequiresTouch = false,
+        LedBehaviorInverted = false
+    };
+}