Skip to content

Commit 99b25f0

Browse files
committed
fix: bound update-check HTTP request with Timeout and body limit
internal/update/update.go issued a GET against api.github.com through a bare http.Client{} - no Timeout, no body cap, no NewRequestWithContext cancellation. Two concrete problems: 1. A slow or hijacked response would hang the binary on launch forever, because the call site uses context.Background() so the caller cannot cancel the request. 2. A malicious or compromised api.github.com response could exhaust memory; io.ReadAll on resp.Body has no upper bound. This change adds a 10s http.Client.Timeout, wraps the body read in io.LimitReader with a 1 MiB cap, and adds two regression tests. Touches: - internal/update/update.go - internal/update/update_test.go Verification: go vet, staticcheck, gofumpt, go test -short all clean.
1 parent 61007bd commit 99b25f0

2 files changed

Lines changed: 78 additions & 3 deletions

File tree

internal/update/update.go

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,20 @@ import (
88
"net/http"
99
"runtime"
1010
"strings"
11+
"time"
1112
)
1213

14+
// checkTimeout bounds the time a single update-check HTTP request can take.
15+
// The package's public entry point runs the request through context.Background
16+
// (so it cannot be cancelled by the caller), so without an http.Client
17+
// Timeout a slow or hijacked response could hang the binary on launch
18+
// forever. 10s is generous for a single GET against api.github.com.
19+
const checkTimeout = 10 * time.Second
20+
21+
// maxResponseBytes caps the body read so a malicious or compromised
22+
// api.github.com response cannot exhaust memory.
23+
const maxResponseBytes = 1 << 20 // 1 MiB
24+
1325
var updateURL = "https://api.github.com/repos/GrayCodeAI/hawk/releases/latest"
1426

1527
func setUpdateURL(url string) {
@@ -26,14 +38,14 @@ type ReleaseInfo struct {
2638

2739
// Check checks for available updates.
2840
func Check(currentVersion string) (*ReleaseInfo, error) {
29-
req, err := http.NewRequestWithContext(context.Background(), "GET", updateURL, nil)
41+
req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, updateURL, nil)
3042
if err != nil {
3143
return nil, err
3244
}
3345
req.Header.Set("Accept", "application/vnd.github.v3+json")
3446
req.Header.Set("User-Agent", "hawk-cli")
3547

36-
client := &http.Client{}
48+
client := &http.Client{Timeout: checkTimeout}
3749
resp, err := client.Do(req)
3850
if err != nil {
3951
return nil, err
@@ -44,7 +56,7 @@ func Check(currentVersion string) (*ReleaseInfo, error) {
4456
return nil, fmt.Errorf("update check failed: %s", resp.Status)
4557
}
4658

47-
body, err := io.ReadAll(resp.Body)
59+
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))
4860
if err != nil {
4961
return nil, err
5062
}

internal/update/update_test.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"net/http"
66
"strings"
77
"testing"
8+
"time"
89

910
"github.com/GrayCodeAI/hawk/internal/testutil"
1011
)
@@ -154,6 +155,68 @@ func TestCheck(t *testing.T) {
154155
t.Error("Check() should return error for unreachable server")
155156
}
156157
})
158+
159+
t.Run("slow server is bounded by client timeout", func(t *testing.T) {
160+
// Server that takes longer than checkTimeout to start writing
161+
// the response. The client.Timeout should abort the request
162+
// well before the server would have responded.
163+
server := testutil.NewLoopbackHTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
164+
time.Sleep(15 * time.Second)
165+
}))
166+
// server.Close() blocks on the still-active connection; kick
167+
// the close off in a goroutine and bound the wait so the test
168+
// finishes promptly once the assertion has run.
169+
t.Cleanup(func() {
170+
done := make(chan struct{})
171+
go func() { server.Close(); close(done) }()
172+
select {
173+
case <-done:
174+
case <-time.After(2 * time.Second):
175+
// best-effort: the test has already passed; the
176+
// runner's process cleanup will finish the job
177+
}
178+
})
179+
180+
origURL := updateURL
181+
setUpdateURL(server.URL)
182+
t.Cleanup(func() { setUpdateURL(origURL) })
183+
184+
start := time.Now()
185+
_, err := Check("0.1.0")
186+
elapsed := time.Since(start)
187+
if err == nil {
188+
t.Fatal("Check() should error when the server stalls")
189+
}
190+
// checkTimeout is 10s; allow a couple seconds of slack for
191+
// goroutine scheduling on slow CI but require the timeout
192+
// to have fired (the server sleeps 15s).
193+
if elapsed > 12*time.Second {
194+
t.Errorf("Check() took %v; expected to be bounded by checkTimeout (10s)", elapsed)
195+
}
196+
})
197+
198+
t.Run("oversize response is bounded", func(t *testing.T) {
199+
// Server returns > 1 MiB; the LimitReader in Check caps the
200+
// body read so JSON unmarshal of a normal release still
201+
// succeeds (release fields are populated from the prefix).
202+
server := testutil.NewLoopbackHTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
203+
w.Header().Set("Content-Type", "application/json")
204+
_, _ = w.Write([]byte(`{"tag_name":"v9.9.9","name":"x","body":"`))
205+
pad := strings.Repeat("a", 2<<20)
206+
_, _ = w.Write([]byte(pad))
207+
_, _ = w.Write([]byte(`","html_url":"https://example"}`))
208+
}))
209+
defer server.Close()
210+
211+
origURL := updateURL
212+
setUpdateURL(server.URL)
213+
defer setUpdateURL(origURL)
214+
215+
// We don't care whether Check parses the truncated body — only
216+
// that it returns *without* exhausting memory. The 1 MiB cap
217+
// guarantees allocation stays bounded.
218+
_, _ = Check("0.1.0")
219+
})
157220
}
158221

159222
func TestSummary(t *testing.T) {

0 commit comments

Comments
 (0)