Prevent syncing row changes between users for views filtered by current user #15163
Conversation
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
aptkingston
changed the title
aptkingston
requested review from
mike12345567
and removed request for
a team
| return sanitizedBinding | ||
| } | ||
| }) | ||
| .some(binding => binding?.includes("[user]")) |
There was a problem hiding this comment.
Will this always be the only true case? Could we not get other issues such as formulas or similar?
There was a problem hiding this comment.
For this fix we only care about data segregated per user. There's a wider problem of data not matching filters in general, but by identifying user bindings we can fix cases where users are seeing row they should not be able to access. Only the current user bindings create this security risk. This PR is deliberately only targetting user bindings and is not meant to be a wider fix.
| valid = false | ||
| } | ||
| } | ||
| if (!valid) { |
There was a problem hiding this comment.
More of a product discussion, but are we exposing this somehow to the user? Otherwise they might think it's actually broken
There was a problem hiding this comment.
We aren't, no. But I think that's ok, as you wouldn't expect to be able to see rows created by other users if using a view that's filtered to show only your own rows.
There was a problem hiding this comment.
It looks like the issue is not related to the user filter, but on any filter. Create a different view that, for example, filters by "title starting with a". The same issue will happen, as the grid socket will push the row ignoring any filter.
Screen.Recording.2024-12-12.at.10.40.37.mov
The same thing does happen with all filters yes, but it's less significant for other filters. We are prioritising a quick solution specifically for user-segregated data as that has larger security implications compared to just seeing rows which don't match the currently applied filters. I agree we need a wider solution for all filters, but it's not an easy fix. It will probably require evaluating all filters server side against rows before broadcasting them down the websocket. |
We already have a way to analyse the filters on the backend. We use it to filter them in memory, so we could check it they are part of the filter or not. If there is rush of shipping this one, we can go for it. Or if you want me to have a look at the filter part, I am happy to do it. I really think we can reuse everything we already have (if we have the context that this websocket is for a view) |
|
I think we'll need to release this in the meantime, because the other solution is a much larger change. Right now you just subscribe to a datasource and that's the identifier for changes. We would have to refactor things so that users resubscribe when their query changes (their local frontend query which can be attached to any DS+). I think the flow would look something like:
It's pretty significant and there is some time pressure to release this one, so I think we should proceed with this for now. |
Description
This PR prevents syncing row changes between users when using views that are filtered by any current user property. This is required as it can otherwise expose data which should not be seen by other users. I'm just stringifying and searching the
queryproperty for the presence of a current user binding as my litmus test.Currently the view doc is fetched and checked whenever a user connects to a certain datasource, which is infrequent - typically just once per page load, so I don't think there's much to be gained my computing this on update and storing a flag into the doc, but opinions are welcome.
Also contains a tiny fix for scrollbar styles as they have looked wrong for a few months and it's about time they were fixed!
Addresses