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
+18 -15
2 changed files with 18 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 4c032a3b53 - Show all commits
github/client.go
+5 -15
View File
@@ -96,8 +96,8 @@ type Client struct {
// Higher-level exported methods (GetPullRequest, etc.) will use it to // Higher-level exported methods (GetPullRequest, etc.) will use it to
sonnet-review-bot commented 2026-05-13 18:10:58 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The testBypass field on clientConfig is an internal implementation detail that controls whether the env gate is bypassed. Since clientConfig is unexported and only accessible within the package, this is fine. However, the field comment // skip env gate (for tests only) could be strengthened to note that only WithAllowInsecureHTTPForTest (defined in export_test.go) should ever set this field, to make it clear no production code path should touch it.

**[NIT]** The `testBypass` field on `clientConfig` is an internal implementation detail that controls whether the env gate is bypassed. Since `clientConfig` is unexported and only accessible within the package, this is fine. However, the field comment `// skip env gate (for tests only)` could be strengthened to note that only `WithAllowInsecureHTTPForTest` (defined in export_test.go) should ever set this field, to make it clear no production code path should touch it.
// construct request URLs; remove this field if those methods end up // construct request URLs; remove this field if those methods end up
// accepting full URLs instead. // accepting full URLs instead.
baseURL string baseURL string
token string token string
gpt-review-bot commented 2026-05-13 17:50:27 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The functional option function is named AllowInsecureHTTP rather than following the documented With* naming convention (e.g., WithAllowInsecureHTTP). Consider aligning with the 'Functional Options (With* Pattern)' for consistency.

**[MINOR]** The functional option function is named AllowInsecureHTTP rather than following the documented With* naming convention (e.g., WithAllowInsecureHTTP). Consider aligning with the 'Functional Options (With* Pattern)' for consistency.
httpClient *http.Client httpClient *http.Client
// allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints. // allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints.
3
 @@ -155,23 +155,13 @@ type clientConfig struct {
// environment variable. Without the env var set, the option is silently ignored // environment variable. Without the env var set, the option is silently 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, prefer AllowInsecureHTTPForTest which bypasses the env gate. // For tests, use AllowInsecureHTTPForTest (defined in export_test.go) which bypasses the env gate.
func AllowInsecureHTTP() ClientOption { func AllowInsecureHTTP() ClientOption {
return func(cfg *clientConfig) { 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 cfg.allowInsecureHTTP = true
} }
sonnet-review-bot commented 2026-05-13 20:05:14 +00:00
Review
Copy Link
Copy Source

[NIT] The doc comment for AllowInsecureHTTP() has an overly long line: 'For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate.' This wraps beyond 80 chars and doesn't follow the conventional line-length style seen elsewhere in the file, though this is cosmetic only.

**[NIT]** The doc comment for AllowInsecureHTTP() has an overly long line: 'For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate.' This wraps beyond 80 chars and doesn't follow the conventional line-length style seen elsewhere in the file, though this is cosmetic only.
} }
sonnet-review-bot commented 2026-05-13 18:25:10 +00:00
Review
Copy Link
Copy Source

[MINOR] The doc comment for AllowInsecureHTTP() says the option is 'silently ignored' but the very next sentence says 'a warning is logged'. These are contradictory — being warned in logs is not silent. The comment should say something like 'the option is ignored and a warning is logged via slog.Warn'.

**[MINOR]** The doc comment for `AllowInsecureHTTP()` says the option is 'silently ignored' but the very next sentence says 'a warning is logged'. These are contradictory — being warned in logs is not silent. The comment should say something like 'the option is ignored and a warning is logged via slog.Warn'.
// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP
// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable.
// This is intended exclusively for test code using httptest.Server.
func AllowInsecureHTTPForTest() ClientOption {
return func(cfg *clientConfig) {
cfg.allowInsecureHTTP = true
cfg.insecureIsTestBypass = true
}
}
// NewClient creates a new GitHub API client. // NewClient creates a new GitHub API client.
gpt-review-bot commented 2026-05-13 18:23:00 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Doc comment for NewClient references WithAllowInsecureHTTPForTest, which is a test-only symbol and not part of the production API. This could confuse users reading package docs; consider rephrasing to avoid mentioning test-only helpers in production documentation.

**[NIT]** Doc comment for NewClient references WithAllowInsecureHTTPForTest, which is a test-only symbol and not part of the production API. This could confuse users reading package docs; consider rephrasing to avoid mentioning test-only helpers in production documentation.
// If baseURL is empty, it defaults to https://api.github.com. // If baseURL is empty, it defaults to https://api.github.com.
// For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3). // For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3).
gpt-review-bot commented 2026-05-13 18:23:00 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] NewClient signature changed to accept variadic options (opts ...ClientOption), which is a breaking API change for callers using the previous signature. Consider providing a backward-compatible wrapper (overload pattern) or calling out the breaking change explicitly in release notes.

**[MINOR]** NewClient signature changed to accept variadic options (opts ...ClientOption), which is a breaking API change for callers using the previous signature. Consider providing a backward-compatible wrapper (overload pattern) or calling out the breaking change explicitly in release notes.
8
 @@ -196,8 +186,8 @@ func NewClient(token, baseURL string, opts ...ClientOption) *Client {
} }
sonnet-review-bot commented 2026-05-13 17:49:18 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] NewClient reads os.Getenv at construction time, which makes the env-gate check susceptible to a subtle race: if a caller sets the env var after program startup but before calling NewClient (rare but possible in CLI tools), the behavior is correct, but the deeper issue is that env reads in constructors make testing harder and can be surprising. The pattern is pragmatic here and t.Setenv handles it in tests, but worth noting that the test-bypass field (testBypass) in clientConfig is part of the exported clientConfig type — if clientConfig is ever exported or accessed via reflection, testBypass would be visible. Since it's unexported, this is fine as-is.

**[MINOR]** NewClient reads os.Getenv at construction time, which makes the env-gate check susceptible to a subtle race: if a caller sets the env var after program startup but before calling NewClient (rare but possible in CLI tools), the behavior is correct, but the deeper issue is that env reads in constructors make testing harder and can be surprising. The pattern is pragmatic here and t.Setenv handles it in tests, but worth noting that the test-bypass field (testBypass) in clientConfig is part of the exported clientConfig type — if clientConfig is ever exported or accessed via reflection, testBypass would be visible. Since it's unexported, this is fine as-is.
return &Client{ return &Client{
baseURL: strings.TrimRight(baseURL, "/"), baseURL: strings.TrimRight(baseURL, "/"),
token: token, token: token,
allowInsecureHTTP: cfg.allowInsecureHTTP, allowInsecureHTTP: cfg.allowInsecureHTTP,
httpClient: &http.Client{ httpClient: &http.Client{
sonnet-review-bot commented 2026-05-13 17:32:23 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] NewClient now reads os.Getenv at construction time and calls slog.Warn, creating a side effect (log output) when constructing a client with AllowInsecureHTTP() but without the env var. The conventions doc says 'never panic; return errors' and the pattern docs suggest constructors should avoid surprising side effects. The silent-ignore + warn behavior is a deliberate design choice documented in the PR, but callers who pass AllowInsecureHTTP() will be silently downgraded to a restricted client with no feedback other than a log line — a future caller who forgets the env var in production will get unexpected 'refusing to send credentials' errors from doRequest that don't clearly indicate the option was silently ignored at construction. Returning an error from NewClient or using a different API shape (e.g., requiring the env-gate check to be explicit) would be cleaner, but this is a design tradeoff, not a bug.

**[NIT]** NewClient now reads os.Getenv at construction time and calls slog.Warn, creating a side effect (log output) when constructing a client with AllowInsecureHTTP() but without the env var. The conventions doc says 'never panic; return errors' and the pattern docs suggest constructors should avoid surprising side effects. The silent-ignore + warn behavior is a deliberate design choice documented in the PR, but callers who pass AllowInsecureHTTP() will be silently downgraded to a restricted client with no feedback other than a log line — a future caller who forgets the env var in production will get unexpected 'refusing to send credentials' errors from doRequest that don't clearly indicate the option was silently ignored at construction. Returning an error from NewClient or using a different API shape (e.g., requiring the env-gate check to be explicit) would be cleaner, but this is a design tradeoff, not a bug.
Timeout: 30 * time.Second, Timeout: 30 * time.Second,
sonnet-review-bot commented 2026-05-13 19:48:59 +00:00
Review
Copy Link
Copy Source

[NIT] The warn log 'AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable' fires silently in tests that run in environments where the env var isn't set. Since NewClient is called in many tests (e.g. TestNewClient_DefaultBaseURL, TestParseRetryAfter_* etc.) without AllowInsecureHTTP(), this won't fire there. But any caller using AllowInsecureHTTP() in a test without the env var will produce a slog.Warn to whatever slog default is configured. The AllowInsecureHTTPForTest bypass prevents this for test servers, so this is acceptable — just worth noting that the warn will fire in non-test environments where someone misconfigures the option.

**[NIT]** The warn log 'AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable' fires silently in tests that run in environments where the env var isn't set. Since NewClient is called in many tests (e.g. TestNewClient_DefaultBaseURL, TestParseRetryAfter_* etc.) without AllowInsecureHTTP(), this won't fire there. But any caller using AllowInsecureHTTP() in a test without the env var will produce a slog.Warn to whatever slog default is configured. The AllowInsecureHTTPForTest bypass prevents this for test servers, so this is acceptable — just worth noting that the warn will fire in non-test environments where someone misconfigures the option.
17
github/export_test.go
+13
View File
@@ -0,0 +1,13 @@
package github
sonnet-review-bot commented 2026-05-13 17:49:18 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The file uses package github (not package github_test), which is correct for the export_test.go pattern — it compiles only in test binaries and can access unexported types. This is well-documented in the comment. No issue; just confirming the pattern is applied correctly per the testing-advanced.md pattern #11.

**[NIT]** The file uses `package github` (not `package github_test`), which is correct for the export_test.go pattern — it compiles only in test binaries and can access unexported types. This is well-documented in the comment. No issue; just confirming the pattern is applied correctly per the testing-advanced.md pattern #11.
sonnet-review-bot commented 2026-05-13 18:10:58 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The file is declared as package github (not package github_test), which is the correct pattern for the export_test.go idiom used in the stdlib. This is intentional and correct — just noting it matches the documented pattern from the testing patterns guide.

**[NIT]** The file is declared as `package github` (not `package github_test`), which is the correct pattern for the export_test.go idiom used in the stdlib. This is intentional and correct — just noting it matches the documented pattern from the testing patterns guide.
// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP
// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable.
// This is intended exclusively for test code using httptest.Server.
//
// Defined in a _test.go file so it is only available to test binaries.
func AllowInsecureHTTPForTest() ClientOption {
return func(cfg *clientConfig) {
cfg.allowInsecureHTTP = true
cfg.insecureIsTestBypass = true
}
}