security-review-bot
  • Joined on 2026-05-02
security-review-bot commented on pull request rodin/review-bot#110 2026-05-13 12:55:58 +00:00
feat(github): support HTTP-date format in Retry-After header

[MINOR] Log injection hardening only sanitizes \n and \r. Other control characters (e.g., tabs, non-printable ASCII, ANSI escape sequences) could still affect log rendering. Consider stripping or escaping a broader set of control characters before formatting the error.

security-review-bot commented on pull request rodin/review-bot#110 2026-05-13 12:55:58 +00:00
feat(github): support HTTP-date format in Retry-After header

[MINOR] APIError.Error() includes up to 200 bytes of upstream response body in the error string. If callers log this error, it may leak sensitive information returned by the upstream service. Consider omitting the body from Error() or gating detailed content behind a debug flag.

security-review-bot commented on pull request rodin/review-bot#109 2026-05-13 12:25:40 +00:00
feat(gitea): harden GetPullRequestDiff against unbounded diff size

[MINOR] In doGetLimited, when maxBytes == math.MaxInt64, limitBytes is set to math.MaxInt64, effectively removing the practical limit and allowing io.ReadAll to attempt to read an unbounded response. While this requires explicit misconfiguration, it can still risk memory exhaustion; consider treating extremely large values as disabled in a controlled way or enforcing a reasonable upper cap.

security-review-bot commented on pull request rodin/review-bot#109 2026-05-13 12:25:40 +00:00
feat(gitea): harden GetPullRequestDiff against unbounded diff size

[MINOR] Logging uses redactURL(reqURL) which only strips query parameters and may leak userinfo credentials if present in baseURL (e.g., https://user:pass@host). Consider redacting userinfo in redactURL to avoid potential secret exposure in logs.

security-review-bot commented on pull request rodin/review-bot#109 2026-05-13 12:18:45 +00:00
feat(gitea): harden GetPullRequestDiff against unbounded diff size

[MINOR] In doGetLimited, when maxBytes equals math.MaxInt64, limitBytes overflows and is clamped to math.MaxInt64, causing the reader to effectively allow an unbounded read (up to MaxInt64). While this only happens with an extreme configuration, it could permit large memory usage if someone sets MaxDiffSize to MaxInt64 instead of using -1 to disable the limit.

security-review-bot commented on pull request rodin/review-bot#110 2026-05-13 12:13:01 +00:00
feat(github): support HTTP-date format in Retry-After header

[MINOR] Unbounded successful response body read may allow resource exhaustion if a server returns a very large payload. While the GitHub API is typically bounded, for defense-in-depth consider imposing a reasonable limit or streaming with safeguards similar to the error path.

security-review-bot commented on pull request rodin/review-bot#110 2026-05-13 12:13:01 +00:00
feat(github): support HTTP-date format in Retry-After header

[NIT] Error message sanitization removes newlines but other control characters or ANSI escape sequences could still facilitate log injection in some environments. Consider further sanitization (e.g., stripping non-printable characters) or using structured logging consistently to mitigate log injection risks.

security-review-bot commented on pull request rodin/review-bot#110 2026-05-13 12:13:01 +00:00
feat(github): support HTTP-date format in Retry-After header

[MINOR] Potential race condition: doRequest uses and mutates the shared c.retryBackoff slice (e.g., later at line 193) when non-nil. Since Client is intended to be safe for concurrent use, concurrent calls could race and cause unpredictable backoff behavior. Consider copying the slice per request (e.g., append([]time.Duration(nil), c.retryBackoff...)) before mutating.

security-review-bot commented on pull request rodin/review-bot#109 2026-05-13 11:59:24 +00:00
feat(gitea): harden GetPullRequestDiff against unbounded diff size

[MINOR] Adding 1 to maxBytes in io.LimitReader(resp.Body, maxBytes+1) can overflow if maxBytes is math.MaxInt64, resulting in a negative limit and an immediate EOF (empty body) instead of enforcing the intended bound. Consider clamping at MaxInt64 or handling the overflow case explicitly.

security-review-bot commented on pull request rodin/review-bot#109 2026-05-13 11:59:24 +00:00
feat(gitea): harden GetPullRequestDiff against unbounded diff size

[NIT] Disabling the diff size limit (MaxDiffSize = -1) removes protection against large responses and could allow memory exhaustion if configured from untrusted input. Ensure this setting cannot be influenced by untrusted sources.

security-review-bot commented on pull request rodin/review-bot#106 2026-05-13 11:35:31 +00:00
feat(cmd): wire --provider and --base-url flags into CLI (Phase 5)

[MINOR] The GitHub client uses a user-supplied base URL (--base-url/VCS_BASE_URL). While HTTPS is enforced and redirects strip Authorization, a misconfigured or attacker-controlled base URL could still receive the token over HTTPS. Consider validating/allowlisting expected hosts for production deployments, or requiring an explicit opt-in when using non-default hosts.

security-review-bot commented on pull request rodin/review-bot#106 2026-05-13 11:16:22 +00:00
feat(cmd): wire --provider and --base-url flags into CLI (Phase 5)

[MINOR] When initializing the Gitea client, there is no enforcement or warning against using an insecure (HTTP) VCS URL with a token. Unlike the GitHub client which refuses to send credentials over HTTP by default, this path may allow plaintext token transmission depending on gitea.Client behavior. Recommend warning or rejecting non-HTTPS --vcs-url unless explicitly allowed for trusted/local use.

security-review-bot commented on pull request rodin/review-bot#106 2026-05-13 11:16:22 +00:00
feat(cmd): wire --provider and --base-url flags into CLI (Phase 5)

[NIT] In supersedeOldReviews (Gitea path), the supersedure body includes a permalink constructed from vcsURL without scheme validation. If misconfigured to a non-HTTPS URL, users may be encouraged to click insecure links. Consider validating or normalizing to HTTPS where applicable or logging a warning.