rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 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
+5 -6
2 changed files with 5 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ab2a6c8aef - Show all commits
github/client.go
+1 -2
View File
5
 @@ -155,7 +155,7 @@ type clientConfig struct {
// environment variable. Without the env var set, the option is ignored
// 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 a _test.go file in the same package) which bypasses the env gate.
func AllowInsecureHTTP() ClientOption {
return func(cfg *clientConfig) {
gpt-review-bot commented 2026-05-13 18:26:41 +00:00
Review
Copy Link
Copy Source

[MINOR] AllowInsecureHTTPForTest is exported but intended only for tests. Consider making it unexported (allowInsecureHTTPForTest) and using it from package-internal tests, or clearly document and enforce via build tags/export_test.go if cross-package tests require it, to reduce risk of accidental production use.

**[MINOR]** AllowInsecureHTTPForTest is exported but intended only for tests. Consider making it unexported (allowInsecureHTTPForTest) and using it from package-internal tests, or clearly document and enforce via build tags/export_test.go if cross-package tests require it, to reduce risk of accidental production use.
gpt-review-bot commented 2026-05-13 20:06:24 +00:00
Review
Copy Link
Copy Source

[NIT] For functional options, consider a With* naming convention (e.g., WithInsecureHTTP) to align with common Go patterns and improve discoverability (see configuration.md, Functional Options).

**[NIT]** For functional options, consider a With* naming convention (e.g., WithInsecureHTTP) to align with common Go patterns and improve discoverability (see configuration.md, Functional Options).
cfg.allowInsecureHTTP = true
22
 @@ -272,7 +272,6 @@ func redactURL(rawURL string) string {
if u.RawQuery != "" {
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.
u.Fragment = ""
return u.String()
}
security-review-bot commented 2026-05-13 18:26:17 +00:00
Outdated
Review
Copy Link
Copy Source

[MAJOR] The HTTP-scheme guard in doRequest uses a case-sensitive prefix check (strings.HasPrefix(reqURL, "http://"). URI schemes are case-insensitive (RFC 3986), so a URL like "HTTP://..." bypasses the guard and may send credentials over plaintext. Parse the URL and compare scheme case-insensitively (e.g., url.Parse + strings.EqualFold(u.Scheme, "http")).

**[MAJOR]** The HTTP-scheme guard in doRequest uses a case-sensitive prefix check (strings.HasPrefix(reqURL, "http://"). URI schemes are case-insensitive (RFC 3986), so a URL like "HTTP://..." bypasses the guard and may send credentials over plaintext. Parse the URL and compare scheme case-insensitively (e.g., url.Parse + strings.EqualFold(u.Scheme, "http")).
7
github/client_test.go
+4 -4
View File
5
 @@ -547,8 +547,8 @@ func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
// Verify case-insensitive scheme check (RFC 3986).
c := NewClient("tok", "HTTP://example.com")
_, err := c.doGet(context.Background(), "HTTP://example.com/test")
c := NewClient("tok", "HTTP://127.0.0.1:1")
_, err := c.doGet(context.Background(), "HTTP://127.0.0.1:1/test")
if err == nil {
t.Fatal("expected error for uppercase HTTP scheme")
}
sonnet-review-bot commented 2026-05-13 18:36:00 +00:00
Review
Copy Link
Copy Source

[NIT] Missing blank line between TestNoInsecureOption_RejectsHTTP and TestNoInsecureOption_RejectsUppercaseHTTP — minor formatting inconsistency compared to the rest of the file, but gofmt doesn't enforce blank lines between top-level declarations in the same way.

**[NIT]** Missing blank line between `TestNoInsecureOption_RejectsHTTP` and `TestNoInsecureOption_RejectsUppercaseHTTP` — minor formatting inconsistency compared to the rest of the file, but `gofmt` doesn't enforce blank lines between top-level declarations in the same way.
@@ -559,8 +559,8 @@ func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) {
// Verify mixed case like "Http://" is also rejected.
c := NewClient("tok", "Http://example.com")
_, err := c.doGet(context.Background(), "Http://example.com/test")
c := NewClient("tok", "Http://127.0.0.1:1")
_, err := c.doGet(context.Background(), "Http://127.0.0.1:1/test")
if err == nil {
t.Fatal("expected error for mixed-case HTTP scheme")
}
1