Feature: More events and event refinement #284
Conversation
WalkthroughThe pull request introduces a new event handling mechanism across several components within the cryptographic filesystem. In the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![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
🧹 Nitpick comments (2)
src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java (2)
99-100: Verify event emission for empty directory fileTest verifies that the event consumer is called with a
BrokenDirFileEventwhen an empty directory file is encountered.Consider enhancing this test to also verify the content of the event (e.g., check that the path in the event matches
dirFilePath). This would provide more thorough test coverage.-var isBrokenDirFileEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof BrokenDirFileEvent; -verify(eventConsumer).accept(ArgumentMatchers.argThat(isBrokenDirFileEvent)); +var isBrokenDirFileEvent = (ArgumentMatcher<FilesystemEvent>) ev -> + ev instanceof BrokenDirFileEvent brokenEvent && brokenEvent.path().equals(dirFilePath); +verify(eventConsumer).accept(ArgumentMatchers.argThat(isBrokenDirFileEvent));
113-114: Verify event emission for oversized directory fileSimilar to the previous case, test verifies event emission for the oversized directory file scenario.
Same suggestion as above - consider enhancing the test to verify the content of the event for more thorough coverage.
-var isBrokenDirFileEvent = (ArgumentMatcher<FilesystemEvent>) ev -> ev instanceof BrokenDirFileEvent; -verify(eventConsumer).accept(ArgumentMatchers.argThat(isBrokenDirFileEvent)); +var isBrokenDirFileEvent = (ArgumentMatcher<FilesystemEvent>) ev -> + ev instanceof BrokenDirFileEvent brokenEvent && brokenEvent.path().equals(dirFilePath); +verify(eventConsumer).accept(ArgumentMatchers.argThat(isBrokenDirFileEvent));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java(5 hunks)src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java(2 hunks)src/main/java/org/cryptomator/cryptofs/event/BrokenDirFileEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/BrokenFileNodeEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/DecryptionFailedEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/event/FilesystemEvent.java(1 hunks)src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java(1 hunks)src/test/java/org/cryptomator/cryptofs/CryptoPathMapperTest.java(18 hunks)src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java(4 hunks)src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyse
🔇 Additional comments (27)
src/main/java/org/cryptomator/cryptofs/event/BrokenDirFileEvent.java (1)
1-12: Well-structured event implementationThis new event record is clearly documented and properly implements the FilesystemEvent interface. The documentation effectively explains the conditions that trigger this event (empty or oversized dir.c9r file).
src/main/java/org/cryptomator/cryptofs/event/BrokenFileNodeEvent.java (1)
1-15: Good implementation with comprehensive documentationThe event record is well-designed with both cleartext and ciphertext paths, providing complete context about the broken file node. The documentation clearly explains its purpose and includes a useful cross-reference to UnknownType.
src/main/java/org/cryptomator/cryptofs/event/DecryptionFailedEvent.java (1)
11-11: Improved exception handling flexibilityGood change to broaden the exception type from a specific
AuthenticationFailedExceptionto genericException. This allows for more comprehensive error reporting by capturing all possible decryption failures, not just authentication issues.src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java (1)
83-83: Simplified error handling consistent with event changesThis change aligns with the modification to
DecryptionFailedEvent, properly passing any exception to the event consumer. The simplification improves the error reporting by treating all exceptions uniformly.src/main/java/org/cryptomator/cryptofs/event/FilesystemEvent.java (1)
25-25: Expansion of FilesystemEvent interface to include new event typesThe sealed interface has been updated to permit two new event types:
BrokenDirFileEventandBrokenFileNodeEvent. This aligns with the PR objective of introducing events for detecting broken directory files and file nodes.src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java (5)
4-5: Added imports for new event typeProperly imported the necessary event-related classes.
17-17: Added Consumer importImport for Java's Consumer functional interface to support event notifications.
24-29: Added event consumer field and constructor parameterEvent consumer is properly injected through constructor, following good dependency injection practices.
37-38: Emit event before throwing exception for empty directory fileCorrectly notifies the system about broken directory files by emitting a
BrokenDirFileEventbefore throwing the exception, enhancing error reporting.
40-41: Emit event before throwing exception for oversized directory fileSimilar to the empty file case, properly notifies about oversized directory files before throwing the exception.
src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java (4)
3-4: Added imports for event classesNecessary imports for the new event types being tested.
9-10: Added Mockito-related and Consumer importsAdded the required imports to support mocking and testing the event consumer functionality.
Also applies to: 20-20, 26-27, 29-29
38-40: Added mock event consumerCreated a mock event consumer and passed it to the constructor, aligning with the changes in the implementation class.
47-47: Setup mock behavior for event consumerConfigured the mock to do nothing when accept is called, allowing verification of interactions later.
src/test/java/org/cryptomator/cryptofs/CryptoPathMapperTest.java (5)
13-13: Added FilesystemEvent importRequired import for the event functionality.
31-31: Added Consumer and static Mockito importsNecessary imports to support the event consumer functionality in tests.
Also applies to: 35-35
48-48: Added mock event consumer fieldCreated a mock event consumer to be used across test methods.
82-83: Updated CryptoPathMapper constructor calls to include event consumerProperly updated constructor calls to pass the event consumer parameter, keeping the tests aligned with implementation changes.
Also applies to: 106-107, 140-141, 178-179, 215-216
263-264: Updated remaining CryptoPathMapper constructor callsConsistently updated all constructor calls across test methods to include the event consumer parameter.
Also applies to: 272-273, 285-286, 301-302, 317-318, 335-336, 396-397, 411-412, 427-428, 443-444
src/test/java/org/cryptomator/cryptofs/fh/FileHeaderHolderTest.java (2)
15-16: LGTM! Good addition of parameterized testing.Adding support for parameterized tests will make the test suite more maintainable.
93-96: Great refactoring to use parameterized tests!Converting the individual test methods into a single parameterized test that handles both
AuthenticationFailedExceptionandIllegalArgumentExceptionreduces code duplication while maintaining the same test coverage. This change aligns with the updatedDecryptionFailedEventthat now accepts general exceptions rather than specific authentication exceptions.src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java (6)
16-17: LGTM! Good addition of necessary imports.These imports support the new event handling functionality for broken file nodes.
Also applies to: 32-32
47-47: LGTM! Well-placed field for event handling.Adding a dedicated field for the event consumer follows good object-oriented design principles.
54-54: LGTM! Constructor updated to support event handling.Good practice to inject the event consumer through the constructor, following dependency injection principles.
60-60: LGTM! Proper field initialization.Field initialization is straightforward and follows the class's initialization pattern.
106-106: Good implementation of event notification for broken file nodes.This line properly notifies the system when a broken file node is detected, which aligns with the PR objective to improve error handling for directory and file nodes.
120-120:❓ Verification inconclusive
Address the TODO comment about cleartext paths in logs.
The TODO comment indicates a potential security concern with logging cleartext paths. Consider implementing this improvement to prevent potential information leakage in logs.
🏁 Script executed:
#!/bin/bash # Search for other instances of cleartext paths in logs to see if there's a pattern or standard approach rg -n "cleartext.*log|log.*cleartext" --type javaLength of output: 51
Security Alert: Potential Cleartext Logging Vulnerability
At
src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java(line 120), the exception message includes the rawcleartextPath, which could lead to sensitive information leakage if logged. Please review this and consider one of the following improvements:
- Remove or Sanitize the File Path: Modify the exception message so that it does not reveal the full cleartext path. If the path (or part of it) is essential for debugging, consider redacting sensitive segments.
- Ensure Consistent Handling: Verify that this pattern isn’t repeated elsewhere in the codebase—if it is, establish a standardized, secure logging approach across all modules.
Given that our automated search for similar logging patterns returned no additional instances, please manually verify whether this instance is an isolated occurrence or part of a broader pattern that needs addressing.
This PR adds mainly to new events: