Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unable to update cipher after editing attachments using Desktop client #2591

Closed
Nyxtorm opened this issue Jul 1, 2022 · 7 comments · Fixed by #3076
Closed

Unable to update cipher after editing attachments using Desktop client #2591

Nyxtorm opened this issue Jul 1, 2022 · 7 comments · Fixed by #3076
Assignees
@BlackDex
Labels
bug Something isn't working low priority Won't fix anytime soon, but will accept PR if provided troubleshooting There might be bug or it could be user error, more info needed

Comments

@Nyxtorm
Copy link

Nyxtorm commented Jul 1, 2022

Subject of the issue

  • Unexpected behavior when deleting an attachment
  • Problem of editing and deleting an attachment at the same time

Deployment environment

  • Vaultwarden version: v1.25.0-af50eae6
  • Web-vault version: v2022.6.0 (compiled w/ PR72)
  • Running within Docker: false (Base: Debian)
  • Environment settings overridden: false
  • Uses a reverse proxy: true
  • IP Header check: true (X-Real-IP)
  • Internet access: true
  • Internet access via a proxy: false
  • DNS Check: true
  • Time Check: true
  • Domain Configuration Check: true
  • HTTPS Check: true
  • Database type: SQLite
  • Database version: 3.35.4
  • Clients used: Web, Android, Windows, Chrome, Firefox
  • Reverse proxy and version: Nginx 1.21.6
  • Other relevant information:

Steps to reproduce

  1. Create an item with an attachment
  2. Edit the item
  3. Delete the attachment, confirm deletion
  4. Before save & close, change any other field
  5. Click on save to finish editing the item, error occurs : https://i.imgur.com/63SzWOq.png

Expected behaviour / Actual behaviour

If the deletion of the attachment has already been taken into account on the server side, there should be no error here.
It is currently impossible to edit a field and delete an attachment at the same time.

Troubleshooting data

  • Image 01 : https://i.imgur.com/63SzWOq.png
  • Sorry, I couldn't test on the official version because I don't have my premium subscription anymore, so I can't add attachments.
@BlackDex
Copy link
Collaborator

BlackDex commented Jul 4, 2022 •
edited
Loading

I'm a bit puzzled here since within the web-vault you can't edit both the attachments and other parts of that same item simultaneously. They both have different interfaces.

If you have two browsers open, or use two different clients, then this is expected behavior since you would overwrite a newer version from an older version in the cache of the other client.

If you could explain a bit better which exact steps you take with which clients then maybe we can figure out what's going on.

@Nyxtorm
Copy link
Author

Nyxtorm commented Jul 5, 2022

It is indeed only the desktop client (Windows for my part) that allows to follow these steps, I forgot to specify it.

The web client and browser addons have a completely separate interface for managing attachments.

So I can guess that this is only a problem with the Windows client itself which should also have a "separate" attachments management?

@BlackDex BlackDex changed the title Unexpected behavior when deleting an attachment Unable to update cipher after editing attachments using Desktop client Jul 13, 2022
@BlackDex BlackDex added the bug Something isn't working label Jul 13, 2022
@BlackDex
Copy link
Collaborator

I just tested this, and it indeed has an issue when using the deskop client.
If you add an attachment, then it will report that the cipher is outdated. If you remove an attachment it reports one of the attachments do not exists anymore.

This doesn't seem to be an issue the official Bitwarden server.

@BlackDex
Copy link
Collaborator

BlackDex commented Jul 13, 2022 •
edited
Loading

I did a bit more digging, and it may also be an issue for the official bitwarden server, but it looks like both there self-hosted and the SAAS version do not implement the websockets correctly.

This in turn causes some code parts to not being triggered, and could in turn make it look like nothing is wrong.
I tested this by turning of websockets, and that seems to solve the issue when adding an attachment and updating the cipher afterwards.

Deleting an attachment still is an issue, that will cause an unknown attachment error. Which may be something we could catch in these cases maybe.

@BlackDex BlackDex added the troubleshooting There might be bug or it could be user error, more info needed label Jul 20, 2022
@BlackDex BlackDex added the low priority Won't fix anytime soon, but will accept PR if provided label Dec 4, 2022
@BlackDex BlackDex self-assigned this Dec 30, 2022
@BlackDex BlackDex mentioned this issue Dec 30, 2022
Merged
@BlackDex
Copy link
Collaborator

@Nyxtorm i had some time today to take a deeper dive into this issue.
And it looks like i fixed it. I can now add and delete attachments via the Desktop client without any errors.
I have to test this a bit more, and i also want to make some more changes in regards to the websocket notifications.

But, I'm working on it.
Basically, what happened was that due to how the websocket notifications were build, they were lacking the device-id from the client who executed the update. This triggered the Desktop client to also sync the cipher, and therefore was complaining about it being out-of-sync.

The clients check if the websocket notification is from them selfs by the device-id, and if so, it will stop processing.

dani-garcia pushed a commit that referenced this issue Jan 9, 2023
@BlackDex @dani-garcia
Update WebSocket Notifications
7e66ab7 
Previously the websocket notifications were using `app_id` as the
`ContextId`. This was incorrect and should have been the device_uuid
from the client device executing the request. The clients will ignore
the websocket request if the uuid matches. This also fixes some issues
with the Desktop client which is able to modify attachments within the
same screen and causes an issue when saving the attachment afterwards.

Also changed the way to handle removed attachments, since that causes an
error saving the vault cipher afterwards, complaining about a missing
attachment. Bitwarden ignores this, and continues with the remaining
attachments (if any). This also fixes #2591 .

Further some more websocket notifications have been added to some other
functions which enhance the user experience.

- Logout users when deauthed, changed password, rotated keys
- Trigger OrgSyncKeys on user confirm and removal
- Added some extra to the send feature

Also renamed UpdateTypes to match Bitwarden naming.
dani-garcia pushed a commit that referenced this issue Jan 9, 2023
@BlackDex @dani-garcia
Update WebSocket Notifications
f1c0aa4 
Previously the websocket notifications were using `app_id` as the
`ContextId`. This was incorrect and should have been the device_uuid
from the client device executing the request. The clients will ignore
the websocket request if the uuid matches. This also fixes some issues
with the Desktop client which is able to modify attachments within the
same screen and causes an issue when saving the attachment afterwards.

Also changed the way to handle removed attachments, since that causes an
error saving the vault cipher afterwards, complaining about a missing
attachment. Bitwarden ignores this, and continues with the remaining
attachments (if any). This also fixes #2591 .

Further some more websocket notifications have been added to some other
functions which enhance the user experience.

- Logout users when deauthed, changed password, rotated keys
- Trigger OrgSyncKeys on user confirm and removal
- Added some extra to the send feature

Also renamed UpdateTypes to match Bitwarden naming.
dani-garcia pushed a commit that referenced this issue Jan 9, 2023
@BlackDex @dani-garcia
Update WebSocket Notifications
2972904 
Previously the websocket notifications were using `app_id` as the
`ContextId`. This was incorrect and should have been the device_uuid
from the client device executing the request. The clients will ignore
the websocket request if the uuid matches. This also fixes some issues
with the Desktop client which is able to modify attachments within the
same screen and causes an issue when saving the attachment afterwards.

Also changed the way to handle removed attachments, since that causes an
error saving the vault cipher afterwards, complaining about a missing
attachment. Bitwarden ignores this, and continues with the remaining
attachments (if any). This also fixes #2591 .

Further some more websocket notifications have been added to some other
functions which enhance the user experience.

- Logout users when deauthed, changed password, rotated keys
- Trigger OrgSyncKeys on user confirm and removal
- Added some extra to the send feature

Also renamed UpdateTypes to match Bitwarden naming.
dani-garcia pushed a commit that referenced this issue Jan 9, 2023
@BlackDex @dani-garcia
Update WebSocket Notifications
0042c3e 
Previously the websocket notifications were using `app_id` as the
`ContextId`. This was incorrect and should have been the device_uuid
from the client device executing the request. The clients will ignore
the websocket request if the uuid matches. This also fixes some issues
with the Desktop client which is able to modify attachments within the
same screen and causes an issue when saving the attachment afterwards.

Also changed the way to handle removed attachments, since that causes an
error saving the vault cipher afterwards, complaining about a missing
attachment. Bitwarden ignores this, and continues with the remaining
attachments (if any). This also fixes #2591 .

Further some more websocket notifications have been added to some other
functions which enhance the user experience.

- Logout users when deauthed, changed password, rotated keys
- Trigger OrgSyncKeys on user confirm and removal
- Added some extra to the send feature

Also renamed UpdateTypes to match Bitwarden naming.
@Nyxtorm
Copy link
Author

Nyxtorm commented Jan 10, 2023 •
edited
Loading

@Nyxtorm i had some time today to take a deeper dive into this issue. And it looks like i fixed it. I can now add and delete attachments via the Desktop client without any errors. I have to test this a bit more, and i also want to make some more changes in regards to the websocket notifications.

But, I'm working on it. Basically, what happened was that due to how the websocket notifications were build, they were lacking the device-id from the client who executed the update. This triggered the Desktop client to also sync the cipher, and therefore was complaining about it being out-of-sync.

The clients check if the websocket notification is from them selfs by the device-id, and if so, it will stop processing.

@BlackDex, I was able to take the time to test and it's just perfect now, thanks again. 👍

@BlackDex
Copy link
Collaborator

Great, good to see it is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BlackDex BlackDex

Labels
bug Something isn't working low priority Won't fix anytime soon, but will accept PR if provided troubleshooting There might be bug or it could be user error, more info needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update WebSocket Notifications BlackDex/vaultwarden
2 participants
@BlackDex @Nyxtorm