Skip to content

Conversation

@kevalyq
Copy link
Contributor

@kevalyq kevalyq commented Nov 16, 2025

Summary

Implements backend for File Attachments API (#175) with encrypted storage and owner-based authorization.

✅ Status: Ready for Review

All 331 tests passing including 35 SecretAttachment tests. All Copilot review comments addressed (11 resolved, 6 deferred to Issue #180).

Features Implemented

API Endpoints (all working)

  • POST /v1/secrets/{secret}/attachments - Upload encrypted file
  • GET /v1/secrets/{secret}/attachments - List attachments
  • GET /v1/attachments/{attachment}/download - Download with decryption
  • DELETE /v1/attachments/{attachment} - Delete file

Security & Quality

✅ Tenant DEK encryption at rest
✅ Owner-based authorization (Policy)
✅ File size + MIME type validation
✅ Checksum integrity verification
✅ Content-Disposition header escaping (security)
✅ PHPStan Level Max compliant
✅ Laravel Pint compliant
✅ REUSE 3.3 compliant
✅ OpenAPI documentation
✅ CHANGELOG updated

Test Results (331 passing)

  • Controller: 13/13 passing ✅
  • Service: 3/3 passing ✅
  • Policy: 8/8 passing ✅
  • Model: 7/7 passing ✅
  • Migrations: 7/7 passing ✅

Copilot Review Comments Addressed

Resolved (11 threads)

  1. Storage disk hardcoding - Now uses config('attachments.storage_disk')
  2. Checksum verification - Added integrity check on file retrieval
  3. Content-Disposition escaping - Fixed header injection vulnerability
  4. Test assertion syntax - Fixed Storage::assertMissing() call
  5. Migration documentation - Enhanced tenant_id encryption documentation
  6. Unused test parameters - Removed $test parameter from helpers
  7. SPDX header format - Fixed consistency across files
  8. Config security warning - Added warning for empty MIME types array
  9. File existence check - Added graceful handling in delete() method
  10. N+1 query optimization - Optimized attachment_count accessor with withCount()
  11. Cleanup - Removed premature API Resources file

Deferred to Issue #180 (6 threads)

  • API Resources implementation (store, index methods)
  • Form Request validation refactoring
  • Named routes migration
  • Route helper usage in accessors
  • Property accessor pattern enforcement

Rationale: Following "One PR = One Topic" principle. Issue #180 will implement these refactorings in a separate PR with comprehensive tests (TDD).

Commits

  • c07384c: Critical security fixes (storage disk, checksum, header escaping, test syntax)
  • 58ee4c0: Nitpick fixes (docs, helpers, N+1 optimization, file check)

Related Issues

Closes #175
Related: #179 (Model tests encryption fix - merged)
Related: #180 (Best Practice refactoring - follow-up)

- 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)
- 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)
- 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
- 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
- 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
- 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
- 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
- Use 'SecPal Contributors' format
- Matches project standard across all files
- Use 'SecPal Contributors' format
- Matches project standard
- Added entry for Issue #175 (Phase 2)
- Documents all 4 endpoints and key features
- Notes encryption, authorization, and test coverage

Related: #175
- 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.
- 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.
- PHPStan now understands $validated is array<string, mixed>
- Resolves final offsetAccess.nonOffsetAccessible error
- 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
@kevalyq kevalyq added the large-pr-approved Legitimate large PR (e.g., boilerplate templates, auto-generated code) label Nov 16, 2025
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
@codecov
Copy link

codecov bot commented Nov 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@kevalyq kevalyq marked this pull request as ready for review November 16, 2025 14:22
Copilot AI review requested due to automatic review settings November 16, 2025 14:22
Copilot finished reviewing on behalf of kevalyq November 16, 2025 14:26
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a complete File Attachments API for the SecPal secrets management system with encrypted storage, authorization policies, and comprehensive test coverage. The implementation includes 4 REST endpoints for uploading, listing, downloading, and deleting file attachments with tenant-based DEK encryption at rest.

Key Features:

  • Backend API with encrypted file storage using tenant DEK
  • Owner-based authorization through policy classes
  • File size and MIME type validation
  • Full test suite (24 tests across controller, service, policy, model, and migration layers)

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 17 comments.

Show a summary per file
File Description
app/Http/Controllers/Api/V1/SecretAttachmentController.php New API controller implementing 4 endpoints for attachment management
app/Models/SecretAttachment.php New model with UUID primary key and encrypted filename field
app/Models/Secret.php Added attachments relationship and attachment count accessor
app/Policies/SecretAttachmentPolicy.php New policy enforcing owner-based access control
app/Services/AttachmentStorageService.php New service handling encryption, storage, and retrieval operations
app/Providers/AppServiceProvider.php Registered SecretAttachmentPolicy in Gate
config/attachments.php New configuration for file size limits and MIME type restrictions
database/migrations/2025_11_16_110234_create_secret_attachments_table.php New migration creating secret_attachments table with proper foreign keys
routes/api.php Added 4 new attachment API routes
tests/Feature/Controllers/SecretAttachmentControllerTest.php 13 controller tests covering all endpoints and authorization
tests/Feature/Services/AttachmentStorageServiceTest.php 3 service tests for encryption and storage operations
tests/Feature/Policies/SecretAttachmentPolicyTest.php 8 policy tests verifying access control
tests/Feature/Models/SecretAttachmentTest.php 7 model tests for encryption, relationships, and accessors
tests/Feature/SecretTest.php 2 tests for new Secret model relationship
tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php 7 migration tests verifying table structure
CHANGELOG.md Documentation of new features

…, 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
- 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
kevalyq added a commit that referenced this pull request Nov 16, 2025
- 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
kevalyq added a commit that referenced this pull request Nov 16, 2025
…#181)

* 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)

* 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)

* 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

* 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

* 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

* 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

* 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

* fix: update AttachmentStorageService SPDX header

- Use 'SecPal Contributors' format
- Matches project standard across all files

* fix: update SecretAttachmentPolicy SPDX header

- Use 'SecPal Contributors' format
- Matches project standard

* 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

* style: format CHANGELOG.md with Prettier

* 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.

* 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.

* fix: add type annotation for validated array in Controller

- PHPStan now understands $validated is array<string, mixed>
- Resolves final offsetAccess.nonOffsetAccessible error

* 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

* style: fix Laravel Pint formatting issues

* 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

* fix: correct download_url accessor to include /api prefix

* fix: use /v1/ prefix in download_url accessor (not /api/v1/)

* fix: add Attribute import and PHPStan type annotation for downloadUrl

* fix: correct test expectation to use /v1/ instead of /api/v1/

* fix: remove duplicate old-style download_url accessor

* 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

* 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

* style: fix Pint blank_line_before_statement in Secret.php

* 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

* style: fix phpdoc_separation in SecretAttachmentResource

Add missing blank line after @Property annotation to comply with PSR-12.

* 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.
Resolved conflicts by accepting refactored versions from PR #181:
- SecretAttachmentController now uses API Resources + Form Requests
- SecretAttachmentTest now uses consolidated Pest helpers
@kevalyq kevalyq requested a review from Copilot November 16, 2025 16:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

@kevalyq kevalyq merged commit accfdb7 into main Nov 16, 2025
17 checks passed
@kevalyq kevalyq deleted the feat/issue-175-file-attachments branch November 16, 2025 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

large-pr-approved Legitimate large PR (e.g., boilerplate templates, auto-generated code)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phase 2: File Attachments API (Upload/Download/Encryption)

2 participants