From security review on PR #93 (review #2870, finding #2):
The AllowInsecureHTTP option permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext.
Proposed improvements:
Require an explicit environment gate (e.g., REVIEW_BOT_ALLOW_INSECURE=1) in addition to the option
Or fail fast unless a dedicated test build tag is present
Or log a warning when AllowInsecureHTTP is active
This adds defense-in-depth so that misconfiguration doesn't silently degrade security.
From security review on PR #93 (review #2870, finding #2):
The `AllowInsecureHTTP` option permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext.
**Proposed improvements:**
- Require an explicit environment gate (e.g., `REVIEW_BOT_ALLOW_INSECURE=1`) in addition to the option
- Or fail fast unless a dedicated test build tag is present
- Or log a warning when AllowInsecureHTTP is active
This adds defense-in-depth so that misconfiguration doesn't silently degrade security.
Ref: PR #93 review #2870
Plan: Add safeguards against accidental AllowInsecureHTTP use in production
Problem
AllowInsecureHTTP() is a client option that permits sending credentials over HTTP. It's currently a simple boolean with no guardrails — if someone passes it in production code, credentials silently flow over cleartext. Tests use it heavily (httptest servers are HTTP), but production usage would be a security incident.
Constraints
Must not break existing tests (all tests use AllowInsecureHTTP() with httptest.Server)
Must not add external dependencies (module only has gopkg.in/yaml.v3)
The github package currently has zero slog or log imports — adding structured logging is acceptable
Solution must be defense-in-depth (multiple layers)
Proposed Approach
Three-layer defense:
Environment gate: When AllowInsecureHTTP() is active, require env var REVIEW_BOT_ALLOW_INSECURE=1. Without it, the option is silently ignored (treated as if not set). This prevents accidental enablement from config drift.
Warning log on activation: When the env gate IS satisfied and insecure mode activates, emit a slog.Warn so operators notice in production logs. The warning fires once at client construction time.
Test build tag alternative: Provide AllowInsecureHTTPForTest() — a separate option that skips the env gate. Tests use this instead, keeping test code clean while production code requires the env gate. This avoids needing t.Setenv(...) in every test.
ifcfg.allowInsecureHTTP&&!cfg.insecureIsTestBypass{ifos.Getenv("REVIEW_BOT_ALLOW_INSECURE")!="1"{slog.Warn("AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable")cfg.allowInsecureHTTP=false}else{slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext","env","REVIEW_BOT_ALLOW_INSECURE=1")}}
AllowInsecureHTTPForTest() sets both allowInsecureHTTP and insecureIsTestBypass to true, bypassing the env check. Existing tests switch from AllowInsecureHTTP() to AllowInsecureHTTPForTest().
Files Changed
File
Change
github/client.go
Add insecureIsTestBypass field, env gate logic in NewClient, AllowInsecureHTTPForTest() option, add log/slog + os imports
github/client_test.go
Switch to AllowInsecureHTTPForTest(), add tests for env gate behavior
github/pr_test.go
Switch to AllowInsecureHTTPForTest()
github/files_test.go
Switch to AllowInsecureHTTPForTest()
github/helpers_test.go
Switch to AllowInsecureHTTPForTest()
Error Cases
AllowInsecureHTTP() without env var → option silently disabled, warning logged, credentials still protected
AllowInsecureHTTP() with env var → works, but warning logged (operators notice)
AllowInsecureHTTPForTest() → always works, no env check (test-only path)
Empty token + AllowInsecureHTTP() → no env gate needed (existing logic already skips check when token is empty)
Edge Cases
Env var set to "true" or "yes" instead of "1" → strict check: only "1" works (fail-closed)
Env var set but to empty string → treated as not set
Multiple clients in same process → each logs independently (acceptable, slog deduplication is caller's concern)
Testing Strategy
Test: AllowInsecureHTTP() without env → HTTP request refused (same as no option)
Test: AllowInsecureHTTP() with env REVIEW_BOT_ALLOW_INSECURE=1 → HTTP permitted
Test: AllowInsecureHTTPForTest() → HTTP permitted without env var
Test: All existing tests pass unchanged after bulk rename to AllowInsecureHTTPForTest()
Open Questions
Should AllowInsecureHTTP() without the env var return an error from NewClient rather than silently disabling? I lean toward silent disable + warning log because NewClient currently has no error return. Changing the signature would be a breaking API change for a defense-in-depth feature. The warning log provides visibility.
Should the env var name be REVIEW_BOT_ALLOW_INSECURE or something more generic like ALLOW_INSECURE_HTTP? Using the REVIEW_BOT_ prefix avoids namespace collisions with other tools.
## Plan: Add safeguards against accidental AllowInsecureHTTP use in production
### Problem
`AllowInsecureHTTP()` is a client option that permits sending credentials over HTTP. It's currently a simple boolean with no guardrails — if someone passes it in production code, credentials silently flow over cleartext. Tests use it heavily (httptest servers are HTTP), but production usage would be a security incident.
### Constraints
- Must not break existing tests (all tests use `AllowInsecureHTTP()` with `httptest.Server`)
- Must not add external dependencies (module only has `gopkg.in/yaml.v3`)
- The `github` package currently has zero `slog` or `log` imports — adding structured logging is acceptable
- Solution must be defense-in-depth (multiple layers)
### Proposed Approach
**Three-layer defense:**
1. **Environment gate**: When `AllowInsecureHTTP()` is active, require env var `REVIEW_BOT_ALLOW_INSECURE=1`. Without it, the option is silently ignored (treated as if not set). This prevents accidental enablement from config drift.
2. **Warning log on activation**: When the env gate IS satisfied and insecure mode activates, emit a `slog.Warn` so operators notice in production logs. The warning fires once at client construction time.
3. **Test build tag alternative**: Provide `AllowInsecureHTTPForTest()` — a separate option that skips the env gate. Tests use this instead, keeping test code clean while production code requires the env gate. This avoids needing `t.Setenv(...)` in every test.
**Implementation detail:**
In `clientConfig` and `Client`:
```go
type clientConfig struct {
allowInsecureHTTP bool
insecureIsTestBypass bool // skips env gate
}
```
In `NewClient`:
```go
if cfg.allowInsecureHTTP && !cfg.insecureIsTestBypass {
if os.Getenv("REVIEW_BOT_ALLOW_INSECURE") != "1" {
slog.Warn("AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable")
cfg.allowInsecureHTTP = false
} else {
slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext",
"env", "REVIEW_BOT_ALLOW_INSECURE=1")
}
}
```
`AllowInsecureHTTPForTest()` sets both `allowInsecureHTTP` and `insecureIsTestBypass` to true, bypassing the env check. Existing tests switch from `AllowInsecureHTTP()` to `AllowInsecureHTTPForTest()`.
### Files Changed
| File | Change |
|------|--------|
| `github/client.go` | Add `insecureIsTestBypass` field, env gate logic in `NewClient`, `AllowInsecureHTTPForTest()` option, add `log/slog` + `os` imports |
| `github/client_test.go` | Switch to `AllowInsecureHTTPForTest()`, add tests for env gate behavior |
| `github/pr_test.go` | Switch to `AllowInsecureHTTPForTest()` |
| `github/files_test.go` | Switch to `AllowInsecureHTTPForTest()` |
| `github/helpers_test.go` | Switch to `AllowInsecureHTTPForTest()` |
### Error Cases
- `AllowInsecureHTTP()` without env var → option silently disabled, warning logged, credentials still protected
- `AllowInsecureHTTP()` with env var → works, but warning logged (operators notice)
- `AllowInsecureHTTPForTest()` → always works, no env check (test-only path)
- Empty token + `AllowInsecureHTTP()` → no env gate needed (existing logic already skips check when token is empty)
### Edge Cases
- Env var set to "true" or "yes" instead of "1" → strict check: only "1" works (fail-closed)
- Env var set but to empty string → treated as not set
- Multiple clients in same process → each logs independently (acceptable, slog deduplication is caller's concern)
### Testing Strategy
- Test: `AllowInsecureHTTP()` without env → HTTP request refused (same as no option)
- Test: `AllowInsecureHTTP()` with env `REVIEW_BOT_ALLOW_INSECURE=1` → HTTP permitted
- Test: `AllowInsecureHTTPForTest()` → HTTP permitted without env var
- Test: All existing tests pass unchanged after bulk rename to `AllowInsecureHTTPForTest()`
### Open Questions
- Should `AllowInsecureHTTP()` without the env var return an error from `NewClient` rather than silently disabling? I lean toward silent disable + warning log because `NewClient` currently has no error return. Changing the signature would be a breaking API change for a defense-in-depth feature. The warning log provides visibility.
- Should the env var name be `REVIEW_BOT_ALLOW_INSECURE` or something more generic like `ALLOW_INSECURE_HTTP`? Using the `REVIEW_BOT_` prefix avoids namespace collisions with other tools.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
From security review on PR #93 (review #2870, finding #2):
The
AllowInsecureHTTPoption permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext.Proposed improvements:
REVIEW_BOT_ALLOW_INSECURE=1) in addition to the optionThis adds defense-in-depth so that misconfiguration doesn't silently degrade security.
Ref: PR #93 review #2870
Plan: Add safeguards against accidental AllowInsecureHTTP use in production
Problem
AllowInsecureHTTP()is a client option that permits sending credentials over HTTP. It's currently a simple boolean with no guardrails — if someone passes it in production code, credentials silently flow over cleartext. Tests use it heavily (httptest servers are HTTP), but production usage would be a security incident.Constraints
AllowInsecureHTTP()withhttptest.Server)gopkg.in/yaml.v3)githubpackage currently has zeroslogorlogimports — adding structured logging is acceptableProposed Approach
Three-layer defense:
Environment gate: When
AllowInsecureHTTP()is active, require env varREVIEW_BOT_ALLOW_INSECURE=1. Without it, the option is silently ignored (treated as if not set). This prevents accidental enablement from config drift.Warning log on activation: When the env gate IS satisfied and insecure mode activates, emit a
slog.Warnso operators notice in production logs. The warning fires once at client construction time.Test build tag alternative: Provide
AllowInsecureHTTPForTest()— a separate option that skips the env gate. Tests use this instead, keeping test code clean while production code requires the env gate. This avoids needingt.Setenv(...)in every test.Implementation detail:
In
clientConfigandClient:In
NewClient:AllowInsecureHTTPForTest()sets bothallowInsecureHTTPandinsecureIsTestBypassto true, bypassing the env check. Existing tests switch fromAllowInsecureHTTP()toAllowInsecureHTTPForTest().Files Changed
github/client.goinsecureIsTestBypassfield, env gate logic inNewClient,AllowInsecureHTTPForTest()option, addlog/slog+osimportsgithub/client_test.goAllowInsecureHTTPForTest(), add tests for env gate behaviorgithub/pr_test.goAllowInsecureHTTPForTest()github/files_test.goAllowInsecureHTTPForTest()github/helpers_test.goAllowInsecureHTTPForTest()Error Cases
AllowInsecureHTTP()without env var → option silently disabled, warning logged, credentials still protectedAllowInsecureHTTP()with env var → works, but warning logged (operators notice)AllowInsecureHTTPForTest()→ always works, no env check (test-only path)AllowInsecureHTTP()→ no env gate needed (existing logic already skips check when token is empty)Edge Cases
Testing Strategy
AllowInsecureHTTP()without env → HTTP request refused (same as no option)AllowInsecureHTTP()with envREVIEW_BOT_ALLOW_INSECURE=1→ HTTP permittedAllowInsecureHTTPForTest()→ HTTP permitted without env varAllowInsecureHTTPForTest()Open Questions
AllowInsecureHTTP()without the env var return an error fromNewClientrather than silently disabling? I lean toward silent disable + warning log becauseNewClientcurrently has no error return. Changing the signature would be a breaking API change for a defense-in-depth feature. The warning log provides visibility.REVIEW_BOT_ALLOW_INSECUREor something more generic likeALLOW_INSECURE_HTTP? Using theREVIEW_BOT_prefix avoids namespace collisions with other tools.