AttachmentDownloadService #379
Open
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds attachment download support: new DownloadableAttachment DTO, AttachmentDownloadService with UID and path/file validation, AttachmentFileNotFoundException, Attachment::FORWARD constant and updated AttachmentAdder URL format, tests, and composer dependency change. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as AttachmentDownloadService
participant Repo as SubscriberRepository
participant FS as Filesystem
participant Utils as GuzzleUtils
participant DTO as DownloadableAttachment
Client->>Service: getDownloadable(attachment, uid)
Service->>Repo: validateUid(uid)
alt uid == FORWARD or subscriber exists
Repo-->>Service: OK
Service->>Service: sanitize filename (basename)
Service->>FS: resolve base repo path & realpath
FS-->>Service: realpath result
Service->>FS: check file exists & readable
alt file missing/unreadable
FS-->>Service: error
Service-->>Client: throw AttachmentFileNotFoundException
else file ok
Service->>Service: determine mimeType (provided or guess)
Service->>FS: stat file (size) and open handle
Service->>Utils: create StreamInterface from handle
Utils-->>Service: StreamInterface
Service->>DTO: new DownloadableAttachment(filename, mimeType, size, stream)
Service-->>Client: return DownloadableAttachment
end
else
Repo-->>Service: not found
Service-->>Client: throw MessageNotReceivedException
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/Model/Dto/DownloadableAttachment.php`:
- Line 14: The DTO declares public readonly int $size but
AttachmentDownloadService may pass null (filesize() can return false); update
the DownloadableAttachment definition to use public readonly ?int $size so it
accepts null, and adjust the constructor/signature and any callers (e.g.,
wherever new DownloadableAttachment(...) is invoked) to handle nullable size;
alternatively, if you prefer non-null sizes, coalesce the service value to 0
when filesize() returns false in AttachmentDownloadService instead of passing
null.
In `@src/Domain/Messaging/Service/AttachmentAdder.php`:
- Line 61: The constructed download URL in AttachmentAdder (variable $viewUrl)
concatenates $hash directly into the uid query parameter which can corrupt
values like emails containing '+'; update the URL construction in the
AttachmentAdder.php code that builds $viewUrl (uses $this->attachmentDownloadUrl
and $att->getId()) to URL-encode the uid value (e.g., use urlencode on $hash) so
the uid query parameter is safely transmitted.
In `@src/Domain/Messaging/Service/AttachmentDownloadService.php`:
- Around line 40-41: The filesize() call can return false, but
DownloadableAttachment::$size is typed int, so coalesce or convert the result
before assigning to the DTO; in AttachmentDownloadService replace the current
$size assignment (the $size = filesize($filePath); $size = $size === false ?
null : $size;) with a safe int value (e.g. coalesce to 0 or cast to int) so
DownloadableAttachment::$size always receives an int, or alternatively change
DownloadableAttachment::$size to ?int if nulls are intended.
- Around line 29-34: The AttachmentDownloadService currently concatenates
$filename (from $attachment->getFilename()) onto $this->attachmentRepositoryPath
allowing path traversal; fix by resolving and validating the target path before
allowing access: compute the repository base path from
$this->attachmentRepositoryPath, sanitize or canonicalize the user filename
(e.g., use basename or join then run realpath on the combined path), verify the
resolved path starts with the repository base (preventing escapes like ../../),
and only then call is_file/is_readable and return or throw
AttachmentFileNotFoundException if validation fails; update the code around the
$path/$filePath construction in AttachmentDownloadService to implement these
checks.
- Line 7: The project imports GuzzleHttp\Psr7\Utils in AttachmentDownloadService
but guzzlehttp/guzzle is only listed in composer.json require-dev; move the
guzzlehttp/guzzle package from "require-dev" to "require" in composer.json so it
is installed in production, then run composer update (or composer install) to
ensure the dependency is available at runtime for the AttachmentDownloadService
class that uses Utils.
🧹 Nitpick comments (2)
src/Domain/Messaging/Exception/AttachmentFileNotFoundException.php (1)
9-12: Consider accepting context for easier debugging.The hardcoded message with no parameters makes it harder to trace which attachment failed. Accepting an optional attachment ID or filename would help with troubleshooting in production.
Suggested improvement
- public function __construct() + public function __construct(string $detail = '') { - parent::__construct('Attachment file not available'); + $message = 'Attachment file not available'; + if ($detail !== '') { + $message .= ': ' . $detail; + } + parent::__construct($message); }src/Domain/Messaging/Service/AttachmentDownloadService.php (1)
22-52: No unit tests forAttachmentDownloadService.This is a new service with non-trivial logic (path resolution, MIME detection, stream creation, error handling). Consider adding tests covering at least: empty filename, missing file, successful download, and path traversal rejection.
#!/bin/bash # Check if any test file exists for AttachmentDownloadService rg -l "AttachmentDownloadService" --type=php -g '*Test*'
TatevikGr
force-pushed
the
feat/attachment
branch
from
039234a to
0838e27
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/Service/AttachmentDownloadService.php`:
- Around line 47-55: The code validates the path into $realPath but continues
using $filePath for downstream file ops; change all uses after validation to
$realPath so operations are on the canonical path—specifically use $realPath
when calling MimeTypes::getDefault()->guessMimeType(...), when calling
filesize(...), and when passing to Utils::tryFopen(...) / Utils::streamFor(...),
while keeping the $attachment->getMimeType() fallback logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Domain/Messaging/Service/AttachmentDownloadService.php`:
- Around line 51-59: validateUid currently only verifies the UID belongs to an
existing subscriber (using subscriberRepository->findOneByEmail) but throws
MessageNotReceivedException, so it must also verify the subscriber actually
received the specific campaign/attachment; update validateUid to, when $uid !==
Attachment::FORWARD, lookup the subscriber via
subscriberRepository->findOneByEmail and then check message/recipient delivery
(e.g. via a MessageRepository or Recipient/Delivery entity lookup) that the
subscriber is a recipient of the campaign/attachment being requested, and only
return success if that delivery record exists; if no delivery record is found,
throw MessageNotReceivedException; alternatively, if the broader acceptance of
any subscriber is intended, add a clear TODO comment in validateUid noting this
design decision and why it omits the delivery check.
- Around line 29-31: The call to basename() currently receives
$attachment->getFilename() which is nullable; update AttachmentDownloadService
(around where $original is set) to guard against null before calling basename —
e.g., retrieve $original = $attachment->getFilename() and if null either
throw/return early or use a safe default via null-coalescing prior to $filename
= basename($original); then pass the safe $original/$filename into
validateFilePath($filename, $original) so basename() never receives null.
b07f4d1 to
c3bcd4b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 by CodeRabbit
New Features
Bug Fixes
Tests
Thanks for contributing to phpList!