Skip to content

Commit 1fbfef7

Browse files
clcollinsclaude
andcommitted
fix: address PR #429 review findings and security hardening
F1: Gate approvals/watcher/input keys to table view only by restructuring keyMsgHandler — move per-mode dispatch above approvals routing so A/W/: keys don't fire in incident/log/docs views or under the approvals overlay. F2: Fix setext heading corruption by rendering watcher buffer entries individually through glamour then joining with Unicode separator (───) instead of rendering the full buffer joined with \n---\n. F3: Remove agent result double-rendering — store raw text in the watcher buffer and let updateWatcherViewport handle the single glamour render pass. F4: Sanitize IncidentTitle with stripControl() in buildAskFromVerdict to prevent terminal injection from attacker-influenced PD data. F5: Clamp approvals pane content to WatcherHeight lines so long approval bodies don't overflow the terminal. F6: Cache the glamour TermRenderer on the model keyed by viewport width, avoiding per-token renderer recreation during streaming. SEC-001: Apply url.PathEscape to clusterID and reportID in backplane client URL construction to prevent path traversal. SEC-002: Reject non-localhost HTTP endpoints when an API key is set in the OpenAI-compatible provider to prevent credential leakage over plaintext. SEC-003: Add .gitignore patterns for .env, *.pem, *.key, credentials, and secrets files. SEC-004b: Sanitize all PagerDuty data (incident titles, service names, note content, alert fields, team/assignee summaries) with stripControl() at the summarize boundary and table row construction to prevent terminal injection. Also removes dead code: prefixLines function (replaced by prefixMessage), duplicate input.Focused() case in key handler switch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent aca6683 commit 1fbfef7

14 files changed

Lines changed: 735 additions & 162 deletions

.gitignore

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,11 @@ coverage.out
1616

1717
# Claude Code local settings (personal permissions, not committed)
1818
.claude/settings.local.json
19+
20+
# Sensitive files
21+
.env
22+
.env.*
23+
*.pem
24+
*.key
25+
credentials.*
26+
secrets.*

pkg/ai/http_safety_test.go

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
package ai
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
func TestNewOpenAICompatProvider_RejectsHTTPWithAPIKey(t *testing.T) {
11+
_, err := newOpenAICompatProvider(Config{
12+
Endpoint: "http://remote-server.example.com:8080",
13+
Model: "gpt-4",
14+
}, "sk-secret-key")
15+
16+
require.Error(t, err,
17+
"must reject non-localhost http:// endpoint when API key is set")
18+
assert.Contains(t, err.Error(), "HTTPS",
19+
"error message should mention HTTPS requirement")
20+
}
21+
22+
func TestNewOpenAICompatProvider_AllowsHTTPLocalhost(t *testing.T) {
23+
tests := []struct {
24+
name string
25+
endpoint string
26+
}{
27+
{"localhost", "http://localhost:8080"},
28+
{"127.0.0.1", "http://127.0.0.1:11434"},
29+
{"[::1]", "http://[::1]:8080"},
30+
}
31+
32+
for _, tt := range tests {
33+
t.Run(tt.name, func(t *testing.T) {
34+
provider, err := newOpenAICompatProvider(Config{
35+
Endpoint: tt.endpoint,
36+
Model: "m",
37+
}, "sk-secret-key")
38+
39+
assert.NoError(t, err,
40+
"localhost http:// with API key should be allowed")
41+
assert.NotNil(t, provider)
42+
})
43+
}
44+
}
45+
46+
func TestNewOpenAICompatProvider_AllowsHTTPSWithAPIKey(t *testing.T) {
47+
provider, err := newOpenAICompatProvider(Config{
48+
Endpoint: "https://api.openai.com",
49+
Model: "gpt-4",
50+
}, "sk-secret-key")
51+
52+
assert.NoError(t, err,
53+
"https:// endpoint with API key should be allowed")
54+
assert.NotNil(t, provider)
55+
}
56+
57+
func TestNewOpenAICompatProvider_AllowsHTTPWithoutAPIKey(t *testing.T) {
58+
provider, err := newOpenAICompatProvider(Config{
59+
Endpoint: "http://remote-server.example.com:8080",
60+
Model: "llama",
61+
}, "")
62+
63+
assert.NoError(t, err,
64+
"http:// without API key should be allowed (e.g., ollama)")
65+
assert.NotNil(t, provider)
66+
}

pkg/ai/openai_compat.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,27 @@ import (
77
"encoding/json"
88
"fmt"
99
"net/http"
10+
"net/url"
1011
"strings"
1112
"time"
1213

1314
"github.com/charmbracelet/log"
1415
)
1516

17+
func validateEndpointSecurity(endpoint string) error {
18+
parsed, err := url.Parse(endpoint)
19+
if err != nil {
20+
return fmt.Errorf("openai: invalid endpoint URL: %w", err)
21+
}
22+
if parsed.Scheme == "http" {
23+
host := parsed.Hostname()
24+
if host != "localhost" && host != "127.0.0.1" && host != "::1" {
25+
return fmt.Errorf("openai: refusing to send API key over HTTP to non-localhost host %q; use HTTPS for remote endpoints", host)
26+
}
27+
}
28+
return nil
29+
}
30+
1631
type openaiCompatProvider struct {
1732
endpoint string
1833
model string
@@ -26,6 +41,12 @@ func newOpenAICompatProvider(cfg Config, apiKey string) (*openaiCompatProvider,
2641
return nil, fmt.Errorf("openai: endpoint is required")
2742
}
2843

44+
if apiKey != "" {
45+
if err := validateEndpointSecurity(cfg.Endpoint); err != nil {
46+
return nil, err
47+
}
48+
}
49+
2950
return &openaiCompatProvider{
3051
endpoint: strings.TrimRight(cfg.Endpoint, "/"),
3152
model: cfg.Model,

pkg/backplane/client.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func NewClient(cfg *Config, tokenFunc func() (string, error)) BackplaneClient {
6060
}
6161

6262
func (c *Client) ListReports(ctx context.Context, clusterID string) ([]ReportSummary, error) {
63-
endpoint := fmt.Sprintf("%s/backplane/cluster/%s/reports?last=10", c.config.URL, clusterID)
63+
endpoint := fmt.Sprintf("%s/backplane/cluster/%s/reports?last=10", c.config.URL, url.PathEscape(clusterID))
6464
log.Debug("backplane.ListReports", "cluster_id", clusterID)
6565

6666
body, err := c.doRequest(ctx, endpoint)
@@ -80,7 +80,7 @@ func (c *Client) ListReports(ctx context.Context, clusterID string) ([]ReportSum
8080
}
8181

8282
func (c *Client) GetReport(ctx context.Context, clusterID, reportID string) (*Report, error) {
83-
endpoint := fmt.Sprintf("%s/backplane/cluster/%s/reports/%s", c.config.URL, clusterID, reportID)
83+
endpoint := fmt.Sprintf("%s/backplane/cluster/%s/reports/%s", c.config.URL, url.PathEscape(clusterID), url.PathEscape(reportID))
8484
log.Debug("backplane.GetReport", "cluster_id", clusterID, "report_id", reportID)
8585

8686
body, err := c.doRequest(ctx, endpoint)

pkg/backplane/path_escape_test.go

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
package backplane
2+
3+
import (
4+
"context"
5+
"encoding/json"
6+
"net/http"
7+
"net/http/httptest"
8+
"testing"
9+
10+
"github.com/stretchr/testify/assert"
11+
"github.com/stretchr/testify/require"
12+
)
13+
14+
func TestClient_ListReports_PathEscapesClusterID(t *testing.T) {
15+
var receivedPath string
16+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
17+
receivedPath = r.URL.EscapedPath()
18+
w.Header().Set("Content-Type", "application/json")
19+
_ = json.NewEncoder(w).Encode(listReportsResponse{Reports: []ReportSummary{}})
20+
}))
21+
defer server.Close()
22+
23+
cfg := &Config{URL: server.URL}
24+
client := NewClient(cfg, func() (string, error) { return "test-token", nil })
25+
26+
_, err := client.ListReports(context.Background(), "../../admin/endpoint")
27+
require.NoError(t, err)
28+
29+
assert.Equal(t, "/backplane/cluster/..%2F..%2Fadmin%2Fendpoint/reports", receivedPath,
30+
"clusterID with path traversal must be URL-escaped in the request path")
31+
}
32+
33+
func TestClient_GetReport_PathEscapesClusterIDAndReportID(t *testing.T) {
34+
var receivedPath string
35+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
36+
receivedPath = r.URL.EscapedPath()
37+
w.Header().Set("Content-Type", "application/json")
38+
_ = json.NewEncoder(w).Encode(Report{ReportID: "rpt-1"})
39+
}))
40+
defer server.Close()
41+
42+
cfg := &Config{URL: server.URL}
43+
client := NewClient(cfg, func() (string, error) { return "test-token", nil })
44+
45+
_, err := client.GetReport(context.Background(), "../admin", "../../secret")
46+
require.NoError(t, err)
47+
48+
assert.Equal(t, "/backplane/cluster/..%2Fadmin/reports/..%2F..%2Fsecret", receivedPath,
49+
"both clusterID and reportID with path traversal must be URL-escaped")
50+
}
51+
52+
func TestClient_ListReports_NormalClusterIDUnchanged(t *testing.T) {
53+
var receivedPath string
54+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
55+
receivedPath = r.URL.Path
56+
w.Header().Set("Content-Type", "application/json")
57+
_ = json.NewEncoder(w).Encode(listReportsResponse{Reports: []ReportSummary{}})
58+
}))
59+
defer server.Close()
60+
61+
cfg := &Config{URL: server.URL}
62+
client := NewClient(cfg, func() (string, error) { return "test-token", nil })
63+
64+
_, err := client.ListReports(context.Background(), "abc-123-def")
65+
require.NoError(t, err)
66+
67+
assert.Equal(t, "/backplane/cluster/abc-123-def/reports", receivedPath,
68+
"normal clusterID should pass through unchanged")
69+
}

pkg/tui/claude.go

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -279,15 +279,7 @@ func (m model) handleAgentSessionEvent(msg agentSessionEventMsg) (tea.Model, tea
279279
return errMsg{fmt.Errorf("agent error: %s", ev.Text)}
280280
}
281281
}
282-
// Render final result through glamour if available
283-
if ev.Text != "" && m.markdownRenderer != nil {
284-
rendered, err := m.markdownRenderer.Render(ev.Text)
285-
if err == nil {
286-
m.watcherBuffer.SetLast(prefixMessage(m.agentMarker, strings.TrimSpace(rendered)))
287-
} else {
288-
m.watcherBuffer.SetLast(prefixMessage(m.agentMarker, ev.Text))
289-
}
290-
} else if ev.Text != "" {
282+
if ev.Text != "" {
291283
m.watcherBuffer.SetLast(prefixMessage(m.agentMarker, ev.Text))
292284
}
293285
m.updateWatcherViewport()

0 commit comments

Comments
 (0)