Conversation
The import inflated zip entries with no byte limit, so a few-megabyte archive holding one entry that expands to gigabytes exhausted memory on a jobs worker. Anyone can reach it: Account::ImportsController#create signs up the account itself. Record entries are now read against a byte budget, checked both on the size the archive declares and on the bytes that actually come out of the extractor, since a crafted archive can understate the first. Storage entries keep streaming, but a read now returns at most the length asked for rather than whatever a compressed slice inflates to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186eyzivcTn6wqjEE4Wnxdt
|
🤖 @codex security review. Don't run the tests. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
An entry whose compressed data runs out before the deflate stream ends leaves the extractor short of eof while it has nothing left to give, so take its first nil as the end rather than asking again forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186eyzivcTn6wqjEE4Wnxdt
There was a problem hiding this comment.
🟡 Changes recommended
Valid large rich-text exports become non-importable, while streamed storage entries remain unbounded in total size.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens account imports against ZIP entries that inflate excessively.
Changes:
- Caps buffered record extraction at 2 MB.
- Bounds individual streaming reads.
- Adds oversized-entry and job-discard tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
app/models/zip_file.rb |
Adds the oversized-entry error. |
app/models/zip_file/reader.rb |
Implements bounded extraction. |
app/models/zip_file/reader/io.rb |
Buffers excess streamed output. |
app/models/account/import.rb |
Documents storage preflight behavior. |
test/models/zip_file_test.rb |
Tests extraction limits and streaming. |
test/models/account/import_test.rb |
Tests oversized-record rejection. |
test/jobs/account/data_import_job_test.rb |
Tests terminal error handling. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # column any of them can fill is a rich text body; every other text column the | ||
| # export writes tops out at 64KB. Reading them with no ceiling let a | ||
| # two-megabyte upload inflate to gigabytes inside a jobs worker. | ||
| MAX_BUFFERED_ENTRY_SIZE = 2.megabytes |
There was a problem hiding this comment.
🤖 Real risk, and worth stating plainly rather than waving at: an account holding a rich text body whose exported JSON is over 2MB would export fine and fail to import, with an "invalid export" reason that does not say why.
I set 2MB against the one threshold the app already states about these bodies. SearchReindexJob defaults rich_text_limit to 100_000 bytes and filters anything larger out of reindexing entirely, because "a single pathological body can stall the batch query or OOM the worker during preload, which has happened in practice". 2MB is twenty times that.
Raising it is not free: RecordSet#import holds IMPORT_BATCH_SIZE of 100 records at once, so the entry ceiling is also what bounds a batch, and RichTextRecordSet#transform_body_for_import runs Nokogiri over each body on top.
A write-time limit on body size would be the real answer, but that is a product decision about what people may type, not something to slip into a security fix. I have flagged the number on the tracking card for Rosa to call: it is one constant, and ZipFile::Reader#read takes max_bytes: if a record set ever needs its own.
I could not measure real body sizes to settle it, since bin/kamal query needs a 1Password prompt I cannot answer from this session.
| if block_given? | ||
| yield ZipFile::Reader::IO.new(entry, @io) |
There was a problem hiding this comment.
🤖 Fixed in 4db5944, same change as the codex thread above: the reader budgets total expanded bytes against the archive size (100x, floor 64MB) and raises ZipFile::ArchiveTooLargeError. That bounds the streamed path on both storage services rather than relying on the disk preflight.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef9baf607a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if block_given? | ||
| yield ZipFile::Reader::IO.new(entry, @io) |
There was a problem hiding this comment.
For a crafted deflated storage/* entry, this block path bypasses extract_within, while ZipFile::Reader::IO limits only the output of each individual extraction and never counts total decompressed bytes. The public import flow can therefore accept a small, highly compressible archive and continuously feed gigabytes into blob.upload; the disk preflight is based on the compressed archive size, so this can fill local storage or generate unbounded S3 work and cost. Enforce a cumulative expansion, account-quota, or expected-size bound while retaining streaming.
AGENTS.md reference: AGENTS.md:L49-L54
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Agreed, and fixed in 4db5944. ZipFile::Reader now budgets every byte it hands out, streamed or buffered, against the size of the archive it came from: MAX_TOTAL_EXPANSION of 100x, with a 64MB floor so a small archive holding one large rich text body still imports. Over budget raises ZipFile::ArchiveTooLargeError, which inherits from InvalidFileError like the entry error, so the import fails with "invalid export" and the job discards.
The ratio is a usable signal here because zip_kit deflates each entry on its own and records are small. I measured a real export built from the test fixtures at 0.76 bytes out per byte of archive, with the most compressible single entry at 2.01:1, so 100x leaves a wide margin over anything legitimate.
| content << chunk | ||
| ensure_within entry, content.bytesize, max_bytes |
There was a problem hiding this comment.
The 2 MB ceiling applies independently to each entry, but RecordSet#import_batch and the rich-text override retain 100 parsed records at once. An attacker can submit 100 highly compressible entries just below this limit and force roughly 200 MB of attacker-controlled strings per import, plus JSON and insert_all! allocation overhead; concurrent public imports can still exhaust a jobs worker despite this mitigation. Apply an aggregate batch budget or process large records without retaining a full 100-entry batch.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Deliberate, and the 2MB entry cap is what bounds it. IMPORT_BATCH_SIZE is 100, so a batch holds at most 200MB of record strings plus the insert_all! overhead. That is an ordinary amount for a job to allocate, where the same archive previously had no ceiling at all, and the new archive budget caps the whole pass on top.
An aggregate batch budget would have to count declared entry sizes, which the archive author writes, so it would only be as good as the per-entry ceiling that already backstops it. I would rather keep one number to reason about. If 200MB turns out to be too much in practice, lowering MAX_BUFFERED_ENTRY_SIZE moves it without new machinery.
| # column any of them can fill is a rich text body; every other text column the | ||
| # export writes tops out at 64KB. Reading them with no ceiling let a | ||
| # two-megabyte upload inflate to gigabytes inside a jobs worker. | ||
| MAX_BUFFERED_ENTRY_SIZE = 2.megabytes |
There was a problem hiding this comment.
Any account containing an Action Text body whose exported JSON exceeds 2 MB can no longer be imported: action_text_rich_texts.body is a LONGTEXT column, and the application has no corresponding 2 MB validation, yet this generic limit rejects the entry and marks the whole export invalid. Use a model-specific limit backed by an equivalent write-time constraint, or otherwise allow existing large rich-text records to round-trip.
AGENTS.md reference: AGENTS.md:L49-L54
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🤖 Real risk, and worth stating plainly rather than waving at: an account holding a rich text body whose exported JSON is over 2MB would export fine and fail to import, with an "invalid export" reason that does not say why.
I set 2MB against the one threshold the app already states about these bodies. SearchReindexJob defaults rich_text_limit to 100_000 bytes and filters anything larger out of reindexing entirely, because "a single pathological body can stall the batch query or OOM the worker during preload, which has happened in practice". 2MB is twenty times that.
Raising it is not free: RecordSet#import holds IMPORT_BATCH_SIZE of 100 records at once, so the entry ceiling is also what bounds a batch, and RichTextRecordSet#transform_body_for_import runs Nokogiri over each body on top.
A write-time limit on body size would be the real answer, but that is a product decision about what people may type, not something to slip into a security fix. I have flagged the number on the tracking card for Rosa to call: it is one constant, and ZipFile::Reader#read takes max_bytes: if a record set ever needs its own.
I could not measure real body sizes to settle it, since bin/kamal query needs a 1Password prompt I cannot answer from this session.
Capping a buffered entry left the streaming path open: a crafted deflated storage entry returns a bounded amount per read but no bounded amount in total, so a small archive could still feed gigabytes into blob.upload. Every byte a reader hands out now counts against a budget set from the archive's own size, with a floor so a small archive holding one large rich text body still imports. A real export measures 0.76 bytes out per byte of archive, and its most compressible entry 2:1, because entries are deflated individually and records are small. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0186eyzivcTn6wqjEE4Wnxdt
|
🤖 @codex security review. Don't run the tests. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
rosa
changed the title
4db5944 to
c9b005f
Compare
Problem
Account::Importinflates zip entries with no byte limit, so a few-megabyte archive holding one entry that expands to gigabytes exhausts memory on a jobs worker. Anyone can reach it, sinceAccount::ImportsController#createsigns up the account itself.Two paths, not one:
Account::DataTransfer::RecordSet#loadcallsJSON.parse(zip.read(file_path)), andZipFile::Reader#readwithout a block inflated the whole entry into a string.ZipFile::Reader::IOintoblob.upload, which looked safe, but the length a caller asks for was passed to the extractor as a count of compressed bytes. Active Storage asking for 5MB of a crafted entry got back everything those 5MB inflate to, in one string. Reading a 1024-byte slice of a 2MB entry of repeated bytes returns 1,040,000 bytes. Bounding each read still leaves the total unbounded, so a small archive can keep feeding a crafted entry into storage.Account::Import#ensure_sufficient_storage_spacedoes not cover any of it: it is a local free-disk preflight, and it compares the archive's own size, which is exactly what a zip bomb keeps small.Solution
Three limits, all in
ZipFile::Reader.A ceiling on an entry read into memory,
MAX_BUFFERED_ENTRY_SIZE, set at 2MB. Record entries hold one database row each, and the only column any of them can fill beyond 64KB is a rich text body;SearchReindexJobalready treats a body over 100KB as pathological and skips it. The ceiling is checked twice, against the size the archive declares and against the bytes that come out of the extractor, because a crafted central directory can understate the first.ZipFile::Reader#readtakesmax_bytes:if a caller ever needs its own.A bound on a streamed read.
ZipFile::Reader::IO#read(length)returns at mostlengthbytes, buffering what a slice inflated to beyond that. A stored entry returns exactly the bytes asked of it, so its reads still pass straight through, which is what our own exports write storage files as. An entry whose compressed data runs out before its deflate stream ends now ends the read rather than being asked again forever.A budget on everything one archive produces,
MAX_TOTAL_EXPANSIONof 100x its own size, with a 64MB floor so a small archive holding one large body still imports. Entries are deflated individually and records are small, so a real export barely shrinks: one built from the test fixtures measures 0.76 bytes out per byte of archive, its most compressible entry 2.01:1. This is what bounds the streaming path in total.Over any of the three raises an error inheriting from
ZipFile::InvalidFileError, so the import already stops with an "invalid export" reason andAccount::DataImportJobalready discards it rather than resuming against the same entry.ensure_sufficient_storage_spaceis unchanged. It skips on S3 because there is no local disk to preflight there: the reader range-requests the archive and blob contents upload straight back. Every service Fizzy configures is either Disk or S3, so theblob.openfallback inZipFile.read_from_disk, which would stage a whole archive locally with no preflight, is not reachable today. Added a comment saying so.Known remaining cases:
RecordSet#importstill reads up toIMPORT_BATCH_SIZEof 100 records at once, so a batch holds at most 200MB. Bounded by the entry ceiling, where it was unbounded before.🤖 Generated with Claude Code
https://claude.ai/code/session_0186eyzivcTn6wqjEE4Wnxdt