refactor: API Resources and Form Requests for SecretAttachment (#180) #181
+1,760
−0
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 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
- 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
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
…, 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
- 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
💡 Tip: Consider Using Draft PRsBenefits of opening PRs as drafts initially:
How to convert:
This is just a friendly reminder - feel free to continue as is! 😊 |
7 tasks
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Copilot finished reviewing on behalf of
kevalyq
November 16, 2025 15:55
kevalyq
added
the
large-pr-approved
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR successfully refactors the SecretAttachment API to use Laravel best practices by introducing API Resources and Form Requests. The implementation includes comprehensive test coverage, proper encryption handling, and follows the project's architecture decisions (contract-first, no named routes).
Key Changes
- Created
SecretAttachmentResourcefor consistent JSON transformation with ISO 8601 date formatting - Created
StoreSecretAttachmentRequestcentralizing upload validation rules - Refactored controller to use Form Request + API Resource pattern
- Added helper functions to
tests/Pest.phpfor parallel test execution
Reviewed Changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Pest.php | Added createTestSecret() and createTestAttachment() helper functions to prevent parallel test execution conflicts |
| tests/Feature/Services/AttachmentStorageServiceTest.php | Tests for encryption, decryption, and storage service operations |
| tests/Feature/SecretTest.php | Tests for Secret model attachments relationship and count accessor |
| tests/Feature/Resources/SecretAttachmentResourceTest.php | Tests for API Resource transformation, collection handling, ISO8601 formatting, and decryption |
| tests/Feature/Requests/StoreSecretAttachmentRequestTest.php | Tests for Form Request validation rules including file requirements, size limits, and MIME types |
| tests/Feature/Policies/SecretAttachmentPolicyTest.php | Tests for owner-based authorization policies (viewAny, view, create, delete) |
| tests/Feature/Models/SecretAttachmentTest.php | Tests for model structure, UUID keys, encryption, relationships, and accessors |
| tests/Feature/Migrations/CreateSecretAttachmentsTableTest.php | Tests for migration schema validation including columns, types, indexes, and foreign keys |
| tests/Feature/Controllers/SecretAttachmentControllerTest.php | Integration tests for all controller endpoints (upload, list, download, delete) with authorization |
| routes/api.php | Added four SecretAttachment endpoints under v1 API group |
| database/migrations/2025_11_16_110234_create_secret_attachments_table.php | Migration creating secret_attachments table with UUID, foreign keys, and encrypted fields |
| config/attachments.php | Configuration file defining max file size, allowed MIME types, and storage disk |
| app/Services/AttachmentStorageService.php | Service handling file encryption, storage, retrieval, and deletion with checksum verification |
| app/Providers/AppServiceProvider.php | Registered SecretAttachmentPolicy with Gate |
| app/Policies/SecretAttachmentPolicy.php | Authorization policy restricting access to secret owners only |
| app/Models/SecretAttachment.php | Model with EncryptedWithDek cast for filename, UUID keys, relationships, and download_url accessor |
| app/Models/Secret.php | Added attachments relationship and attachment_count accessor to Secret model |
| app/Http/Resources/SecretAttachmentResource.php | API Resource transforming attachment metadata with decrypted filename and ISO8601 dates |
| app/Http/Requests/StoreSecretAttachmentRequest.php | Form Request with validation rules for file upload (size, MIME types from config) |
| app/Http/Controllers/Api/V1/SecretAttachmentController.php | Controller using Form Request, API Resource, Policy, and Service for all attachment operations |
| CHANGELOG.md | Documents File Attachments API Phase 2 features and test coverage |
kevalyq
commented
Nov 16, 2025
Contributor
Author
kevalyq
left a comment
kevalyq
left a comment
There was a problem hiding this comment.
Copilot Comments Addressed:
✅ Comment 1 (mimetypes): Changed from mimes to mimetypes validation rule to properly accept full MIME types directly.
✅ Comment 2 (filename null check): Added explicit null check that aborts with 500 error if filename is missing.
✅ Comment 3 (Content-Length type): Cast file_size to string with (string) for Content-Length header.
All fixes committed in 60cc525.
kevalyq
added a commit
that referenced
this pull request
Nov 16, 2025
Resolved conflicts by accepting refactored versions from PR #181: - SecretAttachmentController now uses API Resources + Form Requests - SecretAttachmentTest now uses consolidated Pest helpers
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Refactors SecretAttachment API to use Laravel best practices: API Resources and Form Requests.
Changes
API Resource
SecretAttachmentResourcefor consistent JSON transformationtoIso8601String()filename_plainvia accessorstore()andindex()endpoints@mixinand explicit type castingForm Request
StoreSecretAttachmentRequestfor upload validationController Refactoring
store(): Uses Form Request + API Resourceindex(): Uses API Resource collection with proper JsonResponse wrappingdownload(): Uses property accessor (filename_plainnot getter method)Architecture Decision: NO Named Routes
url()helper, notroute()/contracts/docs/openapi.yamlTest Coverage
Helper Function Consolidation
createTestSecret()andcreateTestAttachment()totests/Pest.phpgetTestKekPath()helperCopilot Review Comments Addressed
From PR #177:
Quality Gates
All checks passing:
Related Issues
Closes #180
Related: #175 (File Attachments API - Phase 2)
Related: #177 (File Attachments API implementation)