-
Notifications
You must be signed in to change notification settings - Fork 217
fix(analytics): validate against incomplete/invalid app_remove events #1738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request provides a solid fix for a crash caused by malformed user properties in analytics events. The approach is robust, adding defensive checks in unwrapValueAsString
and filtering invalid properties during event construction. The addition of a new test case that covers various invalid inputs is excellent for ensuring correctness and preventing future regressions. I have one suggestion to improve the readability of the new filtering logic.
eee4ef2
to
16b1413
Compare
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request effectively addresses a TypeError
caused by malformed user properties in analytics events. The solution is well-implemented, particularly the defensive programming in unwrapValueAsString
and the addition of comprehensive test cases to prevent regressions. I have one suggestion to further improve the robustness of the user property filtering logic to prevent partially-valid objects from being processed.
.filter(([, v]) => { | ||
// Property must be a non-empty object. | ||
if (v == null || typeof v !== "object" || Object.keys(v).length === 0) { | ||
return false; | ||
} | ||
// If 'value' field exists, it must not be null, undefined, or an empty object. | ||
if (!("value" in v)) { | ||
return true; | ||
} | ||
|
||
const value = (v as { value: unknown }).value; | ||
const isEmptyObject = | ||
typeof value === "object" && value !== null && Object.keys(value).length === 0; | ||
return value != null && !isEmptyObject; | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current filtering logic for user properties is not strict enough. It allows property objects without a value
field to be processed (e.g., an object containing only setTimestampUsec
). This leads to the creation of UserPropertyValue
instances where the value
property is undefined
, which violates its type definition (value: string
).
To make the filtering more robust and ensure all processed user properties are valid, I suggest modifying the filter to explicitly require the presence of the value
field. This also simplifies the logic, as the check for an empty object becomes redundant.
.filter(([, v]) => {
// Property must be an object and have a 'value' field.
if (v == null || typeof v !== "object" || !("value" in v)) {
return false;
}
// The 'value' field must not be null, undefined, or an empty object.
const value = (v as { value: unknown }).value;
const isEmptyObject =
typeof value === "object" && value !== null && Object.keys(value).length === 0;
return value != null && !isEmptyObject;
})
This draft PR is a proposed solution to resolve #1712
Problem
When an analytics event payload contains malformed user properties, the SDK throws a
TypeError
before the user's callback can execute:Error:
TypeError: Cannot convert undefined or null to object
at Function.keys ()
at unwrapValueAsString (analytics.js:206)
Solution