Repository: go-git/go-git
Pull request #1487 (2025-04-06 09:05:57)

[v6] plumbing: fix, prevent double decoding of commands and flush
This refactors the decoding of commands and capabilities in the update-requests decoder to avoid double decoding the commands and eating the next pktlines in the reader.

The issue was that the decoder doesn't stop once it reaches a flush packet and continues to read the next line. This is incorrect as an update-requests always expects a flush packet after the commands have been sent.


Patch:
diff --git a/options.go b/options.go
index 6ec6dab0..9040c837 100644
--- a/options.go
+++ b/options.go
@@ -286,7 +286,7 @@ type PushOptions struct {
 	// ForceWithLease allows a force push as long as the remote ref adheres to a "lease"
 	ForceWithLease *ForceWithLease
 	// PushOptions sets options to be transferred to the server during push.
-	Options map[string]string
+	Options []string
 	// Atomic sets option to be an atomic push
 	Atomic bool
 	// ProxyOptions provides info required for connecting to a proxy.
diff --git a/plumbing/protocol/packp/pushopts.go b/plumbing/protocol/packp/pushopts.go
new file mode 100644
index 00000000..6606fa71
--- /dev/null
+++ b/plumbing/protocol/packp/pushopts.go
@@ -0,0 +1,50 @@
+package packp
+
+import (
+	"io"
+
+	"github.com/go-git/go-git/v6/plumbing/format/pktline"
+)
+
+// PushOptions represents a list of update request push-options.
+//
+// See https://git-scm.com/docs/gitprotocol-pack#_reference_update_request_and_packfile_transfer
+type PushOptions struct {
+	Options []string
+}
+
+// Encode encodes the push options into the given writer.
+func (opts *PushOptions) Encode(w io.Writer) error {
+	if len(opts.Options) == 0 {
+		return nil
+	}
+
+	for _, opt := range opts.Options {
+		if _, err := pktline.Writef(w, "%s", opt); err != nil {
+			return err
+		}
+	}
+
+	return pktline.WriteFlush(w)
+}
+
+// Decode decodes the push options from the given reader.
+func (opts *PushOptions) Decode(r io.Reader) error {
+	if opts.Options == nil {
+		opts.Options = make([]string, 0)
+	}
+
+	for {
+		l, line, err := pktline.ReadLine(r)
+		if err != nil {
+			return err
+		}
+		if l == pktline.Flush {
+			break
+		}
+
+		opts.Options = append(opts.Options, string(line))
+	}
+
+	return nil
+}
diff --git a/plumbing/protocol/packp/updreq.go b/plumbing/protocol/packp/updreq.go
index a5a6efe1..e52f2748 100644
--- a/plumbing/protocol/packp/updreq.go
+++ b/plumbing/protocol/packp/updreq.go
@@ -17,8 +17,8 @@ var (
 type UpdateRequests struct {
 	Capabilities *capability.List
 	Commands     []*Command
-	Options      []*Option
 	Shallow      *plumbing.Hash
+	// TODO: Support push-cert
 }
 
 // NewUpdateRequests returns a new UpdateRequests.
@@ -81,8 +81,3 @@ func (c *Command) validate() error {
 
 	return nil
 }
-
-type Option struct {
-	Key   string
-	Value string
-}
diff --git a/plumbing/protocol/packp/updreq_decode.go b/plumbing/protocol/packp/updreq_decode.go
index e0408cd8..52a709cf 100644
--- a/plumbing/protocol/packp/updreq_decode.go
+++ b/plumbing/protocol/packp/updreq_decode.go
@@ -21,6 +21,7 @@ var (
 	ErrEmpty                        = errors.New("empty update-request message")
 	errNoCommands                   = errors.New("unexpected EOF before any command")
 	errMissingCapabilitiesDelimiter = errors.New("capabilities delimiter not found")
+	errNoFlush                      = errors.New("unexpected EOF before flush line")
 )
 
 func errMalformedRequest(reason string) error {
@@ -101,7 +102,6 @@ func (d *updReqDecoder) Decode(req *UpdateRequests) error {
 	funcs := []func() error{
 		d.scanLine,
 		d.decodeShallow,
-		d.decodeCommandAndCapabilities,
 		d.decodeCommands,
 		d.decodeFlush,
 		req.validate,
@@ -118,7 +118,7 @@ func (d *updReqDecoder) Decode(req *UpdateRequests) error {
 
 func (d *updReqDecoder) readLine(e error) error {
 	l, p, err := pktline.ReadLine(d.pr)
-	if err == io.EOF {
+	if errors.Is(err, io.EOF) {
 		return e
 	}
 	if err != nil {
@@ -161,30 +161,51 @@ func (d *updReqDecoder) decodeShallow() error {
 }
 
 func (d *updReqDecoder) decodeCommands() error {
-	for {
-		b := d.payload
-		if len(b) == 0 {
-			return nil
-		}
+	// Process the first line which contains both command and capabilities
+	payload := d.payload
+
+	// The first line must contain capabilities separated by a null byte
+	sep := bytes.IndexByte(payload, 0)
+	if sep == -1 {
+		return errMissingCapabilitiesDelimiter
+	}
+
+	if len(payload) < minCommandAndCapsLength {
+		return errInvalidCommandCapabilitiesLineLength(len(payload))
+	}
+
+	// Extract and decode capabilities (everything after the null byte)
+	if err := d.req.Capabilities.Decode(payload[sep+1:]); err != nil {
+		return err
+	}
+
+	// Extract the command (everything before the null byte)
+	payload = payload[:sep]
 
-		c, err := parseCommand(b)
+	// Read and process commands
+	for {
+		// Parse and add the command
+		cmd, err := parseCommand(payload)
 		if err != nil {
 			return err
 		}
+		d.req.Commands = append(d.req.Commands, cmd)
 
-		d.req.Commands = append(d.req.Commands, c)
-
-		if err := d.readLine(nil); err != nil {
+		// Read the next line
+		if err := d.readLine(errNoFlush); err != nil {
 			return err
 		}
+		payload = d.payload
+
+		// Stop reading once we reach the flush line
+		if d.length == pktline.Flush {
+			return nil
+		}
 	}
 }
 
 func (d *updReqDecoder) decodeFlush() error {
-	if err := d.readLine(nil); err != nil {
-		return err
-	}
-
+	// We should always have a flush line at the end of the request.
 	if len(d.payload) != 0 || d.length != pktline.Flush {
 		return errMalformedRequest("unexpected data after flush")
 	}
@@ -192,35 +213,6 @@ func (d *updReqDecoder) decodeFlush() error {
 	return nil
 }
 
-func (d *updReqDecoder) decodeCommandAndCapabilities() error {
-	b := d.payload
-	i := bytes.IndexByte(b, 0)
-	if i == -1 {
-		return errMissingCapabilitiesDelimiter
-	}
-
-	if len(b) < minCommandAndCapsLength {
-		return errInvalidCommandCapabilitiesLineLength(len(b))
-	}
-
-	cmd, err := parseCommand(b[:i])
-	if err != nil {
-		return err
-	}
-
-	d.req.Commands = append(d.req.Commands, cmd)
-
-	if err := d.req.Capabilities.Decode(b[i+1:]); err != nil {
-		return err
-	}
-
-	if err := d.scanLine(); err != nil {
-		return err
-	}
-
-	return nil
-}
-
 func parseCommand(b []byte) (*Command, error) {
 	if len(b) < minCommandLength {
 		return nil, errInvalidCommandLineLength(len(b))
diff --git a/plumbing/protocol/packp/updreq_encode.go b/plumbing/protocol/packp/updreq_encode.go
index cef8f3d2..507c97c0 100644
--- a/plumbing/protocol/packp/updreq_encode.go
+++ b/plumbing/protocol/packp/updreq_encode.go
@@ -23,12 +23,6 @@ func (req *UpdateRequests) Encode(w io.Writer) error {
 		return err
 	}
 
-	if req.Capabilities.Supports(capability.PushOptions) {
-		if err := req.encodeOptions(w, req.Options); err != nil {
-			return err
-		}
-	}
-
 	return nil
 }
 
@@ -66,15 +60,3 @@ func formatCommand(cmd *Command) string {
 	n := cmd.New.String()
 	return fmt.Sprintf("%s %s %s", o, n, cmd.Name)
 }
-
-func (req *UpdateRequests) encodeOptions(w io.Writer,
-	opts []*Option,
-) error {
-	for _, opt := range opts {
-		if _, err := pktline.Writef(w, "%s=%s", opt.Key, opt.Value); err != nil {
-			return err
-		}
-	}
-
-	return pktline.WriteFlush(w)
-}
diff --git a/plumbing/transport/common.go b/plumbing/transport/common.go
index f6d20d58..c3ff3a7b 100644
--- a/plumbing/transport/common.go
+++ b/plumbing/transport/common.go
@@ -131,7 +131,7 @@ type PushRequest struct {
 	Progress sideband.Progress
 
 	// Options is a set of push-options to be sent to the server during push.
-	Options map[string]string
+	Options []string
 
 	// Atomic indicates an atomic push.
 	// If the server supports atomic push, it will update the refs in one
diff --git a/plumbing/transport/push.go b/plumbing/transport/push.go
index b19f0c32..8f83b3fb 100644
--- a/plumbing/transport/push.go
+++ b/plumbing/transport/push.go
@@ -33,12 +33,6 @@ func buildUpdateRequests(caps *capability.List, req *PushRequest) *packp.UpdateR
 	} else if caps.Supports(capability.NoProgress) {
 		upreq.Capabilities.Set(capability.NoProgress) // nolint: errcheck
 	}
-	if caps.Supports(capability.PushOptions) {
-		upreq.Capabilities.Set(capability.PushOptions) // nolint: errcheck
-		for k, v := range req.Options {
-			upreq.Options = append(upreq.Options, &packp.Option{Key: k, Value: v})
-		}
-	}
 	if req.Atomic && caps.Supports(capability.Atomic) {
 		upreq.Capabilities.Set(capability.Atomic) // nolint: errcheck
 	}
@@ -62,10 +56,23 @@ func SendPack(
 
 	caps := conn.Capabilities()
 	upreq := buildUpdateRequests(caps, req)
+	usePushOptions := len(req.Options) > 0 && caps.Supports(capability.PushOptions)
+	if usePushOptions {
+		upreq.Capabilities.Set(capability.PushOptions) //nolint:errcheck
+	}
+
 	if err := upreq.Encode(writer); err != nil {
 		return err
 	}
 
+	if usePushOptions {
+		var opts packp.PushOptions
+		opts.Options = req.Options
+		if err := opts.Encode(writer); err != nil {
+			return fmt.Errorf("encoding push-options: %w", err)
+		}
+	}
+
 	// Send the packfile.
 	if req.Packfile != nil {
 		if _, err := io.Copy(writer, req.Packfile); err != nil {
diff --git a/plumbing/transport/receive_pack.go b/plumbing/transport/receive_pack.go
index f4c6ac93..f1e4c040 100644
--- a/plumbing/transport/receive_pack.go
+++ b/plumbing/transport/receive_pack.go
@@ -85,8 +85,16 @@ func ReceivePack(
 	var (
 		caps         = updreq.Capabilities
 		needPackfile bool
+		pushOpts     packp.PushOptions
 	)
 
+	// TODO: Pass the options to the server-side hooks.
+	if updreq.Capabilities.Supports(capability.PushOptions) {
+		if err := pushOpts.Decode(rd); err != nil {
+			return fmt.Errorf("decoding push-options: %w", err)
+		}
+	}
+
 	// Should we expect a packfile?
 	for _, cmd := range updreq.Commands {
 		if cmd.New != plumbing.ZeroHash {


Test patch:
diff --git a/plumbing/protocol/packp/pushopts_test.go b/plumbing/protocol/packp/pushopts_test.go
new file mode 100644
index 00000000..e326f465
--- /dev/null
+++ b/plumbing/protocol/packp/pushopts_test.go
@@ -0,0 +1,54 @@
+package packp
+
+import (
+	"bytes"
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+)
+
+type PushOptionsSuite struct {
+	suite.Suite
+}
+
+func TestPushOptionsSuite(t *testing.T) {
+	suite.Run(t, new(PushOptionsSuite))
+}
+
+func (s *PushOptionsSuite) TestPushOptionsEncode() {
+	var r PushOptions
+	r.Options = []string{
+		"SomeKey=SomeValue",
+		"AnotherKey=AnotherValue",
+	}
+
+	expected := pktlines(s.T(),
+		"SomeKey=SomeValue",
+		"AnotherKey=AnotherValue",
+		"",
+	)
+
+	var buf bytes.Buffer
+	s.Require().Nil(r.Encode(&buf))
+	s.Require().Equal(expected, buf.Bytes())
+}
+
+func (s *PushOptionsSuite) TestPushOptionsDecode() {
+	var r PushOptions
+	r.Options = nil
+
+	input := pktlines(s.T(),
+		"SomeKey=SomeValue",
+		"AnotherKey=AnotherValue",
+		"",
+	)
+
+	s.Require().Nil(r.Decode(bytes.NewReader(input)))
+
+	expected := []string{
+		"SomeKey=SomeValue",
+		"AnotherKey=AnotherValue",
+	}
+
+	s.Require().Equal(expected, r.Options)
+}
diff --git a/plumbing/protocol/packp/updreq_decode_test.go b/plumbing/protocol/packp/updreq_decode_test.go
index 48e47b1c..461f8eab 100644
--- a/plumbing/protocol/packp/updreq_decode_test.go
+++ b/plumbing/protocol/packp/updreq_decode_test.go
@@ -22,7 +22,7 @@ func TestUpdReqDecodeSuite(t *testing.T) {
 func (s *UpdReqDecodeSuite) TestEmpty() {
 	r := NewUpdateRequests()
 	var buf bytes.Buffer
-	s.Equal(ErrEmpty, r.Decode(&buf))
+	s.ErrorIs(r.Decode(&buf), ErrEmpty)
 	s.Equal(NewUpdateRequests(), r)
 }
 
diff --git a/plumbing/protocol/packp/updreq_encode_test.go b/plumbing/protocol/packp/updreq_encode_test.go
index 76094b86..3902e15a 100644
--- a/plumbing/protocol/packp/updreq_encode_test.go
+++ b/plumbing/protocol/packp/updreq_encode_test.go
@@ -149,32 +149,6 @@ func (s *UpdReqEncodeSuite) TestWithPackfile() {
 }
 */
 
-func (s *UpdReqEncodeSuite) TestPushOptions() {
-	hash1 := plumbing.NewHash("1ecf0ef2c2dffb796033e5a02219af86ec6584e5")
-	hash2 := plumbing.NewHash("2ecf0ef2c2dffb796033e5a02219af86ec6584e5")
-	name := plumbing.ReferenceName("myref")
-
-	r := NewUpdateRequests()
-	r.Capabilities.Set(capability.PushOptions)
-	r.Commands = []*Command{
-		{Name: name, Old: hash1, New: hash2},
-	}
-	r.Options = []*Option{
-		{Key: "SomeKey", Value: "SomeValue"},
-		{Key: "AnotherKey", Value: "AnotherValue"},
-	}
-
-	expected := pktlines(s.T(),
-		"1ecf0ef2c2dffb796033e5a02219af86ec6584e5 2ecf0ef2c2dffb796033e5a02219af86ec6584e5 myref\x00push-options",
-		"",
-		"SomeKey=SomeValue",
-		"AnotherKey=AnotherValue",
-		"",
-	)
-
-	s.testEncode(r, expected)
-}
-
 func (s *UpdReqEncodeSuite) TestPushAtomic() {
 	hash1 := plumbing.NewHash("1ecf0ef2c2dffb796033e5a02219af86ec6584e5")
 	hash2 := plumbing.NewHash("2ecf0ef2c2dffb796033e5a02219af86ec6584e5")
diff --git a/remote_test.go b/remote_test.go
index d44c3a5a..1533e97d 100644
--- a/remote_test.go
+++ b/remote_test.go
@@ -647,6 +647,29 @@ func (s *RemoteSuite) TestPushContext() {
 	})
 }
 
+func (s *RemoteSuite) TestPushPushOptions() {
+	url := s.T().TempDir()
+	_, err := PlainInit(url, true)
+	s.Require().NoError(err)
+
+	fs := fixtures.Basic().One().DotGit()
+	sto := filesystem.NewStorage(fs, cache.NewObjectLRUDefault())
+
+	r := NewRemote(sto, &config.RemoteConfig{
+		Name: DefaultRemoteName,
+		URLs: []string{url},
+	})
+
+	// TODO: Validate the push options was received by the server and implement
+	// server-side hooks.
+	err = r.Push(&PushOptions{
+		Options: []string{
+			"iam-a-push-option",
+		},
+	})
+	s.Require().NoError(err)
+}
+
 func eventually(s *RemoteSuite, condition func() bool) {
 	select {
 	case <-time.After(5 * time.Second):