From 7a9c45ac4118bfedb7bf7326f8c1a719f1f3fa44 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 18 Oct 2026 11:59:34 -0400 Subject: [PATCH 1/2] fix(accounts): the refresh cookie crosses the REST transcoder as Set-Cookie MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the Connect authentication completions lift the refresh token into the cookie themselves (cookieTheRefreshToken, #1019) and clear it from the body, the REST surface's cookie came back through grpc-gateway as header metadata, which the default matcher writes as Grpc-Metadata-Set-Cookie. A browser stores no cookie from that header and the REST middleware had no body token left to lift, so every browser sign-in lost its session on the next page load and was sent back to sign-in — and the refresh credential travelled in a header page script can read. restOutgoingHeaderMatcher keeps Set-Cookie's name across the transcoder; every other header keeps grpc-gateway's default prefix. Tests drive the transcoder's own response forwarding with and without the matcher, and hold the CUSTOM wiring line in the generated rest_gen.go. Co-Authored-By: Claude Opus 5.5 --- .../accounts/code/pkg/adapters/rest_extras.go | 22 ++++++ .../accounts/code/pkg/adapters/rest_gen.go | 5 ++ .../rest_refresh_cookie_transcoding_test.go | 78 +++++++++++++++++++ 3 files changed, 105 insertions(+) create mode 100644 module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go diff --git a/module/services/accounts/code/pkg/adapters/rest_extras.go b/module/services/accounts/code/pkg/adapters/rest_extras.go index 23c528636..30bfd8e7a 100644 --- a/module/services/accounts/code/pkg/adapters/rest_extras.go +++ b/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 @@ -258,6 +260,26 @@ func clearRefreshCookie(w http.ResponseWriter) { // refreshCookie is the one shape the refresh credential is ever handed to a // browser in: scoped to the auth routes, not script-readable, same-site, and // transport-protected outside local development. +// 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-. +// +// 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 +} + func refreshCookie(token string, maxAge int) *http.Cookie { return &http.Cookie{ Name: refreshTokenCookieName, diff --git a/module/services/accounts/code/pkg/adapters/rest_gen.go b/module/services/accounts/code/pkg/adapters/rest_gen.go index 375852548..66defa71d 100644 --- a/module/services/accounts/code/pkg/adapters/rest_gen.go +++ b/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())} diff --git a/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go b/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go new file mode 100644 index 000000000..e79c065b9 --- /dev/null +++ b/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go @@ -0,0 +1,78 @@ +package adapters + +import ( + "context" + "net/http" + "net/http/httptest" + "os" + "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. A regeneration that drops the matcher would silently bring +// the prefixed cookie back, so the wiring itself is held here. +func TestTheRESTMuxIsBuiltWithTheOutgoingHeaderMatcher(t *testing.T) { + source, err := os.ReadFile("rest_gen.go") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(source), "runtime.WithOutgoingHeaderMatcher(restOutgoingHeaderMatcher)") { + t.Fatal("rest_gen.go no longer builds the REST mux with restOutgoingHeaderMatcher: the refresh cookie would leave as Grpc-Metadata-Set-Cookie") + } +} From b66a7c7ba421d759410b5a416d6146de58dc0210 Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Sun, 18 Oct 2026 12:08:08 -0400 Subject: [PATCH 2/2] test(accounts): the refresh cookie is held across the real REST chain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #1150: the transcoder test fed hand-built metadata, so the joined path was still untested — the gap #1019 shipped through. The chain test now drives REST -> refreshTokenCookie -> grpc-gateway (the matchers rest_gen.go wires) -> gRPC over h2c -> a Connect AuthService using the production lift and cookie readers, for authenticate, refresh and logout: exactly one codefly_rt Set-Cookie, no token in any body, nothing prefixed, and the cookie a browser sends back is what the handler receives. Without the matcher it fails with the cell's symptom. The wiring check parses rest_gen.go like the incoming matcher's lockstep test (a commented-out option no longer passes), and the matcher no longer sits between refreshCookie and its doc comment. Co-Authored-By: Claude Opus 5.5 --- .../accounts/code/pkg/adapters/rest_extras.go | 24 +-- .../rest_refresh_cookie_chain_test.go | 146 ++++++++++++++++++ .../rest_refresh_cookie_transcoding_test.go | 33 +++- 3 files changed, 185 insertions(+), 18 deletions(-) create mode 100644 module/services/accounts/code/pkg/adapters/rest_refresh_cookie_chain_test.go diff --git a/module/services/accounts/code/pkg/adapters/rest_extras.go b/module/services/accounts/code/pkg/adapters/rest_extras.go index 30bfd8e7a..3b525abf7 100644 --- a/module/services/accounts/code/pkg/adapters/rest_extras.go +++ b/module/services/accounts/code/pkg/adapters/rest_extras.go @@ -260,6 +260,18 @@ func clearRefreshCookie(w http.ResponseWriter) { // refreshCookie is the one shape the refresh credential is ever handed to a // browser in: scoped to the auth routes, not script-readable, same-site, and // transport-protected outside local development. +func refreshCookie(token string, maxAge int) *http.Cookie { + return &http.Cookie{ + Name: refreshTokenCookieName, + Value: token, + Path: "/v1/auth", + HttpOnly: true, + Secure: secureRefreshCookie(), + SameSite: http.SameSiteStrictMode, + MaxAge: maxAge, + } +} + // 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 @@ -280,18 +292,6 @@ func restOutgoingHeaderMatcher(key string) (string, bool) { return runtime.MetadataHeaderPrefix + key, true } -func refreshCookie(token string, maxAge int) *http.Cookie { - return &http.Cookie{ - Name: refreshTokenCookieName, - Value: token, - Path: "/v1/auth", - HttpOnly: true, - Secure: secureRefreshCookie(), - SameSite: http.SameSiteStrictMode, - MaxAge: maxAge, - } -} - var ( extraRoutesMu sync.RWMutex extraRoutes = map[string]http.Handler{} diff --git a/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_chain_test.go b/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_chain_test.go new file mode 100644 index 000000000..c0bd035f6 --- /dev/null +++ b/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) + } +} diff --git a/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go b/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go index e79c065b9..b8265c078 100644 --- a/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go +++ b/module/services/accounts/code/pkg/adapters/rest_refresh_cookie_transcoding_test.go @@ -2,9 +2,11 @@ package adapters import ( "context" + "go/ast" + "go/parser" + "go/token" "net/http" "net/http/httptest" - "os" "strings" "testing" @@ -65,14 +67,33 @@ func forwardThroughTranscoder(t *testing.T, option runtime.ServeMuxOption, heade } // rest_gen.go is generated; its CUSTOM lines survive regeneration only by -// being preserved. A regeneration that drops the matcher would silently bring -// the prefixed cookie back, so the wiring itself is held here. +// 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) { - source, err := os.ReadFile("rest_gen.go") + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, "rest_gen.go", nil, 0) if err != nil { t.Fatal(err) } - if !strings.Contains(string(source), "runtime.WithOutgoingHeaderMatcher(restOutgoingHeaderMatcher)") { - t.Fatal("rest_gen.go no longer builds the REST mux with restOutgoingHeaderMatcher: the refresh cookie would leave as Grpc-Metadata-Set-Cookie") + 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") } }