From ffcf5354eb5e28f577d98a3429cb009c46d47294 Mon Sep 17 00:00:00 2001 From: Rory Powell Date: Tue, 25 Jan 2022 22:54:50 +0000 Subject: [PATCH 1/3] Add CSRF Token --- packages/backend-core/src/auth.js | 2 + packages/backend-core/src/constants.js | 1 + .../src/middleware/authenticated.js | 1 + packages/backend-core/src/middleware/csrf.js | 56 +++++++++++ packages/backend-core/src/middleware/index.js | 2 + .../backend-core/src/security/sessions.js | 4 + packages/builder/src/builderStore/api.js | 8 ++ .../src/pages/builder/auth/reset.svelte | 1 + packages/client/src/api/api.js | 10 +- packages/server/src/api/controllers/auth.js | 4 + packages/server/src/api/controllers/user.js | 2 + .../server/src/api/routes/tests/auth.spec.js | 3 +- packages/server/src/middleware/authorized.js | 94 +++++++++++++------ .../src/middleware/tests/authorized.spec.js | 10 +- .../src/tests/utilities/TestConfiguration.js | 12 ++- .../src/api/controllers/global/users.js | 3 + packages/worker/src/api/index.js | 6 ++ .../tests/utilities/TestConfiguration.js | 6 +- .../api/routes/tests/utilities/structures.js | 1 + 19 files changed, 182 insertions(+), 44 deletions(-) create mode 100644 packages/backend-core/src/middleware/csrf.js diff --git a/packages/backend-core/src/auth.js b/packages/backend-core/src/auth.js index 7f66d887ae..41d2bb1cc5 100644 --- a/packages/backend-core/src/auth.js +++ b/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, } diff --git a/packages/backend-core/src/constants.js b/packages/backend-core/src/constants.js index 8e6b01608e..604acb3540 100644 --- a/packages/backend-core/src/constants.js +++ b/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 = { diff --git a/packages/backend-core/src/middleware/authenticated.js b/packages/backend-core/src/middleware/authenticated.js index 87bd4d35ce..4978f7b9dc 100644 --- a/packages/backend-core/src/middleware/authenticated.js +++ b/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) { diff --git a/packages/backend-core/src/middleware/csrf.js b/packages/backend-core/src/middleware/csrf.js new file mode 100644 index 0000000000..1286cd0934 --- /dev/null +++ b/packages/backend-core/src/middleware/csrf.js @@ -0,0 +1,56 @@ +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"] + +/** + * 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 csrf when the internal api key has been used + if (ctx.internal) { + return next() + } + + // reject if no token in session + const userToken = ctx.user.csrfToken + if (!userToken) { + ctx.throw(403, "No CSRF token found") + } + + // 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() + } +} diff --git a/packages/backend-core/src/middleware/index.js b/packages/backend-core/src/middleware/index.js index cf8676a2bc..f4231461af 100644 --- a/packages/backend-core/src/middleware/index.js +++ b/packages/backend-core/src/middleware/index.js @@ -7,6 +7,7 @@ const authenticated = require("./authenticated") const auditLog = require("./auditLog") const tenancy = require("./tenancy") const appTenancy = require("./appTenancy") +const csrf = require("./csrf") module.exports = { google, @@ -18,4 +19,5 @@ module.exports = { tenancy, appTenancy, authError, + csrf, } diff --git a/packages/backend-core/src/security/sessions.js b/packages/backend-core/src/security/sessions.js index ad21627bd9..bbe6be299d 100644 --- a/packages/backend-core/src/security/sessions.js +++ b/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(), diff --git a/packages/builder/src/builderStore/api.js b/packages/builder/src/builderStore/api.js index a5c6ceba54..a932799701 100644 --- a/packages/builder/src/builderStore/api.js +++ b/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, diff --git a/packages/builder/src/pages/builder/auth/reset.svelte b/packages/builder/src/pages/builder/auth/reset.svelte index e38a5d8b24..f78dd19eb9 100644 --- a/packages/builder/src/pages/builder/auth/reset.svelte +++ b/packages/builder/src/pages/builder/auth/reset.svelte @@ -31,6 +31,7 @@ } onMount(async () => { + await auth.checkAuth() await organisation.init() }) diff --git a/packages/client/src/api/api.js b/packages/client/src/api/api.js index 2476030eb0..55dff38c77 100644 --- a/packages/client/src/api/api.js +++ b/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, diff --git a/packages/server/src/api/controllers/auth.js b/packages/server/src/api/controllers/auth.js index 3f680225af..f1b665c069 100644 --- a/packages/server/src/api/controllers/auth.js +++ b/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, diff --git a/packages/server/src/api/controllers/user.js b/packages/server/src/api/controllers/user.js index d87afc4309..1bd8bd6a12 100644 --- a/packages/server/src/api/controllers/user.js +++ b/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) } diff --git a/packages/server/src/api/routes/tests/auth.spec.js b/packages/server/src/api/routes/tests/auth.spec.js index c50780a8d5..fa26eb83ac 100644 --- a/packages/server/src/api/routes/tests/auth.spec.js +++ b/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")) diff --git a/packages/server/src/middleware/authorized.js b/packages/server/src/middleware/authorized.js index b463895a80..7125ec3246 100644 --- a/packages/server/src/middleware/authorized.js +++ b/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 - // builders for now have permission to do anything - let isBuilder = ctx.user && ctx.user.builder && ctx.user.builder.global - const isBuilderApi = permType === PermissionTypes.BUILDER - if (isBuilder) { + // get the resource roles + let resourceRoles = [] + if (ctx.appId && hasResource(ctx)) { + resourceRoles = await getRequiredResourceRole(ctx.appId, permLevel, ctx) + } + + // 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 - const roleId = ctx.roleId || BUILTIN_ROLE_IDS.PUBLIC - const hierarchy = await getUserRoleHierarchy(ctx.appId, roleId, { - 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) + // check authenticated + if (!ctx.isAuthenticated) { + return ctx.throw(403, "Session not authenticated") } - // if they are not authed, then anything using the authorized middleware will fail - if (!isAuthed) { - ctx.throw(403, "Session not authenticated") - } + // check authorized + await checkAuthorized(ctx, resourceRoles, permType, permLevel) - return next() + // csrf protection + return csrf(ctx, next) } diff --git a/packages/server/src/middleware/tests/authorized.spec.js b/packages/server/src/middleware/tests/authorized.spec.js index 9775965b5a..04ef6e2b07 100644 --- a/packages/server/src/middleware/tests/authorized.spec.js +++ b/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: "" diff --git a/packages/server/src/tests/utilities/TestConfiguration.js b/packages/server/src/tests/utilities/TestConfiguration.js index 7aefe4fb78..6fd13fc229 100644 --- a/packages/server/src/tests/utilities/TestConfiguration.js +++ b/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, diff --git a/packages/worker/src/api/controllers/global/users.js b/packages/worker/src/api/controllers/global/users.js index 676c597b84..f2d89e103a 100644 --- a/packages/worker/src/api/controllers/global/users.js +++ b/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, diff --git a/packages/worker/src/api/index.js b/packages/worker/src/api/index.js index a83b39e6cf..607d8283f9 100644 --- a/packages/worker/src/api/index.js +++ b/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) { diff --git a/packages/worker/src/api/routes/tests/utilities/TestConfiguration.js b/packages/worker/src/api/routes/tests/utilities/TestConfiguration.js index 34ce01263d..6b6c0e24b3 100644 --- a/packages/worker/src/api/routes/tests/utilities/TestConfiguration.js +++ b/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, } } diff --git a/packages/worker/src/api/routes/tests/utilities/structures.js b/packages/worker/src/api/routes/tests/utilities/structures.js index 16701ac3d7..45f1f0077c 100644 --- a/packages/worker/src/api/routes/tests/utilities/structures.js +++ b/packages/worker/src/api/routes/tests/utilities/structures.js @@ -1 +1,2 @@ exports.TENANT_ID = "default" +exports.CSRF_TOKEN = "e3727778-7af0-4226-b5eb-f43cbe60a306" From 20ec58b77553777178070bdd31e89050ac90c27a Mon Sep 17 00:00:00 2001 From: Rory Powell Date: Wed, 26 Jan 2022 12:52:53 +0000 Subject: [PATCH 2/3] Don't apply csrf to existing sessions. Handle only supported content types --- packages/backend-core/src/middleware/csrf.js | 26 ++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/packages/backend-core/src/middleware/csrf.js b/packages/backend-core/src/middleware/csrf.js index 1286cd0934..12bd9473e6 100644 --- a/packages/backend-core/src/middleware/csrf.js +++ b/packages/backend-core/src/middleware/csrf.js @@ -9,6 +9,17 @@ const { buildMatcherRegex, matches } = require("./matchers") */ 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. @@ -34,15 +45,26 @@ module.exports = (opts = { noCsrfPatterns: [] }) => { 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() } - // reject if no token in session + // 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) { - ctx.throw(403, "No CSRF token found") + return next() } // reject if no token in request or mismatch From b73ef8d212d4619511107b8104f7adf4013324c1 Mon Sep 17 00:00:00 2001 From: Rory Powell Date: Wed, 26 Jan 2022 14:59:40 +0000 Subject: [PATCH 3/3] Update readme --- README.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/README.md b/README.md index 0f4cfe31c2..7d11ea570f 100644 --- a/README.md +++ b/README.md @@ -201,9 +201,6 @@ Thanks goes to these wonderful people ([emoji key](https://allcontributors.org/d
seoulaja
🌍
Maurits Lourens
⚠️ 💻 - -
Rory Powell
🚇 ⚠️ 💻 -