bitwarden
[PM-11249] Update cipher revision date when an attachment is added or deleted
🎟️ Tracking
📔 Objective
Current Issue: When an attachment is added or deleted, web/browser clients are not being updated of the change.
Cause: The logic within the CoreSyncService on the client side uses the cipher revision date to determine if a sync is needed. Server side the cipher revision date isn't updated.
Proposed Fix: Update the revision date on the cipher if an attachment is added or deleted.
📸 Screenshots
| Before | After |
|---|---|
🦮 Reviewer guidelines
- 👍 (
:+1:) or similar for great changes - 📝 (
:memo:) or ℹ️ (:information_source:) for notes or general info - ❓ (
:question:) for questions - 🤔 (
:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (
:art:) for suggestions / improvements - ❌ (
:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention - 🌱 (
:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt - ⛏ (
:pick:) for minor or nitpick changes
❓ Are you still able to modify the cipher after uploading an attachment? I'm worried that we don't return the updated cipher in the attachment endpoint so the cipher.revisionDate will be stale client side, which will prevent users from making changes until they manually re-sync or refresh their vault (on the client that uploaded the attachment).
Codecov Report
Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.
Project coverage is 44.31%. Comparing base (
a9a1230) to head (72fe323). Report is 1 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #4873 +/- ##
==========================================
- Coverage 44.32% 44.31% -0.01%
==========================================
Files 1482 1483 +1
Lines 68376 68388 +12
Branches 6172 6172
==========================================
Hits 30307 30307
- Misses 36761 36773 +12
Partials 1308 1308
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
❓ Are you still able to modify the cipher after uploading an attachment? I'm worried that we don't return the updated cipher in the attachment endpoint so the
cipher.revisionDatewill be stale client side, which will prevent users from making changes until they manually re-sync or refresh their vault (on the client that uploaded the attachment).
Nice catch, I only tested on the client that received the update which in turn would have the updated cipher.
On the same client you are not able to. 🤔
Checkmarx One – Scan Summary & Details – fcd6880d-cbd0-45e0-a445-4bb8ac2be417
New Issues (12)
Checkmarx found the following issues in this Pull Request
| Severity | Issue | Source File / Package | Checkmarx Insight |
|---|---|---|---|
 |
CSRF | /src/Billing/Controllers/PayPalController.cs: 66 | detailsMethod PostIpn at line 66 of /src/Billing/Controllers/PayPalController.cs gets a parameter from a user request from Body. This parameter value flow... |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 1100 | detailsMethod DeleteAttachment at line 1100 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This paramete... |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 1100 | detailsMethod DeleteAttachment at line 1100 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This paramete... |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 1100 | detailsMethod DeleteAttachment at line 1100 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from DeleteAttachment.... |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 1023 | detailsMethod PostAttachment at line 1023 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter ... |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 997 | detailsMethod PostFileForExistingAttachment at line 997 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. T... |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 1046 | detailsMethod PostAttachmentAdmin at line 1046 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This param... |
|
Privacy_Violation | /src/Core/AdminConsole/OrganizationAuth/UpdateOrganizationAuthRequestCommand.cs: 84 | detailsMethod UpdateAsync at line 84 of /src/Core/AdminConsole/OrganizationAuth/UpdateOrganizationAuthRequestCommand.cs sends user information outside the... |
|
Privacy_Violation | /src/Core/NotificationHub/NotificationHubPushNotificationService.cs: 195 | detailsMethod PushAuthRequestAsync at line 195 of /src/Core/NotificationHub/NotificationHubPushNotificationService.cs sends user information outside the a... |
 |
Log_Forging | /src/Api/Auth/Controllers/AuthRequestsController.cs: 87 | detailsMethod PostAdminRequest at line 87 of /src/Api/Auth/Controllers/AuthRequestsController.cs gets user input from element model. This element’s value ... |
|
Log_Forging | /src/Api/Auth/Controllers/AuthRequestsController.cs: 75 | detailsMethod Post at line 75 of /src/Api/Auth/Controllers/AuthRequestsController.cs gets user input from element model. This element’s value flows throug... |
|
Missing_CSP_Header | /src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs: 7 | detailsA Content Security Policy is not explicitly defined within the web-application. |
Fixed Issues (28)
Great job! The following issues were fixed in this Pull Request
| Severity | Issue | Source File / Package |
|---|---|---|
|
CSRF | /src/Api/Tools/Controllers/OrganizationExportController.cs: 53 |
|
CSRF | /src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 375 |
|
CSRF | /src/Api/AdminConsole/Controllers/GroupsController.cs: 80 |
|
CSRF | /src/Api/AdminConsole/Controllers/GroupsController.cs: 121 |
|
CSRF | /src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 46 |
|
CSRF | /src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 65 |
|
CSRF | /src/Api/Auth/Controllers/AccountsController.cs: 470 |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 220 |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 220 |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 1100 |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 173 |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 533 |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 533 |
|
CSRF | /src/Api/Vault/Controllers/CiphersController.cs: 1100 |
|
Privacy_Violation | /src/Api/Auth/Models/Request/Accounts/SetPasswordRequestModel.cs: 28 |
|
Log_Forging | /src/Api/Billing/Controllers/OrganizationBillingController.cs: 238 |
|
Log_Forging | /src/Api/Billing/Controllers/OrganizationBillingController.cs: 238 |
|
Log_Forging | /src/Api/Billing/Controllers/OrganizationBillingController.cs: 238 |
|
Log_Forging | /src/Api/Billing/Controllers/ProviderBillingController.cs: 104 |
|
Log_Forging | /src/Api/Billing/Controllers/ProviderBillingController.cs: 104 |
|
Log_Forging | /src/Api/Billing/Controllers/ProviderBillingController.cs: 104 |
|
Log_Forging | /src/Billing/Controllers/RecoveryController.cs: 38 |
|
Log_Forging | /src/Billing/Controllers/RecoveryController.cs: 38 |
|
Log_Forging | /src/Api/Vault/Controllers/CiphersController.cs: 173 |
|
Log_Forging | /src/Billing/Controllers/StripeController.cs: 164 |
|
Log_Forging | /src/Billing/Controllers/StripeController.cs: 164 |
|
Log_Forging | /src/Api/Auth/Controllers/AccountsController.cs: 261 |
|
Log_Forging | /src/Api/Auth/Controllers/AccountsController.cs: 381 |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Marking this a draft as I have yet to track down all of the necessary changes on server and client side. Priorities have changed so I'll be taking up some other work in the mean time.
See the ticket for more details about solutioning these scenarios.
@shane-melton I order to support this change for deleting an attachment, the UI needed to know the updated revision timestamp. I added logic to return the updated cipher to both the client and here 13acab5
Moving to a draft as this work is being tabled for a couple weeks why we wait for the extension refresh to roll out.