fix(roms): enforce parent ROM visibility on direct file endpoints#3703
Conversation
The direct ROM-file endpoints (`get_romfile` and `get_romfile_content`) resolved a RomFile purely by its user-supplied id and never mapped it back to the parent Rom, so an authenticated user with roms.read could read or download a file of a ROM hidden from them by knowing/guessing the RomFile.id, bypassing the hidden ROM/platform policy. Resolve the parent Rom and apply assert_rom_visible (404-masking) before returning any file bytes or metadata, matching the ROM-level content endpoints and the existing check in patch.py. Generated-By: PostHog Code Task-Id: 149c191c-7a22-4cf6-aafd-4dcf53f4fba0
Greptile SummaryThis PR adds parent-ROM visibility checks to direct ROM file endpoints. The main changes are:
Confidence Score: 4/5The authenticated path looks fixed, but anonymous download mode can still serve hidden ROM files by direct RomFile ID.
backend/endpoints/roms/files.py
|
| Filename | Overview |
|---|---|
| backend/endpoints/roms/files.py | Adds parent ROM lookup and visibility checks before direct file metadata and content responses. |
| backend/tests/endpoints/roms/test_rom.py | Adds tests for visible and hidden ROM behavior on direct RomFile endpoints. |
Reviews (1): Last reviewed commit: "fix(roms): enforce parent ROM visibility..." | Re-trigger Greptile
There was a problem hiding this comment.
Pull request overview
This pull request closes a visibility bypass in the ROM file endpoints by ensuring requests for a RomFile are gated by the parent Rom’s hidden/platform visibility policy (404-masked), aligning behavior with the ROM-level endpoints.
Changes:
- Resolve the parent
Romfor direct ROM-file endpoints and enforceassert_rom_visible(..., not_found_detail="File not found")before returning metadata or content. - Add regression tests to confirm hidden ROMs’ files return 404 for both content and metadata endpoints.
- Add a small test helper to mark a ROM hidden for a specific user via
HiddenEntity.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| backend/endpoints/roms/files.py | Enforces parent ROM visibility (404-masked) on direct RomFile ID endpoints before serving metadata/content. |
| backend/tests/endpoints/roms/test_rom.py | Adds regression tests ensuring hidden ROM file content/metadata endpoints return 404; includes helper to hide a ROM for a user. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
The direct ROM-file endpoints in
backend/endpoints/roms/files.pyresolved aRomFilepurely by its user-suppliedidand never mapped it back to the parentRom, so they skipped the hidden ROM/platform visibility policy:GET /api/roms/{id}/files/content/{file_name}(get_romfile_content) served the file bytes (via nginxX-Accel-Redirectin production).GET /api/roms/{id}/files(get_romfile) returned the file metadata.An authenticated user with only
roms.readcould download or inspect a file belonging to a ROM hidden from them by knowing or guessing theRomFile.id, even though the ROM-level endpoints (/api/roms/{id},/api/roms/{id}/content/...) correctly return 404 for hidden ROMs.Fix
Both handlers now resolve the parent
Romand callassert_rom_visible(request, rom, not_found_detail="File not found")before returning anything. This is the same 404-masking guard the ROM-level content endpoints use, and it matches the pattern already present inpatch.py(_resolve_library_patch).assert_rom_visibleno-ops for unauthenticated callers, so theDISABLE_DOWNLOAD_ENDPOINT_AUTHdownload path is unaffected.Tests
Added three regression tests in
tests/endpoints/roms/test_rom.py:test_get_romfile_content_visible_rom- baseline, a viewer can download a visible ROM's file byRomFile.id(200 +X-Accel-Redirect).test_get_romfile_content_hidden_rom_returns_404- a hidden ROM's file bytes now 404 by directRomFile.id.test_get_romfile_hidden_rom_returns_404- the metadata endpoint no longer leaks a hidden ROM's file.All 137 tests in the roms + permissions-resolver suites pass, and
trunk checkis clean.AI assistance disclosure
This change was authored with AI assistance (PostHog Code / Claude): investigation of the affected endpoints, the fix, and the regression tests were AI-generated and human-reviewed.
Checklist
Created with PostHog Code