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

Prevent syncing row changes between users for views filtered by current user #15163

Merged
merged 5 commits into from
Dec 13, 2024
Merged

Prevent syncing row changes between users for views filtered by current user #15163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1250aa6
Fix scrollbar styles
aptkingston Dec 11, 2024
62e47cc
Don't sync changes between users for views that are filtered by curre…
aptkingston Dec 11, 2024
dc10bbf
Account for JS current user bindings as well as HBS
aptkingston Dec 12, 2024
3be6800
Merge branch 'master' into cheeks-fixes
aptkingston Dec 12, 2024
df8e7bf
Merge branch 'master' into cheeks-fixes
aptkingston Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/bbui/src/Layout/Page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,11 @@
flex-direction: row;
justify-content: flex-start;
align-items: stretch;
overflow-y: scroll !important;
flex: 1 1 auto;
overflow-x: hidden;
}
.main {
overflow: auto;
overflow-y: scroll;
}
.content {
display: flex;
Expand Down
5 changes: 2 additions & 3 deletions packages/builder/src/global.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ a {
height: 8px;
}
::-webkit-scrollbar-track {
background: var(--spectrum-alias-background-color-default);
background: transparent;
}
::-webkit-scrollbar-thumb {
background-color: var(--spectrum-global-color-gray-400);
Expand All @@ -71,6 +71,5 @@ a {
background: var(--spectrum-alias-background-color-default);
}
html * {
scrollbar-color: var(--spectrum-global-color-gray-400)
var(--spectrum-alias-background-color-default);
scrollbar-color: var(--spectrum-global-color-gray-400) transparent;
}
41 changes: 39 additions & 2 deletions packages/server/src/websockets/grid.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import authorized from "../middleware/authorized"
import currentApp from "../middleware/currentapp"
import { BaseSocket } from "./websocket"
import { auth, permissions } from "@budibase/backend-core"
import { auth, permissions, context } from "@budibase/backend-core"
import http from "http"
import Koa from "koa"
import { getSourceId } from "../api/controllers/row/utils"
Expand All @@ -10,6 +10,12 @@ import { Socket } from "socket.io"
import { GridSocketEvent } from "@budibase/shared-core"
import { userAgent } from "koa-useragent"
import { createContext, runMiddlewares } from "./middleware"
import sdk from "../sdk"
import {
findHBSBlocks,
isJSBinding,
decodeJSBinding,
} from "@budibase/string-templates"

const { PermissionType, PermissionLevel } = permissions

Expand All @@ -18,15 +24,46 @@ export default class GridSocket extends BaseSocket {
super(app, server, "/socket/grid")
}

// Checks if a view's query contains any current user bindings
containsCurrentUserBinding(view: ViewV2): boolean {
return findHBSBlocks(JSON.stringify(view.query))
.map(binding => {
const sanitizedBinding = binding.replace(/\\"/g, '"')
if (isJSBinding(sanitizedBinding)) {
return decodeJSBinding(sanitizedBinding)
} else {
return sanitizedBinding
}
})
.some(binding => binding?.includes("[user]"))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this always be the only true case? Could we not get other issues such as formulas or similar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

async onConnect(socket: Socket) {
// Initial identification of connected spreadsheet
socket.on(GridSocketEvent.SelectDatasource, async (payload, callback) => {
const ds = payload.datasource
const appId = payload.appId
const resourceId = ds?.type === "table" ? ds?.tableId : ds?.id
let valid = true

// Ignore if no table or app specified
// Validate datasource
if (!resourceId || !appId) {
// Ignore if no table or app specified
valid = false
} else if (ds.type === "viewV2") {
// If this is a view filtered by current user, don't sync changes
try {
await context.doInAppContext(appId, async () => {
const view = await sdk.views.get(ds.id)
if (this.containsCurrentUserBinding(view)) {
valid = false
}
})
} catch (err) {
valid = false
}
}
if (!valid) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of a product discussion, but are we exposing this somehow to the user? Otherwise they might think it's actually broken

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

socket.disconnect(true)
return
}
Expand Down
Loading