docs[patch]: Add warnings about SQL Injection for Postgres integrations

[MJDeligan]

Concerning #4396 .

This PR escapes identifiers used in SQL queries made with node-postgres in PGVectorStore and PostgresRecordManager. This implementation handles inputs that previously caused arbitrary queries to be executed.

This can be a breaking change for some users, since the identifiers users pass to the API may be different from the identifiers that are used in the queries. Identifiers containing uppercase letters would have previously been converted to all lowercase but are now conserved. Users can still use the values saved in an object's field directly in their query for example:

const pgVectorStore: PGVectorStore;

await pool.query(`SELECT * FROM ${pgVectorStore.tableName};` // tableName is escaped

AnalyticDB also uses node-postgres with unsafe string interpolation for identifiers, but I could not find a way to test it locally and do not have access to an instance.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
langchainjs-api-refs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 15, 2024 0:47am
langchainjs-docs	✅ Ready (Inspect)	Visit Preview		Feb 15, 2024 0:47am

[jacoblee93]

Ah gotcha - I think our stance on was that is it's kind of XSS'ing yourself since you're initializing the constructor with constants you pass yourself vs documents from userland. I suppose there's a niche case where someone could initialize a table based on user input?

Since it's technically breaking - do you have a strong feeling towards just adding warnings in the documentation instead?

[MJDeligan]

Yeah, I was thinking about the niche case where developers might want a table per user/organization. A warning in the docs would suffice though. Maybe it would make sense to introduce in a breaking patch, since there's also no way to recommend an alternative where developers might sanitize themselves, because the queries are inconsistent with the use of double quotes.

Would you like me to reset the branch and update the docs here or should I close the PR and open a new one?

[jacoblee93]

Thanks - feel free to reuse this same branch. I'd also be down to add a configuration flag that enables this behavior if that sounds like a good compromise?

[MJDeligan]

Updated the docs with warnings (there's no section for RecordManagers, maybe one should be added?).

I looked into using a configuration flag. Because of the inconsistent use of double quotes and identifiers being built from other identifiers there's no straight-forward way to dynamically escape the identifiers. It would just turn the code into a mess for niche use cases. Warnings in the docs should be enough and we can still consider introducing this in a later patch.

[jacoblee93]

Thank you so much!