be42624 fix: address current review findings · gordon

M internal/adapters/in/cli/remote/config.go +6 -2


@@ -97,7 +97,9 @@ func migrateClientConfig(config *ClientConfig) {
97	97		return
98	98		}
99	99
100		-	fmt.Fprintf(os.Stderr, "Notice: migrated [client] config from gordon.toml to 'default' remote. The [client] section is deprecated; use 'gordon remotes' instead.\n")
	100	+	if _, err := fmt.Fprintf(os.Stderr, "Notice: migrated [client] config from gordon.toml to 'default' remote. The [client] section is deprecated; use 'gordon remotes' instead.\n"); err != nil {
	101	+	_, _ = os.Stderr.WriteString("Warning: failed to write remote migration notice\n")
	102	+	}
101	103
102	104		config.Remotes["default"] = RemoteEntry{
103	105		URL: legacy.Client.Remote,

@@ -111,7 +113,9 @@ func migrateClientConfig(config *ClientConfig) {
111	113		}
112	114
113	115		if err := SaveRemotes("", config); err != nil {
114		-	fmt.Fprintf(os.Stderr, "Warning: failed to save migrated remotes: %v\n", err)
	116	+	if _, writeErr := fmt.Fprintf(os.Stderr, "Warning: failed to save migrated remotes: %v\n", err); writeErr != nil {
	117	+	_, _ = os.Stderr.WriteString("Warning: failed to write remote migration warning\n")
	118	+	}
115	119		}
116	120		}
117	121

 		return
 	}
-	fmt.Fprintf(os.Stderr, "Notice: migrated [client] config from gordon.toml to 'default' remote. The [client] section is deprecated; use 'gordon remotes' instead.\n")
+	if _, err := fmt.Fprintf(os.Stderr, "Notice: migrated [client] config from gordon.toml to 'default' remote. The [client] section is deprecated; use 'gordon remotes' instead.\n"); err != nil {
+		_, _ = os.Stderr.WriteString("Warning: failed to write remote migration notice\n")
+	}
 	config.Remotes["default"] = RemoteEntry{
 		URL:         legacy.Client.Remote,
 	}
 	if err := SaveRemotes("", config); err != nil {
-		fmt.Fprintf(os.Stderr, "Warning: failed to save migrated remotes: %v\n", err)
+		if _, writeErr := fmt.Fprintf(os.Stderr, "Warning: failed to save migrated remotes: %v\n", err); writeErr != nil {
+			_, _ = os.Stderr.WriteString("Warning: failed to write remote migration warning\n")
+		}
 	}
 }

M internal/adapters/in/http/httphelper/localhost_test.go +4 -3


@@ -5,6 +5,8 @@ import (
5	5		"net/http/httptest"
6	6		"testing"
7	7
	8	+	"github.com/stretchr/testify/assert"
	9	+
8	10		"github.com/bnema/gordon/internal/adapters/in/http/httphelper"
9	11		)
10	12

@@ -25,9 +27,8 @@ func TestIsLocalhostRequest(t *testing.T) {
25	27		t.Run(tt.name, func(t *testing.T) {
26	28		req := httptest.NewRequest(http.MethodGet, "/", nil)
27	29		req.RemoteAddr = tt.remoteAddr
28		-	if got := httphelper.IsLocalhostRequest(req); got != tt.want {
29		-	t.Errorf("IsLocalhostRequest(%q) = %v, want %v", tt.remoteAddr, got, tt.want)
30		-	}
	30	+	got := httphelper.IsLocalhostRequest(req)
	31	+	assert.Equal(t, tt.want, got, "IsLocalhostRequest(%q)", tt.remoteAddr)
31	32		})
32	33		}
33	34		}

 	"net/http/httptest"
 	"testing"
+	"github.com/stretchr/testify/assert"
 	"github.com/bnema/gordon/internal/adapters/in/http/httphelper"
 )
 		t.Run(tt.name, func(t *testing.T) {
 			req := httptest.NewRequest(http.MethodGet, "/", nil)
 			req.RemoteAddr = tt.remoteAddr
-			if got := httphelper.IsLocalhostRequest(req); got != tt.want {
+			got := httphelper.IsLocalhostRequest(req)
-				t.Errorf("IsLocalhostRequest(%q) = %v, want %v", tt.remoteAddr, got, tt.want)
+			assert.Equal(t, tt.want, got, "IsLocalhostRequest(%q)", tt.remoteAddr)
-			}
 		})
 	}
 }

M internal/adapters/in/http/httphelper/trust.go +5 -0


@@ -16,6 +16,11 @@ var localhostNets = ParseTrustedProxies([]string{"127.0.0.0/8", "::1"})
16	16		func ParseTrustedProxies(proxies []string) []*net.IPNet {
17	17		var nets []*net.IPNet
18	18		for _, proxy := range proxies {
	19	+	proxy = strings.TrimSpace(proxy)
	20	+	if proxy == "" {
	21	+	continue
	22	+	}
	23	+
19	24		// Try parsing as CIDR
20	25		_, ipNet, err := net.ParseCIDR(proxy)
21	26		if err == nil {


@@ -16,6 +16,11 @@ var localhostNets = ParseTrustedProxies([]string{"127.0.0.0/8", "::1"})
16	func ParseTrustedProxies(proxies []string) []*net.IPNet {	16		func ParseTrustedProxies(proxies []string) []*net.IPNet {
17	var nets []*net.IPNet	17		var nets []*net.IPNet
18	for _, proxy := range proxies {	18		for _, proxy := range proxies {
		19	+	proxy = strings.TrimSpace(proxy)
		20	+	if proxy == "" {
		21	+	continue
		22	+	}
		23	+
19	// Try parsing as CIDR	24		// Try parsing as CIDR
20	_, ipNet, err := net.ParseCIDR(proxy)	25		_, ipNet, err := net.ParseCIDR(proxy)
21	if err == nil {	26		if err == nil {

M internal/adapters/in/http/middleware/accesslog_test.go +11 -5


@@ -2,6 +2,7 @@ package middleware
2	2
3	3		import (
4	4		"fmt"
	5	+	"io"
5	6		"net/http"
6	7		"net/http/httptest"
7	8		"testing"

@@ -65,11 +66,16 @@ func TestAccessLogger_RedactsSensitiveQueryParams(t *testing.T) {
65	66		handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
66	67		w.WriteHeader(http.StatusOK)
67	68		}))
68		-
69		-	req := httptest.NewRequest(http.MethodGet, "/oauth/callback?code=secret-code&state=ok&token=secret-token", nil)
70		-	rw := httptest.NewRecorder()
71		-
72		-	handler.ServeHTTP(rw, req)
	69	+	server := httptest.NewServer(handler)
	70	+	defer server.Close()
	71	+
	72	+	resp, err := http.Get(server.URL + "/oauth/callback?code=secret-code&state=ok&token=secret-token")
	73	+	require.NoError(t, err)
	74	+	defer func() {
	75	+	assert.NoError(t, resp.Body.Close())
	76	+	}()
	77	+	_, err = io.Copy(io.Discard, resp.Body)
	78	+	require.NoError(t, err)
73	79
74	80		require.Len(t, mock.entries, 1)
75	81		assert.Equal(t, "code=[REDACTED]&state=ok&token=[REDACTED]", mock.entries[0].Query)


@@ -2,6 +2,7 @@ package middleware
2			2
3		import (	3		import (
4		"fmt"	4		"fmt"
			5	+	"io"
5		"net/http"	6		"net/http"
6		"net/http/httptest"	7		"net/http/httptest"
7		"testing"	8		"testing"

@@ -65,11 +66,16 @@ func TestAccessLogger_RedactsSensitiveQueryParams(t *testing.T) {
65		handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {	66		handler := AccessLogger(mock, false, log, nil)(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
66		w.WriteHeader(http.StatusOK)	67		w.WriteHeader(http.StatusOK)
67		}))	68		}))
68	-		69	+	server := httptest.NewServer(handler)
69	-	req := httptest.NewRequest(http.MethodGet, "/oauth/callback?code=secret-code&state=ok&token=secret-token", nil)	70	+	defer server.Close()
70	-	rw := httptest.NewRecorder()	71	+
71	-		72	+	resp, err := http.Get(server.URL + "/oauth/callback?code=secret-code&state=ok&token=secret-token")
72	-	handler.ServeHTTP(rw, req)	73	+	require.NoError(t, err)
			74	+	defer func() {
			75	+	assert.NoError(t, resp.Body.Close())
			76	+	}()
			77	+	_, err = io.Copy(io.Discard, resp.Body)
			78	+	require.NoError(t, err)
73			79
74		require.Len(t, mock.entries, 1)	80		require.Len(t, mock.entries, 1)
75		assert.Equal(t, "code=[REDACTED]&state=ok&token=[REDACTED]", mock.entries[0].Query)	81		assert.Equal(t, "code=[REDACTED]&state=ok&token=[REDACTED]", mock.entries[0].Query)

M internal/adapters/in/http/proxy/forwarder.go +1 -6


@@ -202,7 +202,7 @@ func newReverseProxy(targetURL *url.URL, originalHost string, transport http.Rou
202	202		// SetXForwarded() unconditionally sets it based on the incoming scheme
203	203		// (HTTP between proxies), but when a trusted proxy already sent the
204	204		// real client proto, we must honor it.
205		-	if existingProto != "" && isTrustedSource(incomingReq, trustedNets) {
	205	+	if existingProto != "" && gordonhttp.IsTrustedSource(incomingReq, trustedNets) {
206	206		pr.Out.Header.Set("X-Forwarded-Proto", existingProto)
207	207		}
208	208		},

@@ -212,11 +212,6 @@ func newReverseProxy(targetURL *url.URL, originalHost string, transport http.Rou
212	212		}
213	213		}
214	214
215		-	// isTrustedSource reports whether the request's remote address is in trustedNets.
216		-	func isTrustedSource(r http.Request, trustedNets []net.IPNet) bool {
217		-	return gordonhttp.IsTrustedSource(r, trustedNets)
218		-	}
219		-
220	215		// modifyResponse returns a function that adds proxy headers and enforces response size limits.
221	216		func modifyResponse(maxResponseSize int64) func(*http.Response) error {
222	217		return func(resp *http.Response) error {

 			// SetXForwarded() unconditionally sets it based on the incoming scheme
 			// (HTTP between proxies), but when a trusted proxy already sent the
 			// real client proto, we must honor it.
-			if existingProto != "" && isTrustedSource(incomingReq, trustedNets) {
+			if existingProto != "" && gordonhttp.IsTrustedSource(incomingReq, trustedNets) {
 				pr.Out.Header.Set("X-Forwarded-Proto", existingProto)
 			}
 		},
 	}
 }
-// isTrustedSource reports whether the request's remote address is in trustedNets.
-func isTrustedSource(r *http.Request, trustedNets []*net.IPNet) bool {
-	return gordonhttp.IsTrustedSource(r, trustedNets)
-}
 // modifyResponse returns a function that adds proxy headers and enforces response size limits.
 func modifyResponse(maxResponseSize int64) func(*http.Response) error {
 	return func(resp *http.Response) error {

M internal/usecase/container/service.go +0 -4


@@ -835,10 +835,6 @@ func isPullFailure(err error) bool {
835	835		return false
836	836		}
837	837
838		-	if errors.Is(err, domain.ErrImagePullFailed) {
839		-	return true
840		-	}
841		-
842	838		msg := strings.ToLower(err.Error())
843	839		return strings.Contains(msg, "failed to pull image") \|\|
844	840		strings.Contains(msg, "pull access denied")


@@ -835,10 +835,6 @@ func isPullFailure(err error) bool {
835		return false	835	return false
836		}	836	}
837			837
838	-	if errors.Is(err, domain.ErrImagePullFailed) {
839	-	return true
840	-	}
841	-
842		msg := strings.ToLower(err.Error())	838	msg := strings.ToLower(err.Error())
843		return strings.Contains(msg, "failed to pull image") \|\|	839	return strings.Contains(msg, "failed to pull image") \|\|
844		strings.Contains(msg, "pull access denied")	840	strings.Contains(msg, "pull access denied")