Merge pull request #7795 from Budibase/fix/7683

Fix for REST query string URL encoding

packages/bbui/src/Tooltip/TooltipWrapper.svelte

@@ -47,7 +47,7 @@
     display: flex;
     justify-content: center;
     top: 15px;
-    z-index: 100;
+    z-index: 200;
     width: 160px;
   }
   .icon {

packages/builder/package.json

@@ -9,6 +9,7 @@
     "dev:builder": "routify -c dev:vite",
     "dev:vite": "vite --host 0.0.0.0",
     "rollup": "rollup -c -w",
+    "test": "jest",
     "cy:setup": "ts-node ./cypress/ts/setup.ts",
     "cy:setup:ci": "node ./cypress/setup.js",
     "cy:open": "cypress open",
@@ -36,7 +37,8 @@
       "components(.*)$": "<rootDir>/src/components$1",
       "builderStore(.*)$": "<rootDir>/src/builderStore$1",
       "stores(.*)$": "<rootDir>/src/stores$1",
-      "analytics(.*)$": "<rootDir>/src/analytics$1"
+      "analytics(.*)$": "<rootDir>/src/analytics$1",
+      "constants/backend": "<rootDir>/src/constants/backend/index.js"
     },
     "moduleFileExtensions": [
       "js",

packages/builder/src/builderStore/dataBinding.js

@@ -9,14 +9,14 @@ import {
 import { store } from "builderStore"
 import {
   queries as queriesStores,
-  tables as tablesStore,
   roles as rolesStore,
+  tables as tablesStore,
 } from "stores/backend"
 import {
-  makePropSafe,
-  isJSBinding,
   decodeJSBinding,
   encodeJSBinding,
+  isJSBinding,
+  makePropSafe,
 } from "@budibase/string-templates"
 import { TableNames } from "../constants"
 import { JSONUtils } from "@budibase/frontend-core"
@@ -118,8 +118,7 @@ export const readableToRuntimeMap = (bindings, ctx) => {
     return {}
   }
   return Object.keys(ctx).reduce((acc, key) => {
-    let parsedQuery = readableToRuntimeBinding(bindings, ctx[key])
+    acc[key] = readableToRuntimeBinding(bindings, ctx[key])
-    acc[key] = parsedQuery
     return acc
   }, {})
 }
@@ -132,8 +131,7 @@ export const runtimeToReadableMap = (bindings, ctx) => {
     return {}
   }
   return Object.keys(ctx).reduce((acc, key) => {
-    let parsedQuery = runtimeToReadableBinding(bindings, ctx[key])
+    acc[key] = runtimeToReadableBinding(bindings, ctx[key])
-    acc[key] = parsedQuery
     return acc
   }, {})
 }

packages/builder/src/helpers/data/utils.js

@@ -1,4 +1,5 @@
 import { IntegrationTypes } from "constants/backend"
+import { findHBSBlocks } from "@budibase/string-templates"
 
 export function schemaToFields(schema) {
   const response = {}
@@ -31,7 +32,7 @@ export function breakQueryString(qs) {
   let paramObj = {}
   for (let param of params) {
     const split = param.split("=")
-    paramObj[split[0]] = split.slice(1).join("=")
+    paramObj[split[0]] = decodeURIComponent(split.slice(1).join("="))
   }
   return paramObj
 }
@@ -46,7 +47,19 @@ export function buildQueryString(obj) {
       if (str !== "") {
         str += "&"
       }
-      str += `${key}=${encodeURIComponent(value || "")}`
+      const bindings = findHBSBlocks(value)
+      let count = 0
+      const bindingMarkers = {}
+      bindings.forEach(binding => {
+        const marker = `BINDING...${count++}`
+        value = value.replace(binding, marker)
+        bindingMarkers[marker] = binding
+      })
+      let encoded = encodeURIComponent(value || "")
+      Object.entries(bindingMarkers).forEach(([marker, binding]) => {
+        encoded = encoded.replace(marker, binding)
+      })
+      str += `${key}=${encoded}`
     }
   }
   return str

packages/builder/src/helpers/tests/dataUtils.spec.js

@@ -0,0 +1,37 @@
+import { breakQueryString, buildQueryString } from "../data/utils"
+
+describe("check query string utils", () => {
+  const obj1 = {
+    key1: "123",
+    key2: "   ",
+    key3: "333",
+  }
+
+  const obj2 = {
+    key1: "{{ binding.awd }}",
+    key2: "{{ binding.sed }}  ",
+  }
+
+  it("should build a basic query string", () => {
+    const queryString = buildQueryString(obj1)
+    expect(queryString).toBe("key1=123&key2=%20%20%20&key3=333")
+  })
+
+  it("should be able to break a basic query string", () => {
+    const broken = breakQueryString("key1=123&key2=%20%20%20&key3=333")
+    expect(broken.key1).toBe(obj1.key1)
+    expect(broken.key2).toBe(obj1.key2)
+    expect(broken.key3).toBe(obj1.key3)
+  })
+
+  it("should be able to build with a binding", () => {
+    const queryString = buildQueryString(obj2)
+    expect(queryString).toBe("key1={{ binding.awd }}&key2={{ binding.sed }}%20%20")
+  })
+
+  it("should be able to break with a binding", () => {
+    const broken = breakQueryString("key1={{ binding.awd }}&key2={{ binding.sed }}%20%20")
+    expect(broken.key1).toBe(obj2.key1)
+    expect(broken.key2).toBe(obj2.key2)
+  })
+})

packages/builder/src/pages/builder/app/[application]/data/datasource/[selectedDatasource]/rest/[query]/index.svelte

@@ -708,6 +708,7 @@
   .url-block {
     display: flex;
     gap: var(--spacing-s);
+    z-index: 200;
   }
   .verb {
     flex: 1;

packages/builder/src/stores/backend/queries.js

@@ -1,7 +1,7 @@
 import { writable, get } from "svelte/store"
 import { datasources, integrations, tables, views } from "./"
 import { API } from "api"
-import { duplicateName } from "../../helpers/duplicate"
+import { duplicateName } from "helpers/duplicate"
 
 const sortQueries = queryList => {
   queryList.sort((q1, q2) => {

packages/builder/src/stores/backend/tables.js

@@ -2,7 +2,7 @@ import { get, writable } from "svelte/store"
 import { datasources, queries, views } from "./"
 import { cloneDeep } from "lodash/fp"
 import { API } from "api"
-import { SWITCHABLE_TYPES } from "../../constants/backend"
+import { SWITCHABLE_TYPES } from "constants/backend"
 
 export function createTablesStore() {
   const store = writable({})

packages/builder/src/stores/backend/tests/datasources.spec.js.disabled

@@ -1,9 +1,9 @@
-import { get } from 'svelte/store'
+import { get } from "svelte/store"
-import api from 'builderStore/api'
+import { API } from "api"
 
-jest.mock('builderStore/api');
+jest.mock("api")
 
-import { SOME_DATASOURCE, SAVE_DATASOURCE} from './fixtures/datasources'
+import { SOME_DATASOURCE, SAVE_DATASOURCE } from "./fixtures/datasources"
 
 import { createDatasourcesStore } from "../datasources"
 import { queries } from '../queries'
@@ -12,39 +12,39 @@ describe("Datasources Store", () => {
   let store = createDatasourcesStore()
 
   beforeEach(async () => {
-    api.get.mockReturnValue({ json: () => [SOME_DATASOURCE]})
+    API.getDatasources.mockReturnValue({ json: () => [SOME_DATASOURCE]})
     await store.init()
   })
 
   it("Initialises correctly", async () => {
-    api.get.mockReturnValue({ json: () => [SOME_DATASOURCE]})
+    API.getDatasources.mockReturnValue({ json: () => [SOME_DATASOURCE]})
-  
+
     await store.init()
     expect(get(store)).toEqual({ list: [SOME_DATASOURCE], selected: null})
   })
 
   it("fetches all the datasources and updates the store", async () => {
-    api.get.mockReturnValue({ json: () => [SOME_DATASOURCE] })
+    API.getDatasources.mockReturnValue({ json: () => [SOME_DATASOURCE] })
 
     await store.fetch()
-    expect(get(store)).toEqual({ list: [SOME_DATASOURCE], selected: null })  
+    expect(get(store)).toEqual({ list: [SOME_DATASOURCE], selected: null })
   })
 
   it("selects a datasource", async () => {
     store.select(SOME_DATASOURCE._id)
-  
+
-    expect(get(store).select).toEqual(SOME_DATASOURCE._id)  
+    expect(get(store).select).toEqual(SOME_DATASOURCE._id)
   })
 
   it("resets the queries store when new datasource is selected", async () => {
-    
+
     await store.select(SOME_DATASOURCE._id)
     const queriesValue = get(queries)
-    expect(queriesValue.selected).toEqual(null)  
+    expect(queriesValue.selected).toEqual(null)
   })
 
   it("saves the datasource, updates the store and returns status message", async () => {
-    api.post.mockReturnValue({ status: 200, json: () => SAVE_DATASOURCE})    
+    API.createDatasource.mockReturnValue({ status: 200, json: () => SAVE_DATASOURCE})
 
     await store.save({
       name: 'CoolDB',
@@ -56,13 +56,13 @@ describe("Datasources Store", () => {
     expect(get(store).list).toEqual(expect.arrayContaining([SAVE_DATASOURCE.datasource]))
   })
   it("deletes a datasource, updates the store and returns status message", async () => {
-    api.get.mockReturnValue({ json: () => SOME_DATASOURCE})
+    API.getDatasources.mockReturnValue({ json: () => SOME_DATASOURCE})
 
     await store.fetch()
 
-    api.delete.mockReturnValue({status: 200, message: 'Datasource deleted.'})    
+    API.deleteDatasource.mockReturnValue({status: 200, message: 'Datasource deleted.'})
 
     await store.delete(SOME_DATASOURCE[0])
-    expect(get(store)).toEqual({ list: [], selected: null})  
+    expect(get(store)).toEqual({ list: [], selected: null})
   })
 })

packages/builder/src/stores/backend/tests/permissions.spec.js.disabled

@@ -1,6 +1,6 @@
-import api from 'builderStore/api'
+import { API } from "api"
 
-jest.mock('builderStore/api');
+jest.mock("api")
 
 const PERMISSIONS_FOR_RESOURCE = {
     "write": "BASIC",
@@ -13,13 +13,12 @@ describe("Permissions Store", () => {
   const store = createPermissionStore()
 
   it("fetches permissions for specific resource", async () => {
-    api.get.mockReturnValueOnce({ json: () => PERMISSIONS_FOR_RESOURCE})
+    API.getPermissionForResource.mockReturnValueOnce({ json: () => PERMISSIONS_FOR_RESOURCE})
 
     const resourceId = "ta_013657543b4043b89dbb17e9d3a4723a"
 
     const permissions = await store.forResource(resourceId)
 
-    expect(api.get).toBeCalledWith(`/api/permission/${resourceId}`)
     expect(permissions).toEqual(PERMISSIONS_FOR_RESOURCE)
   })
 })

packages/builder/src/stores/backend/tests/queries.spec.js.disabled

@@ -1,9 +1,9 @@
-import { get } from 'svelte/store'
+import { get } from "svelte/store"
-import api from 'builderStore/api'
+import { API } from "api"
 
-jest.mock('builderStore/api');
+jest.mock("api")
 
-import { SOME_QUERY, SAVE_QUERY_RESPONSE } from './fixtures/queries'
+import { SOME_QUERY, SAVE_QUERY_RESPONSE } from "./fixtures/queries"
 
 import { createQueriesStore } from "../queries"
 
@@ -11,36 +11,36 @@ describe("Queries Store", () => {
   let store = createQueriesStore()
 
   beforeEach(async () => {
-    api.get.mockReturnValue({ json: () => [SOME_QUERY]})
+    API.getQueries.mockReturnValue({ json: () => [SOME_QUERY]})
     await store.init()
   })
 
   it("Initialises correctly", async () => {
-    api.get.mockReturnValue({ json: () => [SOME_QUERY]})
+    API.getQueries.mockReturnValue({ json: () => [SOME_QUERY]})
-  
+
     await store.init()
     expect(get(store)).toEqual({ list: [SOME_QUERY], selected: null})
   })
 
   it("fetches all the queries", async () => {
-    api.get.mockReturnValue({ json: () => [SOME_QUERY]})
+    API.getQueries.mockReturnValue({ json: () => [SOME_QUERY]})
 
     await store.fetch()
-    expect(get(store)).toEqual({ list: [SOME_QUERY], selected: null})  
+    expect(get(store)).toEqual({ list: [SOME_QUERY], selected: null})
   })
 
   it("saves the query, updates the store and returns status message", async () => {
-    api.post.mockReturnValue({ json: () => SAVE_QUERY_RESPONSE})    
+    API.saveQuery.mockReturnValue({ json: () => SAVE_QUERY_RESPONSE})
 
     await store.select(SOME_QUERY.datasourceId, SOME_QUERY)
 
     expect(get(store).list).toEqual(expect.arrayContaining([SOME_QUERY]))
   })
   it("deletes a query, updates the store and returns status message", async () => {
-    
+
-    api.delete.mockReturnValue({status: 200, message: `Query deleted.`})  
+    API.deleteQuery.mockReturnValue({status: 200, message: `Query deleted.`})
-    
+
     await store.delete(SOME_QUERY)
-    expect(get(store)).toEqual({ list: [], selected: null})  
+    expect(get(store)).toEqual({ list: [], selected: null})
   })
 })

packages/builder/src/stores/backend/tests/roles.spec.js.disabled

@@ -1,10 +1,10 @@
-import { get } from 'svelte/store'
+import { get } from "svelte/store"
-import api from 'builderStore/api'
+import { API } from "api"
 
-jest.mock('builderStore/api');
+jest.mock("api")
 
 import { createRolesStore } from "../roles"
-import { ROLES } from './fixtures/roles'
+import { ROLES } from "./fixtures/roles"
 
 describe("Roles Store", () => {
   let store = createRolesStore()
@@ -14,19 +14,18 @@ describe("Roles Store", () => {
   })
 
   it("fetches roles from backend", async () => {
-    api.get.mockReturnValue({ json: () => ROLES})
+    API.getRoles.mockReturnValue({ json: () => ROLES})
     await store.fetch()
 
-    expect(api.get).toBeCalledWith("/api/roles")
     expect(get(store)).toEqual(ROLES)
   })
 
   it("deletes a role", async () => {
-    api.get.mockReturnValueOnce({ json: () => ROLES})
+    API.getRoles.mockReturnValueOnce({ json: () => ROLES})
     await store.fetch()
-    
+
-    api.delete.mockReturnValue({status: 200, message: `Role deleted.`})
+    API.deleteRole.mockReturnValue({status: 200, message: `Role deleted.`})
-    
+
     const updatedRoles = [...ROLES.slice(1)]
     await store.delete(ROLES[0])
 

packages/builder/src/stores/backend/tests/tables.spec.js.disabled

@@ -1,18 +1,16 @@
-import { get } from 'svelte/store'
+import { get } from "svelte/store"
-import api from 'builderStore/api'
+import { API } from "api"
 
-jest.mock('builderStore/api');
+jest.mock("api")
-
-import { SOME_TABLES, SAVE_TABLES_RESPONSE, A_TABLE } from './fixtures/tables'
 
+import { SOME_TABLES, SAVE_TABLES_RESPONSE, A_TABLE } from "./fixtures/tables"
 import { createTablesStore } from "../tables"
-import { views } from '../views'
 
 describe("Tables Store", () => {
   let store = createTablesStore()
 
   beforeEach(async () => {
-    api.get.mockReturnValue({ json: () => SOME_TABLES})
+    API.getTables.mockReturnValue({ json: () => SOME_TABLES})
     await store.init()
   })
 
@@ -21,46 +19,46 @@ describe("Tables Store", () => {
   })
 
   it("fetches all the tables", async () => {
-    api.get.mockReturnValue({ json: () => SOME_TABLES})
+    API.getTables.mockReturnValue({ json: () => SOME_TABLES})
 
     await store.fetch()
-    expect(get(store)).toEqual({ list: SOME_TABLES, selected: {}, draft: {}})  
+    expect(get(store)).toEqual({ list: SOME_TABLES, selected: {}, draft: {}})
   })
 
   it("selects a table", async () => {
     const tableToSelect = SOME_TABLES[0]
     await store.select(tableToSelect)
-  
+
-    expect(get(store).selected).toEqual(tableToSelect) 
+    expect(get(store).selected).toEqual(tableToSelect)
-    expect(get(store).draft).toEqual(tableToSelect) 
+    expect(get(store).draft).toEqual(tableToSelect)
   })
 
   it("selecting without a param resets the selected property", async () => {
     await store.select()
-  
+
-    expect(get(store).draft).toEqual({}) 
+    expect(get(store).draft).toEqual({})
   })
 
   it("saving a table also selects it", async () => {
-    api.post.mockReturnValue({ status: 200, json: () => SAVE_TABLES_RESPONSE})
+    API.post.mockReturnValue({ status: 200, json: () => SAVE_TABLES_RESPONSE})
 
     await store.save(A_TABLE)
 
-    expect(get(store).selected).toEqual(SAVE_TABLES_RESPONSE) 
+    expect(get(store).selected).toEqual(SAVE_TABLES_RESPONSE)
   })
 
   it("saving the table returns a response", async () => {
-    api.post.mockReturnValue({ status: 200, json: () => SAVE_TABLES_RESPONSE})
+    API.saveTable.mockReturnValue({ status: 200, json: () => SAVE_TABLES_RESPONSE})
 
     const response = await store.save(A_TABLE)
 
-    expect(response).toEqual(SAVE_TABLES_RESPONSE) 
+    expect(response).toEqual(SAVE_TABLES_RESPONSE)
   })
   it("deleting a table removes it from the store", async () => {
-    api.delete.mockReturnValue({status: 200, message: `Table deleted.`})  
+    API.deleteTable.mockReturnValue({status: 200, message: `Table deleted.`})
-    
+
     await store.delete(A_TABLE)
-    expect(get(store).list).toEqual(expect.not.arrayContaining([A_TABLE]))  
+    expect(get(store).list).toEqual(expect.not.arrayContaining([A_TABLE]))
   })
 
   // TODO: Write tests for saving and deleting fields

packages/worker/src/api/controllers/global/self.js

@@ -80,16 +80,15 @@ const addSessionAttributesToUser = ctx => {
   ctx.body.csrfToken = ctx.user.csrfToken
 }
 
-/**
+const sanitiseUserUpdate = ctx => {
- * Remove the attributes that are session based from the current user,
+  const allowed = ["firstName", "lastName", "password", "forceResetPassword"]
- * so that stale values are not written to the db
+  const resp = {}
- */
+  for (let [key, value] of Object.entries(ctx.request.body)) {
-const removeSessionAttributesFromUser = ctx => {
+    if (allowed.includes(key)) {
-  delete ctx.request.body.csrfToken
+      resp[key] = value
-  delete ctx.request.body.account
+    }
-  delete ctx.request.body.accountPortalAccess
+  }
-  delete ctx.request.body.budibaseAccess
+  return resp
-  delete ctx.request.body.license
 }
 
 exports.getSelf = async ctx => {
@@ -117,10 +116,12 @@ exports.updateSelf = async ctx => {
   const db = getGlobalDB()
   const user = await db.get(ctx.user._id)
   let passwordChange = false
-  if (ctx.request.body.password) {
+
+  const userUpdateObj = sanitiseUserUpdate(ctx)
+  if (userUpdateObj.password) {
     // changing password
     passwordChange = true
-    ctx.request.body.password = await hash(ctx.request.body.password)
+    userUpdateObj.password = await hash(userUpdateObj.password)
     // Log all other sessions out apart from the current one
     await platformLogout({
       ctx,
@@ -128,14 +129,10 @@ exports.updateSelf = async ctx => {
       keepActiveSession: true,
     })
   }
-  // don't allow sending up an ID/Rev, always use the existing one
-  delete ctx.request.body._id
-  delete ctx.request.body._rev
-  removeSessionAttributesFromUser(ctx)
 
   const response = await db.put({
     ...user,
-    ...ctx.request.body,
+    ...userUpdateObj,
   })
   await userCache.invalidateUser(user._id)
   ctx.body = {

packages/worker/src/api/controllers/global/users.ts

@@ -14,7 +14,6 @@ import {
   errors,
   events,
   tenancy,
-  users as usersCore,
 } from "@budibase/backend-core"
 import { checkAnyUserExists } from "../../../utilities/users"
 import { groups as groupUtils } from "@budibase/pro"
@@ -148,9 +147,7 @@ export const bulkDelete = async (ctx: any) => {
   }
 
   try {
-    let response = await users.bulkDelete(userIds)
+    ctx.body = await users.bulkDelete(userIds)
-
-    ctx.body = response
   } catch (err) {
     ctx.throw(err)
   }