rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat(github): add safeguards against accidental AllowInsecureHTTP use (#96) #113

Merged
aweiker merged 6 commits from review-bot-issue-96 into main 2026-05-13 20:21:42 +00:00
Conversation 49 Commits 6 Files Changed 3
+6 -4
1 changed files with 6 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 6b7f3f6924 - Show all commits
github/client.go
+6 -4
View File
5
 @@ -152,7 +152,7 @@ type clientConfig struct {
// AllowInsecureHTTP permits sending credentials over plaintext HTTP connections. // AllowInsecureHTTP permits sending credentials over plaintext HTTP connections.
// In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1 // In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1
// environment variable. Without the env var set, the option is silently ignored // environment variable. Without the env var set, the option is ignored
// and a warning is logged. // and a warning is logged.
// //
sonnet-review-bot commented 2026-05-13 18:45:29 +00:00
Review
Copy Link
Copy Source

[MINOR] AllowInsecureHTTPForTest is in the production file (client.go). Per the convention, test-only helpers should ideally live in an export_test.go file or be clearly gated. Since this function is exported and intended exclusively for test code, it bleeds test surface into the production API. Consider moving it to a file compiled only during tests (e.g., export_test.go), or renaming to make its test-only nature even more prominent in godoc.

**[MINOR]** AllowInsecureHTTPForTest is in the production file (client.go). Per the convention, test-only helpers should ideally live in an export_test.go file or be clearly gated. Since this function is exported and intended exclusively for test code, it bleeds test surface into the production API. Consider moving it to a file compiled only during tests (e.g., export_test.go), or renaming to make its test-only nature even more prominent in godoc.
// For tests, use AllowInsecureHTTPForTest (defined in export_test.go) which bypasses the env gate. // For tests, use AllowInsecureHTTPForTest (defined in export_test.go) which bypasses the env gate.
20
 @@ -267,9 +267,8 @@ func redactURL(rawURL string) string {
if err != nil { if err != nil {
sonnet-review-bot commented 2026-05-13 18:21:04 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] hasHTTPSScheme is package-level but unexported and has a single caller. It's well-named and the allocation-avoidance justification is valid. Consider a brief comment noting it's case-insensitive for correctness (not just performance), since HTTP scheme comparison is case-insensitive per RFC 3986. The existing comment mentions performance but not the correctness motivation.

**[NIT]** `hasHTTPSScheme` is package-level but unexported and has a single caller. It's well-named and the allocation-avoidance justification is valid. Consider a brief comment noting it's case-insensitive for correctness (not just performance), since HTTP scheme comparison is case-insensitive per RFC 3986. The existing comment mentions performance but not the correctness motivation.
return "<unparseable URL>" return "<unparseable URL>"
security-review-bot commented 2026-05-13 18:26:17 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] redactURL only strips query parameters and can still leak credentials if present in the userinfo component (e.g., http://user:pass@host/path). Consider parsing with url.Parse and redacting u.User and fragment to avoid potential secret exposure in logs.

**[MINOR]** redactURL only strips query parameters and can still leak credentials if present in the userinfo component (e.g., http://user:pass@host/path). Consider parsing with url.Parse and redacting u.User and fragment to avoid potential secret exposure in logs.
} }
if u.User != nil { u.User = nil
sonnet-review-bot commented 2026-05-13 18:25:10 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] redactURL uses strings.IndexByte which is perfectly fine, but since the project already imports net/url, using url.Parse + clearing RawQuery would handle edge cases like fragment identifiers (#) and malformed URLs more robustly. Not a bug given the controlled usage context (internal URLs), just a note.

**[NIT]** `redactURL` uses `strings.IndexByte` which is perfectly fine, but since the project already imports `net/url`, using `url.Parse` + clearing `RawQuery` would handle edge cases like fragment identifiers (`#`) and malformed URLs more robustly. Not a bug given the controlled usage context (internal URLs), just a note.
gpt-review-bot commented 2026-05-13 20:06:24 +00:00
Review
Copy Link
Copy Source

[NIT] redactURL redacts userinfo and query parameters; consider also clearing URL fragments (u.Fragment) to avoid leaking anchor components in logs, even though they’re generally not sent to servers.

**[NIT]** redactURL redacts userinfo and query parameters; consider also clearing URL fragments (u.Fragment) to avoid leaking anchor components in logs, even though they’re generally not sent to servers.
u.User = nil
}
if u.RawQuery != "" { if u.RawQuery != "" {
u.RawQuery = "<redacted>" u.RawQuery = "<redacted>"
sonnet-review-bot commented 2026-05-13 17:49:18 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] hasHTTPSScheme uses strings.EqualFold for the scheme prefix check. Since "https://" contains only ASCII characters, EqualFold is correct but adds a small amount of unnecessary overhead compared to a direct strings.ToLower or a case-sensitive check (HTTP schemes per RFC 3986 are case-insensitive but in practice always lowercase in this codebase). This is purely a performance NIT on a non-hot path and does not affect correctness.

**[MINOR]** hasHTTPSScheme uses strings.EqualFold for the scheme prefix check. Since "https://" contains only ASCII characters, EqualFold is correct but adds a small amount of unnecessary overhead compared to a direct strings.ToLower or a case-sensitive check (HTTP schemes per RFC 3986 are case-insensitive but in practice always lowercase in this codebase). This is purely a performance NIT on a non-hot path and does not affect correctness.
} }
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-13 17:50:21 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The refusal error includes the full request URL (including query string) which may carry sensitive data in some edge cases; if upstream logs this error verbatim, it could leak information. Consider redacting query parameters or logging a sanitized URL.

**[MINOR]** The refusal error includes the full request URL (including query string) which may carry sensitive data in some edge cases; if upstream logs this error verbatim, it could leak information. Consider redacting query parameters or logging a sanitized URL.
3
 @@ -281,6 +280,9 @@ func redactURL(rawURL string) string {
// It respects the Retry-After header when present, supporting both integer // It respects the Retry-After header when present, supporting both integer
// seconds and HTTP-date formats (capped at maxRetryAfter). // seconds and HTTP-date formats (capped at maxRetryAfter).
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) { func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
gpt-review-bot commented 2026-05-13 18:26:41 +00:00
Outdated
Review
Copy Link
Copy Source

[MAJOR] The insecure-HTTP guard uses strings.HasPrefix(reqURL, "http://") which is case-sensitive. URL schemes are case-insensitive; a URL like "HTTP://..." would bypass this check and send credentials over plaintext. Parse the URL and compare the scheme case-insensitively (e.g., url.Parse + strings.EqualFold(u.Scheme, "http")) or normalize to lower-case before checking.

**[MAJOR]** The insecure-HTTP guard uses strings.HasPrefix(reqURL, "http://") which is case-sensitive. URL schemes are case-insensitive; a URL like "HTTP://..." would bypass this check and send credentials over plaintext. Parse the URL and compare the scheme case-insensitively (e.g., url.Parse + strings.EqualFold(u.Scheme, "http")) or normalize to lower-case before checking.
// NOTE: This parses reqURL a second time (http.NewRequestWithContext parses it
// again internally). Acceptable cost: URL parsing is cheap and threading the
// parsed *url.URL through would complicate the interface for negligible gain.
if !c.allowInsecureHTTP { if !c.allowInsecureHTTP {
parsed, err := url.Parse(reqURL) parsed, err := url.Parse(reqURL)
if err != nil { if err != nil {
security-review-bot commented 2026-05-13 20:06:39 +00:00
Review
Copy Link
Copy Source

[MINOR] The error message includes user-influenced URL data via redactURL(reqURL) without explicit newline/carriage-return sanitization. If upstream code logs this error directly, it could allow limited log injection if a crafted URL with control characters is accepted by url.Parse. Consider sanitizing \n/\r in the formatted error or ensuring redactURL strips such characters.

**[MINOR]** The error message includes user-influenced URL data via redactURL(reqURL) without explicit newline/carriage-return sanitization. If upstream code logs this error directly, it could allow limited log injection if a crafted URL with control characters is accepted by url.Parse. Consider sanitizing \n/\r in the formatted error or ensuring redactURL strips such characters.
3