[Security] Campaign import reads data.json with no cap on its decompressed size #1054

Closed
opened 2026-10-03 23:29:21 +00:00 by claude-bot · 5 comments
Collaborator

Found in the privacy review of #991 part 1. It predates that work and applies to every export schema version.

What

import_service reads data.json out of the uploaded zip with no limit on its decompressed size. Only the upload itself is capped, at 100 MB. A small, highly compressible file can therefore expand to far more than the backend's memory limit (512 MB, see #841) and take the worker down: a zip bomb.

Who can do it

Whoever may import a campaign: a signed-in user on New campaign's import, and an administrator on Admin → Import a campaign. It is not reachable signed out.

Fix

  • Refuse before reading: check the zip entry's declared file_size against a cap, and read through a bounded reader so a lying header cannot get past it.
  • Apply the same to any other entry the import reads (media, once #991 part 2 brings files).
  • The error is the usual 422 {"detail": ...}, in words: the file is too large to import.

Why now

Schema 17 (#991 part 1) stores more JSON as it arrives, and part 2 adds binary files, which is where a per-file and a total limit (the owner's Q145) have to be decided anyway. The data.json cap does not need that answer: a generous fixed bound is enough, and no genuine export comes near it.

Related: a sheet row's name and a sheet's data have no import limit beyond the row and roll counts.

Found in the privacy review of #991 part 1. It predates that work and applies to every export schema version. ## What `import_service` reads `data.json` out of the uploaded zip with no limit on its decompressed size. Only the upload itself is capped, at 100 MB. A small, highly compressible file can therefore expand to far more than the backend's memory limit (512 MB, see #841) and take the worker down: a zip bomb. ## Who can do it Whoever may import a campaign: a signed-in user on New campaign's import, and an administrator on Admin → Import a campaign. It is not reachable signed out. ## Fix - Refuse before reading: check the zip entry's declared `file_size` against a cap, and read through a bounded reader so a lying header cannot get past it. - Apply the same to any other entry the import reads (media, once #991 part 2 brings files). - The error is the usual 422 `{"detail": ...}`, in words: the file is too large to import. ## Why now Schema 17 (#991 part 1) stores more JSON as it arrives, and part 2 adds binary files, which is where a per-file and a total limit (the owner's Q145) have to be decided anyway. The `data.json` cap does not need that answer: a generous fixed bound is enough, and no genuine export comes near it. Related: a sheet row's name and a sheet's `data` have no import limit beyond the row and roll counts.
Author
Collaborator

Progress, and a finding that keeps this open. The work is on feat/991-archive-on-disk (PR A of #991 part 2), not yet a PR.

What the branch does

  • The upload is spooled to disk and every read from the archive is bounded: upload size (an Admin setting, 1 GB by default, 100 MB to 4 GB), total decompressed bytes, data.json size, entry count, each picture.
  • A 1,000 MB import through nginx peaked at 349 MB in the 512 MB backend, with no OOM kill.

The finding
A data.json inside every bound can still get a backend worker OOM-killed:

  • The file: 32 MB, one long string with a single emoji in it. The upload is about 32 KB compressed.
  • Why: Python holds that text at four bytes a character, twice (the decoded text, then the parsed value). Measured extra peak: +193 MB for the string alone, +378 MB for the worst mix inside both bounds.
  • Headroom: a warm backend sits at about 336 MB of 512 MB.
  • Seen on the real stack: a 502 to the client, oom_kill 1, and the worker restarted.

Decision taken

  • Lower the caps: data.json to 12 MB (8 MB if the measurement says so) and 500,000 values. Production's whole database is 17 MB across 2 campaigns, so no real export is refused.
  • The worst case at the new caps is being measured in a 512 MB container before the PR opens.

What stays open here

  • Parse the import in the worker (1 GB), as a background import. That is the real fix and what would let the caps rise.
  • For the owner: raising the backend's mem_limit to 1g is the alternative. It changes what each host needs, so I have not done it.

The PR will say it addresses this issue, not that it closes it.

**Progress, and a finding that keeps this open.** The work is on `feat/991-archive-on-disk` (PR A of #991 part 2), not yet a PR. **What the branch does** - The upload is spooled to disk and every read from the archive is bounded: upload size (an Admin setting, 1 GB by default, 100 MB to 4 GB), total decompressed bytes, `data.json` size, entry count, each picture. - A 1,000 MB import through nginx peaked at 349 MB in the 512 MB backend, with no OOM kill. **The finding** A `data.json` inside every bound can still get a backend worker OOM-killed: - **The file:** 32 MB, one long string with a single emoji in it. The upload is about 32 KB compressed. - **Why:** Python holds that text at four bytes a character, twice (the decoded text, then the parsed value). Measured extra peak: +193 MB for the string alone, +378 MB for the worst mix inside both bounds. - **Headroom:** a warm backend sits at about 336 MB of 512 MB. - **Seen on the real stack:** a 502 to the client, `oom_kill 1`, and the worker restarted. **Decision taken** - Lower the caps: `data.json` to 12 MB (8 MB if the measurement says so) and 500,000 values. Production's whole database is 17 MB across 2 campaigns, so no real export is refused. - The worst case at the new caps is being measured in a 512 MB container before the PR opens. **What stays open here** - **Parse the import in the worker** (1 GB), as a background import. That is the real fix and what would let the caps rise. - **For the owner:** raising the backend's `mem_limit` to 1g is the alternative. It changes what each host needs, so I have not done it. The PR will say it addresses this issue, not that it closes it.
Author
Collaborator

PR #1072 is merged. This issue stays open for one thing. CI was green on the merged commit.

Now bounded

  • Upload size: the Admin setting (1 GB by default, 100 MB to 4 GB), spooled to disk.
  • Total decompressed bytes: the same limit.
  • data.json: 8 MB and 400,000 values (commas, colons and opening brackets outside strings). These ended lower than the 12 MB and 500,000 in my earlier comment: 12 MB missed the memory target when measured.
  • Entries: 20,000, checked against the record zipfile itself reads.
  • Compression: only stored and deflated entries are read. A bzip2 or LZMA entry is refused at open; it decompressed without bound before.
  • Imports in flight: two per worker process, one per user, and a 120 s stall bound.

Measured

  • The dearest in-bounds data.json found costs about 117 MB while parsing, in a 512 MB backend.
  • On a real stack, 18 hostile in-bounds uploads (up to four at once) all answered and oom_kill stayed 0. The sampled peak reached 505 MB of 512 MB, so the margin is thin.

What keeps this open

  • Parse the import in the worker (1 GB), as a background import. That removes the thin margin and lets the data.json caps rise. A campaign that runs for years will reach 8 MB; its export still works, and its import is refused with a sentence.
  • For the owner: raising the backend's mem_limit to 1g is the quicker alternative. Not done.

Production (4.4.1) still has the original uncapped read until 4.5.0.

**PR #1072 is merged. This issue stays open for one thing.** CI was green on the merged commit. **Now bounded** - **Upload size:** the Admin setting (1 GB by default, 100 MB to 4 GB), spooled to disk. - **Total decompressed bytes:** the same limit. - **`data.json`:** 8 MB and 400,000 values (commas, colons and opening brackets outside strings). These ended lower than the 12 MB and 500,000 in my earlier comment: 12 MB missed the memory target when measured. - **Entries:** 20,000, checked against the record `zipfile` itself reads. - **Compression:** only stored and deflated entries are read. A bzip2 or LZMA entry is refused at open; it decompressed without bound before. - **Imports in flight:** two per worker process, one per user, and a 120 s stall bound. **Measured** - The dearest in-bounds `data.json` found costs about 117 MB while parsing, in a 512 MB backend. - On a real stack, 18 hostile in-bounds uploads (up to four at once) all answered and `oom_kill` stayed 0. The sampled peak reached 505 MB of 512 MB, so the margin is thin. **What keeps this open** - **Parse the import in the worker** (1 GB), as a background import. That removes the thin margin and lets the `data.json` caps rise. A campaign that runs for years will reach 8 MB; its export still works, and its import is refused with a sentence. - **For the owner:** raising the backend's `mem_limit` to 1g is the quicker alternative. Not done. Production (4.4.1) still has the original uncapped read until 4.5.0.
Author
Collaborator

Owner's answer to Q153 (O33), 2026-10-05: (a), as recommended. Move the reading to the worker, which has 1 GB; an import then answers at once and finishes in the background.

Also answered, Q154 (O34): the limits are fine as they are. Two imports at once per process, one per person, a 120 s stall bound, 200 map pictures and 5,000 attachments per import, and the picture and GIF bounds.

What this means for the build

  • The import route stores the upload, answers with something to follow, and a worker task reads and writes the campaign.
  • The form shows progress and then the same result (the new campaign, what was left out, the rule links), or the reason it was refused.
  • The data.json caps can rise once the read is in the worker; by how much is to be measured there.
  • A killed worker must not leave half a campaign: the import stays one transaction, and #1075's sweep already removes files with no campaign.

Order: after #1095 lands, since both change import_service.py. To be recorded as a decision with today's other answers.

**Owner's answer to Q153 (O33), 2026-10-05: (a), as recommended.** Move the reading to the worker, which has 1 GB; an import then answers at once and finishes in the background. **Also answered, Q154 (O34): the limits are fine as they are.** Two imports at once per process, one per person, a 120 s stall bound, 200 map pictures and 5,000 attachments per import, and the picture and GIF bounds. **What this means for the build** - The import route stores the upload, answers with something to follow, and a worker task reads and writes the campaign. - The form shows progress and then the same result (the new campaign, what was left out, the rule links), or the reason it was refused. - The `data.json` caps can rise once the read is in the worker; by how much is to be measured there. - A killed worker must not leave half a campaign: the import stays one transaction, and #1075's sweep already removes files with no campaign. **Order:** after #1095 lands, since both change `import_service.py`. To be recorded as a decision with today's other answers.
Author
Collaborator

Picking up the worker move (D281), on feat/1054-import-in-the-worker, now that #1101 has landed.

Shape

  • The upload arrives and is checked as today, then is kept in a private directory on the shared media volume.
  • The route answers 202 with a job's id. A new route gives the job's state to the person who started it.
  • A worker task reads the file and writes the campaign in one transaction, as the route does today.
  • The form shows that the import is being read, then today's result or today's refusal.

Adds

  • One small table for import jobs (an additive migration).
  • A sweep for uploads whose job is finished or lost.

Changes in meaning

  • "Two imports at once, one per person" is enforced from the job table, so it holds across the whole server. Today it is per backend process.
  • The data.json caps rise by what measurement in the worker allows, beside a map decode.

Before merging: an independent review, as for #1072.

**Picking up the worker move (D281)**, on `feat/1054-import-in-the-worker`, now that #1101 has landed. **Shape** - The upload arrives and is checked as today, then is kept in a private directory on the shared media volume. - The route answers 202 with a job's id. A new route gives the job's state to the person who started it. - A worker task reads the file and writes the campaign in one transaction, as the route does today. - The form shows that the import is being read, then today's result or today's refusal. **Adds** - One small table for import jobs (an additive migration). - A sweep for uploads whose job is finished or lost. **Changes in meaning** - "Two imports at once, one per person" is enforced from the job table, so it holds across the whole server. Today it is per backend process. - The `data.json` caps rise by what measurement in the worker allows, beside a map decode. **Before merging:** an independent review, as for #1072.
Author
Collaborator

Done: PR #1104, merged to main (D281). CI was green on the merged commit.

  • An import is a background job. The upload is stored, the route answers 202, and the worker reads the file and writes the campaign in one transaction.
  • The backend's memory is flat during an import (380 → 384 MB in the stack run; on main the same import took it from 381 to 441 MB, where it stayed).
  • data.json may be 24 MB and 1,200,000 values, measured in the worker.
  • A memory stop ends an import that builds more than the worker can hold, with a sentence.
  • A killed or timed-out import ends "failed" exactly once and leaves no campaign, no files and no upload. Shown on a real worker.
  • "Two at once, one per person" is counted from the job table, so it holds across the server.
  • Migration: d1054a2b3c4e adds import_jobs.

Review: an independent review found no authorisation or path defect and seven robustness gaps; all are fixed. A real-worker run found one more (an import killed at its hard time limit was not redelivered); fixed.

Open after this

  • #1105: count what a file holds before building it. Waiting on the owner's Q162.
  • The sheet evaluator swallows the soft time limit; the hard limit ends such an import two minutes later.
  • An import with many large JPEGs has not been measured against the memory stop.
  • Not run after the fixes: an end-to-end pass through nginx for the slow upload, the memory stop and the account-deletion refusal.

Reaches dev with v4.5.0-rc8.

**Done: PR #1104, merged to main (D281). CI was green on the merged commit.** - **An import is a background job.** The upload is stored, the route answers 202, and the worker reads the file and writes the campaign in one transaction. - **The backend's memory is flat during an import** (380 → 384 MB in the stack run; on main the same import took it from 381 to 441 MB, where it stayed). - **`data.json` may be 24 MB and 1,200,000 values**, measured in the worker. - **A memory stop** ends an import that builds more than the worker can hold, with a sentence. - **A killed or timed-out import** ends "failed" exactly once and leaves no campaign, no files and no upload. Shown on a real worker. - **"Two at once, one per person"** is counted from the job table, so it holds across the server. - **Migration:** `d1054a2b3c4e` adds `import_jobs`. **Review:** an independent review found no authorisation or path defect and seven robustness gaps; all are fixed. A real-worker run found one more (an import killed at its hard time limit was not redelivered); fixed. **Open after this** - #1105: count what a file holds before building it. Waiting on the owner's Q162. - The sheet evaluator swallows the soft time limit; the hard limit ends such an import two minutes later. - An import with many large JPEGs has not been measured against the memory stop. - Not run after the fixes: an end-to-end pass through nginx for the slow upload, the memory stop and the account-deletion refusal. Reaches dev with `v4.5.0-rc8`.
rbrooks referenced this issue from a commit 2026-10-05 11:58:48 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rbrooks/Quest-Board#1054
No description provided.