From 48ea6bb33d90df513ffce05333cc6e9d1031732a Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Mon, 5 Oct 2026 03:15:34 +0200 Subject: [PATCH] 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 | 10 +++++ lib/Backend/IBackend.php | 7 ++++ lib/Backend/LocalBackend.php | 11 +++++ lib/Backend/StorageBackend.php | 8 ++++ lib/Repository/Repository.php | 14 +++---- tests/integration/roundtrip.sh | 9 ++-- tests/integration/targets-matrix.sh | 65 +++++++++++++++++++++++++++++ 7 files changed, 113 insertions(+), 11 deletions(-) create mode 100755 tests/integration/targets-matrix.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e67837d..1726e07 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/lib/Backend/IBackend.php b/lib/Backend/IBackend.php index f52eef4..131b293 100644 --- a/lib/Backend/IBackend.php +++ b/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 object paths below $prefix (recursive) */ public function list(string $prefix): array; diff --git a/lib/Backend/LocalBackend.php b/lib/Backend/LocalBackend.php index defb827..f2a61d5 100644 --- a/lib/Backend/LocalBackend.php +++ b/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); diff --git a/lib/Backend/StorageBackend.php b/lib/Backend/StorageBackend.php index 57b2504..0719d8e 100644 --- a/lib/Backend/StorageBackend.php +++ b/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; diff --git a/lib/Repository/Repository.php b/lib/Repository/Repository.php index e6e3ffc..f575fa5 100644 --- a/lib/Repository/Repository.php +++ b/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); } diff --git a/tests/integration/roundtrip.sh b/tests/integration/roundtrip.sh index 10de8a5..b94ce2a 100755 --- a/tests/integration/roundtrip.sh +++ b/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 "" +# Usage: roundtrip.sh "" [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" diff --git a/tests/integration/targets-matrix.sh b/tests/integration/targets-matrix.sh new file mode 100755 index 0000000..e6c6173 --- /dev/null +++ b/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