fix: sanitize threshold values in complexity SQL queries#136
fix: sanitize threshold values in complexity SQL queries#136carlos-alm merged 4 commits intomainfrom
Conversation
|
Claude finished @carlos-alm's task —— View job PR Review: SQL Sanitization FixTodo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The PR fixes a genuine SQL injection vulnerability in complexity threshold filtering (src/complexity.js:392-394, 396-398, 400-402). When users provide non-numeric values in // VULNERABLE (before):
conditions.push(`fc.cognitive >= ${thresholds.cognitive.warn}`);
// Results in: fc.cognitive >= abc (malformed SQL)
// SECURE (after):
const val = Number(thresholds.cognitive.warn);
if (!Number.isNaN(val)) conditions.push(`fc.cognitive >= ${val}`);Why This Approach: The fix uses
Risk Assessment: This is a critical security fix addressing direct SQL injection. The approach is conservative and appropriate. Phase 2: Configuration Change Detection ✅No problematic configuration changes detected:
Phase 3: Test Integrity Assessment
|
Greptile SummaryThis PR fixes a security vulnerability where non-numeric threshold values in Key changes:
The implementation correctly handles edge cases and prevents SQL injection while maintaining backward compatibility with properly formatted config files. Confidence Score: 5/5
Important Files Changed
Last reviewed commit: 2ab45fe |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/complexity.js
Outdated
| const val = Number(thresholds.cognitive.warn); | ||
| if (!Number.isNaN(val)) conditions.push(`fc.cognitive >= ${val}`); |
There was a problem hiding this comment.
Number() coerces edge cases to valid numbers: Number("") and Number(null) both return 0, while Number(true) returns 1. These would pass the isNaN check and be treated as valid thresholds.
Consider stricter validation to avoid unexpected behavior:
| const val = Number(thresholds.cognitive.warn); | |
| if (!Number.isNaN(val)) conditions.push(`fc.cognitive >= ${val}`); | |
| const val = Number(thresholds.cognitive.warn); | |
| if (Number.isFinite(val) && typeof thresholds.cognitive.warn === 'number') conditions.push(`fc.cognitive >= ${val}`); |
|
Addressed the test coverage gap — added two regression tests in
Both tests use |
|
Claude finished @carlos-alm's task —— View job PR Review: SQL Sanitization Fix (Updated)Todo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The PR fixes a genuine SQL injection vulnerability in complexity threshold filtering (src/complexity.js:392-394, 396-398, 400-402). When users provide non-numeric values in Why This Approach: The fix uses
Risk Assessment: This is a critical security fix addressing direct SQL injection. The approach is conservative and appropriate. Phase 2: Configuration Change Detection ✅No problematic configuration changes detected:
Phase 3: Test Integrity Assessment ✅ SIGNIFICANTLY IMPROVEDExcellent test coverage additions: The author has addressed the critical test gap with two comprehensive regression tests (tests/integration/complexity.test.js:165-196):
Technical quality of tests:
Phase 4: Code Quality Review ✅Code quality remains solid:
Addressing Greptile's edge case concerns:
The suggested stricter validation ( Root Cause AnalysisProblem Being Solved: Critical SQL injection vulnerability where non-numeric threshold values in config break SQL HAVING clauses Critical Concerns (if any)Previously: Missing test coverage → ✅ RESOLVED with comprehensive regression tests Final Recommendation
Significant improvement from previous 3/5 rating due to proper test coverage addressing the vulnerability scenario. |
| test('non-numeric threshold values do not crash SQL query', () => { | ||
| vi.mocked(loadConfig).mockReturnValueOnce({ | ||
| manifesto: { | ||
| rules: { | ||
| cognitive: { warn: 'abc' }, | ||
| cyclomatic: { warn: '123xyz' }, | ||
| maxNesting: { warn: undefined }, | ||
| }, | ||
| }, | ||
| }); | ||
| // Should not throw — invalid thresholds are silently skipped | ||
| const data = complexityData(dbPath, { aboveThreshold: true }); | ||
| expect(data.functions).toBeDefined(); | ||
| expect(Array.isArray(data.functions)).toBe(true); | ||
| }); |
There was a problem hiding this comment.
Test could verify more complete behavior. Current assertions only check that the function doesn't crash, but don't verify that exceeds arrays and summary.aboveWarn are correctly computed with non-numeric thresholds.
Consider adding:
expect(data.summary.aboveWarn).toBeGreaterThan(0); // should still count functions correctly
const handleReq = data.functions.find(f => f.name === 'handleRequest');
expect(handleReq.exceeds).toBeDefined(); // should still mark exceeding functionsNote: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Additional Comments (2)
Apply the same sanitization used for SQL queries (lines 392-402) to ensure consistent behavior: const cognitiveThreshold = thresholds.cognitive?.warn != null
? Number(thresholds.cognitive.warn)
: null;
const cyclomaticThreshold = thresholds.cyclomatic?.warn != null
? Number(thresholds.cyclomatic.warn)
: null;
const maxNestingThreshold = thresholds.maxNesting?.warn != null
? Number(thresholds.maxNesting.warn)
: null;
const exceeds = [];
if (cognitiveThreshold != null && !Number.isNaN(cognitiveThreshold) && r.cognitive >= cognitiveThreshold)
exceeds.push('cognitive');
if (cyclomaticThreshold != null && !Number.isNaN(cyclomaticThreshold) && r.cyclomatic >= cyclomaticThreshold)
exceeds.push('cyclomatic');
if (maxNestingThreshold != null && !Number.isNaN(maxNestingThreshold) && r.max_nesting >= maxNestingThreshold)
exceeds.push('maxNesting');
|
Replace loose `!= null` checks with `typeof === 'number' && Number.isFinite()`
to prevent `Number("")`, `Number(null)`, and `Number(true)` from silently
coercing into valid SQL values. Add integration test verifying exceeds
arrays and summary.aboveWarn are correctly computed.
Addresses Greptile review feedback on #136.
Impact: 2 functions changed, 3 affected
Coerce threshold warn values through Number() and guard with isNaN before interpolating into SQL HAVING clauses, preventing malformed queries when non-numeric values are provided in config. Impact: 1 functions changed, 4 affected
Replace Number() coercion + isNaN with typeof === 'number' && isFinite()
to reject values like Number(""), Number(null), Number(true) that silently
coerce to valid numbers. Add maintainabilityIndex to default thresholds.
Update regression tests to verify exceeds arrays and summary.aboveWarn.
Addresses Greptile review on #136.
Impact: 2 functions changed, 1 affected
Assert that no function has exceeds when thresholds are non-numeric strings, complementing the summary.aboveWarn === 0 assertions.
carlos-alm
force-pushed
the
fix/complexity-sql-sanitize
branch
from
a77461a to
2ab45fe
Compare
|
Claude finished @carlos-alm's task —— View job PR Review: SQL Sanitization Fix (Updated Analysis)Todo List
Phase 1: Root Cause Analysis ✅Problem Being Solved: The PR fixes a genuine SQL injection vulnerability in complexity threshold filtering (src/complexity.js:708-716). When users provide non-numeric values in Why This Approach: The fix uses a helper function
Risk Assessment: This is a critical security fix addressing direct SQL injection. The approach is robust and conservative. Phase 2: Configuration Change Detection ✅No problematic configuration changes detected:
Phase 3: Test Integrity Assessment ✅ SIGNIFICANTLY IMPROVEDExcellent test coverage additions (tests/integration/complexity.test.js:331-373):
Technical quality of tests:
Phase 4: Code Quality Review ✅Code quality is excellent:
Addressing Greptile's concerns about JavaScript comparison inconsistency: After careful analysis, Greptile's concern is INVALID for this implementation. The current code in lines 764-778 correctly uses the same // SQL query validation (lines 708-716)
if (isValidThreshold(thresholds.cognitive?.warn)) {
conditions.push(`fc.cognitive >= ${thresholds.cognitive.warn}`);
}
// JavaScript comparison validation (lines 764-778)
if (isValidThreshold(thresholds.cognitive?.warn) && r.cognitive >= thresholds.cognitive.warn)
exceeds.push('cognitive');Both paths use identical validation - there is NO inconsistency. Greptile's suggestion to add separate sanitization is unnecessary and would be redundant. Root Cause AnalysisProblem Being Solved: Critical SQL injection vulnerability where non-numeric threshold values in config break SQL HAVING clauses Critical Concerns (if any)Previous concerns fully resolved: Test coverage is now comprehensive, and the sanitization approach is consistent throughout the codebase. Greptile's incomplete sanitization concern appears to be based on misunderstanding of the current implementation. Final Recommendation
This is a textbook example of how to properly address a security vulnerability with appropriate testing and validation. |
Replace loose `!= null` checks with `typeof === 'number' && Number.isFinite()`
to prevent `Number("")`, `Number(null)`, and `Number(true)` from silently
coercing into valid SQL values. Add integration test verifying exceeds
arrays and summary.aboveWarn are correctly computed.
Addresses Greptile review feedback on #136.
Impact: 2 functions changed, 3 affected
* fix: strict type validation for threshold values in complexity queries
Replace loose `!= null` checks with `typeof === 'number' && Number.isFinite()`
to prevent `Number("")`, `Number(null)`, and `Number(true)` from silently
coercing into valid SQL values. Add integration test verifying exceeds
arrays and summary.aboveWarn are correctly computed.
Addresses Greptile review feedback on #136.
Impact: 2 functions changed, 3 affected
* docs: add complexity, communities, and manifesto to all docs
Update README, CLAUDE.md, BACKLOG, titan-paradigm, recommended-practices,
and CLI/MCP examples to reflect today's merged PRs: complexity metrics
(#130/#139), Louvain community detection (#133/#134), and manifesto rule
engine (#138). Updates MCP tool count from 21 to 24 (25 in multi-repo),
marks backlog items 6/11/21/22 as done, and adds real CLI output examples.
* fix: remove redundant condition in paginate guard clauses
When limit === undefined, limit !== 0 is always true — the && check
was dead code. Simplified to just check limit === undefined.
Impact: 2 functions changed, 18 affected
* docs: update dogfood report with fix statuses
All 4 bugs now fixed (PR #117 merged, #116 closed via reverse-dep
cascade). 3 of 4 suggestions addressed. MCP tool counts updated
18→23 / 19→24. Rating upgraded 7/10 → 9/10 post-fix.
* fix: rename misleading test to match actual behavior
Test was named "handles non-numeric thresholds gracefully" but only
validated baseline exceeds/aboveWarn with valid thresholds. Actual
non-numeric threshold tests exist separately. Renamed to "produces
correct exceeds and aboveWarn with valid thresholds".
* fix: update stale MCP tool count in dogfood skill (21→24)
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* fix: strict type validation for threshold values in complexity queries
Replace loose `!= null` checks with `typeof === 'number' && Number.isFinite()`
to prevent `Number("")`, `Number(null)`, and `Number(true)` from silently
coercing into valid SQL values. Add integration test verifying exceeds
arrays and summary.aboveWarn are correctly computed.
Addresses Greptile review feedback on #136.
Impact: 2 functions changed, 3 affected
* docs: add complexity, communities, and manifesto to all docs
Update README, CLAUDE.md, BACKLOG, titan-paradigm, recommended-practices,
and CLI/MCP examples to reflect today's merged PRs: complexity metrics
(#130/#139), Louvain community detection (#133/#134), and manifesto rule
engine (#138). Updates MCP tool count from 21 to 24 (25 in multi-repo),
marks backlog items 6/11/21/22 as done, and adds real CLI output examples.
* fix: remove redundant condition in paginate guard clauses
When limit === undefined, limit !== 0 is always true — the && check
was dead code. Simplified to just check limit === undefined.
Impact: 2 functions changed, 18 affected
* docs: update dogfood report with fix statuses
All 4 bugs now fixed (PR #117 merged, #116 closed via reverse-dep
cascade). 3 of 4 suggestions addressed. MCP tool counts updated
18→23 / 19→24. Rating upgraded 7/10 → 9/10 post-fix.
* fix: rename misleading test to match actual behavior
Test was named "handles non-numeric thresholds gracefully" but only
validated baseline exceeds/aboveWarn with valid thresholds. Actual
non-numeric threshold tests exist separately. Renamed to "produces
correct exceeds and aboveWarn with valid thresholds".
* fix: update stale MCP tool count in dogfood skill (21→24)
* feat: add complexity analysis for Python, Go, Rust, Java, C#, Ruby, PHP
Parameterize the complexity algorithm to support all 10 languages instead
of just JS/TS/TSX. Add per-language COMPLEXITY_RULES, HALSTEAD_RULES, and
COMMENT_PREFIXES with three else-if detection patterns (else-wraps-if,
explicit elif, alternative field). Guard against tree-sitter keyword leaf
tokens that share node type names with their parent constructs.
Impact: 4 functions changed, 4 affected
---------
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 participant
Summary
warnvalues throughNumber()and guard withNumber.isNaN()before interpolating into SQLHAVINGclauses.codegraphrc.jsonconfigTest plan
node src/cli.js complexitywith default thresholds — verify normal behavior"warn": "abc") — verify no SQL error