refactor: remove nested if statements #355

oskardanielsson-yubico · 2025-12-01T11:17:09Z

Description

Refactors many if statements to make them more readable

Fixes: # https://yubico.atlassian.net/jira/software/c/projects/YESDK/boards/1466?issueType=10004&selectedIssue=YESDK-1494>

Type of change

Refactor (non-breaking change which improves code quality or performance)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

github-actions · 2025-12-02T11:29:32Z

Test Results: Windows

2 files 2 suites 16s ⏱️
4 038 tests 4 019 ✅ 19 💤 0 ❌
4 040 runs 4 021 ✅ 19 💤 0 ❌

Results for commit 7acc27d.

♻️ This comment has been updated with latest results.

github-actions · 2025-12-02T11:29:41Z

Test Results: Ubuntu

2 files 2 suites 46s ⏱️
4 030 tests 4 011 ✅ 19 💤 0 ❌
4 032 runs 4 013 ✅ 19 💤 0 ❌

Results for commit 7acc27d.

♻️ This comment has been updated with latest results.

github-actions · 2025-12-02T11:29:49Z

Test Results: MacOS

4 files 4 suites 31s ⏱️
4 012 tests 4 012 ✅ 0 💤 0 ❌
4 014 runs 4 014 ✅ 0 💤 0 ❌

Results for commit 7acc27d.

♻️ This comment has been updated with latest results.

Copilot

Pull request overview

This PR refactors nested if statements throughout the codebase to improve readability by combining conditions with logical operators. The changes flatten control flow by using && and || operators to merge multiple condition checks, and in several cases invert logic to handle early returns more efficiently.

Key changes:

Nested if statements combined into single conditions using logical AND (&&) operators
Some conditional logic inverted to enable early returns/throws, reducing nesting levels
Dispose pattern implementations simplified by combining disposal checks

Reviewed changes

Copilot reviewed 43 out of 43 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
StaticConverters.cs	Combined nested enum parsing validation checks
HollowConnection.cs	Merged authentication command condition checks
Fido2ResetForTest.cs	Combined reset status and PIN setting checks
SimpleKeyCollector.cs	Merged null check and retry count validation
Fido2InfoTests.cs	Refactored multiple test methods to use inverted logic with early breaks
CredMgmtDataTests.cs	Combined credential validation checks and added LINQ for collection comparison
YubiOtp.cs	Inverted status check logic and reorganized error handling
U2fCommandTests.cs	Combined platform and elevation checks across multiple test classes
SimpleU2fTests.cs	Merged Windows elevation checks in test methods
SetDeviceInfoTests.cs	Combined platform and elevation checks in constructor
SessionRegisterTests.cs	Merged Windows elevation checks in constructor
SessionPinTests.cs	Combined platform and elevation checks in constructor
PinTests.cs	Merged Windows elevation checks in constructor
CommandTests.cs	Combined platform and elevation checks in constructor
X500NameBuilder.cs	Merged null check and dictionary lookup
BioEnrollTests.cs	Combined bio enrollment sample validation checks
RegistrationData.cs	Inverted length validation to enable early throws
TouchFingerprintTask.cs	Combined connection type and cancel loading checks
StaticKeys.cs	Simplified dispose pattern by combining disposal checks
SessionKeys.cs	Simplified dispose pattern by combining disposal checks
Session.cs	Simplified dispose pattern by combining disposal checks
Scp03Connection.cs	Simplified dispose pattern by combining disposal checks
ScpConnection.cs	Simplified dispose pattern by combining disposal checks
PivSession.Pinonly.cs	Inverted multiple condition checks to enable early returns
PivSession.Pin.cs	Inverted nested checks to enable early returns in retry count changes
PivDataObject.cs	Combined data tag validation conditions
CardholderUniqueId.cs	Inverted validity checks in multiple helper methods to enable early returns
CardCapabilityContainer.cs	Inverted validity check to enable early return
PutDataCommand.cs	Combined vendor tag validation and inverted encoding validation
GetDataCommand.cs	Combined vendor tag and special tag validation
CompleteAuthenticateManagementKeyResponse.cs	Combined TLV reading checks
Scp03ApduTransform.cs	Simplified dispose pattern by combining disposal checks
OathSession.Password.cs	Combined password protection and verification checks
SetDeviceInfoBaseCommand.cs	Combined nullable value and range validation
EnumerateRpsGetNextResponse.cs	Combined null checks and validation
EnumerateRpsBeginResponse.cs	Reorganized null checks and validation
U2fSampleRun.cs	Combined YubiKey selection and menu item execution checks
PivSampleRun.cs	Combined YubiKey selection and menu item execution checks
SignatureAlgIdConverter.cs	Combined PSS parameter validation conditions
PemOperations.cs	Combined length and header verification checks
DsaSignatureConverter.cs	Combined TLV reading operations
Fido2SampleRun.cs	Combined YubiKey selection and menu item execution checks
Fido2SampleRun.Operations.cs	Combined authenticator info and extension checks

Comments suppressed due to low confidence (3)

Yubico.YubiKey/src/Yubico/YubiKey/Scp03/StaticKeys.cs:1

Missing closing brace for the class definition. The file ends at line 241 but the closing brace for the if statement and class are missing.

// Copyright 2025 Yubico AB

Yubico.YubiKey/src/Yubico/YubiKey/Scp03/SessionKeys.cs:65

Missing closing brace for the class definition. The file ends at line 69 but the closing brace for the if statement and class are missing.

            if (!_disposed && disposing)
            {
                CryptographicOperations.ZeroMemory(_sessionMacKey.AsSpan());
                CryptographicOperations.ZeroMemory(_sessionEncryptionKey.AsSpan());
                CryptographicOperations.ZeroMemory(_sessionRmacKey.AsSpan());
            
                _disposed = true;

Yubico.YubiKey/src/Yubico/YubiKey/Pipelines/Scp03ApduTransform.cs:154

Missing closing brace for the class definition. The file ends at line 158 but the closing brace for the if statement and class are missing.

            if (!_disposed && disposing)
            {
                Scp03Keys.Dispose();
                _session.Dispose();
            
                _disposed = true;

Yubico.YubiKey/src/Yubico/YubiKey/Fido2/Commands/EnumerateRpsBeginResponse.cs

Yubico.YubiKey/src/Yubico/YubiKey/Piv/Objects/CardCapabilityContainer.cs

oskardanielsson-yubico · 2025-12-04T10:09:12Z

Yubico.YubiKey/tests/utilities/Yubico/YubiKey/TestUtilities/HollowConnection.cs

+
+                using TripleDES tripleDes = CryptographyProviders.TripleDesCreator();
+
+                tripleDes.Mode = CipherMode.ECB;


To fix the flagged issue, replace the use of CipherMode.ECB with CipherMode.CBC in line 97. CBC (Cipher Block Chaining) mode is more secure for general encryption, as it adds randomness through an Initialization Vector (IV). You must supply a valid IV to the encryptor. This requires generating an IV of the correct size for TripleDES (which is 8 bytes). For deterministic testing, you may use a constant IV; for more realistic yet varying tests, generate a random IV.

In this code, the encryptor is created (line 99) with keyBytes and null as the IV (the second argument). For CBC, the IV argument must be a non-null, 8-byte array.
Therefore, in file Yubico.YubiKey/tests/utilities/Yubico/YubiKey/TestUtilities/HollowConnection.cs, update:

Line 97: Change CipherMode.ECB to CipherMode.CBC.

Line 99: Pass an IV of length 8 (e.g., a static IV: byte[] iv = new byte[8]; // All zero IV for determinism in tests), so replace null with iv.

No additional imports are needed, as usage is within the scope of System.Security.Cryptography.

I don't really understand why this comes up now. Nothing was changed inside the if statement

No worries. It comes up every once in a while with Copilot / CodeQL analysis.
ECB is a part of the TripleDES implementation and critical to certain YubiKey operations, such as within the PIV standard (more)

github-actions · 2025-12-15T12:16:06Z

Package	Line Rate	Branch Rate	Complexity	Health
Yubico.Core	45%	35%	4557	➖
Yubico.YubiKey	51%	47%	21216	➖
Summary	50% (37197 / 74856)	45% (9077 / 20337)	25773	➖

Minimum allowed line rate is 40%

oskardanielsson-yubico force-pushed the oskar/remove-nested-if-statements branch from aa5f56e to 6360c7b Compare December 2, 2025 11:33

oskardanielsson-yubico marked this pull request as ready for review December 2, 2025 11:40

oskardanielsson-yubico changed the title ~~Oskar/remove nested if statements~~ refactor: remove nested if statements Dec 3, 2025

oskardanielsson-yubico force-pushed the oskar/remove-nested-if-statements branch from 6360c7b to e7ae545 Compare December 3, 2025 12:18

DennisDyallo requested a review from Copilot December 3, 2025 15:51

Copilot AI reviewed Dec 3, 2025

View reviewed changes

Yubico.YubiKey/src/Yubico/YubiKey/Fido2/Commands/EnumerateRpsBeginResponse.cs Show resolved Hide resolved

Yubico.YubiKey/src/Yubico/YubiKey/Piv/Objects/CardCapabilityContainer.cs Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Dec 4, 2025

View reviewed changes

oskardanielsson-yubico added 4 commits December 15, 2025 13:13

Refactor remove nested if-statements

0974c71

Make functions more readable

eed6c91

Fix null reference for compilation

cd8502f

Remove whitespaces

7acc27d

DennisDyallo force-pushed the oskar/remove-nested-if-statements branch from 8655ead to 7acc27d Compare December 15, 2025 12:13

@@ -93,10 +93,11 @@
                             Array.Copy(data, 14, responseData, 4, 8);
                             using TripleDES tripleDes = CryptographyProviders.TripleDesCreator();
-                            tripleDes.Mode = CipherMode.ECB;
+                            tripleDes.Mode = CipherMode.CBC; // Use CBC mode for better security
                             tripleDes.Padding = PaddingMode.None;
-                            using ICryptoTransform encryptor = tripleDes.CreateEncryptor(keyBytes, null);
+                            byte[] iv = new byte[8]; // Use zero IV for determinism in test code
+                            using ICryptoTransform encryptor = tripleDes.CreateEncryptor(keyBytes, iv);
                             _ = encryptor.TransformBlock(data, 14, 8, responseData, 4);
                             var responseApdu = new ResponseApdu(responseData);

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: remove nested if statements #355

refactor: remove nested if statements #355

Uh oh!

oskardanielsson-yubico commented Dec 1, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 2, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 2, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 2, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Check failure

Copilot Autofix

oskardanielsson-yubico Dec 4, 2025

Uh oh!

DennisDyallo Dec 4, 2025

Uh oh!

github-actions bot commented Dec 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants


		using TripleDES tripleDes = CryptographyProviders.TripleDesCreator();

		tripleDes.Mode = CipherMode.ECB;

refactor: remove nested if statements #355

Are you sure you want to change the base?

refactor: remove nested if statements #355

Uh oh!

Conversation

oskardanielsson-yubico commented Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Uh oh!

github-actions bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results: Windows

Uh oh!

github-actions bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results: Ubuntu

Uh oh!

github-actions bot commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results: MacOS

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Check failure

Copilot Autofix

oskardanielsson-yubico Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

DennisDyallo Dec 4, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

oskardanielsson-yubico commented Dec 1, 2025 •

edited

Loading

github-actions bot commented Dec 2, 2025 •

edited

Loading

github-actions bot commented Dec 2, 2025 •

edited

Loading

github-actions bot commented Dec 2, 2025 •

edited

Loading