From 9137eae6dfddb904063d469e2c60c07a8029ce23 Mon Sep 17 00:00:00 2001
From: nexxo <nexxo@clupilot.local>
Date: Sat, 25 Jul 2026 21:50:57 +0200
Subject: [PATCH] fix(vpn): a stale enable job can no longer resurrect a
 deleted access
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

If an add job was retried after the access had been revoked and its tombstone
purged, it found no row and trusted its own enabled=true payload — re-adding a
key that no longer had a row anyone could revoke it with. A missing row can only
mean the access is gone, so the captured payload may now remove but never
enable.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 app/Provisioning/Jobs/ApplyVpnPeer.php | 10 +++++-----
 tests/Feature/Admin/VpnTest.php        | 15 +++++++++++++++
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/app/Provisioning/Jobs/ApplyVpnPeer.php b/app/Provisioning/Jobs/ApplyVpnPeer.php
index b35f4cf..1dd6676 100644
--- a/app/Provisioning/Jobs/ApplyVpnPeer.php
+++ b/app/Provisioning/Jobs/ApplyVpnPeer.php
@@ -44,11 +44,11 @@ class ApplyVpnPeer implements ShouldQueue
         // SyncVpnPeers only observes, it never re-applies intent.
         $peer = VpnPeer::withTrashed()->where('public_key', $this->publicKey)->first();
 
-        // No row, or a revoked one, means the peer must be off the hub. The
-        // constructor payload only matters when the row is already gone.
-        $enabled = $peer === null
-            ? $this->enabled && $this->allowedIp !== null
-            : (! $peer->trashed() && $peer->enabled);
+        // A missing row can only mean the access is gone — deleted, tombstone
+        // already purged. A stale enable payload must never be able to re-add a
+        // key that no longer has a row to revoke it with, so the captured state
+        // can only ever remove, never enable.
+        $enabled = $peer !== null && ! $peer->trashed() && $peer->enabled;
         $ip = $peer?->allowed_ip ?? $this->allowedIp;
 
         if ($enabled && $ip !== null) {
diff --git a/tests/Feature/Admin/VpnTest.php b/tests/Feature/Admin/VpnTest.php
index ae2fade..9f13bef 100644
--- a/tests/Feature/Admin/VpnTest.php
+++ b/tests/Feature/Admin/VpnTest.php
@@ -306,3 +306,18 @@ it('holds the same allocation lock the host pipeline uses', function () {
     expect(VpnPeer::query()->count())->toBe(1)
         ->and($elapsed)->toBeGreaterThan(0.8);   // it waited for the lock to expire
 });
+
+it('never re-adds a peer whose row is gone, however stale the job', function () {
+    $hub = vpnHub();
+    $peer = VpnPeer::factory()->create();
+
+    // The add job is retried late — after the access was revoked and its
+    // tombstone already purged by a sync that saw the peer gone.
+    $staleAdd = ApplyVpnPeer::for($peer);
+    $peer->forceDelete();
+
+    $staleAdd->handle($hub);
+
+    expect($hub->peerIps())->toBe([])
+        ->and(VpnPeer::withTrashed()->count())->toBe(0);
+});