allow passing wildcard domains in serverActions.allowedDomains #59428

akawalsky · 2023-12-09T01:26:42Z

Implementation of feature request opened here - #59427

Approach:

~~We are using micromatch in the csrf protection step of actionHandler to allow for wildcard domains passed in allowedDomains. This is the same library used for matching domains for remote images.~~

If any of the allowed domains match the origin of the request, we skip the downstream error thrown for csrf protection.

Edit:

Micromatch is not available in this context as it is only compatible with Node. This codepath can be run from the edge, so we need to rely on vanilla js compatible code only.

Instead of falling back to allowing the user to pass in a regex, which can be somewhat insecure, we opt into continuing to use a wildcard pattern from a configuration standpoint and instead use a simple function that matches on wildcards using string comparison and iteration.

Ideally, Micromatch can be retrofitted to work in non-Node settings and this piece of code can be replaced in the future, without deprecating or changing the next.config interface.

JustinMartinDev · 2023-12-11T15:01:45Z

Can you add and exemple with wildcard in docs

config-shared.ts:

  /**
     * 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"]
     */
    allowedOrigins?: string[]

serverActions.mdx:

module.exports = {
  experimental: {
    serverActions: {
      allowedOrigins: ['my-proxy.com', '*.my-proxy.com'],
    },
  },
}

02-server-actions-and-mutations.mdx:

```js filename="next.config.js"
/** @type {import('next').NextConfig} */
module.exports = {
  experimental: {
    serverActions: {
      allowedOrigins: ['my-proxy.com', '*.my-proxy.com'],
    },
  },
}

akawalsky · 2023-12-11T15:49:02Z

@JustinMartinDev good call, added your suggestions

gnoff · 2023-12-11T16:15:26Z

packages/next/src/server/app-render/csrf-protection.ts

+
+export const isCsrfOriginAllowed = (
+  originDomain: string,
+  allowedOrigins: string[] | undefined


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

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

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

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 ^.

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

gnoff · 2023-12-11T16:15:55Z

packages/next/src/server/app-render/action-handler.ts

@@ -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


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

ijjk · 2023-12-11T18:13:01Z

Allow CI Workflow Run

approve CI run for commit: ee662fa

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

gnoff

Can you update the PR description to describe the change

akawalsky · 2023-12-11T18:15:42Z

Can you update the PR description to describe the change

Yep, done

gnoff · 2023-12-11T18:17:57Z

Yep, done

I mean the PR body. Currently it just links to the discussion which justifies the change but we want to also articulate the approach. i.e. you're using minimatch which is the same library used to match wildcard domains for remote images etc...

ijjk · 2023-12-11T18:18:38Z

Tests Passed

errors suggest something is up with bundling new dep

gnoff · 2023-12-11T19:06:53Z

@akawalsky seems like micromatch is node.js only. Some of these tests are failing because it is being used in edge runtimes where transitive deps like "path" are not available. Not really sure how remote images works but we'll need a different approach here

akawalsky · 2023-12-11T19:09:28Z

Ah I see. Would reverting to using regex be acceptable in this case then?

gnoff

You don't have to take me exact suggestion but I think we should go with an approach that is more memory efficient and strict about the wildcard matching rules

gnoff · 2023-12-12T00:58:52Z

packages/next/src/server/app-render/csrf-protection.ts

+  // 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
+}


Suggested change

// 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

}

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 pattern = patternParts.pop();

const domain = domainParts.pop();

switch (pattern) {

case "":

case "*":

case "**" {

// invalid pattern. pattern segments must be non empty

// Additionally wildcards are only supported below the domain level

return false

}

default: {

if (domain !== pattern) {

return false;

}

}

}

while (patternParts.length) {

const pattern = patternParts.pop();

const domain = domainParts.pop();

switch (pattern) {

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 (domain.length) {

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 domain.length > 0

}

default: {

if (domain !== pattern) {

return false;

}

}

}

// We exhausted the pattern. If we also exhausted the domain we have a match

return domainParts.length === 0

}

This implementation is a much lower overhead on memory allocations and iterations
It also is more strict, only allowing ** at the front and only supporting wildcards at the subdomain and deeper level

works for me, got it running and replaced with your implementation

ijjk · 2023-12-12T01:42:02Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
buildDuration	10.9s	10.7s	N/A
buildDurationCached	6.1s	6.1s	N/A
nodeModulesSize	200 MB	200 MB	⚠️ +74 kB
nextStartRea..uration (ms)	422ms	422ms	✓

Client Bundles (main, webpack)

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
170-HASH.js gzip	26.7 kB	26.7 kB	N/A
199.HASH.js gzip	181 B	185 B	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	✓
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	240 B	241 B	N/A
main-HASH.js gzip	31.7 kB	31.6 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	N/A
Overall change	98.5 kB	98.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
_app-HASH.js gzip	195 B	194 B	N/A
_error-HASH.js gzip	183 B	182 B	N/A
amp-HASH.js gzip	501 B	501 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	255 B	255 B	✓
head-HASH.js gzip	349 B	350 B	N/A
hooks-HASH.js gzip	368 B	369 B	N/A
image-HASH.js gzip	4.27 kB	4.27 kB	N/A
index-HASH.js gzip	255 B	256 B	N/A
link-HASH.js gzip	2.61 kB	2.6 kB	N/A
routerDirect..HASH.js gzip	311 B	309 B	N/A
script-HASH.js gzip	384 B	384 B	✓
withRouter-HASH.js gzip	307 B	306 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	1.57 kB	1.57 kB	✓

Client Build Manifests

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
_buildManifest.js gzip	484 B	482 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
index.html gzip	530 B	527 B	N/A
link.html gzip	542 B	540 B	N/A
withRouter.html gzip	524 B	524 B	✓
Overall change	524 B	524 B	✓

Edge SSR bundle Size Overall increase ⚠️

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
edge-ssr.js gzip	93.7 kB	93.7 kB	N/A
page.js gzip	146 kB	146 kB	⚠️ +158 B
Overall change	146 kB	146 kB	⚠️ +158 B

Middleware size

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
middleware-b..fest.js gzip	627 B	624 B	N/A
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	37.4 kB	37.4 kB	N/A
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	2.07 kB	2.07 kB	✓

Next Runtimes Overall increase ⚠️

	vercel/next.js canary	akawalsky/next.js server-actions-regex-allowed-domains	Change
app-page-exp...dev.js gzip	168 kB	168 kB	⚠️ +147 B
app-page-exp..prod.js gzip	93.9 kB	94.1 kB	⚠️ +144 B
app-page-tur..prod.js gzip	94.7 kB	94.8 kB	⚠️ +144 B
app-page-tur..prod.js gzip	89.2 kB	89.4 kB	⚠️ +146 B
app-page.run...dev.js gzip	138 kB	138 kB	⚠️ +152 B
app-page.run..prod.js gzip	88.5 kB	88.7 kB	⚠️ +145 B
app-route-ex...dev.js gzip	23.9 kB	23.9 kB	✓
app-route-ex..prod.js gzip	16.6 kB	16.6 kB	✓
app-route-tu..prod.js gzip	16.6 kB	16.6 kB	✓
app-route-tu..prod.js gzip	16.2 kB	16.2 kB	✓
app-route.ru...dev.js gzip	23.4 kB	23.4 kB	✓
app-route.ru..prod.js gzip	16.2 kB	16.2 kB	✓
pages-api-tu..prod.js gzip	9.37 kB	9.37 kB	✓
pages-api.ru...dev.js gzip	9.64 kB	9.64 kB	✓
pages-api.ru..prod.js gzip	9.37 kB	9.37 kB	✓
pages-turbo...prod.js gzip	21.9 kB	21.9 kB	✓
pages.runtim...dev.js gzip	22.5 kB	22.5 kB	✓
pages.runtim..prod.js gzip	21.9 kB	21.9 kB	✓
server.runti..prod.js gzip	49.4 kB	49.4 kB	✓
Overall change	929 kB	930 kB	⚠️ +878 B

Diff details

Diff for page.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

Diff too large to display

Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Commit: c330fb7

allow passing regex in serverActions.allowedDomains

9f3127b

akawalsky requested review from timneutkens, ijjk, shuding, huozhi, feedthejim, ztanner and wyattjoh as code owners December 9, 2023 01:26

ijjk added the type: next label Dec 9, 2023

use micromatch instead of regex

b6964ce

add to docs

5b4978c

akawalsky requested review from a team as code owners December 11, 2023 15:48

akawalsky requested review from manovotny and lydiahallie and removed request for a team December 11, 2023 15:48

gnoff requested changes Dec 11, 2023

View reviewed changes

akawalsky added 2 commits December 11, 2023 11:20

remove errant comment

cdffa1b

default allowedOrigins to empty array

eb3ff9d

akawalsky requested a review from gnoff December 11, 2023 16:43

gnoff previously approved these changes Dec 11, 2023

View reviewed changes

akawalsky changed the title ~~allow passing regex in serverActions.allowedDomains~~ allow passing wildcard domains in serverActions.allowedDomains Dec 11, 2023

Merge branch 'canary' into server-actions-regex-allowed-domains

3b2bc3c

remove micromatch as a dependency for csrf protection

ee662fa

gnoff requested changes Dec 12, 2023

View reviewed changes

akawalsky and others added 2 commits December 11, 2023 20:26

use suggestion for faster and more memory efficient solution

b9404a6

Merge branch 'canary' into server-actions-regex-allowed-domains

c330fb7

akawalsky requested a review from gnoff December 12, 2023 15:24

gnoff approved these changes Dec 12, 2023

View reviewed changes

gnoff merged commit 6fbff29 into vercel:canary Dec 12, 2023
68 checks passed

dario-piotrowicz mentioned this pull request Dec 12, 2023

[🐛 Bug]: Server actions don't work with wrangler pages dev cloudflare/next-on-pages#588

Closed

github-actions bot added the locked label Dec 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow passing wildcard domains in serverActions.allowedDomains #59428

allow passing wildcard domains in serverActions.allowedDomains #59428

akawalsky commented Dec 9, 2023 •

edited

Loading

JustinMartinDev commented Dec 11, 2023 •

edited

Loading

akawalsky commented Dec 11, 2023

gnoff Dec 11, 2023

gnoff Dec 11, 2023

akawalsky Dec 11, 2023

akawalsky Dec 11, 2023

gnoff Dec 11, 2023

gnoff Dec 11, 2023

ijjk commented Dec 11, 2023 •

edited

Loading

gnoff left a comment

akawalsky commented Dec 11, 2023

gnoff commented Dec 11, 2023

ijjk commented Dec 11, 2023 •

edited

Loading

gnoff commented Dec 11, 2023

akawalsky commented Dec 11, 2023

gnoff left a comment

gnoff Dec 12, 2023 •

edited

Loading

akawalsky Dec 12, 2023

ijjk commented Dec 12, 2023

allow passing wildcard domains in serverActions.allowedDomains #59428

allow passing wildcard domains in serverActions.allowedDomains #59428

Conversation

akawalsky commented Dec 9, 2023 • edited Loading

JustinMartinDev commented Dec 11, 2023 • edited Loading

akawalsky commented Dec 11, 2023

gnoff Dec 11, 2023

Choose a reason for hiding this comment

gnoff Dec 11, 2023

Choose a reason for hiding this comment

akawalsky Dec 11, 2023

Choose a reason for hiding this comment

akawalsky Dec 11, 2023

Choose a reason for hiding this comment

gnoff Dec 11, 2023

Choose a reason for hiding this comment

gnoff Dec 11, 2023

Choose a reason for hiding this comment

ijjk commented Dec 11, 2023 • edited Loading

gnoff left a comment

Choose a reason for hiding this comment

akawalsky commented Dec 11, 2023

gnoff commented Dec 11, 2023

ijjk commented Dec 11, 2023 • edited Loading

Tests Passed

gnoff commented Dec 11, 2023

akawalsky commented Dec 11, 2023

gnoff left a comment

Choose a reason for hiding this comment

gnoff Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

akawalsky Dec 12, 2023

Choose a reason for hiding this comment

ijjk commented Dec 12, 2023

Stats from current PR

akawalsky commented Dec 9, 2023 •

edited

Loading

JustinMartinDev commented Dec 11, 2023 •

edited

Loading

ijjk commented Dec 11, 2023 •

edited

Loading

ijjk commented Dec 11, 2023 •

edited

Loading

gnoff Dec 12, 2023 •

edited

Loading