Fix: Move files via PUT /files/:uuid bypasses mission checks

[wp99cp]

Closes #1915. Fixes Vulnerability C where moving files via PUT /files/:uuid bypassed mission checks and allowed API keys/users to move files to missions they do not have CREATE access to. Added integration test cases.

[greptile-apps]

Greptile Summary

This PR closes Vulnerability C by adding a checkMovePermission gate inside FileLifecycleService.update that runs whenever a PUT /files/:uuid request attempts to change a file's mission, and passes the full ApiKeyEntity through the controller so the new check can inspect the key's scope.

• API-key branch: throws ForbiddenException when the destination mission does not match apiKey.mission.uuid, or when the key lacks CREATE rights — intended to prevent keys from moving files to arbitrary missions.
• User branch: delegates to MissionGuardService.canAccessMission with CREATE rights on the destination mission, mirroring the existing @CanMoveFiles() guard used by the bulk-move endpoint.
• Tests: two integration tests are added covering the blocked cross-mission API-key move and the permitted same-mission filename update.

Confidence Score: 3/5

The core permission gate works for the most obvious attack path (API key moving to an unrelated mission), but a cross-mission move from a sibling mission in the same project remains possible for API keys with CREATE rights, which is the scenario this fix was meant to close.

The checkMovePermission correctly blocks API keys from moving files to arbitrary foreign missions, but FileGuardService.canKeyAccessFile grants write access at the project level rather than the mission level. An API key for mission B with CREATE rights can therefore pass @CanWriteFile() on a file residing in mission A (same project) and then satisfy checkMovePermission because the destination equals the key's own mission — completing a cross-mission A→B move. This is a present defect in the changed security path, not a future possibility.

backend/src/services/file-lifecycle.service.ts — specifically the API-key branch of checkMovePermission; the source-mission ownership of the file is never asserted against the API key's mission.

Security Review

• Residual cross-mission move via API key (file-lifecycle.service.ts checkMovePermission lines 615–625): FileGuardService.canKeyAccessFile grants write access at the project level (checks apiKey.mission.project.uuid === file.mission.project.uuid), not the mission level. An API key for mission B (CREATE rights) can therefore pass @CanWriteFile() on a file currently in mission A (same project) and then pass checkMovePermission because the destination equals the key's own mission — completing a cross-mission A→B move that the fix was meant to block.

Important Files Changed

Filename	Overview
backend/src/services/file-lifecycle.service.ts	Adds checkMovePermission guard for cross-mission file moves; the API-key branch only validates the destination mission, leaving a residual bypass where an API key for mission B can move files from mission A (same project) to mission B via project-level write access.
backend/src/endpoints/file/file.controller.ts	Passes the full auth.apiKey entity (in addition to the action) to FileLifecycleService.update, enabling the new permission check downstream — change is correct and minimal.
backend/tests/actions/api-key-scope.test.ts	Adds two integration tests: one confirming an action API key cannot move a file to a sibling mission, and one confirming non-move updates still work; user-path coverage for the new check is absent.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant C as Caller (user / API key)
    participant Ctrl as FileController PUT /files/:uuid
    participant Guard as FileAccessGuard @CanWriteFile()
    participant FGS as FileGuardService
    participant Svc as FileLifecycleService
    participant CMP as checkMovePermission()
    participant MGS as MissionGuardService

    C->>Ctrl: "PUT /files/:uuid {missionUuid: newMission}"
    Ctrl->>Guard: canActivate()
    alt API key present
        Guard->>FGS: canKeyAccessFile(apiKey, fileUUID, WRITE)
        Note over FGS: checks project-level match only
        FGS-->>Guard: true/false
    else user present
        Guard->>FGS: canAccessFile(user, fileUUID, WRITE)
        FGS-->>Guard: true/false
    end
    Guard-->>Ctrl: allowed
    Ctrl->>Svc: update(uuid, dto, user, action, apiKey)
    alt missionUuid changed
        Svc->>CMP: checkMovePermission(newMissionUuid, actor, apiKey)
        alt API key present
            CMP->>CMP: "newMissionUuid !== apiKey.mission.uuid → 403"
            CMP->>CMP: "apiKey.rights < CREATE → 403"
            Note over CMP: Gap - source mission not checked
        else user present
            CMP->>MGS: canAccessMission(user, newMissionUuid, CREATE)
            MGS-->>CMP: true/false
        else neither
            CMP->>CMP: throw 403
        end
    end
    Svc-->>Ctrl: updated FileEntity
    Ctrl-->>C: 200 FileDto

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant C as Caller (user / API key)
    participant Ctrl as FileController PUT /files/:uuid
    participant Guard as FileAccessGuard @CanWriteFile()
    participant FGS as FileGuardService
    participant Svc as FileLifecycleService
    participant CMP as checkMovePermission()
    participant MGS as MissionGuardService

    C->>Ctrl: "PUT /files/:uuid {missionUuid: newMission}"
    Ctrl->>Guard: canActivate()
    alt API key present
        Guard->>FGS: canKeyAccessFile(apiKey, fileUUID, WRITE)
        Note over FGS: checks project-level match only
        FGS-->>Guard: true/false
    else user present
        Guard->>FGS: canAccessFile(user, fileUUID, WRITE)
        FGS-->>Guard: true/false
    end
    Guard-->>Ctrl: allowed
    Ctrl->>Svc: update(uuid, dto, user, action, apiKey)
    alt missionUuid changed
        Svc->>CMP: checkMovePermission(newMissionUuid, actor, apiKey)
        alt API key present
            CMP->>CMP: "newMissionUuid !== apiKey.mission.uuid → 403"
            CMP->>CMP: "apiKey.rights < CREATE → 403"
            Note over CMP: Gap - source mission not checked
        else user present
            CMP->>MGS: canAccessMission(user, newMissionUuid, CREATE)
            MGS-->>CMP: true/false
        else neither
            CMP->>CMP: throw 403
        end
    end
    Svc-->>Ctrl: updated FileEntity
    Ctrl-->>C: 200 FileDto

Loading

Comments Outside Diff (1)

1. backend/tests/actions/api-key-scope.test.ts, line 296-298 (link)

   Missing user-path coverage for new permission check

   The two new test cases only exercise the API-key branch of checkMovePermission. There is no integration test verifying that a regular authenticated user who lacks CREATE access on the destination mission receives a 403 when calling PUT /files/:uuid with a different missionUuid. The user branch of checkMovePermission calls missionGuardService.canAccessMission but goes completely untested, so a regression there would be silent.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: backend/tests/actions/api-key-scope.test.ts
   Line: 296-298
   
   Comment:
   **Missing user-path coverage for new permission check**
   
   The two new test cases only exercise the API-key branch of `checkMovePermission`. There is no integration test verifying that a regular authenticated *user* who lacks CREATE access on the destination mission receives a 403 when calling `PUT /files/:uuid` with a different `missionUuid`. The user branch of `checkMovePermission` calls `missionGuardService.canAccessMission` but goes completely untested, so a regression there would be silent.
   
   How can I resolve this? If you propose a fix, please make it concise.

   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!

Prompt To Fix All With AI

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
backend/src/services/file-lifecycle.service.ts:615-625
**Incomplete API-key mission-move restriction**

`checkMovePermission` only validates the *destination* — it allows the move when `newMissionUuid === apiKey.mission.uuid`. However, `FileGuardService.canKeyAccessFile` (the gate for `@CanWriteFile()`) grants project-level access: it passes when `apiKey.mission.project.uuid === file.mission.project.uuid`, regardless of which mission the file currently belongs to. So an API key for mission B (with CREATE rights) can pass the `@CanWriteFile()` guard on a file that lives in mission A (same project), then pass `checkMovePermission` because the destination is mission B — completing a cross-mission move from A→B that should be forbidden.

The simplest safe fix: API keys should never be permitted to change a file's mission at all, since the only possible "allowed" destination (their own mission) is always a cross-mission operation from the source's perspective. Rejecting all API-key-initiated moves unconditionally, or additionally asserting that the file's current mission matches the API key's mission before entering this path, closes the gap.

### Issue 2 of 2
backend/tests/actions/api-key-scope.test.ts:296-298
**Missing user-path coverage for new permission check**

The two new test cases only exercise the API-key branch of `checkMovePermission`. There is no integration test verifying that a regular authenticated *user* who lacks CREATE access on the destination mission receives a 403 when calling `PUT /files/:uuid` with a different `missionUuid`. The user branch of `checkMovePermission` calls `missionGuardService.canAccessMission` but goes completely untested, so a regression there would be silent.

_{Reviews (1): Last reviewed commit: "Refactor file move permission checks int..." | Re-trigger Greptile}

[greptile-apps]

Incomplete API-key mission-move restriction

checkMovePermission only validates the destination — it allows the move when newMissionUuid === apiKey.mission.uuid. However, FileGuardService.canKeyAccessFile (the gate for @CanWriteFile()) grants project-level access: it passes when apiKey.mission.project.uuid === file.mission.project.uuid, regardless of which mission the file currently belongs to. So an API key for mission B (with CREATE rights) can pass the @CanWriteFile() guard on a file that lives in mission A (same project), then pass checkMovePermission because the destination is mission B — completing a cross-mission move from A→B that should be forbidden.

The simplest safe fix: API keys should never be permitted to change a file's mission at all, since the only possible "allowed" destination (their own mission) is always a cross-mission operation from the source's perspective. Rejecting all API-key-initiated moves unconditionally, or additionally asserting that the file's current mission matches the API key's mission before entering this path, closes the gap.

Prompt To Fix With AI

This is a comment left during a code review.
Path: backend/src/services/file-lifecycle.service.ts
Line: 615-625

Comment:
**Incomplete API-key mission-move restriction**

`checkMovePermission` only validates the *destination* — it allows the move when `newMissionUuid === apiKey.mission.uuid`. However, `FileGuardService.canKeyAccessFile` (the gate for `@CanWriteFile()`) grants project-level access: it passes when `apiKey.mission.project.uuid === file.mission.project.uuid`, regardless of which mission the file currently belongs to. So an API key for mission B (with CREATE rights) can pass the `@CanWriteFile()` guard on a file that lives in mission A (same project), then pass `checkMovePermission` because the destination is mission B — completing a cross-mission move from A→B that should be forbidden.

The simplest safe fix: API keys should never be permitted to change a file's mission at all, since the only possible "allowed" destination (their own mission) is always a cross-mission operation from the source's perspective. Rejecting all API-key-initiated moves unconditionally, or additionally asserting that the file's current mission matches the API key's mission before entering this path, closes the gap.

How can I resolve this? If you propose a fix, please make it concise.