appview: improve patch validation #670

+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() {

+1

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 + }

+2

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 })