Fix audit log integrity bugs and CI warnings - #323
Merged
Conversation
WalkthroughAudit and signing audit subsystems: enum discriminants added, hashing switched to presence flags and length-prefixed fields, storage traits extended with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
keep-mobile/src/audit.rs (1)
186-199:⚠️ Potential issue | 🟡 MinorMove max-entry check inside the
last_hashlock to avoid a race window.At Line 186, the count check happens before mutex acquisition; concurrent callers can both pass the check and exceed
MAX_AUDIT_ENTRIES. Same pattern exists in the signing path after the signature expansion.Suggested fix
- let entry_count = self.storage.entry_count()? as usize; - if entry_count >= MAX_AUDIT_ENTRIES { - return Err(KeepMobileError::StorageError { - msg: format!("Audit log full: {entry_count} entries (max {MAX_AUDIT_ENTRIES})"), - }); - } - let mut last_hash = self .last_hash .lock() .map_err(|_| KeepMobileError::StorageError { msg: "Lock poisoned".into(), })?; + + let entry_count = self.storage.entry_count()? as usize; + if entry_count >= MAX_AUDIT_ENTRIES { + return Err(KeepMobileError::StorageError { + msg: format!("Audit log full: {entry_count} entries (max {MAX_AUDIT_ENTRIES})"), + }); + }Mirror this change in
SigningAuditLog::log_event.Also applies to: 490-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-mobile/src/audit.rs` around lines 186 - 199, The entry-count check currently happens before acquiring the last_hash mutex which allows a race where two callers both see space and then exceed MAX_AUDIT_ENTRIES; move the entry_count = self.storage.entry_count()? as usize and the comparison against MAX_AUDIT_ENTRIES to occur while holding the last_hash lock (i.e., after acquiring self.last_hash.lock()), so the check-and-reserve is atomic with respect to other loggers; apply the same change to the signing path and duplicate the fix inside SigningAuditLog::log_event (including the check after signature expansion) so both code paths perform the count check under the same mutex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-mobile/src/audit.rs`:
- Around line 96-115: The verify() logic currently only recomputes hashes using
the new schema (presence flags + length prefixes) so old persisted entries will
fail; update verify() to accept both formats by (a) attempting to recompute the
hash using the new serialization and, if that fails, recomputing using the
legacy flat serialization (or detect format via a leading version/presence
marker) and (b) when writing new entries, emit a clear version byte/prefix so
future reads know which schema to use; apply the same dual-format verification
and versioned writing changes to SigningAuditEntry as well (ensure
functions/methods named verify(), SigningAuditEntry
serialization/deserialization paths, and the blocks that push presence flags +
length prefixes are updated).
- Around line 135-136: Add the missing AuditStorage interface declarations to
keep_mobile.udl so the new Rust trait methods are exported to bindings: declare
a [Trait, WithForeign] interface AuditStorage that includes the existing
store_entry and load_entries signatures plus the two new methods as UDL
signatures ([Throws=KeepMobileError] u32 entry_count(); and
[Throws=KeepMobileError] void clear_entries(string confirm);). Ensure the method
names match the Rust trait symbols entry_count() and clear_entries(confirm:
String) and that the interface is included in the file consumed by
uniffi::generate_scaffolding("src/keep_mobile.udl").
---
Outside diff comments:
In `@keep-mobile/src/audit.rs`:
- Around line 186-199: The entry-count check currently happens before acquiring
the last_hash mutex which allows a race where two callers both see space and
then exceed MAX_AUDIT_ENTRIES; move the entry_count =
self.storage.entry_count()? as usize and the comparison against
MAX_AUDIT_ENTRIES to occur while holding the last_hash lock (i.e., after
acquiring self.last_hash.lock()), so the check-and-reserve is atomic with
respect to other loggers; apply the same change to the signing path and
duplicate the fix inside SigningAuditLog::log_event (including the check after
signature expansion) so both code paths perform the count check under the same
mutex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10067503-091b-4c2a-8c5e-e1b5c22c0711
📒 Files selected for processing (2)
keep-mobile/src/audit.rskeep-mobile/src/lib.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
keep-mobile/src/audit.rs (1)
96-115:⚠️ Potential issue | 🟠 MajorPreserve audit-chain compatibility across hash schema upgrades.
On Line [125] and Line [429], verification recomputes only the current hash layout. Entries persisted with the prior layout will be reported as tampered after upgrade, which breaks historical chain verification.
Proposed direction (versioned hash format + dual verification)
#[derive(uniffi::Record, Clone, Debug, Serialize, Deserialize)] pub struct AuditEntry { + #[serde(default = "default_hash_format")] + pub hash_format: u8, pub timestamp: i64, ... } +const HASH_FORMAT_LEGACY: u8 = 1; +const HASH_FORMAT_V2: u8 = 2; +fn default_hash_format() -> u8 { HASH_FORMAT_LEGACY } impl AuditEntry { pub fn finalize(mut self) -> Self { - self.hash = self.compute_hash().to_vec(); + self.hash_format = HASH_FORMAT_V2; + self.hash = self.compute_hash_v2().to_vec(); self } fn verify(&self, prev_hash: &[u8]) -> bool { ... - let computed = self.compute_hash(); + let computed = match self.hash_format { + HASH_FORMAT_V2 => self.compute_hash_v2(), + HASH_FORMAT_LEGACY => self.compute_hash_legacy(), + _ => return false, + }; ... } }Apply the same pattern to
SigningAuditEntry.Also applies to: 120-127, 395-419, 424-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-mobile/src/audit.rs` around lines 96 - 115, The current serialization for audit entries (see fields used in the shown block: event_type, pubkey, success, details) does not include a version marker, and verification code (recompute in functions around lines 125 and 429) only attempts the current hash layout so older-format entries appear tampered after a hash-schema upgrade; fix by adopting a versioned hash format for SigningAuditEntry and the other audit entry serializers (emit a single version byte at the start of the serialized record) and update verification to perform dual verification: first attempt the current (vX) hash verification, and if that fails, attempt the prior (vY) hash verification path before flagging tampering. Locate and change the serializer/deserializer/hasher implementations for SigningAuditEntry and the corresponding verify function(s) referenced in the diff to include the version byte and add the fallback verification branch for older schema(s).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@keep-mobile/src/audit.rs`:
- Around line 96-115: The current serialization for audit entries (see fields
used in the shown block: event_type, pubkey, success, details) does not include
a version marker, and verification code (recompute in functions around lines 125
and 429) only attempts the current hash layout so older-format entries appear
tampered after a hash-schema upgrade; fix by adopting a versioned hash format
for SigningAuditEntry and the other audit entry serializers (emit a single
version byte at the start of the serialized record) and update verification to
perform dual verification: first attempt the current (vX) hash verification, and
if that fails, attempt the prior (vY) hash verification path before flagging
tampering. Locate and change the serializer/deserializer/hasher implementations
for SigningAuditEntry and the corresponding verify function(s) referenced in the
diff to include the version byte and add the fallback verification branch for
older schema(s).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 061265dc-6a8d-4d8b-86d8-7c2abd8eacd3
📒 Files selected for processing (2)
keep-mobile/src/audit.rskeep-mobile/src/lib.rs
4 tasks
This was referenced Jul 28, 2026
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
2 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 by CodeRabbit
Refactor
Bug Fix