Compare commits
12 Commits
dc2e1ca5de
...
issue-124
| Author | SHA1 | Date | |
|---|---|---|---|
| f8b9d7d282 | |||
| 7a8fc166ec | |||
| 5e351b85f0 | |||
| ab2a6c8aef | |||
| 6b7f3f6924 | |||
| 4c032a3b53 | |||
| 64c9d551ba | |||
| db7b7e66bf | |||
| 0232343126 | |||
| b26514714f | |||
| 028d46942a | |||
| e59c2bc831 |
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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
@@ -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
@@ -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 {
|
||||
|
||||
@@ -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
@@ -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
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user