From 520651119b1f139872ac6794342687af0846453a Mon Sep 17 00:00:00 2001 From: Andrew Kingston Date: Thu, 22 Jul 2021 15:53:20 +0100 Subject: [PATCH 1/5] Fix lucene filtering of all types by parsing values as expected types, and correctly wrapping non-numeric types while building queries --- .../src/api/controllers/row/internalSearch.js | 79 ++++++++++++++----- packages/standard-components/src/lucene.js | 16 ++-- 2 files changed, 70 insertions(+), 25 deletions(-) diff --git a/packages/server/src/api/controllers/row/internalSearch.js b/packages/server/src/api/controllers/row/internalSearch.js index 72f5e6e5c8..041f030477 100644 --- a/packages/server/src/api/controllers/row/internalSearch.js +++ b/packages/server/src/api/controllers/row/internalSearch.js @@ -3,13 +3,35 @@ const env = require("../../../environment") const fetch = require("node-fetch") /** - * Escapes any characters in a string which lucene searches require to be - * escaped. - * @param value The value to escape - * @returns {string} + * Preprocesses a value before going into a lucene search. + * Transforms strings to lowercase and wraps strings and bools in quotes. + * @param value The value to process + * @param options The preprocess options + * @returns {string|*} */ -const luceneEscape = value => { - return `${value}`.replace(/[ #+\-&|!(){}\]^"~*?:\\]/g, "\\$&") +const preprocess = ( + value, + options = { escape: true, lowercase: true, wrap: true } +) => { + // Determine if type needs wrapped + const originalType = typeof value + + // Convert to lowercase + if (options.lowercase) { + value = value?.toLowerCase ? value.toLowerCase() : value + } + + // Escape characters + if (options.escape) { + value = `${value}`.replace(/[ #+\-&|!(){}\]^"~*?:\\]/g, "\\$&") + } + + // Wrap in quotes + if (options.wrap) { + value = originalType === "number" ? value : `"${value}"` + } + + return value } /** @@ -113,7 +135,10 @@ class QueryBuilder { function build(structure, queryFn) { for (let [key, value] of Object.entries(structure)) { - const expression = queryFn(luceneEscape(key.replace(/ /, "_")), value) + key = preprocess(key.replace(/ /, "_"), { + escape: true, + }) + const expression = queryFn(key, value) if (expression == null) { continue } @@ -124,7 +149,14 @@ class QueryBuilder { // Construct the actual lucene search query string from JSON structure if (this.query.string) { build(this.query.string, (key, value) => { - return value ? `${key}:${luceneEscape(value.toLowerCase())}*` : null + if (!value) { + return null + } + value = preprocess(value, { + escape: true, + lowercase: true, + }) + return `${key}:${value}*` }) } if (this.query.range) { @@ -138,30 +170,37 @@ class QueryBuilder { if (value.high == null || value.high === "") { return null } - return `${key}:[${value.low} TO ${value.high}]` + const low = preprocess(value.low) + const high = preprocess(value.high) + return `${key}:[${low} TO ${high}]` }) } if (this.query.fuzzy) { build(this.query.fuzzy, (key, value) => { - return value ? `${key}:${luceneEscape(value.toLowerCase())}~` : null + if (!value) { + return null + } + value = preprocess(value, { + escape: true, + lowercase: true, + }) + return `${key}:${value}~` }) } if (this.query.equal) { build(this.query.equal, (key, value) => { - const escapedValue = luceneEscape(value.toLowerCase()) - // have to do the or to manage straight values, or strings - return value - ? `(${key}:${escapedValue} OR ${key}:"${escapedValue}")` - : null + if (!value) { + return null + } + return `${key}:${preprocess(value)}` }) } if (this.query.notEqual) { build(this.query.notEqual, (key, value) => { - const escapedValue = luceneEscape(value.toLowerCase()) - // have to do the or to manage straight values, or strings - return value - ? `(!${key}:${escapedValue} OR !${key}:"${escapedValue}")` - : null + if (!value) { + return null + } + return `!${key}:${preprocess(value)}` }) } if (this.query.empty) { diff --git a/packages/standard-components/src/lucene.js b/packages/standard-components/src/lucene.js index 91c69dfda2..317d8c3e74 100644 --- a/packages/standard-components/src/lucene.js +++ b/packages/standard-components/src/lucene.js @@ -15,10 +15,16 @@ export const buildLuceneQuery = filter => { if (Array.isArray(filter)) { filter.forEach(expression => { let { operator, field, type, value } = expression - // Ensure date fields are transformed into ISO strings + // Parse all values into correct types if (type === "datetime" && value) { value = new Date(value).toISOString() } + if (type === "number") { + value = parseFloat(value) + } + if (type === "boolean") { + value = value?.toLowerCase() === "true" + } if (operator.startsWith("range")) { if (!query.range[field]) { query.range[field] = { @@ -42,10 +48,10 @@ export const buildLuceneQuery = filter => { // Transform boolean filters to cope with null. // "equals false" needs to be "not equals true" // "not equals false" needs to be "equals true" - if (operator === "equal" && value === "false") { - query.notEqual[field] = "true" - } else if (operator === "notEqual" && value === "false") { - query.equal[field] = "true" + if (operator === "equal" && value === false) { + query.notEqual[field] = true + } else if (operator === "notEqual" && value === false) { + query.equal[field] = true } else { query[operator][field] = value } From e24c6bafd1a1ac217cf8b9b61496c6f43c8b4955 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 23 Jul 2021 13:07:10 +0100 Subject: [PATCH 2/5] Removing optional chaining, not valid in Node. --- packages/server/src/api/controllers/row/internalSearch.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/api/controllers/row/internalSearch.js b/packages/server/src/api/controllers/row/internalSearch.js index 041f030477..148ba3806f 100644 --- a/packages/server/src/api/controllers/row/internalSearch.js +++ b/packages/server/src/api/controllers/row/internalSearch.js @@ -17,8 +17,8 @@ const preprocess = ( const originalType = typeof value // Convert to lowercase - if (options.lowercase) { - value = value?.toLowerCase ? value.toLowerCase() : value + if (value && options.lowercase) { + value = value.toLowerCase ? value.toLowerCase() : value } // Escape characters From a5d9883f280103f23e4327c68779cf1cbe3150c0 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 23 Jul 2021 13:29:50 +0100 Subject: [PATCH 3/5] Updating internal search to disable features were required. --- .../src/api/controllers/row/internalSearch.js | 15 +++++---------- packages/server/src/middleware/currentapp.js | 7 ++++++- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/packages/server/src/api/controllers/row/internalSearch.js b/packages/server/src/api/controllers/row/internalSearch.js index 148ba3806f..11d729051c 100644 --- a/packages/server/src/api/controllers/row/internalSearch.js +++ b/packages/server/src/api/controllers/row/internalSearch.js @@ -22,7 +22,7 @@ const preprocess = ( } // Escape characters - if (options.escape) { + if (options.escape && originalType === "string") { value = `${value}`.replace(/[ #+\-&|!(){}\]^"~*?:\\]/g, "\\$&") } @@ -136,7 +136,8 @@ class QueryBuilder { function build(structure, queryFn) { for (let [key, value] of Object.entries(structure)) { key = preprocess(key.replace(/ /, "_"), { - escape: true, + wrap: false, + lowercase: false, }) const expression = queryFn(key, value) if (expression == null) { @@ -152,10 +153,7 @@ class QueryBuilder { if (!value) { return null } - value = preprocess(value, { - escape: true, - lowercase: true, - }) + value = preprocess(value) return `${key}:${value}*` }) } @@ -180,10 +178,7 @@ class QueryBuilder { if (!value) { return null } - value = preprocess(value, { - escape: true, - lowercase: true, - }) + value = preprocess(value) return `${key}:${value}~` }) } diff --git a/packages/server/src/middleware/currentapp.js b/packages/server/src/middleware/currentapp.js index 0e9591456c..bf043f67b3 100644 --- a/packages/server/src/middleware/currentapp.js +++ b/packages/server/src/middleware/currentapp.js @@ -12,7 +12,12 @@ module.exports = async (ctx, next) => { // try to get the appID from the request const requestAppId = getAppId(ctx) // get app cookie if it exists - const appCookie = getCookie(ctx, Cookies.CurrentApp) + let appCookie = null + try { + appCookie = getCookie(ctx, Cookies.CurrentApp) + } catch (err) { + clearCookie(ctx, Cookies.CurrentApp) + } if (!appCookie && !requestAppId) { return next() } From 94744ffbd8f7241e24014b98c20f9e68ba1032e7 Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 23 Jul 2021 13:44:46 +0100 Subject: [PATCH 4/5] Updating to use default false for search pre-processing. --- .../src/api/controllers/row/internalSearch.js | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/packages/server/src/api/controllers/row/internalSearch.js b/packages/server/src/api/controllers/row/internalSearch.js index 11d729051c..ae4c55453f 100644 --- a/packages/server/src/api/controllers/row/internalSearch.js +++ b/packages/server/src/api/controllers/row/internalSearch.js @@ -9,25 +9,22 @@ const fetch = require("node-fetch") * @param options The preprocess options * @returns {string|*} */ -const preprocess = ( - value, - options = { escape: true, lowercase: true, wrap: true } -) => { +const preprocess = (value, { escape, lowercase, wrap } = {}) => { // Determine if type needs wrapped const originalType = typeof value // Convert to lowercase - if (value && options.lowercase) { + if (value && lowercase) { value = value.toLowerCase ? value.toLowerCase() : value } // Escape characters - if (options.escape && originalType === "string") { + if (escape && originalType === "string") { value = `${value}`.replace(/[ #+\-&|!(){}\]^"~*?:\\]/g, "\\$&") } // Wrap in quotes - if (options.wrap) { + if (wrap) { value = originalType === "number" ? value : `"${value}"` } @@ -132,12 +129,12 @@ class QueryBuilder { buildSearchQuery() { let query = "*:*" + const allPreProcessingOpts = { escape: true, lowercase: true, wrap: true } function build(structure, queryFn) { for (let [key, value] of Object.entries(structure)) { key = preprocess(key.replace(/ /, "_"), { - wrap: false, - lowercase: false, + escape: true, }) const expression = queryFn(key, value) if (expression == null) { @@ -153,7 +150,10 @@ class QueryBuilder { if (!value) { return null } - value = preprocess(value) + value = preprocess(value, { + escape: true, + lowercase: true, + }) return `${key}:${value}*` }) } @@ -168,8 +168,8 @@ class QueryBuilder { if (value.high == null || value.high === "") { return null } - const low = preprocess(value.low) - const high = preprocess(value.high) + const low = preprocess(value.low, allPreProcessingOpts) + const high = preprocess(value.high, allPreProcessingOpts) return `${key}:[${low} TO ${high}]` }) } @@ -178,7 +178,10 @@ class QueryBuilder { if (!value) { return null } - value = preprocess(value) + value = preprocess(value, { + escape: true, + lowercase: true, + }) return `${key}:${value}~` }) } @@ -187,7 +190,7 @@ class QueryBuilder { if (!value) { return null } - return `${key}:${preprocess(value)}` + return `${key}:${preprocess(value, allPreProcessingOpts)}` }) } if (this.query.notEqual) { @@ -195,7 +198,7 @@ class QueryBuilder { if (!value) { return null } - return `!${key}:${preprocess(value)}` + return `!${key}:${preprocess(value, allPreProcessingOpts)}` }) } if (this.query.empty) { From fa3cf585d73b5f2eed2d2a3a015731a1f0d496cb Mon Sep 17 00:00:00 2001 From: mike12345567 Date: Fri, 23 Jul 2021 15:29:14 +0100 Subject: [PATCH 5/5] Adding concept of version to APIs. --- packages/auth/constants.js | 1 + packages/auth/src/constants.js | 7 ++ packages/auth/src/middleware/authenticated.js | 12 +-- packages/auth/src/utils.js | 5 +- .../src/api/controllers/row/internal.js | 1 + .../src/api/controllers/row/internalSearch.js | 76 ++++++++++--------- .../src/tests/utilities/TestConfiguration.js | 8 +- .../server/src/utilities/workerRequests.js | 3 +- 8 files changed, 65 insertions(+), 48 deletions(-) create mode 100644 packages/auth/constants.js diff --git a/packages/auth/constants.js b/packages/auth/constants.js new file mode 100644 index 0000000000..4abb7703db --- /dev/null +++ b/packages/auth/constants.js @@ -0,0 +1 @@ +module.exports = require("./src/constants") diff --git a/packages/auth/src/constants.js b/packages/auth/src/constants.js index 230c80b609..ab0749559a 100644 --- a/packages/auth/src/constants.js +++ b/packages/auth/src/constants.js @@ -8,6 +8,13 @@ exports.Cookies = { Auth: "budibase:auth", } +exports.Headers = { + API_KEY: "x-budibase-api-key", + API_VER: "x-budibase-api-version", + APP_ID: "x-budibase-app-id", + TYPE: "x-budibase-type", +} + exports.GlobalRoles = { OWNER: "owner", ADMIN: "admin", diff --git a/packages/auth/src/middleware/authenticated.js b/packages/auth/src/middleware/authenticated.js index 64494f709d..d9b4ed196a 100644 --- a/packages/auth/src/middleware/authenticated.js +++ b/packages/auth/src/middleware/authenticated.js @@ -1,4 +1,4 @@ -const { Cookies } = require("../constants") +const { Cookies, Headers } = require("../constants") const database = require("../db") const { getCookie, clearCookie } = require("../utils") const { StaticDatabases } = require("../db/utils") @@ -22,15 +22,17 @@ function buildNoAuthRegex(patterns) { }) } -function finalise(ctx, { authenticated, user, internal } = {}) { +function finalise(ctx, { authenticated, user, internal, version } = {}) { ctx.isAuthenticated = authenticated || false ctx.user = user ctx.internal = internal || false + ctx.version = version } module.exports = (noAuthPatterns = [], opts) => { const noAuthOptions = noAuthPatterns ? buildNoAuthRegex(noAuthPatterns) : [] return async (ctx, next) => { + const version = ctx.request.headers[Headers.API_VER] // the path is not authenticated const found = noAuthOptions.find(({ regex, method }) => { return ( @@ -58,7 +60,7 @@ module.exports = (noAuthPatterns = [], opts) => { clearCookie(ctx, Cookies.Auth) } } - const apiKey = ctx.request.headers["x-budibase-api-key"] + const apiKey = ctx.request.headers[Headers.API_KEY] // this is an internal request, no user made it if (!authenticated && apiKey && apiKey === env.INTERNAL_API_KEY) { authenticated = true @@ -69,12 +71,12 @@ module.exports = (noAuthPatterns = [], opts) => { authenticated = false } // isAuthenticated is a function, so use a variable to be able to check authed state - finalise(ctx, { authenticated, user, internal }) + finalise(ctx, { authenticated, user, internal, version }) return next() } catch (err) { // allow configuring for public access if (opts && opts.publicAllowed) { - finalise(ctx, { authenticated: false }) + finalise(ctx, { authenticated: false, version }) } else { ctx.throw(err.status || 403, err) } diff --git a/packages/auth/src/utils.js b/packages/auth/src/utils.js index 278ad07174..5604eb1d79 100644 --- a/packages/auth/src/utils.js +++ b/packages/auth/src/utils.js @@ -8,6 +8,7 @@ const jwt = require("jsonwebtoken") const { options } = require("./middleware/passport/jwt") const { createUserEmailView } = require("./db/views") const { getDB } = require("./db") +const { Headers } = require("./constants") const APP_PREFIX = DocumentTypes.APP + SEPARATOR @@ -23,7 +24,7 @@ function confirmAppId(possibleAppId) { * @returns {string|undefined} If an appId was found it will be returned. */ exports.getAppId = ctx => { - const options = [ctx.headers["x-budibase-app-id"], ctx.params.appId] + const options = [ctx.headers[Headers.APP_ID], ctx.params.appId] if (ctx.subdomains) { options.push(ctx.subdomains[1]) } @@ -102,7 +103,7 @@ exports.clearCookie = (ctx, name) => { * @return {boolean} returns true if the call is from the client lib (a built app rather than the builder). */ exports.isClient = ctx => { - return ctx.headers["x-budibase-type"] === "client" + return ctx.headers[Headers.TYPE] === "client" } /** diff --git a/packages/server/src/api/controllers/row/internal.js b/packages/server/src/api/controllers/row/internal.js index f0009a4413..25ebb5375b 100644 --- a/packages/server/src/api/controllers/row/internal.js +++ b/packages/server/src/api/controllers/row/internal.js @@ -278,6 +278,7 @@ exports.search = async ctx => { const { tableId } = ctx.params const db = new CouchDB(appId) const { paginate, query, ...params } = ctx.request.body + params.version = ctx.version params.tableId = tableId let response diff --git a/packages/server/src/api/controllers/row/internalSearch.js b/packages/server/src/api/controllers/row/internalSearch.js index ae4c55453f..a1aa77b34a 100644 --- a/packages/server/src/api/controllers/row/internalSearch.js +++ b/packages/server/src/api/controllers/row/internalSearch.js @@ -2,35 +2,6 @@ const { SearchIndexes } = require("../../../db/utils") const env = require("../../../environment") const fetch = require("node-fetch") -/** - * Preprocesses a value before going into a lucene search. - * Transforms strings to lowercase and wraps strings and bools in quotes. - * @param value The value to process - * @param options The preprocess options - * @returns {string|*} - */ -const preprocess = (value, { escape, lowercase, wrap } = {}) => { - // Determine if type needs wrapped - const originalType = typeof value - - // Convert to lowercase - if (value && lowercase) { - value = value.toLowerCase ? value.toLowerCase() : value - } - - // Escape characters - if (escape && originalType === "string") { - value = `${value}`.replace(/[ #+\-&|!(){}\]^"~*?:\\]/g, "\\$&") - } - - // Wrap in quotes - if (wrap) { - value = originalType === "number" ? value : `"${value}"` - } - - return value -} - /** * Class to build lucene query URLs. * Optionally takes a base lucene query object. @@ -52,6 +23,11 @@ class QueryBuilder { this.sortOrder = "ascending" this.sortType = "string" this.includeDocs = true + this.version = null + } + + setVersion(version) { + this.version = version } setTable(tableId) { @@ -127,13 +103,40 @@ class QueryBuilder { return this } + /** + * Preprocesses a value before going into a lucene search. + * Transforms strings to lowercase and wraps strings and bools in quotes. + * @param value The value to process + * @param options The preprocess options + * @returns {string|*} + */ + preprocess(value, { escape, lowercase, wrap } = {}) { + const hasVersion = !!this.version + // Determine if type needs wrapped + const originalType = typeof value + // Convert to lowercase + if (value && lowercase) { + value = value.toLowerCase ? value.toLowerCase() : value + } + // Escape characters + if (escape && originalType === "string") { + value = `${value}`.replace(/[ #+\-&|!(){}\]^"~*?:\\]/g, "\\$&") + } + // Wrap in quotes + if (hasVersion && wrap) { + value = originalType === "number" ? value : `"${value}"` + } + return value + } + buildSearchQuery() { + const builder = this let query = "*:*" const allPreProcessingOpts = { escape: true, lowercase: true, wrap: true } function build(structure, queryFn) { for (let [key, value] of Object.entries(structure)) { - key = preprocess(key.replace(/ /, "_"), { + key = builder.preprocess(key.replace(/ /, "_"), { escape: true, }) const expression = queryFn(key, value) @@ -150,7 +153,7 @@ class QueryBuilder { if (!value) { return null } - value = preprocess(value, { + value = builder.preprocess(value, { escape: true, lowercase: true, }) @@ -168,8 +171,8 @@ class QueryBuilder { if (value.high == null || value.high === "") { return null } - const low = preprocess(value.low, allPreProcessingOpts) - const high = preprocess(value.high, allPreProcessingOpts) + const low = builder.preprocess(value.low, allPreProcessingOpts) + const high = builder.preprocess(value.high, allPreProcessingOpts) return `${key}:[${low} TO ${high}]` }) } @@ -178,7 +181,7 @@ class QueryBuilder { if (!value) { return null } - value = preprocess(value, { + value = builder.preprocess(value, { escape: true, lowercase: true, }) @@ -190,7 +193,7 @@ class QueryBuilder { if (!value) { return null } - return `${key}:${preprocess(value, allPreProcessingOpts)}` + return `${key}:${builder.preprocess(value, allPreProcessingOpts)}` }) } if (this.query.notEqual) { @@ -198,7 +201,7 @@ class QueryBuilder { if (!value) { return null } - return `!${key}:${preprocess(value, allPreProcessingOpts)}` + return `!${key}:${builder.preprocess(value, allPreProcessingOpts)}` }) } if (this.query.empty) { @@ -287,6 +290,7 @@ const recursiveSearch = async (appId, query, params) => { pageSize = params.limit - rows.length } const page = await new QueryBuilder(appId, query) + .setVersion(params.version) .setTable(params.tableId) .setBookmark(bookmark) .setLimit(pageSize) diff --git a/packages/server/src/tests/utilities/TestConfiguration.js b/packages/server/src/tests/utilities/TestConfiguration.js index 83786e0155..204af06b0a 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.js +++ b/packages/server/src/tests/utilities/TestConfiguration.js @@ -14,7 +14,7 @@ const { const controllers = require("./controllers") const supertest = require("supertest") const { cleanup } = require("../../utilities/fileSystem") -const { Cookies } = require("@budibase/auth").constants +const { Cookies, Headers } = require("@budibase/auth").constants const { jwt } = require("@budibase/auth").auth const { StaticDatabases } = require("@budibase/auth/db") const CouchDB = require("../../db") @@ -118,7 +118,7 @@ class TestConfiguration { ], } if (this.appId) { - headers["x-budibase-app-id"] = this.appId + headers[Headers.APP_ID] = this.appId } return headers } @@ -128,7 +128,7 @@ class TestConfiguration { Accept: "application/json", } if (this.appId) { - headers["x-budibase-app-id"] = this.appId + headers[Headers.APP_ID] = this.appId } return headers } @@ -349,7 +349,7 @@ class TestConfiguration { `${Cookies.Auth}=${authToken}`, `${Cookies.CurrentApp}=${appToken}`, ], - "x-budibase-app-id": this.appId, + [Headers.APP_ID]: this.appId, } } } diff --git a/packages/server/src/utilities/workerRequests.js b/packages/server/src/utilities/workerRequests.js index cb06b5b8d4..4a8d10ecb8 100644 --- a/packages/server/src/utilities/workerRequests.js +++ b/packages/server/src/utilities/workerRequests.js @@ -3,13 +3,14 @@ const env = require("../environment") const { checkSlashesInUrl } = require("./index") const { getDeployedAppID } = require("@budibase/auth/db") const { updateAppRole, getGlobalUser } = require("./global") +const { Headers } = require("@budibase/auth/constants") function request(ctx, request, noApiKey) { if (!request.headers) { request.headers = {} } if (!noApiKey) { - request.headers["x-budibase-api-key"] = env.INTERNAL_API_KEY + request.headers[Headers.API_KEY] = env.INTERNAL_API_KEY } if (request.body && Object.keys(request.body).length > 0) { request.headers["Content-Type"] = "application/json"