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

+18 -22

appview/pulls/pulls.go

···

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

     

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

     

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

     

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

     

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

     

       27
       28
        
       	"tangled.org/core/idresolver"

     

       28
       29
        
       	"tangled.org/core/patchutil"

     
···

       47
       48
        
       	notifier     notify.Notifier

     

       48
       49
        
       	enforcer     *rbac.Enforcer

     

       49
       50
        
       	logger       *slog.Logger

     

       51
       51
       +
       	validator    *validator.Validator

     

       50
       52
        
       }

     

       51
       53
        
       

     

       52
       54
        
       func New(

     
···

       58
       60
        
       	config *config.Config,

     

       59
       61
        
       	notifier notify.Notifier,

     

       60
       62
        
       	enforcer *rbac.Enforcer,

     

       63
       63
       +
       	validator *validator.Validator,

     

       61
       64
        
       	logger *slog.Logger,

     

       62
       65
        
       ) *Pulls {

     

       63
       66
        
       	return &Pulls{

     
···

       70
       73
        
       		notifier:     notifier,

     

       71
       74
        
       		enforcer:     enforcer,

     

       72
       75
        
       		logger:       logger,

     

       76
       76
       +
       		validator:    validator,

     

       73
       77
        
       	}

     

       74
       78
        
       }

     

       75
       79
        
       

     
···

       965
       969
        
       	patch := comparison.FormatPatchRaw

     

       966
       970
        
       	combined := comparison.CombinedPatchRaw

     

       967
       971
        
       

     

       968
       968
       -
       	if !patchutil.IsPatchValid(patch) {

     

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

     

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

     

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

     

       970
       975
        
       		return

     

       971
       976
        
       	}

     
···

       982
       987
        
       }

     

       983
       988
        
       

     

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

     

       985
       985
       -
       	if !patchutil.IsPatchValid(patch) {

     

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

     

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

     

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

     

       987
       993
        
       		return

     

       988
       994
        
       	}

     
···

       1073
       1079
        
       	patch := comparison.FormatPatchRaw

     

       1074
       1080
        
       	combined := comparison.CombinedPatchRaw

     

       1075
       1081
        
       

     

       1076
       1076
       -
       	if !patchutil.IsPatchValid(patch) {

     

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

     

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

     

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

     

       1078
       1085
        
       		return

     

       1079
       1086
        
       	}

     
···

       1337
       1344
        
       		return

     

       1338
       1345
        
       	}

     

       1339
       1346
        
       

     

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

     

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

     

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

     

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

     

       1342
       1350
        
       		return

     

       1343
       1351
        
       	}

     
···

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

     

       1755
       1763
        
       }

     

       1756
       1764
        
       

     

       1757
       1757
       -
       // validate a resubmission against a pull request

     

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

     

       1759
       1759
       -
       	if patch == "" {

     

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

     

       1761
       1761
       -
       	}

     

       1762
       1762
       -
       

     

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

     

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

     

       1765
       1765
       -
       	}

     

       1766
       1766
       -
       

     

       1767
       1767
       -
       	if !patchutil.IsPatchValid(patch) {

     

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

     

       1769
       1769
       -
       	}

     

       1770
       1770
       -
       

     

       1771
       1771
       -
       	return nil

     

       1772
       1772
       -
       }

     

       1773
       1773
       -
       

     

       1774
       1765
        
       func (s *Pulls) resubmitPullHelper(

     

       1775
       1766
        
       	w http.ResponseWriter,

     

       1776
       1767
        
       	r *http.Request,

     
···

       1787
       1778
        
       		return

     

       1788
       1779
        
       	}

     

       1789
       1780
        
       

     

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

     

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

     

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

     

       1792
       1783
        
       		return

     

       1793
       1784
        
       	}

     

       1794
       1785
        
       

     

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

     

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

     

       1788
       1788
       +
       		return

     

       1789
       1789
       +
       	}

     

       1790
       1790
       +
       

     

       1795
       1791
        
       	// validate sourceRev if branch/fork based

     

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

     

       1797
       1793
        
       		if sourceRev == pull.LatestSha() {

appview/state/router.go

···

       277
       277
        
       		s.config,

     

       278
       278
        
       		s.notifier,

     

       279
       279
        
       		s.enforcer,

     

       280
       280
       +
       		s.validator,

     

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

     

       281
       282
        
       	)

     

       282
       283
        
       	return pulls.Router(mw)

+25

appview/validator/patch.go

···

       1
       1
       +
       package validator

     

       2
       2
       +
       

     

       3
       3
       +
       import (

     

       4
       4
       +
       	"fmt"

     

       5
       5
       +
       	"strings"

     

       6
       6
       +
       

     

       7
       7
       +
       	"tangled.org/core/patchutil"

     

       8
       8
       +
       )

     

       9
       9
       +
       

     

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

     

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

     

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

     

       13
       13
       +
       	}

     

       14
       14
       +
       

     

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

     

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

     

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

     

       18
       18
       +
       	}

     

       19
       19
       +
       

     

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

     

       21
       21
       +
       		return err

     

       22
       22
       +
       	}

     

       23
       23
       +
       

     

       24
       24
       +
       	return nil

     

       25
       25
       +
       }

knotserver/xrpc/merge_check.go

···

       81
       81
        
       		}

     

       82
       82
        
       	}

     

       83
       83
        
       

     

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

     

       85
       85
       +
       

     

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

     

       85
       87
        
       	w.WriteHeader(http.StatusOK)

     

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

+18 -7

patchutil/patchutil.go

···

       1
       1
        
       package patchutil

     

       2
       2
        
       

     

       3
       3
        
       import (

     

       4
       4
       +
       	"errors"

     

       4
       5
        
       	"fmt"

     

       5
       6
        
       	"log"

     

       6
       7
        
       	"os"

     
···

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

     

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

     

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

     

       45
       45
       -
       func IsPatchValid(patch string) bool {

     

       46
       46
       +
       var (

     

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

     

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

     

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

     

       50
       50
       +
       )

     

       51
       51
       +
       

     

       52
       52
       +
       func IsPatchValid(patch string) error {

     

       46
       53
        
       	if len(patch) == 0 {

     

       47
       47
       -
       		return false

     

       54
       54
       +
       		return EmptyPatchError

     

       48
       55
        
       	}

     

       49
       56
        
       

     

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

     

       51
       58
        
       	if len(lines) < 2 {

     

       52
       52
       -
       		return false

     

       59
       59
       +
       		return EmptyPatchError

     

       53
       60
        
       	}

     

       54
       61
        
       

     

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

     
···

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

     

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

     

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

     

       63
       63
       -
       		return true

     

       70
       70
       +
       		return nil

     

       64
       71
        
       	}

     

       65
       72
        
       

     

       66
       73
        
       	// check if it's format-patch

     
···

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

     

       71
       78
        
       		patches, err := ExtractPatches(patch)

     

       72
       79
        
       		if err != nil {

     

       73
       73
       -
       			return false

     

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

     

       74
       81
        
       		}

     

       75
       75
       -
       		return len(patches) > 0

     

       82
       82
       +
       		if len(patches) == 0 {

     

       83
       83
       +
       			return EmptyPatchError

     

       84
       84
       +
       		}

     

       85
       85
       +
       

     

       86
       86
       +
       		return nil

     

       76
       87
        
       	}

     

       77
       88
        
       

     

       78
       78
       -
       	return false

     

       89
       89
       +
       	return GenericPatchError

     

       79
       90
        
       }

     

       80
       91
        
       

     

       81
       92
        
       func IsFormatPatch(patch string) bool {

+13 -12

patchutil/patchutil_test.go

···

       1
       1
        
       package patchutil

     

       2
       2
        
       

     

       3
       3
        
       import (

     

       4
       4
       +
       	"errors"

     

       4
       5
        
       	"reflect"

     

       5
       6
        
       	"testing"

     

       6
       7
        
       )

     
···

       9
       10
        
       	tests := []struct {

     

       10
       11
        
       		name     string

     

       11
       12
        
       		patch    string

     

       12
       12
       -
       		expected bool

     

       13
       13
       +
       		expected error

     

       13
       14
        
       	}{

     

       14
       15
        
       		{

     

       15
       16
        
       			name:     `empty patch`,

     

       16
       17
        
       			patch:    ``,

     

       17
       17
       -
       			expected: false,

     

       18
       18
       +
       			expected: EmptyPatchError,

     

       18
       19
        
       		},

     

       19
       20
        
       		{

     

       20
       21
        
       			name:     `single line patch`,

     

       21
       22
        
       			patch:    `single line`,

     

       22
       22
       -
       			expected: false,

     

       23
       23
       +
       			expected: EmptyPatchError,

     

       23
       24
        
       		},

     

       24
       25
        
       		{

     

       25
       26
        
       			name: `valid diff patch`,

     
···

       31
       32
        
       -old line

     

       32
       33
        
       +new line

     

       33
       34
        
       	context`,

     

       34
       34
       -
       			expected: true,

     

       35
       35
       +
       			expected: nil,

     

       35
       36
        
       		},

     

       36
       37
        
       		{

     

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

     
···

       41
       42
        
       -old line

     

       42
       43
        
       +new line

     

       43
       44
        
       	context`,

     

       44
       44
       -
       			expected: true,

     

       45
       45
       +
       			expected: nil,

     

       45
       46
        
       		},

     

       46
       47
        
       		{

     

       47
       48
        
       			name: `valid patch starting with Index`,

     
···

       53
       54
        
       -old line

     

       54
       55
        
       +new line

     

       55
       56
        
       	context`,

     

       56
       56
       -
       			expected: true,

     

       57
       57
       +
       			expected: nil,

     

       57
       58
        
       		},

     

       58
       59
        
       		{

     

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

     
···

       63
       64
        
       -old line

     

       64
       65
        
       +new line

     

       65
       66
        
       	context`,

     

       66
       66
       -
       			expected: true,

     

       67
       67
       +
       			expected: nil,

     

       67
       68
        
       		},

     

       68
       69
        
       		{

     

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

     
···

       72
       73
        
       +new line

     

       73
       74
        
       	context

     

       74
       75
        
       `,

     

       75
       75
       -
       			expected: true,

     

       76
       76
       +
       			expected: nil,

     

       76
       77
        
       		},

     

       77
       78
        
       		{

     

       78
       79
        
       			name: `valid format patch`,

     
···

       90
       91
        
       +new content

     

       91
       92
        
       --

     

       92
       93
        
       2.48.1`,

     

       93
       93
       -
       			expected: true,

     

       94
       94
       +
       			expected: nil,

     

       94
       95
        
       		},

     

       95
       96
        
       		{

     

       96
       97
        
       			name: `invalid format patch`,

     

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

     

       98
       99
        
       From: Author <author@example.com>

     

       99
       100
        
       This is not a valid patch format`,

     

       100
       100
       -
       			expected: false,

     

       101
       101
       +
       			expected: FormatPatchError,

     

       101
       102
        
       		},

     

       102
       103
        
       		{

     

       103
       104
        
       			name: `not a patch at all`,

     
···

       105
       106
        
       just some

     

       106
       107
        
       random text

     

       107
       108
        
       that isn't a patch`,

     

       108
       108
       -
       			expected: false,

     

       109
       109
       +
       			expected: GenericPatchError,

     

       109
       110
        
       		},

     

       110
       111
        
       	}

     

       111
       112
        
       

     

       112
       113
        
       	for _, tt := range tests {

     

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

     

       114
       115
        
       			result := IsPatchValid(tt.patch)

     

       115
       115
       -
       			if result != tt.expected {

     

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

     

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

     

       117
       118
        
       			}

     

       118
       119
        
       		})