From 9f3127b4bf83976dd3ab1a50e5daa1dc2e6a5879 Mon Sep 17 00:00:00 2001 From: Aaron Kawalsky Date: Fri, 8 Dec 2023 20:24:41 -0500 Subject: [PATCH 1/7] allow passing regex in serverActions.allowedDomains --- .../src/server/app-render/action-handler.ts | 4 +- .../server/app-render/csrf-protection.test.ts | 39 +++++++++++++++++++ .../src/server/app-render/csrf-protection.ts | 11 ++++++ 3 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 packages/next/src/server/app-render/csrf-protection.test.ts create mode 100644 packages/next/src/server/app-render/csrf-protection.ts diff --git a/packages/next/src/server/app-render/action-handler.ts b/packages/next/src/server/app-render/action-handler.ts index c89751955d905..53513ec13a346 100644 --- a/packages/next/src/server/app-render/action-handler.ts +++ b/packages/next/src/server/app-render/action-handler.ts @@ -40,6 +40,7 @@ import { getIsServerAction, getServerActionRequestMetadata, } from '../lib/server-action-request-meta' +import { isCsrfOriginAllowed } from './csrf-protection' function formDataFromSearchQueryString(query: string) { const searchParams = new URLSearchParams(query) @@ -331,7 +332,8 @@ export async function handleAction({ // If the customer sets a list of allowed origins, we'll allow the request. // These are considered safe but might be different from forwarded host set // by the infra (i.e. reverse proxies). - if (serverActions?.allowedOrigins?.includes(originDomain)) { + // regex is used to support wildcard domains + if (isCsrfOriginAllowed(originDomain, serverActions?.allowedOrigins)) { // Ignore it } else { if (host) { diff --git a/packages/next/src/server/app-render/csrf-protection.test.ts b/packages/next/src/server/app-render/csrf-protection.test.ts new file mode 100644 index 0000000000000..b068b4028f936 --- /dev/null +++ b/packages/next/src/server/app-render/csrf-protection.test.ts @@ -0,0 +1,39 @@ +import { isCsrfOriginAllowed } from './csrf-protection' + +describe('isCsrfOriginAllowed', () => { + it('should return true when allowedOrigins contains originDomain', () => { + expect(isCsrfOriginAllowed('vercel.com', ['vercel.com'])).toBe(true) + expect(isCsrfOriginAllowed('www.vercel.com', ['www.vercel.com'])).toBe(true) + }) + + it('should return true when allowedOrigins contains originDomain with matching regex', () => { + expect(isCsrfOriginAllowed('asdf.vercel.com', ['(.*).vercel.com'])).toBe( + true + ) + expect( + isCsrfOriginAllowed('asdf.jkl.vercel.com', ['(.*).vercel.com']) + ).toBe(true) + }) + + it('should return false when allowedOrigins contains originDomain with non-matching regex', () => { + expect(isCsrfOriginAllowed('asdf.vercel.com', ['(.*).vercel.app'])).toBe( + false + ) + }) + + it('should return false when allowedOrigins does not contain originDomain', () => { + expect(isCsrfOriginAllowed('vercel.com', ['nextjs.org'])).toBe(false) + }) + + it('should return false when allowedOrigins is undefined', () => { + expect(isCsrfOriginAllowed('vercel.com', undefined)).toBe(false) + }) + + it('should return false when allowedOrigins is empty', () => { + expect(isCsrfOriginAllowed('vercel.com', [])).toBe(false) + }) + + it('should return false when allowedOrigins is empty string', () => { + expect(isCsrfOriginAllowed('vercel.com', [''])).toBe(false) + }) +}) diff --git a/packages/next/src/server/app-render/csrf-protection.ts b/packages/next/src/server/app-render/csrf-protection.ts new file mode 100644 index 0000000000000..c29f87ec2285d --- /dev/null +++ b/packages/next/src/server/app-render/csrf-protection.ts @@ -0,0 +1,11 @@ +export const isCsrfOriginAllowed = ( + originDomain: string, + allowedOrigins: string[] | undefined +): boolean => { + return ( + allowedOrigins?.some( + (allowedOrigin) => + allowedOrigin && originDomain.match(new RegExp(allowedOrigin)) + ) ?? false + ) +} From b6964cea2cc8afde6465883cea1540d12d79e792 Mon Sep 17 00:00:00 2001 From: Aaron Kawalsky Date: Mon, 11 Dec 2023 09:30:20 -0500 Subject: [PATCH 2/7] use micromatch instead of regex --- .../next/src/server/app-render/csrf-protection.test.ts | 10 +++------- packages/next/src/server/app-render/csrf-protection.ts | 5 ++++- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/next/src/server/app-render/csrf-protection.test.ts b/packages/next/src/server/app-render/csrf-protection.test.ts index b068b4028f936..84b2aebe94151 100644 --- a/packages/next/src/server/app-render/csrf-protection.test.ts +++ b/packages/next/src/server/app-render/csrf-protection.test.ts @@ -7,18 +7,14 @@ describe('isCsrfOriginAllowed', () => { }) it('should return true when allowedOrigins contains originDomain with matching regex', () => { - expect(isCsrfOriginAllowed('asdf.vercel.com', ['(.*).vercel.com'])).toBe( + expect(isCsrfOriginAllowed('asdf.vercel.com', ['*.vercel.com'])).toBe(true) + expect(isCsrfOriginAllowed('asdf.jkl.vercel.com', ['*.vercel.com'])).toBe( true ) - expect( - isCsrfOriginAllowed('asdf.jkl.vercel.com', ['(.*).vercel.com']) - ).toBe(true) }) it('should return false when allowedOrigins contains originDomain with non-matching regex', () => { - expect(isCsrfOriginAllowed('asdf.vercel.com', ['(.*).vercel.app'])).toBe( - false - ) + expect(isCsrfOriginAllowed('asdf.vercel.com', ['*.vercel.app'])).toBe(false) }) it('should return false when allowedOrigins does not contain originDomain', () => { diff --git a/packages/next/src/server/app-render/csrf-protection.ts b/packages/next/src/server/app-render/csrf-protection.ts index c29f87ec2285d..522ab7d19e656 100644 --- a/packages/next/src/server/app-render/csrf-protection.ts +++ b/packages/next/src/server/app-render/csrf-protection.ts @@ -1,3 +1,5 @@ +import { isMatch } from 'next/dist/compiled/micromatch' + export const isCsrfOriginAllowed = ( originDomain: string, allowedOrigins: string[] | undefined @@ -5,7 +7,8 @@ export const isCsrfOriginAllowed = ( return ( allowedOrigins?.some( (allowedOrigin) => - allowedOrigin && originDomain.match(new RegExp(allowedOrigin)) + allowedOrigin && + (allowedOrigin === originDomain || isMatch(originDomain, allowedOrigin)) ) ?? false ) } From 5b4978ccb520aca5e47ebe9ff7ba7b656531122b Mon Sep 17 00:00:00 2001 From: Aaron Kawalsky Date: Mon, 11 Dec 2023 10:48:41 -0500 Subject: [PATCH 3/7] add to docs --- .../02-data-fetching/02-server-actions-and-mutations.mdx | 2 +- .../02-app/02-api-reference/05-next-config-js/serverActions.mdx | 2 +- packages/next/src/server/config-shared.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/02-app/01-building-your-application/02-data-fetching/02-server-actions-and-mutations.mdx b/docs/02-app/01-building-your-application/02-data-fetching/02-server-actions-and-mutations.mdx index 08d5d22ba1f5c..cff421c36516c 100644 --- a/docs/02-app/01-building-your-application/02-data-fetching/02-server-actions-and-mutations.mdx +++ b/docs/02-app/01-building-your-application/02-data-fetching/02-server-actions-and-mutations.mdx @@ -893,7 +893,7 @@ For large applications that use reverse proxies or multi-layered backend archite module.exports = { experimental: { serverActions: { - allowedOrigins: ['my-proxy.com'], + allowedOrigins: ['my-proxy.com', '*.my-proxy.com'], }, }, } diff --git a/docs/02-app/02-api-reference/05-next-config-js/serverActions.mdx b/docs/02-app/02-api-reference/05-next-config-js/serverActions.mdx index 2896a41c86d9d..6d1a594816ef1 100644 --- a/docs/02-app/02-api-reference/05-next-config-js/serverActions.mdx +++ b/docs/02-app/02-api-reference/05-next-config-js/serverActions.mdx @@ -15,7 +15,7 @@ A list of extra safe origin domains from which Server Actions can be invoked. Ne module.exports = { experimental: { serverActions: { - allowedOrigins: ['my-proxy.com'], + allowedOrigins: ['my-proxy.com', '*.my-proxy.com'], }, }, } diff --git a/packages/next/src/server/config-shared.ts b/packages/next/src/server/config-shared.ts index 7ba9a6ad3bb7c..748311ac03804 100644 --- a/packages/next/src/server/config-shared.ts +++ b/packages/next/src/server/config-shared.ts @@ -313,7 +313,7 @@ export interface ExperimentalConfig { * Allowed origins that can bypass Server Action's CSRF check. This is helpful * when you have reverse proxy in front of your app. * @example - * ["my-app.com"] + * ["my-app.com", "*.my-app.com"] */ allowedOrigins?: string[] } From cdffa1b69c5c3fe341158ff71e075db31cb51a06 Mon Sep 17 00:00:00 2001 From: Aaron Kawalsky Date: Mon, 11 Dec 2023 11:20:51 -0500 Subject: [PATCH 4/7] remove errant comment --- packages/next/src/server/app-render/action-handler.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/next/src/server/app-render/action-handler.ts b/packages/next/src/server/app-render/action-handler.ts index 53513ec13a346..eafba6c75866d 100644 --- a/packages/next/src/server/app-render/action-handler.ts +++ b/packages/next/src/server/app-render/action-handler.ts @@ -332,7 +332,6 @@ export async function handleAction({ // If the customer sets a list of allowed origins, we'll allow the request. // These are considered safe but might be different from forwarded host set // by the infra (i.e. reverse proxies). - // regex is used to support wildcard domains if (isCsrfOriginAllowed(originDomain, serverActions?.allowedOrigins)) { // Ignore it } else { From eb3ff9dff6b184f859d6da75c2288c7ee55dbab7 Mon Sep 17 00:00:00 2001 From: Aaron Kawalsky Date: Mon, 11 Dec 2023 11:30:15 -0500 Subject: [PATCH 5/7] default allowedOrigins to empty array --- .../next/src/server/app-render/csrf-protection.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/next/src/server/app-render/csrf-protection.ts b/packages/next/src/server/app-render/csrf-protection.ts index 522ab7d19e656..8d48774e8f1bb 100644 --- a/packages/next/src/server/app-render/csrf-protection.ts +++ b/packages/next/src/server/app-render/csrf-protection.ts @@ -2,13 +2,11 @@ import { isMatch } from 'next/dist/compiled/micromatch' export const isCsrfOriginAllowed = ( originDomain: string, - allowedOrigins: string[] | undefined + allowedOrigins: string[] = [] ): boolean => { - return ( - allowedOrigins?.some( - (allowedOrigin) => - allowedOrigin && - (allowedOrigin === originDomain || isMatch(originDomain, allowedOrigin)) - ) ?? false + return allowedOrigins.some( + (allowedOrigin) => + allowedOrigin && + (allowedOrigin === originDomain || isMatch(originDomain, allowedOrigin)) ) } From ee662fa0eacd03b35b778dbe405506b3a40f1ce9 Mon Sep 17 00:00:00 2001 From: Aaron Kawalsky Date: Mon, 11 Dec 2023 15:37:42 -0500 Subject: [PATCH 6/7] remove micromatch as a dependency for csrf protection --- .../server/app-render/csrf-protection.test.ts | 13 +++++-- .../src/server/app-render/csrf-protection.ts | 37 ++++++++++++++++++- 2 files changed, 45 insertions(+), 5 deletions(-) diff --git a/packages/next/src/server/app-render/csrf-protection.test.ts b/packages/next/src/server/app-render/csrf-protection.test.ts index 84b2aebe94151..f7c515395db39 100644 --- a/packages/next/src/server/app-render/csrf-protection.test.ts +++ b/packages/next/src/server/app-render/csrf-protection.test.ts @@ -6,15 +6,22 @@ describe('isCsrfOriginAllowed', () => { expect(isCsrfOriginAllowed('www.vercel.com', ['www.vercel.com'])).toBe(true) }) - it('should return true when allowedOrigins contains originDomain with matching regex', () => { + it('should return true when allowedOrigins contains originDomain with matching pattern', () => { expect(isCsrfOriginAllowed('asdf.vercel.com', ['*.vercel.com'])).toBe(true) - expect(isCsrfOriginAllowed('asdf.jkl.vercel.com', ['*.vercel.com'])).toBe( + expect(isCsrfOriginAllowed('asdf.vercel.com', ['**.vercel.com'])).toBe(true) + expect(isCsrfOriginAllowed('asdf.jkl.vercel.com', ['**.vercel.com'])).toBe( true ) }) - it('should return false when allowedOrigins contains originDomain with non-matching regex', () => { + it('should return false when allowedOrigins contains originDomain with non-matching pattern', () => { expect(isCsrfOriginAllowed('asdf.vercel.com', ['*.vercel.app'])).toBe(false) + expect(isCsrfOriginAllowed('asdf.jkl.vercel.com', ['*.vercel.com'])).toBe( + false + ) + expect(isCsrfOriginAllowed('asdf.jkl.vercel.app', ['**.vercel.com'])).toBe( + false + ) }) it('should return false when allowedOrigins does not contain originDomain', () => { diff --git a/packages/next/src/server/app-render/csrf-protection.ts b/packages/next/src/server/app-render/csrf-protection.ts index 8d48774e8f1bb..8b842312c5f21 100644 --- a/packages/next/src/server/app-render/csrf-protection.ts +++ b/packages/next/src/server/app-render/csrf-protection.ts @@ -1,4 +1,36 @@ -import { isMatch } from 'next/dist/compiled/micromatch' +// micromatch is only available at node runtime, so it cannot be used here since the code path that calls this function +// can be run from edge. This is a simple implementation that safely achieves the required functionality. +// the goal is to match the functionality for remotePatterns as defined here - +// https://nextjs.org/docs/app/api-reference/components/image#remotepatterns +// TODO - retrofit micromatch to work in edge and use that instead +function matchWildcardDomain(domain: string, pattern: string) { + const domainParts = domain.split('.') + const patternParts = pattern.split('.') + + // Iterate through each part and compare them + for (let i = 0; i < patternParts.length; i++) { + if (patternParts[i] === '**') { + // If '**' is encountered, ensure remaining domain ends with remaining pattern + // e.g. **.y.z and a.b.c.y.z should match (b.c.y.z ends with y.z) + const remainingPattern = patternParts.slice(i + 1).join('.') + const remainingDomain = domainParts.slice(i + 1).join('.') + return remainingDomain.endsWith(remainingPattern) + } else if (patternParts[i] === '*') { + // If '*' is encountered, ensure remaining domain is equal to remaining pattern + // e.g. *.y.z and c.y.z should match (y.z is equal to y.z) + const remainingPattern = patternParts.slice(i + 1).join('.') + const remainingDomain = domainParts.slice(i + 1).join('.') + return remainingDomain === remainingPattern + } + + // If '*' is not encountered, compare the parts + if (patternParts[i] !== domainParts[i]) { + return false + } + } + + return true +} export const isCsrfOriginAllowed = ( originDomain: string, @@ -7,6 +39,7 @@ export const isCsrfOriginAllowed = ( return allowedOrigins.some( (allowedOrigin) => allowedOrigin && - (allowedOrigin === originDomain || isMatch(originDomain, allowedOrigin)) + (allowedOrigin === originDomain || + matchWildcardDomain(originDomain, allowedOrigin)) ) } From b9404a6c91c2559535393f67000da47a63baa412 Mon Sep 17 00:00:00 2001 From: Aaron Kawalsky Date: Mon, 11 Dec 2023 20:26:21 -0500 Subject: [PATCH 7/7] use suggestion for faster and more memory efficient solution --- .../src/server/app-render/csrf-protection.ts | 79 ++++++++++++++----- 1 file changed, 61 insertions(+), 18 deletions(-) diff --git a/packages/next/src/server/app-render/csrf-protection.ts b/packages/next/src/server/app-render/csrf-protection.ts index 8b842312c5f21..f78ecc5cf782d 100644 --- a/packages/next/src/server/app-render/csrf-protection.ts +++ b/packages/next/src/server/app-render/csrf-protection.ts @@ -7,29 +7,72 @@ function matchWildcardDomain(domain: string, pattern: string) { const domainParts = domain.split('.') const patternParts = pattern.split('.') - // Iterate through each part and compare them - for (let i = 0; i < patternParts.length; i++) { - if (patternParts[i] === '**') { - // If '**' is encountered, ensure remaining domain ends with remaining pattern - // e.g. **.y.z and a.b.c.y.z should match (b.c.y.z ends with y.z) - const remainingPattern = patternParts.slice(i + 1).join('.') - const remainingDomain = domainParts.slice(i + 1).join('.') - return remainingDomain.endsWith(remainingPattern) - } else if (patternParts[i] === '*') { - // If '*' is encountered, ensure remaining domain is equal to remaining pattern - // e.g. *.y.z and c.y.z should match (y.z is equal to y.z) - const remainingPattern = patternParts.slice(i + 1).join('.') - const remainingDomain = domainParts.slice(i + 1).join('.') - return remainingDomain === remainingPattern + if (patternParts.length < 1) { + // pattern is empty and therefore invalid to match against + return false + } + + if (domainParts.length < patternParts.length) { + // domain has too few segments and thus cannot match + return false + } + + let depth = 0 + while (patternParts.length && depth++ < 2) { + const patternPart = patternParts.pop() + const domainPart = domainParts.pop() + + switch (patternPart) { + case '': + case '*': + case '**': { + // invalid pattern. pattern segments must be non empty + // Additionally wildcards are only supported below the domain level + return false + } + default: { + if (domainPart !== patternPart) { + return false + } + } } + } + + while (patternParts.length) { + const patternPart = patternParts.pop() + const domainPart = domainParts.pop() - // If '*' is not encountered, compare the parts - if (patternParts[i] !== domainParts[i]) { - return false + switch (patternPart) { + case '': { + // invalid pattern. pattern segments must be non empty + return false + } + case '*': { + // wildcard matches anything so we continue if the domain part is non-empty + if (domainPart) { + continue + } else { + return false + } + } + case '**': { + // if this is not the last item in the pattern the pattern is invalid + if (patternParts.length > 0) { + return false + } + // recursive wildcard matches anything so we terminate here if the domain part is non empty + return domainPart !== undefined + } + default: { + if (domainPart !== patternPart) { + return false + } + } } } - return true + // We exhausted the pattern. If we also exhausted the domain we have a match + return domainParts.length === 0 } export const isCsrfOriginAllowed = (