merge latest master

Author: andz-bb

packages/backend-core/src/middleware/passport/sso/sso.ts

@@ -69,9 +69,24 @@ export async function authenticate(
   }
 
   let pendingInvite: InviteWithCode | undefined
+  let blockedInvite: InviteWithCode | undefined
   if (!dbUser) {
     const invites = await cache.invite.getExistingInvites([details.email])
-    pendingInvite = invites[0]
+    if (details.emailVerified) {
+      pendingInvite = invites[0]
+    } else if (allowUnverifiedEmailLinking) {
+      pendingInvite = invites.find(invite => !invite.info.admin?.global)
+    }
+    if (!pendingInvite) {
+      blockedInvite = invites[0]
+    }
+  }
+
+  if (blockedInvite) {
+    return authError(
+      done,
+      "Email verification is required to accept this invite."
+    )
   }
 
   const emailLookupWasSkipped =

packages/backend-core/src/middleware/passport/sso/tests/sso.spec.ts

@@ -51,7 +51,7 @@ const getErrorMessage = () => {
 describe("sso", () => {
   describe("authenticate", () => {
     beforeEach(() => {
-      jest.clearAllMocks()
+      jest.resetAllMocks()
       testEnv.singleTenant()
       nock.cleanAll()
       mockInvite.getExistingInvites.mockResolvedValue([])
@@ -466,15 +466,25 @@ describe("sso", () => {
         mockInvite.deleteCode.mockResolvedValueOnce(undefined)
       })
 
-      it("reconciles the invite without requiring a verified email, deletes it, and fires the accepted event", async () => {
+      it("rejects the login rather than creating an account when the email is unverified", async () => {
+        await sso.authenticate(details, false, mockDone, mockSaveUser)
+
+        expect(mockSaveUser).not.toHaveBeenCalled()
+        expect(mockInvite.deleteCode).not.toHaveBeenCalled()
+        expect(events.user.inviteAccepted).not.toHaveBeenCalled()
+        expect(mockDone.mock.calls.length).toBe(1)
+        expect(getErrorMessage()).toContain(
+          "Email verification is required to accept this invite."
+        )
+      })
+
+      it("reconciles the invite when the email is verified, deletes it, and fires the accepted event", async () => {
+        details.emailVerified = true
         const ssoUser = structures.users.ssoUser({ details })
         mockSaveUser.mockReturnValueOnce(ssoUser)
 
         await sso.authenticate(details, false, mockDone, mockSaveUser)
 
-        // the invite is matched purely on email - no account-linking lookup happens
-        expect(users.getGlobalUserByEmail).not.toHaveBeenCalled()
-
         expect(mockSaveUser).toHaveBeenCalledWith(
           expect.objectContaining({
             _id: "us_" + details.userId,
@@ -493,16 +503,61 @@ describe("sso", () => {
         expect(mockDone).toHaveBeenCalledWith(null, ssoUser)
       })
 
-      it("reconciles the invite even when a local account would otherwise be required", async () => {
+      it("reconciles a non-admin invite when unverified email linking is explicitly allowed", async () => {
         const ssoUser = structures.users.ssoUser({ details })
         mockSaveUser.mockReturnValueOnce(ssoUser)
 
-        await sso.authenticate(details, true, mockDone, mockSaveUser)
+        await sso.authenticate(details, true, mockDone, mockSaveUser, true)
+
+        expect(mockDone).toHaveBeenCalledWith(null, ssoUser)
+      })
+
+      it("reconciles an eligible invite when an admin invite for the same email appears first", async () => {
+        const adminInvite: InviteWithCode = {
+          code: structures.uuid(),
+          email: details.email!,
+          info: {
+            tenantId: context.getTenantId(),
+            admin: { global: true },
+          },
+        }
+        mockInvite.getExistingInvites.mockReset()
+        mockInvite.getExistingInvites.mockResolvedValueOnce([
+          adminInvite,
+          invite,
+        ])
+        const ssoUser = structures.users.ssoUser({ details })
+        mockSaveUser.mockReturnValueOnce(ssoUser)
+
+        await sso.authenticate(details, true, mockDone, mockSaveUser, true)
 
+        expect(mockInvite.getCode).toHaveBeenCalledWith(
+          invite.code,
+          invite.info.tenantId
+        )
+        expect(mockInvite.deleteCode).toHaveBeenCalledWith(
+          invite.code,
+          invite.info.tenantId
+        )
         expect(mockDone).toHaveBeenCalledWith(null, ssoUser)
       })
 
+      it("rejects an unverified login for an admin invite even when unverified email linking is allowed", async () => {
+        invite.info.admin = { global: true }
+
+        await sso.authenticate(details, false, mockDone, mockSaveUser, true)
+
+        expect(mockSaveUser).not.toHaveBeenCalled()
+        expect(mockInvite.deleteCode).not.toHaveBeenCalled()
+        expect(events.user.inviteAccepted).not.toHaveBeenCalled()
+        expect(mockDone.mock.calls.length).toBe(1)
+        expect(getErrorMessage()).toContain(
+          "Email verification is required to accept this invite."
+        )
+      })
+
       it("reuses the account when the same identity's own concurrent login already claimed the invite", async () => {
+        details.emailVerified = true
         // simulates a second, racing request for this exact identity
         // (e.g. a double-submitted login) losing the lock race: the
         // winner already consumed the invite and saved the account for
@@ -524,8 +579,6 @@ describe("sso", () => {
 
         await sso.authenticate(details, false, mockDone, mockSaveUser)
 
-        // reused via the deterministic id, never by email match
-        expect(users.getGlobalUserByEmail).not.toHaveBeenCalled()
         // the winner's account is reused - no second document is created
         expect(mockSaveUser).toHaveBeenCalledWith(
           expect.objectContaining({ _id: existingUser._id }),
@@ -537,6 +590,7 @@ describe("sso", () => {
       })
 
       it("fails closed when the invite can no longer be validated and no concurrent claim for this identity can be confirmed", async () => {
+        details.emailVerified = true
         // covers expired/revoked invites and failed reads alike - none of
         // these are a positively identified concurrent claim, so this must
         // not fall back to linking by email or creating a fresh account
@@ -548,7 +602,6 @@ describe("sso", () => {
 
         await sso.authenticate(details, false, mockDone, mockSaveUser)
 
-        expect(users.getGlobalUserByEmail).not.toHaveBeenCalled()
         expect(mockSaveUser).not.toHaveBeenCalled()
         expect(mockInvite.deleteCode).not.toHaveBeenCalled()
         expect(events.user.inviteAccepted).not.toHaveBeenCalled()

packages/backend-core/src/sql/sqlTable.ts

@@ -16,6 +16,18 @@ import { helpers, utils } from "@budibase/shared-core"
 import SchemaBuilder = Knex.SchemaBuilder
 import CreateTableBuilder = Knex.CreateTableBuilder
 
+const quoteMySqlIdentifier = (identifier: string) => {
+  return `\`${identifier.replace(/`/g, "``")}\``
+}
+
+const quoteSqlServerIdentifier = (identifier: string) => {
+  return `[${identifier.replace(/]/g, "]]")}]`
+}
+
+const quoteSqlServerUnicodeString = (value: string) => {
+  return `N'${value.replace(/'/g, "''")}'`
+}
+
 function isIgnoredType(type: FieldType) {
   const ignored = [FieldType.LINK, FieldType.FORMULA, FieldType.AI]
   return ignored.indexOf(type) !== -1
@@ -272,10 +284,14 @@ class SqlTableQueryBuilder {
         if (this.sqlClient === SqlClient.MY_SQL && json.meta?.renamed) {
           const updatedColumn = json.meta.renamed.updated
           const tableName = json?.schema
-            ? `\`${json.schema}\`.\`${json.table.name}\``
-            : `\`${json.table.name}\``
+            ? `${quoteMySqlIdentifier(json.schema)}.${quoteMySqlIdentifier(
+                json.table.name
+              )}`
+            : quoteMySqlIdentifier(json.table.name)
           return {
-            sql: `alter table ${tableName} rename column \`${json.meta.renamed.old}\` to \`${updatedColumn}\`;`,
+            sql: `alter table ${tableName} rename column ${quoteMySqlIdentifier(
+              json.meta.renamed.old
+            )} to ${quoteMySqlIdentifier(updatedColumn)};`,
             bindings: [],
           }
         }
@@ -293,14 +309,22 @@ class SqlTableQueryBuilder {
         if (this.sqlClient === SqlClient.MS_SQL && json.meta?.renamed) {
           const oldColumn = json.meta.renamed.old
           const updatedColumn = json.meta.renamed.updated
-          const tableName = json?.schema
-            ? `${json.schema}.${json.table.name}`
-            : `${json.table.name}`
+          const oldColumnName = [
+            ...(json.schema ? [json.schema] : []),
+            json.table.name,
+            oldColumn,
+          ]
+            .map(quoteSqlServerIdentifier)
+            .join(".")
           const sql = getNativeSql(query)
           if (Array.isArray(sql)) {
             for (const query of sql) {
               if (query.sql.startsWith("exec sp_rename")) {
-                query.sql = `exec sp_rename '${tableName}.${oldColumn}', '${updatedColumn}', 'COLUMN'`
+                query.sql = `exec sp_rename ${quoteSqlServerUnicodeString(
+                  oldColumnName
+                )}, ${quoteSqlServerUnicodeString(
+                  updatedColumn
+                )}, ${quoteSqlServerUnicodeString("COLUMN")}`
                 query.bindings = []
               }
             }

packages/backend-core/src/sql/tests/sqlTable.spec.ts

@@ -0,0 +1,95 @@
+import SqlTable from "../sqlTable"
+import {
+  FieldType,
+  Operation,
+  SqlClient,
+  TableSourceType,
+} from "@budibase/types"
+import type { EnrichedQueryJson, SqlQuery, Table } from "@budibase/types"
+
+const buildTable = ({
+  name,
+  column,
+}: {
+  name: string
+  column: string
+}): Table => ({
+  _id: "tbl",
+  type: "table",
+  name,
+  sourceId: "datasource",
+  sourceType: TableSourceType.EXTERNAL,
+  schema: {
+    [column]: {
+      name: column,
+      type: FieldType.STRING,
+    },
+  },
+})
+
+const buildRenameQuery = ({
+  tableName,
+  oldColumn,
+  updatedColumn,
+  schema,
+}: {
+  tableName: string
+  oldColumn: string
+  updatedColumn: string
+  schema?: string
+}): EnrichedQueryJson => {
+  const oldTable = buildTable({ name: tableName, column: oldColumn })
+  const table = buildTable({ name: tableName, column: updatedColumn })
+
+  return {
+    operation: Operation.UPDATE_TABLE,
+    table,
+    tables: { [tableName]: table },
+    schema,
+    meta: {
+      oldTable,
+      renamed: {
+        old: oldColumn,
+        updated: updatedColumn,
+      },
+    },
+  }
+}
+
+describe("SQL table identifier escaping", () => {
+  it("escapes MySQL rename identifiers", () => {
+    const query = buildRenameQuery({
+      schema: "schema`; DROP TABLE audit; --",
+      tableName: "table`; DROP TABLE users; --",
+      oldColumn: "old`; SELECT SLEEP(10); --",
+      updatedColumn: "new`name",
+    })
+
+    const result = new SqlTable(SqlClient.MY_SQL)._tableQuery(query) as SqlQuery
+
+    expect(result).toEqual({
+      sql: "alter table `schema``; DROP TABLE audit; --`.`table``; DROP TABLE users; --` rename column `old``; SELECT SLEEP(10); --` to `new``name`;",
+      bindings: [],
+    })
+  })
+
+  it("escapes SQL Server rename identifiers and string literals", () => {
+    const query = buildRenameQuery({
+      schema: "schema漢]",
+      tableName: "table字'",
+      oldColumn: "old名]'; DROP TABLE users; --",
+      updatedColumn: "new列'name",
+    })
+
+    const result = new SqlTable(SqlClient.MS_SQL)._tableQuery(query)
+    const queries = result as SqlQuery[]
+    const renameQuery = queries.find(query =>
+      query.sql.startsWith("exec sp_rename")
+    )
+
+    expect(renameQuery).toEqual({
+      sql: "exec sp_rename N'[schema漢]]].[table字''].[old名]]''; DROP TABLE users; --]', N'new列''name', N'COLUMN'",
+      bindings: [],
+    })
+  })
+})

packages/server/__mocks__/chat.ts

@@ -29,6 +29,8 @@ const mockWebhookState: Record<MockProvider, MockPostEphemeralResult> = {
   teams: defaultPostEphemeralResult(),
 }
 const mockChatOptions: ChatOptions[] = []
+const subscribedThreads = new Set<string>()
+let subscribeError: Error | undefined
 
 const toMessageText = (value: unknown) =>
   typeof value === "string" ? value : JSON.stringify(value)
@@ -165,6 +167,12 @@ export const resetMockChatState = () => {
   mockWebhookState.slack = defaultPostEphemeralResult()
   mockWebhookState.teams = defaultPostEphemeralResult()
   mockChatOptions.length = 0
+  subscribedThreads.clear()
+  subscribeError = undefined
+}
+
+export const setMockSubscribeError = (error?: Error) => {
+  subscribeError = error
 }
 
 export const setMockPostEphemeralResult = (
@@ -322,7 +330,12 @@ export class Chat {
           id: `slack:${channelId}:${threadTs}`,
           channelId,
           ...createMessageCollector("slack", messages),
-          subscribe: async () => {},
+          subscribe: async () => {
+            if (subscribeError) {
+              throw subscribeError
+            }
+            subscribedThreads.add(`slack:${channelId}:${threadTs}`)
+          },
           channel,
         }
         const isMention = isSlackMentionMessage(event)
@@ -342,7 +355,10 @@ export class Chat {
         } else {
           if (isMention) {
             await invokeHandlers(this.mentionHandlers, thread, message)
-          } else if (event.thread_ts) {
+          } else if (
+            event.thread_ts &&
+            subscribedThreads.has(`slack:${channelId}:${threadTs}`)
+          ) {
             await invokeHandlers(this.subscribedHandlers, thread, message)
           }
           await invokeHandlers(this.newMessageHandlers, thread, message)

packages/server/src/api/controllers/static/index.ts

@@ -82,17 +82,30 @@ const MAX_PWA_ZIP_FILE_COUNT = 100
 const MAX_PWA_ZIP_ENTRY_SIZE = 10 * 1024 * 1024 // 10MB per file
 const MAX_PWA_ZIP_TOTAL_SIZE = 50 * 1024 * 1024 // 50MB uncompressed total
 const MAX_PWA_ZIP_DEPTH = 10
+// used in attribute checks for security remediation, see
+// https://github.com/Budibase/budibase/pull/19564
+const ZIP_FILE_TYPE_MASK = 0o170000
+const ZIP_SYMLINK_FILE_TYPE = 0o120000
 
 const validatePWAZipEntries = () => {
   let fileCount = 0
   let totalUncompressedSize = 0
 
-  return (entry: { fileName: string; uncompressedSize: number }) => {
+  return (entry: {
+    fileName: string
+    uncompressedSize: number
+    externalFileAttributes: number
+  }) => {
     // extract-zip skips these itself, so don't count them against the limits.
     if (entry.fileName.startsWith("__MACOSX/")) {
       return
     }
 
+    const fileType = (entry.externalFileAttributes >>> 16) & ZIP_FILE_TYPE_MASK
+    if (fileType === ZIP_SYMLINK_FILE_TYPE) {
+      throw new BadRequestError(`Invalid zip`)
+    }
+
     const depth =
       entry.fileName.split("/").filter(Boolean).length -
       (entry.fileName.endsWith("/") ? 0 : 1)