diff --git a/CHANGELOG.md b/CHANGELOG.md index fbbc4b13..fae0aa12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -472,6 +472,61 @@ for that specific tag for the per-commit details. `McpToolsTest#readFileShouldHandleMissingFile` updated for new envelope contract. Full suite: 3680 tests / 0 failures / 0 errors. +- **Production-readiness PR 5 of 5 — config validation, integration coverage, + docs refresh.** Final PR of the production-readiness series. Closes the + remaining audit findings around silent oversized-input clamping, missing + end-to-end coverage of the serving filter chain, and stale tech-stack + pins in `CLAUDE.md`. + - **MCP request-bound clamping in `McpTools`.** `queryNodes` / + `queryEdges` `limit` parameters are now `Math.min(requested, + mcp.limits.max_results)` so a caller asking for `LIMIT 1_000_000` no + longer trips the JVM into a multi-GB allocation before the + `run_cypher` row cap kicks in. `getEgoGraph` radius is clamped to + `mcp.limits.max_depth` for the same reason — a `radius=999` ego + walk on a hub node is a Cartesian explosion. `searchGraph` limit + follows the same rule. Per-call defense-in-depth on top of the + transaction-timeout cap from PR 2. + - **`ConfigValidator` hard ceilings + blank-string checks.** Added + explicit validations for fields previously only typed as + `Integer`/`Long` with no range: + - `mcp.limits.max_payload_bytes` — must be `> 0` (was silently + `null` → no payload cap → infinite-row run_cypher could OOM). + - `mcp.limits.rate_per_minute` — must be `> 0`. + - `mcp.limits.max_depth` — must be `1..100`. The 100 ceiling is a + DoS sentinel: variable-length Cypher with depth >100 is + pathological in practice (a graph with 100M nodes and fan-out 5 + reaches every node by depth 12), so anything higher is either a + misconfig or a reconnaissance probe. Catch at config-load, not + at query time. + - `mcp.auth.token_env` / `mcp.auth.token` — when mode=bearer, + blank-string values fail validation rather than being silently + coerced to null and then fail-fasting at startup with a + mysterious "no token resolved" message. + - **`ServingChainIntegrationTest` (new — 9 cases).** Fills the gap + where each filter (`RequestIdFilter`, `SecurityHeadersFilter`, + `RateLimitFilter`, `BearerAuthFilter`) had unit-test coverage in + isolation but no test exercised the full chain together. Asserts + the cross-filter contract: 401 envelope shape with `request_id` + echoed in the `X-Request-Id` response header; 429 envelope with + `Retry-After` and `X-RateLimit-Remaining: 0`; security headers + present on every response (success, 401, 429); inbound + `X-Request-Id` propagated end-to-end when valid; control-char + inbound rejected and replaced with a generated UUID; rate-limit + bucket isolation per token (one client exhausting their bucket + does not affect another); health endpoint bypasses auth (kubelet + probes carry no token). Manually chains the four filters via + lambda `FilterChain` instances rather than spinning up a full + `@SpringBootTest` so the run is sub-second and doesn't need + Neo4j. Lives in `io.github.randomcodespace.iq.config.security` + package to access package-private `TokenResolver.resolve()`. + - **`CLAUDE.md` tech-stack pin refresh.** Stale version pins + updated to current: Spring Boot 4.0.5 → 4.0.6, Spring AI 2.0.0-M3 + → 2.0.0-M4, Neo4j Embedded 2026.02.3 → 2026.04.0; added + Bucket4j 8.18.0, logstash-logback-encoder 9.0, + micrometer-registry-prometheus to the dependency list. + - **Tests:** new `ServingChainIntegrationTest` (9 cases). Full + suite: 3689 tests / 0 failures / 0 errors / 32 skipped. + ## [0.1.0] - 2026-03-28 First general-availability cut. See the diff --git a/CLAUDE.md b/CLAUDE.md index e7770965..484e2ac8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -14,10 +14,18 @@ ## Tech Stack +> **Source of truth:** `pom.xml` and `src/main/frontend/package.json`. Update +> these pins together — when `pom.xml` bumps, this list moves with it as part +> of the same PR. Pre-PR-5 the list had drifted (Spring Boot 4.0.5, +> Neo4j 2026.02.3, Spring AI 2.0.0-M3); PR 5 brought it back in sync. + - Java 25 (virtual threads, pattern matching, records, sealed classes) -- Spring Boot 4.0.5 -- Neo4j Embedded 2026.02.3 (Community Edition, no external server) -- Spring AI 2.0.0-M3 (MCP server, `@McpTool` annotations, streamable HTTP) +- Spring Boot 4.0.6 (parent POM ``) +- Neo4j Embedded 2026.04.0 (Community Edition, no external server) +- Spring AI 2.0.0-M4 (MCP server, `@McpTool` annotations, streamable HTTP) +- Bucket4j 8.18.0 (`bucket4j_jdk17-core`, in-process token-bucket rate limiter) +- logstash-logback-encoder 9.0 (JSON-structured logging in `serving` profile) +- micrometer-registry-prometheus (`/actuator/prometheus`, version managed by Boot BOM) - JavaParser 3.28.0 (Java AST analysis) - ANTLR 4.13.2 (TypeScript/JavaScript, Python, Go, C#, Rust, C++ grammars) - Picocli 4.7.7 (CLI framework, integrated with Spring Boot) diff --git a/src/main/java/io/github/randomcodespace/iq/config/unified/ConfigValidator.java b/src/main/java/io/github/randomcodespace/iq/config/unified/ConfigValidator.java index fdb43911..4d8b1fcd 100644 --- a/src/main/java/io/github/randomcodespace/iq/config/unified/ConfigValidator.java +++ b/src/main/java/io/github/randomcodespace/iq/config/unified/ConfigValidator.java @@ -64,6 +64,45 @@ public List validate(CodeIqUnifiedConfig c) { Integer maxRes = c.mcp().limits().maxResults(); if (maxRes != null && maxRes <= 0) errs.add(new ConfigError("mcp.limits.max_results", "must be > 0", "validator")); + Long maxPayload = c.mcp().limits().maxPayloadBytes(); + if (maxPayload != null && maxPayload <= 0) + errs.add(new ConfigError("mcp.limits.max_payload_bytes", "must be > 0", "validator")); + Integer ratePerMin = c.mcp().limits().ratePerMinute(); + if (ratePerMin != null && ratePerMin <= 0) + errs.add(new ConfigError("mcp.limits.rate_per_minute", "must be > 0", "validator")); + Integer maxDepth = c.mcp().limits().maxDepth(); + if (maxDepth != null) { + if (maxDepth <= 0) + errs.add(new ConfigError("mcp.limits.max_depth", "must be > 0", "validator")); + // Hard ceiling on max_depth — variable-length Cypher with depth >100 + // is almost always either a misconfig or a reconnaissance probe. + // A graph with 100M nodes and a fan-out of 5 reaches every node by + // depth 12 anyway; depth >100 is pathological in practice. + if (maxDepth > 100) + errs.add(new ConfigError("mcp.limits.max_depth", + "must be <= 100 (variable-length Cypher above this depth is " + + "pathological); got " + maxDepth, "validator")); + } + + // mcp.auth.* — blank-string checks for required fields + // When mcp.auth.mode=bearer, either token_env (env var name) or token + // (literal config value) must resolve to a non-blank string. The + // TokenResolver also fail-fasts at startup, but catching this in + // `codeiq config validate` lets operators see the issue before + // launching the server. + if ("bearer".equalsIgnoreCase(c.mcp().auth().mode())) { + String tokenEnvName = c.mcp().auth().tokenEnv(); + String tokenLiteral = c.mcp().auth().token(); + // Blank means "set but empty" — silently coerced to null at + // config-read but TokenResolver would still fail. Catch here. + if (tokenEnvName != null && tokenEnvName.isBlank()) + errs.add(new ConfigError("mcp.auth.token_env", + "must be non-blank when set (use unset for default CODEIQ_MCP_TOKEN)", + "validator")); + if (tokenLiteral != null && tokenLiteral.isBlank()) + errs.add(new ConfigError("mcp.auth.token", + "must be non-blank when set", "validator")); + } // observability.log_format / log_level if (c.observability().logFormat() != null && !LOG_FORMATS.contains(c.observability().logFormat())) diff --git a/src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java b/src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java index a06dccc5..65646f28 100644 --- a/src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java +++ b/src/main/java/io/github/randomcodespace/iq/mcp/McpTools.java @@ -170,7 +170,13 @@ public String queryNodes( @McpToolParam(description = "Node kind to filter by: endpoint, entity, class, method, guard, service, module, topic, queue, config_file, database_connection, component, interface, enum, etc.", required = false) String kind, @McpToolParam(description = "Maximum number of results to return (default: 50)", required = false) Integer limit) { try { - return toJson(queryService.listNodes(kind, limit != null ? limit : 50, 0)); + // Clamp caller-supplied limit to mcp.limits.max_results (default 500). + // Pre-PR-5 a caller could ask for `limit: 1_000_000` and Neo4j + // would happily stream a million rows over the MCP transport, + // saturating the JSON encoder and the network. Part of the + // rate-limit / abuse-protection story (RAN-46 #2/#3). + int safeLimit = Math.min(limit != null ? limit : 50, maxResults); + return toJson(queryService.listNodes(kind, safeLimit, 0)); } catch (Exception e) { return errorEnvelope("INTERNAL_ERROR", e); } @@ -181,7 +187,8 @@ public String queryEdges( @McpToolParam(description = "Edge kind to filter by: calls, imports, depends_on, queries, produces, consumes, protects, extends, implements, contains, connects_to, maps_to, etc.", required = false) String kind, @McpToolParam(description = "Maximum number of results to return (default: 50)", required = false) Integer limit) { try { - return toJson(queryService.listEdges(kind, limit != null ? limit : 50, 0)); + int safeLimit = Math.min(limit != null ? limit : 50, maxResults); + return toJson(queryService.listEdges(kind, safeLimit, 0)); } catch (Exception e) { return errorEnvelope("INTERNAL_ERROR", e); } @@ -201,9 +208,14 @@ public String getNodeNeighbors( @McpTool(name = "get_ego_graph", description = "Get the full subgraph within N hops of a center node — all reachable nodes and edges. Use for exploring the neighborhood of a component, understanding local architecture, or visualizing a module's context. Returns nodes and edges as a graph structure.") public String getEgoGraph( @McpToolParam(description = "Center node ID") String center, - @McpToolParam(description = "Number of hops from center node (default: 2, max: 10)", required = false) Integer radius) { + @McpToolParam(description = "Number of hops from center node (default: 2, max: configured mcp.limits.max_depth)", required = false) Integer radius) { try { - return toJson(queryService.egoGraph(center, radius != null ? radius : 2)); + // Clamp radius to mcp.limits.max_depth (default 10). Pre-PR-5 + // the description claimed "max: 10" but no code enforced it — + // a caller could ask for radius=999 and Neo4j would attempt a + // [*1..999] variable-length match. + int safeRadius = Math.min(radius != null ? radius : 2, maxDepth); + return toJson(queryService.egoGraph(center, safeRadius)); } catch (Exception e) { return errorEnvelope("INTERNAL_ERROR", e); } @@ -429,9 +441,10 @@ public String findRelatedEndpoints( @McpTool(name = "search_graph", description = "Full-text search across all node labels, IDs, file paths, and properties. Use as the starting point when the user mentions a name but you don't have the exact node ID. Returns matching nodes ranked by relevance.") public String searchGraph( @McpToolParam(description = "Search query") String query, - @McpToolParam(description = "Maximum results (default: 20)", required = false) Integer limit) { + @McpToolParam(description = "Maximum results (default: 20, hard cap: configured mcp.limits.max_results)", required = false) Integer limit) { try { - return toJson(queryService.searchGraph(query, limit != null ? limit : 20)); + int safeLimit = Math.min(limit != null ? limit : 20, maxResults); + return toJson(queryService.searchGraph(query, safeLimit)); } catch (Exception e) { return errorEnvelope("INTERNAL_ERROR", e); } diff --git a/src/test/java/io/github/randomcodespace/iq/config/security/ServingChainIntegrationTest.java b/src/test/java/io/github/randomcodespace/iq/config/security/ServingChainIntegrationTest.java new file mode 100644 index 00000000..ea50dd61 --- /dev/null +++ b/src/test/java/io/github/randomcodespace/iq/config/security/ServingChainIntegrationTest.java @@ -0,0 +1,215 @@ +package io.github.randomcodespace.iq.config.security; + +import com.fasterxml.jackson.databind.ObjectMapper; +import io.github.randomcodespace.iq.config.unified.CodeIqUnifiedConfig; +import io.github.randomcodespace.iq.config.unified.ConfigDefaults; +import io.github.randomcodespace.iq.config.unified.McpAuthConfig; +import io.github.randomcodespace.iq.config.unified.McpConfig; +import io.github.randomcodespace.iq.config.unified.McpLimitsConfig; +import io.github.randomcodespace.iq.config.unified.McpToolsConfig; +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletResponse; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.core.env.StandardEnvironment; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; + +import java.io.IOException; +import java.util.Map; + +import static org.junit.jupiter.api.Assertions.*; + +/** + * End-to-end test of the serving filter chain wired in the order + * {@link io.github.randomcodespace.iq.config.security.SecurityConfig} + * registers it: RequestIdFilter → SecurityHeadersFilter → RateLimitFilter + * → BearerAuthFilter. Validates the cross-filter contract (X-Request-Id + * echo, 401 envelope, 429 envelope, security headers, rate-limit token + * bucket isolation per client) without spinning up Spring or Neo4j. + * + *

This is the "serving chain integration" gap — pre-PR-5 each filter + * had unit-test coverage in isolation but no test exercised the full + * chain together. A regression that breaks header propagation between + * RequestIdFilter and downstream filters would slip through unit tests. + * + *

Lives in the {@code io.github.randomcodespace.iq.config.security} + * package so it can call package-private {@link TokenResolver#resolve()}. + */ +class ServingChainIntegrationTest { + + private static final String TEST_TOKEN = "test-bearer-token-12345-abcdef"; + + private RequestIdFilter requestIdFilter; + private SecurityHeadersFilter securityHeadersFilter; + private RateLimitFilter rateLimitFilter; + private BearerAuthFilter bearerAuthFilter; + + @BeforeEach + void setUp() { + // Build a minimal config: bearer auth + 5 req/min so we can exercise + // the rate-limit path quickly without timing out the test. + McpAuthConfig auth = new McpAuthConfig("bearer", null, TEST_TOKEN, null); + McpLimitsConfig limits = new McpLimitsConfig(15_000, 500, 2_000_000L, 5, 10); + McpConfig mcp = new McpConfig(true, "http", null, auth, limits, McpToolsConfig.empty()); + CodeIqUnifiedConfig defaults = ConfigDefaults.builtIn(); + CodeIqUnifiedConfig config = new CodeIqUnifiedConfig( + defaults.project(), + defaults.indexing(), + defaults.serving(), + mcp, + defaults.observability(), + defaults.detectors()); + + // Active `serving` profile so TokenResolver doesn't fail-fast on + // mode=none (it would also fail-fast on mode=bearer with no token, + // but we configured a token). + StandardEnvironment env = new StandardEnvironment(); + env.setActiveProfiles("serving"); + TokenResolver tokenResolver = new TokenResolver(config, env); + tokenResolver.resolve(); + + requestIdFilter = new RequestIdFilter(); + securityHeadersFilter = new SecurityHeadersFilter(); + rateLimitFilter = new RateLimitFilter(config); + bearerAuthFilter = new BearerAuthFilter(tokenResolver); + } + + private MockHttpServletResponse runChain(String method, String uri, Map headers) + throws ServletException, IOException { + MockHttpServletRequest req = new MockHttpServletRequest(method, uri); + if (headers != null) headers.forEach(req::addHeader); + MockHttpServletResponse resp = new MockHttpServletResponse(); + // Run through the chain in the same order SecurityConfig registers them. + FilterChain terminal = (r, s) -> ((HttpServletResponse) s).setStatus(200); + FilterChain c4 = (r, s) -> bearerAuthFilter.doFilter(r, s, terminal); + FilterChain c3 = (r, s) -> rateLimitFilter.doFilter(r, s, c4); + FilterChain c2 = (r, s) -> securityHeadersFilter.doFilter(r, s, c3); + FilterChain c1 = (r, s) -> requestIdFilter.doFilter(r, s, c2); + c1.doFilter(req, resp); + return resp; + } + + @Test + void requestWithoutAuthGets401WithEnvelope() throws Exception { + MockHttpServletResponse resp = runChain("GET", "/api/stats", Map.of()); + + assertEquals(401, resp.getStatus()); + assertEquals("Bearer realm=\"codeiq\"", resp.getHeader("WWW-Authenticate")); + Map body = new ObjectMapper().readValue(resp.getContentAsByteArray(), Map.class); + assertEquals("UNAUTHORIZED", body.get("code")); + assertEquals("Bearer token required.", body.get("message")); + assertNotNull(body.get("request_id")); + // The same request_id is echoed in X-Request-Id response header so + // operators can grep their JSON logs for the matching log line. + assertEquals(body.get("request_id"), resp.getHeader("X-Request-Id")); + } + + @Test + void requestWithValidTokenPassesThrough() throws Exception { + MockHttpServletResponse resp = runChain("GET", "/api/stats", + Map.of("Authorization", "Bearer " + TEST_TOKEN)); + + assertEquals(200, resp.getStatus()); + assertNotNull(resp.getHeader("X-Request-Id"), + "Successful requests must also receive a correlation ID"); + assertNotNull(resp.getHeader("X-RateLimit-Remaining"), + "Rate-limit visibility headers must accompany every authed response"); + } + + @Test + void requestWithWrongTokenGets401() throws Exception { + MockHttpServletResponse resp = runChain("GET", "/api/stats", + Map.of("Authorization", "Bearer wrong-token-bytes-here-please")); + + assertEquals(401, resp.getStatus()); + } + + @Test + void securityHeadersArePresentOnSuccessResponses() throws Exception { + MockHttpServletResponse resp = runChain("GET", "/api/stats", + Map.of("Authorization", "Bearer " + TEST_TOKEN)); + + // SecurityHeadersFilter runs before auth, so headers appear on every + // response — exact set varies by filter impl but X-Content-Type-Options + // and X-Frame-Options are baseline. + assertNotNull(resp.getHeader("X-Content-Type-Options")); + assertNotNull(resp.getHeader("X-Frame-Options")); + assertNotNull(resp.getHeader("Referrer-Policy")); + } + + @Test + void inboundRequestIdHeaderIsPropagated() throws Exception { + String upstream = "abc-trace-12345"; + MockHttpServletResponse resp = runChain("GET", "/api/stats", + Map.of("Authorization", "Bearer " + TEST_TOKEN, + "X-Request-Id", upstream)); + + assertEquals(upstream, resp.getHeader("X-Request-Id"), + "Valid upstream X-Request-Id must propagate end-to-end"); + } + + @Test + void inboundRequestIdWithControlCharsIsRejectedAndReplaced() throws Exception { + // Log-injection attempt — newline embedded in upstream header + String malicious = "abc\nINFO: granted access"; + MockHttpServletResponse resp = runChain("GET", "/api/stats", + Map.of("Authorization", "Bearer " + TEST_TOKEN, + "X-Request-Id", malicious)); + + String emitted = resp.getHeader("X-Request-Id"); + assertNotNull(emitted); + assertNotEquals(malicious, emitted); + assertFalse(emitted.contains("\n"), + "Sanitized request_id must never contain control characters"); + } + + @Test + void rateLimitFiresAfterCapacityWith429Envelope() throws Exception { + // Configured 5 req/min above. Send 5 OK requests, the 6th should be 429. + Map auth = Map.of("Authorization", "Bearer " + TEST_TOKEN); + for (int i = 0; i < 5; i++) { + MockHttpServletResponse ok = runChain("GET", "/api/stats", auth); + assertEquals(200, ok.getStatus(), "Request " + i + " must pass under cap"); + } + MockHttpServletResponse limited = runChain("GET", "/api/stats", auth); + assertEquals(429, limited.getStatus(), + "Sixth request beyond per-minute capacity must be 429"); + assertNotNull(limited.getHeader("Retry-After"), + "429 response must include Retry-After (RFC 6585 §4)"); + assertEquals("0", limited.getHeader("X-RateLimit-Remaining")); + Map body = new ObjectMapper().readValue(limited.getContentAsByteArray(), Map.class); + assertEquals("RATE_LIMITED", body.get("code")); + assertNotNull(body.get("request_id")); + } + + @Test + void healthEndpointBypassesAuth() throws Exception { + // shouldNotFilter() in BearerAuthFilter excludes /actuator/health/* — + // verify the chain returns 200 without an Authorization header + // (kubelet probes don't carry bearer tokens). + MockHttpServletResponse resp = runChain("GET", "/actuator/health", Map.of()); + + assertEquals(200, resp.getStatus(), + "Health probe must succeed without auth"); + } + + @Test + void rateLimitBucketsAreIsolatedPerToken() throws Exception { + // First token exhausts its bucket + Map tokenA = Map.of("Authorization", "Bearer " + TEST_TOKEN); + for (int i = 0; i < 5; i++) runChain("GET", "/api/stats", tokenA); + MockHttpServletResponse limitedA = runChain("GET", "/api/stats", tokenA); + assertEquals(429, limitedA.getStatus()); + + // Second token (different client) gets its own fresh bucket. The + // rate-limit filter keys by SHA-256(Authorization), so a different + // header value = different bucket. The request is admitted by the + // bucket and reaches the auth filter, which rejects with 401. + Map tokenB = Map.of("Authorization", "Bearer different-token-but-formatted-ok"); + MockHttpServletResponse respB = runChain("GET", "/api/stats", tokenB); + assertEquals(401, respB.getStatus(), + "Different token = different bucket; rate-limit must not bleed across clients"); + } +}