-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat(langchain): enhance SQL security with validation, parameterization and injection protection #8377
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
base: main
Are you sure you want to change the base?
Conversation
…on and injection protection This commit introduces comprehensive security improvements to the SqlDatabase class: Security Features: - Add SQL injection pattern detection with 20+ dangerous patterns - Implement configurable allowed statements (default: SELECT only in future) - Add parameterized query support with safer parameter binding - Enable SQL validation by default with opt-out capability - Enforce maximum query length limits (default: 10000 chars) - Block multiple statement execution and stacked queries - Detect comment-based and encoding-based injection attempts Breaking Changes (Future): - In next major version, default allowedStatements will be ["SELECT"] only - Applications requiring other SQL operations must explicitly configure allowedStatements Configuration Options: - allowedStatements: Array of permitted SQL statement types - enableSqlValidation: Toggle SQL validation (default: true) - maxQueryLength: Maximum query length in characters (default: 10000) Documentation: - Enhanced security notices and best practices in JSDoc - Added comprehensive examples for secure usage patterns - Detailed security configuration guidance Testing: - Added 400+ lines of comprehensive security test coverage - Unit tests for all injection patterns and edge cases - Parameterized query validation tests - Configuration validation tests This addresses potential SQL injection vulnerabilities while maintaining backward compatibility through configurable security settings.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the surface I think it's a good change to codify our security best practices rather than saying "just do this," but I wonder if there's a better form factor than baking this into the sql abstraction directly. That way we can reuse it and we're not forcing people to use our opinionated way of interfacing with their data.
Something to the tune of:
// hypothetical
const validator = new SqlValidator({
allowedStatements: ["SELECT"],
enableSqlValidation: true,
maxQueryLength: 10000
});
const db = await SqlDatabase.fromDataSourceParams({
appDataSource: datasource
});
await model.pipe(validator).pipe(db).invoke(...);
@christian-bromann thoughts?
I can definitely see the appeal of extracting the validation logic into a separate That said, from my point of view, there's also something reassuring about having strong validation and protection built directly into the
Of course, happy to follow whatever direction the team feels is best here—whether that's evolving this into a separate component or keeping the validation built-in. |
Maybe there's a 'best of both' worlds approach to this where we have a default validator instance inside of SqlDatabase. That way someone can break it out if they want to, but it still lets them bring some kind of validation if they choose to have their own way of issuing queries: class SqlDatabase {
// the default
protected validator: SqlValidator | null = new SqlValidator({
allowedStatements: ["SELECT"],
enableSqlValidation: true,
maxQueryLength: 10000
});
}
// 1. I can either pipe to SqlDatabase that has the default validator
model.pipe(new SqlDatabase(...));
// 2. Use a custom validator in SqlDatabase
model.pipe(new SqlDatabase({
validator: new SqlValidator(...),
...
}));
// 3. Or use SqlValidator before using my own database call
model.pipe(new SqlValidator(...)).pipe(dbRunnable); I could also see it being useful in traces to have a distinct validation step. Looking into this some more, I'm gathering that the straight to |
This PR introduces comprehensive security improvements to the
SqlDatabase
class to protect against SQL injection attacks and other security vulnerabilities.🚀 Key Features
Security Enhancements:
; DROP TABLE
)OR 1=1
)--
,/* */
)UNION SELECT
)xp_cmdshell
,sp_executesql
)run()
method supports safer parameter bindingConfiguration Options:
🔧 Usage Examples
Secure parameterized queries (recommended):
Traditional string queries (with validation):
🔄 Backward Compatibility
All existing functionality remains unchanged. Security features are enabled by default but can be configured or disabled for compatibility with legacy applications.
I would consider this non-breaking, however it is technically breaking as security defaults are stricter.
🛡️ Security Impact
This addresses potential SQL injection vulnerabilities while maintaining flexibility for legitimate use cases through configurable security settings.