patch of appview: improve patch validation · round #1 · pull #670 · tangled.org/core

+18 -22

appview/pulls/pulls.go

···

       23
        
       	"tangled.org/core/appview/pages"

     

       24
        
       	"tangled.org/core/appview/pages/markup"

     

       25
        
       	"tangled.org/core/appview/reporesolver"

     

       0
        
       
     

       26
        
       	"tangled.org/core/appview/xrpcclient"

     

       27
        
       	"tangled.org/core/idresolver"

     

       28
        
       	"tangled.org/core/patchutil"

     
···

       47
        
       	notifier     notify.Notifier

     

       48
        
       	enforcer     *rbac.Enforcer

     

       49
        
       	logger       *slog.Logger

     

       0
        
       
     

       50
        
       }

     

       51
        
       

     

       52
        
       func New(

     
···

       58
        
       	config *config.Config,

     

       59
        
       	notifier notify.Notifier,

     

       60
        
       	enforcer *rbac.Enforcer,

     

       0
        
       
     

       61
        
       	logger *slog.Logger,

     

       62
        
       ) *Pulls {

     

       63
        
       	return &Pulls{

     
···

       70
        
       		notifier:     notifier,

     

       71
        
       		enforcer:     enforcer,

     

       72
        
       		logger:       logger,

     

       0
        
       
     

       73
        
       	}

     

       74
        
       }

     

       75
        
       

     
···

       965
        
       	patch := comparison.FormatPatchRaw

     

       966
        
       	combined := comparison.CombinedPatchRaw

     

       967
        
       

     

       968
       -
       	if !patchutil.IsPatchValid(patch) {

     

       0
        
       
     

       969
        
       		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")

     

       970
        
       		return

     

       971
        
       	}

     
···

       982
        
       }

     

       983
        
       

     

       984
        
       func (s *Pulls) handlePatchBasedPull(w http.ResponseWriter, r *http.Request, f *reporesolver.ResolvedRepo, user *oauth.User, title, body, targetBranch, patch string, isStacked bool) {

     

       985
       -
       	if !patchutil.IsPatchValid(patch) {

     

       0
        
       
     

       986
        
       		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")

     

       987
        
       		return

     

       988
        
       	}

     
···

       1073
        
       	patch := comparison.FormatPatchRaw

     

       1074
        
       	combined := comparison.CombinedPatchRaw

     

       1075
        
       

     

       1076
       -
       	if !patchutil.IsPatchValid(patch) {

     

       0
        
       
     

       1077
        
       		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")

     

       1078
        
       		return

     

       1079
        
       	}

     
···

       1337
        
       		return

     

       1338
        
       	}

     

       1339
        
       

     

       1340
       -
       	if patch == "" || !patchutil.IsPatchValid(patch) {

     

       0
        
       
     

       1341
        
       		s.pages.Notice(w, "patch-error", "Invalid patch format. Please provide a valid git diff or format-patch.")

     

       1342
        
       		return

     

       1343
        
       	}

     
···

       1754
        
       	s.resubmitPullHelper(w, r, f, user, pull, patch, combined, sourceRev)

     

       1755
        
       }

     

       1756
        
       

     

       1757
       -
       // validate a resubmission against a pull request

     

       1758
       -
       func validateResubmittedPatch(pull *models.Pull, patch string) error {

     

       1759
       -
       	if patch == "" {

     

       1760
       -
       		return fmt.Errorf("Patch is empty.")

     

       1761
       -
       	}

     

       1762
       -
       

     

       1763
       -
       	if patch == pull.LatestPatch() {

     

       1764
       -
       		return fmt.Errorf("Patch is identical to previous submission.")

     

       1765
       -
       	}

     

       1766
       -
       

     

       1767
       -
       	if !patchutil.IsPatchValid(patch) {

     

       1768
       -
       		return fmt.Errorf("Invalid patch format. Please provide a valid diff.")

     

       1769
       -
       	}

     

       1770
       -
       

     

       1771
       -
       	return nil

     

       1772
       -
       }

     

       1773
       -
       

     

       1774
        
       func (s *Pulls) resubmitPullHelper(

     

       1775
        
       	w http.ResponseWriter,

     

       1776
        
       	r *http.Request,

     
···

       1787
        
       		return

     

       1788
        
       	}

     

       1789
        
       

     

       1790
       -
       	if err := validateResubmittedPatch(pull, patch); err != nil {

     

       1791
        
       		s.pages.Notice(w, "resubmit-error", err.Error())

     

       1792
        
       		return

     

       1793
        
       	}

     

       1794
        
       

     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       1795
        
       	// validate sourceRev if branch/fork based

     

       1796
        
       	if pull.IsBranchBased() || pull.IsForkBased() {

     

       1797
        
       		if sourceRev == pull.LatestSha() {

···

       23
        
       	"tangled.org/core/appview/pages"

     

       24
        
       	"tangled.org/core/appview/pages/markup"

     

       25
        
       	"tangled.org/core/appview/reporesolver"

     

       26
       +
       	"tangled.org/core/appview/validator"

     

       27
        
       	"tangled.org/core/appview/xrpcclient"

     

       28
        
       	"tangled.org/core/idresolver"

     

       29
        
       	"tangled.org/core/patchutil"

     
···

       48
        
       	notifier     notify.Notifier

     

       49
        
       	enforcer     *rbac.Enforcer

     

       50
        
       	logger       *slog.Logger

     

       51
       +
       	validator    *validator.Validator

     

       52
        
       }

     

       53
        
       

     

       54
        
       func New(

     
···

       60
        
       	config *config.Config,

     

       61
        
       	notifier notify.Notifier,

     

       62
        
       	enforcer *rbac.Enforcer,

     

       63
       +
       	validator *validator.Validator,

     

       64
        
       	logger *slog.Logger,

     

       65
        
       ) *Pulls {

     

       66
        
       	return &Pulls{

     
···

       73
        
       		notifier:     notifier,

     

       74
        
       		enforcer:     enforcer,

     

       75
        
       		logger:       logger,

     

       76
       +
       		validator:    validator,

     

       77
        
       	}

     

       78
        
       }

     

       79
        
       

     
···

       969
        
       	patch := comparison.FormatPatchRaw

     

       970
        
       	combined := comparison.CombinedPatchRaw

     

       971
        
       

     

       972
       +
       	if err := s.validator.ValidatePatch(&patch); err != nil {

     

       973
       +
       		s.logger.Error("failed to validate patch", "err", err)

     

       974
        
       		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")

     

       975
        
       		return

     

       976
        
       	}

     
···

       987
        
       }

     

       988
        
       

     

       989
        
       func (s *Pulls) handlePatchBasedPull(w http.ResponseWriter, r *http.Request, f *reporesolver.ResolvedRepo, user *oauth.User, title, body, targetBranch, patch string, isStacked bool) {

     

       990
       +
       	if err := s.validator.ValidatePatch(&patch); err != nil {

     

       991
       +
       		s.logger.Error("patch validation failed", "err", err)

     

       992
        
       		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")

     

       993
        
       		return

     

       994
        
       	}

     
···

       1079
        
       	patch := comparison.FormatPatchRaw

     

       1080
        
       	combined := comparison.CombinedPatchRaw

     

       1081
        
       

     

       1082
       +
       	if err := s.validator.ValidatePatch(&patch); err != nil {

     

       1083
       +
       		s.logger.Error("failed to validate patch", "err", err)

     

       1084
        
       		s.pages.Notice(w, "pull", "Invalid patch format. Please provide a valid diff.")

     

       1085
        
       		return

     

       1086
        
       	}

     
···

       1344
        
       		return

     

       1345
        
       	}

     

       1346
        
       

     

       1347
       +
       	if err := s.validator.ValidatePatch(&patch); err != nil {

     

       1348
       +
       		s.logger.Error("faield to validate patch", "err", err)

     

       1349
        
       		s.pages.Notice(w, "patch-error", "Invalid patch format. Please provide a valid git diff or format-patch.")

     

       1350
        
       		return

     

       1351
        
       	}

     
···

       1762
        
       	s.resubmitPullHelper(w, r, f, user, pull, patch, combined, sourceRev)

     

       1763
        
       }

     

       1764
        
       

     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       1765
        
       func (s *Pulls) resubmitPullHelper(

     

       1766
        
       	w http.ResponseWriter,

     

       1767
        
       	r *http.Request,

     
···

       1778
        
       		return

     

       1779
        
       	}

     

       1780
        
       

     

       1781
       +
       	if err := s.validator.ValidatePatch(&patch); err != nil {

     

       1782
        
       		s.pages.Notice(w, "resubmit-error", err.Error())

     

       1783
        
       		return

     

       1784
        
       	}

     

       1785
        
       

     

       1786
       +
       	if patch == pull.LatestPatch() {

     

       1787
       +
       		s.pages.Notice(w, "resubmit-error", "Patch is identical to previous submission.")

     

       1788
       +
       		return

     

       1789
       +
       	}

     

       1790
       +
       

     

       1791
        
       	// validate sourceRev if branch/fork based

     

       1792
        
       	if pull.IsBranchBased() || pull.IsForkBased() {

     

       1793
        
       		if sourceRev == pull.LatestSha() {

appview/state/router.go

···

       277
        
       		s.config,

     

       278
        
       		s.notifier,

     

       279
        
       		s.enforcer,

     

       0
        
       
     

       280
        
       		log.SubLogger(s.logger, "pulls"),

     

       281
        
       	)

     

       282
        
       	return pulls.Router(mw)

···

       277
        
       		s.config,

     

       278
        
       		s.notifier,

     

       279
        
       		s.enforcer,

     

       280
       +
       		s.validator,

     

       281
        
       		log.SubLogger(s.logger, "pulls"),

     

       282
        
       	)

     

       283
        
       	return pulls.Router(mw)

+25

appview/validator/patch.go

···

       1
       +
       package validator

     

       2
       +
       

     

       3
       +
       import (

     

       4
       +
       	"fmt"

     

       5
       +
       	"strings"

     

       6
       +
       

     

       7
       +
       	"tangled.org/core/patchutil"

     

       8
       +
       )

     

       9
       +
       

     

       10
       +
       func (v *Validator) ValidatePatch(patch *string) error {

     

       11
       +
       	if patch == nil || *patch == "" {

     

       12
       +
       		return fmt.Errorf("patch is empty")

     

       13
       +
       	}

     

       14
       +
       

     

       15
       +
       	// add newline if not present to diff style patches

     

       16
       +
       	if !patchutil.IsFormatPatch(*patch) && !strings.HasSuffix(*patch, "\n") {

     

       17
       +
       		*patch = *patch + "\n"

     

       18
       +
       	}

     

       19
       +
       

     

       20
       +
       	if err := patchutil.IsPatchValid(*patch); err != nil {

     

       21
       +
       		return err

     

       22
       +
       	}

     

       23
       +
       

     

       24
       +
       	return nil

     

       25
       +
       }

knotserver/xrpc/merge_check.go

···

       81
        
       		}

     

       82
        
       	}

     

       83
        
       

     

       0
        
       
     

       0
        
       
     

       84
        
       	w.Header().Set("Content-Type", "application/json")

     

       85
        
       	w.WriteHeader(http.StatusOK)

     

       86
        
       	json.NewEncoder(w).Encode(response)

···

       81
        
       		}

     

       82
        
       	}

     

       83
        
       

     

       84
       +
       	l.Debug("merge check response", "isConflicted", response.Is_conflicted, "err", response.Error, "conflicts", response.Conflicts)

     

       85
       +
       

     

       86
        
       	w.Header().Set("Content-Type", "application/json")

     

       87
        
       	w.WriteHeader(http.StatusOK)

     

       88
        
       	json.NewEncoder(w).Encode(response)

+18 -7

patchutil/patchutil.go

···

       1
        
       package patchutil

     

       2
        
       

     

       3
        
       import (

     

       0
        
       
     

       4
        
       	"fmt"

     

       5
        
       	"log"

     

       6
        
       	"os"

     
···

       42
        
       // IsPatchValid checks if the given patch string is valid.

     

       43
        
       // It performs very basic sniffing for either git-diff or git-format-patch

     

       44
        
       // header lines. For format patches, it attempts to extract and validate each one.

     

       45
       -
       func IsPatchValid(patch string) bool {

     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       46
        
       	if len(patch) == 0 {

     

       47
       -
       		return false

     

       48
        
       	}

     

       49
        
       

     

       50
        
       	lines := strings.Split(patch, "\n")

     

       51
        
       	if len(lines) < 2 {

     

       52
       -
       		return false

     

       53
        
       	}

     

       54
        
       

     

       55
        
       	firstLine := strings.TrimSpace(lines[0])

     
···

       60
        
       		strings.HasPrefix(firstLine, "Index: ") ||

     

       61
        
       		strings.HasPrefix(firstLine, "+++ ") ||

     

       62
        
       		strings.HasPrefix(firstLine, "@@ ") {

     

       63
       -
       		return true

     

       64
        
       	}

     

       65
        
       

     

       66
        
       	// check if it's format-patch

     
···

       70
        
       		// it's safe to say it's broken.

     

       71
        
       		patches, err := ExtractPatches(patch)

     

       72
        
       		if err != nil {

     

       73
       -
       			return false

     

       74
        
       		}

     

       75
       -
       		return len(patches) > 0

     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       0
        
       
     

       76
        
       	}

     

       77
        
       

     

       78
       -
       	return false

     

       79
        
       }

     

       80
        
       

     

       81
        
       func IsFormatPatch(patch string) bool {

···

       1
        
       package patchutil

     

       2
        
       

     

       3
        
       import (

     

       4
       +
       	"errors"

     

       5
        
       	"fmt"

     

       6
        
       	"log"

     

       7
        
       	"os"

     
···

       43
        
       // IsPatchValid checks if the given patch string is valid.

     

       44
        
       // It performs very basic sniffing for either git-diff or git-format-patch

     

       45
        
       // header lines. For format patches, it attempts to extract and validate each one.

     

       46
       +
       var (

     

       47
       +
       	EmptyPatchError   error = errors.New("patch is empty")

     

       48
       +
       	GenericPatchError error = errors.New("patch is invalid")

     

       49
       +
       	FormatPatchError  error = errors.New("patch is not a valid format-patch")

     

       50
       +
       )

     

       51
       +
       

     

       52
       +
       func IsPatchValid(patch string) error {

     

       53
        
       	if len(patch) == 0 {

     

       54
       +
       		return EmptyPatchError

     

       55
        
       	}

     

       56
        
       

     

       57
        
       	lines := strings.Split(patch, "\n")

     

       58
        
       	if len(lines) < 2 {

     

       59
       +
       		return EmptyPatchError

     

       60
        
       	}

     

       61
        
       

     

       62
        
       	firstLine := strings.TrimSpace(lines[0])

     
···

       67
        
       		strings.HasPrefix(firstLine, "Index: ") ||

     

       68
        
       		strings.HasPrefix(firstLine, "+++ ") ||

     

       69
        
       		strings.HasPrefix(firstLine, "@@ ") {

     

       70
       +
       		return nil

     

       71
        
       	}

     

       72
        
       

     

       73
        
       	// check if it's format-patch

     
···

       77
        
       		// it's safe to say it's broken.

     

       78
        
       		patches, err := ExtractPatches(patch)

     

       79
        
       		if err != nil {

     

       80
       +
       			return fmt.Errorf("%w: %w", FormatPatchError, err)

     

       81
        
       		}

     

       82
       +
       		if len(patches) == 0 {

     

       83
       +
       			return EmptyPatchError

     

       84
       +
       		}

     

       85
       +
       

     

       86
       +
       		return nil

     

       87
        
       	}

     

       88
        
       

     

       89
       +
       	return GenericPatchError

     

       90
        
       }

     

       91
        
       

     

       92
        
       func IsFormatPatch(patch string) bool {

+13 -12

patchutil/patchutil_test.go

···

       1
        
       package patchutil

     

       2
        
       

     

       3
        
       import (

     

       0
        
       
     

       4
        
       	"reflect"

     

       5
        
       	"testing"

     

       6
        
       )

     
···

       9
        
       	tests := []struct {

     

       10
        
       		name     string

     

       11
        
       		patch    string

     

       12
       -
       		expected bool

     

       13
        
       	}{

     

       14
        
       		{

     

       15
        
       			name:     `empty patch`,

     

       16
        
       			patch:    ``,

     

       17
       -
       			expected: false,

     

       18
        
       		},

     

       19
        
       		{

     

       20
        
       			name:     `single line patch`,

     

       21
        
       			patch:    `single line`,

     

       22
       -
       			expected: false,

     

       23
        
       		},

     

       24
        
       		{

     

       25
        
       			name: `valid diff patch`,

     
···

       31
        
       -old line

     

       32
        
       +new line

     

       33
        
       	context`,

     

       34
       -
       			expected: true,

     

       35
        
       		},

     

       36
        
       		{

     

       37
        
       			name: `valid patch starting with ---`,

     
···

       41
        
       -old line

     

       42
        
       +new line

     

       43
        
       	context`,

     

       44
       -
       			expected: true,

     

       45
        
       		},

     

       46
        
       		{

     

       47
        
       			name: `valid patch starting with Index`,

     
···

       53
        
       -old line

     

       54
        
       +new line

     

       55
        
       	context`,

     

       56
       -
       			expected: true,

     

       57
        
       		},

     

       58
        
       		{

     

       59
        
       			name: `valid patch starting with +++`,

     
···

       63
        
       -old line

     

       64
        
       +new line

     

       65
        
       	context`,

     

       66
       -
       			expected: true,

     

       67
        
       		},

     

       68
        
       		{

     

       69
        
       			name: `valid patch starting with @@`,

     
···

       72
        
       +new line

     

       73
        
       	context

     

       74
        
       `,

     

       75
       -
       			expected: true,

     

       76
        
       		},

     

       77
        
       		{

     

       78
        
       			name: `valid format patch`,

     
···

       90
        
       +new content

     

       91
        
       --

     

       92
        
       2.48.1`,

     

       93
       -
       			expected: true,

     

       94
        
       		},

     

       95
        
       		{

     

       96
        
       			name: `invalid format patch`,

     

       97
        
       			patch: `From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001

     

       98
        
       From: Author <author@example.com>

     

       99
        
       This is not a valid patch format`,

     

       100
       -
       			expected: false,

     

       101
        
       		},

     

       102
        
       		{

     

       103
        
       			name: `not a patch at all`,

     
···

       105
        
       just some

     

       106
        
       random text

     

       107
        
       that isn't a patch`,

     

       108
       -
       			expected: false,

     

       109
        
       		},

     

       110
        
       	}

     

       111
        
       

     

       112
        
       	for _, tt := range tests {

     

       113
        
       		t.Run(tt.name, func(t *testing.T) {

     

       114
        
       			result := IsPatchValid(tt.patch)

     

       115
       -
       			if result != tt.expected {

     

       116
        
       				t.Errorf("IsPatchValid() = %v, want %v", result, tt.expected)

     

       117
        
       			}

     

       118
        
       		})

···

       1
        
       package patchutil

     

       2
        
       

     

       3
        
       import (

     

       4
       +
       	"errors"

     

       5
        
       	"reflect"

     

       6
        
       	"testing"

     

       7
        
       )

     
···

       10
        
       	tests := []struct {

     

       11
        
       		name     string

     

       12
        
       		patch    string

     

       13
       +
       		expected error

     

       14
        
       	}{

     

       15
        
       		{

     

       16
        
       			name:     `empty patch`,

     

       17
        
       			patch:    ``,

     

       18
       +
       			expected: EmptyPatchError,

     

       19
        
       		},

     

       20
        
       		{

     

       21
        
       			name:     `single line patch`,

     

       22
        
       			patch:    `single line`,

     

       23
       +
       			expected: EmptyPatchError,

     

       24
        
       		},

     

       25
        
       		{

     

       26
        
       			name: `valid diff patch`,

     
···

       32
        
       -old line

     

       33
        
       +new line

     

       34
        
       	context`,

     

       35
       +
       			expected: nil,

     

       36
        
       		},

     

       37
        
       		{

     

       38
        
       			name: `valid patch starting with ---`,

     
···

       42
        
       -old line

     

       43
        
       +new line

     

       44
        
       	context`,

     

       45
       +
       			expected: nil,

     

       46
        
       		},

     

       47
        
       		{

     

       48
        
       			name: `valid patch starting with Index`,

     
···

       54
        
       -old line

     

       55
        
       +new line

     

       56
        
       	context`,

     

       57
       +
       			expected: nil,

     

       58
        
       		},

     

       59
        
       		{

     

       60
        
       			name: `valid patch starting with +++`,

     
···

       64
        
       -old line

     

       65
        
       +new line

     

       66
        
       	context`,

     

       67
       +
       			expected: nil,

     

       68
        
       		},

     

       69
        
       		{

     

       70
        
       			name: `valid patch starting with @@`,

     
···

       73
        
       +new line

     

       74
        
       	context

     

       75
        
       `,

     

       76
       +
       			expected: nil,

     

       77
        
       		},

     

       78
        
       		{

     

       79
        
       			name: `valid format patch`,

     
···

       91
        
       +new content

     

       92
        
       --

     

       93
        
       2.48.1`,

     

       94
       +
       			expected: nil,

     

       95
        
       		},

     

       96
        
       		{

     

       97
        
       			name: `invalid format patch`,

     

       98
        
       			patch: `From 1234567890123456789012345678901234567890 Mon Sep 17 00:00:00 2001

     

       99
        
       From: Author <author@example.com>

     

       100
        
       This is not a valid patch format`,

     

       101
       +
       			expected: FormatPatchError,

     

       102
        
       		},

     

       103
        
       		{

     

       104
        
       			name: `not a patch at all`,

     
···

       106
        
       just some

     

       107
        
       random text

     

       108
        
       that isn't a patch`,

     

       109
       +
       			expected: GenericPatchError,

     

       110
        
       		},

     

       111
        
       	}

     

       112
        
       

     

       113
        
       	for _, tt := range tests {

     

       114
        
       		t.Run(tt.name, func(t *testing.T) {

     

       115
        
       			result := IsPatchValid(tt.patch)

     

       116
       +
       			if !errors.Is(result, tt.expected) {

     

       117
        
       				t.Errorf("IsPatchValid() = %v, want %v", result, tt.expected)

     

       118
        
       			}

     

       119
        
       		})