-
Notifications
You must be signed in to change notification settings - Fork 4
Expand SQLite3 data validation #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Expand SQLite3 data validation #23
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 146 146
Lines 3881 3914 +33
=====================================
- Misses 3881 3914 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Schamper
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a benchmark test too? I'll look at the actual checksum checking part later when I have a bit more time.
| fh: Path | BinaryIO, | ||
| wal: WAL | Path | BinaryIO | None = None, | ||
| checkpoint: Checkpoint | int | None = None, | ||
| validate_checksum: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| validate_checksum: bool = False, | |
| *, | |
| validate_checksums: bool = False, |
Can you also add it to the docstring?
|
|
||
| @property | ||
| def valid(self) -> bool: | ||
| def valid(self, validate_checksum: bool = True) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe change this to a verb-style name now that it's a method. is_valid?
| def validate_salt(self) -> bool: | ||
| """Check if the frame's salt values match those in the WAL header. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def validate_salt(self) -> bool: | |
| """Check if the frame's salt values match those in the WAL header. | |
| def is_valid_salt(self) -> bool: | |
| """Return whether the frame's salt values match those in the WAL header. |
| def validate_checksum(self) -> bool: | ||
| """Check if the frame's checksum matches the calculated checksum. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def validate_checksum(self) -> bool: | |
| """Check if the frame's checksum matches the calculated checksum. | |
| def is_valid_checksum(self) -> bool: | |
| """Return whether the frame's checksum matches the calculated checksum. |
|
Take your time, I don't think I will be doing a whole lot of Dissect dev to coming weeks ... |
This PR close #16 by expanding the data validation capabilities in SQLite3.
The SQLite3 WAL file can store multiple versions of the same frame, when reading only valid frames should be returned. The docs define a valid frame as follows:
The first check was already implemented, I have interpreted the second check as:
When initializing a database the option
validate_checksumcan be passed to use the new validation. I have chosen to only calculate the salts by default (just like before) as this will probably be good enough, and a lot faster. See the example below for the time impact: