Skip to content

Commit 5248ede

Browse files
committed
fix: keep concrete sql security findings
1 parent c3471c8 commit 5248ede

4 files changed

Lines changed: 54 additions & 3 deletions

File tree

__tests__/unit/analysis/finding-filter.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -828,6 +828,37 @@ describe('FindingFilter', () => {
828828
expect(filtered[0].line).toBe(1);
829829
});
830830

831+
test('keeps SQL interpolation regressions even when phrased cautiously', () => {
832+
const diff = `diff --git a/src/users.js b/src/users.js
833+
@@ -7,7 +7,7 @@ export function normalizeEmail(email) {
834+
}
835+
836+
export async function findUserByEmail(db, email) {
837+
- const rows = await db.query('SELECT * FROM users WHERE email = ? LIMIT 1', [email]);
838+
+ const rows = await db.query(\`SELECT * FROM users WHERE email = '\${email}' LIMIT 1\`);
839+
return rows[0] || null;
840+
}`;
841+
842+
const findings: Finding[] = [
843+
{
844+
file: 'src/users.js',
845+
line: 10,
846+
severity: 'major',
847+
title: 'Email is interpolated into SQL',
848+
message:
849+
'The user-controlled email is directly interpolated into the SQL query, so a crafted value can alter the WHERE clause. Keep using a parameterized query.',
850+
suggestion: "const rows = await db.query('SELECT * FROM users WHERE email = ? LIMIT 1', [email]);",
851+
},
852+
];
853+
854+
const { findings: filtered, stats } = filter.filter(findings, diff);
855+
856+
expect(filtered).toHaveLength(1);
857+
expect(filtered[0].title).toBe('Email is interpolated into SQL');
858+
expect(stats.kept).toBe(1);
859+
expect(stats.filtered).toBe(0);
860+
});
861+
831862
test('filters generic findings with line:1 that mention "entire file" or "class lacks"', () => {
832863
const findings: Finding[] = [
833864
{

dist/index.js

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23033,7 +23033,10 @@ var FindingFilter = class {
2303323033
}
2303423034
isTrueSecurityIssue(finding) {
2303523035
const text = (finding.title + " " + finding.message).toLowerCase();
23036-
return text.includes("sql injection") || text.includes("xss") || text.includes("cross-site scripting") || text.includes("command injection") || text.includes("path traversal") || text.includes("remote code execution") || text.includes("arbitrary code") || text.includes("prototype pollution") || text.includes("wrong throttle") || text.includes("wrong throttling") || text.includes("rate limit bypass") || text.includes("password reset") && (text.includes("rate limit") || text.includes("throttl") || text.includes("lockout")) || (text.includes("authorization") || text.includes("permission") || text.includes("privilege") || text.includes("access control")) && (text.includes("bypass") || text.includes("unauthorized") || text.includes("allows"));
23036+
const mentionsSqlSink = /\b(sql|query|database)\b/.test(text) || text.includes("db.query");
23037+
const mentionsUntrustedInput = /\b(user input|untrusted|attacker|crafted|request|email|parameter|input)\b/.test(text);
23038+
const mentionsUnsafeSqlConstruction = text.includes("interpolat") || text.includes("concatenat") || text.includes("template literal") || text.includes("raw sql") || text.includes("unescaped") || text.includes("unsanitized") || text.includes("directly") || text.includes("not parameterized") || text.includes("parameterized query") || text.includes("${");
23039+
return text.includes("sql injection") || mentionsSqlSink && text.includes("injection") || mentionsSqlSink && mentionsUntrustedInput && mentionsUnsafeSqlConstruction || text.includes("xss") || text.includes("cross-site scripting") || text.includes("command injection") || text.includes("path traversal") || text.includes("remote code execution") || text.includes("arbitrary code") || text.includes("prototype pollution") || text.includes("wrong throttle") || text.includes("wrong throttling") || text.includes("rate limit bypass") || text.includes("password reset") && (text.includes("rate limit") || text.includes("throttl") || text.includes("lockout")) || (text.includes("authorization") || text.includes("permission") || text.includes("privilege") || text.includes("access control")) && (text.includes("bypass") || text.includes("unauthorized") || text.includes("allows"));
2303723040
}
2303823041
isTestCodeQualityIssue(finding) {
2303923042
const text = (finding.title + " " + finding.message).toLowerCase();

dist/index.js.map

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/analysis/finding-filter.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,8 +326,25 @@ export class FindingFilter {
326326

327327
private isTrueSecurityIssue(finding: Finding): boolean {
328328
const text = (finding.title + ' ' + finding.message).toLowerCase();
329+
const mentionsSqlSink = /\b(sql|query|database)\b/.test(text) || text.includes('db.query');
330+
const mentionsUntrustedInput =
331+
/\b(user input|untrusted|attacker|crafted|request|email|parameter|input)\b/.test(text);
332+
const mentionsUnsafeSqlConstruction =
333+
text.includes('interpolat') ||
334+
text.includes('concatenat') ||
335+
text.includes('template literal') ||
336+
text.includes('raw sql') ||
337+
text.includes('unescaped') ||
338+
text.includes('unsanitized') ||
339+
text.includes('directly') ||
340+
text.includes('not parameterized') ||
341+
text.includes('parameterized query') ||
342+
text.includes('${');
343+
329344
return (
330345
text.includes('sql injection') ||
346+
(mentionsSqlSink && text.includes('injection')) ||
347+
(mentionsSqlSink && mentionsUntrustedInput && mentionsUnsafeSqlConstruction) ||
331348
text.includes('xss') ||
332349
text.includes('cross-site scripting') ||
333350
text.includes('command injection') ||

0 commit comments

Comments
 (0)