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

Merged

jfro merged 2 commits from jtk/export-path-traversal into main

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

jfro commented

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

Owner

Closes #31 (release blocker).

Bug: Job.export_changeset/import_changeset cast file_path, file_name, file_size, user_id and progress fields from the form. A forged job[file_path] on a failed export stayed on the job, and deleting the job ran File.rm(Path.join(base, job.file_path)), so ../.. deleted any file the app could write. cleanup_expired_jobs called File.rm(job.file_path) with no base at all.

Fix

export_changeset and import_changeset cast only :filters. user_id is set on the struct, and an import's file fields come from a new file_info argument: create_import_job(user, file_info, attrs \\ %{}). The controller no longer merges form params into it.
New ImportExport.job_file_path/1 resolves a job's file and returns :error (with a Logger.warning carrying the job id) unless it's inside <storage base>/exports/ for exports or the system temp dir for imports. delete_job_file/1 wraps it. Controller delete/2 and download/2 and cleanup_expired_jobs/0 all go through it, so expired exports' files now really get removed (before, the relative key was never joined to the base).
Import temp-file deletes aren't widened: they're now confined to the temp dir.

Tests

Creating an export ignores a forged file_path/file_name/file_size/user_id/status (context and controller).
Deleting a job whose file_path is ../sentinel or an absolute path leaves the sentinel file intact. So does cleanup_expired_jobs, which still removes a real expired export.
Downloading a forged completed export is refused.
Happy path: a real JSON export completes, downloads, and its file is removed on delete.

mix precommit: 482 passed, 4 skipped.

Out of scope (existing behaviour): with the S3 storage backend, export download/delete still look for a local file.

Closes #31 (release blocker). **Bug:** `Job.export_changeset`/`import_changeset` cast `file_path`, `file_name`, `file_size`, `user_id` and progress fields from the form. A forged `job[file_path]` on a failed export stayed on the job, and deleting the job ran `File.rm(Path.join(base, job.file_path))`, so `../..` deleted any file the app could write. `cleanup_expired_jobs` called `File.rm(job.file_path)` with no base at all. **Fix** - `export_changeset` and `import_changeset` cast only `:filters`. `user_id` is set on the struct, and an import's file fields come from a new `file_info` argument: `create_import_job(user, file_info, attrs \\ %{})`. The controller no longer merges form params into it. - New `ImportExport.job_file_path/1` resolves a job's file and returns `:error` (with a `Logger.warning` carrying the job id) unless it's inside `<storage base>/exports/` for exports or the system temp dir for imports. `delete_job_file/1` wraps it. Controller `delete/2` and `download/2` and `cleanup_expired_jobs/0` all go through it, so expired exports' files now really get removed (before, the relative key was never joined to the base). - Import temp-file deletes aren't widened: they're now confined to the temp dir. **Tests** - Creating an export ignores a forged `file_path`/`file_name`/`file_size`/`user_id`/`status` (context and controller). - Deleting a job whose `file_path` is `../sentinel` or an absolute path leaves the sentinel file intact. So does `cleanup_expired_jobs`, which still removes a real expired export. - Downloading a forged completed export is refused. - Happy path: a real JSON export completes, downloads, and its file is removed on delete. `mix precommit`: 482 passed, 4 skipped. Out of scope (existing behaviour): with the S3 storage backend, export download/delete still look for a local file.

jfro added the

main

release-blocker

security

labels

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

jfro added 2 commits

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

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

The export and import job changesets cast file_path, file_name, file_size,
user_id and progress fields from the form, so a forged job[file_path] on a
failed export survived, and deleting the job then removed any file the app
could write (path traversal). Expired-job cleanup ran File.rm on the raw
path with no base at all.

- export_changeset/import_changeset only cast :filters; user_id and the
  upload's file fields are set on the struct by server code.
- ImportExport.job_file_path/1 resolves a job's file and refuses anything
  outside <storage>/exports (exports) or the temp dir (imports), with a
  warning; delete, download and cleanup_expired_jobs all go through it.

Closes #31

Merge remote-tracking branch 'origin/main' into jtk/export-path-traversal

CI / test (pull_request) Successful in 54s

Details

3b16033a7a

jfro reviewed

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

jfro left a comment

Review from Nye: APPROVE (posted by Upper Wolfjaw, since Forgejo won't let jfro approve its own PR)

Looks good. The export/import changesets cast only :filters; user_id and file fields are set on the struct. Checked every caller: the exporter's complete_job writes the relative exports/<file> key that Storage.store_file returns, and importer/controller update_job only set status/total/filters. job_file_path/1 is correct: Path.expand(path, base) handles both relative export keys and absolute import paths. The root <> "/" prefix check refuses the bare exports dir, sibling dirs like exports2/, and ../. Cleanup, delete and download all go through it, and the tests cover the forged form path, traversal on delete/cleanup/download, and the happy path.

Non-blocking notes: (a) Path.expand doesn't resolve symlinks, which is fine now that the path is server-set. (b) job_file_root assumes the local backend, as the old code did.

**Review from Nye: APPROVE** (posted by Upper Wolfjaw, since Forgejo won't let jfro approve its own PR) Looks good. The export/import changesets cast only `:filters`; user_id and file fields are set on the struct. Checked every caller: the exporter's complete_job writes the relative `exports/<file>` key that Storage.store_file returns, and importer/controller update_job only set status/total/filters. `job_file_path/1` is correct: `Path.expand(path, base)` handles both relative export keys and absolute import paths. The `root <> "/"` prefix check refuses the bare `exports` dir, sibling dirs like `exports2/`, and `../`. Cleanup, delete and download all go through it, and the tests cover the forged form path, traversal on delete/cleanup/download, and the happy path. Non-blocking notes: (a) Path.expand doesn't resolve symlinks, which is fine now that the path is server-set. (b) job_file_root assumes the local backend, as the old code did.

jfro merged commit 6a8a1c1098 into main

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

jfro deleted branch jtk/export-path-traversal

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

jfro referenced this pull request from a commit

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

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

jfro referenced this pull request

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

Export files can be downloaded without auth through public /uploads/exports #35