Conversation
…basic file type check
|
|
|
Will run through E2E testing tomorrow to confirm everything is working as intended |
PR Summary
Overall, this PR focuses on enhancing the messaging capabilities of our service, enabling users to send multimedia messages, along with maintaining the optimum performance by controlling the size of attachments. |
|
Tested and working! 
MMS attachments can be added via URLs (up to 10) as long as the attachments do not exceed 1.5MB due to carrier limits. |
Greptile SummaryThis PR adds MMS (Multimedia Messaging Service) support to the httpSMS application, enabling users to send messages with attachments like images and videos. Key Changes:
Issues Found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Web/Discord
participant API
participant Validator
participant Android
participant AttachmentServer
participant MMS
User->>Web/Discord: Send message with attachment URLs
Web/Discord->>API: POST /v1/messages/send
API->>Validator: Validate attachment URLs
Validator->>AttachmentServer: HEAD request (check size)
AttachmentServer-->>Validator: Content-Length header
Validator->>Validator: Cache result (24h TTL)
Validator-->>API: Validation result
API->>API: Store message with attachments
API->>Android: Push notification via FCM
Android->>AttachmentServer: Download attachments
AttachmentServer-->>Android: File data
Android->>Android: Build MMS PDU with attachments
Android->>Android: Write PDU to cache file
Android->>MMS: sendMultimediaMessage(pduUri)
MMS-->>Android: Broadcast (success/failure)
Android->>Android: Cleanup PDU file
Android->>API: Report status
Last reviewed commit: 47dde30 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return null | ||
| } | ||
|
|
||
| val maxSizeBytes = 1.5 * 1024 * 1024 // most (modern?) carriers have a 2MB limit, so targetting 1.5MB should be safe |
There was a problem hiding this comment.
Magic number duplicated in api/pkg/validators/validator.go:203. Consider defining as a constant in a shared location.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Pull request overview
Adds end-to-end MMS support by introducing “attachments” across the API, web UI, bulk upload templates, and Android client message sending pipeline (including MMS PDU composition and dispatch).
Changes:
- Extend message send/bulk send APIs (and Discord slash command) to accept attachment URLs with content types, and persist them on
entities.Message. - Update bulk message CSV/XLSX templates + bulk-file parsing/validation to support
AttachmentURLs(optional). - Add web UI fields for composing MMS and displaying message attachments; add Android MMS composition/sending + attachment download and cache cleanup.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| web/static/templates/httpsms-bulk.xlsx | Updates Excel bulk-send template to include optional attachment URLs. |
| web/static/templates/httpsms-bulk.csv | Updates CSV bulk-send template to include optional send time + attachment URLs. |
| web/pages/threads/_id/index.vue | Renders per-message attachment gallery in thread view. |
| web/pages/messages/index.vue | Adds attachment URL input and includes attachments in send payload. |
| web/models/message.ts | Adds attachments to the web Message model. |
| web/components/MessageThread.vue | Minor UI/asset changes in thread list component (icon import). |
| api/pkg/validators/validator.go | Adds URL reachability/size validation with caching for attachments. |
| api/pkg/validators/message_handler_validator.go | Validates attachments on single + bulk send endpoints. |
| api/pkg/validators/bulk_message_handler_validator.go | Parses/validates attachment URLs from bulk CSV/XLSX uploads. |
| api/pkg/services/message_service.go | Propagates attachments through send flow and persistence. |
| api/pkg/services/discord_service.go | Adds optional attachment_urls option to Discord slash command. |
| api/pkg/requests/message_send_request.go | Adds attachments field to send request and params conversion. |
| api/pkg/requests/message_bulk_send_request.go | Adds attachments field to bulk-send request and params conversion. |
| api/pkg/requests/bulk_message_request.go | Adds AttachmentURLs CSV column and maps to attachments on send. |
| api/pkg/handlers/discord_handler.go | Parses attachment_urls and displays attachment URLs in Discord response embed. |
| api/pkg/events/message_api_sent_event.go | Includes attachments in the emitted “message.api.sent” event payload. |
| api/pkg/entities/message.go | Adds persisted Attachments JSON field + content-type helper. |
| api/pkg/di/container.go | Wires app cache into validators for attachment URL validation caching. |
| android/app/src/main/res/xml/file_paths.xml | Adds FileProvider cache path for MMS PDU/attachments. |
| android/app/src/main/java/com/httpsms/SmsManagerService.kt | Adds sendMultimediaMessage wrapper. |
| android/app/src/main/java/com/httpsms/SentReceiver.kt | Cleans up cached MMS PDU file on send completion. |
| android/app/src/main/java/com/httpsms/Models.kt | Adds attachment model + exposes message attachments in API model. |
| android/app/src/main/java/com/httpsms/HttpSmsApiService.kt | Downloads attachments with a size limit into cache for MMS composition. |
| android/app/src/main/java/com/httpsms/FirebaseMessagingService.kt | Detects MMS vs SMS; composes MMS PDU with downloaded media and dispatches via SmsManager. |
| android/app/src/main/AndroidManifest.xml | Registers FileProvider for sharing MMS PDU to platform MMS service. |
| android/app/build.gradle | Adds SMS/MMS library dependency used for PDU composition. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if resp.StatusCode < 200 || resp.StatusCode >= 400 { | ||
| errMsg := fmt.Sprintf("url returned an error status code: %d", resp.StatusCode) | ||
| saveToCache(ctx, c, cacheKey, errMsg) | ||
| return fmt.Errorf(errMsg) | ||
| } | ||
|
|
||
| const maxSizeBytes = 1.5 * 1024 * 1024 | ||
|
|
||
| if resp.ContentLength > int64(maxSizeBytes) { | ||
| errMsg := fmt.Sprintf("file size (%.2f MB) exceeds the 1.5 MB carrier limit", float64(resp.ContentLength)/(1024*1024)) | ||
| saveToCache(ctx, c, cacheKey, errMsg) | ||
| return fmt.Errorf(errMsg) | ||
| } | ||
|
|
||
| saveToCache(ctx, c, cacheKey, "valid") |
There was a problem hiding this comment.
The size validation relies on resp.ContentLength from a HEAD response. For many servers this is -1/unknown (or HEAD is unsupported), which will incorrectly treat potentially huge attachments as valid. Consider treating unknown length as invalid or falling back to a bounded GET/Range request so the 1.5MB limit is actually enforced.
| } else { | ||
| if err := validateAttachmentURL(ctx, v.cache, cleanURL); err != nil { | ||
| result.Add("attachments", fmt.Sprintf("Row [%d]: The attachment URL [%s] failed validation: %s", index+2, cleanURL, err.Error())) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
validateAttachmentURL performs a network call per attachment during bulk file validation. With up to 1000 rows and 10 attachments each, this can trigger thousands of outbound requests and make the upload endpoint extremely slow/unreliable (or easy to DoS). Consider removing live URL reachability checks from bulk validation or validating asynchronously/with strict concurrency limits.
| } else { | |
| if err := validateAttachmentURL(ctx, v.cache, cleanURL); err != nil { | |
| result.Add("attachments", fmt.Sprintf("Row [%d]: The attachment URL [%s] failed validation: %s", index+2, cleanURL, err.Error())) | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } | |
| } |
| v-if="message.attachments" | ||
| shaped | ||
| > |
There was a problem hiding this comment.
v-if="message.attachments" will render the attachments card even when attachments is an empty array (since [] is truthy). Consider checking message.attachments?.length instead so the section only appears when there are actual attachments.
| v-if="message.attachments" | |
| shaped | |
| > | |
| v-if="message.attachments?.length" | |
| shaped | |
| > |
| val response = client.newCall(request).execute() | ||
| if (!response.isSuccessful) { | ||
| Timber.e("Failed to download attachment: ${response.code}") | ||
| response.close() | ||
| return null | ||
| } | ||
|
|
||
| val maxSizeBytes = 1.5 * 1024 * 1024 // most (modern?) carriers have a 2MB limit, so targetting 1.5MB should be safe | ||
| val contentLength = response.body?.contentLength() ?: -1L | ||
| if (contentLength > maxSizeBytes) { | ||
| Timber.e("Attachment is too large ($contentLength bytes).") | ||
| response.close() | ||
| return null | ||
| } | ||
|
|
||
| val mmsDir = File(context.cacheDir, "mms_attachments") | ||
| if (!mmsDir.exists()) { | ||
| mmsDir.mkdirs() | ||
| } | ||
|
|
||
| val tempFile = File(mmsDir, "mms_${messageId}_$attachmentIndex") | ||
| val inputStream = response.body?.byteStream() | ||
| val outputStream = FileOutputStream(tempFile) | ||
|
|
||
| inputStream?.copyToWithLimit(outputStream, maxSizeBytes.toLong()) | ||
|
|
||
| outputStream.close() | ||
| inputStream?.close() | ||
| response.close() | ||
|
|
||
| return tempFile |
There was a problem hiding this comment.
response.body can be null; in that case inputStream?.copyToWithLimit(...) is a no-op and the method returns an empty temp file as “success”. Also, if copyToWithLimit throws, outputStream/response may not be closed. Treat null bodies as an error and use use {} blocks (or try/finally) to reliably close streams and the response.
| val response = client.newCall(request).execute() | |
| if (!response.isSuccessful) { | |
| Timber.e("Failed to download attachment: ${response.code}") | |
| response.close() | |
| return null | |
| } | |
| val maxSizeBytes = 1.5 * 1024 * 1024 // most (modern?) carriers have a 2MB limit, so targetting 1.5MB should be safe | |
| val contentLength = response.body?.contentLength() ?: -1L | |
| if (contentLength > maxSizeBytes) { | |
| Timber.e("Attachment is too large ($contentLength bytes).") | |
| response.close() | |
| return null | |
| } | |
| val mmsDir = File(context.cacheDir, "mms_attachments") | |
| if (!mmsDir.exists()) { | |
| mmsDir.mkdirs() | |
| } | |
| val tempFile = File(mmsDir, "mms_${messageId}_$attachmentIndex") | |
| val inputStream = response.body?.byteStream() | |
| val outputStream = FileOutputStream(tempFile) | |
| inputStream?.copyToWithLimit(outputStream, maxSizeBytes.toLong()) | |
| outputStream.close() | |
| inputStream?.close() | |
| response.close() | |
| return tempFile | |
| client.newCall(request).execute().use { response -> | |
| if (!response.isSuccessful) { | |
| Timber.e("Failed to download attachment: ${response.code}") | |
| return null | |
| } | |
| val body = response.body | |
| if (body == null) { | |
| Timber.e("Failed to download attachment: response body is null") | |
| return null | |
| } | |
| val maxSizeBytes = 1.5 * 1024 * 1024 // most (modern?) carriers have a 2MB limit, so targetting 1.5MB should be safe | |
| val contentLength = body.contentLength() | |
| if (contentLength > maxSizeBytes) { | |
| Timber.e("Attachment is too large ($contentLength bytes).") | |
| return null | |
| } | |
| val mmsDir = File(context.cacheDir, "mms_attachments") | |
| if (!mmsDir.exists()) { | |
| mmsDir.mkdirs() | |
| } | |
| val tempFile = File(mmsDir, "mms_${messageId}_$attachmentIndex") | |
| val inputStream = body.byteStream() | |
| FileOutputStream(tempFile).use { outputStream -> | |
| inputStream.use { input -> | |
| input.copyToWithLimit(outputStream, maxSizeBytes.toLong()) | |
| } | |
| } | |
| return tempFile | |
| } |
| } finally { | ||
| downloadedFiles.forEach { file -> | ||
| if (file.exists()) { | ||
| file.delete() | ||
| } | ||
| } |
There was a problem hiding this comment.
The MMS PDU file (pdu_${message.id}.dat) is only deleted in SentReceiver. If sendMultimediaMessage fails before the sent broadcast is delivered, the PDU can be left behind in cache. Consider deleting the PDU file in this method’s finally block as well (after dispatch / on failure) to avoid cache buildup.
| } finally { | |
| downloadedFiles.forEach { file -> | |
| if (file.exists()) { | |
| file.delete() | |
| } | |
| } | |
| } finally { | |
| // Clean up any downloaded temporary files | |
| downloadedFiles.forEach { file -> | |
| if (file.exists()) { | |
| file.delete() | |
| } | |
| } | |
| // Also clean up the MMS PDU file to avoid cache buildup in cases where | |
| // sendMultimediaMessage fails before the sent broadcast is delivered. | |
| try { | |
| val pduFile = java.io.File(applicationContext.cacheDir, "pdu_${message.id}.dat") | |
| if (pduFile.exists()) { | |
| val deleted = pduFile.delete() | |
| if (!deleted) { | |
| Timber.w("Failed to delete MMS PDU file for message ID [${message.id}] at [${pduFile.absolutePath}]") | |
| } | |
| } | |
| } catch (cleanupException: Exception) { | |
| // Best-effort cleanup; log but do not change the original result. | |
| Timber.w(cleanupException, "Error while cleaning up MMS PDU file for message ID [${message.id}]") | |
| } |
| if message.AttachmentURLs != "" { | ||
| urls := strings.Split(message.AttachmentURLs, ",") | ||
|
|
||
| if len(urls) > 10 { |
There was a problem hiding this comment.
len(strings.Split(message.AttachmentURLs, ",")) counts empty entries (e.g. trailing commas), which can incorrectly reject messages as having ">10" attachments even when fewer valid URLs are present. Count only non-empty trimmed URLs before enforcing the limit.
| if len(urls) > 10 { | |
| validAttachmentCount := 0 | |
| for _, u := range urls { | |
| if strings.TrimSpace(u) != "" { | |
| validAttachmentCount++ | |
| } | |
| } | |
| if validAttachmentCount > 10 { |
| import { | ||
| mdiPlus, | ||
| mdiDownload, | ||
| mdiCheckAll, | ||
| mdiCheck, | ||
| mdiAlert, | ||
| mdiAccount, | ||
| mdiPaperclip, | ||
| } from '@mdi/js' | ||
|
|
||
| @Component | ||
| export default class MessageThread extends Vue { | ||
| mdiPlus = mdiPlus | ||
| mdiDownload = mdiDownload | ||
| mdiAccount = mdiAccount | ||
| mdiAlert = mdiAlert | ||
| mdiCheck = mdiCheck | ||
| mdiCheckAll = mdiCheckAll | ||
| mdiPaperclip = mdiPaperclip |
There was a problem hiding this comment.
mdiPaperclip is imported and assigned on the component instance but never used in the template/script. If this isn’t needed, remove it to avoid unused-symbol warnings (or add the intended attachment indicator to the UI).
| val maxSizeBytes = 1.5 * 1024 * 1024 // most (modern?) carriers have a 2MB limit, so targetting 1.5MB should be safe | ||
| val contentLength = response.body?.contentLength() ?: -1L | ||
| if (contentLength > maxSizeBytes) { | ||
| Timber.e("Attachment is too large ($contentLength bytes).") | ||
| response.close() | ||
| return null | ||
| } |
There was a problem hiding this comment.
maxSizeBytes is a Double, but contentLength is a Long; if (contentLength > maxSizeBytes) won’t compile in Kotlin without an explicit conversion. Define maxSizeBytes as a Long (or convert contentLength to Double) so the comparison is type-correct.
| func validateAttachmentURL(ctx context.Context, c cache.Cache, attachmentURL string) error { | ||
| cacheKey := "mms-url-validation:" + attachmentURL | ||
|
|
||
| if cachedVal, err := c.Get(ctx, cacheKey); err == nil { | ||
| if cachedVal == "valid" { | ||
| return nil | ||
| } | ||
| return fmt.Errorf(cachedVal) | ||
| } | ||
|
|
||
| client := &http.Client{ | ||
| Timeout: 5 * time.Second, | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodHead, attachmentURL, nil) | ||
| if err != nil { | ||
| errMsg := fmt.Sprintf("invalid url format") | ||
| saveToCache(ctx, c, cacheKey, errMsg) | ||
| return fmt.Errorf(errMsg) | ||
| } | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| errMsg := fmt.Sprintf("could not reach the url") | ||
| saveToCache(ctx, c, cacheKey, errMsg) | ||
| return fmt.Errorf(errMsg) |
There was a problem hiding this comment.
validateAttachmentURL makes a server-side HTTP request to a user-supplied URL. As-is this enables SSRF (including redirects/DNS rebinding) to internal/metadata endpoints. Consider blocking private/loopback/link-local ranges after DNS resolution and disabling/fencing redirects (or validating final destination) before performing any request.
| client := &http.Client{ | ||
| Timeout: 5 * time.Second, | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodHead, attachmentURL, nil) | ||
| if err != nil { | ||
| errMsg := fmt.Sprintf("invalid url format") | ||
| saveToCache(ctx, c, cacheKey, errMsg) | ||
| return fmt.Errorf(errMsg) | ||
| } | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { |
There was a problem hiding this comment.
validateAttachmentURL creates a new http.Client and does a blocking request per attachment; with up to 10 attachments this can add ~50s to a single API call (and much more in bulk upload). Consider reusing a shared client and performing the request with http.NewRequestWithContext(ctx, ...) so cancellations/timeouts propagate.
Implementation for: #262