Python: keep attachments close#7038
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 94%
✓ Correctness
Clean security hardening of the attachment store's path resolution. The implementation correctly prevents path traversal (../, absolute paths) and nested directory access by resolving paths and checking both containment and direct-child constraints. The tests cover all relevant edge cases. No correctness issues found.
✓ Security Reliability
This PR correctly implements path traversal prevention for the local attachment store. The fix resolves paths canonically and validates that the resulting path is a direct child of the uploads directory, blocking directory traversal (../), nested paths, absolute paths, empty strings, and symlink-based escapes. The implementation is sound and well-tested.
✓ Test Coverage
The test coverage for this security-focused change is solid. The PR adds both a positive test (valid attachment ID resolves correctly) and a well-chosen parametrized negative test covering path traversal (../), nested paths, absolute paths, empty strings, and current-directory references. Assertions are meaningful—checking exact path equality and specific ValueError messages. The core path-resolution boundary is adequately tested.
✓ Failure Modes
This PR correctly hardens the file-based attachment store against path traversal by resolving paths and validating that attachment IDs resolve to direct children of the uploads directory. The two-part check (is_relative_to AND parent equality) covers traversal via '..', subdirectory injection, absolute paths, empty strings, and '.' correctly. All existing calers (delete_attachment, read_attachment_bytes) benefit from the check since they call get_file_path, and the raised ValueError is appropriate behavior for invalid input. No silent failure modes are introduced.
✗ Design Approach
The boundary check in
get_file_pathis a good hardening step, but the newValueErrorpath is not carried through the request boundary: tampered attachment IDs now surface as 500s from the sample API instead of a client error. I found one design issue to address before merging.
Flagged Issues
-
get_file_pathnow raisesValueErrorfor invalid IDs, but both/upload/{attachment_id}and/preview/{attachment_id}catch that as a generic exception and return HTTP 500 (app.py:595,612-614,627,643-645). Malicious or malformed requests like/preview/.are correctly rejected by the store but still reported to clients as internal server failures instead of 4xx client errors.
Automated review by moonbox3's agents
|
Flagged issue
Source: automated DevFlow PR review |
There was a problem hiding this comment.
Pull request overview
This PR hardens the ChatKit integration sample’s local attachment store by constraining attachment path resolution to stay within the configured uploads directory, and adds tests to cover path traversal and related edge cases.
Changes:
- Resolve
uploads_dirto an absolute path at initialization to ensure consistent boundary checks. - Validate
attachment_id-derived paths inget_file_pathand raise on IDs that escape the uploads directory. - Add end-to-end sample tests exercising valid IDs and rejecting invalid/path-like IDs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/samples/05-end-to-end/chatkit-integration/attachment_store.py | Tightens attachment filesystem path resolution to enforce uploads directory boundaries. |
| python/tests/samples/end_to_end/test_chatkit_attachment_store.py | Adds coverage for valid IDs and rejects invalid/path-like attachment IDs. |
|
Addressed in ccf5270. Invalid attachment IDs now return HTTP 400 from both upload and preview routes, with focused regression coverage. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 90%
✓ Correctness
The PR correctly implements path traversal protection for the attachment store. The get_file_path method uses defense-in-depth: a syntactic pre-check (rejecting empty, '.', '..', '/', '') followed by a resolved-path verification ensuring the result is a direct child of uploads_dir. The HTTP endpoints properly catch ValueError and return 400. Internal calers (delete_attachment, read_attachment_bytes) allow the exception to propagate, which is appropriate since those receive IDs from the application's own data store. No correctness issues found.
✓ Security Reliability
The PR implements a well-structured path traversal defense for the attachment store. It applies syntactic pre-validation (rejecting separators, empty strings, dot-segments) combined with post-resolution verification (confirming the resolved path is a direct child of uploads_dir). The HTTP endpoints correctly catch ValueError and return 400 responses. The uploads_dir is resolved at init time, preventing TOCTOU issues. No security or reliability concerns were found.
✓ Test Coverage
Test coverage for this PR is solid. The parametrized unit test covers 9 invalid ID patterns, the happy path is verified, and the integration test confirms both HTTP routes return 400 for invalid IDs. The only notable gap is that
delete_attachmentandread_attachment_bytesalso callget_file_path(and thus inherit the new ValueError) but have no test documenting this behavior, though this is low severity since those are internal methods and the HTTP boundary is well covered.
✓ Failure Modes
The PR cleanly hardens attachment path resolution with both a pre-validation check (rejecting separators, empty, dot-segments) and a post-resolve check (parent must equal uploads_dir). ValueError is properly caught at both HTTP-facing boundaries and translated to 400 responses. Internal callers (delete_attachment, read_attachment_bytes) use system-generated IDs so the unhandled ValueError there is acceptable. No new silent failure modes, lost errors, or partial-write issues are introduced.
✗ Design Approach
The new filename validation is a good hardening step, but it is only translated at the upload and preview HTTP boundaries. One remaining attachment-read path now turns the same malformed IDs into an unhandled server error, so the storage boundary is still inconsistent.
Flagged Issues
-
get_file_path()now raisesValueErrorfor malformed IDs, butWeatherChatKitServer._fetch_attachment_data()still forwardsattachment_idstraight intoread_attachment_bytes()and the main request handler re-raises any exception from processing (app.py:249-258and:565-578). A thread item referencingattachment_id='.'will therefore fail the whole ChatKit request with a 500 instead of being rejected at the boundary the way/upload/{attachment_id}and/preview/{attachment_id}now are.
Automated review by moonbox3's agents
|
Flagged issue
Source: automated DevFlow PR review |
False positive. No action. |
Motivation & Context
A local store should honor its own boundary.
Description & Review Guide
Related Issue
N/A
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.