review-bot/github/client_test.go

package github

import (
	"context"
	"errors"
	"net/http"
	"net/http/httptest"
	"net/url"
	"strings"
	"testing"
	"time"
)

func TestNewClient_DefaultBaseURL(t *testing.T) {
	c := NewClient("tok", "")
	if c.baseURL != defaultBaseURL {
		t.Errorf("baseURL = %q, want %q", c.baseURL, defaultBaseURL)
	}
}

func TestNewClient_CustomBaseURL(t *testing.T) {
	c := NewClient("tok", "https://github.concur.com/api/v3/")
	if c.baseURL != "https://github.concur.com/api/v3" {
		t.Errorf("baseURL = %q, want trailing slash stripped", c.baseURL)
	}
}

func TestDoRequest_Success(t *testing.T) {
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if got := r.Header.Get("Authorization"); got != "Bearer test-token" {
			t.Errorf("Authorization = %q, want Bearer test-token", got)
		}
		w.WriteHeader(http.StatusOK)
		w.Write([]byte(`{"ok":true}`))
	}))
	defer srv.Close()

	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)
	}
	if string(body) != `{"ok":true}` {
		t.Errorf("body = %q, want %q", body, `{"ok":true}`)
	}
}

func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
	attempts := 0
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		attempts++
		if attempts == 1 {
			w.Header().Set("Retry-After", "0")
			w.WriteHeader(http.StatusTooManyRequests)
			w.Write([]byte("rate limited"))
			return
		}
		w.WriteHeader(http.StatusOK)
		w.Write([]byte("success"))
	}))
	defer srv.Close()

	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
	c.SetRetryBackoff([]time.Duration{0, 0})

	body, err := c.doGet(context.Background(), srv.URL+"/test")
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
	if string(body) != "success" {
		t.Errorf("body = %q, want %q", body, "success")
	}
	if attempts != 2 {
		t.Errorf("attempts = %d, want 2", attempts)
	}
}

func TestDoRequest_429_RetryAfter_HTTPDate(t *testing.T) {
	// Fix "now" to a known time for deterministic testing.
	fixedNow := time.Date(2025, 12, 1, 15, 59, 59, 0, time.UTC)
	retryAt := "Mon, 01 Dec 2025 16:00:00 GMT" // 1 second in the future

	attempts := 0
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		attempts++
		if attempts == 1 {
			w.Header().Set("Retry-After", retryAt)
			w.WriteHeader(http.StatusTooManyRequests)
			w.Write([]byte("rate limited"))
			return
		}
		w.WriteHeader(http.StatusOK)
		w.Write([]byte("success"))
	}))
	defer srv.Close()

	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})

	body, err := c.doGet(context.Background(), srv.URL+"/test")
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
	if string(body) != "success" {
		t.Errorf("body = %q, want %q", body, "success")
	}
	if attempts != 2 {
		t.Errorf("attempts = %d, want 2", attempts)
	}
}

func TestDoRequest_429_RetryAfter_HTTPDate_InPast(t *testing.T) {
	// If the HTTP-date is in the past, delay should be 0 (retry immediately).
	fixedNow := time.Date(2025, 12, 1, 17, 0, 0, 0, time.UTC)
	retryAt := "Mon, 01 Dec 2025 16:00:00 GMT" // 1 hour in the past

	attempts := 0
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		attempts++
		if attempts == 1 {
			w.Header().Set("Retry-After", retryAt)
			w.WriteHeader(http.StatusTooManyRequests)
			w.Write([]byte("rate limited"))
			return
		}
		w.WriteHeader(http.StatusOK)
		w.Write([]byte("success"))
	}))
	defer srv.Close()

	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
	c.now = func() time.Time { return fixedNow }
	c.SetRetryBackoff([]time.Duration{0, 0})

	body, err := c.doGet(context.Background(), srv.URL+"/test")
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
	if string(body) != "success" {
		t.Errorf("body = %q, want %q", body, "success")
	}
}

func TestDoRequest_429_NoRetryAfter_UsesDefaultBackoff(t *testing.T) {
	attempts := 0
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		attempts++
		if attempts == 1 {
			w.WriteHeader(http.StatusTooManyRequests)
			w.Write([]byte("rate limited"))
			return
		}
		w.WriteHeader(http.StatusOK)
		w.Write([]byte("success"))
	}))
	defer srv.Close()

	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
	c.SetRetryBackoff([]time.Duration{0, 0})

	body, err := c.doGet(context.Background(), srv.URL+"/test")
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
	if string(body) != "success" {
		t.Errorf("body = %q, want %q", body, "success")
	}
	if attempts != 2 {
		t.Errorf("attempts = %d, want 2", attempts)
	}
}

func TestDoRequest_429_InvalidRetryAfter_UsesDefaultBackoff(t *testing.T) {
	attempts := 0
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		attempts++
		if attempts == 1 {
			w.Header().Set("Retry-After", "not-a-number-or-date")
			w.WriteHeader(http.StatusTooManyRequests)
			w.Write([]byte("rate limited"))
			return
		}
		w.WriteHeader(http.StatusOK)
		w.Write([]byte("success"))
	}))
	defer srv.Close()

	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
	c.SetRetryBackoff([]time.Duration{0, 0})

	body, err := c.doGet(context.Background(), srv.URL+"/test")
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
	if string(body) != "success" {
		t.Errorf("body = %q, want %q", body, "success")
	}
}

func TestDoRequest_404_NoRetry(t *testing.T) {
	attempts := 0
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		attempts++
		w.WriteHeader(http.StatusNotFound)
		w.Write([]byte("not found"))
	}))
	defer srv.Close()

	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
	_, err := c.doGet(context.Background(), srv.URL+"/test")
	if err == nil {
		t.Fatal("expected error, got nil")
	}
	if !IsNotFound(err) {
		t.Errorf("expected IsNotFound, got %v", err)
	}
	if attempts != 1 {
		t.Errorf("attempts = %d, want 1 (no retry on 404)", attempts)
	}
}

func TestDoRequest_401_NoRetry(t *testing.T) {
	attempts := 0
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		attempts++
		w.WriteHeader(http.StatusUnauthorized)
		w.Write([]byte("unauthorized"))
	}))
	defer srv.Close()

	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
	_, err := c.doGet(context.Background(), srv.URL+"/test")
	if err == nil {
		t.Fatal("expected error, got nil")
	}
	if !IsUnauthorized(err) {
		t.Errorf("expected IsUnauthorized, got %v", err)
	}
	if attempts != 1 {
		t.Errorf("attempts = %d, want 1 (no retry on 401)", attempts)
	}
}

func TestDoRequest_ContextCanceled(t *testing.T) {
	// This test exercises the timer-cancel path in the retry select:
	//   select { case <-timer.C; case <-ctx.Done() }
	// The server returns 429 with a long Retry-After, and we cancel the
	// context shortly after the first response so that cancellation races
	// against the timer rather than preventing the initial HTTP round-trip.
	requestReceived := make(chan struct{}, 1)
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		select {
		case requestReceived <- struct{}{}:
		default:
		}
		w.Header().Set("Retry-After", "10")
		w.WriteHeader(http.StatusTooManyRequests)
	}))
	defer srv.Close()

	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
	c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})

	ctx, cancel := context.WithCancel(context.Background())
	defer cancel()

	// Cancel the context after the first request completes, while the
	// client is blocked in the retry timer select.
	go func() {
		<-requestReceived
		// Small delay to ensure we're inside the timer select.
		time.Sleep(50 * time.Millisecond)
		cancel()
	}()

	_, err := c.doGet(ctx, srv.URL+"/test")
	if err == nil {
		t.Fatal("expected error, got nil")
	}
	if !errors.Is(err, context.Canceled) {
		t.Errorf("err = %v, want context.Canceled", err)
	}
}

func TestParseRetryAfter_IntegerSeconds(t *testing.T) {
	c := NewClient("tok", "")
	delay, ok := c.parseRetryAfter("42")
	if !ok {
		t.Fatal("expected ok=true")
	}
	if delay != 42*time.Second {
		t.Errorf("delay = %v, want 42s", delay)
	}
}

func TestParseRetryAfter_ZeroSeconds(t *testing.T) {
	c := NewClient("tok", "")
	delay, ok := c.parseRetryAfter("0")
	if !ok {
		t.Fatal("expected ok=true for zero seconds (RFC 7231 allows immediate retry)")
	}
	if delay != 0 {
		t.Errorf("delay = %v, want 0", delay)
	}
}

func TestParseRetryAfter_NegativeSeconds(t *testing.T) {
	c := NewClient("tok", "")
	_, ok := c.parseRetryAfter("-5")
	if ok {
		t.Error("expected ok=false for negative seconds")
	}
}

func TestParseRetryAfter_HTTPDate_Future(t *testing.T) {
	fixedNow := time.Date(2025, 12, 1, 15, 59, 50, 0, time.UTC)
	c := NewClient("tok", "")
	c.now = func() time.Time { return fixedNow }

	delay, ok := c.parseRetryAfter("Mon, 01 Dec 2025 16:00:00 GMT")
	if !ok {
		t.Fatal("expected ok=true")
	}
	// Should be 10 seconds in the future.
	if delay != 10*time.Second {
		t.Errorf("delay = %v, want 10s", delay)
	}
}

func TestParseRetryAfter_HTTPDate_Past(t *testing.T) {
	fixedNow := time.Date(2025, 12, 1, 17, 0, 0, 0, time.UTC)
	c := NewClient("tok", "")
	c.now = func() time.Time { return fixedNow }

	delay, ok := c.parseRetryAfter("Mon, 01 Dec 2025 16:00:00 GMT")
	if !ok {
		t.Fatal("expected ok=true")
	}
	if delay != 0 {
		t.Errorf("delay = %v, want 0 (past date)", delay)
	}
}

func TestParseRetryAfter_RFC850_Format(t *testing.T) {
	fixedNow := time.Date(2025, 12, 1, 15, 59, 50, 0, time.UTC)
	c := NewClient("tok", "")
	c.now = func() time.Time { return fixedNow }

	// RFC 850 format
	delay, ok := c.parseRetryAfter("Monday, 01-Dec-25 16:00:00 GMT")
	if !ok {
		t.Fatal("expected ok=true for RFC 850 format")
	}
	if delay != 10*time.Second {
		t.Errorf("delay = %v, want 10s", delay)
	}
}

func TestParseRetryAfter_Invalid(t *testing.T) {
	c := NewClient("tok", "")
	_, ok := c.parseRetryAfter("not-valid")
	if ok {
		t.Error("expected ok=false for invalid value")
	}
}

func TestParseRetryAfter_EmptyString(t *testing.T) {
	c := NewClient("tok", "")
	_, ok := c.parseRetryAfter("")
	if ok {
		t.Error("expected ok=false for empty string")
	}
}

func TestParseRetryAfter_MaxCap(t *testing.T) {
	// Verify that parseRetryAfter returns the raw value (capping is done by caller).
	c := NewClient("tok", "")
	delay, ok := c.parseRetryAfter("3600")
	if !ok {
		t.Fatal("expected ok=true")
	}
	if delay != 3600*time.Second {
		t.Errorf("delay = %v, want 3600s (caller is responsible for capping)", delay)
	}
}

func TestAPIError_Error_Truncation(t *testing.T) {
	longBody := make([]byte, 300)
	for i := range longBody {
		longBody[i] = 'x'
	}
	apiErr := &APIError{StatusCode: 500, Body: string(longBody)}
	msg := apiErr.Error()
	if len(msg) > 250 {
		// "HTTP 500: " (10) + 200 + "...(truncated)" (14) = 224
		t.Errorf("error message too long: %d chars", len(msg))
	}
}

func TestAPIError_Error_NewlineSanitized(t *testing.T) {
	apiErr := &APIError{StatusCode: 400, Body: "line1\nline2\rline3"}
	msg := apiErr.Error()
	for _, c := range msg {
		if c == '\n' || c == '\r' {
			t.Errorf("error message contains unsanitized newline: %q", msg)
			break
		}
	}
}

func TestNewClient_HasCheckRedirect(t *testing.T) {
	c := NewClient("secret-token", "https://api.github.com")
	if c.httpClient.CheckRedirect == nil {
		t.Fatal("expected CheckRedirect to be set")
	}
}

func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
	req := &http.Request{
		URL:    &url.URL{Scheme: "http", Host: "api.github.com", Path: "/foo"},
		Header: http.Header{"Authorization": []string{"Bearer token"}},
	}
	err := defaultCheckRedirect(req, []*http.Request{prev})
	if err == nil {
		t.Fatal("expected error on HTTPS->HTTP redirect")
	}
	if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") {
		t.Errorf("unexpected error message: %v", err)
	}
}

func TestDefaultCheckRedirect_RejectsCrossHost(t *testing.T) {
	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
	req := &http.Request{
		URL:    &url.URL{Scheme: "https", Host: "objects.githubusercontent.com", Path: "/bar"},
		Header: http.Header{"Authorization": []string{"Bearer token"}},
	}
	err := defaultCheckRedirect(req, []*http.Request{prev})
	if err == nil {
		t.Fatal("expected error on cross-host redirect")
	}
	if !strings.Contains(err.Error(), "cross-host") {
		t.Errorf("unexpected error message: %v", err)
	}
}

func TestDefaultCheckRedirect_AllowsSameHost(t *testing.T) {
	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
	req := &http.Request{
		URL:    &url.URL{Scheme: "https", Host: "api.github.com", Path: "/bar"},
		Header: http.Header{"Authorization": []string{"Bearer token"}},
	}
	err := defaultCheckRedirect(req, []*http.Request{prev})
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
	// Auth should be preserved on same-host redirect
	if auth := req.Header.Get("Authorization"); auth != "Bearer token" {
		t.Errorf("expected Authorization to be preserved, got %q", auth)
	}
}

func TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP(t *testing.T) {
	prev := &http.Request{URL: &url.URL{Scheme: "http", Host: "localhost:8080", Path: "/foo"}}
	req := &http.Request{
		URL:    &url.URL{Scheme: "http", Host: "localhost:8080", Path: "/bar"},
		Header: http.Header{},
	}
	err := defaultCheckRedirect(req, []*http.Request{prev})
	if err != nil {
		t.Fatalf("unexpected error: %v", err)
	}
}

func TestDefaultCheckRedirect_RejectsTooManyRedirects(t *testing.T) {
	via := make([]*http.Request, 10)
	for i := range via {
		via[i] = &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/"}}
	}
	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/final"}}
	err := defaultCheckRedirect(req, via)
	if err == nil {
		t.Fatal("expected error after 10 redirects")
	}
	if !strings.Contains(err.Error(), "10 redirects") {
		t.Errorf("unexpected error message: %v", err)
	}
}

func TestDefaultCheckRedirect_EmptyViaAllowed(t *testing.T) {
	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
	err := defaultCheckRedirect(req, nil)
	if err != nil {
		t.Fatalf("unexpected error with empty via: %v", err)
	}
}

func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
	c := NewClient("token", "https://api.github.com")
	c.SetHTTPClient(nil)
	if c.httpClient == nil {
		t.Fatal("expected non-nil httpClient after SetHTTPClient(nil)")
	}
	if c.httpClient.Timeout != 30*time.Second {
		t.Errorf("expected 30s timeout, got %v", c.httpClient.Timeout)
	}
	if c.httpClient.CheckRedirect == nil {
		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)
	}
}