Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow passing wildcard domains in serverActions.allowedDomains #59428

Merged
merged 9 commits into from
Dec 12, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
},
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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'],
},
},
}
Expand Down
4 changes: 3 additions & 1 deletion packages/next/src/server/app-render/action-handler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is left over from when it was using regex directly I believe

if (isCsrfOriginAllowed(originDomain, serverActions?.allowedOrigins)) {
// Ignore it
} else {
if (host) {
Expand Down
35 changes: 35 additions & 0 deletions packages/next/src/server/app-render/csrf-protection.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
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)
})
})
14 changes: 14 additions & 0 deletions packages/next/src/server/app-render/csrf-protection.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { isMatch } from 'next/dist/compiled/micromatch'

export const isCsrfOriginAllowed = (
originDomain: string,
allowedOrigins: string[] | undefined
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should default the config to always have an allowedOrigins value, it should just be an empty array if it was omitted by the user. then we can get rid of some of the existential operators

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that is too invasive a change for this PR then update the function to only accept an array and then move the existence check to the callsite

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existence check in this case is to account for TypeError: Expected pattern to be a non-empty string which appears to be thrown at runtime when the user passes a blank string as an element of allowedOrigins

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the array itself required would only allow for us to remove the optional chaining at the top level and the fallback to false, which I am also happy to do but wanted to point out above ^.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was suggesting is that wherever the next.config.js is processed and turned into the value passed as serverActions we do a normalization step. We make allowedOrigins an array if it doesn't exist. we error if you provided a value but it was the incorrect format. We error if you provide an empty string etc...

We could even go as far as constructing the minimatch regex there so all we have to do is call the match here.

I just took a look though and it seems this logic is pretty spread out

): boolean => {
return (
allowedOrigins?.some(
(allowedOrigin) =>
allowedOrigin &&
(allowedOrigin === originDomain || isMatch(originDomain, allowedOrigin))
) ?? false
)
}
2 changes: 1 addition & 1 deletion packages/next/src/server/config-shared.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[]
}
Expand Down
Loading