Security hardening: path traversal, resource limits, and documentation#42
Merged
jjhafer merged 10 commits intosilvermine:masterfrom Feb 25, 2026
Merged
Conversation
jjhafer
requested changes
Feb 25, 2026
pmorris-dev
force-pushed
the
vulnerabilities-audit
branch
3 times, most recently
from
6622ab5 to
a58a4fe
Compare
velocitysystems
suggested changes
Feb 25, 2026
These were not exploitable vulnerabilities but best practice is to fix them Use the pragma_table_info() table-valued function in the observer crate so the table name is bound as a parameter rather than interpolated into the SQL string. Quote schema names in DETACH DATABASE statements in the conn-mgr crate for defense-in-depth alongside the existing allowlist validation.
resolve_database_path() passed user input directly to PathBuf::join() without validation. Since join() replaces the base on absolute paths and preserves ".." segments, a malicious path could escape the app config directory. Add multi-layer validation in validate_and_resolve(): reject null bytes, absolute paths, and parent-directory components, then canonicalize and verify containment. In-memory database paths are passed through unchanged. Refactor resolve_migration_path() to delegate to resolve_database_path() so all entry points share the same validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pmorris-dev
force-pushed
the
vulnerabilities-audit
branch
from
a58a4fe to
fc650e1
Compare
begin_interruptible_transaction holds the exclusive writer until the frontend commits or rolls back. If the frontend never does, the writer is locked indefinitely. Add a configurable idle timeout (default 5 min) with lazy cleanup: expired transactions are evicted on the next insert or rejected on the next remove, auto-rolling back via Drop. Convert ActiveInterruptibleTransactions from a tuple struct to named fields with a timeout Duration, and add a created_at Instant to each ActiveInterruptibleTransaction. Expose Builder::transaction_timeout() in the plugin for configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pmorris-dev
force-pushed
the
vulnerabilities-audit
branch
from
fc650e1 to
5643729
Compare
observe() passed user-supplied channel_capacity directly to tokio::sync::broadcast::channel() without bounds. A capacity of 0 panics, and a very large value pre-allocates an enormous ring buffer. Add an assertion in ObservationBroker::new() to reject zero capacity at the crate level, preventing a panic regardless of how the crate is used. In the plugin layer, validate that channel_capacity is between 1 and 10,000 before applying it, returning an INVALID_CONFIG error otherwise. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Unbounded load() calls can exhaust memory via connection pool proliferation. Add a configurable max (default 50) to DbInstances, rejecting new load() calls with TOO_MANY_DATABASES when the limit is reached. Convert DbInstances from a tuple struct to named fields (inner, max) and expose Builder::max_databases() for configuration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
observe() and subscribe() accepted unbounded input that could exhaust memory and spawn unlimited tasks. Add validation: observe() rejects empty or >100 table lists, subscribe() rejects >100 subscriptions per database. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add Security Considerations section to README covering shared state across windows, resource limits, unbounded fetchAll, and path validation. Add @remarks to the Database class and fetchAll() in TypeScript, and a doc comment to the Rust fetch_all command. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pmorris-dev
force-pushed
the
vulnerabilities-audit
branch
from
5643729 to
5b5c409
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jjhafer
approved these changes
Feb 25, 2026
Contributor
jjhafer
left a comment
jjhafer
left a comment
There was a problem hiding this comment.
@pmorris-dev Minor comments, up to you if they deserve further evaluation.
The path traversal validation introduced in e58189d canonicalizes the parent directory to verify containment, but this fails with an IO error when intermediate directories in a relative path (e.g. "subdir/mydb.db") do not yet exist. The previous implementation returned the joined path without requiring the parent to exist. Call create_dir_all on the parent before canonicalizing so that nested relative paths work without the caller needing to pre-create subdirectories. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Setting max_databases to 0 prevents any database from loading since the length check always fails. Similarly, a zero transaction_timeout causes every interruptible transaction to expire immediately. Both are obvious misconfigurations that silently break the plugin. Return Error::InvalidConfig from the builder setters so callers get a clear error at configuration time rather than puzzling runtime behavior. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
velocitysystems
approved these changes
Feb 25, 2026
Contributor
velocitysystems
left a comment
velocitysystems
left a comment
There was a problem hiding this comment.
Looks great @pmorris-dev.
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
3 participants
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.
Summary
Addresses vulnerabilities identified in a comprehensive security audit:
cases did not appear to be exploitable, hardened SQL identifier handling
against injection.
paths against directory escape via absolute paths,
..segments, andnull bytes. Refactor
resolve_migration_path()to share the samevalidation.
min) for interruptible transactions that hold the exclusive writer.
Expired transactions are lazily cleaned up on the next access attempt.
channel_capacityof 0 at thecrate level (
ObservationBroker::new()) to prevent abroadcast::channelpanic, and cap at 10,000 at the plugin level.(configurable via
Builder::max_databases()).observe()call and active subscriptions at 100 per database.covering shared state, resource limits, unbounded
fetchAll, and pathvalidation. Add
@remarksto TypeScriptDatabaseclass andfetchAll().Verification
cargo test --workspacepasses (includes new unit tests for pathvalidation, transaction timeout expiry/eviction, and error codes)
npm run standardsclean (eslint, type-check, markdownlint, clippy,fmt, commitlint)