t3stack-todo-app icon indicating copy to clipboard operation
t3stack-todo-app copied to clipboard

Missing access control while doing some CRUD operations.

Open ytkimirti opened this issue 2 years ago • 0 comments

In src/trpc/rotuer/todo.ts these procedures only checks if the user is authenticated but does not check if the task it wants to mutate belongs to him.

This allows any logged in user to see every other users tasks by modifying trpc requests and trying different task id's.

I just started learning trpc and prisma and this really confused me. The getTasks and createTask function does use ctx.session.user.id but these do not

  getSingleTask: authedProcedure
    .input(getSingleTaskSchema)
    .query(({ ctx, input }) => {
      return ctx.prisma.task.findUnique({
        where: {
          id: input.taskId,
        },
      });
    }),
  updateTask: authedProcedure
    .input(updateTaskSchema)
    .mutation(async ({ ctx, input }) => {
      const task = await ctx.prisma.task.update({
        where: {
          id: input.taskId,
        },
        data: {
          title: input.title,
          body: input.body,
        },
      });
      return task;
    }),
  deleteTask: authedProcedure
    .input(deleteTaskSchema)
    .mutation(async ({ ctx, input }) => {
      await ctx.prisma.task.delete({
        where: {
          id: input.taskId,
        },
      });
    }),

The fixed code

  getSingleTask: authedProcedure
    .input(getSingleTaskSchema)
    .query(({ ctx, input }) => {
      // Used findFirst because findUnique doesn't allow userID?? I don't know why
      return ctx.prisma.task.findFirst({
        where: {
          id: input.taskId,
          // Check for userID
          userId: ctx.session.user.id,
        },
      });
    }),
  updateTask: authedProcedure
    .input(updateTaskSchema)
    .mutation(async ({ ctx, input }) => {
      // Check if THIS user actually has the task
      if (
        !(await ctx.prisma.task.findFirst({
          where: {
            id: input.taskId,
            userId: ctx.session.user.id,
          },
        }))
      )
        throw new Error("Task not found");
      const task = await ctx.prisma.task.update({
        where: {
          id: input.taskId,
        },
        data: {
          title: input.title,
          body: input.body,
        },
      });
      return task;
    }),
  deleteTask: authedProcedure
    .input(deleteTaskSchema)
    .mutation(async ({ ctx, input }) => {
      // Same as above
      if (
        !(await ctx.prisma.task.findFirst({
          where: {
            id: input.taskId,
            userId: ctx.session.user.id,
          },
        }))
      )
        throw new Error("Task not found");
      await ctx.prisma.task.delete({
        where: {
          id: input.taskId,
        },
      });
    }),

Diff

diff --git a/src/server/trpc/router/todo.ts b/src/server/trpc/router/todo.ts
index e8a4aaa..8b7f60c 100644
--- a/src/server/trpc/router/todo.ts
+++ b/src/server/trpc/router/todo.ts
@@ -35,15 +35,28 @@ export const todoRouter = t.router({
   getSingleTask: authedProcedure
     .input(getSingleTaskSchema)
     .query(({ ctx, input }) => {
-      return ctx.prisma.task.findUnique({
+      // Used findFirst because findUnique doesn't allow userID?? I don't know why
+      return ctx.prisma.task.findFirst({
         where: {
           id: input.taskId,
+          // Check for userID
+          userId: ctx.session.user.id,
         },
       });
     }),
   updateTask: authedProcedure
     .input(updateTaskSchema)
     .mutation(async ({ ctx, input }) => {
+      // Check if THIS user actually has the task
+      if (
+        !(await ctx.prisma.task.findFirst({
+          where: {
+            id: input.taskId,
+            userId: ctx.session.user.id,
+          },
+        }))
+      )
+        throw new Error("Task not found");
       const task = await ctx.prisma.task.update({
         where: {
           id: input.taskId,
@@ -58,6 +71,16 @@ export const todoRouter = t.router({
   deleteTask: authedProcedure
     .input(deleteTaskSchema)
     .mutation(async ({ ctx, input }) => {
+      // Same as above
+      if (
+        !(await ctx.prisma.task.findFirst({
+          where: {
+            id: input.taskId,
+            userId: ctx.session.user.id,
+          },
+        }))
+      )
+        throw new Error("Task not found");
       await ctx.prisma.task.delete({
         where: {
           id: input.taskId,

ytkimirti avatar Feb 10 '23 11:02 ytkimirti