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

address review feedback: rename to With* convention, extract env const, redact query params, fix misleading test
PR Ready Gate / clear-labels (pull_request) Successful in 2s
Details
CI / test (pull_request) Successful in 17s
Details
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s
Details
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m22s
Details
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m0s
Details

Browse Source
This commit is contained in:
claw
2026-05-13 11:09:57 -07:00
parent 91f31ff2d7
commit 06b92a6834
3 changed files with 62 additions and 33 deletions
Download Patch File Download Diff File Expand all files Collapse all files
github/client_test.go
+41 -20
View File
@@ -35,7 +35,7 @@ func TestDoRequest_Success(t *testing.T) {
}))
defer srv.Close()
c := NewClient("test-token", srv.URL, AllowInsecureHTTPForTest())
c := NewClient("test-token", srv.URL, WithAllowInsecureHTTPForTest())
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, AllowInsecureHTTPForTest())
c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
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, AllowInsecureHTTPForTest())
c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
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, AllowInsecureHTTPForTest())
c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
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, AllowInsecureHTTPForTest())
c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
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, AllowInsecureHTTPForTest())
c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
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, AllowInsecureHTTPForTest())
c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
_, 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, AllowInsecureHTTPForTest())
c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
_, 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, AllowInsecureHTTPForTest())
c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
ctx, cancel := context.WithCancel(context.Background())
@@ -519,7 +519,7 @@ func TestDoRequest_RejectsHTTPWithToken(t *testing.T) {
}))
defer srv.Close()
// Without AllowInsecureHTTP, should refuse to send token over HTTP
// Without WithAllowInsecureHTTP, should refuse to send token over HTTP
c := NewClient("secret-token", srv.URL)
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
@@ -530,6 +530,27 @@ func TestDoRequest_RejectsHTTPWithToken(t *testing.T) {
}
}
func TestDoRequest_RejectsHTTPWithToken_RedactsQueryParams(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte("{}"))
}))
defer srv.Close()
c := NewClient("secret-token", srv.URL)
_, err := c.doGet(context.Background(), srv.URL+"/test?secret=hunter2&token=abc")
if err == nil {
t.Fatal("expected error when sending token over HTTP")
}
errMsg := err.Error()
if strings.Contains(errMsg, "hunter2") || strings.Contains(errMsg, "token=abc") {
t.Errorf("error message should not contain query params, got: %v", errMsg)
}
if !strings.Contains(errMsg, "?[REDACTED]") {
t.Errorf("error message should contain redacted marker, got: %v", errMsg)
}
}
func TestDoRequest_AllowsHTTPWithoutToken(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
@@ -538,7 +559,7 @@ func TestDoRequest_AllowsHTTPWithoutToken(t *testing.T) {
defer srv.Close()
// Without token, HTTP should be fine (no credentials to leak)
c := NewClient("", srv.URL, AllowInsecureHTTPForTest())
c := NewClient("", srv.URL)
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
@@ -555,7 +576,7 @@ func TestDoRequest_AllowsHTTPWithInsecureTestOption(t *testing.T) {
}))
defer srv.Close()
c := NewClient("secret-token", srv.URL, AllowInsecureHTTPForTest())
c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTPForTest())
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
@@ -565,7 +586,7 @@ func TestDoRequest_AllowsHTTPWithInsecureTestOption(t *testing.T) {
}
}
func TestAllowInsecureHTTP_RequiresEnvVar(t *testing.T) {
func TestWithAllowInsecureHTTP_RequiresEnvVar(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`{"ok":true}`))
@@ -574,17 +595,17 @@ func TestAllowInsecureHTTP_RequiresEnvVar(t *testing.T) {
// Without the env var, AllowInsecureHTTP should be ignored
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "")
c := NewClient("secret-token", srv.URL, AllowInsecureHTTP())
c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error: AllowInsecureHTTP without env var should be rejected")
t.Fatal("expected error: WithAllowInsecureHTTP without env var should be rejected")
}
if !strings.Contains(err.Error(), "refusing to send credentials") {
t.Errorf("unexpected error: %v", err)
}
}
func TestAllowInsecureHTTP_WithEnvVar(t *testing.T) {
func TestWithAllowInsecureHTTP_WithEnvVar(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`{"ok":true}`))
@@ -592,7 +613,7 @@ func TestAllowInsecureHTTP_WithEnvVar(t *testing.T) {
defer srv.Close()
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1")
c := NewClient("secret-token", srv.URL, AllowInsecureHTTP())
c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP())
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
@@ -602,7 +623,7 @@ func TestAllowInsecureHTTP_WithEnvVar(t *testing.T) {
}
}
func TestAllowInsecureHTTP_EnvVarMustBeExactlyOne(t *testing.T) {
func TestWithAllowInsecureHTTP_EnvVarMustBeExactlyOne(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`{"ok":true}`))
@@ -611,9 +632,9 @@ func TestAllowInsecureHTTP_EnvVarMustBeExactlyOne(t *testing.T) {
// "true" is not "1" — should be rejected
t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true")
c := NewClient("secret-token", srv.URL, AllowInsecureHTTP())
c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error: REVIEW_BOT_ALLOW_INSECURE=true should not satisfy gate")
t.Fatal("expected error: env var must be exactly \"1\" to satisfy gate")
}
}