feat: add MMS attachment upload/download support#854
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Show only /{index}/{filename} instead of full URL for received attachments
- Fix 'succesfully' typo in message_service.go
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add AttachmentStorage selection (GCS vs memory) based on GCS_BUCKET_NAME env var - Wire AttachmentHandler for public download endpoint - Pass storage and API base URL to MessageService - Add GCS_BUCKET_NAME to .env.docker Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Use deterministic reverse map for ContentTypeFromExtension (I-2)
- Initialize attachmentURLs to []string{} to avoid null in JSON (I-3)
- Distinguish 404 vs 500 in download handler with ErrAttachmentNotFound (I-4)
- Remove unused stacktrace import from memory storage
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Greptile SummaryThis PR adds MMS attachment support: incoming Android MMS attachments are validated, base64-decoded, uploaded to GCS (or in-memory for dev), and download URLs are stored on the message entity and included in webhook payloads. The overall design is solid — parallel upload with errgroup, cleanup-on-failure, and a public unauthenticated download endpoint — but two correctness issues need addressing before production use.
Confidence Score: 4/5Two P1 bugs must be fixed before this is safe to merge: GCS downloads return 500 for missing objects, and MMS-only (no-text) messages are rejected by the validator. Both P1 findings affect the primary user path in production: the GCS error mapping breaks the download endpoint for real missing-file 404s, and the content validation breaks ingest for attachment-only MMS. These are straightforward to fix but need to land before merge. api/pkg/repositories/gcs_attachment_storage.go (GCS 404 mapping) and api/pkg/validators/message_handler_validator.go (content required with attachments) Important Files Changed
Sequence DiagramsequenceDiagram
participant Android as Android App
participant API as POST /v1/messages/receive
participant Validator
participant MsgService as MessageService
participant Storage as AttachmentStorage (GCS/Memory)
participant DB as MessageRepository
participant Webhook as Event Dispatcher
Android->>API: POST with Attachments (base64)
API->>Validator: ValidateMessageReceive
Validator-->>API: validation errors (content still required!)
API->>MsgService: ReceiveMessage(params)
MsgService->>Storage: Upload each attachment (parallel errgroup)
Storage-->>MsgService: URLs or error (cleanup on failure)
MsgService->>Webhook: Dispatch MessagePhoneReceived event (with attachment URLs)
MsgService->>DB: storeReceivedMessage (Attachments: []string URLs)
DB-->>MsgService: saved message
MsgService-->>API: *entities.Message
Note over Android,API: Download (unauthenticated)
Android->>API: GET /v1/attachments/:userID/:messageID/:index/:filename
API->>Storage: Download(path)
Storage-->>API: []byte or ErrAttachmentNotFound (GCS missing mapping!)
API-->>Android: binary with Content-Type header
Reviews (1): Last reviewed commit: "fix: address code review findings for MM..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| reader, err := s.client.Bucket(s.bucket).Object(path).NewReader(ctx) | ||
| if err != nil { | ||
| return nil, s.tracer.WrapErrorSpan(span, stacktrace.Propagate(err, fmt.Sprintf("cannot open GCS reader for path [%s]", path))) | ||
| } |
There was a problem hiding this comment.
ErrAttachmentNotFound
When a GCS object does not exist, Object.NewReader returns storage.ErrObjectNotExist, but this is not translated into repositories.ErrAttachmentNotFound. The handler in attachment_handler.go relies on errors.Is(err, repositories.ErrAttachmentNotFound) to return a 404 — without this mapping, any request for a non-existent attachment in production will return a 500 instead.
| reader, err := s.client.Bucket(s.bucket).Object(path).NewReader(ctx) | |
| if err != nil { | |
| return nil, s.tracer.WrapErrorSpan(span, stacktrace.Propagate(err, fmt.Sprintf("cannot open GCS reader for path [%s]", path))) | |
| } | |
| reader, err := s.client.Bucket(s.bucket).Object(path).NewReader(ctx) | |
| if err != nil { | |
| if errors.Is(err, storage.ErrObjectNotExist) { | |
| return nil, ErrAttachmentNotFound | |
| } | |
| return nil, s.tracer.WrapErrorSpan(span, stacktrace.Propagate(err, fmt.Sprintf("cannot open GCS reader for path [%s]", path))) | |
| } |
| @@ -73,7 +79,47 @@ func (validator MessageHandlerValidator) ValidateMessageReceive(_ context.Contex | |||
| }, | |||
| }) | |||
|
|
|||
| return v.ValidateStruct() | |||
| errors := v.ValidateStruct() | |||
|
|
|||
| if len(request.Attachments) > 0 { | |||
| attachmentErrors := validator.validateAttachments(request.Attachments) | |||
| for key, values := range attachmentErrors { | |||
| for _, value := range values { | |||
| errors.Add(key, value) | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| return errors | |||
There was a problem hiding this comment.
content required even when attachments present
ValidateMessageReceive always enforces "required" and "min:1" on content. An MMS message that carries only attachments (no text body) would be rejected with a validation error before any attachment is processed. The requirement should be relaxed to allow empty content when at least one attachment is provided.
| "content": func() []string { | |
| if len(request.Attachments) > 0 { | |
| return []string{"max:2048"} | |
| } | |
| return []string{"required", "min:1", "max:2048"} | |
| }(), |
There was a problem hiding this comment.
Implement this logic.
There was a problem hiding this comment.
Pull request overview
Adds end-to-end MMS attachment support to the httpSMS stack: the API now accepts base64 attachments on message receive, uploads them to pluggable storage (GCS or in-memory), persists download URLs on the Message entity, exposes a public download endpoint, and includes attachment URLs in the phone-received webhook payload. The web UI is updated to display shorter attachment names.
Changes:
- API: introduce
AttachmentStorageabstraction (GCS + in-memory), attachment validation, upload flow inMessageService.ReceiveMessage(), and public download handler. - API: extend receive request + webhook event payload to include attachments.
- Web: format attachment display names in thread view.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| web/pages/threads/_id/index.vue | Display a shortened attachment name instead of the full URL. |
| docs/superpowers/specs/2026-04-11-mms-attachments-design.md | Design spec documenting storage, validation, URL format, handler behavior. |
| docs/superpowers/plans/2026-04-11-mms-attachments.md | Implementation plan/checklist for the feature. |
| api/pkg/validators/message_handler_validator.go | Adds attachment count/type/size validation for receive requests. |
| api/pkg/services/message_service.go | Upload attachments (parallel) during receive flow; store URLs on message + include in event payload. |
| api/pkg/requests/message_receive_request.go | Adds attachments field + struct; converts to service-layer attachment params. |
| api/pkg/repositories/memory_attachment_storage.go | In-memory attachment storage implementation. |
| api/pkg/repositories/gcs_attachment_storage.go | GCS attachment storage implementation. |
| api/pkg/repositories/attachment_storage.go | Storage interface + content-type/extension helpers + filename sanitization + not-found sentinel. |
| api/pkg/repositories/attachment_storage_test.go | Unit tests for extension mapping + filename sanitization. |
| api/pkg/handlers/attachment_handler.go | Public GET endpoint to download attachments from storage. |
| api/pkg/events/message_phone_received_event.go | Adds attachments array to webhook payload. |
| api/pkg/di/container.go | Wires storage selection (GCS vs memory), registers attachment routes, passes storage/base URL into MessageService. |
| api/go.mod | Bumps/adds cloud.google.com/go/storage dependency version. |
| api/go.sum | Updates dependency checksums to match module changes. |
| api/.env.docker | Adds GCS_BUCKET_NAME env var for selecting GCS storage. |
| func (s *GCSAttachmentStorage) Download(ctx context.Context, path string) ([]byte, error) { | ||
| ctx, span := s.tracer.Start(ctx) | ||
| defer span.End() | ||
|
|
||
| reader, err := s.client.Bucket(s.bucket).Object(path).NewReader(ctx) | ||
| if err != nil { | ||
| return nil, s.tracer.WrapErrorSpan(span, stacktrace.Propagate(err, fmt.Sprintf("cannot open GCS reader for path [%s]", path))) | ||
| } |
There was a problem hiding this comment.
GCSAttachmentStorage.Download currently wraps the error from Object(path).NewReader without translating a missing-object condition into ErrAttachmentNotFound. As a result, the public download handler will return 500 for non-existent attachments in GCS instead of 404. Consider mapping errors.Is(err, storage.ErrObjectNotExist) to ErrAttachmentNotFound (and keep other errors wrapped) so callers can reliably distinguish not-found across implementations.
| // SanitizeFilename removes path separators and traversal sequences from a filename. | ||
| // Returns "attachment-{index}" if the sanitized name is empty. | ||
| func SanitizeFilename(name string, index int) string { | ||
| name = strings.TrimSuffix(name, filepath.Ext(name)) | ||
| name = strings.ReplaceAll(name, "/", "") | ||
| name = strings.ReplaceAll(name, "\\", "") | ||
| name = strings.ReplaceAll(name, "..", "") | ||
| name = strings.TrimSpace(name) | ||
|
|
||
| if name == "" { | ||
| return fmt.Sprintf("attachment-%d", index) | ||
| } | ||
| return name | ||
| } |
There was a problem hiding this comment.
SanitizeFilename currently only strips path separators, "..", and surrounding whitespace, but it can still return names containing URL-reserved characters (spaces, #, ?, %, etc.) and non-printable characters. Since the sanitized name is embedded directly into the generated download URL path (without PathEscape), this can produce invalid or ambiguous URLs. Consider normalizing to a URL-safe character set (e.g., replace anything outside [A-Za-z0-9.-] with '', drop control chars) and/or URL-escaping when constructing the download URL.
There was a problem hiding this comment.
When sanitizing the name. stripe all characters ecelpt alpha numeric a-z 0-9 and replace space chracter by -
| const ( | ||
| maxAttachmentCount = 10 | ||
| maxAttachmentSize = (3 * 1024 * 1024) / 2 // 1.5 MB | ||
| ) |
There was a problem hiding this comment.
With maxAttachmentCount=10 and maxAttachmentSize=1.5MB decoded, a single receive request can realistically exceed ~20MB once base64-encoded + JSON overhead. The Fiber app is currently created with fiber.New() defaults (no BodyLimit configured), which may reject legitimate MMS payloads depending on defaults/proxy limits. Consider explicitly configuring an appropriate request body limit (ideally via env) and/or enforcing a total attachment payload cap during validation to prevent unexpected 413s and reduce DoS risk.
There was a problem hiding this comment.
Total size of all attachments should not go over 3 MB.
| // @Success 200 {file} binary | ||
| // @Failure 404 {object} responses.NotFoundResponse | ||
| // @Failure 500 {object} responses.InternalServerError | ||
| // @Router /attachments/{userID}/{messageID}/{attachmentIndex}/{filename} [get] |
There was a problem hiding this comment.
The registered route includes the "/v1" prefix (router.Get("/v1/attachments/...")), but the Swagger annotation uses "@router /attachments/{...} [get]". This will generate incorrect API docs/client code. Update the @Router path to match the actual route (including /v1).
| // @Router /attachments/{userID}/{messageID}/{attachmentIndex}/{filename} [get] | |
| // @Router /v1/attachments/{userID}/{messageID}/{attachmentIndex}/{filename} [get] |
| cloud.google.com/go/longrunning v0.9.0 // indirect | ||
| cloud.google.com/go/monitoring v1.25.0 // indirect | ||
| cloud.google.com/go/storage v1.61.3 // indirect | ||
| cloud.google.com/go/storage v1.62.0 // indirect |
There was a problem hiding this comment.
cloud.google.com/go/storage is now imported and used directly (DI container + GCSAttachmentStorage), but go.mod still marks it as an indirect dependency. This will be reverted by go mod tidy / pre-commit hooks and can cause noisy diffs. Please run go mod tidy so storage is a direct requirement (remove the // indirect marker).
| cloud.google.com/go/storage v1.62.0 // indirect | |
| cloud.google.com/go/storage v1.62.0 |
| ) | ||
|
|
||
| // GCSAttachmentStorage stores attachments in Google Cloud Storage | ||
| type GCSAttachmentStorage struct { |
There was a problem hiding this comment.
Call it GoogleCloudStorageAttachmentRepository
| ) | ||
|
|
||
| // AttachmentStorage is the interface for storing and retrieving message attachments | ||
| type AttachmentStorage interface { |
There was a problem hiding this comment.
Rename this file AttachmentRepository and also the interface name.
| } | ||
|
|
||
| // ErrAttachmentNotFound is returned when an attachment is not found in storage | ||
| var ErrAttachmentNotFound = fmt.Errorf("attachment not found") |
There was a problem hiding this comment.
Reuse the same NotFound error that is in this package.
| return s.tracer.WrapErrorSpan(span, stacktrace.Propagate(err, fmt.Sprintf("cannot close GCS writer for path [%s]", path))) | ||
| } | ||
|
|
||
| s.logger.Info(fmt.Sprintf("uploaded attachment to GCS path [%s/%s] with size [%d]", s.bucket, path, len(data))) |
There was a problem hiding this comment.
Always use ctxLogger so the trace_id and span_id will get captured.
| return s.tracer.WrapErrorSpan(span, stacktrace.Propagate(err, fmt.Sprintf("cannot delete GCS object at path [%s]", path))) | ||
| } | ||
|
|
||
| s.logger.Info(fmt.Sprintf("deleted attachment from GCS path [%s/%s]", s.bucket, path)) |
There was a problem hiding this comment.
always use ctxLogger.
| defer span.End() | ||
|
|
||
| s.data.Delete(path) | ||
| s.logger.Info(fmt.Sprintf("deleted attachment at path [%s]", path)) |
There was a problem hiding this comment.
Use ctxLogger in this class.
| // Timestamp is the time when the event was emitted, Please send the timestamp in UTC with as much precision as possible | ||
| Timestamp time.Time `json:"timestamp" example:"2022-06-05T14:26:09.527976+03:00"` | ||
| // Attachments is the list of MMS attachments received with the message | ||
| Attachments []MessageAttachment `json:"attachments"` |
There was a problem hiding this comment.
This should be marked as optional for so the swag.go generates the proper documentation.
| } | ||
|
|
||
| func (service *MessageService) uploadAttachments(ctx context.Context, userID entities.UserID, messageID uuid.UUID, attachments []ServiceAttachment) ([]string, error) { | ||
| ctx, span := service.tracer.Start(ctx) |
There was a problem hiding this comment.
use the format ctx, span, ctxLogger := service.tracer.StartWithLogger(ctx, service.logger)
| ctx, span := s.tracer.Start(ctx) | ||
| defer span.End() | ||
|
|
||
| writer := s.client.Bucket(s.bucket).Object(path).NewWriter(ctx) |
There was a problem hiding this comment.
when storing the attachment can we also include the contet type in the artifact?
…tachmentRepository - Rename interface AttachmentStorage -> AttachmentRepository - Rename GCSAttachmentStorage -> GoogleCloudStorageAttachmentRepository - Rename MemoryAttachmentStorage -> MemoryAttachmentRepository - Rename all files: attachment_storage.go -> attachment_repository.go, etc. - Replace ErrAttachmentNotFound with existing ErrCodeNotFound pattern - Map GCS storage.ErrObjectNotExist to ErrCodeNotFound in Download - Use StartWithLogger + ctxLogger in all repository methods - Use StartWithLogger in uploadAttachments service method - Add contentType parameter to Upload interface, set on GCS writer - Mark Attachments field as optional in swagger docs - Fix Swagger @router to include /v1 prefix - Run go mod tidy to fix indirect marker Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nt_repository.go Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Make content field optional when attachments are present (MMS-only messages) - SanitizeFilename: strip all non-alphanumeric chars, replace spaces with dashes - Add 3MB total attachment size limit in validation - Update tests for new SanitizeFilename behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if len(params.Attachments) > 0 { | ||
| ctxLogger.Info(fmt.Sprintf("uploading [%d] attachments for message [%s]", len(params.Attachments), messageID)) | ||
| var err error | ||
| attachmentURLs, err = service.uploadAttachments(ctx, params.UserID, messageID, params.Attachments) |
There was a problem hiding this comment.
use attachmentURLs, err := service.uploadAttachments(ctx, params.UserID, messageID, params.Attachments)
| attachmentURLs := []string{} | ||
|
|
||
| if len(params.Attachments) > 0 { | ||
| ctxLogger.Info(fmt.Sprintf("uploading [%d] attachments for message [%s]", len(params.Attachments), messageID)) |
There was a problem hiding this comment.
Add the user ID in the log message
| var err error | ||
| attachmentURLs, err = service.uploadAttachments(ctx, params.UserID, messageID, params.Attachments) | ||
| if err != nil { | ||
| msg := fmt.Sprintf("cannot upload attachments for message [%s]", messageID) |
There was a problem hiding this comment.
add user id in log message
- Use attachmentURLs, err := instead of var err error pattern - Add early return in uploadAttachments for empty slices - Include user ID in upload and error log messages Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 50 minor |
🟢 Metrics 75 complexity · -1 duplication
Metric Results Complexity 75 Duplication -1
TIP This summary will be updated as you push new changes. Give us feedback
Summary
Add MMS attachment support to the httpSMS API. When an Android phone receives an MMS message with attachments, the API now:
Changes
New Files
Modified Files
Configuration
Set \GCS_BUCKET_NAME\ env var to enable GCS storage. Leave empty for in-memory storage.
Download Endpoint
\GET /v1/attachments/{userID}/{messageID}/{attachmentIndex}/{filename}\ — unauthenticated, returns binary with proper Content-Type headers.