fix(accounts): the refresh cookie crosses the REST transcoder as Set-Cookie

module/services/accounts/code/pkg/adapters/rest_extras.go

@@ -20,6 +20,8 @@ import (
 	"strings"
 	"sync"
 	"time"
+
+	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
 )
 
 // refreshTokenCookieName is the httpOnly cookie that carries the long-lived
@@ -270,6 +272,26 @@ func refreshCookie(token string, maxAge int) *http.Cookie {
 	}
 }
 
+// restOutgoingHeaderMatcher maps the Connect handler's response headers onto
+// the REST response. The transcoder speaks gRPC to the Connect surface, so every
+// header a handler sets arrives as header metadata; grpc-gateway's default writes
+// each one back as Grpc-Metadata-<key>.
+//
+// Set-Cookie is the one that must keep its own name. The authentication
+// completions lift the refresh token into the cookie on the Connect side
+// (cookieTheRefreshToken) and clear it from the body, so the REST middleware has
+// no body token left to lift. Prefixed, the cookie was never stored — every
+// browser sign-in through REST lost its session on the next page load — and the
+// refresh credential travelled in a header page script CAN read, which is
+// exactly what the HttpOnly cookie exists to prevent. Every other header keeps
+// grpc-gateway's default spelling.
+func restOutgoingHeaderMatcher(key string) (string, bool) {
+	if strings.EqualFold(key, "set-cookie") {
+		return "Set-Cookie", true
+	}
+	return runtime.MetadataHeaderPrefix + key, true
+}
+
 var (
 	extraRoutesMu sync.RWMutex
 	extraRoutes   = map[string]http.Handler{}

module/services/accounts/code/pkg/adapters/rest_gen.go

@@ -70,6 +70,11 @@ func (s *RestServer) Run(ctx context.Context) error {
 		// across the transcoding hop. See restIdentityHeaderMatcher.
 		// Preserve this line when regenerating.
 		runtime.WithIncomingHeaderMatcher(restIdentityHeaderMatcher),
+		// CUSTOM: a Set-Cookie the Connect handler sets (the refresh cookie)
+		// reaches the browser as Set-Cookie, never as a JS-readable
+		// Grpc-Metadata-Set-Cookie. See restOutgoingHeaderMatcher.
+		// Preserve this line when regenerating.
+		runtime.WithOutgoingHeaderMatcher(restOutgoingHeaderMatcher),
 		runtime.WithErrorHandler(customErrorHandler))
 
 	opts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}

module/services/accounts/code/pkg/adapters/rest_refresh_cookie_chain_test.go

@@ -0,0 +1,146 @@
+package adapters
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync"
+	"testing"
+
+	"connectrpc.com/connect"
+	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
+	"golang.org/x/net/http2"
+	"golang.org/x/net/http2/h2c"
+	"google.golang.org/grpc"
+	"google.golang.org/grpc/credentials/insecure"
+	"google.golang.org/protobuf/types/known/emptypb"
+
+	gen "accounts/pkg/gen/saas/accounts/v1"
+	"accounts/pkg/gen/saas/accounts/v1/accountsv1connect"
+)
+
+// chainAuthHandler serves the three cookie-carrying AuthService methods exactly
+// as authConnectHandler does — through the production lift and cookie readers —
+// with the business layer replaced by fixed answers.
+type chainAuthHandler struct {
+	accountsv1connect.UnimplementedAuthServiceHandler
+	mu       sync.Mutex
+	received map[string]string // method -> refresh token the handler saw
+}
+
+func (h *chainAuthHandler) saw(method, token string) {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	h.received[method] = token
+}
+
+func (h *chainAuthHandler) Authenticate(ctx context.Context, req *connect.Request[gen.AuthenticateRequest]) (*connect.Response[gen.AuthenticateResponse], error) {
+	return unaryCookieingRefreshToken(ctx, req, func(context.Context, *gen.AuthenticateRequest) (*gen.AuthenticateResponse, error) {
+		return &gen.AuthenticateResponse{AccessToken: "access-1", RefreshToken: "refresh-1"}, nil
+	})
+}
+
+func (h *chainAuthHandler) RefreshToken(ctx context.Context, req *connect.Request[gen.RefreshTokenRequest]) (*connect.Response[gen.RefreshTokenResponse], error) {
+	refreshTokenFromCookie(req.Header(), req.Msg.GetRefreshToken(), func(token string) { req.Msg.RefreshToken = token })
+	return unaryCookieingRefreshToken(ctx, req, func(_ context.Context, msg *gen.RefreshTokenRequest) (*gen.RefreshTokenResponse, error) {
+		h.saw("refresh", msg.GetRefreshToken())
+		return &gen.RefreshTokenResponse{AccessToken: "access-2", RefreshToken: "refresh-2"}, nil
+	})
+}
+
+func (h *chainAuthHandler) Logout(ctx context.Context, req *connect.Request[gen.LogoutRequest]) (*connect.Response[emptypb.Empty], error) {
+	refreshTokenFromCookie(req.Header(), req.Msg.GetRefreshToken(), func(token string) { req.Msg.RefreshToken = token })
+	return unary(ctx, req, func(_ context.Context, msg *gen.LogoutRequest) (*emptypb.Empty, error) {
+		h.saw("logout", msg.GetRefreshToken())
+		return &emptypb.Empty{}, nil
+	})
+}
+
+// TestABrowserSessionSurvivesTheRealRESTChain drives the whole path a browser's
+// sign-in takes: REST -> the cookie middleware -> grpc-gateway (with the mux
+// options rest_gen.go builds) -> gRPC over h2c -> the Connect AuthService with
+// the production cookie lift. #1019 shipped a REST sign-in whose cookie never
+// reached the browser because each half was tested alone; this is the joined
+// test. It asserts, for authenticate, refresh and logout: exactly one Set-Cookie
+// for the refresh credential, no token in any body, nothing prefixed, and that
+// the cookie a browser would send back is what the handler receives.
+func TestABrowserSessionSurvivesTheRealRESTChain(t *testing.T) {
+	handler := &chainAuthHandler{received: map[string]string{}}
+	connectMux := http.NewServeMux()
+	connectMux.Handle(accountsv1connect.NewAuthServiceHandler(handler))
+	backend := httptest.NewServer(h2c.NewHandler(connectMux, &http2.Server{}))
+	t.Cleanup(backend.Close)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+	gwMux := runtime.NewServeMux(
+		runtime.WithIncomingHeaderMatcher(restIdentityHeaderMatcher),
+		runtime.WithOutgoingHeaderMatcher(restOutgoingHeaderMatcher),
+	)
+	endpoint := strings.TrimPrefix(backend.URL, "http://")
+	if err := gen.RegisterAuthServiceHandlerFromEndpoint(ctx, gwMux, endpoint,
+		[]grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}); err != nil {
+		t.Fatal(err)
+	}
+	rest := httptest.NewServer(refreshTokenCookie(gwMux))
+	t.Cleanup(rest.Close)
+
+	post := func(path string, cookie *http.Cookie) *http.Response {
+		t.Helper()
+		req, _ := http.NewRequest(http.MethodPost, rest.URL+path, strings.NewReader(`{}`))
+		req.Header.Set("Content-Type", "application/json")
+		if cookie != nil {
+			req.AddCookie(cookie)
+		}
+		resp, err := http.DefaultClient.Do(req)
+		if err != nil {
+			t.Fatal(err)
+		}
+		t.Cleanup(func() { resp.Body.Close() })
+		return resp
+	}
+	onlyCookie := func(path string, resp *http.Response) *http.Cookie {
+		t.Helper()
+		if resp.StatusCode != http.StatusOK {
+			body, _ := io.ReadAll(resp.Body)
+			t.Fatalf("%s: HTTP %d %s", path, resp.StatusCode, body)
+		}
+		for name := range resp.Header {
+			if strings.HasPrefix(strings.ToLower(name), strings.ToLower(runtime.MetadataHeaderPrefix)) &&
+				strings.Contains(strings.ToLower(name), "cookie") {
+				t.Fatalf("%s: a cookie left as %s, readable by page script", path, name)
+			}
+		}
+		cookies := resp.Cookies()
+		if len(cookies) != 1 || cookies[0].Name != refreshTokenCookieName {
+			t.Fatalf("%s: want exactly one %s cookie, got %v", path, refreshTokenCookieName, cookies)
+		}
+		var body map[string]any
+		_ = json.NewDecoder(resp.Body).Decode(&body)
+		for _, key := range []string{"refreshToken", "refresh_token"} {
+			if v, ok := body[key].(string); ok && v != "" {
+				t.Fatalf("%s: the refresh credential is readable in the body", path)
+			}
+		}
+		return cookies[0]
+	}
+
+	signedIn := onlyCookie("/v1/auth/authenticate", post("/v1/auth/authenticate", nil))
+	if signedIn.Value != "refresh-1" || !signedIn.HttpOnly || signedIn.SameSite != http.SameSiteStrictMode || signedIn.Path != "/v1/auth" {
+		t.Fatalf("authenticate cookie = %+v", signedIn)
+	}
+	rotated := onlyCookie("/v1/auth/refresh", post("/v1/auth/refresh", signedIn))
+	if rotated.Value != "refresh-2" {
+		t.Fatalf("refresh did not rotate the cookie: %+v", rotated)
+	}
+	cleared := onlyCookie("/v1/auth/logout", post("/v1/auth/logout", rotated))
+	if cleared.Value != "" || cleared.MaxAge >= 0 {
+		t.Fatalf("logout did not clear the cookie: %+v", cleared)
+	}
+	if handler.received["refresh"] != "refresh-1" || handler.received["logout"] != "refresh-2" {
+		t.Fatalf("the cookie a browser sends back did not reach the handler: %v", handler.received)
+	}
+}

module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go

@@ -0,0 +1,99 @@
+package adapters
+
+import (
+	"context"
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/grpc-ecosystem/grpc-gateway/v2/runtime"
+	"google.golang.org/grpc/metadata"
+	"google.golang.org/protobuf/types/known/emptypb"
+)
+
+// The refresh cookie the Connect handler sets crosses the REST transcoder as
+// header metadata. Through the matcher the REST mux is built with, it must
+// reach the browser as Set-Cookie — storable and HttpOnly — and never as
+// Grpc-Metadata-Set-Cookie, which a browser does not store and page script can
+// read. Before the matcher, every browser sign-in through REST lost its session
+// on the next load.
+func TestTheRefreshCookieCrossesTheRESTTranscoderAsSetCookie(t *testing.T) {
+	cookie := refreshCookie("rt-value", refreshCookieMaxAgeSeconds).String()
+	recorder := forwardThroughTranscoder(t, runtime.WithOutgoingHeaderMatcher(restOutgoingHeaderMatcher),
+		metadata.Pairs("set-cookie", cookie, "x-ratelimit-limit", "60"))
+
+	if got := recorder.Header().Values("Set-Cookie"); len(got) != 1 || got[0] != cookie {
+		t.Fatalf("Set-Cookie = %q, want the refresh cookie %q", got, cookie)
+	}
+	for name := range recorder.Header() {
+		if strings.EqualFold(name, runtime.MetadataHeaderPrefix+"set-cookie") {
+			t.Fatalf("the refresh cookie also leaked as %s, readable by page script", name)
+		}
+	}
+	// Every other header keeps grpc-gateway's default spelling.
+	if recorder.Header().Get(runtime.MetadataHeaderPrefix+"x-ratelimit-limit") != "60" {
+		t.Fatalf("other metadata lost its default prefix: %v", recorder.Header())
+	}
+}
+
+// The failure this guards, shown on the default matcher: the cookie arrives
+// prefixed, and no Set-Cookie is written at all.
+func TestTheDefaultTranscoderPrefixesTheCookie(t *testing.T) {
+	recorder := forwardThroughTranscoder(t, nil,
+		metadata.Pairs("set-cookie", refreshCookie("rt-value", 60).String()))
+	if len(recorder.Header().Values("Set-Cookie")) != 0 ||
+		recorder.Header().Get(runtime.MetadataHeaderPrefix+"set-cookie") == "" {
+		t.Fatalf("expected the default transcoder to prefix the cookie, got %v", recorder.Header())
+	}
+}
+
+func forwardThroughTranscoder(t *testing.T, option runtime.ServeMuxOption, header metadata.MD) *httptest.ResponseRecorder {
+	t.Helper()
+	var options []runtime.ServeMuxOption
+	if option != nil {
+		options = append(options, option)
+	}
+	mux := runtime.NewServeMux(options...)
+	ctx := runtime.NewServerMetadataContext(context.Background(), runtime.ServerMetadata{HeaderMD: header})
+	request := httptest.NewRequest(http.MethodPost, "/v1/auth/authenticate", strings.NewReader(`{}`))
+	recorder := httptest.NewRecorder()
+	_, marshaler := runtime.MarshalerForRequest(mux, request)
+	runtime.ForwardResponseMessage(ctx, mux, marshaler, recorder, request, &emptypb.Empty{})
+	return recorder
+}
+
+// rest_gen.go is generated; its CUSTOM lines survive regeneration only by
+// being preserved. Parsed, not string-matched (as the incoming matcher's
+// lockstep test does), so a commented-out or renamed option fails here and the
+// prefixed cookie cannot come back silently.
+func TestTheRESTMuxIsBuiltWithTheOutgoingHeaderMatcher(t *testing.T) {
+	fset := token.NewFileSet()
+	f, err := parser.ParseFile(fset, "rest_gen.go", nil, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	wired := false
+	ast.Inspect(f, func(n ast.Node) bool {
+		call, isCall := n.(*ast.CallExpr)
+		if !isCall || !isSelector(call.Fun, "runtime", "NewServeMux") {
+			return true
+		}
+		for _, arg := range call.Args {
+			option, isCall := arg.(*ast.CallExpr)
+			if !isCall || !isSelector(option.Fun, "runtime", "WithOutgoingHeaderMatcher") || len(option.Args) != 1 {
+				continue
+			}
+			if name, isIdent := option.Args[0].(*ast.Ident); isIdent && name.Name == "restOutgoingHeaderMatcher" {
+				wired = true
+			}
+		}
+		return true
+	})
+	if !wired {
+		t.Fatal("rest_gen.go must pass runtime.WithOutgoingHeaderMatcher(restOutgoingHeaderMatcher) to runtime.NewServeMux: without it the refresh cookie leaves as Grpc-Metadata-Set-Cookie")
+	}
+}