From 69973c56e22301e960576488dc04b8e7863bef17 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:00:36 -0700 Subject: [PATCH] feat: add canonical avatar resolver --- backend/__tests__/service/auth.test.js | 6 +- backend/__tests__/service/users.test.js | 4 +- .../unit/controllers/authController.test.js | 7 +- .../unit/controllers/userController.test.js | 14 ++- .../registry.cloud-entitlement-gate.test.js | 4 + .../registry.install-runtime-type.test.js | 60 ++++++++++++ .../routes/registry.templates-avatar.test.js | 92 +++++++++++++++++++ .../unit/routes/uploads.post.test.js | 4 +- ...tIdentityService.collisionResolver.test.js | 33 +++++++ .../unit/services/avatarService.test.js | 67 ++++++++++++++ backend/controllers/authController.ts | 5 +- backend/controllers/userController.ts | 5 +- backend/routes/registry/install.ts | 24 +++++ backend/routes/registry/templates.ts | 24 +---- backend/routes/uploads.ts | 23 +---- backend/scripts/generate-team-avatars.ts | 3 +- backend/scripts/seed-native-agents.ts | 3 +- backend/services/agentIdentityService.ts | 4 +- backend/services/avatarService.ts | 81 ++++++++++++++++ frontend/src/v2/__tests__/V2Avatar.test.tsx | 37 ++++++++ .../src/v2/__tests__/V2MessageBubble.test.tsx | 42 +++++++++ frontend/src/v2/components/V2Avatar.tsx | 4 +- .../src/v2/components/V2MessageBubble.tsx | 7 +- 23 files changed, 493 insertions(+), 60 deletions(-) create mode 100644 backend/__tests__/unit/routes/registry.templates-avatar.test.js create mode 100644 backend/__tests__/unit/services/avatarService.test.js create mode 100644 backend/services/avatarService.ts create mode 100644 frontend/src/v2/__tests__/V2Avatar.test.tsx create mode 100644 frontend/src/v2/__tests__/V2MessageBubble.test.tsx diff --git a/backend/__tests__/service/auth.test.js b/backend/__tests__/service/auth.test.js index 337acdd0b..c16d9e74f 100644 --- a/backend/__tests__/service/auth.test.js +++ b/backend/__tests__/service/auth.test.js @@ -417,7 +417,7 @@ describe('Auth Routes Integration Tests', () => { const token = generateTestToken(user._id); const updateData = { - profilePicture: 'new-profile-pic-url', + profilePicture: 'https://api-dev.commonly.me/api/uploads/new-profile-pic.png', }; const response = await request(app) @@ -426,11 +426,11 @@ describe('Auth Routes Integration Tests', () => { .send(updateData) .expect(200); - expect(response.body.profilePicture).toBe('new-profile-pic-url'); + expect(response.body.profilePicture).toBe('/api/uploads/new-profile-pic.png'); // Verify database update const updatedUser = await User.findById(user._id); - expect(updatedUser.profilePicture).toBe('new-profile-pic-url'); + expect(updatedUser.profilePicture).toBe('/api/uploads/new-profile-pic.png'); }); it('should return 401 without token', async () => { diff --git a/backend/__tests__/service/users.test.js b/backend/__tests__/service/users.test.js index aa7cb0f4c..b3ef0cec9 100644 --- a/backend/__tests__/service/users.test.js +++ b/backend/__tests__/service/users.test.js @@ -67,9 +67,9 @@ describe('User Routes Integration Tests', () => { .send({ profilePicture: 'newpic.jpg' }) .expect(200); - expect(res.body.profilePicture).toBe('newpic.jpg'); + expect(res.body.profilePicture).toBe('/api/uploads/newpic.jpg'); const updated = await User.findById(user._id); - expect(updated.profilePicture).toBe('newpic.jpg'); + expect(updated.profilePicture).toBe('/api/uploads/newpic.jpg'); }); }); diff --git a/backend/__tests__/unit/controllers/authController.test.js b/backend/__tests__/unit/controllers/authController.test.js index 557034648..43fa67d00 100644 --- a/backend/__tests__/unit/controllers/authController.test.js +++ b/backend/__tests__/unit/controllers/authController.test.js @@ -558,7 +558,7 @@ describe('Auth Controller Tests', () => { _id: userId, username: 'testuser', email: 'test@example.com', - profilePicture: 'new-profile-pic-url', + profilePicture: '/api/uploads/new-profile-pic.png', }; // First mock for the initial findById @@ -573,7 +573,7 @@ describe('Auth Controller Tests', () => { const req = { userId, body: { - profilePicture: 'new-profile-pic-url', + profilePicture: 'https://api.commonly.me/api/uploads/new-profile-pic.png', }, }; @@ -587,12 +587,13 @@ describe('Auth Controller Tests', () => { // Verify response expect(res.json).toHaveBeenCalledWith( expect.objectContaining({ - profilePicture: 'new-profile-pic-url', + profilePicture: '/api/uploads/new-profile-pic.png', }), ); // Verify save was called expect(initialUser.save).toHaveBeenCalled(); + expect(initialUser.profilePicture).toBe('/api/uploads/new-profile-pic.png'); }); it('should return 404 if user not found', async () => { diff --git a/backend/__tests__/unit/controllers/userController.test.js b/backend/__tests__/unit/controllers/userController.test.js index 095cda987..e4f71f0c1 100644 --- a/backend/__tests__/unit/controllers/userController.test.js +++ b/backend/__tests__/unit/controllers/userController.test.js @@ -49,12 +49,15 @@ describe('User Controller', () => { describe('updateProfile', () => { it('updates the profile picture of the user', async () => { - const updatedUser = mockUserDoc({ _id: 'u1', profilePicture: 'newpic' }); + const updatedUser = mockUserDoc({ _id: 'u1', profilePicture: '/api/uploads/newpic.png' }); User.findByIdAndUpdate = jest .fn() .mockReturnValue({ select: jest.fn().mockResolvedValueOnce(updatedUser) }); - const req = { user: { id: 'u1' }, body: { profilePicture: 'newpic' } }; + const req = { + user: { id: 'u1' }, + body: { profilePicture: 'https://api-dev.commonly.me/api/uploads/newpic.png' }, + }; const res = { json: jest.fn(), status: jest.fn().mockReturnThis(), @@ -63,10 +66,13 @@ describe('User Controller', () => { await userController.updateProfile(req, res); expect(User.findByIdAndUpdate).toHaveBeenCalledWith( 'u1', - { $set: { profilePicture: 'newpic' } }, + { $set: { profilePicture: '/api/uploads/newpic.png' } }, { new: true }, ); - expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ _id: 'u1', profilePicture: 'newpic' })); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ + _id: 'u1', + profilePicture: '/api/uploads/newpic.png', + })); }); }); diff --git a/backend/__tests__/unit/routes/registry.cloud-entitlement-gate.test.js b/backend/__tests__/unit/routes/registry.cloud-entitlement-gate.test.js index bf880e7a9..f06809f7a 100644 --- a/backend/__tests__/unit/routes/registry.cloud-entitlement-gate.test.js +++ b/backend/__tests__/unit/routes/registry.cloud-entitlement-gate.test.js @@ -26,6 +26,10 @@ jest.mock('../../../models/AgentProfile', () => ({ findOneAndUpdate: jest.fn(), })); +jest.mock('../../../models/AgentTemplate', () => ({ + find: jest.fn(), +})); + jest.mock('../../../models/Activity', () => ({ create: jest.fn(), })); diff --git a/backend/__tests__/unit/routes/registry.install-runtime-type.test.js b/backend/__tests__/unit/routes/registry.install-runtime-type.test.js index e973453f7..d07e58e0c 100644 --- a/backend/__tests__/unit/routes/registry.install-runtime-type.test.js +++ b/backend/__tests__/unit/routes/registry.install-runtime-type.test.js @@ -23,6 +23,10 @@ jest.mock('../../../models/AgentProfile', () => ({ findOneAndUpdate: jest.fn(), })); +jest.mock('../../../models/AgentTemplate', () => ({ + find: jest.fn(), +})); + jest.mock('../../../models/Activity', () => ({ create: jest.fn(), })); @@ -59,6 +63,7 @@ const { AgentRegistry, AgentInstallation } = require('../../../models/AgentRegis const Pod = require('../../../models/Pod'); const User = require('../../../models/User'); const AgentProfile = require('../../../models/AgentProfile'); +const AgentTemplate = require('../../../models/AgentTemplate'); const Activity = require('../../../models/Activity'); const AgentIdentityService = require('../../../services/agentIdentityService'); const FirstContactService = require('../../../services/firstContactService'); @@ -120,6 +125,11 @@ describe('registry install runtimeType fallback', () => { User.findById.mockReturnValue(buildSelectLeanChain({ username: 'installer', role: 'admin' })); AgentProfile.findOneAndUpdate.mockResolvedValue(true); + AgentTemplate.find.mockReturnValue({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue([]), + }), + }); Activity.create.mockResolvedValue(true); }); @@ -254,4 +264,54 @@ describe('registry install runtimeType fallback', () => { expect(res.status).not.toHaveBeenCalledWith(500); expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ success: true })); }); + + it('seeds a new identity from the normalized per-instance template icon', async () => { + AgentRegistry.getByName.mockResolvedValue({ + agentName: 'sample-agent', + displayName: 'Sample Agent', + description: 'Community marketplace app', + iconUrl: 'https://api.commonly.me/api/uploads/registry.png', + latestVersion: '1.0.0', + manifest: { + context: { required: [] }, + runtime: { type: 'standalone' }, + }, + }); + AgentTemplate.find.mockReturnValue({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue([{ + displayName: 'Aria', + iconUrl: 'https://api-dev.commonly.me/api/uploads/aria.png', + createdBy: 'user-1', + visibility: 'private', + }]), + }), + }); + const req = { + body: { + agentName: 'sample-agent', + podId: 'pod-1', + version: '1.0.0', + displayName: 'Aria', + config: {}, + scopes: [], + }, + user: { id: 'user-1', username: 'installer' }, + userId: 'user-1', + }; + const res = { + status: jest.fn().mockReturnThis(), + json: jest.fn(), + }; + + await installHandler(req, res); + + expect(AgentIdentityService.getOrCreateAgentUser).toHaveBeenCalledWith( + 'sample-agent', + expect.objectContaining({ + displayName: 'Aria', + profilePicture: '/api/uploads/aria.png', + }), + ); + }); }); diff --git a/backend/__tests__/unit/routes/registry.templates-avatar.test.js b/backend/__tests__/unit/routes/registry.templates-avatar.test.js new file mode 100644 index 000000000..5e8e3a3ed --- /dev/null +++ b/backend/__tests__/unit/routes/registry.templates-avatar.test.js @@ -0,0 +1,92 @@ +jest.mock('../../../models/AgentRegistry', () => ({ + AgentRegistry: { + getByName: jest.fn(), + }, +})); + +jest.mock('../../../models/AgentTemplate', () => ({ + find: jest.fn(), + findOne: jest.fn(), + findById: jest.fn(), + create: jest.fn(), + deleteOne: jest.fn(), +})); + +const { AgentRegistry } = require('../../../models/AgentRegistry'); +const AgentTemplate = require('../../../models/AgentTemplate'); +const templatesRouter = require('../../../routes/registry/templates'); + +const getHandler = (method, path) => { + const layer = templatesRouter.stack.find((entry) => ( + entry.route && entry.route.path === path && entry.route.methods[method] + )); + if (!layer) throw new Error(`${method.toUpperCase()} ${path} handler not found`); + return layer.route.stack[layer.route.stack.length - 1].handle; +}; + +const response = () => ({ + status: jest.fn().mockReturnThis(), + json: jest.fn(), +}); + +describe('registry template avatar writes', () => { + beforeEach(() => { + jest.clearAllMocks(); + AgentRegistry.getByName.mockResolvedValue({ agentName: 'openclaw' }); + AgentTemplate.findOne.mockReturnValue({ + select: jest.fn().mockReturnValue({ + lean: jest.fn().mockResolvedValue(null), + }), + }); + AgentTemplate.create.mockImplementation(async (data) => ({ + ...data, + _id: { toString: () => 'template-1' }, + })); + }); + + it('stores a newly created package icon as a relative upload URL', async () => { + const req = { + userId: 'user-1', + body: { + agentName: 'openclaw', + displayName: 'Aria', + iconUrl: 'https://api-dev.commonly.me/api/uploads/aria.png', + }, + }; + const res = response(); + + await getHandler('post', '/templates')(req, res); + + expect(AgentTemplate.create).toHaveBeenCalledWith(expect.objectContaining({ + iconUrl: '/api/uploads/aria.png', + })); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ + template: expect.objectContaining({ iconUrl: '/api/uploads/aria.png' }), + })); + }); + + it('normalizes an updated package icon without mutating identity data', async () => { + const template = { + _id: 'template-1', + agentName: 'openclaw', + displayName: 'Aria', + description: '', + iconUrl: '/api/uploads/old.png', + visibility: 'private', + createdBy: { toString: () => 'user-1' }, + save: jest.fn().mockResolvedValue(undefined), + }; + AgentTemplate.findById.mockResolvedValue(template); + const req = { + userId: 'user-1', + params: { id: 'template-1' }, + body: { iconUrl: 'http://localhost:5000/api/uploads/new.png' }, + }; + const res = response(); + + await getHandler('patch', '/templates/:id')(req, res); + + expect(template.iconUrl).toBe('/api/uploads/new.png'); + expect(template.save).toHaveBeenCalled(); + }); +}); diff --git a/backend/__tests__/unit/routes/uploads.post.test.js b/backend/__tests__/unit/routes/uploads.post.test.js index 1b98bf956..84aaa5aad 100644 --- a/backend/__tests__/unit/routes/uploads.post.test.js +++ b/backend/__tests__/unit/routes/uploads.post.test.js @@ -45,7 +45,7 @@ describe('uploads POST / (ADR-002 Phase 1)', () => { }); it('writes bytes through the driver and saves metadata-only File', async () => { - await request(app) + const res = await request(app) .post('/api/uploads') .attach('image', Buffer.from('data'), 'photo.png') .expect(200); @@ -64,6 +64,8 @@ describe('uploads POST / (ADR-002 Phase 1)', () => { expect(fileArgs.contentType).toBe('image/png'); expect(fileArgs.uploadedBy).toBe('user1'); expect(File.__saveMock).toHaveBeenCalled(); + expect(res.body.url).toMatch(/^\/api\/uploads\/[^/]+\.png$/); + expect(res.body.url).not.toMatch(/^https?:\/\//); }); it('returns 400 when no file is provided', async () => { diff --git a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js index 54b4eef84..a896e61da 100644 --- a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js +++ b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js @@ -77,6 +77,39 @@ describe('inline displayName collision resolver (sticky dedup)', () => { expect(mockSaved[0].botMetadata.displayName).toBe('Pixel'); }); + test('new identity seeds a normalized package avatar', async () => { + await AgentIdentityService.getOrCreateAgentUser('openclaw', { + instanceId: 'aria', + displayName: 'Aria', + profilePicture: 'https://api-dev.commonly.me/api/uploads/aria.png', + }); + + expect(mockSaved[0].profilePicture).toBe('/api/uploads/aria.png'); + }); + + test('reinstall does not overwrite an existing customized identity avatar', async () => { + mockExisting = { + _id: 'aria-id', + username: 'openclaw-aria', + profilePicture: '/api/uploads/customized.png', + isBot: true, + botMetadata: { + agentName: 'openclaw', + instanceId: 'aria', + displayName: 'Aria', + runtime: 'moltbot', + }, + }; + + const user = await AgentIdentityService.getOrCreateAgentUser('openclaw', { + instanceId: 'aria', + profilePicture: '/api/uploads/package-seed.png', + }); + + expect(user.profilePicture).toBe('/api/uploads/customized.png'); + expect(mockSaved).toHaveLength(0); + }); + test('new install collides with an existing canonical — gets suffix', async () => { // openclaw-pixel already has displayName="Pixel" with instanceId "pixel" (shorter) mockPeers = [ diff --git a/backend/__tests__/unit/services/avatarService.test.js b/backend/__tests__/unit/services/avatarService.test.js new file mode 100644 index 000000000..fdb61c656 --- /dev/null +++ b/backend/__tests__/unit/services/avatarService.test.js @@ -0,0 +1,67 @@ +const mockFind = jest.fn(); + +jest.mock('../../../models/User', () => ({ + find: mockFind, +})); + +const { + normalizeAvatarUrl, + resolveAvatarUrl, + resolveAvatarUrls, +} = require('../../../services/avatarService'); + +describe('avatarService', () => { + beforeEach(() => { + mockFind.mockReset(); + }); + + test.each([ + ['https://api-dev.commonly.me/api/uploads/dev-avatar.png', '/api/uploads/dev-avatar.png'], + ['https://api.commonly.me/api/uploads/live-avatar.png?old=1', '/api/uploads/live-avatar.png'], + ['http://localhost:5000/api/uploads/local-avatar.png', '/api/uploads/local-avatar.png'], + ['/api/uploads/already-relative.png', '/api/uploads/already-relative.png'], + ['avatar.png', '/api/uploads/avatar.png'], + ['blue', 'blue'], + ['legacy-avatar-id', 'legacy-avatar-id'], + ['data:image/png;base64,abc123', 'data:image/png;base64,abc123'], + [' ', null], + [null, null], + ['default', null], + ])('normalizes %p to %p', (raw, expected) => { + expect(normalizeAvatarUrl(raw)).toBe(expected); + }); + + it('preserves external absolute URLs that have no local upload key', () => { + expect(normalizeAvatarUrl('https://cdn.example.com/avatars/a.png')) + .toBe('https://cdn.example.com/avatars/a.png'); + }); + + it('resolves one user without inventing a default avatar', () => { + expect(resolveAvatarUrl({ profilePicture: 'default' })).toBeNull(); + expect(resolveAvatarUrl({ profilePicture: 'blue' })).toBeNull(); + expect(resolveAvatarUrl(undefined)).toBeNull(); + }); + + it('batch-resolves users in one query and includes missing ids as null', async () => { + const lean = jest.fn().mockResolvedValue([ + { + _id: { toString: () => 'user-1' }, + profilePicture: 'https://api-dev.commonly.me/api/uploads/avatar-1.png', + }, + ]); + const select = jest.fn().mockReturnValue({ lean }); + mockFind.mockReturnValue({ select }); + + const result = await resolveAvatarUrls(['user-1', 'missing-user']); + + expect(mockFind).toHaveBeenCalledTimes(1); + expect(mockFind).toHaveBeenCalledWith({ + _id: { $in: ['user-1', 'missing-user'] }, + }); + expect(select).toHaveBeenCalledWith('_id profilePicture'); + expect(result).toEqual(new Map([ + ['user-1', '/api/uploads/avatar-1.png'], + ['missing-user', null], + ])); + }); +}); diff --git a/backend/controllers/authController.ts b/backend/controllers/authController.ts index 7bba0e740..169930548 100644 --- a/backend/controllers/authController.ts +++ b/backend/controllers/authController.ts @@ -7,6 +7,7 @@ const WaitlistRequest = require('../models/WaitlistRequest'); const AgentIdentityService = require('../services/agentIdentityService'); const { sendEmail } = require('../services/emailService'); const { ensureUserInCommunityPod } = require('../services/communityPodService'); +const { normalizeAvatarUrl } = require('../services/avatarService'); const joinCommunityPodBestEffort = (userId: any) => { void ensureUserInCommunityPod(userId).catch((err: Error) => { @@ -652,8 +653,8 @@ exports.updateProfile = async (req: any, res: any) => { } // Update profile picture if provided - if (profilePicture) { - user.profilePicture = profilePicture; + if (profilePicture !== undefined) { + user.profilePicture = normalizeAvatarUrl(profilePicture); } await user.save(); diff --git a/backend/controllers/userController.ts b/backend/controllers/userController.ts index 6c011bbe9..af9e5ec38 100644 --- a/backend/controllers/userController.ts +++ b/backend/controllers/userController.ts @@ -3,6 +3,7 @@ const Activity = require('../models/Activity'); const Post = require('../models/Post'); const Pod = require('../models/Pod'); const AgentIdentityService = require('../services/agentIdentityService'); +const { normalizeAvatarUrl } = require('../services/avatarService'); const toSocialProfile = (userDoc: any, viewerId: any = null) => { const followers = Array.isArray(userDoc.followers) ? userDoc.followers : []; @@ -44,7 +45,9 @@ exports.updateProfile = async (req: any, res: any) => { const userFields: any = {}; if (username) userFields.username = username; if (bio) userFields.bio = bio; - if (profilePicture) userFields.profilePicture = profilePicture; + if (profilePicture !== undefined) { + userFields.profilePicture = normalizeAvatarUrl(profilePicture); + } // Update user const user = await User.findByIdAndUpdate( diff --git a/backend/routes/registry/install.ts b/backend/routes/registry/install.ts index 91e0d5138..d09cb7464 100644 --- a/backend/routes/registry/install.ts +++ b/backend/routes/registry/install.ts @@ -5,12 +5,14 @@ const rateLimit = require('express-rate-limit'); const auth = require('../../middleware/auth'); const { AgentRegistry, AgentInstallation } = require('../../models/AgentRegistry'); const AgentProfile = require('../../models/AgentProfile'); +const AgentTemplate = require('../../models/AgentTemplate'); const Activity = require('../../models/Activity'); const Pod = require('../../models/Pod'); const User = require('../../models/User'); const AgentIdentityService = require('../../services/agentIdentityService'); const AgentMessageService = require('../../services/agentMessageService'); const FirstContactService = require('../../services/firstContactService'); +const { normalizeAvatarUrl } = require('../../services/avatarService'); const { getUserId, normalizeInstanceId, @@ -437,9 +439,31 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any const explicitDisplayName = typeof displayName === 'string' && displayName.trim() ? displayName.trim() : undefined; + let avatarSeed = normalizeAvatarUrl(agent.iconUrl); + if (explicitDisplayName) { + // Query by the sanitized package key, then compare the user-provided + // display label in memory. Besides avoiding a tainted Mongo filter, + // this lets the installer's private template win over a public one. + const templateCandidates = await AgentTemplate.find({ + agentName: safeAgentName, + $or: [ + { createdBy: userId }, + { visibility: 'public' }, + ], + }).select('displayName iconUrl createdBy visibility').lean(); + const matchingTemplates = templateCandidates.filter((template: any) => ( + String(template.displayName || '').trim().toLowerCase() + === explicitDisplayName.toLowerCase() + )); + const template = matchingTemplates.find( + (candidate: any) => String(candidate.createdBy || '') === String(userId), + ) || matchingTemplates.find((candidate: any) => candidate.visibility === 'public'); + avatarSeed = normalizeAvatarUrl(template?.iconUrl) || avatarSeed; + } const agentUser = await AgentIdentityService.getOrCreateAgentUser(agent.agentName, { instanceId: normalizedInstanceId, ...(explicitDisplayName ? { displayName: explicitDisplayName } : {}), + ...(avatarSeed ? { profilePicture: avatarSeed } : {}), }); await AgentIdentityService.ensureAgentInPod(agentUser, podId); } catch (identityError: unknown) { diff --git a/backend/routes/registry/templates.ts b/backend/routes/registry/templates.ts index 45a404980..7731651ef 100644 --- a/backend/routes/registry/templates.ts +++ b/backend/routes/registry/templates.ts @@ -3,7 +3,7 @@ const express = require('express'); const auth = require('../../middleware/auth'); const { AgentRegistry } = require('../../models/AgentRegistry'); const AgentTemplate = require('../../models/AgentTemplate'); -const User = require('../../models/User'); +const { normalizeAvatarUrl } = require('../../services/avatarService'); const { getUserId, escapeRegExp } = require('./helpers'); const templatesRouter = express.Router(); @@ -92,20 +92,11 @@ templatesRouter.post('/templates', auth, async (req: any, res: any) => { agentName: agentName.toLowerCase(), displayName: trimmedDisplayName, description, - iconUrl, + iconUrl: normalizeAvatarUrl(iconUrl) || '', visibility, createdBy: userId, }); - // Sync iconUrl to User.profilePicture so post/comment populates pick it up - if (iconUrl) { - const instanceId = trimmedDisplayName.toLowerCase(); - await User.updateMany( - { 'botMetadata.agentName': agentName.toLowerCase(), 'botMetadata.instanceId': instanceId }, - { profilePicture: iconUrl }, - ); - } - return res.json({ success: true, template: { @@ -170,20 +161,11 @@ templatesRouter.patch('/templates/:id', auth, async (req: any, res: any) => { } if (iconUrl !== undefined) { - template.iconUrl = iconUrl || ''; + template.iconUrl = normalizeAvatarUrl(iconUrl) || ''; } await template.save(); - // Sync iconUrl to User.profilePicture so post/comment populates pick it up - if (iconUrl !== undefined) { - const instanceId = template.displayName.toLowerCase(); - await User.updateMany( - { 'botMetadata.agentName': template.agentName, 'botMetadata.instanceId': instanceId }, - { profilePicture: template.iconUrl || 'default' }, - ); - } - return res.json({ success: true, template: { diff --git a/backend/routes/uploads.ts b/backend/routes/uploads.ts index 30105af70..3fef94e9a 100644 --- a/backend/routes/uploads.ts +++ b/backend/routes/uploads.ts @@ -30,6 +30,7 @@ import { } from '../services/attachmentAccess'; import { logAttachmentTokenMint } from '../services/auditService'; import { cloudflareIpRateLimitKeyGenerator } from '../middleware/ipRateLimit'; +import { normalizeAvatarUrl } from '../services/avatarService'; // eslint-disable-next-line global-require const express = require('express'); // eslint-disable-next-line global-require @@ -43,7 +44,6 @@ import { getObjectStore } from '../services/objectStore'; interface AuthReq { userId?: string; ip?: string; - protocol?: string; get?: (header: string) => string | undefined; file?: { originalname: string; mimetype: string; size: number; buffer: Buffer }; params?: { fileName?: string }; @@ -217,23 +217,10 @@ const handleUpload = async ( }); await newFile.save(); - // Cloudflare Tunnel doesn't forward a reliable X-Forwarded-Proto, so - // `app.set('trust proxy')` alone leaves req.protocol stuck at 'http' - // for the cluster-internal hop. Detect the public scheme from explicit - // headers Cloudflare DOES send (cf-visitor.scheme) before falling back - // to the connection protocol. The downstream effect of getting this - // wrong is the URL we emit landing as `http://` and the browser firing - // a Mixed Content warning on every avatar/upload load. - const cfVisitor = req.get?.('cf-visitor'); - let scheme = req.get?.('x-forwarded-proto') || req.protocol || 'http'; - if (cfVisitor) { - try { - const parsed = JSON.parse(cfVisitor); - if (parsed?.scheme) scheme = parsed.scheme; - } catch { /* ignore malformed cf-visitor */ } - } - const host = req.get?.('host'); - const url = `${scheme}://${host}/api/uploads/${fileName}`; + // Stored upload references must survive hostname changes. Callers can + // persist this response directly without baking the current API origin + // into User.profilePicture or package icon metadata. + const url = normalizeAvatarUrl(fileName); res.json({ url, diff --git a/backend/scripts/generate-team-avatars.ts b/backend/scripts/generate-team-avatars.ts index 8e51b728e..fb1b69be8 100644 --- a/backend/scripts/generate-team-avatars.ts +++ b/backend/scripts/generate-team-avatars.ts @@ -26,6 +26,7 @@ import 'dotenv/config'; import mongoose from 'mongoose'; +import { normalizeAvatarUrl } from '../services/avatarService'; // Use require to match the rest of the backend's module idiom. // eslint-disable-next-line @typescript-eslint/no-var-requires @@ -183,7 +184,7 @@ async function run(): Promise { continue; } - user.profilePicture = result.avatar; + user.profilePicture = normalizeAvatarUrl(result.avatar) || 'default'; user.avatarMetadata = { ...(user.avatarMetadata || {}), source: result.metadata?.source || 'unknown', diff --git a/backend/scripts/seed-native-agents.ts b/backend/scripts/seed-native-agents.ts index 3ad342b4f..9767af3dd 100644 --- a/backend/scripts/seed-native-agents.ts +++ b/backend/scripts/seed-native-agents.ts @@ -19,6 +19,7 @@ import mongoose from 'mongoose'; import { AgentRegistry, AgentInstallation } from '../models/AgentRegistry'; import { FIRST_PARTY_APPS } from '../config/native-agents/apps'; import type { NativeAgentDefinition } from '../config/native-agents/apps'; +import { normalizeAvatarUrl } from '../services/avatarService'; // Lazy requires keep this file import-safe even if dependent services move // and avoid pulling the full Mongoose model graph into typecheck. @@ -169,7 +170,7 @@ async function seedOneApp(app: NativeAgentDefinition): Promise { $set: { displayName: app.displayName, description: app.description, - iconUrl: app.iconUrl || '', + iconUrl: normalizeAvatarUrl(app.iconUrl) || '', registry: 'commonly-official', verified: true, status: 'active', diff --git a/backend/services/agentIdentityService.ts b/backend/services/agentIdentityService.ts index e611a25b0..30a9d8eec 100644 --- a/backend/services/agentIdentityService.ts +++ b/backend/services/agentIdentityService.ts @@ -1,5 +1,6 @@ import User from '../models/User'; import Pod from '../models/Pod'; +import { normalizeAvatarUrl } from './avatarService'; let dbPg: { pool: { query: (sql: string, params?: unknown[]) => Promise<{ rows: unknown[] }> } } | null; try { @@ -285,6 +286,7 @@ interface GetOrCreateOptions { runtimeId?: string; capabilities?: string[]; botType?: string; + profilePicture?: string | null; } /** @@ -393,7 +395,7 @@ class AgentIdentityService { email: buildAgentEmail(resolvedType, instanceId), password: `agent-password-${Date.now()}`, verified: true, - profilePicture: 'default', + profilePicture: normalizeAvatarUrl(options.profilePicture) || 'default', role: 'user', isBot: true, botType: typeConfig?.botType || options.botType || 'agent', diff --git a/backend/services/avatarService.ts b/backend/services/avatarService.ts new file mode 100644 index 000000000..3e6ea478d --- /dev/null +++ b/backend/services/avatarService.ts @@ -0,0 +1,81 @@ +import User from '../models/User'; + +type AvatarUser = { + profilePicture?: string | null; +}; + +type AvatarUserRow = AvatarUser & { + _id: unknown; +}; + +const UPLOAD_PATH = '/api/uploads/'; +const LEGACY_COLOR_AVATARS = new Set([ + 'red', + 'purple', + 'blue', + 'teal', + 'green', + 'orange', + 'brown', + 'gray', +]); +const UPLOAD_FILE_NAME = /^[^/?#]+\.[a-z0-9]{2,10}$/i; + +/** + * Canonicalize avatar references without coupling them to an instance origin. + * External absolute URLs are preserved because Commonly cannot safely invent a + * local object key for them; Commonly upload URLs are always made relative. + */ +export function normalizeAvatarUrl(raw: string | null | undefined): string | null { + if (raw == null) return null; + + const value = String(raw).trim(); + if (!value || value === 'default') return null; + // Legacy profiles store color choices in profilePicture. Preserve those + // identifiers for the existing color-avatar renderer; they are not upload + // object keys and must never become `/api/uploads/blue`. + if (LEGACY_COLOR_AVATARS.has(value)) return value; + if (/^data:/i.test(value) || value.startsWith('/')) return value; + + if (/^https?:\/\//i.test(value)) { + try { + const parsed = new URL(value); + const uploadPathIndex = parsed.pathname.indexOf(UPLOAD_PATH); + if (uploadPathIndex >= 0) { + return parsed.pathname.slice(uploadPathIndex); + } + } catch { + return value; + } + return value; + } + + // Older upload responses stored only the object filename. Normalize those, + // but preserve other opaque legacy identifiers rather than inventing a + // broken local upload path for them. + return UPLOAD_FILE_NAME.test(value) ? `${UPLOAD_PATH}${value}` : value; +} + +export function resolveAvatarUrl(user: AvatarUser | null | undefined): string | null { + const normalized = normalizeAvatarUrl(user?.profilePicture); + if (!normalized || LEGACY_COLOR_AVATARS.has(normalized)) return null; + return normalized; +} + +export async function resolveAvatarUrls( + userIds: string[], +): Promise> { + const ids = Array.from(new Set(userIds.map((id) => String(id)))); + const resolved = new Map(ids.map((id) => [id, null])); + if (ids.length === 0) return resolved; + + const users = await User.find({ _id: { $in: ids } }) + .select('_id profilePicture') + .lean(); + + for (const user of users) { + resolved.set(String(user._id), resolveAvatarUrl(user)); + } + + return resolved; +} diff --git a/frontend/src/v2/__tests__/V2Avatar.test.tsx b/frontend/src/v2/__tests__/V2Avatar.test.tsx new file mode 100644 index 000000000..44ea36db8 --- /dev/null +++ b/frontend/src/v2/__tests__/V2Avatar.test.tsx @@ -0,0 +1,37 @@ +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import V2Avatar from '../components/V2Avatar'; + +describe('V2Avatar', () => { + const originalApiUrl = process.env.REACT_APP_API_URL; + + beforeEach(() => { + process.env.REACT_APP_API_URL = 'https://api.commonly.me'; + }); + + afterEach(() => { + if (originalApiUrl === undefined) { + delete process.env.REACT_APP_API_URL; + } else { + process.env.REACT_APP_API_URL = originalApiUrl; + } + }); + + test('resolves canonical relative upload URLs against the API origin', () => { + render(); + + expect(screen.getByRole('img', { name: 'Agent Ada' })).toHaveAttribute( + 'src', + 'https://api.commonly.me/api/uploads/avatar.png', + ); + }); + + test('leaves data URI avatars unchanged', () => { + render(); + + expect(screen.getByRole('img', { name: 'Agent Ada' })).toHaveAttribute( + 'src', + 'data:image/png;base64,avatar', + ); + }); +}); diff --git a/frontend/src/v2/__tests__/V2MessageBubble.test.tsx b/frontend/src/v2/__tests__/V2MessageBubble.test.tsx new file mode 100644 index 000000000..326c64e1d --- /dev/null +++ b/frontend/src/v2/__tests__/V2MessageBubble.test.tsx @@ -0,0 +1,42 @@ +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; +import V2MessageBubble from '../components/V2MessageBubble'; + +jest.mock('../../context/AuthContext', () => ({ + useAuth: () => ({ currentUser: { username: 'viewer' } }), +})); + +describe('V2MessageBubble', () => { + const previousApiUrl = process.env.REACT_APP_API_URL; + + afterEach(() => { + process.env.REACT_APP_API_URL = previousApiUrl; + }); + + it('resolves relative uploaded images against the configured API origin', () => { + process.env.REACT_APP_API_URL = 'https://api.commonly.me'; + + render( + + + , + ); + + expect(screen.getByRole('img', { name: 'Uploaded attachment' })).toHaveAttribute( + 'src', + 'https://api.commonly.me/api/uploads/avatar.png', + ); + expect(screen.getByRole('link')).toHaveAttribute( + 'href', + 'https://api.commonly.me/api/uploads/avatar.png', + ); + }); +}); diff --git a/frontend/src/v2/components/V2Avatar.tsx b/frontend/src/v2/components/V2Avatar.tsx index 14f3b5825..13d872089 100644 --- a/frontend/src/v2/components/V2Avatar.tsx +++ b/frontend/src/v2/components/V2Avatar.tsx @@ -1,4 +1,5 @@ import React from 'react'; +import { normalizeUploadUrl } from '../../utils/apiBaseUrl'; import { colorFor, initialsFor } from '../utils/avatars'; export type V2AvatarSize = 'sm' | 'md' | 'lg'; @@ -26,7 +27,8 @@ const V2Avatar: React.FC = ({ name, src, size = 'md', online, tit const bg = colorFor(seed); const initials = initialsFor(seed); const display = title || seed || undefined; - const cleanSrc = typeof src === 'string' && src.trim().length > 0 ? src.trim() : null; + const rawSrc = typeof src === 'string' && src.trim().length > 0 ? src.trim() : null; + const cleanSrc = normalizeUploadUrl(rawSrc) || null; const [imgFailed, setImgFailed] = React.useState(false); React.useEffect(() => { diff --git a/frontend/src/v2/components/V2MessageBubble.tsx b/frontend/src/v2/components/V2MessageBubble.tsx index a2ef4f5cc..af713ab7e 100644 --- a/frontend/src/v2/components/V2MessageBubble.tsx +++ b/frontend/src/v2/components/V2MessageBubble.tsx @@ -8,6 +8,7 @@ import { formatRelativeTime } from '../utils/grouping'; import { useAuth } from '../../context/AuthContext'; import { useV2Api } from '../hooks/useV2Api'; import { getSignedAttachmentUrl } from '../../utils/signedAttachmentUrl'; +import { normalizeUploadUrl } from '../../utils/apiBaseUrl'; // Minimal v2-scoped markdown renderer. Plain HTML elements (no MUI), so // styling stays in v2.css under `.v2-msg__content`. The body comes pre-stripped @@ -345,9 +346,13 @@ const V2MessageBubble: React.FC = ({ message, isLead, agen const { stripped: noReactions, reactions } = parseReactions(message.content || ''); const { stripped: afterFiles, files } = parseFiles(noReactions); const markdownImage = afterFiles.match(MARKDOWN_IMAGE_RE)?.[1]; - const imageUrl = message.message_type === 'image' || message.messageType === 'image' || IMAGE_URL_RE.test(afterFiles) + const rawImageUrl = message.message_type === 'image' || message.messageType === 'image' || IMAGE_URL_RE.test(afterFiles) ? afterFiles : markdownImage; + // Upload responses use instance-portable `/api/uploads/...` references. + // Resolve them against the configured API origin at render time so the + // browser never requests image bytes from the frontend host. + const imageUrl = rawImageUrl ? normalizeUploadUrl(rawImageUrl) : undefined; // GitHub PR URL detection — if the message body contains a `pull/` URL, // we render an inline preview card below the text. Card fetch is lazy +