Skip to content

Commit 65b54f1

Browse files
appleboyclaude
andauthored
fix(main): address medium-severity security findings (#15)
* fix(main): address medium-severity security findings - Bound HTTP response reads to 1 MB via io.LimitReader to prevent unbounded memory allocation - Replace configInitialized bool with sync.Once to eliminate potential data race - Warn when client secret is passed via CLI flag as it may be visible in process listings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * ci(workflows): remove non-existent make generate step The lint job referenced `make generate` which has no corresponding Makefile target, causing CI to fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(main): return explicit error on oversized responses Address Copilot review feedback: - Fix comment from "1 MB" to "1 MiB" (1<<20 is a mebibyte) - Read maxResponseSize+1 bytes and return a clear "response too large" error instead of silently truncating (which caused confusing JSON parse errors) - Extract readResponseBody helper to deduplicate the pattern - Add tests for readResponseBody (within limit, at limit, exceeds limit) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 9bc1791 commit 65b54f1

3 files changed

Lines changed: 80 additions & 24 deletions

File tree

.github/workflows/testing.yml

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,6 @@ jobs:
3232
with:
3333
go-version: ${{ matrix.go }}
3434

35-
- name: Run Generate
36-
run: |
37-
make generate
38-
3935
- name: Setup golangci-lint
4036
uses: golangci/golangci-lint-action@v9
4137
with:

main.go

Lines changed: 46 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"os/signal"
1515
"strconv"
1616
"strings"
17+
"sync"
1718
"syscall"
1819
"time"
1920

@@ -28,18 +29,18 @@ import (
2829
)
2930

3031
var (
31-
serverURL string
32-
clientID string
33-
clientSecret string
34-
redirectURI string
35-
callbackPort int
36-
scope string
37-
tokenFile string
38-
tokenStoreMode string
39-
tokenStore credstore.Store[credstore.Token]
40-
configInitialized bool
41-
retryClient *retry.Client
42-
configWarnings []string
32+
serverURL string
33+
clientID string
34+
clientSecret string
35+
redirectURI string
36+
callbackPort int
37+
scope string
38+
tokenFile string
39+
tokenStoreMode string
40+
tokenStore credstore.Store[credstore.Token]
41+
configOnce sync.Once
42+
retryClient *retry.Client
43+
configWarnings []string
4344

4445
flagServerURL *string
4546
flagClientID *string
@@ -55,6 +56,7 @@ const (
5556
tokenExchangeTimeout = 10 * time.Second
5657
tokenVerificationTimeout = 10 * time.Second
5758
refreshTokenTimeout = 10 * time.Second
59+
maxResponseSize = 1 << 20 // 1 MiB
5860
)
5961

6062
func init() {
@@ -96,16 +98,21 @@ func init() {
9698

9799
// initConfig parses flags and initializes all configuration.
98100
func initConfig() {
99-
if configInitialized {
100-
return
101-
}
102-
configInitialized = true
101+
configOnce.Do(doInitConfig)
102+
}
103103

104+
func doInitConfig() {
104105
flag.Parse()
105106

106107
serverURL = getConfig(*flagServerURL, "SERVER_URL", "http://localhost:8080")
107108
clientID = getConfig(*flagClientID, "CLIENT_ID", "")
108109
clientSecret = getConfig(*flagClientSecret, "CLIENT_SECRET", "")
110+
if *flagClientSecret != "" {
111+
configWarnings = append(configWarnings,
112+
"Client secret passed via command-line flag. "+
113+
"This may be visible in process listings. "+
114+
"Consider using CLIENT_SECRET env var or .env file instead.")
115+
}
109116
scope = getConfig(*flagScope, "SCOPE", "read write")
110117
tokenFile = getConfig(*flagTokenFile, "TOKEN_FILE", ".authgate-tokens.json")
111118

@@ -262,6 +269,25 @@ type tokenResponse struct {
262269
Scope string `json:"scope"`
263270
}
264271

272+
// errResponseTooLarge is returned when a server response exceeds maxResponseSize.
273+
var errResponseTooLarge = fmt.Errorf(
274+
"response body exceeds maximum allowed size of %d bytes",
275+
maxResponseSize,
276+
)
277+
278+
// readResponseBody reads up to maxResponseSize bytes from r and returns an
279+
// explicit error when the response is too large (rather than silently truncating).
280+
func readResponseBody(r io.Reader) ([]byte, error) {
281+
body, err := io.ReadAll(io.LimitReader(r, maxResponseSize+1))
282+
if err != nil {
283+
return nil, err
284+
}
285+
if int64(len(body)) > maxResponseSize {
286+
return nil, errResponseTooLarge
287+
}
288+
return body, nil
289+
}
290+
265291
// parseOAuthError attempts to extract a structured OAuth error from a non-200
266292
// response body. Falls back to including the raw body in the error message.
267293
func parseOAuthError(statusCode int, body []byte, action string) error {
@@ -341,7 +367,7 @@ func exchangeCode(ctx context.Context, code, codeVerifier string) (*tui.TokenSto
341367
}
342368
defer resp.Body.Close()
343369

344-
body, err := io.ReadAll(resp.Body)
370+
body, err := readResponseBody(resp.Body)
345371
if err != nil {
346372
return nil, fmt.Errorf("failed to read response: %w", err)
347373
}
@@ -405,7 +431,7 @@ func refreshAccessToken(ctx context.Context, refreshToken string) (*tui.TokenSto
405431
}
406432
defer resp.Body.Close()
407433

408-
body, err := io.ReadAll(resp.Body)
434+
body, err := readResponseBody(resp.Body)
409435
if err != nil {
410436
return nil, fmt.Errorf("failed to read response: %w", err)
411437
}
@@ -472,7 +498,7 @@ func verifyToken(ctx context.Context, accessToken string) (string, error) {
472498
}
473499
defer resp.Body.Close()
474500

475-
body, err := io.ReadAll(resp.Body)
501+
body, err := readResponseBody(resp.Body)
476502
if err != nil {
477503
return "", fmt.Errorf("failed to read response: %w", err)
478504
}
@@ -531,7 +557,7 @@ func makeAPICallWithAutoRefresh(ctx context.Context, storage *tui.TokenStorage)
531557
defer resp.Body.Close()
532558
}
533559

534-
body, err := io.ReadAll(resp.Body)
560+
body, err := readResponseBody(resp.Body)
535561
if err != nil {
536562
return fmt.Errorf("failed to read response: %w", err)
537563
}

main_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package main
22

33
import (
4+
"bytes"
5+
"errors"
46
"path/filepath"
57
"testing"
68
"time"
@@ -276,6 +278,38 @@ func TestInitTokenStore_Invalid(t *testing.T) {
276278
}
277279
}
278280

281+
func TestReadResponseBody(t *testing.T) {
282+
t.Run("within limit", func(t *testing.T) {
283+
data := bytes.Repeat([]byte("a"), 100)
284+
body, err := readResponseBody(bytes.NewReader(data))
285+
if err != nil {
286+
t.Fatalf("unexpected error: %v", err)
287+
}
288+
if len(body) != 100 {
289+
t.Errorf("expected 100 bytes, got %d", len(body))
290+
}
291+
})
292+
293+
t.Run("exactly at limit", func(t *testing.T) {
294+
data := bytes.Repeat([]byte("a"), maxResponseSize)
295+
body, err := readResponseBody(bytes.NewReader(data))
296+
if err != nil {
297+
t.Fatalf("unexpected error: %v", err)
298+
}
299+
if len(body) != maxResponseSize {
300+
t.Errorf("expected %d bytes, got %d", maxResponseSize, len(body))
301+
}
302+
})
303+
304+
t.Run("exceeds limit", func(t *testing.T) {
305+
data := bytes.Repeat([]byte("a"), maxResponseSize+1)
306+
_, err := readResponseBody(bytes.NewReader(data))
307+
if !errors.Is(err, errResponseTooLarge) {
308+
t.Errorf("expected errResponseTooLarge, got: %v", err)
309+
}
310+
})
311+
}
312+
279313
// containsSubstring is a helper to avoid importing strings in tests.
280314
func containsSubstring(s, sub string) bool {
281315
return len(s) >= len(sub) && findSubstring(s, sub)

0 commit comments

Comments
 (0)