[feature] Desensitize sensitive information(new) #13
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (10)
Tip CodeRabbit can use Shopify Theme Check to improve the quality of Shopify theme reviews.Add a configuration file to your project to customize how CodeRabbit runs Shopify Theme Check. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @arvi18, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented a new feature to enhance privacy by desensitizing sensitive information, specifically phone numbers and email addresses, for notification recipients. This change introduces automatic masking of these details when displayed, while also incorporating a caching mechanism to temporarily store the original values. This caching allows the system to correctly restore the unmasked contact information when users need to edit recipient details or send test messages, ensuring data integrity during these operations. Additionally, I've introduced a new value object for notification recipients to support multiple communication channels and integrated the Hutool utility library to assist with data manipulation.
Highlights
- Sensitive Data Desensitization: Implemented automatic masking of phone numbers and email addresses for notification recipients to improve privacy and data security.
- Original Data Caching: Introduced a caching mechanism to temporarily store the original, unmasked contact information (phone and email) when recipients are viewed, allowing for restoration.
- Data Restoration for Operations: Ensured that the original contact details are correctly restored from the cache when editing notification recipients or sending test messages, preventing operations on masked data.
- New Notification Recipient Value Object: Created a new value object,
NoticeReceiverVO, to support multiple notification channels and integrate the new desensitization features. - New Project Dependency: Added the Hutool utility library as a new project dependency, providing various utility functions used in the desensitization process.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
🤖 Refacto.AI: PR Summary & Reviewer Guide🎯 1. The Goal (Why was this PR raised?)This PR aims to implement data desensitization for sensitive information such as email addresses and phone numbers in the HertzBeat application. The goal is to protect user privacy by masking sensitive data when it's displayed in the application interface. 🗺️ 2. The Changes (What was implemented?)
🤔 3. Key Areas for Human ReviewArea of Concern: Desensitization Logic Implementation
Area of Concern: Handling of Original Values in Edit Operations
Area of Concern: Cache Implementation for Desensitization Mappings
Area of Concern: API Response Transformation
|
There was a problem hiding this comment.
Code Review
This pull request introduces a feature to desensitize sensitive information like phone numbers and emails in notification recipients. The implementation uses custom Jackson serializers and a caching mechanism to store and restore original values. The changes are generally well-structured, but there are several areas for improvement regarding dependency management, code style, test correctness, and code duplication. I've provided specific comments with suggestions to address these points.
| public void test() throws JsonProcessingException { | ||
| NoticeReceiverVO noticeReceiver = new NoticeReceiverVO(); | ||
| noticeReceiver.setId(1L); | ||
| noticeReceiver.setEmail("[email protected]"); | ||
|
|
||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| SimpleModule simpleModule = new SimpleModule(); | ||
| simpleModule.addSerializer(String.class,emailDesensitizationSerializer); | ||
|
|
||
| objectMapper.registerModule(simpleModule); | ||
|
|
||
| String jsonString = objectMapper.writeValueAsString(noticeReceiver); | ||
| NoticeReceiverVO noticeReceiverVO = objectMapper.readValue(jsonString, NoticeReceiverVO.class); | ||
|
|
||
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
|
|
||
| assertEquals(noticeReceiver.getEmail(),desensitizationMapCache.get(noticeReceiverVO.getId()+"_"+noticeReceiverVO.getEmail())); | ||
| } |
There was a problem hiding this comment.
The test setup is incorrect. By registering the serializer for String.class (simpleModule.addSerializer(String.class,emailDesensitizationSerializer)), you are applying it to all string properties of NoticeReceiverVO, not just the email field as intended by the @JsonSerialize annotation. This can lead to unexpected side effects and does not accurately test the production behavior.
The test should rely on the @JsonSerialize annotation on the NoticeReceiverVO.email field to be picked up by Jackson. Also, it's good practice to assert that the serialized JSON contains the desensitized value.
Please refactor the test to correctly reflect its usage.
public void test() throws JsonProcessingException {
NoticeReceiverVO noticeReceiver = new NoticeReceiverVO();
noticeReceiver.setId(1L);
noticeReceiver.setEmail("[email protected]");
ObjectMapper objectMapper = new ObjectMapper();
String jsonString = objectMapper.writeValueAsString(noticeReceiver);
// Assert that the email in the JSON string is desensitized
String desensitizedEmail = "1****@163.com";
assertTrue(jsonString.contains("\"email\":\"" + desensitizedEmail + "\""));
NoticeReceiverVO noticeReceiverVO = objectMapper.readValue(jsonString, NoticeReceiverVO.class);
assertEquals(desensitizedEmail, noticeReceiverVO.getEmail());
CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache();
assertEquals("[email protected]", desensitizationMapCache.get(noticeReceiverVO.getId() + "_" + noticeReceiverVO.getEmail()));
}| public void test() throws JsonProcessingException { | ||
| NoticeReceiverVO noticeReceiver = new NoticeReceiverVO(); | ||
| noticeReceiver.setId(1L); | ||
| noticeReceiver.setPhone("12345678910"); | ||
|
|
||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| SimpleModule simpleModule = new SimpleModule(); | ||
| simpleModule.addSerializer(String.class,phoneDesensitizationSerializer); | ||
|
|
||
| objectMapper.registerModule(simpleModule); | ||
|
|
||
| String jsonString = objectMapper.writeValueAsString(noticeReceiver); | ||
| NoticeReceiverVO noticeReceiverVO = objectMapper.readValue(jsonString, NoticeReceiverVO.class); | ||
|
|
||
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
|
|
||
| assertEquals(noticeReceiver.getPhone(),desensitizationMapCache.get(noticeReceiverVO.getId()+"_"+noticeReceiverVO.getPhone())); | ||
| } |
There was a problem hiding this comment.
The test setup is incorrect. By registering the serializer for String.class (simpleModule.addSerializer(String.class,phoneDesensitizationSerializer)), you are applying it to all string properties of NoticeReceiverVO, not just the phone field as intended by the @JsonSerialize annotation. This can lead to unexpected side effects and does not accurately test the production behavior.
The test should rely on the @JsonSerialize annotation on the NoticeReceiverVO.phone field to be picked up by Jackson. Also, it's good practice to assert that the serialized JSON contains the desensitized value.
Please refactor the test to correctly reflect its usage.
public void test() throws JsonProcessingException {
NoticeReceiverVO noticeReceiver = new NoticeReceiverVO();
noticeReceiver.setId(1L);
noticeReceiver.setPhone("12345678910");
ObjectMapper objectMapper = new ObjectMapper();
String jsonString = objectMapper.writeValueAsString(noticeReceiver);
// Assert that the phone in the JSON string is desensitized
String desensitizedPhone = "123****8910";
assertTrue(jsonString.contains("\"phone\":\"" + desensitizedPhone + "\""));
NoticeReceiverVO noticeReceiverVO = objectMapper.readValue(jsonString, NoticeReceiverVO.class);
assertEquals(desensitizedPhone, noticeReceiverVO.getPhone());
CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache();
assertEquals("12345678910", desensitizationMapCache.get(noticeReceiverVO.getId() + "_" + noticeReceiverVO.getPhone()));
}| * get desensitizationMap cache | ||
| * @return desensitizationMap cache | ||
| */ | ||
| public static CommonCacheService<String, Object> getDesensitizationMapCache(){return DESENSITIZATION_MAP_CACHE;}; |
There was a problem hiding this comment.
The formatting of this method is inconsistent with the other getter methods in this class. For better readability and maintainability, please format it to follow the same style.
public static CommonCacheService<String, Object> getDesensitizationMapCache() {
return DESENSITIZATION_MAP_CACHE;
}| int index = StrUtil.indexOf(email, '@'); | ||
| emailDesensitization = index <= 1 ? email : | ||
| StrUtil.replace(email, 1, index, '*'); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); |
There was a problem hiding this comment.
Casting jsonGenerator.getOutputContext().getCurrentValue() to NoticeReceiverVO creates a tight coupling between this serializer and the NoticeReceiverVO class. This makes the serializer less reusable and could lead to a ClassCastException if it's ever used on a property of a different class. While this might be acceptable for now, consider if a more decoupled approach is possible for future maintainability.
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (StrUtil.isNotBlank(phone)){ | ||
| phoneDesensitization = DesensitizedUtil.mobilePhone(phone); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue(); |
There was a problem hiding this comment.
Casting jsonGenerator.getOutputContext().getCurrentValue() to NoticeReceiverVO creates a tight coupling between this serializer and the NoticeReceiverVO class. This makes the serializer less reusable and could lead to a ClassCastException if it's ever used on a property of a different class. While this might be acceptable for now, consider if a more decoupled approach is possible for future maintainability.
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | ||
| } |
There was a problem hiding this comment.
This logic for restoring sensitive data from cache performs a cache lookup twice for each field (once in the if condition and once to get the value). This is inefficient. You can improve this by getting the value once and then checking if it's not null.
This block of code is also duplicated in the sendTestMsg method (lines 279-287). To adhere to the DRY principle, consider extracting this logic into a private helper method to be called from both places. This would improve maintainability and reduce redundancy.
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | |
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | |
| } | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | |
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | |
| } | |
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail())) { | |
| Object originalEmail = desensitizationMapCache.get(noticeReceiver.getId() + "_" + noticeReceiver.getEmail()); | |
| if (ObjectUtil.isNotEmpty(originalEmail)) { | |
| noticeReceiver.setEmail((String) originalEmail); | |
| } | |
| } | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone())) { | |
| Object originalPhone = desensitizationMapCache.get(noticeReceiver.getId() + "_" + noticeReceiver.getPhone()); | |
| if (ObjectUtil.isNotEmpty(originalPhone)) { | |
| noticeReceiver.setPhone((String) originalPhone); | |
| } | |
| } |
| <!-- hutool --> | ||
| <dependency> | ||
| <groupId>cn.hutool</groupId> | ||
| <artifactId>hutool-all</artifactId> | ||
| <version>${hutool-all.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
For better dependency management and to avoid bloating all modules, it's recommended to add this dependency to the <dependencyManagement> section. This allows modules to include it on an as-needed basis. Also, consider using specific hutool artifacts (e.g., hutool-core) instead of hutool-all to minimize the application's dependency footprint.
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Data Desensitization Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
![refacto-test[bot]](https://avatars.githubusercontent.com/u/196302493?s=60&v=4){kind=small}
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); | ||
|
|
||
| private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE = | ||
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); |
There was a problem hiding this comment.
Cache Expiration Risk
Sensitive data stored in cache for 1 day creates unnecessary exposure risk. Long-lived sensitive data cache increases potential compromise window if application is breached.
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); | |
| private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE = | |
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofMinutes(30), false); |
Standards
- CWE-922
- OWASP-A02
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | ||
| } |
There was a problem hiding this comment.
Missing Cache Validation
Cache retrieval lacks null check on noticeReceiver.getId(). NullPointerException risk if ID is null, causing service failure during receiver editing.
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | |
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | |
| } | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | |
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | |
| } | |
| if (noticeReceiver.getId() != null) { | |
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | |
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | |
| } | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | |
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | |
| } | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Precondition-Validation
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); |
There was a problem hiding this comment.
Redundant Cache Lookups
Duplicate cache lookups for the same key cause unnecessary overhead. The cache is queried twice for each field - once for existence check and again for retrieval.
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | |
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | |
| } | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | |
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | |
| Object cachedEmail = ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) ? desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()) : null; | |
| if (ObjectUtil.isNotEmpty(cachedEmail)) { | |
| noticeReceiver.setEmail((String) cachedEmail); | |
| } | |
| Object cachedPhone = ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) ? desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()) : null; | |
| if (ObjectUtil.isNotEmpty(cachedPhone)) { | |
| noticeReceiver.setPhone((String) cachedPhone); | |
| } |
Standards
- ISO-IEC-25010-Performance-Time-Behaviour
- Cache-Access-Optimization
- Algorithm-Opt-Redundancy-Elimination
| if (null != noticeReceiver.getId()){ | ||
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate Cache Logic
Duplicate desensitization logic in editReceiver and sendTestMsg methods creates maintenance burden. Code duplication increases risk of inconsistent behavior and future bugs.
| if (null != noticeReceiver.getId()){ | |
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | |
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | |
| } | |
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | |
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | |
| } | |
| } | |
| /** | |
| * Restore desensitized fields from cache | |
| * @param noticeReceiver The notice receiver to restore | |
| */ | |
| private void restoreDesensitizedFields(NoticeReceiver noticeReceiver) { | |
| if (null != noticeReceiver.getId()) { | |
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | |
| Object cachedEmail = ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) ? desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()) : null; | |
| if (ObjectUtil.isNotEmpty(cachedEmail)) { | |
| noticeReceiver.setEmail((String) cachedEmail); | |
| } | |
| Object cachedPhone = ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) ? desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()) : null; | |
| if (ObjectUtil.isNotEmpty(cachedPhone)) { | |
| noticeReceiver.setPhone((String) cachedPhone); | |
| } | |
| } | |
| } |
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- SRE-Code-Maintainability
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); |
There was a problem hiding this comment.
Cache Key Collision
Cache key construction using desensitized value creates potential collisions when different emails produce identical desensitized patterns. This can cause incorrect data retrieval during restoration.
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | |
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); | |
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | |
| desensitizationMapCache.put(currentValue.getId() + "_email", email); |
Standards
- ISO-IEC-25010-Performance-Resource-Utilization
- Algorithm-Opt-Hash-Map
- Cache-Key-Uniqueness
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Desensitization Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); | ||
|
|
||
| private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE = | ||
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); |
There was a problem hiding this comment.
Cache Expiration Risk
Desensitization cache uses a long expiration time (1 day) which increases sensitive data exposure risk. Attackers with access to the application could access sensitive data longer than necessary.
private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE =
new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofMinutes(30), false);
Standards
- CWE-539
- OWASP-A02
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); |
There was a problem hiding this comment.
Cache Entry Leakage
Cache entries are created but never removed, potentially causing memory leaks over time. Long-lived cached sensitive data increases exposure risk and memory consumption.
// Add cache entry with expiration time
NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue();
if (currentValue.getId() != null) {
desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email);
}
Standards
- ISO-IEC-25010-Reliability-Resource-Utilization
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| phoneDesensitization = DesensitizedUtil.mobilePhone(phone); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId()+"_"+phoneDesensitization, phone); |
There was a problem hiding this comment.
Null ID Risk
No null check on currentValue.getId() before cache key creation. NullPointerException risk if serializing object with null ID, causing serialization failures.
phoneDesensitization = DesensitizedUtil.mobilePhone(phone);
NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue();
if (currentValue.getId() != null) {
desensitizationMapCache.put(currentValue.getId()+"_"+phoneDesensitization, phone);
}
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Robustness
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); |
There was a problem hiding this comment.
Cache Key Collision
Cache key uses desensitized email which creates collision risk when multiple receivers have similar emails like '[email protected]' and '[email protected]' (both masked as '*@example.com'). This would overwrite cache entries causing data retrieval errors.
NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue();
// Use a more unique key pattern to prevent collisions
desensitizationMapCache.put(currentValue.getId() + "_email", email);
Standards
- Logic-Verification-Data-Integrity
- Business-Rule-Unique-Key-Generation
- Algorithm-Correctness-Cache-Management
| NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId()+"_"+phoneDesensitization, phone); |
There was a problem hiding this comment.
Phone Desensitization Collision
Cache key uses desensitized phone which creates collision risk when multiple receivers have similar phone numbers that produce identical masked values. This would overwrite cache entries causing data retrieval errors.
NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue();
// Use a more unique key pattern to prevent collisions
desensitizationMapCache.put(currentValue.getId()+"_phone", phone);
Standards
- Logic-Verification-Data-Integrity
- Business-Rule-Unique-Key-Generation
- Algorithm-Correctness-Cache-Management
| public class PhoneDesensitizationSerializer extends JsonSerializer<String> { | ||
|
|
||
| @Override | ||
| public void serialize(String phone, JsonGenerator jsonGenerator, SerializerProvider serializerProvider) throws IOException { | ||
| String phoneDesensitization = ""; | ||
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (StrUtil.isNotBlank(phone)){ | ||
| phoneDesensitization = DesensitizedUtil.mobilePhone(phone); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId()+"_"+phoneDesensitization, phone); | ||
| } | ||
|
|
||
| jsonGenerator.writeString(phoneDesensitization); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Duplicate Cache Logic
Similar caching logic is duplicated across Email and Phone serializers. This violates DRY principle and makes future changes error-prone as modifications must be synchronized across multiple classes.
public abstract class AbstractDesensitizationSerializer<T> extends JsonSerializer<String> {
protected abstract String desensitize(String value);
@Override
public void serialize(String value, JsonGenerator jsonGenerator, SerializerProvider serializerProvider) throws IOException {
String desensitized = "";
CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache();
if (StrUtil.isNotBlank(value)) {
desensitized = desensitize(value);
NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue();
desensitizationMapCache.put(currentValue.getId() + "_" + desensitized, value);
}
jsonGenerator.writeString(desensitized);
}
}
public class EmailDesensitizationSerializer extends AbstractDesensitizationSerializer<String> {
@Override
protected String desensitize(String email) {
int index = StrUtil.indexOf(email, '@');
return index <= 1 ? email : StrUtil.replace(email, 1, index, '*');
}
}
public class PhoneDesensitizationSerializer extends AbstractDesensitizationSerializer<String> {
@Override
protected String desensitize(String phone) {
return DesensitizedUtil.mobilePhone(phone);
}
}
Standards
- SOLID-DRY
- Clean-Code-Duplication
- Design-Pattern-Template-Method
| private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE = | ||
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); |
There was a problem hiding this comment.
Cache Size Limitation
The DESENSITIZATION_MAP_CACHE is configured with a maximum size of 1000 entries and TTL of 1 day. For large systems with many users viewing sensitive data, this could lead to cache evictions and reduced performance. The cache size should be adjusted based on expected user load and sensitive data volume.
private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE =
new CaffeineCacheServiceImpl<>(10, 5000, Duration.ofHours(12), false);
Standards
- ISO-IEC-25010-Performance-Resource-Utilization
- Netflix-Multi-Layer-Caching
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | ||
| } |
There was a problem hiding this comment.
Inconsistent Desensitization Handling
Duplicate cache key construction logic creates maintenance risk. Inconsistent handling between serialization and deserialization could cause data integrity issues.
CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache();
Long id = noticeReceiver.getId();
if (id != null) {
String email = noticeReceiver.getEmail();
String phone = noticeReceiver.getPhone();
String emailCacheKey = id + "_" + email;
String phoneCacheKey = id + "_" + phone;
if (ObjectUtil.isNotEmpty(email) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(emailCacheKey))) {
noticeReceiver.setEmail((String) desensitizationMapCache.get(emailCacheKey));
}
if (ObjectUtil.isNotEmpty(phone) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(phoneCacheKey))) {
noticeReceiver.setPhone((String) desensitizationMapCache.get(phoneCacheKey));
}
}
Standards
- ISO-IEC-25010-Reliability-Maturity
- ISO-IEC-25010-Functional-Correctness-Consistency
|
/refacto-test |
| @Test | ||
| public void test() throws JsonProcessingException { | ||
| NoticeReceiverVO noticeReceiver = new NoticeReceiverVO(); | ||
| noticeReceiver.setId(1L); | ||
| noticeReceiver.setEmail("[email protected]"); | ||
|
|
||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| SimpleModule simpleModule = new SimpleModule(); | ||
| simpleModule.addSerializer(String.class,emailDesensitizationSerializer); | ||
|
|
||
| objectMapper.registerModule(simpleModule); | ||
|
|
||
| String jsonString = objectMapper.writeValueAsString(noticeReceiver); | ||
| NoticeReceiverVO noticeReceiverVO = objectMapper.readValue(jsonString, NoticeReceiverVO.class); | ||
|
|
||
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
|
|
||
| assertEquals(noticeReceiver.getEmail(),desensitizationMapCache.get(noticeReceiverVO.getId()+"_"+noticeReceiverVO.getEmail())); |
There was a problem hiding this comment.
Duplicate Test Logic
Test logic is duplicated between EmailDesensitizationSerializerTest and PhoneDesensitizationSerializerTest. This violates DRY principle and increases maintenance burden when test approach needs to change.
// Create a base test class
abstract class AbstractDesensitizationSerializerTest<T extends JsonSerializer<String>> {
protected T serializer;
protected abstract T createSerializer();
protected abstract void setupTestData(NoticeReceiverVO receiver);
protected abstract String getFieldName();
@BeforeEach
void setUp() {
serializer = createSerializer();
}
@Test
public void testDesensitization() throws JsonProcessingException {
NoticeReceiverVO noticeReceiver = new NoticeReceiverVO();
noticeReceiver.setId(1L);
setupTestData(noticeReceiver);
ObjectMapper objectMapper = new ObjectMapper();
SimpleModule simpleModule = new SimpleModule();
simpleModule.addSerializer(String.class, serializer);
objectMapper.registerModule(simpleModule);
String jsonString = objectMapper.writeValueAsString(noticeReceiver);
NoticeReceiverVO noticeReceiverVO = objectMapper.readValue(jsonString, NoticeReceiverVO.class);
CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache();
String fieldValue = (String) PropertyUtils.getProperty(noticeReceiver, getFieldName());
String desensitizedValue = (String) PropertyUtils.getProperty(noticeReceiverVO, getFieldName());
assertEquals(fieldValue, desensitizationMapCache.get(noticeReceiverVO.getId()+"_"+desensitizedValue));
}
}
Standards
- Clean-Code-DRY
- Design-Pattern-Template-Method
|
/refacto-test |
1 similar comment
|
/refacto-test |
|
The user @visz11 does not have an active seat allocated for code reviews. Please contact your organization admin to assign a seat or upgrade your subscription. |
1 similar comment
|
The user @visz11 does not have an active seat allocated for code reviews. Please contact your organization admin to assign a seat or upgrade your subscription. |
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Desensitization Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); | ||
|
|
||
| private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE = | ||
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); |
There was a problem hiding this comment.
Missing Cache Expiration
Sensitive data cache retains information for 1 day without explicit expiration control. Extended cache retention increases exposure window of sensitive information. Shorter TTL reduces risk window.
private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE =
new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofMinutes(30), true);
Standards
- CWE-524
- OWASP-A04
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | ||
| } |
There was a problem hiding this comment.
Null Safety Issue
The code doesn't check if noticeReceiver.getId() is null before concatenation. This can cause NullPointerException when constructing cache keys, leading to runtime failures during desensitization operations.
CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache();
if (noticeReceiver.getId() != null) {
if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){
noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()));
}
if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){
noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()));
}
}
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | ||
| } |
There was a problem hiding this comment.
Missing Cache Null-Check
Cache lookup is performed twice for each field, causing redundant cache access and potential inconsistency if cache changes between lookups. The retrieved value should be stored in a variable to ensure consistency.
// Store cache lookup results to avoid redundant access
if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail())) {
Object cachedEmail = desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail());
if (ObjectUtil.isNotEmpty(cachedEmail)) {
noticeReceiver.setEmail((String) cachedEmail);
}
}
if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone())) {
Object cachedPhone = desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone());
if (ObjectUtil.isNotEmpty(cachedPhone)) {
noticeReceiver.setPhone((String) cachedPhone);
}
}
Standards
- Algorithm-Correctness-Cache-Access
- Logic-Verification-Consistency
- Business-Rule-Data-Integrity
| public class EmailDesensitizationSerializer extends JsonSerializer<String> { | ||
|
|
||
| @Override | ||
| public void serialize(String email, JsonGenerator jsonGenerator, SerializerProvider serializerProvider) throws IOException { |
There was a problem hiding this comment.
Missing Javadoc Comments
Class and method lack Javadoc comments explaining purpose and behavior. Missing documentation reduces maintainability by making code intent and usage less clear for future developers.
/**
* Custom serializer that desensitizes email addresses by replacing characters with '*'.
* Stores original value in cache for retrieval when needed.
*/
public class EmailDesensitizationSerializer extends JsonSerializer<String> {
/**
* Serializes email by replacing characters between first character and '@' with '*'.
* Stores the mapping between desensitized and original email in cache.
*
* @param email Email address to desensitize
* @param jsonGenerator JSON generator
* @param serializerProvider Serializer provider
* @throws IOException If an I/O error occurs
*/
@Override
public void serialize(String email, JsonGenerator jsonGenerator, SerializerProvider serializerProvider) throws IOException {
Standards
- Clean-Code-Documentation
- Clean-Code-Readability
| emailDesensitization = index <= 1 ? email : | ||
| StrUtil.replace(email, 1, index, '*'); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); |
There was a problem hiding this comment.
Potential Cache Leakage
Cache entries are created but never expired or cleaned up, potentially causing memory leaks as the cache grows unbounded over time. Without TTL or eviction policy, sensitive data remains in memory indefinitely.
// Add cache entry with TTL to prevent memory leaks
desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email, Duration.ofMinutes(30));
Standards
- Logic-Verification-Resource-Management
- Algorithm-Correctness-Memory-Management
- Business-Rule-Data-Protection
|
/refacto-test |
|
Refacto is reviewing this PR. Please wait for the review comments to be posted. |
Code Review: Desensitization Implementation👍 Well Done
📌 Files Processed
📝 Additional Comments
|
| emailDesensitization = index <= 1 ? email : | ||
| StrUtil.replace(email, 1, index, '*'); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); |
There was a problem hiding this comment.
Cache Key Collision
Cache key construction using desensitized value creates potential collision risk. Multiple different emails could produce identical desensitized values, causing cache overwrite and incorrect data retrieval. This would lead to incorrect information being restored during edit operations.
if (currentValue.getId() != null) {
// Use original email in key to prevent collisions from different emails with same desensitized pattern
desensitizationMapCache.put(currentValue.getId() + "_email_" + email, email);
}
Commitable Suggestion
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); | |
| if (currentValue.getId() != null) { | |
| // Use original email in key to prevent collisions from different emails with same desensitized pattern | |
| desensitizationMapCache.put(currentValue.getId() + "_email_" + email, email); | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Data-Integrity
| if (StrUtil.isNotBlank(phone)){ | ||
| phoneDesensitization = DesensitizedUtil.mobilePhone(phone); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId()+"_"+phoneDesensitization, phone); |
There was a problem hiding this comment.
Cache Key Collision
Cache key construction using desensitized value creates potential collision risk. Different phone numbers with same desensitized pattern could overwrite each other in cache. This would lead to incorrect information being restored during edit operations.
if (currentValue.getId() != null) {
// Use original phone in key to prevent collisions from different phones with same desensitized pattern
desensitizationMapCache.put(currentValue.getId() + "_phone_" + phone, phone);
}
Commitable Suggestion
| desensitizationMapCache.put(currentValue.getId()+"_"+phoneDesensitization, phone); | |
| if (currentValue.getId() != null) { | |
| // Use original phone in key to prevent collisions from different phones with same desensitized pattern | |
| desensitizationMapCache.put(currentValue.getId() + "_phone_" + phone, phone); | |
| } |
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Data-Integrity
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); |
There was a problem hiding this comment.
Null ID Handling
The code assumes currentValue.getId() is non-null when constructing the cache key, but there's no null check. If ID is null during serialization (e.g., for new unsaved entities), this will cause a NullPointerException when concatenating with the string.
NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue();
if (currentValue.getId() != null) {
// Use a prefix and original email to ensure key uniqueness
desensitizationMapCache.put(currentValue.getId() + "_email_" + email, email);
}
Commitable Suggestion
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | |
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); | |
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | |
| if (currentValue.getId() != null) { | |
| // Use a prefix and original email to ensure key uniqueness | |
| desensitizationMapCache.put(currentValue.getId() + "_email_" + email, email); | |
| } |
Standards
- Algorithm-Correctness-Null-Safety
- Business-Rule-Error-Prevention
- Logic-Verification-Defensive-Programming
| NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId()+"_"+phoneDesensitization, phone); |
There was a problem hiding this comment.
Missing Null Check
No null check on currentValue or its getId() method before using in cache key. NullPointerException could occur if the context value is null or doesn't have a valid ID, causing serialization failure and potential data loss.
Standards
- ISO-IEC-25010-Reliability-Fault-Tolerance
- ISO-IEC-25010-Functional-Correctness-Appropriateness
- DbC-Defensive-Programming
| int index = StrUtil.indexOf(email, '@'); | ||
| emailDesensitization = index <= 1 ? email : | ||
| StrUtil.replace(email, 1, index, '*'); |
There was a problem hiding this comment.
Email Desensitization Logic
The email desensitization logic has an edge case where emails with only one character before @ (e.g., [email protected]) won't be desensitized at all. This contradicts the desensitization purpose and creates inconsistent behavior between similar email addresses.
Standards
- Algorithm-Correctness-Edge-Cases
- Business-Rule-Consistency
- Logic-Verification-Boundary-Conditions
| StrUtil.replace(email, 1, index, '*'); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); |
There was a problem hiding this comment.
Cache Security Risk
Sensitive email data is stored in a cache without expiration controls or access restrictions. This creates a potential data exposure risk if the cache is compromised or accessed by unauthorized code. The original sensitive data remains accessible through the cache indefinitely.
Standards
- CWE-524
- OWASP-A04
- NIST-SSDF-PW.1
| phoneDesensitization = DesensitizedUtil.mobilePhone(phone); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO)jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId()+"_"+phoneDesensitization, phone); |
There was a problem hiding this comment.
Cache Security Risk
Sensitive phone number data is stored in a cache without expiration controls or access restrictions. This creates a potential data exposure risk if the cache is compromised or accessed by unauthorized code. The original sensitive data remains accessible through the cache indefinitely.
Standards
- CWE-524
- OWASP-A04
- NIST-SSDF-PW.1
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | ||
| } |
There was a problem hiding this comment.
Duplicate Desensitization Logic
Duplicate desensitization logic appears in both editReceiver and sendTestMsg methods. This violates DRY principle and increases maintenance burden when desensitization logic changes. Extract this common functionality into a separate method for better maintainability.
Standards
- Clean-Code-DRY
- Refactoring-Extract-Method
- Maintainability-Quality-Duplication
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); | ||
|
|
||
| private static final CommonCacheService<String, Object> DESENSITIZATION_MAP_CACHE = | ||
| new CaffeineCacheServiceImpl<>(10, 1000, Duration.ofDays(1), false); |
There was a problem hiding this comment.
Cache Eviction Strategy
Desensitization cache uses fixed 1-day expiration without explicit eviction strategy. This could lead to memory pressure if many records are processed. Consider implementing a more aggressive expiration policy or size-based eviction.
Standards
- ISO-IEC-25010-Reliability-Resource-Utilization
- SRE-Resource-Management
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getEmail()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail()))){ | ||
| noticeReceiver.setEmail((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getEmail())); | ||
| } | ||
| if (ObjectUtil.isNotEmpty(noticeReceiver.getPhone()) && ObjectUtil.isNotEmpty(desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone()))){ | ||
| noticeReceiver.setPhone((String) desensitizationMapCache.get(noticeReceiver.getId()+"_"+noticeReceiver.getPhone())); | ||
| } |
There was a problem hiding this comment.
Inconsistent Desensitization Logic
Desensitization restoration logic depends on cache having exact match for desensitized values. If cache expires or is cleared, original values cannot be restored. This creates a reliability dependency on cache state for critical functionality.
Standards
- ISO-IEC-25010-Reliability-Recoverability
- ISO-IEC-25010-Functional-Correctness-Appropriateness
| @@ -0,0 +1,42 @@ | |||
| package org.apache.hertzbeat.common.serialize; | |||
There was a problem hiding this comment.
Missing License Header
Missing Apache License header violates project standards for source code files. This inconsistency makes license compliance tracking difficult and could cause legal issues. All source files should include the standard Apache License header for consistency.
Standards
- Infringement-License-Header
- Maintainability-Quality-Consistency
- Clean-Code-Standardization
| @@ -0,0 +1,42 @@ | |||
| package org.apache.hertzbeat.common.serialize; | |||
There was a problem hiding this comment.
Missing License Header
Missing Apache License header violates project standards for source code files. This inconsistency makes license compliance tracking difficult and could cause legal issues. All source files should include the standard Apache License header for consistency.
Standards
- Infringement-License-Header
- Maintainability-Quality-Consistency
- Clean-Code-Standardization
|
|
||
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
|
|
||
| assertEquals(noticeReceiver.getEmail(),desensitizationMapCache.get(noticeReceiverVO.getId()+"_"+noticeReceiverVO.getEmail())); |
There was a problem hiding this comment.
Test Assertion Validity
The test assertion compares the original email with the cached value, but noticeReceiverVO.getEmail() returns the desensitized email, not the original. This creates a logical inconsistency in the test as it's comparing using a key that doesn't match how keys are actually constructed in the serializer.
Standards
- Algorithm-Correctness-Test-Logic
- Business-Rule-Test-Validity
- Logic-Verification-Test-Consistency
| String emailDesensitization = ""; | ||
| CommonCacheService<String, Object> desensitizationMapCache = CacheFactory.getDesensitizationMapCache(); | ||
| if (StrUtil.isNotBlank(email)) { | ||
| int index = StrUtil.indexOf(email, '@'); | ||
| emailDesensitization = index <= 1 ? email : | ||
| StrUtil.replace(email, 1, index, '*'); | ||
| NoticeReceiverVO currentValue = (NoticeReceiverVO) jsonGenerator.getOutputContext().getCurrentValue(); | ||
| desensitizationMapCache.put(currentValue.getId() + "_" + emailDesensitization, email); | ||
| } | ||
| jsonGenerator.writeString(emailDesensitization); |
There was a problem hiding this comment.
Inconsistent Null Handling
The code handles null/blank emails by setting emailDesensitization to an empty string, but doesn't add anything to the cache. This creates inconsistent behavior where desensitized empty values can't be reversed through the cache lookup mechanism used elsewhere.
Standards
- Algorithm-Correctness-Null-Handling
- Business-Rule-Consistency
- Logic-Verification-Edge-Cases
5 participants
New Features
Introduced a value object for notification recipients supporting multiple channels and enhanced API documentation.
Added automatic desensitization (masking) of phone numbers and emails in notification recipient data for improved privacy.
Implemented caching to restore original contact information when editing or testing notification recipients.
Bug Fixes
Ensured original contact details are correctly restored from cache during recipient edits and test message sends.
Tests
Added tests to verify phone and email desensitization and caching behavior.
Chores
Added Hutool utility library as a new project dependency.
Summary by CodeRabbit