diff --git a/module/services/accounts/code/pkg/adapters/rest_extras.go b/module/services/accounts/code/pkg/adapters/rest_extras.go index 23c528636..3b525abf7 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 @@ -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-. +// +// 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{} 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_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 new file mode 100644 index 000000000..b8265c078 --- /dev/null +++ b/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") + } +}