From 72ce00c57c686994e631ce919ea9e7602994266b Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:04:19 +0100 Subject: [PATCH 01/28] feat: create secret_attachments table migration with tests - Add create_secret_attachments_table migration (2025_11_16_110234) - UUID primary key, foreign keys to secrets/users (CASCADE) - Encrypted filename_enc field, file metadata (size, mime, storage_path, checksum) - Indexes on secret_id and (secret_id, created_at) - Add comprehensive migration tests (7 tests, 18 assertions) - Tests verify table structure, column types, FK constraints, indexes, cascade behavior Part of #175 (Phase 2: File Attachments API) --- ...110234_create_secret_attachments_table.php | 46 +++++++++++ .../CreateSecretAttachmentsTableTest.php | 80 +++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 database/migrations/2025_11_16_110234_create_secret_attachments_table.php create mode 100644 tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php diff --git a/database/migrations/2025_11_16_110234_create_secret_attachments_table.php b/database/migrations/2025_11_16_110234_create_secret_attachments_table.php new file mode 100644 index 0000000..adbc744 --- /dev/null +++ b/database/migrations/2025_11_16_110234_create_secret_attachments_table.php @@ -0,0 +1,46 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +use Illuminate\Database\Migrations\Migration; +use Illuminate\Database\Schema\Blueprint; +use Illuminate\Support\Facades\Schema; + +return new class extends Migration +{ + /** + * Run the migrations. + */ + public function up(): void + { + Schema::create('secret_attachments', function (Blueprint $table) { + $table->uuid('id')->primary(); + $table->foreignUuid('secret_id')->constrained('secrets')->cascadeOnDelete(); + + // File metadata (encrypted) + $table->text('filename_enc'); // Original filename (encrypted) + $table->unsignedBigInteger('file_size'); // Original size in bytes + $table->string('mime_type', 255); // MIME type + $table->text('storage_path'); // Path to encrypted blob + $table->string('checksum_sha256', 64)->nullable(); // File integrity + + // Audit + $table->foreignUuid('uploaded_by')->constrained('users'); + $table->timestamps(); + + // Indexes + $table->index('secret_id'); + $table->index(['secret_id', 'created_at']); + }); + } + + /** + * Reverse the migrations. + */ + public function down(): void + { + Schema::dropIfExists('secret_attachments'); + } +}; diff --git a/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php b/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php new file mode 100644 index 0000000..80f9df5 --- /dev/null +++ b/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php @@ -0,0 +1,80 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Support\Facades\Schema; + +uses(RefreshDatabase::class); + +test('secret_attachments table exists', function (): void { + expect(Schema::hasTable('secret_attachments'))->toBeTrue(); +}); + +test('secret_attachments has correct columns', function (): void { + expect(Schema::hasColumns('secret_attachments', [ + 'id', + 'secret_id', + 'filename_enc', + 'file_size', + 'mime_type', + 'storage_path', + 'checksum_sha256', + 'uploaded_by', + 'created_at', + 'updated_at', + ]))->toBeTrue(); +}); + +test('secret_attachments has UUID primary key', function (): void { + $columns = Schema::getColumns('secret_attachments'); + $idColumn = collect($columns)->firstWhere('name', 'id'); + + expect($idColumn)->not->toBeNull(); + expect($idColumn['type_name'])->toBe('uuid'); + + // Check primary key via indexes + $indexes = Schema::getIndexes('secret_attachments'); + $primaryKey = collect($indexes)->first(fn ($idx) => $idx['primary'] ?? false); + + expect($primaryKey)->not->toBeNull(); + expect($primaryKey['columns'])->toContain('id'); +}); + +test('secret_attachments has correct column types', function (): void { + $columns = Schema::getColumns('secret_attachments'); + $columnTypes = collect($columns)->mapWithKeys(fn ($col) => [$col['name'] => $col['type_name']]); + + expect($columnTypes['secret_id'])->toBe('uuid'); + expect($columnTypes['filename_enc'])->toBe('text'); + expect($columnTypes['file_size'])->toBe('int8'); + expect($columnTypes['mime_type'])->toBe('varchar'); + expect($columnTypes['storage_path'])->toBe('text'); + expect($columnTypes['checksum_sha256'])->toBe('varchar'); + expect($columnTypes['uploaded_by'])->toBe('uuid'); +}); + +test('secret_attachments has foreign key constraints', function (): void { + $foreignKeys = Schema::getForeignKeys('secret_attachments'); + $foreignKeyColumns = collect($foreignKeys)->pluck('columns')->flatten()->toArray(); + + expect($foreignKeyColumns)->toContain('secret_id'); + expect($foreignKeyColumns)->toContain('uploaded_by'); +}); + +test('secret_attachments has correct indexes', function (): void { + $indexes = Schema::getIndexes('secret_attachments'); + $indexColumns = collect($indexes)->pluck('columns')->flatten()->unique()->toArray(); + + expect($indexColumns)->toContain('secret_id'); +}); + +test('secret_id foreign key cascades on deletion', function (): void { + $foreignKeys = Schema::getForeignKeys('secret_attachments'); + $secretFk = collect($foreignKeys)->firstWhere('columns', ['secret_id']); + + expect($secretFk)->not->toBeNull(); + expect($secretFk['on_delete'])->toBe('cascade'); +}); From c339e9d0cb8983bf67a5479669e0ff88d3d27001 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:11:33 +0100 Subject: [PATCH 02/28] feat: add SecretAttachment model with encryption - Create SecretAttachment model with EncryptedWithDek cast for filename - UUID primary key, transient filename_plain property - Relationships: belongsTo Secret, belongsTo User (uploader) - Hidden fields: filename_enc, storage_path - Download URL accessor for API routes - Add basic model tests (fillable fields, hidden fields) Part of #175 (Phase 2: File Attachments API) --- app/Models/SecretAttachment.php | 151 ++++++++++++++++ .../CreateSecretAttachmentsTableTest.php | 4 +- tests/Feature/Models/SecretAttachmentTest.php | 164 ++++++++++++++++++ 3 files changed, 317 insertions(+), 2 deletions(-) create mode 100644 app/Models/SecretAttachment.php create mode 100644 tests/Feature/Models/SecretAttachmentTest.php diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php new file mode 100644 index 0000000..31861cb --- /dev/null +++ b/app/Models/SecretAttachment.php @@ -0,0 +1,151 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +namespace App\Models; + +use App\Casts\EncryptedWithDek; +use Illuminate\Database\Eloquent\Concerns\HasUuids; +use Illuminate\Database\Eloquent\Factories\HasFactory; +use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\Eloquent\Relations\BelongsTo; + +/** + * Secret attachment model with encrypted filename. + * + * Encrypted fields (*_enc) use EncryptedWithDek cast and are stored as TEXT. + * Transient properties (*_plain) provide plaintext access with automatic encryption. + * + * @property string $id UUID primary key + * @property string $secret_id Foreign key to secrets table + * @property string $filename_enc Encrypted original filename (JSON) + * @property int $file_size Original file size in bytes + * @property string $mime_type MIME type (e.g., application/pdf) + * @property string $storage_path Path to encrypted blob + * @property ?string $checksum_sha256 SHA-256 checksum of original file + * @property string $uploaded_by UUID foreign key to users table + * @property \Illuminate\Support\Carbon $created_at + * @property \Illuminate\Support\Carbon $updated_at + * @property-write string $filename_plain Transient plaintext filename + * @property-read string $download_url Download URL accessor + * @property-read Secret $secret Relationship to secret + * @property-read User $uploader Relationship to uploader + * + * @method static \Illuminate\Database\Eloquent\Builder|SecretAttachment newModelQuery() + * @method static \Illuminate\Database\Eloquent\Builder|SecretAttachment newQuery() + * @method static \Illuminate\Database\Eloquent\Builder|SecretAttachment query() + */ +class SecretAttachment extends Model +{ + /** @use HasFactory<\Illuminate\Database\Eloquent\Factories\Factory> */ + use HasFactory, HasUuids; + + /** + * The table associated with the model. + * + * @var string + */ + protected $table = 'secret_attachments'; + + /** + * The attributes that are mass assignable. + * + * @var list + */ + protected $fillable = [ + 'secret_id', + 'filename_enc', + 'file_size', + 'mime_type', + 'storage_path', + 'checksum_sha256', + 'uploaded_by', + ]; + + /** + * The attributes that should be hidden for serialization. + * + * Protects encrypted fields and storage path from JSON exposure. + * + * @var list + */ + protected $hidden = [ + 'filename_enc', + 'storage_path', + ]; + + /** + * The attributes that should be cast. + * + * @return array + */ + protected function casts(): array + { + return [ + 'filename_enc' => EncryptedWithDek::class, + 'file_size' => 'integer', + 'created_at' => 'datetime', + 'updated_at' => 'datetime', + ]; + } + + /** + * Transient plaintext filename (write-only). + */ + private ?string $filenamePlain = null; + + /** + * Set plaintext filename (transient). + * + * @param string $value + */ + public function setFilenamePlainAttribute(string $value): void + { + $this->filenamePlain = $value; + $this->filename_enc = $value; // Trigger encrypted cast + } + + /** + * Get plaintext filename (read accessor). + * + * Falls back to decrypting filename_enc if transient is null. + * + * @return string|null + */ + public function getFilenamePlainAttribute(): ?string + { + return $this->filenamePlain ?? $this->filename_enc; + } + + /** + * Get download URL for this attachment. + * + * @return string + */ + public function getDownloadUrlAttribute(): string + { + return route('api.v1.attachments.download', ['attachment' => $this->id]); + } + + /** + * Get the secret that owns this attachment. + * + * @return BelongsTo + */ + public function secret(): BelongsTo + { + return $this->belongsTo(Secret::class); + } + + /** + * Get the user who uploaded this attachment. + * + * @return BelongsTo + */ + public function uploader(): BelongsTo + { + return $this->belongsTo(User::class, 'uploaded_by'); + } +} diff --git a/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php b/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php index 80f9df5..886b0e4 100644 --- a/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php +++ b/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php @@ -34,11 +34,11 @@ expect($idColumn)->not->toBeNull(); expect($idColumn['type_name'])->toBe('uuid'); - + // Check primary key via indexes $indexes = Schema::getIndexes('secret_attachments'); $primaryKey = collect($indexes)->first(fn ($idx) => $idx['primary'] ?? false); - + expect($primaryKey)->not->toBeNull(); expect($primaryKey['columns'])->toContain('id'); }); diff --git a/tests/Feature/Models/SecretAttachmentTest.php b/tests/Feature/Models/SecretAttachmentTest.php new file mode 100644 index 0000000..33ae982 --- /dev/null +++ b/tests/Feature/Models/SecretAttachmentTest.php @@ -0,0 +1,164 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +use App\Models\Secret; +use App\Models\SecretAttachment; +use App\Models\TenantKey; +use App\Models\User; +use Illuminate\Foundation\Testing\RefreshDatabase; + +uses(RefreshDatabase::class); + +beforeEach(function (): void { + // Use process-specific KEK file for parallel test isolation + TenantKey::setKekPath(getTestKekPath()); + TenantKey::generateKek(); + + // Create tenant + $keys = TenantKey::generateEnvelopeKeys(); + $this->tenant = TenantKey::create($keys); + + // Create user + $this->user = User::factory()->create(); +}); + +afterEach(function (): void { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +test('secret attachment has correct fillable fields', function (): void { + $model = new SecretAttachment(); + + expect($model->getFillable())->toContain('secret_id'); + expect($model->getFillable())->toContain('filename_enc'); + expect($model->getFillable())->toContain('file_size'); + expect($model->getFillable())->toContain('mime_type'); + expect($model->getFillable())->toContain('storage_path'); + expect($model->getFillable())->toContain('checksum_sha256'); + expect($model->getFillable())->toContain('uploaded_by'); +}); + +test('secret attachment hides encrypted fields', function (): void { + $model = new SecretAttachment(); + + expect($model->getHidden())->toContain('filename_enc'); + expect($model->getHidden())->toContain('storage_path'); +}); + +test('secret attachment uses UUID primary key', function (): void { + $secret = Secret::create([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Test Secret for Attachment', + ]); + + $attachment = SecretAttachment::create([ + 'secret_id' => $secret->id, + 'filename_plain' => 'test.pdf', + 'file_size' => 1024, + 'mime_type' => 'application/pdf', + 'storage_path' => 'attachments/test/123.enc', + 'checksum_sha256' => hash('sha256', 'test'), + 'uploaded_by' => $this->user->id, + ]); + + expect($attachment->id)->toBeString(); + expect(strlen($attachment->id))->toBe(36); // UUID format +}); + +test('secret attachment encrypts filename with EncryptedWithDek cast', function (): void { + $secret = Secret::create([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Secret with Encrypted Attachment', + ]); + + $attachment = SecretAttachment::create([ + 'secret_id' => $secret->id, + 'filename_plain' => 'secret-document.pdf', + 'file_size' => 2048, + 'mime_type' => 'application/pdf', + 'storage_path' => 'attachments/test/456.enc', + 'checksum_sha256' => hash('sha256', 'content'), + 'uploaded_by' => $this->user->id, + ]); + + // filename_enc should be encrypted JSON in database + $raw = $attachment->getRawOriginal('filename_enc'); + expect($raw)->toBeString(); + expect($raw)->toContain('ciphertext'); + expect($raw)->toContain('nonce'); + + // filename_plain should decrypt correctly + expect($attachment->filename_plain)->toBe('secret-document.pdf'); +}); + +test('secret attachment belongs to secret', function (): void { + $secret = Secret::create([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Secret with Relationship', + ]); + + $attachment = SecretAttachment::create([ + 'secret_id' => $secret->id, + 'filename_plain' => 'test.jpg', + 'file_size' => 512, + 'mime_type' => 'image/jpeg', + 'storage_path' => 'attachments/test/789.enc', + 'checksum_sha256' => hash('sha256', 'image'), + 'uploaded_by' => $this->user->id, + ]); + + expect($attachment->secret)->toBeInstanceOf(Secret::class); + expect($attachment->secret->id)->toBe($secret->id); +}); + +test('secret attachment belongs to user (uploaded_by)', function (): void { + $secret = Secret::create([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Secret for Uploader Test', + ]); + + $attachment = SecretAttachment::create([ + 'secret_id' => $secret->id, + 'filename_plain' => 'report.docx', + 'file_size' => 4096, + 'mime_type' => 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + 'storage_path' => 'attachments/test/abc.enc', + 'checksum_sha256' => hash('sha256', 'document'), + 'uploaded_by' => $this->user->id, + ]); + + expect($attachment->uploader)->toBeInstanceOf(User::class); + expect($attachment->uploader->id)->toBe($this->user->id); +}); + +test('secret attachment has download_url accessor', function (): void { + $secret = Secret::create([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Secret for Download URL Test', + ]); + + $attachment = SecretAttachment::create([ + 'secret_id' => $secret->id, + 'filename_plain' => 'invoice.pdf', + 'file_size' => 1536, + 'mime_type' => 'application/pdf', + 'storage_path' => 'attachments/test/def.enc', + 'checksum_sha256' => hash('sha256', 'invoice'), + 'uploaded_by' => $this->user->id, + ]); + + expect($attachment->download_url)->toBeString(); + expect($attachment->download_url)->toContain('/api/v1/attachments/'); + expect($attachment->download_url)->toContain($attachment->id); + expect($attachment->download_url)->toContain('/download'); +}); + From 30af79d3ddd44b08463304c144dfa7d00ad862bb Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:22:11 +0100 Subject: [PATCH 03/28] feat: add AttachmentStorageService with tenant encryption - Implements store() with tenant DEK encryption (JSON blob: ciphertext + nonce) - Implements retrieve() with decryption from storage - Implements delete() for file and DB record removal - Storage: attachments/{tenant_id}/{secret_id}/{uuid}.enc - Fixed: tenant_id required in secret_attachments table for EncryptedWithDek cast - Fixed: explicit property assignment pattern for encrypted fields - Tests: 3 passing (store, encrypt, retrieve/decrypt) - Migration: added tenant_id column with foreign key constraint Related: #175 --- app/Models/SecretAttachment.php | 1 + app/Services/AttachmentStorageService.php | 109 ++++++++++++++++++ ...110234_create_secret_attachments_table.php | 1 + .../CreateSecretAttachmentsTableTest.php | 2 + .../Services/AttachmentStorageServiceTest.php | 91 +++++++++++++++ 5 files changed, 204 insertions(+) create mode 100644 app/Services/AttachmentStorageService.php create mode 100644 tests/Feature/Services/AttachmentStorageServiceTest.php diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php index 31861cb..f9cac6e 100644 --- a/app/Models/SecretAttachment.php +++ b/app/Models/SecretAttachment.php @@ -56,6 +56,7 @@ class SecretAttachment extends Model */ protected $fillable = [ 'secret_id', + 'tenant_id', // Required for EncryptedWithDek cast 'filename_enc', 'file_size', 'mime_type', diff --git a/app/Services/AttachmentStorageService.php b/app/Services/AttachmentStorageService.php new file mode 100644 index 0000000..5215730 --- /dev/null +++ b/app/Services/AttachmentStorageService.php @@ -0,0 +1,109 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +namespace App\Services; + +use App\Models\Secret; +use App\Models\SecretAttachment; +use App\Models\User; +use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Storage; +use Illuminate\Support\Str; + +/** + * Service for storing, retrieving, and deleting encrypted file attachments. + * + * Files are encrypted using the tenant's DEK before storage. + * Storage structure: attachments/{tenant_id}/{secret_id}/{attachment_id}.enc + */ +class AttachmentStorageService +{ + /** + * Store an uploaded file with encryption. + * + * @param UploadedFile $file The uploaded file + * @param Secret $secret The secret to attach the file to + * @param User $user The user uploading the file + * @return SecretAttachment The created attachment record + */ + public function store(UploadedFile $file, Secret $secret, User $user): SecretAttachment + { + // Read original file content + $content = file_get_contents($file->getRealPath()); + + // Calculate checksum (before encryption) + $checksum = hash('sha256', $content); + + // Encrypt with tenant DEK + $tenant = $secret->tenantKey; + $encrypted = $tenant->encrypt($content); + + // Generate storage path + $attachmentId = Str::uuid()->toString(); + $storagePath = sprintf( + 'attachments/%d/%s/%s.enc', + $tenant->id, + $secret->id, + $attachmentId + ); + + // Store encrypted blob as JSON + Storage::disk('local')->put($storagePath, json_encode([ + 'ciphertext' => base64_encode($encrypted['ciphertext']), + 'nonce' => base64_encode($encrypted['nonce']), + ])); + + // Create attachment record + $attachment = new SecretAttachment(); + $attachment->id = $attachmentId; + $attachment->secret_id = $secret->id; + $attachment->tenant_id = $secret->tenant_id; // MUST be set BEFORE encrypted fields + $attachment->filename_plain = $file->getClientOriginalName(); // Triggers encryption + $attachment->file_size = $file->getSize(); + $attachment->mime_type = $file->getMimeType(); + $attachment->storage_path = $storagePath; + $attachment->checksum_sha256 = $checksum; + $attachment->uploaded_by = $user->id; + $attachment->save(); + + return $attachment; + } /** + * Retrieve and decrypt file content. + * + * @param SecretAttachment $attachment The attachment to retrieve + * @return string The decrypted file content + */ + public function retrieve(SecretAttachment $attachment): string + { + // Read encrypted blob from storage + $encryptedBlob = Storage::disk('local')->get($attachment->storage_path); + $decoded = json_decode($encryptedBlob, true); + + // Decrypt with tenant DEK + $tenant = $attachment->secret->tenantKey; + $decrypted = $tenant->decrypt( + base64_decode($decoded['ciphertext']), + base64_decode($decoded['nonce']) + ); + + return $decrypted; + } + + /** + * Delete attachment file from storage. + * + * @param SecretAttachment $attachment The attachment to delete + * @return bool True if deletion was successful + */ + public function delete(SecretAttachment $attachment): bool + { + // Delete file from storage + Storage::disk('local')->delete($attachment->storage_path); + + // Delete attachment record + return $attachment->delete(); + } +} diff --git a/database/migrations/2025_11_16_110234_create_secret_attachments_table.php b/database/migrations/2025_11_16_110234_create_secret_attachments_table.php index adbc744..05e74a8 100644 --- a/database/migrations/2025_11_16_110234_create_secret_attachments_table.php +++ b/database/migrations/2025_11_16_110234_create_secret_attachments_table.php @@ -18,6 +18,7 @@ public function up(): void Schema::create('secret_attachments', function (Blueprint $table) { $table->uuid('id')->primary(); $table->foreignUuid('secret_id')->constrained('secrets')->cascadeOnDelete(); + $table->foreignId('tenant_id')->constrained('tenant_keys'); // Required for EncryptedWithDek cast // File metadata (encrypted) $table->text('filename_enc'); // Original filename (encrypted) diff --git a/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php b/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php index 886b0e4..84cc3ba 100644 --- a/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php +++ b/tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php @@ -48,6 +48,7 @@ $columnTypes = collect($columns)->mapWithKeys(fn ($col) => [$col['name'] => $col['type_name']]); expect($columnTypes['secret_id'])->toBe('uuid'); + expect($columnTypes['tenant_id'])->toBe('int8'); // Required for EncryptedWithDek cast expect($columnTypes['filename_enc'])->toBe('text'); expect($columnTypes['file_size'])->toBe('int8'); expect($columnTypes['mime_type'])->toBe('varchar'); @@ -61,6 +62,7 @@ $foreignKeyColumns = collect($foreignKeys)->pluck('columns')->flatten()->toArray(); expect($foreignKeyColumns)->toContain('secret_id'); + expect($foreignKeyColumns)->toContain('tenant_id'); expect($foreignKeyColumns)->toContain('uploaded_by'); }); diff --git a/tests/Feature/Services/AttachmentStorageServiceTest.php b/tests/Feature/Services/AttachmentStorageServiceTest.php new file mode 100644 index 0000000..c91cc6f --- /dev/null +++ b/tests/Feature/Services/AttachmentStorageServiceTest.php @@ -0,0 +1,91 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +use App\Models\Secret; +use App\Models\SecretAttachment; +use App\Models\TenantKey; +use App\Models\User; +use App\Services\AttachmentStorageService; +use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Storage; + +uses(RefreshDatabase::class); + +beforeEach(function (): void { + Storage::fake('local'); + + TenantKey::setKekPath(getTestKekPath()); + TenantKey::generateKek(); + + $keys = TenantKey::generateEnvelopeKeys(); + $this->tenant = TenantKey::create($keys); + $this->user = User::factory()->create(); + + $this->service = new AttachmentStorageService(); +}); + +afterEach(function (): void { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +test('service stores file with encryption', function (): void { + $secret = new Secret(); + $secret->tenant_id = $this->tenant->id; + $secret->owner_id = $this->user->id; + $secret->title_plain = 'Secret with Attachment'; + $secret->save(); + + $file = UploadedFile::fake()->create('test-document.pdf', 100, 'application/pdf'); + + $attachment = $this->service->store($file, $secret, $this->user); + + expect($attachment)->toBeInstanceOf(SecretAttachment::class); + expect($attachment->filename_plain)->toBe('test-document.pdf'); + expect($attachment->file_size)->toBe(102400); + expect($attachment->mime_type)->toBe('application/pdf'); + expect($attachment->checksum_sha256)->toBeString(); + expect($attachment->storage_path)->toStartWith('attachments/'); + expect($attachment->uploaded_by)->toBe($this->user->id); +}); + +test('service encrypts file content in storage', function (): void { + $secret = new Secret(); + $secret->tenant_id = $this->tenant->id; + $secret->owner_id = $this->user->id; + $secret->title_plain = 'Secret for Encryption Test'; + $secret->save(); + + $fileContent = 'This is secret file content that must be encrypted'; + $file = UploadedFile::fake()->createWithContent('secret.txt', $fileContent); + + $attachment = $this->service->store($file, $secret, $this->user); + + $encryptedBlob = Storage::disk('local')->get($attachment->storage_path); + + $decoded = json_decode($encryptedBlob, true); + expect($decoded)->toBeArray(); + expect($decoded)->toHaveKey('ciphertext'); + expect($decoded)->toHaveKey('nonce'); + expect($encryptedBlob)->not->toContain($fileContent); +}); + +test('service retrieves and decrypts file content', function (): void { + $secret = new Secret(); + $secret->tenant_id = $this->tenant->id; + $secret->owner_id = $this->user->id; + $secret->title_plain = 'Secret for Decryption Test'; + $secret->save(); + + $originalContent = 'Original file content to be encrypted and decrypted'; + $file = UploadedFile::fake()->createWithContent('original.txt', $originalContent); + + $attachment = $this->service->store($file, $secret, $this->user); + $decryptedContent = $this->service->retrieve($attachment); + + expect($decryptedContent)->toBe($originalContent); +}); From c0699a198d706ae26003896e127c8dee2b634941 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:23:40 +0100 Subject: [PATCH 04/28] feat: add attachments relationship to Secret model - Add hasMany relationship to SecretAttachment - Add attachment_count accessor for convenience - Tests: 2 new tests (relationship + accessor) - All SecretTest.php tests passing (13 total) Related: #175 --- app/Models/Secret.php | 20 ++++++++++++++++++++ tests/Feature/SecretTest.php | 23 +++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/app/Models/Secret.php b/app/Models/Secret.php index 5e04045..b3ee600 100644 --- a/app/Models/Secret.php +++ b/app/Models/Secret.php @@ -252,6 +252,26 @@ public function owner(): BelongsTo return $this->belongsTo(User::class, 'owner_id'); } + /** + * Relation to SecretAttachment (file attachments). + * + * @return \Illuminate\Database\Eloquent\Relations\HasMany + */ + public function attachments(): \Illuminate\Database\Eloquent\Relations\HasMany + { + return $this->hasMany(SecretAttachment::class); + } + + /** + * Get count of attachments for this secret. + * + * @return int Number of attachments + */ + public function getAttachmentCountAttribute(): int + { + return $this->attachments()->count(); + } + /** * Scope to filter by owner. * diff --git a/tests/Feature/SecretTest.php b/tests/Feature/SecretTest.php index ba778df..025d081 100644 --- a/tests/Feature/SecretTest.php +++ b/tests/Feature/SecretTest.php @@ -181,3 +181,26 @@ expect($result->notes_tsv)->not->toBeNull(); }); }); + +describe('Secret Model - Relationships', function () { + test('has attachments relationship', function (): void { + $secret = new Secret; + $secret->tenant_id = $this->tenant->id; + $secret->owner_id = $this->user->id; + $secret->title_plain = 'Test Secret'; + $secret->save(); + + expect($secret->attachments())->toBeInstanceOf(\Illuminate\Database\Eloquent\Relations\HasMany::class); + expect($secret->attachments)->toBeEmpty(); + }); + + test('attachment_count accessor returns zero for secrets without attachments', function (): void { + $secret = new Secret; + $secret->tenant_id = $this->tenant->id; + $secret->owner_id = $this->user->id; + $secret->title_plain = 'Test Secret'; + $secret->save(); + + expect($secret->attachment_count)->toBe(0); + }); +}); From 1eed66c9e06fa7e2b6bbe5149a4a509d3dfbcc60 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:28:22 +0100 Subject: [PATCH 05/28] feat: add SecretAttachmentPolicy with owner authorization - Implement viewAny, view, create, delete authorization - Only secret owners can manage attachments (sharing support TODO) - Register policy in AppServiceProvider - Tests: 8 passing (owner/non-owner scenarios) - Note: Factory removed (EncryptedWithDek incompatible with mass assignment) Related: #175 --- app/Policies/SecretAttachmentPolicy.php | 56 +++++++++++++ app/Providers/AppServiceProvider.php | 5 ++ .../Policies/SecretAttachmentPolicyTest.php | 81 +++++++++++++++++++ 3 files changed, 142 insertions(+) create mode 100644 app/Policies/SecretAttachmentPolicy.php create mode 100644 tests/Feature/Policies/SecretAttachmentPolicyTest.php diff --git a/app/Policies/SecretAttachmentPolicy.php b/app/Policies/SecretAttachmentPolicy.php new file mode 100644 index 0000000..e34034e --- /dev/null +++ b/app/Policies/SecretAttachmentPolicy.php @@ -0,0 +1,56 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +namespace App\Policies; + +use App\Models\Secret; +use App\Models\SecretAttachment; +use App\Models\User; + +/** + * Authorization policy for SecretAttachment model. + * + * Attachment permissions are derived from Secret ownership: + * - viewAny/view: Secret owner only + * - create: Secret owner only + * - delete: Secret owner only + * + * TODO: Extend with sharing permissions when Secret sharing is implemented + */ +class SecretAttachmentPolicy +{ + /** + * Determine if user can view any attachments for a secret. + */ + public function viewAny(User $user, Secret $secret): bool + { + return $user->id === $secret->owner_id; + } + + /** + * Determine if user can view a specific attachment. + */ + public function view(User $user, SecretAttachment $attachment): bool + { + return $user->id === $attachment->secret->owner_id; + } + + /** + * Determine if user can upload attachments to a secret. + */ + public function create(User $user, Secret $secret): bool + { + return $user->id === $secret->owner_id; + } + + /** + * Determine if user can delete an attachment. + */ + public function delete(User $user, SecretAttachment $attachment): bool + { + return $user->id === $attachment->secret->owner_id; + } +} diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 163bacc..c917f78 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -8,10 +8,12 @@ use App\Models\Permission; use App\Models\Person; use App\Models\Secret; +use App\Models\SecretAttachment; use App\Observers\PersonObserver; use App\Observers\SecretObserver; use App\Policies\PermissionManagementPolicy; use App\Policies\RoleManagementPolicy; +use App\Policies\SecretAttachmentPolicy; use Illuminate\Support\Facades\Gate; use Illuminate\Support\ServiceProvider; use Spatie\Permission\Models\Role; @@ -40,6 +42,9 @@ public function boot(): void // Register policy for Spatie Permission model Gate::policy(Permission::class, PermissionManagementPolicy::class); + // Register policy for SecretAttachment model + Gate::policy(SecretAttachment::class, SecretAttachmentPolicy::class); + // Register gates for user permission management $this->registerUserPermissionGates(); } diff --git a/tests/Feature/Policies/SecretAttachmentPolicyTest.php b/tests/Feature/Policies/SecretAttachmentPolicyTest.php new file mode 100644 index 0000000..28a7990 --- /dev/null +++ b/tests/Feature/Policies/SecretAttachmentPolicyTest.php @@ -0,0 +1,81 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +use App\Models\Secret; +use App\Models\SecretAttachment; +use App\Models\TenantKey; +use App\Models\User; +use App\Policies\SecretAttachmentPolicy; +use Illuminate\Foundation\Testing\RefreshDatabase; + +uses(RefreshDatabase::class); + +beforeEach(function (): void { + TenantKey::setKekPath(getTestKekPath()); + TenantKey::generateKek(); + + $keys = TenantKey::generateEnvelopeKeys(); + $this->tenant = TenantKey::create($keys); + + $this->owner = User::factory()->create(); + $this->otherUser = User::factory()->create(); + $this->policy = new SecretAttachmentPolicy; + + $this->secret = new Secret; + $this->secret->tenant_id = $this->tenant->id; + $this->secret->owner_id = $this->owner->id; + $this->secret->title_plain = 'Test Secret'; + $this->secret->save(); + + $this->attachment = new SecretAttachment; + $this->attachment->id = \Illuminate\Support\Str::uuid(); + $this->attachment->secret_id = $this->secret->id; + $this->attachment->tenant_id = $this->tenant->id; + $this->attachment->filename_plain = 'test.pdf'; + $this->attachment->file_size = 1024; + $this->attachment->mime_type = 'application/pdf'; + $this->attachment->storage_path = 'test/path.enc'; + $this->attachment->checksum_sha256 = hash('sha256', 'test'); + $this->attachment->uploaded_by = $this->owner->id; + $this->attachment->save(); +}); + +afterEach(function (): void { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +test('owner can view any attachments', function (): void { + expect($this->policy->viewAny($this->owner, $this->secret))->toBeTrue(); +}); + +test('non-owner cannot view any attachments', function (): void { + expect($this->policy->viewAny($this->otherUser, $this->secret))->toBeFalse(); +}); + +test('owner can view specific attachment', function (): void { + expect($this->policy->view($this->owner, $this->attachment))->toBeTrue(); +}); + +test('non-owner cannot view specific attachment', function (): void { + expect($this->policy->view($this->otherUser, $this->attachment))->toBeFalse(); +}); + +test('owner can create attachments', function (): void { + expect($this->policy->create($this->owner, $this->secret))->toBeTrue(); +}); + +test('non-owner cannot create attachments', function (): void { + expect($this->policy->create($this->otherUser, $this->secret))->toBeFalse(); +}); + +test('owner can delete attachments', function (): void { + expect($this->policy->delete($this->owner, $this->attachment))->toBeTrue(); +}); + +test('non-owner cannot delete attachments', function (): void { + expect($this->policy->delete($this->otherUser, $this->attachment))->toBeFalse(); +}); From f08075b47552baf013a12249851d031e873405fa Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:42:03 +0100 Subject: [PATCH 06/28] feat: add SecretAttachmentController with 4 endpoints - POST /v1/secrets/{secret}/attachments (upload with validation) - GET /v1/secrets/{secret}/attachments (list) - GET /v1/attachments/{attachment}/download (with headers) - DELETE /v1/attachments/{attachment} - Config: attachments.php (max_file_size, allowed_mime_types) - Tests: 13 passing (upload, list, download, delete, authorization) - Standards: /v1/ prefix, no named routes, correct copyright format Related: #175 --- .../Api/V1/SecretAttachmentController.php | 167 +++++++++++ app/Models/SecretAttachment.php | 2 +- config/attachments.php | 72 +++++ routes/api.php | 7 + .../SecretAttachmentControllerTest.php | 278 ++++++++++++++++++ 5 files changed, 525 insertions(+), 1 deletion(-) create mode 100644 app/Http/Controllers/Api/V1/SecretAttachmentController.php create mode 100644 config/attachments.php create mode 100644 tests/Feature/Controllers/SecretAttachmentControllerTest.php diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php new file mode 100644 index 0000000..79f73e5 --- /dev/null +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -0,0 +1,167 @@ +validate([ + 'file' => [ + 'required', + 'file', + 'max:'.(($maxSize / 1024)), // Laravel expects KB + 'mimes:'.implode(',', array_map(fn ($mime) => $this->mimeToExtension($mime), $allowedMimes)), + ], + ]); + + /** @var \Illuminate\Http\UploadedFile $file */ + $file = $validated['file']; + + $attachment = $this->storageService->store($file, $secret, $request->user()); + + return response()->json([ + 'data' => [ + 'id' => $attachment->id, + 'filename' => $attachment->getFilenamePlainAttribute(), + 'file_size' => $attachment->file_size, + 'mime_type' => $attachment->mime_type, + 'download_url' => $attachment->download_url, + 'uploaded_by' => $attachment->uploaded_by, + 'created_at' => $attachment->created_at->toIso8601String(), + ], + ], 201); + } + + /** + * List attachments for secret. + * + * @param \App\Models\Secret $secret + * @return \Illuminate\Http\JsonResponse + */ + public function index(Secret $secret): JsonResponse + { + Gate::authorize('viewAny', [SecretAttachment::class, $secret]); + + $attachments = $secret->attachments()->latest()->get(); + + return response()->json([ + 'data' => $attachments->map(fn ($attachment) => [ + 'id' => $attachment->id, + 'filename' => $attachment->getFilenamePlainAttribute(), + 'file_size' => $attachment->file_size, + 'mime_type' => $attachment->mime_type, + 'download_url' => $attachment->download_url, + 'created_at' => $attachment->created_at->toIso8601String(), + ]), + ]); + } + + /** + * Download attachment. + * + * @param \App\Models\SecretAttachment $attachment + * @return \Illuminate\Http\Response + */ + public function download(SecretAttachment $attachment): Response + { + Gate::authorize('view', $attachment); + + $content = $this->storageService->retrieve($attachment); + $filename = $attachment->getFilenamePlainAttribute(); + + return response($content, 200, [ + 'Content-Type' => $attachment->mime_type, + 'Content-Disposition' => 'attachment; filename="'.$filename.'"', + 'Content-Length' => $attachment->file_size, + ]); + } + + /** + * Delete attachment. + * + * @param \App\Models\SecretAttachment $attachment + * @return \Illuminate\Http\Response + */ + public function destroy(SecretAttachment $attachment): Response + { + Gate::authorize('delete', $attachment); + + $this->storageService->delete($attachment); + + return response()->noContent(); + } + + /** + * Map MIME type to file extension for validation. + * + * @param string $mime + * @return string + */ + private function mimeToExtension(string $mime): string + { + return match ($mime) { + 'image/jpeg' => 'jpg,jpeg', + 'image/png' => 'png', + 'image/gif' => 'gif', + 'image/webp' => 'webp', + 'application/pdf' => 'pdf', + 'application/msword' => 'doc', + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' => 'docx', + 'application/vnd.ms-excel' => 'xls', + 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' => 'xlsx', + 'application/vnd.ms-powerpoint' => 'ppt', + 'application/vnd.openxmlformats-officedocument.presentationml.presentation' => 'pptx', + 'text/plain' => 'txt', + 'text/csv' => 'csv', + 'text/html' => 'html', + 'text/markdown' => 'md', + 'application/zip' => 'zip', + 'application/x-7z-compressed' => '7z', + 'application/x-rar-compressed' => 'rar', + 'application/json' => 'json', + 'application/xml' => 'xml', + default => '*', + }; + } +} diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php index f9cac6e..719f839 100644 --- a/app/Models/SecretAttachment.php +++ b/app/Models/SecretAttachment.php @@ -127,7 +127,7 @@ public function getFilenamePlainAttribute(): ?string */ public function getDownloadUrlAttribute(): string { - return route('api.v1.attachments.download', ['attachment' => $this->id]); + return url("/v1/attachments/{$this->id}/download"); } /** diff --git a/config/attachments.php b/config/attachments.php new file mode 100644 index 0000000..ff93d6f --- /dev/null +++ b/config/attachments.php @@ -0,0 +1,72 @@ + env('ATTACHMENT_MAX_SIZE', 10 * 1024 * 1024), + + /* + |-------------------------------------------------------------------------- + | Allowed MIME Types + |-------------------------------------------------------------------------- + | + | List of permitted MIME types for attachments. + | Empty array allows all types. + | + */ + 'allowed_mime_types' => [ + // Images + 'image/jpeg', + 'image/png', + 'image/gif', + 'image/webp', + + // Documents + 'application/pdf', + 'application/msword', + 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + 'application/vnd.ms-excel', + 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', + 'application/vnd.ms-powerpoint', + 'application/vnd.openxmlformats-officedocument.presentationml.presentation', + + // Text + 'text/plain', + 'text/csv', + 'text/html', + 'text/markdown', + + // Archives + 'application/zip', + 'application/x-7z-compressed', + 'application/x-rar-compressed', + + // Other + 'application/json', + 'application/xml', + ], + + /* + |-------------------------------------------------------------------------- + | Storage Disk + |-------------------------------------------------------------------------- + | + | Disk to use for storing encrypted attachment files. + | Must be configured in config/filesystems.php + | + */ + 'storage_disk' => env('ATTACHMENT_STORAGE_DISK', 'local'), +]; diff --git a/routes/api.php b/routes/api.php index 2e73686..a5c1004 100644 --- a/routes/api.php +++ b/routes/api.php @@ -5,6 +5,7 @@ use App\Http\Controllers\Api\V1\PermissionManagementController; use App\Http\Controllers\Api\V1\RoleManagementController; +use App\Http\Controllers\Api\V1\SecretAttachmentController; use App\Http\Controllers\Api\V1\UserPermissionController; use App\Http\Controllers\AuthController; use App\Http\Controllers\PersonController; @@ -100,5 +101,11 @@ Route::get('/persons/by-email', [PersonController::class, 'byEmail']) ->middleware('permission:person.read'); }); + + // Secret Attachment endpoints (File Attachments API - Phase 2) + Route::post('/secrets/{secret}/attachments', [SecretAttachmentController::class, 'store']); + Route::get('/secrets/{secret}/attachments', [SecretAttachmentController::class, 'index']); + Route::get('/attachments/{attachment}/download', [SecretAttachmentController::class, 'download']); + Route::delete('/attachments/{attachment}', [SecretAttachmentController::class, 'destroy']); }); }); diff --git a/tests/Feature/Controllers/SecretAttachmentControllerTest.php b/tests/Feature/Controllers/SecretAttachmentControllerTest.php new file mode 100644 index 0000000..cea3f2f --- /dev/null +++ b/tests/Feature/Controllers/SecretAttachmentControllerTest.php @@ -0,0 +1,278 @@ + +// +// SPDX-License-Identifier: AGPL-3.0-or-later + +use App\Models\Secret; +use App\Models\SecretAttachment; +use App\Models\TenantKey; +use App\Models\User; +use Illuminate\Foundation\Testing\RefreshDatabase; +use Illuminate\Http\UploadedFile; +use Illuminate\Support\Facades\Storage; + +uses(RefreshDatabase::class); + +beforeEach(function (): void { + Storage::fake('local'); + + TenantKey::setKekPath(getTestKekPath()); + TenantKey::generateKek(); + + $keys = TenantKey::generateEnvelopeKeys(); + $this->tenant = TenantKey::create($keys); + + $this->user = User::factory()->create(); + $this->actingAs($this->user, 'sanctum'); + + $this->secret = new Secret; + $this->secret->tenant_id = $this->tenant->id; + $this->secret->owner_id = $this->user->id; + $this->secret->title_plain = 'Test Secret'; + $this->secret->save(); +}); + +afterEach(function (): void { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +describe('POST /v1/secrets/{secret}/attachments - Upload', function () { + test('uploads file successfully', function (): void { + $file = UploadedFile::fake()->create('document.pdf', 100, 'application/pdf'); + + $response = $this->postJson("/v1/secrets/{$this->secret->id}/attachments", [ + 'file' => $file, + ]); + + $response->assertStatus(201) + ->assertJsonStructure([ + 'data' => [ + 'id', + 'filename', + 'file_size', + 'mime_type', + 'download_url', + 'uploaded_by', + 'created_at', + ], + ]); + + // Verify attachment created in DB + $this->assertDatabaseHas('secret_attachments', [ + 'secret_id' => $this->secret->id, + 'mime_type' => 'application/pdf', + 'uploaded_by' => $this->user->id, + ]); + + // Verify file encrypted in storage + $attachment = SecretAttachment::latest()->first(); + Storage::disk('local')->assertExists($attachment->storage_path); + }); + + test('validates file is required', function (): void { + $response = $this->postJson("/v1/secrets/{$this->secret->id}/attachments", []); + + $response->assertStatus(422) + ->assertJsonValidationErrors(['file']); + }); + + test('validates file size limit', function (): void { + $maxSize = config('attachments.max_file_size'); + $file = UploadedFile::fake()->create('large.pdf', ($maxSize / 1024) + 1, 'application/pdf'); + + $response = $this->postJson("/v1/secrets/{$this->secret->id}/attachments", [ + 'file' => $file, + ]); + + $response->assertStatus(422) + ->assertJsonValidationErrors(['file']); + }); + + test('validates allowed mime types', function (): void { + $file = UploadedFile::fake()->create('executable.exe', 100, 'application/x-msdownload'); + + $response = $this->postJson("/v1/secrets/{$this->secret->id}/attachments", [ + 'file' => $file, + ]); + + $response->assertStatus(422) + ->assertJsonValidationErrors(['file']); + }); + + test('requires authorization (non-owner)', function (): void { + $otherUser = User::factory()->create(); + $this->actingAs($otherUser, 'sanctum'); + + $file = UploadedFile::fake()->create('document.pdf', 100); + + $response = $this->postJson("/v1/secrets/{$this->secret->id}/attachments", [ + 'file' => $file, + ]); + + $response->assertStatus(403); + }); +}); + +describe('GET /v1/secrets/{secret}/attachments - List', function () { + test('lists attachments for secret', function (): void { + // Create 2 attachments + $att1 = new SecretAttachment; + $att1->id = \Illuminate\Support\Str::uuid(); + $att1->secret_id = $this->secret->id; + $att1->tenant_id = $this->tenant->id; + $att1->filename_plain = 'file1.pdf'; + $att1->file_size = 1024; + $att1->mime_type = 'application/pdf'; + $att1->storage_path = 'test/path1.enc'; + $att1->checksum_sha256 = hash('sha256', 'test1'); + $att1->uploaded_by = $this->user->id; + $att1->save(); + + $att2 = new SecretAttachment; + $att2->id = \Illuminate\Support\Str::uuid(); + $att2->secret_id = $this->secret->id; + $att2->tenant_id = $this->tenant->id; + $att2->filename_plain = 'file2.jpg'; + $att2->file_size = 2048; + $att2->mime_type = 'image/jpeg'; + $att2->storage_path = 'test/path2.enc'; + $att2->checksum_sha256 = hash('sha256', 'test2'); + $att2->uploaded_by = $this->user->id; + $att2->save(); + + $response = $this->getJson("/v1/secrets/{$this->secret->id}/attachments"); + + $response->assertStatus(200) + ->assertJsonCount(2, 'data') + ->assertJsonStructure([ + 'data' => [ + '*' => ['id', 'filename', 'file_size', 'mime_type', 'download_url', 'created_at'], + ], + ]); + }); + + test('returns empty array when no attachments', function (): void { + $response = $this->getJson("/v1/secrets/{$this->secret->id}/attachments"); + + $response->assertStatus(200) + ->assertJson(['data' => []]); + }); + + test('requires authorization (non-owner)', function (): void { + $otherUser = User::factory()->create(); + $this->actingAs($otherUser, 'sanctum'); + + $response = $this->getJson("/v1/secrets/{$this->secret->id}/attachments"); + + $response->assertStatus(403); + }); +}); + +describe('GET /v1/attachments/{attachment}/download - Download', function () { + test('downloads attachment with correct headers', function (): void { + $file = UploadedFile::fake()->create('document.pdf', 100, 'application/pdf'); + + // Upload first + $uploadResponse = $this->postJson("/v1/secrets/{$this->secret->id}/attachments", [ + 'file' => $file, + ]); + + $attachmentId = $uploadResponse->json('data.id'); + + // Download + $response = $this->get("/v1/attachments/{$attachmentId}/download"); + + $response->assertStatus(200) + ->assertHeader('Content-Type', 'application/pdf') + ->assertHeader('Content-Disposition', 'attachment; filename="document.pdf"'); + }); + + test('requires authorization (non-owner)', function (): void { + $attachment = new SecretAttachment; + $attachment->id = \Illuminate\Support\Str::uuid(); + $attachment->secret_id = $this->secret->id; + $attachment->tenant_id = $this->tenant->id; + $attachment->filename_plain = 'test.pdf'; + $attachment->file_size = 1024; + $attachment->mime_type = 'application/pdf'; + $attachment->storage_path = 'test/path.enc'; + $attachment->checksum_sha256 = hash('sha256', 'test'); + $attachment->uploaded_by = $this->user->id; + $attachment->save(); + + $otherUser = User::factory()->create(); + $this->actingAs($otherUser, 'sanctum'); + + $response = $this->get("/v1/attachments/{$attachment->id}/download"); + + $response->assertStatus(403); + }); +}); + +describe('DELETE /v1/attachments/{attachment} - Delete', function () { + test('deletes attachment successfully', function (): void { + $file = UploadedFile::fake()->create('document.pdf', 100, 'application/pdf'); + + // Upload first + $uploadResponse = $this->postJson("/v1/secrets/{$this->secret->id}/attachments", [ + 'file' => $file, + ]); + + $attachmentId = $uploadResponse->json('data.id'); + + // Delete + $response = $this->deleteJson("/v1/attachments/{$attachmentId}"); + + $response->assertStatus(204); + + // Verify deleted from DB + $this->assertDatabaseMissing('secret_attachments', [ + 'id' => $attachmentId, + ]); + }); + + test('deletes attachment and verifies cascade', function (): void { + $file = UploadedFile::fake()->create('document.pdf', 100, 'application/pdf'); + Storage::put('test/path.enc', 'encrypted content'); + + $attachment = new SecretAttachment; + $attachment->id = \Illuminate\Support\Str::uuid(); + $attachment->secret_id = $this->secret->id; + $attachment->tenant_id = $this->tenant->id; + $attachment->filename_plain = 'document.pdf'; + $attachment->file_size = 100; + $attachment->mime_type = 'application/pdf'; + $attachment->storage_path = 'test/path.enc'; + $attachment->checksum_sha256 = hash('sha256', 'test'); + $attachment->uploaded_by = $this->user->id; + $attachment->save(); + + $response = $this->deleteJson("/v1/attachments/{$attachment->id}"); + + $response->assertStatus(204); + Storage::assertMissing('local', $attachment->storage_path); + }); + + test('requires authorization (non-owner)', function (): void { + $attachment = new SecretAttachment; + $attachment->id = \Illuminate\Support\Str::uuid(); + $attachment->secret_id = $this->secret->id; + $attachment->tenant_id = $this->tenant->id; + $attachment->filename_plain = 'test.pdf'; + $attachment->file_size = 1024; + $attachment->mime_type = 'application/pdf'; + $attachment->storage_path = 'test/path.enc'; + $attachment->checksum_sha256 = hash('sha256', 'test'); + $attachment->uploaded_by = $this->user->id; + $attachment->save(); + + $otherUser = User::factory()->create(); + $this->actingAs($otherUser, 'sanctum'); + + $response = $this->deleteJson("/v1/attachments/{$attachment->id}"); + + $response->assertStatus(403); + }); +}); From edb17bf4e53c2327f0f20215cbd5482fe4ccd84a Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:44:11 +0100 Subject: [PATCH 07/28] fix: improve attachment validation and SPDX headers - Use 'mimetypes:' instead of 'mimes:' for true MIME type validation - Remove mimeToExtension helper (no longer needed) - Fix test file SPDX header to use 'SecPal Contributors' format - Security: Prevents upload of malicious files with fake extensions --- .../Api/V1/SecretAttachmentController.php | 35 +------------------ .../SecretAttachmentControllerTest.php | 2 +- 2 files changed, 2 insertions(+), 35 deletions(-) diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php index 79f73e5..650c367 100644 --- a/app/Http/Controllers/Api/V1/SecretAttachmentController.php +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -51,7 +51,7 @@ public function store(Request $request, Secret $secret): JsonResponse 'required', 'file', 'max:'.(($maxSize / 1024)), // Laravel expects KB - 'mimes:'.implode(',', array_map(fn ($mime) => $this->mimeToExtension($mime), $allowedMimes)), + 'mimetypes:'.implode(',', $allowedMimes), ], ]); @@ -131,37 +131,4 @@ public function destroy(SecretAttachment $attachment): Response return response()->noContent(); } - - /** - * Map MIME type to file extension for validation. - * - * @param string $mime - * @return string - */ - private function mimeToExtension(string $mime): string - { - return match ($mime) { - 'image/jpeg' => 'jpg,jpeg', - 'image/png' => 'png', - 'image/gif' => 'gif', - 'image/webp' => 'webp', - 'application/pdf' => 'pdf', - 'application/msword' => 'doc', - 'application/vnd.openxmlformats-officedocument.wordprocessingml.document' => 'docx', - 'application/vnd.ms-excel' => 'xls', - 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' => 'xlsx', - 'application/vnd.ms-powerpoint' => 'ppt', - 'application/vnd.openxmlformats-officedocument.presentationml.presentation' => 'pptx', - 'text/plain' => 'txt', - 'text/csv' => 'csv', - 'text/html' => 'html', - 'text/markdown' => 'md', - 'application/zip' => 'zip', - 'application/x-7z-compressed' => '7z', - 'application/x-rar-compressed' => 'rar', - 'application/json' => 'json', - 'application/xml' => 'xml', - default => '*', - }; - } } diff --git a/tests/Feature/Controllers/SecretAttachmentControllerTest.php b/tests/Feature/Controllers/SecretAttachmentControllerTest.php index cea3f2f..2db3d40 100644 --- a/tests/Feature/Controllers/SecretAttachmentControllerTest.php +++ b/tests/Feature/Controllers/SecretAttachmentControllerTest.php @@ -1,6 +1,6 @@ +// SPDX-FileCopyrightText: 2025 SecPal Contributors // // SPDX-License-Identifier: AGPL-3.0-or-later From c504d694cec07e989ca185ffd200bad7d1b82976 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:45:02 +0100 Subject: [PATCH 08/28] fix: update AttachmentStorageService SPDX header - Use 'SecPal Contributors' format - Matches project standard across all files --- app/Services/AttachmentStorageService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Services/AttachmentStorageService.php b/app/Services/AttachmentStorageService.php index 5215730..6e92cb9 100644 --- a/app/Services/AttachmentStorageService.php +++ b/app/Services/AttachmentStorageService.php @@ -1,6 +1,6 @@ +// SPDX-FileCopyrightText: 2025 SecPal Contributors // // SPDX-License-Identifier: AGPL-3.0-or-later From a9a6954c20674c1db6f103bea55ae6f1d2690d61 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:49:36 +0100 Subject: [PATCH 09/28] fix: update SecretAttachmentPolicy SPDX header - Use 'SecPal Contributors' format - Matches project standard --- app/Policies/SecretAttachmentPolicy.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Policies/SecretAttachmentPolicy.php b/app/Policies/SecretAttachmentPolicy.php index e34034e..9dbb6b5 100644 --- a/app/Policies/SecretAttachmentPolicy.php +++ b/app/Policies/SecretAttachmentPolicy.php @@ -1,6 +1,6 @@ +// SPDX-FileCopyrightText: 2025 SecPal Contributors // // SPDX-License-Identifier: AGPL-3.0-or-later From 266ff3ab9284064795ef01e5d14f35618fb5f23a Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:50:43 +0100 Subject: [PATCH 10/28] docs: update CHANGELOG for File Attachments API - Added entry for Issue #175 (Phase 2) - Documents all 4 endpoints and key features - Notes encryption, authorization, and test coverage Related: #175 --- CHANGELOG.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d2a82c..0967d19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **File Attachments API (Phase 2)** (#175) + + - Upload encrypted file attachments to secrets (POST `/v1/secrets/{secret}/attachments`) + - List attachments for a secret (GET `/v1/secrets/{secret}/attachments`) + - Download decrypted attachments (GET `/v1/attachments/{attachment}/download`) + - Delete attachments (DELETE `/v1/attachments/{attachment}`) + - Files encrypted at rest using tenant DEK encryption + - Configurable file size limits and MIME type restrictions + - Owner-based authorization via `SecretAttachmentPolicy` + - OpenAPI documentation for all attachment endpoints + - Comprehensive test coverage: 13 Controller tests, 3 Service tests, 2 Model tests, 8 Policy tests + - **Code Coverage Integration** (#170) - Integrated Codecov for automated coverage tracking - PHPUnit now generates Clover XML coverage reports @@ -35,6 +47,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **Role Management CRUD API** (#108, Phase 4) + - New endpoint: `GET /v1/roles` - List all roles with permission count and user count - New endpoint: `POST /v1/roles` - Create new role with permissions - New endpoint: `GET /v1/roles/{id}` - Get role details with assigned permissions @@ -48,6 +61,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), completes role management capabilities - **Predefined Roles Seeder** (#108, Phase 4) + - New seeder: `RolesAndPermissionsSeeder` - Creates 5 predefined roles with permissions - Predefined roles: Admin, Manager, Guard, Client, Works Council - Idempotent design: Safe to run multiple times, uses `firstOrCreate` @@ -58,6 +72,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), provides production-ready role foundation - **RBAC Documentation** (#108, Phase 4) + - New guide: `docs/guides/role-management.md` - How to create/manage roles (872 lines) - New guide: `docs/guides/permission-system.md` - Permission naming conventions and organization (716 lines) - New guide: `docs/guides/temporal-roles.md` - Temporal role assignment patterns @@ -69,6 +84,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), completes RBAC documentation requirements - **User Direct Permission Assignment API** (#138) + - New endpoint: `GET /v1/users/{user}/permissions` - List all user permissions (direct + inherited from roles) - New endpoint: `POST /v1/users/{user}/permissions` - Assign direct permission(s) to user with temporal tracking (audit trail) - New endpoint: `DELETE /v1/users/{user}/permissions/{permission}` - Revoke direct permission from user @@ -81,6 +97,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), enables fine-grained permission control bypassing roles - **Permission Management CRUD API** (#137) + - New endpoint: `GET /v1/permissions` - List all permissions grouped by resource - New endpoint: `POST /v1/permissions` - Create new permission with strict naming validation (resource.action) - New endpoint: `GET /v1/permissions/{id}` - Get permission details with assigned roles @@ -93,6 +110,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), enables Issue #138 (User Direct Permission Assignment) - **RBAC Architecture Documentation** (#143) + - New file: `docs/rbac-architecture.md` - Central RBAC system documentation - System architecture: High-level component diagrams (Users → Roles → Permissions + Direct Permissions) - Core concepts: Roles, Permissions, Direct Permissions, Temporal Assignments From a86e31d1a37b129a9ca8eb9d439c7360f1ee9afd Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:51:33 +0100 Subject: [PATCH 11/28] style: format CHANGELOG.md with Prettier --- CHANGELOG.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0967d19..3ff240e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **File Attachments API (Phase 2)** (#175) - - Upload encrypted file attachments to secrets (POST `/v1/secrets/{secret}/attachments`) - List attachments for a secret (GET `/v1/secrets/{secret}/attachments`) - Download decrypted attachments (GET `/v1/attachments/{attachment}/download`) @@ -47,7 +46,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - **Role Management CRUD API** (#108, Phase 4) - - New endpoint: `GET /v1/roles` - List all roles with permission count and user count - New endpoint: `POST /v1/roles` - Create new role with permissions - New endpoint: `GET /v1/roles/{id}` - Get role details with assigned permissions @@ -61,7 +59,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), completes role management capabilities - **Predefined Roles Seeder** (#108, Phase 4) - - New seeder: `RolesAndPermissionsSeeder` - Creates 5 predefined roles with permissions - Predefined roles: Admin, Manager, Guard, Client, Works Council - Idempotent design: Safe to run multiple times, uses `firstOrCreate` @@ -72,7 +69,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), provides production-ready role foundation - **RBAC Documentation** (#108, Phase 4) - - New guide: `docs/guides/role-management.md` - How to create/manage roles (872 lines) - New guide: `docs/guides/permission-system.md` - Permission naming conventions and organization (716 lines) - New guide: `docs/guides/temporal-roles.md` - Temporal role assignment patterns @@ -84,7 +80,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), completes RBAC documentation requirements - **User Direct Permission Assignment API** (#138) - - New endpoint: `GET /v1/users/{user}/permissions` - List all user permissions (direct + inherited from roles) - New endpoint: `POST /v1/users/{user}/permissions` - Assign direct permission(s) to user with temporal tracking (audit trail) - New endpoint: `DELETE /v1/users/{user}/permissions/{permission}` - Revoke direct permission from user @@ -97,7 +92,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), enables fine-grained permission control bypassing roles - **Permission Management CRUD API** (#137) - - New endpoint: `GET /v1/permissions` - List all permissions grouped by resource - New endpoint: `POST /v1/permissions` - Create new permission with strict naming validation (resource.action) - New endpoint: `GET /v1/permissions/{id}` - Get permission details with assigned roles @@ -110,7 +104,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Part of RBAC Phase 4 Epic (#108), enables Issue #138 (User Direct Permission Assignment) - **RBAC Architecture Documentation** (#143) - - New file: `docs/rbac-architecture.md` - Central RBAC system documentation - System architecture: High-level component diagrams (Users → Roles → Permissions + Direct Permissions) - Core concepts: Roles, Permissions, Direct Permissions, Temporal Assignments From 120d56d5c746722062c9146808fd9607e7d14b30 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:55:15 +0100 Subject: [PATCH 12/28] fix: resolve PHPStan Level Max type safety issues - Add User import to Controller - Add type annotations for config values - Add null checks for file_get_contents, json_encode, tenantKey - Add null check for getMimeType() - Add runtime exceptions for validation failures - Fix BelongsTo return types using $this - Simplify delete() return type (always returns true) - Cast division to int for max file size validation All changes maintain runtime behavior while satisfying static analysis. --- .../Api/V1/SecretAttachmentController.php | 10 ++++- app/Models/SecretAttachment.php | 4 +- app/Services/AttachmentStorageService.php | 37 +++++++++++++++++-- 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php index 650c367..5022751 100644 --- a/app/Http/Controllers/Api/V1/SecretAttachmentController.php +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -11,6 +11,7 @@ use App\Http\Controllers\Controller; use App\Models\Secret; use App\Models\SecretAttachment; +use App\Models\User; use App\Services\AttachmentStorageService; use Illuminate\Http\JsonResponse; use Illuminate\Http\Request; @@ -43,14 +44,16 @@ public function store(Request $request, Secret $secret): JsonResponse { Gate::authorize('create', [SecretAttachment::class, $secret]); + /** @var int<1, max> $maxSize */ $maxSize = config('attachments.max_file_size'); + /** @var array $allowedMimes */ $allowedMimes = config('attachments.allowed_mime_types'); $validated = $request->validate([ 'file' => [ 'required', 'file', - 'max:'.(($maxSize / 1024)), // Laravel expects KB + 'max:'.((int) ($maxSize / 1024)), // Laravel expects KB 'mimetypes:'.implode(',', $allowedMimes), ], ]); @@ -58,7 +61,10 @@ public function store(Request $request, Secret $secret): JsonResponse /** @var \Illuminate\Http\UploadedFile $file */ $file = $validated['file']; - $attachment = $this->storageService->store($file, $secret, $request->user()); + $user = $request->user(); + assert($user instanceof User, 'User must be authenticated'); + + $attachment = $this->storageService->store($file, $secret, $user); return response()->json([ 'data' => [ diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php index 719f839..f63e7d3 100644 --- a/app/Models/SecretAttachment.php +++ b/app/Models/SecretAttachment.php @@ -133,7 +133,7 @@ public function getDownloadUrlAttribute(): string /** * Get the secret that owns this attachment. * - * @return BelongsTo + * @return BelongsTo */ public function secret(): BelongsTo { @@ -143,7 +143,7 @@ public function secret(): BelongsTo /** * Get the user who uploaded this attachment. * - * @return BelongsTo + * @return BelongsTo */ public function uploader(): BelongsTo { diff --git a/app/Services/AttachmentStorageService.php b/app/Services/AttachmentStorageService.php index 6e92cb9..ebb212f 100644 --- a/app/Services/AttachmentStorageService.php +++ b/app/Services/AttachmentStorageService.php @@ -33,12 +33,19 @@ public function store(UploadedFile $file, Secret $secret, User $user): SecretAtt { // Read original file content $content = file_get_contents($file->getRealPath()); + if ($content === false) { + throw new \RuntimeException('Failed to read uploaded file'); + } // Calculate checksum (before encryption) $checksum = hash('sha256', $content); // Encrypt with tenant DEK $tenant = $secret->tenantKey; + if ($tenant === null) { + throw new \RuntimeException('Secret must have an associated tenant key'); + } + $encrypted = $tenant->encrypt($content); // Generate storage path @@ -51,10 +58,15 @@ public function store(UploadedFile $file, Secret $secret, User $user): SecretAtt ); // Store encrypted blob as JSON - Storage::disk('local')->put($storagePath, json_encode([ + $jsonBlob = json_encode([ 'ciphertext' => base64_encode($encrypted['ciphertext']), 'nonce' => base64_encode($encrypted['nonce']), - ])); + ]); + if ($jsonBlob === false) { + throw new \RuntimeException('Failed to encode encrypted data'); + } + + Storage::disk('local')->put($storagePath, $jsonBlob); // Create attachment record $attachment = new SecretAttachment(); @@ -63,7 +75,11 @@ public function store(UploadedFile $file, Secret $secret, User $user): SecretAtt $attachment->tenant_id = $secret->tenant_id; // MUST be set BEFORE encrypted fields $attachment->filename_plain = $file->getClientOriginalName(); // Triggers encryption $attachment->file_size = $file->getSize(); - $attachment->mime_type = $file->getMimeType(); + $mimeType = $file->getMimeType(); + if ($mimeType === null) { + throw new \RuntimeException('Failed to determine file MIME type'); + } + $attachment->mime_type = $mimeType; $attachment->storage_path = $storagePath; $attachment->checksum_sha256 = $checksum; $attachment->uploaded_by = $user->id; @@ -80,10 +96,21 @@ public function retrieve(SecretAttachment $attachment): string { // Read encrypted blob from storage $encryptedBlob = Storage::disk('local')->get($attachment->storage_path); + if ($encryptedBlob === null) { + throw new \RuntimeException('Attachment file not found in storage'); + } + $decoded = json_decode($encryptedBlob, true); + if (! is_array($decoded) || ! isset($decoded['ciphertext'], $decoded['nonce'])) { + throw new \RuntimeException('Invalid encrypted blob format'); + } // Decrypt with tenant DEK $tenant = $attachment->secret->tenantKey; + if ($tenant === null) { + throw new \RuntimeException('Attachment secret must have an associated tenant key'); + } + $decrypted = $tenant->decrypt( base64_decode($decoded['ciphertext']), base64_decode($decoded['nonce']) @@ -104,6 +131,8 @@ public function delete(SecretAttachment $attachment): bool Storage::disk('local')->delete($attachment->storage_path); // Delete attachment record - return $attachment->delete(); + $attachment->delete(); + + return true; } } From 747d5a1cb369416a4dfc15332af3329f24b7deb4 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 12:59:59 +0100 Subject: [PATCH 13/28] fix: resolve remaining PHPStan type safety issues - Add explicit check and type guard for validated['file'] - Add type annotation for tenant_id assignment - Add string type validation for decoded ciphertext/nonce before base64_decode All 13 Controller tests still passing. --- .../Controllers/Api/V1/SecretAttachmentController.php | 4 ++++ app/Services/AttachmentStorageService.php | 8 +++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php index 5022751..ac45864 100644 --- a/app/Http/Controllers/Api/V1/SecretAttachmentController.php +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -58,6 +58,10 @@ public function store(Request $request, Secret $secret): JsonResponse ], ]); + if (! isset($validated['file']) || ! ($validated['file'] instanceof \Illuminate\Http\UploadedFile)) { + throw new \InvalidArgumentException('Valid file upload required'); + } + /** @var \Illuminate\Http\UploadedFile $file */ $file = $validated['file']; diff --git a/app/Services/AttachmentStorageService.php b/app/Services/AttachmentStorageService.php index ebb212f..f9b4e3c 100644 --- a/app/Services/AttachmentStorageService.php +++ b/app/Services/AttachmentStorageService.php @@ -72,7 +72,9 @@ public function store(UploadedFile $file, Secret $secret, User $user): SecretAtt $attachment = new SecretAttachment(); $attachment->id = $attachmentId; $attachment->secret_id = $secret->id; - $attachment->tenant_id = $secret->tenant_id; // MUST be set BEFORE encrypted fields + /** @var int<0, max> $tenantId */ + $tenantId = $secret->tenant_id; + $attachment->tenant_id = $tenantId; // MUST be set BEFORE encrypted fields $attachment->filename_plain = $file->getClientOriginalName(); // Triggers encryption $attachment->file_size = $file->getSize(); $mimeType = $file->getMimeType(); @@ -105,6 +107,10 @@ public function retrieve(SecretAttachment $attachment): string throw new \RuntimeException('Invalid encrypted blob format'); } + if (! is_string($decoded['ciphertext']) || ! is_string($decoded['nonce'])) { + throw new \RuntimeException('Invalid encrypted blob data types'); + } + // Decrypt with tenant DEK $tenant = $attachment->secret->tenantKey; if ($tenant === null) { From 31ead28caa5a2444229fec5690e74425fed01ce6 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:03:01 +0100 Subject: [PATCH 14/28] fix: add type annotation for validated array in Controller - PHPStan now understands $validated is array - Resolves final offsetAccess.nonOffsetAccessible error --- app/Http/Controllers/Api/V1/SecretAttachmentController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php index ac45864..ae01106 100644 --- a/app/Http/Controllers/Api/V1/SecretAttachmentController.php +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -49,6 +49,7 @@ public function store(Request $request, Secret $secret): JsonResponse /** @var array $allowedMimes */ $allowedMimes = config('attachments.allowed_mime_types'); + /** @var array $validated */ $validated = $request->validate([ 'file' => [ 'required', From cc14df6420e23eb9ec2d24330b49ec6382039f55 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:14:25 +0100 Subject: [PATCH 15/28] wip: SecretAttachment Model tests need fixing - Secret::create() doesn't follow explicit property pattern - SecretAttachment::create() doesn't set tenant_id before encrypted fields - 5 Model unit tests failing due to encryption pattern violations - Controller tests (13/13) all passing - Will create separate issue to fix Model tests properly Related: #175 --- tests/Feature/Models/SecretAttachmentTest.php | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/tests/Feature/Models/SecretAttachmentTest.php b/tests/Feature/Models/SecretAttachmentTest.php index 33ae982..19ea0a8 100644 --- a/tests/Feature/Models/SecretAttachmentTest.php +++ b/tests/Feature/Models/SecretAttachmentTest.php @@ -30,6 +30,18 @@ TenantKey::setKekPath(null); }); +// Helper to create Secret with proper encryption pattern (tenant_id BEFORE title_plain) +function createTestSecret($test, array $attributes): Secret +{ + $secret = new Secret(); + $secret->tenant_id = $attributes['tenant_id']; + $secret->owner_id = $attributes['owner_id']; + $secret->title_plain = $attributes['title_plain'] ?? 'Test Secret'; + $secret->save(); + + return $secret; +} + test('secret attachment has correct fillable fields', function (): void { $model = new SecretAttachment(); @@ -50,7 +62,7 @@ }); test('secret attachment uses UUID primary key', function (): void { - $secret = Secret::create([ + $secret = createTestSecret($this, [ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Test Secret for Attachment', @@ -58,6 +70,7 @@ $attachment = SecretAttachment::create([ 'secret_id' => $secret->id, + 'tenant_id' => $this->tenant->id, 'filename_plain' => 'test.pdf', 'file_size' => 1024, 'mime_type' => 'application/pdf', @@ -71,7 +84,7 @@ }); test('secret attachment encrypts filename with EncryptedWithDek cast', function (): void { - $secret = Secret::create([ + $secret = createTestSecret($this, [ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Secret with Encrypted Attachment', @@ -79,6 +92,7 @@ $attachment = SecretAttachment::create([ 'secret_id' => $secret->id, + 'tenant_id' => $this->tenant->id, 'filename_plain' => 'secret-document.pdf', 'file_size' => 2048, 'mime_type' => 'application/pdf', @@ -98,7 +112,7 @@ }); test('secret attachment belongs to secret', function (): void { - $secret = Secret::create([ + $secret = createTestSecret($this, [ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Secret with Relationship', @@ -106,6 +120,7 @@ $attachment = SecretAttachment::create([ 'secret_id' => $secret->id, + 'tenant_id' => $this->tenant->id, 'filename_plain' => 'test.jpg', 'file_size' => 512, 'mime_type' => 'image/jpeg', @@ -119,7 +134,7 @@ }); test('secret attachment belongs to user (uploaded_by)', function (): void { - $secret = Secret::create([ + $secret = createTestSecret($this, [ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Secret for Uploader Test', @@ -127,6 +142,7 @@ $attachment = SecretAttachment::create([ 'secret_id' => $secret->id, + 'tenant_id' => $this->tenant->id, 'filename_plain' => 'report.docx', 'file_size' => 4096, 'mime_type' => 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', @@ -140,7 +156,7 @@ }); test('secret attachment has download_url accessor', function (): void { - $secret = Secret::create([ + $secret = createTestSecret($this, [ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Secret for Download URL Test', @@ -148,6 +164,7 @@ $attachment = SecretAttachment::create([ 'secret_id' => $secret->id, + 'tenant_id' => $this->tenant->id, 'filename_plain' => 'invoice.pdf', 'file_size' => 1536, 'mime_type' => 'application/pdf', From 47aef018a4aed973c34fa8755d66e425d0e8d361 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:24:18 +0100 Subject: [PATCH 16/28] style: fix Laravel Pint formatting issues --- .../Api/V1/SecretAttachmentController.php | 13 ------------- app/Models/SecretAttachment.php | 6 ------ app/Services/AttachmentStorageService.php | 6 ++++-- tests/Feature/Models/SecretAttachmentTest.php | 7 +++---- .../Services/AttachmentStorageServiceTest.php | 8 ++++---- 5 files changed, 11 insertions(+), 29 deletions(-) diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php index ae01106..4be0179 100644 --- a/app/Http/Controllers/Api/V1/SecretAttachmentController.php +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -35,10 +35,6 @@ public function __construct( /** * Upload attachment to secret. - * - * @param \Illuminate\Http\Request $request - * @param \App\Models\Secret $secret - * @return \Illuminate\Http\JsonResponse */ public function store(Request $request, Secret $secret): JsonResponse { @@ -86,9 +82,6 @@ public function store(Request $request, Secret $secret): JsonResponse /** * List attachments for secret. - * - * @param \App\Models\Secret $secret - * @return \Illuminate\Http\JsonResponse */ public function index(Secret $secret): JsonResponse { @@ -110,9 +103,6 @@ public function index(Secret $secret): JsonResponse /** * Download attachment. - * - * @param \App\Models\SecretAttachment $attachment - * @return \Illuminate\Http\Response */ public function download(SecretAttachment $attachment): Response { @@ -130,9 +120,6 @@ public function download(SecretAttachment $attachment): Response /** * Delete attachment. - * - * @param \App\Models\SecretAttachment $attachment - * @return \Illuminate\Http\Response */ public function destroy(SecretAttachment $attachment): Response { diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php index f63e7d3..84de288 100644 --- a/app/Models/SecretAttachment.php +++ b/app/Models/SecretAttachment.php @@ -99,8 +99,6 @@ protected function casts(): array /** * Set plaintext filename (transient). - * - * @param string $value */ public function setFilenamePlainAttribute(string $value): void { @@ -112,8 +110,6 @@ public function setFilenamePlainAttribute(string $value): void * Get plaintext filename (read accessor). * * Falls back to decrypting filename_enc if transient is null. - * - * @return string|null */ public function getFilenamePlainAttribute(): ?string { @@ -122,8 +118,6 @@ public function getFilenamePlainAttribute(): ?string /** * Get download URL for this attachment. - * - * @return string */ public function getDownloadUrlAttribute(): string { diff --git a/app/Services/AttachmentStorageService.php b/app/Services/AttachmentStorageService.php index f9b4e3c..64857ff 100644 --- a/app/Services/AttachmentStorageService.php +++ b/app/Services/AttachmentStorageService.php @@ -69,7 +69,7 @@ public function store(UploadedFile $file, Secret $secret, User $user): SecretAtt Storage::disk('local')->put($storagePath, $jsonBlob); // Create attachment record - $attachment = new SecretAttachment(); + $attachment = new SecretAttachment; $attachment->id = $attachmentId; $attachment->secret_id = $secret->id; /** @var int<0, max> $tenantId */ @@ -88,7 +88,9 @@ public function store(UploadedFile $file, Secret $secret, User $user): SecretAtt $attachment->save(); return $attachment; - } /** + } + + /** * Retrieve and decrypt file content. * * @param SecretAttachment $attachment The attachment to retrieve diff --git a/tests/Feature/Models/SecretAttachmentTest.php b/tests/Feature/Models/SecretAttachmentTest.php index 19ea0a8..b2ca6ed 100644 --- a/tests/Feature/Models/SecretAttachmentTest.php +++ b/tests/Feature/Models/SecretAttachmentTest.php @@ -33,7 +33,7 @@ // Helper to create Secret with proper encryption pattern (tenant_id BEFORE title_plain) function createTestSecret($test, array $attributes): Secret { - $secret = new Secret(); + $secret = new Secret; $secret->tenant_id = $attributes['tenant_id']; $secret->owner_id = $attributes['owner_id']; $secret->title_plain = $attributes['title_plain'] ?? 'Test Secret'; @@ -43,7 +43,7 @@ function createTestSecret($test, array $attributes): Secret } test('secret attachment has correct fillable fields', function (): void { - $model = new SecretAttachment(); + $model = new SecretAttachment; expect($model->getFillable())->toContain('secret_id'); expect($model->getFillable())->toContain('filename_enc'); @@ -55,7 +55,7 @@ function createTestSecret($test, array $attributes): Secret }); test('secret attachment hides encrypted fields', function (): void { - $model = new SecretAttachment(); + $model = new SecretAttachment; expect($model->getHidden())->toContain('filename_enc'); expect($model->getHidden())->toContain('storage_path'); @@ -178,4 +178,3 @@ function createTestSecret($test, array $attributes): Secret expect($attachment->download_url)->toContain($attachment->id); expect($attachment->download_url)->toContain('/download'); }); - diff --git a/tests/Feature/Services/AttachmentStorageServiceTest.php b/tests/Feature/Services/AttachmentStorageServiceTest.php index c91cc6f..059699a 100644 --- a/tests/Feature/Services/AttachmentStorageServiceTest.php +++ b/tests/Feature/Services/AttachmentStorageServiceTest.php @@ -25,7 +25,7 @@ $this->tenant = TenantKey::create($keys); $this->user = User::factory()->create(); - $this->service = new AttachmentStorageService(); + $this->service = new AttachmentStorageService; }); afterEach(function (): void { @@ -34,7 +34,7 @@ }); test('service stores file with encryption', function (): void { - $secret = new Secret(); + $secret = new Secret; $secret->tenant_id = $this->tenant->id; $secret->owner_id = $this->user->id; $secret->title_plain = 'Secret with Attachment'; @@ -54,7 +54,7 @@ }); test('service encrypts file content in storage', function (): void { - $secret = new Secret(); + $secret = new Secret; $secret->tenant_id = $this->tenant->id; $secret->owner_id = $this->user->id; $secret->title_plain = 'Secret for Encryption Test'; @@ -75,7 +75,7 @@ }); test('service retrieves and decrypts file content', function (): void { - $secret = new Secret(); + $secret = new Secret; $secret->tenant_id = $this->tenant->id; $secret->owner_id = $this->user->id; $secret->title_plain = 'Secret for Decryption Test'; From fdf15f8c454536ec3d5c1dc86febe2891846220a Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:30:19 +0100 Subject: [PATCH 17/28] fix: use explicit property assignment pattern in SecretAttachment tests Fixes #179 - Add createTestAttachment() helper function - Replace all SecretAttachment::create() with createTestAttachment() - Ensures tenant_id is set BEFORE encrypted fields (filename_plain) - Required for EncryptedWithDek cast to work correctly --- tests/Feature/Models/SecretAttachmentTest.php | 27 +++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/tests/Feature/Models/SecretAttachmentTest.php b/tests/Feature/Models/SecretAttachmentTest.php index b2ca6ed..7b75005 100644 --- a/tests/Feature/Models/SecretAttachmentTest.php +++ b/tests/Feature/Models/SecretAttachmentTest.php @@ -42,6 +42,23 @@ function createTestSecret($test, array $attributes): Secret return $secret; } +// Helper to create SecretAttachment with proper encryption pattern (tenant_id BEFORE filename_plain) +function createTestAttachment($test, array $attributes): SecretAttachment +{ + $attachment = new SecretAttachment; + $attachment->secret_id = $attributes['secret_id']; + $attachment->tenant_id = $attributes['tenant_id']; + $attachment->filename_plain = $attributes['filename_plain']; + $attachment->file_size = $attributes['file_size']; + $attachment->mime_type = $attributes['mime_type']; + $attachment->storage_path = $attributes['storage_path']; + $attachment->checksum_sha256 = $attributes['checksum_sha256']; + $attachment->uploaded_by = $attributes['uploaded_by']; + $attachment->save(); + + return $attachment; +} + test('secret attachment has correct fillable fields', function (): void { $model = new SecretAttachment; @@ -68,7 +85,7 @@ function createTestSecret($test, array $attributes): Secret 'title_plain' => 'Test Secret for Attachment', ]); - $attachment = SecretAttachment::create([ + $attachment = createTestAttachment($this, [ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'test.pdf', @@ -90,7 +107,7 @@ function createTestSecret($test, array $attributes): Secret 'title_plain' => 'Secret with Encrypted Attachment', ]); - $attachment = SecretAttachment::create([ + $attachment = createTestAttachment($this, [ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'secret-document.pdf', @@ -118,7 +135,7 @@ function createTestSecret($test, array $attributes): Secret 'title_plain' => 'Secret with Relationship', ]); - $attachment = SecretAttachment::create([ + $attachment = createTestAttachment($this, [ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'test.jpg', @@ -140,7 +157,7 @@ function createTestSecret($test, array $attributes): Secret 'title_plain' => 'Secret for Uploader Test', ]); - $attachment = SecretAttachment::create([ + $attachment = createTestAttachment($this, [ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'report.docx', @@ -162,7 +179,7 @@ function createTestSecret($test, array $attributes): Secret 'title_plain' => 'Secret for Download URL Test', ]); - $attachment = SecretAttachment::create([ + $attachment = createTestAttachment($this, [ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'invoice.pdf', From b44b434f815f57a0fe813acdd1bef5171c5adc94 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:32:33 +0100 Subject: [PATCH 18/28] fix: correct download_url accessor to include /api prefix --- app/Models/SecretAttachment.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php index 84de288..697682f 100644 --- a/app/Models/SecretAttachment.php +++ b/app/Models/SecretAttachment.php @@ -121,7 +121,7 @@ public function getFilenamePlainAttribute(): ?string */ public function getDownloadUrlAttribute(): string { - return url("/v1/attachments/{$this->id}/download"); + return url("/api/v1/attachments/{$this->id}/download"); } /** From 7fb86c19a5998ef1ca1add154966179a656c3815 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:34:50 +0100 Subject: [PATCH 19/28] fix: use /v1/ prefix in download_url accessor (not /api/v1/) --- app/Models/SecretAttachment.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php index 697682f..bdffbd3 100644 --- a/app/Models/SecretAttachment.php +++ b/app/Models/SecretAttachment.php @@ -143,4 +143,14 @@ public function uploader(): BelongsTo { return $this->belongsTo(User::class, 'uploaded_by'); } + + /** + * Get the download URL for this attachment. + */ + protected function downloadUrl(): Attribute + { + return Attribute::make( + get: fn () => url("/v1/attachments/{$this->id}/download") + ); + } } From c6262b155addaa9047c1eb37446ad12253f651ec Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:37:21 +0100 Subject: [PATCH 20/28] fix: add Attribute import and PHPStan type annotation for downloadUrl --- app/Models/SecretAttachment.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php index bdffbd3..145eb0f 100644 --- a/app/Models/SecretAttachment.php +++ b/app/Models/SecretAttachment.php @@ -7,6 +7,7 @@ namespace App\Models; use App\Casts\EncryptedWithDek; +use Illuminate\Database\Eloquent\Casts\Attribute; use Illuminate\Database\Eloquent\Concerns\HasUuids; use Illuminate\Database\Eloquent\Factories\HasFactory; use Illuminate\Database\Eloquent\Model; @@ -146,6 +147,8 @@ public function uploader(): BelongsTo /** * Get the download URL for this attachment. + * + * @return Attribute */ protected function downloadUrl(): Attribute { From 771685a1c6c3a5d8cfa9a3a0c5e57d3bae23946f Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:40:26 +0100 Subject: [PATCH 21/28] fix: correct test expectation to use /v1/ instead of /api/v1/ --- tests/Feature/Models/SecretAttachmentTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Feature/Models/SecretAttachmentTest.php b/tests/Feature/Models/SecretAttachmentTest.php index 7b75005..dceba3c 100644 --- a/tests/Feature/Models/SecretAttachmentTest.php +++ b/tests/Feature/Models/SecretAttachmentTest.php @@ -191,7 +191,7 @@ function createTestAttachment($test, array $attributes): SecretAttachment ]); expect($attachment->download_url)->toBeString(); - expect($attachment->download_url)->toContain('/api/v1/attachments/'); + expect($attachment->download_url)->toContain('/v1/attachments/'); expect($attachment->download_url)->toContain($attachment->id); expect($attachment->download_url)->toContain('/download'); }); From 03da84932f5aa7f20d483b792904fc692ca9e2ed Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 13:43:50 +0100 Subject: [PATCH 22/28] fix: remove duplicate old-style download_url accessor --- app/Models/SecretAttachment.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/Models/SecretAttachment.php b/app/Models/SecretAttachment.php index 145eb0f..857972d 100644 --- a/app/Models/SecretAttachment.php +++ b/app/Models/SecretAttachment.php @@ -117,14 +117,6 @@ public function getFilenamePlainAttribute(): ?string return $this->filenamePlain ?? $this->filename_enc; } - /** - * Get download URL for this attachment. - */ - public function getDownloadUrlAttribute(): string - { - return url("/api/v1/attachments/{$this->id}/download"); - } - /** * Get the secret that owns this attachment. * From c07384cb9bdfdbf94838ef782fde89bbbb7d52b2 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 15:39:09 +0100 Subject: [PATCH 23/28] fix: address critical Copilot review comments (storage disk, checksum, header escaping) - Use config('attachments.storage_disk') instead of hardcoded 'local' (3 locations) - Add checksum verification in retrieve() method for file integrity - Escape Content-Disposition filename to prevent header injection - Fix Storage::assertMissing() test syntax (remove disk parameter) - Add PHPStan type annotations for config() return values Addresses Copilot review comments: - Storage disk hardcoded (lines 69, 102, 139) - Missing checksum verification (line 126) - SECURITY - Unescaped Content-Disposition header (line 116) - SECURITY - Incorrect test syntax (line 255) Tests: 13 Controller + 3 Service tests passing Quality: PHPStan Level Max + Pint compliant Related: #175, #179 --- .../Api/V1/SecretAttachmentController.php | 5 ++++- app/Services/AttachmentStorageService.php | 18 +++++++++++++++--- .../SecretAttachmentControllerTest.php | 2 +- 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php index 4be0179..ec530d0 100644 --- a/app/Http/Controllers/Api/V1/SecretAttachmentController.php +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -111,9 +111,12 @@ public function download(SecretAttachment $attachment): Response $content = $this->storageService->retrieve($attachment); $filename = $attachment->getFilenamePlainAttribute(); + // Escape filename for Content-Disposition header (RFC 2231/5987) + $safeFilename = str_replace(['"', '\\'], ['', ''], $filename ?? 'download'); + return response($content, 200, [ 'Content-Type' => $attachment->mime_type, - 'Content-Disposition' => 'attachment; filename="'.$filename.'"', + 'Content-Disposition' => 'attachment; filename="'.$safeFilename.'"', 'Content-Length' => $attachment->file_size, ]); } diff --git a/app/Services/AttachmentStorageService.php b/app/Services/AttachmentStorageService.php index 64857ff..031de47 100644 --- a/app/Services/AttachmentStorageService.php +++ b/app/Services/AttachmentStorageService.php @@ -66,7 +66,9 @@ public function store(UploadedFile $file, Secret $secret, User $user): SecretAtt throw new \RuntimeException('Failed to encode encrypted data'); } - Storage::disk('local')->put($storagePath, $jsonBlob); + /** @var string $disk */ + $disk = config('attachments.storage_disk'); + Storage::disk($disk)->put($storagePath, $jsonBlob); // Create attachment record $attachment = new SecretAttachment; @@ -99,7 +101,9 @@ public function store(UploadedFile $file, Secret $secret, User $user): SecretAtt public function retrieve(SecretAttachment $attachment): string { // Read encrypted blob from storage - $encryptedBlob = Storage::disk('local')->get($attachment->storage_path); + /** @var string $disk */ + $disk = config('attachments.storage_disk'); + $encryptedBlob = Storage::disk($disk)->get($attachment->storage_path); if ($encryptedBlob === null) { throw new \RuntimeException('Attachment file not found in storage'); } @@ -124,6 +128,12 @@ public function retrieve(SecretAttachment $attachment): string base64_decode($decoded['nonce']) ); + // Verify checksum (integrity check) + $actualChecksum = hash('sha256', $decrypted); + if ($actualChecksum !== $attachment->checksum_sha256) { + throw new \RuntimeException('File integrity check failed: checksum mismatch'); + } + return $decrypted; } @@ -136,7 +146,9 @@ public function retrieve(SecretAttachment $attachment): string public function delete(SecretAttachment $attachment): bool { // Delete file from storage - Storage::disk('local')->delete($attachment->storage_path); + /** @var string $disk */ + $disk = config('attachments.storage_disk'); + Storage::disk($disk)->delete($attachment->storage_path); // Delete attachment record $attachment->delete(); diff --git a/tests/Feature/Controllers/SecretAttachmentControllerTest.php b/tests/Feature/Controllers/SecretAttachmentControllerTest.php index 2db3d40..60078ba 100644 --- a/tests/Feature/Controllers/SecretAttachmentControllerTest.php +++ b/tests/Feature/Controllers/SecretAttachmentControllerTest.php @@ -252,7 +252,7 @@ $response = $this->deleteJson("/v1/attachments/{$attachment->id}"); $response->assertStatus(204); - Storage::assertMissing('local', $attachment->storage_path); + Storage::assertMissing($attachment->storage_path); }); test('requires authorization (non-owner)', function (): void { From 58ee4c07f4c83665faf00cccb69ebf724f9faee0 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 16:03:46 +0100 Subject: [PATCH 24/28] fix: address Copilot review nitpicks (docs, helpers, N+1, file check) - Enhanced migration tenant_id documentation for EncryptedWithDek cast - Removed unused $test parameters from test helper functions - Fixed SPDX header format consistency (SecPal Contributors) - Added security warning for empty MIME types array in config - Added file existence check in delete() method (graceful handling) - Optimized attachment_count accessor to prevent N+1 queries - Removed SecretAttachmentResource (deferred to Issue #180) Addresses Copilot review comments: - Migration documentation (PRRT_kwDOQJgcLc5iPRKb) - Unused parameters (PRRT_kwDOQJgcLc5iPRLC, PRRT_kwDOQJgcLc5iPRLl) - SPDX format (PRRT_kwDOQJgcLc5iPRLh) - Config security (PRRT_kwDOQJgcLc5iPRLX) - File existence check (PRRT_kwDOQJgcLc5iPRKp) - N+1 optimization (PRRT_kwDOQJgcLc5iPRK9) Related: #175, #177, #180 --- app/Models/Secret.php | 12 ++++++++++ app/Services/AttachmentStorageService.php | 7 +++++- config/attachments.php | 5 +++- ...110234_create_secret_attachments_table.php | 6 ++++- tests/Feature/Models/SecretAttachmentTest.php | 24 +++++++++---------- .../Services/AttachmentStorageServiceTest.php | 2 +- 6 files changed, 40 insertions(+), 16 deletions(-) diff --git a/app/Models/Secret.php b/app/Models/Secret.php index b3ee600..2edbb65 100644 --- a/app/Models/Secret.php +++ b/app/Models/Secret.php @@ -265,10 +265,22 @@ public function attachments(): \Illuminate\Database\Eloquent\Relations\HasMany /** * Get count of attachments for this secret. * + * Uses the aggregated count from withCount('attachments') if available, + * otherwise performs a count query. To avoid N+1 queries, use: + * Secret::withCount('attachments')->get() + * * @return int Number of attachments */ public function getAttachmentCountAttribute(): int { + // Use aggregated count if available (from withCount()) + if (isset($this->attributes['attachments_count'])) { + /** @var int|numeric-string $count */ + $count = $this->attributes['attachments_count']; + return (int) $count; + } + + // Fallback to query (may cause N+1 if used in a loop) return $this->attachments()->count(); } diff --git a/app/Services/AttachmentStorageService.php b/app/Services/AttachmentStorageService.php index 031de47..e6e196d 100644 --- a/app/Services/AttachmentStorageService.php +++ b/app/Services/AttachmentStorageService.php @@ -148,7 +148,12 @@ public function delete(SecretAttachment $attachment): bool // Delete file from storage /** @var string $disk */ $disk = config('attachments.storage_disk'); - Storage::disk($disk)->delete($attachment->storage_path); + $storage = Storage::disk($disk); + + if ($storage->exists($attachment->storage_path)) { + $storage->delete($attachment->storage_path); + } + // Note: If file is missing, we continue - the database record should still be cleaned up // Delete attachment record $attachment->delete(); diff --git a/config/attachments.php b/config/attachments.php index ff93d6f..d24b20d 100644 --- a/config/attachments.php +++ b/config/attachments.php @@ -24,7 +24,10 @@ |-------------------------------------------------------------------------- | | List of permitted MIME types for attachments. - | Empty array allows all types. + | + | WARNING: This array MUST NOT be empty in production. + | Empty array allows ALL file types including executables (SECURITY RISK). + | Only explicitly listed MIME types are permitted when array contains values. | */ 'allowed_mime_types' => [ diff --git a/database/migrations/2025_11_16_110234_create_secret_attachments_table.php b/database/migrations/2025_11_16_110234_create_secret_attachments_table.php index 05e74a8..643b143 100644 --- a/database/migrations/2025_11_16_110234_create_secret_attachments_table.php +++ b/database/migrations/2025_11_16_110234_create_secret_attachments_table.php @@ -18,7 +18,11 @@ public function up(): void Schema::create('secret_attachments', function (Blueprint $table) { $table->uuid('id')->primary(); $table->foreignUuid('secret_id')->constrained('secrets')->cascadeOnDelete(); - $table->foreignId('tenant_id')->constrained('tenant_keys'); // Required for EncryptedWithDek cast + + // Foreign key to tenant_keys table + // REQUIRED: EncryptedWithDek cast needs tenant_id to load the correct DEK for encryption/decryption + // The tenant_id MUST be set before accessing any encrypted fields (filename_enc) + $table->foreignId('tenant_id')->constrained('tenant_keys'); // File metadata (encrypted) $table->text('filename_enc'); // Original filename (encrypted) diff --git a/tests/Feature/Models/SecretAttachmentTest.php b/tests/Feature/Models/SecretAttachmentTest.php index dceba3c..3cfed79 100644 --- a/tests/Feature/Models/SecretAttachmentTest.php +++ b/tests/Feature/Models/SecretAttachmentTest.php @@ -31,7 +31,7 @@ }); // Helper to create Secret with proper encryption pattern (tenant_id BEFORE title_plain) -function createTestSecret($test, array $attributes): Secret +function createTestSecret(array $attributes): Secret { $secret = new Secret; $secret->tenant_id = $attributes['tenant_id']; @@ -43,7 +43,7 @@ function createTestSecret($test, array $attributes): Secret } // Helper to create SecretAttachment with proper encryption pattern (tenant_id BEFORE filename_plain) -function createTestAttachment($test, array $attributes): SecretAttachment +function createTestAttachment(array $attributes): SecretAttachment { $attachment = new SecretAttachment; $attachment->secret_id = $attributes['secret_id']; @@ -79,13 +79,13 @@ function createTestAttachment($test, array $attributes): SecretAttachment }); test('secret attachment uses UUID primary key', function (): void { - $secret = createTestSecret($this, [ + $secret = createTestSecret([ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Test Secret for Attachment', ]); - $attachment = createTestAttachment($this, [ + $attachment = createTestAttachment([ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'test.pdf', @@ -101,13 +101,13 @@ function createTestAttachment($test, array $attributes): SecretAttachment }); test('secret attachment encrypts filename with EncryptedWithDek cast', function (): void { - $secret = createTestSecret($this, [ + $secret = createTestSecret([ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Secret with Encrypted Attachment', ]); - $attachment = createTestAttachment($this, [ + $attachment = createTestAttachment([ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'secret-document.pdf', @@ -129,13 +129,13 @@ function createTestAttachment($test, array $attributes): SecretAttachment }); test('secret attachment belongs to secret', function (): void { - $secret = createTestSecret($this, [ + $secret = createTestSecret([ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Secret with Relationship', ]); - $attachment = createTestAttachment($this, [ + $attachment = createTestAttachment([ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'test.jpg', @@ -151,13 +151,13 @@ function createTestAttachment($test, array $attributes): SecretAttachment }); test('secret attachment belongs to user (uploaded_by)', function (): void { - $secret = createTestSecret($this, [ + $secret = createTestSecret([ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Secret for Uploader Test', ]); - $attachment = createTestAttachment($this, [ + $attachment = createTestAttachment([ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'report.docx', @@ -173,13 +173,13 @@ function createTestAttachment($test, array $attributes): SecretAttachment }); test('secret attachment has download_url accessor', function (): void { - $secret = createTestSecret($this, [ + $secret = createTestSecret([ 'tenant_id' => $this->tenant->id, 'owner_id' => $this->user->id, 'title_plain' => 'Secret for Download URL Test', ]); - $attachment = createTestAttachment($this, [ + $attachment = createTestAttachment([ 'secret_id' => $secret->id, 'tenant_id' => $this->tenant->id, 'filename_plain' => 'invoice.pdf', diff --git a/tests/Feature/Services/AttachmentStorageServiceTest.php b/tests/Feature/Services/AttachmentStorageServiceTest.php index 059699a..03f9c37 100644 --- a/tests/Feature/Services/AttachmentStorageServiceTest.php +++ b/tests/Feature/Services/AttachmentStorageServiceTest.php @@ -1,6 +1,6 @@ +// SPDX-FileCopyrightText: 2025 SecPal Contributors // // SPDX-License-Identifier: AGPL-3.0-or-later From e327e1f474f0d712c75913ac0818a0480abdc3a0 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 16:11:37 +0100 Subject: [PATCH 25/28] style: fix Pint blank_line_before_statement in Secret.php --- app/Models/Secret.php | 1 + 1 file changed, 1 insertion(+) diff --git a/app/Models/Secret.php b/app/Models/Secret.php index 2edbb65..c235610 100644 --- a/app/Models/Secret.php +++ b/app/Models/Secret.php @@ -277,6 +277,7 @@ public function getAttachmentCountAttribute(): int if (isset($this->attributes['attachments_count'])) { /** @var int|numeric-string $count */ $count = $this->attributes['attachments_count']; + return (int) $count; } From 948cd20b0e5aeea23da3eee7c0799741747f1257 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 16:49:23 +0100 Subject: [PATCH 26/28] refactor: implement API Resources and Form Requests for SecretAttachment - Create SecretAttachmentResource for consistent JSON transformation - Create StoreSecretAttachmentRequest with config-based validation - Refactor SecretAttachmentController to use Resource and Form Request - Remove inline validation and manual JSON array building - Fix byte-to-KB conversion for file size validation - Use property accessor pattern (filename_plain vs getter method) - Add PHPStan type hints (@var, @mixin) for Level Max compliance Tests added: - 4 Resource transformation tests (single, collection, ISO8601, decryption) - 7 Form Request validation tests (required, type, size, MIME, messages) - Consolidate helper functions in tests/Pest.php for parallel test isolation Quality gates: - PHPStan Level Max: PASSING (fixed 12 type errors) - Laravel Pint: PASSING (PSR-12 compliant) - Full test suite: 342 tests passing (1085 assertions) Architecture note: - Maintained url() helper usage (NOT route() named routes) - Project uses OpenAPI contracts, not Laravel route names - Consistent with existing 20+ routes in routes/api.php Addresses Copilot review comments from PR #177: - Thread PRRT_kwDOQJgcLc5iPRK2: Use API Resources (store method) - Thread PRRT_kwDOQJgcLc5iPRKm: Use API Resources (index method) - Thread PRRT_kwDOQJgcLc5iPRLg: Move validation to Form Request - Thread PRRT_kwDOQJgcLc5iPRLc: Use property accessor pattern Related: #175, #177, #180 --- .../Api/V1/SecretAttachmentController.php | 51 +----- .../Requests/StoreSecretAttachmentRequest.php | 70 ++++++++ .../Resources/SecretAttachmentResource.php | 41 +++++ tests/Feature/Models/SecretAttachmentTest.php | 31 +--- .../StoreSecretAttachmentRequestTest.php | 78 +++++++++ .../SecretAttachmentResourceTest.php | 156 ++++++++++++++++++ tests/Pest.php | 35 ++++ 7 files changed, 390 insertions(+), 72 deletions(-) create mode 100644 app/Http/Requests/StoreSecretAttachmentRequest.php create mode 100644 app/Http/Resources/SecretAttachmentResource.php create mode 100644 tests/Feature/Requests/StoreSecretAttachmentRequestTest.php create mode 100644 tests/Feature/Resources/SecretAttachmentResourceTest.php diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php index ec530d0..2d8b205 100644 --- a/app/Http/Controllers/Api/V1/SecretAttachmentController.php +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -9,12 +9,13 @@ namespace App\Http\Controllers\Api\V1; use App\Http\Controllers\Controller; +use App\Http\Requests\StoreSecretAttachmentRequest; +use App\Http\Resources\SecretAttachmentResource; use App\Models\Secret; use App\Models\SecretAttachment; use App\Models\User; use App\Services\AttachmentStorageService; use Illuminate\Http\JsonResponse; -use Illuminate\Http\Request; use Illuminate\Http\Response; use Illuminate\Support\Facades\Gate; @@ -36,48 +37,21 @@ public function __construct( /** * Upload attachment to secret. */ - public function store(Request $request, Secret $secret): JsonResponse + public function store(StoreSecretAttachmentRequest $request, Secret $secret): JsonResponse { Gate::authorize('create', [SecretAttachment::class, $secret]); - /** @var int<1, max> $maxSize */ - $maxSize = config('attachments.max_file_size'); - /** @var array $allowedMimes */ - $allowedMimes = config('attachments.allowed_mime_types'); - - /** @var array $validated */ - $validated = $request->validate([ - 'file' => [ - 'required', - 'file', - 'max:'.((int) ($maxSize / 1024)), // Laravel expects KB - 'mimetypes:'.implode(',', $allowedMimes), - ], - ]); - - if (! isset($validated['file']) || ! ($validated['file'] instanceof \Illuminate\Http\UploadedFile)) { - throw new \InvalidArgumentException('Valid file upload required'); - } - /** @var \Illuminate\Http\UploadedFile $file */ - $file = $validated['file']; + $file = $request->validated()['file']; $user = $request->user(); assert($user instanceof User, 'User must be authenticated'); $attachment = $this->storageService->store($file, $secret, $user); - return response()->json([ - 'data' => [ - 'id' => $attachment->id, - 'filename' => $attachment->getFilenamePlainAttribute(), - 'file_size' => $attachment->file_size, - 'mime_type' => $attachment->mime_type, - 'download_url' => $attachment->download_url, - 'uploaded_by' => $attachment->uploaded_by, - 'created_at' => $attachment->created_at->toIso8601String(), - ], - ], 201); + return SecretAttachmentResource::make($attachment) + ->response() + ->setStatusCode(201); } /** @@ -90,14 +64,7 @@ public function index(Secret $secret): JsonResponse $attachments = $secret->attachments()->latest()->get(); return response()->json([ - 'data' => $attachments->map(fn ($attachment) => [ - 'id' => $attachment->id, - 'filename' => $attachment->getFilenamePlainAttribute(), - 'file_size' => $attachment->file_size, - 'mime_type' => $attachment->mime_type, - 'download_url' => $attachment->download_url, - 'created_at' => $attachment->created_at->toIso8601String(), - ]), + 'data' => SecretAttachmentResource::collection($attachments), ]); } @@ -109,7 +76,7 @@ public function download(SecretAttachment $attachment): Response Gate::authorize('view', $attachment); $content = $this->storageService->retrieve($attachment); - $filename = $attachment->getFilenamePlainAttribute(); + $filename = $attachment->filename_plain; // Escape filename for Content-Disposition header (RFC 2231/5987) $safeFilename = str_replace(['"', '\\'], ['', ''], $filename ?? 'download'); diff --git a/app/Http/Requests/StoreSecretAttachmentRequest.php b/app/Http/Requests/StoreSecretAttachmentRequest.php new file mode 100644 index 0000000..4e03f19 --- /dev/null +++ b/app/Http/Requests/StoreSecretAttachmentRequest.php @@ -0,0 +1,70 @@ +|string> + */ + public function rules(): array + { + /** @var int $maxSize */ + $maxSize = config('attachments.max_file_size'); + /** @var array $allowedMimes */ + $allowedMimes = config('attachments.allowed_mime_types'); + + return [ + 'file' => [ + 'required', + 'file', + 'max:'.($maxSize / 1024), // Convert bytes to KB for Laravel validation + 'mimes:'.implode(',', array_map( + fn (string $mime): string => str_replace('application/', '', $mime), + $allowedMimes + )), + ], + ]; + } + + /** + * Get custom error messages for validation rules. + * + * @return array + */ + public function messages(): array + { + /** @var int $maxSizeBytes */ + $maxSizeBytes = config('attachments.max_file_size'); + $maxSize = $maxSizeBytes / (1024 * 1024); // Convert bytes to MB + + return [ + 'file.required' => 'Please select a file to upload.', + 'file.file' => 'The uploaded file is not valid.', + 'file.max' => 'The file size must not exceed '.$maxSize.' MB.', + 'file.mimes' => 'The file type is not allowed. Only certain file types are permitted.', + ]; + } +} diff --git a/app/Http/Resources/SecretAttachmentResource.php b/app/Http/Resources/SecretAttachmentResource.php new file mode 100644 index 0000000..5bf8439 --- /dev/null +++ b/app/Http/Resources/SecretAttachmentResource.php @@ -0,0 +1,41 @@ + + */ + public function toArray(Request $request): array + { + /** @var \App\Models\SecretAttachment $attachment */ + $attachment = $this->resource; + + return [ + 'id' => $attachment->id, + 'filename' => $attachment->filename_plain, + 'file_size' => $attachment->file_size, + 'mime_type' => $attachment->mime_type, + 'download_url' => $attachment->download_url, + 'uploaded_by' => $attachment->uploaded_by, + 'created_at' => $attachment->created_at->toIso8601String(), + ]; + } +} diff --git a/tests/Feature/Models/SecretAttachmentTest.php b/tests/Feature/Models/SecretAttachmentTest.php index 3cfed79..959f591 100644 --- a/tests/Feature/Models/SecretAttachmentTest.php +++ b/tests/Feature/Models/SecretAttachmentTest.php @@ -25,40 +25,11 @@ $this->user = User::factory()->create(); }); -afterEach(function (): void { +afterEach(function () { cleanupTestKekFile(); TenantKey::setKekPath(null); }); -// Helper to create Secret with proper encryption pattern (tenant_id BEFORE title_plain) -function createTestSecret(array $attributes): Secret -{ - $secret = new Secret; - $secret->tenant_id = $attributes['tenant_id']; - $secret->owner_id = $attributes['owner_id']; - $secret->title_plain = $attributes['title_plain'] ?? 'Test Secret'; - $secret->save(); - - return $secret; -} - -// Helper to create SecretAttachment with proper encryption pattern (tenant_id BEFORE filename_plain) -function createTestAttachment(array $attributes): SecretAttachment -{ - $attachment = new SecretAttachment; - $attachment->secret_id = $attributes['secret_id']; - $attachment->tenant_id = $attributes['tenant_id']; - $attachment->filename_plain = $attributes['filename_plain']; - $attachment->file_size = $attributes['file_size']; - $attachment->mime_type = $attributes['mime_type']; - $attachment->storage_path = $attributes['storage_path']; - $attachment->checksum_sha256 = $attributes['checksum_sha256']; - $attachment->uploaded_by = $attributes['uploaded_by']; - $attachment->save(); - - return $attachment; -} - test('secret attachment has correct fillable fields', function (): void { $model = new SecretAttachment; diff --git a/tests/Feature/Requests/StoreSecretAttachmentRequestTest.php b/tests/Feature/Requests/StoreSecretAttachmentRequestTest.php new file mode 100644 index 0000000..a7b5031 --- /dev/null +++ b/tests/Feature/Requests/StoreSecretAttachmentRequestTest.php @@ -0,0 +1,78 @@ +rules()); + + expect($validator->fails())->toBeTrue(); + expect($validator->errors()->has('file'))->toBeTrue(); +}); + +test('form request validates file must be a file', function () { + $request = new StoreSecretAttachmentRequest; + + $validator = Validator::make(['file' => 'not-a-file'], $request->rules()); + + expect($validator->fails())->toBeTrue(); + expect($validator->errors()->has('file'))->toBeTrue(); +}); + +test('form request validates file size limit from config', function () { + $request = new StoreSecretAttachmentRequest; + + // Create file larger than max size (default 10MB = 10240KB) + $file = UploadedFile::fake()->create('large-file.pdf', 10241); // 10241KB > 10240KB + + $validator = Validator::make(['file' => $file], $request->rules()); + + expect($validator->fails())->toBeTrue(); + expect($validator->errors()->has('file'))->toBeTrue(); +}); + +test('form request validates allowed mime types from config', function () { + $request = new StoreSecretAttachmentRequest; + + // Create file with disallowed mime type (assume .exe is not allowed) + $file = UploadedFile::fake()->create('virus.exe', 100); + + $validator = Validator::make(['file' => $file], $request->rules()); + + // This test assumes config has mime type restrictions + // If config allows all types, this test will fail - that's intentional + // to catch security issues + expect($validator->fails())->toBeTrue(); + expect($validator->errors()->has('file'))->toBeTrue(); +}); + +test('form request accepts valid file with allowed mime type', function () { + $request = new StoreSecretAttachmentRequest; + + // Create valid PDF file (should be in allowed types) + $file = UploadedFile::fake()->create('document.pdf', 100); + + $validator = Validator::make(['file' => $file], $request->rules()); + + expect($validator->passes())->toBeTrue(); +}); + +test('form request has custom error messages', function () { + $request = new StoreSecretAttachmentRequest; + + expect($request->messages())->toBeArray(); + expect($request->messages())->not->toBeEmpty(); +}); + +test('form request authorization always returns true for authenticated users', function () { + $request = new StoreSecretAttachmentRequest; + + // Authorization is handled by Policy, not Form Request + expect($request->authorize())->toBeTrue(); +}); diff --git a/tests/Feature/Resources/SecretAttachmentResourceTest.php b/tests/Feature/Resources/SecretAttachmentResourceTest.php new file mode 100644 index 0000000..e036c5d --- /dev/null +++ b/tests/Feature/Resources/SecretAttachmentResourceTest.php @@ -0,0 +1,156 @@ +tenant = TenantKey::create($keys); + + // Create user + $this->user = User::factory()->create(); +}); + +afterEach(function () { + cleanupTestKekFile(); + TenantKey::setKekPath(null); +}); + +test('resource transforms single attachment correctly', function () { + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Test Secret', + ]); + + $attachment = createTestAttachment([ + 'tenant_id' => $this->tenant->id, + 'secret_id' => $secret->id, + 'uploaded_by' => $this->user->id, + 'filename_plain' => 'test-document.pdf', + 'file_size' => 1024, + 'mime_type' => 'application/pdf', + 'storage_path' => 'test-storage-path', + 'checksum_sha256' => hash('sha256', 'test-content'), + ]); + + $resource = SecretAttachmentResource::make($attachment); + $array = $resource->toArray(request()); + + expect($array)->toHaveKeys([ + 'id', + 'filename', + 'file_size', + 'mime_type', + 'download_url', + 'uploaded_by', + 'created_at', + ]); + + expect($array['id'])->toBe($attachment->id); + expect($array['filename'])->toBe('test-document.pdf'); + expect($array['file_size'])->toBe(1024); + expect($array['mime_type'])->toBe('application/pdf'); + expect($array['uploaded_by'])->toBe($this->user->id); + expect($array['download_url'])->toBeString(); + expect($array['created_at'])->toBeString(); +}); + +test('resource transforms collection of attachments correctly', function () { + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Test Secret', + ]); + + $attachment1 = createTestAttachment([ + 'tenant_id' => $this->tenant->id, + 'secret_id' => $secret->id, + 'uploaded_by' => $this->user->id, + 'filename_plain' => 'file1.pdf', + 'file_size' => 1024, + 'mime_type' => 'application/pdf', + 'storage_path' => 'test-storage-path-1', + 'checksum_sha256' => hash('sha256', 'test-content-1'), + ]); + + $attachment2 = createTestAttachment([ + 'tenant_id' => $this->tenant->id, + 'secret_id' => $secret->id, + 'uploaded_by' => $this->user->id, + 'filename_plain' => 'file2.pdf', + 'file_size' => 2048, + 'mime_type' => 'application/pdf', + 'storage_path' => 'test-storage-path-2', + 'checksum_sha256' => hash('sha256', 'test-content-2'), + ]); + + $collection = SecretAttachmentResource::collection([$attachment1, $attachment2]); + $array = $collection->toArray(request()); + + expect($array)->toHaveCount(2); + expect($array[0]['filename'])->toBe('file1.pdf'); + expect($array[1]['filename'])->toBe('file2.pdf'); +}); + +test('resource formats created_at as ISO8601 string', function () { + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Test Secret', + ]); + + $attachment = createTestAttachment([ + 'tenant_id' => $this->tenant->id, + 'secret_id' => $secret->id, + 'uploaded_by' => $this->user->id, + 'filename_plain' => 'test.pdf', + 'file_size' => 1024, + 'mime_type' => 'application/pdf', + 'storage_path' => 'test-storage-path', + 'checksum_sha256' => hash('sha256', 'test-content'), + ]); + + $resource = SecretAttachmentResource::make($attachment); + $array = $resource->toArray(request()); + + // ISO 8601 format: 2025-11-16T15:30:00+00:00 + expect($array['created_at'])->toMatch('/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[+-]\d{2}:\d{2}$/'); +}); + +test('resource decrypts filename_plain correctly', function () { + $secret = createTestSecret([ + 'tenant_id' => $this->tenant->id, + 'owner_id' => $this->user->id, + 'title_plain' => 'Test Secret', + ]); + + $attachment = createTestAttachment([ + 'tenant_id' => $this->tenant->id, + 'secret_id' => $secret->id, + 'uploaded_by' => $this->user->id, + 'filename_plain' => 'sensitive-document.pdf', + 'file_size' => 1024, + 'mime_type' => 'application/pdf', + 'storage_path' => 'test-storage-path', + 'checksum_sha256' => hash('sha256', 'test-content'), + ]); + + $resource = SecretAttachmentResource::make($attachment); + $array = $resource->toArray(request()); + + // Verify decryption works (filename should be decrypted) + expect($array['filename'])->toBe('sensitive-document.pdf'); +}); diff --git a/tests/Pest.php b/tests/Pest.php index 617ab31..72ffe19 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -97,3 +97,38 @@ function assignTemporalRole( // Clear relationship cache $user->unsetRelation('roles'); } + +/** + * Create Secret with proper encryption pattern. + * Sets tenant_id BEFORE title_plain to ensure correct encryption context. + */ +function createTestSecret(array $attributes): \App\Models\Secret +{ + $secret = new \App\Models\Secret; + $secret->tenant_id = $attributes['tenant_id']; + $secret->owner_id = $attributes['owner_id']; + $secret->title_plain = $attributes['title_plain'] ?? 'Test Secret'; + $secret->save(); + + return $secret; +} + +/** + * Create SecretAttachment with proper encryption pattern. + * Sets tenant_id BEFORE filename_plain to ensure correct encryption context. + */ +function createTestAttachment(array $attributes): \App\Models\SecretAttachment +{ + $attachment = new \App\Models\SecretAttachment; + $attachment->secret_id = $attributes['secret_id']; + $attachment->tenant_id = $attributes['tenant_id']; + $attachment->filename_plain = $attributes['filename_plain']; + $attachment->file_size = $attributes['file_size']; + $attachment->mime_type = $attributes['mime_type']; + $attachment->storage_path = $attributes['storage_path']; + $attachment->checksum_sha256 = $attributes['checksum_sha256']; + $attachment->uploaded_by = $attributes['uploaded_by']; + $attachment->save(); + + return $attachment; +} From 7c09df98633c6fa4b8895731ae31a153a850b2ab Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 16:58:50 +0100 Subject: [PATCH 27/28] style: fix phpdoc_separation in SecretAttachmentResource Add missing blank line after @property annotation to comply with PSR-12. --- app/Http/Resources/SecretAttachmentResource.php | 1 + 1 file changed, 1 insertion(+) diff --git a/app/Http/Resources/SecretAttachmentResource.php b/app/Http/Resources/SecretAttachmentResource.php index 5bf8439..aeb0b8b 100644 --- a/app/Http/Resources/SecretAttachmentResource.php +++ b/app/Http/Resources/SecretAttachmentResource.php @@ -14,6 +14,7 @@ * Transforms attachment metadata for JSON responses. * * @property \App\Models\SecretAttachment $resource + * * @mixin \App\Models\SecretAttachment */ class SecretAttachmentResource extends JsonResource From 60cc5257e2b12b55a53cb6a7fe86f28e7f1f9956 Mon Sep 17 00:00:00 2001 From: Holger Schmermbeck Date: Sun, 16 Nov 2025 17:05:49 +0100 Subject: [PATCH 28/28] fix: address Copilot review comments - Use mimetypes validation instead of mimes for MIME type validation - Add explicit null check for filename in download method - Cast file_size to string for Content-Length header Resolves 3 Copilot review comments in PR #181. --- .../Controllers/Api/V1/SecretAttachmentController.php | 9 +++++++-- app/Http/Requests/StoreSecretAttachmentRequest.php | 7 ++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/app/Http/Controllers/Api/V1/SecretAttachmentController.php b/app/Http/Controllers/Api/V1/SecretAttachmentController.php index 2d8b205..7d98fe7 100644 --- a/app/Http/Controllers/Api/V1/SecretAttachmentController.php +++ b/app/Http/Controllers/Api/V1/SecretAttachmentController.php @@ -78,13 +78,18 @@ public function download(SecretAttachment $attachment): Response $content = $this->storageService->retrieve($attachment); $filename = $attachment->filename_plain; + // Ensure filename is present; attachments should always have a filename. + if ($filename === null) { + abort(500, 'Attachment is missing a filename.'); + } + // Escape filename for Content-Disposition header (RFC 2231/5987) - $safeFilename = str_replace(['"', '\\'], ['', ''], $filename ?? 'download'); + $safeFilename = str_replace(['"', '\\'], ['', ''], $filename); return response($content, 200, [ 'Content-Type' => $attachment->mime_type, 'Content-Disposition' => 'attachment; filename="'.$safeFilename.'"', - 'Content-Length' => $attachment->file_size, + 'Content-Length' => (string) $attachment->file_size, ]); } diff --git a/app/Http/Requests/StoreSecretAttachmentRequest.php b/app/Http/Requests/StoreSecretAttachmentRequest.php index 4e03f19..3c1df6e 100644 --- a/app/Http/Requests/StoreSecretAttachmentRequest.php +++ b/app/Http/Requests/StoreSecretAttachmentRequest.php @@ -41,10 +41,7 @@ public function rules(): array 'required', 'file', 'max:'.($maxSize / 1024), // Convert bytes to KB for Laravel validation - 'mimes:'.implode(',', array_map( - fn (string $mime): string => str_replace('application/', '', $mime), - $allowedMimes - )), + 'mimetypes:'.implode(',', $allowedMimes), ], ]; } @@ -64,7 +61,7 @@ public function messages(): array 'file.required' => 'Please select a file to upload.', 'file.file' => 'The uploaded file is not valid.', 'file.max' => 'The file size must not exceed '.$maxSize.' MB.', - 'file.mimes' => 'The file type is not allowed. Only certain file types are permitted.', + 'file.mimetypes' => 'The file type is not allowed. Only certain file types are permitted.', ]; } }