Repository navigation
UN-1924 [FIX] Reject unsupported files in API deployment - #2267
Conversation
…e staging API-deployment uploads were gated on the multipart Content-Type, which the caller supplies and nothing verifies, with a fallback to application/octet-stream that is itself in AllowedFileTypes. Any file passed that check, reached the bucket, and failed at extraction with an error that did not name the cause. Detect the type from the file's own bytes with libmagic before writing anything, matching the filesystem source path, and report a rejected file as a failed entry in the API response instead of staging it under a placeholder hash that later surfaced as an empty-file error.
 |
Rejecting files at staging means the dispatch set can now be empty, which reached a path that was previously unreachable: the API worker's _unified_api_execution short-circuits an empty file set and returns status COMPLETED without ever writing that status back, so the row kept the status it was dispatched with and the caller polled a PENDING execution forever. Skip the dispatch entirely when staging yields nothing, marking the execution COMPLETED and returning the per-file rejection entries, and make the worker's own short-circuit persist the status so an empty set from any other caller cannot strand an execution either.
athul-rs
left a comment
athul-rs
left a comment
There was a problem hiding this comment.
Standardized review — INITIAL, 16/16 lenses (unstract plugin v0.18.1), head 7af4732 vs base 62a41e9.
Verdict: BLOCK — Critical: 1 · High: 4 · Medium: 8 · Low: 2
The 1 Critical and 4 High findings are posted inline below. The 8 Medium and 2 Low are held out of this comment to keep the thread focused; happy to post them on request. Headline items among them: MIME detection is unguarded so one unreadable stream fails the whole batch (source.py:1203-1210); the early return never calls _set_result_acknowledge, so a later GET /status re-serves the same results with 200 instead of 406; release_slot(api.organization, ...) is a silent no-op (key is built from str(organization.organization_id)), the trap documented at undispatched_sweep.py:245-253; and two comments describe pre-change behaviour, including one this PR's own commit 2 invalidated.
Lens checklist — 1 see #1 · 2 medium · 3 see #1,#2,#3 · 4 clean · 5 see #5 · 6 clean · 7 see #1 · 8 see #2 · 9 clean · 10 see #3,#5 · 11 see #1 · 12 N/A · 13 see #4 · 14 clean · 15 medium · 16 see #1
Lenses 4, 6, 9, 11, 14 were assessed directly rather than by a specialist agent: sniffing replaces a caller-controlled header at a trust boundary and is strictly stronger; staging is synchronous pre-dispatch with no new shared state; an 8 KiB read is cheaper than every existing sniff site; python-magic==0.4.27 is already declared and pinned. Lens 11 is the exception — there is no flag or rollout gate on a change that flips accept/reject on the main upload path, which is what makes finding #1 expensive to unwind.
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
PR Review — Standardized (LITE)
Verdict — REQUEST CHANGES
Mode: LITE — single-pass, 16/16 lenses, one reader, no subagents. Eligible: 6 files / 309 lines, no disqualifiers.
Summary — Critical: 0 · High: 2 · Medium: 3 · Low: 1 · Lenses run: 16/16
The core change is right. Sniffing bytes instead of trusting the multipart Content-Type is the correct fix, it matches what the filesystem/ETL path already does, and the test that stages evil.pdf is a genuine reproduction of the reported bug. The findings are all in commit 2 — the empty-dispatch handling — plus one behaviour claim in the PR body that does not hold.
Findings are posted as inline comments (#1–#6).
Lens checklist
| # | Lens | Result |
|---|---|---|
| 1 | Spec & intent | See finding #4 |
| 2 | Architectural fit & precedent | Clean — mirrors the libmagic sniffing already in the filesystem source path; reuses ResultCacheUtils / FileExecutionResult rather than inventing a reporting channel |
| 3 | Correctness & edge cases | See findings #1, #3 |
| 4 | Security | Clean — this closes a spoofed-Content-Type hole; no authn/authz/tenancy surface touched |
| 5 | Data integrity & migrations | N/A — no schema, migration, or backfill; the status write routes through the existing guarded model method |
| 6 | Concurrency | Clean — release_slot is zrem (rate_limiter.py:356-357), so the explicit release plus update_execution's own terminal release is not a double-decrement |
| 7 | API & contract compatibility | See finding #4 — response shape unchanged, per-file status semantics change |
| 8 | Reliability & resilience | See finding #1 |
| 9 | Performance & cost | Clean — 8 KiB read + rewind per upload, before any write. Verified DOCX/XLSX/PPTX, CSV, JSON and PDF all classify correctly from the first 8 KiB |
| 10 | Observability | Clean — rejection logged via workflow_log.log_error and surfaced to the caller; no PII added |
| 11 | Operational safety | See finding #3; no flag or rollout surface in this diff |
| 12 | LLM/agent-specific | N/A — no model call, prompt, tool config, or eval touched |
| 13 | Testing | See finding #5 |
| 14 | Dependencies & build | N/A — no dependency change; python-magic==0.4.27 already declared and already imported at source.py:13 |
| 15 | Code quality | See finding #6 |
| 16 | Doc & comment accuracy | See finding #2 |
Open questions
- Finding #1 — was the conversion-failure case (non-empty input, empty
converted_files) considered, or is the guard only meant for the genuinely-empty set the backend now produces? - Finding #4 — do you know of tenants pushing zips or RTF through API deployments today? They pass on
mainwhen the header is absent oroctet-stream, and stop passing after this. - The all-rejected path never calls
delete_api_results/_set_result_acknowledge, so the rejection entries live untilEXECUTION_RESULT_TTL_SECONDS. Intentional (a later status poll still shows them) or an oversight?
Assumptions made
- The libmagic results in finding #4 come from the venv at
backend/.venv. If CI or the runtime image ships a different libmagic, that table could shift — the OOXML-from-8KiB result in particular is version-sensitive, though it held here. - I took CI-green and the dev-env verification in the PR body at face value; I did not re-run the suite.
- Finding #2 assumes API deployments still run on the Celery transport by default (
queue_message_id IS NULL). If the PG queue transport is now universal, that one drops to Low.
Resolving an 8 KiB sample alone rejected every legacy Office upload larger
than the window: libmagic reads .doc/.xls/.ppt through the OLE2 directory
sector at the end of the file, so the sample only ever showed the container
(application/x-ole-storage), which is not allow-listed. Reproduced on a
710 KB .doc and a 1.2 MB .xls, on the API path and the UI execute endpoint
alike. Container samples now escalate to a full-file classification, using
the upload's temp path when Django has spilled it to disk.
Also from review:
- Isolate the terminal status write on the all-rejected path so the rate
limit slot and staging dir are released even if it raises, matching the
staging-failure path above it.
- Report the execution's stored status instead of asserting COMPLETED; the
row can be missing or the terminal guard can refuse the change, and
claiming success only hides a stranded execution behind a 200.
- Write total_files/failed_files alongside the status, since a terminal row
with a NULL failed_files reads as a clean success to is_failure_run and to
run history.
- Distinguish an empty dispatch from a total conversion failure in the
worker: convert_file_hash_data swallows per-file errors and returns {} for
both, so the second was being reported as a zero-file success.
- Scope the guard comment on update_execution_completed to the PG transport;
the legacy path applies the status unconditionally.
Tests: pin the short-circuit to the staging result rather than the upload
list (the previous test passed with the original bug reintroduced), cover
the cleanup-on-DB-error path, the container escalation, and the empty-upload
branch. Mutation-checked: reverting the escalation, gating the short-circuit
on file_objs, and dropping the cleanup isolation each fail the suite.
release_slot formats its argument into the Redis key, and acquire_slot built that key from str(organization.organization_id). Passing the Organization instance produced a different key, so the ZREM removed a non-member: it returns 0 and raises nothing, leaving the slot held for the full TTL and throttling every other API-deployment call for that org. All three call sites in this module were affected, including the one added for the all-rejected path. The two correct call sites in the codebase (undispatched_sweep.py, models/execution.py) already pass the id string, and the former carries a comment describing this exact trap. The same instance-instead-of-id call remains in api_deployment_views.py and in two places in mcp_server/tools/execution.py; those are outside this change's surface and are left for a separate fix.
MIME detection reads the upload, so a broken stream raises inside the staging loop and aborts every remaining file in the request. Rejection is already per-file for an unsupported type; an unreadable one now behaves the same way. The message says detection failed rather than naming a type, since an I/O fault and an unsupported format need different follow-ups.
…jected run The all-rejected early return reaches a terminal status without going through WorkflowHelper, so it skipped two things the dispatch path does. It serves the per-file results in its own response but never marked them consumed, so a follow-up GET /status served them a second time with 200 where the contract is 406. The synchronous dispatch path acknowledges at exactly this point. It also never reached PipelineUtils.update_pipeline_status, the only dispatcher of API deployment notifications, so an all-rejected request alerted nobody where a dispatched-and-failed run would have. Both are wrapped so a failing webhook cannot turn a handled rejection into a 500, and the response is unchanged. set_result_acknowledge loses its underscore: it is now called from another module, so it is part of the contract rather than an internal detail.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Sharing one try block meant a failed acknowledgement returned before the notification ran, so an all-rejected execution could reach a terminal state without alerting API deployment subscribers. They are independent obligations; neither now depends on the other succeeding.
Reformatted by a newer ruff than the v0.3.4 the pre-commit config pins, in the opposite direction to how the file already reads. Restores both to the committed formatting so the diff is additive.
The check exists so a file Unstract accepts is one LLMWhisperer can extract. Any divergence is a bug in one of two directions: accept something it cannot read and the failure just moves downstream to a 415, or reject something it can read and a working file is lost. Reconciled against the gate itself — Util.is_valid_file_type_from_path in unstract-llm-whisperer, not the public docs page, which lists formats the gate does not name and omits how it actually decides. - Any text/* is accepted. That is how html, xml, tsv, rtf and markdown reach the extractor: as text, not as listed formats. They were being rejected here despite LLMWhisperer handling them. - Dropped application/octet-stream. Nothing libmagic cannot name is extractable, and this entry is why a zip renamed .pdf reached LLMWhisperer and came back 415 — the symptom this ticket was raised for. - Dropped the macro-enabled xlsx type, which LLMWhisperer's gate does not list. The two enums now match member for member, 20 MIME types. An empty upload has no type to judge, so detection returns None and the gate is skipped rather than passed through a placeholder. LLMWhisperer additionally re-identifies octet-stream and zip with Magika and falls back to scanning for a %PDF- header. We have neither, so this gate is marginally stricter than theirs for unidentifiable bytes.
praveen-formido
left a comment
praveen-formido
left a comment
There was a problem hiding this comment.
Review of the diff for correctness bugs. 3 findings below.
libmagic's PDF rule matches only at offset 0, so eight bytes ahead of the header are enough to make a real PDF sniff as application/octet-stream. Dropping octet-stream from the allow-list therefore started rejecting stream-wrapped PDFs — and this allow-list also gates the filesystem connector and the worker, so those files would have begun failing in ETL and Task pipelines that process them today. LLMWhisperer scans the same 1 KB window for the same reason, after giving octet-stream and zip a Magika second opinion first. Without Magika this covers the PDF case, which is the one that reaches us; anything else libmagic cannot name is still rejected. Applied at all three gates that share the list: API staging, the filesystem source, and the worker's own check. Also: the worker's two new terminal branches wrote the execution status but never updated the pipeline, so a run that ended there left the pipeline's last run stale and alerted no subscriber. They now notify the way the sibling empty-files branch does.
athul-rs
left a comment
athul-rs
left a comment
There was a problem hiding this comment.
Re-review — Standardized (FOLLOWUP), 16/16 lenses
unstract plugin v0.18.1. Head 938a9572 vs base 6fe7d8df; previous-review boundary 7af47324.
Verdict: BLOCK — Critical: 0 · High: 9 · Medium: 15 · Low: 6
Scope change: YES — the original review surface was 6 files / 282 added lines; it is now 11 files / 746. Four files entered the surface after the last round, including both copies of the cross-process allow-list and a private-to-public method rename. That triggers a full re-scan rather than a verification pass, which is what this review is.
All five prior findings from the previous round are resolved or materially improved, and the fixes are good ones. The Critical is genuinely closed: the OLE2 escalation was verified here against real files with libmagic 5.45, not accepted on the description's word. The block is about what arrived afterwards, in cb97bf67.
Prior findings status
| # | Severity | Finding | Status |
|---|---|---|---|
| 1 | Critical | 8 KiB sniff window rejects legacy OLE2 Office | RESOLVED (code) — test does not pin it, see inline on source.py:1233 |
| 2 | High | Cleanup skipped if the status write raises | RESOLVED — deployment_helper.py:325-332, pinned by test_all_files_rejected_cleanup_survives_db_marking_error |
| 3 | High | Response asserts COMPLETED regardless of the write | RESOLVED — deployment_helper.py:377-379 reports the stored status |
| 4 | High | Guard predicate unpinned (not file_objs mutant passed) |
RESOLVED — the [MagicMock()] upload plus test_files_staged_successfully_are_dispatched now pin the branch to the staging result |
| 5 | High | No durable record; all-rejected stored as a clean success | PARTIALLY RESOLVED — clean-success half fixed; durable-record half still open, replied on the original thread |
| m1 | Medium | MIME detection unguarded, one stream fails the batch | RESOLVED — but a new whole-request failure mode replaced it, see inline on source.py:1296 |
| m2 | Medium | Early return never acknowledges results | RESOLVED — deployment_helper.py:351 |
| m3 | Medium | release_slot silent no-op |
PARTIALLY RESOLVED — three sites fixed, three still live |
Unanchored findings
[High] [Lens 1 · 16] — the PR description asserts the opposite of the code on the change with the widest blast radius. The body still states that "application/octet-stream is intentionally left in AllowedFileTypes… removing it would also change ETL/filesystem behaviour", that "Files libmagic cannot identify still pass", that a zip is "allowed on the former" libmagic build, and that clients pushing text/html, text/xml, markdown and RTF "will start seeing a per-file Failed entry". All four are now false — OCTET_STREAM is gone from both enums, and those four types are now accepted through the new text/* branch. The XLSM removal and the _set_result_acknowledge to set_result_acknowledge rename are not mentioned at all. "Can this PR break any existing features" is the section a reviewer or release manager reads to size blast radius, so as written it asks people to approve a change they have not been shown. This one is a description fix, not a code fix.
[Medium] [Lens 2 · 8] — the release_slot trap this PR documents is fixed at three sites and still live at three others. The new comment at deployment_helper.py:298-302 states the trap exactly and it is real: check_and_acquire builds the Redis key from str(organization.organization_id) (api_v2/rate_limiter.py:120) while release_slot(organization_id: str, ...) formats its argument straight into _get_org_key (:344-358). Still passing the model instance: api_v2/api_deployment_views.py:153, mcp_server/tools/execution.py:224 and :229. The first of those is the handler that runs when execute_workflow itself raises — precisely when the slot most needs releasing. Pre-existing rather than introduced here, and outside this diff, but the PR establishes the invariant in a comment while leaving live counter-examples two files away. Minor follow-on: the comment names acquire_slot, which does not exist anywhere in the repo — the method is check_and_acquire.
Concurrence with @praveen-formido
Two of his findings reproduce independently here, so they are not restated as separate comments:
source.py:1305— partial rejections missing from the counters. Confirmed. One addition to his citation: besides theEXECUTINGwrite attasks.py:476-480, the finalisation path overwrites the counters again from dispatched files only —workers/callback/tasks.py:390-403readstotal_files/successful_files/failed_filesstraight out ofaggregated_results. So the drift is re-applied at the end of the run, not only at the start.enums.py:39— theOCTET_STREAMremoval reaching ETL and Task pipelines. Confirmed, and replied on that thread with the LLMWhisperer-parity half, which argues for raising it to High.
His third finding (the new worker branches never calling update_pipeline_status) is not one this review reached independently and looks correct.
Medium and Low, listed rather than argued
Not merge blockers in my reading, but they should not be lost: mime_type=None now crossing the queue into FileHashData.mime_type, which is typed str and whose data.get("mime_type", "") does not fire on an explicit null (source.py:1216 to data_models.py:1063) — no consumer crashes today, but three of them survive by truthiness checks rather than by design. The _detect_container_mime_type docstring's FILE_UPLOAD_MAX_MEMORY_SIZE bound does not hold for presigned-URL uploads, which are hand-built InMemoryUploadedFiles bounded by API_DEPL_PRESIGNED_URL_MAX_FILE_SIZE_MB (default 20 MB) at deployment_helper.py:718-725; that also makes the verdict transport-dependent, since the two branches feed libmagic different evidence. The text/* wildcard is unpinned in both directions — narrowing it to ("text/html", "text/xml") survives all 948 tests — and it now admits text/x-shellscript, text/x-script.python and text/x-msdos-batch, measured; whether that is intended is a product call, but no test records it either way, and workers/shared/enums/file_types.py has no test file at all. Detection faults and unsupported types are indistinguishable downstream, since both become FileExecutionResult(error=<prose>) with status=FAILED, though the source comment says they "need different follow-ups". The CONTAINER_MIME_TYPES comment's OOXML claim is empirically false — real .docx/.xlsx/.pptx resolve directly from the leading 8 KiB, so only the OLE2 half holds and application/zip in that set is effectively inert. The comment at deployment_helper.py:318-320 describes worker behaviour that this same PR fixes. The test module docstring says MIME detection is "deliberately not patched" while five tests patch it. _detect_container_mime_type's fallback parameter is unreachable. release_slot arguments are asserted bare at three of four sites, so a revert to the model instance stays green.
Lens checklist
1 see unanchored · 2 see unanchored · 3 see source.py:1284, source.py:1296, execution.py:403, deployment_helper.py:325 · 4 Clean · 5 see execution.py:403 · 6 see execution.py:403 · 7 see tasks.py:239 · 8 see source.py:1296, deployment_helper.py:325 · 9 Medium, listed · 10 see source.py:1284, tasks.py:239 · 11 no flag or rollout gate on a change that flips accept/reject on two independent ingest paths; rollback is revert-only · 12 N/A — no model call, prompt, tool config or eval touched · 13 see source.py:1233, execution.py:391, tasks.py:221 · 14 N/A — no dependency change; python-magic==0.4.27 already declared at backend/pyproject.toml:35, no manifest or lockfile diff · 15 Medium, listed · 16 see execution.py:412 and unanchored
Lenses 4, 6, 11, 12 and 14 were assessed directly rather than by a specialist agent. On security: the change strictly closes a spoofed-Content-Type hole and touches no authn/authz/tenancy surface. The text/* widening admits shell scripts and batch files, but they reach an extractor rather than an executor, and header-less uploads already passed through the octet-stream fallback on main, so net exposure is not widened.
Open questions
- Was the
octet-streamremoval's effect on the ETL and connector paths intended? The description argues against doing it; the code does it. - Full LLMWhisperer parity needs Magika and the
%PDF-fallback, neither of which exists here. Is partial parity the intent, or shouldapplication/octet-streamjoinCONTAINER_MIME_TYPES? - Is admitting
text/x-shellscript/text/x-script.python/text/x-msdos-batchan accepted consequence of thetext/*branch, or incidental?
Assumptions
libmagic results are from 5.45 with the pinned python-magic==0.4.27; container reporting is version-sensitive, though the LLMWhisperer divergence is structural and would not shift. CI was taken at face value and e2e was not re-run. The mutation runs cover the backend unit tier only. @chandrasekharan-zipstack's open threads are assessed but deliberately not marked resolved here — they are his to close.
Posted as a comment rather than a formal change request; the BLOCK verdict is this review's reading, not a merge gate.
Three faults in the staging loop, all raised in review. The PDF rescue promoted on a bare `%PDF-` substring anywhere in the first kilobyte, so any unidentifiable blob carrying that text near its start could be staged as a PDF — a way past the gate, in a check whose whole purpose is to close one. It now leaves a recognised zip alone and re-classifies from the marker's own offset, so the promotion has to be corroborated rather than guessed. Stricter than LLMWhisperer's equivalent scan, which is the safe direction for something that moves a file into the allow-list. Detection caught `Exception`, which turned any systemic fault — a broken libmagic database, an unreadable upload temp dir — into a per-file rejection. Every file in every request would fail that way, and the request still answers 200 COMPLETED, so a platform outage would read to callers as "your files are invalid" and to metrics as success. Now narrowed to the faults that really are about this upload's bytes; anything else propagates and fails the request. Reporting a rejection wrote to the result cache inside the per-file loop through an unguarded pipeline execute, so a transient Redis fault while rejecting one bad file would fail the whole execution and discard the good files already staged — contradicting the invariant the suite claims in test_supported_files_survive_a_rejected_sibling. Both call sites now go through one reporter that degrades to a log line. Tests cover all three, plus the disk-backed detection branch, which no test reached: SimpleUploadedFile has no temporary_file_path, so every existing container test exercised only the in-memory path — while real legacy Office uploads, the files that branch exists for, are usually over the 2.5 MB threshold and take the other one.
Re-classifying from the marker's offset was not the safeguard it looked like: libmagic's PDF rule is only the magic bytes, so "%PDF- not a pdf" still came back application/pdf and the file was still promoted into the allow-list. A real header carries a version, so require %PDF-<n>.<n> before treating the marker as evidence. The offset re-classification stays as a second check. The test that was supposed to cover this passed for the wrong reason - its blob sniffed as image/x-tga and never reached the rescue at all. It now asserts the bytes are octet-stream first, so it fails if it stops exercising the branch.
…om a buffer Measured on the shipped libmagic 5.46: magic.from_buffer never returns application/zip at any sample size, not even given every byte of the file — only magic.from_file does. So the zip half of the escalation set could never match, and the value a zip container actually reports from a buffer, application/octet-stream, was neither allow-listed nor escalated. A valid .docx whose first member is not [Content_Types].xml — what re-zipping and streaming writers produce — was therefore rejected at upload, having worked on main under the octet-stream allowance. octet-stream now counts as undecided like the named wrappers, one shared set so the API and connector paths cannot disagree, and classification always goes through a path: an in-memory upload is spilled to a temporary file rather than classified from a buffer that cannot answer the question. from_file still only says 'application/zip' for a repackaged OOXML file, so the archive's own entries decide — the same markers libmagic's msooxml rule looks for. That recognises the document without taking on an ML classifier; it can only return types already in the allow-list, so it widens what is recognised and never what is permitted. Tests now feed real container bytes with libmagic unmocked. Every existing container test scripted libmagic's answers, which is precisely why a detector asking a question libmagic cannot answer looked green.
The backend and the workers were each carrying a verbatim copy of the inconclusive-type set, the PDF rescue and the zip inspection, kept together by a 'keep in step' comment — which the quality gate then failed at 8.2% duplication on new code against a 3% limit. unstract/core already hosts exactly this kind of cross-cutting predicate (is_failure_run), so the logic lives there now and both packages import it. The allow-list enums stay where they are; only the interpretation of libmagic's answer is shared, which is the part that must not drift. identify_zip_container takes the allow-check as an argument rather than importing an enum, so the shared module stays free of either package.
… through it Looking inside an archive to name the document meant reading its mimetype member whole. That member comes from an unvalidated upload, and read() decompresses whatever the archive declares, so a compact bomb could expand into backend memory during synchronous staging and fail the request before anything was dispatched. An ODF mimetype member holds one media type string, so anything larger is not what is being looked for: the declared size is checked first and the read is bounded regardless.
Reading the ODF mimetype member and accepting any allow-listed value it named handed the decision to the upload: a zip declaring application/pdf would have been staged as a PDF and dispatched, which is precisely the deferred extraction failure this gate exists to prevent. The member can identify which ODF document an archive is; it cannot choose a format. Only the three ODF types are honoured now, and only when the archive meets the conditions the spec requires of a real one — first entry, stored uncompressed, and small. A member bolted onto an arbitrary zip meets none of them. identify_zip_container no longer takes the allow-check, because every value it returns is now a fixed constant rather than a string from the archive.
An entry stored uncompressed puts %PDF-1.7 in the archive's literal leading bytes, so a scan of the head sees it exactly as it would in a real document. Nothing pinned that the wrapper is resolved before the marker is consulted, which is the order that keeps it from becoming a way past the gate. Worth having as a test rather than a reasoned argument: checking the helper on its own suggests the marker wins, and only the real entry point shows the escalation to from_file naming the archive first.
The worker's COMPLETED arm wrote total_files=0 and left the other two
counters NULL — the exact shape update_execution_completed exists to avoid,
since a terminal row with a NULL failed_files reads as a clean success.
Nothing ran, so both are now written as zero.
Its ERROR arm named the reason under "message" while the function's other
two ERROR returns use "error", so a consumer reading .get("error") to learn
why an execution failed got None for this variant alone.
When the all-rejected status write raises, the response carried no error at
all, unlike the sibling staging-failure path — a bare 422 with no reason,
while the row is still PENDING so a follow-up GET /status says something
else. The swallowed exception is now reported.
Also corrects a comment that claimed a field-scoped save() is safe against
concurrent writers: it is, for the columns, but save() re-runs
_handle_execution_cache() regardless of update_fields, which another comment
in the same file already explains. No live bug at this call site — the row is
freshly created and undispatched — but the comment said otherwise.
A partially-rejected run reported as a clean success: rejected files are not dispatched, so the worker's batch aggregate never saw them, and a run that turned away a file still finished failed_files=0. That reads as success to is_failure_run, so subscribers who asked to hear about failures heard nothing, and the only trace was a cache entry deleted on the first status poll. A rejected file now gets a terminal ERROR file execution carrying its real MIME type and the reason. The execution serializer already derives successful/failed by counting those rows, so run history and the UI become correct for free, and the rejection survives long enough to answer a support question. The stored counters are then floored at what the rows say when the worker reports its aggregate. That only ever corrects upward, and only to a number already true in the database, so runs with nothing rejected are untouched — which is what lets this stay inside the backend rather than changing the counter contract the ETL and Task pipelines share. The row write is guarded: bookkeeping must not cost a caller the good files staged alongside the bad one. Content is hashed for the row's identity, since the API path has no file_path and two rejects would otherwise collide. Mutation-checked: stubbing the row write fails both new tests.
The row identity was the content hash alone, so the same bytes uploaded twice under different names folded into one row — recording one rejection instead of two, under-counting failed_files, and losing a durable record the caller was owed. The API path has no file_path, so this value is all that separates them. The name is folded into the identity. Deliberately no longer a plain content hash: a rejected file was never processed and should not look to file history as though it had been. The test missed this because it used different bytes for the two files, so it never exercised the collision. It now sends identical bytes under different names, and fails on a content-only identity.
|
Thanks for working through all the review threads. I checked the resolved ones against One general request before merge: please make the code comments shorter and more generic. A lot of the new comments read like review-thread replies. They record line numbers, other files, library versions and the history of earlier attempts. Those details go stale quickly, and some of them are already wrong. A comment should say why the code does something unusual, in one or two lines. The review history belongs in the PR and the commit messages. Some examples:
As a rule: no line numbers, no references to other files or repos, no library versions, and no "this used to…" narratives. If the code needs a paragraph of explanation, it might need a test or a clearer name instead. Separately, the PR description is still out of date. This was the unanchored High in the follow-up review. It still says |
The comment pointed at undispatched_sweep.py:245 to explain the trap, and nothing keeps that honest — it was already off by two lines when written. Both reviewers flagged it. Says the constraint concisely instead: pass the id string, never the model instance, because release_slot builds its key from what it is given.
Many of them read like review-thread replies: they recorded line numbers, paths into other repos, libmagic versions and the reasoning behind earlier attempts. Those details rot, and two were already wrong — a cited method name that does not exist, and a version-specific measurement that does not hold on a newer libmagic. Each now says why the code does something unusual, in a line or two. The reasoning itself is not lost; it is in the PR and the commit history, which is where it stays true.
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Approving at 188f3e091. All review threads I raised are addressed, the comment cleanup and the PR description rewrite look good, and CI is green.
Before merging:
- Reply to and resolve the two new Greptile P1s (the OOXML marker check in
mime_gate.py, and duplicate same-name rejections sharing a row). Neither looks blocking to me. - Update the branch with
main.
Follow-ups for separate tickets:
- Move the allow-list and
is_allowedintounstract.coreso the backend and worker enums can't drift, and give the connector paths the same detection as API staging. release_slotis still called with the organization model instance inapi_deployment_views.pyandmcp_server/tools/execution.py.
Cognitive complexity on _update_file_aggregates had gone to 17 against a limit of 15, so the flooring moved into its own method. Reads better split anyway: one method decides which counters are being written, the other reconciles them with the file rows. The composite assertion in the rejection test is split, so a failure says which half went wrong. Note the extracted signature annotates with a quoted "list[str]": the enclosing viewset defines its own list() action, and a class-body annotation resolves in the class namespace, so the bare form binds to that method and raises on import. Caught by importing the module rather than parsing it.
…ument Marker directories alone were enough, so an archive carrying [Content_Types].xml and a word/ member was named a document without being loadable. Greptile was right about this one, and my own live run had already shown it: an archive of exactly that shape was accepted here and then rejected by the extractor, which is the failure this gate exists to prevent. A real package always carries _rels/.rels, so that is required now. Only affects the repackaged shape, which is what this code resolves — with [Content_Types].xml first, libmagic names the package and this never runs. The test fixture was the incomplete shape, which is why it failed extraction when deployed. It is a real package now, and the incomplete one is pinned as rejected.
The rescue returned early for application/zip but not for application/x-ole-storage, so an OLE2 container libmagic could not classify further was promoted to a PDF by a %PDF- marker sitting in its first kilobyte. Both are wrappers libmagic positively identified, and neither should be overruled by its own contents. Also fixes a CI-only failure of my own making: a guard assertion pinned from_buffer to application/octet-stream, which is build-dependent — CI reports application/zip for the same bytes. It now asserts the property the test needs, that the archive is not named a document and so gets inspected. That disagreement settles an earlier one on this PR: the reviewer's application/zip measurement was right and my local one was the outlier. Mutation-checked: narrowing the guard back to zip alone fails the OLE case. The first version of that test could not have caught it — a single mocked from_buffer answered the rescue's own re-classification too, hiding the promotion.
…part Two holes, both confirmed rather than argued. Member names alone were enough to name a zip an Office document, so an archive carrying the right names with nothing behind them was accepted and then failed conversion. Confirmed against the deployment: an archive of exactly that shape came back error_in_ingestion. A real package declares its main part's content type, so that declaration is required now — matched as bytes, since the input is untrusted and an XML parser is more surface than this check warrants. Rejected rows keyed on content and name alone, so two byte-identical parts sent under one name collapsed into a single record and the counts came up short. The upload's position in the request is part of the identity now, so every rejected part keeps its own row. The test fixture was the shape that fails conversion, which is why it needed the live check to expose. It declares a main part now, and the names-only archive is pinned as rejected. Mutation-checked: dropping either requirement fails its test.
Requiring the marker was too loose: an archive with main+xml as stray text, an empty _rels/.rels and any word/ member still passed as a document. A package must now declare its main part's content type, relate that same part from the root, and contain it. The reported type comes from the declaration rather than guessed from a member-name prefix. Still bytes and bounded reads, not an XML parser — the input is an untrusted upload. Attributes are matched in any order and either quoting style, so a real package is recognised however its XML is written. Checked against three LibreOffice artefacts and four near-miss shapes: stray marker, declared-but-unrelated, relationship disagreeing with the declaration, and declared part absent from the archive. All four rejected, all three real packages identified.
Matching raw bytes kept admitting archives that are not documents: an XML comment satisfied the Override match, and the ODF branch trusted the mimetype member on its own. Tightening the patterns was not converging — a comment, a namespace prefix or an escaped attribute each defeat matching, so the declarations are parsed now. OOXML must declare its main part, relate that same part from the root, and contain it. ODF must have a manifest declaring the same media type for the package root. The reported type comes from the declaration, not from a member-name prefix. Reads stay bounded, and a member carrying a DTD is refused rather than parsed, so entity expansion never reaches the parser. Stdlib only: defusedxml is present for the backend and the workers but is not declared by unstract-core, and this did not warrant adding a dependency there. Checked against four genuine LibreOffice artefacts and seven near-miss shapes, including both bypasses Greptile found and an entity bomb. The ODF cases are asserted against the helper directly: libmagic names that archive shape itself, so staging never escalates it on this build — but its container verdicts differ between builds, which is exactly why the branch should not be left loose.
Parsing the declarations took _identify_ooxml to cognitive complexity 18 against a limit of 15. Finding the declared main part and confirming what relates it are separate questions, so they are separate functions now. All eleven verification shapes still behave the same.
The guard searched the raw bytes for ASCII markers, but the parser detects the document's encoding — so a DTD written in UTF-16 was invisible to the search and still expanded once parsing began. Confirmed: a 1.1 KB archive carrying a UTF-16 entity chain was accepted as a document. The member is now checked against every encoding the parser accepts, so the DTD is found regardless of how it was written. Mutation-checked: restricting the search back to UTF-8 fails the UTF-16 case.
|
Unstract test resultsPer-group results
Critical paths
|
4 participants
What
Reject unsupported files in API deployments at the staging step, by detecting the MIME type from the file's own bytes instead of trusting the caller-supplied
Content-Type.A rejected file is no longer written to the API storage bucket and is no longer dispatched for processing. It is reported back to the caller as its own failed entry naming the offending type. Verified live on the dev env:
{ "execution_status": "COMPLETED", "result": [{ "file": "evil.pdf", "status": "Failed", "error": "Rejecting file 'evil.pdf' with unsupported MIME type 'text/html'" }] }Why
SourceConnector.add_input_file_to_api_storageis the single funnel through which API-deployment uploads reach the bucket, and its MIME check readfile.content_type— the multipartContent-Typesupplied by the caller, which nothing verifies. It also fell back toapplication/octet-streamwhen that header was absent, andoctet-streamis itself inAllowedFileTypes. Between the two, effectively any file passed the check.The consequences, both reported on the ticket:
temp-hash-…withis_executed=True. The worker then read a path that had never been written, copied 0 bytes, and raisedEmptyFileError— so a wrong-file-type upload surfaced as a misleading "empty file" error.The filesystem/ETL source path already sniffs with libmagic (
source.py, and_copy_filesystem_filein the workers). The API path was the asymmetry.How
Commit 1 — sniff the bytes.
SourceConnector._detect_uploaded_file_mime_type, which reads the leading 8 KiB of the upload, rewinds, and classifies withmagic.from_buffer(..., mime=True). libmagic only needs the leading bytes, so this does not pull large uploads into memory.AllowedFileTypesbefore any bytes are written.workflow_log.log_errorand pushed toResultCacheUtils.update_api_resultsas aFileExecutionResultcarrying the error. Both the async status endpoint and the synchronoustimeout > 0wait read that same cache, so the entry surfaces either way.uuidimport.Commit 2 — don't strand an execution when everything is rejected.
Dropping rejected files from the dispatch set means that set can now be empty, which reaches a path that was previously unreachable.
_unified_api_executionin the API worker short-circuits an empty file set and returnsstatus: "COMPLETED"but never callsupdate_workflow_execution_status, so the row keeps the status it was dispatched with and the caller polls aPENDINGexecution forever. (The no-files branch in_run_workflow_apithat does write COMPLETED sits after this guard and is never reached.)This was caught by testing against the dev env, not by the unit tests — worth noting for reviewers.
Fixed on both sides:
DeploymentHelper.execute_workflowskips the dispatch entirely when staging yields nothing, marks the execution COMPLETED via a newWorkflowExecutionServiceHelper.update_execution_completed, releases the rate-limit slot, cleans up the staging dir, and returns the per-file rejection entries.Empty uploads are deliberately let through the MIME check: libmagic reports
application/x-empty, which is not in the allow-list, and rejecting them there would relabel an empty-file problem as an unsupported-type one. They continue to be reported asEmptyFileErrordownstream.application/octet-streamhas since been removed fromAllowedFileTypes. The allow-list is now reconciled against LLMWhisperer's own gate rather than maintained independently, andoctet-streamis not in it — keeping it was what let a zip renamed.pdfreach the extractor and come back415, which is the symptom this ticket was raised for. A file libmagic cannot name is one LLMWhisperer cannot read.Two deliberate exceptions, so this does not reject files that work today:
octet-stream; it is recognised by looking for the header. Eight bytes ahead of it are enough to trigger this, so it is not a corner case.[Content_Types].xml— what re-zipping and streaming writers produce — is identified from the archive's own entries.Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
The MIME change is scoped to
add_input_file_to_api_storage, which has two callers: the API-deployment execution path and the workflow "execute" endpoint used from the UI. Behaviour changes only for files that are actually unsupported:Files that were already processing successfully are unaffected. They sniff to their real type, which is in the allow-list. Verified live: a genuine PDF still returns
Successwith full extraction output. A supported file whoseContent-Typeheader was absent or wrong is now more likely to be accepted, since the bytes decide rather than the header.Genuinely unsupported files now fail fast, with a clear message, instead of failing later at extraction. This is the intended change. A caller relying on such a file being silently skipped will now see a
Failedentry — but it never produced a usable result before either, it produced a misleadingEmptyFileError.Multi-file requests are not failed wholesale — verified live that a good file and a rejected file in one request return
SuccessandFailedrespectively.The empty-dispatch fix is strictly a bug fix. It only changes behaviour for an execution with no files to process, which previously hung in
PENDING.The worker change touches only the
if not converted_filesshort-circuit; the normal path is untouched.A missing
Content-Typeused to mean "accept anything" — that is the behaviour change. The old code fell back toapplication/octet-streamwhen a part carried no declared type, andoctet-streamis itself allow-listed, so every header-less upload was accepted regardless of its content (plaincurl -Fand several SDKs send parts this way). Those uploads are now sniffed. Clients that were relying on that fallback to pushtext/html,text/xml,.eml, markdown, RTF, SVG or HEIC through an API deployment will start seeing a per-fileFailedentry. This is the point of the ticket rather than a side effect, but it is a real change for existing integrations and is called out here deliberately.Files libmagic cannot identify are now rejected. This reverses what an earlier revision of this description said.
octet-streamis no longer allow-listed, so an unidentifiable upload is turned away at our door instead of failing at extraction. A zip renamed.pdfis the case that prompted the ticket, and it is now rejected here. Stream-wrapped PDFs and repackaged Office documents are recognised rather than caught by this — see above.Three allow-list entries changed.
text/*is now accepted as a class, which is how html, xml, tsv, rtf and markdown reach the extractor — they were being rejected despite LLMWhisperer handling them.application/octet-streamwas removed, as wasapplication/vnd.ms-excel.sheet.macroenabled.12(XLSM), which LLMWhisperer's gate does not list; a real.xlsmsniffs as the ordinary OOXML spreadsheet type and is still accepted, so that entry was unreachable.Newly rejected, previously accepted via the
octet-streamfallback:message/rfc822(.eml),image/svg+xml,image/heic, and plain archives. These are the release-note items.Legacy Office uploads were briefly regressed and are fixed. Sniffing only the leading 8 KiB reported
application/x-ole-storagefor every.doc/.xls/.pptlarger than that window, which is not allow-listed — libmagic resolves OLE2 through a directory sector at the end of the file. Container types are now escalated to a full-file classification. Reproduced on a 710 KB.docand a 1.2 MB.xls, and pinned by tests.A rejected file is now recorded as a failed file execution. Run history and the executions list count it, where before a partially rejected run reported as a clean success and failure-notification subscribers heard nothing. Stored counters are floored at what the file rows hold, which only ever corrects a count upward.
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
python-magic==0.4.27was already declared inbackend/pyproject.tomland already imported by this module.Notes on Testing
Unit tests.
backend/workflow_manager/endpoint_v2/tests/test_api_storage_mime_validation.pyexercises the realSourceConnector.add_input_file_to_api_storagewith its DB/storage collaborators patched. MIME detection is deliberately not mocked — libmagic sniffing is the behaviour under test — and the fixture bytes were chosen against what libmagic actually reports. Covers: a real PDF is staged; an HTML document announced asapplication/pdfis not staged, not written, not dispatched; the rejection reaches the caller with the offending type and statusFailed; a supported file with no declaredContent-Typeis still staged asapplication/pdf; a supported file alongside a rejected one still processes.These were checked to actually discriminate: reverting only the detection line to
file.content_typemakes 4 of the 5 fail, includingevil.pdfbeing staged to the bucket — the reported bug reproduced as a test.backend/api_v2/tests/test_deployment_helper.pyadds a test that an all-rejected request reaches a terminal status without dispatching, and still returns the rejection entries.8 unit tests pass locally; CI is green on unit, integration and e2e.
Live verification against an API deployment on the dev env (
deepak-unstract-dev), after deploying this branch:COMPLETED—good.pdf: Success, full extraction output.pdf, part declaredapplication/pdfCOMPLETED—evil.pdf: Failed,"Rejecting file 'evil.pdf' with unsupported MIME type 'text/html'"COMPLETED—good.pdf: Success,evil.pdf: FailedThe spoofed file is the important one: the multipart part explicitly declares
application/pdf, so it defeats any header-based check.Screenshots
Not applicable — no UI surface. API responses are inline above.
Checklist
I have read and understood the Contribution Guidelines.