File - Migrate avatarUrl > avatarFile on person (data migration + logic) + Attachment data migration

[etiennejouan]

• Migration command

  • Check IS_FILES_FIELD_MIGRATED:false

  • Check or create avatarFile field

  • Fetch all people with avatarUrl
    - Move (Copy/move) file in storage
    - Create core.file record
    - Update person record

  • bonus : attachment migration : fullPath > file (same logic)

• BE logic

  • Add avatarFile field on person

• FE logic

  • Adapt logic to upload on/display avatarFile data

The whole imageIdentifier logic will be done later

[greptile-apps]

Greptile Overview

Greptile Summary

This PR migrates person avatar storage from the legacy avatarUrl field to the new avatarFile field, using the standardized FILES field type.

Key Changes:

• Backend: Added avatarFile field to Person entity with FILES type, marked avatarUrl as deprecated
• Migration Command: Created 1-18-migrate-person-avatar-files.command to copy existing avatar files to new storage location and create FileEntity records
• Frontend: Updated avatar upload/display logic to check IS_FILES_FIELD_MIGRATED feature flag and use appropriate field
• Tests: Updated integration tests to include avatarFile field in GraphQL queries

Critical Issues Found:

• Migration sets size: 0 for all migrated files (line 249 in migration command)
• Migration marks files as isTemporaryFile: true which may cause cleanup jobs to delete avatars (line 250-253)
• Error handling throws but continues with other persons, potentially leaving partial migration state

Implementation Notes:

• The migration properly checks for existing feature flag to avoid re-running
• Frontend uses feature flag branching to maintain backward compatibility during rollout
• Both old and new fields coexist to support gradual migration

Confidence Score: 2/5

• Not safe to merge - migration has critical bugs that will corrupt file metadata
• The migration command contains two critical bugs: it sets file size to 0 bytes and marks migrated avatars as temporary files, which will cause them to be deleted by cleanup jobs. These issues will break avatar display and cause data loss.
• Critical attention needed for packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-person-avatar-files.command.ts - fix file size and isTemporaryFile settings before merging

Important Files Changed

Filename	Overview
packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-person-avatar-files.command.ts	Critical migration logic with potential error handling issues and file size not being set correctly
packages/twenty-front/src/modules/object-record/record-show/hooks/usePersonAvatarUpload.ts	New hook handling avatar upload with proper feature flag branching for backward compatibility
packages/twenty-front/src/modules/object-metadata/utils/getAvatarUrl.ts	Updated to support both avatarUrl and avatarFile fields with feature flag check
packages/twenty-server/src/modules/person/standard-objects/person.workspace-entity.ts	Added avatarFile field with proper deprecation marking on avatarUrl

Sequence Diagram

sequenceDiagram
    participant Admin as Admin/System
    participant MigCmd as Migration Command
    participant FeatureFlag as Feature Flag Service
    participant FileStorage as File Storage
    participant DB as Database (Core + Workspace)
    participant FE as Frontend (React)
    participant User as End User

    Note over Admin,DB: Migration Phase (Backend)
    Admin->>MigCmd: Run upgrade:1-18:migrate-person-avatar-files
    MigCmd->>FeatureFlag: Check IS_FILES_FIELD_MIGRATED
    alt Already Migrated
        FeatureFlag-->>MigCmd: true
        MigCmd-->>Admin: Skip (already completed)
    else Not Migrated
        FeatureFlag-->>MigCmd: false
        MigCmd->>DB: Check/Create avatarFile field metadata
        MigCmd->>DB: Fetch persons with avatarUrl (ILike PersonPicture)
        loop For each person
            MigCmd->>FileStorage: Copy file from old path to new path
            MigCmd->>DB: Create FileEntity record (size=0, isTemporaryFile=true)
            MigCmd->>DB: Update person.avatarFile with fileId
        end
        MigCmd->>FeatureFlag: Enable IS_FILES_FIELD_MIGRATED
        MigCmd-->>Admin: Migration complete
    end

    Note over FE,User: Runtime Phase (Frontend)
    User->>FE: View person record
    FE->>FeatureFlag: Check IS_FILES_FIELD_MIGRATED
    alt Files Field Migrated
        FeatureFlag-->>FE: true
        FE->>FE: Use avatarFile[0].url
        FE->>User: Display avatar from new field
    else Not Migrated
        FeatureFlag-->>FE: false
        FE->>FE: Use avatarUrl with getImageAbsoluteURI
        FE->>User: Display avatar from legacy field
    end

    Note over FE,DB: Upload Phase (Frontend)
    User->>FE: Upload new avatar
    FE->>FeatureFlag: Check IS_FILES_FIELD_MIGRATED
    alt Files Field Migrated
        FeatureFlag-->>FE: true
        FE->>DB: uploadFilesFieldFile mutation
        DB-->>FE: FileEntity with id
        FE->>DB: Update person.avatarFile
    else Not Migrated
        FeatureFlag-->>FE: false
        FE->>DB: uploadImage mutation (PersonPicture folder)
        DB-->>FE: Signed file path
        FE->>DB: Update person.avatarUrl
    end
    FE-->>User: Avatar updated

Loading

[greptile-apps]

_{4 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[cubic-dev-ai]

1 issue found across 32 files

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-person-avatar-files.command.ts">

<violation number="1" location="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-person-avatar-files.command.ts:251">
P1: Migrated avatar files should be marked as permanent (`isTemporaryFile: false`). These files are being migrated from an existing permanent storage location, not uploaded as new temporary files awaiting confirmation. Setting `isTemporaryFile: true` could cause data loss if there's a cleanup process that removes temporary files.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[github-actions]

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:19839

This environment will automatically shut down when the PR is closed or after 5 hours.

[cubic-dev-ai]

1 issue found across 5 files (changes from recent commits).

Prompt for AI agents (all issues)

Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-attachment-files.command.ts">

<violation number="1" location="packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-attachment-files.command.ts:252">
P1: Migrated files are marked as `isTemporaryFile: true`, but these should be permanent files. Temporary file cleanup jobs could inadvertently delete these migrated attachments. Consider setting `isTemporaryFile: false` for permanently migrated data.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[cubic-dev-ai]

P1: Migrated files are marked as isTemporaryFile: true, but these should be permanent files. Temporary file cleanup jobs could inadvertently delete these migrated attachments. Consider setting isTemporaryFile: false for permanently migrated data.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-attachment-files.command.ts, line 252:

<comment>Migrated files are marked as `isTemporaryFile: true`, but these should be permanent files. Temporary file cleanup jobs could inadvertently delete these migrated attachments. Consider setting `isTemporaryFile: false` for permanently migrated data.</comment>

<file context>
@@ -0,0 +1,287 @@
+            applicationId: twentyStandardApplication.id,
+            size: -1,
+            settings: {
+              isTemporaryFile: true,
+              toDelete: false,
+            },
</file context>

[sentry]

Bug: The migrateAttachmentFilesCommand migration script checks for the IS_FILES_FIELD_MIGRATED feature flag but does not set it upon completion, unlike the migratePersonAvatarFilesCommand.
_{Severity: HIGH}

Suggested Fix

Add logic to the end of the migrateAttachmentFilesCommand to set the IS_FILES_FIELD_MIGRATED feature flag to true upon successful completion, mirroring the implementation in migratePersonAvatarFilesCommand. This ensures the migration's state is correctly tracked.

Prompt for AI Agent

Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location:
packages/twenty-server/src/database/commands/upgrade-version-command/1-18/1-18-migrate-attachment-files.command.ts#L59-L62

Potential issue: The `migrateAttachmentFilesCommand` migration script checks for the
`IS_FILES_FIELD_MIGRATED` feature flag to ensure it only runs once, but it does not set
this flag upon completion. The subsequent `migratePersonAvatarFilesCommand` does set the
flag. If the migration process fails after the attachment migration but before the
person avatar migration completes, or if the execution order is changed, the attachment
migration could be skipped on a re-run, leading to an incomplete data migration. This
inconsistency makes the migration process brittle and not truly idempotent.

_{Did we get this right? 👍 / 👎 to inform future reviews.}

[prastoin]

Hey there first step review here

export const extractFileInfo = async ({
  file,
  declaredMimeType,
  filename,
}: {
  file: Buffer;
  declaredMimeType: string | undefined;
  filename: string;
}) => {
  const { ext: declaredExt } = buildFileInfo(filename);

  const detectedFileType = await FileType.fromBuffer(file);

  const mimeType = detectedFileType?.mime ?? declaredMimeType;

  const ext = detectedFileType?.ext ?? declaredExt;

  return {
    mimeType,
    ext,
  };
};

I'm not sure about this here, I wouldn't never trust the declared client data on this. And at the same time I don't know how it would behave if you upload a markdown mime type file but as txt
Unless I'm mistaken there's a possibility to insert a file with a mimetype and file ext fully provided by the client
Maybe never trust the mime type but do for the extension as several can match a single mime type ?
We might wanna dig that a little bit further on what's done usually

EDIT claude response
Great question — this is a nuanced topic. Here's my take:

The core issue

FileType.fromBuffer (magic bytes detection) is the gold standard for binary files (images, PDFs, archives, executables, etc.), but it returns undefined for all text-based formats — .txt, .md, .csv, .json, .html, .css, .yaml, etc. — because these don't have distinctive magic byte signatures.

So the current fallback logic means:

• Binary files: Detected type wins — good, secure.
• Text files: Falls back entirely to client-declared data — that's the risky part.

The dangerous scenario

Someone uploads a text file but declares Content-Type: application/pdf or even application/javascript. Since fromBuffer returns undefined, the code blindly trusts the declared MIME type. Depending on how the file is later served (e.g., Content-Type header on download), this could lead to MIME confusion or XSS if served inline.

The markdown-as-txt case

Not really a security concern. Both are text formats, fromBuffer returns undefined for both, so you'd get the declared values. The worst that happens is a .md file being served as text/plain — harmless.

Industry best practice

The common approach is a three-tier strategy:

1. Prefer magic byte detection when available (binary files).
2. When detection fails, derive MIME type from the extension using a trusted server-side mapping (e.g., the mime-types npm package), rather than trusting the client's declared MIME type.
3. Fall back to the declared extension — this is more acceptable because extensions are less security-sensitive than MIME types (MIME types control how browsers interpret content).

Something like:

import mime from 'mime-types';

export const extractFileInfo = async ({
  file,
  declaredMimeType,
  filename,
}: {
  file: Buffer;
  declaredMimeType: string | undefined;
  filename: string;
}) => {
  const { ext: declaredExt } = buildFileInfo(filename);

  const detectedFileType = await FileType.fromBuffer(file);

  // For extension: prefer detected, fall back to declared (acceptable)
  const ext = detectedFileType?.ext ?? declaredExt;

  // For MIME type: prefer detected, then derive from extension,
  // only trust declared as last resort
  const mimeType =
    detectedFileType?.mime ?? mime.lookup(ext) || declaredMimeType;

  return { mimeType, ext };
};

TL;DR — The reviewer's instinct is right

• Never trust the declared MIME type as a primary source. Derive it from the extension (via a trusted mapping) when magic bytes fail.
• The declared extension is okay as a fallback — it's less security-critical, and as you noted, multiple extensions can map to one MIME type so the client's choice is reasonable.
• An additional layer of defense would be a whitelist of allowed MIME types depending on the upload context, but that's a separate concern.

[prastoin]

Nice job ! Solid
Left few comments on the upgrade commands ! Please let me know

[prastoin]

Question: Have you checked all production occurrences in order to search for any some edge cases ? ( such as no extension etc )