feat: Add filter support for Prisma #1234
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dqbd
added
question
…L injection, fix examples and migrations
|
Hi! I've done some small adjustments and polish, addressing some of the pain points mentioned in the review above. Notably:
|
dqbd
removed
question
examples/src/indexes/vector_stores/prisma_vectorstore/prisma/migrations/00_init/migration.sql
Show resolved
Hide resolved
langchain/src/vectorstores/prisma.ts
Outdated
| this.Prisma.join( | ||
| [ | ||
| this.Prisma.sql` | ||
| SELECT ${this.selectSql}, ${this.vectorColumnSql} <=> ${vectorQuery}::vector as "_distance" |
There was a problem hiding this comment.
This is ok because everything here is generated in from a config file right or a vector array right?
I still think it would be nice to parameterize things for best practices.
There was a problem hiding this comment.
Or does this.Prisma.sql magically sanitize somehow?
There was a problem hiding this comment.
Yeah actually I think if someone wasn't using TypeScript (or ignored warnings) there's an attack where someone could pass in some argument to this method like:
["* FROM tables; DROP TABLES; SELECT"];
Can we sanitize this?
There was a problem hiding this comment.
Base on the doc:
$executeRaw can only run one query at a time. You cannot append a second query - for example, adding DROP bobby_tables to the end of an ALTER.
Although it didn't mention queryRaw, i found a related issue says:
queryRaw(sql) and executeRaw(sql) will only be executing a single statement, which means some injections won't work (the one with multiple queries trying to drop tables for example) but some will (unveiling additional data using UNION for example)
So I don't think this will be a problem.
There was a problem hiding this comment.
Yep! Parameters are by default sanitised unless they are wrapped in a Prisma.raw call.
There are some dynamic fields, which cannot be parametrised (such as table names and column names). Moved these Prisma.raw calls closer to the executeRaw and queryRaw invocations alongside a note in docs to warn users.
There was a problem hiding this comment.
Ok. Using a single statement helps but I'm sure there's still sneaky issues and damage some bad metadata on a loaded document could do even in a single query, so please escape here:
Raw SQL from user provided fields is also really not ideal but at least it'll be the user's own fault if something happens there.
jacoblee93
added
question
| WHERE ${idSql} = ${documents[idx].metadata[this.idColumn]} | ||
| UPDATE ${tableNameRaw} | ||
| SET ${vectorColumnRaw} = ${`[${vector.join(",")}]`}::vector | ||
| WHERE ${idColumnRaw} = ${documents[idx].metadata[this.idColumn]} |
There was a problem hiding this comment.
What if documents[idx].metadata[this.idColumn] can't be trusted? E.g. if you're loading documents in from a different source?
There was a problem hiding this comment.
Oh, right didn't see the other comment about executeRaw only being able to execute one query. I still don't love this and it should be escaped.
There was a problem hiding this comment.
afaict documents[idx].metadata[this.idColumn] and [${vector.join(",")}] is still escaped and parametrised, as Prisma uses https://github.com/blakeembrey/sql-template-tag behind the scenes :)
|
Thanks for the patience here, will merge with the next release! |
jacoblee93
added
lgtm
Closes #1426