[Sm-1588] Event logging for secret versioning restore event - #7533
[Sm-1588] Event logging for secret versioning restore event#7533cd-bitwarden wants to merge 12 commits into
Conversation
…at editorName gets returns to clients side properly
…date issues with version dates
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7533 +/- ##
==========================================
- Coverage 59.50% 59.50% -0.01%
==========================================
Files 2089 2089
Lines 92421 92442 +21
Branches 8214 8217 +3
==========================================
+ Hits 54994 55005 +11
- Misses 35484 35493 +9
- Partials 1943 1944 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR adds Code Review Details
Two existing tests already assert the behavior the new short-circuits remove — Several of these defects (the duplicate declaration and the reverted |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Store the current value before restoration | ||
| var currentValue = secret.Value; |
There was a problem hiding this comment.
❌ CRITICAL: currentValue is declared twice in the same method scope — the Api project does not compile.
Details and fix
var currentValue = secret.Value; is declared here at line 188, and again at line 235 further down in the same method body (no intervening scope). That is CS0128: A local variable named 'currentValue' is already defined in this scope.
This looks like a bad conflict resolution in the merge commit 027a6fa: on the pre-merge branch (027a6fa^1) there was a single declaration at line 207 that both paths shared, and line 255 simply reused it. The merge reintroduced the old declaration.
Fix: delete line 235 (var currentValue = secret.Value;) so the user path reuses the value captured here.
Corroboration: Build Docker images (Api, ./src, true, Core backend API) and Run tests are both currently failing on this PR.
| var currentValueRevisionDate = secret.RevisionDate; | ||
|
|
||
| // For service accounts and organization API, skip user-level access checks | ||
| if (_currentContext.IdentityClientType == IdentityClientType.ServiceAccount) |
There was a problem hiding this comment.
❌ CRITICAL: This early return lets any service account overwrite any secret in the organization, bypassing the per-secret write check.
Details and fix
The only authorization performed before this branch is _currentContext.AccessSecretsManager(secret.OrganizationId) (line 176). For a machine account that check is organization-wide:
// CurrentContext.AccessSecretsManager
if (ServiceAccountOrganizationId.HasValue && ServiceAccountOrganizationId.Value == orgId)
{
return true;
}It says nothing about whether this service account has an access policy on this secret. By returning before line 229's AccessToSecretAsync(secretId, userId.Value, accessClient) / !access.Write gate, a machine account with no policy on the secret — or a read-only policy — can restore an arbitrary version and overwrite secret.Value.
AccessClientHelper.ToAccessClient maps IdentityClientType.ServiceAccount to AccessClientType.ServiceAccount, a real access-checked client, so the pre-existing path was already correct for service accounts; there is nothing here that the check below would have wrongly rejected.
Fix: drop this branch and let service accounts fall through to the existing access.Write check. The snapshot logic below already handles IdentityClientType.ServiceAccount at line 241.
This also regresses RestoreVersion_ServiceAccount_NoWriteAccess_Throws in test/Api.Test/SecretsManager/Controllers/SecretVersionsControllerTests.cs:338, which asserts exactly this behavior.
| if (_currentContext.IdentityClientType == IdentityClientType.ServiceAccount || | ||
| _currentContext.IdentityClientType == IdentityClientType.Organization) | ||
| { | ||
| // Already verified Secrets Manager access above | ||
| var versionList = await _secretVersionRepository.GetManyBySecretIdAsync(secretId); | ||
| var responseList = versionList.Select(v => new SecretVersionResponseModel(v)).ToList(); | ||
| return new ListResponseModel<SecretVersionResponseModel>(responseList); | ||
| } |
There was a problem hiding this comment.
❌ CRITICAL: This early return exposes every historical secret value to any service account in the organization, bypassing the per-secret read check.
Details and fix
The comment says "Already verified Secrets Manager access above", but line 47 only checked _currentContext.AccessSecretsManager(secret.OrganizationId), which returns true for any secret in the machine account's own organization:
// CurrentContext.AccessSecretsManager
if (ServiceAccountOrganizationId.HasValue && ServiceAccountOrganizationId.Value == orgId)
{
return true;
}Returning here skips the AccessToSecretAsync(...) / !access.Read gate at lines 71-75. SecretVersionResponseModel carries Value, so a service account with no access policy on the secret receives its full version history.
Fix: remove this branch. AccessClientHelper.ToAccessClient already maps ServiceAccount and Organization to access-checked client types, so the existing path handles both correctly — the same pattern used by SecretsController.GetAsync and by GetByIdAsync/GetManyByIdsAsync in this very file.
This also regresses GetVersionsBySecretId_ServiceAccount_NoReadAccess_Throws in test/Api.Test/SecretsManager/Controllers/SecretVersionsControllerTests.cs:316.
| { | ||
| SecretId = secretId, | ||
| Value = currentValue!, | ||
| VersionDate = currentValueRevisionDate, |
There was a problem hiding this comment.
DateTime.UtcNow.
Details and fix
This branch correctly records VersionDate = currentValueRevisionDate — the revision date of the value being snapshotted — matching the fix made in SecretsController.UpdateSecretAsync (VersionDate = valueRevisionDate).
The user path at line 260 still uses VersionDate = DateTime.UtcNow, so the same endpoint records a different version date depending on the caller's identity type, and the user case still shows the restore time rather than the value's own revision date. That contradicts the objective "ensure the version date is correct".
The pre-merge branch (027a6fa^1, line 267) had VersionDate = currentValueRevisionDate here, so this looks like it was reverted by the same merge that introduced the duplicate currentValue declaration.
Fix: change line 260 to VersionDate = currentValueRevisionDate, — which also resolves once the duplicate declaration at line 235 is removed.
2 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/SM-1588?assignee=625cb516fd06270069beaf5d&search_id=dd3af888-5ba7-4252-9b3a-7e557eca28fc
📔 Objective
Track when a restore event occurs, it should create a secret edited event log. Also ensure the version date is correct.