fix: issues with database implementation #20
Conversation
WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Node
participant Logger
Client->>API: Submit encrypted indicators
API->>API: Decrypt all indicators, filter failures
API->>Logger: Log decryption errors (if any)
loop For each decrypted indicator
API->>Node: Attempt to add/increment indicator
alt Success
API->>API: Increment success count
else Failure
API->>Logger: Log insertion error
end
end
API-->>Client: Return count of successful insertions
Possibly related PRs
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
cadenkoj
changed the title
cadenkoj
changed the base branch from
05-21-feat_db_rough_database_implementation
to
graphite-base/20
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.env.example(1 hunks)diesel.toml(1 hunks)migrations/2025-05-21-234457_create_encrypted_indicators/down.sql(1 hunks)src/api.rs(1 hunks)src/crypto/mesh_identity.rs(2 hunks)src/crypto/symmetric_key_manager.rs(5 hunks)src/logging.rs(2 hunks)src/models.rs(3 hunks)src/node.rs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/api.rs (3)
src/node.rs (1)
indicators(186-189)src/crypto/symmetric_key_manager.rs (1)
decrypt(114-138)src/models.rs (1)
decrypt(103-118)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test-lint
🔇 Additional comments (23)
diesel.toml (1)
9-9: Excellent improvement: switched to relative path for migrationsChanging from an absolute path to a relative path makes the configuration portable across different development environments. This prevents path-related issues when different team members work on the project.
src/crypto/mesh_identity.rs (2)
5-5: Good imports to support secure file operationsThe imports correctly include the necessary components for file operations with specific permissions.
Also applies to: 7-7
52-65:Details
✅ Verification successful
Excellent security improvement: restricted file permissions for key files
Setting file permissions to mode 0o600 (owner read/write only) ensures that cryptographic key material is accessible only to the process owner, which is a crucial security practice.
🏁 Script executed:
#!/bin/bash # Check if this implementation might have issues on non-Unix platforms echo "Checking for Windows-specific handling for file permissions:" rg -A 5 "OpenOptionsExt" --glob "!**/mesh_identity.rs" -C 5Length of output: 991
Consistent Unix file-permission handling confirmed
Setting mode 0o600 for the private and public key files is a strong security improvement and aligns with the existing use of
std::os::unix::fs::OpenOptionsExtinsymmetric_key_manager.rs. There are no new cross-platform concerns beyond those already present.src/logging.rs (2)
4-4: Clean import organizationExplicitly importing the Log trait improves code readability.
109-109: Code cleanup: simplified trait implementationRemoving the
log::prefix makes the code cleaner while maintaining the same functionality..env.example (3)
2-3: Good addition: centralized logging configurationAdding a standardized
RUST_LOGenvironment variable enables consistent application-level logging and follows Rust ecosystem conventions.
6-6: Smart configuration: unified log level referenceReferencing
RUST_LOGfromDTIM__DEFAULT__LOG_LEVELcreates a single source of truth for log level configuration.
9-9: Improved documentation: clearer database configuration guidanceThe enhanced comments provide better guidance for users, especially regarding password usage in production and the relationship between different database connection strings.
Also applies to: 11-11
migrations/2025-05-21-234457_create_encrypted_indicators/down.sql (1)
1-1: Improved database safety withIF EXISTSandCASCADEThe addition of
IF EXISTSprevents errors when the table doesn't exist, andCASCADEproperly handles dependent objects during table removal. This makes the migration more robust during rollbacks.src/api.rs (2)
203-206: Improved error handling for indicator decryptionThe change properly filters out indicators that fail to decrypt instead of potentially crashing the entire process. This gracefully handles decryption failures while still processing valid indicators.
209-213: Better error resilience in indicator processingThe code now continues processing remaining indicators even when some fail to insert, logging errors instead of failing the entire batch. This improves system resilience by allowing partial success instead of complete failure.
src/crypto/symmetric_key_manager.rs (5)
4-6: Added imports for secure file operationsThese imports support the secure file permission controls added throughout the file.
30-39: Improved key loading with better error handlingThe code now properly checks both file existence AND successful reading before using a key, with graceful fallback to key generation. This prevents using partially read or corrupted key files.
61-72: Enhanced key generation with secure file permissionsThe function now creates key files with proper Unix permissions (0o600 = owner read/write only) using OpenOptions, significantly improving security by preventing other users from accessing the key files.
82-94: Secure handling of previous keys during rotationKey rotation now properly sets secure permissions (0o600) for the previous key file and adds cleanup logic to remove stale files when appropriate. This maintains the security of key material throughout its lifecycle.
121-123: Added nonce validation to prevent cryptographic errorsEarly validation of nonce length prevents cryptographic operation failures later in the process and provides a clear error message.
src/node.rs (3)
84-85: Added conflict handling for database insertionsThe inclusion of
on_conflict(id).do_nothing()prevents database errors when inserting duplicate indicators. This aligns with the changes in the API handler, which now logs errors instead of failing when indicators can't be inserted.Also applies to: 89-93
96-99: Improved logging to prevent sensitive data exposureThe log message now only includes the indicator ID instead of the entire record, reducing the risk of sensitive data exposure in logs.
189-189: Standardized error handling with std::io::ErrorThe change from string errors to
std::io::Errorstandardizes error handling across the codebase, making error propagation more consistent and providing better error context.src/models.rs (4)
75-77: Great improvement for deterministic ID computation!Sorting tags before hashing ensures consistent UUID generation regardless of the original tag order. This is a solid fix that prevents the same set of tags in different orders from producing different UUIDs.
88-89: Good error handling improvement.Converting serialization failures into proper
std::io::Errorinstances rather than panicking improves robustness. This change properly propagates errors to the caller, allowing for graceful error handling.
106-106: Well-executed error standardization.The changes to the
decryptmethod correctly standardize error handling by:
- Changing the return type to consistently use
std::io::Error- Properly handling both decryption and deserialization errors with detailed error messages
This aligns with the PR objective of improving error handling throughout the codebase.
Also applies to: 113-117
65-82:Details
✅ Verification successful
Verify consistent hash generation in other methods.
The
compute_idmethod now properly sorts tags, but note that a similar pattern exists in theHashimplementation forThreatIndicator(lines 258-275). Confirm that both implementations are consistent.
🏁 Script executed:
#!/bin/bash # Check if the Hash implementation for ThreatIndicator also sorts tags echo "Checking Hash implementation in the same file..." # The implementation does sort tags (lines 262-264) cat src/models.rs | awk 'NR>=258 && NR<=275' echo "Checking if sorting is used in all places that compute hashes from tags..." rg -A 5 -B 5 "tags\.(\w+\.)*(sort|collect)" --type rustLength of output: 2121
No action needed—tag sorting is consistent
Both
compute_idand thestd::hash::Hashimplementation forThreatIndicatorsorttagslexicographically before hashing, ensuring identical ordering and consistent hash/UUID generation across methods.
| /// FIXME: `add_indicator` inserts unconditionally – handle primary-key clashes. | ||
| /// If an indicator with the same deterministic UUID already exists, this insertion will violate the primary-key constraint and bubble an error. | ||
| /// Possible solution: | ||
| /// Use ON CONFLICT (id) DO UPDATE (diesel::upsert) to increment sightings. (TODO: possibly make sightings unencrypted metadata, TBD) | ||
| /// Failing to do so exposes the API to 500s on legitimate duplicate submissions. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Documented known issue with primary key conflicts
The detailed FIXME comment clearly explains the current limitation with indicator insertion and proposes a solution using Diesel's upsert functionality. This is good documentation for future improvements.
Consider implementing the suggested fix using Diesel's upsert to increment sightings on conflict, as this would further improve robustness:
- let res = diesel::insert_into(encrypted_indicators)
- .values(&encrypted)
- .returning(EncryptedIndicator::as_returning())
- .get_result(&mut conn)?;
+ let res = diesel::insert_into(encrypted_indicators)
+ .values(&encrypted)
+ .on_conflict(id)
+ .do_update()
+ .set(ciphertext.eq(excluded(ciphertext))) // Update with the new values
+ .returning(EncryptedIndicator::as_returning())
+ .get_result(&mut conn)?;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/node.rs around lines 107 to 111, the current add_indicator function
inserts indicators unconditionally, causing primary-key conflicts and errors on
duplicates. To fix this, refactor the insertion logic to use Diesel's upsert
feature with ON CONFLICT (id) DO UPDATE, incrementing the sightings count
instead of failing. This will prevent 500 errors on duplicate submissions and
improve API robustness.
chore(env): provide detailed comments
fix(db): use relative migrations path
chore(db): safely drop tables on down
fix(api): minimise lock contention
chore: refactor
fix: sort tags when computing id hash
fix(db): handle insert conflicts, secure debug logs
fix(crypto): set keyfile permissions
fix(crypto): handle missing or corrupt keyfiles
Summary by CodeRabbit
Bug Fixes
New Features
Chores