Server Actions and Security

[ScreamZ]

Hi NextJS Team, React Core team.

Sorry guys, I'm pinging @leerob @styfle @timneutkens, because I really think this is a really important subject for the community.

I'm posting this today because I'm really concerned about server actions and what can occurs.
It's been 10 times that I'm reading the doc and I'm not sure yet about how secure it is. I think we might be creating a demon that can be out of control

My goal: Trying to improve the documentation, especially for beginners and not the best engineers working with react and NextJS. Globally improve security overall for all apps on NextJS

How accessible is code from the function?

While there is a dedicated paragraph here it doesn't talk about a NO-GO scenario, when you can accidentally leak data.

I know things get encrypted, but this magic serialization and things scares me a bit.

Here are some questions I ask, maybe you can make me confident with it, and maybe we could improve the doc in the same manner?

1️⃣ When I define functions at top-level in a use server file:

• Are all of the declared functions available to potential malicious users:
  • Are they each available only if used somewhere? And maybe stripped out in another case by something like three shaking.
  • and/or passed to the client component?
• Can anyone guess a list of server actions and call them, without them being triggered by UI?
• Are only top-level functions accessible? Or nested inline callback too? See the following important note

Important

See alfonsusac/nextjs-better-unstable-cache#9 and alfonsusac/nextjs-better-unstable-cache#10 which makes me not really confident about why callback are defined as async here, does it means they are exported and accessible outside? This can be really really security risky.

poke @TheEdoRan Maybe you're concerned with next-safe-action as the action builder is passing a callback. Does it mean you can access the function without handling middleware or schema?

Note

Risks: Major breaches, hacking, data-leaking sneaky, and ability to interact with private methods.
Cons: Bad developer experience, thinking a function is not async where it is

They are queued, they are designed for mutation only

From here

Server Actions are designed for mutations that update the server-side state; they are not recommended for data fetching. Accordingly, frameworks implementing Server Actions typically process one action at a time and do not have a way to cache the return value.

I see plenty of tutorials and content about data fetching using server actions. Especially with things like Infinite paging.
The fact: It works, maybe it's not optimized, but there is no mention of it in nextJS doc here. Do you think maybe we could add some topic about it. Is there any risks hidden here in doing that? Or this is just the nature of "One by One processing" that makes it not optimized for querying async data after the initial page has loaded?

It's a very powerful tool, but with power comes great responsibility.

1. There is no mention that useTransition is highly recommended.
2. There is no mention that when spamming a server action call, without waiting for the result, they are queued one by one and not cancellable.

Note

Risks: Bad usage, low performance, data leak, hack breach in the backend, misconception.

No warning in runtime that it shouldn't export anything else than function.

If you export a constant, or anything. It simply doesn't work and most of the time fails silently. Try exporting an enumeration from a file with use server. You get an empty object. But no error.

If you export not async function you get a warning. Why no more?

Note

Risk: Has no one yield, you might be missing runtime error here, very hard to debug.

---

That's a lot of content, but I'm really scared because even with almost 10 years of experience in React, I'm not confident with server action magic. I can only imagine newcomers, making breaches in their production apps and hackers having joy because we missed some key points showing server action as the go very easy tool.

tRPC was already a great step, but it was not magic, that was only a wrapper for the network. Here it's a bit more risky.

Thanks for reading me,
Hope we find the answers, improve this and mark the risks or counter them upfront :)

Andréas

[r1tsuu]

tRPC was already a great step, but it was not magic, that was only a wrapper for the network.

I can't see server actions are different from this. There's a bit magic on how Next.js creates them, but under the hood, they're all just POST route handlers or something. You shouldn't think that user can't guess the action URL or pass unsanitized data the same, as in normal route handlers. I would even bet that you actually can somehow debug the list of actions (at least that are used on the current page), because Next.js needs to pass some metadata about them to a client too.

I think you're overthinking it, but I kinda agree that it's easy for newcomers to write unsafe code with them..

No mention that it shouldn't export anything else than function

Here

or at the top of a separate file to mark all exports of that file as Server Actions

All exports, meaning all of them should be async functions. Should still be fine with exporting type or interface, but enum for example exports a JS object.

[ScreamZ]

I can't see server actions are different from this.

Yes, but you have full control on endpoint, as there is no transpilation magic. Therefore you're less prone to error leaks.

Here

They mention it, but its not crystal clear. Adding a small linter or things like they are doing when you try to get more invalid props from layout or page could be nice.

I think you're overthinking it, but I kinda agree that it's easy for newcomers to write unsafe code with them..

I might be. But:

• How can nested callback be also transformed as async function as described in Does not revalidate on 14.2 alfonsusac/nextjs-better-unstable-cache#9 and Functions are passed as Promises in Server Actions alfonsusac/nextjs-better-unstable-cache#10 ?

I'm actually validating my payload on the top-level server actions. But if someone manage to access a nested callback, it could bypass that! Which is critical. Maybe you have some idea for this?

[leerob]

Hey there, have you given this post a read? 🙏

https://nextjs.org/blog/security-nextjs-server-components-actions

[ScreamZ]

Hey, thanks for sharing this and thanks for taking part to the conversation.
I've been reading it in past time, but I've been re-reading it to be sure I'm not asking dumb questions.

While it gives some precisions (and highly appreciated) about the behavior, it still remains some dark areas. In fact I was pretty confident until I found this issue that impact non-exported code. But sometimes I used to be pessimistic about some things, where stakes could be really important. Like in medical care or other related softwares. What happens if someone can kill someone if he can reach an not supposed to be accessible functions? 😔

I understand the concept of:

• tainting objects, and double check payload that is serialized to avoid data leak through client component.
• having a Data Access layer to share memory, and reduce risks of data leak.
• That closure declared inline get encrypted to secure the scope.
• That server-only import helps. (and I'm already using it).

The "use server" annotation exposes an end point that makes all exported functions invokable by the client. The identifiers is currently a hash of the source code location. As long as a user gets the handle to the id of an action, it can invoke it with any arguments.

Alright « As long as a user gets the handle to the id of an action » is my point. I can read all exported functions but my question is do nested callback like the one of this post that say « calls to other functions get promisified. Is this a react behavior? This wasnt a problem in 14.1.0 » are exposed to and have a handle, because they get promisified, or only the function declared at module level. Maybe I'm getting it too far, but why are they promisied as they are not exported.

Does it also means that not exported function remains not invokable by client?

If I write

"use server";
import { buildFn } from "./B"
// File A
export const A = buildFn((some: any) => {
   // Some Read / Write to the database, expected to be already secured by server action data validation in buildFn
  return some.data.from.above.comment.operation
}));
export const B = buildFn((some: any) => some.other));

// Hope it's not by any way even by reverse engineering the frontend code in browser.
function helloImSupposedToNotBeInvokable() {
 return "do i ?";
}

// File B
async function buildFn(cb: any){
	return (somePayloadFromServerAction: any) => {
      const validatedPayload = validate(somePayloadFromServerAction);

    return cb(validatedPayload)
   }
}

• Why are the callback (some: any) => some.other) promisified as described in linked post ?
• Are they exposed with an handle that allows to call them bypassing buildFn returned function (that does validation` ?

---

About other topics above

They are queued, they are designed for mutation only
Do you have any thoughts?

No warning in runtime that it shouldn't export anything else than function.
What about warning at runtime for other things than non-async fn? Currently there is a warning for non-async fn but bot for things like enums.