Validate DnD actions#4893
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Wayland protocol validation for drag-and-drop (DnD) action masks/values, ensuring invalid dnd_action bits are rejected with wl_data_source / wl_data_offer protocol errors.
Changes:
- Validate
wl_data_source.set_actions(dnd_actions)mask and raiseinvalid_action_maskon invalid bits. - Validate
wl_data_offer.set_actions(dnd_actions, preferred_action)mask/value and raiseinvalid_action_mask/invalid_actionon invalid inputs. - Introduce local helpers for DnD action validation in both the data source and data offer paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/server/frontend_wayland/wl_data_source.cpp |
Adds mask validation and posts wl_data_source protocol error on invalid action bits. |
src/server/frontend_wayland/wl_data_device.cpp |
Adds mask/value validation for wl_data_offer.set_actions() and posts protocol errors on invalid inputs. |
| resource, | ||
| Error::invalid_action, | ||
| "Invalid DnD action 0x%x", preferred_action); | ||
| } |
There was a problem hiding this comment.
wl_data_offer.set_actions requires raising a protocol error if preferred_action is not present in the wl_data_offer.source_actions mask (i.e., not supported by the source). Currently only the enum value is validated; please also validate preferred_action against source->actions() (allowing 'none'), and raise Error::invalid_action when it doesn't match.
| } | |
| } | |
| if (preferred_action != mw::DataDeviceManager::DndAction::none && | |
| (source->actions() & preferred_action) == 0) | |
| { | |
| throw mw::ProtocolError( | |
| resource, | |
| Error::invalid_action, | |
| "DnD preferred action 0x%x not supported by source action mask 0x%x", | |
| preferred_action, | |
| source->actions()); | |
| } |
| dnd_action == mw::DataDeviceManager::DndAction::ask; | ||
| } | ||
|
|
||
| void set_actions(uint32_t dnd_actions, uint32_t preferred_action) override |
There was a problem hiding this comment.
wl_data_offer.set_actions is only valid for drag-and-drop offers; this Offer class is also used for clipboard selection offers (see paste_source_set()). Please track whether an Offer represents a DnD operation and raise Error::invalid_offer (or appropriate wl_data_offer error) when set_actions is called on a non-DnD offer.
| static bool valid_actions(uint32_t dnd_actions) | ||
| { | ||
| return (dnd_actions & ~( | ||
| mw::DataDeviceManager::DndAction::none | | ||
| mw::DataDeviceManager::DndAction::copy | | ||
| mw::DataDeviceManager::DndAction::move | | ||
| mw::DataDeviceManager::DndAction::ask)) == 0; | ||
| } |
There was a problem hiding this comment.
valid_actions() duplicates the same allowed-bitmask logic that is also introduced in wl_data_source.cpp. To avoid the two sites diverging, consider centralizing the allowed-action mask (e.g., a shared helper/constant in a common frontend_wayland utility header or a single internal function reused by both).
There was a problem hiding this comment.
I was thinking the same but wasn't sure where exactly this should live - any suggestions?
| static bool valid_actions(uint32_t dnd_actions) | ||
| { | ||
| return (dnd_actions & ~( | ||
| mw::DataDeviceManager::DndAction::none | | ||
| mw::DataDeviceManager::DndAction::copy | | ||
| mw::DataDeviceManager::DndAction::move | | ||
| mw::DataDeviceManager::DndAction::ask)) == 0; | ||
| } |
There was a problem hiding this comment.
valid_actions() is introduced here with the same logic as the helper added in wl_data_device.cpp. Consider centralizing the allowed DnD action mask in one place to reduce duplication and the risk of the two checks drifting over time.
No description provided.