plumbing: packp, validate push options to prevent invalid characters#2057
Open
aymanbagabas wants to merge 1 commit into
Open
plumbing: packp, validate push options to prevent invalid characters#2057aymanbagabas wants to merge 1 commit into
aymanbagabas wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds protocol-level validation for pack push-options so options containing LF (\n) or NUL (\x00) are rejected, preventing construction of invalid protocol streams.
Changes:
- Introduce
ErrInvalidPushOptionand validate option contents duringPushOptions.Encode. - Add unit tests covering invalid push-options encoding cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plumbing/protocol/packp/pushopts.go | Adds an exported error and rejects options containing newline or null bytes during encoding. |
| plumbing/protocol/packp/pushopts_test.go | Adds tests asserting invalid options fail encoding and wrap ErrInvalidPushOption. |
aymanbagabas
force-pushed
the
validate-server-options
branch
from
680fe5c to
dc92700
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
plumbing/protocol/packp/pushopts.go:39
PushOptions.Encodenow always writes a flush packet even whenopts.Optionsis empty/nil (previously it was a no-op). This is an observable behavior change and can corrupt the surrounding protocol stream if callers unconditionally callEncodeeven when they are not sending a push-options section. Consider restoring the early-return forlen(opts.Options)==0(or otherwise documenting/guarding this behavior).
func (opts *PushOptions) Encode(w io.Writer) error {
if slices.ContainsFunc(opts.Options, func(opt string) bool {
return strings.ContainsAny(opt, "\n\x00")
}) {
return fmt.Errorf("%w: contains newline or null byte", ErrInvalidPushOption)
}
for _, opt := range opts.Options {
if _, err := pktline.Writef(w, "%s", opt); err != nil {
return err
}
}
return pktline.WriteFlush(w)
}
dc92700 to
05833f2
Compare
Comment on lines
+56
to
+59
| opt := string(line) | ||
| if strings.ContainsAny(opt, "\n\x00") { | ||
| return fmt.Errorf("%w: contains newline or null byte", ErrInvalidPushOption) | ||
| } |
aymanbagabas
force-pushed
the
validate-server-options
branch
from
05833f2 to
4981ae0
Compare
pjbgf
reviewed
May 4, 2026
| func (opts *PushOptions) Encode(w io.Writer) error { | ||
| if len(opts.Options) == 0 { | ||
| return nil | ||
| if slices.ContainsFunc(opts.Options, func(opt string) bool { |
Member
There was a problem hiding this comment.
As part of validation, if an option is too large (e.g. pktline.MaxPayloadSize) shouldn't it also return ErrInvalidPushOption?
Member
Author
There was a problem hiding this comment.
Correct, I just added that. We handle that during pktline.Write but it should also be part of the validation before getting into writing the push-options. It turned out that I was referencing the wrong docs for push-options. I was looking at server-options which are clone time options. Push-options are even more strict and only allow printable characters. Here I'm using unicode.IsGraphic which should also allow UTF-8 printables
aymanbagabas
force-pushed
the
validate-server-options
branch
2 times, most recently
from
d444ec1 to
e71c2fa
Compare
aymanbagabas
force-pushed
the
validate-server-options
branch
from
e71c2fa to
e21c502
Compare
Git pack push server options must not contain newlines or null bytes, as these would break the protocol. See https://git-scm.com/docs/pack-protocol#_reference_update_request_and_packfile_transfer Signed-off-by: Ayman Bagabas <ayman.bagabas@gmail.com>
Comment on lines
+27
to
+30
| if slices.ContainsFunc(opts.Options, func(opt string) bool { | ||
| return strings.ContainsFunc(opt, isNotGraphic) || len(opt) > pktline.MaxPayloadSize | ||
| }) { | ||
| return fmt.Errorf("%w: contains invalid character", ErrInvalidPushOption) |
Comment on lines
+27
to
31
| if slices.ContainsFunc(opts.Options, func(opt string) bool { | ||
| return strings.ContainsFunc(opt, isNotGraphic) || len(opt) > pktline.MaxPayloadSize | ||
| }) { | ||
| return fmt.Errorf("%w: contains invalid character", ErrInvalidPushOption) | ||
| } |
Comment on lines
+49
to
+75
| func (s *PushOptionsSuite) TestPushOptionsEncodeInvalidOption() { | ||
| cases := []struct { | ||
| name string | ||
| options []string | ||
| }{ | ||
| { | ||
| name: "option with newline", | ||
| options: []string{"SomeKey=SomeValue\n"}, | ||
| }, | ||
| { | ||
| name: "option with null byte", | ||
| options: []string{"SomeKey=SomeValue\x00"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, c := range cases { | ||
| s.Run(c.name, func() { | ||
| var r PushOptions | ||
| r.Options = c.options | ||
|
|
||
| var buf bytes.Buffer | ||
| err := r.Encode(&buf) | ||
| s.Require().Error(err) | ||
| s.Require().True(errors.Is(err, ErrInvalidPushOption)) | ||
| }) | ||
| } | ||
| } |
Comment on lines
+68
to
+70
| func isNotGraphic(r rune) bool { | ||
| return !unicode.IsGraphic(r) | ||
| } |
| if len(opts.Options) == 0 { | ||
| return nil | ||
| if slices.ContainsFunc(opts.Options, func(opt string) bool { | ||
| return strings.ContainsFunc(opt, isNotGraphic) || len(opt) > pktline.MaxPayloadSize |
Comment on lines
+49
to
+75
| func (s *PushOptionsSuite) TestPushOptionsEncodeInvalidOption() { | ||
| cases := []struct { | ||
| name string | ||
| options []string | ||
| }{ | ||
| { | ||
| name: "option with newline", | ||
| options: []string{"SomeKey=SomeValue\n"}, | ||
| }, | ||
| { | ||
| name: "option with null byte", | ||
| options: []string{"SomeKey=SomeValue\x00"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, c := range cases { | ||
| s.Run(c.name, func() { | ||
| var r PushOptions | ||
| r.Options = c.options | ||
|
|
||
| var buf bytes.Buffer | ||
| err := r.Encode(&buf) | ||
| s.Require().Error(err) | ||
| s.Require().True(errors.Is(err, ErrInvalidPushOption)) | ||
| }) | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Git pack push server options must not contain newlines or null bytes, as these would break the protocol.
See https://git-scm.com/docs/pack-protocol#_reference_update_request_and_packfile_transfer