-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: add SES to wallet-mobile #420
Conversation
2479ce2
to
4b42122
Compare
e7261a1
to
752c1db
Compare
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.
question: It looks good to me, but I feel there's something missing on the documentation on how to maintain the code from now on.
When we add new dependencies, should we change anything on the lavamoat files, or only on the "lavamoat" config inside the package.json
?
thought: Since this will be a primary concern for any modifications on our dependencies, I think it's important to document it better on the readme file. Both for Hathor Labs devs and for the community members that wish to contribute.
Hi. I think we could chat about what we're working on in LavaMoat. Get in touch if you think so too :) |
Hey @naugtur, thanks for reaching out! I've just sent you a DM on twitter, please tell me if this is the best way to contact you |
8ecb435
to
0628d10
Compare
0628d10
to
a54da44
Compare
Update on our progress:
BTW |
d3f602a
to
98b3ac1
Compare
// SES was enabled, we should disable it in storage which gets read in the | ||
// react-native initialization (more on this in patches/react-native+0.72.5.patch) | ||
// and restart the react-native bundle. | ||
disableSes(); |
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.
question: If there is a problem with the Unleash connection or with the MMKV storage, is there a chance of the application entering an infinite loop of self restarting?
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.
If there is a problem with the Unleash connection, we would default to false
(we could also default to true
, not sure if we want to do this in the first release though), which would cause this method to be called
If there is a problem with the MMKV storage it would indeed cause a infinite restart loop, but I don't see how this could happen without us catching it in QA or internal tests...
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.
I would suggest adding a comment or opening an issue/KTLO to address this in the future.
I agree that the QA would get this situation, but our QA is pretty big already and there is a possibility of human error.
In my opinion, this is not critical enough to demand a fix right now, but it's a nice to have for future improvements as a KTLO.
// SES was enabled, we should disable it in storage which gets read in the | ||
// react-native initialization (more on this in patches/react-native+0.72.5.patch) | ||
// and restart the react-native bundle. | ||
disableSes(); |
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.
I would suggest adding a comment or opening an issue/KTLO to address this in the future.
I agree that the QA would get this situation, but our QA is pretty big already and there is a possibility of human error.
In my opinion, this is not critical enough to demand a fix right now, but it's a nice to have for future improvements as a KTLO.
src/sagas/ses.js
Outdated
const storage = new MMKV(); | ||
|
||
function disableSes(restart = true) { | ||
logger.debug('Disabling SAS'); |
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.
logger.debug('Disabling SAS'); | |
logger.debug('Disabling SES'); |
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.
Fixed! Thanks
6084586
to
4c95a93
Compare
4c95a93
to
e9a0644
Compare
Motivation
We want to defend against supply chain attacks as those kind of attacks have already hit the cryptocurrency ecosystem and present a significant risk for our developers and users of our wallets and apps.
This PR is part of a series of PRs using a set of tools called LavaMoat to improve security on our Javascript projects with a set of good defaults, preventing us from having to rewrite them from scratch
Important: SES is not yet fully compatible with react-native, there is a compatibility tracker issue here
What is not yet supported (that affect us)?
lavamoat-node
Also, to prevent multiple patches in react-native, we had to inject SES in a patch to react-native's
initializeCore
method, so it gets loaded before anything else in our bundleAcceptance Criteria
postinstall
scripts, blocking all the rest by using LavaMoat allow-scriptsSecurity Checklist