Skip to content

Commit 245d856

Browse files
aksOpsclaude
aksOps
and
claude
committed
Fix all code review bugs: security, thread safety, correctness, and cleanup
Critical: - C1: Block Cypher injection in McpTools.runCypher() — reject mutating queries - C2: Add synchronized to all AnalysisCache methods for H2 thread safety - C3: Remove dead cachePath/standardCachePath variables in McpTools.loadCacheData() Important: - I1: Validate EdgeKind enum in EnrichCommand before Neo4j relationship creation - I2: Add TODO for QueryService.getStats() full-graph load (needs Cypher aggregation) - I3: Wrap unmodifiable list in QueryService.egoGraph() with new ArrayList<>() - I4: Pre-compile glob exclude patterns once, reuse for all files in Analyzer - I5: Add PARSER.remove() after JavaParser parse to clean ThreadLocal - I6: Fix analyzeCosdebase typo, pass incremental param through to analyzer - I7: Remove redundant shutdown hook in Neo4jConfig (Spring destroyMethod suffices) Minor: - S1: Replace SQL-injectable countTable(String) with specific count methods - S2: Remove duplicate source/javadoc plugins from default build (keep in release) - S3: Fix greedy command detection — check only first non-flag arg - S5: Escape all regex special chars in glob-to-regex conversion - S6: Add missing "method" property in SpringRestDetector AST path - S7: Add static Map for O(1) NodeKind/EdgeKind.fromValue() lookup All 1227 tests pass. E2E verified on nest project. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 1ac7acd commit 245d856

13 files changed

Lines changed: 144 additions & 114 deletions

File tree

‎pom.xml‎

Lines changed: 3 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -301,37 +301,6 @@
301301
</configuration>
302302
</plugin>
303303

304-
<!-- Source JAR (required by Maven Central) -->
305-
<plugin>
306-
<groupId>org.apache.maven.plugins</groupId>
307-
<artifactId>maven-source-plugin</artifactId>
308-
<executions>
309-
<execution>
310-
<id>attach-sources</id>
311-
<goals>
312-
<goal>jar-no-fork</goal>
313-
</goals>
314-
</execution>
315-
</executions>
316-
</plugin>
317-
318-
<!-- Javadoc JAR (required by Maven Central) -->
319-
<plugin>
320-
<groupId>org.apache.maven.plugins</groupId>
321-
<artifactId>maven-javadoc-plugin</artifactId>
322-
<configuration>
323-
<doclint>none</doclint>
324-
</configuration>
325-
<executions>
326-
<execution>
327-
<id>attach-javadocs</id>
328-
<goals>
329-
<goal>jar</goal>
330-
</goals>
331-
</execution>
332-
</executions>
333-
</plugin>
334-
335304
</plugins>
336305
</build>
337306

@@ -366,6 +335,9 @@
366335
<plugin>
367336
<groupId>org.apache.maven.plugins</groupId>
368337
<artifactId>maven-javadoc-plugin</artifactId>
338+
<configuration>
339+
<doclint>none</doclint>
340+
</configuration>
369341
<executions>
370342
<execution>
371343
<id>attach-javadocs</id>

‎src/main/java/io/github/randomcodespace/iq/CodeIqApplication.java‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,14 @@ public static void main(String[] args) {
4848
var app = new SpringApplication(CodeIqApplication.class);
4949
app.setBannerMode(org.springframework.boot.Banner.Mode.OFF);
5050

51-
// Detect command from arguments
52-
boolean isServe = Arrays.stream(args)
53-
.anyMatch(arg -> "serve".equalsIgnoreCase(arg));
54-
boolean isIndex = Arrays.stream(args)
55-
.anyMatch(arg -> "index".equalsIgnoreCase(arg));
56-
boolean isEnrich = Arrays.stream(args)
57-
.anyMatch(arg -> "enrich".equalsIgnoreCase(arg));
51+
// Detect command from first non-flag argument only
52+
String command = Arrays.stream(args)
53+
.filter(arg -> !arg.startsWith("-"))
54+
.findFirst()
55+
.orElse("");
56+
boolean isServe = "serve".equalsIgnoreCase(command);
57+
boolean isIndex = "index".equalsIgnoreCase(command);
58+
boolean isEnrich = "enrich".equalsIgnoreCase(command);
5859

5960
if (isServe) {
6061
app.setAdditionalProfiles("serving");

‎src/main/java/io/github/randomcodespace/iq/analyzer/Analyzer.java‎

Lines changed: 34 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -187,8 +187,9 @@ private AnalysisResult runWithCache(Path root, Integer parallelism, AnalysisCach
187187
// Apply exclude patterns from project config
188188
if (projectConfig.hasExcludePatterns()) {
189189
List<String> excludes = projectConfig.getExclude();
190+
List<java.util.regex.Pattern> compiledExcludes = compileExcludePatterns(excludes);
190191
files = files.stream()
191-
.filter(f -> !matchesAnyExclude(f.path().toString(), excludes))
192+
.filter(f -> !matchesAnyCompiledExclude(f.path().toString(), compiledExcludes))
192193
.toList();
193194
report.accept("Exclude patterns: " + excludes);
194195
}
@@ -422,8 +423,9 @@ private AnalysisResult runBatchedWithCache(Path root, Integer parallelism, int b
422423
}
423424
if (projectConfig.hasExcludePatterns()) {
424425
List<String> excludes = projectConfig.getExclude();
426+
List<java.util.regex.Pattern> compiledExcludes = compileExcludePatterns(excludes);
425427
files = files.stream()
426-
.filter(f -> !matchesAnyExclude(f.path().toString(), excludes))
428+
.filter(f -> !matchesAnyCompiledExclude(f.path().toString(), compiledExcludes))
427429
.toList();
428430
report.accept("Exclude patterns: " + excludes);
429431
}
@@ -740,27 +742,44 @@ private String getGitHead(Path repoPath) {
740742
}
741743

742744
/**
743-
* Check whether a file path matches any of the given exclude patterns.
744-
* Supports simple glob-like patterns: '*' matches any sequence of chars,
745-
* '**' matches across directory separators.
745+
* Pre-compile exclude glob patterns into regex Pattern objects.
746+
*/
747+
private static List<java.util.regex.Pattern> compileExcludePatterns(List<String> excludePatterns) {
748+
if (excludePatterns == null) return List.of();
749+
return excludePatterns.stream()
750+
.map(p -> compileGlob(p.replace('\\', '/')))
751+
.toList();
752+
}
753+
754+
/**
755+
* Check whether a file path matches any of the given pre-compiled exclude patterns.
746756
*/
747757
private static boolean matchesAnyExclude(String filePath, List<String> excludePatterns) {
748758
if (excludePatterns == null) return false;
759+
List<java.util.regex.Pattern> compiled = compileExcludePatterns(excludePatterns);
760+
return matchesAnyCompiledExclude(filePath, compiled);
761+
}
762+
763+
/**
764+
* Check whether a file path matches any of the given pre-compiled patterns.
765+
*/
766+
private static boolean matchesAnyCompiledExclude(String filePath, List<java.util.regex.Pattern> compiledPatterns) {
767+
if (compiledPatterns == null || compiledPatterns.isEmpty()) return false;
749768
String normalized = filePath.replace('\\', '/');
750-
for (String pattern : excludePatterns) {
751-
if (matchGlob(normalized, pattern.replace('\\', '/'))) {
769+
for (java.util.regex.Pattern pattern : compiledPatterns) {
770+
if (pattern.matcher(normalized).matches()) {
752771
return true;
753772
}
754773
}
755774
return false;
756775
}
757776

758777
/**
759-
* Simple glob matching: '*' matches any non-separator sequence,
760-
* '**' matches everything (including separators).
778+
* Compile a glob pattern into a regex Pattern.
779+
* '*' matches any non-separator sequence, '**' matches everything (including separators).
780+
* All regex special characters are properly escaped.
761781
*/
762-
private static boolean matchGlob(String text, String pattern) {
763-
// Convert glob to regex
782+
private static java.util.regex.Pattern compileGlob(String pattern) {
764783
StringBuilder regex = new StringBuilder("^");
765784
int i = 0;
766785
while (i < pattern.length()) {
@@ -780,15 +799,16 @@ private static boolean matchGlob(String text, String pattern) {
780799
} else if (c == '?') {
781800
regex.append("[^/]");
782801
i++;
783-
} else if (c == '.') {
784-
regex.append("\\.");
802+
} else if (".+^${}()|[]\\".indexOf(c) >= 0) {
803+
// S5: Properly escape all regex special characters
804+
regex.append('\\').append(c);
785805
i++;
786806
} else {
787807
regex.append(c);
788808
i++;
789809
}
790810
}
791811
regex.append("$");
792-
return text.matches(regex.toString());
812+
return java.util.regex.Pattern.compile(regex.toString());
793813
}
794814
}

‎src/main/java/io/github/randomcodespace/iq/cache/AnalysisCache.java‎

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ private void initDb() throws SQLException {
121121
/**
122122
* Return the commit SHA from the most recent analysis run, or null.
123123
*/
124-
public String getLastCommit() {
124+
public synchronized String getLastCommit() {
125125
try (var stmt = conn.prepareStatement(
126126
"SELECT commit_sha FROM analysis_runs ORDER BY timestamp DESC LIMIT 1")) {
127127
ResultSet rs = stmt.executeQuery();
@@ -139,7 +139,7 @@ public String getLastCommit() {
139139
/**
140140
* Check whether results for the given content hash are cached.
141141
*/
142-
public boolean isCached(String contentHash) {
142+
public synchronized boolean isCached(String contentHash) {
143143
try (var stmt = conn.prepareStatement(
144144
"SELECT 1 FROM files WHERE content_hash = ?")) {
145145
stmt.setString(1, contentHash);
@@ -155,7 +155,7 @@ public boolean isCached(String contentHash) {
155155
/**
156156
* Persist analysis results for a single file.
157157
*/
158-
public void storeResults(String contentHash, String filePath, String language,
158+
public synchronized void storeResults(String contentHash, String filePath, String language,
159159
List<CodeNode> nodes, List<CodeEdge> edges) {
160160
try {
161161
conn.setAutoCommit(false);
@@ -230,7 +230,7 @@ public void storeResults(String contentHash, String filePath, String language,
230230
*
231231
* @return a CachedResult with the nodes and edges, or null if not cached
232232
*/
233-
public CachedResult loadCachedResults(String contentHash) {
233+
public synchronized CachedResult loadCachedResults(String contentHash) {
234234
try {
235235
List<CodeNode> nodes = new ArrayList<>();
236236
try (var stmt = conn.prepareStatement("SELECT data FROM nodes WHERE content_hash = ?")) {
@@ -265,7 +265,7 @@ public CachedResult loadCachedResults(String contentHash) {
265265
/**
266266
* Delete all cached results associated with a content hash.
267267
*/
268-
public void removeFile(String contentHash) {
268+
public synchronized void removeFile(String contentHash) {
269269
try {
270270
conn.setAutoCommit(false);
271271
try (var stmt = conn.prepareStatement("DELETE FROM nodes WHERE content_hash = ?")) {
@@ -300,7 +300,7 @@ public void removeFile(String contentHash) {
300300
/**
301301
* Record an analysis run with its commit SHA and file count.
302302
*/
303-
public void recordRun(String commitSha, int fileCount) {
303+
public synchronized void recordRun(String commitSha, int fileCount) {
304304
try (var stmt = conn.prepareStatement(
305305
"INSERT INTO analysis_runs (run_id, commit_sha, timestamp, file_count) VALUES (?, ?, ?, ?)")) {
306306
stmt.setString(1, UUID.randomUUID().toString());
@@ -318,13 +318,13 @@ public void recordRun(String commitSha, int fileCount) {
318318
/**
319319
* Return cache statistics.
320320
*/
321-
public Map<String, Object> getStats() {
321+
public synchronized Map<String, Object> getStats() {
322322
Map<String, Object> stats = new LinkedHashMap<>();
323323
try {
324-
stats.put("cached_files", countTable("files"));
324+
stats.put("cached_files", countFiles());
325325
stats.put("cached_nodes", getNodeCount());
326-
stats.put("cached_edges", countTable("edges"));
327-
stats.put("total_runs", countTable("analysis_runs"));
326+
stats.put("cached_edges", countEdges());
327+
stats.put("total_runs", countAnalysisRuns());
328328
stats.put("db_path", dbPath.toString());
329329
} catch (SQLException e) {
330330
stats.put("error", e.getMessage());
@@ -335,7 +335,7 @@ public Map<String, Object> getStats() {
335335
/**
336336
* Clear all cached data.
337337
*/
338-
public void clear() {
338+
public synchronized void clear() {
339339
try (var stmt = conn.createStatement()) {
340340
stmt.execute("DELETE FROM edges");
341341
stmt.execute("DELETE FROM nodes");
@@ -456,9 +456,25 @@ private CodeEdge deserializeEdge(String json) {
456456
}
457457
}
458458

459-
private long countTable(String table) throws SQLException {
459+
private long countFiles() throws SQLException {
460460
try (var stmt = conn.createStatement()) {
461-
ResultSet rs = stmt.executeQuery("SELECT COUNT(*) FROM " + table);
461+
ResultSet rs = stmt.executeQuery("SELECT COUNT(*) FROM files");
462+
rs.next();
463+
return rs.getLong(1);
464+
}
465+
}
466+
467+
private long countEdges() throws SQLException {
468+
try (var stmt = conn.createStatement()) {
469+
ResultSet rs = stmt.executeQuery("SELECT COUNT(*) FROM edges");
470+
rs.next();
471+
return rs.getLong(1);
472+
}
473+
}
474+
475+
private long countAnalysisRuns() throws SQLException {
476+
try (var stmt = conn.createStatement()) {
477+
ResultSet rs = stmt.executeQuery("SELECT COUNT(*) FROM analysis_runs");
462478
rs.next();
463479
return rs.getLong(1);
464480
}
@@ -467,7 +483,7 @@ private long countTable(String table) throws SQLException {
467483
/**
468484
* Return the total number of cached nodes.
469485
*/
470-
public long getNodeCount() {
486+
public synchronized long getNodeCount() {
471487
try (var stmt = conn.createStatement()) {
472488
ResultSet rs = stmt.executeQuery("SELECT COUNT(DISTINCT id) FROM nodes");
473489
rs.next();
@@ -481,9 +497,9 @@ public long getNodeCount() {
481497
/**
482498
* Return the total number of cached edges.
483499
*/
484-
public long getEdgeCount() {
500+
public synchronized long getEdgeCount() {
485501
try {
486-
return countTable("edges");
502+
return countEdges();
487503
} catch (SQLException e) {
488504
log.debug("Failed to count edges", e);
489505
return 0;
@@ -511,7 +527,7 @@ public void storeBatchResults(String batchId, String filePath, String language,
511527
*
512528
* @return list of all cached nodes
513529
*/
514-
public List<CodeNode> loadAllNodes() {
530+
public synchronized List<CodeNode> loadAllNodes() {
515531
List<CodeNode> nodes = new ArrayList<>();
516532
// Use MIN(data) with GROUP BY id to deduplicate nodes that appear in multiple files
517533
try (var stmt = conn.prepareStatement("SELECT MIN(data) FROM nodes GROUP BY id")) {
@@ -530,7 +546,7 @@ public List<CodeNode> loadAllNodes() {
530546
*
531547
* @return list of all cached edges
532548
*/
533-
public List<CodeEdge> loadAllEdges() {
549+
public synchronized List<CodeEdge> loadAllEdges() {
534550
List<CodeEdge> edges = new ArrayList<>();
535551
try (var stmt = conn.prepareStatement("SELECT data FROM edges")) {
536552
ResultSet rs = stmt.executeQuery();

‎src/main/java/io/github/randomcodespace/iq/cli/EnrichCommand.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import io.github.randomcodespace.iq.config.CodeIqConfig;
88
import io.github.randomcodespace.iq.model.CodeEdge;
99
import io.github.randomcodespace.iq.model.CodeNode;
10+
import io.github.randomcodespace.iq.model.EdgeKind;
1011
import org.neo4j.dbms.api.DatabaseManagementService;
1112
import org.neo4j.dbms.api.DatabaseManagementServiceBuilder;
1213
import org.neo4j.graphdb.GraphDatabaseService;
@@ -216,9 +217,17 @@ private int enrichFromCache(AnalysisCache cache, Path root, NumberFormat nf, Ins
216217
String targetId = edge.getTarget() != null ? edge.getTarget().getId() : null;
217218
if (sourceId == null || targetId == null) continue;
218219

220+
// Validate edge kind comes from EdgeKind enum
221+
String edgeKindValue = edge.getKind().getValue();
222+
try {
223+
EdgeKind.fromValue(edgeKindValue);
224+
} catch (IllegalArgumentException ex) {
225+
log.warn("Skipping edge with unknown kind: {}", edgeKindValue);
226+
continue;
227+
}
219228
var result = tx.execute(
220229
"MATCH (s:CodeNode {id: $sourceId}), (t:CodeNode {id: $targetId}) "
221-
+ "CREATE (s)-[r:" + sanitizeRelType(edge.getKind().getValue())
230+
+ "CREATE (s)-[r:" + sanitizeRelType(edgeKindValue)
222231
+ " {id: $edgeId}]->(t)",
223232
java.util.Map.of(
224233
"sourceId", sourceId,

‎src/main/java/io/github/randomcodespace/iq/config/Neo4jConfig.java‎

Lines changed: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,16 +28,7 @@ public class Neo4jConfig {
2828
@Bean(destroyMethod = "shutdown")
2929
DatabaseManagementService databaseManagementService(
3030
@Value("${codeiq.graph.path:.osscodeiq/graph.db}") String dbPath) {
31-
DatabaseManagementService dbms = new DatabaseManagementServiceBuilder(Path.of(dbPath)).build();
32-
// Ensure clean shutdown even if Spring context is not closed gracefully
33-
Runtime.getRuntime().addShutdownHook(new Thread(() -> {
34-
try {
35-
dbms.shutdown();
36-
} catch (Exception ignored) {
37-
// Already shut down by Spring's destroyMethod, or JVM is exiting
38-
}
39-
}, "neo4j-shutdown-hook"));
40-
return dbms;
31+
return new DatabaseManagementServiceBuilder(Path.of(dbPath)).build();
4132
}
4233

4334
@Bean

‎src/main/java/io/github/randomcodespace/iq/detector/java/AbstractJavaParserDetector.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ protected Optional<CompilationUnit> parse(DetectorContext ctx) {
2929
// JavaParser may throw AssertionError for unrecognized token kinds
3030
// (e.g. newer Java syntax). Fall back to regex in those cases.
3131
return Optional.empty();
32+
} finally {
33+
PARSER.remove();
3234
}
3335
}
3436

‎src/main/java/io/github/randomcodespace/iq/detector/java/SpringRestDetector.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ private DetectorResult detectWithAst(CompilationUnit cu, DetectorContext ctx) {
139139
node.getAnnotations().add("@" + annName);
140140
node.getProperties().put("http_method", httpMethod);
141141
node.getProperties().put("path", fullPath);
142+
node.getProperties().put("method", methodName);
142143
if (produces != null) node.getProperties().put("produces", produces);
143144
if (consumes != null) node.getProperties().put("consumes", consumes);
144145

0 commit comments

Comments
 (0)