Repository navigation
BE-930: Add Workspace::snapshot - #10002
thehabbos007 wants to merge 1 commit into
Conversation
github-actions
Bot
added
area/libs
![semgrep-code-hashintel[bot]](https://avatars.githubusercontent.com/u/22116112?s=60&v=4){kind=small}
| .parent() | ||
| .filter(|parent| !parent.as_os_str().is_empty()) | ||
| .unwrap_or_else(|| Path::new(".")); | ||
| File::open(directory)?.sync_all() |
There was a problem hiding this comment.
🟠 High severity issue identified in your code:
The application builds a file path from potentially untrusted data, which can lead to a path traversal vulnerability. An attacker can manipulate the path which the application uses to access files. If the application does not validate user input and sanitize file paths, sensitive files such as configuration or user data can be accessed, potentially creating or overwriting files. To prevent this vulnerability, validate and sanitize any input that is used to create references to file paths. Also, enforce strict file access controls. For example, choose privileges allowing public-facing applications to access only the required files.
🧹 Fixed in commit 4341978 🧹
There was a problem hiding this comment.
/fp Workspace::snapshot is a Rust filesystem API whose caller selects a trusted destination. Writing to a caller-chosen path outside the database directory is supported behavior. This library has no HTTP handler or untrusted request-to-path dataflow. Restricting the destination would change the API contract.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## asa/hash-integrations/04-engine-workspace #10002 +/- ##
==========================================================================
Coverage 68.29% 68.29%
==========================================================================
Files 2003 2003
Lines 205743 205815 +72
Branches 8734 8739 +5
==========================================================================
+ Hits 140513 140568 +55
- Misses 63554 63565 +11
- Partials 1676 1682 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4341978 to
231b26a
Compare
231b26a to
760587e
Compare
760587e to
13a1b5f
Compare
13a1b5f to
5d48946
Compare
5d48946 to
856c763
Compare
856c763 to
df216b1
Compare
PR SummaryMedium Risk Overview The workspace thread CHECKPOINTs, then copies the live database via a Integration tests cover snapshot timing vs later Reviewed by Cursor Bugbot for commit a561bc2. Bugbot is set up for automated code reviews on this repo. Configure here. |
df216b1 to
ae04656
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| [&self.database, &self.log, &self.temp, &self.lock] | ||
| .into_iter() | ||
| .any(|file| Self::resolve(file).is_some_and(|file| file == path)) | ||
| } |
There was a problem hiding this comment.
Snapshot can overwrite live database
High Severity
WorkspaceFiles::contains only canonicalizes parent directories and then compares the original filename, so it does not treat a file symlink and its target as the same path. A workspace opened through a symlink can therefore accept a snapshot whose target or target.partial is the real database file. write_copy then unlinks that live file with rename (or remove_file on the partial path) while DuckDB still holds the original inode, which can drop later writes and leave WAL and data files out of sync.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ae04656. Configure here.
ae04656 to
28a45a9
Compare
28a45a9 to
a561bc2
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a561bc2. Configure here.
| .parent() | ||
| .filter(|parent| !parent.as_os_str().is_empty()) | ||
| .unwrap_or_else(|| Path::new(".")); | ||
| File::open(directory)?.sync_all() |
There was a problem hiding this comment.
Snapshot replace leaves DuckDB sidecars
Medium Severity
snapshot replaces only the main file at target. A leftover <target>.wal (or .tmp) from a previous database at that path is left in place. DuckDB then replays that log against the new copy and can refuse to open it with a WAL mismatch, so an in-place snapshot is not always a database DuckDB can open on its own.
Reviewed by Cursor Bugbot for commit a561bc2. Configure here.
This branch had an error being deployed
2 participants
🌟 Purpose
This PR adds
Workspace::snapshot(target), which writes a copy of the database that DuckDB can open on its own.🔍 Changes
snapshotrunsCHECKPOINT, copies the database file to a new<target>.partial, syncs it and renames it totarget.targetonly ever holds a complete copy.<target>.partialis deleted first, because copying onto a link to the database file would truncate the database.<target>.partial.