From dae90d497c4a7f8f784c01fe9f31dd623511a91a Mon Sep 17 00:00:00 2001
From: AMiWR <amiwrpremium@gmail.com>
Date: Tue, 21 Apr 2026 11:34:36 +0000
Subject: [PATCH] fix(codeql): check Close() on writable file handles
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

CodeQL's go/unhandled-writable-file-close flagged three spots
where a writable file handle's Close() return was ignored via
`defer`. Two are instance-lock files (never written; close
genuinely can't lose data but CodeQL's flow analysis traces the
O_RDWR open and warns anyway). One is a real upload-target in
internal/files/files.go — ignoring that Close() would mask flush
or fsync errors, the only way a successful Copy → silent data
loss can happen.

- `internal/files/files.go:126` — real fix. Replace
  `defer out.Close()` with an explicit close-and-check on each
  exit path; surface close errors as a failed upload.
- `cmd/shellboto/main.go:95` — lockfile Close() wrapped in
  a defer that logs on error (via zap Warn).
- `cmd/shellboto/cmd_db.go:174` — same, logs to stderr.

Closes 3 of the 7 open CodeQL alerts. The remaining 4
(go/incorrect-integer-conversion on uid/gid → int narrowing)
are false positives on our 64-bit-only build targets; dismissed
via the security tab separately.
---
 cmd/shellboto/cmd_db.go | 8 +++++++-
 cmd/shellboto/main.go   | 8 +++++++-
 internal/files/files.go | 9 ++++++++-
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/cmd/shellboto/cmd_db.go b/cmd/shellboto/cmd_db.go
index 6cab6cc..202af05 100644
--- a/cmd/shellboto/cmd_db.go
+++ b/cmd/shellboto/cmd_db.go
@@ -171,7 +171,13 @@ func cmdDBVacuum(args []string) int {
 		fmt.Fprintln(os.Stderr, err)
 		return exitErr
 	}
-	defer lock.Close()
+	// Lockfile is never written — only flock'd. Close() returning an error
+	// is unexpected; surface it rather than silently defer.
+	defer func() {
+		if err := lock.Close(); err != nil {
+			fmt.Fprintf(os.Stderr, "lockfile close: %v\n", err)
+		}
+	}()
 
 	gormDB, cleanup, err := openDBForCLI(cfg.DBPath)
 	if err != nil {
diff --git a/cmd/shellboto/main.go b/cmd/shellboto/main.go
index e0c2057..dc67e43 100644
--- a/cmd/shellboto/main.go
+++ b/cmd/shellboto/main.go
@@ -92,7 +92,13 @@ func main() {
 	if err != nil {
 		logger.Fatal("instance lock", zap.Error(err))
 	}
-	defer instanceLock.Close()
+	// Lockfile is never written — only flock'd. Close() returning an error
+	// is unexpected; log it rather than silently defer.
+	defer func() {
+		if err := instanceLock.Close(); err != nil {
+			logger.Warn("instance lock close", zap.Error(err))
+		}
+	}()
 
 	gormDB, err := db.Open(cfg.DBPath)
 	if err != nil {
diff --git a/internal/files/files.go b/internal/files/files.go
index f6e371c..07bf563 100644
--- a/internal/files/files.go
+++ b/internal/files/files.go
@@ -123,7 +123,6 @@ func Receive(ctx context.Context, b *bot.Bot, doc *models.Document, caption stri
 	if err != nil {
 		return "", 0, fmt.Errorf("open dest: %w", err)
 	}
-	defer out.Close()
 	// Defense-in-depth — Telegram's bot API caps uploads at
 	// 50 MB, but a misconfigured self-hosted bot API server (or a
 	// future protocol change) could serve larger. Bound the Copy with
@@ -133,13 +132,21 @@ func Receive(ctx context.Context, b *bot.Bot, doc *models.Document, caption stri
 	limit := int64(maxDownloadBytes) + 1
 	n, err := io.Copy(out, io.LimitReader(resp.Body, limit))
 	if err != nil {
+		_ = out.Close()
 		_ = os.Remove(dest)
 		return "", 0, err
 	}
 	if n > int64(maxDownloadBytes) {
+		_ = out.Close()
 		_ = os.Remove(dest)
 		return "", 0, fmt.Errorf("upload exceeds %d-byte cap", maxDownloadBytes)
 	}
+	// Check Close() on writable files so a flush / fsync failure surfaces
+	// as an error rather than silent data loss.
+	if err := out.Close(); err != nil {
+		_ = os.Remove(dest)
+		return "", 0, fmt.Errorf("close dest: %w", err)
+	}
 	if chown != nil {
 		_ = os.Chown(dest, int(chown.Uid), int(chown.Gid))
 	}