Skip to content

security(api): confine file endpoints to managed downloads#91

Merged
KyleTryon merged 3 commits into
mainfrom
codex/fix-file-path-confinement
Jun 19, 2026
Merged

security(api): confine file endpoints to managed downloads#91
KyleTryon merged 3 commits into
mainfrom
codex/fix-file-path-confinement

Conversation

@KyleTryon

Copy link
Copy Markdown
Contributor

Summary

  • restrict file download/delete requests to paths under the configured downloads directory
  • require requested files to match stored download-file records before serving or deleting
  • add regression coverage for managed file serving and rejected unmanaged paths

Validation

  • uv run black app/api/v1/endpoints/files.py app/db/repositories.py tests/test_api/test_files.py
  • uv run ruff check app/api/v1/endpoints/files.py app/db/repositories.py tests/test_api/test_files.py
  • uv run pytest tests/test_api/test_files.py tests/test_api/test_downloads.py

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 41.17647% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.47%. Comparing base (1ec3045) to head (8f36096).
⚠️ Report is 49 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
packages/hermes-api/app/api/v1/endpoints/files.py 43.90% 23 Missing ⚠️
packages/hermes-api/app/db/repositories.py 30.00% 7 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #91       +/-   ##
===========================================
+ Coverage   40.59%   64.47%   +23.87%     
===========================================
  Files         145       53       -92     
  Lines        7119     3631     -3488     
  Branches     1206        0     -1206     
===========================================
- Hits         2890     2341      -549     
+ Misses       4222     1290     -2932     
+ Partials        7        0        -7     
Flag Coverage Δ
backend 64.47% <41.17%> (+7.29%) ⬆️
frontend ?
Components Coverage Δ
Frontend (hermes-app) ∅ <ø> (∅)
Backend (hermes-api) 64.47% <74.21%> (+7.29%) ⬆️
Files with missing lines Coverage Δ
packages/hermes-api/app/db/repositories.py 49.05% <30.00%> (+8.65%) ⬆️
packages/hermes-api/app/api/v1/endpoints/files.py 43.90% <43.90%> (+23.02%) ⬆️

... and 90 files with indirect coverage changes

@KyleTryon KyleTryon marked this pull request as ready for review June 19, 2026 23:32
logger.warning(f"File not found: {file_path}")

await repos["download_files"].delete(managed_file.id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The delete_downloaded_files function deletes database records even when the physical file is not found, leading to an inconsistent state where a failure is reported but a partial success occurs.
Severity: MEDIUM

Suggested Fix

Move the database deletion logic, including the calls to repos["download_files"].delete() and repos["downloads"].delete(), inside the if resolved_path.exists(): block. This will ensure database records are only removed after the corresponding physical file has been successfully deleted.

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/hermes-api/app/api/v1/endpoints/files.py#L232

Potential issue: In the `delete_downloaded_files` function, when a requested file path
does not exist on disk, the operation is correctly added to the `failed_files` list.
However, the code proceeds to delete the associated `DownloadFile` and `Download`
records from the database regardless. This creates an inconsistent state where the API
caller is informed of a failure, but the database is modified as if the operation was
successful. This can occur if a file is removed from the disk by an external process.

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

@KyleTryon KyleTryon merged commit d02d64d into main Jun 19, 2026
9 checks passed
@KyleTryon KyleTryon deleted the codex/fix-file-path-confinement branch June 19, 2026 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant