Add more checks in migration code #21011

zeripath · 2022-08-31T17:00:41Z

When migrating add several more important sanity checks:

SHAs must be SHAs
Refs must be valid Refs
URLs must be reasonable

Signed-off-by: Andrew Thornton art27@cantab.net

When migrating add several more important sanity checks: * SHAs must be SHAs * Refs must be valid Refs * URLs must be reasonable Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2022-08-31T17:03:51Z

There is some duplicated code between services/migrations/dump.go and services/migrations/gitea-uploader.go which should be rationalised.

Further it's probably the case that the uploader should have some access to the downloader - so that the downloadURLs can be checked by the uploader at that point.

Signed-off-by: Andrew Thornton <art27@cantab.net>

modules/validation/binding.go

services/migrations/dump.go

Co-authored-by: techknowlogick <matti@mdranta.net>

Signed-off-by: Andrew Thornton <art27@cantab.net>

services/migrations/dump.go

services/migrations/gogs.go

services/migrations/gitbucket.go

routers/api/v1/repo/notes.go

services/migrations/common.go

services/migrations/github.go

delvh · 2022-09-03T17:27:24Z

services/migrations/gitea_uploader.go

@@ -217,11 +220,17 @@ func (g *GiteaLocalUploader) CreateMilestones(milestones ...*base.Milestone) err
 func (g *GiteaLocalUploader) CreateLabels(labels ...*base.Label) error {
 	lbs := make([]*issues_model.Label, 0, len(labels))
 	for _, label := range labels {
+		// We must validate color here:
+		if !issues_model.LabelColorPattern.MatchString("#" + label.Color) {
+			log.Warn("Invalid label color: #%s for label: %s in migration to %s/%s", label.Color, label.Name, g.repoOwner, g.repoName)


I think we should mention the baseURL here as well.

TBH all of the logs need to have the context in them - which is simply g either logged as %s or %-v

However, this is less of an issue because all of our logs log the [PID] so you can figure of what the context is from that.

Actually, we don't have the baseURL here.

This is in the Uploader which does not have access to the Downloader - this is a design issue which will need to be fixed.

delvh · 2022-09-03T17:38:47Z

services/migrations/gitea_uploader.go

-	// download patch file
+	// SECURITY: this pr must have been must have been ensured safe
+	if !pr.EnsuredSafe {
+		log.Error("PR #%d in %s/%s has not been checked for safety.", pr.Number, g.repoOwner, g.repoName)


Isn't that even one of the rare cases where a panic() would be appropriate?
If someone adds another validation that doesn't set this flag, we know that something is wrong and should be fixed as soon as possible.

I'm not fond of having to do this EnsureSafe check at all. It's not the right thing at all. We need to redesign the uploader/downloader framework to allow for stuff to be checked in a common and clean way so that no downloader can ever create an unsafe PR.

Realistically I think it's fine to just not panic so I'm not going to do it unless you really want it.

services/migrations/gitea_uploader.go

services/migrations/dump.go

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath · 2022-09-04T08:40:47Z

OK done as per reviews.

I've not done the panic. I don't think it's worth doing - we should instead restructure this code and interfaces to make it impossible to not create safe things.

Signed-off-by: Andrew Thornton <art27@cantab.net>

codecov-commenter · 2022-09-04T09:07:48Z

Codecov Report

Merging #21011 (e494fc5) into main (93a610a) will increase coverage by 47.07%.
The diff coverage is 32.90%.

@@            Coverage Diff            @@
##           main   #21011       +/-   ##
=========================================
+ Coverage      0   47.07%   +47.07%     
=========================================
  Files         0     1008     +1008     
  Lines         0   137566   +137566     
=========================================
+ Hits          0    64756    +64756     
- Misses        0    64884    +64884     
- Partials      0     7926     +7926

Impacted Files	Coverage Δ
models/activities/action.go	`49.71% <0.00%> (ø)`
modules/migration/pullrequest.go	`81.81% <ø> (ø)`
modules/migration/release.go	`0.00% <ø> (ø)`
services/migrations/codebase.go	`0.78% <0.00%> (ø)`
services/migrations/gitbucket.go	`12.50% <0.00%> (ø)`
services/migrations/gitlab.go	`5.14% <0.00%> (ø)`
services/migrations/gogs.go	`1.94% <0.00%> (ø)`
services/migrations/onedev.go	`0.82% <0.00%> (ø)`
services/migrations/github.go	`64.01% <14.63%> (ø)`
services/migrations/gitea_downloader.go	`66.47% <16.66%> (ø)`
... and 1019 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Backport go-gitea#21011 When migrating add several more important sanity checks: * SHAs must be SHAs * Refs must be valid Refs * URLs must be reasonable Signed-off-by: Andrew Thornton <art27@cantab.net>

lunny · 2022-09-04T12:08:02Z

Please send backport

Backport #21011 When migrating add several more important sanity checks: * SHAs must be SHAs * Refs must be valid Refs * URLs must be reasonable Signed-off-by: Andrew Thornton <art27@cantab.net>

* upstream/main: (22 commits) [skip ci] Updated translations via Crowdin Webhook for Wiki changes (go-gitea#20219) test: use `T.TempDir` to create temporary test directory (go-gitea#21043) Set uploadpack.allowFilter etc on gitea serv to enable partial clones with ssh (go-gitea#20902) Fix 500 on time tracking in timeline API (go-gitea#21052) Add more checks in migration code (go-gitea#21011) Fill the specified ref in webhook test payload (go-gitea#20961) [skip ci] Updated licenses and gitignores Add go licenses to licenses.txt (go-gitea#21034) Added docs for agit-setup (go-gitea#21027) Add another index for Action table on postgres (go-gitea#21033) Delete unreferenced packages when deleting a package version (go-gitea#20977) Improve arc-green code theme (go-gitea#21039) Add down key check has tribute container (go-gitea#21016) Do not add links to Posters or Assignees with ID < 0 (go-gitea#20577) [skip ci] Updated translations via Crowdin Show language name on hover (go-gitea#20923) fix: PackageMetadataVersion deps (go-gitea#21017) Fix the quick-submit for pending review comment (go-gitea#20992) Kd/ci playwright go test (go-gitea#20123) ...

@tag

commit 32eef4a Author: Lunny Xiao <xiaolunwen@gmail.com> Date: Wed Sep 7 05:32:20 2022 +0800 Add changelog for v1.17.2 (go-gitea#21089) Co-authored-by: John Olheiser <john+github@jolheiser.com> Co-authored-by: 6543 <6543@obermui.de> Co-authored-by: delvh <dev.lh@web.de> Co-authored-by: techknowlogick <techknowlogick@gitea.io> commit 449b39e Author: Tyrone Yeh <tyrone_yeh@draytek.com> Date: Tue Sep 6 16:42:05 2022 +0800 Fix sub folder in repository missing add file dropdown (go-gitea#21069) (go-gitea#21083) Backport go-gitea#21069 In repository sub folder missing add file dropdown menu, Probably broken since go-gitea#20602 commit 06f968d Author: zeripath <art27@cantab.net> Date: Tue Sep 6 07:54:47 2022 +0100 Fix hard-coded timeout and error panic in API archive download endpoint (go-gitea#20925) (go-gitea#21051) Backport go-gitea#20925 This commit updates the `GET /api/v1/repos/{owner}/{repo}/archive/{archive}` endpoint which prior to this PR had a couple of issues. 1. The endpoint had a hard-coded 20s timeout for the archiver to complete after which a 500 (Internal Server Error) was returned to client. For a scripted API client there was no clear way of telling that the operation timed out and that it should retry. 2. Whenever the timeout _did occur_, the code used to panic. This was caused by the API endpoint "delegating" to the same call path as the web, which uses a slightly different way of reporting errors (HTML rather than JSON for example). More specifically, `api/v1/repo/file.go#GetArchive` just called through to `web/repo/repo.go#Download`, which expects the `Context` to have a `Render` field set, but which is `nil` for API calls. Hence, a `nil` pointer error. The code addresses (1) by dropping the hard-coded timeout. Instead, any timeout/cancelation on the incoming `Context` is used. The code addresses (2) by updating the API endpoint to use a separate call path for the API-triggered archive download. This avoids producing HTML-errors on errors (it now produces JSON errors). Signed-off-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com> Signed-off-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com> Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com> commit 084797b Author: Lunny Xiao <xiaolunwen@gmail.com> Date: Tue Sep 6 06:48:57 2022 +0800 Fix delete user missed some comments (go-gitea#21067) (go-gitea#21068) commit 7888a55 Author: zeripath <art27@cantab.net> Date: Sun Sep 4 17:17:48 2022 +0100 Delete unreferenced packages when deleting a package version (go-gitea#20977) (go-gitea#21060) Backport go-gitea#20977 Delete a package if its last version got deleted. Otherwise removing the owner works only after the clean up job ran. Fix go-gitea#20969 Co-authored-by: KN4CK3R <admin@oldschoolhack.me> commit ea416d7 Author: zeripath <art27@cantab.net> Date: Sun Sep 4 17:17:35 2022 +0100 Redirect if user does not exist on admin pages (go-gitea#20981) (go-gitea#21059) Backport go-gitea#20981 When on /admin/users/ endpoints if the user is no longer in the DB, redirect instead of causing a http 500. Co-authored-by: KN4CK3R <admin@oldschoolhack.me> commit 0db6add Author: zeripath <art27@cantab.net> Date: Sun Sep 4 17:17:27 2022 +0100 Set uploadpack.allowFilter etc on gitea serv to enable partial clones with ssh (go-gitea#20902) (go-gitea#21058) Backport go-gitea#20902 When setting.Git.DisablePartialClone is set to false then the web server will add filter support to web http. It does this by using`-c` command arguments but this will not work on gitea serv as the upload-pack and receive-pack commands do not support this. Instead we move these options into the .gitconfig instead. Fix go-gitea#20400 Signed-off-by: Andrew Thornton <art27@cantab.net> Signed-off-by: Andrew Thornton <art27@cantab.net> commit 0ecbb71 Author: qwerty287 <80460567+qwerty287@users.noreply.github.com> Date: Sun Sep 4 17:12:37 2022 +0200 Fix 500 on time in timeline API (go-gitea#21052) (go-gitea#21057) Backport go-gitea#21052 Before converting a TrackedTime for the API we need to load its attributes - otherwise we get an NPE. Fix go-gitea#21041 commit ea38455 Author: Jason Song <i@wolfogre.com> Date: Sun Sep 4 23:12:01 2022 +0800 Fill the specified ref in webhook test payload (go-gitea#20961) (go-gitea#21055) Backport go-gitea#20961 The webhook payload should use the right ref when it‘s specified in the testing request. The compare URL should not be empty, a URL like `compare/A...A` seems useless in most cases but is helpful when testing. commit 8fc80b3 Author: zeripath <art27@cantab.net> Date: Sun Sep 4 16:11:02 2022 +0100 Add another index for Action table on postgres (go-gitea#21033) (go-gitea#21054) Backport go-gitea#21033 In go-gitea#21031 we have discovered that on very big tables postgres will use a search involving the sort term in preference to the restrictive index. Therefore we add another index for postgres and update the original migration. Fix go-gitea#21031 Signed-off-by: Andrew Thornton <art27@cantab.net> commit 71aa64a Author: zeripath <art27@cantab.net> Date: Sun Sep 4 14:59:20 2022 +0100 fix broken insecureskipverify handling in rediss connection uris (go-gitea#20967) (go-gitea#21053) Backport go-gitea#20967 Currently, it's impossible to connect to self-signed TLS encrypted redis instances. The problem lies in inproper error handling, when building redis tls options - only invalid booleans are allowed to be used in `tlsConfig` builder. The problem is, when `strconv.ParseBool(...)` returns error, it always defaults to false - meaning it's impossible to set `tlsOptions.InsecureSkipVerify` to true. Fixes go-gitea#19213 Co-authored-by: Igor Rzegocki <ajgon@users.noreply.github.com> commit 3aba72c Author: zeripath <art27@cantab.net> Date: Sun Sep 4 14:41:21 2022 +0100 Add more checks in migration code (go-gitea#21011) (go-gitea#21050) Backport go-gitea#21011 When migrating add several more important sanity checks: * SHAs must be SHAs * Refs must be valid Refs * URLs must be reasonable Signed-off-by: Andrew Thornton <art27@cantab.net> commit bd1412c Author: José Carlos <joecarlhr@gmail.com> Date: Sat Sep 3 21:11:03 2022 +0200 Add Dev, Peer and Optional dependencies to npm PackageMetadataVersion (go-gitea#21017) (go-gitea#21044) Backport go-gitea#21017 Set DevDependencies, PeerDependencies & OptionalDependencies in npm package metadatas Fix go-gitea#21013 commit 3973ce3 Author: silverwind <me@silverwind.io> Date: Sat Sep 3 19:51:09 2022 +0200 Improve arc-green code theme (go-gitea#21039) (go-gitea#21042) Backport go-gitea#21039 - Increase contrasts overall - Add various missing theme classes - Ensure strings and constants are colored the same across languages commit fbde31f Author: Tyrone Yeh <tyrone_yeh@draytek.com> Date: Sat Sep 3 21:36:27 2022 +0800 Add down key check has tribute container (go-gitea#21016) (go-gitea#21038) Backport go-gitea#21016 Fixes an issue where users would not be able to select by pressing the down arrow when using @tag above a message Bug videos: https://user-images.githubusercontent.com/1255041/188095999-c4ccde18-e53b-4251-8a14-d90c4042d768.mp4

* src/release/v1.17: (26 commits) Fix reaction of issues (go-gitea#21185) (go-gitea#21196) Fix CSV diff for added/deleted files (go-gitea#21189) (go-gitea#21193) Fix pagination limit parameter problem (go-gitea#21111) Add MD5 back to template helper functions to avoid breaking (go-gitea#21102) Add changelog for v1.17.2 (go-gitea#21089) Fix sub folder in repository missing add file dropdown (go-gitea#21069) (go-gitea#21083) Fix hard-coded timeout and error panic in API archive download endpoint (go-gitea#20925) (go-gitea#21051) Fix delete user missed some comments (go-gitea#21067) (go-gitea#21068) Delete unreferenced packages when deleting a package version (go-gitea#20977) (go-gitea#21060) Redirect if user does not exist on admin pages (go-gitea#20981) (go-gitea#21059) Set uploadpack.allowFilter etc on gitea serv to enable partial clones with ssh (go-gitea#20902) (go-gitea#21058) Fix 500 on time in timeline API (go-gitea#21052) (go-gitea#21057) Fill the specified ref in webhook test payload (go-gitea#20961) (go-gitea#21055) Add another index for Action table on postgres (go-gitea#21033) (go-gitea#21054) fix broken insecureskipverify handling in rediss connection uris (go-gitea#20967) (go-gitea#21053) Add more checks in migration code (go-gitea#21011) (go-gitea#21050) Add Dev, Peer and Optional dependencies to npm PackageMetadataVersion (go-gitea#21017) (go-gitea#21044) Improve arc-green code theme (go-gitea#21039) (go-gitea#21042) Add down key check has tribute container (go-gitea#21016) (go-gitea#21038) Do not add links to Posters or Assignees with ID < 0 (go-gitea#20577) (go-gitea#21037) ...

Add more checks in migration code

f971a3b

When migrating add several more important sanity checks: * SHAs must be SHAs * Refs must be valid Refs * URLs must be reasonable Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath added type/bug backport/v1.17 labels Aug 31, 2022

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 31, 2022

zeripath added 5 commits August 31, 2022 18:10

oops

657d54d

Signed-off-by: Andrew Thornton <art27@cantab.net>

oops 2

6305ad2

Signed-off-by: Andrew Thornton <art27@cantab.net>

only set head.SHA when the SHA is available

992ba59

Signed-off-by: Andrew Thornton <art27@cantab.net>

ensure test-backend passes

4ecf851

Signed-off-by: Andrew Thornton <art27@cantab.net>

fix dump

fddbb29

Signed-off-by: Andrew Thornton <art27@cantab.net>

lunny added this to the 1.18.0 milestone Sep 1, 2022

lunny added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Sep 1, 2022

zeripath removed the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Sep 1, 2022

Merge branch 'main' into migration-checks

0ca166c

techknowlogick reviewed Sep 2, 2022

View reviewed changes

modules/validation/binding.go Show resolved Hide resolved

techknowlogick reviewed Sep 2, 2022

View reviewed changes

services/migrations/dump.go Show resolved Hide resolved

zeripath and others added 4 commits September 2, 2022 21:31

Update modules/validation/binding.go

6995b11

Co-authored-by: techknowlogick <matti@mdranta.net>

Update services/migrations/dump.go

b0df6d0

Co-authored-by: techknowlogick <matti@mdranta.net>

Merge branch 'main' into migration-checks

cf5a1d2

Add check against external->local

9dddb96

Signed-off-by: Andrew Thornton <art27@cantab.net>

zeripath force-pushed the migration-checks branch from 80fdf57 to 9dddb96 Compare September 2, 2022 21:09

zeripath added 4 commits September 2, 2022 22:30

use common function for testing against baseurl

1d44aca

Signed-off-by: Andrew Thornton <art27@cantab.net>

add check to restore

4b31472

Signed-off-by: Andrew Thornton <art27@cantab.net>

oops

63715dd

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge branch 'main' into migration-checks

f1de1b2

techknowlogick approved these changes Sep 3, 2022

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 3, 2022

jolheiser reviewed Sep 3, 2022

View reviewed changes

services/migrations/dump.go Outdated Show resolved Hide resolved

services/migrations/dump.go Show resolved Hide resolved

delvh reviewed Sep 3, 2022

View reviewed changes

zeripath added 2 commits September 4, 2022 09:39

as per review

e32d0a3

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge remote-tracking branch 'origin/main' into migration-checks

38353f3

placate the linter

be367a3

Signed-off-by: Andrew Thornton <art27@cantab.net>

Merge branch 'main' into migration-checks

e494fc5

delvh approved these changes Sep 4, 2022

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 4, 2022

lafriks merged commit e6b3be4 into go-gitea:main Sep 4, 2022

zeripath deleted the migration-checks branch September 4, 2022 11:13

zeripath mentioned this pull request Sep 4, 2022

Add more checks in migration code (#21011) #21050

Merged

zeripath added the backport/done All backports for this PR have been created label Sep 4, 2022

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more checks in migration code #21011

Add more checks in migration code #21011

zeripath commented Aug 31, 2022

zeripath commented Aug 31, 2022

delvh Sep 3, 2022

zeripath Sep 4, 2022

zeripath Sep 4, 2022

delvh Sep 3, 2022

zeripath Sep 4, 2022

zeripath commented Sep 4, 2022

codecov-commenter commented Sep 4, 2022 •

edited

Loading

lunny commented Sep 4, 2022

Add more checks in migration code #21011

Add more checks in migration code #21011

Conversation

zeripath commented Aug 31, 2022

zeripath commented Aug 31, 2022

delvh Sep 3, 2022

Choose a reason for hiding this comment

zeripath Sep 4, 2022

Choose a reason for hiding this comment

zeripath Sep 4, 2022

Choose a reason for hiding this comment

delvh Sep 3, 2022

Choose a reason for hiding this comment

zeripath Sep 4, 2022

Choose a reason for hiding this comment

zeripath commented Sep 4, 2022

codecov-commenter commented Sep 4, 2022 • edited Loading

Codecov Report

lunny commented Sep 4, 2022

codecov-commenter commented Sep 4, 2022 •

edited

Loading