0ef5b19 fix(review): address code review findings — correctness, robustness, observability · gordon

M internal/adapters/in/cli/push.go +9 -9


@@ -464,8 +464,8 @@ func buildAndPush(ctx context.Context, version, platform, dockerfile string, bui
464	464		// Cloudflare's 100MB per-request limit. Loading locally then
465	465		// using docker push gives us chunked uploads (~5MB per request).
466	466		fmt.Println("\nBuilding image...")
467		-	buildCmd := exec.CommandContext(ctx, "docker", buildImageArgs(version, platform, dockerfile, buildArgs, versionRef, latestRef)...) // #nosec G204
468		-	buildCmd.Env = os.Environ() // VERSION is now passed as --build-arg VERSION=<value>
	467	+	buildCmd := exec.CommandContext(ctx, "docker", buildImageArgs(ctx, version, platform, dockerfile, buildArgs, versionRef, latestRef)...) // #nosec G204
	468	+	buildCmd.Env = os.Environ() // VERSION is now passed as --build-arg VERSION=<value>
469	469		buildCmd.Stdout = os.Stdout
470	470		buildCmd.Stderr = os.Stderr
471	471		if err := buildCmd.Run(); err != nil {

@@ -488,8 +488,8 @@ func buildAndPush(ctx context.Context, version, platform, dockerfile string, bui
488	488		// standardBuildArgs returns the standard set of git-related build args as
489	489		// explicit KEY=VALUE pairs. User-supplied args are appended after and take
490	490		// precedence (Docker uses the last occurrence of a duplicate key).
491		-	func standardBuildArgs(version string) []string {
492		-	gitSHA := resolveGitSHA()
	491	+	func standardBuildArgs(ctx context.Context, version string) []string {
	492	+	gitSHA := resolveGitSHA(ctx)
493	493		buildTime := time.Now().UTC().Format(time.RFC3339)
494	494		return []string{
495	495		"VERSION=" + version,

@@ -500,8 +500,8 @@ func standardBuildArgs(version string) []string {
500	500		}
501	501
502	502		// resolveGitSHA returns the short git SHA of HEAD, or "unknown" if unavailable.
503		-	func resolveGitSHA() string {
504		-	out, err := exec.Command("git", "rev-parse", "--short", "HEAD").Output() // #nosec G204
	503	+	func resolveGitSHA(ctx context.Context) string {
	504	+	out, err := exec.CommandContext(ctx, "git", "rev-parse", "--short", "HEAD").Output() // #nosec G204
505	505		if err != nil {
506	506		return "unknown"
507	507		}

@@ -511,7 +511,7 @@ func resolveGitSHA() string {
511	511		// buildImageArgs constructs the docker buildx build arguments.
512	512		// Uses --load instead of --push so the image is loaded into the local
513	513		// daemon, allowing docker push to handle the upload with chunked requests.
514		-	func buildImageArgs(version, platform, dockerfile string, buildArgs []string, versionRef, latestRef string) []string {
	514	+	func buildImageArgs(ctx context.Context, version, platform, dockerfile string, buildArgs []string, versionRef, latestRef string) []string {
515	515		args := []string{
516	516		"buildx", "build",
517	517		"--platform", platform,

@@ -524,7 +524,7 @@ func buildImageArgs(version, platform, dockerfile string, buildArgs []string, ve
524	524
525	525		// Inject standard git build args as explicit KEY=VALUE pairs.
526	526		// User-supplied --build-arg flags are appended AFTER so they override defaults.
527		-	for _, ba := range standardBuildArgs(version) {
	527	+	for _, ba := range standardBuildArgs(ctx, version) {
528	528		args = append(args, "--build-arg", ba)
529	529		}
530	530		for _, ba := range buildArgs {

@@ -717,7 +717,7 @@ func parseImageRef(image string) (registry, name, tag string) {
717	717		func getGitVersion(ctx context.Context) string {
718	718		out, err := exec.CommandContext(ctx, "git", "describe", "--tags", "--dirty").Output() // #nosec G204
719	719		if err != nil {
720		-	fmt.Fprintln(os.Stderr, "Warning: no git tags found — image version will be 'latest'. Tag your repo to get versioned images.")
	720	+	fmt.Fprintf(os.Stderr, "Warning: unable to determine git tag (%v) — image version will be 'latest'. Tag your repo to get versioned images.\n", err)
721	721		return ""
722	722		}
723	723		return strings.TrimSpace(string(out))


@@ -464,8 +464,8 @@ func buildAndPush(ctx context.Context, version, platform, dockerfile string, bui
464		// Cloudflare's 100MB per-request limit. Loading locally then	464		// Cloudflare's 100MB per-request limit. Loading locally then
465		// using docker push gives us chunked uploads (~5MB per request).	465		// using docker push gives us chunked uploads (~5MB per request).
466		fmt.Println("\nBuilding image...")	466		fmt.Println("\nBuilding image...")
467	-	buildCmd := exec.CommandContext(ctx, "docker", buildImageArgs(version, platform, dockerfile, buildArgs, versionRef, latestRef)...) // #nosec G204	467	+	buildCmd := exec.CommandContext(ctx, "docker", buildImageArgs(ctx, version, platform, dockerfile, buildArgs, versionRef, latestRef)...) // #nosec G204
468	-	buildCmd.Env = os.Environ() // VERSION is now passed as --build-arg VERSION=<value>	468	+	buildCmd.Env = os.Environ() // VERSION is now passed as --build-arg VERSION=<value>
469		buildCmd.Stdout = os.Stdout	469		buildCmd.Stdout = os.Stdout
470		buildCmd.Stderr = os.Stderr	470		buildCmd.Stderr = os.Stderr
471		if err := buildCmd.Run(); err != nil {	471		if err := buildCmd.Run(); err != nil {

@@ -488,8 +488,8 @@ func buildAndPush(ctx context.Context, version, platform, dockerfile string, bui
488		// standardBuildArgs returns the standard set of git-related build args as	488		// standardBuildArgs returns the standard set of git-related build args as
489		// explicit KEY=VALUE pairs. User-supplied args are appended after and take	489		// explicit KEY=VALUE pairs. User-supplied args are appended after and take
490		// precedence (Docker uses the last occurrence of a duplicate key).	490		// precedence (Docker uses the last occurrence of a duplicate key).
491	-	func standardBuildArgs(version string) []string {	491	+	func standardBuildArgs(ctx context.Context, version string) []string {
492	-	gitSHA := resolveGitSHA()	492	+	gitSHA := resolveGitSHA(ctx)
493		buildTime := time.Now().UTC().Format(time.RFC3339)	493		buildTime := time.Now().UTC().Format(time.RFC3339)
494		return []string{	494		return []string{
495		"VERSION=" + version,	495		"VERSION=" + version,

@@ -500,8 +500,8 @@ func standardBuildArgs(version string) []string {
500		}	500		}
501			501
502		// resolveGitSHA returns the short git SHA of HEAD, or "unknown" if unavailable.	502		// resolveGitSHA returns the short git SHA of HEAD, or "unknown" if unavailable.
503	-	func resolveGitSHA() string {	503	+	func resolveGitSHA(ctx context.Context) string {
504	-	out, err := exec.Command("git", "rev-parse", "--short", "HEAD").Output() // #nosec G204	504	+	out, err := exec.CommandContext(ctx, "git", "rev-parse", "--short", "HEAD").Output() // #nosec G204
505		if err != nil {	505		if err != nil {
506		return "unknown"	506		return "unknown"
507		}	507		}

@@ -511,7 +511,7 @@ func resolveGitSHA() string {
511		// buildImageArgs constructs the docker buildx build arguments.	511		// buildImageArgs constructs the docker buildx build arguments.
512		// Uses --load instead of --push so the image is loaded into the local	512		// Uses --load instead of --push so the image is loaded into the local
513		// daemon, allowing docker push to handle the upload with chunked requests.	513		// daemon, allowing docker push to handle the upload with chunked requests.
514	-	func buildImageArgs(version, platform, dockerfile string, buildArgs []string, versionRef, latestRef string) []string {	514	+	func buildImageArgs(ctx context.Context, version, platform, dockerfile string, buildArgs []string, versionRef, latestRef string) []string {
515		args := []string{	515		args := []string{
516		"buildx", "build",	516		"buildx", "build",
517		"--platform", platform,	517		"--platform", platform,

@@ -524,7 +524,7 @@ func buildImageArgs(version, platform, dockerfile string, buildArgs []string, ve
524			524
525		// Inject standard git build args as explicit KEY=VALUE pairs.	525		// Inject standard git build args as explicit KEY=VALUE pairs.
526		// User-supplied --build-arg flags are appended AFTER so they override defaults.	526		// User-supplied --build-arg flags are appended AFTER so they override defaults.
527	-	for _, ba := range standardBuildArgs(version) {	527	+	for _, ba := range standardBuildArgs(ctx, version) {
528		args = append(args, "--build-arg", ba)	528		args = append(args, "--build-arg", ba)
529		}	529		}
530		for _, ba := range buildArgs {	530		for _, ba := range buildArgs {

@@ -717,7 +717,7 @@ func parseImageRef(image string) (registry, name, tag string) {
717		func getGitVersion(ctx context.Context) string {	717		func getGitVersion(ctx context.Context) string {
718		out, err := exec.CommandContext(ctx, "git", "describe", "--tags", "--dirty").Output() // #nosec G204	718		out, err := exec.CommandContext(ctx, "git", "describe", "--tags", "--dirty").Output() // #nosec G204
719		if err != nil {	719		if err != nil {
720	-	fmt.Fprintln(os.Stderr, "Warning: no git tags found — image version will be 'latest'. Tag your repo to get versioned images.")	720	+	fmt.Fprintf(os.Stderr, "Warning: unable to determine git tag (%v) — image version will be 'latest'. Tag your repo to get versioned images.\n", err)
721		return ""	721		return ""
722		}	722		}
723		return strings.TrimSpace(string(out))	723		return strings.TrimSpace(string(out))

M internal/adapters/in/cli/push_test.go +12 -5


@@ -69,7 +69,7 @@ func TestParseImageRef(t *testing.T) {
69	69
70	70		func TestBuildAndPush_BuildArgs(t *testing.T) {
71	71		// Verify buildImageArgs produces --load instead of --push
72		-	args := buildImageArgs("v1.0.0", "linux/amd64", "Dockerfile", []string{"CGO_ENABLED=0"}, "reg.example.com/app:v1.0.0", "reg.example.com/app:latest")
	72	+	args := buildImageArgs(context.Background(), "v1.0.0", "linux/amd64", "Dockerfile", []string{"CGO_ENABLED=0"}, "reg.example.com/app:v1.0.0", "reg.example.com/app:latest")
73	73
74	74		assert.Contains(t, args, "--load")
75	75		assert.NotContains(t, args, "--push")

@@ -80,7 +80,7 @@ func TestBuildAndPush_BuildArgs(t *testing.T) {
80	80		}
81	81
82	82		func TestBuildImageArgsInjectsGitBuildArgs(t *testing.T) {
83		-	args := buildImageArgs("v1.2.3", "linux/amd64", "Dockerfile", nil, "registry/img:v1.2.3", "registry/img:latest")
	83	+	args := buildImageArgs(context.Background(), "v1.2.3", "linux/amd64", "Dockerfile", nil, "registry/img:v1.2.3", "registry/img:latest")
84	84
85	85		// Must contain explicit KEY=VALUE for all standard git build args
86	86		argStr := strings.Join(args, " ")

@@ -100,18 +100,25 @@ func TestBuildImageArgsInjectsGitBuildArgs(t *testing.T) {
100	100
101	101		func TestBuildImageArgsUserArgsOverrideDefaults(t *testing.T) {
102	102		userArgs := []string{"GIT_TAG=custom-override"}
103		-	args := buildImageArgs("v1.2.3", "linux/amd64", "Dockerfile", userArgs, "r/i:v1.2.3", "r/i:latest")
	103	+	args := buildImageArgs(context.Background(), "v1.2.3", "linux/amd64", "Dockerfile", userArgs, "r/i:v1.2.3", "r/i:latest")
104	104
105		-	// Count how many times GIT_TAG appears — user override should come last (Docker uses last value)
	105	+	// Count how many times GIT_TAG appears and track the last occurrence index.
	106	+	// Docker uses the last occurrence of a duplicate --build-arg key, so the user
	107	+	// override must come after the default injected value.
106	108		count := 0
	109	+	lastIdx := -1
107	110		for i, a := range args {
108	111		if a == "--build-arg" && i+1 < len(args) && strings.HasPrefix(args[i+1], "GIT_TAG=") {
109	112		count++
	113	+	lastIdx = i + 1
110	114		}
111	115		}
112	116		if count < 2 {
113	117		t.Errorf("expected GIT_TAG to appear twice (default + override), got %d", count)
114	118		}
	119	+	if lastIdx < 0 \|\| args[lastIdx] != "GIT_TAG="+userArgs[0][len("GIT_TAG="):] {
	120	+	t.Errorf("expected last GIT_TAG= arg to be the user override %q, got %q", "GIT_TAG=custom-override", args[lastIdx])
	121	+	}
115	122		}
116	123
117	124		func TestParseTagRef(t *testing.T) {

@@ -180,7 +187,7 @@ func TestVersionFromTagRefs(t *testing.T) {
180	187		}
181	188
182	189		func TestBuildImageArgs_CustomDockerfile(t *testing.T) {
183		-	args := buildImageArgs("v1.0.0", "linux/amd64", "docker/app/Dockerfile", nil, "reg.example.com/app:v1.0.0", "reg.example.com/app:latest")
	190	+	args := buildImageArgs(context.Background(), "v1.0.0", "linux/amd64", "docker/app/Dockerfile", nil, "reg.example.com/app:v1.0.0", "reg.example.com/app:latest")
184	191
185	192		assert.Contains(t, args, "-f")
186	193		assert.Contains(t, args, "docker/app/Dockerfile")

 func TestBuildAndPush_BuildArgs(t *testing.T) {
 	// Verify buildImageArgs produces --load instead of --push
-	args := buildImageArgs("v1.0.0", "linux/amd64", "Dockerfile", []string{"CGO_ENABLED=0"}, "reg.example.com/app:v1.0.0", "reg.example.com/app:latest")
+	args := buildImageArgs(context.Background(), "v1.0.0", "linux/amd64", "Dockerfile", []string{"CGO_ENABLED=0"}, "reg.example.com/app:v1.0.0", "reg.example.com/app:latest")
 	assert.Contains(t, args, "--load")
 	assert.NotContains(t, args, "--push")
 }
 func TestBuildImageArgsInjectsGitBuildArgs(t *testing.T) {
-	args := buildImageArgs("v1.2.3", "linux/amd64", "Dockerfile", nil, "registry/img:v1.2.3", "registry/img:latest")
+	args := buildImageArgs(context.Background(), "v1.2.3", "linux/amd64", "Dockerfile", nil, "registry/img:v1.2.3", "registry/img:latest")
 	// Must contain explicit KEY=VALUE for all standard git build args
 	argStr := strings.Join(args, " ")
 func TestBuildImageArgsUserArgsOverrideDefaults(t *testing.T) {
 	userArgs := []string{"GIT_TAG=custom-override"}
-	args := buildImageArgs("v1.2.3", "linux/amd64", "Dockerfile", userArgs, "r/i:v1.2.3", "r/i:latest")
+	args := buildImageArgs(context.Background(), "v1.2.3", "linux/amd64", "Dockerfile", userArgs, "r/i:v1.2.3", "r/i:latest")
-	// Count how many times GIT_TAG appears — user override should come last (Docker uses last value)
+	// Count how many times GIT_TAG appears and track the last occurrence index.
+	// Docker uses the last occurrence of a duplicate --build-arg key, so the user
+	// override must come after the default injected value.
 	count := 0
+	lastIdx := -1
 	for i, a := range args {
 		if a == "--build-arg" && i+1 < len(args) && strings.HasPrefix(args[i+1], "GIT_TAG=") {
 			count++
+			lastIdx = i + 1
 		}
 	}
 	if count < 2 {
 		t.Errorf("expected GIT_TAG to appear twice (default + override), got %d", count)
 	}
+	if lastIdx < 0 || args[lastIdx] != "GIT_TAG="+userArgs[0][len("GIT_TAG="):] {
+		t.Errorf("expected last GIT_TAG= arg to be the user override %q, got %q", "GIT_TAG=custom-override", args[lastIdx])
+	}
 }
 func TestParseTagRef(t *testing.T) {
 }
 func TestBuildImageArgs_CustomDockerfile(t *testing.T) {
-	args := buildImageArgs("v1.0.0", "linux/amd64", "docker/app/Dockerfile", nil, "reg.example.com/app:v1.0.0", "reg.example.com/app:latest")
+	args := buildImageArgs(context.Background(), "v1.0.0", "linux/amd64", "docker/app/Dockerfile", nil, "reg.example.com/app:v1.0.0", "reg.example.com/app:latest")
 	assert.Contains(t, args, "-f")
 	assert.Contains(t, args, "docker/app/Dockerfile")

M internal/adapters/in/cli/remote/client.go +27 -16


@@ -94,6 +94,28 @@ func WithTokenRefreshCallback(fn func(newToken string)) ClientOption {
94	94		}
95	95		}
96	96
	97	+	// observeTokenRotation checks the X-Gordon-Token header on a response and, if
	98	+	// the server issued a refreshed token, updates the client's in-memory token and
	99	+	// invokes the optional persistence callback. Panics in the callback are caught
	100	+	// so they never propagate to the caller.
	101	+	func (c Client) observeTokenRotation(resp http.Response) {
	102	+	newToken := resp.Header.Get("X-Gordon-Token")
	103	+	if newToken == "" \|\| newToken == c.token {
	104	+	return
	105	+	}
	106	+	c.token = newToken
	107	+	if c.onTokenRefreshed != nil {
	108	+	func() {
	109	+	defer func() {
	110	+	if r := recover(); r != nil {
	111	+	fmt.Fprintf(os.Stderr, "warning: token refresh callback panicked: %v\n", r)
	112	+	}
	113	+	}()
	114	+	c.onTokenRefreshed(newToken)
	115	+	}()
	116	+	}
	117	+	}
	118	+
97	119		func (c *Client) applyTLSConfig() {
98	120		if !c.insecureTLS {
99	121		return

@@ -155,22 +177,8 @@ func (c Client) request(ctx context.Context, method, path string, body any) (h
155	177		return nil, err
156	178		}
157	179
158		-	// If the server returned a refreshed token, update the client's token and notify any callback.
159		-	// This implements the sliding expiry: the server re-signs the token and sends it back.
160		-	if newToken := resp.Header.Get("X-Gordon-Token"); newToken != "" && newToken != c.token {
161		-	c.token = newToken
162		-	if c.onTokenRefreshed != nil {
163		-	// Token persistence is non-fatal: a panic in the callback must not crash the request.
164		-	func() {
165		-	defer func() {
166		-	if r := recover(); r != nil {
167		-	fmt.Fprintf(os.Stderr, "warning: token refresh callback panicked: %v\n", r)
168		-	}
169		-	}()
170		-	c.onTokenRefreshed(newToken)
171		-	}()
172		-	}
173		-	}
	180	+	// Observe token rotation: the server re-signs and returns the token via X-Gordon-Token.
	181	+	c.observeTokenRotation(resp)
174	182
175	183		return resp, nil
176	184		}

@@ -919,6 +927,9 @@ func (c *Client) streamLogs(ctx context.Context, path string) (<-chan string, er
919	927		return nil, fmt.Errorf("failed to connect: %w", err)
920	928		}
921	929
	930	+	// Observe token rotation on streaming responses, same as request().
	931	+	c.observeTokenRotation(resp)
	932	+
922	933		if resp.StatusCode >= 400 {
923	934		body, _ := io.ReadAll(resp.Body)
924	935		resp.Body.Close()

 	}
 }
+// observeTokenRotation checks the X-Gordon-Token header on a response and, if
+// the server issued a refreshed token, updates the client's in-memory token and
+// invokes the optional persistence callback. Panics in the callback are caught
+// so they never propagate to the caller.
+func (c *Client) observeTokenRotation(resp *http.Response) {
+	newToken := resp.Header.Get("X-Gordon-Token")
+	if newToken == "" || newToken == c.token {
+		return
+	}
+	c.token = newToken
+	if c.onTokenRefreshed != nil {
+		func() {
+			defer func() {
+				if r := recover(); r != nil {
+					fmt.Fprintf(os.Stderr, "warning: token refresh callback panicked: %v\n", r)
+				}
+			}()
+			c.onTokenRefreshed(newToken)
+		}()
+	}
+}
 func (c *Client) applyTLSConfig() {
 	if !c.insecureTLS {
 		return
 		return nil, err
 	}
-	// If the server returned a refreshed token, update the client's token and notify any callback.
+	// Observe token rotation: the server re-signs and returns the token via X-Gordon-Token.
-	// This implements the sliding expiry: the server re-signs the token and sends it back.
+	c.observeTokenRotation(resp)
-	if newToken := resp.Header.Get("X-Gordon-Token"); newToken != "" && newToken != c.token {
-		c.token = newToken
-		if c.onTokenRefreshed != nil {
-			// Token persistence is non-fatal: a panic in the callback must not crash the request.
-			func() {
-				defer func() {
-					if r := recover(); r != nil {
-						fmt.Fprintf(os.Stderr, "warning: token refresh callback panicked: %v\n", r)
-					}
-				}()
-				c.onTokenRefreshed(newToken)
-			}()
-		}
-	}
 	return resp, nil
 }
 		return nil, fmt.Errorf("failed to connect: %w", err)
 	}
+	// Observe token rotation on streaming responses, same as request().
+	c.observeTokenRotation(resp)
 	if resp.StatusCode >= 400 {
 		body, _ := io.ReadAll(resp.Body)
 		resp.Body.Close()

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


@@ -152,6 +152,8 @@ func ResolveRemoteFull(flagRemote, flagToken string, flagInsecure bool) (url, to
152	152		var name string
153	153		url, name, isRemote = resolveRemoteURL(flagRemote, config, remotes)
154	154		if isRemote {
	155	+	// resolveToken looks up remotes.Active internally; name == remotes.Active here
	156	+	// because resolveRemoteURL only sets name when it finds the active remote entry.
155	157		token = resolveToken(flagToken, config, remotes)
156	158		insecureTLS = resolveInsecureTLS(flagInsecure, config, remotes, name)
157	159		remoteName = name


@@ -152,6 +152,8 @@ func ResolveRemoteFull(flagRemote, flagToken string, flagInsecure bool) (url, to
152	var name string	152		var name string
153	url, name, isRemote = resolveRemoteURL(flagRemote, config, remotes)	153		url, name, isRemote = resolveRemoteURL(flagRemote, config, remotes)
154	if isRemote {	154		if isRemote {
		155	+	// resolveToken looks up remotes.Active internally; name == remotes.Active here
		156	+	// because resolveRemoteURL only sets name when it finds the active remote entry.
155	token = resolveToken(flagToken, config, remotes)	157		token = resolveToken(flagToken, config, remotes)
156	insecureTLS = resolveInsecureTLS(flagInsecure, config, remotes, name)	158		insecureTLS = resolveInsecureTLS(flagInsecure, config, remotes, name)
157	remoteName = name	159		remoteName = name

M internal/adapters/in/http/admin/middleware.go +3 -1


@@ -108,7 +108,9 @@ func AuthMiddleware(
108	108
109	109		// Attempt to slide token expiry. Non-fatal if it fails.
110	110		// The new token is returned in X-Gordon-Token so the CLI can persist it atomically.
111		-	if newToken, extErr := authSvc.ExtendToken(ctx, token); extErr == nil && newToken != token {
	111	+	if newToken, extErr := authSvc.ExtendToken(ctx, token); extErr != nil {
	112	+	log.Warn().Err(extErr).Str("subject", claims.Subject).Msg("token extension failed")
	113	+	} else if newToken != token {
112	114		w.Header().Set("X-Gordon-Token", newToken)
113	115		}
114	116


@@ -108,7 +108,9 @@ func AuthMiddleware(
108			108
109		// Attempt to slide token expiry. Non-fatal if it fails.	109		// Attempt to slide token expiry. Non-fatal if it fails.
110		// The new token is returned in X-Gordon-Token so the CLI can persist it atomically.	110		// The new token is returned in X-Gordon-Token so the CLI can persist it atomically.
111	-	if newToken, extErr := authSvc.ExtendToken(ctx, token); extErr == nil && newToken != token {	111	+	if newToken, extErr := authSvc.ExtendToken(ctx, token); extErr != nil {
			112	+	log.Warn().Err(extErr).Str("subject", claims.Subject).Msg("token extension failed")
			113	+	} else if newToken != token {
112		w.Header().Set("X-Gordon-Token", newToken)	114		w.Header().Set("X-Gordon-Token", newToken)
113		}	115		}
114			116

M internal/adapters/in/http/admin/middleware_test.go +55 -0


@@ -2,6 +2,7 @@ package admin
2	2
3	3		import (
4	4		"context"
	5	+	"errors"
5	6		"net/http"
6	7		"net/http/httptest"
7	8		"testing"

@@ -364,6 +365,60 @@ func TestRequireScope(t *testing.T) {
364	365		}
365	366		}
366	367
	368	+	func TestAuthMiddleware_ExtendTokenFailureIsNonFatal(t *testing.T) {
	369	+	authSvc := inmocks.NewMockAuthService(t)
	370	+
	371	+	authSvc.EXPECT().IsEnabled().Return(true)
	372	+	authSvc.EXPECT().ValidateToken(mock.Anything, "valid-admin-token").Return(&domain.TokenClaims{
	373	+	Subject: "admin",
	374	+	Scopes: []string{"admin::"},
	375	+	}, nil)
	376	+	// ExtendToken fails — response must still be 200 (non-fatal)
	377	+	authSvc.EXPECT().ExtendToken(mock.Anything, "valid-admin-token").Return("", errors.New("store unavailable"))
	378	+
	379	+	handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
	380	+	w.WriteHeader(http.StatusOK)
	381	+	})
	382	+
	383	+	mw := AuthMiddleware(authSvc, nil, nil, nil, adminTestLogger())
	384	+	wrappedHandler := mw(handler)
	385	+
	386	+	req := httptest.NewRequest(http.MethodGet, "/admin/status", nil)
	387	+	req.Header.Set("Authorization", "Bearer valid-admin-token")
	388	+	rec := httptest.NewRecorder()
	389	+	wrappedHandler.ServeHTTP(rec, req)
	390	+
	391	+	assert.Equal(t, http.StatusOK, rec.Code)
	392	+	assert.Empty(t, rec.Header().Get("X-Gordon-Token"))
	393	+	}
	394	+
	395	+	func TestAuthMiddleware_ExtendTokenRotationHeader(t *testing.T) {
	396	+	authSvc := inmocks.NewMockAuthService(t)
	397	+
	398	+	authSvc.EXPECT().IsEnabled().Return(true)
	399	+	authSvc.EXPECT().ValidateToken(mock.Anything, "old-token").Return(&domain.TokenClaims{
	400	+	Subject: "admin",
	401	+	Scopes: []string{"admin::"},
	402	+	}, nil)
	403	+	// ExtendToken returns a rotated token — must appear in response header
	404	+	authSvc.EXPECT().ExtendToken(mock.Anything, "old-token").Return("rotated-token", nil)
	405	+
	406	+	handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
	407	+	w.WriteHeader(http.StatusOK)
	408	+	})
	409	+
	410	+	mw := AuthMiddleware(authSvc, nil, nil, nil, adminTestLogger())
	411	+	wrappedHandler := mw(handler)
	412	+
	413	+	req := httptest.NewRequest(http.MethodGet, "/admin/status", nil)
	414	+	req.Header.Set("Authorization", "Bearer old-token")
	415	+	rec := httptest.NewRecorder()
	416	+	wrappedHandler.ServeHTTP(rec, req)
	417	+
	418	+	assert.Equal(t, http.StatusOK, rec.Code)
	419	+	assert.Equal(t, "rotated-token", rec.Header().Get("X-Gordon-Token"))
	420	+	}
	421	+
367	422		func TestHasAccess(t *testing.T) {
368	423		ctx := context.WithValue(context.Background(), domain.ContextKeyScopes, []string{"admin:routes:read", "admin:secrets:*"})
369	424


@@ -2,6 +2,7 @@ package admin
2		2
3	import (	3		import (
4	"context"	4		"context"
		5	+	"errors"
5	"net/http"	6		"net/http"
6	"net/http/httptest"	7		"net/http/httptest"
7	"testing"	8		"testing"

@@ -364,6 +365,60 @@ func TestRequireScope(t *testing.T) {
364	}	365		}
365	}	366		}
366		367
		368	+	func TestAuthMiddleware_ExtendTokenFailureIsNonFatal(t *testing.T) {
		369	+	authSvc := inmocks.NewMockAuthService(t)
		370	+
		371	+	authSvc.EXPECT().IsEnabled().Return(true)
		372	+	authSvc.EXPECT().ValidateToken(mock.Anything, "valid-admin-token").Return(&domain.TokenClaims{
		373	+	Subject: "admin",
		374	+	Scopes: []string{"admin::"},
		375	+	}, nil)
		376	+	// ExtendToken fails — response must still be 200 (non-fatal)
		377	+	authSvc.EXPECT().ExtendToken(mock.Anything, "valid-admin-token").Return("", errors.New("store unavailable"))
		378	+
		379	+	handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
		380	+	w.WriteHeader(http.StatusOK)
		381	+	})
		382	+
		383	+	mw := AuthMiddleware(authSvc, nil, nil, nil, adminTestLogger())
		384	+	wrappedHandler := mw(handler)
		385	+
		386	+	req := httptest.NewRequest(http.MethodGet, "/admin/status", nil)
		387	+	req.Header.Set("Authorization", "Bearer valid-admin-token")
		388	+	rec := httptest.NewRecorder()
		389	+	wrappedHandler.ServeHTTP(rec, req)
		390	+
		391	+	assert.Equal(t, http.StatusOK, rec.Code)
		392	+	assert.Empty(t, rec.Header().Get("X-Gordon-Token"))
		393	+	}
		394	+
		395	+	func TestAuthMiddleware_ExtendTokenRotationHeader(t *testing.T) {
		396	+	authSvc := inmocks.NewMockAuthService(t)
		397	+
		398	+	authSvc.EXPECT().IsEnabled().Return(true)
		399	+	authSvc.EXPECT().ValidateToken(mock.Anything, "old-token").Return(&domain.TokenClaims{
		400	+	Subject: "admin",
		401	+	Scopes: []string{"admin::"},
		402	+	}, nil)
		403	+	// ExtendToken returns a rotated token — must appear in response header
		404	+	authSvc.EXPECT().ExtendToken(mock.Anything, "old-token").Return("rotated-token", nil)
		405	+
		406	+	handler := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
		407	+	w.WriteHeader(http.StatusOK)
		408	+	})
		409	+
		410	+	mw := AuthMiddleware(authSvc, nil, nil, nil, adminTestLogger())
		411	+	wrappedHandler := mw(handler)
		412	+
		413	+	req := httptest.NewRequest(http.MethodGet, "/admin/status", nil)
		414	+	req.Header.Set("Authorization", "Bearer old-token")
		415	+	rec := httptest.NewRecorder()
		416	+	wrappedHandler.ServeHTTP(rec, req)
		417	+
		418	+	assert.Equal(t, http.StatusOK, rec.Code)
		419	+	assert.Equal(t, "rotated-token", rec.Header().Get("X-Gordon-Token"))
		420	+	}
		421	+
367	func TestHasAccess(t *testing.T) {	422		func TestHasAccess(t *testing.T) {
368	ctx := context.WithValue(context.Background(), domain.ContextKeyScopes, []string{"admin:routes:read", "admin:secrets:*"})	423		ctx := context.WithValue(context.Background(), domain.ContextKeyScopes, []string{"admin:routes:read", "admin:secrets:*"})
369		424

M internal/adapters/in/http/httputil/localhost.go +12 -6

@@ -1,16 +1,22 @@
1	1		package httputil
2	2
3	3		import (
	4	+	"net"
4	5		"net/http"
5		-	"strings"
	6	+	"net/netip"
6	7		)
7	8
8	9		// IsLocalhostRequest reports whether the request originates from localhost.
9	10		// SECURITY: Uses RemoteAddr (server-set) instead of Host header (client-spoofable).
10		-	// Go's net/http always formats RemoteAddr as "host:port" where IPv6 is bracketed:
11		-	// e.g., "127.0.0.1:12345" or "[::1]:12345".
12	11		func IsLocalhostRequest(r *http.Request) bool {
13		-	host := r.RemoteAddr
14		-	return strings.HasPrefix(host, "127.") \|\|
15		-	strings.HasPrefix(host, "[::1]")
	12	+	host, _, err := net.SplitHostPort(r.RemoteAddr)
	13	+	if err != nil {
	14	+	// RemoteAddr has no port (unusual but possible in tests/Unix sockets).
	15	+	host = r.RemoteAddr
	16	+	}
	17	+	addr, err := netip.ParseAddr(host)
	18	+	if err != nil {
	19	+	return false
	20	+	}
	21	+	return addr.IsLoopback()
16	22		}

 package httputil
 import (
+	"net"
 	"net/http"
-	"strings"
+	"net/netip"
 )
 // IsLocalhostRequest reports whether the request originates from localhost.
 // SECURITY: Uses RemoteAddr (server-set) instead of Host header (client-spoofable).
-// Go's net/http always formats RemoteAddr as "host:port" where IPv6 is bracketed:
-// e.g., "127.0.0.1:12345" or "[::1]:12345".
 func IsLocalhostRequest(r *http.Request) bool {
-	host := r.RemoteAddr
+	host, _, err := net.SplitHostPort(r.RemoteAddr)
-	return strings.HasPrefix(host, "127.") ||
+	if err != nil {
-		strings.HasPrefix(host, "[::1]")
+		// RemoteAddr has no port (unusual but possible in tests/Unix sockets).
+		host = r.RemoteAddr
+	}
+	addr, err := netip.ParseAddr(host)
+	if err != nil {
+		return false
+	}
+	return addr.IsLoopback()
 }

M internal/adapters/out/tokenstore/pass.go +17 -0


@@ -53,6 +53,10 @@ func validateSubject(subject string) error {
53	53		return fmt.Errorf("invalid subject: cannot contain '..' to prevent path traversal")
54	54		}
55	55
	56	+	if strings.HasSuffix(subject, ".meta") {
	57	+	return fmt.Errorf("invalid subject: cannot end with '.meta' (reserved for token metadata files)")
	58	+	}
	59	+
56	60		return nil
57	61		}
58	62

@@ -442,7 +446,20 @@ func (s *PassStore) IsRevoked(ctx context.Context, tokenID string) (bool, error)
442	446
443	447		// UpdateTokenExpiry updates the JWT and expiry metadata for an existing token.
444	448		// LastExtendedAt is also updated to track debounce timing.
	449	+	// UpdateTokenExpiry enforces update-only semantics: it returns an error if
	450	+	// token is nil or if no existing record is found for token.Subject.
445	451		func (s PassStore) UpdateTokenExpiry(ctx context.Context, token domain.Token, newJWT string) error {
	452	+	if token == nil {
	453	+	return fmt.Errorf("UpdateTokenExpiry: token must not be nil")
	454	+	}
	455	+	// Confirm the token exists before overwriting to avoid silent creation.
	456	+	_, existing, err := s.GetToken(ctx, token.Subject)
	457	+	if err != nil {
	458	+	return fmt.Errorf("UpdateTokenExpiry: token not found for subject %q: %w", token.Subject, err)
	459	+	}
	460	+	if existing == nil {
	461	+	return fmt.Errorf("UpdateTokenExpiry: no existing token for subject %q", token.Subject)
	462	+	}
446	463		return s.SaveToken(ctx, token, newJWT)
447	464		}
448	465


@@ -53,6 +53,10 @@ func validateSubject(subject string) error {
53	return fmt.Errorf("invalid subject: cannot contain '..' to prevent path traversal")	53		return fmt.Errorf("invalid subject: cannot contain '..' to prevent path traversal")
54	}	54		}
55		55
		56	+	if strings.HasSuffix(subject, ".meta") {
		57	+	return fmt.Errorf("invalid subject: cannot end with '.meta' (reserved for token metadata files)")
		58	+	}
		59	+
56	return nil	60		return nil
57	}	61		}
58		62

@@ -442,7 +446,20 @@ func (s *PassStore) IsRevoked(ctx context.Context, tokenID string) (bool, error)
442		446
443	// UpdateTokenExpiry updates the JWT and expiry metadata for an existing token.	447		// UpdateTokenExpiry updates the JWT and expiry metadata for an existing token.
444	// LastExtendedAt is also updated to track debounce timing.	448		// LastExtendedAt is also updated to track debounce timing.
		449	+	// UpdateTokenExpiry enforces update-only semantics: it returns an error if
		450	+	// token is nil or if no existing record is found for token.Subject.
445	func (s PassStore) UpdateTokenExpiry(ctx context.Context, token domain.Token, newJWT string) error {	451		func (s PassStore) UpdateTokenExpiry(ctx context.Context, token domain.Token, newJWT string) error {
		452	+	if token == nil {
		453	+	return fmt.Errorf("UpdateTokenExpiry: token must not be nil")
		454	+	}
		455	+	// Confirm the token exists before overwriting to avoid silent creation.
		456	+	_, existing, err := s.GetToken(ctx, token.Subject)
		457	+	if err != nil {
		458	+	return fmt.Errorf("UpdateTokenExpiry: token not found for subject %q: %w", token.Subject, err)
		459	+	}
		460	+	if existing == nil {
		461	+	return fmt.Errorf("UpdateTokenExpiry: no existing token for subject %q", token.Subject)
		462	+	}
446	return s.SaveToken(ctx, token, newJWT)	463		return s.SaveToken(ctx, token, newJWT)
447	}	464		}
448		465

M internal/adapters/out/tokenstore/unsafe_test.go +9 -2


@@ -46,16 +46,23 @@ func TestUnsafeStoreRevokeConcurrentNoLoss(t *testing.T) {
46	46		}
47	47		}
48	48
49		-	// Revoke all concurrently
	49	+	// Revoke all concurrently; collect errors via a buffered channel.
50	50		var wg sync.WaitGroup
	51	+	errs := make(chan error, len(tokenIDs))
51	52		for _, id := range tokenIDs {
52	53		wg.Add(1)
53	54		go func(id string) {
54	55		defer wg.Done()
55		-	_ = store.Revoke(ctx, id)
	56	+	if err := store.Revoke(ctx, id); err != nil {
	57	+	errs <- fmt.Errorf("Revoke(%q): %w", id, err)
	58	+	}
56	59		}(id)
57	60		}
58	61		wg.Wait()
	62	+	close(errs)
	63	+	for err := range errs {
	64	+	t.Error(err)
	65	+	}
59	66
60	67		// Verify all n token IDs are in the revoked list
61	68		for _, id := range tokenIDs {

 		}
 	}
-	// Revoke all concurrently
+	// Revoke all concurrently; collect errors via a buffered channel.
 	var wg sync.WaitGroup
+	errs := make(chan error, len(tokenIDs))
 	for _, id := range tokenIDs {
 		wg.Add(1)
 		go func(id string) {
 			defer wg.Done()
-			_ = store.Revoke(ctx, id)
+			if err := store.Revoke(ctx, id); err != nil {
+				errs <- fmt.Errorf("Revoke(%q): %w", id, err)
+			}
 		}(id)
 	}
 	wg.Wait()
+	close(errs)
+	for err := range errs {
+		t.Error(err)
+	}
 	// Verify all n token IDs are in the revoked list
 	for _, id := range tokenIDs {

M internal/app/run.go +11 -2


@@ -855,20 +855,29 @@ func getInternalCredentialsCandidates() []string {
855	855		// GetInternalCredentialsFromCandidates reads credentials from the first candidate file that exists.
856	856		// Exported for testing.
857	857		func GetInternalCredentialsFromCandidates(candidates []string) (*InternalCredentials, error) {
	858	+	var lastErr error
858	859		for _, path := range candidates {
859	860		data, err := os.ReadFile(path)
860	861		if os.IsNotExist(err) {
861	862		continue
862	863		}
863	864		if err != nil {
864		-	return nil, fmt.Errorf("failed to read credentials file %s: %w", path, err)
	865	+	// Non-permission errors (e.g. EACCES) may be transient or path-specific;
	866	+	// record and try the next candidate rather than failing immediately.
	867	+	lastErr = fmt.Errorf("failed to read credentials file %s: %w", path, err)
	868	+	continue
865	869		}
866	870		var creds InternalCredentials
867	871		if err := json.Unmarshal(data, &creds); err != nil {
868		-	return nil, fmt.Errorf("failed to parse credentials at %s: %w", path, err)
	872	+	// Corrupt file — record and fall through to lower-priority candidates.
	873	+	lastErr = fmt.Errorf("failed to parse credentials at %s: %w", path, err)
	874	+	continue
869	875		}
870	876		return &creds, nil
871	877		}
	878	+	if lastErr != nil {
	879	+	return nil, lastErr
	880	+	}
872	881		return nil, fmt.Errorf("no credentials file found (is Gordon running?): checked %v", candidates)
873	882		}
874	883

 // GetInternalCredentialsFromCandidates reads credentials from the first candidate file that exists.
 // Exported for testing.
 func GetInternalCredentialsFromCandidates(candidates []string) (*InternalCredentials, error) {
+	var lastErr error
 	for _, path := range candidates {
 		data, err := os.ReadFile(path)
 		if os.IsNotExist(err) {
 			continue
 		}
 		if err != nil {
-			return nil, fmt.Errorf("failed to read credentials file %s: %w", path, err)
+			// Non-permission errors (e.g. EACCES) may be transient or path-specific;
+			// record and try the next candidate rather than failing immediately.
+			lastErr = fmt.Errorf("failed to read credentials file %s: %w", path, err)
+			continue
 		}
 		var creds InternalCredentials
 		if err := json.Unmarshal(data, &creds); err != nil {
-			return nil, fmt.Errorf("failed to parse credentials at %s: %w", path, err)
+			// Corrupt file — record and fall through to lower-priority candidates.
+			lastErr = fmt.Errorf("failed to parse credentials at %s: %w", path, err)
+			continue
 		}
 		return &creds, nil
 	}
+	if lastErr != nil {
+		return nil, lastErr
+	}
 	return nil, fmt.Errorf("no credentials file found (is Gordon running?): checked %v", candidates)
 }

M internal/usecase/auth/service.go +12 -0


@@ -494,6 +494,18 @@ func (s *Service) ExtendToken(ctx context.Context, tokenString string) (string,
494	494		return "", fmt.Errorf("failed to fetch stored token: %w", err)
495	495		}
496	496
	497	+	// TOCTOU guard: confirm the store's current JTI matches the JWT we validated.
	498	+	// A mismatch means the token was rotated between ValidateToken and GetToken;
	499	+	// in that case return the original (still-valid) token without re-signing.
	500	+	if storedToken.ID != tokenClaims.ID {
	501	+	log.Debug().
	502	+	Str("subject", tokenClaims.Subject).
	503	+	Str("jwt_jti", tokenClaims.ID).
	504	+	Str("store_id", storedToken.ID).
	505	+	Msg("token ID mismatch after rotation, skipping extension")
	506	+	return tokenString, nil
	507	+	}
	508	+
497	509		// Debounce: skip if already extended within the last hour
498	510		if !storedToken.LastExtendedAt.IsZero() && time.Since(storedToken.LastExtendedAt) < tokenExtensionDebounce {
499	511		log.Debug().


@@ -494,6 +494,18 @@ func (s *Service) ExtendToken(ctx context.Context, tokenString string) (string,
494	return "", fmt.Errorf("failed to fetch stored token: %w", err)	494		return "", fmt.Errorf("failed to fetch stored token: %w", err)
495	}	495		}
496		496
		497	+	// TOCTOU guard: confirm the store's current JTI matches the JWT we validated.
		498	+	// A mismatch means the token was rotated between ValidateToken and GetToken;
		499	+	// in that case return the original (still-valid) token without re-signing.
		500	+	if storedToken.ID != tokenClaims.ID {
		501	+	log.Debug().
		502	+	Str("subject", tokenClaims.Subject).
		503	+	Str("jwt_jti", tokenClaims.ID).
		504	+	Str("store_id", storedToken.ID).
		505	+	Msg("token ID mismatch after rotation, skipping extension")
		506	+	return tokenString, nil
		507	+	}
		508	+
497	// Debounce: skip if already extended within the last hour	509		// Debounce: skip if already extended within the last hour
498	if !storedToken.LastExtendedAt.IsZero() && time.Since(storedToken.LastExtendedAt) < tokenExtensionDebounce {	510		if !storedToken.LastExtendedAt.IsZero() && time.Since(storedToken.LastExtendedAt) < tokenExtensionDebounce {
499	log.Debug().	511		log.Debug().

M internal/usecase/container/service.go +11 -1


@@ -1073,9 +1073,19 @@ func (s *Service) UpdateConfig(config Config) {
1073	1073
1074	1074		// UpdateAttachments updates only the attachment configuration in the service.
1075	1075		// This is called after a config reload to propagate attachment changes without restart.
	1076	+	// The incoming map is deep-copied so external callers cannot mutate service state.
1076	1077		func (s *Service) UpdateAttachments(attachments map[string][]string) {
	1078	+	var copied map[string][]string
	1079	+	if attachments != nil {
	1080	+	copied = make(map[string][]string, len(attachments))
	1081	+	for k, v := range attachments {
	1082	+	sl := make([]string, len(v))
	1083	+	copy(sl, v)
	1084	+	copied[k] = sl
	1085	+	}
	1086	+	}
1077	1087		s.mu.Lock()
1078		-	s.config.Attachments = attachments
	1088	+	s.config.Attachments = copied
1079	1089		s.mu.Unlock()
1080	1090		}
1081	1091


@@ -1073,9 +1073,19 @@ func (s *Service) UpdateConfig(config Config) {
1073			1073
1074		// UpdateAttachments updates only the attachment configuration in the service.	1074		// UpdateAttachments updates only the attachment configuration in the service.
1075		// This is called after a config reload to propagate attachment changes without restart.	1075		// This is called after a config reload to propagate attachment changes without restart.
			1076	+	// The incoming map is deep-copied so external callers cannot mutate service state.
1076		func (s *Service) UpdateAttachments(attachments map[string][]string) {	1077		func (s *Service) UpdateAttachments(attachments map[string][]string) {
			1078	+	var copied map[string][]string
			1079	+	if attachments != nil {
			1080	+	copied = make(map[string][]string, len(attachments))
			1081	+	for k, v := range attachments {
			1082	+	sl := make([]string, len(v))
			1083	+	copy(sl, v)
			1084	+	copied[k] = sl
			1085	+	}
			1086	+	}
1077		s.mu.Lock()	1087		s.mu.Lock()
1078	-	s.config.Attachments = attachments	1088	+	s.config.Attachments = copied
1079		s.mu.Unlock()	1089		s.mu.Unlock()
1080		}	1090		}
1081			1091