Export job file_path is mass-assignable, so deleting the job deletes any file #31

New issue

Closed

opened 2026-10-18 17:02:23 +00:00 by jfro · 0 comments

jfro commented

2026-10-18 17:02:23 +00:00

Owner

Severity: medium (release blocker per Haystack) · found by Nye

Job.export_changeset (lib/fuzzy_catalog/import_export/job.ex) casts :file_path, :file_name, :file_size, :progress, :total_items, :processed_items, :error_message and :user_id from the form's job params, and create_export passes job_params straight in.

POST job[file_path]=../../../config/runtime.exs with job[filters][format]=bogus: the export fails (exporter.ex ~L56) and the forged file_path stays on the job. (A completed export overwrites file_path, but a failed or still-running one doesn't.)

DELETE /admin/import-export/:id then runs File.rm(Path.join(base_path, job.file_path)), so path traversal deletes any file the app user can write.
ImportExport.cleanup_expired_jobs is worse: File.rm(job.file_path) with no base at all.
download/2 reads from the same joined path.

Admin-only once #17 lands (#8); any logged-in user before that.

Fix

export_changeset casts only :filters; set user_id and file fields with put_change from server code. Internal progress updates use a separate internal changeset.
One helper that resolves a job's file path and refuses anything outside the storage dir (Path.expand + prefix check), used by delete, download and expired-job cleanup, with a Logger.warning (job id) on rejection.
Don't widen import temp-file deletes.
Tests: forged job[file_path] is ignored; delete and cleanup of a job with file_path: "../x" leave a sentinel file outside the base intact.

**Severity: medium (release blocker per Haystack)** · found by Nye `Job.export_changeset` (`lib/fuzzy_catalog/import_export/job.ex`) casts `:file_path`, `:file_name`, `:file_size`, `:progress`, `:total_items`, `:processed_items`, `:error_message` and `:user_id` from the form's job params, and `create_export` passes `job_params` straight in. POST `job[file_path]=../../../config/runtime.exs` with `job[filters][format]=bogus`: the export fails (exporter.ex ~L56) and the forged `file_path` stays on the job. (A completed export overwrites `file_path`, but a failed or still-running one doesn't.) - `DELETE /admin/import-export/:id` then runs `File.rm(Path.join(base_path, job.file_path))`, so path traversal deletes any file the app user can write. - `ImportExport.cleanup_expired_jobs` is worse: `File.rm(job.file_path)` with no base at all. - `download/2` reads from the same joined path. Admin-only once #17 lands (#8); any logged-in user before that. **Fix** 1. `export_changeset` casts only `:filters`; set `user_id` and file fields with `put_change` from server code. Internal progress updates use a separate internal changeset. 2. One helper that resolves a job's file path and refuses anything outside the storage dir (`Path.expand` + prefix check), used by delete, download and expired-job cleanup, with a `Logger.warning` (job id) on rejection. 3. Don't widen import temp-file deletes. 4. Tests: forged `job[file_path]` is ignored; delete and cleanup of a job with `file_path: "../x"` leave a sentinel file outside the base intact.

jfro added the

labels

2026-10-18 17:02:23 +00:00

jfro referenced this issue from a pull request that will close it,

2026-10-18 17:10:49 +00:00

fix: keep import/export job file paths inside their storage dirs #32

jfro closed this issue

2026-10-18 17:16:35 +00:00