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

Add safeguards against accidental AllowInsecureHTTP use in production #96

New Issue
Closed
opened 2026-05-13 00:30:10 +00:00 by rodin · 1 comment
rodin commented 2026-05-13 00:30:10 +00:00
Owner
Copy Link
Copy Source

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

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
rodin self-assigned this 2026-05-13 17:24:46 +00:00
rodin commented 2026-05-13 17:26:18 +00:00
Author
Owner
Copy Link
Copy Source

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:

type clientConfig struct {
    allowInsecureHTTP    bool
    insecureIsTestBypass bool // skips env gate
}

In NewClient:

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.
## 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.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ai-review
bug
needs-review
ready
Ready for human review
 self-reviewed
Self-review passed since last push
 wip
Work in progress - do not start new work on this PR
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
security-review-bot rodin (Rodin)
No Assignees rodin
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#96
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?