Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds attachment download support: new DownloadableAttachment DTO, AttachmentDownloadService that validates and reads files (with MIME detection), a new AttachmentFileNotFoundException, a URL format change for attachment links, updated tests, and composer dependency adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Service as AttachmentDownloadService
participant FS as Filesystem (repository)
participant Mime as MimeTypes/Guesser
participant Utils as Guzzle Utils
participant DTO as DownloadableAttachment
Client->>Service: request getDownloadable(Attachment)
Service->>Service: sanitize filename (basename) and resolve path
Service->>FS: check path safety, exists, readable
alt file missing or unsafe
FS-->>Service: error
Service-->>Client: throw AttachmentFileNotFoundException
else file ok
Service->>Mime: ask for mime type (provided or guess from path)
Mime-->>Service: mimeType
Service->>FS: stat file (size) and open stream
FS-->>Utils: create StreamInterface from file
Utils-->>Service: StreamInterface
Service->>DTO: instantiate DownloadableAttachment(filename, mimeType, size, stream)
Service-->>Client: return DownloadableAttachment
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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*'
| $hash = $forwarded ? 'forwarded' : $email->getTo()[0]->getAddress(); | ||
| // todo: add endpoint in rest-api project | ||
| $viewUrl = $this->attachmentDownloadUrl . '/?id=' . $att->getId() . '&uid=' . $hash; | ||
| $viewUrl = $this->attachmentDownloadUrl . '/' . $att->getId() . '/?uid=' . $hash; |
There was a problem hiding this comment.
Consider URL-encoding the uid query parameter value.
$hash can be an email address containing characters like + that have special meaning in query strings. Without urlencode(), a + would be interpreted as a space by the receiving end.
Suggested fix
- $viewUrl = $this->attachmentDownloadUrl . '/' . $att->getId() . '/?uid=' . $hash;
+ $viewUrl = $this->attachmentDownloadUrl . '/' . $att->getId() . '/?uid=' . urlencode($hash);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $viewUrl = $this->attachmentDownloadUrl . '/' . $att->getId() . '/?uid=' . $hash; | |
| $viewUrl = $this->attachmentDownloadUrl . '/' . $att->getId() . '/?uid=' . urlencode($hash); |
🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/AttachmentAdder.php` at 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.
| $size = filesize($filePath); | ||
| $size = $size === false ? null : $size; |
There was a problem hiding this comment.
$size can be null but DownloadableAttachment::$size is typed int.
This is the root cause of the type mismatch flagged on the DTO. Either coalesce here or fix the DTO type.
Quick fix: coalesce to 0
- $size = $size === false ? null : $size;
+ $size = $size === false ? 0 : $size;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $size = filesize($filePath); | |
| $size = $size === false ? null : $size; | |
| $size = filesize($filePath); | |
| $size = $size === false ? 0 : $size; |
🤖 Prompt for AI Agents
In `@src/Domain/Messaging/Service/AttachmentDownloadService.php` around lines 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.
TatevikGr
force-pushed
the
feat/attachment
branch
from
039234a to
0838e27
Compare
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Thanks for contributing to phpList!