Disallow MP3 Full Server Cleanse, improve metadata-wipe handling, and pin Node runtime#19
Merged
ChrisAdamsdevelopment merged 1 commit intoMay 12, 2026
Conversation
Reviewer's GuideImplements server-side rejection of MP3 files for Full Server Cleanse, hardens the metadata wipe flow by using exiftool.write with proper cleanup and 422 responses on failure, surfaces server-provided detail messages to the client, and documents/pins a compatible Node runtime and new behavior in docs and engine fields. Sequence diagram for updated /api/process MP3 handling and metadata wipe errorssequenceDiagram
actor User
participant BrowserApp
participant ApiServer
participant exiftool
participant fs
User->>BrowserApp: Submit Full Server Cleanse
BrowserApp->>ApiServer: POST /api/process (file)
alt [file is MP3]
ApiServer->>fs: remove(inputPath)
ApiServer-->>BrowserApp: 422 { error, detail }
BrowserApp->>BrowserApp: throw new Error(errBody.detail || errBody.error)
else [non-MP3, supported]
ApiServer->>exiftool: read(outputPath)
ApiServer->>exiftool: write(outputPath, {}, wipeArgs)
ApiServer-->>BrowserApp: 200 cleansed file
else [metadata wipe fails]
ApiServer->>fs: remove(inputPath)
ApiServer->>fs: remove(outputPath)
ApiServer-->>BrowserApp: 422 { error, detail }
BrowserApp->>BrowserApp: throw new Error(errBody.detail || errBody.error)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The MP3 detection in
/api/processonly checks foraudio/mpegand.mp3, so consider broadening this (e.g., handlingaudio/mp3or other common variants) to avoid accidentally allowing MP3s through under alternate MIME types. - When handling the metadata wipe failure, you only log
wipeErr.message; logging or capturing the full error (e.g.,wipeErr.stack) would make debugging unexpected exiftool failures easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The MP3 detection in `/api/process` only checks for `audio/mpeg` and `.mp3`, so consider broadening this (e.g., handling `audio/mp3` or other common variants) to avoid accidentally allowing MP3s through under alternate MIME types.
- When handling the metadata wipe failure, you only log `wipeErr.message`; logging or capturing the full error (e.g., `wipeErr.stack`) would make debugging unexpected exiftool failures easier.
## Individual Comments
### Comment 1
<location path="server.js" line_range="455-458" />
<code_context>
const userId = req.user.sub;
+ const inputPath = req.file.path;
+ const originalName = req.file.originalname || '';
+ const ext = path.extname(originalName).toLowerCase() || '.mp3';
+ const mime = (req.file.mimetype || '').toLowerCase();
+ const isMp3 = ext === '.mp3' || mime === 'audio/mpeg';
+
+ if (isMp3) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Defaulting unknown extensions to `.mp3` risks misclassification and mismatched output extensions.
When `originalname` is empty or has no extension, `path.extname` returns `''`, so `ext` becomes `.mp3`. For non‑MP3 uploads without a usable filename, this can result in `isMp3 === false` (based on MIME) but an `.mp3` output extension, and in other supported formats being rejected because they’re treated as `.mp3`. Consider deriving `ext` from `mimetype` when `originalname` is unusable (e.g., `audio/wav` → `.wav`, `audio/flac` → `.flac`), and only then falling back to a neutral extension rather than `.mp3`, to avoid reintroducing MP3‑specific assumptions.
</issue_to_address>
### Comment 2
<location path="docs/manual-qa-checklist.md" line_range="103" />
<code_context>
+1. Run server cleanse on a supported non-MP3 file.
2. Verify `/api/process` returns downloadable file response.
-3. Verify usage meter/counter updates.
+3. Verify usage meter/counter updates only after successful delivered files.
4. Verify forensic/report information appears when present.
-5. Force or simulate `401` from protected endpoint.
</code_context>
<issue_to_address>
**nitpick (typo):** Rephrase "successful delivered files" to improve grammar and clarity.
Consider rephrasing to something like "updates only after files are successfully delivered" or "updates only after successful file delivery" for clearer, grammatical wording.
```suggestion
3. Verify usage meter/counter updates only after files are successfully delivered.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+455
to
+458
| const originalName = req.file.originalname || ''; | ||
| const ext = path.extname(originalName).toLowerCase() || '.mp3'; | ||
| const mime = (req.file.mimetype || '').toLowerCase(); | ||
| const isMp3 = ext === '.mp3' || mime === 'audio/mpeg'; |
There was a problem hiding this comment.
issue (bug_risk): Defaulting unknown extensions to .mp3 risks misclassification and mismatched output extensions.
When originalname is empty or has no extension, path.extname returns '', so ext becomes .mp3. For non‑MP3 uploads without a usable filename, this can result in isMp3 === false (based on MIME) but an .mp3 output extension, and in other supported formats being rejected because they’re treated as .mp3. Consider deriving ext from mimetype when originalname is unusable (e.g., audio/wav → .wav, audio/flac → .flac), and only then falling back to a neutral extension rather than .mp3, to avoid reintroducing MP3‑specific assumptions.
| 1. Run server cleanse on a supported non-MP3 file. | ||
| 2. Verify `/api/process` returns downloadable file response. | ||
| 3. Verify usage meter/counter updates. | ||
| 3. Verify usage meter/counter updates only after successful delivered files. |
There was a problem hiding this comment.
nitpick (typo): Rephrase "successful delivered files" to improve grammar and clarity.
Consider rephrasing to something like "updates only after files are successfully delivered" or "updates only after successful file delivery" for clearer, grammatical wording.
Suggested change
| 3. Verify usage meter/counter updates only after successful delivered files. | |
| 3. Verify usage meter/counter updates only after files are successfully delivered. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
detailmessages to the client when a format is unsupported or metadata wipe fails.better-sqlite3).Description
/api/processand return HTTP422witherroranddetailexplaining that MP3s must use Quick Cleanse (Browser), while allowing MP4/M4A/WAV/FLAC for Full Server Cleanse, and remove the uploaded temp file when rejecting.exiftool.writepath and, on failure, clean up uploaded files and return HTTP422indicating the format is unsupported for server-side cleanse.detailfields on client errors by falling back toerrBody.detailinapp.tsxerror handling.>=18 <23, add.node-versionset to20.20.2, and add README guidance about using Node20.20.2for native deployments and that Node 24 is unsupported for native installs.docs/manual-qa-checklist.mdto reflect the new MP3 rejection behavior and clarify usage-meter semantics; updatepackage.jsonandpackage-lock.jsonengine fields accordingly.Testing
npm run buildto compile TypeScript and build the frontend, and the build completed successfully.Codex Task
Summary by Sourcery
Disallow server-side MP3 full cleanse, tighten server metadata-wipe handling, and pin Node runtime compatibility.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation: