From ede9ec3e105d2121be626733a5fec119214ec153 Mon Sep 17 00:00:00 2001 From: silverwind Date: Fri, 6 Nov 2026 20:45:17 +0000 Subject: [PATCH] fix: stop discarding file close and test setup errors (#39987) - Fix https://github.com/go-gitea/gitea/security/quality/rules/go%2Funhandled-writable-file-close, ignored close errors hide failed flushes, which could lose rotated logs and truncate authorized_keys - Fix `TestProjects` fixture reset, from https://github.com/go-gitea/gitea/security/quality/ai-findings, `TruncateBeans` rejected the table names and the ignored error let the test run against fixture data --- cmd/embedded.go | 8 +------- models/asymkey/ssh_key_authorized_keys.go | 5 +++-- modules/assetfs/embed.go | 5 +++-- modules/util/rotatingfilewriter/writer.go | 9 +++------ services/asymkey/ssh_key_authorized_keys.go | 4 +++- .../asymkey/ssh_key_authorized_principals.go | 4 +++- services/pull/merge_prepare.go | 16 ++++++++-------- services/pull/temp_repo.go | 5 +++-- tests/integration/project_test.go | 10 +++++----- 9 files changed, 32 insertions(+), 34 deletions(-) diff --git a/cmd/embedded.go b/cmd/embedded.go index 56d7e4cb364..996453b7514 100644 --- a/cmd/embedded.go +++ b/cmd/embedded.go @@ -261,13 +261,7 @@ func extractAsset(c *cli.Command, d string, a assetFile, overwrite, rename bool) perms = fi.Mode() } - file, err := os.OpenFile(dest, os.O_WRONLY|os.O_TRUNC|os.O_CREATE, perms) - if err != nil { - return fmt.Errorf("%s: %w", dest, err) - } - defer file.Close() - - if _, err = file.Write(data); err != nil { + if err := os.WriteFile(dest, data, perms); err != nil { return fmt.Errorf("%s: %w", dest, err) } diff --git a/models/asymkey/ssh_key_authorized_keys.go b/models/asymkey/ssh_key_authorized_keys.go index 850c982b966..9b0d0070801 100644 --- a/models/asymkey/ssh_key_authorized_keys.go +++ b/models/asymkey/ssh_key_authorized_keys.go @@ -6,6 +6,7 @@ package asymkey import ( "bufio" "context" + "errors" "fmt" "io" "os" @@ -107,7 +108,7 @@ func writeAuthorizedStringForKey(key *PublicKey, w io.Writer) (keyValid bool, er } // appendAuthorizedKeysToFile appends new SSH keys' content to authorized_keys file. -func appendAuthorizedKeysToFile(keys ...*PublicKey) error { +func appendAuthorizedKeysToFile(keys ...*PublicKey) (err error) { // Don't need to rewrite this file if builtin SSH server is enabled. if setting.SSH.StartBuiltinServer || !setting.SSH.CreateAuthorizedKeysFile { return nil @@ -133,7 +134,7 @@ func appendAuthorizedKeysToFile(keys ...*PublicKey) error { if err != nil { return err } - defer f.Close() + defer func() { err = errors.Join(err, f.Close()) }() // Note: chmod command does not support in Windows. if !consts.IsWindows { diff --git a/modules/assetfs/embed.go b/modules/assetfs/embed.go index 28d0fc25f03..1e7797ddd1c 100644 --- a/modules/assetfs/embed.go +++ b/modules/assetfs/embed.go @@ -6,6 +6,7 @@ package assetfs import ( "bytes" "compress/gzip" + "errors" "io" "io/fs" "os" @@ -301,12 +302,12 @@ var GetExecutableModTime = sync.OnceValue(func() (modTime time.Time) { return st.ModTime() }) -func GenerateEmbedBindata(fsRootPath, outputFile string) error { +func GenerateEmbedBindata(fsRootPath, outputFile string) (err error) { output, err := os.OpenFile(outputFile, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.ModePerm) if err != nil { return err } - defer output.Close() + defer func() { err = errors.Join(err, output.Close()) }() meta := &EmbeddedMeta{} meta.Root = &embeddedFileInfo{} diff --git a/modules/util/rotatingfilewriter/writer.go b/modules/util/rotatingfilewriter/writer.go index fc5c10881b3..2b4a8e61ed0 100644 --- a/modules/util/rotatingfilewriter/writer.go +++ b/modules/util/rotatingfilewriter/writer.go @@ -191,19 +191,16 @@ func compressOldFile(fname string, compressionLevel int) error { if err != nil { return fmt.Errorf("compressOldFile: failed to open new file %s: %w", fnameGz, err) } - defer fw.Close() zw, err := gzip.NewWriterLevel(fw, compressionLevel) if err != nil { + _ = fw.Close() return fmt.Errorf("compressOldFile: failed to create gzip writer: %w", err) } - defer zw.Close() _, err = buffer.WriteTo(zw) - if err != nil { - _ = zw.Close() - _ = fw.Close() - _ = util.RemoveWithRetry(fname + ".gz") + if err = errors.Join(err, zw.Close(), fw.Close()); err != nil { + _ = util.RemoveWithRetry(fnameGz) return fmt.Errorf("compressOldFile: failed to write to gz file: %w", err) } _ = reader.Close() diff --git a/services/asymkey/ssh_key_authorized_keys.go b/services/asymkey/ssh_key_authorized_keys.go index 79b21cbaf84..de3dc8d1546 100644 --- a/services/asymkey/ssh_key_authorized_keys.go +++ b/services/asymkey/ssh_key_authorized_keys.go @@ -74,6 +74,8 @@ func rewriteAllPublicKeys(ctx context.Context) error { return err } - _ = t.Close() + if err := t.Close(); err != nil { + return err + } return util.RenameWithRetry(tmpPath, fPath) } diff --git a/services/asymkey/ssh_key_authorized_principals.go b/services/asymkey/ssh_key_authorized_principals.go index 1a3dfd80e93..6eccb8084db 100644 --- a/services/asymkey/ssh_key_authorized_principals.go +++ b/services/asymkey/ssh_key_authorized_principals.go @@ -85,7 +85,9 @@ func rewriteAllPrincipalKeys(ctx context.Context) error { return err } - _ = t.Close() + if err := t.Close(); err != nil { + return err + } return util.RenameWithRetry(tmpPath, fPath) } diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index be5df102db0..59b0e45297d 100644 --- a/services/pull/merge_prepare.go +++ b/services/pull/merge_prepare.go @@ -152,16 +152,16 @@ func prepareTemporaryRepoForMerge(ctx *mergeContext) error { log.Error("%-v Unable to write .git/info/sparse-checkout file in %s: %v", ctx.pr, ctx.tmpBasePath, err) return fmt.Errorf("Unable to write .git/info/sparse-checkout file in tmpBasePath: %w", err) } - defer sparseCheckoutListFile.Close() // we will close it earlier but we need to ensure it is closed if there is an error - if err := getDiffTree(ctx, ctx.tmpRepo, tmpRepoBaseBranch, tmpRepoTrackingBranch, sparseCheckoutListFile); err != nil { - log.Error("%-v getDiffTree(%s, %s, %s): %v", ctx.pr, ctx.tmpBasePath, tmpRepoBaseBranch, tmpRepoTrackingBranch, err) - return fmt.Errorf("getDiffTree: %w", err) + diffErr := getDiffTree(ctx, ctx.tmpRepo, tmpRepoBaseBranch, tmpRepoTrackingBranch, sparseCheckoutListFile) + closeErr := sparseCheckoutListFile.Close() + if diffErr != nil { + log.Error("%-v getDiffTree(%s, %s, %s): %v", ctx.pr, ctx.tmpBasePath, tmpRepoBaseBranch, tmpRepoTrackingBranch, diffErr) + return fmt.Errorf("getDiffTree: %w", diffErr) } - - if err := sparseCheckoutListFile.Close(); err != nil { - log.Error("%-v Unable to close .git/info/sparse-checkout file in %s: %v", ctx.pr, ctx.tmpBasePath, err) - return fmt.Errorf("Unable to close .git/info/sparse-checkout file in tmpBasePath: %w", err) + if closeErr != nil { + log.Error("%-v Unable to close .git/info/sparse-checkout file in %s: %v", ctx.pr, ctx.tmpBasePath, closeErr) + return fmt.Errorf("Unable to close .git/info/sparse-checkout file in tmpBasePath: %w", closeErr) } setConfig := func(key, value string) error { diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index ac530838b86..6484eb38788 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -7,6 +7,7 @@ package pull import ( "bytes" "context" + "errors" "fmt" "os" "path/filepath" @@ -103,8 +104,8 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) if err != nil { return fmt.Errorf("unable to open %s file in %s: %w", altFileRelPath, filepath.Base(repoPath), err) } - defer f.Close() - if _, err := fmt.Fprintln(f, newAltLine); err != nil { + _, err = fmt.Fprintln(f, newAltLine) + if err = errors.Join(err, f.Close()); err != nil { return fmt.Errorf("unable to write %s file in %s: %w", altFileRelPath, filepath.Base(repoPath), err) } return nil diff --git a/tests/integration/project_test.go b/tests/integration/project_test.go index ffbbdec9748..cfb5d96674a 100644 --- a/tests/integration/project_test.go +++ b/tests/integration/project_test.go @@ -395,18 +395,18 @@ func TestProjects(t *testing.T) { assert.Contains(t, resp.Body.String(), linkedPR) }) t.Run("LoadIssuesAssigneesForProject", func(t *testing.T) { - _ = db.TruncateBeans(t.Context(), "project_issue", "issue_assignees") - _ = db.Insert(t.Context(), + require.NoError(t, db.TruncateBeans(t.Context(), &project_model.ProjectIssue{}, &issues_model.IssueAssignees{})) + require.NoError(t, db.Insert(t.Context(), &project_model.ProjectIssue{ProjectID: 1, IssueID: 1}, &project_model.ProjectIssue{ProjectID: 1, IssueID: 6}, - ) - _ = db.Insert(t.Context(), + )) + require.NoError(t, db.Insert(t.Context(), &issues_model.IssueAssignees{IssueID: 1, AssigneeID: 1}, &issues_model.IssueAssignees{IssueID: 1, AssigneeID: 10}, &issues_model.IssueAssignees{IssueID: 1, AssigneeID: 2}, &issues_model.IssueAssignees{IssueID: 6, AssigneeID: 2}, &issues_model.IssueAssignees{IssueID: 6, AssigneeID: 4}, - ) + )) assignees, err := project.LoadIssuesAssigneesForProject(t.Context(), 1) require.NoError(t, err) require.Len(t, assignees, 4)