francesco/termanager2
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Correggi bug di sicurezza e integrita' dei dati

Browse Source 
I due piu' gravi potevano causare perdita di dati silenziosa.

AppServiceProvider chiamava key:generate --force a ogni richiesta quando
APP_KEY risultava vuota. Poiche' nome e cognome dei proclamatori sono
cifrati con quella chiave, ogni riga scritta sotto una chiave poi
sostituita diventava irrecuperabile. La chiave e' ora risolta una sola
volta all'avvio del container e il provider solleva un'eccezione se manca,
invece di rigenerarla.

importTerritoryPdfFolder() caricava i PDF, costruiva l'elenco dei file e
poi lo scartava senza mai accodare il job: nessun errore, nessun territorio
aggiornato, e i file restavano su disco a crescita illimitata perche' la
pulizia viveva solo nel finally del job mai eseguito.

Sicurezza:
- i permessi non venivano riapplicati alle richieste /livewire/update, per
  cui un permesso revocato restava attivo fino al reload della pagina
- host header injection: nginx passava l'Host del client come
  X-Forwarded-Host e Laravel si fidava di tutti i proxy, abbastanza per far
  generare a route() link verso domini arbitrari, inclusi i PDF pubblici
  che contengono un codice di accesso valido
- lo schema forwarded e' ora deciso dal server e non dall'header del client
- campagna_id in Rientra e' #[Locked]: era derivato lato server ma
  manipolabile dal client, permettendo di conteggiare un rientro su una
  campagna arbitraria
- sortField del registro passava non validato a orderBy(), con 500 su
  colonna inesistente
- SESSION_SECURE_COOKIE ora vale true fuori da local
- limiti su dimensione e numero di voci negli import ZIP

Integrita' e prestazioni:
- il default di assignment_link_ttl_hours era rimasto 24 dopo la
  conversione da ore a mesi: i link PDF pubblici duravano due anni invece
  di uno mese
- il seeder usava syncPermissions(), che a un secondo giro azzerava i
  permessi personalizzati dei ruoli; ora e' additivo
- pdftoppm senza timeout poteva bloccare il worker indefinitamente
- nomi file deterministici nell'import: "1 A" e "1-A" scrivevano sullo
  stesso PDF, sovrascrivendosi
- Setting::flushCache() dopo le cancellazioni massive, che non emettono
  eventi e lasciavano la cache puntata a una riga inesistente
- riga settings creata dal seeder, per evitare due singleton concorrenti
- rimosse due migrazioni ridondanti che rompevano migrate:rollback
- indici su assigned_at, (attivo, deleted_at) e deleted_at
- risolti gli N+1 negli elenchi territori e proclamatori

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
francesco
2026-07-19 16:49:42 +02:00
parent cf78550516
commit 0502fad996
19 changed files with 347 additions and 93 deletions
Download Patch File Download Diff File Expand all files Collapse all files

35
app/Jobs/ImportTerritoryPdfFolder.php
View File

@@ -22,7 +22,11 @@ class ImportTerritoryPdfFolder implements ShouldQueue
use SerializesModels;
public int $tries = 1;
public int $timeout = 0;
// A bounded timeout, not 0. With no limit a single PDF that hangs the
// renderer blocks the worker forever and the import state never leaves
// "running", so the UI polls a job that will never finish.
public int $timeout = 900;
public function __construct(
public string $importId,
@@ -115,7 +119,14 @@ class ImportTerritoryPdfFolder implements ShouldQueue
$thumbnailService->delete($territorio->thumbnail_path);
}
$storedFilename = Str::slug('territorio-' . $territorio->numero) . '.pdf';
// The territory id is part of the filename because the slug
// alone is not unique: "1 A" and "1-A" are distinct
// territory numbers that both slug to "territorio-1-a".
// Without it the second import overwrites the first's PDF
// and both rows point at one file — then removing the PDF
// from either territory breaks the other.
$storedFilename = Str::slug('territorio-' . $territorio->numero)
. '-' . $territorio->id . '.pdf';
$publicPath = 'territori-pdf/' . $storedFilename;
Storage::disk('public')->put($publicPath, file_get_contents($sourcePath));
$thumbnailPath = $thumbnailService->generate($publicPath);
@@ -166,6 +177,26 @@ class ImportTerritoryPdfFolder implements ShouldQueue
}
}
/**
* Runs when the job exceeds $timeout or dies outside handle()'s try block.
*
* Without this the import state is stranded in "running" — the UI polls it
* until the 24h cache entry expires — and the uploaded temp directory is
* never reclaimed, since handle()'s finally never executes.
*/
public function failed(?\Throwable $exception): void
{
$stateService = app(TerritorioPdfImportState::class);
$stateService->appendLog(
$this->importId,
'[ERR] Job terminato senza completare: ' . ($exception?->getMessage() ?? 'timeout')
);
$stateService->markFailed($this->importId);
Storage::disk('local')->deleteDirectory('bulk-territori-imports/' . $this->importId);
}
protected function resolveTerritoryFromFilename(string $filename, array $territoriMap): ?array
{
if ($territoriMap === []) {

14
app/Livewire/Assegnazioni/Rientra.php
View File

@@ -2,6 +2,7 @@
namespace App\Livewire\Assegnazioni;
use Livewire\Attributes\Locked;
use Livewire\Component;
use App\Models\Assegnazione;
use App\Models\Campagna;
@@ -11,7 +12,14 @@ class Rientra extends Component
public Assegnazione $assegnazione;
public string $returned_at = '';
public bool $showCampaignPrompt = false;
// Derived server-side by checkCampaign() and never bound in the view.
// Without #[Locked] the client can inject an arbitrary campaign id into
// the update payload and have the assignment counted toward a campaign it
// has no date overlap with (or hit an FK violation with a bogus id).
#[Locked]
public ?int $campagna_id = null;
public bool $counted_in_campaign = false;
public function mount(Assegnazione $assegnazione)
@@ -62,6 +70,12 @@ class Rientra extends Component
{
$this->validate();
// mount() rejects an already-returned assignment, but the component
// state persists across requests — two open tabs would both submit and
// the second would overwrite returned_at/returned_by and log a
// duplicate activity entry. Re-check against the current row.
abort_if($this->assegnazione->fresh()?->returned_at !== null, 409, 'Assegnazione già rientrata.');
$returnDate = \Carbon\Carbon::parse($this->returned_at);
$this->assegnazione->update([

7
app/Livewire/Proclamatori/ProclamatoreIndex.php
View File

@@ -68,7 +68,12 @@ class ProclamatoreIndex extends Component
public function render()
{
$query = Proclamatore::query();
// withCount rather than letting the view call
// ->assegnazioni()->aperte()->count() per row, which issued one
// COUNT(*) per displayed proclamatore on every render.
$query = Proclamatore::query()->withCount([
'assegnazioni as assegnazioni_aperte_count' => fn ($q) => $q->whereNull('returned_at'),
]);
if ($this->search !== '') {
// In-memory filter because nome/cognome are encrypted

30
app/Livewire/Registro.php
View File

@@ -54,8 +54,26 @@ class Registro extends Component
$this->resetPage();
}
/**
* Columns that may be sorted on.
*
* $sortField reaches orderBy() directly, and it is a public Livewire
* property, so the client controls it. Laravel's grammar prevents SQL
* injection, but an unknown column still raises "Column not found" and
* returns a 500 — trivially triggerable from the browser console.
*/
private const SORTABLE_FIELDS = [
'territorio_numero',
'assigned_at',
'returned_at',
];
public function sortBy(string $field)
{
if (! in_array($field, self::SORTABLE_FIELDS, true)) {
return;
}
if ($this->sortField === $field) {
$this->sortDirection = $this->sortDirection === 'asc' ? 'desc' : 'asc';
} else {
@@ -179,13 +197,19 @@ class Registro extends Component
$query->whereHas('territorio', fn($q) => $q->where('tipologia_id', $this->filtroTipologia));
}
if ($this->sortField === 'territorio_numero') {
$dir = $this->sortDirection === 'asc' ? 'ASC' : 'DESC';
// Re-check here too: sortBy() guards the action, but the property can
// also be set directly in a Livewire update payload.
$sortField = in_array($this->sortField, self::SORTABLE_FIELDS, true)
? $this->sortField
: 'territorio_numero';
$dir = $this->sortDirection === 'asc' ? 'ASC' : 'DESC';
if ($sortField === 'territorio_numero') {
$query->orderByRaw(
"CAST((SELECT numero FROM territori WHERE territori.id = assegnazioni.territorio_id) AS UNSIGNED) $dir"
);
} else {
$query->orderBy($this->sortField, $this->sortDirection);
$query->orderBy($sortField, $dir);
}
// In-memory search for encrypted proclamatore fields / territorio numero

11
app/Livewire/Settings/XmlExchange.php
View File

@@ -72,6 +72,11 @@ class XmlExchange extends Component
$this->pdfFolder = [];
$this->dispatchPdfImport(
$importId,
$storedFiles,
'Cartella ricevuta: ' . count($storedFiles) . ' PDF in coda.'
);
}
public function refreshPdfImportStatus(): void
@@ -197,6 +202,12 @@ class XmlExchange extends Component
AnnoTeocratico::query()->delete();
Setting::query()->delete();
// The mass delete above fires no model events, so Setting's
// memoized instance still points at the deleted row. Without this
// the update() below would target a nonexistent id, match zero
// rows, and silently leave the app with no settings at all.
Setting::flushCache();
$settingsNode = $xml->settings;
if ($settingsNode) {
Setting::instance()->update([

11
app/Livewire/Territori/TerritorioIndex.php
View File

@@ -123,7 +123,16 @@ class TerritorioIndex extends Component
public function render()
{
$query = Territorio::with(['zona', 'tipologia', 'assegnazioneCorrente.proclamatore']);
// ultimaAssegnazione is eager-loaded because getGiorniGiacenzaAttribute()
// and is_prioritario read it for every row — and this query fetches the
// whole table (pagination happens in memory below), so lazy-loading it
// meant one extra query per territory on every render.
$query = Territorio::with([
'zona',
'tipologia',
'assegnazioneCorrente.proclamatore',
'ultimaAssegnazione',
]);
if ($this->search) {
$search = $this->search;

13
app/Models/Setting.php
View File

@@ -47,6 +47,19 @@ class Setting extends Model
});
}
/**
* Drop the memoized instance.
*
* Mass operations (Setting::query()->delete(), truncate, raw SQL) do not
* fire model events, so the `saved`/`deleted` hooks above can't catch
* them and the cache would keep serving a row that no longer exists.
* Call this explicitly after any such operation.
*/
public static function flushCache(): void
{
static::$cachedInstance = null;
}
/**
* Get the singleton settings instance (first row).
*/

56
app/Providers/AppServiceProvider.php
View File

@@ -4,8 +4,9 @@ namespace App\Providers;
use Illuminate\Support\ServiceProvider;
use Illuminate\Support\Facades\Gate;
use Illuminate\Support\Facades\Artisan;
use Illuminate\Support\Facades\URL;
use Livewire\Livewire;
use RuntimeException;
class AppServiceProvider extends ServiceProvider
{
@@ -16,16 +17,57 @@ class AppServiceProvider extends ServiceProvider
public function boot(): void
{
// Force HTTPS when behind a reverse proxy
if (str_starts_with(config('app.url'), 'https://')) {
URL::forceScheme('https');
// Pin generated URLs to APP_URL.
//
// Behind a trusted proxy, Symfony derives the port from
// X-Forwarded-Port and falls back to the scheme's default when that
// header is absent — which silently drops a non-standard port from
// every generated link. Anchoring the root to APP_URL makes the
// public address explicit instead of reconstructed per request, and
// keeps PDF/e-mail links correct regardless of proxy behaviour.
$appUrl = (string) config('app.url');
if ($appUrl !== '') {
URL::forceRootUrl($appUrl);
if (str_starts_with($appUrl, 'https://')) {
URL::forceScheme('https');
}
}
// Auto-generate APP_KEY if missing
if (empty(config('app.key'))) {
Artisan::call('key:generate', ['--force' => true]);
// NOTE: APP_KEY is deliberately NOT auto-generated here.
//
// This provider boots on every request. Generating a key at runtime
// meant that if the key ever failed to persist, each request could
// mint a *different* one — and since proclamatori names are encrypted
// at column level with APP_KEY, every row written under a superseded
// key becomes permanently unreadable.
//
// The key is now resolved once at container start (docker/php/
// entrypoint.sh) and persisted to the storage volume. Fail loudly
// instead of silently rotating.
// Only enforced when serving HTTP. Build-time tooling (package:discover,
// config:cache) legitimately runs before the key exists, and failing
// there would break the image build rather than surface a real problem.
if (empty(config('app.key')) && ! $this->app->runningInConsole()) {
throw new RuntimeException(
'APP_KEY is not set. Set it in the environment before starting the application. '
.'Generate one with: php artisan key:generate --show'
);
}
// Livewire only re-applies a fixed allow-list of middleware to
// /livewire/update requests. Spatie's permission middleware is not on
// that list, so a component whose route is guarded by
// `permission:territori.manage` would keep accepting mutating actions
// from a user whose permission was revoked after page load, until they
// reload. Registering them as persistent closes that window.
Livewire::addPersistentMiddleware([
\Spatie\Permission\Middleware\PermissionMiddleware::class,
\Spatie\Permission\Middleware\RoleMiddleware::class,
\Spatie\Permission\Middleware\RoleOrPermissionMiddleware::class,
]);
Gate::before(function ($user, $ability) {
return $user->hasRole('amministratore') ? true : null;
});

32
app/Services/TerritorioPdfImportDispatcher.php
View File

@@ -4,12 +4,19 @@ namespace App\Services;
use App\Jobs\ImportTerritoryPdfFolder;
use Illuminate\Http\UploadedFile;
use Illuminate\Support\Facades\Storage;
use Illuminate\Support\Str;
use RuntimeException;
use ZipArchive;
class TerritorioPdfImportDispatcher
{
/** Per-entry uncompressed ceiling, matching the 10 MB Livewire upload rule. */
private const MAX_ENTRY_BYTES = 10 * 1024 * 1024;
/** Caps the job payload — every entry is serialized into one queue message. */
private const MAX_ENTRIES = 2000;
public function __construct(
protected TerritorioPdfImportState $stateService,
) {
@@ -56,6 +63,25 @@ class TerritorioPdfImportDispatcher
continue;
}
if ($entryIndex >= self::MAX_ENTRIES) {
throw new RuntimeException(
'Lo ZIP contiene più di ' . self::MAX_ENTRIES . ' PDF. Suddividilo in archivi più piccoli.'
);
}
// Check the *uncompressed* size before reading. getFromIndex()
// inflates the entry entirely into memory, so a small zip with
// one hugely compressible entry would otherwise exhaust PHP's
// memory limit and abort the request mid-extraction.
$uncompressedSize = $zip->statIndex($index)['size'] ?? 0;
if ($uncompressedSize > self::MAX_ENTRY_BYTES) {
throw new RuntimeException(sprintf(
'Il file "%s" nello ZIP supera %d MB.',
basename($entryName),
self::MAX_ENTRY_BYTES / 1048576
));
}
$content = $zip->getFromIndex($index);
if ($content === false) {
continue;
@@ -74,6 +100,12 @@ class TerritorioPdfImportDispatcher
$entryIndex++;
}
} catch (\Throwable $exception) {
// Nothing was queued, so the job's own cleanup will never run —
// drop the partially extracted files rather than orphaning them.
Storage::disk('local')->deleteDirectory('bulk-territori-imports/' . $importId);
throw $exception;
} finally {
$zip->close();
}

35
app/Services/TerritorioThumbnailService.php
View File

@@ -2,10 +2,20 @@
namespace App\Services;
use Illuminate\Support\Facades\Log;
use Illuminate\Support\Facades\Storage;
use Symfony\Component\Process\Exception\ProcessTimedOutException;
use Symfony\Component\Process\Process;
class TerritorioThumbnailService
{
/**
* Wall-clock limit for pdftoppm. A malformed PDF can make it spin
* indefinitely; unbounded, that wedges the queue worker for good.
*/
private const RENDER_TIMEOUT_SECONDS = 60;
/**
* Generate a PNG thumbnail of the first page of a PDF stored on the public disk.
*
@@ -22,11 +32,26 @@ class TerritorioThumbnailService
$tempPrefix = sys_get_temp_dir() . '/terr_thumb_' . uniqid();
exec(
'pdftoppm -r 72 -png -l 1 ' . escapeshellarg($pdfAbsPath) . ' ' . escapeshellarg($tempPrefix),
$output,
$exitCode
);
$process = new Process([
'pdftoppm', '-r', '72', '-png', '-l', '1', $pdfAbsPath, $tempPrefix,
]);
$process->setTimeout(self::RENDER_TIMEOUT_SECONDS);
try {
$process->run();
} catch (ProcessTimedOutException) {
Log::warning('pdftoppm timed out generating a territory thumbnail.', [
'pdf' => $pdfStoragePath,
'timeout' => self::RENDER_TIMEOUT_SECONDS,
]);
// Clean up whatever partial pages it managed to write.
foreach (glob($tempPrefix . '*.png') ?: [] as $partial) {
@unlink($partial);
}
return null;
}
// pdftoppm may produce -1.png, -01.png or even -001.png depending on page count
$generated = null;