refactor: several negative positioning/margin hacks#3775
refactor: several negative positioning/margin hacks#3775kaloudis merged 2 commits intoZeusLN:masterfrom
Conversation
Summary of ChangesHello, 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! This pull request focuses on refactoring several UI components to remove 'negative positioning/margin hacks' and improve overall styling consistency. The changes streamline the layout of the 'Add Note' input, enhance the visual alignment of the contact avatar with its banner, and standardize the 'Existing account' switch in the 'Import Account' view, resulting in a more robust and predictable user interface. Highlights
Changelog
Activity
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors several UI components, specifically AddNotes.tsx, ContactDetails.tsx, and ImportAccount.tsx, to remove styling hacks like negative margins and positioning, resulting in cleaner, more maintainable, and more idiomatic React Native code. However, a security audit revealed two significant vulnerabilities: a high-severity Broken Access Control (IDOR) in AddNotes.tsx allowing arbitrary storage key overwrites via deep links, and a medium-severity path traversal in ContactDetails.tsx related to loading contact banners from local file URIs. Additionally, a code quality suggestion for ContactDetails.tsx involves moving inline styles to a StyleSheet. Addressing these security vulnerabilities and the suggested code improvement is critical for user data protection, application integrity, and overall code quality.
| height: '100%', | ||
| textAlignVertical: 'top', | ||
| marginTop: -13 | ||
| textAlignVertical: 'top' |
There was a problem hiding this comment.
The AddNotes component uses noteKey from route.params (line 41) as a direct key for storage operations (Storage.getItem, Storage.setItem). Since route parameters can be influenced via deep links, an attacker could potentially trick a user into overwriting critical application data (e.g., settings or contacts) by providing a malicious noteKey. For example, a link like zeusln://AddNotes?noteKey=zeus-settings-v2 would allow a user to inadvertently overwrite their entire application configuration if they save a note.
Recommendation: Validate that noteKey belongs to an expected set of keys or implement a restricted namespace for notes in storage to prevent unauthorized access to other application data.
| <Text style={{ fontSize: 64 }}>⚡</Text> | ||
| )} | ||
| <Image | ||
| source={{ uri: contact.getBanner }} |
There was a problem hiding this comment.
The Image component's source.uri is populated from contact.getBanner, which is derived from untrusted contact data (e.g., from Nostr). The implementation of getBanner in models/Contact.ts (line 181) constructs a file:// URI by appending a user-controlled string to the app's document directory without sanitizing for path traversal sequences (..). This could allow a malicious contact to point the banner to sensitive internal files (e.g., rnfs://../../../../etc/hosts). While the impact is limited as the file is treated as an image, it still constitutes a path traversal vulnerability.
Recommendation: Sanitize the input in models/Contact.ts to prevent path traversal and validate that the resulting URI points to an expected location.
| const avatarCircle = contact.photo ? ( | ||
| <Image | ||
| source={{ uri: contact.getPhoto }} | ||
| style={{ width: 150, height: 150, borderRadius: 75 }} | ||
| /> | ||
| ) : ( | ||
| <View | ||
| style={{ | ||
| width: 150, | ||
| height: 150, | ||
| borderRadius: 75, | ||
| backgroundColor: themeColor('secondary'), | ||
| alignItems: 'center', | ||
| justifyContent: 'center' | ||
| }} | ||
| > | ||
| {contact.getAvatarInitials ? ( | ||
| <Text | ||
| style={{ | ||
| fontSize: 48, | ||
| fontWeight: 'bold', | ||
| color: themeColor('text') | ||
| }} | ||
| > | ||
| {contact.getAvatarInitials} | ||
| </Text> | ||
| ) : contact.hasOnlyCashuPubkey ? ( | ||
| <Ecash fill="#FACC15" width={64} height={64} /> | ||
| ) : ( | ||
| <Text style={{ fontSize: 64 }}>⚡</Text> | ||
| )} | ||
| </View> | ||
| ); |
There was a problem hiding this comment.
While refactoring to create the avatarCircle component is a great improvement for code deduplication, the component still uses inline styles. To improve maintainability and adhere to best practices, consider moving these styles to the StyleSheet at the bottom of the file. This makes styles reusable and keeps the component's render method cleaner.
Here's a suggestion for the styles to add to your StyleSheet:
const styles = StyleSheet.create({
// ... other styles
avatarContainer: {
width: 150,
height: 150,
borderRadius: 75,
},
avatarView: {
backgroundColor: themeColor('secondary'),
alignItems: 'center',
justifyContent: 'center',
},
avatarInitials: {
fontSize: 48,
fontWeight: 'bold',
color: themeColor('text'),
},
avatarIcon: {
fontSize: 64,
}
});You can then apply these styles as shown in the code suggestion.
const avatarCircle = contact.photo ? (
<Image
source={{ uri: contact.getPhoto }}
style={styles.avatarContainer}
/>
) : (
<View style={[styles.avatarContainer, styles.avatarView]}>
{contact.getAvatarInitials ? (
<Text style={styles.avatarInitials}>
{contact.getAvatarInitials}
</Text>
) : contact.hasOnlyCashuPubkey ? (
<Ecash fill="#FACC15" width={64} height={64} />
) : (
<Text style={styles.avatarIcon}>⚡</Text>
)}
</View>
);
There was a problem hiding this comment.
done (only moved those with more than 1 style, otherwise it is kind of pointless imo; also removed unused styles)
kaloudis
added
Refactor
3 participants
Description
3 files refactored:
Add a note
Contacts
75pxinstead of80px), I like symmetry... :)Import Account
Existing accountswitch now similar to other menusThis pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: