Optimize storage and use assets CDN for PFPs and group images

[lourou]

• Add assets CDN url config
• Implement AssetURLResolver
• Remove trailing slash from config for backendUrl, appendingPathComponent will add it automatically
• Update and refactor ConvosAPIClient for asset upload, rename filename with assetKey
• Extract path from old URL and load asset from new CDN URLs

[claude]

Claude finished @lourou's task —— View job

---

PR Review: Optimize storage and use assets CDN

Overview

This PR implements CDN-based asset handling for profile pictures and group images. However, there are critical missing components and several important issues to address.

---

🚨 Critical Issues

1. Missing AssetURLResolver Implementation

The PR description mentions "Implement AssetURLResolver" but this file doesn't exist in the codebase. Multiple review comments from macroscopeapp reference AssetURLResolver.swift, but the file is not present in the changed files or repository.

Impact: Without the resolver, the system cannot:

• Convert asset keys to CDN URLs
• Extract asset keys from old URLs
• Validate allowed hosts
• Handle URL migration

Required Action: The AssetURLResolver.swift file needs to be added to the PR.

2. Missing CDN Configuration Fields

The PR mentions adding assetsCdnUrl and allowedAssetHosts to configs, but:

• These fields are not present in any config files (config.local.json:1, config.dev.json, config.prod.json)
• ConvosConfiguration.swift:11 doesn't include these properties
• AppEnvironment.swift:1 doesn't expose these values

Impact: The CDN cannot be configured, making the entire feature non-functional.

Required Action: Add to ConvosConfiguration:

public let assetsCdnUrl: String?
public let allowedAssetHosts: [String]

And add to config files:

"assetsCdnUrl": "https://assets.convos.org",
"allowedAssetHosts": ["assets.convos.org", "convos-uploads.s3.amazonaws.com"]

---

🐛 Bug & Logic Issues

3. API Response Contract Change Without Backward Compatibility (ConvosCore/Sources/ConvosCore/API/ConvosAPIClient.swift:354)

struct PresignedResponse: Codable {
    let objectKey: String
    let uploadUrl: String    // Upload pre-signed URL
    let assetUrl: String     // Final asset URL
}

Problem: All three fields are required. If the backend still returns the legacy url field, decoding will throw and break all uploads.

Solution: Add backward compatibility:

struct PresignedResponse: Codable {
    let objectKey: String
    let uploadUrl: String?
    let assetUrl: String?
    let url: String?  // Legacy field
    
    var effectiveUploadUrl: String {
        uploadUrl ?? url ?? ""
    }
    
    var effectiveAssetUrl: String {
        assetUrl ?? url ?? ""
    }
}

4. Returning Full URL Instead of Asset Key (ConvosCore/Sources/ConvosCore/API/ConvosAPIClient.swift:398)

let assetPath = assetUrl.absoluteString
Log.info("Successfully uploaded to S3, assetUrl: \(assetPath)")
return assetPath

Problem: The function returns a full URL (https://cdn.../key.jpg) instead of just the asset key. This makes it impossible to migrate assets between CDNs later.

Recommendation: Return presignedResponse.objectKey for storage, and resolve to full URL at display time using AssetURLResolver.

5. No AssetURLResolver Configuration (Convos/ConvosApp.swift:14)

The app initializer doesn't configure AssetURLResolver with CDN settings. Even if the resolver exists, it won't work without initialization.

Required: Add after line 30:

// Configure AssetURLResolver with CDN settings
if let cdnUrl = environment.assetsCdnUrl {
    AssetURLResolver.configure(
        cdnBaseURL: cdnUrl,
        allowedHosts: environment.allowedAssetHosts
    )
}

6. QuicknameSettings Profile Has Nil Avatar by Design (Convos/Profile/QuicknameSettings.swift:34)

var profile: Profile {
    .init(
        inboxId: "",
        name: displayName.isEmpty ? nil : displayName,
        avatar: nil  // Intentionally nil
    )
}

Concern: The comment states this is intentional because profileImage is passed as placeholderImage. However, if a user uploads a quickname avatar, where is the URL stored? This could lead to data loss.

---

🔒 Security Concerns

7. Presigned URL Logging Leaks Credentials (ConvosCore/Sources/ConvosCore/API/ConvosAPIClient.swift:362)

Log.info("Received presigned response for objectKey: \(presignedResponse.objectKey)")

While this specific line is okay, if the presigned URLs themselves are logged (uploadUrl contains AWS credentials in query params), they'll be exposed in logs.

Recommendation: Audit all logging statements to ensure presigned URLs with query parameters are never logged.

8. No Host Validation Without AssetURLResolver

Currently, there's no validation that uploaded asset URLs come from trusted sources. Malicious actors could potentially inject arbitrary URLs.

Required: Implement host allowlist validation in AssetURLResolver.

---

⚡ Performance Considerations

9. Image Compression Happens Multiple Times

In MyProfileWriter.swift:91 and ConversationMetadataWriter.swift:142, images are resized and cached via ImageCache.shared.resizeCacheAndGetData(). The caching is good, but verify that:

• Compression quality is appropriate (currently not specified)
• Large images don't cause memory spikes
• The cache has reasonable size limits

10. No Retry Logic for S3 Upload Failures (ConvosCore/Sources/ConvosCore/API/ConvosAPIClient.swift:377)

let (s3Data, s3Response) = try await URLSession.shared.data(for: s3Request)

If the S3 upload fails (network issues, timeouts), the entire operation fails. Consider adding retry logic with exponential backoff.

---

🧪 Test Coverage

11. No Tests for Upload Flow

The PR doesn't include:

• Tests for uploadAttachment() with the new response format
• Tests for URL resolution (AssetURLResolver)
• Tests for backward compatibility with legacy URLs
• Tests for error cases (invalid presigned URL, S3 failure, etc.)

12. MockAPIClient Returns Wrong URL Format (ConvosCore/Sources/ConvosCore/API/MockAPIClient.swift:41)

"https://mock-api.example.com/uploads/\(filename)"

This returns a full URL, but based on the new contract, it should return an asset URL that matches production behavior.

---

📝 Code Quality

13. Config Files Only Remove Trailing Slash

The config changes only remove the trailing slash from backendUrl:

• config.local.json: "http://localhost:4000/api" (removed /)
• config.dev.json: Similar change
• config.prod.json: Similar change

This is good (as mentioned in the PR description, appendingPathComponent adds it automatically), but there are no CDN-related fields added.

14. Inconsistent Error Handling

Some functions throw descriptive errors (ConversationMetadataError.conversationNotFound), while others throw generic APIError.invalidResponse. Consider more specific error types for CDN/asset operations.

---

✅ Positive Aspects

1. Good separation of concerns: Upload flow in API client, writers handle business logic
2. Proper logging: Good use of Log statements for debugging
3. Image caching: ImageCache.shared usage prevents redundant downloads
4. Cleanup on avatar removal: MyProfileWriter.swift:84 properly removes cached images

---

📋 Summary & Recommendations

Before Merging:

1. ✅ Add AssetURLResolver.swift implementation
2. ✅ Add assetsCdnUrl and allowedAssetHosts to configs and data models
3. ✅ Configure AssetURLResolver in app initialization
4. ✅ Add backward compatibility to PresignedResponse decoding
5. ✅ Add unit tests for upload flow and URL resolution
6. ⚠️ Consider returning asset keys instead of full URLs
7. ⚠️ Add retry logic for S3 uploads
8. ⚠️ Audit logging to ensure no credential leaks

Priority: Address items 1-5 (critical blocking issues) before merge. Items 6-8 can be follow-up improvements.

---

| Branch: assets-endpoints

[lourou]

• Optimize storage and use assets CDN for PFPs and group images #254 👈 (View in Graphite)
• dev

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[macroscopeapp]

Route image uploads through CDN by returning ConvosAPIClient.uploadAttachment asset URLs and set avatar to nil in QuicknameSettings while normalizing API base URLs without trailing slashes

Switch uploadAttachment to consume presigned responses with uploadUrl and return a validated assetUrl for CDN access; set avatar=nil in QuicknameSettings while using a local placeholder; and remove trailing slashes from API base URLs across configs and tests. Mocks update to https://mock-api.example.com/uploads/{filename}.

📍Where to Start

Start with ConvosAPIClient.uploadAttachment in ConvosCore/Sources/ConvosCore/API/ConvosAPIClient.swift.

---

Macroscope summarized 1908afc.

[yewreeka]

As long as the change removing the avatar URL has been thoroughly tested, I think this is good to go

[yewreeka]

I feel like I did this because there was a component that wasn't using the profileImage and needed the URL.

[lourou]

Thanks! I'll check it out and resolve the conflicts too!