Fix/security and algo hardening - #576
Merged
Merged
Conversation
…rden 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.
… 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.
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
📝 WalkthroughWalkthroughAdds opt-in SSRF protection across outbound HTTP flows, hardens cron parsing and validation, caps dead-letter retention, and adds calculator input guards. It also updates related tests, config defaults, and the changelog. ChangesSecurity & Algorithm Hardening
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens several core modules against SSRF / file-read vectors, corrects cron parsing semantics to align with standard cron behavior, and adds resource-exhaustion guards—while keeping the new SSRF protections opt-in (disabled by default via configuration).
Changes:
- Hardened outbound URL handling (OpenAPI spec discovery scheme-gate; optional SSRF validation + redirect disabling for agent-driven HTTP calls and A2A peer calls).
- Fixed cron parsing bugs (DOW
7support; dom/dow OR semantics; clearer malformed-field handling). - Added DoS/resource guards (calculator expression length/stack defense; dead-letter queue cap) and expanded test coverage.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java | Blocks non-http(s) spec locations; adds inline-vs-location heuristic to prevent file reads via swagger-parser. |
| src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java | Adds opt-in SSRF validation + disables redirects per request; fixes exponential backoff algorithm with a ceiling. |
| src/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.java | Adds opt-in SSRF validation for agent card fetch and task execution URLs. |
| src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java | Adds per-request redirect control API (default no-op). |
| src/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.java | Implements per-request redirect control for the Vert.x-backed client wrapper. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java | Corrects DOW range and dom/dow semantics; adds malformed step/range validation. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java | Caps dead-letter retention to prevent unbounded growth under failure storms. |
| src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java | Adds expression length cap and StackOverflowError handling for deeply nested inputs. |
| src/main/resources/application.properties | Documents and introduces eddi.security.ssrf-protection.enabled (default false). |
| src/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.java | Adds tests for scheme-gating and inline spec handling. |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java | Adds tests for opt-in SSRF behavior, redirect disabling, and exponential backoff curve/cap. |
| src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java | Adds tests for DOW 7 handling, dom/dow OR semantics, and malformed-field rejection. |
| src/test/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorToolTest.java | Adds robustness tests for length cap and deep nesting behavior. |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.java | Updates constructor usage to include new SSRF flag parameter. |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.java | Updates constructor usage to include new SSRF flag parameter. |
| src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerTest.java | Updates constructor usage to include new SSRF flag parameter. |
| src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerExtendedTest.java | Updates constructor usage to include new SSRF flag parameter. |
| src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerBranchTest.java | Updates constructor usage to include new SSRF flag parameter. |
| src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerDeepBranchTest.java | Updates constructor usage to include new SSRF flag parameter. |
| src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerUncoveredBranchTest.java | Updates constructor usage to include new SSRF flag parameter. |
| docs/changelog.md | Adds a detailed changelog entry describing the security and algorithm changes and associated tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/ai/labs/eddi/engine/httpclient/IRequest.java`:
- Around line 25-35: The default setFollowRedirects(boolean) in IRequest is
currently a silent no-op, which can let unsupported request implementations
bypass SSRF protections without notice. Change IRequest so redirect control
cannot be ignored: either make setFollowRedirects(boolean) abstract or have the
default implementation fail fast by throwing, and ensure any concrete
implementation like the Vert.x-backed request class explicitly overrides it to
handle redirect behavior.
In `@src/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.java`:
- Around line 168-183: The OpenAPI parsing path in McpApiToolBuilder still
allows nested external $ref fetches because ParseOptions is configured with
resolve enabled for both readContents and readLocation. Update the parsing flow
to prevent automatic resolution here, or ensure that any external reference
fetching is routed through the same http(s)-only policy enforced by
looksLikeInlineSpec, UrlValidationUtils.isValidHttpUrl, and the OpenAPIV3Parser
calls so untrusted specs cannot trigger unrestricted fetches.
In `@src/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.java`:
- Around line 83-87: The day-field star detection in CronParser is too strict
because it only treats exact "*" as starred, so expressions like "*/2" are
misclassified. Update the logic around bothDayFieldsRestricted in CronParser to
use Vixie-style star semantics for the day-of-month and day-of-week fields, and
rely on that check when deciding the AND vs OR path so leading "*/" patterns are
handled as starred fields.
In `@src/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.java`:
- Around line 44-50: Move the expression length guard in CalculatorTool before
any logging or eager string concatenation of the user input. The current
validation in the expression evaluation flow happens after the full expression
has already been built for logs, so update the relevant method in CalculatorTool
to check MAX_EXPRESSION_LENGTH first and return the error immediately, then only
log/process the expression if it passes the guard.
In `@src/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.java`:
- Around line 194-197: The malformed-step test in CronParserTest is too broad
because assertThrows(IllegalArgumentException.class, ...) also accepts
NumberFormatException, so tighten it to verify the domain-specific failure from
CronParser.parseField("*/", 0, 59) by asserting on the exception message or
explicitly checking the thrown type/message to ensure a cron-validation error is
raised instead of a low-level parse error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fd5a9fa-cb61-41d4-898a-095d07ca56a4
📒 Files selected for processing (21)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/httpclient/IRequest.javasrc/main/java/ai/labs/eddi/engine/httpclient/impl/HttpClientWrapper.javasrc/main/java/ai/labs/eddi/engine/mcp/McpApiToolBuilder.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/CronParser.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.javasrc/main/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorTool.javasrc/main/resources/application.propertiessrc/test/java/ai/labs/eddi/engine/mcp/McpApiToolBuilderTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/CronParserTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorBranchCoverageTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorExtendedTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerDeepBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerUncoveredBranchTest.javasrc/test/java/ai/labs/eddi/modules/llm/tools/impl/CalculatorToolTest.java
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java`:
- Around line 199-207: Serialize the dead-letter enqueue and trim logic in
routeToDeadLetter() so concurrent callers cannot interleave addLast() with the
maxDeadLetters eviction loop. Add a small lock around the deadLetters update
block in InMemoryConversationCoordinator to make the cap enforcement
deterministic and prevent over-eviction under concurrent failures.
- Around line 89-96: Reject invalid dead-letter limits in
InMemoryConversationCoordinator at startup by validating the maxDeadLetters
constructor argument. Keep -1 as the only allowed unbounded sentinel, allow 0
and positive values, and throw an initialization error for any value less than
-1 so typos cannot silently disable trimming. Make the check in the
InMemoryConversationCoordinator constructor right after the injected config
values are assigned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9911ef11-3194-4281-854c-2700ce949d2c
📒 Files selected for processing (5)
docs/changelog.mdsrc/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.javasrc/main/resources/application.propertiessrc/test/java/ai/labs/eddi/engine/runtime/internal/ConversationCoordinatorTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinatorTest.java
✅ Files skipped from review due to trivial changes (1)
- docs/changelog.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/application.properties
…iction, 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).
… 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).
rolandpickl
approved these changes
Jun 30, 2026
This was referenced Jul 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request introduces a comprehensive set of security hardening and algorithm bug fixes across several core modules, with a focus on SSRF/file-read protections, cron scheduling correctness, retry/backoff logic, and resource usage guards. Most changes are behavior-preserving for valid input, with two intentional behavior corrections in cron parsing and retry backoff. New SSRF protections are opt-in and off by default to ensure backward compatibility.
Security and SSRF Protections:
http(s)URLs, preventing local-file reads and non-http SSRF vectors (e.g.,file://,classpath:). Inline content detection is improved with a newlooksLikeInlineSpec()method. [1] [2] [3]eddi.security.ssrf-protection.enabledflag (default off). When enabled, all agent-driven outbound URLs are validated to block private/loopback/link-local/CGNAT/cloud-metadata and non-http targets. Redirect following can be disabled per request via the newsetFollowRedirectsmethod inIRequest, now supported in Vert.x. [1] [2]Algorithm and Logic Corrections:
7as Sunday and normalizes it to0, matching standard cron semantics. [1] [2]IllegalArgumentExceptions if invalid or reversed. [1] [2]Resource Usage Guards:
InMemoryConversationCoordinatornow caps the dead-letter queue length via a configurable property (eddi.coordinator.max-dead-letters, default 1000,-1disables). Oldest entries are evicted first to prevent unbounded memory growth. [1] [2]Other Notable Changes:
These changes significantly improve the security posture and correctness of the system, while preserving expected behavior for valid, existing configurations.
Type of Change
Checklist
./mvnw clean verify -DskipITs)Summary by CodeRabbit