Incident Report (Open Saas updateCurrentUser) blog post

* add martin as an author + post

* add all files

* rename post

* fix header size

* Incident report final touch.

---------

Co-authored-by: Martin Sosic <[email protected]>

opensaas-sh/blog/astro.config.mjs

@@ -99,6 +99,12 @@ export default defineConfig({
               picture: '/milica.jpg', // Images in the `public` directory are supported.
               url: 'https://wasp.sh',
             },
+            martin: {
+              name: 'Martin',
+              title: 'CTO @ Wasp',
+              picture: '/martin.jpg', // Images in the `public` directory are supported.
+              url: 'https://wasp.sh',
+            },
           },
         }),
       ],

opensaas-sh/blog/src/content/docs/blog/2025-02-26-incident-report-vulnerability-in-open-saas.mdx

@@ -0,0 +1,210 @@
+---
+title: "Incident Report: Security Vulnerability in Open SaaS \"updateCurrentUser\" function"
+date: 2025-02-26
+tags:
+  - incident-report
+authors: martin
+---
+import { Image } from 'astro:assets';
+
+On Feb 12th, 2025, we learned about a security vulnerability in the `updateCurrentUser` function of our Open SaaS template. Users of apps built with Open SaaS can exploit this vulnerability to edit any field in their own `User` database record, including fields they weren't supposed to have write permissions for, like `subscriptionPlan` and `isAdmin`.
+
+**If you created your app with the Open SaaS template before Feb 14th of '25, your app potentially suffers from this vulnerability, and you should apply the fix from this report as soon as possible**. Check out [vulnerability](#the-vulnerability) and [fix](#the-fix) sections.
+
+The vulnerability does not affect the "vanilla" Wasp apps (Wasp apps not built with Open SaaS template) or those that modified the problematic part of the code enough to eliminate the problem.
+
+Since then we fixed the vulnerability in all the versions of the Open SaaS template, did a [coordinated vulnerability disclosure](https://en.wikipedia.org/wiki/Coordinated_vulnerability_disclosure) (culminating with this report) with the suggested fix, reviewed all the other templates and example apps of ours for similar security vulnerabilities, analyzed what at the first place enabled such omission to happen on our side, and prepared a plan on how to minimize the chance of similar mistakes happening in the future. 
+
+We sincerely apologize for the impact and inconvenience caused by our mistake. Caring about code quality is at the center of our culture here at Wasp, but in this instance, we failed to follow up on our standards. We are deeply disappointed by it and will ensure we learn from it, improve, and regain your trust in the code we ship, especially as Wasp is heading from Beta toward 1.0.
+
+## The vulnerability
+
+The vulnerability is caused by the `updateCurrentUser` function in `src/user/operations.ts` (or in `src/server/actions.ts` if you used an older version of Open SaaS):
+
+```tsx
+export const updateCurrentUser: UpdateCurrentUser<Partial<User>, User> = async (user, context) => {
+  if (!context.user) {
+    throw new HttpError(401);
+  }
+
+  return context.entities.User.update({
+    where: {
+      id: context.user.id,
+    },
+    data: user,  // <- This is the problem!
+  });
+};
+```
+
+While this Wasp Action correctly allows the user to modify only data in their own `User` database record, and not of other users, it does also allow them to potentially change ANY of the fields on their own User db model, including fields like `credits`, `subscriptionPlan`, and `isAdmin`, due to `data: user` line in `User.update()` call. Particularly troublesome is the ability to set `isAdmin` to `true`, as it gives them further privileges they shouldn't have.
+
+An example of how a bad actor could exploit this is by creating a user account in your app, obtaining their own auth credentials via browser dev tools, and then sending a modified request to the HTTP route of `updateCurrentUser` Wasp Action with a payload that sets the `isAdmin` field to `true` for themselves.
+
+## The fix
+
+The fix consists of three main steps:
+
+1. Refactor `updateCurrentUser` function to `updateCurrentUserLastActiveTimestamp` 
+2. Implement additional Wasp Action(s) for updating user data if needed
+3. Refactor `updateUserById` function to `updateUserIsAdminById`
+
+### Refactor `updateCurrentUser` to `updateCurrentUserLastActiveTimestamp`
+
+In the Open SaaS template, as it comes when you create a new project with it, the Wasp Action `updateCurrentUser` isn't used for anything else but updating the `lastActiveTimestamp` field on the `User` model, despite its general nature. Therefore, we recommend the following fix:
+
+1. Rename the operation `updateCurrentUser` to `updateCurrentUserLastActiveTimestamp`. Make sure to update its name in all the places: `main.wasp`, client code (i.e. `src/client/App.tsx`), server code (i.e. `src/user/operations.ts`).
+2. Rewrite the operation `updateCurrentUserLastActiveTimestamp` in `src/user/operations.ts` so it receives no arguments and only updates the `lastActiveTimestamp` field on the `User`:
+
+    ```tsx
+    export const updateCurrentUserLastActiveTimestamp: UpdateCurrentUserLastActiveTimestamp<void, User> = async (_args, context) => {
+      if (!context.user) {
+        throw new HttpError(401);
+      }
+
+      return context.entities.User.update({
+        where: {
+          id: context.user.id,
+        },
+        data: {
+          lastActiveTimestamp: new Date(),
+        },
+      });
+    };
+    ```
+
+    Notice that also the name of the type of the operation changed, so you will want to update the type import, and we also changed the operation's Input type to `void`.
+
+3. Remove all arguments from the call to `updateCurrentUserLastActiveTimestamp` in `src/client/App.tsx:`
+
+    ```tsx
+          if (today.getTime() - lastSeenAt.getTime() > 5 * 60 * 1000) {
+            updateCurrentUserLastActiveTimestamp(); // <- no args anymore
+          }
+    ```
+
+
+### Implement additional Wasp Action(s) for updating user data if needed
+
+If you were using `updateCurrentUser` in your code beyond just updating `lastActiveTimestamp`, to allow the user to update some other `User` fields, we recommend also defining additional, more specialized Wasp Action(s) that will handle this additional usage.
+
+For example, let's say that in your app you additionally defined `fullName` and `address` fields on the `User` model, and you were using `updateCurrentUser` to allow the user to update those. In that case, we recommend defining an additional Wasp Action called `updateCurrentUserPersonalData`. It could look something like this:
+
+```tsx
+export const updateCurrentUserPersonalData: UpdateCurrentUserPersonalData<Pick<User, "fullName" | "address">, User> = async (personalData, context) => {
+  if (!context.user) {
+    throw new HttpError(401);
+  }
+
+  // NOTE: This is also a good place to do data validation if you want to.
+  const fullName = personalData.fullName
+  const address = personalData.address
+
+  return context.entities.User.update({
+    where: {
+      id: context.user.id,
+    },
+    data: { fullName, address }
+  });
+};
+```
+
+### Refactor `updateUserById` to `updateUserIsAdminById`
+
+Finally, while not a security vulnerability, we also recommend updating the related Wasp Action, `updateUserById` (you can find it next to where the `updateCurrentUser` function was), in a similar fashion, to ensure it can't do more than we need it to:
+
+1. Rename from `updateUserById` to `updateUserIsAdminById`.
+2. Rewrite `updateUserIsAdminById` to only allow setting the `isAdmin` field:
+
+```tsx
+export const updateUserIsAdminById: UpdateUserIsAdminById<{ id: User['id'], isAdmin: User['isAdmin'] }, User> = async ({ id, isAdmin }, context) => {
+  if (!context.user) {
+    throw new HttpError(401);
+  }
+  if (!context.user.isAdmin) {
+    throw new HttpError(403);
+  }
+  if (id === undefined || isAdmin === undefined) {
+    throw new HttpError(400);
+  }
+
+  return context.entities.User.update({
+    where: { id },
+    data: { isAdmin },
+  });
+};
+```
+
+Notice that we modified the shape of the operation input (now it is `{ id, isAdmin }`), so you will also want to update the calls to this operation accordingly.
+
+---
+
+## Additional reading
+
+This second part of the report is not required reading: all you need to know in order to fix the vulnerability is in the "[The vulnerability](#the-vulnerability)" and the "[The fix](#the-fix)" sections. But, if you want to learn more about what caused the vulnerability, how we handled it, and what are we doing to prevent similar mistakes from happening in the future, read on!
+
+
+## Coordinated vulnerability disclosure
+
+The challenging part about handling a security vulnerability like this one is that we have to make the knowledge of it public so that all the people with affected apps learn about it and how to fix it, but then at the same time we are also making that knowledge easily available to any bad actors that might want to try to exploit it.
+
+One of the popular approaches is [coordinated vulnerability disclosure](https://en.wikipedia.org/wiki/Coordinated_vulnerability_disclosure) and it is also what we chose to follow in this instance. We decided to disclose the vulnerability in stages, with 1-week pauses in between:
+
+1. Stage 1 (private disclosure): We assembled a list of everybody we knew was building and deploying apps with Wasp / Open SaaS, be it from our community on Discord, from online search, or from interacting with them in the past. We privately reached out to everybody on the list and shared the details of the vulnerability and the fix, while also asking them to keep it confidential till we go public with it.
+2. Stage 2 (community disclosure): About a week later, we shared the details of the vulnerability in our Discord community, while again asking people not to share it publicly till we go public with it.
+3. Stage 3 (public disclosure): Finally, a week after the Stage 2, we shared the vulnerability fully publicly.
+
+## How did this happen?
+
+**TL;DR**: Failure in our code review process.
+
+At Wasp, we care a lot about code quality, the code review process, and [software craftsmanship](https://martinsosic.com/book/2018/08/17/book-the-software-craftsman.html) in general. PRs get thoroughly reviewed, we do our best to write Clean Code (with a grain of salt), we think a lot about [naming](https://wasp.sh/blog/2023/10/12/on-importance-of-naming-in-programming), we produce [RFCs](https://wasp.sh/blog/2023/12/05/writing-rfcs) for any complex feature, our codebase is strongly typed (Haskell and TypeScript), we keep track of all the issues and ideas publicly on our GitHub to not lose sight of them and to also get community input and be transparent.
+
+Commitment to these practices does get tested regularly: Wasp is moving fast and is changing a lot since it is still pre-1.0, so there is always more tech debt going on than one would like, but we always felt like we managed to stay on the good side of our commitment to these practices: they enable us to be efficient but also to enjoy and be proud of our work.
+
+So what happened then, what enabled this vulnerability in Open SaaS?
+
+Open SaaS started as a one-person experiment, a simple template/boilerplate starter for Wasp, so we didn't do the usual thorough code reviewing of every PR at the very start but thought we would do it fully later, once it shaped up a bit. Also, it is just a template, not a library/framework, people can read/modify the code as they like.
+
+Open SaaS did shape up, and not only have people started using it, but it really picked up, more than we ever expected, and we were getting a lot of positive and constructive feedback, feature requests, ideas, bug reports, … . We started reviewing all the new code thoroughly, but we still haven't done the full retroactive review. We have done some of it, for parts of more sensitive modules, and some of it happened naturally through refactoring, but we haven't done it systematically for the whole codebase. We would discuss during every quarterly planning how we should do it this quarter, but there was always something with a higher priority, especially on the Wasp side, and Open SaaS was doing great, if there was anything serious, we would already know about it, we thought.
+
+And then we learned about a function in our codebase that allows a user to set any data, without runtime validation, as a partial update for their `User` record in the database. This function was barely even used in the Open SaaS codebase at this point: it was used only to update a single field in the `User` database model, and even that usage should have been refactored into something better already. This function was an obvious code smell, but we never reviewed it properly.
+
+The fact is, we never should have made Open SaaS publicly available without doing a full code review of it first. Once the code is out there, be it just an example app, a template, or a library, we can't guess how it or its usage will evolve, or how will our priorities evolve. Once an exception in the (review) process is made, it is much harder to find the time to catch up on it, than if we did it when we should have done it in the first place.
+
+## What we are doing to prevent similar mistakes
+
+- **No code/documentation goes public without a thorough review.**
+We have been doing this from the very start for the Wasp framework codebase, but we were more lenient with the templates and example apps. From now on, there will be no exceptions.
+- **We checked all our existing templates and example apps for vulnerabilities.**
+- **We have done a thorough review of the Open SaaS template codebase.**
+We have already merged a lot of code quality improvements based on it, and we are in the process of merging the rest.
+- **We will make it harder at the Wasp framework level to make a similar mistake.**
+The mistake of passing unvalidated/unparsed data is too easy to make - we will, latest for Wasp 1.0, enforce [runtime data validation in Wasp](https://github.com/wasp-lang/wasp/issues/1241), for Operations, APIs, and other externally facing methods. We also have good ideas for advanced access control support in Wasp, which should further make it harder to make these kinds of mistakes.
+
+## Timeline
+
+What follows is the timeline of the actions we have taken since we learned about the vulnerability, in order to minimize its impact:
+
+- Feb 12th, 2025 (Wed), 10 pm CET: we learned about the vulnerability (thanks [Ivan Vlahov](https://github.com/vlahovivan)!)
+- Feb 13th (Thu):
+    - Made an action plan on how to handle the incident, including how we will execute the coordinated disclosure.
+    - Fixed all the versions of the Open SaaS template, to prevent new projects from being affected.
+- Feb 14th (Fri):
+    - Wrote the "Incident Notification" document with a detailed explanation of the problem and the suggested fix.
+    - Compiled a list of the people we know are deploying Open SaaS / Wasp apps and privately shared the "Incident Notification" document with them, giving them ~ a week of head start before we go more public with the incident.
+    - Reviewed all the other Wasp templates and example apps for similar security issues.
+    - Started a deep (re)review of all the Open SaaS code (that will continue into the next week).
+- Feb 17th (Mon):
+    - Continued deep review of Open SaaS code.
+- Feb 18th (Tue):
+    - Continued deep review of Open SaaS code.
+    - Finalized first draft of this Incident Report document.
+- Feb 19th (Wed):
+    - Continued deep review of Open SaaS code.
+- Feb 20th (Thu):
+    - Continued deep review of Open SaaS code.
+    - Notified our Discord community about the incident by sharing the "Incident Notification" document with them, giving them a week of head start before we go fully public with the incident.
+- Feb 21st (Fri):
+    - Finalized the deep review of the Open SaaS code (while continuing with the code improvements).
+- Feb 26th (Wed):
+    - Went public with the incident by publishing and sharing this Incident Report.