CI: round trip against real S3 and SFTP backends, not just a local path

Added tests/integration/targets-matrix.sh, which runs the full backup/restore
round trip through files_external's AmazonS3 and SFTP backend classes (S3
via a disposable s3mock container, SFTP via atmoz/sftp), since the existing
matrix only ever exercised the local backend.

This caught a real bug: moving a snapshot to the trash read it from the
location and streamed it straight back into a write on the same connection,
which SFTP can't do reliably (phpseclib fails to interleave a read and a
write on one connection). Backends now expose a native move() and the
id-bound snapshot object is moved with that instead of a read+write copy;
the path-bound database dump still goes through decrypt/re-encrypt, since
its AEAD authentication is tied to its path.

.github/workflows/ci.yml

@@ -38,3 +38,13 @@ jobs:
       - uses: actions/checkout@v4
       - name: Round trip on Nextcloud ${{ matrix.nextcloud }} / ${{ matrix.database }}
         run: tests/integration/matrix.sh "${{ matrix.nextcloud }}" "${{ matrix.database }}"
+
+  integration-targets:
+    # Same round trip, but through the real S3 (s3mock) and SFTP backends instead of a local
+    # path, so files_external's AmazonS3/SFTP backend and auth classes are actually exercised.
+    # Backend correctness does not depend on the Nextcloud version or database, so this runs once.
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - name: Round trip against S3 and SFTP
+        run: tests/integration/targets-matrix.sh

lib/Backend/IBackend.php

@@ -27,6 +27,13 @@ interface IBackend {
 
 	public function exists(string $path): bool;
 
+	/**
+	 * Move an object within the same location (used for the trash). Backends must use a native
+	 * rename, not a read+write copy: some storages (e.g. SFTP) cannot reliably read and write on
+	 * the same connection at once, since the read side is a lazily-pulled stream.
+	 */
+	public function move(string $from, string $to): void;
+
 	/** @return list<string> object paths below $prefix (recursive) */
 	public function list(string $prefix): array;
 

lib/Backend/LocalBackend.php

@@ -106,6 +106,17 @@ final class LocalBackend implements IBackend {
 		@unlink($this->abs($path));
 	}
 
+	public function move(string $from, string $to): void {
+		$target = $this->abs($to);
+		$dir = dirname($target);
+		if (!is_dir($dir) && !mkdir($dir, 0700, true) && !is_dir($dir)) {
+			throw new BackendException('Cannot create ' . $dir);
+		}
+		if (!rename($this->abs($from), $target)) {
+			throw new BackendException("Cannot move $from to $to");
+		}
+	}
+
 	private function abs(string $path): string {
 		if (str_contains($path, '..') || str_starts_with($path, '/')) {
 			throw new BackendException('Invalid path: ' . $path);

lib/Backend/StorageBackend.php

@@ -115,6 +115,14 @@ final class StorageBackend implements IBackend {
 		$this->storage->unlink($this->abs($path));
 	}
 
+	public function move(string $from, string $to): void {
+		$target = $this->abs($to);
+		$this->mkdirs(dirname($target));
+		if (!$this->storage->rename($this->abs($from), $target)) {
+			throw new BackendException("Cannot move $from to $to");
+		}
+	}
+
 	private function listInto(string $dir, array &$result): void {
 		if (!$this->storage->is_dir($dir)) {
 			return;

lib/Repository/Repository.php

@@ -345,20 +345,19 @@ final class Repository {
 		$meta = $this->snapshotMeta($snapshotId);
 		$dbInTrash = null;
 		if (isset($meta['db']) && is_string($meta['db']) && $this->backend->exists($meta['db'])) {
+			// Its encryption is bound to its path (object:db/...), which changes on a move, so it
+			// must be decrypted and re-encrypted under the new path, not moved as raw bytes.
 			$dbInTrash = 'trash/' . $meta['db'];
 			$this->putObject($dbInTrash, $this->getObject($meta['db']));
+			$this->backend->delete($meta['db']);
 		}
-		// The snapshot object is bound to its id (not its path), so it can be copied as-is.
-		$this->backend->put('trash/snapshots/' . $snapshotId, $this->backend->get('snapshots/' . $snapshotId));
+		// The snapshot object is bound to its id (not its path), so it can be moved as raw bytes.
+		$this->backend->move('snapshots/' . $snapshotId, 'trash/snapshots/' . $snapshotId);
 		$this->putObject('trash/info/' . $snapshotId, json_encode(['forgottenAt' => time(), 'by' => $by, 'db' => $meta['db'] ?? null,
 			'dbInTrash' => $dbInTrash, 'time' => $meta['time'] ?? null, 'label' => $meta['label'] ?? ''], JSON_THROW_ON_ERROR));
 		unset($this->catalog->snapshots[$snapshotId]);
 		$this->catalog->trash[$snapshotId] = true;
 		$this->catalog->write();
-		if ($dbInTrash !== null) {
-			$this->backend->delete($meta['db']);
-		}
-		$this->backend->delete('snapshots/' . $snapshotId);
 	}
 
 	/** Undo forget(): move a snapshot back from the trash. */
@@ -368,11 +367,10 @@ final class Repository {
 			$this->putObject($info['db'], $this->getObject($info['dbInTrash']));
 			$this->backend->delete($info['dbInTrash']);
 		}
-		$this->backend->put('snapshots/' . $snapshotId, $this->backend->get('trash/snapshots/' . $snapshotId));
+		$this->backend->move('trash/snapshots/' . $snapshotId, 'snapshots/' . $snapshotId);
 		$this->catalog->snapshots[$snapshotId] = true;
 		unset($this->catalog->trash[$snapshotId]);
 		$this->catalog->write();
-		$this->backend->delete('trash/snapshots/' . $snapshotId);
 		$this->backend->delete('trash/info/' . $snapshotId);
 	}
 

tests/integration/roundtrip.sh

@@ -1,9 +1,12 @@
 #!/bin/bash
 # End-to-end round trip of NG Backup inside a Nextcloud installation (CI and local matrix).
-# Usage: roundtrip.sh "<command prefix to run occ/php as the web user in the instance>" <datadir> <apps-path>
+# Usage: roundtrip.sh "<command prefix to run occ/php as the web user in the instance>" <datadir> <apps-path> [target-args]
 #   e.g. roundtrip.sh "docker exec -u www-data ngb-nc35-pgsql" /var/www/html/data /var/www/html/custom_apps
+#   target-args defaults to a local files_external location; pass backend/auth/options to exercise
+#   a different backend (e.g. amazons3 or sftp), see tests/integration/targets-matrix.sh.
 set -euo pipefail
 X="$1"; DATA="$2"; APPS="$3"
+TARGET_ARGS="${4:-local -a null::null -o datadir=/tmp/ngb-rt-target}"
 occ() { $X php occ "$@"; }
 fail() { echo "FAIL: $*"; exit 1; }
 ok() { echo "ok   $*"; }
@@ -17,8 +20,8 @@ CODE=$(occ backup:key:kit | php -r 'echo json_decode(stream_get_contents(STDIN),
 occ backup:key:confirm -u admin --code="$CODE" --confirm="Yes, I confirm" >/dev/null || fail "confirm"
 ok "key, kit code and confirmation"
 
-occ backup:target:add ci local -a null::null -o datadir=/tmp/ngb-rt-target --path=repo >/dev/null || fail "target add"
-ok "local location added"
+occ backup:target:add ci $TARGET_ARGS --path=repo >/dev/null || fail "target add"
+ok "location added"
 
 # Test data: a user with files.
 $X sh -c "OC_PASS='Roundtrip-pw-2026' php occ user:add --password-from-env $U >/dev/null"

tests/integration/targets-matrix.sh

@@ -0,0 +1,65 @@
+#!/bin/bash
+# Round trip of NG Backup against real S3 (s3mock) and SFTP backends, through files_external's own
+# backend/auth classes, same as a production location would use. Backend correctness does not
+# depend on the Nextcloud version or database, so this runs once (not matrixed across NC/DB),
+# on a recent Nextcloud with sqlite. Each backend gets its own fresh Nextcloud container, since
+# NG Backup's key can only be initialised once per instance.
+# Usage: tests/integration/targets-matrix.sh [nextcloud-version]
+set -euo pipefail
+V=${1:-35}
+APP=$(cd "$(dirname "$0")/../.." && pwd)
+NET=ngb-targets-net
+RESULTS=()
+
+cleanup() { docker rm -fv ngb-targets-nc-s3 ngb-targets-nc-sftp ngb-targets-s3 ngb-targets-sftp >/dev/null 2>&1; docker network rm "$NET" >/dev/null 2>&1; }
+trap cleanup EXIT
+
+wait_tcp() { # host port
+  docker run --rm --network "$NET" alpine:3 sh -c "for i in \$(seq 1 30); do nc -z $1 $2 2>/dev/null && exit 0; sleep 1; done; exit 1" \
+    || { echo "FAIL: $1:$2 never came up"; exit 1; }
+}
+
+start_nc() { # container-name
+  docker run -d --name "$1" --network "$NET" \
+    -e NEXTCLOUD_ADMIN_USER=admin -e NEXTCLOUD_ADMIN_PASSWORD=admin-targets-pw -e NEXTCLOUD_TRUSTED_DOMAINS=localhost \
+    -e SQLITE_DATABASE=nextcloud \
+    -v "$APP:/var/www/html/custom_apps/ng_backup:ro" nextcloud:$V-apache >/dev/null
+  for i in $(seq 1 90); do docker exec -u www-data "$1" php occ status 2>/dev/null | grep -q "installed: true" && break; sleep 4; done
+  docker exec "$1" chown www-data:www-data /var/www/html/custom_apps
+  echo "=== NC $V / sqlite ($1): $(docker exec -u www-data "$1" php occ -V 2>/dev/null)"
+}
+
+run() { # label, nc-container, target-args
+  echo "=== $1"
+  if bash ${NGB_TRACE:+-x} "$APP/tests/integration/roundtrip.sh" "docker exec -u www-data $2" /var/www/html/data /var/www/html/custom_apps "$3"; then
+    RESULTS+=("$1: PASS")
+  else
+    RESULTS+=("$1: FAIL")
+    docker exec "$2" tail -c 2000 /var/www/html/data/nextcloud.log | tr ',' '\n' | grep -E '"message"' | tail -3
+  fi
+}
+
+docker network create "$NET" >/dev/null
+
+# adobe/s3mock: an S3-compatible test server (MinIO's Docker Hub image now needs a paid login;
+# this one is freely pullable and pre-creates a bucket via INITIAL_BUCKETS, no auth to configure).
+docker run -d --name ngb-targets-s3 --network "$NET" \
+  -e INITIAL_BUCKETS=ngbackups \
+  adobe/s3mock >/dev/null
+
+# atmoz/sftp: "user:pass:::dir" creates /home/user/dir as the upload root.
+docker run -d --name ngb-targets-sftp --network "$NET" \
+  atmoz/sftp ngbackup:ngbackuppw:::upload >/dev/null
+
+wait_tcp ngb-targets-s3 9090
+wait_tcp ngb-targets-sftp 22
+
+start_nc ngb-targets-nc-s3
+run "S3 (s3mock)" ngb-targets-nc-s3 "amazons3 -a amazons3::accesskey -o bucket=ngbackups -o hostname=ngb-targets-s3 -o port=9090 -o region=us-east-1 -o use_ssl=false -o use_path_style=true -o key=test -o secret=test"
+
+start_nc ngb-targets-nc-sftp
+run "SFTP" ngb-targets-nc-sftp "sftp -a password::password -o host=ngb-targets-sftp -o root=/upload -o user=ngbackup -o password=ngbackuppw"
+
+printf '%s\n' "${RESULTS[@]}"
+printf '%s\n' "${RESULTS[@]}" | grep -q FAIL && exit 1
+exit 0