From 8b631acb72a21cab3d90c1e4c2f7cc32f99f3df8 Mon Sep 17 00:00:00 2001 From: nexxo Date: Tue, 4 Aug 2026 15:51:35 +0200 Subject: [PATCH] Fix-Welle Ganz-Branch-Review: weltlesbare .env-Sicherung, blinder Test, drei Installer-Texte MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fuenf Befunde aus der abschliessenden Review, in einer Runde behoben: - EnvFileEditor::backup() liess PHPs copy() die Umask entscheiden statt den Modus der Quelle zu uebernehmen — .env stand mit deploy/install.sh auf 0600, jede Sicherung landete trotzdem weltlesbar bei 0644, mit APP_KEY, DB_PASSWORD, VPN_CONFIG_KEY und STRIPE_SECRET darin. Reproduziert (per kurzzeitigem git stash des Fixes: 420 statt 384) und jetzt durch einen expliziten chmod nach dem Kopieren sowie einen neuen Test verhindert. - HostSeparationTest pruefte "jede Route hat einen Hostnamen" nur scheinbar allgemein — deploy/install.sh schreibt ADMIN_HOST_EXCLUSIVE=false als Vorgabe, und im nicht-exklusiven Fallback registriert routes/web.php jede /admin/*-Route absichtlich ohne Domain. Der Test setzte zwar ADMIN_HOST_EXCLUSIVE=true, sagte aber nirgends, dass genau das die Voraussetzung der Pruefung ist. hostSeparationTable() gibt AdminArea:: isExclusive() jetzt als Out-Parameter zurueck, gelesen waehrend die zweite Anwendung noch gebootet ist, und der Test besteht darauf. - clupilot:bind-hosts existierte, aber nichts sagte einem Operator, dass es ihn braucht. deploy/update.sh druckt jetzt einen eigenen Hinweis, wenn APP_HOST in .env leer ist — mit der vollen docker-compose-Zeile statt der internen in_app-Abkuerzung, weil der Operator sie in seiner eigenen Shell eintippt. - ask STATUS_DOMAIN und ask FILES_DOMAIN versprachen "blank to keep it auf jedem Host", fuellten Enter aber ueber den dritten ask()-Parameter mit dem Default. Fuer FILES_DOMAIN war das kein Schoenheitsfehler: der Default verschiebt /bootstrap.tar.gz vom Portal weg, bevor DNS fuer den neuen Namen existiert. Beide Defaults entfernt. - ask WWW_DOMAIN erklaerte nicht, dass SITE_HOST mehrere kommagetrennte Namen traegt (der erste kanonisch, der Rest leitet dauerhaft um) — ein Operator, der die Apex-Domain zusaetzlich zu www. binden wollte, hatte keinen Weg, das aus dem Prompt zu erfahren. Nur der Prompt-Text geaendert, kein neuer Prompt, Default unveraendert. Voller Testlauf: 2953 passed (Baseline 2952 + der neue Backup-Berechtigungs- Test), 0 failed. routes/web.php, RestrictAdminHost, config/fortify.php und PublicSiteGate unangetastet. Co-Authored-By: Claude Opus 5 --- app/Services/Env/EnvFileEditor.php | 11 +++++++ deploy/install.sh | 13 ++++++-- deploy/update.sh | 15 +++++++++ tests/Feature/Admin/EnvFileEditorTest.php | 18 +++++++++++ tests/Feature/HostSeparationTest.php | 37 +++++++++++++++++++++-- 5 files changed, 89 insertions(+), 5 deletions(-) diff --git a/app/Services/Env/EnvFileEditor.php b/app/Services/Env/EnvFileEditor.php index af8f85b..b07dec4 100644 --- a/app/Services/Env/EnvFileEditor.php +++ b/app/Services/Env/EnvFileEditor.php @@ -130,6 +130,17 @@ final class EnvFileEditor throw new \RuntimeException("Could not back up [{$this->path()}] before writing it."); } + // .env trägt jedes Zugangsdatum dieser Installation — APP_KEY, + // DB_PASSWORD, VPN_CONFIG_KEY, STRIPE_SECRET. File::copy() ruft PHPs + // copy() auf, und das legt die Kopie mit der Prozess-Umask an statt mit + // dem Modus der Quelldatei: deploy/install.sh:347 setzt .env auf 0600, + // die Kopie landete mit einer üblichen Umask trotzdem bei 0644 — + // weltlesbar direkt neben dem Original. Der Modus wird deshalb explizit + // vom Original übernommen statt der Umask überlassen, und das direkt + // nach dem Kopieren, damit die Kopie so kurz wie möglich mit offenen + // Rechten daliegt. + File::chmod($target, fileperms($this->path()) & 0777); + return $target; } diff --git a/deploy/install.sh b/deploy/install.sh index b6f7972..8ec77ff 100755 --- a/deploy/install.sh +++ b/deploy/install.sh @@ -109,14 +109,21 @@ log "CluPilot installer on ${PRETTY_NAME}" # ── 1. Answers we need before doing anything ───────────────────────────────── ask APP_DOMAIN "Public domain for the customer portal" "app.clupilot.com" -ask WWW_DOMAIN "Domain for the marketing site" "www.clupilot.com" +ask WWW_DOMAIN "Domain for the marketing site (comma-separated for several, first is canonical, e.g. clupilot.com,www.clupilot.com)" "www.clupilot.com" ask ADMIN_DOMAIN "Private domain for the operator console" "admin.clupilot.com" ask WS_DOMAIN "Domain for the websocket server" "ws.clupilot.com" # Stripe posts server-to-server and never sees the portal, so the webhook has # its own name. Naming the portal here was wrong in the printed instructions. ask API_DOMAIN "Domain Stripe posts its webhooks to" "api.clupilot.com" -ask STATUS_DOMAIN "Public status page (blank to keep it on every host)" "status.clupilot.com" -ask FILES_DOMAIN "Domain for downloads: terms, DPA, bootstrap archive (blank to keep them on the portal)" "files.clupilot.com" +# No third argument (default) on either of these two: ask() fills a blank +# answer with the default via ${answer:-$default}, so a default here would make +# the "blank to keep it on …" hint in the prompt a lie — pressing Enter would +# never actually leave it blank. For FILES_DOMAIN this was not cosmetic: a +# default here moves /bootstrap.tar.gz off the portal onto a name whose DNS +# does not exist yet, and a server in rescue mode then cannot fetch its own +# archive. +ask STATUS_DOMAIN "Public status page (blank to keep it on every host)" +ask FILES_DOMAIN "Domain for downloads: terms, DPA, bootstrap archive (blank to keep them on the portal)" ask ADMIN_EMAIL "Your login address" ask ADMIN_NAME "Your display name" "Administrator" ask_secret ADMIN_PASSWORD "Your password (min. 12 characters)" diff --git a/deploy/update.sh b/deploy/update.sh index c143f93..624e5aa 100755 --- a/deploy/update.sh +++ b/deploy/update.sh @@ -777,4 +777,19 @@ if [[ -n "$agent_hint" ]]; then printf ' %s\n' "Run once: sudo bash $(pwd)/deploy/install-agent.sh" fi +# Wer clupilot:bind-hosts braucht, ist genau der, der diese Zeile nicht liest. +# APP_HOST bleibt leer auf jeder Maschine, die vor den vier Host-Variablen +# installiert wurde (install.sh schreibt sie nur bei einer NEUEN .env, siehe +# BindHosts-Klassenkommentar) — und ohne APP_HOST laufen Website und Portal +# host-unabhängig: BEIDE antworten unter JEDEM Namen, ohne Fehler, ohne +# Logzeile, die davon erzählt. Nur ein Hinweis, kein Aufruf: `in_app` läuft mit +# `-T` für dieses Skript und ist der Operatorin, die den Befehl gleich selbst +# in ihrer eigenen Shell eintippt, gar nicht bekannt — wie schon beim +# `docker compose exec …` in finish() oben braucht der Text hier die +# vollständige Zeile, keine Abkürzung, die nur innerhalb dieses Skripts gilt. +if [[ -z "$(sed -n 's/^APP_HOST=//p' .env 2>/dev/null | tail -1)" ]]; then + printf '\033[1;33m !\033[0m %s\n' "Hostnamen sind nicht gebunden — Website und Portal antworten unter jedem Namen." + printf ' %s\n' "Beheben mit: docker compose exec -u www-data app php artisan clupilot:bind-hosts" +fi + log "Done — $(release_version) on $(git rev-parse --short HEAD) (${source_ref})" diff --git a/tests/Feature/Admin/EnvFileEditorTest.php b/tests/Feature/Admin/EnvFileEditorTest.php index 352a0ec..91c8289 100644 --- a/tests/Feature/Admin/EnvFileEditorTest.php +++ b/tests/Feature/Admin/EnvFileEditorTest.php @@ -92,6 +92,24 @@ it('backs up the previous content before writing the new content', function () { ->and(File::get($path))->toBe("NEW_KEY=new-value\n"); }); +it('backs up .env with the same restrictive mode as the original, not the umask', function () { + // .env trägt jedes Zugangsdatum der Installation — APP_KEY, DB_PASSWORD, + // VPN_CONFIG_KEY, STRIPE_SECRET. File::copy() ruft PHPs copy() auf, und das + // wendet die Prozess-Umask an statt den Modus der Quelle zu übernehmen: + // deploy/install.sh setzt .env auf 0600, eine unbehandelte Kopie landete + // mit einer üblichen Umask trotzdem bei 0644 — weltlesbar. Verifiziert auf + // dieser Maschine, bevor der Fix stand: .env und .env.bak-* existierten, + // die Sicherung war 644. + $path = envEditorPath(); + File::put($path, "OLD_KEY=old-value\n"); + chmod($path, 0600); + $editor = new EnvFileEditor($path); + + $backupPath = $editor->write("NEW_KEY=new-value\n"); + + expect(fileperms($backupPath) & 0777)->toBe(0600); +}); + it('does not collide when two writes land in the same second', function () { $path = envEditorPath(); File::put($path, "V=1\n"); diff --git a/tests/Feature/HostSeparationTest.php b/tests/Feature/HostSeparationTest.php index 639b438..6b54b14 100644 --- a/tests/Feature/HostSeparationTest.php +++ b/tests/Feature/HostSeparationTest.php @@ -1,5 +1,6 @@ 'app.clupilot.test', @@ -63,6 +70,15 @@ function hostSeparationTable(): array $app = require base_path('bootstrap/app.php'); $app->make(Kernel::class)->bootstrap(); + // AdminArea::isExclusive() liest von config(), und config() beantwortet + // JEDE Anwendung, die gerade Container::getInstance() ist — hier also + // die zweite. Nach diesem try-Block ist sie im finally schon wieder + // abgebaut und Container::getInstance() zeigt zurück auf die erste + // Testanwendung; ein Aufruf nach return hätte also nicht den hier + // gesetzten ADMIN_HOST_EXCLUSIVE gelesen, sondern den der Suite, und + // die Prüfung unten hätte sich still auf das Falsche verlassen. + $exclusive = AdminArea::isExclusive(); + $table = []; foreach ($app['router']->getRoutes()->getRoutes() as $route) { $table[] = [ @@ -156,8 +172,25 @@ it('bindet jede Route an einen Hostnamen, außer den hier benannten', function ( 'status', ]; + $table = hostSeparationTable($exclusive); + + // Diese Zusicherung ist der ganze Sinn der Prüfung unten, und ohne sie war + // sie nicht wahr: deploy/install.sh:311 schreibt ADMIN_HOST_EXCLUSIVE=false + // als Vorgabe für jede neue Installation. Im nicht-exklusiven Fallback + // registriert routes/web.php:127-139 JEDE /admin/*-Route ohne Domain — die + // Konsole lebt dort absichtlich unter /admin auf jedem Hostnamen, damit ein + // Upgrade niemanden aussperrt (siehe AdminArea-Klassenkommentar). "Jede + // Route hat einen Hostnamen" ist in diesem Modus also schlicht falsch und + // nicht gemeint — die Prüfung hat nur im exklusiven Modus etwas zu sagen, + // und $env oben setzt ADMIN_HOST_EXCLUSIVE=true genau deshalb explizit. + expect($exclusive)->toBeTrue( + 'ADMIN_HOST_EXCLUSIVE aus $env wurde nicht wirksam — ohne exklusiven ' + .'Modus bindet routes/web.php admin/* an keine Domain, und die ' + .'Prüfung unten würde das nicht bemerken.' + ); + $unbound = []; - foreach (hostSeparationTable() as $route) { + foreach ($table as $route) { if ($route['domain'] === null) { $unbound[] = $route['uri']; }