security-review-bot
  • Joined on 2026-05-02
security-review-bot approved rodin/review-bot#102 2026-05-13 04:22:24 +00:00
feat(github): implement PRReader interface

Security Review

security-review-bot approved rodin/review-bot#103 2026-05-13 04:14:40 +00:00
feat(github): implement FileReader interface

Security Review

security-review-bot commented on pull request rodin/review-bot#93 2026-05-13 03:50:05 +00:00
feat(github): implement PRReader + FileReader client (#80)

[MINOR] SetHTTPClient allows replacing the underlying http.Client without enforcing a safe CheckRedirect policy, which could cause Authorization to be forwarded to untrusted hosts on redirects. The docstring warns about this, but adding a runtime safeguard (e.g., rejecting clients with nil CheckRedirect or wrapping it) would further reduce misuse risk.

security-review-bot commented on pull request rodin/review-bot#93 2026-05-13 03:50:05 +00:00
feat(github): implement PRReader + FileReader client (#80)

[MINOR] APIError.Error() includes up to 200 characters of the upstream response body in the error message. While sanitized and truncated, this can still leak information into logs if generic error logging prints err.Error(). Consider omitting body content from Error() or making inclusion opt-in to reduce information exposure.

security-review-bot commented on pull request rodin/review-bot#93 2026-05-13 01:44:32 +00:00
feat(github): implement PRReader + FileReader client (#80)

[MINOR] defaultCheckRedirect follows HTTPS→HTTP redirects after stripping Authorization. While credentials are protected, this still permits plaintext requests to proceed, which can leak metadata and expands attack surface if a misconfigured or compromised server issues such redirects. Prefer failing closed on protocol downgrades.

security-review-bot commented on pull request rodin/review-bot#93 2026-05-13 01:44:32 +00:00
feat(github): implement PRReader + FileReader client (#80)

[MINOR] defaultCheckRedirect allows cross-host redirects (with Authorization stripped). Although token leakage is mitigated, following cross-host redirects can facilitate SSRF-like behavior if baseURL is misconfigured or points to a compromised server. Consider rejecting cross-host redirects by default or enforcing an allowlist of trusted hosts.

security-review-bot commented on pull request rodin/review-bot#93 2026-05-13 01:44:32 +00:00
feat(github): implement PRReader + FileReader client (#80)

[MINOR] APIError.Error includes up to 200 bytes of upstream response body in the error string. If these errors are logged, this can leak sensitive details from upstream or allow log injection (newlines) if baseURL points to an untrusted endpoint. Consider sanitizing newlines and/or omitting body content from the error string.

security-review-bot commented on pull request rodin/review-bot#93 2026-05-13 01:18:57 +00:00
feat(github): implement PRReader + FileReader client (#80)

[MINOR] APIError stores up to 64KB of error body in the Body field. While Error() truncates to 200 bytes, exposing the full Body increases the risk of sensitive data leakage if callers log or propagate it. Consider further limiting or redacting Body contents.

security-review-bot commented on pull request rodin/review-bot#93 2026-05-13 01:18:57 +00:00
feat(github): implement PRReader + FileReader client (#80)

[MINOR] SetHTTPClient accepts an arbitrary *http.Client and does not enforce the safe redirect policy. If a caller supplies a client without a CheckRedirect that strips Authorization on cross-host or downgrade redirects, credentials could leak during redirects.

security-review-bot suggested changes for rodin/review-bot#89 2026-05-13 00:42:38 +00:00
fix(deps): replace gopkg.in/yaml.v3 with github.com/goccy/go-yaml

Security Review

security-review-bot commented on pull request rodin/review-bot#89 2026-05-13 00:42:38 +00:00
fix(deps): replace gopkg.in/yaml.v3 with github.com/goccy/go-yaml

[MAJOR] checkYAMLDepth treats MergeKeyNode as a leaf (listed in the default case comment) and does not recurse into its referenced value(s). YAML merge keys (<<: *anchor) can introduce aliased content at a deeper effective depth without being traversed, potentially bypassing the MaxYAMLDepth and enabling stack exhaustion/DoS during decoding. Explicitly handle *ast.MergeKeyNode by recursing into its child node(s) (e.g., its Value/Values and any contained AliasNode) similar to other node types.