UN-1924 [FIX] Reject unsupported files in API deployment - #2267
UN-1924 [FIX] Reject unsupported files in API deployment#2267Deepak-Kesavan wants to merge 2 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.
|
| Filename | Overview |
|---|---|
| backend/workflow_manager/endpoint_v2/source.py | Sniffs uploaded bytes with libmagic, rejects unsupported MIME types before storage, and records per-file failures. |
| backend/api_v2/deployment_helper.py | Completes and cleans up deployment executions when staging leaves no files to dispatch. |
| backend/workflow_manager/workflow_v2/execution.py | Adds a helper for safely transitioning no-work executions to completed. |
| workers/api-deployment/tasks.py | Persists completed status when conversion produces no valid files. |
| backend/workflow_manager/endpoint_v2/tests/test_api_storage_mime_validation.py | Adds focused MIME-sniffing and mixed-upload staging coverage. |
| backend/api_v2/tests/test_deployment_helper.py | Verifies that an all-rejected request completes without worker dispatch. |
Sequence Diagram
sequenceDiagram
participant Client
participant API as Deployment API
participant Stage as Source staging
participant Cache as Result cache
participant Worker
Client->>API: Upload documents
API->>Stage: Detect MIME from file bytes
alt Unsupported file
Stage->>Cache: Append failed file result
Stage-->>API: Exclude file from dispatch set
else Supported file
Stage-->>API: Return staged file hash
end
alt No supported files remain
API->>API: Mark execution completed
API->>Cache: Read rejection results
API-->>Client: Completed response with failures
else Supported files remain
API->>Worker: Dispatch supported files
Worker->>Cache: Append processing results
API-->>Client: Execution status/result
end
Reviews (2): Last reviewed commit: "UN-1924 [FIX] Terminalise an API executi..." | Re-trigger Greptile
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.
|
Unstract test resultsPer-group results
Critical paths
|
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.
| # libmagic classifies from the leading bytes; reading more only costs memory. | ||
| MIME_DETECT_CHUNK_SIZE = 8192 |
There was a problem hiding this comment.
[Critical] [Lens 3 · 1 · 16] — the 8 KiB sniff window rejects legacy Office uploads that work today
Failure mode. libmagic resolves an OLE2 compound file through a directory sector that normally sits near the end of the file. Given only these 8192 bytes it falls back to application/x-ole-storage, which is not in AllowedFileTypes (the list carries application/msword, application/vnd.ms-excel, application/vnd.ms-powerpoint, application/CDFV2 — enums.py:19,21,24,28). Every .doc/.xls/.ppt larger than the sample is therefore rejected as unsupported, on both callers of add_input_file_to_api_storage: the API-deployment path (deployment_helper.py:280) and the UI workflow execute endpoint (workflow_v2/views.py:263). Before this PR the caller-declared type was accepted and the file processed.
Evidence. Reproduced twice independently, on a real 248 KB .ppt and on LibreOffice-produced .doc/.xls, with libmagic 5.45 and with 5.46 inside the shipped backend image:
sample4.ppt 8KiB -> application/x-ole-storage 64KiB -> application/x-ole-storage full -> application/vnd.ms-powerpoint
doc500.doc 8KiB -> application/x-ole-storage full -> application/msword
big.xls 8KiB -> application/x-ole-storage full -> application/vnd.ms-excel
Widening to 64 KiB does not fix it. The same root cause degrades a .docx whose [Content_Types].xml compresses past the window to application/zip, also not allow-listed.
Every other sniff site in this codebase reads 4 MiB (source.py:75, workers/shared/workflow/execution/service.py:1220), which is why these files pass the existing downstream check and fail only the new one — 8 KiB is 512x narrower and introduced here.
This also makes the comment on line 76 false, and it is the stated justification for the constant. The PR description's "files that were already processing successfully are unaffected — they sniff to their real type, which is in the allow-list" does not hold for this class.
Suggested fix. Sniff the full staged object (magic.from_file), or treat application/x-ole-storage and application/zip as inconclusive rather than unsupported. Either way the regression test needs a fixture larger than the window — the current ones are 45 bytes (tests/test_api_storage_mime_validation.py:27-28), which is why neither CI nor the dev-env run surfaced this.
Confidence: High.
| if not hash_values_of_files: | ||
| WorkflowExecutionServiceHelper.update_execution_completed(str(execution_id)) | ||
| APIDeploymentRateLimiter.release_slot(api.organization, str(execution_id)) | ||
| DestinationConnector.delete_api_storage_dir( | ||
| workflow_id=workflow_id, execution_id=execution_id | ||
| ) |
There was a problem hiding this comment.
[High] [Lens 3 · 8] — this branch can raise before its own cleanup runs
Failure mode. update_execution_completed catches only WorkflowExecution.DoesNotExist (execution.py:393-401). Any other DB failure — OperationalError, a statement or lock timeout on the select_for_update inside update_execution (models/execution.py:418-423), a deadlock, a dropped connection — propagates out of execute_workflow, so line 315 and lines 316-318 never run. The org's rate-limit slot stays held for the full 6h TTL and throttles every other API-deployment call for that org, the staging dir is never deleted, the row stays PENDING, and the caller gets a 500 with no execution id to poll.
Evidence. The sibling block immediately above (lines 289-308) deliberately isolates its DB write in an inner try/except with logger.exception so that cleanup always runs, and has a regression test pinning exactly that: test_staging_failure_cleanup_survives_db_marking_error (tests/test_deployment_helper.py:75-91). The new path copies the shape but not the guard, and has no equivalent test.
Suggested fix. Wrap the update_execution_completed call in its own try/except Exception: logger.exception(...) so release_slot and delete_api_storage_dir always execute, and add the mirror-image test.
Confidence: High.
| return APIExecutionResponseSerializer( | ||
| ExecutionResponse( | ||
| workflow_id=workflow_id, | ||
| execution_id=execution_id, | ||
| execution_status=ExecutionStatus.COMPLETED.value, | ||
| result=ResultCacheUtils.get_api_results( | ||
| workflow_id=str(workflow_id), execution_id=str(execution_id) | ||
| ), | ||
| ) | ||
| ).data |
There was a problem hiding this comment.
[High] [Lens 3 · 10] — the response asserts COMPLETED whether or not the status write landed
Failure mode. There are three ways the call on line 314 returns normally without the row reaching COMPLETED:
- row missing —
execution.py:399-401logs and returnsNone; - row vanished under the lock —
models/execution.py:424-425,if locked is None: return, silent; - row already terminal with a different value —
models/execution.py:520-535refuses, logs a warning, returns([], False).
The return value is discarded and execution_status on line 323 is a hardcoded literal rather than the row's actual status. The API then answers COMPLETED while a follow-up GET /status/<execution_id> reads the DB and returns PENDING — the stranded-execution bug this PR exists to fix, now concealed behind a success response instead of being visible.
update_execution_completed was given a WorkflowExecution | None return type to carry exactly this signal, and no caller reads it.
Suggested fix. Bind the result: if it is None, or its status is not COMPLETED, log at error level and return the row's real status (or ERROR) rather than claiming COMPLETED.
Confidence: High.
| response = dh.DeploymentHelper.execute_workflow( | ||
| organization_name="org", | ||
| api=_api(), | ||
| file_objs=[], | ||
| timeout=-1, | ||
| ) |
There was a problem hiding this comment.
[High] [Lens 13] — the guard's predicate is unpinned; the original bug can be reintroduced with the suite green
Failure mode. This test passes file_objs=[] with SourceConnector fully mocked, so it exercises the zero-files-uploaded path, not the zero-files-staged path the guard exists for. Nothing in the suite asserts that a non-empty upload whose staging result is empty takes the short-circuit, and nothing asserts the short-circuit does not fire when staging returns files.
Evidence (mutants run against the branch, then reverted):
if not hash_values_of_files:->if not file_objs:atdeployment_helper.py:313— 3/3 pass. That mutant is the production bug verbatim: one HTML file uploaded, staging rejects it and returns{},file_objsis non-empty, control falls through toexecute_workflow_async, execution stranded in PENDING.if not hash_values_of_files:->if True:— the wholebackend/api_v2/tests/suite is identical to baseline (48 passed).- Control: deleting the
update_execution_completedcall does fail this test, so it pins the branch body, not the branch condition.
Also worth noting: assert response["result"][0]["status"] == "Failed" on line 148 reads back the fixture's own literal from line 115, so it proves the branch forwards the cache verbatim, not what source.py writes.
Suggested fix. Pass a non-empty file_objs (a bare MagicMock() suffices — with SourceConnector mocked, the only read is len(file_objs) at deployment_helper.py:243) so the two cases become distinguishable, and add a sibling test with add_input_file_to_api_storage.return_value = {"good.pdf": MagicMock()} asserting execute_workflow_async is called and update_execution_completed is not. Parametrising timeout over {-1, 10} closes the untested synchronous path at negligible cost.
Confidence: High (mutants executed).
| # Rejected files are never dispatched, so nothing downstream will | ||
| # report on them - surface the failure in the API response here. | ||
| ResultCacheUtils.update_api_results( | ||
| workflow_id=workflow_id, | ||
| execution_id=execution_id, | ||
| api_result=FileExecutionResult( | ||
| file=file_name, | ||
| error=log_message, | ||
| ), |
There was a problem hiding this comment.
[High] [Lens 5 · 10] — a rejected file leaves no durable record, and an all-rejected run is stored as a clean success
Failure mode. Before this change a rejected file produced a FileHash, was dispatched, and the worker's own libmagic check (workers/shared/workflow/execution/service.py:1223-1229) created a real WorkflowFileExecution row. After this change it produces no FileHash, no WorkflowFileExecution, no file-history row. The rejection exists only as an entry in the Redis list api_results:{workflow_id}:{execution_id}, which is deleted the first time anyone polls /status (workflow_helper.py:451-453), expires after EXECUTION_RESULT_TTL_SECONDS (3h default), and is gone on any eviction or restart. After any of those, nothing in Postgres can answer "why was my file not processed?".
Compounding it on the all-rejected path: deployment_helper.py:313-318 writes only status=COMPLETED. The row keeps total_files = len(file_objs) from line 243 while failed_files and successful_files stay NULL (models/execution.py:191-208, nullable, no default). is_failure_run is is_failure(status) or (failed_files or 0) > 0 (unstract/core/.../data_models.py:663), so COMPLETED + NULL reads as a success — the response body says every file Failed while the execution row says N files, zero failures. This is the hazard already written up at internal_views.py:546-550 ("a terminal status with failed_files=None ... silently bypasses notify_on_failures subscribers"). Run history is affected too: get_last_run_statuses derives PARTIAL_SUCCESS from these counters (models/execution.py:622-636).
Separately, the early return never reaches PipelineUtils.update_pipeline_status, the only dispatcher of API-deployment notifications (pipeline_utils.py:58 -> APIDeploymentUtils.send_notification), so an all-rejected request now sends no webhook at all where the dispatched-and-failed run previously alerted.
Note also that the worker-side check already raises UnsupportedMimeTypeError naming the file and the MIME type, which softens the PR description's premise that an unsupported file today "fails at extraction with an error that does not name the real cause".
Suggested fix. Write the aggregates alongside the status (failed_files=len(file_objs), successful_files=0), and keep a persisted per-file record for a rejected file — a WorkflowFileExecution row in terminal ERROR carrying the real MIME type — so the rejection is auditable after the cache entry is gone. If cache-only is deliberate, it is worth stating in the PR description as a support/audit trade-off.
Confidence: High.
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.
| api_client.update_workflow_execution_status( | ||
| execution_id=execution_id, | ||
| status=ExecutionStatus.COMPLETED.value, | ||
| total_files=0, |
There was a problem hiding this comment.
[High] [Lens 3, 8] — Short-circuit now reports COMPLETED for a conversion failure, not just an empty input
FileProcessingUtils.convert_file_hash_data catches per-file exceptions, logs, and continues (workers/shared/processing/files/utils.py:65-69), returning only what converted. So if not converted_files is reachable with a non-empty hash_values_of_files when every file failed conversion.
The new unconditional write turns that into a successful, zero-file execution: the row goes COMPLETED with total_files=0, the caller gets status: "COMPLETED" with no results and no error, and the only trace is a logger.warning. Before this diff that case stranded in PENDING — also wrong, but loudly wrong. A silent success is the worse of the two.
# utils.py:65-69 — errors are collected and swallowed
except Exception as e:
conversion_errors.append(error_msg)
continue
...
return converted_files # can be {} for non-empty inputThe branch's own log line already says the quiet part: "No valid files to process after conversion" — after conversion, not nothing was sent.
Suggested fix — distinguish the two:
if not converted_files:
if hash_values_of_files:
api_client.update_workflow_execution_status(
execution_id=execution_id,
status=ExecutionStatus.ERROR.value,
error_message="No files could be converted for processing",
)
return {"execution_id": execution_id, "status": "ERROR", ...}
api_client.update_workflow_execution_status(..., status=ExecutionStatus.COMPLETED.value, total_files=0)Confidence: High that the branch is reachable with non-empty input. Medium on production frequency — FileHashData.from_dict over backend-produced dicts rarely throws.
| try: | ||
| execution = WorkflowExecution.objects.get(pk=execution_id) | ||
| # Same reason as update_execution_err: the model method owns the | ||
| # terminal-one-way guard, so this cannot revert an already-final row. |
There was a problem hiding this comment.
[High] [Lens 16] — This comment asserts a guard that does not hold on the Celery transport
"the model method owns the terminal-one-way guard, so this cannot revert an already-final row" is only true on the PG path.
update_execution routes on queue_message_id: when it is NULL (the Celery/legacy transport, still the default), _apply_legacy_update runs — and it sets self.status = status.value unconditionally, with no guard at all (models/execution.py:455-471). Only _apply_guarded_status (models/execution.py:506-517) guards.
Both sibling comments in this same file carry the qualifier this one drops:
execution.py:176-178— "…terminal-one-way guard (atomic select_for_update, PG-scoped, field-scoped writes)"execution.py:382-384(update_execution_err) — "…so a late error handler can't revert a PG execution the callback already finalized"
A maintainer trusting this comment and reusing update_execution_completed in a late completion callback would overwrite an already-ERROR execution with COMPLETED.
Suggested fix — comment-only; restore the scoping, e.g. "…so a PG execution the callback already finalized cannot be reverted."
No live bug at the current call site — the row is freshly created and PENDING — which is why this is High and not Critical.
| # short-circuits an empty file set without writing a status back, which | ||
| # would strand this execution in PENDING — terminalise it here instead. | ||
| if not hash_values_of_files: | ||
| WorkflowExecutionServiceHelper.update_execution_completed(str(execution_id)) |
There was a problem hiding this comment.
[Medium] [Lens 3, 11] — Cleanup is skipped if this status write raises
update_execution_completed catches only WorkflowExecution.DoesNotExist; the update_execution it calls does select_for_update plus a save() and can raise OperationalError/DatabaseError. That propagates out of execute_workflow, so release_slot and delete_api_storage_dir on the next two lines never run: the org's rate-limit slot stays occupied and the staging dir is orphaned.
The sibling staging-failure path 25 lines above guards against precisely this, and there is a regression test pinning it:
# deployment_helper.py:285-291
try:
WorkflowExecutionServiceHelper.update_execution_err(...)
except Exception:
logger.exception(f"Failed to mark execution {execution_id} as ERROR")
# then release_slot + delete_api_storage_dirbackend/api_v2/tests/test_deployment_helper.py:75 — test_staging_failure_cleanup_survives_db_marking_error.
Suggested fix — same try/except Exception: logger.exception(...) wrapper around the update_execution_completed call.
Medium rather than High because the leak self-heals: _cleanup_expired_entries sweeps the zset by score (backend/api_v2/rate_limiter.py:44-47).
| destination_path = os.path.join(api_storage_dir, file_name) | ||
|
|
||
| mime_type = file.content_type | ||
| mime_type = cls._detect_uploaded_file_mime_type(file) |
There was a problem hiding this comment.
[Medium] [Lens 1, 7] — The PR body's compatibility claim about zips is wrong
The description states: "Mislabelled binaries that libmagic reports as octet-stream (e.g. plain zips) still pass, as before". libmagic does not report zips as octet-stream. Measured against the pinned python-magic:
| bytes | sniffed | in AllowedFileTypes? |
|---|---|---|
| plain zip | application/zip |
✗ |
| RTF | text/rtf |
✗ |
| HEIC | image/heic |
✗ |
| BMP header | application/octet-stream |
✓ |
Any upload of those that previously reached the bucket by declaring application/octet-stream or by omitting Content-Type is now a hard Failed (AllowedFileTypes, backend/workflow_manager/endpoint_v2/enums.py:9-35).
That may well be the intent — the check itself is correct and this is not a code defect. But "Can this PR break any existing features" currently asserts the opposite, so whoever approves this is approving an understated blast radius.
Suggested fix — correct the claim in the PR body. If any tenant is known to push zips/RTF through API deployments, that is a rollout question worth answering before merge rather than after.
Confidence: High on the libmagic behaviour (measured, not recalled). Medium on real-world impact — depends on tenant traffic I cannot see.
| sample = file.read(cls.MIME_DETECT_CHUNK_SIZE) | ||
| file.seek(0) | ||
| if not sample: | ||
| # libmagic reports "application/x-empty" here, which would reject the |
There was a problem hiding this comment.
[Medium] [Lens 13] — The two deliberate new branches are the two without tests
This empty-upload branch exists specifically to preserve the downstream EmptyFileError path — it is the one branch of _detect_uploaded_file_mime_type reachable only with zero bytes, and nothing pins it. Delete it and the suite stays green while empty uploads start being relabelled as unsupported-type failures, which is the exact outcome the comment says to avoid.
Same for workers/api-deployment/tasks.py:225-231: nothing asserts the worker now persists the status, which is the whole point of that hunk.
Suggested fix — two assertions:
_stage([_upload("empty.pdf", b"", "application/pdf")])returns the file staged withmime_type == "application/octet-stream".- A worker test asserting
update_workflow_execution_statusis called withCOMPLETEDon the empty short-circuit.
Noted in the PR's favour: the existing 5 tests were verified to discriminate (reverting the detection line fails 4), which is more than most PRs do.
| # Staging rejected every file, so there is nothing to dispatch. The worker | ||
| # short-circuits an empty file set without writing a status back, which | ||
| # would strand this execution in PENDING — terminalise it here instead. | ||
| if not hash_values_of_files: |
There was a problem hiding this comment.
[Low] [Lens 15] — The two fixes for the same scenario disagree on total_files
The worker sets total_files=0 (workers/api-deployment/tasks.py:230); this branch leaves it at the creation-time len(file_objs) (deployment_helper.py:241). An all-rejected run therefore lands COMPLETED with total_files=1 and zero file executions.
Cosmetic in the API response (which reads the result cache), but the executions list shows a completed run whose counts do not add up.
Suggested fix — have update_execution_completed zero the count, or accept a total_files argument, so both paths agree.
3 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.octet-stream(e.g. plain zips) still pass, as before —octet-streamremains allow-listed.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.