Compare commits

...

12 Commits

Author SHA1 Message Date
aweiker f8b9d7d282 fix: portable checksum on darwin, anchor grep pattern
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 27s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m4s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m13s
CI / test (push) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
- sha256sum is not available on macOS; use shasum -a 256 on darwin.
  Select based on steps.version.outputs.os which is already computed.
  Fixes MAJOR finding from gpt-review-bot (PR #127 review).

- Anchor checksum grep with ^ to avoid matching on partial lines.
  Fixes MINOR finding from gpt-review-bot (PR #127 review).
2026-05-14 05:48:21 +00:00
aweiker 7a8fc166ec feat(action): derive binary name from uname for multi-arch support (#124)
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 26s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m12s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m33s
Previously the Install step hard-coded 'review-bot-linux-amd64'. This
fails on arm64 runners (Graviton, Apple Silicon) where uname -m returns
'aarch64' or 'arm64'.

Changes:
- Add OS/arch detection in 'Determine version' step using uname -s/-m
- Map uname output to asset name format: x86_64→amd64, aarch64/arm64→arm64,
  linux→linux, darwin→darwin
- Emit 'os' and 'arch' as step outputs alongside 'version'
- Update cache key: review-bot-{os}-{arch}-{version}
- Update Install step: BINARY derived from step outputs
- Anchor checksum grep to exact filename (not substring match)
- Unsupported OS or arch exits with a clear error message

Supported platforms: linux-amd64, linux-arm64, darwin-amd64, darwin-arm64
(matches what the release workflow builds)
2026-05-14 05:45:20 +00:00
aweiker 5e351b85f0 Merge pull request 'feat(github): add safeguards against accidental AllowInsecureHTTP use (#96)' (#113) from review-bot-issue-96 into main
CI / test (push) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
Reviewed-on: #113
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
2026-05-13 20:21:42 +00:00
claw ab2a6c8aef Address review feedback on PR #113
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 22s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m33s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m47s
- Fix AllowInsecureHTTP doc comment: say '_test.go file in the same
  package' instead of 'export_test.go' (MINOR #1)
- Remove dead u.Fragment = "" from redactURL: HTTP requests never
  carry fragments over the wire per RFC 7230 §5.1 (MINOR #2)
- Use 127.0.0.1:1 in scheme-rejection tests to make intent clearer
  that no network call should occur (NIT #3)
2026-05-13 13:04:23 -07:00
claw 6b7f3f6924 fix: address NIT findings from sonnet review (#113)
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 35s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m7s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m32s
- Fix AllowInsecureHTTP doc comment: 'silently ignored' -> 'ignored' (it logs a warning)
- Remove unnecessary nil guard in redactURL (assigning nil to nil is a no-op)
- Add clarifying comment about acceptable double URL parse in doRequest
2026-05-13 12:48:02 -07:00
claw 4c032a3b53 fix: move AllowInsecureHTTPForTest to export_test.go, fix gofmt alignment
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 30s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m14s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m45s
2026-05-13 11:58:25 -07:00
claw 64c9d551ba fix: address review feedback — restore timer.Stop() and fix test spacing
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 24s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 32s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m14s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m8s
- Restore timer.Stop() no-op in case <-timer.C for symmetry with ctx.Done
- Add missing blank line between TestNoInsecureOption_RejectsHTTP and
  TestNoInsecureOption_RejectsUppercaseHTTP
- Remove double blank line before TestAllowInsecureHTTP_WithoutEnvVar_Rejected

Resolves review comments from sonnet-review-bot on PR #113.
2026-05-13 11:44:28 -07:00
claw db7b7e66bf fix: use case-insensitive HTTP scheme check and redact userinfo
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 30s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 56s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m53s
Address review feedback on PR #113:

- MAJOR (both reviews): Replace strings.HasPrefix(reqURL, "http://")
  with url.Parse + strings.EqualFold for case-insensitive scheme
  comparison per RFC 3986. Prevents bypass via HTTP:// or Http://.

- MINOR (security): Enhance redactURL to strip userinfo component
  (user:pass@host) in addition to query params, preventing credential
  leakage in error messages and logs.

- NIT (gpt): Remove redundant timer.Stop() after timer.C fires —
  it's a no-op and the comment was misleading.

- Add tests for uppercase/mixed-case HTTP scheme rejection and
  userinfo redaction.
2026-05-13 11:35:10 -07:00
claw 0232343126 feat(github): add safeguards against accidental AllowInsecureHTTP use in production
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 13s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m45s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m9s
Three-layer defense for the AllowInsecureHTTP client option:

1. Environment gate: AllowInsecureHTTP() requires REVIEW_BOT_ALLOW_INSECURE=1
   env var. Without it, the option is silently ignored with a slog.Warn.

2. Warning log on activation: When the env gate IS satisfied, a slog.Warn
   fires at client construction time so operators notice in production logs.

3. Test bypass: AllowInsecureHTTPForTest() skips the env gate entirely,
   keeping test code clean (no t.Setenv needed in every test).

Additionally, doRequest now rejects HTTP URLs unless allowInsecureHTTP is set
on the client, providing defense-in-depth against credential leakage.

Closes #96
2026-05-13 11:24:07 -07:00
aweiker b26514714f Merge pull request 'feat(gitea): pass commit_id explicitly in PostReview (#107)' (#112) from review-bot-issue-107 into main
CI / test (push) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
Reviewed-on: #112
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>
2026-05-13 18:07:38 +00:00
claw 028d46942a fix(gitea): update PostReview doc comment to include COMMENT event value
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 16s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 25s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 53s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m4s
2026-05-13 10:41:52 -07:00
claw e59c2bc831 feat(gitea): pass commit_id explicitly in PostReview (#107)
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 25s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 42s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 57s
Add commitID parameter to gitea.Client.PostReview so the review is
anchored to the specific commit that was evaluated. The caller
(cmd/review-bot) already computes evaluatedSHA from pr.Head.Sha;
this wires it through to the Gitea API payload.

When commitID is empty, omitempty drops it from the JSON and Gitea
defaults to the current PR head (backward-compatible).

Closes #107
2026-05-13 10:29:05 -07:00
9 changed files with 321 additions and 33 deletions
+34 -5
View File
@@ -124,14 +124,38 @@ runs:
else
VERSION="${{ inputs.version }}"
fi
# Detect OS and architecture for platform-specific binary download
OS_RAW=$(uname -s | tr '[:upper:]' '[:lower:]')
case "$OS_RAW" in
linux) OS="linux" ;;
darwin) OS="darwin" ;;
*)
echo "Error: unsupported OS: $(uname -s)" >&2
exit 1
;;
esac
RAW_ARCH=$(uname -m)
case "$RAW_ARCH" in
x86_64) ARCH="amd64" ;;
aarch64 | arm64) ARCH="arm64" ;;
*)
echo "Error: unsupported architecture: $RAW_ARCH" >&2
exit 1
;;
esac
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
echo "os=${OS}" >> "$GITHUB_OUTPUT"
echo "arch=${ARCH}" >> "$GITHUB_OUTPUT"
- name: Cache review-bot binary
id: cache
uses: actions/cache@v4
with:
path: ${{ runner.temp }}/review-bot
key: review-bot-linux-amd64-${{ steps.version.outputs.version }}
key: review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}-${{ steps.version.outputs.version }}
- name: Install review-bot
if: steps.cache.outputs.cache-hit != 'true'
@@ -140,7 +164,7 @@ runs:
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
VERSION="${{ steps.version.outputs.version }}"
BINARY="review-bot-linux-amd64"
BINARY="review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
-o "${{ runner.temp }}/review-bot"
@@ -149,8 +173,13 @@ runs:
# Verify SHA-256 checksum
cd "${{ runner.temp }}"
EXPECTED=$(grep "${BINARY}" checksums.txt | awk '{print $1}')
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
EXPECTED=$(grep -E "^[[:xdigit:]]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')
# sha256sum (GNU) is not available on macOS; use shasum -a 256 on darwin.
if [ "${{ steps.version.outputs.os }}" = "darwin" ]; then
ACTUAL=$(shasum -a 256 review-bot | awk '{print $1}')
else
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
fi
if [ -z "$EXPECTED" ]; then
echo "Error: no checksum found for ${BINARY}" >&2
@@ -164,7 +193,7 @@ runs:
fi
chmod +x "${{ runner.temp }}/review-bot"
echo "Installed review-bot ${VERSION} (checksum verified)"
echo "Installed review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }} ${VERSION} (checksum verified)"
- name: Run review
shell: bash
+1 -1
View File
@@ -130,7 +130,7 @@ func TestIntegration_PostAndCleanup(t *testing.T) {
// Post a test review
sentinel := "<!-- review-bot:integration-test -->"
testBody := "# Integration Test Review\n\nThis is a test review.\n\n" + sentinel
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, nil)
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, "", nil)
if err != nil {
t.Fatalf("PostReview: %v", err)
}
+1 -1
View File
@@ -444,7 +444,7 @@ func main() {
// POST new review
slog.Info("posting review", "event", event, "pr", prNumber)
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)
if err != nil {
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
os.Exit(1)
+6 -2
View File
@@ -262,18 +262,22 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
}
// PostReview submits a review to a PR and returns the created review.
// event should be "APPROVED" or "REQUEST_CHANGES".
// event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT".
// commitID anchors the review to a specific commit SHA. If empty, Gitea
// defaults to the current PR head.
// comments are optional inline comments attached to specific lines.
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) {
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
payload := struct {
Body string `json:"body"`
Event string `json:"event"`
CommitID string `json:"commit_id,omitempty"`
Comments []ReviewComment `json:"comments,omitempty"`
}{
Body: body,
Event: event,
CommitID: commitID,
Comments: comments,
}
+31 -7
View File
@@ -117,8 +117,9 @@ func TestPostReview(t *testing.T) {
}
var payload struct {
Body string `json:"body"`
Event string `json:"event"`
Body string `json:"body"`
Event string `json:"event"`
CommitID string `json:"commit_id"`
}
if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
t.Fatalf("failed to decode payload: %v", err)
@@ -129,14 +130,16 @@ func TestPostReview(t *testing.T) {
if payload.Event != "APPROVED" {
t.Errorf("expected event %q, got %q", "APPROVED", payload.Event)
}
if payload.CommitID != "abc123def" {
t.Errorf("expected commit_id %q, got %q", "abc123def", payload.CommitID)
}
w.WriteHeader(http.StatusOK)
w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", nil)
review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "abc123def", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
@@ -183,12 +186,35 @@ func TestPostReview_Non200(t *testing.T) {
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", nil)
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", "", nil)
if err == nil {
t.Fatal("expected error for 403, got nil")
}
}
func TestPostReview_EmptyCommitID_OmittedFromPayload(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
body, _ := io.ReadAll(r.Body)
var raw map[string]interface{}
if err := json.Unmarshal(body, &raw); err != nil {
t.Fatalf("failed to decode payload: %v", err)
}
if _, exists := raw["commit_id"]; exists {
t.Errorf("expected commit_id to be omitted from payload when empty, but it was present")
}
w.WriteHeader(http.StatusOK)
w.Write([]byte(`{"id":200,"user":{"login":"bot"},"state":"APPROVED","stale":false}`))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "ok", "", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
}
func TestGetFileContent(t *testing.T) {
expected := "# Conventions\n- Be nice\n"
@@ -945,8 +971,6 @@ func TestDoGet_RespectsContextCancellation(t *testing.T) {
t.Errorf("attempts = %d, expected 1 before context cancel during backoff", attempts)
}
}
// mockTransport is a test helper that returns errors for the first N calls,
// then delegates to a real server.
type mockTransport struct {
+2 -2
View File
@@ -37,7 +37,7 @@ func TestPostReview_WithComments(t *testing.T) {
{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
}
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", comments)
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", "", comments)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
@@ -72,7 +72,7 @@ func TestPostReview_NilComments(t *testing.T) {
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", nil)
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", "", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
+79 -6
View File
@@ -8,7 +8,10 @@ import (
"errors"
"fmt"
"io"
"log/slog"
"net/http"
"net/url"
"os"
"strconv"
"strings"
"time"
@@ -93,10 +96,14 @@ type Client struct {
// Higher-level exported methods (GetPullRequest, etc.) will use it to
// construct request URLs; remove this field if those methods end up
// accepting full URLs instead.
baseURL string
token string
baseURL string
token string
httpClient *http.Client
// allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints.
// When false, doRequest rejects URLs with an http:// scheme.
allowInsecureHTTP bool
// retryBackoff defines the delays between retry attempts for 429 responses.
// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
// If nil, defaults to {1s, 2s}.
@@ -135,16 +142,53 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
return nil
}
// ClientOption configures optional behavior of a Client.
type ClientOption func(*clientConfig)
type clientConfig struct {
allowInsecureHTTP bool
insecureIsTestBypass bool
}
// AllowInsecureHTTP permits sending credentials over plaintext HTTP connections.
// In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1
// environment variable. Without the env var set, the option is ignored
// and a warning is logged.
//
// 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) {
cfg.allowInsecureHTTP = true
}
}
// NewClient creates a new GitHub API client.
// 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).
func NewClient(token, baseURL string) *Client {
func NewClient(token, baseURL string, opts ...ClientOption) *Client {
if baseURL == "" {
baseURL = defaultBaseURL
}
var cfg clientConfig
for _, opt := range opts {
opt(&cfg)
}
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")
}
}
return &Client{
baseURL: strings.TrimRight(baseURL, "/"),
token: token,
baseURL: strings.TrimRight(baseURL, "/"),
token: token,
allowInsecureHTTP: cfg.allowInsecureHTTP,
httpClient: &http.Client{
Timeout: 30 * time.Second,
CheckRedirect: defaultCheckRedirect,
@@ -215,10 +259,39 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
return 0, false
}
// redactURL redacts sensitive components from a URL for safe inclusion in error
// messages and log output. It removes userinfo (e.g., user:pass@) and replaces
// query parameters with a placeholder.
func redactURL(rawURL string) string {
u, err := url.Parse(rawURL)
if err != nil {
return "<unparseable URL>"
}
u.User = nil
if u.RawQuery != "" {
u.RawQuery = "<redacted>"
}
return u.String()
}
// doRequest performs an HTTP request with retry on 429 rate limit responses.
// It respects the Retry-After header when present, supporting both integer
// seconds and HTTP-date formats (capped at maxRetryAfter).
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
// 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 {
parsed, err := url.Parse(reqURL)
if err != nil {
return nil, fmt.Errorf("parse request URL: %w", err)
}
if strings.EqualFold(parsed.Scheme, "http") {
return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL))
}
}
var backoff []time.Duration
if c.retryBackoff != nil {
backoff = append([]time.Duration(nil), c.retryBackoff...)
@@ -237,7 +310,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
timer := time.NewTimer(delay)
select {
case <-timer.C:
timer.Stop()
timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
case <-ctx.Done():
timer.Stop()
return nil, ctx.Err()
+154 -9
View File
@@ -35,7 +35,7 @@ func TestDoRequest_Success(t *testing.T) {
}))
defer srv.Close()
c := NewClient("test-token", srv.URL)
c := NewClient("test-token", srv.URL, AllowInsecureHTTPForTest())
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
@@ -60,7 +60,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{0, 0})
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -94,7 +94,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.now = func() time.Time { return fixedNow }
// Initial backoff is 0; the HTTP-date parser will compute 1s and override.
c.SetRetryBackoff([]time.Duration{0, 0})
@@ -130,7 +130,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate_InPast(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.now = func() time.Time { return fixedNow }
c.SetRetryBackoff([]time.Duration{0, 0})
@@ -157,7 +157,7 @@ func TestDoRequest_429_NoRetryAfter_UsesDefaultBackoff(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{0, 0})
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -187,7 +187,7 @@ func TestDoRequest_429_InvalidRetryAfter_UsesDefaultBackoff(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{0, 0})
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -208,7 +208,7 @@ func TestDoRequest_404_NoRetry(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error, got nil")
@@ -230,7 +230,7 @@ func TestDoRequest_401_NoRetry(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error, got nil")
@@ -260,7 +260,7 @@ func TestDoRequest_ContextCanceled(t *testing.T) {
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
ctx, cancel := context.WithCancel(context.Background())
@@ -511,3 +511,148 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
}
}
func TestAllowInsecureHTTPForTest_PermitsHTTP(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
w.Write([]byte("ok"))
}))
defer srv.Close()
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(body) != "ok" {
t.Errorf("body = %q, want %q", body, "ok")
}
}
func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not have been sent")
}))
defer srv.Close()
c := NewClient("tok", srv.URL)
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error for HTTP request without AllowInsecureHTTP")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
// Verify case-insensitive scheme check (RFC 3986).
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")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) {
// Verify mixed case like "Http://" is also rejected.
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")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not have been sent")
}))
defer srv.Close()
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "")
c := NewClient("tok", srv.URL, AllowInsecureHTTP())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error: AllowInsecureHTTP without env var should be rejected")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestAllowInsecureHTTP_WithEnvVar_Permitted(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
w.Write([]byte("insecure-ok"))
}))
defer srv.Close()
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1")
c := NewClient("tok", srv.URL, AllowInsecureHTTP())
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(body) != "insecure-ok" {
t.Errorf("body = %q, want %q", body, "insecure-ok")
}
}
func TestAllowInsecureHTTP_EnvVarNotOne_Rejected(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not have been sent")
}))
defer srv.Close()
// "true" is not "1" — strict check
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true")
c := NewClient("tok", srv.URL, AllowInsecureHTTP())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error: env var 'true' is not '1'")
}
if !strings.Contains(err.Error(), "refusing HTTP request") {
t.Errorf("unexpected error message: %v", err)
}
}
func TestRedactURL_WithQuery(t *testing.T) {
got := redactURL("http://localhost:1234/path?secret=token&foo=bar")
want := "http://localhost:1234/path?<redacted>"
if got != want {
t.Errorf("redactURL = %q, want %q", got, want)
}
}
func TestRedactURL_NoQuery(t *testing.T) {
got := redactURL("http://localhost:1234/path")
want := "http://localhost:1234/path"
if got != want {
t.Errorf("redactURL = %q, want %q", got, want)
}
}
func TestRedactURL_Userinfo(t *testing.T) {
got := redactURL("http://user:pass@localhost:1234/path")
want := "http://localhost:1234/path"
if got != want {
t.Errorf("redactURL = %q, want %q", got, want)
}
}
func TestRedactURL_UserinfoWithQuery(t *testing.T) {
got := redactURL("http://user:pass@localhost:1234/path?secret=token")
want := "http://localhost:1234/path?<redacted>"
if got != want {
t.Errorf("redactURL = %q, want %q", got, want)
}
}
+13
View File
@@ -0,0 +1,13 @@
package github
// 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
}
}