UN-1924 [FIX] Reject unsupported files in API deployment - #2267
Deepak-Kesavan wants to merge 23 commits into
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.
Building the upload inside pytest.raises meant two calls could satisfy it, so the test would still pass if constructing the file were what raised rather than staging it. Sonar flags the shape (python:S5778) and it is the same way a test passes for the wrong reason that bit two other cases here. The upload is built outside the block, and the expected message is matched, so only the staging call can satisfy it. Verified by mutation: widening the narrowed except back to Exception fails this test with DID NOT RAISE.
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.
|
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-streamis intentionally left inAllowedFileTypes. Sniffing already closes the reported hole; removing it would also change ETL/filesystem behaviour and risks rejecting valid-but-unrecognised files. Worth a separate discussion if we want to go further.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: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.Failedentry — but it never produced a usable result before either, it produced a misleadingEmptyFileError.SuccessandFailedrespectively.PENDING.if not converted_filesshort-circuit; the normal path is untouched.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.application/octet-streamis allow-listed, unchanged. What a given build reports for a plain zip varies — measured asapplication/octet-streamon one libmagic andapplication/zipon another — so a zip is allowed on the former and rejected on the latter. Whichever it is, the verdict now comes from the file's own bytes rather than from its declared type.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.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.