security-review-bot
  • Joined on 2026-05-02
security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 13:44:22 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MINOR] The safe HTTP client clones http.DefaultTransport and thus preserves ProxyFromEnvironment. If HTTPS_PROXY/HTTP_PROXY are set to an attacker-controlled proxy in the runtime environment, Authorization headers could be exposed to that proxy despite SSRF IP checks. Consider disabling proxies (Transport.Proxy = nil) or providing an option to ignore proxies when connecting to user-supplied VCS URLs.

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 13:44:22 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[NIT] The Python IP classification (using ipaddress.is_reserved) may be stricter than the Go IsBlockedIP (which allows RFC5737 TEST-NET ranges). This inconsistency can cause false positives in pre-checks versus runtime. Consider aligning the block lists to avoid surprises (not a security bug; just consistency).

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 13:44:22 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MINOR] The Python pre-check prints the blocked IP address to stderr (print(f"blocked: {a}")), which can leak internal IPs in CI logs. Consider removing or masking the exact IP in logs to reduce information disclosure while still failing safely.

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 13:44:22 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MINOR] safeDialContext returns errors that include the resolved private/reserved IP (e.g., "blocked: host resolves to IP"). When logged in CI, this can disclose internal IPs. Consider redacting the concrete IP in error messages to avoid information leakage (e.g., state that it resolved to a private/reserved range without printing the exact address).

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 11:51:56 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MAJOR] The Python preflight SSRF check does not block Carrier-Grade NAT (100.64.0.0/10). ipaddress.IPv4Address.is_private returns False for CGN, and is_reserved does not cover it. An attacker could supply a hostname resolving to 100.64/10 and bypass this pre-check, potentially sending ACTION_TOKEN to non-public infrastructure.

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 11:51:56 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MAJOR] Safe dialer does not protect against proxy-assisted SSRF. newSafeHTTPClient clones http.DefaultTransport (retaining ProxyFromEnvironment). When a proxy is configured, DialContext connects to the proxy (which passes the Authorization header through), and the target host/IP is not validated by safeDialContext. An attacker could point baseURL to a private/reserved IP (e.g., 169.254.169.254) and rely on an environment proxy to reach it, bypassing the IP block.

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 11:51:56 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MAJOR] Same CGN gap in the re-validation block before downloads: the Python IP checks omit 100.64.0.0/10, enabling a DNS rebind or split-horizon scenario to CGN addresses even after initial validation.

security-review-bot suggested changes for rodin/review-bot#129 2026-05-14 11:51:56 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

Security Review

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 11:51:56 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MINOR] WithUnsafeDialer() is exported and removes IP-level SSRF protection. While tests use it appropriately, exporting this method increases the risk of accidental misuse in production code. Consider making it unexported and exposing only a test-only constructor (as already added via NewTestClient in export_test.go).

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 08:48:07 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MINOR] The Python pre-check does not explicitly reject URLs containing user-info (user:pass@host). While the Go validate-url subcommand rejects this, the shell step’s Python check only extracts hostname and proceeds, allowing user-info to be used with curl. This can lead to confusion and potential logging of attacker-supplied credentials.

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 08:48:07 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MAJOR] The Python IP pre-check does not normalize IPv6-mapped IPv4 addresses (e.g., ::ffff:10.0.0.1). ipaddress.IPv6Address("::ffff:10.0.0.1").is_private is typically false, allowing a crafted AAAA record to bypass the check and potentially send ACTION_TOKEN to private/internal IPs via curl. The Go client normalizes these in IsBlockedIP, but the curl step is only protected by the Python script.

security-review-bot suggested changes for rodin/review-bot#129 2026-05-14 08:48:07 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

Security Review

security-review-bot commented on pull request rodin/review-bot#129 2026-05-14 07:47:47 +00:00
feat(#123): add IP-level SSRF defense to Gitea client and action

[MINOR] The Install review-bot step relies on the SERVER_URL IP check performed earlier in the Determine version step. A DNS rebinding between steps could result in curl sending the ACTION_TOKEN to a newly-resolved private/reserved IP. Re-running the IP validation (e.g., via the validate-url subcommand or the same Python check) at the start of the Install step would narrow this window.

security-review-bot commented on pull request rodin/review-bot#121 2026-05-14 07:16:59 +00:00
fix(action): detect VCS host type for version resolution and binary download

[MINOR] On Gitea, ACTION_TOKEN defaults to inputs.reviewer-token when action-repo-token is not provided, which may grant broader privileges than necessary and increases the impact if tokens are exfiltrated. Prefer requiring a dedicated, least-privileged token for downloading release assets (or fail closed when missing) to adhere to least privilege.

security-review-bot commented on pull request rodin/review-bot#121 2026-05-14 07:16:59 +00:00
fix(action): detect VCS host type for version resolution and binary download

[MINOR] On Gitea, SERVER_URL is derived from inputs.vcs-url with only a basic regex check (https scheme and no whitespace). This leaves potential SSRF/token exfiltration risk if a misconfigured workflow allows untrusted input to control this value. Consider validating the hostname against an allowlist or ensuring it matches the platform-provided server_url, and reject private/link-local IPs (e.g., 169.254.169.254) to prevent metadata endpoint access.