[go: nahoru, domu]

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

where: { shop: undefined }: The entire table data was accidentally deleted #20169

Open
bithaolee opened this issue Jul 11, 2023 · 9 comments
Open
Labels
domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. kind/improvement An improvement to existing feature and code. tech/engines Issue for tech Engines. topic: deleteMany() topic: null vs undefined topic: undefined

Comments

@bithaolee
Copy link
bithaolee commented Jul 11, 2023

Bug description

let shop = undefined; // shop is variable, and in some cases, it may be undefined

await prisma.theme.deleteMany({
  where: {
    shop,
  }
})

I left out the shop parameter check, causing the shop to be undefined in some cases, triggering the deletion of the entire table. This operation of Prisma is very risky, I hope to improve it.

@bithaolee bithaolee added the kind/bug A reported bug. label Jul 11, 2023
@millsp millsp added kind/improvement An improvement to existing feature and code. tech/engines Issue for tech Engines. domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. topic: deleteMany() and removed kind/bug A reported bug. labels Jul 12, 2023
@millsp
Copy link
Member
millsp commented Jul 12, 2023

Hey, thanks for the report, we are aware of this issue and I fully agree with you that it can be dangerous. We should improve it.

@bithaolee
Copy link
Author

Thank you @millsp, maybe updateMany operation has a similar issue that I haven't tested, and if so, I'd like to see it solved together

@janpio
Copy link
Member
janpio commented Jul 12, 2023

From our point of view this is not surprising and a "bug we have to fix", but how Prisma is currently designed to work: https://www.prisma.io/docs/concepts/components/prisma-client/null-and-undefined We know this can be dangerous, but it has been like this for quite some while and switching would also have other deficits.

So as soon as we find the proper issue that requests us to change this, we will close this issue here as a duplicate.

@janpio janpio changed the title The entire table data was accidentally deleted where: { shop: undefined }: The entire table data was accidentally deleted Jul 12, 2023
@why-el
Copy link
why-el commented Aug 27, 2023

This is extremely dangerous, I do not think it's being treated as such. You are offering an abstraction that does the opposite of what the SQL standard does (and the majority of your users understand SQL before they understand Prisma). When filtering by emptiness (where id in [], or undefined), no database will return records, but Prisma does. This is even worse in the case of deletes.

I fear this is one famous case of mass-deletion away from a PR disaster for Prisma. I am a happy user of the library, mind you, but I'd recommend more urgency to this issue. I spent 2 minutes googling, and I already found a case of someone who had to suffer from this issue: https://www.farmin.dev/posts/35ad668d-29ae-4d86-8b4a-126b6d561aaa.

@lnfel
Copy link
lnfel commented Nov 23, 2023

Dude I just shot myself on the foot right now. As a Prisma advocate, my spirit is in SHAMBLES right this moment.

@lnfel
Copy link
lnfel commented Nov 23, 2023

I don't care if breaking change were to be introduced, this evil shall be corrected.

@lnfel
Copy link
lnfel commented Nov 23, 2023

Apparently if the data we are querying is expected to be undefined for some reason we can use nullish coalescing operator just like:

// this will return null back to us
const user = await prisma.user.findFirst({
  where: {
    // let's use an empty string instead and prisma will use it in the SQL query!
    id: somethingNull?.id ?? '',
  }
})

OP's issue can be avoided by doing the same thing shop ?? ''.
My spirit is now in shambles.

@jakezegil
Copy link
jakezegil commented Jan 19, 2024

We unfortunately ran into this issue (luckily had snapshots we could restore), and I had to write some middleware because this was super dangerous. I hope this can help somebody!

import { isEmpty } from "lodash";
import { Prisma } from "prisma/generated";

/**
 * Middleware featured below
 */

// This naming convention specifically matches the
// safe word to the exported constant to support the
// middleware error handling
export const safeWordDelete = "FORCE_DELETE_ALL";
export const FORCE_DELETE_ALL = { id: safeWordDelete };

export const safeWordFind = "FIND_ALL";
export const FIND_ALL = { id: safeWordFind };

export const safeWordUpdate = "UPDATE_ALL";
export const UPDATE_ALL = { id: safeWordUpdate };

/**
 * If you have found yourself here after a hard lesson:
 *
 * These middleware specifically do NOT guard in the case of a nested find, update, delete.
 * These middleware also don't protect in the case of an undefined variable being passed in alongside a specific condition.
 *
 * Ex: {
 *    id, // undefined
 *    otherAttr: null
 * }
 *
 * The above where clause will delete/modify/find all regardless of undefined status of variable appId.
 *
 * The !! best !! guard here is proper type adherence - do not bang, do not coerce.
 */
const _guardEnMasse = ({
  params,
  actions,
  safeWord,
}: {
  params: Prisma.MiddlewareParams;
  actions: Prisma.MiddlewareParams["action"][];
  safeWord: string;
}) => {
  // Check if empty, if not and safeWord is provided, then set where = {} and proceed to execute
  if (actions.includes(params.action)) {
    // Don't allow delete all queries
    if (isEmpty(params.args.where)) {
      throw new Error(
        `It looks like you just tried to perform a ${params.action} on all of the ${params.model}s. If this was intentional, pass 'where: ${safeWord}'`,
      );
    }
  }

  if (params.args?.where?.id === safeWord) {
    params.args.where = {};
  }
};

export const guardEnMasse: PrismaMiddleware = async (params, next) => {
  /* DELETION PROTECTION MIDDLEWARE */
  _guardEnMasse({
    actions: ["delete", "deleteMany"],
    params,
    safeWord: safeWordDelete,
  });

  /* MASS FIND PROTECTION MIDDLEWARE */
  _guardEnMasse({
    actions: ["findMany"],
    params,
    safeWord: safeWordFind,
  });

  /* MASS UPDATE PROTECTION MIDDLEWARE */
  _guardEnMasse({
    actions: ["updateMany"],
    params,
    safeWord: safeWordUpdate,
  });

  return next(params);
};

and then

const dbClient = new PrismaClient();

dbClient.$use(guardEnMasse);

const whoops = undefined;

// Throws an error
dbClient.sensitiveData.deleteMany({ where: { id: whoops } });

// Deletes everything
dbClient.insensitiveData.deleteMany({ where: FORCE_DELETE_ALL });

Best guard here is strict type safety and attribute validation ahead of db writes. Also, always always always backup your production database!

@scumbkt19
Copy link
scumbkt19 commented Jul 9, 2024

Thank you @jakezegil for sharing how to add runtime safety to these queries, but I wondered if more could be done to prevent this sort of issue at compile time since all our infrastructure would be happy shipping dbClient.sensitiveData.deleteMany() to prod.

Heavily influenced by @kz-d's https://github.com/kz-d/prisma-where-required generator, I wrote the following generator which 1) makes the argument to all the bulk operations required and then 2) makes the where clause in the parameter required.

Combining the buildtime and runtime checks provides a degree of safety I feel more comfortable with.

Setup is the same as in prisma-where-required but the actual source code was modified to

generator.ts

#!/usr/bin/env node

import { generatorHandler } from "@prisma/generator-helper";
import { logger } from "@prisma/internals";

import { convert } from "./convertor";

generatorHandler({
  onManifest: () => ({
    prettyName: "where-required",
    requiresGenerators: ["prisma-client-js"],
    defaultOutput: ".",
  }),
  onGenerate: async (options) => {
    const nodeModulePath = options.generator.config.nodeModulePath;
    if (!nodeModulePath) {
      logger.error("No nodeModulePath config.");
      return;
    }

    const debug = options.generator.config.debug === "true";

    const models = options.dmmf.datamodel.models;

    convert({
      path: `${nodeModulePath}/.prisma/client/index.d.ts`,
      models: models.map((model) => model.name),
      debug,
    });
  },
});

convertor.ts

import { Project, SourceFile, ts } from "ts-morph";
import isPropertySignature = ts.isPropertySignature;

function removeQuestionTokenFromWhereFieldProperty(
  sourceFile: SourceFile,
  models: string[]
) {
  const prismaNamespace = sourceFile.getModuleOrThrow("Prisma");
  let isInTypeAlias = false;
  prismaNamespace.transform((tr) => {
    const currentNode = tr.currentNode;
    if (
      ts.isTypeAliasDeclaration(currentNode) &&
      models.some(
        (targetModel) =>
          currentNode.name.getText().startsWith(targetModel) &&
          !currentNode.name.getText().startsWith(`${targetModel}$`) &&
          (currentNode.name.getText().endsWith("FindManyArgs") ||
            currentNode.name.getText().endsWith("DeleteManyArgs") ||
            currentNode.name.getText().endsWith("DeleteArgs") ||
            currentNode.name.getText().endsWith("UpdateManyArgs"))
      )
    ) {
      isInTypeAlias = true;
      const newNode = tr.visitChildren();
      isInTypeAlias = false;
      return newNode;
    }
    if (
      isInTypeAlias &&
      isPropertySignature(currentNode) &&
      currentNode.name.getText() === "where"
    ) {
      isInTypeAlias = false;
      return tr.factory.updatePropertySignature(
        currentNode,
        currentNode.modifiers,
        currentNode.name,
        undefined,
        currentNode.type
      );
    }
    return tr.visitChildren();
  });
}

function removeQuestionTokenFromActionFunctionProperty(
  sourceFile: SourceFile,
  models: string[]
) {
  const prismaNamespace = sourceFile.getModuleOrThrow("Prisma");

  let isInDelegate = false;
  let isInFindMany = false;
  prismaNamespace.transform((tr) => {
    const currentNode = tr.currentNode;
    if (
      ts.isInterfaceDeclaration(currentNode) &&
      models.some(
        (targetModel) => currentNode.name.getText() === `${targetModel}Delegate`
      )
    ) {
      isInDelegate = true;
      const newNode = tr.visitChildren();
      isInDelegate = false;
      return newNode;
    }
    if (
      isInDelegate &&
      ts.isMethodSignature(currentNode) &&
      (currentNode.name.getText() === "findMany" ||
        currentNode.name.getText() === "deleteMany" ||
        currentNode.name.getText() === "delete" ||
        currentNode.name.getText() === "updateMany")
    ) {
      isInFindMany = true;
      const newNode = tr.visitChildren();
      isInFindMany = false;
      return newNode;
    }
    if (isInFindMany && ts.isParameter(currentNode)) {
      isInDelegate = false;
      isInFindMany = false;
      return tr.factory.updateParameterDeclaration(
        currentNode,
        currentNode.modifiers,
        currentNode.dotDotDotToken,
        currentNode.name,
        undefined,
        currentNode.type,
        currentNode.initializer
      );
    }
    return tr.visitChildren();
  });
}

export function convert(arguments_: {
  path: string;
  models: string[];
  debug: boolean;
}) {
  const project = new Project();
  const sourceFile = project.addSourceFileAtPath(arguments_.path);

  removeQuestionTokenFromWhereFieldProperty(sourceFile, arguments_.models);
  removeQuestionTokenFromActionFunctionProperty(sourceFile, arguments_.models);

  sourceFile.saveSync();
}

(I'll confess I found the ts-morph transform visitor difficult to reason about, there might be a more efficient way to write that loop)

this added <5 seconds to the prisma generate time for my 100+ model file

so then

// Compile error (missing an arg)
dbClient.sensitiveData.deleteMany()

// Compile error (missing a where)
dbClient.sensitiveData.findMany({select: {...}})

// Runtime error (as above)
dbClient.sensitiveData.findMany({ where: {id: undefined}})

// Success via runtime override with maximal clarity on what's happening
dbClient.sensitiveData.findMany({ where: FIND_ALL })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain/client Issue in the "Client" domain: Prisma Client, Prisma Studio etc. kind/improvement An improvement to existing feature and code. tech/engines Issue for tech Engines. topic: deleteMany() topic: null vs undefined topic: undefined
Projects
None yet
Development

No branches or pull requests

7 participants