Sanitize entry/page ids used as export staging paths - #340
Closed
Flerpharos wants to merge 1 commit into
Closed
Conversation
Notebook.export(source="walk") built staging file paths directly from API-returned entry and page ids with no validation. A hostile id such as "../../outside" could escape the temporary entries directory when the staged file was written, allowing arbitrary process-writable files to be overwritten. Reuse the existing _writable_name() sanitizer (already applied to the exported file/dir names) on the id before it is used as a staging path component in the three affected spots in _walk_tree(). The reported metadata still records the real (unsanitized) id; only the on-disk staging filename changes. Fixes #339
Collaborator
Author
|
this is overly paranoid? It's not in threat model for LabArchives the API to be giving out hostile ids. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #339
Notebook.export(source="walk")built staging file paths in_walk_tree()(src/labapi/tree/_export.py) directly from API-returned entry and page ids, with no validation:source = entries / entry.id(attachment staging)source = entries / f"{entry.id}.{...}"(text entry staging)source = entries / f"{page.id}.json"(page metadata)The
_writable_name(...)output-name sanitizer was only applied to thefilesdict key (the final exported name), not tosource(the path actually opened for writing). A hostile/compromised id such as../../outside(or an absolute path) could makesourceescape the temporaryentriesstaging directory, letting the write overwrite arbitrary process-writable files.Fix: reuse the existing
_writable_name()sanitizer on the id at all three sites before it's used as a staging path component._writable_namealready strips path separators/invalid characters and collapses all-dot names (e.g...) tountitled, so a hostile id can no longer produce a path that traverses out ofentries. Thefilesdict keys, the on-disk export layout, and themetadata"id"field (which still reports the real, unsanitized id) are unchanged — only the staging filename is affected.The native backup export path (
_backup_tree) was checked too: it builds staging paths from SQLite integer row ids (part["id"],node["id"]), not attacker-controlled strings, so it wasn't affected and wasn't changed.Added
test_walk_tree_sanitizes_hostile_entry_idsintests/tree/test_export.py, which runs_walk_treewith a page/entry/attachment whose ids contain../and asserts every staged file lives directly inside theentriesstaging directory (no escape) while the export still succeeds and the metadata still reports the real ids.Test plan
uv run pytest --ignore=tests/test_integration.py -q— 431 passed, 1 skippeduv run ruff check .— all checks passed