From f6b9d812400f0b90002ef12dc30114bad6542cea Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Mon, 29 Jun 2026 21:14:16 +0200 Subject: [PATCH 1/5] fix(security): enforce http(s) scheme for OpenAPI spec locations + harden cron/calculator/coordinator algorithms Easy-to-fix findings from a security & algorithm review: - McpApiToolBuilder.parseSpec: require an http(s) URL for remote spec locations (UrlValidationUtils.isValidHttpUrl) before readLocation(), blocking file:// local-file reads, classpath:/jar:/other non-http schemes (reachable via /apicalls/discover-endpoints and create_api_agent). Scheme-only by design: private/internal hosts stay allowed so internal OpenAPI discovery keeps working (endpoint is eddi-admin/eddi-editor gated). - CronParser: accept day-of-week 7 as Sunday (standard cron); reject malformed steps (*/) and reversed ranges (5-1) with clean errors instead of AIOOBE / silent never-fire. - CalculatorTool: cap expression length + catch StackOverflowError to prevent recursive-descent stack exhaustion from LLM-supplied input. - InMemoryConversationCoordinator: cap the dead-letter deque (1000, oldest-first eviction) so a failure storm can't grow heap unbounded. Reported but intentionally unchanged (behavior change needs its own decision): cron dom/dow AND-vs-OR semantics; ApiCallExecutor linear 'exponential' backoff. See docs/changelog.md. Tests: +12 across McpApiToolBuilderTest, CronParserTest, CalculatorToolTest; affected suites green, mvnw compile clean. --- docs/changelog.md | 40 +++++++++++++++++++ .../eddi/engine/mcp/McpApiToolBuilder.java | 37 +++++++++++++++-- .../engine/runtime/internal/CronParser.java | 34 +++++++++++++++- .../InMemoryConversationCoordinator.java | 16 ++++++++ .../llm/tools/impl/CalculatorTool.java | 17 ++++++++ .../engine/mcp/McpApiToolBuilderTest.java | 38 ++++++++++++++++++ .../runtime/internal/CronParserTest.java | 34 ++++++++++++++++ .../llm/tools/impl/CalculatorToolTest.java | 18 +++++++++ 8 files changed, 228 insertions(+), 6 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 75595cc227..59aeec853b 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -3,6 +3,46 @@ > **Purpose:** Living document tracking all changes, decisions, and reasoning during implementation. Updated as work progresses for easy reference and review. +--- + +## šŸ”’ Security & Algorithm Hardening ā€” SSRF/File-Read, Cron, DoS Guards (2026-06-29) + +**Repo:** EDDI (`fix/security-and-algo-hardening`) +**What changed:** Easy-to-fix findings from a code/security/algorithm review, plus algorithm-bug hunt. All changes are surgical and behavior-preserving for valid input. + +### Security fixes + +1. **Local-file read / non-http SSRF in OpenAPI spec discovery (`McpApiToolBuilder.parseSpec`)** ā€” The `GET /apicallstore/apicalls/discover-endpoints?specUrl=ā€¦` endpoint (and `create_api_agent`) handed a user-supplied location straight to swagger-parser's `readLocation()`, which fetches URLs **and** reads local files (`file:///etc/passwd`) and resolves external `$ref`s. Now, when the input is a remote location (not inline content), it must be an `http(s)` URL (`UrlValidationUtils.isValidHttpUrl()`) ā€” rejecting `file://` (local-file read), `classpath:`, `jar:`, and other non-http schemes. Inline JSON/YAML still parses with no network/file access. Inline-vs-location detection broadened via new `looksLikeInlineSpec()` (handles `swagger:` and multi-line YAML). + - **Scheme-only by design:** private/internal hosts stay allowed so internal OpenAPI discovery keeps working. The endpoint is `eddi-admin`/`eddi-editor` gated, so SSRF to private/metadata IPs via an `http(s)` spec URL is an accepted residual ā€” as is the remote-`$ref` vector (swagger-parser has no clean toggle to disable only remote-ref resolution). Use full `UrlValidationUtils.validateUrl()` here if a deployment needs private-IP blocking. + +### Algorithm bugs found & fixed + +2. **`CronParser` ā€” day-of-week `7` not accepted as Sunday.** Standard cron treats `0` and `7` as Sunday; the parser rejected `7` (range `0ā€“6`) and, even if allowed, `DayOfWeek % 7` never yields `7`, so it would never match. Now `7` is accepted and normalized to `0` (`normalizeDaysOfWeek`). +3. **`CronParser` ā€” malformed fields crashed or silently never-fired.** `*/` threw `ArrayIndexOutOfBoundsException` (not a clean validation error); a reversed range like `5-1` produced an empty set ā†’ a schedule that never fires until the 2-year scan limit threw a confusing `IllegalStateException`. Both now throw a clear `IllegalArgumentException` at parse time (step structure + `start <= end` checks). +4. **`CalculatorTool` ā€” unbounded recursion DoS.** The recursive-descent `SafeMathParser` recurses on nested parens; a long/deeply-nested LLM-supplied expression could throw `StackOverflowError` (an `Error`, not caught by `calculate()`). Added a 1000-char input cap plus a defensive `StackOverflowError` catch. +5. **`InMemoryConversationCoordinator` ā€” unbounded dead-letter deque.** The active-conversation map was capped but `deadLetters` grew without limit under a failure storm. Added a `MAX_DEAD_LETTERS` (1000) cap with oldest-first eviction. + +### Algorithm bugs found ā€” reported, NOT changed (behavior change too risky) + +- **`CronParser` dom/dow semantics:** uses **AND** of day-of-month and day-of-week; standard (Vixie) cron uses **OR** when both are restricted (e.g. `0 0 13 * FRI` should fire on the 13th *or* any Friday). The smart-skip logic is built around AND ā€” flagged for a deliberate follow-up with dedicated tests. +- **`ApiCallExecutor` retry backoff:** `delay * amountOfExecutions` is **linear**, despite the `exponentialBackoffDelayInMillis` field name. Flagged; changing retry timing needs its own decision. + +### Files changed +- `engine/mcp/McpApiToolBuilder.java` ā€” URL validation in `parseSpec`, `looksLikeInlineSpec()` +- `engine/runtime/internal/CronParser.java` ā€” DOW 7, step/range validation, `normalizeDaysOfWeek()` +- `modules/llm/tools/impl/CalculatorTool.java` ā€” length cap + `StackOverflowError` catch +- `engine/runtime/internal/InMemoryConversationCoordinator.java` ā€” dead-letter cap + +### Tests added +- `McpApiToolBuilderTest` ā€” +5 (file/classpath/metadata rejection, inline still works, classifier) +- `CronParserTest` ā€” +5 (DOW 7 = Sunday, 0ā‰”7, reversed-range + malformed-step rejection) +- `CalculatorToolTest` ā€” +2 (over-long rejected, deep-nesting returns cleanly) +- All affected suites green (176 tests, 0 failures); full `mvnw compile` clean. + +### Not addressed here (needs design decision, not "easy") +- **httpcall execution SSRF** (`ApiCallExecutor`/`VertxHttpClient` follow redirects with no validation): blocking would break legitimate internal-API calls; needs an opt-in allowlist/flag. +- **Open-by-default MCP/admin surface** and **role- vs tenant-based isolation** for config resources: architectural, out of scope for a hardening pass. + --- ## šŸ”’ PR Review Fixes ā€” DynamicAgentConfig Propagation, Null Safety, Code Dedup (2026-06-26) diff --git a/src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java b/src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java index c77186abd0..199dfb4662 100644 --- a/src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java +++ b/src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java @@ -7,6 +7,7 @@ import ai.labs.eddi.configs.apicalls.model.ApiCall; import ai.labs.eddi.configs.apicalls.model.ApiCallsConfiguration; import ai.labs.eddi.configs.apicalls.model.Request; +import ai.labs.eddi.modules.llm.tools.UrlValidationUtils; import io.swagger.v3.oas.models.OpenAPI; import io.swagger.v3.oas.models.Operation; import io.swagger.v3.oas.models.PathItem; @@ -152,18 +153,34 @@ public static ApiBuildResult parseAndBuild(String openApiSpec, String endpointFi /** * Parse an OpenAPI spec from a JSON/YAML string or URL. + *

+ * Security: when the input is a location (not inline content), it is + * required to be an {@code http}/{@code https} URL via + * {@link UrlValidationUtils#isValidHttpUrl(String)} before being fetched. This + * prevents the underlying swagger-parser {@code readLocation} from reading + * local files (e.g. {@code file:///etc/passwd}) or using other non-http schemes + * (classpath:, jar:, ftp:). Private/internal hosts are intentionally still + * permitted so internal OpenAPI specs remain discoverable (the calling REST/MCP + * surface is {@code eddi-admin}/{@code eddi-editor} gated). Inline JSON/YAML + * content is parsed directly without any network access. */ public static OpenAPI parseSpec(String specInput) { var parseOptions = new ParseOptions(); parseOptions.setResolve(true); SwaggerParseResult result; - if (specInput.trim().startsWith("{") || specInput.trim().startsWith("openapi")) { - // Inline JSON or YAML content + if (looksLikeInlineSpec(specInput)) { + // Inline JSON or YAML content ā€” no network/file access. result = new OpenAPIV3Parser().readContents(specInput, null, parseOptions); } else { - // URL or file path - result = new OpenAPIV3Parser().readLocation(specInput, null, parseOptions); + // Remote location. Enforce an http(s) scheme so the parser's fetcher + // cannot read local files (file://), classpath/jar resources, or use + // other non-http schemes. Internal/private hosts stay allowed. + String location = specInput.trim(); + if (!UrlValidationUtils.isValidHttpUrl(location)) { + throw new IllegalArgumentException("OpenAPI spec location must be an http or https URL"); + } + result = new OpenAPIV3Parser().readLocation(location, null, parseOptions); } if (result == null || result.getOpenAPI() == null) { @@ -178,6 +195,18 @@ public static OpenAPI parseSpec(String specInput) { return result.getOpenAPI(); } + /** + * Heuristic: does the input look like an inline OpenAPI document (JSON/YAML + * content) rather than a remote location? A JSON object, an OpenAPI/Swagger + * marker, or any multi-line content is inline. A single-token string such as + * {@code https://host/openapi.json} is treated as a remote location and + * validated as a URL before fetching. + */ + static boolean looksLikeInlineSpec(String specInput) { + String trimmed = specInput.trim(); + return trimmed.startsWith("{") || trimmed.startsWith("openapi") || trimmed.startsWith("swagger") || trimmed.contains("\n"); + } + /** * Build a single ApiCall from an OpenAPI operation. */ diff --git a/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java b/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java index 8f11ca006f..16e4b61308 100644 --- a/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java +++ b/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java @@ -54,7 +54,7 @@ public static void validate(String cronExpression) { parseField(parts[1], 0, 23); // hour parseField(parts[2], 1, 31); // day of month parseField(substituteNames(parts[3], MONTH_NAMES), 1, 12); // month - parseField(substituteNames(parts[4], DOW_NAMES), 0, 6); // day of week + parseField(substituteNames(parts[4], DOW_NAMES), 0, 7); // day of week (0 and 7 both = Sunday) } /** @@ -78,7 +78,7 @@ public static Instant computeNextFire(String cronExpression, Instant after, Zone Set hours = parseField(parts[1], 0, 23); Set daysOfMonth = parseField(parts[2], 1, 31); Set months = parseField(substituteNames(parts[3], MONTH_NAMES), 1, 12); - Set daysOfWeek = parseField(substituteNames(parts[4], DOW_NAMES), 0, 6); + Set daysOfWeek = normalizeDaysOfWeek(parseField(substituteNames(parts[4], DOW_NAMES), 0, 7)); // Walk forward minute-by-minute from 'after + 1 minute' (aligned to minute // boundary) @@ -136,6 +136,9 @@ static Set parseField(String field, int min, int max) { if (part.contains("/")) { // Step: */15 or 1-30/5 String[] stepParts = part.split("/"); + if (stepParts.length != 2) { + throw new IllegalArgumentException("Invalid step expression '" + part + "' in field: " + field); + } int step = Integer.parseInt(stepParts[1]); if (step <= 0) throw new IllegalArgumentException("Step must be > 0: " + field); @@ -144,20 +147,32 @@ static Set parseField(String field, int min, int max) { if (!stepParts[0].equals("*")) { if (stepParts[0].contains("-")) { String[] range = stepParts[0].split("-"); + if (range.length != 2) { + throw new IllegalArgumentException("Invalid range expression '" + stepParts[0] + "' in field: " + field); + } start = Integer.parseInt(range[0]); end = Integer.parseInt(range[1]); } else { start = Integer.parseInt(stepParts[0]); } } + if (start > end) { + throw new IllegalArgumentException("Range start must be <= end ('" + part + "') in field: " + field); + } for (int i = start; i <= end; i += step) { values.add(i); } } else if (part.contains("-")) { // Range: 1-5 String[] range = part.split("-"); + if (range.length != 2) { + throw new IllegalArgumentException("Invalid range expression '" + part + "' in field: " + field); + } int start = Integer.parseInt(range[0]); int end = Integer.parseInt(range[1]); + if (start > end) { + throw new IllegalArgumentException("Range start must be <= end ('" + part + "') in field: " + field); + } for (int i = start; i <= end; i++) { values.add(i); } @@ -177,6 +192,21 @@ static Set parseField(String field, int min, int max) { return values; } + /** + * Normalize day-of-week 7 to 0 (both denote Sunday in standard cron). Java's + * {@code DayOfWeek.getValue() % 7} yields 0 for Sunday, so a parsed value of 7 + * would otherwise never match. + */ + private static Set normalizeDaysOfWeek(Set daysOfWeek) { + if (!daysOfWeek.contains(7)) { + return daysOfWeek; + } + Set normalized = new TreeSet<>(daysOfWeek); + normalized.remove(7); + normalized.add(0); + return normalized; + } + private static String substituteNames(String field, Map names) { String result = field.toUpperCase(); for (Map.Entry entry : names.entrySet()) { diff --git a/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java b/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java index 374631a294..211f8373bf 100644 --- a/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java +++ b/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java @@ -65,6 +65,14 @@ public class InMemoryConversationCoordinator implements IConversationCoordinator private static final int MAX_RETRIES = 3; + /** + * Upper bound on retained dead-letter entries. The active-conversation map is + * already capped; without this bound a storm of permanently-failing + * conversations would grow {@link #deadLetters} without limit. Oldest entries + * are evicted first (the dashboard inspects the most recent failures). + */ + private static final int MAX_DEAD_LETTERS = 1000; + private final Map>> conversationQueues = new ConcurrentHashMap<>(); private final ConcurrentLinkedDeque deadLetters = new ConcurrentLinkedDeque<>(); private final AtomicLong totalProcessed = new AtomicLong(0); @@ -184,6 +192,14 @@ private void routeToDeadLetter(String conversationId, Throwable failure) { deadLetters.addLast(new DeadLetterEntry(id, conversationId, error, timestamp, payload)); totalDeadLettered.incrementAndGet(); + + // Bound memory: evict oldest entries beyond the cap. size() on a + // ConcurrentLinkedDeque is O(n), so only walk when we know we're over. + while (deadLetters.size() > MAX_DEAD_LETTERS) { + if (deadLetters.pollFirst() == null) { + break; + } + } } private void submitNext(String conversationId, BlockingQueue> queue) { diff --git a/src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java b/src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java index 4f22f3ca0f..979512dd09 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java +++ b/src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java @@ -23,6 +23,12 @@ public class CalculatorTool { private static final Logger LOGGER = Logger.getLogger(CalculatorTool.class); + /** + * Maximum accepted expression length ā€” guards the recursive parser's stack + * depth. + */ + private static final int MAX_EXPRESSION_LENGTH = 1000; + @Tool("Performs mathematical calculations. Supports basic operations (+, -, *, /, ^, %), " + "functions (sqrt, pow, abs, ceil, floor, round, min, max, sin, cos, tan, atan, log, exp), " + "and constants (PI, E). Returns the numeric result.") @@ -35,6 +41,13 @@ public String calculate(@P("expression") String expression) { return "Error: Expression must not be empty."; } + // Bound input length: the recursive-descent parser recurses on nested + // parentheses, so an over-long expression could exhaust the stack + // (StackOverflowError is an Error, not caught below). + if (expression.length() > MAX_EXPRESSION_LENGTH) { + return "Error: Expression too long (max " + MAX_EXPRESSION_LENGTH + " characters)."; + } + double result = new SafeMathParser(expression).parse(); // Handle special values @@ -57,6 +70,10 @@ public String calculate(@P("expression") String expression) { } catch (IllegalArgumentException e) { LOGGER.error("Calculation error: " + e.getMessage()); return "Error: " + e.getMessage(); + } catch (StackOverflowError e) { + // Defense-in-depth alongside the length cap: deeply nested input. + LOGGER.error("Calculation aborted: expression nesting too deep"); + return "Error: Expression is too deeply nested."; } catch (Exception e) { LOGGER.error("Unexpected calculation error", e); return "Error: An unexpected error occurred during calculation."; diff --git a/src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java b/src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java index 24ee585a8c..efff108618 100644 --- a/src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java +++ b/src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java @@ -322,4 +322,42 @@ void parseAndBuild_httpCallParameterDescriptions() { assertNotNull(getPet.getParameters()); assertEquals("The pet ID", getPet.getParameters().get("petId")); } + + // === Security: spec-location validation (SSRF + local file read) === + + @Test + void parseSpec_rejectsFileScheme() { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> McpApiToolBuilder.parseSpec("file:///etc/passwd")); + assertTrue(ex.getMessage().toLowerCase().contains("http"), "Expected scheme rejection, got: " + ex.getMessage()); + } + + @Test + void parseSpec_rejectsNonHttpScheme() { + assertThrows(IllegalArgumentException.class, () -> McpApiToolBuilder.parseSpec("classpath:/internal-spec.yaml")); + assertThrows(IllegalArgumentException.class, () -> McpApiToolBuilder.parseSpec("not-a-valid-url")); + } + + @Test + void parseSpec_allowsInternalHttpHostAtSchemeGate() { + // Scheme-only policy: private/internal hosts are intentionally NOT rejected + // (internal OpenAPI discovery must keep working). The scheme gate accepts + // them ā€” only a subsequent fetch/parse can fail, never the URL check itself. + assertTrue(McpApiToolBuilder.looksLikeInlineSpec("http://10.0.0.5/openapi.json") == false); + assertTrue(ai.labs.eddi.modules.llm.tools.UrlValidationUtils.isValidHttpUrl("http://169.254.169.254/latest/meta-data/")); + assertTrue(ai.labs.eddi.modules.llm.tools.UrlValidationUtils.isValidHttpUrl("http://internal-svc.cluster.local/spec.json")); + } + + @Test + void parseSpec_acceptsInlineContentWithoutNetworkAccess() { + assertNotNull(McpApiToolBuilder.parseSpec(PETSTORE_SPEC)); + } + + @Test + void looksLikeInlineSpec_classifiesContentVsLocation() { + assertTrue(McpApiToolBuilder.looksLikeInlineSpec("{\"openapi\":\"3.0.0\"}")); + assertTrue(McpApiToolBuilder.looksLikeInlineSpec("openapi: 3.0.0\ninfo:\n title: x")); + assertTrue(McpApiToolBuilder.looksLikeInlineSpec("swagger: \"2.0\"\ninfo: {}")); + assertFalse(McpApiToolBuilder.looksLikeInlineSpec("https://petstore.example.com/openapi.json")); + assertFalse(McpApiToolBuilder.looksLikeInlineSpec("file:///etc/passwd")); + } } diff --git a/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java b/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java index 11d2c387ad..c1ce1b7a97 100644 --- a/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java +++ b/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java @@ -161,4 +161,38 @@ void computeMinIntervalSeconds_every15min() { long interval = CronParser.computeMinIntervalSeconds("*/15 * * * *", UTC); assertEquals(900, interval); // 15 * 60 } + + // --- Day-of-week 7 = Sunday (standard cron compatibility) --- + + @Test + void validate_acceptsDayOfWeek7AsSunday() { + assertDoesNotThrow(() -> CronParser.validate("0 0 * * 7")); + } + + @Test + void computeNextFire_dayOfWeek7MatchesSunday() { + // 2024-01-06 is a Saturday; the next Sunday is 2024-01-07. + Instant saturday = ZonedDateTime.of(2024, 1, 6, 12, 0, 0, 0, UTC).toInstant(); + Instant next = CronParser.computeNextFire("0 0 * * 7", saturday, UTC); + assertEquals(java.time.DayOfWeek.SUNDAY, next.atZone(UTC).getDayOfWeek()); + } + + @Test + void computeNextFire_dayOfWeek0AndDayOfWeek7AgreeOnSunday() { + Instant base = ZonedDateTime.of(2024, 1, 6, 12, 0, 0, 0, UTC).toInstant(); + assertEquals(CronParser.computeNextFire("0 0 * * 0", base, UTC), CronParser.computeNextFire("0 0 * * 7", base, UTC)); + } + + // --- Malformed-field rejection (clean errors, not AIOOBE / silent never-fire) + // --- + + @Test + void parseField_rejectsReversedRange() { + assertThrows(IllegalArgumentException.class, () -> CronParser.parseField("5-1", 0, 59)); + } + + @Test + void parseField_rejectsMalformedStep() { + assertThrows(IllegalArgumentException.class, () -> CronParser.parseField("*/", 0, 59)); + } } diff --git a/src/test/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorToolTest.java b/src/test/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorToolTest.java index 9e02bd288a..c91e432323 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorToolTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorToolTest.java @@ -361,4 +361,22 @@ void testConvertUnits_BoilingPointAccuracy() { // Locale-independent check: result contains "212" and "fahrenheit" assertTrue(result.contains("212") && result.contains("fahrenheit"), "Expected 212 fahrenheit in: " + result); } + + // === Robustness: stack-exhaustion guard === + + @Test + void testCalculate_RejectsOverlongExpression() { + String longExpr = "1" + "+1".repeat(600); // 1201 chars, > MAX_EXPRESSION_LENGTH + String result = calculatorTool.calculate(longExpr); + assertTrue(result.startsWith("Error: Expression too long"), "Expected length rejection, got: " + result); + } + + @Test + void testCalculate_DeeplyNestedReturnsCleanlyWithoutError() { + // Within the length cap but heavily nested ā€” must return a value, never + // throw StackOverflowError out of calculate(). + String expr = "(".repeat(300) + "1" + ")".repeat(300); + String result = calculatorTool.calculate(expr); + assertEquals("1", result); + } } From 5957b0ce7af5da5398b8ba9e41e0f8803e03e965 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Mon, 29 Jun 2026 22:33:41 +0200 Subject: [PATCH 2/5] feat(security): opt-in SSRF protection for httpcalls/A2A; fix cron OR semantics + exponential backoff Tackles the remaining fixable review items (the rest are architectural): - Opt-in SSRF protection (eddi.security.ssrf-protection.enabled, default off). When on: ApiCallExecutor validates the resolved httpcall URL and disables redirect-following (new IRequest.setFollowRedirects, honoured by the Vert.x wrapper); A2AToolProviderManager validates peer card-fetch + tasks/send URLs. RemoteApiResourceSource is intentionally out of scope (admin-initiated, internal imports legitimate, JDK client is Redirect.NEVER). - CronParser: standard-cron dom/dow OR semantics when both fields are restricted (was AND); single-restricted still reduces to AND so existing schedules are unaffected. - ApiCallExecutor: retry backoff is now truly exponential (base * 2^(attempt-1), overflow-safe, 5-min ceiling) instead of linear. Tests: +14 (ApiCallExecutor SSRF + backoff curve, CronParser OR cases); 8 constructor-call sites updated. Mock-based suites green; A2A/embedded- server suites compile but are unrunnable in this sandbox (JDK HttpClient selector) - covered by CI. mvnw compile + test-compile clean. --- docs/changelog.md | 41 +++++----- .../labs/eddi/engine/httpclient/IRequest.java | 12 +++ .../httpclient/impl/HttpClientWrapper.java | 6 ++ .../engine/runtime/internal/CronParser.java | 27 +++++-- .../apicalls/impl/ApiCallExecutor.java | 35 +++++++-- .../llm/impl/A2AToolProviderManager.java | 17 ++++- src/main/resources/application.properties | 7 ++ .../runtime/internal/CronParserTest.java | 34 +++++++++ .../ApiCallExecutorBranchCoverageTest.java | 2 +- .../impl/ApiCallExecutorExtendedTest.java | 2 +- .../apicalls/impl/ApiCallExecutorTest.java | 74 ++++++++++++++++++- .../A2AToolProviderManagerBranchTest.java | 2 +- .../A2AToolProviderManagerDeepBranchTest.java | 2 +- .../A2AToolProviderManagerExtendedTest.java | 2 +- .../llm/impl/A2AToolProviderManagerTest.java | 2 +- ...oolProviderManagerUncoveredBranchTest.java | 2 +- 16 files changed, 229 insertions(+), 38 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 59aeec853b..6cbb30691e 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -8,40 +8,47 @@ ## šŸ”’ Security & Algorithm Hardening ā€” SSRF/File-Read, Cron, DoS Guards (2026-06-29) **Repo:** EDDI (`fix/security-and-algo-hardening`) -**What changed:** Easy-to-fix findings from a code/security/algorithm review, plus algorithm-bug hunt. All changes are surgical and behavior-preserving for valid input. +**What changed:** Findings from a code/security/algorithm review and bug hunt. Most changes are surgical and behavior-preserving for valid input; the two intentional behavior corrections (cron dom/dow **OR** semantics, **exponential** retry backoff) are called out explicitly below. New SSRF protection is opt-in and **off by default**. ### Security fixes 1. **Local-file read / non-http SSRF in OpenAPI spec discovery (`McpApiToolBuilder.parseSpec`)** ā€” The `GET /apicallstore/apicalls/discover-endpoints?specUrl=ā€¦` endpoint (and `create_api_agent`) handed a user-supplied location straight to swagger-parser's `readLocation()`, which fetches URLs **and** reads local files (`file:///etc/passwd`) and resolves external `$ref`s. Now, when the input is a remote location (not inline content), it must be an `http(s)` URL (`UrlValidationUtils.isValidHttpUrl()`) ā€” rejecting `file://` (local-file read), `classpath:`, `jar:`, and other non-http schemes. Inline JSON/YAML still parses with no network/file access. Inline-vs-location detection broadened via new `looksLikeInlineSpec()` (handles `swagger:` and multi-line YAML). - **Scheme-only by design:** private/internal hosts stay allowed so internal OpenAPI discovery keeps working. The endpoint is `eddi-admin`/`eddi-editor` gated, so SSRF to private/metadata IPs via an `http(s)` spec URL is an accepted residual ā€” as is the remote-`$ref` vector (swagger-parser has no clean toggle to disable only remote-ref resolution). Use full `UrlValidationUtils.validateUrl()` here if a deployment needs private-IP blocking. +2. **Opt-in SSRF protection for agent-driven outbound calls** ā€” New `eddi.security.ssrf-protection.enabled` flag (**default off** to preserve internal-API calls in self-hosted deployments). When on: + - `ApiCallExecutor` (httpcalls): the fully-resolved, templated target URL is validated with `UrlValidationUtils.validateUrl()` (blocks private/loopback/link-local/CGNAT/cloud-metadata + non-http), and redirect-following is **disabled** per request (new `IRequest.setFollowRedirects`, honoured by the Vert.x `HttpClientWrapper`) so a `3xx ā†’ internal host` can't bypass validation. + - `A2AToolProviderManager` (peer Agent-Card fetch + `tasks/send`): both target URLs validated. The JDK client already defaults to `Redirect.NEVER`, so no redirect hop to re-check. + - **Scoped out intentionally:** `RemoteApiResourceSource` (admin-initiated import-from-URL) ā€” admin explicitly targets a URL, internal-instance imports are common, and the JDK client is `Redirect.NEVER`. Forcing private-IP blocking there would break legitimate internal imports. ### Algorithm bugs found & fixed -2. **`CronParser` ā€” day-of-week `7` not accepted as Sunday.** Standard cron treats `0` and `7` as Sunday; the parser rejected `7` (range `0ā€“6`) and, even if allowed, `DayOfWeek % 7` never yields `7`, so it would never match. Now `7` is accepted and normalized to `0` (`normalizeDaysOfWeek`). -3. **`CronParser` ā€” malformed fields crashed or silently never-fired.** `*/` threw `ArrayIndexOutOfBoundsException` (not a clean validation error); a reversed range like `5-1` produced an empty set ā†’ a schedule that never fires until the 2-year scan limit threw a confusing `IllegalStateException`. Both now throw a clear `IllegalArgumentException` at parse time (step structure + `start <= end` checks). -4. **`CalculatorTool` ā€” unbounded recursion DoS.** The recursive-descent `SafeMathParser` recurses on nested parens; a long/deeply-nested LLM-supplied expression could throw `StackOverflowError` (an `Error`, not caught by `calculate()`). Added a 1000-char input cap plus a defensive `StackOverflowError` catch. -5. **`InMemoryConversationCoordinator` ā€” unbounded dead-letter deque.** The active-conversation map was capped but `deadLetters` grew without limit under a failure storm. Added a `MAX_DEAD_LETTERS` (1000) cap with oldest-first eviction. - -### Algorithm bugs found ā€” reported, NOT changed (behavior change too risky) - -- **`CronParser` dom/dow semantics:** uses **AND** of day-of-month and day-of-week; standard (Vixie) cron uses **OR** when both are restricted (e.g. `0 0 13 * FRI` should fire on the 13th *or* any Friday). The smart-skip logic is built around AND ā€” flagged for a deliberate follow-up with dedicated tests. -- **`ApiCallExecutor` retry backoff:** `delay * amountOfExecutions` is **linear**, despite the `exponentialBackoffDelayInMillis` field name. Flagged; changing retry timing needs its own decision. +3. **`CronParser` ā€” day-of-week `7` not accepted as Sunday.** Standard cron treats `0` and `7` as Sunday; the parser rejected `7` (range `0ā€“6`) and, even if allowed, `DayOfWeek % 7` never yields `7`, so it would never match. Now `7` is accepted and normalized to `0` (`normalizeDaysOfWeek`). +4. **`CronParser` ā€” dom/dow used AND instead of standard-cron OR.** When **both** day-of-month and day-of-week are restricted (neither is `*`), Vixie cron fires when **either** matches (e.g. `0 0 13 * FRI` = the 13th *or* any Friday). The parser ANDed them. Now `dayMatches()` applies OR when both fields are restricted, AND otherwise (single-restricted reduces to the restricted field, so existing schedules are unaffected). The smart-skip loop was reworked around `dayMatches`. +5. **`CronParser` ā€” malformed fields crashed or silently never-fired.** `*/` threw `ArrayIndexOutOfBoundsException` (not a clean validation error); a reversed range like `5-1` produced an empty set ā†’ a schedule that never fires until the 2-year scan limit threw a confusing `IllegalStateException`. Both now throw a clear `IllegalArgumentException` at parse time (step structure + `start <= end` checks). +6. **`ApiCallExecutor` retry backoff was linear, not exponential.** `delay * amountOfExecutions` (linear) despite the `exponentialBackoffDelayInMillis` field name. Now true exponential ā€” `base * 2^(attempt-1)` ā€” with an overflow-safe shift and a 5-minute ceiling (`MAX_BACKOFF_MILLIS`). First retry delay is unchanged (`base`), so the change only affects later retries. +7. **`CalculatorTool` ā€” unbounded recursion DoS.** The recursive-descent `SafeMathParser` recurses on nested parens; a long/deeply-nested LLM-supplied expression could throw `StackOverflowError` (an `Error`, not caught by `calculate()`). Added a 1000-char input cap plus a defensive `StackOverflowError` catch. +8. **`InMemoryConversationCoordinator` ā€” unbounded dead-letter deque.** The active-conversation map was capped but `deadLetters` grew without limit under a failure storm. Added a `MAX_DEAD_LETTERS` (1000) cap with oldest-first eviction. ### Files changed - `engine/mcp/McpApiToolBuilder.java` ā€” URL validation in `parseSpec`, `looksLikeInlineSpec()` -- `engine/runtime/internal/CronParser.java` ā€” DOW 7, step/range validation, `normalizeDaysOfWeek()` +- `modules/apicalls/impl/ApiCallExecutor.java` ā€” opt-in SSRF validation + redirect disable; exponential backoff +- `modules/llm/impl/A2AToolProviderManager.java` ā€” opt-in URL validation on peer fetch/send +- `engine/httpclient/IRequest.java` + `impl/HttpClientWrapper.java` ā€” `setFollowRedirects` (default no-op; Vert.x honours it) +- `engine/runtime/internal/CronParser.java` ā€” DOW 7, OR semantics (`dayMatches`), step/range validation - `modules/llm/tools/impl/CalculatorTool.java` ā€” length cap + `StackOverflowError` catch - `engine/runtime/internal/InMemoryConversationCoordinator.java` ā€” dead-letter cap +- `resources/application.properties` ā€” documented `eddi.security.ssrf-protection.enabled` ### Tests added -- `McpApiToolBuilderTest` ā€” +5 (file/classpath/metadata rejection, inline still works, classifier) -- `CronParserTest` ā€” +5 (DOW 7 = Sunday, 0ā‰”7, reversed-range + malformed-step rejection) +- `McpApiToolBuilderTest` ā€” +5 (file/classpath/non-http rejection, scheme-gate allows internal hosts, inline works, classifier) +- `ApiCallExecutorTest` ā€” +6 (SSRF block internal URL, disable redirects on public, protection-off no-op; exponential curve, ceiling cap, no-retry zero) +- `CronParserTest` ā€” +6 (DOW 7 = Sunday, 0ā‰”7, OR fires on dom and on weekday, single-restricted stays AND, reversed-range + malformed-step rejection) - `CalculatorToolTest` ā€” +2 (over-long rejected, deep-nesting returns cleanly) -- All affected suites green (176 tests, 0 failures); full `mvnw compile` clean. +- `ApiCallExecutor`/`A2AToolProviderManager` constructor-call sites updated across 8 test files. +- Mock-based suites green; A2A + embedded-server suites are unrunnable in the sandbox (JDK `HttpClient`/`HttpServer` can't open a selector) but compile and are exercised in CI. -### Not addressed here (needs design decision, not "easy") -- **httpcall execution SSRF** (`ApiCallExecutor`/`VertxHttpClient` follow redirects with no validation): blocking would break legitimate internal-API calls; needs an opt-in allowlist/flag. -- **Open-by-default MCP/admin surface** and **role- vs tenant-based isolation** for config resources: architectural, out of scope for a hardening pass. +### Not addressed here (architectural ā€” out of scope for a hardening pass) +- **Open-by-default MCP/admin surface** and **role- vs tenant-based isolation** for config resources. +- **Conversation-memory 16 MB BSON ceiling** ā€” needs a proper step-archival design, not a quick guard. --- diff --git a/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java b/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java index 99786bd100..5665607646 100644 --- a/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java +++ b/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java @@ -22,6 +22,18 @@ public interface IRequest { IRequest setTimeout(long timeout, TimeUnit timeUnit); + /** + * Enable or disable automatic HTTP redirect following for this request. + *

+ * The default implementation is a no-op (preserves the client's configured + * behaviour); the Vert.x-backed implementation honours it. SSRF-protected + * callers disable redirects to prevent a {@code 3xx ā†’ internal host} bypass of + * URL validation. + */ + default IRequest setFollowRedirects(boolean follow) { + return this; + } + IResponse send() throws HttpRequestException; Map toMap(); diff --git a/src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java b/src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java index a3920e6f46..86f2ab4961 100644 --- a/src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java +++ b/src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java @@ -162,6 +162,12 @@ public IRequest setTimeout(long timeout, TimeUnit timeUnit) { return this; } + @Override + public IRequest setFollowRedirects(boolean follow) { + request.followRedirects(follow); + return this; + } + @Override public IResponse send() throws HttpRequestException { CompletableFuture future = new CompletableFuture<>(); diff --git a/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java b/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java index 16e4b61308..7968913d0d 100644 --- a/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java +++ b/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java @@ -80,6 +80,12 @@ public static Instant computeNextFire(String cronExpression, Instant after, Zone Set months = parseField(substituteNames(parts[3], MONTH_NAMES), 1, 12); Set daysOfWeek = normalizeDaysOfWeek(parseField(substituteNames(parts[4], DOW_NAMES), 0, 7)); + // Standard (Vixie) cron: when BOTH day-of-month and day-of-week are + // restricted (neither is "*"), a day matches if EITHER field matches. + // If only one is restricted, the "*" field is always true, so the AND + // below naturally reduces to the restricted field. + boolean bothDayFieldsRestricted = !parts[2].trim().equals("*") && !parts[4].trim().equals("*"); + // Walk forward minute-by-minute from 'after + 1 minute' (aligned to minute // boundary) ZonedDateTime candidate = after.atZone(zoneId).withSecond(0).withNano(0).plusMinutes(1); @@ -88,9 +94,9 @@ public static Instant computeNextFire(String cronExpression, Instant after, Zone ZonedDateTime limit = candidate.plusYears(2); while (candidate.isBefore(limit)) { - if (months.contains(candidate.getMonthValue()) && daysOfMonth.contains(candidate.getDayOfMonth()) - && daysOfWeek.contains(candidate.getDayOfWeek().getValue() % 7) // Java DayOfWeek: MON=1..SUN=7 - && hours.contains(candidate.getHour()) && minutes.contains(candidate.getMinute())) { + boolean dayMatches = dayMatches(candidate, daysOfMonth, daysOfWeek, bothDayFieldsRestricted); + if (months.contains(candidate.getMonthValue()) && dayMatches && hours.contains(candidate.getHour()) + && minutes.contains(candidate.getMinute())) { return candidate.toInstant(); } @@ -99,8 +105,8 @@ public static Instant computeNextFire(String cronExpression, Instant after, Zone candidate = skipToNextMonth(candidate, months); continue; } - // If day doesn't match, jump to next day - if (!daysOfMonth.contains(candidate.getDayOfMonth()) || !daysOfWeek.contains(candidate.getDayOfWeek().getValue() % 7)) { + // If day doesn't match (per OR/AND semantics above), jump to next day + if (!dayMatches) { candidate = candidate.plusDays(1).withHour(0).withMinute(0); continue; } @@ -192,6 +198,17 @@ static Set parseField(String field, int min, int max) { return values; } + /** + * Determine whether the candidate's date matches the day-of-month and + * day-of-week sets, applying standard cron semantics: OR when both fields are + * restricted, AND otherwise. + */ + private static boolean dayMatches(ZonedDateTime candidate, Set daysOfMonth, Set daysOfWeek, boolean bothRestricted) { + boolean domMatch = daysOfMonth.contains(candidate.getDayOfMonth()); + boolean dowMatch = daysOfWeek.contains(candidate.getDayOfWeek().getValue() % 7); // Java DayOfWeek: MON=1..SUN=7 + return bothRestricted ? (domMatch || dowMatch) : (domMatch && dowMatch); + } + /** * Normalize day-of-week 7 to 0 (both denote Sunday in standard cron). Java's * {@code DayOfWeek.getValue() % 7} yields 0 for Sunday, so a parsed value of 7 diff --git a/src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java b/src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java index 7ce661fc88..978163303c 100644 --- a/src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java +++ b/src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java @@ -14,10 +14,12 @@ import ai.labs.eddi.engine.memory.IConversationMemory; import ai.labs.eddi.engine.memory.IConversationMemory.IWritableConversationStep; import ai.labs.eddi.engine.runtime.IRuntime; +import ai.labs.eddi.modules.llm.tools.UrlValidationUtils; import ai.labs.eddi.modules.templating.ITemplatingEngine; import ai.labs.eddi.secrets.SecretResolver; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; import org.jboss.logging.Logger; import java.net.URI; @@ -50,22 +52,30 @@ public class ApiCallExecutor implements IApiCallExecutor { private static final Logger LOGGER = Logger.getLogger(ApiCallExecutor.class); + /** + * Ceiling for a single retry backoff delay (5 min) ā€” bounds exponential growth. + */ + private static final int MAX_BACKOFF_MILLIS = 300_000; + private final IHttpClient httpClient; private final IJsonSerialization jsonSerialization; private final IRuntime runtime; private final PrePostUtils prePostUtils; private final GlobalVariableResolver globalVariableResolver; private final SecretResolver secretResolver; + private final boolean ssrfProtectionEnabled; @Inject public ApiCallExecutor(IHttpClient httpClient, IJsonSerialization jsonSerialization, IRuntime runtime, PrePostUtils prePostUtils, - GlobalVariableResolver globalVariableResolver, SecretResolver secretResolver) { + GlobalVariableResolver globalVariableResolver, SecretResolver secretResolver, + @ConfigProperty(name = "eddi.security.ssrf-protection.enabled", defaultValue = "false") boolean ssrfProtectionEnabled) { this.httpClient = httpClient; this.jsonSerialization = jsonSerialization; this.runtime = runtime; this.prePostUtils = prePostUtils; this.globalVariableResolver = globalVariableResolver; this.secretResolver = secretResolver; + this.ssrfProtectionEnabled = ssrfProtectionEnabled; } @Override @@ -244,13 +254,18 @@ private static void logExecutionResponse(IResponse response, String httpCallsNam LOGGER.info(httpCallsName + format(" Execution time: %sms\n", duration)); } - private static int getDelayInMillis(ApiCall call, boolean retryCall, int amountOfExecutions) { + // Package-private for unit testing of the backoff curve. + static int getDelayInMillis(ApiCall call, boolean retryCall, int amountOfExecutions) { int delayInMillis = 0; if (retryCall) { - Integer exponentialBackoffDelay = call.getPostResponse().getRetryApiCallInstruction().getExponentialBackoffDelayInMillis(); - if (exponentialBackoffDelay != null) { - delayInMillis = exponentialBackoffDelay * amountOfExecutions; + Integer baseDelay = call.getPostResponse().getRetryApiCallInstruction().getExponentialBackoffDelayInMillis(); + if (baseDelay != null && baseDelay > 0) { + // True exponential backoff: base * 2^(attempt-1), capped to avoid + // overflow and unbounded waits. (Previously linear: base * attempt.) + int exponent = Math.max(0, amountOfExecutions - 1); + long computed = (long) baseDelay << Math.min(exponent, 20); + delayInMillis = (int) Math.min(computed, MAX_BACKOFF_MILLIS); } } @@ -326,8 +341,18 @@ private IRequest buildRequest(String targetServerUrl, Request requestConfig, Map requestBody = globalVariableResolver.resolveValue(requestBody); requestBody = secretResolver.resolveValue(requestBody); + // SSRF protection (opt-in): validate the fully-resolved target and disable + // redirect-following so a 3xx cannot bounce the request to an internal host. + // Off by default to preserve calls to internal/private APIs. + if (ssrfProtectionEnabled) { + UrlValidationUtils.validateUrl(targetUri.toString()); + } + var method = IHttpClient.Method.valueOf(requestConfig.getMethod().toUpperCase()); IRequest request = httpClient.newRequest(targetUri, method); + if (ssrfProtectionEnabled) { + request.setFollowRedirects(false); + } if (!isNullOrEmpty(requestBody)) { String contentType = requestConfig.getContentType(); request.setBodyEntity(requestBody, UTF_8, !isNullOrEmpty(contentType) ? contentType : TEXT_PLAIN); diff --git a/src/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.java b/src/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.java index be4e53e7f1..1772de97ff 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.java +++ b/src/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.java @@ -6,6 +6,7 @@ import ai.labs.eddi.configs.variables.GlobalVariableResolver; import ai.labs.eddi.modules.llm.model.LlmConfiguration.A2AAgentConfig; +import ai.labs.eddi.modules.llm.tools.UrlValidationUtils; import ai.labs.eddi.secrets.SecretResolver; import com.fasterxml.jackson.databind.ObjectMapper; import dev.langchain4j.agent.tool.ToolExecutionRequest; @@ -14,6 +15,7 @@ import dev.langchain4j.service.tool.ToolExecutor; import jakarta.enterprise.context.ApplicationScoped; import jakarta.inject.Inject; +import org.eclipse.microprofile.config.inject.ConfigProperty; import org.jboss.logging.Logger; import java.net.URI; @@ -42,6 +44,7 @@ public class A2AToolProviderManager { private final GlobalVariableResolver globalVariableResolver; private final SecretResolver secretResolver; private final HttpClient httpClient; + private final boolean ssrfProtectionEnabled; /** Cached Agent Card data per URL to avoid re-fetching on every request. */ private final Map agentCache = new ConcurrentHashMap<>(); @@ -63,9 +66,13 @@ record A2AToolsResult(List toolSpecs, Map fetchAgentCard(String agentUrl, A2AAgentConfig confi String cardUrl = agentUrl + "/agent.json"; + if (ssrfProtectionEnabled) { + UrlValidationUtils.validateUrl(cardUrl); + } + HttpRequest.Builder requestBuilder = HttpRequest.newBuilder().uri(URI.create(cardUrl)) .timeout(Duration.ofMillis(config.getTimeoutMs() != null ? config.getTimeoutMs() : 30000)).GET(); @@ -249,6 +260,10 @@ private String executeA2ATask(String agentUrl, A2AAgentConfig config, ToolExecut String body = MAPPER.writeValueAsString(jsonRpc); + if (ssrfProtectionEnabled) { + UrlValidationUtils.validateUrl(agentUrl); + } + HttpRequest.Builder requestBuilder = HttpRequest.newBuilder().uri(URI.create(agentUrl)) .timeout(Duration.ofMillis(config.getTimeoutMs() != null ? config.getTimeoutMs() : 30000)).header("Content-Type", "application/json") .POST(HttpRequest.BodyPublishers.ofString(body)); diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index a1a4ebf94d..f6a7b687ab 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -187,6 +187,13 @@ quarkus.oidc.application-type=service authorization.enabled=${quarkus.oidc.tenant-enabled} # Security escape hatch ā€” MUST be explicitly set to true to allow unauthenticated access in prod eddi.security.allow-unauthenticated=false +# SSRF protection for agent-configured outbound calls (httpcalls + A2A peer fetch). +# When true, resolved target URLs are validated (no private/loopback/link-local/ +# cloud-metadata addresses, http(s) only) and redirect-following is disabled on +# httpcalls so a 3xx cannot bounce to an internal host. Default OFF to preserve +# calls to internal/private APIs in self-hosted deployments ā€” enable for +# multi-tenant / internet-facing deployments where agent configs are less trusted. +eddi.security.ssrf-protection.enabled=false # Static assets + SPA entry points ā€” GET/HEAD only (no mutation allowed) # Root-level icons/images must be listed explicitly because /* is caught by the authenticated policy quarkus.http.auth.permission.static-assets.paths=\ diff --git a/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java b/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java index c1ce1b7a97..fb6d5aa627 100644 --- a/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java +++ b/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java @@ -195,4 +195,38 @@ void parseField_rejectsReversedRange() { void parseField_rejectsMalformedStep() { assertThrows(IllegalArgumentException.class, () -> CronParser.parseField("*/", 0, 59)); } + + // --- Standard cron dom/dow OR semantics (both fields restricted) --- + + @Test + void computeNextFire_domOrDow_firesOnDayOfMonthEvenIfNotWeekday() { + // "0 0 13 * 5" = midnight on the 13th OR any Friday. 2024-01-13 is a Saturday. + // From the 12th (a Friday) at noon, the next fire is the 13th at 00:00 ā€” + // proving day-of-month matches independently of weekday (OR, not AND). + Instant base = ZonedDateTime.of(2024, 1, 12, 12, 0, 0, 0, UTC).toInstant(); + Instant next = CronParser.computeNextFire("0 0 13 * 5", base, UTC); + ZonedDateTime z = next.atZone(UTC); + assertEquals(13, z.getDayOfMonth()); + assertEquals(java.time.DayOfWeek.SATURDAY, z.getDayOfWeek()); + } + + @Test + void computeNextFire_domOrDow_firesOnWeekdayEvenIfNotDayOfMonth() { + // From 2024-01-01 (a Monday), "0 0 13 * 5" next fires on Fri 2024-01-05 ā€” + // a Friday that is not the 13th ā€” proving weekday matches independently. + Instant base = ZonedDateTime.of(2024, 1, 1, 0, 0, 0, 0, UTC).toInstant(); + Instant next = CronParser.computeNextFire("0 0 13 * 5", base, UTC); + ZonedDateTime z = next.atZone(UTC); + assertEquals(java.time.DayOfWeek.FRIDAY, z.getDayOfWeek()); + assertEquals(5, z.getDayOfMonth()); + } + + @Test + void computeNextFire_singleDayFieldRestricted_staysAnd() { + // Only day-of-month restricted (dow is *): must fire strictly on the 1st, + // not on arbitrary weekdays. + Instant base = ZonedDateTime.of(2024, 3, 15, 0, 0, 0, 0, UTC).toInstant(); + Instant next = CronParser.computeNextFire("0 0 1 * *", base, UTC); + assertEquals(ZonedDateTime.of(2024, 4, 1, 0, 0, 0, 0, UTC).toInstant(), next); + } } diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java index 0c34b44f1a..3743a0772c 100644 --- a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java @@ -69,7 +69,7 @@ class ApiCallExecutorBranchCoverageTest { void setUp() throws Exception { openMocks(this); executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, - prePostUtils, globalVariableResolver, secretResolver); + prePostUtils, globalVariableResolver, secretResolver, false); when(memory.getCurrentStep()).thenReturn(currentStep); when(mockRequest.toMap()).thenReturn(new HashMap<>()); diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java index ffb6bb0a86..8342f9964f 100644 --- a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java @@ -53,7 +53,7 @@ void setUp() throws Exception { GlobalVariableResolver globalVariableResolver = mock(GlobalVariableResolver.class); when(globalVariableResolver.resolveValue(anyString())).thenAnswer(inv -> inv.getArgument(0)); - executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, secretResolver); + executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, secretResolver, false); memory = mock(IConversationMemory.class); currentStep = mock(IWritableConversationStep.class); diff --git a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java index a0d28256a1..af6dee47b7 100644 --- a/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java +++ b/src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java @@ -11,6 +11,7 @@ import ai.labs.eddi.engine.httpclient.IHttpClient; import ai.labs.eddi.engine.httpclient.IRequest; import ai.labs.eddi.engine.httpclient.IResponse; +import ai.labs.eddi.engine.lifecycle.exceptions.LifecycleException; import ai.labs.eddi.engine.memory.IConversationMemory; import ai.labs.eddi.engine.memory.IConversationMemory.IWritableConversationStep; import ai.labs.eddi.engine.runtime.IRuntime; @@ -42,6 +43,8 @@ class ApiCallExecutorTest { private IWritableConversationStep currentStep; private IRequest mockRequest; private IResponse mockResponse; + private SecretResolver secretResolver; + private GlobalVariableResolver globalVariableResolver; @BeforeEach void setUp() throws Exception { @@ -49,12 +52,12 @@ void setUp() throws Exception { jsonSerialization = mock(IJsonSerialization.class); runtime = mock(IRuntime.class); prePostUtils = mock(PrePostUtils.class); - SecretResolver secretResolver = mock(SecretResolver.class); + secretResolver = mock(SecretResolver.class); when(secretResolver.resolveValue(anyString())).thenAnswer(inv -> inv.getArgument(0)); - GlobalVariableResolver globalVariableResolver = mock(GlobalVariableResolver.class); + globalVariableResolver = mock(GlobalVariableResolver.class); when(globalVariableResolver.resolveValue(anyString())).thenAnswer(inv -> inv.getArgument(0)); - executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, secretResolver); + executor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, secretResolver, false); memory = mock(IConversationMemory.class); currentStep = mock(IWritableConversationStep.class); @@ -465,6 +468,71 @@ void execute_successfulSave_resultContainsHttpCode() throws Exception { // ==================== Helpers ==================== + // ==================== SSRF Protection (opt-in) ==================== + + @Test + void execute_ssrfProtectionEnabled_blocksInternalUrl() { + ApiCallExecutor protectedExecutor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, + secretResolver, true); + ApiCall call = createSimpleApiCall("ssrf-call", false); + // 169.254.169.254 is a literal IP (no DNS) blocked by UrlValidationUtils. + assertThrows(LifecycleException.class, () -> protectedExecutor.execute(call, memory, new HashMap<>(), "http://169.254.169.254")); + } + + @Test + void execute_ssrfProtectionEnabled_disablesRedirectsOnPublicUrl() throws Exception { + ApiCallExecutor protectedExecutor = new ApiCallExecutor(httpClient, jsonSerialization, runtime, prePostUtils, globalVariableResolver, + secretResolver, true); + ApiCall call = createSimpleApiCall("redir-call", false); + setupSuccessResponse(200, "ok", "text/plain"); + // 1.1.1.1 is a public literal IP ā€” passes validation without a DNS lookup. + protectedExecutor.execute(call, memory, new HashMap<>(), "http://1.1.1.1"); + verify(mockRequest).setFollowRedirects(false); + } + + @Test + void execute_ssrfProtectionDisabled_allowsInternalUrlAndKeepsRedirects() throws Exception { + // Default executor (protection off): no validation, no redirect override. + ApiCall call = createSimpleApiCall("internal-call", false); + setupSuccessResponse(200, "ok", "text/plain"); + executor.execute(call, memory, new HashMap<>(), "http://169.254.169.254"); + verify(mockRequest, never()).setFollowRedirects(anyBoolean()); + } + + // ==================== Exponential Backoff Curve ==================== + + @Test + void getDelayInMillis_isExponentialNotLinear() { + ApiCall call = callWithBackoff(100); + assertEquals(100, ApiCallExecutor.getDelayInMillis(call, true, 1)); // 100 * 2^0 + assertEquals(200, ApiCallExecutor.getDelayInMillis(call, true, 2)); // 100 * 2^1 + assertEquals(400, ApiCallExecutor.getDelayInMillis(call, true, 3)); // 100 * 2^2 + assertEquals(800, ApiCallExecutor.getDelayInMillis(call, true, 4)); // 100 * 2^3 + } + + @Test + void getDelayInMillis_cappedAtCeiling() { + ApiCall call = callWithBackoff(100_000); + // 100000 * 2^9 = 51,200,000 ā€” capped to the 5-minute ceiling. + assertEquals(300_000, ApiCallExecutor.getDelayInMillis(call, true, 10)); + } + + @Test + void getDelayInMillis_noRetry_returnsZeroWithoutPreRequestDelay() { + ApiCall call = callWithBackoff(100); + assertEquals(0, ApiCallExecutor.getDelayInMillis(call, false, 3)); + } + + private ApiCall callWithBackoff(int baseDelayMillis) { + ApiCall call = createSimpleApiCall("backoff", false); + HttpPostResponse postResponse = new HttpPostResponse(); + RetryApiCallInstruction retry = new RetryApiCallInstruction(); + retry.setExponentialBackoffDelayInMillis(baseDelayMillis); + postResponse.setRetryApiCallInstruction(retry); + call.setPostResponse(postResponse); + return call; + } + private ApiCall createSimpleApiCall(String name, boolean saveResponse) { ApiCall call = new ApiCall(); call.setName(name); diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerBranchTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerBranchTest.java index 68fbf13a82..67c668787d 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerBranchTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerBranchTest.java @@ -40,7 +40,7 @@ class A2AToolProviderManagerBranchTest { @BeforeEach void setUp() { openMocks(this); - manager = new A2AToolProviderManager(globalVariableResolver, secretResolver); + manager = new A2AToolProviderManager(globalVariableResolver, secretResolver, false); } // ========================================================= diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerDeepBranchTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerDeepBranchTest.java index 2d5968575e..95db022051 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerDeepBranchTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerDeepBranchTest.java @@ -43,7 +43,7 @@ void setUp() throws IOException { doReturn("resolved-key").when(globalVariableResolver).resolveValue(anyString()); doReturn("resolved-key").when(secretResolver).resolveValue(anyString()); - manager = new A2AToolProviderManager(globalVariableResolver, secretResolver); + manager = new A2AToolProviderManager(globalVariableResolver, secretResolver, false); // Create a lightweight HTTP server for testing real HTTP calls httpServer = HttpServer.create(new InetSocketAddress(0), 0); diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerExtendedTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerExtendedTest.java index 77d442f717..3b05bee8fb 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerExtendedTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerExtendedTest.java @@ -35,7 +35,7 @@ void setUp() { when(secretResolver.resolveValue(anyString())).thenAnswer(i -> i.getArgument(0)); GlobalVariableResolver globalVariableResolver = mock(GlobalVariableResolver.class); when(globalVariableResolver.resolveValue(anyString())).thenAnswer(i -> i.getArgument(0)); - manager = new A2AToolProviderManager(globalVariableResolver, secretResolver); + manager = new A2AToolProviderManager(globalVariableResolver, secretResolver, false); } // ā”€ā”€ā”€ Discovery with unreachable agents ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerTest.java index 3b4511f23f..a42fb6a0ab 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerTest.java @@ -30,7 +30,7 @@ class A2AToolProviderManagerTest { void setUp() { SecretResolver secretResolver = mock(SecretResolver.class); GlobalVariableResolver globalVariableResolver = mock(GlobalVariableResolver.class); - manager = new A2AToolProviderManager(globalVariableResolver, secretResolver); + manager = new A2AToolProviderManager(globalVariableResolver, secretResolver, false); } // ā”€ā”€ā”€ discoverTools empty/null ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ diff --git a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerUncoveredBranchTest.java b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerUncoveredBranchTest.java index a1d32cf6bc..8a5ca4eae1 100644 --- a/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerUncoveredBranchTest.java +++ b/src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerUncoveredBranchTest.java @@ -51,7 +51,7 @@ class A2AToolProviderManagerUncoveredBranchTest { @BeforeEach void setUp() { openMocks(this); - manager = new A2AToolProviderManager(globalVariableResolver, secretResolver); + manager = new A2AToolProviderManager(globalVariableResolver, secretResolver, false); } // ========================================================= From 41e5cedac9201577dae7247e1289a41199b9f72c Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Mon, 29 Jun 2026 22:59:47 +0200 Subject: [PATCH 3/5] refactor(coordinator): make dead-letter cap configurable The dead-letter retention limit was hardcoded at 1000. Expose it as eddi.coordinator.max-dead-letters (default 1000, -1 = unbounded, 0 = retain none), matching the existing eddi.coordinator. max-active-conversations property and EDDI's config-driven design. Tests: +2 (cap evicts oldest; -1 disables). Coordinator suites green. --- docs/changelog.md | 5 +- .../InMemoryConversationCoordinator.java | 34 ++++++++------ src/main/resources/application.properties | 3 ++ .../internal/ConversationCoordinatorTest.java | 2 +- .../InMemoryConversationCoordinatorTest.java | 47 +++++++++++++++++-- 5 files changed, 69 insertions(+), 22 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 6cbb30691e..01ddefde24 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -26,7 +26,7 @@ 5. **`CronParser` ā€” malformed fields crashed or silently never-fired.** `*/` threw `ArrayIndexOutOfBoundsException` (not a clean validation error); a reversed range like `5-1` produced an empty set ā†’ a schedule that never fires until the 2-year scan limit threw a confusing `IllegalStateException`. Both now throw a clear `IllegalArgumentException` at parse time (step structure + `start <= end` checks). 6. **`ApiCallExecutor` retry backoff was linear, not exponential.** `delay * amountOfExecutions` (linear) despite the `exponentialBackoffDelayInMillis` field name. Now true exponential ā€” `base * 2^(attempt-1)` ā€” with an overflow-safe shift and a 5-minute ceiling (`MAX_BACKOFF_MILLIS`). First retry delay is unchanged (`base`), so the change only affects later retries. 7. **`CalculatorTool` ā€” unbounded recursion DoS.** The recursive-descent `SafeMathParser` recurses on nested parens; a long/deeply-nested LLM-supplied expression could throw `StackOverflowError` (an `Error`, not caught by `calculate()`). Added a 1000-char input cap plus a defensive `StackOverflowError` catch. -8. **`InMemoryConversationCoordinator` ā€” unbounded dead-letter deque.** The active-conversation map was capped but `deadLetters` grew without limit under a failure storm. Added a `MAX_DEAD_LETTERS` (1000) cap with oldest-first eviction. +8. **`InMemoryConversationCoordinator` ā€” unbounded dead-letter deque.** The active-conversation map was capped but `deadLetters` grew without limit under a failure storm. Added a **configurable** cap (`eddi.coordinator.max-dead-letters`, default 1000; `-1` disables, `0` retains none) with oldest-first eviction ā€” consistent with the existing `eddi.coordinator.max-active-conversations` property. ### Files changed - `engine/mcp/McpApiToolBuilder.java` ā€” URL validation in `parseSpec`, `looksLikeInlineSpec()` @@ -43,7 +43,8 @@ - `ApiCallExecutorTest` ā€” +6 (SSRF block internal URL, disable redirects on public, protection-off no-op; exponential curve, ceiling cap, no-retry zero) - `CronParserTest` ā€” +6 (DOW 7 = Sunday, 0ā‰”7, OR fires on dom and on weekday, single-restricted stays AND, reversed-range + malformed-step rejection) - `CalculatorToolTest` ā€” +2 (over-long rejected, deep-nesting returns cleanly) -- `ApiCallExecutor`/`A2AToolProviderManager` constructor-call sites updated across 8 test files. +- `InMemoryConversationCoordinatorTest` ā€” +2 (dead-letter cap evicts oldest; `-1` disables) +- `ApiCallExecutor`/`A2AToolProviderManager`/`InMemoryConversationCoordinator` constructor-call sites updated across test files. - Mock-based suites green; A2A + embedded-server suites are unrunnable in the sandbox (JDK `HttpClient`/`HttpServer` can't open a selector) but compile and are exercised in CI. ### Not addressed here (architectural ā€” out of scope for a hardening pass) diff --git a/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java b/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java index 211f8373bf..f86d313ac3 100644 --- a/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java +++ b/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java @@ -65,14 +65,6 @@ public class InMemoryConversationCoordinator implements IConversationCoordinator private static final int MAX_RETRIES = 3; - /** - * Upper bound on retained dead-letter entries. The active-conversation map is - * already capped; without this bound a storm of permanently-failing - * conversations would grow {@link #deadLetters} without limit. Oldest entries - * are evicted first (the dashboard inspects the most recent failures). - */ - private static final int MAX_DEAD_LETTERS = 1000; - private final Map>> conversationQueues = new ConcurrentHashMap<>(); private final ConcurrentLinkedDeque deadLetters = new ConcurrentLinkedDeque<>(); private final AtomicLong totalProcessed = new AtomicLong(0); @@ -83,14 +75,25 @@ public class InMemoryConversationCoordinator implements IConversationCoordinator private final MeterRegistry meterRegistry; private final int maxActiveConversations; + /** + * Upper bound on retained dead-letter entries. The active-conversation map is + * already capped; without this bound a storm of permanently-failing + * conversations would grow {@link #deadLetters} without limit. Oldest entries + * are evicted first (the dashboard inspects the most recent failures). Set to + * {@code -1} to disable the cap (unbounded). + */ + private final int maxDeadLetters; + private static final Logger log = Logger.getLogger(InMemoryConversationCoordinator.class); @Inject public InMemoryConversationCoordinator(IRuntime runtime, MeterRegistry meterRegistry, - @ConfigProperty(name = "eddi.coordinator.max-active-conversations", defaultValue = "10000") int maxActiveConversations) { + @ConfigProperty(name = "eddi.coordinator.max-active-conversations", defaultValue = "10000") int maxActiveConversations, + @ConfigProperty(name = "eddi.coordinator.max-dead-letters", defaultValue = "1000") int maxDeadLetters) { this.runtime = runtime; this.meterRegistry = meterRegistry; this.maxActiveConversations = maxActiveConversations; + this.maxDeadLetters = maxDeadLetters; } @PostConstruct @@ -193,11 +196,14 @@ private void routeToDeadLetter(String conversationId, Throwable failure) { deadLetters.addLast(new DeadLetterEntry(id, conversationId, error, timestamp, payload)); totalDeadLettered.incrementAndGet(); - // Bound memory: evict oldest entries beyond the cap. size() on a - // ConcurrentLinkedDeque is O(n), so only walk when we know we're over. - while (deadLetters.size() > MAX_DEAD_LETTERS) { - if (deadLetters.pollFirst() == null) { - break; + // Bound memory: evict oldest entries beyond the cap (maxDeadLetters < 0 + // disables the cap). size() on a ConcurrentLinkedDeque is O(n), so only + // walk when we know we're over. + if (maxDeadLetters >= 0) { + while (deadLetters.size() > maxDeadLetters) { + if (deadLetters.pollFirst() == null) { + break; + } } } } diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties index f6a7b687ab..04a5be1c25 100644 --- a/src/main/resources/application.properties +++ b/src/main/resources/application.properties @@ -314,4 +314,7 @@ quarkus.otel.sdk.disabled=true # ā•‘ Follow-up messages to existing conversations are always accepted. ā•‘ # ā•šā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā•ā• eddi.coordinator.max-active-conversations=10000 +# Maximum retained dead-letter entries (in-memory coordinator). Oldest are evicted +# first when exceeded. Set to -1 for unbounded, 0 to retain none. +eddi.coordinator.max-dead-letters=1000 diff --git a/src/test/java/ai/labs/eddi/engine/runtime/internal/ConversationCoordinatorTest.java b/src/test/java/ai/labs/eddi/engine/runtime/internal/ConversationCoordinatorTest.java index 0d3f51e2f4..6ed882dbcb 100644 --- a/src/test/java/ai/labs/eddi/engine/runtime/internal/ConversationCoordinatorTest.java +++ b/src/test/java/ai/labs/eddi/engine/runtime/internal/ConversationCoordinatorTest.java @@ -33,7 +33,7 @@ class ConversationCoordinatorTest { @BeforeEach void setUp() { runtime = mock(IRuntime.class); - coordinator = new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 10000); + coordinator = new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 10000, 1000); } @Test diff --git a/src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java b/src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java index 1137e95c10..bec0fc431f 100644 --- a/src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java +++ b/src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java @@ -35,7 +35,7 @@ class InMemoryConversationCoordinatorTest { @BeforeEach void setUp() { runtime = mock(IRuntime.class); - coordinator = new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 10000); + coordinator = new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 10000, 1000); } // ==================== Status ==================== @@ -159,6 +159,39 @@ void shouldReturnFalseReplayingNonExistent() { assertFalse(coordinator.replayDeadLetter("nonexistent")); } + @Test + @SuppressWarnings("unchecked") + void shouldCapDeadLettersAndEvictOldest() { + IRuntime localRuntime = mock(IRuntime.class); + // maxDeadLetters = 2 + var capped = new InMemoryConversationCoordinator(localRuntime, new SimpleMeterRegistry(), 10000, 2); + + causeDeadLetter(capped, localRuntime, "conv-1", mock(Callable.class)); + causeDeadLetter(capped, localRuntime, "conv-2", mock(Callable.class)); + causeDeadLetter(capped, localRuntime, "conv-3", mock(Callable.class)); + + // Cap is 2 ā†’ oldest (conv-1) evicted; all 3 still counted in the total. + assertEquals(2, capped.getDeadLetters().size()); + assertEquals(3, capped.getTotalDeadLettered()); + var ids = capped.getDeadLetters().stream().map(DeadLetterEntry::conversationId).toList(); + assertTrue(ids.contains("conv-2") && ids.contains("conv-3")); + assertFalse(ids.contains("conv-1")); + } + + @Test + @SuppressWarnings("unchecked") + void shouldNotCapDeadLettersWhenDisabled() { + IRuntime localRuntime = mock(IRuntime.class); + // maxDeadLetters = -1 ā†’ unbounded + var uncapped = new InMemoryConversationCoordinator(localRuntime, new SimpleMeterRegistry(), 10000, -1); + + causeDeadLetter(uncapped, localRuntime, "c-1", mock(Callable.class)); + causeDeadLetter(uncapped, localRuntime, "c-2", mock(Callable.class)); + causeDeadLetter(uncapped, localRuntime, "c-3", mock(Callable.class)); + + assertEquals(3, uncapped.getDeadLetters().size()); + } + @Test @SuppressWarnings("unchecked") void shouldReportQueueDepths() { @@ -178,7 +211,7 @@ void shouldReportQueueDepths() { @SuppressWarnings("unchecked") void shouldRejectNewConversationAtCapacity() { // Create coordinator with maxActiveConversations=2 - var smallCoordinator = new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 2); + var smallCoordinator = new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 2, 1000); Callable task1 = mock(Callable.class); Callable task2 = mock(Callable.class); @@ -196,7 +229,7 @@ void shouldRejectNewConversationAtCapacity() { @SuppressWarnings("unchecked") void shouldAllowFollowUpToExistingConversationAtCapacity() { // Create coordinator with maxActiveConversations=2 - var smallCoordinator = new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 2); + var smallCoordinator = new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 2, 1000); Callable task1 = mock(Callable.class); Callable task2 = mock(Callable.class); @@ -282,12 +315,16 @@ void shouldHandleResubmitAfterDrain() throws Exception { @SuppressWarnings("unchecked") private void causeDeadLetter(String conversationId, Callable task) { - coordinator.submitInOrder(conversationId, task); + causeDeadLetter(coordinator, runtime, conversationId, task); + } + + private void causeDeadLetter(InMemoryConversationCoordinator coord, IRuntime rt, String conversationId, Callable task) { + coord.submitInOrder(conversationId, task); // Simulate MAX_RETRIES (3) failures for (int i = 0; i < 3; i++) { ArgumentCaptor> captor = ArgumentCaptor.forClass(IRuntime.IFinishedExecution.class); - verify(runtime, atLeast(1)).submitCallable(eq(task), captor.capture(), isNull()); + verify(rt, atLeast(1)).submitCallable(eq(task), captor.capture(), isNull()); List> callbacks = captor.getAllValues(); callbacks.get(callbacks.size() - 1).onFailure(new RuntimeException("forced failure")); From 890363f3f35319d053291e280ccff573ad0f7688 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Mon, 29 Jun 2026 23:23:38 +0200 Subject: [PATCH 4/5] refactor: address bot-review feedback (fail-closed redirects, O(n) eviction, Vixie star, field-aware cron errors) From Copilot + CodeRabbit review of the branch: - IRequest.setFollowRedirects is now abstract (not a no-op default), so a new IRequest impl cannot silently re-enable the SSRF redirect bypass. - InMemoryConversationCoordinator dead-letter eviction computes the excess once and evicts that many (was size()-per-iteration -> potential O(n^2)). - CronParser: a day field is "starred" when it begins with '*' (so */2 takes the standard AND path, not OR) - matches Vixie DOM_STAR/DOW_STAR. - CronParser: parseIntField wraps NumberFormatException into a field-aware IllegalArgumentException (e.g. */abc) instead of a vague parse message. - CalculatorTool: length guard runs before the eager debug-log concatenation, so oversized input isn't built/logged. Not changed: OpenAPI external $ref resolution stays on - disabling it breaks legitimate in-document #/components refs; documented as an accepted, role-gated residual in docs/changelog.md. Tests: CronParserTest +2 (non-numeric step, */2 AND path); affected suites green (CronParser 30, ApiCallExecutor 38, coordinator 18/6, Calculator 79). --- docs/changelog.md | 10 ++++++ .../labs/eddi/engine/httpclient/IRequest.java | 13 ++++--- .../engine/runtime/internal/CronParser.java | 35 +++++++++++++------ .../InMemoryConversationCoordinator.java | 6 ++-- .../llm/tools/impl/CalculatorTool.java | 11 +++--- .../runtime/internal/CronParserTest.java | 22 +++++++++++- 6 files changed, 70 insertions(+), 27 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 01ddefde24..0dc742c4f2 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -47,6 +47,16 @@ - `ApiCallExecutor`/`A2AToolProviderManager`/`InMemoryConversationCoordinator` constructor-call sites updated across test files. - Mock-based suites green; A2A + embedded-server suites are unrunnable in the sandbox (JDK `HttpClient`/`HttpServer` can't open a selector) but compile and are exercised in CI. +### Review follow-ups (Copilot + CodeRabbit) +- **`IRequest.setFollowRedirects` fails closed** ā€” made it a non-default (abstract) interface method instead of a no-op default, so any new `IRequest` impl must honour it and cannot silently re-enable the redirect bypass. +- **Coordinator eviction is O(n), not O(nĀ²)** ā€” compute the dead-letter excess once and evict that many, instead of calling `ConcurrentLinkedDeque.size()` per loop iteration. +- **`CronParser` Vixie star semantics** ā€” a day field is "starred" (not restricted, takes the AND path) when it *begins* with `*`, so `*/2` is treated like `*` (was exact `equals("*")`, which wrongly took the OR path). +- **`CronParser` field-aware parse errors** ā€” `parseIntField()` wraps `NumberFormatException` into an `IllegalArgumentException` carrying the offending field (e.g. `*/abc` ā†’ "Invalid number 'abc' in field: ā€¦"), instead of leaking a vague low-level message. +- **`CalculatorTool` guards before logging** ā€” the length check now runs before the eager `LOGGER.debug("ā€¦ " + expression)` concatenation, so an oversized payload is rejected without building/logging the big string. + +### Known residual (accepted, documented) +- **OpenAPI external `$ref` resolution** (`McpApiToolBuilder`, `setResolve(true)`): the http(s) gate validates the top-level spec location but not external `$ref`s inside the spec, so a crafted spec can still make the parser fetch a remote/file ref. Disabling resolution (`setResolve(false)`) would also break legitimate in-document `#/components` refs that real specs rely on, so resolution is kept on. Mitigated by the `eddi-admin`/`eddi-editor` gate; a constrained ref-resolver is the proper (heavier) fix. + ### Not addressed here (architectural ā€” out of scope for a hardening pass) - **Open-by-default MCP/admin surface** and **role- vs tenant-based isolation** for config resources. - **Conversation-memory 16 MB BSON ceiling** ā€” needs a proper step-archival design, not a quick guard. diff --git a/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java b/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java index 5665607646..f03a5f5665 100644 --- a/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java +++ b/src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java @@ -25,14 +25,13 @@ public interface IRequest { /** * Enable or disable automatic HTTP redirect following for this request. *

- * The default implementation is a no-op (preserves the client's configured - * behaviour); the Vert.x-backed implementation honours it. SSRF-protected - * callers disable redirects to prevent a {@code 3xx ā†’ internal host} bypass of - * URL validation. + * SSRF-protected callers disable redirects to prevent a + * {@code 3xx ā†’ internal host} bypass of URL validation. This is intentionally + * not a default no-op: any {@link IRequest} implementation must honour + * it (or explicitly throw) so a new client cannot silently re-enable the + * redirect bypass ā€” it fails closed at compile time instead. */ - default IRequest setFollowRedirects(boolean follow) { - return this; - } + IRequest setFollowRedirects(boolean follow); IResponse send() throws HttpRequestException; diff --git a/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java b/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java index 7968913d0d..10bb1ecab6 100644 --- a/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java +++ b/src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java @@ -81,10 +81,11 @@ public static Instant computeNextFire(String cronExpression, Instant after, Zone Set daysOfWeek = normalizeDaysOfWeek(parseField(substituteNames(parts[4], DOW_NAMES), 0, 7)); // Standard (Vixie) cron: when BOTH day-of-month and day-of-week are - // restricted (neither is "*"), a day matches if EITHER field matches. - // If only one is restricted, the "*" field is always true, so the AND - // below naturally reduces to the restricted field. - boolean bothDayFieldsRestricted = !parts[2].trim().equals("*") && !parts[4].trim().equals("*"); + // restricted, a day matches if EITHER field matches. A field is "starred" + // (not restricted) when it begins with "*" ā€” this includes "*/2", matching + // Vixie's DOM_STAR/DOW_STAR flag. If only one is restricted, the starred + // field is always true, so the AND below reduces to the restricted field. + boolean bothDayFieldsRestricted = !parts[2].trim().startsWith("*") && !parts[4].trim().startsWith("*"); // Walk forward minute-by-minute from 'after + 1 minute' (aligned to minute // boundary) @@ -145,7 +146,7 @@ static Set parseField(String field, int min, int max) { if (stepParts.length != 2) { throw new IllegalArgumentException("Invalid step expression '" + part + "' in field: " + field); } - int step = Integer.parseInt(stepParts[1]); + int step = parseIntField(stepParts[1], field); if (step <= 0) throw new IllegalArgumentException("Step must be > 0: " + field); int start = min; @@ -156,10 +157,10 @@ static Set parseField(String field, int min, int max) { if (range.length != 2) { throw new IllegalArgumentException("Invalid range expression '" + stepParts[0] + "' in field: " + field); } - start = Integer.parseInt(range[0]); - end = Integer.parseInt(range[1]); + start = parseIntField(range[0], field); + end = parseIntField(range[1], field); } else { - start = Integer.parseInt(stepParts[0]); + start = parseIntField(stepParts[0], field); } } if (start > end) { @@ -174,8 +175,8 @@ static Set parseField(String field, int min, int max) { if (range.length != 2) { throw new IllegalArgumentException("Invalid range expression '" + part + "' in field: " + field); } - int start = Integer.parseInt(range[0]); - int end = Integer.parseInt(range[1]); + int start = parseIntField(range[0], field); + int end = parseIntField(range[1], field); if (start > end) { throw new IllegalArgumentException("Range start must be <= end ('" + part + "') in field: " + field); } @@ -185,7 +186,7 @@ static Set parseField(String field, int min, int max) { } else if (part.equals("*")) { IntStream.rangeClosed(min, max).forEach(values::add); } else { - values.add(Integer.parseInt(part)); + values.add(parseIntField(part, field)); } } @@ -198,6 +199,18 @@ static Set parseField(String field, int min, int max) { return values; } + /** + * Parse an integer cron token, wrapping low-level {@link NumberFormatException} + * into a field-aware {@link IllegalArgumentException} for actionable errors. + */ + private static int parseIntField(String token, String field) { + try { + return Integer.parseInt(token.trim()); + } catch (NumberFormatException e) { + throw new IllegalArgumentException("Invalid number '" + token + "' in field: " + field, e); + } + } + /** * Determine whether the candidate's date matches the day-of-month and * day-of-week sets, applying standard cron semantics: OR when both fields are diff --git a/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java b/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java index f86d313ac3..5b758c135e 100644 --- a/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java +++ b/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java @@ -197,10 +197,10 @@ private void routeToDeadLetter(String conversationId, Throwable failure) { totalDeadLettered.incrementAndGet(); // Bound memory: evict oldest entries beyond the cap (maxDeadLetters < 0 - // disables the cap). size() on a ConcurrentLinkedDeque is O(n), so only - // walk when we know we're over. + // disables the cap). size() on a ConcurrentLinkedDeque is O(n), so compute + // the excess once and evict that many rather than calling size() per loop. if (maxDeadLetters >= 0) { - while (deadLetters.size() > maxDeadLetters) { + for (int excess = deadLetters.size() - maxDeadLetters; excess > 0; excess--) { if (deadLetters.pollFirst() == null) { break; } diff --git a/src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java b/src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java index 979512dd09..6573502476 100644 --- a/src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java +++ b/src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java @@ -35,19 +35,20 @@ public class CalculatorTool { public String calculate(@P("expression") String expression) { try { - LOGGER.debug("Calculating expression: " + expression); - if (expression == null || expression.isBlank()) { return "Error: Expression must not be empty."; } - // Bound input length: the recursive-descent parser recurses on nested - // parentheses, so an over-long expression could exhaust the stack - // (StackOverflowError is an Error, not caught below). + // Bound input length BEFORE logging/parsing: the recursive-descent parser + // recurses on nested parentheses, so an over-long expression could exhaust + // the stack (StackOverflowError is an Error, not caught below) and the + // eager log concatenation below would also process the oversized payload. if (expression.length() > MAX_EXPRESSION_LENGTH) { return "Error: Expression too long (max " + MAX_EXPRESSION_LENGTH + " characters)."; } + LOGGER.debug("Calculating expression: " + expression); + double result = new SafeMathParser(expression).parse(); // Handle special values diff --git a/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java b/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java index fb6d5aa627..16f5c59642 100644 --- a/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java +++ b/src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java @@ -193,7 +193,16 @@ void parseField_rejectsReversedRange() { @Test void parseField_rejectsMalformedStep() { - assertThrows(IllegalArgumentException.class, () -> CronParser.parseField("*/", 0, 59)); + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> CronParser.parseField("*/", 0, 59)); + // Must be a field-aware cron error, not a leaked low-level parse message. + assertTrue(ex.getMessage().toLowerCase().contains("step") || ex.getMessage().toLowerCase().contains("field"), + "Expected a field-aware cron error, got: " + ex.getMessage()); + } + + @Test + void parseField_rejectsNonNumericStep() { + IllegalArgumentException ex = assertThrows(IllegalArgumentException.class, () -> CronParser.parseField("*/abc", 0, 59)); + assertTrue(ex.getMessage().contains("field"), "Expected field context in message, got: " + ex.getMessage()); } // --- Standard cron dom/dow OR semantics (both fields restricted) --- @@ -229,4 +238,15 @@ void computeNextFire_singleDayFieldRestricted_staysAnd() { Instant next = CronParser.computeNextFire("0 0 1 * *", base, UTC); assertEquals(ZonedDateTime.of(2024, 4, 1, 0, 0, 0, 0, UTC).toInstant(), next); } + + @Test + void computeNextFire_starSlashStepInDayField_usesAndNotOr() { + // "0 0 */2 * 1": */2 day-of-month is "starred" (Vixie DOM_STAR), so this is + // AND with Mondays, not OR. */2 over 1..31 yields odd days; the next + // odd-numbered Monday after 2024-01-01 is 2024-01-15. (An OR reading would + // instead fire on the next odd day, 2024-01-03.) + Instant base = ZonedDateTime.of(2024, 1, 1, 0, 0, 0, 0, UTC).toInstant(); + Instant next = CronParser.computeNextFire("0 0 */2 * 1", base, UTC); + assertEquals(ZonedDateTime.of(2024, 1, 15, 0, 0, 0, 0, UTC).toInstant(), next); + } } From 9802af84c6d364c97c3f40230b17ee0cb9469381 Mon Sep 17 00:00:00 2001 From: Gregor Jarisch Date: Mon, 29 Jun 2026 23:31:42 +0200 Subject: [PATCH 5/5] refactor(coordinator): validate max-dead-letters sentinel + serialize add/trim Addresses CodeRabbit review of the dead-letter cap: - Reject eddi.coordinator.max-dead-letters < -1 at startup. Only -1 (unbounded), 0 (retain none) and positive values are valid; a typo like -2 would otherwise silently disable trimming. - Serialize the dead-letter add+trim under a dedicated lock so concurrent failures enforce the cap deterministically. (pollFirst already evicts oldest-first, so the newest failures were never dropped; the lock just removes transient under-retention below the cap.) Tests: +2 (reject -2; accept -1 and 0). Coordinator suites green (20/6). --- docs/changelog.md | 1 + .../InMemoryConversationCoordinator.java | 26 +++++++++++++------ .../InMemoryConversationCoordinatorTest.java | 13 ++++++++++ 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index 0dc742c4f2..7f5ac2ad8c 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -50,6 +50,7 @@ ### Review follow-ups (Copilot + CodeRabbit) - **`IRequest.setFollowRedirects` fails closed** ā€” made it a non-default (abstract) interface method instead of a no-op default, so any new `IRequest` impl must honour it and cannot silently re-enable the redirect bypass. - **Coordinator eviction is O(n), not O(nĀ²)** ā€” compute the dead-letter excess once and evict that many, instead of calling `ConcurrentLinkedDeque.size()` per loop iteration. +- **Coordinator dead-letter cap hardening** ā€” reject `max-dead-letters < -1` at startup (only `-1`/`0`/positive are valid, so a typo like `-2` can't silently disable trimming), and serialize the add+trim under a small lock so concurrent failures enforce the cap deterministically (the existing `pollFirst` already evicts oldest-first, so the newest failures were never dropped ā€” the lock just removes transient under-retention). - **`CronParser` Vixie star semantics** ā€” a day field is "starred" (not restricted, takes the AND path) when it *begins* with `*`, so `*/2` is treated like `*` (was exact `equals("*")`, which wrongly took the OR path). - **`CronParser` field-aware parse errors** ā€” `parseIntField()` wraps `NumberFormatException` into an `IllegalArgumentException` carrying the offending field (e.g. `*/abc` ā†’ "Invalid number 'abc' in field: ā€¦"), instead of leaking a vague low-level message. - **`CalculatorTool` guards before logging** ā€” the length check now runs before the eager `LOGGER.debug("ā€¦ " + expression)` concatenation, so an oversized payload is rejected without building/logging the big string. diff --git a/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java b/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java index 5b758c135e..9abfed6b74 100644 --- a/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java +++ b/src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java @@ -67,6 +67,8 @@ public class InMemoryConversationCoordinator implements IConversationCoordinator private final Map>> conversationQueues = new ConcurrentHashMap<>(); private final ConcurrentLinkedDeque deadLetters = new ConcurrentLinkedDeque<>(); + /** Serializes dead-letter add+trim so the cap is enforced deterministically. */ + private final Object deadLetterLock = new Object(); private final AtomicLong totalProcessed = new AtomicLong(0); private final AtomicLong totalDeadLettered = new AtomicLong(0); private final AtomicLong deadLetterIdCounter = new AtomicLong(0); @@ -90,6 +92,10 @@ public class InMemoryConversationCoordinator implements IConversationCoordinator public InMemoryConversationCoordinator(IRuntime runtime, MeterRegistry meterRegistry, @ConfigProperty(name = "eddi.coordinator.max-active-conversations", defaultValue = "10000") int maxActiveConversations, @ConfigProperty(name = "eddi.coordinator.max-dead-letters", defaultValue = "1000") int maxDeadLetters) { + if (maxDeadLetters < -1) { + throw new IllegalArgumentException( + "eddi.coordinator.max-dead-letters must be >= -1 (-1 = unbounded, 0 = retain none), got " + maxDeadLetters); + } this.runtime = runtime; this.meterRegistry = meterRegistry; this.maxActiveConversations = maxActiveConversations; @@ -193,16 +199,20 @@ private void routeToDeadLetter(String conversationId, Throwable failure) { String payload = String.format("{\"conversationId\":\"%s\",\"error\":\"%s\",\"timestamp\":%d}", conversationId, error.replace("\"", "\\\""), timestamp); - deadLetters.addLast(new DeadLetterEntry(id, conversationId, error, timestamp, payload)); totalDeadLettered.incrementAndGet(); - // Bound memory: evict oldest entries beyond the cap (maxDeadLetters < 0 - // disables the cap). size() on a ConcurrentLinkedDeque is O(n), so compute - // the excess once and evict that many rather than calling size() per loop. - if (maxDeadLetters >= 0) { - for (int excess = deadLetters.size() - maxDeadLetters; excess > 0; excess--) { - if (deadLetters.pollFirst() == null) { - break; + // Serialize add+trim so concurrent failures enforce the cap deterministically + // (without the lock, parallel trims could transiently leave the deque a few + // entries below the cap). pollFirst() evicts the oldest; the just-added entry + // is at the tail, so the newest failures are always retained (for cap > 0). + // size() on a ConcurrentLinkedDeque is O(n), so the excess is computed once. + synchronized (deadLetterLock) { + deadLetters.addLast(new DeadLetterEntry(id, conversationId, error, timestamp, payload)); + if (maxDeadLetters >= 0) { + for (int excess = deadLetters.size() - maxDeadLetters; excess > 0; excess--) { + if (deadLetters.pollFirst() == null) { + break; + } } } } diff --git a/src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java b/src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java index bec0fc431f..b11bd912b8 100644 --- a/src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java +++ b/src/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java @@ -192,6 +192,19 @@ void shouldNotCapDeadLettersWhenDisabled() { assertEquals(3, uncapped.getDeadLetters().size()); } + @Test + void shouldRejectMaxDeadLettersBelowMinusOne() { + // -1 (unbounded) and 0 (retain none) are the only valid sentinels; -2 is a + // typo. + assertThrows(IllegalArgumentException.class, () -> new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 10000, -2)); + } + + @Test + void shouldAcceptUnboundedAndZeroSentinels() { + assertDoesNotThrow(() -> new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 10000, -1)); + assertDoesNotThrow(() -> new InMemoryConversationCoordinator(runtime, new SimpleMeterRegistry(), 10000, 0)); + } + @Test @SuppressWarnings("unchecked") void shouldReportQueueDepths() {