Merge pull request #4211 from Budibase/csrf

CSRF Tokens

README.md

@@ -201,9 +201,6 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
     <td align="center"><a href="https://github.com/seoulaja"><img src="https://avatars.githubusercontent.com/u/15101654?v=4?s=100" width="100px;" alt=""/><br /><sub><b>seoulaja</b></sub></a><br /><a href="#translation-seoulaja" title="Translation">🌍</a></td>
     <td align="center"><a href="https://github.com/mslourens"><img src="https://avatars.githubusercontent.com/u/1907152?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Maurits Lourens</b></sub></a><br /><a href="https://github.com/Budibase/budibase/commits?author=mslourens" title="Tests">⚠️</a> <a href="https://github.com/Budibase/budibase/commits?author=mslourens" title="Code">💻</a></td>
   </tr>
-  <tr>
-    <td align="center"><a href="https://github.com/Rory-Powell"><img src="https://avatars.githubusercontent.com/u/8755148?v=4?s=100" width="100px;" alt=""/><br /><sub><b>Rory Powell</b></sub></a><br /><a href="#infra-Rory-Powell" title="Infrastructure (Hosting, Build-Tools, etc)">🚇</a> <a href="https://github.com/Budibase/budibase/commits?author=Rory-Powell" title="Tests">⚠️</a> <a href="https://github.com/Budibase/budibase/commits?author=Rory-Powell" title="Code">💻</a></td>
-  </tr>
 </table>
 
 <!-- markdownlint-restore -->

packages/backend-core/src/auth.js

@@ -12,6 +12,7 @@ const {
   tenancy,
   appTenancy,
   authError,
+  csrf,
 } = require("./middleware")
 
 // Strategies
@@ -42,4 +43,5 @@ module.exports = {
   buildAppTenancyMiddleware: appTenancy,
   auditLog,
   authError,
+  buildCsrfMiddleware: csrf,
 }

packages/backend-core/src/constants.js

@@ -18,6 +18,7 @@ exports.Headers = {
   TYPE: "x-budibase-type",
   TENANT_ID: "x-budibase-tenant-id",
   TOKEN: "x-budibase-token",
+  CSRF_TOKEN: "x-csrf-token",
 }
 
 exports.GlobalRoles = {

packages/backend-core/src/middleware/authenticated.js

@@ -60,6 +60,7 @@ module.exports = (
             } else {
               user = await getUser(userId, session.tenantId)
             }
+            user.csrfToken = session.csrfToken
             delete user.password
             authenticated = true
           } catch (err) {

packages/backend-core/src/middleware/csrf.js

@@ -0,0 +1,78 @@
+const { Headers } = require("../constants")
+const { buildMatcherRegex, matches } = require("./matchers")
+
+/**
+ * GET, HEAD and OPTIONS methods are considered safe operations
+ *
+ * POST, PUT, PATCH, and DELETE methods, being state changing verbs,
+ * should have a CSRF token attached to the request
+ */
+const EXCLUDED_METHODS = ["GET", "HEAD", "OPTIONS"]
+
+/**
+ * There are only three content type values that can be used in cross domain requests.
+ * If any other value is used, e.g. application/json, the browser will first make a OPTIONS
+ * request which will be protected by CORS.
+ */
+const INCLUDED_CONTENT_TYPES = [
+  "application/x-www-form-urlencoded",
+  "multipart/form-data",
+  "text/plain",
+]
+
+/**
+ * Validate the CSRF token generated aganst the user session.
+ * Compare the token with the x-csrf-token header.
+ *
+ * If the token is not found within the request or the value provided
+ * does not match the value within the user session, the request is rejected.
+ *
+ * CSRF protection provided using the 'Synchronizer Token Pattern'
+ * https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#synchronizer-token-pattern
+ *
+ */
+module.exports = (opts = { noCsrfPatterns: [] }) => {
+  const noCsrfOptions = buildMatcherRegex(opts.noCsrfPatterns)
+  return async (ctx, next) => {
+    // don't apply for excluded paths
+    const found = matches(ctx, noCsrfOptions)
+    if (found) {
+      return next()
+    }
+
+    // don't apply for the excluded http methods
+    if (EXCLUDED_METHODS.indexOf(ctx.method) !== -1) {
+      return next()
+    }
+
+    // don't apply when the content type isn't supported
+    let contentType = ctx.get("content-type")
+      ? ctx.get("content-type").toLowerCase()
+      : ""
+    if (
+      !INCLUDED_CONTENT_TYPES.filter(type => contentType.includes(type)).length
+    ) {
+      return next()
+    }
+
+    // don't apply csrf when the internal api key has been used
+    if (ctx.internal) {
+      return next()
+    }
+
+    // apply csrf when there is a token in the session (new logins)
+    // in future there should be a hard requirement that the token is present
+    const userToken = ctx.user.csrfToken
+    if (!userToken) {
+      return next()
+    }
+
+    // reject if no token in request or mismatch
+    const requestToken = ctx.get(Headers.CSRF_TOKEN)
+    if (!requestToken || requestToken !== userToken) {
+      ctx.throw(403, "Invalid CSRF token")
+    }
+
+    return next()
+  }
+}

packages/backend-core/src/middleware/index.js

@@ -8,6 +8,7 @@ const auditLog = require("./auditLog")
 const tenancy = require("./tenancy")
 const appTenancy = require("./appTenancy")
 const datasourceGoogle = require("./passport/datasource/google")
+const csrf = require("./csrf")
 
 module.exports = {
   google,
@@ -22,4 +23,5 @@ module.exports = {
   datasource: {
     google: datasourceGoogle,
   },
+  csrf,
 }

packages/backend-core/src/security/sessions.js

@@ -1,4 +1,5 @@
 const redis = require("../redis/authRedis")
+const { v4: uuidv4 } = require("uuid")
 
 // a week in seconds
 const EXPIRY_SECONDS = 86400 * 7
@@ -16,6 +17,9 @@ function makeSessionID(userId, sessionId) {
 exports.createASession = async (userId, session) => {
   const client = await redis.getSessionClient()
   const sessionId = session.sessionId
+  if (!session.csrfToken) {
+    session.csrfToken = uuidv4()
+  }
   session = {
     createdAt: new Date().toISOString(),
     lastAccessedAt: new Date().toISOString(),

packages/builder/src/builderStore/api.js

@@ -1,12 +1,20 @@
 import { store } from "./index"
 import { get as svelteGet } from "svelte/store"
 import { removeCookie, Cookies } from "./cookies"
+import { auth } from "stores/portal"
 
 const apiCall =
   method =>
   async (url, body, headers = { "Content-Type": "application/json" }) => {
     headers["x-budibase-app-id"] = svelteGet(store).appId
     headers["x-budibase-api-version"] = "1"
+
+    // add csrf token if authenticated
+    const user = svelteGet(auth).user
+    if (user && user.csrfToken) {
+      headers["x-csrf-token"] = user.csrfToken
+    }
+
     const json = headers["Content-Type"] === "application/json"
     const resp = await fetch(url, {
       method: method,

packages/builder/src/pages/builder/auth/reset.svelte

@@ -31,6 +31,7 @@
   }
 
   onMount(async () => {
+    await auth.checkAuth()
     await organisation.init()
   })
 </script>

packages/client/src/api/api.js

@@ -1,4 +1,5 @@
-import { notificationStore } from "stores"
+import { notificationStore, authStore } from "stores"
+import { get } from "svelte/store"
 import { ApiVersion } from "constants"
 
 /**
@@ -28,6 +29,13 @@ const makeApiCall = async ({ method, url, body, json = true }) => {
       ...(json && { "Content-Type": "application/json" }),
       ...(!inBuilder && { "x-budibase-type": "client" }),
     }
+
+    // add csrf token if authenticated
+    const auth = get(authStore)
+    if (auth && auth.csrfToken) {
+      headers["x-csrf-token"] = auth.csrfToken
+    }
+
     const response = await fetch(url, {
       method,
       headers,

packages/server/src/api/controllers/auth.js

@@ -16,6 +16,8 @@ exports.fetchSelf = async ctx => {
   const user = await getFullUser(ctx, userId)
   // this shouldn't be returned by the app self
   delete user.roles
+  // forward the csrf token from the session
+  user.csrfToken = ctx.user.csrfToken
 
   if (appId) {
     const db = new CouchDB(appId)
@@ -24,6 +26,8 @@ exports.fetchSelf = async ctx => {
     try {
       const userTable = await db.get(InternalTables.USER_METADATA)
       const metadata = await db.get(userId)
+      // make sure there is never a stale csrf token
+      delete metadata.csrfToken
       // specifically needs to make sure is enriched
       ctx.body = await outputProcessing(ctx, userTable, {
         ...user,

packages/server/src/api/controllers/user.js

@@ -167,6 +167,8 @@ exports.updateSelfMetadata = async function (ctx) {
   ctx.request.body._id = ctx.user._id
   // make sure no stale rev
   delete ctx.request.body._rev
+  // make sure no csrf token
+  delete ctx.request.body.csrfToken
   await exports.updateMetadata(ctx)
 }
 

packages/server/src/api/routes/tests/auth.spec.js

@@ -13,10 +13,9 @@ describe("/authenticate", () => {
 
   describe("fetch self", () => {
     it("should be able to fetch self", async () => {
-      const headers = await config.login()
       const res = await request
         .get(`/api/self`)
-        .set(headers)
+        .set(config.defaultHeaders())
         .expect("Content-Type", /json/)
         .expect(200)
       expect(res.body._id).toEqual(generateUserMetadataID("us_uuid1"))

packages/server/src/middleware/authorized.js

@@ -9,11 +9,59 @@ const {
 } = require("@budibase/backend-core/permissions")
 const builderMiddleware = require("./builder")
 const { isWebhookEndpoint } = require("./utils")
+const { buildCsrfMiddleware } = require("@budibase/backend-core/auth")
 
 function hasResource(ctx) {
   return ctx.resourceId != null
 }
 
+const csrf = buildCsrfMiddleware()
+
+/**
+ * Apply authorization to the requested resource:
+ * - If this is a builder resource the user must be a builder.
+ * - Builders can access all resources.
+ * - Otherwise the user must have the required role.
+ */
+const checkAuthorized = async (ctx, resourceRoles, permType, permLevel) => {
+  // check if this is a builder api and the user is not a builder
+  const isBuilder = ctx.user && ctx.user.builder && ctx.user.builder.global
+  const isBuilderApi = permType === PermissionTypes.BUILDER
+  if (isBuilderApi && !isBuilder) {
+    return ctx.throw(403, "Not Authorized")
+  }
+
+  // check for resource authorization
+  if (!isBuilder) {
+    await checkAuthorizedResource(ctx, resourceRoles, permType, permLevel)
+  }
+}
+
+const checkAuthorizedResource = async (
+  ctx,
+  resourceRoles,
+  permType,
+  permLevel
+) => {
+  // get the user's roles
+  const roleId = ctx.roleId || BUILTIN_ROLE_IDS.PUBLIC
+  const userRoles = await getUserRoleHierarchy(ctx.appId, roleId, {
+    idOnly: false,
+  })
+  const permError = "User does not have permission"
+  // check if the user has the required role
+  if (resourceRoles.length > 0) {
+    // deny access if the user doesn't have the required resource role
+    const found = userRoles.find(role => resourceRoles.indexOf(role._id) !== -1)
+    if (!found) {
+      ctx.throw(403, permError)
+    }
+    // fallback to the base permissions when no resource roles are found
+  } else if (!doesHaveBasePermission(permType, permLevel, userRoles)) {
+    ctx.throw(403, permError)
+  }
+}
+
 module.exports =
   (permType, permLevel = null) =>
   async (ctx, next) => {
@@ -31,40 +79,26 @@ module.exports =
     // to find API endpoints which are builder focused
     await builderMiddleware(ctx, permType)
 
-    const isAuthed = ctx.isAuthenticated
+    // get the resource roles
-    // builders for now have permission to do anything
+    let resourceRoles = []
-    let isBuilder = ctx.user && ctx.user.builder && ctx.user.builder.global
+    if (ctx.appId && hasResource(ctx)) {
-    const isBuilderApi = permType === PermissionTypes.BUILDER
+      resourceRoles = await getRequiredResourceRole(ctx.appId, permLevel, ctx)
-    if (isBuilder) {
+    }
+
+    // if the resource is public, proceed
+    const isPublicResource = resourceRoles.includes(BUILTIN_ROLE_IDS.PUBLIC)
+    if (isPublicResource) {
       return next()
-    } else if (isBuilderApi && !isBuilder) {
-      return ctx.throw(403, "Not Authorized")
     }
 
-    // need to check this first, in-case public access, don't check authed until last
+    // check authenticated
-    const roleId = ctx.roleId || BUILTIN_ROLE_IDS.PUBLIC
+    if (!ctx.isAuthenticated) {
-    const hierarchy = await getUserRoleHierarchy(ctx.appId, roleId, {
+      return ctx.throw(403, "Session not authenticated")
-      idOnly: false,
-    })
-    const permError = "User does not have permission"
-    let possibleRoleIds = []
-    if (hasResource(ctx)) {
-      possibleRoleIds = await getRequiredResourceRole(ctx.appId, permLevel, ctx)
-    }
-    // check if we found a role, if not fallback to base permissions
-    if (possibleRoleIds.length > 0) {
-      const found = hierarchy.find(
-        role => possibleRoleIds.indexOf(role._id) !== -1
-      )
-      return found ? next() : ctx.throw(403, permError)
-    } else if (!doesHaveBasePermission(permType, permLevel, hierarchy)) {
-      ctx.throw(403, permError)
     }
 
-    // if they are not authed, then anything using the authorized middleware will fail
+    // check authorized
-    if (!isAuthed) {
+    await checkAuthorized(ctx, resourceRoles, permType, permLevel)
-      ctx.throw(403, "Session not authenticated")
-    }
 
-    return next()
+    // csrf protection
+    return csrf(ctx, next)
   }

packages/server/src/middleware/tests/authorized.spec.js

@@ -17,6 +17,7 @@ class TestConfiguration {
     this.middleware = authorizedMiddleware(role)
     this.next = jest.fn()
     this.throw = jest.fn()
+    this.headers = {}
     this.ctx = {
       headers: {},
       request: {
@@ -25,7 +26,8 @@ class TestConfiguration {
       appId: "",
       auth: {},
       next: this.next,
-      throw: this.throw
+      throw: this.throw,
+      get: (name) => this.headers[name],
     }
   }
 
@@ -46,7 +48,7 @@ class TestConfiguration {
   }
 
   setAuthenticated(isAuthed) {
-    this.ctx.auth = { authenticated: isAuthed }
+    this.ctx.isAuthenticated = isAuthed
   }
 
   setRequestUrl(url) {
@@ -107,7 +109,7 @@ describe("Authorization middleware", () => {
       expect(config.next).toHaveBeenCalled()
     })
     
-    it("throws if the user has only builder permissions", async () => {
+    it("throws if the user does not have builder permissions", async () => {
       config.setEnvironment(false)
       config.setMiddlewareRequiredPermission(PermissionTypes.BUILDER)
       config.setUser({
@@ -133,7 +135,7 @@ describe("Authorization middleware", () => {
       expect(config.next).toHaveBeenCalled()
     })
 
-    it("throws if the user session is not authenticated after permission checks", async () => {
+    it("throws if the user session is not authenticated", async () => {
       config.setUser({
         role: {
           _id: ""

packages/server/src/tests/utilities/TestConfiguration.js

@@ -27,6 +27,7 @@ core.init(CouchDB)
 
 const GLOBAL_USER_ID = "us_uuid1"
 const EMAIL = "babs@babs.com"
+const CSRF_TOKEN = "e3727778-7af0-4226-b5eb-f43cbe60a306"
 
 class TestConfiguration {
   constructor(openServer = true) {
@@ -86,7 +87,11 @@ class TestConfiguration {
       roles: roles || {},
       tenantId: TENANT_ID,
     }
-    await createASession(id, { sessionId: "sessionid", tenantId: TENANT_ID })
+    await createASession(id, {
+      sessionId: "sessionid",
+      tenantId: TENANT_ID,
+      csrfToken: CSRF_TOKEN,
+    })
     if (builder) {
       user.builder = { global: true }
     } else {
@@ -133,6 +138,7 @@ class TestConfiguration {
         `${Cookies.Auth}=${authToken}`,
         `${Cookies.CurrentApp}=${appToken}`,
       ],
+      [Headers.CSRF_TOKEN]: CSRF_TOKEN,
     }
     if (this.appId) {
       headers[Headers.APP_ID] = this.appId
@@ -426,10 +432,6 @@ class TestConfiguration {
         roles: { [this.prodAppId]: roleId },
       })
     }
-    await createASession(userId, {
-      sessionId: "sessionid",
-      tenantId: TENANT_ID,
-    })
     // have to fake this
     const auth = {
       userId,

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

@@ -172,6 +172,7 @@ exports.getSelf = async ctx => {
   ctx.body.account = ctx.user.account
   ctx.body.budibaseAccess = ctx.user.budibaseAccess
   ctx.body.accountPortalAccess = ctx.user.accountPortalAccess
+  ctx.body.csrfToken = ctx.user.csrfToken
 }
 
 exports.updateSelf = async ctx => {
@@ -190,6 +191,8 @@ exports.updateSelf = async ctx => {
   // don't allow sending up an ID/Rev, always use the existing one
   delete ctx.request.body._id
   delete ctx.request.body._rev
+  // don't allow setting the csrf token
+  delete ctx.request.body.csrfToken
   const response = await db.put({
     ...user,
     ...ctx.request.body,

packages/worker/src/api/index.js

@@ -6,6 +6,7 @@ const {
   buildAuthMiddleware,
   auditLog,
   buildTenancyMiddleware,
+  buildCsrfMiddleware,
 } = require("@budibase/backend-core/auth")
 
 const PUBLIC_ENDPOINTS = [
@@ -68,6 +69,10 @@ const NO_TENANCY_ENDPOINTS = [
   },
 ]
 
+// most public endpoints are gets, but some are posts
+// add them all to be safe
+const NO_CSRF_ENDPOINTS = [...PUBLIC_ENDPOINTS]
+
 const router = new Router()
 router
   .use(
@@ -85,6 +90,7 @@ router
   .use("/health", ctx => (ctx.status = 200))
   .use(buildAuthMiddleware(PUBLIC_ENDPOINTS))
   .use(buildTenancyMiddleware(PUBLIC_ENDPOINTS, NO_TENANCY_ENDPOINTS))
+  .use(buildCsrfMiddleware({ noCsrfPatterns: NO_CSRF_ENDPOINTS }))
   // for now no public access is allowed to worker (bar health check)
   .use((ctx, next) => {
     if (ctx.publicEndpoint) {

packages/worker/src/api/routes/tests/utilities/TestConfiguration.js

@@ -2,12 +2,12 @@ const env = require("../../../../environment")
 const controllers = require("./controllers")
 const supertest = require("supertest")
 const { jwt } = require("@budibase/backend-core/auth")
-const { Cookies } = require("@budibase/backend-core/constants")
+const { Cookies, Headers } = require("@budibase/backend-core/constants")
 const { Configs, LOGO_URL } = require("../../../../constants")
 const { getGlobalUserByEmail } = require("@budibase/backend-core/utils")
 const { createASession } = require("@budibase/backend-core/sessions")
 const { newid } = require("@budibase/backend-core/src/hashing")
-const { TENANT_ID } = require("./structures")
+const { TENANT_ID, CSRF_TOKEN } = require("./structures")
 const core = require("@budibase/backend-core")
 const CouchDB = require("../../../../db")
 const { doInTenant } = require("@budibase/backend-core/tenancy")
@@ -72,6 +72,7 @@ class TestConfiguration {
     await createASession("us_uuid1", {
       sessionId: "sessionid",
       tenantId: TENANT_ID,
+      csrfToken: CSRF_TOKEN,
     })
   }
 
@@ -98,6 +99,7 @@ class TestConfiguration {
     return {
       Accept: "application/json",
       ...this.cookieHeader([`${Cookies.Auth}=${authToken}`]),
+      [Headers.CSRF_TOKEN]: CSRF_TOKEN,
     }
   }
 

packages/worker/src/api/routes/tests/utilities/structures.js

@@ -1 +1,2 @@
 exports.TENANT_ID = "default"
+exports.CSRF_TOKEN = "e3727778-7af0-4226-b5eb-f43cbe60a306"