Refuse credentials in a storage URL's endpoint or rclone remote name (closes #322)

storage_url refused only the outer URL's userinfo and unknown query
parameters. Userinfo inside the s3 endpoint= value, and rclone's
remote,param=value parameters in the remote name, were accepted and
then printed by info, remote info and snapshot create. Both are now
refused without echoing the value: a host name never contains "@", and
an rclone remote name never contains a comma. The comma is checked
before url.Parse, because url.Parse refuses a name such as
":s3,secret_access_key=..." or "x,secret=a:b", and its error prints
the name from the colon on. The rclone case has its own error pointing
at rclone config, since the existing credentials error names the s3.*
keys.

Model: opus-5-5

internal/database/blob_chunks.go

@@ -58,7 +58,12 @@ func (r *BlobChunkRepository) GetByBlobID(
 		return nil, fmt.Errorf("querying blob chunks: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var blobChunks []*BlobChunk
 

internal/database/blobs.go

@@ -83,7 +83,12 @@ func (r *BlobRepository) GetAll(ctx context.Context) (map[string]*Blob, error) {
 		return nil, fmt.Errorf("querying blobs: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	out := make(map[string]*Blob)
 

internal/database/chunk_files.go

@@ -61,7 +61,12 @@ func (r *ChunkFileRepository) GetByChunkHash(
 		return nil, fmt.Errorf("querying chunk files: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	return r.scanChunkFiles(rows)
 }
@@ -82,7 +87,12 @@ func (r *ChunkFileRepository) GetByFilePath(
 		return nil, fmt.Errorf("querying chunk files: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	return r.scanChunkFiles(rows)
 }
@@ -102,7 +112,12 @@ func (r *ChunkFileRepository) GetByFileID(
 		return nil, fmt.Errorf("querying chunk files: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	return r.scanChunkFiles(rows)
 }

internal/database/chunks.go

@@ -107,7 +107,12 @@ func (r *ChunkRepository) GetByHashes(
 		return nil, fmt.Errorf("querying chunks: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var chunks []*Chunk
 
@@ -147,7 +152,12 @@ func (r *ChunkRepository) ListUnpacked(
 		return nil, fmt.Errorf("querying unpacked chunks: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var chunks []*Chunk
 

internal/database/chunks_ext.go

@@ -38,7 +38,12 @@ func (r *ChunkRepository) list(ctx context.Context, query string) ([]*Chunk, err
 		return nil, fmt.Errorf("querying chunks: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var chunks []*Chunk
 

internal/database/errors.go

@@ -0,0 +1,12 @@
+package database
+
+import (
+	"fmt"
+	"os"
+)
+
+// Fatalf prints an error message to stderr and exits with status 1
+func Fatalf(format string, args ...any) {
+	fmt.Fprintf(os.Stderr, "FATAL: "+format+"\n", args...)
+	os.Exit(1)
+}

internal/database/file_chunks.go

@@ -62,7 +62,12 @@ func (r *FileChunkRepository) GetByPath(
 		return nil, fmt.Errorf("querying file chunks: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	return r.scanFileChunks(rows)
 }
@@ -83,7 +88,12 @@ func (r *FileChunkRepository) GetByFileID(
 		return nil, fmt.Errorf("querying file chunks: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	return r.scanFileChunks(rows)
 }
@@ -107,7 +117,12 @@ func (r *FileChunkRepository) GetByPathTx(
 		return nil, fmt.Errorf("querying file chunks: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	fileChunks, err := r.scanFileChunks(rows)
 	LogSQL("GetByPathTx", "Complete", path, "count", len(fileChunks))

internal/database/files.go

@@ -175,7 +175,12 @@ func (r *FileRepository) ListModifiedSince(
 		return nil, fmt.Errorf("querying files: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var files []*File
 
@@ -251,7 +256,12 @@ func (r *FileRepository) ListUnderPath(
 		return nil, fmt.Errorf("querying files: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var files []*File
 
@@ -294,7 +304,12 @@ func (r *FileRepository) ListIDsWithChunksNotInUploadedBlobs(
 		return nil, fmt.Errorf("querying files: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var ids []types.FileID
 
@@ -325,7 +340,12 @@ func (r *FileRepository) ListAll(ctx context.Context) ([]*File, error) {
 		return nil, fmt.Errorf("querying files: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var files []*File
 

internal/database/repositories.go

@@ -106,24 +106,22 @@ func (r *Repositories) WithReadTx(ctx context.Context, fn TxFunc) error {
 
 	defer func() {
 		if p := recover(); p != nil {
-			// The panic is the failure to report, so a rollback error
-			// is dropped.
-			_ = tx.Rollback()
+			rollbackErr := tx.Rollback()
+			if rollbackErr != nil {
+				Fatalf("failed to rollback transaction: %v", rollbackErr)
+			}
 
 			panic(p)
+		} else if err != nil {
+			rollbackErr := tx.Rollback()
+			if rollbackErr != nil {
+				Fatalf("failed to rollback transaction: %v", rollbackErr)
+			}
 		}
 	}()
 
 	err = fn(ctx, tx)
 	if err != nil {
-		// Once ctx is cancelled, database/sql rolls the transaction
-		// back itself and Rollback returns sql.ErrTxDone.
-		rollbackErr := tx.Rollback()
-		if rollbackErr != nil && !errors.Is(rollbackErr, sql.ErrTxDone) {
-			return fmt.Errorf("%w; rolling back transaction: %w",
-				err, rollbackErr)
-		}
-
 		return err
 	}
 

internal/database/repositories_test.go

@@ -411,41 +411,3 @@ func TestRepositoriesReadTransaction(t *testing.T) {
 		t.Error("expected to retrieve file in read transaction")
 	}
 }
-
-// When the context is cancelled during a read transaction's body,
-// database/sql rolls the transaction back itself, so WithReadTx's own
-// rollback finds it already done. WithReadTx must return the body's error
-// alone, without that rollback.
-func TestRepositoriesReadTransactionContextCancelled(t *testing.T) {
-	t.Parallel()
-
-	db, cleanup := setupTestDB(t)
-	defer cleanup()
-
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	repos := database.NewRepositories(db)
-
-	err := repos.WithReadTx(ctx, func(ctx context.Context, tx *sql.Tx) error {
-		cancel()
-
-		// database/sql rolls back from a goroutine of its own; wait
-		// until it has, so the test does not depend on that race.
-		for {
-			_, err := tx.ExecContext(context.WithoutCancel(ctx), "SELECT 1")
-			if errors.Is(err, sql.ErrTxDone) {
-				return ctx.Err()
-			}
-
-			time.Sleep(time.Millisecond)
-		}
-	})
-	if !errors.Is(err, context.Canceled) {
-		t.Fatalf("expected context.Canceled, got: %v", err)
-	}
-
-	if errors.Is(err, sql.ErrTxDone) {
-		t.Fatalf("the already-done rollback was reported: %v", err)
-	}
-}

internal/database/snapshots.go

@@ -239,7 +239,12 @@ func (r *SnapshotRepository) GetOnlySnapshot(ctx context.Context) (*Snapshot, er
 		return nil, fmt.Errorf("querying snapshots: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	snapshots, err := r.scanSnapshotRows(rows)
 	if err != nil {
@@ -274,7 +279,12 @@ func (r *SnapshotRepository) ListRecent(
 		return nil, fmt.Errorf("querying snapshots: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	return r.scanSnapshotRows(rows)
 }
@@ -489,7 +499,12 @@ func (r *SnapshotRepository) GetBlobHashes(
 		return nil, fmt.Errorf("querying blob hashes: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	var blobs []string
 
@@ -638,7 +653,12 @@ func (r *SnapshotRepository) GetIncompleteSnapshots(
 		return nil, fmt.Errorf("querying incomplete snapshots: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	return r.scanSnapshotRows(rows)
 }
@@ -661,7 +681,12 @@ func (r *SnapshotRepository) GetIncompleteByHostname(
 		return nil, fmt.Errorf("querying incomplete snapshots: %w", err)
 	}
 
-	defer func() { _ = rows.Close() }()
+	defer func() {
+		err := rows.Close()
+		if err != nil {
+			Fatalf("failed to close rows: %v", err)
+		}
+	}()
 
 	// Same column set as every other multi-row snapshot query, so the
 	// shared scanner applies — including its timestamp normalization.

internal/snapshot/blob_temp_file_test.go

@@ -1,62 +0,0 @@
-//nolint:testpackage // exercises the unexported cleanupBlobTempFile
-package snapshot
-
-import (
-	"errors"
-	"os"
-	"testing"
-
-	"github.com/spf13/afero"
-	"sneak.berlin/go/vaultik/internal/blob"
-)
-
-// A blob temp file that cannot be closed or removed must make the cleanup
-// return an error, not end the process.
-func TestCleanupBlobTempFileReturnsErrors(t *testing.T) {
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	s := &Scanner{fs: fs}
-
-	tests := []struct {
-		name string
-		// prepare runs before the cleanup and makes its close or remove
-		// fail.
-		prepare func(tempFile afero.File) error
-		want    error
-	}{
-		{
-			name:    "close fails",
-			prepare: func(tempFile afero.File) error { return tempFile.Close() },
-			want:    os.ErrClosed,
-		},
-		{
-			name: "remove fails",
-			prepare: func(tempFile afero.File) error {
-				return fs.Remove(tempFile.Name())
-			},
-			want: os.ErrNotExist,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			tempFile, err := afero.TempFile(fs, t.TempDir(), "blob-*.tmp")
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			err = tt.prepare(tempFile)
-			if err != nil {
-				t.Fatal(err)
-			}
-
-			err = s.cleanupBlobTempFile(&blob.WithReader{TempFile: tempFile})
-			if !errors.Is(err, tt.want) {
-				t.Fatalf("expected an error wrapping %v, got: %v", tt.want, err)
-			}
-		})
-	}
-}

internal/snapshot/scanner.go

@@ -1553,24 +1553,20 @@ func (s *Scanner) handleBlobReady(
 	blobExists, err := s.uploadBlobIfNeeded(
 		ctx, blobPath, blobWithReader, startTime, result)
 	if err != nil {
-		cleanupErr := s.cleanupBlobTempFile(blobWithReader)
+		s.cleanupBlobTempFile(blobWithReader)
 
-		return errors.Join(
-			fmt.Errorf("uploading blob %s: %w", finishedBlob.Hash, err), cleanupErr)
+		return fmt.Errorf("uploading blob %s: %w", finishedBlob.Hash, err)
 	}
 
 	err = s.recordBlobMetadata(ctx, finishedBlob, blobExists, startTime)
 	if err != nil {
-		cleanupErr := s.cleanupBlobTempFile(blobWithReader)
+		s.cleanupBlobTempFile(blobWithReader)
 
-		return errors.Join(err, cleanupErr)
-	}
-
-	err = s.cleanupBlobTempFile(blobWithReader)
-	if err != nil {
 		return err
 	}
 
+	s.cleanupBlobTempFile(blobWithReader)
+
 	// Chunks from this blob are now committed to DB - remove from pending set
 	s.removePendingChunkHashes(blobWithReader.InsertedChunkHashes)
 
@@ -1763,22 +1759,20 @@ func (s *Scanner) recordBlobMetadata(
 }
 
 // cleanupBlobTempFile closes and removes the blob's temporary file
-func (s *Scanner) cleanupBlobTempFile(blobWithReader *blob.WithReader) error {
+func (s *Scanner) cleanupBlobTempFile(blobWithReader *blob.WithReader) {
 	if blobWithReader.TempFile != nil {
 		tempName := blobWithReader.TempFile.Name()
 
 		err := blobWithReader.TempFile.Close()
 		if err != nil {
-			return fmt.Errorf("closing blob temp file: %w", err)
+			log.Fatal("Failed to close temp file", "file", tempName, "error", err)
 		}
 
 		err = s.fs.Remove(tempName)
 		if err != nil {
-			return fmt.Errorf("removing blob temp file: %w", err)
+			log.Fatal("Failed to remove temp file", "file", tempName, "error", err)
 		}
 	}
-
-	return nil
 }
 
 // streamingChunkInfo tracks chunk metadata collected during streaming

internal/storage/url.go

@@ -29,7 +29,10 @@ var (
 	ErrURLCredentials     = errors.New(
 		"storage URL must not carry credentials; " +
 			"set s3.access_key_id and s3.secret_access_key in the config instead")
-	ErrURLUnknownParam = errors.New("unknown query parameter in storage URL")
+	ErrURLUnknownParam    = errors.New("unknown query parameter in storage URL")
+	ErrRcloneRemoteParams = errors.New(
+		"rclone URL must not carry parameters in the remote name; " +
+			"set them on the remote with rclone config instead")
 )
 
 // URL represents a parsed storage URL.
@@ -90,8 +93,9 @@ func parseFileURL(rawPath string) (*URL, error) {
 }
 
 // parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
-// the userinfo and any query parameter other than endpoint, region and
-// ssl, so a credential-bearing URL is never stored or echoed.
+// the userinfo or in the endpoint parameter's userinfo, and any query
+// parameter other than endpoint, region and ssl, so a credential-bearing
+// URL is never stored or echoed.
 func parseS3URL(rawURL string) (*URL, error) {
 	u, err := url.Parse(rawURL)
 	if err != nil {
@@ -114,6 +118,14 @@ func parseS3URL(rawURL string) (*URL, error) {
 		return nil, err
 	}
 
+	// A host name never contains "@", so one in an endpoint, with or
+	// without a scheme, can only introduce userinfo.
+	for _, endpoint := range query["endpoint"] {
+		if strings.Contains(endpoint, "@") {
+			return nil, ErrURLCredentials
+		}
+	}
+
 	return &URL{
 		Scheme:   schemeS3,
 		Bucket:   bucket,
@@ -125,9 +137,23 @@ func parseS3URL(rawURL string) (*URL, error) {
 }
 
 // parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no
-// query parameters, so credentials in the userinfo and any parameter at
-// all are rejected rather than silently ignored.
+// query parameters, so credentials in the userinfo, parameters in the
+// remote name and any query parameter at all are rejected rather than
+// silently ignored.
 func parseRcloneURL(rawURL string) (*URL, error) {
+	// The remote name runs up to the first "/", "?" or "#". It never
+	// contains a comma; one starts rclone's remote,param=value form,
+	// whose values can be credentials. This is checked before url.Parse,
+	// whose error for such a name can print part of it.
+	remote := strings.TrimPrefix(rawURL, "rclone://")
+	if end := strings.IndexAny(remote, "/?#"); end >= 0 {
+		remote = remote[:end]
+	}
+
+	if strings.Contains(remote, ",") {
+		return nil, ErrRcloneRemoteParams
+	}
+
 	u, err := url.Parse(rawURL)
 	if err != nil {
 		return nil, wrapParseError(err)
@@ -137,7 +163,7 @@ func parseRcloneURL(rawURL string) (*URL, error) {
 		return nil, ErrURLCredentials
 	}
 
-	remote := u.Host
+	remote = u.Host
 	if remote == "" {
 		return nil, ErrMissingRemote
 	}

internal/storage/url_parse_test.go

@@ -150,6 +150,19 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 			wantErr: storage.ErrURLCredentials,
 			secrets: []string{key, secret, "mybucket"},
 		},
+		{
+			name: "s3 userinfo in endpoint",
+			raw: "s3://mybucket?endpoint=https://" +
+				key + ":" + secret + "@minio.example.com",
+			wantErr: storage.ErrURLCredentials,
+			secrets: []string{key, secret},
+		},
+		{
+			name:    "s3 userinfo in endpoint without scheme",
+			raw:     "s3://mybucket?endpoint=" + key + ":" + secret + "@minio.example.com",
+			wantErr: storage.ErrURLCredentials,
+			secrets: []string{key, secret},
+		},
 		{
 			name:    "s3 unknown query param",
 			raw:     "s3://mybucket?access_key=" + key + "&secret=" + secret,
@@ -203,6 +216,54 @@ func TestParseStorageURLRejectsCredentials(t *testing.T) {
 	}
 }
 
+// TestParseStorageURLRejectsRcloneRemoteParams checks that an rclone
+// remote name carrying parameters (rclone's "remote,param=value" form,
+// which can hold a credential) is rejected without echoing the value,
+// including the URLs that url.Parse itself refuses.
+func TestParseStorageURLRejectsRcloneRemoteParams(t *testing.T) {
+	t.Parallel()
+
+	cases := []struct {
+		name    string
+		raw     string
+		secrets []string // must not appear in the error message
+	}{
+		{
+			name:    "configured remote",
+			raw:     "rclone://gdrive,client_secret=topsecret/backups",
+			secrets: []string{"topsecret"},
+		},
+		{
+			name:    "remote defined inline",
+			raw:     "rclone://:s3,access_key_id=KEY,secret_access_key=SECRET/bucket",
+			secrets: []string{"KEY", "SECRET"},
+		},
+		{
+			name:    "colon in a parameter value",
+			raw:     "rclone://gdrive,client_secret=ab:cd/backups",
+			secrets: []string{"cd"},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := storage.ParseStorageURL(tc.raw)
+			if !errors.Is(err, storage.ErrRcloneRemoteParams) {
+				t.Fatalf("ParseStorageURL(%q) error = %v, want %v",
+					tc.raw, err, storage.ErrRcloneRemoteParams)
+			}
+
+			for _, secret := range tc.secrets {
+				if strings.Contains(err.Error(), secret) {
+					t.Errorf("error message leaked %q: %v", secret, err.Error())
+				}
+			}
+		})
+	}
+}
+
 // TestParseStorageURLParseFailureHidesURL checks that when url.Parse
 // itself fails, the wrapped error carries only the inner cause, not the
 // *url.Error whose text embeds the raw (possibly credential-bearing) URL.