feat(kb): accept spreadsheet, presentation and OpenDocument uploads (#16775)

[mrveiss]

Stacked on #16783 (issue-16773-zip-format-verify), and targeted at that branch rather than main. The ordering is load-bearing: content verification has to be in place before the upload form accepts five more ZIP-container parsers. GitHub retargets this to main automatically once #16783 merges.

Thinking Path

ALLOWED_EXTENSIONS listed .txt .md .pdf .docx .json .csv .html. DocumentParser has carried working, tested parsers for .xlsx .pptx .odt .ods .odp all along, so a user could not upload a file the backend could already read if it arrived by any other path. The owner decision on the ingestion review is that every format in it is handled.

The trap worth naming: five more elif ext == ... branches on a function that trusts the filename would have widened the attack surface and reintroduced the extension-trust #16773 just removed — and would have done it on exactly the formats that share the PK prefix. So the dispatch resolves format from content first, name second, using the same helper this stack already introduced. A file uploaded as notes.txt that is really a workbook reaches the workbook parser; a .xlsx that is really a presentation reaches the presentation parser.

Two formats are deliberately left out, and the tests pin both so the next audit does not re-flag them:

• .doc and .ppt are OLE2 containers. python-docx and python-pptx read only the OOXML ones, so advertising these would accept a valid file and then report it corrupt — that is bug(knowledge): .doc is advertised as supported and always fails as 'invalid or corrupted' #16786, which needs an owner call on whether to add a real OLE2 reader or stop advertising the format.
• .odg is a drawing: it carries almost no extractable text, and this endpoint stores flattened text only. Say the word and it is a one-line change.

What Changed

Area	Change
api/knowledge_office_upload.py (new)	The routing: verified_upload_extension (content first) and extract_office_upload (temp file → DocumentParser). No new extraction logic.
api/knowledge.py	ALLOWED_EXTENSIONS gains the five; _extract_file_content dispatches on the verified extension and routes the office set. Net-zero — see below.
utils/document_parser.py	parse_document_text: the sync entry point for a caller already off the event loop.

_extract_file_content already runs inside asyncio.to_thread, so the blocking parse belongs there — but DocumentParser.extract_text is async and hands work to a thread itself. parse_document_text gives that caller the same dispatch without a second thread hop and without a second copy of the parser table, which is how these two routes drifted apart before (#14333).

Verification

• Pre-push pytest on the changed areas: passing.
• api/knowledge.py is at its size ceiling (3414) and is still at exactly 3414: the routing lives in the new module, and the three added lines are paid for by rewrapping three comments — same words, fewer lines.
• Failure path: an unparseable office upload raises a 400 carrying the parser's own reason, matching how the neighbouring pdf/docx helpers report a bad upload.
• Unchanged: .txt, .md, .csv, .json, .html, .pdf and .docx routing is untouched, and covered by a test that a plain text upload still comes back verbatim.

Model Used

Claude Opus 5 (claude-opus-5)

Closes #16775
Refs #16772

Single-issue rationale: widening the upload allowlist is a different risk from the content verification it depends on (#16773), which is why it is stacked on that PR rather than batched with it.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (2)

• main
• release

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e678628a-5749-4898-887c-1e700b0a7a62

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mrveiss]

Review verdict: BLOCK. Three substantive findings plus a structural one. The format matching is genuinely well done — the allowlist matches what the parser can actually read, and the exclusions are deliberate and documented — but this cannot merge yet.

0. Structural — this is stacked, and not evaluable against main

The base is issue-16773-zip-format-verify (#16783), which is still open. That is not cosmetic:

$ git show origin/main:autobot-backend/media/document/zip_formats.py
fatal: path ... does not exist in 'origin/main'

knowledge_office_upload.py:16 imports verified_suffix from that module. Every content-verification property below depends on #16783 landing first.

Correcting my own earlier note on this PR: I checked file overlap against main and found none, and treated that as "safe to merge". The check was true and the conclusion did not follow — a stacked PR can have zero overlap and still be unmergeable, because the thing it needs is absent from the base rather than conflicting with it. Overlap answers "will this conflict", not "will this resolve".

CI is also not green at review time: zero SUCCESS conclusions, most checks pending, and CodeRabbit reports reviews disabled for this base branch.

1. 🔴 Decompression bomb — unmitigated

The content sniff is safe: it reads only namelist() and a 128-byte capped member (_MIMETYPE_MAX_BYTES). But the parsers this PR newly makes HTTP-reachable fully inflate the archive with no cap:

def _parse_xlsx(self, file_path, metadata):
    wb = load_workbook(str(file_path), data_only=True)   # inflates every sheet into memory

Same in _parse_pptx (python-pptx) and _parse_odt/_parse_ods/_parse_odp (odfpy's load()). MAX_FILE_SIZE_BYTES = 10MB (api/knowledge.py:126) caps the compressed upload only — nothing caps uncompressed size, entry count, or per-entry ratio.

The guard already exists and is not used here. autobot-backend/archive_safety.py carries exactly this, written for the plugin/theme installers:

MAX_ZIP_UNCOMPRESSED_BYTES = 256 * 1024 * 1024
MAX_ZIP_ENTRIES = 5000
MAX_COMPRESSION_RATIO = 100

Neither knowledge_office_upload.py nor document_parser.py imports it. Run those checks before handing the temp file to any parser. Being behind check_admin_permission mitigates but does not close it — admin accounts are precisely what gets targeted.

2. 🟡 A content-verified but unrouted format is stored as garbage

verified_upload_extension can return .odg (it is in SUFFIX_BY_FORMAT), but OFFICE_EXTENSIONS deliberately excludes it. Trace: upload drawing.txt whose real content is .odg → name-based validation passes → the verifier logs "content is odg, name says txt — parsing as odg" and returns .odg → .odg matches no branch in _extract_file_content → falls through to:

    # Default: treat as text
    return file_content.decode("utf-8", errors="replace"), None

Raw ZIP bytes are replacement-decoded and stored as a knowledge fact. No error, despite the log line claiming otherwise, and _has_usable_content passes on the resulting garbage. Reject with the same 400 the office path uses when the verified extension is recognised-but-unsupported.

3. 🟡 No negative control on the allowlist

Searched repo-wide for a test calling _validate_file_upload or the endpoint with a disallowed extension: none found. This PR's test_the_legacy_binary_formats_stay_out asserts only that .doc/.ppt are absent from the set literal — deleting the actual if ext not in ALLOWED_EXTENSIONS: raise HTTPException(...) at api/knowledge.py:842-846 would fail no test. Stated as "nothing found" after a targeted repo-wide search, not "did not look".

4. XXE — a stated gap, deliberately not a verdict

defusedxml 0.7.1 and lxml 6.1.1 are present; openpyxl prefers defusedxml when importable, and lxml defaults block classic external-entity fetches. odfpy uses stdlib xml.sax/expat, where modern expat caps entity expansion — but that was not confirmed for every deployment target, and odfpy sets no explicit entity policy. This cannot be settled without running code, which is out of scope for a review. Recording it as unresolved rather than inferring a pass from library defaults. A test feeding each parser a billion-laughs/external-entity payload would close it.

Checked and correct

• The allowlist is not decorative — _validate_file_upload runs at line 1251, before _extract_file_content at ~1256. The gate precedes parsing.
• Zip Slip does not apply — nothing calls ZipFile.extract() or writes member content to paths derived from entry names; all access is in-memory.
• Allowlist matches parser capability — OFFICE_EXTENSIONS is exactly what DocumentParser can read. .doc/.ppt (OLE2) and .odg are excluded deliberately with bug(knowledge): .doc is advertised as supported and always fails as 'invalid or corrupted' #16786 tracking. This is the right answer to the bug KB upload allowlist omits already-supported office/ODF formats #16775 describes.
• Function lengths all well under 30 lines; get_logger throughout, no print(), no hardcoded secrets.

Context, not a regression

file_content = await file.read() (api/knowledge.py:1249) reads the whole upload into memory before the size check. Pre-existing and untouched here, but its impact grows now that the allowlist admits memory-heavier parsers. Worth its own issue.

To pass

1. feat(documents): verify ZIP-based office and ODF formats by content (#16773) #16783 merges and GitHub retargets this to main.
2. Decompression-bomb guards before any parser touches the archive, reusing archive_safety.py.
3. Reject the recognised-but-unsupported extension instead of falling through to text.
4. A negative-control test on the allowlist.
5. An XXE test, or a written justification that library defaults suffice.

[mrveiss]

Carried by vehicle #17077, which includes this PR's approved head cfb0d4932. Closed now as carried, per the owner's ruling (2026-09-19) that consolidated work shouldn't keep open duplicates or trigger extra CI. The branch is kept. The vehicle's own Closes lines close the linked issues when it lands. If #17077 is abandoned, this PR gets reopened.