Fix/codeql remediation pr455#458
Merged
Merged
Conversation
…verflow, log injection High severity (4 findings): - CronDescriber: add v >= 0 guard in formatSet() (2 findings) - RestConversationStore: clamp pagination params, guard index++ overflow (1 finding) - DescriptorStore: use long arithmetic for skip calculation (1 finding, 30 annotations) Medium severity (77 findings): - Create LogSanitizer utility (CWE-117 log injection prevention) - Wrap user-controlled values with sanitize() across 13 files: RestSecretStore, RagIngestionService, ExpressionProvider, ToolRateLimiter, ToolCostTracker, ToolCacheService, McpToolProviderManager, LlmTask, EmbeddingStoreFactory, ConversationSummarizer, ChatModelRegistry, AgentOrchestrator, RestSlackWebhook
…gSanitizer.sanitize() Replaced private sanitizeForLog() methods in 6 files with the centralized LogSanitizer.sanitize() utility. The centralized version is more thorough: handles tabs and all control characters (0x00-0x1F, 0x7F), not just CR/LF. Files consolidated: - InMemoryConversationCoordinator - NatsConversationCoordinator - RestAgentEngineStreaming - RestCoordinatorAdmin - InMemoryTenantQuotaStore - RestAgentGroupStore Also fixed Javadoc encoding issue in LogSanitizer (em-dash in hex range) and added braces to if-statement for project style consistency.
…Service and EmbeddingStoreFactory
…er bounds, and pagination clamping
Contributor
📝 WalkthroughWalkthroughCentralizes log sanitization via a new Changes
Sequence Diagram(s)(Skipped — changes are widespread logging and defensive fixes without a single new multi-component sequential flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Remediates CodeQL findings across the EDDI codebase by centralizing log value sanitization (CWE-117) and adding guards against array bounds and integer overflow issues in pagination/skip calculations.
Changes:
- Introduces
LogSanitizer.sanitize(String)and applies it to user-controlled values across multiple log statements. - Fixes bounds/overflow issues in
CronDescriber,RestConversationStorepagination, andDescriptorStoreskip calculation. - Adds/updates unit tests to cover the sanitizer behavior and the bounds/overflow fixes.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/ai/labs/eddi/utils/LogSanitizer.java | New centralized log-sanitization utility. |
| src/test/java/ai/labs/eddi/utils/LogSanitizerTest.java | New tests covering sanitizer behavior and attack patterns. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/CronDescriber.java | Adds v >= 0 guard to prevent negative index access. |
| src/test/java/ai/labs/eddi/engine/runtime/internal/CronDescriberExtendedTest.java | Adds coverage for formatSet() negative/out-of-bounds cases (via reflection). |
| src/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.java | Clamps pagination params and guards index++ overflow. |
| src/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.java | Adds tests for pagination parameter clamping behavior. |
| src/main/java/ai/labs/eddi/datastore/DescriptorStore.java | Prevents index * limit overflow by using long and clamping. |
| src/main/java/ai/labs/eddi/secrets/rest/RestSecretStore.java | Sanitizes tenant/key values in error logs. |
| src/main/java/ai/labs/eddi/modules/rag/RagIngestionService.java | Sanitizes KB/document identifiers in logs. |
| src/main/java/ai/labs/eddi/modules/nlp/expressions/utilities/ExpressionProvider.java | Sanitizes expression text in parse error logs. |
| src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java | Sanitizes tool name in rate-limit logs. |
| src/main/java/ai/labs/eddi/modules/llm/tools/ToolCostTracker.java | Sanitizes tool/conversation identifiers in logs. |
| src/main/java/ai/labs/eddi/modules/llm/tools/ToolCacheService.java | Sanitizes tool name in cache debug logs. |
| src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java | Sanitizes MCP server name/URL in discovery/connect logs. |
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Sanitizes conversation id in summary warning logs. |
| src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactory.java | Sanitizes store/config fields in embedding-store build logs. |
| src/main/java/ai/labs/eddi/modules/llm/impl/ConversationSummarizer.java | Sanitizes conversation id in summary logs. |
| src/main/java/ai/labs/eddi/modules/llm/impl/ChatModelRegistry.java | Sanitizes secret reference fields in eviction logs. |
| src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java | Sanitizes IDs/budget text in logs. |
| src/main/java/ai/labs/eddi/integrations/slack/rest/RestSlackWebhook.java | Sanitizes Slack event type/id in debug logs. |
| src/main/java/ai/labs/eddi/engine/tenancy/InMemoryTenantQuotaStore.java | Replaces local sanitization helper with centralized sanitizer. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java | Replaces local sanitization helper with centralized sanitizer. |
| src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java | Replaces local sanitization helper with centralized sanitizer. |
| src/main/java/ai/labs/eddi/engine/internal/RestCoordinatorAdmin.java | Uses centralized sanitizer in errors/logging for dead-letter actions. |
| src/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.java | Uses centralized sanitizer for conversation id in logs. |
| src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.java | Updates test to validate centralized sanitizer behavior. |
| src/main/java/ai/labs/eddi/configs/groups/rest/RestAgentGroupStore.java | Removes custom sanitization logic in favor of centralized sanitizer. |
| docs/changelog.md | Documents the remediation work and affected areas. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…bug log guards, brace style, RestRagIngestion
…o resolve CodeQL log injection
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/ai/labs/eddi/integrations/slack/rest/RestSlackWebhook.java (1)
99-99:⚠️ Potential issue | 🟡 MinorUnsanitized header value in adjacent log site.
The
timestampparameter on line 99 originates from the attacker-controllableX-Slack-Request-Timestampheader (especially since signature verification just failed, so the request is by definition untrusted). This is the same CWE-117 class being remediated elsewhere in the PR; consider sanitizing it for consistency.🛡️ Proposed fix
- LOGGER.warnf("Slack signature verification failed (timestamp=%s)", timestamp); + LOGGER.warnf("Slack signature verification failed (timestamp=%s)", sanitize(timestamp));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/integrations/slack/rest/RestSlackWebhook.java` at line 99, The log call LOGGER.warnf("Slack signature verification failed (timestamp=%s)", timestamp) in RestSlackWebhook uses the untrusted X-Slack-Request-Timestamp header value; sanitize or validate the timestamp before logging (e.g., canonicalize to a digits-only substring or strip control/formatting characters and fall back to a safe placeholder) and then log the sanitized value via LOGGER.warnf so the adjacent log site no longer emits attacker-controlled characters.src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java (1)
322-336:⚠️ Potential issue | 🟡 MinorNote: dead-letter JSON payload still embeds raw
conversationId.At line 326–327 the JSON payload is constructed with
String.format(...)interpolating the rawconversationId. IfconversationIdcontains a"or control character, the resulting payload will be malformed and consumers reading from the dead-letter stream may misparse it. Onlyfailure.getMessage()has its quotes escaped. Consider escapingconversationIdthe same way (or building the payload with a proper JSON serializer) — this is independent of the log-injection scope of this PR but follows the same threat model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java` around lines 322 - 336, The dead-letter payload in routeToDeadLetter currently interpolates a raw conversationId into the JSON string (DEAD_LETTER_PREFIX + sanitizeSubject(...)) which can break JSON if conversationId contains quotes/control chars; update routeToDeadLetter to produce a safe JSON payload by escaping conversationId the same way failure.getMessage() is escaped or, better, by using a proper JSON serializer to build the payload before calling jetStream.publish; ensure sanitizeSubject/sanitize are still used for subject and logs, and keep totalDeadLettered.incrementAndGet() and getMetrics() logic unchanged.
🧹 Nitpick comments (8)
docs/changelog.md (1)
20-20: Anchor “all findings remediated” to an authoritative artifact.Since this is an absolute claim, add a direct reference (e.g., CodeQL run URL/SARIF artifact/check run ID) next to the verification line so the changelog remains auditable.
As per coding guidelines, "Verify Factual Claims Against Authoritative Sources ... always verify against canonical source".
Also applies to: 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/changelog.md` at line 20, Update the changelog entry that reads "**What changed:** Remediated all CodeQL findings from PR `#455` scan — 4 High severity... and 77 Medium severity..." to include an authoritative reference (e.g., CodeQL run URL, SARIF artifact ID, or check run ID) immediately adjacent to the claim so it is auditable; also apply the same change to the other occurrence referenced (the entry at lines noted as 52-52) so both places point to the canonical verification artifact.src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java (1)
137-167: Prefer JBoss parameterized loggers (warnf/infof) overString.formatand concatenation.Both log sites bypass the JBoss Logger's lazy formatting (
String.formatalways allocates; concatenation always concatenates) and diverge from the parameterized style used across the rest of the codebase.♻️ Proposed change
- LOGGER.warn(String.format("Rate limit exceeded for tool '%s'. Reset in %d seconds.", sanitize(toolName), waitTimeMs / 1000)); + LOGGER.warnf("Rate limit exceeded for tool '%s'. Reset in %d seconds.", sanitize(toolName), waitTimeMs / 1000);- LOGGER.info("Reset rate limit for tool: " + sanitize(toolName)); + LOGGER.infof("Reset rate limit for tool: %s", sanitize(toolName));As per coding guidelines, "Logging — Use JBoss Logger (Logger.getLogger(Class.class))".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java` around lines 137 - 167, The two logging sites in ToolRateLimiter (the warn in acquire flow and the info in reset) use String.format and string concatenation which bypass JBoss Logger's lazy formatting; change them to the parameterized JBoss Logger methods (use LOGGER.warnf(...) for the rate-limit exceeded message and LOGGER.infof(...) for the reset message), pass sanitized toolName and the computed wait/reset values as format arguments (e.g., %s and %d/%s placeholders) instead of preformatting strings, and keep the sanitize(toolName) call as the argument so no string allocation happens before logging.src/main/java/ai/labs/eddi/modules/llm/tools/ToolCostTracker.java (1)
186-186: UseLOGGER.warnf(...)instead ofLOGGER.warn(String.format(...)).
String.formatis evaluated even when WARN is disabled, and it diverges from the parameterized JBoss-Logger style used elsewhere in this file (e.g., thedebugfyou just added on line 143).♻️ Proposed change
- LOGGER.warn(String.format("Conversation %s exceeded budget: $%.4f > $%.4f", sanitize(conversationId), metrics.getTotalCost(), maxBudget)); + LOGGER.warnf("Conversation %s exceeded budget: $%.4f > $%.4f", sanitize(conversationId), metrics.getTotalCost(), maxBudget);As per coding guidelines, "Logging — Use JBoss Logger (Logger.getLogger(Class.class))" with appropriate parameterized levels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/llm/tools/ToolCostTracker.java` at line 186, Replace the current String.format logging call in ToolCostTracker with the JBoss-Logger parameterized variant: change LOGGER.warn(String.format("Conversation %s exceeded budget: $%.4f > $%.4f", sanitize(conversationId), metrics.getTotalCost(), maxBudget)); to use LOGGER.warnf(...) and pass the format string and the arguments (sanitize(conversationId), metrics.getTotalCost(), maxBudget) directly so the formatting is skipped when WARN is disabled and matches the debugf style used elsewhere.src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java (1)
152-176: Pass the sanitized id throughexecuteWithRetryto avoid re-sanitizing on every retry.
submitInOrderalready computessafeConversationIdon line 103, butexecuteWithRetryreceives the rawconversationIdand callssanitize(conversationId)again on every failure (lines 164 and 168). Each retry/dead-letter path re-walks the string — and since retries can happen up toMAX_RETRIEStimes in tight succession, you also lose the guarantee that all log lines display the same sanitized form (LogSanitizer is deterministic today, but the duplication is fragile and noisy).Either thread the sanitized id through the helper, or sanitize once locally inside
executeWithRetry.♻️ Proposed refactor
private void executeWithRetry(String conversationId, BlockingQueue<Callable<Void>> queue, Callable<Void> callable, int attempt) { + final String safeConversationId = sanitize(conversationId); runtime.submitCallable(callable, new IRuntime.IFinishedExecution<>() { `@Override` public void onComplete(Void result) { totalProcessed.incrementAndGet(); submitNext(conversationId, queue); } `@Override` public void onFailure(Throwable t) { int nextAttempt = attempt + 1; if (nextAttempt < MAX_RETRIES) { - log.warnf(t, "In-memory task failed (conversationId=%s, attempt=%d/%d), retrying...", sanitize(conversationId), nextAttempt, + log.warnf(t, "In-memory task failed (conversationId=%s, attempt=%d/%d), retrying...", safeConversationId, nextAttempt, MAX_RETRIES); executeWithRetry(conversationId, queue, callable, nextAttempt); } else { - log.errorf(t, "In-memory task exhausted retries (conversationId=%s, attempts=%d), dead-lettering", sanitize(conversationId), + log.errorf(t, "In-memory task exhausted retries (conversationId=%s, attempts=%d), dead-lettering", safeConversationId, nextAttempt); routeToDeadLetter(conversationId, t);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java` around lines 152 - 176, submit the sanitized conversation id into executeWithRetry (or sanitize once at the top of that method) so we don't re-run sanitize(...) on every retry; update the executeWithRetry signature/locals to accept a safeConversationId (use the already computed safeConversationId from submitInOrder) and replace sanitize(conversationId) calls in the log.warnf/log.errorf invocations (and any places that log the id) with that safeConversationId, while continuing to pass the original conversationId where the raw id is required (e.g., to routeToDeadLetter or submitNext if they need the raw id).src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java (1)
274-275: Minor: sanitize the untrusted source, not the composed message.
budgetErroris"Budget exceeded for conversation " + conversationId. Sanitizing the whole string works, but it's clearer to sanitize the untrusted input once and reuse it (the same pattern used inNatsConversationCoordinator.submitInOrderwithsafeConversationId). It also avoids feeding the static literal portion through the sanitizer on every call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java` around lines 274 - 275, In AgentOrchestrator (around the budget handling), sanitize the untrusted conversationId once into a local safeConversationId (like the pattern used in NatsConversationCoordinator.submitInOrder) and then build the log message from the static literal plus safeConversationId; replace the current sanitizing of the entire budgetError string so LOGGER.warn(sanitize(...)) becomes LOGGER.warn("Budget exceeded for conversation " + safeConversationId) (or equivalent), ensuring sanitize is called only on the untrusted conversationId.src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.java (1)
39-51: Optional: nested test now coversLogSanitizer, notRestAgentEngineStreaming.After the refactor,
SanitizeForLogexercises the centralized utility directly rather than any behavior ofRestAgentEngineStreaming. Per the PR description,LogSanitizerTestalready provides 23 dedicated tests for this. This nested test is now redundant with that suite — consider removing it (or repurposing it to assert that a publicRestAgentEngineStreaminglog path actually goes throughLogSanitizer.sanitize).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.java` around lines 39 - 51, The nested test SanitizeForLog in RestAgentEngineStreamingTest is redundant because it directly tests ai.labs.eddi.utils.LogSanitizer.sanitize; either remove the SanitizeForLog class entirely, or replace it with a targeted test that verifies RestAgentEngineStreaming uses LogSanitizer.sanitize (e.g., exercise the public logging path on RestAgentEngineStreaming and assert the emitted log/message is sanitized via LogSanitizer.sanitize). Locate the nested class SanitizeForLog and either delete it or rewrite its test method to call the RestAgentEngineStreaming public method that produces logs and assert the output matches LogSanitizer.sanitize(originalInput).src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java (1)
260-308: Optional: cache the sanitized conversationId once per call.
publishAndExecuteinvokessanitize(conversationId)up to four times across the success/failure paths (lines 276, 295, 301, 335 inrouteToDeadLetter). Computing it once at the top of each method (mirroring thesafeConversationIdpattern insubmitInOrderat line 215) would be cheaper and easier to read. Not a correctness issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java` around lines 260 - 308, Compute a single local sanitizedConversationId = sanitize(conversationId) at the top of publishAndExecute and use that variable wherever sanitize(conversationId) is currently called (logs and routeToDeadLetter) to avoid repeated sanitization; keep using sanitizeSubject(conversationId) for the NATS subject as before. Update references inside the runtime.submitCallable callbacks (onComplete/onFailure) and the error log in the publish catch to use sanitizedConversationId, and pass sanitizedConversationId (or the original conversationId if routeToDeadLetter expects raw id) consistently to routeToDeadLetter and log calls to mirror the safeConversationId pattern used in submitInOrder.src/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.java (1)
244-311: Consider adding tests for remaining pagination edge cases.The new suite covers null/negative
index, nulllimit, excessivelimit, and a validlimit, but leaves a few branches from the PR's stated normalization rules (index ≥ 0,limit 1–100with null default 20, plus theInteger.MAX_VALUEoverflow guard on index increment) untested:
limit = 0andlimit < 0(per the PR description, the lower bound is 1 — worth pinning down whether these clamp to 1 or to 20).- The
Integer.MAX_VALUEoverflow guard on the page-advancement increment inRestConversationStore— this is one of the high-severity findings the PR claims to fix, but there's no regression test for it here.Adding two more cases would make the regression suite complete for the documented behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.java` around lines 244 - 311, Add two unit tests to cover the missing pagination edge cases: (1) test that limit=0 and limit<0 are clamped to the lower bound (per PR rules, assert readDescriptors is called with limit=1 or the documented lower bound) by calling RestConversationStore.readConversationDescriptors(0, 0, ...) and with a negative limit and verifying conversationDescriptorStore.readDescriptors(anyString(), any(), eq(0), eq(1), anyBoolean()); and (2) test the Integer.MAX_VALUE overflow guard by invoking RestConversationStore.readConversationDescriptors(Integer.MAX_VALUE, 1, ...) and exercising the page-advance path (e.g., simulate a next-page increment or call the method path that would increment index) and then verify that readDescriptors was called with a clamped index (not overflowing) — reference RestConversationStore.readConversationDescriptors and conversationDescriptorStore.readDescriptors in your assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/changelog.md`:
- Line 30: Update the changelog wording to accurately reflect
LogSanitizer.java's behavior: state that LogSanitizer.sanitize() first replaces
occurrences of \r, \n, and \t with '_' and then removes/strips other control
characters, rather than saying those characters are simply "stripped"; adjust
the sentence to use "replaces \r\n\t with '_' and strips other control
characters" (or equivalent precise phrasing) to avoid the misleading security
summary.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 420-421: The log call that prints groupIds is a potential
log-injection vector because groupIds (built from Property.getValueString()) is
not sanitized; update the LOGGER.infof call so each element of groupIds is
passed through sanitize (or sanitize the joined string) before formatting —
i.e., map groupIds via sanitize(...) (reference: LOGGER.infof invocation,
variable groupIds, and sanitize(...) method; related getters memory.getAgentId()
and memory.getUserId() remain sanitized as-is).
In `@src/test/java/ai/labs/eddi/utils/LogSanitizerTest.java`:
- Around line 94-109: The `@DisplayName` on the test
stripsAllNonWhitespaceControlChars is inaccurate because the loop also includes
0x0B (VT) and 0x0C (FF); update the annotation on the test method
stripsAllNonWhitespaceControlChars to correctly describe the range — for example
use "should strip all control chars 0x01-0x1F except \\r, \\n, \\t" or
explicitly "0x01-0x08, 0x0B-0x0C, 0x0E-0x1F"; modify only the `@DisplayName`
string and keep the test logic and the sanitize(...) call unchanged.
---
Outside diff comments:
In
`@src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java`:
- Around line 322-336: The dead-letter payload in routeToDeadLetter currently
interpolates a raw conversationId into the JSON string (DEAD_LETTER_PREFIX +
sanitizeSubject(...)) which can break JSON if conversationId contains
quotes/control chars; update routeToDeadLetter to produce a safe JSON payload by
escaping conversationId the same way failure.getMessage() is escaped or, better,
by using a proper JSON serializer to build the payload before calling
jetStream.publish; ensure sanitizeSubject/sanitize are still used for subject
and logs, and keep totalDeadLettered.incrementAndGet() and getMetrics() logic
unchanged.
In `@src/main/java/ai/labs/eddi/integrations/slack/rest/RestSlackWebhook.java`:
- Line 99: The log call LOGGER.warnf("Slack signature verification failed
(timestamp=%s)", timestamp) in RestSlackWebhook uses the untrusted
X-Slack-Request-Timestamp header value; sanitize or validate the timestamp
before logging (e.g., canonicalize to a digits-only substring or strip
control/formatting characters and fall back to a safe placeholder) and then log
the sanitized value via LOGGER.warnf so the adjacent log site no longer emits
attacker-controlled characters.
---
Nitpick comments:
In `@docs/changelog.md`:
- Line 20: Update the changelog entry that reads "**What changed:** Remediated
all CodeQL findings from PR `#455` scan — 4 High severity... and 77 Medium
severity..." to include an authoritative reference (e.g., CodeQL run URL, SARIF
artifact ID, or check run ID) immediately adjacent to the claim so it is
auditable; also apply the same change to the other occurrence referenced (the
entry at lines noted as 52-52) so both places point to the canonical
verification artifact.
In
`@src/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.java`:
- Around line 152-176: submit the sanitized conversation id into
executeWithRetry (or sanitize once at the top of that method) so we don't re-run
sanitize(...) on every retry; update the executeWithRetry signature/locals to
accept a safeConversationId (use the already computed safeConversationId from
submitInOrder) and replace sanitize(conversationId) calls in the
log.warnf/log.errorf invocations (and any places that log the id) with that
safeConversationId, while continuing to pass the original conversationId where
the raw id is required (e.g., to routeToDeadLetter or submitNext if they need
the raw id).
In
`@src/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.java`:
- Around line 260-308: Compute a single local sanitizedConversationId =
sanitize(conversationId) at the top of publishAndExecute and use that variable
wherever sanitize(conversationId) is currently called (logs and
routeToDeadLetter) to avoid repeated sanitization; keep using
sanitizeSubject(conversationId) for the NATS subject as before. Update
references inside the runtime.submitCallable callbacks (onComplete/onFailure)
and the error log in the publish catch to use sanitizedConversationId, and pass
sanitizedConversationId (or the original conversationId if routeToDeadLetter
expects raw id) consistently to routeToDeadLetter and log calls to mirror the
safeConversationId pattern used in submitInOrder.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 274-275: In AgentOrchestrator (around the budget handling),
sanitize the untrusted conversationId once into a local safeConversationId (like
the pattern used in NatsConversationCoordinator.submitInOrder) and then build
the log message from the static literal plus safeConversationId; replace the
current sanitizing of the entire budgetError string so
LOGGER.warn(sanitize(...)) becomes LOGGER.warn("Budget exceeded for conversation
" + safeConversationId) (or equivalent), ensuring sanitize is called only on the
untrusted conversationId.
In `@src/main/java/ai/labs/eddi/modules/llm/tools/ToolCostTracker.java`:
- Line 186: Replace the current String.format logging call in ToolCostTracker
with the JBoss-Logger parameterized variant: change
LOGGER.warn(String.format("Conversation %s exceeded budget: $%.4f > $%.4f",
sanitize(conversationId), metrics.getTotalCost(), maxBudget)); to use
LOGGER.warnf(...) and pass the format string and the arguments
(sanitize(conversationId), metrics.getTotalCost(), maxBudget) directly so the
formatting is skipped when WARN is disabled and matches the debugf style used
elsewhere.
In `@src/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.java`:
- Around line 137-167: The two logging sites in ToolRateLimiter (the warn in
acquire flow and the info in reset) use String.format and string concatenation
which bypass JBoss Logger's lazy formatting; change them to the parameterized
JBoss Logger methods (use LOGGER.warnf(...) for the rate-limit exceeded message
and LOGGER.infof(...) for the reset message), pass sanitized toolName and the
computed wait/reset values as format arguments (e.g., %s and %d/%s placeholders)
instead of preformatting strings, and keep the sanitize(toolName) call as the
argument so no string allocation happens before logging.
In
`@src/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.java`:
- Around line 39-51: The nested test SanitizeForLog in
RestAgentEngineStreamingTest is redundant because it directly tests
ai.labs.eddi.utils.LogSanitizer.sanitize; either remove the SanitizeForLog class
entirely, or replace it with a targeted test that verifies
RestAgentEngineStreaming uses LogSanitizer.sanitize (e.g., exercise the public
logging path on RestAgentEngineStreaming and assert the emitted log/message is
sanitized via LogSanitizer.sanitize). Locate the nested class SanitizeForLog and
either delete it or rewrite its test method to call the RestAgentEngineStreaming
public method that produces logs and assert the output matches
LogSanitizer.sanitize(originalInput).
In
`@src/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.java`:
- Around line 244-311: Add two unit tests to cover the missing pagination edge
cases: (1) test that limit=0 and limit<0 are clamped to the lower bound (per PR
rules, assert readDescriptors is called with limit=1 or the documented lower
bound) by calling RestConversationStore.readConversationDescriptors(0, 0, ...)
and with a negative limit and verifying
conversationDescriptorStore.readDescriptors(anyString(), any(), eq(0), eq(1),
anyBoolean()); and (2) test the Integer.MAX_VALUE overflow guard by invoking
RestConversationStore.readConversationDescriptors(Integer.MAX_VALUE, 1, ...) and
exercising the page-advance path (e.g., simulate a next-page increment or call
the method path that would increment index) and then verify that readDescriptors
was called with a clamped index (not overflowing) — reference
RestConversationStore.readConversationDescriptors and
conversationDescriptorStore.readDescriptors in your assertions.
🪄 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: 7c179641-f610-4266-a9a2-82b4cd113bb0
📒 Files selected for processing (29)
docs/changelog.mdsrc/main/java/ai/labs/eddi/configs/groups/rest/RestAgentGroupStore.javasrc/main/java/ai/labs/eddi/configs/rag/rest/RestRagIngestion.javasrc/main/java/ai/labs/eddi/datastore/DescriptorStore.javasrc/main/java/ai/labs/eddi/engine/internal/RestAgentEngineStreaming.javasrc/main/java/ai/labs/eddi/engine/internal/RestCoordinatorAdmin.javasrc/main/java/ai/labs/eddi/engine/memory/rest/RestConversationStore.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/CronDescriber.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/InMemoryConversationCoordinator.javasrc/main/java/ai/labs/eddi/engine/runtime/internal/NatsConversationCoordinator.javasrc/main/java/ai/labs/eddi/engine/tenancy/InMemoryTenantQuotaStore.javasrc/main/java/ai/labs/eddi/integrations/slack/rest/RestSlackWebhook.javasrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ChatModelRegistry.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ConversationSummarizer.javasrc/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactory.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/main/java/ai/labs/eddi/modules/llm/tools/ToolCacheService.javasrc/main/java/ai/labs/eddi/modules/llm/tools/ToolCostTracker.javasrc/main/java/ai/labs/eddi/modules/llm/tools/ToolRateLimiter.javasrc/main/java/ai/labs/eddi/modules/nlp/expressions/utilities/ExpressionProvider.javasrc/main/java/ai/labs/eddi/modules/rag/RagIngestionService.javasrc/main/java/ai/labs/eddi/secrets/rest/RestSecretStore.javasrc/main/java/ai/labs/eddi/utils/LogSanitizer.javasrc/test/java/ai/labs/eddi/engine/internal/RestAgentEngineStreamingTest.javasrc/test/java/ai/labs/eddi/engine/memory/rest/RestConversationStoreTest.javasrc/test/java/ai/labs/eddi/engine/runtime/internal/CronDescriberExtendedTest.javasrc/test/java/ai/labs/eddi/utils/LogSanitizerTest.java
- changelog: corrected LogSanitizer behavior description (replace vs strip) - AgentOrchestrator: sanitize groupIds entries before logging (CWE-117) - LogSanitizerTest: fixed @DisplayName range to include 0x0B-0x0C
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java (1)
420-421:groupIdssanitization addressed — minor nit: use a method reference.The previously flagged log-injection vector on
groupIdsis now closed by mapping each element throughsanitize(...)before formatting. As a small readability improvement, the lambdag -> sanitize(g)can be replaced with a method reference.♻️ Proposed nit
- LOGGER.infof("[MEMORY] UserMemoryTool enabled for agent='%s', user='%s', groups=%s", sanitize(memory.getAgentId()), - sanitize(memory.getUserId()), groupIds.stream().map(g -> sanitize(g)).toList()); + LOGGER.infof("[MEMORY] UserMemoryTool enabled for agent='%s', user='%s', groups=%s", + sanitize(memory.getAgentId()), sanitize(memory.getUserId()), + groupIds.stream().map(ai.labs.eddi.utils.LogSanitizer::sanitize).toList());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java` around lines 420 - 421, Replace the lambda mapping in the log line so it uses a method reference to sanitize each group id instead of "g -> sanitize(g)"; update the call in the LOGGER.infof statement that references groupIds (the line that currently does groupIds.stream().map(g -> sanitize(g)).toList()) to use a method reference to the existing sanitize(...) method (e.g., groupIds.stream().map(YourClassName::sanitize).toList() or this::sanitize as appropriate) so readability is improved while preserving the sanitization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.java`:
- Around line 420-421: Replace the lambda mapping in the log line so it uses a
method reference to sanitize each group id instead of "g -> sanitize(g)"; update
the call in the LOGGER.infof statement that references groupIds (the line that
currently does groupIds.stream().map(g -> sanitize(g)).toList()) to use a method
reference to the existing sanitize(...) method (e.g.,
groupIds.stream().map(YourClassName::sanitize).toList() or this::sanitize as
appropriate) so readability is improved while preserving the sanitization
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c05903e-fc69-4056-b220-2cd2ac647130
📒 Files selected for processing (3)
docs/changelog.mdsrc/main/java/ai/labs/eddi/modules/llm/impl/AgentOrchestrator.javasrc/test/java/ai/labs/eddi/utils/LogSanitizerTest.java
✅ Files skipped from review due to trivial changes (1)
- docs/changelog.md
rolandpickl
approved these changes
Apr 27, 2026
This was referenced May 5, 2026
13 tasks
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.
fix(security): remediate CodeQL findings — log injection, array bounds, arithmetic overflow
Summary
Remediates 81 CodeQL findings flagged during the PR #455 scan (pre-existing in codebase, surfaced by large diff).
LogSanitizer.sanitize()utilityChanges
New:
LogSanitizerutilityai.labs.eddi.utils.LogSanitizer— single staticsanitize(String)method\r\n\t→_(preserving readability), then strips remaining control characters (U+0000–U+001F,U+007F)"null"string for null inputprivate sanitizeForLog()methods that were scattered across the codebaseHigh severity fixes
CronDescriber — Added
v >= 0lower-bound guard informatSet()to preventArrayIndexOutOfBoundsExceptionfrom negative values. Falls back toString.valueOf(v)gracefully.RestConversationStore — Pagination parameter sanitization:
index: clamped to>= 0(null/negative → 0)limit: clamped to1–100(null → 20, >100 → 100)Integer.MAX_VALUEoverflow guard on the index increment loopDescriptorStore — Promoted
index * effectiveLimitskip calculation tolongarithmetic withMath.min(skipLong, Integer.MAX_VALUE)clamp to prevent integer multiplication overflow.Medium severity — log injection (19 files)
Applied
sanitize()to all user-controlled values in log statements:tenantId,keyName,conversationId,agentId,userId,toolName,serverName,kbId,storeType,eventType,eventId, etc.Consolidated files (removed duplicated
sanitizeForLog()):InMemoryConversationCoordinator,NatsConversationCoordinatorRestAgentEngineStreaming,RestCoordinatorAdminInMemoryTenantQuotaStore,RestAgentGroupStoreTests
LogSanitizerTestCronDescriberExtendedTestformatSetnegative value (single + multi-element), boundary atlabels.length, valid max indexRestConversationStoreTest74 tests pass, 0 failures.
Not in scope
e.getMessage()in log statements — JVM-generated, not user-controlledSummary by CodeRabbit