security-review-bot
  • Joined on 2026-05-02
security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:47:09 +00:00
feat: add native SAP AI Core support

[MINOR] No validation that --aicore-auth-url and --aicore-api-url use HTTPS. A misconfiguration could transmit client credentials over plain HTTP. Enforce or warn on non-https schemes for these URLs.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:47:09 +00:00
feat: add native SAP AI Core support

[MINOR] Unbounded io.ReadAll in CompleteOpenAI; consider io.LimitReader or streaming decode to limit memory use.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:47:09 +00:00
feat: add native SAP AI Core support

[MINOR] Unbounded io.ReadAll in CompleteAnthropic; add a maximum body size to mitigate potential memory exhaustion if the server returns an unexpectedly large payload.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:42:20 +00:00
feat: add native SAP AI Core support

[MINOR] The code trusts the deploymentUrl returned by the AI Core API and directly constructs the invoke endpoint without validating the URL's scheme/host. An attacker controlling the API endpoint (or misconfiguration) could return a malicious URL, leading to SSRF attempts from the runner. Consider parsing and validating that the URL uses https and matches an expected domain or host pattern.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:42:20 +00:00
feat: add native SAP AI Core support

[MINOR] Same SSRF concern for OpenAI-style models: the chat/completions URL is derived directly from deploymentUrl without scheme/host validation. Add URL parsing and enforce https and allowed host/domain.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:42:20 +00:00
feat: add native SAP AI Core support

[MINOR] Header value for AI-Resource-Group is taken from unvalidated input and inserted into an HTTP header. While Go's net/http prevents CRLF injection in headers, consider validating this value (e.g., allow only [A-Za-z0-9-_]) to reduce risk and fail fast on invalid configuration.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:42:20 +00:00
feat: add native SAP AI Core support

[MINOR] Error paths include the full response body in the returned error (token request and AI Core API errors). This can leak potentially sensitive server responses into logs. Consider truncating bodies in errors or logging them at a debug level only.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:42:20 +00:00
feat: add native SAP AI Core support

[MINOR] Same body-echoing concern on AI Core chat/completions error path; consider truncation or lower log level to avoid leaking large or sensitive content into logs.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:42:20 +00:00
feat: add native SAP AI Core support

[NIT] When using provider=aicore, AuthURL and APIURL are accepted as provided. Consider warning or rejecting non-HTTPS URLs to avoid sending credentials or tokens over insecure transport.

security-review-bot approved rodin/review-bot#50 2026-05-10 06:42:20 +00:00
feat: add native SAP AI Core support

Security Review

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:38:39 +00:00
feat: add native SAP AI Core support

[MINOR] The OAuth token request uses the default http.Client redirect behavior, which will resend the POST body on 307/308 redirects. If the AuthURL endpoint (or a misconfigured proxy) issues a cross-host 307/308, client_id/client_secret could be sent to a different host. Consider setting a CheckRedirect function to prevent cross-host redirects or disallow redirects for the token endpoint.

security-review-bot approved rodin/review-bot#50 2026-05-10 06:38:39 +00:00
feat: add native SAP AI Core support

Security Review

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:38:39 +00:00
feat: add native SAP AI Core support

[MINOR] CompleteOpenAI also reads the response body without Content-Length validation. Add a length check similar to llm/client.go#doRequest to guard against truncated responses.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:38:39 +00:00
feat: add native SAP AI Core support

[MINOR] CompleteAnthropic reads the response body without validating it against Content-Length, so a truncated response could be parsed if it remains valid JSON. Consider adding the same Content-Length vs body-length validation used in llm/client.go#doRequest to detect truncation before attempting to parse.

security-review-bot approved rodin/review-bot#50 2026-05-10 06:25:50 +00:00
feat: add native SAP AI Core support

Security Review

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:25:50 +00:00
feat: add native SAP AI Core support

[MINOR] fetchToken constructs the token endpoint from AICoreConfig.AuthURL without enforcing HTTPS. If misconfigured to use http, client credentials would be sent in cleartext. Consider validating that AuthURL uses the https scheme (and fail or warn otherwise).

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:25:50 +00:00
feat: add native SAP AI Core support

[MINOR] On non-2xx responses from the OAuth or AI Core APIs, the code includes the full response body in returned errors (e.g., "token request failed ...: %s"). Upstream callers log these errors, which can inadvertently leak request/response details to CI logs. Consider truncating or redacting error bodies before returning/logging.

security-review-bot commented on pull request rodin/review-bot#50 2026-05-10 06:25:50 +00:00
feat: add native SAP AI Core support

[NIT] fetchDeployments trusts the deploymentUrl from the AI Core control plane and later sends Bearer tokens to that URL. As a defense-in-depth measure, consider validating that the deploymentUrl host matches (or is a subdomain of) the expected API domain to reduce risk of token exfiltration if the control plane is misconfigured.

security-review-bot approved rodin/review-bot#50 2026-05-10 06:20:37 +00:00
feat: add native SAP AI Core support

Security Review