From 9c58283d8e7a036a45820f76da7f05ffcf81d611 Mon Sep 17 00:00:00 2001 From: dylanhuff-at-coder Date: Tue, 15 Sep 2026 11:18:54 -0400 Subject: [PATCH] fix(coderd/agentapi): reject missing RPC messages (#29150) Reject `UpdateLifecycle`, `UpdateStartup`, and `BatchUpdateMetadata` requests with a missing sub-message using `InvalidArgument` instead of panicking. The whole metadata batch is validated before any value is trimmed or queued. Adds tests for each handler, a mixed metadata batch, and one DRPC round-trip confirming the code propagates and the connection stays usable. Refs PLAT-596 Reviewed and updated by Coder Agents on behalf of @dylanhuff-at-coder. --- coderd/agentapi/lifecycle.go | 10 ++++ coderd/agentapi/metadata.go | 12 ++++ coderd/agentapi/validation_test.go | 90 ++++++++++++++++++++++++++++++ 3 files changed, 112 insertions(+) create mode 100644 coderd/agentapi/validation_test.go diff --git a/coderd/agentapi/lifecycle.go b/coderd/agentapi/lifecycle.go index 5003a16f04..d9b1aa296a 100644 --- a/coderd/agentapi/lifecycle.go +++ b/coderd/agentapi/lifecycle.go @@ -10,7 +10,9 @@ import ( "github.com/google/uuid" "golang.org/x/mod/semver" "golang.org/x/xerrors" + "google.golang.org/grpc/codes" "google.golang.org/protobuf/types/known/timestamppb" + "storj.io/drpc/drpcerr" "cdr.dev/slog/v3" agentproto "github.com/coder/coder/v2/agent/proto" @@ -45,6 +47,10 @@ func (a *LifecycleAPI) now() time.Time { } func (a *LifecycleAPI) UpdateLifecycle(ctx context.Context, req *agentproto.UpdateLifecycleRequest) (*agentproto.Lifecycle, error) { + if req.GetLifecycle() == nil { + return nil, drpcerr.WithCode(xerrors.New("lifecycle is required"), uint64(codes.InvalidArgument)) + } + workspaceAgent, err := a.AgentFn(ctx) if err != nil { return nil, err @@ -146,6 +152,10 @@ func (a *LifecycleAPI) UpdateLifecycle(ctx context.Context, req *agentproto.Upda } func (a *LifecycleAPI) UpdateStartup(ctx context.Context, req *agentproto.UpdateStartupRequest) (*agentproto.Startup, error) { + if req.GetStartup() == nil { + return nil, drpcerr.WithCode(xerrors.New("startup is required"), uint64(codes.InvalidArgument)) + } + apiVersion, ok := ctx.Value(contextKeyAPIVersion{}).(string) if !ok { return nil, xerrors.Errorf("internal error; api version unspecified") diff --git a/coderd/agentapi/metadata.go b/coderd/agentapi/metadata.go index 12efe362ab..8baee8eb3f 100644 --- a/coderd/agentapi/metadata.go +++ b/coderd/agentapi/metadata.go @@ -8,6 +8,8 @@ import ( "github.com/google/uuid" "golang.org/x/xerrors" + "google.golang.org/grpc/codes" + "storj.io/drpc/drpcerr" "cdr.dev/slog/v3" agentproto "github.com/coder/coder/v2/agent/proto" @@ -34,6 +36,16 @@ func (a *MetadataAPI) now() time.Time { } func (a *MetadataAPI) BatchUpdateMetadata(ctx context.Context, req *agentproto.BatchUpdateMetadataRequest) (*agentproto.BatchUpdateMetadataResponse, error) { + if req == nil { + return nil, drpcerr.WithCode(xerrors.New("metadata request is required"), uint64(codes.InvalidArgument)) + } + // Validate the entire batch before trimming values or discarding excess keys. + for i, md := range req.Metadata { + if md.GetResult() == nil { + return nil, drpcerr.WithCode(xerrors.Errorf("metadata result at index %d is required", i), uint64(codes.InvalidArgument)) + } + } + const ( // maxAllKeysLen is the maximum length of all metadata keys. This is // 6144 to stay below the Postgres NOTIFY limit of 8000 bytes, with some diff --git a/coderd/agentapi/validation_test.go b/coderd/agentapi/validation_test.go new file mode 100644 index 0000000000..2a32556b21 --- /dev/null +++ b/coderd/agentapi/validation_test.go @@ -0,0 +1,90 @@ +package agentapi_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "storj.io/drpc/drpcerr" + + agentproto "github.com/coder/coder/v2/agent/proto" + "github.com/coder/coder/v2/coderd/agentapi" + "github.com/coder/coder/v2/codersdk/drpcsdk" + "github.com/coder/coder/v2/testutil" +) + +func TestUpdateLifecycleMissingMessage(t *testing.T) { + t.Parallel() + + resp, err := (&agentapi.LifecycleAPI{}).UpdateLifecycle(context.Background(), &agentproto.UpdateLifecycleRequest{}) + require.ErrorContains(t, err, "lifecycle is required") + require.EqualValues(t, codes.InvalidArgument, drpcerr.Code(err)) + require.Nil(t, resp) +} + +func TestUpdateStartupMissingMessage(t *testing.T) { + t.Parallel() + + resp, err := (&agentapi.LifecycleAPI{}).UpdateStartup(context.Background(), &agentproto.UpdateStartupRequest{}) + require.ErrorContains(t, err, "startup is required") + require.EqualValues(t, codes.InvalidArgument, drpcerr.Code(err)) + require.Nil(t, resp) +} + +func TestBatchUpdateMetadataMissingMessage(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + req *agentproto.BatchUpdateMetadataRequest + }{ + {name: "NilRequest"}, + {name: "MixedBatch", req: &agentproto.BatchUpdateMetadataRequest{Metadata: []*agentproto.Metadata{ + {Key: "valid", Result: &agentproto.WorkspaceAgentMetadata_Result{Value: "value"}}, + {Key: "invalid"}, + }}}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + resp, err := (&agentapi.MetadataAPI{}).BatchUpdateMetadata(context.Background(), tt.req) + require.Error(t, err) + require.EqualValues(t, codes.InvalidArgument, drpcerr.Code(err)) + require.Nil(t, resp) + }) + } +} + +func TestAgentRPCMissingMessage(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + serverCtx, cancel := context.WithCancel(agentapi.WithAPIVersion(ctx, "2.0")) + defer cancel() + conn, listener := drpcsdk.MemTransportPipe() + defer conn.Close() + defer listener.Close() + + api := &agentapi.API{ + LifecycleAPI: &agentapi.LifecycleAPI{}, + StatsAPI: &agentapi.StatsAPI{}, + } + server, err := api.Server(serverCtx) + require.NoError(t, err) + done := make(chan error, 1) + go func() { + done <- server.Serve(serverCtx, listener) + }() + client := agentproto.NewDRPCAgentClient(conn) + + _, err = client.UpdateLifecycle(ctx, &agentproto.UpdateLifecycleRequest{}) + require.ErrorContains(t, err, "lifecycle is required") + require.EqualValues(t, codes.InvalidArgument, drpcerr.Code(err)) + stats, err := client.UpdateStats(ctx, &agentproto.UpdateStatsRequest{}) + require.NoError(t, err) + require.NotNil(t, stats.ReportInterval) + + cancel() + require.NoError(t, testutil.RequireReceive(ctx, t, done)) +}