fix(documentai-toolbox): contain split_pdf output to output_path - #18063
fix(documentai-toolbox): contain split_pdf output to output_path#18063Samin061 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces sanitization for entity types in split_pdf by replacing path separators (/ and \) with underscores to prevent path traversal vulnerabilities, and adds a corresponding unit test. The reviewer suggested also replacing : with _ to ensure cross-platform compatibility on Windows, where : is an invalid filename character.
| subdoc_type = ( | ||
| (entity.type_ or "subdoc").replace("/", "_").replace("\\", "_") | ||
| ) |
There was a problem hiding this comment.
While replacing / and \ successfully prevents directory traversal, entity types can also contain other characters like : (especially in custom processors or namespaces, e.g., custom:entity_type). On Windows, : is an invalid filename character (or denotes an alternate data stream), which can cause split_pdf to fail with an OSError or behave unexpectedly.
Consider also replacing : with _ to ensure robust cross-platform compatibility.
subdoc_type = (
(entity.type_ or "subdoc")
.replace("/", "_")
.replace("\\", "_")
.replace(":", "_")
)
split_pdf builds each output filename from entity.type_, which is read straight from the parsed Document (loaded via from_gcs or from_document_path). Document AI entity types can contain "/" (the fixtures already carry "vat/tax_amount"), and a document whose type_ is set to something like ../../../../tmp/pwned makes os.path.join(output_path, output_filename) write the split PDF outside output_path. The entity type is now flattened before it goes into the filename, so the write stays inside output_path.