Skip to content

Commit fa64a4f

Browse files
committed
fix(copilot-review): address all 10 Copilot PR comments
- nexus-cli: use X-API-Key header (was Authorization: Bearer) - nexus-cli: add 30s timeout to shared http.Client - nexus-cli: diff-based update detection via providerDrifted() — skip no-op UPDATEs and print '= OK' for providers with no changes - nexus-cli/go.mod: align go directive to 1.21 (matches CI) - .github/workflows/nexus-cli.yml: guard plan step for forked PRs where secrets are unavailable (no-op with info message instead of failing) - audit/service.go: validate extracted IP with net.ParseIP before storing to prevent INSERT failures on arbitrary X-Forwarded-For values - handlers/providers.go: log audit errors with log.Printf instead of discarding with _ = - handlers/callback.go: log audit errors in logAuditEvent wrapper - handlers/audit_test.go: add unit tests for AuditHandler.List covering no filters, event_type filter, invalid since param (400), custom limit, and out-of-range limit clamping - docs/reference/audit-log.md: correct endpoint path to /audit (no /v1 prefix) with note explaining the Broker is currently unversioned
1 parent 1fef9ee commit fa64a4f

8 files changed

Lines changed: 258 additions & 39 deletions

File tree

.github/workflows/nexus-cli.yml

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,18 @@ jobs:
3333
run: go build -o nexus-cli
3434

3535
- name: Plan (Pull Request)
36-
if: github.event_name == 'pull_request'
36+
# Only run plan when BROKER_BASE_URL is set (not available for forked PRs).
37+
# This prevents the job from failing with "connection refused" for external contributors.
38+
if: github.event_name == 'pull_request' && secrets.BROKER_BASE_URL != ''
3739
env:
3840
BROKER_BASE_URL: ${{ secrets.BROKER_BASE_URL }}
3941
API_KEY: ${{ secrets.BROKER_API_KEY }}
4042
run: ./nexus-cli plan
4143

44+
- name: Plan skipped (no Broker secret)
45+
if: github.event_name == 'pull_request' && secrets.BROKER_BASE_URL == ''
46+
run: echo "ℹ️ Skipping live plan — BROKER_BASE_URL secret not available (forked PR)."
47+
4248
- name: Apply (Push to Main)
4349
if: github.event_name == 'push' && github.ref == 'refs/heads/main'
4450
env:

docs/reference/audit-log.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,13 @@ This provides a queryable history of who changed what and when, which is essenti
2121
## Query the Audit Log
2222

2323
```
24-
GET /v1/audit
24+
GET /audit
2525
```
2626

2727
Returns recent audit events in descending chronological order. This endpoint is protected by `ApiKeyMiddleware`.
2828

29+
> **Note:** The Nexus Broker API is unversioned — all routes are mounted at the root (e.g., `/providers`, `/audit`). The `/v1/audit` path referenced elsewhere is aspirational and will apply if/when the Broker adopts a versioned API prefix.
30+
2931
### Query Parameters
3032

3133
| Parameter | Type | Description |

nexus-broker/internal/audit/service.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,16 +23,16 @@ func (s *Service) Log(eventType string, connectionID *uuid.UUID, data map[string
2323
var userAgent *string
2424

2525
if r != nil {
26-
// Extract IP
26+
// Extract IP — validate with net.ParseIP to avoid storing arbitrary text in the inet column.
2727
if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" {
2828
ip := strings.TrimSpace(strings.Split(fwd, ",")[0])
29-
ipVal = &ip
29+
if net.ParseIP(ip) != nil {
30+
ipVal = &ip
31+
}
3032
} else {
3133
host, _, err := net.SplitHostPort(r.RemoteAddr)
32-
if err == nil {
34+
if err == nil && net.ParseIP(host) != nil {
3335
ipVal = &host
34-
} else {
35-
ipVal = &r.RemoteAddr
3636
}
3737
}
3838

Lines changed: 174 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
package handlers_test
2+
3+
import (
4+
"encoding/json"
5+
"net/http"
6+
"net/http/httptest"
7+
"testing"
8+
"time"
9+
10+
"github.com/Prescott-Data/nexus-framework/nexus-broker/pkg/handlers"
11+
"github.com/google/uuid"
12+
"github.com/jmoiron/sqlx"
13+
sqlmock "gopkg.in/DATA-DOG/go-sqlmock.v1"
14+
)
15+
16+
// newSqlxDB wraps a sqlmock connection in a sqlx.DB so handlers can use it.
17+
func newSqlxDB(t *testing.T) (*sqlx.DB, sqlmock.Sqlmock) {
18+
t.Helper()
19+
db, mock, err := sqlmock.New()
20+
if err != nil {
21+
t.Fatalf("failed to create sqlmock: %v", err)
22+
}
23+
return sqlx.NewDb(db, "postgres"), mock
24+
}
25+
26+
func TestAuditList_NoFilters_ReturnsDefaultLimit(t *testing.T) {
27+
db, mock := newSqlxDB(t)
28+
defer db.Close()
29+
30+
id := uuid.New()
31+
connID := uuid.New()
32+
now := time.Now()
33+
34+
rows := sqlmock.NewRows([]string{
35+
"id", "connection_id", "event_type", "event_data", "ip_address", "user_agent", "created_at",
36+
}).AddRow(
37+
id, &connID, "provider.created", `{"name":"google"}`, "127.0.0.1", "curl/7.88", now,
38+
)
39+
40+
mock.ExpectQuery(`SELECT id, connection_id, event_type, event_data, ip_address, user_agent, created_at`).
41+
WithArgs(50). // default limit
42+
WillReturnRows(rows)
43+
44+
handler := handlers.NewAuditHandler(db)
45+
req := httptest.NewRequest(http.MethodGet, "/audit", nil)
46+
w := httptest.NewRecorder()
47+
48+
handler.List(w, req)
49+
50+
if w.Code != http.StatusOK {
51+
t.Fatalf("expected 200, got %d", w.Code)
52+
}
53+
54+
var result []map[string]interface{}
55+
if err := json.NewDecoder(w.Body).Decode(&result); err != nil {
56+
t.Fatalf("failed to decode response: %v", err)
57+
}
58+
if len(result) != 1 {
59+
t.Fatalf("expected 1 event, got %d", len(result))
60+
}
61+
if result[0]["event_type"] != "provider.created" {
62+
t.Errorf("expected event_type provider.created, got %v", result[0]["event_type"])
63+
}
64+
65+
if err := mock.ExpectationsWereMet(); err != nil {
66+
t.Errorf("unmet sqlmock expectations: %v", err)
67+
}
68+
}
69+
70+
func TestAuditList_EventTypeFilter(t *testing.T) {
71+
db, mock := newSqlxDB(t)
72+
defer db.Close()
73+
74+
rows := sqlmock.NewRows([]string{
75+
"id", "connection_id", "event_type", "event_data", "ip_address", "user_agent", "created_at",
76+
}) // empty result
77+
78+
mock.ExpectQuery(`SELECT id, connection_id, event_type, event_data, ip_address, user_agent, created_at`).
79+
WithArgs("provider.deleted", 50).
80+
WillReturnRows(rows)
81+
82+
handler := handlers.NewAuditHandler(db)
83+
req := httptest.NewRequest(http.MethodGet, "/audit?event_type=provider.deleted", nil)
84+
w := httptest.NewRecorder()
85+
86+
handler.List(w, req)
87+
88+
if w.Code != http.StatusOK {
89+
t.Fatalf("expected 200, got %d", w.Code)
90+
}
91+
92+
// Empty result should return [] not null
93+
var result []map[string]interface{}
94+
if err := json.NewDecoder(w.Body).Decode(&result); err != nil {
95+
t.Fatalf("failed to decode response: %v", err)
96+
}
97+
if result == nil {
98+
t.Error("expected empty array, got null")
99+
}
100+
101+
if err := mock.ExpectationsWereMet(); err != nil {
102+
t.Errorf("unmet sqlmock expectations: %v", err)
103+
}
104+
}
105+
106+
func TestAuditList_InvalidSinceParam_Returns400(t *testing.T) {
107+
db, _ := newSqlxDB(t)
108+
defer db.Close()
109+
110+
handler := handlers.NewAuditHandler(db)
111+
req := httptest.NewRequest(http.MethodGet, "/audit?since=not-a-date", nil)
112+
w := httptest.NewRecorder()
113+
114+
handler.List(w, req)
115+
116+
if w.Code != http.StatusBadRequest {
117+
t.Errorf("expected 400 for invalid since param, got %d", w.Code)
118+
}
119+
}
120+
121+
func TestAuditList_CustomLimit(t *testing.T) {
122+
db, mock := newSqlxDB(t)
123+
defer db.Close()
124+
125+
rows := sqlmock.NewRows([]string{
126+
"id", "connection_id", "event_type", "event_data", "ip_address", "user_agent", "created_at",
127+
})
128+
129+
mock.ExpectQuery(`SELECT id, connection_id, event_type, event_data, ip_address, user_agent, created_at`).
130+
WithArgs(10).
131+
WillReturnRows(rows)
132+
133+
handler := handlers.NewAuditHandler(db)
134+
req := httptest.NewRequest(http.MethodGet, "/audit?limit=10", nil)
135+
w := httptest.NewRecorder()
136+
137+
handler.List(w, req)
138+
139+
if w.Code != http.StatusOK {
140+
t.Fatalf("expected 200, got %d", w.Code)
141+
}
142+
143+
if err := mock.ExpectationsWereMet(); err != nil {
144+
t.Errorf("unmet sqlmock expectations: %v", err)
145+
}
146+
}
147+
148+
func TestAuditList_LimitAboveMax_ClampedTo1000(t *testing.T) {
149+
db, mock := newSqlxDB(t)
150+
defer db.Close()
151+
152+
rows := sqlmock.NewRows([]string{
153+
"id", "connection_id", "event_type", "event_data", "ip_address", "user_agent", "created_at",
154+
})
155+
156+
// limit=9999 should be clamped to 50 (default, since > 1000 is rejected)
157+
mock.ExpectQuery(`SELECT id, connection_id, event_type, event_data, ip_address, user_agent, created_at`).
158+
WithArgs(50).
159+
WillReturnRows(rows)
160+
161+
handler := handlers.NewAuditHandler(db)
162+
req := httptest.NewRequest(http.MethodGet, "/audit?limit=9999", nil)
163+
w := httptest.NewRecorder()
164+
165+
handler.List(w, req)
166+
167+
if w.Code != http.StatusOK {
168+
t.Fatalf("expected 200, got %d", w.Code)
169+
}
170+
171+
if err := mock.ExpectationsWereMet(); err != nil {
172+
t.Errorf("unmet sqlmock expectations: %v", err)
173+
}
174+
}

nexus-broker/pkg/handlers/callback.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"encoding/json"
77
"fmt"
88
"io"
9+
"log"
910
"net/http"
1011
"net/url"
1112
"strings"
@@ -844,7 +845,9 @@ func (h *CallbackHandler) logAuditEvent(connectionID *uuid.UUID, eventType strin
844845
auditData[k] = v
845846
}
846847

847-
_ = h.audit.Log(eventType, connectionID, auditData, r)
848+
if err := h.audit.Log(eventType, connectionID, auditData, r); err != nil {
849+
log.Printf("audit: failed to log %s (connection_id=%v): %v", eventType, connectionID, err)
850+
}
848851
}
849852

850853
// handleError handles OAuth errors

nexus-broker/pkg/handlers/providers.go

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package handlers
33
import (
44
"encoding/json"
55
"fmt"
6+
"log"
67
"net/http"
78
"strings"
89

@@ -64,7 +65,9 @@ func (h *ProvidersHandler) Update(w http.ResponseWriter, r *http.Request) {
6465
}
6566

6667
if h.audit != nil {
67-
_ = h.audit.Log("provider.updated", nil, map[string]interface{}{"provider_id": profile.ID, "name": profile.Name}, r)
68+
if err := h.audit.Log("provider.updated", nil, map[string]interface{}{"provider_id": profile.ID, "name": profile.Name}, r); err != nil {
69+
log.Printf("audit: failed to log provider.updated for provider_id=%s: %v", profile.ID, err)
70+
}
6871
}
6972

7073
w.WriteHeader(http.StatusOK)
@@ -91,7 +94,9 @@ func (h *ProvidersHandler) Patch(w http.ResponseWriter, r *http.Request) {
9194
}
9295

9396
if h.audit != nil {
94-
_ = h.audit.Log("provider.updated", nil, map[string]interface{}{"provider_id": id.String(), "updates": updates}, r)
97+
if err := h.audit.Log("provider.updated", nil, map[string]interface{}{"provider_id": id.String(), "updates": updates}, r); err != nil {
98+
log.Printf("audit: failed to log provider.updated for provider_id=%s: %v", id, err)
99+
}
95100
}
96101

97102
w.WriteHeader(http.StatusOK)
@@ -112,7 +117,9 @@ func (h *ProvidersHandler) Delete(w http.ResponseWriter, r *http.Request) {
112117
}
113118

114119
if h.audit != nil {
115-
_ = h.audit.Log("provider.deleted", nil, map[string]interface{}{"provider_id": id.String()}, r)
120+
if err := h.audit.Log("provider.deleted", nil, map[string]interface{}{"provider_id": id.String()}, r); err != nil {
121+
log.Printf("audit: failed to log provider.deleted for provider_id=%s: %v", id, err)
122+
}
116123
}
117124

118125
w.WriteHeader(http.StatusOK)
@@ -165,7 +172,9 @@ func (h *ProvidersHandler) Register(w http.ResponseWriter, r *http.Request) {
165172
}
166173

167174
if h.audit != nil {
168-
_ = h.audit.Log("provider.created", nil, map[string]interface{}{"provider_id": profile.ID, "name": profile.Name}, r)
175+
if err := h.audit.Log("provider.created", nil, map[string]interface{}{"provider_id": profile.ID, "name": profile.Name}, r); err != nil {
176+
log.Printf("audit: failed to log provider.created for provider_id=%s: %v", profile.ID, err)
177+
}
169178
}
170179

171180
httputil.WriteJSON(w, http.StatusCreated, map[string]interface{}{
@@ -224,7 +233,9 @@ func (h *ProvidersHandler) DeleteByName(w http.ResponseWriter, r *http.Request)
224233
}
225234

226235
if h.audit != nil {
227-
_ = h.audit.Log("provider.deleted", nil, map[string]interface{}{"provider_name": name, "rows_affected": rowsAffected}, r)
236+
if err := h.audit.Log("provider.deleted", nil, map[string]interface{}{"provider_name": name, "rows_affected": rowsAffected}, r); err != nil {
237+
log.Printf("audit: failed to log provider.deleted for provider_name=%s: %v", name, err)
238+
}
228239
}
229240

230241
httputil.WriteJSON(w, http.StatusOK, map[string]string{"message": fmt.Sprintf("Deleted %d provider(s)", rowsAffected)})

nexus-cli/go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
module github.com/Prescott-Data/nexus-framework/nexus-cli
22

3-
go 1.26.2
3+
go 1.21
44

55
require gopkg.in/yaml.v3 v3.0.1 // indirect

0 commit comments

Comments
 (0)