[PM-19357] - [Defect] Unauthorised access allows limited access user to change custom hidden field of Items (#5572)

* prevent hidden password users from modifying hidden fields

* add tests

* fix serialization issues

* DRY up code

* return newly created cipher

* add sshKey data type

* fix tests

src/Api/Vault/Controllers/CiphersController.cs

@@ -177,12 +177,7 @@ public class CiphersController : Controller
         }
 
         await _cipherService.SaveDetailsAsync(cipher, user.Id, model.Cipher.LastKnownRevisionDate, model.CollectionIds, cipher.OrganizationId.HasValue);
-        var response = new CipherResponseModel(
+        return await Get(cipher.Id);
-            cipher,
-            user,
-            await _applicationCacheService.GetOrganizationAbilitiesAsync(),
-            _globalSettings);
-        return response;
     }
 
     [HttpPost("admin")]

src/Core/Vault/Services/Implementations/CipherService.cs

@@ -1003,7 +1003,7 @@ public class CipherService : ICipherService
 
     private async Task ValidateViewPasswordUserAsync(Cipher cipher)
     {
-        if (cipher.Type != CipherType.Login || cipher.Data == null || !cipher.OrganizationId.HasValue)
+        if (cipher.Data == null || !cipher.OrganizationId.HasValue)
         {
             return;
         }
@@ -1014,20 +1014,57 @@ public class CipherService : ICipherService
         // Check if user is a "hidden password" user
         if (!cipherPermissions.TryGetValue(cipher.Id, out var permission) || !(permission.ViewPassword && permission.Edit))
         {
+            var existingCipherData = DeserializeCipherData(existingCipher);
+            var newCipherData = DeserializeCipherData(cipher);
+
             // "hidden password" users may not add cipher key encryption
             if (existingCipher.Key == null && cipher.Key != null)
             {
                 throw new BadRequestException("You do not have permission to add cipher key encryption.");
             }
+            // Keep only non-hidden fileds from the new cipher
+            var nonHiddenFields = newCipherData.Fields?.Where(f => f.Type != FieldType.Hidden) ?? [];
+            // Get hidden fields from the existing cipher
+            var hiddenFields = existingCipherData.Fields?.Where(f => f.Type == FieldType.Hidden) ?? [];
+            // Replace the hidden fields in new cipher data with the existing ones
+            newCipherData.Fields = nonHiddenFields.Concat(hiddenFields);
+            cipher.Data = SerializeCipherData(newCipherData);
+            if (existingCipherData is CipherLoginData existingLoginData && newCipherData is CipherLoginData newLoginCipherData)
+            {
                 // "hidden password" users may not change passwords, TOTP codes, or passkeys, so we need to set them back to the original values
-            var existingCipherData = JsonSerializer.Deserialize<CipherLoginData>(existingCipher.Data);
+                newLoginCipherData.Fido2Credentials = existingLoginData.Fido2Credentials;
-            var newCipherData = JsonSerializer.Deserialize<CipherLoginData>(cipher.Data);
+                newLoginCipherData.Totp = existingLoginData.Totp;
-            newCipherData.Fido2Credentials = existingCipherData.Fido2Credentials;
+                newLoginCipherData.Password = existingLoginData.Password;
-            newCipherData.Totp = existingCipherData.Totp;
+                cipher.Data = SerializeCipherData(newLoginCipherData);
-            newCipherData.Password = existingCipherData.Password;
-            cipher.Data = JsonSerializer.Serialize(newCipherData);
             }
         }
+    }
+
+    private string SerializeCipherData(CipherData data)
+    {
+        return data switch
+        {
+            CipherLoginData loginData => JsonSerializer.Serialize(loginData),
+            CipherIdentityData identityData => JsonSerializer.Serialize(identityData),
+            CipherCardData cardData => JsonSerializer.Serialize(cardData),
+            CipherSecureNoteData noteData => JsonSerializer.Serialize(noteData),
+            CipherSSHKeyData sshKeyData => JsonSerializer.Serialize(sshKeyData),
+            _ => throw new ArgumentException("Unsupported cipher data type.", nameof(data))
+        };
+    }
+
+    private CipherData DeserializeCipherData(Cipher cipher)
+    {
+        return cipher.Type switch
+        {
+            CipherType.Login => JsonSerializer.Deserialize<CipherLoginData>(cipher.Data),
+            CipherType.Identity => JsonSerializer.Deserialize<CipherIdentityData>(cipher.Data),
+            CipherType.Card => JsonSerializer.Deserialize<CipherCardData>(cipher.Data),
+            CipherType.SecureNote => JsonSerializer.Deserialize<CipherSecureNoteData>(cipher.Data),
+            CipherType.SSHKey => JsonSerializer.Deserialize<CipherSSHKeyData>(cipher.Data),
+            _ => throw new ArgumentException("Unsupported cipher type.", nameof(cipher))
+        };
+    }
 
     // This method is used to filter ciphers based on the user's permissions to delete them.
     // It supports both the old and new logic depending on the feature flag.

test/Core.Test/Vault/Services/CipherServiceTests.cs

@@ -1228,7 +1228,8 @@ public class CipherServiceTests
         bool editPermission,
         string? key = null,
         string? totp = null,
-        CipherLoginFido2CredentialData[]? passkeys = null
+        CipherLoginFido2CredentialData[]? passkeys = null,
+        CipherFieldData[]? fields = null
         )
     {
         var cipherDetails = new CipherDetails
@@ -1241,12 +1242,13 @@ public class CipherServiceTests
             Key = key,
         };
 
-        var newLoginData = new CipherLoginData { Username = "user", Password = newPassword, Totp = totp, Fido2Credentials = passkeys };
+        var newLoginData = new CipherLoginData { Username = "user", Password = newPassword, Totp = totp, Fido2Credentials = passkeys, Fields = fields };
         cipherDetails.Data = JsonSerializer.Serialize(newLoginData);
 
         var existingCipher = new Cipher
         {
             Id = cipherDetails.Id,
+            Type = CipherType.Login,
             Data = JsonSerializer.Serialize(
                 new CipherLoginData
                 {
@@ -1442,6 +1444,56 @@ public class CipherServiceTests
         Assert.Equal(passkeys.Length, updatedLoginData.Fido2Credentials.Length);
     }
 
+    [Theory]
+    [BitAutoData]
+    public async Task SaveDetailsAsync_HiddenFieldsChangedWithoutPermission(string _, SutProvider<CipherService> sutProvider)
+    {
+        var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: false, editPermission: false, fields:
+        [
+            new CipherFieldData
+            {
+                Name = "FieldName",
+                Value = "FieldValue",
+                Type = FieldType.Hidden,
+            }
+        ]);
+
+        await deps.SutProvider.Sut.SaveDetailsAsync(
+            deps.CipherDetails,
+            deps.CipherDetails.UserId.Value,
+            deps.CipherDetails.RevisionDate,
+            null,
+            true);
+
+        var updatedLoginData = JsonSerializer.Deserialize<CipherLoginData>(deps.CipherDetails.Data);
+        Assert.Empty(updatedLoginData.Fields);
+    }
+
+    [Theory]
+    [BitAutoData]
+    public async Task SaveDetailsAsync_HiddenFieldsChangedWithPermission(string _, SutProvider<CipherService> sutProvider)
+    {
+        var deps = GetSaveDetailsAsyncDependencies(sutProvider, "NewPassword", viewPassword: true, editPermission: true, fields:
+        [
+            new CipherFieldData
+            {
+                Name = "FieldName",
+                Value = "FieldValue",
+                Type = FieldType.Hidden,
+            }
+        ]);
+
+        await deps.SutProvider.Sut.SaveDetailsAsync(
+            deps.CipherDetails,
+            deps.CipherDetails.UserId.Value,
+            deps.CipherDetails.RevisionDate,
+            null,
+            true);
+
+        var updatedLoginData = JsonSerializer.Deserialize<CipherLoginData>(deps.CipherDetails.Data);
+        Assert.Single(updatedLoginData.Fields.ToArray());
+    }
+
     [Theory]
     [BitAutoData]
     public async Task DeleteAsync_WithPersonalCipherOwner_DeletesCipher(