Improve onyx state export masking#73428
Conversation
melvin-bot
Bot
requested review from
MarioExpensify
and removed request for
a team
|
@MarioExpensify Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -249,9 +288,35 @@ const maskFragileData = (data: OnyxState | unknown[] | null, parentKey?: string) | |||
|
|
|||
| const value = data[propertyName]; | |||
There was a problem hiding this comment.
❌ Potential Bug (docs)
There's a logical error here. You're accessing data[propertyName] but propertyName could be different from key when the key is an email (lines 283-287). This means you might be accessing undefined values.
Fix: Use the original key to access the data:
const value = (data as Record<string, unknown>)[key];
| } else if (key.startsWith(ONYXKEYS.COLLECTION.TRANSACTION) && typeof value === 'object') { | ||
| maskedData[propertyName] = maskFragileData(value as OnyxState, key); | ||
| } else if (amountKeysToRandomize.includes(key) && typeof value === 'number') { | ||
| maskedData[propertyName] = randomizeAmount(value); |
There was a problem hiding this comment.
❌ Code Style (docs)
Trailing whitespace detected at the end of this line.
Fix: Remove the trailing space after randomizeAmount(value);
There was a problem hiding this comment.
@MarioExpensify the funny thing is the trailing space was never there. I triple checked that.
|
LGTM |
. Thank you for your hard work!
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
LGTM |
|
@MarioExpensify I'll need to fix jest tests and ESLint |
| return datePatterns.some((pattern) => pattern.test(value)); | ||
| }; | ||
|
|
||
| const getCurrentDate = (): string => { |
There was a problem hiding this comment.
What kind of date are we masking? Returning always the same or very close dates may make it difficult / impossible to triage problems in date arithmetics. Not sure how much value we lose on that though.
There was a problem hiding this comment.
@MarioExpensify travel dates - most notably flight dates. Just for safety, I don't want anybody to know when I'm not home.
|
LGTM |
| } | ||
|
|
||
| const value = data[propertyName]; | ||
| const value = (data as Record<string, unknown>)[key]; |
There was a problem hiding this comment.
❌ Type Safety Issue
There's a potential type safety issue here. The code accesses (data as Record<string, unknown>)[key] but then assigns to maskedData[propertyName] where propertyName might be different from key (when the key is an email that gets masked). This could lead to inconsistent key handling.
Consider ensuring the key transformation is consistent:
const value = (data as Record<string, unknown>)[key];
// ... processing logic ...
maskedData[propertyName] = processedValue; // propertyName might be different from keyThe logic should use the same key reference consistently or clearly document why different keys are used.
|
|
||
| const emailRegex = /[a-zA-Z0-9._%+-]+@[a-zA-Z0-9.-]+\.[a-zA-Z]{2,}/; | ||
|
|
||
| const emailMap = new Map<string, string>(); |
There was a problem hiding this comment.
❌ Missing Memory Management
The emailMap is a module-level Map that persists across function calls. While it's cleared at the end of maskOnyxState, if the function throws an error before reaching the cleanup, the map will retain potentially sensitive data in memory.
const emailMap = new Map<string, string>();Consider using a try-finally block or moving the map inside the function to ensure proper cleanup:
const maskOnyxState: MaskOnyxState = (data, isMaskingFragileDataEnabled) => {
const emailMap = new Map<string, string>();
try {
// ... masking logic ...
} finally {
emailMap.clear();
}
return onyxState;
};
| maskedData[propertyName] = maskFragileData(value as OnyxState, key); | ||
| } else if (key.startsWith(ONYXKEYS.COLLECTION.TRANSACTION) && typeof value === 'object') { | ||
| maskedData[propertyName] = maskFragileData(value as OnyxState, key); | ||
| } else if (amountKeysToRandomize.includes(key) && typeof value === 'number') { |
There was a problem hiding this comment.
❌ Logic Issue - Inconsistent Value Assignment
There's an inconsistency in how values are assigned. In some places, the code uses maskedData[key] = ... and in others maskedData[propertyName] = .... This could lead to incorrect key mappings, especially when the propertyName is different from the original key (e.g., when emails are masked).
} else if (amountKeysToRandomize.includes(key) && typeof value === 'number') {
maskedData[propertyName] = randomizeAmount(value);
// Handle expensify_text_title masking
} else if (parentKey === 'expensify_text_title' && key === 'value' && typeof value === 'string') {
maskedData[propertyName] = maskValuePreservingLength(value);vs.
} else if (keysToMask.includes(key)) {
if (Array.isArray(value)) {
maskedData[key] = value.map(() => MASKING_PATTERN);
} else {
maskedData[key] = maskValuePreservingLength(value);
}The assignment should be consistent - either always use propertyName or always use key, but not mix them.
Code Review SummaryI've completed a comprehensive review of PR #73428. The changes improve onyx state export masking functionality, but I've identified several issues that should be addressed: Key Issues Found:
Positive Aspects:
The functionality appears sound, but the implementation could be more robust and performant. Please address the inline comments for a production-ready solution. |
Codecov Report❌ Patch coverage is
... and 35 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
LGTM |
1 similar comment
|
LGTM |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/MarioExpensify in version: 9.2.39-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.2.39-3 🚀
|
3 participants
Explanation of Change
Additionally, improves on #72932, which made masking a little bit trigger happy in some circumstances
Fixed Issues
$
PROPOSAL:
Tests
Manual tests:
reservationListnode is randomized, travel itineraries and confirmation codes are masked or randomized (all the dates should be set to today)reservationListnode are not randomizedOffline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.